fix: keep the PKCE verifier out of OAuth state

[devin-ai-integration]

Summary

The PKCE code verifier was serialized into the OAuth state parameter, so it round-tripped through the authorization response URL. This change keeps the verifier in the cookie only and reconstructs it on callback, so it no longer travels in the URL.

Please review before merging.

Deployment compatibility

This security fix intentionally rejects the previous shared state/cookie format. Old-format callbacks reaching updated instances and new-format callbacks reaching old instances both fail during a mixed-version rollout. Accepting legacy URL state would retain the callback-replay vulnerability.

Deploy sign-in and callback handlers together with a coordinated cutover. In-flight sign-ins using the old format must restart afterward. Existing authenticated sessions are unaffected. The README now documents this upgrade requirement.

[devin-ai-integration]

Original prompt from Linear User

Please work on ticket "AuthKit React Router OAuth callback: PKCE code verifier sealed into the state URL parameter with self-satisfiable double-submit check — leaked callback URL yields full account takeover" (SEC-1309)

@playbook:playbook-b588614117c7477a9b9729928385384f

[devin-ai-integration]

🤖 Devin AI Engineer

I'll be helping with this pull request! Here's what you should know:

✅ I will automatically:

• Address comments on this PR. Add '(aside)' to your comment to have me ignore it.
• Look at CI failures and help fix them

Note: I can only respond to comments from users who have write access to this repository.

⚙️ Control Options:

• Disable automatic comment, CI, and merge conflict monitoring

[linear-code]

SEC-1309

[greptile-apps]

Confidence Score: 4/5

The PR should not be merged until the previously reported mixed-version callback failure is operationally resolved or the coordinated cutover requirement is confirmed acceptable.

Summary

This security update separates OAuth state from the PKCE verifier.

• Stores the verifier only in a separately sealed, flow-specific HttpOnly cookie.
• Binds URL state and the verifier cookie through a shared nonce during callback validation.
• Rejects legacy and replayed state formats and adds regression coverage for callback replay and concurrent flows.
• Documents the coordinated deployment cutover required by the incompatible format change.

Diagram

sequenceDiagram
    participant B as Browser
    participant A as Updated application
    participant W as WorkOS
    A->>A: Generate verifier, challenge, and nonce
    A-->>B: Set sealed verifier cookie
    A-->>B: Redirect with sealed non-secret state
    B->>W: Authorize with challenge and state
    W-->>B: Return code and state
    B->>A: Callback with code, state, and cookie
    A->>A: Unseal both payloads and compare nonces
    A->>W: Exchange code with cookie-held verifier
    W-->>A: Authentication result
    A-->>B: Create session and clear verifier cookie

Loading

_{Reviews (5) · Last reviewed commit: "docs: explain PKCE security update deplo..."}

[devin-ai-integration]

Re: Greptile's P1 "Mixed Versions Reject Valid Callbacks" (src/get-authorization-url.ts:50-55) — acknowledged, but I don't think it should change this PR.

This is a transient rolling-deploy artifact, not a correctness bug in the new code, and it's inherent to any change of the state/cookie wire format:

• It's one-directional and self-healing. Only a flow started on a new instance whose callback lands on an old instance fails (old code requires state == cookie), and only until old callback instances drain. The reverse (old-instance flow → new-instance callback) still succeeds: the old sealed value carries { nonce, codeVerifier, ... }, so getVerifierFromPKCECookieValue recovers the verifier and the shared-nonce check passes. Affected in-flight logins during the brief window just retry.
• The only way to avoid it is to keep the new callback accepting the old state == cookie format during rollout — which is exactly the self-satisfiable double-submit path this PR removes. Preserving it would re-open the account-takeover vector (SEC-1309) for the duration of every deploy, a worse trade than a short window of retryable 500s.

So I'm intentionally leaving this as-is. If zero login disruption during rollout is a hard requirement, the right lever is deploy-level (drain / blue-green, or a temporary dual-read window that is explicitly not the vulnerable equality check), which is out of scope for this security fix. Flagging for the human reviewer to decide.

[nicknisi]

Recovered this branch onto current main and added a regression for legacy callback-state rejection. Old-format in-flight sign-ins must restart after deployment. New sign-ins, custom state, and concurrent flows remain covered.

Verification: 174 tests passed, including coverage gates. Lint, typecheck, formatting, and build passed. A fresh independent code review found no blocker. Not merged or released.

[devin-ai-integration]

❌ Cannot revive Devin session - the session is too old. Please start a new session instead.

View session

[nicknisi]

Addressed the mixed-version rollout concern in b339f01 by documenting the deployment requirement in the README and PR description. The validation remains fail-closed.

Greptile is correct that sign-ins crossing versions can fail. The earlier Devin comment saying old-format callbacks still succeed is no longer accurate: the current implementation explicitly rejects URL state containing codeVerifier before schema parsing and code exchange. Both mixed-version directions fail. Accepting the legacy payload would restore the callback-replay vulnerability, so there is no compatibility fallback.

Use a coordinated cutover for sign-in and callback handlers, and restart interrupted sign-ins afterward. Existing authenticated sessions are unaffected.

Verification: all 174 tests pass, including legacy replay rejection and genuine concurrent callback coverage. git diff --check passes. This is a documented security migration requirement, not a reason to weaken callback validation.