Skip to content

Add PartitionedCookies for third-party (CHIPS) deployments, and unpin the image byte counts - #315

Closed
paskal wants to merge 3 commits into
go-pkgz:masterfrom
paskal:feat/partitioned-cookies
Closed

Add PartitionedCookies for third-party (CHIPS) deployments, and unpin the image byte counts#315
paskal wants to merge 3 commits into
go-pkgz:masterfrom
paskal:feat/partitioned-cookies

Conversation

@paskal

@paskal paskal commented Aug 22, 2026

Copy link
Copy Markdown
Collaborator

An application framed by another site receives its auth cookies as third-party cookies, and browsers now drop those unless they carry Partitioned (CHIPS). Neither Set nor Reset could 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 through auth.Opts to 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 set None for unrelated reasons. A bool rather than a richer type, since the attribute takes no parameters.

Both Set and Reset, which matters more than it looks

All 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 Set would 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:

ordinary third-party cookie    dropped   <- why the attribute is needed at all
partitioned cookie             stored
after unpartitioned clear      alive     <- what Reset does today
after partitioned clear        <nil>

The first line is the case for the change: without Partitioned the auth cookies are not stored at all once the browser enforces the phase-out. The last two are the case for covering Reset, and dropping the attribute from Reset alone fails the test.

Worth noting for anyone reproducing this: Playwright's own default --disable-features argument contains ThirdPartyStoragePartitioning, and it beats --test-third-party-cookie-phaseout passed in args. Measured without dropping that default, an ordinary third-party cookie is still stored and the run proves nothing.

Notes

  • http.Cookie.Partitioned needs Go 1.23; both modules are already on 1.25, so the change is identical in each.
  • Browsers reject Partitioned without Secure, and it is only meaningful alongside SameSite=None. Documented on the field rather than enforced, to match how SecureCookies and SameSite are already left to the caller.
  • Mirrored in the root module and 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.

paskal added 2 commits August 22, 2026 17:13
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.
@paskal
paskal requested a review from umputun as a code owner August 22, 2026 16:15
@coveralls

Copy link
Copy Markdown

Coverage Report for CI Build 32584147060

Coverage increased (+0.05%) to 86.101%

Details

  • Coverage increased (+0.05%) from the base build.
  • Patch coverage: 24 of 24 lines across 2 files are fully covered (100%).
  • No coverage regressions found.

Uncovered Changes

No uncovered changes found.

Coverage Regressions

No coverage regressions found.


Coverage Stats

Coverage Status
Relevant Lines: 3734
Covered Lines: 3215
Line Coverage: 86.1%
Coverage Strength: 9.65 hits per line

💛 - Coveralls

@coveralls

coveralls commented Aug 22, 2026

Copy link
Copy Markdown

Coverage Report for CI Build 32597525585

Coverage decreased (-0.08%) to 85.972%

Details

  • Coverage decreased (-0.08%) from the base build.
  • Patch coverage: 6 uncovered changes across 1 file (40 of 46 lines covered, 86.96%).
  • No coverage regressions found.

Uncovered Changes

File Changed Covered %
auth.go 27 21 77.78%
Total (2 files) 46 40 86.96%

Coverage Regressions

No coverage regressions found.


Coverage Stats

Coverage Status
Relevant Lines: 3721
Covered Lines: 3199
Line Coverage: 85.97%
Coverage Strength: 9.54 hits per line

💛 - Coveralls

@paskal paskal changed the title Add PartitionedCookies for third-party (CHIPS) deployments Add PartitionedCookies for third-party (CHIPS) deployments, and unpin the image byte counts Aug 22, 2026

@umputun umputun left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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()) at avatar/avatar_test.go:194 and :223 doesn't survive the next line, image.Decode on a 0-byte file already fails. Both tests build Proxy{} with no ResizeLimit, so resize returns the body verbatim and the stored identicon is deterministically 300x300 - assertDecodableImage could assert that instead.
  • auth_test.go:349-350: the dev provider serves a 300x300 identicon and prepService sets AvatarResizeLimit: 120, so the result is exactly 120x120. LessOrEqual passes a 60x60 regression; assert.Equal matches what TestAvatar_resize:466 already does. assert.NotEmpty(t, b) on the next line is dead after image.Decode succeeded.
  • nothing in either module inspects a pixel any more. A resize() that lost its draw.BiLinear.Scale call still encodes a valid 120x120 PNG - I checked, 334 bytes, every pixel transparent - and the whole suite stays green. One non-uniformity assertion on imgRz in TestAvatar_resize covers it, which is the test that actually exercises the scale path.
  • TestJWT_PartitionedCookiesOffByDefault ranges over resp.Header.Values("Set-Cookie") with no length check, so it passes if Set writes 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.
@paskal

paskal commented Aug 22, 2026

