refactor: oauth flow#726
Merged
Merged
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughCentralizes OAuth into a session-based flow: adds an OAuth session cookie name, consolidates broker initialization, replaces per-provider implementations with a unified OAuthService + presets/extractors, adds pending-session state and cleanup to AuthService, renames two background routines, and updates controller and bootstrap wiring. Changes
Sequence DiagramsequenceDiagram
actor Client
participant Controller as OAuth Controller
participant Auth as AuthService
participant Broker as OAuthBrokerService
participant Provider as OAuth Provider
rect rgba(100,150,200,0.5)
Note over Client,Provider: Authorization request flow
Client->>Controller: GET /oauth/authorize?provider=github
Controller->>Auth: NewOAuthSession(provider)
Auth->>Broker: GetService(provider) / NewRandom()
Broker-->>Auth: service, state+verifier
Auth-->>Controller: sessionId
Controller->>Client: Set OAuth session cookie, Redirect to Provider (via Auth->Broker->GetAuthURL)
end
rect rgba(200,150,100,0.5)
Note over Client,Auth: Callback & token exchange
Provider-->>Client: Redirect /oauth/callback?code=...
Client->>Controller: GET /oauth/callback?code=...
Controller->>Controller: Read session cookie
Controller->>Auth: GetOAuthToken(sessionId, code)
Auth->>Broker: GetService(...).GetToken(code, verifier)
Broker->>Provider: Exchange code -> token
Broker-->>Auth: token
Controller->>Auth: GetOAuthUserinfo(sessionId)
Auth->>Broker: GetService(...).GetUserinfo(token)
Broker-->>Auth: claims
Controller->>Auth: EndOAuthSession(sessionId)
Controller-->>Client: Create app session + Redirect
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested Labels
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment Tip You can customize the high-level summary generated by CodeRabbit.Configure the |
Rycochet
reviewed
Mar 21, 2026
Rycochet
reviewed
Mar 21, 2026
![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: 4
🧹 Nitpick comments (1)
internal/service/oauth_extractors.go (1)
29-67: Unusedurlparameter ingithubExtractor.The
urlparameter is ignored since GitHub API endpoints are hardcoded. This is correct behavior for GitHub (which doesn't use a standard userinfo endpoint), but consider documenting this or using_to make the intent explicit.🔧 Optional: Make the unused parameter explicit
-func githubExtractor(client *http.Client, url string) (config.Claims, error) { +func githubExtractor(client *http.Client, _ string) (config.Claims, error) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/service/oauth_extractors.go` around lines 29 - 67, The function githubExtractor currently has an unused parameter url; to make intent explicit and silence linters, rename the parameter to an anonymous identifier or a clearly ignored name (e.g., replace the named parameter `url string` with `_ string` or `_url string`) in the githubExtractor function signature, or add a one-line comment above the function noting that GitHub uses hardcoded endpoints and the url parameter is intentionally unused; update only the githubExtractor signature/comment so callers and linters understand the parameter is intentionally ignored.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/controller/oauth_controller.go`:
- Line 76: The debug logging currently emits sensitive OAuth secrets
(session.State, session.Verifier and the callback authorization code) which can
be used to replay exchanges; update the logging in the OAuth session creation
and callback handlers (refer to the tlog.App.Debug() call that logs "Created new
OAuth session" and the callback log around line 149) to sanitize or omit secrets
before logging — e.g., build a sanitized copy of the session object that removes
or masks State and Verifier, and ensure the authorization code is never included
in any log message, logging only non-secret fields (or a redacted placeholder)
instead.
- Around line 158-160: Check the error returned by
controller.auth.GetOAuthUserinfo before accessing claims like user.Email: if err
!= nil handle/return the error (or log and abort the flow) instead of
proceeding, so functions like GetOAuthUserinfo do not lead to treating an error
as "missing email"; update the oauth handling code around GetOAuthUserinfo and
the subsequent user.Email check to first verify err and only read claims when
err is nil.
In `@internal/service/auth_service.go`:
- Around line 672-675: The cleanup goroutine is deadlocking because it uses
defer auth.oauthMutex.Unlock() inside the for range ticker.C loop so the mutex
stays locked after the first tick; change the loop to call
auth.oauthMutex.Lock() at the start of each iteration and then call
auth.oauthMutex.Unlock() explicitly at the end of that iteration (after the
cleanup work) instead of deferring, ensuring the lock is released before the
next tick and allowing OAuth session creation/callbacks/EndOAuthSession to
proceed; locate the auth.oauthMutex usage in the cleanup loop in auth_service.go
and remove the defer, placing a direct Unlock() after the cleanup steps.
In `@internal/service/oauth_service.go`:
- Around line 72-74: In OAuthService.GetToken remove the sensitive logging of
the authorization code and PKCE verifier: locate the tlog.App.Debug() call in
the GetToken method and either delete it or replace it with a non-secret message
(e.g., "Exchanging authorization code for token" or include only non-sensitive
context like request ID), ensuring you do NOT log or format the raw code or
verifier anywhere; keep the s.config.Exchange(s.ctx, code,
oauth2.VerifierOption(verifier)) call as-is.
---
Nitpick comments:
In `@internal/service/oauth_extractors.go`:
- Around line 29-67: The function githubExtractor currently has an unused
parameter url; to make intent explicit and silence linters, rename the parameter
to an anonymous identifier or a clearly ignored name (e.g., replace the named
parameter `url string` with `_ string` or `_url string`) in the githubExtractor
function signature, or add a one-line comment above the function noting that
GitHub uses hardcoded endpoints and the url parameter is intentionally unused;
update only the githubExtractor signature/comment so callers and linters
understand the parameter is intentionally ignored.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 11896ac4-9bf8-471d-a5f1-378a46df4f27
📒 Files selected for processing (15)
internal/bootstrap/app_bootstrap.gointernal/bootstrap/router_bootstrap.gointernal/bootstrap/service_bootstrap.gointernal/config/config.gointernal/controller/oauth_controller.gointernal/controller/proxy_controller_test.gointernal/controller/user_controller_test.gointernal/service/auth_service.gointernal/service/generic_oauth_service.gointernal/service/github_oauth_service.gointernal/service/google_oauth_service.gointernal/service/oauth_broker_service.gointernal/service/oauth_extractors.gointernal/service/oauth_presets.gointernal/service/oauth_service.go
💤 Files with no reviewable changes (3)
- internal/service/google_oauth_service.go
- internal/service/generic_oauth_service.go
- internal/service/github_oauth_service.go
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #726 +/- ##
==========================================
+ Coverage 16.85% 16.92% +0.06%
==========================================
Files 50 50
Lines 3820 3806 -14
==========================================
Hits 644 644
+ Misses 3112 3098 -14
Partials 64 64 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
internal/controller/oauth_controller.go (1)
154-160:⚠️ Potential issue | 🟠 MajorCheck
GetOAuthUserinfobefore reading claims.Right now provider/network failures get flattened into “did not return an email”, and any extractor that returns partial claims on error could still let the flow continue with a bad state.
🩹 Minimal fix
user, err := controller.auth.GetOAuthUserinfo(sessionIdCookie) + + if err != nil { + tlog.App.Error().Err(err).Msg("Failed to get OAuth userinfo") + c.Redirect(http.StatusTemporaryRedirect, fmt.Sprintf("%s/error", controller.config.AppURL)) + return + } if user.Email == "" {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/controller/oauth_controller.go` around lines 154 - 160, GetOAuthUserinfo errors are not checked before reading claims, which flattens provider/network failures into "did not return an email"; update the handler to first check err returned by controller.auth.GetOAuthUserinfo(sessionIdCookie) and if err != nil log the error via tlog.App.Error().Err(err).Msg(...) and redirect to fmt.Sprintf("%s/error", controller.config.AppURL) (same flow as missing email), returning early; only after err is nil should you inspect user.Email and handle the empty-email case as currently done (references: controller.auth.GetOAuthUserinfo, user.Email, tlog.App.Error(), c.Redirect, controller.config.AppURL).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/controller/oauth_controller.go`:
- Around line 208-215: The code is persisting the provider from the incoming
route input (req.Provider) instead of the provider resolved earlier from the
pending OAuth session (the sessionIdCookie used during callback resolution),
which allows clients to spoof the provider; update the code that builds the
repository.Session (sessionCookie) to read the provider ID from the stored
pending OAuth session (the value retrieved when resolving
sessionIdCookie/pending OAuth session) and use that value for OAuthName/Provider
fields instead of req.Provider so the persisted session/audit reflects the
actual exchanged token's provider.
- Around line 123-131: After successfully reading sessionIdCookie (the result of
c.Cookie(controller.config.OAuthSessionCookieName)), immediately call defer
controller.auth.EndOAuthSession(sessionIdCookie) so the server-side OAuth
session (State/Verifier/token) is removed on every exit path; keep the existing
c.SetCookie(...) removal of the browser cookie but remove the duplicate
success-only cleanup currently done in the success path (the code that calls
controller.auth.EndOAuthSession(sessionIdCookie) at the end of the happy path)
to avoid double-handling.
In `@internal/service/auth_service.go`:
- Around line 61-62: The oauthPendingSessions map currently grows unbounded; add
a bounded eviction strategy by introducing a maxPendingSessions constant and
managing entries with an LRU or FIFO eviction structure (e.g., a linked list or
github.com/hashicorp/golang-lru) tied to oauthPendingSessions and protected by
oauthMutex; on Insert (the handler that creates /oauth/url/:provider entries)
check len(oauthPendingSessions) and evict oldest entries before inserting, or
refuse new inserts when the cap per client/IP is exceeded (track client key in
the session struct), and ensure all access points that read/write
oauthPendingSessions use oauthMutex consistently (including the code paths
referenced around the oauthPendingSessions declaration and the handlers at the
other noted locations).
---
Duplicate comments:
In `@internal/controller/oauth_controller.go`:
- Around line 154-160: GetOAuthUserinfo errors are not checked before reading
claims, which flattens provider/network failures into "did not return an email";
update the handler to first check err returned by
controller.auth.GetOAuthUserinfo(sessionIdCookie) and if err != nil log the
error via tlog.App.Error().Err(err).Msg(...) and redirect to
fmt.Sprintf("%s/error", controller.config.AppURL) (same flow as missing email),
returning early; only after err is nil should you inspect user.Email and handle
the empty-email case as currently done (references:
controller.auth.GetOAuthUserinfo, user.Email, tlog.App.Error(), c.Redirect,
controller.config.AppURL).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 734fbd39-03f2-4a73-b87b-0429616f7064
📒 Files selected for processing (3)
internal/controller/oauth_controller.gointernal/service/auth_service.gointernal/service/oauth_service.go
Rycochet
approved these changes
Mar 22, 2026
This was referenced Mar 27, 2026
Merged
This was referenced May 3, 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
2 participants
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 by CodeRabbit
Refactor
New Features
Bug Fixes