Skip to content

fix: isolate concurrent PKCE flows to prevent cookie clobbering#403

Open
cn-stephen wants to merge 2 commits intoworkos:mainfrom
cn-stephen:fix/pkce-concurrent-flows
Open

fix: isolate concurrent PKCE flows to prevent cookie clobbering#403
cn-stephen wants to merge 2 commits intoworkos:mainfrom
cn-stephen:fix/pkce-concurrent-flows

Conversation

@cn-stephen
Copy link
Copy Markdown
Contributor

When a user's session expires (e.g. inactivity timeout) and they have multiple tabs open all tabs wake up and fire requests simultaneously, or even if a single tab fires off multiple requests at once that trigger the auth flow. Each request hits the middleware, which generates a fresh PKCE state and overwrites the single shared wos-auth-verifier cookie. The last write wins, so when callbacks return from WorkOS, every tab except the last one fails with "OAuth state mismatch" or "Auth cookie missing".

This is reliably reproducible: open 3-4 tabs, wait for session expiry, switch back to the browser. The tabs wake up concurrently, each triggers a middleware redirect with a different PKCE state, and the shared cookie gets clobbered.

Two changes fix this:

  1. Per-flow cookie names: derive a unique suffix from an FNV-1a hash of the sealed state (e.g. wos-auth-verifier-a3f7c012). Each concurrent flow gets its own cookie, and each callback reads the correct one. Uses @sindresorhus/fnv1a for a fast, format-agnostic 32-bit hash.

  2. Document-only cookies: reuse the existing isInitialDocumentRequest check to only set PKCE cookies on full page navigations. Fetch, XHR, RSC, and prefetch requests never follow cross-origin redirects so they can never complete the OAuth flow. Without this guard, the per-flow cookie names cause many cookies per tab (one per concurrent request e.g., react-query reissuing XHR fetches on window activate), which quickly exceeds browser header limits and triggers HTTP 431 (Request Header Fields Too Large).

When a user's session expires (e.g. inactivity timeout) and they have
multiple tabs open, all tabs wake up and fire requests simultaneously.
Each request hits the middleware, which generates a fresh PKCE state and
overwrites the single shared `wos-auth-verifier` cookie. The last write
wins, so when callbacks return from WorkOS, every tab except the last
one fails with "OAuth state mismatch" or "Auth cookie missing".

This is reliably reproducible: open 3-4 tabs, wait for session expiry,
switch back to the browser. The tabs wake up concurrently, each triggers
a middleware redirect with a different PKCE state, and the shared cookie
gets clobbered.

Two changes fix this:

1. Per-flow cookie names: derive a unique suffix from an FNV-1a hash of
   the sealed state (e.g. `wos-auth-verifier-a3f7c012`). Each concurrent
   flow gets its own cookie, and each callback reads the correct one.
   Uses @sindresorhus/fnv1a for a fast, format-agnostic 32-bit hash.

2. Document-only cookies: reuse the existing `isInitialDocumentRequest`
   check to only set PKCE cookies on full page navigations. Fetch, XHR,
   RSC, and prefetch requests never follow cross-origin redirects so
   they can never complete the OAuth flow. Without this guard, the
   per-flow cookie names cause ~7 cookies per tab (one per concurrent
   request), which quickly exceeds browser header limits and triggers
   HTTP 431 (Request Header Fields Too Large).
The PKCE cookie max-age (600s / 10 minutes) was only applied in the
server component path (setPKCECookie). The middleware path used
getPKCECookieOptions which delegated to getCookieOptions — inheriting
the session cookie's 400-day max-age.

This was invisible before because the single shared cookie name meant
the cookie would be deleted on callback. With per-flow cookie names,
orphaned cookies (e.g. from a tab that never completed the OAuth flow)
now survive with a 400-day expiry instead of self-cleaning after 10
minutes.

Move PKCE_COOKIE_MAX_AGE into getPKCECookieOptions so both the server
component and middleware paths use the same 10-minute TTL.
@cn-stephen cn-stephen marked this pull request as ready for review April 10, 2026 02:45
@cn-stephen cn-stephen requested a review from a team as a code owner April 10, 2026 02:45
@cn-stephen
Copy link
Copy Markdown
Contributor Author

Tested locally, works great.

Comment on lines +117 to +119
return options
.replace(/SameSite=Strict/i, 'SameSite=Lax')
.replace(/Max-Age=\d+/, `Max-Age=${expired ? 0 : PKCE_COOKIE_MAX_AGE}`);
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

ngl this string replace technique is fragile as heck. But it existed and I'm not trying to clean up everything possible.

Hope this is OK for now

@cn-stephen
Copy link
Copy Markdown
Contributor Author

cc @nicknisi as well, as you're the only guy I know on the team xD and we had a good collab previously.

Hope the explanations are sufficient

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

Labels

None yet

Development

Successfully merging this pull request may close these issues.

1 participant