fix(auth): align cookie lifetimes with session and refresh token boundaries - #3940
rossnelson wants to merge 3 commits into
Conversation
The refresh cookie was given the access token's lifetime, and the user* cookies a flat minute regardless of the session boundary. Both are fixed here, along with the missing Docker settings that made the second one impossible to configure. Refresh cookie (#3210). Its MaxAge came from oauth2.Token.Expiry, which is populated from the token response's expires_in. Per RFC 6749 5.1 and OIDC Core 3.2.2.5 that describes the access token, not the refresh token, so the cookie was dropped at the moment the access token expired and the refresh it existed to perform came back 401. The lifetime now comes from the refresh token's own exp claim where the provider issues a JWT, then from a new per-provider refreshTokenDuration for providers that issue opaque tokens, then a 7 day default. The existing 30 day cap is kept. User cookies (#3223). They were always issued for 60 seconds, so a refresh shortly before the session boundary left the browser holding credentials the server had already stopped honouring: a signed-in UI whose every API call returned 401. They are now clamped to whatever is left of the session. Docker config (#3223). maxSessionDuration was enforced but absent from docker.yaml, so it could not be set without a wholly custom config file. Both it and refreshTokenDuration are now exposed, and both default to unset so existing deployments are unaffected. Also corrects docs advising `maxSessionDuration: 0`, which yaml.v3 rejects as an int rather than a duration. The working spelling is `0s`.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Verified end to end against a running stackReal Chrome → real Demo config, chosen to make the failure quick rather than to be realistic: mock IdP access token 10s, refresh token 24h,
Both numbers are read from the literal That 401 is exactly #3210 as reported: the refresh cookie is gone at the moment it is needed, so the browser sends nothing and the user is signed out mid-session. The 60s vs 45s row is the #3223 cookie half — a signed-in UI whose every API call returns 401. Recording of both runs to follow. |
The Go tests added alongside the fix drive the refresh handler in process. That covers the logic but stops short of the thing users actually hit: a browser, a real identity provider, and cookies the Go server writes as Set-Cookie headers. tests/integration/oauth-flow.spec.ts cannot fill the gap either, since it mocks /auth/sso and /auth/refresh outright, which is exactly what would need to be real. This starts a second, auth-enabled ui-server on 8081 alongside the existing E2E one, backed by the repo's own mock OIDC provider, and drives a real login against it. The e2e CI job already builds the Go server and runs the Playwright suite, so this needs no new job or infrastructure. Three of the four tests fail against the previous behaviour: the refresh cookie carries the access token's 5s lifetime rather than 24h, the user0 cookie is issued for 60s against a 45s session, and the refresh after the access token expires returns 401. The fourth asserts that session expiry still ends a session, and passes either way, so that lengthening the cookies cannot quietly disable maxSessionDuration. The ui-server test harness tracked one server in a module level variable, which two concurrent servers would clobber, leaving the first without a handle to shut down. It is now keyed by env.
Added a real end-to-end testCorrecting my earlier wording: the Go tests I first described as end-to-end are integration tests of the refresh handler. Useful, but they never touch a browser or a real identity provider. There is now a genuine E2E test as well.
I looked at extending Against the previous behaviour, three of the four fail: The one that passes both ways is deliberate — it waits past Full E2E suite is 37 passed with this in, so the second server and the harness change break nothing. One harness change worth a reviewer's eye: |
Map.get returns UIServer | undefined. The previous module level variable was typed as UIServer and hid that, so keying the registry surfaced a strict mode error the old shape had been papering over. The return type now says what it returns, and the one caller that assumed a server handles its absence.
1 participant
Description & motivation 💭
Supersedes #3235, rebuilt from current
main. That PR was opened from a fork'smainbranch, so it could not be updated in place, andSetUserhas since gained asecureparameter. Thanks to @ralf157 for the original diagnosis and fix — the analysis in #3235 is what this is built on.auth-cookie-lifetimes.mp4
Two bugs, both confirmed still present on
main:1. Refresh cookie expires with the access token (#3210)
The
refreshcookie'sMaxAgecame fromoauth2.Token.Expiry, which is populated from the token response'sexpires_in. Per RFC 6749 §5.1 and OIDC Core §3.2.2.5 that describes the access token. The cookie was therefore dropped at the exact moment the access token expired, so the refresh it exists to perform arrived with no cookie and came back 401. Reported against Keycloak, reproducible on any IdP whose refresh token outlives its access token.The lifetime now resolves in this order:
expclaim, when the IdP issues a JWT refresh token (Keycloak and friends — no configuration needed).refreshTokenDuration, for IdPs that issue opaque refresh tokens whose lifetime the server cannot read.The existing 30-day cap is kept.
2. User cookies outlive the session boundary (#3223)
user*cookies were always issued for a flat 60 seconds. A refresh performed shortly beforemaxSessionDurationelapsed handed the browser a full minute of credentials the server had already stopped honouring — a UI that looks signed in while every API call behind it returns 401. They are now clamped tomin(60s, time left in session).3.
maxSessionDurationunreachable in Docker (#3223)The field was enforced server-side but absent from
docker.yaml, so it could not be set without supplying a wholly custom config file. Both it andrefreshTokenDurationare now exposed as environment variables.Design Considerations 🎨
#3235defaultedTEMPORAL_MAX_SESSION_DURATIONto2m, copied from the local dev config, which would have silently started logging every Docker deployment out every two minutes.TestDockerConfigSessionDefaultsAreUnsetpins this.refreshTokenDurationsits onAuthProviderrather thanAuth, since refresh token lifetime is a property of the IdP.expis read without signature verification, because it only chooses a cookie lifetime. Nothing is trusted on the strength of it — the IdP still validates the token on every refresh, and a forgedexpcan only make the browser drop a cookie earlier or later than it needed to. The comment onjwtExpsays so.SetUser's three positional booleans/durations would have grown to five, so the trailing arguments are now aCookieOptionsstruct. Only two call sites.with-auth.yamlboth advisemaxSessionDuration: 0for "unlimited", whichyaml.v3rejects (cannot unmarshal !!int 0 into time.Duration). Corrected to0s.Scope vs #3235
#3235 also added ~1100 lines of new E2E infrastructure: two
docker-composestacks, a Keycloak 26 realm import, two extra Playwright configs and a duplicate mock-OIDC config, none of it wired into CI. That is not carried over.The equivalent coverage here reuses what the repo already has. The
e2e-testsjob already builds the Go server and runs Playwright against a realui-server, andutilities/oidc-serveris already a working identity provider, so the browser-level coverage cost one config file, one harness module and one spec, inside CI jobs that already run.Testing 🧪
How was this tested 👻
End to end (
tests/e2e/auth-cookie-lifetimes.spec.ts)Real Chromium, a real login at the repo's own mock OIDC provider, and a real auth-enabled
ui-serverprocess. The cookie lifetimes under test are decided by the Go server and reach the browser only asSet-Cookieheaders, so the assertions read those headers directly.tests/integration/oauth-flow.spec.tscould not be extended for this: it mocks/auth/ssoand/auth/refreshwithpage.route, which is precisely what has to be real here.The
e2e-testsCI job already builds the Go server and runs Playwright, so this adds no new CI job and no new infrastructure.tests/global-setup.tsstarts a second, auth-enabledui-serveron 8081 alongside the existing one on 8080, plus the mock provider, viautilities/auth-e2e-stack.tsand a newserver/config/e2e-auth.yaml.refresh cookie outlives the access tokenrefreshMax-Age is 24h, not the 5s access token lifetimeuser cookies do not outlive the sessionuser0Max-Age ≤ the 45smaxSessionDuration, and under the 60s defaulttoken refresh succeeds after the access token has expiredGET /auth/refresh→ 200refresh is refused once the session has expiredmaxSessionDuration, then/auth/refresh→ 401Against the previous behaviour, the first three fail and the fourth passes:
That fourth test is the guard: lengthening the cookies must not quietly turn
maxSessionDurationinto a limit that no longer ends a session, so it has to pass both before and after.Full E2E suite: 37 passed, so the second server and the harness change break nothing.
Integration (
server/server/route/auth_cookies_test.go)Six Go tests that call the
refreshTokenshandler in process against anhttptestidentity provider — real HTTP token exchange, realoauth2client, real cookie serialization. Faster than the browser tests and covers the opaque-token and default-fallback branches that the mock provider alone cannot reach.TestRefreshCookieOutlivesTheAccessTokenexpires_in=5while issuing a 7-day JWT refresh token → the cookie tracks the token'sexpTestRefreshCookieForOpaqueTokenUsesConfiguredDurationrefreshTokenDurationis usedTestRefreshCookieForOpaqueTokenFallsBackToDefaultTestUserCookiesNeverOutliveTheSessionuser0Max-Age is 30, not 60TestUserCookiesKeepDefaultWhenSessionHasRoomuser0Max-Age stays 60TestUserCookiesUnaffectedWithoutMaxSessionDurationuser0Max-Age stays 60The four bug-specific ones fail against the old logic; the two "nothing should change" ones pass against both.
Unit
server/server/auth/cookie_test.gocoversjwtExp(valid, fractionalexp, base64 padding, and ten rejection cases including opaque tokens and malformed JWTs), the fullrefreshCookieMaxAgepriority chain including both cap paths and an already-expiredexp, and theuserCookieMaxAgeboundaries.server/plugins/fs_config_provider/loader_test.goloads the realdocker.yamlthrough the real template pipeline, pinning the unset defaults and checking both environment variables take effect.Not tested here
No live Keycloak. The Keycloak-specific behaviour is the JWT
expbranch, exercised byTestRefreshCookieOutlivesTheAccessTokenwith a Keycloak-shaped refresh token, but that is a stand-in rather than the real server — the mock provider issues opaque refresh tokens, so the browser tests cover the configured-duration branch instead. @ralf157 tested #3235 against live Keycloak 26; a confirmation on this branch would be welcome.Checklists
Merge Checklist
Issue(s) closed
Closes #3210
Closes #3223
Docs
AUTHENTICATION.mdgains a Refresh token lifetime section describing the priority chain and whyexpires_inis not used, a Docker Environment Variables table for the auth settings, a note thatuser*cookies are held to the session boundary, and a troubleshooting entry for refresh failing with 401 at access-token expiry.