fix(google-oauth): add PKCE support to fix missing code_verifier error#1481
Merged
mergify[bot] merged 1 commit intomainfrom Apr 29, 2026
Merged
Conversation
…ing code_verifier error workspace-mcp generates Google OAuth URLs with PKCE (code_challenge + code_challenge_method=S256), but the backend's /oauth2callback handler was calling exchangeOAuthCode without the code_verifier, causing Google to reject every token exchange with: invalid_grant: Missing code verifier. Fix: - Generate a cryptographically random code_verifier in GetGoogleOAuthURLGlobal and GetOAuthURL (session-specific flow) - Store the verifier in a K8s Secret (oauth-pkce-verifiers) keyed by SHA256(state) so keys are always valid K8s secret data keys regardless of base64 padding characters in the state token - Retrieve and delete the verifier (one-time use) at the start of HandleOAuth2Callback, then pass it through both the cluster-level and legacy session-specific token exchange paths - Update exchangeOAuthCode to accept an optional codeVerifier parameter and include it as code_verifier in the POST body when non-empty - Retrieval is non-fatal: unknown states (e.g. from third-party MCP tools) return "" and fall through to exchange without PKCE, preserving backwards compatibility Also add 39 unit tests covering all new functions: generateCodeVerifier, generateCodeChallenge, pkceSecretKey, storePKCEVerifier, retrievePKCEVerifier, exchangeOAuthCode, storeGoogleCredentials, GetGoogleCredentials, GetGoogleOAuthURLGlobal, GetGoogleOAuthStatusGlobal, DisconnectGoogleOAuthGlobal, and the full PKCE lifecycle. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
✅ Deploy Preview for cheerful-kitten-f556a0 canceled.
|
Contributor
📝 WalkthroughWalkthroughAdds PKCE (Proof Key for Code Exchange) support to the Google OAuth flow by generating and storing verifier/challenge pairs, persisting them in Kubernetes Secrets, and including the verifier during token exchange. Function signatures for OAuth callback and token exchange handlers are updated to accept the code verifier parameter. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Client as Client<br/>(Web App)
participant Handler as Backend<br/>Handler
participant K8s as Kubernetes<br/>Secret
participant Google as Google OAuth<br/>Provider
Client->>Handler: Request OAuth URL
Handler->>Handler: Generate PKCE verifier<br/>& challenge
Handler->>K8s: Store verifier<br/>(keyed by state)
Handler->>Client: Return URL with<br/>code_challenge
Client->>Google: Redirect with<br/>code_challenge
User->>Google: Authenticate
Google->>Client: Redirect with code
Client->>Handler: Callback with code
Handler->>K8s: Retrieve verifier
Handler->>Google: Exchange code<br/>+ verifier
Google->>Handler: Return token
Handler->>K8s: Remove verifier
Handler->>Client: Auth complete
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 1 warning)
✅ Passed checks (6 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
✨ Simplify code
Review rate limit: 9/10 reviews remaining, refill in 6 minutes. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
components/backend/handlers/oauth.go (1)
284-302:⚠️ Potential issue | 🔴 CriticalValidate
statebefore routing the cluster callback.Lines 294-302 still branch on JSON decoded from
statewithout verifying the HMAC or timestamp first. With the new PKCE flow, Lines 285-289 also consume the stored verifier before that validation happens. A forged or replayed callback can therefore burn a real verifier, and the cluster path can exchange/store tokens for an attacker-controlleduserID.Please move state verification ahead of both
retrievePKCEVerifier(...)and theclusterdispatch, and make the cluster path consume only validated state.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/backend/handlers/oauth.go` around lines 284 - 302, The state must be validated (verify HMAC and timestamp) before parsing/dispatching or consuming PKCE verifiers; move the call to retrievePKCEVerifier(c.Request.Context(), state) and any single-use consumption until after you validate the state blob, and only decode/unmarshal state into stateMap after validation succeeds; then, when branching to the cluster path (the code that calls HandleGoogleOAuthCallback), pass the validated stateMap and consume the codeVerifier there (or retrieve it just before exchange) so a forged or replayed callback cannot burn a real verifier or allow attacker-controlled userID storage.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@components/backend/handlers/oauth.go`:
- Around line 285-289: The current code in the oauth handler treats any non-nil
error from retrievePKCEVerifier as “not found” and falls back to
codeVerifier="", hiding real failures; change the logic in the block handling
retrievePKCEVerifier(c.Request.Context(), state) so that you only fall back to
an empty codeVerifier when the function returns the explicit “not found”
sentinel/zero-error case (the same value retrievePKCEVerifier uses to indicate
unknown state), but for all other non-nil errors (e.g., K8s read/update
failures) log the error with context and return/abort the request (e.g.,
500/internal error) instead of proceeding with an empty verifier; reference
retrievePKCEVerifier and the codeVerifier variable to locate and update the
branch.
---
Outside diff comments:
In `@components/backend/handlers/oauth.go`:
- Around line 284-302: The state must be validated (verify HMAC and timestamp)
before parsing/dispatching or consuming PKCE verifiers; move the call to
retrievePKCEVerifier(c.Request.Context(), state) and any single-use consumption
until after you validate the state blob, and only decode/unmarshal state into
stateMap after validation succeeds; then, when branching to the cluster path
(the code that calls HandleGoogleOAuthCallback), pass the validated stateMap and
consume the codeVerifier there (or retrieve it just before exchange) so a forged
or replayed callback cannot burn a real verifier or allow attacker-controlled
userID storage.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 9876fdb9-ff3f-46d2-9c70-0549b48f4f82
📒 Files selected for processing (3)
components/backend/handlers/oauth.gocomponents/backend/handlers/oauth_test.gocomponents/backend/tests/constants/labels.go
Comment on lines
+285
to
+289
| codeVerifier, verifierErr := retrievePKCEVerifier(c.Request.Context(), state) | ||
| if verifierErr != nil { | ||
| log.Printf("Warning: could not retrieve PKCE verifier for state: %v", verifierErr) | ||
| codeVerifier = "" | ||
| } |
Contributor
There was a problem hiding this comment.
Don't downgrade PKCE storage failures into a non-PKCE exchange.
retrievePKCEVerifier already returns ("", nil) when the state is unknown. Lines 286-289 treat all other errors the same way, so a real K8s read/update failure gets turned into codeVerifier="", which then fails later as invalid_grant and hides the actual outage.
Only fall back on the explicit “not found” case; surface real retrieval errors to the client/server logs as a backend failure.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@components/backend/handlers/oauth.go` around lines 285 - 289, The current
code in the oauth handler treats any non-nil error from retrievePKCEVerifier as
“not found” and falls back to codeVerifier="", hiding real failures; change the
logic in the block handling retrievePKCEVerifier(c.Request.Context(), state) so
that you only fall back to an empty codeVerifier when the function returns the
explicit “not found” sentinel/zero-error case (the same value
retrievePKCEVerifier uses to indicate unknown state), but for all other non-nil
errors (e.g., K8s read/update failures) log the error with context and
return/abort the request (e.g., 500/internal error) instead of proceeding with
an empty verifier; reference retrievePKCEVerifier and the codeVerifier variable
to locate and update the branch.
Contributor
Merge Queue Status
This pull request spent 15 seconds in the queue, including 2 seconds running CI. Required conditions to merge |
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
1 participant
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.
Summary
workspace-mcpgenerates Google OAuth URLs with PKCE (code_challenge+code_challenge_method=S256), but the backend's/oauth2callbackhandler was callingexchangeOAuthCodewithout the correspondingcode_verifier, causing every token exchange to fail withinvalid_grant: Missing code verifierGetGoogleOAuthURLGlobalandGetOAuthURL: generate a randomcode_verifier, compute the S256code_challenge, store the verifier in a K8s Secret (oauth-pkce-verifiers) keyed bySHA256(state), and include the challenge in the auth URLHandleOAuth2Callback, then thread it through both the cluster-level (HandleGoogleOAuthCallback) and legacy session-specific exchange pathsexchangeOAuthCodeto accept an optionalcodeVerifierparam and appendcode_verifierto the POST body when non-empty; retrieval is non-fatal so unknown states (e.g. from third-party tools) still fall through without PKCERoot cause
The auth URL included
code_challenge(PKCE-initiated), so Google requiredcode_verifierduring token exchange. The backend was omitting it entirely.Test plan
handlers/oauth_test.gocovering all new functions:generateCodeVerifier,generateCodeChallenge,pkceSecretKey,storePKCEVerifier,retrievePKCEVerifier,exchangeOAuthCode,storeGoogleCredentials,GetGoogleCredentials,GetGoogleOAuthURLGlobal,GetGoogleOAuthStatusGlobal,DisconnectGoogleOAuthGlobal, and full PKCE verifier lifecyclego test -tags test -run TestBackend ./handlers/ --ginkgo.label-filter="google-auth"— all 39 passgo test -tags test -run TestBackend ./handlers/)🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
New Features
Tests