Copy link
Copy Markdown
Collaborator Author

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 Opts godocs now say the flag helps only where Set runs inside the frame, name direct/telegram/email-code as the flows it does help, and point at requestStorageAccess or the one-shot code exchanged inside the frame for OAuth. The PR description no longer claims it unblocks remark42's separate-domain setup.

Reset clears both forms when the flag is on. Your read of the ordering is what makes it bite: the legacy cookie is expired-but-signed, so the refresh path revives it. The /reset subtest is split and asserts four headers, two of each form.

The missing-Secure warning is in at construction, after the logger is defaulted, worded as you suggested and warning rather than forcing. The godoc claim about SameSite is corrected too: Valid gates on Secure alone, and SameSite=None is about the cookie being sent from a third-party frame rather than accepted.

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 bind: address already in use and a remark42 hostname mask it. I read the log and stopped there instead of the assertion. They now assert the generator's 300x300, measured directly rather than inferred, which is version-independent since the encoder moves bytes and not geometry.

Worth flagging: neither my machine nor CI can demonstrate that fix, because both workflows pin go-version: "1.26". I have not touched that here, since bumping it belongs in its own change, but that pin is why this class reaches you and not CI.

Test smalls: exact dimensions instead of bounds in both places, dead Positive/NotEmpty gone, require.Len on the off-by-default test, and TestAvatar_resize now asserts the result is not uniform. I checked that last one bites: scaling from an empty source rectangle keeps every dimension assertion green and fails only the new one.

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.

@paskal

paskal commented Aug 22, 2026

Copy link
Copy Markdown
Collaborator Author

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 bind: address already in use before reaching any assertion. Running them in a throwaway container gives a clean namespace, and golang:1.27-alpine pins the version that matters:

docker run --rm -v "$PWD":/src -w /src golang:1.27-alpine \
  go test ./provider/ -run "TestCustomProvider|TestDevProvider" -count 1
ok  github.com/go-pkgz/auth/provider  0.634s

Both pass with the fix. Restoring the old assertion in dev_provider_test.go fails with exactly your numbers, expected 960 and actual 1564, so the four sites are confirmed rather than inferred.

The CI observation stands unchanged: both workflows pin go-version: "1.26", so none of this class is visible there whatever the assertions say. The container invocation above is a cheap way to check locally without touching the pin, if that is useful to you.

@paskal

paskal commented Aug 22, 2026

Copy link
Copy Markdown
Collaborator Author

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.

root: auth, avatar, logger, middleware, provider, provider/sender, token   all ok
v2:   the same seven                                                        all ok

So TestCustomProvider and TestDevProvider are green on the version that exposed them, alongside everything else in both modules. No remaining caveat from me about what has and has not been executed.

@paskal

paskal commented Aug 23, 2026

Copy link
Copy Markdown
Collaborator Author

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 remark-fixed.terrty.dev, embedded from a page on host-test.paskal.dev — different registrable domains — and drove Chromium with third-party cookie blocking genuinely in effect. Control assertion in both runs: a plain SameSite=None cookie set from inside the frame comes back nil, so the blocking is real rather than nominal.

Anonymous sign-in, then a reload:

stock master this branch, Partitioned on
control: plain 3p cookie stored blocked blocked
XSRF cookie readable in the frame false true
sign-in, /auth/user 401 token cookie was not presented 200, user returned
after reload 401 200

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 Set runs inside the frame work cross-domain with this on, and do not without it. Your scoping was right and my original framing was wrong; this is the evidence for the version of the feature that survived your review, not for the one I proposed.

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.

@paskal

paskal commented Aug 23, 2026

Copy link
Copy Markdown
Collaborator Author

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. Partitioned does work for the flows whose Set runs inside the frame, anonymous sign-in surviving a reload where stock returns 401.

But it is fixing something already fixed. With AUTH_SEND_JWT_HEADER=true, remark42's own client writes those cookies and marks them Partitioned itself, so cross-domain persistence for anonymous, email and telegram already works on current master. There is an e2e case demonstrating exactly that. So what this PR adds is narrower again: the same behaviour without requiring that flag.

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 SameSite in bd39e5e and XSRFIgnoreMethods in 59656e4 and nothing would have caught either. I kept #317 minimal deliberately so it merges without discussion, but say the word and it goes in as its own small PR covering the existing fields.

Thank you for the review. The OAuth partition-key point in particular was something I should have found by testing and did not.

@paskal

paskal commented Aug 23, 2026

Copy link
Copy Markdown
Collaborator Author

Reopened as #318, because GitHub refuses to reopen a closed PR whose branch has been force-pushed.

I closed this one saying remark42's AUTH_SEND_JWT_HEADER already achieves cross-domain persistence, so the library change bought nothing. That was wrong, and measuring it is what showed me how. The header path gets persistence by returning the token so the frontend can write its own cookie, which requires the token to be readable from JavaScript. This change gets the same persistence with the cookie still HttpOnly. I had treated those as the same outcome.

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, document.cookie inside the frame returns the JWT under the header flag and does not under this change, while both keep the reader signed in across a reload.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants