fix: resolve intermittent invalid_grant error in cluster-level Google OAuth #496
Conversation
…ration - Moved error handling to the beginning of the OAuth2 callback process for improved clarity and efficiency. - Introduced early detection of cluster-level OAuth flows, allowing for specific handling of Google OAuth callbacks. - Consolidated callback data creation to reduce redundancy and enhance readability. - Updated legacy session-specific OAuth flow handling to ensure compatibility with new cluster-level logic. This refactor enhances the overall structure and maintainability of the OAuth2 callback handling, aligning with recent improvements in session management and Google OAuth integration.
Claude Code ReviewSummaryThis PR successfully fixes the intermittent Issues by Severity🚫 Blocker IssuesNone - the PR is safe to merge. 🔴 Critical IssuesNone identified. 🟡 Major Issues1. Silent Error Handling for State Parsing (oauth.go:262-286) stateBytes, err := base64.URLEncoding.DecodeString(strings.Split(state, ".")[0])
if err == nil { // ⚠️ Silently ignores decode errors
if jsonErr := json.Unmarshal(stateBytes, &stateMap); jsonErr == nil { // ⚠️ Silently ignores unmarshal errors
// ...
}
}Issue: Errors during base64 decoding or JSON unmarshaling are silently ignored. This makes debugging difficult if the state format is corrupted or tampered with. Recommendation: Add logging for parsing failures: stateBytes, err := base64.URLEncoding.DecodeString(strings.Split(state, ".")[0])
if err != nil {
log.Printf("Warning: failed to decode state parameter: %v (continuing with legacy flow)", err)
} else {
if jsonErr := json.Unmarshal(stateBytes, &stateMap); jsonErr != nil {
log.Printf("Warning: failed to parse state JSON: %v (continuing with legacy flow)", jsonErr)
} else {
// Check for cluster-level OAuth...
}
}2. Missing Bounds Check on strings.Split (oauth.go:262) strings.Split(state, ".")[0] // ⚠️ Assumes at least 1 elementIssue: If Recommendation: Add explicit check or comment explaining why this is safe: stateParts := strings.Split(state, ".")
if len(stateParts) > 0 {
stateBytes, err := base64.URLEncoding.DecodeString(stateParts[0])
// ...
}🔵 Minor Issues1. Missing Unit Tests Recommendation: Add tests for:
2. Code Duplication in HTML Responses (oauth.go:273-282) func renderOAuthSuccess(c *gin.Context) {
c.Data(http.StatusOK, "text/html; charset=utf-8", []byte(
"<html><body><h1>Authorization Successful!</h1>...",
))
}3. Error Response Uses StatusOK Instead of Error Code (oauth.go:273) c.Data(http.StatusOK, "text/html; charset=utf-8", []byte(
"<html><body><h1>Authorization Error</h1>...",
))Issue: Error responses return HTTP 200 OK. While this may be intentional for user-facing OAuth flows (to render the error page), it's worth documenting why. Recommendation: Add a comment explaining this is intentional for user experience, or consider using 4xx/5xx status codes if the frontend can handle them gracefully. Positive Highlights✅ Excellent Problem Diagnosis: The PR description clearly explains the root cause (double code exchange) and solution. ✅ Correct Fix Strategy: Moving cluster-level detection before token exchange is the right architectural approach. ✅ Clear Code Comments: The IMPORTANT comment on line 259-260 explains why the order matters, which is invaluable for future maintainers. ✅ Error Handling: The fix properly handles errors from ✅ Backward Compatibility: The legacy session-specific OAuth flow remains intact, ensuring no breaking changes. ✅ Security: No tokens are logged, following established security patterns. RecommendationsHigh Priority
Medium Priority
Low Priority
Architecture Alignment✅ Follows Error Handling Patterns: Errors are logged with context, generic messages returned to users Final AssessmentStatus: ✅ APPROVED - Safe to Merge This is a well-executed bug fix that solves a real production issue. The refactoring improves code organization and the fix is architecturally sound. The main gaps are in observability (silent error handling) and test coverage, which can be addressed in follow-up work without blocking this PR. The fix prevents a critical OAuth failure and should be merged to resolve the intermittent 🔍 View AI decision process (logs available for 90 days) 📋 View memory system files loaded (click to expand)What Amber Loaded for Code ReviewAmber automatically loaded these repository standards from the memory system:
Impact: This review used your repository's specific code quality standards, security patterns, and best practices from the memory system (PRs #359, #360) - not just generic code review guidelines. |
1 participant
Summary
Fixes intermittent
invalid_granterrors when connecting Google Drive via the integrations page.Problem
The OAuth2 callback handler was exchanging the authorization code twice for cluster-level OAuth flows:
HandleOAuth2Callbackhandler (line 282)HandleGoogleOAuthCallbackwhich tried to exchange the same code againSince OAuth authorization codes are single-use, the second exchange failed with
invalid_grant.Solution
Moved the cluster-level OAuth detection before any token exchange:
HandleGoogleOAuthCallback(which does its own exchange) and returnThis ensures the authorization code is only exchanged once.
Testing