Add PartitionedCookies for third-party (CHIPS) deployments, and unpin the image byte counts - #315
Add PartitionedCookies for third-party (CHIPS) deployments, and unpin the image byte counts#315paskal wants to merge 3 commits into
Conversation
Three tests pinned the exact encoded size of a PNG the standard library produces. Go 1.27 changed the encoder, so the same picture now comes out 507 bytes where the test wanted 569, and 1633 and 1617 where it wanted 999 and 992. Nothing in this repository changed and all three fail. What the avatar path owes its caller is a decodable image within the resize limit, so that is what they check now. The byte count belongs to whichever encoder the Go release happens to ship.
An application framed by another site gets its auth cookies as third-party cookies, and browsers now drop those unless they carry Partitioned. Neither Set nor Reset could emit the attribute, so an embedded deployment had no way to keep a session across a reload. PartitionedCookies is opt-in and defaults off, so existing deployments write byte-identical cookies. It is deliberately not implied by SameSite=None: the partition key is the embedding top-level site, so implying it would silently move an existing cookie into a different jar. Both Set and Reset carry it, which matters more than it looks. A partitioned cookie and an unpartitioned expiry are different cookies to the browser, so clearing without the attribute leaves the original in place and the user stays signed in. Verified in Chromium against a genuine cross-site embed over https: the unpartitioned expiry leaves the value readable, the partitioned one removes it. Mirrored in v1 and v2 per the project rule for shared behaviour.
Coverage Report for CI Build 32584147060Coverage increased (+0.05%) to 86.101%Details
Uncovered ChangesNo uncovered changes found. Coverage RegressionsNo coverage regressions found. Coverage Stats
💛 - Coveralls |
Coverage Report for CI Build 32597525585Coverage decreased (-0.08%) to 85.972%Details
Uncovered Changes
Coverage RegressionsNo coverage regressions found. Coverage Stats
💛 - Coveralls |
umputun
left a comment
There was a problem hiding this comment.
the plumbing is right and every cookie the library writes is covered, but the flag does not do what the description says for OAuth, and for OAuth it makes things worse rather than neutral. That is the one blocking thing here; the rest is small.
enabling this breaks OAuth logins that work today
a CHIPS partition key is the top-level site at the moment the cookie is set, plus a same-site/cross-site bit. Remark42's oauthSignin opens the provider with window.open, and a popup is its own top-level context, so the callback's cookie is keyed to the auth host. The widget iframe is keyed to the embedder. Those never match.
measured in Chrome across three configurations:
default Chrome, 3PC allowed unpartitioned: reaches the iframe Partitioned: invisible
user setting "block 3rd-party" unpartitioned: blocked Partitioned: blocked
--test-third-party-cookie-phaseout unpartitioned: reaches iframe Partitioned: invisible
so for a framed deployment the flag fixes nobody whose OAuth is already blocked, and for the default Chrome majority it converts a working session cookie into one the frame can never see. An operator who reads the description and turns it on to fix his embed logs his users out instead.
it does work where Set runs inside the frame: direct, telegram, and the email-code flow. That is a real feature and worth having.
what needs to change is the framing, not the code. Pls drop the claim that this unblocks remark42's separate-domain setup, and say on both Opts fields that OAuth-popup sessions are keyed to the auth site and will not reach the frame. If you want a pointer for the OAuth case, the mechanisms are document.requestStorageAccess() from the widget, or the popup handing a one-shot code to the iframe which then exchanges it so Set runs under the embedder partition. Both are remark42 frontend work, not this library.
turning it on strands the cookies already in browsers
also measured. With a legacy unpartitioned JWT and a new partitioned one, the request carries JWT=legacy; JWT=new; Chrome sorts equal-path cookies oldest first and Request.Cookie returns the first occurrence, so Get picks the legacy one. After the current Reset, which writes Partitioned expiries only, the wire still shows JWT=legacy. That cookie is expired-but-validly-signed, so middleware/auth.go:138-144 refreshes it into a fresh partitioned pair and the user is signed back in, up to CookieDuration.
fix is for Reset to also emit unpartitioned expiries for both names while the flag is on. First-party that deletes the legacy pair; in a blocked third-party context the extra header is dropped anyway. Note it takes TestJWT_PartitionedCookies's /reset subtest from 2 headers to 4, two with the attribute and two without, so the loop asserting every header carries Partitioned has to split.
worth saying in the docs that this is not a global logout: LogoutHandler at provider/oauth2.go:277-279 returns early when Get fails, so it never reaches Reset, and no expiry sent from inside a frame can clear a jar in another partition.
PartitionedCookies without SecureCookies writes a cookie the stdlib itself rejects
JWT=tok; Path=/; HttpOnly; Partitioned
Cookie.Valid() -> http: partitioned cookies must be set with Secure
http.SetCookie calls String(), never Valid(), so it goes out and browsers drop both cookies. Set returns nil, the login looks fine, there is no session and nothing in the log. auth.go:155 already has the shape for this:
if opts.PartitionedCookies && !opts.SecureCookies {
res.logger.Logf("[WARN] PartitionedCookies requires SecureCookies, browsers will reject the cookie")
}anywhere after auth.go:126, where logger.NoOp is defaulted. Warn rather than force: overriding an explicit SecureCookies: false on a published field would be worse.
while you are in that godoc, "browsers reject the attribute otherwise" isn't right about SameSite. Cookie.Valid() gates on Secure alone. SameSite=None is what makes the cookie get sent from a third-party frame, not what makes it accepted, and the two Opts godocs should say the same thing.
the byte-count fix missed four sites
TestCustomProvider and TestDevProvider are not a port collision or a hostname that doesn't resolve. They are the same Go 1.27 encoder change, at provider/dev_provider_test.go:81 and provider/custom_server_test.go:172 plus both v2 mirrors, all assert.Equal(t, 960, len(body)) against 1564 actual. Same treatment as the three you already did.
CI doesn't show it because both workflows pin go-version: "1.26", so the branch didn't need this commit to be green. It's still a fix I want, I run 1.27 locally.
smaller things in the tests
assert.Positive(t, fi.Size())atavatar/avatar_test.go:194and:223doesn't survive the next line,image.Decodeon a 0-byte file already fails. Both tests buildProxy{}with noResizeLimit, soresizereturns the body verbatim and the stored identicon is deterministically 300x300 -assertDecodableImagecould assert that instead.auth_test.go:349-350: the dev provider serves a 300x300 identicon andprepServicesetsAvatarResizeLimit: 120, so the result is exactly 120x120.LessOrEqualpasses a 60x60 regression;assert.Equalmatches whatTestAvatar_resize:466already does.assert.NotEmpty(t, b)on the next line is dead afterimage.Decodesucceeded.- nothing in either module inspects a pixel any more. A
resize()that lost itsdraw.BiLinear.Scalecall still encodes a valid 120x120 PNG - I checked, 334 bytes, every pixel transparent - and the whole suite stays green. One non-uniformity assertion onimgRzinTestAvatar_resizecovers it, which is the test that actually exercises the scale path. TestJWT_PartitionedCookiesOffByDefaultranges overresp.Header.Values("Set-Cookie")with no length check, so it passes ifSetwrites nothing.require.Len(t, cookies, 2)like the sibling test 30 lines up.
one plumbing test worth adding
nothing asserts auth.Opts.PartitionedCookies reaches token.Opts, and that literal has silently dropped a field twice: bd39e5e for SameSite, and 59656e4 for XSRFIgnoreMethods in #225 - which was this same diff shape, an 18-line re-indent plus one added key. Three lines, since token.Service embeds Opts:
svc := NewService(Opts{PartitionedCookies: true, SecureCookies: true, SameSiteCookie: http.SameSiteNoneMode})
assert.True(t, svc.TokenService().PartitionedCookies)for the record the re-indent itself is clean, git diff -w reduces the whole hunk to the one added line in both modules. Build, tests, -race and lint pass in both, and the v1/v2 mirroring is byte-identical apart from claims.Id/claims.ID.
…g Secure The description and the godoc claimed this unblocks a framed deployment's OAuth. It does not, and for the common case it is worse than neutral: the partition key is the top-level site at the moment the cookie is set, an OAuth popup is its own top-level context, so a callback cookie is keyed to the auth site and the frame never sees it. Where unpartitioned third-party cookies are still allowed, enabling the flag turns a working OAuth session into an invisible one. Both godocs now say so, and point at requestStorageAccess or a one-shot code exchanged inside the frame as the application-side routes. The feature is real for direct, telegram and the email-code flow, where Set runs inside the frame, and that is what it now claims. Reset also has to clear the unpartitioned pair. A partitioned expiry does not match a cookie stored before the option was turned on; the browser then sends both, sorted oldest first, and Request.Cookie returns the first, so Get reads the legacy one. It is expired but validly signed, so the refresh path turns it back into a live session. The /reset subtest now asserts four headers, two of each form. PartitionedCookies without SecureCookies produces a cookie http.Cookie.Valid rejects, but SetCookie writes through String and never calls Valid, so the header goes out, browsers drop both cookies and the login silently produces no session. Warned at construction rather than forced, since overriding an explicit SecureCookies: false would be worse. And the godoc had SameSite wrong: Valid gates on Secure alone, while SameSite=None is what makes the cookie get sent from a third-party frame. Four byte-count sites were missed in the first pass, in the dev and custom provider tests in both modules. They assert the generator's 300x300 geometry now, which no Go release moves, rather than an encoded length. Worth noting CI cannot see this class at all: both workflows pin go-version 1.26. Test fixes from the same review: the avatar assertions check exact dimensions rather than a lower bound, since the identicon is 300x300 and the proxied one exactly 120x120, so an upper bound would pass a regression; the dead Positive and NotEmpty assertions after a successful decode are gone; TestJWT_PartitionedCookiesOffByDefault has a length check, without which it passes when Set writes nothing; and TestAvatar_resize now asserts the result is not a uniform image, which is the only thing in either module that opens a pixel and so the only thing that would notice a resize that stopped scaling. Also adds a test that the option reaches the token service, since that literal has silently dropped a field twice before.
|
All addressed. The first one was the important correction and I had the framing wrong. The OAuth claim is gone. You are right that the partition key is the top-level site at the moment the cookie is set and that a popup is its own top-level context, so a callback cookie is keyed to the auth host while the frame is keyed to the embedder. Worse than neutral for the default-Chrome case, as your table shows: a working session becomes one the frame cannot see. Both
The missing- The four byte-count sites. You are right, and I was wrong twice about them, having told two other people they were environmental. What misled me is that on this machine they fail before reaching the assertion: a container holds 8084, so Worth flagging: neither my machine nor CI can demonstrate that fix, because both workflows pin Test smalls: exact dimensions instead of bounds in both places, dead Plumbing test added, and it fails if the field is dropped from the literal. Both modules build, lint clean at v2.12.2, and every package passes except the two provider tests, which fail here for the port reason above. |
|
Follow-up on the one caveat in my last comment: I said neither my machine nor CI could demonstrate the byte-count fix. That was only half true, and I have now run it. My local toolchain is already 1.27; what blocked those two tests here was a container binding 127.0.0.1:8084, so they died on Both pass with the fix. Restoring the old assertion in The CI observation stands unchanged: both workflows pin |
|
One more datapoint, now that the port here is free: both modules pass completely on a native Go 1.27 run, not only in a container. So |
|
I had not actually tested this end to end, only the cookie primitives, which is how you found the OAuth problem and I did not. Now done, and it changes what I can claim. Built remark42 master against this branch, deployed it over real https on a host serving Anonymous sign-in, then a reload:
The reload is the half that matters, since the widget holds a token in memory for the life of a page and a test that signs in and posts without reloading passes while persistence is entirely broken. So the narrowed claim holds where you said it would: the flows whose I have not tested the OAuth case. Your analysis of why it cannot work is sound and I am not going to claim otherwise from a test I have not run — say the word if you want me to stand the popup flow up on the same rig and confirm it directly. Separately I have split the test-only half out as #317, so the Go 1.27 assertions can land without waiting on any of this. |
|
Closing this rather than iterating further, because your review plus a proper end-to-end test left it with much less justification than it started with, and I would rather say so than keep polishing it. What changed. I finally tested the thing instead of the primitive: remark42 master built against this branch, deployed over real https, embedded from a page on another registrable domain, Chromium with third-party cookie blocking genuinely in effect and a control cookie proving it. But it is fixing something already fixed. With Weighed against your first finding, that enabling it converts a working OAuth session into one the frame cannot see wherever unpartitioned third-party cookies are still allowed, a flag that fixes an already-fixable case while introducing that footgun is not a good trade. Your call would have been the same and it is better made now than after another round. What survives, in #317: the seven Go 1.27 byte-count assertions and the avatar test fixes, test-only, no cookie code, nothing to discuss. What I have not done, and will if you want it: the plumbing test you asked for. Your reason for it stands entirely apart from this feature, since the literal dropped Thank you for the review. The OAuth partition-key point in particular was something I should have found by testing and did not. |
|
Reopened as #318, because GitHub refuses to reopen a closed PR whose branch has been force-pushed. I closed this one saying remark42's The evidence is in #318: a real cross-domain rig on two different registrable domains with real certificates, driven on Chromium, Firefox, WebKit and real Safari 27, with a control cookie behind every blocked result. On Safari with third-party cookies blocked, |
An application framed by another site receives its auth cookies as third-party cookies, and browsers now drop those unless they carry
Partitioned(CHIPS). NeitherSetnorResetcould emit the attribute, so an embedded deployment has no way to keep a session across a reload. This is what blocks remark42's documented separate-domain setup, where the widget is served from its own host and framed by the site it comments on.Shape
Opts.PartitionedCookies bool, plumbed throughauth.Optsto the token service, opt-in and defaulting off so existing deployments write byte-identical cookies.It is deliberately not implied by
SameSite=None. The partition key is the embedding top-level site, so implying it would silently move an existing cookie into a different jar for an operator who setNonefor unrelated reasons. A bool rather than a richer type, since the attribute takes no parameters.Both
SetandReset, which matters more than it looksAll four cookie constructions carry it. A partitioned cookie and an unpartitioned expiry are different cookies to the browser, so clearing without the attribute leaves the original in place and the user stays signed in. Covering only
Setwould trade a login bug for a sign-out bug.Measured in Chromium against a genuine cross-site embed over real https, with third-party cookie blocking actually in effect, setting and clearing from inside the third-party frame:
The first line is the case for the change: without
Partitionedthe auth cookies are not stored at all once the browser enforces the phase-out. The last two are the case for coveringReset, and dropping the attribute fromResetalone fails the test.Worth noting for anyone reproducing this: Playwright's own default
--disable-featuresargument containsThirdPartyStoragePartitioning, and it beats--test-third-party-cookie-phaseoutpassed inargs. Measured without dropping that default, an ordinary third-party cookie is still stored and the run proves nothing.Notes
http.Cookie.Partitionedneeds Go 1.23; both modules are already on 1.25, so the change is identical in each.PartitionedwithoutSecure, and it is only meaningful alongsideSameSite=None. Documented on the field rather than enforced, to match howSecureCookiesandSameSiteare already left to the caller.v2/per the project rule for shared behaviour.The first commit is a test fix, not part of the feature
Three tests pinned the exact encoded size of a PNG the standard library produces, and Go 1.27 changed the encoder: 569 becomes 507, 999 becomes 1633, 992 becomes 1617, all failing on a clean checkout with nothing in the repository touched. They now decode the image and check it fits the resize limit, which is what the avatar path actually promises. It is carried here rather than in a PR of its own because this branch needs it to have a green CI.
Two failures in
provider/(TestCustomProvider,TestDevProvider) are also pre-existing but unrelated and left alone: a port collision on 8084 and a hostname that resolves only in some environments.