Skip to content

oauth: derive the PKCE verifier instead of sealing it into the state - #880

Open
paucommit wants to merge 1 commit into
yc-software:mainfrom
paucommit:oauth-pkce-verifier-derivation
Open

oauth: derive the PKCE verifier instead of sealing it into the state#880
paucommit wants to merge 1 commit into
yc-software:mainfrom
paucommit:oauth-pkce-verifier-derivation

Conversation

@paucommit

@paucommit paucommit commented Aug 31, 2026

Copy link
Copy Markdown
Contributor

The problem

sealOAuthState mints a JWS — signed, not encrypted. The code_verifier was sealed inside it, so it travelled through the user-agent in readable base64url alongside the code it exists to protect. Anyone able to read a callback URL held both halves, and PKCE contributed nothing against authorization-code interception.

Two adjacent gaps in the same machinery:

  • The connector callback is auth: "public". Possession of a state was sufficient to finish the flow, so a state obtained from a leaked consent URL could bind a provider account to someone else's principal.
  • startClaudeLogin used the verifier as the authorize URL's state, so the code#state string the user copies out of the browser carried the code and its verifier side by side.

Mitigating context on the first: x is a confidential client, so the exchange still required the client secret. This restores PKCE's actual defence rather than closing an active breach.

What changed

Derive the verifier. HMAC(subkey, nonce) where subkey = HMAC(state_secret, "pkce.v1"). It never leaves the server; the callback recomputes it from state.nonce. The label matters: without it the verifier is an HMAC under the same key that signs OAuth state and portal sessions, and PKCE hands that value to the token endpoint by design. sealPkceState / pkceVerifierFor own both directions, so randomUUID, deriveCodeVerifier, codeChallengeS256 and sealOAuthState all drop out of the route file and "is this provider PKCE?" has one spelling instead of three.

nonce is now required on sealOAuthState. The optional ?? randomUUID() branch had no production callers and was a footgun: sealing a PKCE provider's state without a nonce derived a verifier no code_challenge was ever built from, failing at the provider with an opaque invalid_grant. Making it required turned that into a compile error.

Bind the callback. It now requires a portal identity for the same principal, rejects an impersonated identity on both the callback and oauthStart, and refuses a principal who is no longer active in the workspace. The check runs before the replay claim, so a rejected attempt cannot burn the legitimate user's state. Deployments with no browser-facing surface (PUBLIC_WEB_URL unset) are unaffected — pinned by a test.

This is possession-plus-identity, not full session binding: the identity is not bound to state.nonce, so it proves "a currently-valid identity for that principal" rather than "the browser that started this flow". Binding to the nonce via a short-lived SameSite=Lax cookie set by the surface is the natural next step and is deliberately not in this PR.

Separate the subscription login's state from its verifier. Independent randomness, returned from startClaudeLogin, held by the client alongside the verifier, and verified on completion. A pasted state that disagrees is rejected; a bare code still works when the expected state is known, and otherwise fails with a message naming the format instead of an opaque provider 400.

Sanitize branding before it reaches a style block. injectBranding interpolates accent and mark into <style> and a content= attribute. Accent is now hex-only, mark and label are stripped of quotes, angle brackets, braces and control characters, and every replacement uses a function replacer so $& in attacker text is inert. The portal sanitizes the accent where it stores it rather than at each read site.

Portal sign-in

Same derivation, under its own portal.pkce.v1 key beside the existing portal.session.v1 / portal.tmp.v1 / portal.impersonate.v1 labels. openTmp now rejects an empty state/nonce, which would otherwise have collapsed the derived verifier to a constant.

Deployment notes

  • In-flight PKCE flows fail for the state's 10-minute TTL across the rollout: states sealed by the old code carry a random verifier the new callback cannot reproduce. x is the only PKCE provider. Rollback is symmetric. The portal's portal_oidc_tmp cookie has the same one-TTL window.
  • The verifier is only as private as the OAuth state secret. Where OAUTH_STATE_SECRET is unset it falls back to CORE_SIGNING_SECRET, which every surface holds — still strictly better than plaintext in the URL, but setting it explicitly is worth documenting.
  • pkceVerifierFor decides whether to send a verifier from the current PROVIDERS table rather than from the sealed state, so flipping a provider's pkce flag breaks that provider's in-flight flows. The old design carried the decision in the state itself.

Testing

typecheck, lint and format:check clean. 105 core tests pass across oauth, oauth-routes, connector-invariants, oauth-consent-bridge, web-ui-connector-callback, subscription-oauth, portal-identity-gate, model-credential-route, user-model-auth-injection; portal plugin 110; auth plugin 53.

The regression guard is written to fail if the vulnerability is reinstated the way a natural revert would do it — it asserts against sealPkceState, the only function that mints PKCE states in production, rather than hand-building a state that never held a verifier. A companion test asserts the portal verifier differs from what the tmp-cookie MAC key would produce, so collapsing the key separation fails a test rather than passing silently.

No screenshots: nothing here changes a rendered surface. The branding change is a sanitizer on an existing style block, covered by unit tests asserting the escaped output.


View with [code]smith Autofix with [code]smith
Need help on this PR? Tag @codesmith-bot with what you need. Autofix is disabled.

…session

sealOAuthState mints a JWS: signed, not encrypted. The code_verifier was
sealed inside it, so it travelled through the user-agent in readable
base64url next to the code it exists to protect, and PKCE contributed
nothing against authorization-code interception.

The verifier is now derived as HMAC(subkey, nonce), where subkey is
HMAC(state secret, "pkce.v1"), so it never leaves the server and the
callback recomputes it from state.nonce. The label keeps it off the key
that signs state and sessions, so a value handed to the provider is not
also a valid MAC elsewhere. sealPkceState and pkceVerifierFor own both
directions, and nonce is now required on sealOAuthState: the optional
randomUUID() branch silently produced a verifier no code_challenge was
built from, failing later at the provider with an opaque invalid_grant.

The connector callback is a public route, so possession of a state was
enough to finish someone else's flow. It now requires a portal identity
for the same principal, rejects impersonation on both the callback and
oauthStart, and refuses a principal who is no longer active. Deployments
with no browser-facing surface are unaffected. Portal sign-in gets the
same derivation under its own portal.pkce.v1 key.

The Claude subscription login used the verifier as its state parameter,
so the pasted code#state string carried the code and the verifier
together. State is now independent randomness, threaded through the flow
and checked on completion.

Org branding reaches a style block through chassis, so accent, mark and
label are sanitized before interpolation, and the portal sanitizes the
accent where it stores it rather than at each read.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

2 participants