Cover the cookie attributes from Opts, and assert the avatar path instead of byte counts - #317
Conversation
Seven tests pinned the exact encoded size of a PNG the standard library produces, and Go 1.27 changed the encoder, so all seven fail on a clean checkout with nothing in the repository touched: 569 becomes 507, 999 becomes 1633, 992 becomes 1617, and 960 becomes 1564 in four places. They assert what the code actually owes its caller now. The generator emits a 300x300 identicon and the proxy resizes to its configured limit, and neither moves with a Go release, so the geometry is the durable claim and the byte count never was. Three smaller things in the same tests, all of which let a real regression through: - exact dimensions rather than an upper bound, since LessOrEqual passes a 60x60 result where the resize limit is 120 - the Positive and NotEmpty assertions that sit after a successful image.Decode, which cannot fail if the decode succeeded - nothing in either module inspected a pixel, so a resize that lost its Scale call still encodes a correctly sized, entirely transparent PNG and the suite stays green. TestAvatar_resize now asserts the result is not uniform, which is the only assertion that would notice CI cannot see any of this: both workflows pin go-version 1.26. Verified on 1.27 natively and in golang:1.27-alpine, where every package in both modules passes, and each old assertion fails when restored.
Coverage Report for CI Build 32636410209Coverage increased (+0.05%) to 86.097%Details
Uncovered ChangesNo uncovered changes found. Coverage RegressionsNo coverage regressions found. Coverage Stats
💛 - Coveralls |
Coverage Report for CI Build 32643976318Coverage remained the same at 86.05%Details
Uncovered ChangesNo uncovered changes found. Coverage RegressionsNo coverage regressions found. Coverage Stats
💛 - Coveralls |
Secure and SameSite are read from Opts in four places, the two cookies Set writes and the two Reset writes, and every existing test in the file runs with SecureCookies false and the zero SameSite, which emits no attribute at all. All four could stop passing them through and the suite would stay green, which is the plumbing this covers, along with the deliberate HttpOnly difference between the token cookie and the XSRF one the caller has to read. Each of the eight mutations, Secure and SameSite dropped from each of the four literals in turn, fails this test and nothing else.
d13d39d to
b98b46e
Compare
paskal
changed the title
|
Added the plumbing test you asked for on #315, in both modules, since that PR is closed and this is the test-only one.
Title and description updated to cover both halves. |
The rewrite traded exact encoded sizes for geometry, and geometry alone cannot tell a drawn identicon from a blank canvas of the same dimensions. Mutating GenerateAvatar to emit an empty 300x300 PNG passes the entire suite on both modules, where master catches it in TestIntegrationAvatar, TestAvatar_PutIdenticon and TestAvatar_PutFailed, because a blank image encodes to a different size. So the brittle assertions were inspecting pixel content by accident, and removing them opened a gap rather than closing one. assertStoredImage now also rejects a uniform image, and the three tests that decode a served avatar do the same. The blank-identicon mutation fails four tests, and the lost-Scale mutation still fails TestAvatar_resize. Also corrects the reset subtest's comment, which explained the repeated cookie attributes by cookie identity. Identity is name, domain and path, which is why Reset writing HttpOnly false over a cookie Set wrote HttpOnly true replaces it normally. The attributes matter because Chrome and Firefox reject SameSite=None without Secure, so a discarded removal cookie leaves the original in place.
umputun
left a comment
umputun
left a comment
There was a problem hiding this comment.
good swap, the byte counts were pinning the encoder rather than the behaviour. One thing worth a follow-up, not blocking.
the new assertions accept a generator that ignores its user argument. Decode, png, 300x300 bounds and not-uniform all pass for a single fixed image returned to every caller, and nothing else in the suite asserts that user affects the pixels. The old byte counts rejected that case, since different users gave different lengths, so the replacement loses a mutation it was catching. Your PR body already calls this out, so mostly checking it was a deliberate trade rather than something you ran out of room for.
cheap to close: generate for two stable user IDs, decode both, assert the pixel content differs. Not the PNG bytes or the length, those are the thing we just got rid of.
merge note: this and #318 both carry the same identicon hunk in provider/dev_provider_test.go and provider/custom_server_test.go. Yours is the superset, it has the uniformImage assertion #318's copy lacks, so this one should go in first and #318 drops its copy on rebase.
…ow carries Two follow-ups from go-pkgz#317's review. The geometry assertions accept a generator that ignores its user argument: one fixed image decodes, is png, is 300x300 and is not uniform for every caller. The byte counts they replaced rejected that by accident, since different users gave different lengths, so the swap lost a mutation it had been catching. TestGenerateAvatar_DiffersPerUser compares pixels for two user ids instead of encoded size, which is the thing that moved with the Go release, and also asserts the same user twice is stable, since an identicon that churns per login is the other way this can go wrong. Mutation-checked: making Draw ignore its argument fails this test and nothing else, which is the point. The identicon hunks in provider/dev_provider_test.go and provider/custom_server_test.go are dropped here, since go-pkgz#317 landed the superset with the uniformImage assertion this branch's copy lacked.
3 participants
Split out of #315 so it can go in without waiting on the Partitioned discussion, and now carrying the cookie plumbing test you asked for there. Test-only in both halves, no production code touched.
The avatar assertions
Ten assertions, five mirrored in each module, pin the exact encoded size of a PNG the standard library produces. Go 1.27 changed the encoder, so all ten fail on a clean checkout with nothing in the repository touched:
TestIntegrationAvatar, both modulesTestAvatar_PutIdenticon, both modulesTestAvatar_PutFailed, both modulesTestDevProviderandTestCustomProvider, both modulesThey now assert the geometry: the generator emits a 300x300 identicon, the proxy resizes to its configured limit, and neither moves with a Go release.
What that trade costs, and what pays it back. A byte count is a bad assertion but it is not an empty one: it notices pixel content, by accident. Geometry does not. Mutating
GenerateAvatarto emit a blank 300x300 PNG passes the entire suite once the counts are replaced by dimensions, while master catches it inTestIntegrationAvatar,TestAvatar_PutIdenticonandTestAvatar_PutFailed. So replacing them one-for-one would have opened a gap rather than closed one.assertStoredImageand the three tests that decode a served avatar therefore also reject a uniform image. With that in place the blank-identicon mutation fails four tests, and aresizethat loses itsdraw.BiLinear.Scalecall failsTestAvatar_resize, which is the only test that notices it in either module. Both checked by mutation, reading the named failures.One thing this still does not cover, and I would rather name it than leave it implied: nothing asserts that two different users get two different identicons. A generator returning the same image for everyone passes everything here.
Two smaller changes in the same pass:
LessOrEqual(bounds.Dx(), 120)passes a 60x60 result where the limit is 120, andprepServicesets that limit, so the answer is exactly 120x120.TestAvatar_resizeinspects pixels at all, which nothing did before. Checked by scaling from an empty source rectangle: every dimension assertion stays green and only the new one fires.Worth knowing: CI cannot see this class at all, since both test workflows pin
go-version: "1.26". I have not touched the pin. Verified natively on 1.27 and ingolang:1.27-alpine, where every package in both modules passes, and each old assertion fails when restored.The cookie attributes
TestJWT_CookieAttributesFromOptsin both modules, which is the plumbing test from your #315 review.SecureandSameSiteare read from the service config in four places, the two cookiesSetwrites and the twoResetwrites, and every existing test intoken/jwt_test.goruns withSecureCookies: falseand the zeroSameSite, which emits no attribute at all. All four literals could stop passing the config through and the suite would stay green.It also pins the deliberate
HttpOnlydifference on theSetpair, since the token has to be hidden from JS and the XSRF value has to be readable for the caller to echo it back in a header.Mutation-checked one attribute at a time, ten per module:
SecureandSameSitedropped from each of the four literals, plusHttpOnlyflipped on each of theSettwo. Every one fails this test, and in each case this test is the only one that fails.On the
Resetpair the test checks the attributes are repeated, but the reason is not what I first wrote. Cookie identity is name, domain and path, so a removal cookie carrying different flags replaces the original normally, which is also whyResetwritingHttpOnly: falseover a cookieSetwroteHttpOnly: trueworks. The attributes matter because Chrome and Firefox rejectSameSite=NonewithoutSecureoutright, and a removal cookie the browser discards leaves the original in place. The comment in the test says that now.