Add PartitionedCookies so an embedded app can keep an HttpOnly session - #318
Add PartitionedCookies so an embedded app can keep an HttpOnly session#318paskal wants to merge 4 commits into
Conversation
Coverage Report for CI Build 32834275027Coverage increased (+0.1%) to 86.178%Details
Uncovered ChangesNo uncovered changes found. Coverage RegressionsNo coverage regressions found. Coverage Stats
💛 - Coveralls |
umputun
left a comment
umputun
left a comment
There was a problem hiding this comment.
the legacy-clearing branch in Reset needs its counterpart in Set, otherwise the feature never reaches an existing session.
Reset handles this already (token/jwt.go:363, same in v2) and its comment states the premise exactly: a pair written before the option was on survives a partitioned expiry, the browser sends both, Request.Cookie returns the first. Set writes only the partitioned pair and never touches the old one.
so, deployment has sessions, operator turns PartitionedCookies on, reader comes back without signing out:
Getreads the legacy cookie, since it comes first in the header- it is past
TokenDuration, middleware refreshes it Setwrites fresh claims into the partitioned jar, legacy cookie untouched- next request, back to 1
the refresh never sticks. Anything ClaimsUpd sets on refresh, role, admin, a blocked attribute, never reaches the reader, and no pre-existing session gets the CHIPS behaviour this exists for. Runs until the legacy cookie hits its original CookieDuration, so a month on a typical setting. It is quiet because the XSRF pair shadows in step, so double-submit still matches and nothing 401s.
confirmed on Chromium: both forms are stored, both sent, legacy first. Not claiming every browser orders it that way, RFC 6265bis makes it a SHOULD and says not all UAs comply, but Chromium is the browser CHIPS is for.
fix is a legacy-expiry branch in Set, not a mirror of Reset: keep writing the live partitioned pair and additionally expire the unpartitioned one. Reset expires both because it is signing out. Mirrored in v2, and worth a test for the coexistence case since nothing covers it now.
one merge note: #317 carries the same identicon hunk as this branch in provider/dev_provider_test.go and provider/custom_server_test.go, and its version is the superset. Cleanest is #317 first, then this rebases and drops its copy.
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.
…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.
Rebasing this branch onto current master hit conflicts in auth_test.go, and I resolved them by taking master's side, which silently dropped the test that came with the change. Coveralls caught it: the new field reaches the token service through a struct literal nothing asserted, and the warning branch for Partitioned without Secure had no coverage at all. TestNewService_PassesCookieOptionsToTokenService is the recovered one, and it exists because that literal has dropped a field twice before, for SameSite and for XSRFIgnoreMethods, both in diffs shaped exactly like this one. TestNewService_WarnsOnPartitionedWithoutSecure covers the warning, which is the only signal a caller gets for a pairing that produces a header browsers discard without any error. Both mutation-checked: removing the struct field fails the first and not the second, removing the warning fails the second and not the first.
…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.
142c48f to
c4752d7
Compare
|
Both #317 notes addressed, and rebased onto master now that it carries #317. The identicon uniqueness gap is closed. You were right that it was a deliberate trade and not something I ran out of room for; my PR body flagged it and then left it, which is the wrong place to leave a known gap. Mutation-checked the way the finding describes: making The duplicate hunks are gone. One thing worth recording, since it nearly cost the same test twice. The earlier rebase of this branch hit a conflict in |
3 participants
Reopening the work from #315, which I closed on a mistaken reading. This time with measurements instead of an argument.
Why I closed it, and why that was wrong
I closed #315 because remark42's
AUTH_SEND_JWT_HEADERalready delivers cross-domain persistence, so the library change looked redundant. That is true about persistence and misses what the header path costs. It works by having the server return the token inX-JWTso the frontend can write its own cookie, which means the token has to be readable from JavaScript.Partitionedon the server-set cookie gets the same persistence while the cookie staysHttpOnly. Those are not the same outcome, and I treated them as if they were.The rig
Two genuinely different registrable domains, not hostname aliases: the application on subdomains of one, the embedding page on another, real wildcard certificate, real DNS, no resolver flags. Chromium, Firefox and WebKit through Playwright; Safari 27 through its own WebDriver, so the Safari result is Safari and not an approximation.
Every "blocked" result carries a control. An ordinary
SameSite=Nonecookie and a validPartitionedsentinel are written from inside the embedded frame and read back throughdocument.cookiein the same evaluate. The ordinary one must be gone and the sentinel present, otherwise the run is not blocking anything and its passes mean nothing. That control caught two runs I would otherwise have reported as evidence.The result
Both configurations keep a reader signed in across a reload with third-party cookies blocked. The difference is what JavaScript can reach. Read from inside the frame, on real Safari, blocking confirmed by the control:
document.cookieinside the frameSendJWTHeader, frontend writes the cookiexdpart=1; JWT=eyJhbGci…PartitionedCookies, server writes the cookiexdpart=1; XSRF-TOKEN=c1090a08…The first row is the token, readable by any script on the page. In the second the JWT is absent from
document.cookiebecause it is stillHttpOnly; only the XSRF value is visible, which is the double-submit pattern working as designed.The
Set-Cookiethis produces:Per engine, with the partitioned build: Chromium keeps the session under enforced partitioning, WebKit keeps it, Safari 27 keeps it, Firefox keeps it in accept-all and under Total Cookie Protection. Firefox with "block all third-party cookies" loses it, and so does every other configuration, because that mode discards partitioned cookies as well; nothing in this library can change that.
What it does not fix
Not OAuth. The partition key is the top-level site at the moment the cookie is set, and an OAuth callback runs in a popup that is its own top-level context, so the callback cookie is keyed to the auth host and the embedded frame never sees it. The flows this helps are the ones whose
Setruns inside the frame. The option's godoc says so, and says what OAuth would actually need.The change
PartitionedCookiesonOpts, threaded to the four cookiesSetandResetbuild, mirrored in both modules. It requiresSecureCookies, sincehttp.Cookie.ValidrejectsPartitionedwithoutSecureand browsers drop such a cookie; the constructor warns when that combination is missing rather than emitting something the browser will silently discard.Resetalso clears the unpartitioned form. A partitioned expiry does not match an unpartitioned cookie, so a pair written before the option was enabled would otherwise survive sign-out: the browser sends both,Request.Cookiereturns the first, and the refresh in middleware turns the stale one back into a live session.Tests in both modules, and the library's own suites pass under
-racewith lint clean.Coverage
Rebasing onto current master hit conflicts in
auth_test.goand I resolved them by taking master's side, which silently dropped the test that shipped with this change. Coveralls caught it.TestNewService_PassesCookieOptionsToTokenServiceis the recovered one, and it exists because thattoken.Optsliteral has dropped a field twice before, forSameSiteand forXSRFIgnoreMethods, both in diffs shaped like this one: a re-indent plus one added key, with nothing failing either time.TestNewService_WarnsOnPartitionedWithoutSecurecovers the warning branch, which is the only signal a caller gets for a pairing that produces a header browsers discard silently.Both mutation-checked against each other: removing the struct field fails the first and not the second, removing the warning fails the second and not the first.