OAuth2 Authentication System#1711
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughSelf-contained in-memory Go OAuth2 server and client: /authorize and /token endpoints, PKCE support, authorization-code issuance/consumption, refresh-token rotation, token validation/revocation, HTTP wiring, and demo main. ChangesOAuth2 Server and Client Implementation
Sequence Diagram(s)sequenceDiagram
participant Client as OAuth2Client
participant Server as OAuth2Server
participant Store as InMemoryStore
Client->>Server: GET /authorize (client_id, scope, code_challenge, ...)
Server->>Store: GenerateAuthCode (store code with client,user,PKCE)
Server-->>Client: Redirect with authorization_code or error
Client->>Server: POST /token (grant_type=authorization_code, code, code_verifier)
Server->>Store: consume auth code (delete) then validate (expiry, client, redirect, PKCE)
Server->>Store: generate and store access_token + refresh_token
Server-->>Client: JSON token response
Client->>Server: POST /token (grant_type=refresh_token, refresh_token)
Server->>Store: validate and rotate refresh token, store new tokens
Server-->>Client: JSON rotated tokens
Client->>Server: GET /protected (Authorization: Bearer access_token)
Server-->>Client: 200 OK protected resource
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies" 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0ecdd7a6-0d02-4db0-90e9-2390ffeb2aed
📒 Files selected for processing (1)
challenge-15/submissions/PopovMarko/solution-template.go
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 92fc4896-a11d-4e48-9e24-73c5ff051421
📒 Files selected for processing (1)
challenge-15/submissions/PopovMarko/solution-template.go
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
challenge-15/submissions/PopovMarko/solution-template.go (1)
558-560:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winMissing
returnafter client-ID mismatch error.Line 559 writes an error response but execution continues. If the client secret happens to match, tokens are issued despite the client-ID mismatch—a token leakage / authorization bypass.
Proposed fix
if dto.clientID != client.ClientID { tokenErrorResponse(w, "client not found", ErrInvalidClient) + return }
🧹 Nitpick comments (1)
challenge-15/submissions/PopovMarko/solution-template.go (1)
672-672: 💤 Low valueRemove stale TODO comment.
The PKCE verification is fully implemented below, but the TODO suggests it isn't.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: aac49e61-15b9-4d6d-b4ed-6bd44e45499e
📒 Files selected for processing (1)
challenge-15/submissions/PopovMarko/solution-template.go
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
challenge-15/submissions/PopovMarko/solution-template.go (1)
281-288:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winUse
uri.Query()instead ofr.URL.Query()to preserve redirect URI params.Line 283 reads query parameters from the incoming authorization request (
r.URL.Query()), which copies all the authorize endpoint's params (client_id, redirect_uri, scope, code_challenge, etc.) onto the callback URL. This violates the OAuth2 spec and loses any query params already present on the registered redirect URI.The callback should only contain the OAuth2 response params (code/error and state) plus any params originally on the registered redirect URI (e.g.,
?tenant=acme).🔧 Proposed fix
func authResponse(w http.ResponseWriter, r *http.Request, redirectUri string, response map[string]string) { uri, _ := url.Parse(redirectUri) - val := r.URL.Query() + val := uri.Query() for k, v := range response { val.Set(k, v) } uri.RawQuery = val.Encode() http.Redirect(w, r, uri.String(), http.StatusFound) }
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3a524ed3-93dc-47a9-95a3-59a4a4f14d69
📒 Files selected for processing (1)
challenge-15/submissions/PopovMarko/solution-template.go
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
challenge-15/submissions/PopovMarko/solution-template.go (2)
299-302:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winReject unauthenticated
/authorizerequests instead of defaulting touser1.Any request that arrives without an authenticated user context currently gets an auth code for the preloaded
user1, which means a third-party app can mint tokens without a real login step.Suggested fix
func getAuthRequestParams(r *http.Request) AuthRequestDTO { query := r.URL.Query() userID, _ := r.Context().Value("user_id").(string) - if userID == "" { - userID = "user1" - } method := query.Get("code_challenge_method") if method == "" { method = "plain" @@ func (s *OAuth2Server) validAuthRequestParams(dto AuthRequestDTO) error { s.mu.RLock() defer s.mu.RUnlock() + if dto.userID == "" { + return fmt.Errorf("user not authenticated: %w", ErrInvalidRequest) + } if _, exists := s.users[dto.userID]; !exists { return fmt.Errorf("user not found %w", ErrInvalidRequest) }Also applies to: 323-328
610-620:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReturn copies instead of the server’s live token records.
Both exported methods hand out the exact structs stored in the in-memory maps. Callers can mutate
ExpiresAt,ClientID, orScopesand change the server’s authorization state from outside the mutex.Suggested fix
+func cloneToken(t *Token) *Token { + if t == nil { + return nil + } + cp := *t + cp.Scopes = append([]string(nil), t.Scopes...) + return &cp +} + +func cloneRefreshToken(t *RefreshToken) *RefreshToken { + if t == nil { + return nil + } + cp := *t + cp.Scopes = append([]string(nil), t.Scopes...) + return &cp +} + func (s *OAuth2Server) ValidateToken(token string) (*Token, error) { s.mu.RLock() accToken, ok := s.tokens[token] - s.mu.RUnlock() if !ok { + s.mu.RUnlock() return nil, fmt.Errorf("token not found: %w", ErrInvalidRequest) } if accToken.ExpiresAt.Before(time.Now()) { + s.mu.RUnlock() return nil, fmt.Errorf("token expired: %w", ErrInvalidGrant) } - return accToken, nil + out := cloneToken(accToken) + s.mu.RUnlock() + return out, nil } @@ - return accToken, newRefToken, nil + return cloneToken(accToken), cloneRefreshToken(newRefToken), nil }Also applies to: 626-648
♻️ Duplicate comments (1)
challenge-15/submissions/PopovMarko/solution-template.go (1)
626-646:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winMake
RefreshAccessTokenrotation atomic too.This helper still does lookup/validation under
RLockand only deletes the old refresh token later underLock, so two concurrent callers can both rotate the same single-use refresh token.Suggested fix
func (s *OAuth2Server) RefreshAccessToken(refreshToken string) (*Token, *RefreshToken, error) { - s.mu.RLock() + s.mu.Lock() + defer s.mu.Unlock() + refToken, ok := s.refreshTokens[refreshToken] - s.mu.RUnlock() if !ok { return nil, nil, fmt.Errorf("refresh token not found: %w", ErrInvalidGrant) } if refToken.ExpiresAt.Before(time.Now()) { return nil, nil, fmt.Errorf("refresh token expired: %w", ErrInvalidGrant) @@ - s.mu.Lock() delete(s.refreshTokens, refreshToken) s.tokens[accToken.AccessToken] = accToken s.refreshTokens[newRefToken.RefreshToken] = newRefToken - s.mu.Unlock() return accToken, newRefToken, nil }
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 40d99f00-52f3-45c9-af2c-ad4eba74edbd
📒 Files selected for processing (1)
challenge-15/submissions/PopovMarko/solution-template.go
|
@coderabbitai resume |
✅ Actions performedReviews resumed. |
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e22458c1-aa36-43d0-8714-73fdfc1c5b6b
📒 Files selected for processing (1)
challenge-15/submissions/PopovMarko/solution-template.go
1 participant
Implemeented Server that supports OAuth authorization code flow, allowing third party application to authenticate users