Skip to content

fix: evict stale PKCE verifier cookies to prevent HTTP 431 (#76)#104

Open
nicknisi wants to merge 3 commits into
mainfrom
fix/pkce-verifier-cookie-eviction
Open

fix: evict stale PKCE verifier cookies to prevent HTTP 431 (#76)#104
nicknisi wants to merge 3 commits into
mainfrom
fix/pkce-verifier-cookie-eviction

Conversation

@nicknisi

Copy link
Copy Markdown
Member

Closes #76.

Problem

getSignInUrl() / getSignUpUrl() / getAuthorizationUrl() read like pure getters, but each call starts an OAuth flow and sets a wos-auth-verifier-<hash> cookie. The per-flow hashed name (so concurrent sign-ins across tabs don't clobber each other) means cookies never overwrite. Using these helpers to display a link — e.g. prefetching in a route loader, or calling unconditionally in beforeLoad even when already signed in — mints a fresh orphan cookie on every navigation. They pile up until the 10-minute PKCE TTL and eventually bloat the Cookie request header into an HTTP 431.

This is the consumer half of the fix; the shared primitive shipped in @workos/authkit-session@0.6.0 (workos/authkit-session#42).

Changes

  • Bump @workos/authkit-session^0.6.0 (required for the new exports).
  • Bounded eviction in server-fn-bodies.ts. After minting a verifier, parse the incoming Cookie header, compute stale verifier names via selectStalePKCEVerifierCookieNames(names, { keep: result.cookieName }) (cap 5, all-but-newest), and clear each via AuthService.clearPendingVerifierByName through middleware's existing pending-Set-Cookie channel. Best-effort — wrapped so a cleanup failure can never break URL generation. Applies to all three helpers.
  • JSDoc warnings on the public helpers in server-functions.ts: they have a cookie side effect and should be called on user action / from a redirect route, not prefetched in a loader for display.

The lazy body/shell boundary is preserved: the new @workos/authkit-session value import lives in server-fn-bodies.ts (a body file, already allowed to import server-only modules), reached only via the RPC-rewritten shell's dynamic import().

Why eviction (not just docs or "don't set a cookie")

  • Can't "return URL without a cookie": PKCE requires the codeVerifier be persisted before the callback. The only cookie-free variant is the redirect-route pattern, which the example already uses.
  • Docs alone are insufficient: the API shape invites the misuse (the Convex TanStack guide prefetches these in loaders). Eviction bounds the damage regardless of how the helper is called; the JSDoc steers people to the right pattern.
  • Mirrors authkit-nextjs (MAX_PKCE_COOKIES = 5).

Tests

  • server-functions.spec.ts — 5 new cases: no eviction within cap; evict all-but-newest over cap; never evicts the freshly minted cookie (even if already on the request); applies to getSignUpUrl/getAuthorizationUrl; URL still generated when eviction throws.
  • Full suite 227 passing; typecheck, lint, SDK build, example build, and build:check (no server-only fingerprints in the client bundle) all clean against the published 0.6.0.

The example app already uses the recommended redirect-route pattern (/api/auth/sign-in), so no example change was needed.

Calling getSignInUrl()/getSignUpUrl()/getAuthorizationUrl() to display a
link — e.g. prefetching in a route loader — mints a uniquely-named
wos-auth-verifier-* cookie on every call that is never navigated to. The
per-flow naming means cookies never overwrite, so orphans accumulate until
their 10-minute TTL and eventually bloat the Cookie header into an HTTP 431.

Wire up the eviction primitives added in @workos/authkit-session@0.6.0:
after minting a new verifier, parse the incoming Cookie header, compute the
stale verifier names via selectStalePKCEVerifierCookieNames (keeping the one
just created), and clear each through middleware's pending-Set-Cookie channel
via clearPendingVerifierByName. Best-effort — a cleanup failure never breaks
URL generation.

Also document the side effect on the public helpers: each call starts an
OAuth flow and sets a cookie, so they should be invoked on user action or
from a redirect route, not prefetched in a loader for display.

Bumps @workos/authkit-session to ^0.6.0 (required for the new exports).

Closes #76
@greptile-apps

greptile-apps Bot commented Jun 15, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR fixes an HTTP 431 error caused by accumulating orphaned PKCE verifier cookies when getSignInUrl()/getSignUpUrl()/getAuthorizationUrl() are called in route loaders for display rather than on user action. It upgrades @workos/authkit-session to 0.6.0 and adds bounded eviction logic that clears all-but-the-newest verifier cookies once the running total would exceed a cap of 5.

  • Eviction logic in server-fn-bodies.ts: After the new PKCE cookie is forwarded via the pending-header channel, evictStalePendingVerifiers parses incoming cookie names, calls selectStalePKCEVerifierCookieNames to identify stale ones, and clears them via Promise.allSettled for best-effort independent cleanup that never blocks URL generation.
  • JSDoc warnings added on all three public helpers in server-functions.ts documenting the cookie side effect and steering callers toward the redirect-route pattern.
  • Tests: 5 new cases cover: within-cap no-op, over-cap full eviction, freshly-minted cookie is never self-evicted, coverage across all three helpers, and resilience when eviction throws.

Confidence Score: 5/5

Safe to merge — the change is additive cleanup logic that never touches the PKCE flow itself, and a suite of 227 tests plus explicit resilience coverage confirm the URL is always returned even when eviction fails.

The eviction runs after the new cookie is already written, the new cookie is protected from self-eviction by the keep parameter, failures are absorbed by Promise.allSettled, and the no-cookie-header early return avoids unnecessary work. The 5 new test cases cover every relevant edge case. No logic or security concerns were found in the changed code.

No files require special attention.

Important Files Changed

Filename Overview
src/server/server-fn-bodies.ts Core logic change: adds evictStalePendingVerifiers called after the new PKCE cookie is set; uses Promise.allSettled for independent best-effort cleanup; guards against absent cookie header with early return.
src/server/server-functions.spec.ts Adds 5 eviction test cases covering within-cap, over-cap, self-eviction guard, multi-helper coverage, and resilience to eviction failures; mock context updated to support per-test cookie headers.
src/server/server-functions.ts JSDoc @remarks blocks added to all three URL-generating helpers, warning callers about the cookie side effect and recommending the redirect-route pattern.
package.json Bumps @workos/authkit-session from ^0.5.3 to ^0.6.0 to pick up selectStalePKCEVerifierCookieNames and clearPendingVerifierByName.
pnpm-lock.yaml Lock file updated to resolve @workos/authkit-session@0.6.0; also widens the peer dependency range for @workos-inc/node to include ^10.0.0.

Sequence Diagram

%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
    participant Client
    participant Middleware
    participant ServerFn as ServerFn
    participant AuthSession as AuthSession

    Client->>Middleware: Request with existing verifier cookies
    Middleware->>ServerFn: getSignInUrl / getSignUpUrl / getAuthorizationUrl
    ServerFn->>AuthSession: createSignIn / createSignUp / createAuthorization
    AuthSession-->>ServerFn: url plus cookieName plus Set-Cookie header
    ServerFn->>Middleware: setPendingHeader Set-Cookie new verifier
    Note over ServerFn: New PKCE cookie forwarded first
    ServerFn->>AuthSession: selectStalePKCEVerifierCookieNames
    AuthSession-->>ServerFn: stale cookie names
    ServerFn->>AuthSession: Promise.allSettled clearPendingVerifierByName for each stale
    AuthSession->>Middleware: setPendingHeader delete stale cookies
    ServerFn-->>Client: authorization URL
Loading
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
    participant Client
    participant Middleware
    participant ServerFn as ServerFn
    participant AuthSession as AuthSession

    Client->>Middleware: Request with existing verifier cookies
    Middleware->>ServerFn: getSignInUrl / getSignUpUrl / getAuthorizationUrl
    ServerFn->>AuthSession: createSignIn / createSignUp / createAuthorization
    AuthSession-->>ServerFn: url plus cookieName plus Set-Cookie header
    ServerFn->>Middleware: setPendingHeader Set-Cookie new verifier
    Note over ServerFn: New PKCE cookie forwarded first
    ServerFn->>AuthSession: selectStalePKCEVerifierCookieNames
    AuthSession-->>ServerFn: stale cookie names
    ServerFn->>AuthSession: Promise.allSettled clearPendingVerifierByName for each stale
    AuthSession->>Middleware: setPendingHeader delete stale cookies
    ServerFn-->>Client: authorization URL
Loading

Reviews (3): Last reviewed commit: "refactor: thread request context into ve..." | Re-trigger Greptile

Comment thread src/server/server-fn-bodies.ts Outdated

@devin-ai-integration devin-ai-integration Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

✅ Devin Review: No Issues Found

Devin Review analyzed this PR and found no bugs or issues to report.

Open in Devin Review

nicknisi added 2 commits June 15, 2026 18:15
Per PR review: the stale-verifier deletes are independent, so fire them
together via Promise.allSettled rather than awaiting sequentially. allSettled
isolates per-cookie failures, preserving the best-effort semantics (a failed
clear just leaves an orphan to expire on its TTL) without the per-iteration
try/catch.
Pass the already-resolved middleware context from forwardAuthorizationCookies
into evictStalePendingVerifiers instead of re-deriving it. This collapses three
getGlobalStartContext() reads per authorization request to one (the function
re-fetched the context and separately called getRedirectUriFromContext, both of
which resolve the same object the caller already holds), uses ctx.redirectUri
directly, and drops a now-dead null guard. Fully explicit data flow; behavior
unchanged.

Also strengthen the "freshly minted cookie is never evicted" test to actually
exercise the over-cap boundary with the kept cookie present on the request,
rather than sitting under the cap where no eviction runs at all.
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.

Cookie multiplying eventually kills server with header too large code 431

1 participant