oauth: derive the PKCE verifier instead of sealing it into the state - #880
Open
paucommit wants to merge 1 commit into
Open
oauth: derive the PKCE verifier instead of sealing it into the state#880paucommit wants to merge 1 commit into
paucommit wants to merge 1 commit into
Conversation
…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.
iqbalbhatti49
approved these changes
Sep 1, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The problem
sealOAuthStatemints a JWS — signed, not encrypted. Thecode_verifierwas sealed inside it, so it travelled through the user-agent in readable base64url alongside thecodeit 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:
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.startClaudeLoginused the verifier as the authorize URL'sstate, so thecode#statestring the user copies out of the browser carried the code and its verifier side by side.Mitigating context on the first:
xis 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)wheresubkey = HMAC(state_secret, "pkce.v1"). It never leaves the server; the callback recomputes it fromstate.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/pkceVerifierForown both directions, sorandomUUID,deriveCodeVerifier,codeChallengeS256andsealOAuthStateall drop out of the route file and "is this provider PKCE?" has one spelling instead of three.nonceis now required onsealOAuthState. The optional?? randomUUID()branch had no production callers and was a footgun: sealing a PKCE provider's state without a nonce derived a verifier nocode_challengewas ever built from, failing at the provider with an opaqueinvalid_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_URLunset) 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-livedSameSite=Laxcookie 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.
injectBrandinginterpolatesaccentandmarkinto<style>and acontent=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.v1key beside the existingportal.session.v1/portal.tmp.v1/portal.impersonate.v1labels.openTmpnow rejects an emptystate/nonce, which would otherwise have collapsed the derived verifier to a constant.Deployment notes
xis the only PKCE provider. Rollback is symmetric. The portal'sportal_oidc_tmpcookie has the same one-TTL window.OAUTH_STATE_SECRETis unset it falls back toCORE_SIGNING_SECRET, which every surface holds — still strictly better than plaintext in the URL, but setting it explicitly is worth documenting.pkceVerifierFordecides whether to send a verifier from the currentPROVIDERStable rather than from the sealed state, so flipping a provider'spkceflag breaks that provider's in-flight flows. The old design carried the decision in the state itself.Testing
typecheck,lintandformat:checkclean. 105 core tests pass acrossoauth,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.
Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.