Integrate phenotype-go-kit for auth token storage#866
Conversation
…Copilot, Gemini) Replace duplicated token storage implementations across Claude, Copilot, and Gemini auth providers with a shared BaseTokenStorage from phenotype-go-kit. Changes: - Add phenotype-go-kit as a dependency with local path replace directive - Update Claude token storage to embed and use BaseTokenStorage - Update Copilot token storage to embed and use BaseTokenStorage - Update Gemini token storage to embed and use BaseTokenStorage - Implement provider-specific constructor functions for each auth provider - Update auth bundle conversions to use new constructors - Maintain backward compatibility with SaveTokenToFile interface This reduces code duplication across auth implementations while preserving provider-specific customizations and maintaining the existing API surface.
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly upgrades the application's authentication infrastructure by incorporating a new Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
📝 WalkthroughWalkthroughThis PR introduces OAuth2 authentication implementations for three AI providers: Claude (via Anthropic OAuth2-PKCE), GitHub Copilot (via device flow), and Google Gemini (via interactive web-based OAuth2). It adds token storage mechanisms for each provider and includes a local module dependency on Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request integrates phenotype-go-kit for authentication token storage and adds authentication flows for Claude, Copilot, and Gemini. The changes are extensive and introduce new packages for each provider. My review has identified a critical security vulnerability due to a hardcoded client secret in the Gemini authentication flow. Additionally, there are several areas for improvement regarding maintainability, consistency, and correctness, such as brittle field copying for token storage, inconsistent error handling, and an incorrect backoff implementation. I've also noted a potential issue with the local dependency configuration in go.mod that could affect CI/CD and team collaboration.
| // OAuth configuration constants for Gemini | ||
| const ( | ||
| ClientID = "681255809395-oo8ft2oprdrnp9e3aqf6av3hmdib135j.apps.googleusercontent.com" | ||
| ClientSecret = "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" |
There was a problem hiding this comment.
A hardcoded ClientSecret has been found. Client secrets are sensitive credentials and should never be hardcoded in source code. This poses a significant security risk, as anyone with access to the source code can compromise the secret. Please remove the hardcoded secret and load it from a secure source, such as environment variables or a secret management system.
| modernc.org/memory v1.11.0 // indirect | ||
| ) | ||
|
|
||
| replace github.com/KooshaPari/phenotype-go-kit => ../../template-commons/phenotype-go-kit |
There was a problem hiding this comment.
The replace directive with a local file path (../../template-commons/phenotype-go-kit) makes the build process dependent on a specific local directory structure. This will cause build failures for other developers and in CI/CD environments. This should be replaced with a versioned dependency from a repository before this change is merged into a shared branch. If phenotype-go-kit is under development, consider using a specific commit hash in go.mod or pushing it to a temporary branch.
| defer func() { | ||
| _ = resp.Body.Close() | ||
| }() |
There was a problem hiding this comment.
The error from resp.Body.Close() is ignored here. This is inconsistent with other parts of the codebase (e.g., in ExchangeCodeForTokens) where this error is checked and logged. It's good practice to always check and handle errors from Close() on response bodies to detect potential issues.
| defer func() { | |
| _ = resp.Body.Close() | |
| }() | |
| defer func() { | |
| if errClose := resp.Body.Close(); errClose != nil { | |
| log.Errorf("failed to close refresh response body: %v", errClose) | |
| } | |
| }() |
| select { | ||
| case <-ctx.Done(): | ||
| return nil, ctx.Err() | ||
| case <-time.After(time.Duration(attempt) * time.Second): |
There was a problem hiding this comment.
The retry logic uses a linear backoff (time.Duration(attempt) * time.Second), but the function comment for RefreshTokensWithRetry at line 298 states it implements "exponential backoff". This is misleading. For better resilience against transient server issues, you should implement a proper exponential backoff.
| case <-time.After(time.Duration(attempt) * time.Second): | |
| case <-time.After(time.Duration(1<<attempt) * time.Second): |
| // Create a new token storage with the file path and copy the fields | ||
| base := auth.NewBaseTokenStorage(authFilePath) | ||
| base.IDToken = ts.IDToken | ||
| base.AccessToken = ts.AccessToken | ||
| base.RefreshToken = ts.RefreshToken | ||
| base.LastRefresh = ts.LastRefresh | ||
| base.Email = ts.Email | ||
| base.Type = ts.Type | ||
| base.Expire = ts.Expire | ||
| base.SetMetadata(ts.Metadata) | ||
|
|
||
| return base.Save() |
There was a problem hiding this comment.
The manual field-by-field copy from ts.BaseTokenStorage to a new base instance is brittle. If new fields are added to auth.BaseTokenStorage in the future, they will be missed here, leading to data loss on save. This pattern is repeated across copilot/token.go and gemini/gemini_token.go. Consider refactoring this to be more robust. A better approach would be to have a method on BaseTokenStorage to update its file path, or to use a struct copy if the dependency's API allows for it.
| // Create a new token storage with the file path and copy the fields | ||
| base := auth.NewBaseTokenStorage(authFilePath) | ||
| base.IDToken = ts.IDToken | ||
| base.AccessToken = ts.AccessToken | ||
| base.RefreshToken = ts.RefreshToken | ||
| base.LastRefresh = ts.LastRefresh | ||
| base.Email = ts.Email | ||
| base.Type = ts.Type | ||
| base.Expire = ts.Expire | ||
| base.SetMetadata(ts.Metadata) | ||
|
|
||
| return base.Save() |
There was a problem hiding this comment.
The manual field-by-field copy from ts.BaseTokenStorage to a new base instance is brittle. If new fields are added to auth.BaseTokenStorage in the future, they will be missed here, leading to data loss on save. This pattern is repeated across claude/token.go and gemini/gemini_token.go. Consider refactoring this to be more robust. A better approach would be to have a method on BaseTokenStorage to update its file path, or to use a struct copy if the dependency's API allows for it.
| // Create a new token storage with the file path and copy the fields | ||
| base := auth.NewBaseTokenStorage(authFilePath) | ||
| base.IDToken = ts.IDToken | ||
| base.AccessToken = ts.AccessToken | ||
| base.RefreshToken = ts.RefreshToken | ||
| base.LastRefresh = ts.LastRefresh | ||
| base.Email = ts.Email | ||
| base.Type = ts.Type | ||
| base.Expire = ts.Expire | ||
| base.SetMetadata(ts.Metadata) | ||
|
|
||
| return base.Save() |
There was a problem hiding this comment.
The manual field-by-field copy from ts.BaseTokenStorage to a new base instance is brittle. If new fields are added to auth.BaseTokenStorage in the future, they will be missed here, leading to data loss on save. This pattern is repeated across claude/token.go and copilot/token.go. Consider refactoring this to be more robust. A better approach would be to have a method on BaseTokenStorage to update its file path, or to use a struct copy if the dependency's API allows for it.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@go.mod`:
- Line 6: The CI fails because the go.mod replace directive pointing to
../../template-commons/phenotype-go-kit (referencing module
github.com/KooshaPari/phenotype-go-kit) targets a local path that doesn't exist
in CI; fix by either publishing phenotype-go-kit as a proper module/tag,
vendoring the dependency into this repo, or replacing the local replace with a
Git-based pseudo-version (e.g., v0.0.0-<date>-<commit>+incompatible) in go.mod,
and while you’re updating dependencies evaluate whether the custom OAuth2 token
storage implementation is needed by checking OSS stores (e.g.,
github.com/go-oauth2/oauth2, gopkg.in/oauth2.v3/store) and adopting one if
suitable to avoid maintaining custom token storage.
In `@internal/auth/claude/anthropic_auth.go`:
- Around line 132-208: The ExchangeCodeForTokens method in ClaudeAuth is too
long; extract smaller helpers to reduce its length: create a
buildTokenRequestBody(code string, state string, pkce *PKCECodes) (io.Reader,
error) to prepare and JSON-marshal the reqBody (handling parseCodeAndState and
newState injection), and a parseTokenResponse(body []byte) (*ClaudeTokenData,
error) to unmarshal tokenResponse and construct ClaudeTokenData (including
Expire calculation); then refactor ExchangeCodeForTokens to use these helpers
plus a small helper (sendTokenRequest(req *http.Request) ([]byte,
*http.Response, error) or inline httpClient.Do) so the main function only
orchestrates flow and returns a ClaudeAuthBundle.
- Around line 310-333: Replace the custom retry loop in
ClaudeAuth.RefreshTokensWithRetry with github.com/cenkalti/backoff/v5
exponential backoff: create a backoff.NewExponentialBackOff configured with
InitialInterval=500*time.Millisecond, RandomizationFactor=0.5, Multiplier=1.6,
MaxInterval=5*time.Minute and set eb.MaxElapsedTime = 0; wrap the RefreshTokens
call in an operation closure that assigns the successful *ClaudeTokenData to a
variable (e.g., tokenData), sets lastErr on failure, logs via log.Warnf, and
returns nil on success or the error on failure; call
backoff.RetryNotify(operation, backoff.WithMaxRetries(eb, uint64(maxRetries)),
notifyFunc) and if it returns an error, return fmt.Errorf("token refresh failed
after %d attempts: %w", maxRetries, err), otherwise return the tokenData.
In `@internal/auth/copilot/copilot_auth.go`:
- Around line 226-228: The function buildChatCompletionURL is dead code; either
remove it or replace inline URL constructions with it. Locate the unused
function buildChatCompletionURL in copilot_auth.go and either delete the
function entirely to remove dead code, or update any places that construct
copilotAPIEndpoint + "/chat/completions" to call buildChatCompletionURL() so the
helper is used consistently.
In `@internal/auth/copilot/token.go`:
- Around line 87-103: SaveTokenToFile on CopilotTokenStorage currently copies
only BaseTokenStorage fields so Copilot-specific fields (TokenType, Scope,
Username, Name, ExpiresAt) are lost; update SaveTokenToFile (or a helper on
CopilotTokenStorage) to persist these fields by adding them into
base.SetMetadata before calling base.Save() (e.g., serialize TokenType, Scope,
Username, Name, ExpiresAt into the metadata map), or alternatively implement
custom JSON serialization on CopilotTokenStorage to include those fields when
saving, ensuring Load/Save symmetry with BaseTokenStorage.
In `@internal/auth/gemini/gemini_auth.go`:
- Around line 76-148: GetAuthenticatedClient is too long; extract the proxy
setup block (the URL parsing, SOCKS5 dialer creation, http.Transport creation
and setting oauth2.HTTPClient in ctx) into a new helper like
configureProxyContext(ctx context.Context, proxyRaw string) (context.Context,
error). Move all proxy-specific code (parsing cfg.ProxyURL, proxy.SOCKS5
handling, http.ProxyURL handling, creation of transport and proxyClient) into
that function and return an updated ctx or an error; then replace the original
inline block in GetAuthenticatedClient with a call to configureProxyContext and
handle its error. Keep all unique symbols unchanged (GetAuthenticatedClient,
DefaultCallbackPort, oauth2.HTTPClient) and ensure tests/auth paths still get
the returned ctx containing the custom HTTP client.
- Line 141: Handle the ignored errors from json.Marshal and io.ReadAll: check
and handle the error returned by json.Marshal(ts.Token) (the tsToken
assignment), check and handle the error from io.ReadAll(resp.Body) (the
body/read into bytes), and check and handle the later json.Marshal call (the
request/response payload marshal). For each, either return the error upstream or
log it via the package logger consistent with other functions in gemini_auth.go;
replace the `_` ignores with `tsToken, err := json.Marshal(ts.Token)` / `body,
err := io.ReadAll(resp.Body)` / `reqBody, err := json.Marshal(...)` and handle
`err` immediately (log contextual message including the failing symbol and
return the error if the function cannot proceed).
- Around line 227-387: The getTokenFromWeb function is too large; split
responsibilities into smaller helpers: extract the HTTP server creation and
callback handler into startCallbackServer(callbackPort int) (*http.Server, chan
string, chan error) which sets up the ServeMux, registers "/oauth2callback" and
returns server plus code/error channels; extract the browser and
SSH-instructions logic into openBrowserOrShowInstructions(authURL string,
callbackPort int, noBrowser bool) which checks browser.IsAvailable(), calls
browser.OpenURL and logs/prints fallback instructions; extract the
select/timeout/manual prompt loop into waitForAuthCode(codeChan <-chan string,
errChan <-chan error, opts *WebLoginOptions, timeout time.Duration) (string,
error) which implements the manual Prompt flow and timeout logic; then simplify
getTokenFromWeb to call startCallbackServer, defer server.Shutdown, call
openBrowserOrShowInstructions, call waitForAuthCode to obtain authCode, and
finally call config.Exchange to get the token.
- Around line 33-34: The ClientSecret constant is hardcoded (ClientSecret) which
exposes credentials; replace it by reading the secret from a secure
configuration or environment variable (e.g., os.Getenv("GEMINI_CLIENT_SECRET"))
and fail fast if missing, and/or refactor the OAuth flow to use PKCE/no client
secret for native apps (remove reliance on ClientSecret in code paths that
perform token exchange), and add a short comment near ClientID/ClientSecret
explaining why a secret may be optional for native apps or where the secret is
sourced (env/config) for auditability.
- Line 280: The OAuth state parameter is hardcoded in the call to
config.AuthCodeURL("state-token", ...), which breaks CSRF protection; update the
flow that builds authURL (the call to config.AuthCodeURL in gemini_auth.go) to
generate a cryptographically secure random state per request (e.g., using
crypto/rand), store that state in the user session or a server-side store tied
to the request, and pass the generated state string into AuthCodeURL instead of
"state-token", then validate the returned state on callback to ensure it matches
the stored value.
In `@internal/auth/gemini/gemini_token.go`:
- Around line 77-88: CredentialFileName currently embeds the raw email (and
projectID) into a filename which allows path traversal characters; update
CredentialFileName to sanitize/normalize email and projectID before formatting
the filename (e.g., strip or replace path separators "/" and "\" and any ".."
sequences, or better: apply a whitelist/regexp to allow only safe characters
such as alphanumerics, @, ., -, _ and replace all others with an underscore).
Ensure the sanitized values are used in the fmt.Sprintf call, and provide a safe
fallback (e.g., "unknown") if sanitization results in an empty string so
filenames are never able to reference parent paths.
- Around line 20-21: The Token field is weakly typed as any; change the Token
field's type from any to *oauth2.Token (from golang.org/x/oauth2) or a dedicated
struct mirroring OAuth2 fields, update the import accordingly, and adjust any
code that reads/writes the Token field (marshal/unmarshal and access sites) to
handle the pointer type (nil checks) so you regain compile-time safety and
clearer semantics for the Token field.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 8abb6994-8091-4e62-af0a-7998861b1f19
📒 Files selected for processing (7)
go.modinternal/auth/claude/anthropic_auth.gointernal/auth/claude/token.gointernal/auth/copilot/copilot_auth.gointernal/auth/copilot/token.gointernal/auth/gemini/gemini_auth.gointernal/auth/gemini/gemini_token.go
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: golangci-lint
🧰 Additional context used
📓 Path-based instructions (1)
**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*.go: NEVER create a v2 file - refactor the original instead
NEVER create a new class if an existing one can be made generic
NEVER create custom implementations when an OSS library exists - search pkg.go.dev for existing libraries before writing code
Build generic building blocks (provider interface + registry) before application logic
Use chi for HTTP routing (NOT custom routers)
Use zerolog for logging (NOT fmt.Print)
Use viper for configuration (NOT manual env parsing)
Use go-playground/validator for validation (NOT manual if/else validation)
Use golang.org/x/time/rate for rate limiting (NOT custom limiters)
Use template strings for messages instead of hardcoded messages and config-driven logic instead of code-driven
Zero new lint suppressions without inline justification
All new code must pass: go fmt, go vet, golint
Maximum function length: 40 lines
No placeholder TODOs in committed code
Files:
internal/auth/claude/token.gointernal/auth/gemini/gemini_token.gointernal/auth/copilot/token.gointernal/auth/claude/anthropic_auth.gointernal/auth/copilot/copilot_auth.gointernal/auth/gemini/gemini_auth.go
🧠 Learnings (3)
📚 Learning: 2026-02-25T10:11:41.448Z
Learnt from: CR
Repo: KooshaPari/cliproxyapi-plusplus PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-25T10:11:41.448Z
Learning: Applies to **/*.go : Build generic building blocks (provider interface + registry) before application logic
Applied to files:
go.mod
📚 Learning: 2026-02-25T10:11:41.448Z
Learnt from: CR
Repo: KooshaPari/cliproxyapi-plusplus PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-25T10:11:41.448Z
Learning: Always research existing implementations on pkg.go.dev and GitHub before writing new code (target 80%+ existing implementations to fork/adapt)
Applied to files:
go.mod
📚 Learning: 2026-02-25T10:11:41.448Z
Learnt from: CR
Repo: KooshaPari/cliproxyapi-plusplus PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-25T10:11:41.448Z
Learning: Applies to **/*.go : NEVER create custom implementations when an OSS library exists - search pkg.go.dev for existing libraries before writing code
Applied to files:
go.mod
🪛 Betterleaks (1.1.1)
internal/auth/gemini/gemini_auth.go
[high] 34-34: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🪛 GitHub Actions: codeql
internal/auth/claude/token.go
[error] 7-7: go build ./... failed: github.com/KooshaPari/phenotype-go-kit@v0.0.0: replacement directory ../../template-commons/phenotype-go-kit does not exist.
🪛 GitHub Check: Analyze (Go) (go)
internal/auth/claude/token.go
[failure] 8-8:
github.com/KooshaPari/phenotype-go-kit@v0.0.0 (replaced by ../../template-commons/phenotype-go-kit): reading ../../template-commons/phenotype-go-kit/go.mod: open /home/runner/work/template-commons/phenotype-go-kit/go.mod: no such file or directory
[failure] 7-7:
github.com/KooshaPari/phenotype-go-kit@v0.0.0: replacement directory ../../template-commons/phenotype-go-kit does not exist
internal/auth/claude/anthropic_auth.go
[failure] 16-16:
github.com/KooshaPari/phenotype-go-kit@v0.0.0 (replaced by ../../template-commons/phenotype-go-kit): reading ../../template-commons/phenotype-go-kit/go.mod: open /home/runner/work/template-commons/phenotype-go-kit/go.mod: no such file or directory
internal/auth/copilot/copilot_auth.go
[failure] 14-14:
github.com/KooshaPari/phenotype-go-kit@v0.0.0 (replaced by ../../template-commons/phenotype-go-kit): reading ../../template-commons/phenotype-go-kit/go.mod: open /home/runner/work/template-commons/phenotype-go-kit/go.mod: no such file or directory
internal/auth/gemini/gemini_auth.go
[failure] 19-19:
github.com/KooshaPari/phenotype-go-kit@v0.0.0 (replaced by ../../template-commons/phenotype-go-kit): reading ../../template-commons/phenotype-go-kit/go.mod: open /home/runner/work/template-commons/phenotype-go-kit/go.mod: no such file or directory
[failure] 18-18:
github.com/KooshaPari/phenotype-go-kit@v0.0.0 (replaced by ../../template-commons/phenotype-go-kit): reading ../../template-commons/phenotype-go-kit/go.mod: open /home/runner/work/template-commons/phenotype-go-kit/go.mod: no such file or directory
🪛 golangci-lint (2.11.3)
internal/auth/claude/anthropic_auth.go
[error] 65-65: undefined: NewAnthropicHttpClient
(typecheck)
[error] 81-81: undefined: PKCECodes
(typecheck)
[error] 132-132: undefined: PKCECodes
(typecheck)
[error] 202-202: undefined: ClaudeAuthBundle
(typecheck)
[error] 221-221: undefined: ClaudeTokenData
(typecheck)
[error] 287-287: undefined: ClaudeAuthBundle
(typecheck)
[error] 289-289: storage.AccessToken undefined (type *ClaudeTokenStorage has no field or method AccessToken)
(typecheck)
[error] 290-290: storage.RefreshToken undefined (type *ClaudeTokenStorage has no field or method RefreshToken)
(typecheck)
[error] 310-310: undefined: ClaudeTokenData
(typecheck)
[error] 342-342: undefined: ClaudeTokenData
(typecheck)
🔇 Additional comments (4)
internal/auth/claude/anthropic_auth.go (2)
158-158: 🧹 Nitpick | 🔵 TrivialRemove commented-out debug statements.
Multiple commented debug log statements remain in the code (lines 158, 181, 186, 262). Per coding guidelines, no placeholder TODOs or debug artifacts should be committed.
🧹 Remove commented debug code
- // log.Debugf("Token exchange request: %s", string(jsonBody)) ... - // log.Debugf("Token response: %s", string(body)) ... - // log.Debugf("Token response: %s", string(body)) ... - // log.Debugf("Token response: %s", string(body))Also applies to: 181-182, 186-186, 262-262
⛔ Skipped due to learnings
Learnt from: CR Repo: KooshaPari/cliproxyapi-plusplus PR: 0 File: AGENTS.md:0-0 Timestamp: 2026-02-25T10:11:41.448Z Learning: Applies to **/*.go : No placeholder TODOs in committed code
65-65: The types and functions referenced are actually defined in the codebase. Search results confirm:
PKCECodesexists inpkg/llmproxy/auth/claude/anthropic.go:4ClaudeAuthBundleexists inpkg/llmproxy/auth/claude/anthropic.go:25ClaudeTokenDataexists inpkg/llmproxy/auth/claude/anthropic.go:13NewAnthropicHttpClientexists inpkg/llmproxy/auth/claude/utls_transport.go:161No build failure will occur due to undefined types.
> Likely an incorrect or invalid review comment.internal/auth/claude/token.go (1)
40-56: The approach inSaveTokenToFilefollows the same pattern used consistently across all token storage implementations (Claude, Copilot, and Gemini). While the method creates a newBaseTokenStorageinstance and copies fields, this pattern appears to be systematic across the codebase. Without access to thephenotype-go-kitpackage source, it's unclear whether simplification is possible or if this approach is required by the API design.internal/auth/copilot/copilot_auth.go (1)
78-101: Code references undefined types and will not compile.
internal/auth/copilot/copilot_auth.gousesDeviceFlowClientandNewDeviceFlowClient()(lines 57, 66, 79, 85, 159) which are not defined in this package and not imported from anywhere. The code will fail compilation with "undefined: DeviceFlowClient" and "undefined: NewDeviceFlowClient" errors.Additionally, even if
DeviceFlowClientwere properly imported frompkg/llmproxy/auth/copilot/oauth.go, the code at line 85 treatsFetchUserInfo's return value as a struct with.Login,.Namefields, butFetchUserInfoin that package returns(string, error), not a struct type.Verify the intended location of
DeviceFlowClientand either import it correctly or define it in this package. If importing frompkg/llmproxy/auth/copilot/, also reconcile theFetchUserInforeturn type mismatch.> Likely an incorrect or invalid review comment.
| go 1.26.0 | ||
|
|
||
| require ( | ||
| github.com/KooshaPari/phenotype-go-kit v0.0.0 |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
golang oauth2 token storage library pkg.go.dev
💡 Result:
The primary Golang library providing OAuth2 token storage on pkg.go.dev is the store package from the go-oauth2/oauth2 project, specifically at https://pkg.go.dev/gopkg.in/oauth2.v3/store (v3.12.0, published 2019) and forks like https://pkg.go.dev/code.superseriousbusiness.org/oauth2/v4/store (updated 2025). It offers TokenStore implementations including memory (NewMemoryTokenStore), file (NewFileTokenStore(filename)), and BuntDB-based storage with methods like Create, GetByAccess, GetByRefresh, RemoveByAccess, etc. The main repo is https://github.com/go-oauth2/oauth2 (latest v4.5.4 as of 2025-08-20), an OAuth2 server library implementing RFC 6749 with pluggable storage backends (BuntDB default, Redis, MongoDB, MySQL, PostgreSQL, etc.). The official golang.org/x/oauth2 (v0.36.0 as of 2026-02-11) handles OAuth2 clients but does not provide built-in persistent token storage; users typically implement custom storage (e.g., file, database) or use wrappers like the TokenStore example in tutorials. For client-side token persistence best practices (from recent sources), use encrypted file storage, databases, or secrets managers; never log tokens or store in plain text.
Citations:
- 1: https://pkg.go.dev/gopkg.in/oauth2.v3/store
- 2: https://pkg.go.dev/code.superseriousbusiness.org/oauth2/v4/store
- 3: https://pkg.go.dev/golang.org/x/oauth2
- 4: https://github.com/go-oauth2/oauth2
- 5: https://pkg.go.dev/github.com/dikhimartin/oauth2/v5/store
CI build fails: local replace directive points to non-existent path.
The replace directive on line 115 references ../../template-commons/phenotype-go-kit, which does not exist in the CI environment. This causes the build to fail immediately.
Before merging, resolve this by one of:
- Publish
phenotype-go-kitas a proper module with a GitHub release tag, or - Vendor the dependency directly, or
- Use a Git-based pseudo-version (e.g.,
v0.0.0-20260324...+incompatible)
Additionally, review whether a custom token storage implementation is necessary. Existing OSS libraries for OAuth2 token storage are available on pkg.go.dev, including:
github.com/go-oauth2/oauth2(TokenStore implementations for memory, file, BuntDB, Redis, MongoDB, MySQL, PostgreSQL)gopkg.in/oauth2.v3/storeand later forks
Verify against these before maintaining a custom implementation per coding guidelines.
Also applies to: 115-115
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@go.mod` at line 6, The CI fails because the go.mod replace directive pointing
to ../../template-commons/phenotype-go-kit (referencing module
github.com/KooshaPari/phenotype-go-kit) targets a local path that doesn't exist
in CI; fix by either publishing phenotype-go-kit as a proper module/tag,
vendoring the dependency into this repo, or replacing the local replace with a
Git-based pseudo-version (e.g., v0.0.0-<date>-<commit>+incompatible) in go.mod,
and while you’re updating dependencies evaluate whether the custom OAuth2 token
storage implementation is needed by checking OSS stores (e.g.,
github.com/go-oauth2/oauth2, gopkg.in/oauth2.v3/store) and adopting one if
suitable to avoid maintaining custom token storage.
| func (o *ClaudeAuth) ExchangeCodeForTokens(ctx context.Context, code, state string, pkceCodes *PKCECodes) (*ClaudeAuthBundle, error) { | ||
| if pkceCodes == nil { | ||
| return nil, fmt.Errorf("PKCE codes are required for token exchange") | ||
| } | ||
| newCode, newState := o.parseCodeAndState(code) | ||
|
|
||
| // Prepare token exchange request | ||
| reqBody := map[string]interface{}{ | ||
| "code": newCode, | ||
| "state": state, | ||
| "grant_type": "authorization_code", | ||
| "client_id": ClientID, | ||
| "redirect_uri": RedirectURI, | ||
| "code_verifier": pkceCodes.CodeVerifier, | ||
| } | ||
|
|
||
| // Include state if present | ||
| if newState != "" { | ||
| reqBody["state"] = newState | ||
| } | ||
|
|
||
| jsonBody, err := json.Marshal(reqBody) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to marshal request body: %w", err) | ||
| } | ||
|
|
||
| // log.Debugf("Token exchange request: %s", string(jsonBody)) | ||
|
|
||
| req, err := http.NewRequestWithContext(ctx, "POST", TokenURL, strings.NewReader(string(jsonBody))) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to create token request: %w", err) | ||
| } | ||
| req.Header.Set("Content-Type", "application/json") | ||
| req.Header.Set("Accept", "application/json") | ||
|
|
||
| resp, err := o.httpClient.Do(req) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("token exchange request failed: %w", err) | ||
| } | ||
| defer func() { | ||
| if errClose := resp.Body.Close(); errClose != nil { | ||
| log.Errorf("failed to close response body: %v", errClose) | ||
| } | ||
| }() | ||
|
|
||
| body, err := io.ReadAll(resp.Body) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to read token response: %w", err) | ||
| } | ||
| // log.Debugf("Token response: %s", string(body)) | ||
|
|
||
| if resp.StatusCode != http.StatusOK { | ||
| return nil, fmt.Errorf("token exchange failed with status %d: %s", resp.StatusCode, string(body)) | ||
| } | ||
| // log.Debugf("Token response: %s", string(body)) | ||
|
|
||
| var tokenResp tokenResponse | ||
| if err = json.Unmarshal(body, &tokenResp); err != nil { | ||
| return nil, fmt.Errorf("failed to parse token response: %w", err) | ||
| } | ||
|
|
||
| // Create token data | ||
| tokenData := ClaudeTokenData{ | ||
| AccessToken: tokenResp.AccessToken, | ||
| RefreshToken: tokenResp.RefreshToken, | ||
| Email: tokenResp.Account.EmailAddress, | ||
| Expire: time.Now().Add(time.Duration(tokenResp.ExpiresIn) * time.Second).Format(time.RFC3339), | ||
| } | ||
|
|
||
| // Create auth bundle | ||
| bundle := &ClaudeAuthBundle{ | ||
| TokenData: tokenData, | ||
| LastRefresh: time.Now().Format(time.RFC3339), | ||
| } | ||
|
|
||
| return bundle, nil | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Function exceeds maximum length guideline (40 lines).
ExchangeCodeForTokens spans ~75 lines. Per coding guidelines, the maximum function length is 40 lines. Consider extracting helpers for request building and response parsing.
🧰 Tools
🪛 golangci-lint (2.11.3)
[error] 132-132: undefined: PKCECodes
(typecheck)
[error] 202-202: undefined: ClaudeAuthBundle
(typecheck)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/auth/claude/anthropic_auth.go` around lines 132 - 208, The
ExchangeCodeForTokens method in ClaudeAuth is too long; extract smaller helpers
to reduce its length: create a buildTokenRequestBody(code string, state string,
pkce *PKCECodes) (io.Reader, error) to prepare and JSON-marshal the reqBody
(handling parseCodeAndState and newState injection), and a
parseTokenResponse(body []byte) (*ClaudeTokenData, error) to unmarshal
tokenResponse and construct ClaudeTokenData (including Expire calculation); then
refactor ExchangeCodeForTokens to use these helpers plus a small helper
(sendTokenRequest(req *http.Request) ([]byte, *http.Response, error) or inline
httpClient.Do) so the main function only orchestrates flow and returns a
ClaudeAuthBundle.
| func (o *ClaudeAuth) RefreshTokensWithRetry(ctx context.Context, refreshToken string, maxRetries int) (*ClaudeTokenData, error) { | ||
| var lastErr error | ||
|
|
||
| for attempt := 0; attempt < maxRetries; attempt++ { | ||
| if attempt > 0 { | ||
| // Wait before retry | ||
| select { | ||
| case <-ctx.Done(): | ||
| return nil, ctx.Err() | ||
| case <-time.After(time.Duration(attempt) * time.Second): | ||
| } | ||
| } | ||
|
|
||
| tokenData, err := o.RefreshTokens(ctx, refreshToken) | ||
| if err == nil { | ||
| return tokenData, nil | ||
| } | ||
|
|
||
| lastErr = err | ||
| log.Warnf("Token refresh attempt %d failed: %v", attempt+1, err) | ||
| } | ||
|
|
||
| return nil, fmt.Errorf("token refresh failed after %d attempts: %w", maxRetries, lastErr) | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
🧩 Analysis chain
🌐 Web query:
golang retry backoff library cenkalti
💡 Result:
The Go library for retry backoff by cenkalti is github.com/cenkalti/backoff. It implements exponential backoff (a port from Google's Java HTTP client), constant backoff, zero backoff, and stop backoff strategies for retrying failed operations. Key features: - backoff.Retry(operation, backoff) to retry until success or backoff stops (guaranteed at least one attempt). - backoff.NewExponentialBackOff with defaults: InitialInterval=500ms, RandomizationFactor=0.5, Multiplier=1.6, MaxInterval=5m, MaxElapsedTime=0 (unlimited). - Supports context (WithContext), Ticker for channel-based retries, generics in v5 (RetryWithData). - MIT licensed, actively maintained (last push 2026-02-21 on default v5 branch). Installation: go get github.com/cenkalti/backoff/v5 Basic usage example: import ( "log" "github.com/cenkalti/backoff/v5" ) func main { operation := func error { // Simulate failure sometimes return errors.New("failed") } err := backoff.Retry(operation, backoff.NewExponentialBackOff) if err != nil { log.Fatal("permanent error:", err) } log.Println("succeeded") } Official docs: https://pkg.go.dev/github.com/cenkalti/backoff/v5 GitHub: https://github.com/cenkalti/backoff (default branch v5)
Citations:
- 1: https://github.com/cenkalti/backoff
- 2: https://pkg.go.dev/github.com/cenkalti/backoff/v4
- 3: https://pkg.go.dev/github.com/cenkalti/backoff/v5
- 4: https://tessl.io/registry/tessl/golang-github-com-cenkalti--backoff-v4
- 5: https://ref.gotd.dev/pkg/github.com/cenkalti/backoff/v4.html
- 6: https://github.com/cenkalti/backoff/blob/v5/backoff.go
🏁 Script executed:
# Check if the file exists and view the target lines
if [ -f "internal/auth/claude/anthropic_auth.go" ]; then
echo "=== File found, checking lines 310-333 ==="
sed -n '310,333p' internal/auth/claude/anthropic_auth.go
else
echo "File not found at expected path"
find . -name "anthropic_auth.go" -type f
fiRepository: KooshaPari/cliproxyapi-plusplus
Length of output: 776
🏁 Script executed:
# Search for any existing retry or backoff patterns in the codebase
echo "=== Searching for retry patterns ==="
rg "retry|backoff|exponential" --type go -i -B 2 -A 2
echo ""
echo "=== Checking for cenkalti/backoff dependency ==="
if [ -f "go.mod" ]; then
grep -i "cenkalti\|backoff" go.mod
else
echo "go.mod not found"
fiRepository: KooshaPari/cliproxyapi-plusplus
Length of output: 50388
🏁 Script executed:
# Search for other RefreshTokens methods or retry logic patterns
echo "=== Looking for RefreshToken methods ==="
rg "RefreshToken" --type go -B 1 -A 5
echo ""
echo "=== Checking for time.After patterns (linear backoff indicators) ==="
rg "time\.After.*attempt|attempt.*time\.(Second|Millisecond)" --type go -B 2 -A 2Repository: KooshaPari/cliproxyapi-plusplus
Length of output: 50388
Replace custom retry logic with cenkalti/backoff library.
The current linear backoff implementation (attempt * time.Second) violates the coding guideline to use existing OSS libraries instead of custom implementations. Use github.com/cenkalti/backoff/v5 for exponential backoff with proper defaults: InitialInterval=500ms, RandomizationFactor=0.5, Multiplier=1.6, MaxInterval=5m. This aligns with patterns already used elsewhere in the codebase (Kiro, Gemini executors) and provides better resilience for transient failures.
Example refactor
Replace the retry loop with:
operation := func() error {
tokenData, err := o.RefreshTokens(ctx, refreshToken)
if err == nil {
return nil
}
lastErr = err
log.Warnf("Token refresh attempt failed: %v", err)
return err
}
eb := backoff.NewExponentialBackOff()
eb.MaxElapsedTime = 0 // Let maxRetries control attempts
if err := backoff.RetryNotify(operation, backoff.WithMaxRetries(eb, uint64(maxRetries)),
func(err error, duration time.Duration) {
// Optional: log retry delays
}); err != nil {
return nil, fmt.Errorf("token refresh failed after %d attempts: %w", maxRetries, err)
}
return tokenData, nil🧰 Tools
🪛 golangci-lint (2.11.3)
[error] 310-310: undefined: ClaudeTokenData
(typecheck)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/auth/claude/anthropic_auth.go` around lines 310 - 333, Replace the
custom retry loop in ClaudeAuth.RefreshTokensWithRetry with
github.com/cenkalti/backoff/v5 exponential backoff: create a
backoff.NewExponentialBackOff configured with
InitialInterval=500*time.Millisecond, RandomizationFactor=0.5, Multiplier=1.6,
MaxInterval=5*time.Minute and set eb.MaxElapsedTime = 0; wrap the RefreshTokens
call in an operation closure that assigns the successful *ClaudeTokenData to a
variable (e.g., tokenData), sets lastErr on failure, logs via log.Warnf, and
returns nil on success or the error on failure; call
backoff.RetryNotify(operation, backoff.WithMaxRetries(eb, uint64(maxRetries)),
notifyFunc) and if it returns an error, return fmt.Errorf("token refresh failed
after %d attempts: %w", maxRetries, err), otherwise return the tokenData.
| func buildChatCompletionURL() string { | ||
| return copilotAPIEndpoint + "/chat/completions" | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Unused function buildChatCompletionURL.
This function is defined but not called anywhere in the file. Consider removing dead code or using it where appropriate.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/auth/copilot/copilot_auth.go` around lines 226 - 228, The function
buildChatCompletionURL is dead code; either remove it or replace inline URL
constructions with it. Locate the unused function buildChatCompletionURL in
copilot_auth.go and either delete the function entirely to remove dead code, or
update any places that construct copilotAPIEndpoint + "/chat/completions" to
call buildChatCompletionURL() so the helper is used consistently.
| func (ts *CopilotTokenStorage) SaveTokenToFile(authFilePath string) error { | ||
| misc.LogSavingCredentials(authFilePath) | ||
| ts.Type = "github-copilot" | ||
|
|
||
| // Create a new token storage with the file path and copy the fields | ||
| base := auth.NewBaseTokenStorage(authFilePath) | ||
| base.IDToken = ts.IDToken | ||
| base.AccessToken = ts.AccessToken | ||
| base.RefreshToken = ts.RefreshToken | ||
| base.LastRefresh = ts.LastRefresh | ||
| base.Email = ts.Email | ||
| base.Type = ts.Type | ||
| base.Expire = ts.Expire | ||
| base.SetMetadata(ts.Metadata) | ||
|
|
||
| return base.Save() | ||
| } |
There was a problem hiding this comment.
Data loss: Copilot-specific fields not persisted.
SaveTokenToFile copies only the BaseTokenStorage fields to the new instance, but the Copilot-specific fields (TokenType, Scope, Username, Name, ExpiresAt) defined on lines 17-26 are not saved. When the token is loaded later, this metadata will be missing.
Either:
- Add metadata storage for Copilot-specific fields (e.g., via
SetMetadata), or - Override the JSON serialization to include all fields
🐛 Proposed fix using metadata
func (ts *CopilotTokenStorage) SaveTokenToFile(authFilePath string) error {
misc.LogSavingCredentials(authFilePath)
ts.Type = "github-copilot"
// Create a new token storage with the file path and copy the fields
base := auth.NewBaseTokenStorage(authFilePath)
base.IDToken = ts.IDToken
base.AccessToken = ts.AccessToken
base.RefreshToken = ts.RefreshToken
base.LastRefresh = ts.LastRefresh
base.Email = ts.Email
base.Type = ts.Type
base.Expire = ts.Expire
- base.SetMetadata(ts.Metadata)
+ // Include Copilot-specific fields in metadata
+ metadata := ts.Metadata
+ if metadata == nil {
+ metadata = make(map[string]any)
+ }
+ metadata["token_type"] = ts.TokenType
+ metadata["scope"] = ts.Scope
+ metadata["username"] = ts.Username
+ metadata["name"] = ts.Name
+ metadata["expires_at"] = ts.ExpiresAt
+ base.SetMetadata(metadata)
return base.Save()
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func (ts *CopilotTokenStorage) SaveTokenToFile(authFilePath string) error { | |
| misc.LogSavingCredentials(authFilePath) | |
| ts.Type = "github-copilot" | |
| // Create a new token storage with the file path and copy the fields | |
| base := auth.NewBaseTokenStorage(authFilePath) | |
| base.IDToken = ts.IDToken | |
| base.AccessToken = ts.AccessToken | |
| base.RefreshToken = ts.RefreshToken | |
| base.LastRefresh = ts.LastRefresh | |
| base.Email = ts.Email | |
| base.Type = ts.Type | |
| base.Expire = ts.Expire | |
| base.SetMetadata(ts.Metadata) | |
| return base.Save() | |
| } | |
| func (ts *CopilotTokenStorage) SaveTokenToFile(authFilePath string) error { | |
| misc.LogSavingCredentials(authFilePath) | |
| ts.Type = "github-copilot" | |
| // Create a new token storage with the file path and copy the fields | |
| base := auth.NewBaseTokenStorage(authFilePath) | |
| base.IDToken = ts.IDToken | |
| base.AccessToken = ts.AccessToken | |
| base.RefreshToken = ts.RefreshToken | |
| base.LastRefresh = ts.LastRefresh | |
| base.Email = ts.Email | |
| base.Type = ts.Type | |
| base.Expire = ts.Expire | |
| // Include Copilot-specific fields in metadata | |
| metadata := ts.Metadata | |
| if metadata == nil { | |
| metadata = make(map[string]any) | |
| } | |
| metadata["token_type"] = ts.TokenType | |
| metadata["scope"] = ts.Scope | |
| metadata["username"] = ts.Username | |
| metadata["name"] = ts.Name | |
| metadata["expires_at"] = ts.ExpiresAt | |
| base.SetMetadata(metadata) | |
| return base.Save() | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/auth/copilot/token.go` around lines 87 - 103, SaveTokenToFile on
CopilotTokenStorage currently copies only BaseTokenStorage fields so
Copilot-specific fields (TokenType, Scope, Username, Name, ExpiresAt) are lost;
update SaveTokenToFile (or a helper on CopilotTokenStorage) to persist these
fields by adding them into base.SetMetadata before calling base.Save() (e.g.,
serialize TokenType, Scope, Username, Name, ExpiresAt into the metadata map), or
alternatively implement custom JSON serialization on CopilotTokenStorage to
include those fields when saving, ensuring Load/Save symmetry with
BaseTokenStorage.
| } | ||
|
|
||
| // Unmarshal the stored token into an oauth2.Token object. | ||
| tsToken, _ := json.Marshal(ts.Token) |
There was a problem hiding this comment.
Ignored errors may mask issues.
Multiple errors are silently ignored with _:
- Line 141:
json.Marshalerror ignored - Line 181:
io.ReadAllerror ignored - Line 194:
json.Marshalerror ignored
While these are unlikely to fail, ignoring errors violates defensive coding practices. At minimum, log them.
🛡️ Proposed fix for line 141
- tsToken, _ := json.Marshal(ts.Token)
+ tsToken, err := json.Marshal(ts.Token)
+ if err != nil {
+ return nil, fmt.Errorf("failed to marshal stored token: %w", err)
+ }Also applies to: 181-181, 194-194
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/auth/gemini/gemini_auth.go` at line 141, Handle the ignored errors
from json.Marshal and io.ReadAll: check and handle the error returned by
json.Marshal(ts.Token) (the tsToken assignment), check and handle the error from
io.ReadAll(resp.Body) (the body/read into bytes), and check and handle the later
json.Marshal call (the request/response payload marshal). For each, either
return the error upstream or log it via the package logger consistent with other
functions in gemini_auth.go; replace the `_` ignores with `tsToken, err :=
json.Marshal(ts.Token)` / `body, err := io.ReadAll(resp.Body)` / `reqBody, err
:= json.Marshal(...)` and handle `err` immediately (log contextual message
including the failing symbol and return the error if the function cannot
proceed).
| func (g *GeminiAuth) getTokenFromWeb(ctx context.Context, config *oauth2.Config, opts *WebLoginOptions) (*oauth2.Token, error) { | ||
| callbackPort := DefaultCallbackPort | ||
| if opts != nil && opts.CallbackPort > 0 { | ||
| callbackPort = opts.CallbackPort | ||
| } | ||
| callbackURL := fmt.Sprintf("http://localhost:%d/oauth2callback", callbackPort) | ||
|
|
||
| // Use a channel to pass the authorization code from the HTTP handler to the main function. | ||
| codeChan := make(chan string, 1) | ||
| errChan := make(chan error, 1) | ||
|
|
||
| // Create a new HTTP server with its own multiplexer. | ||
| mux := http.NewServeMux() | ||
| server := &http.Server{Addr: fmt.Sprintf(":%d", callbackPort), Handler: mux} | ||
| config.RedirectURL = callbackURL | ||
|
|
||
| mux.HandleFunc("/oauth2callback", func(w http.ResponseWriter, r *http.Request) { | ||
| if err := r.URL.Query().Get("error"); err != "" { | ||
| _, _ = fmt.Fprintf(w, "Authentication failed: %s", err) | ||
| select { | ||
| case errChan <- fmt.Errorf("authentication failed via callback: %s", err): | ||
| default: | ||
| } | ||
| return | ||
| } | ||
| code := r.URL.Query().Get("code") | ||
| if code == "" { | ||
| _, _ = fmt.Fprint(w, "Authentication failed: code not found.") | ||
| select { | ||
| case errChan <- fmt.Errorf("code not found in callback"): | ||
| default: | ||
| } | ||
| return | ||
| } | ||
| _, _ = fmt.Fprint(w, "<html><body><h1>Authentication successful!</h1><p>You can close this window.</p></body></html>") | ||
| select { | ||
| case codeChan <- code: | ||
| default: | ||
| } | ||
| }) | ||
|
|
||
| // Start the server in a goroutine. | ||
| go func() { | ||
| if err := server.ListenAndServe(); !errors.Is(err, http.ErrServerClosed) { | ||
| log.Errorf("ListenAndServe(): %v", err) | ||
| select { | ||
| case errChan <- err: | ||
| default: | ||
| } | ||
| } | ||
| }() | ||
|
|
||
| // Open the authorization URL in the user's browser. | ||
| authURL := config.AuthCodeURL("state-token", oauth2.AccessTypeOffline, oauth2.SetAuthURLParam("prompt", "consent")) | ||
|
|
||
| noBrowser := false | ||
| if opts != nil { | ||
| noBrowser = opts.NoBrowser | ||
| } | ||
|
|
||
| if !noBrowser { | ||
| fmt.Println("Opening browser for authentication...") | ||
|
|
||
| // Check if browser is available | ||
| if !browser.IsAvailable() { | ||
| log.Warn("No browser available on this system") | ||
| util.PrintSSHTunnelInstructions(callbackPort) | ||
| fmt.Printf("Please manually open this URL in your browser:\n\n%s\n", authURL) | ||
| } else { | ||
| if err := browser.OpenURL(authURL); err != nil { | ||
| authErr := codex.NewAuthenticationError(codex.ErrBrowserOpenFailed, err) | ||
| log.Warn(codex.GetUserFriendlyMessage(authErr)) | ||
| util.PrintSSHTunnelInstructions(callbackPort) | ||
| fmt.Printf("Please manually open this URL in your browser:\n\n%s\n", authURL) | ||
|
|
||
| // Log platform info for debugging | ||
| platformInfo := browser.GetPlatformInfo() | ||
| log.Debugf("Browser platform info: %+v", platformInfo) | ||
| } else { | ||
| log.Debug("Browser opened successfully") | ||
| } | ||
| } | ||
| } else { | ||
| util.PrintSSHTunnelInstructions(callbackPort) | ||
| fmt.Printf("Please open this URL in your browser:\n\n%s\n", authURL) | ||
| } | ||
|
|
||
| fmt.Println("Waiting for authentication callback...") | ||
|
|
||
| // Wait for the authorization code or an error. | ||
| var authCode string | ||
| timeoutTimer := time.NewTimer(5 * time.Minute) | ||
| defer timeoutTimer.Stop() | ||
|
|
||
| var manualPromptTimer *time.Timer | ||
| var manualPromptC <-chan time.Time | ||
| if opts != nil && opts.Prompt != nil { | ||
| manualPromptTimer = time.NewTimer(15 * time.Second) | ||
| manualPromptC = manualPromptTimer.C | ||
| defer manualPromptTimer.Stop() | ||
| } | ||
|
|
||
| waitForCallback: | ||
| for { | ||
| select { | ||
| case code := <-codeChan: | ||
| authCode = code | ||
| break waitForCallback | ||
| case err := <-errChan: | ||
| return nil, err | ||
| case <-manualPromptC: | ||
| manualPromptC = nil | ||
| if manualPromptTimer != nil { | ||
| manualPromptTimer.Stop() | ||
| } | ||
| select { | ||
| case code := <-codeChan: | ||
| authCode = code | ||
| break waitForCallback | ||
| case err := <-errChan: | ||
| return nil, err | ||
| default: | ||
| } | ||
| input, err := opts.Prompt("Paste the Gemini callback URL (or press Enter to keep waiting): ") | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| parsed, err := misc.ParseOAuthCallback(input) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| if parsed == nil { | ||
| continue | ||
| } | ||
| if parsed.Error != "" { | ||
| return nil, fmt.Errorf("authentication failed via callback: %s", parsed.Error) | ||
| } | ||
| if parsed.Code == "" { | ||
| return nil, fmt.Errorf("code not found in callback") | ||
| } | ||
| authCode = parsed.Code | ||
| break waitForCallback | ||
| case <-timeoutTimer.C: | ||
| return nil, fmt.Errorf("oauth flow timed out") | ||
| } | ||
| } | ||
|
|
||
| // Shutdown the server. | ||
| if err := server.Shutdown(ctx); err != nil { | ||
| log.Errorf("Failed to shut down server: %v", err) | ||
| } | ||
|
|
||
| // Exchange the authorization code for a token. | ||
| token, err := config.Exchange(ctx, authCode) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to exchange token: %w", err) | ||
| } | ||
|
|
||
| fmt.Println("Authentication successful.") | ||
| return token, nil | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Function getTokenFromWeb exceeds maximum length (~160 lines vs 40 line guideline).
This function handles too many concerns: server setup, browser launching, timeout management, manual prompt handling, and token exchange. Per coding guidelines, extract these into smaller, focused helpers.
Suggested extraction:
startCallbackServer(port int) (*http.Server, chan string, chan error)openBrowserOrShowInstructions(url string, port int, noBrowser bool)waitForAuthCode(codeChan, errChan, promptOpts, timeout) (string, error)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/auth/gemini/gemini_auth.go` around lines 227 - 387, The
getTokenFromWeb function is too large; split responsibilities into smaller
helpers: extract the HTTP server creation and callback handler into
startCallbackServer(callbackPort int) (*http.Server, chan string, chan error)
which sets up the ServeMux, registers "/oauth2callback" and returns server plus
code/error channels; extract the browser and SSH-instructions logic into
openBrowserOrShowInstructions(authURL string, callbackPort int, noBrowser bool)
which checks browser.IsAvailable(), calls browser.OpenURL and logs/prints
fallback instructions; extract the select/timeout/manual prompt loop into
waitForAuthCode(codeChan <-chan string, errChan <-chan error, opts
*WebLoginOptions, timeout time.Duration) (string, error) which implements the
manual Prompt flow and timeout logic; then simplify getTokenFromWeb to call
startCallbackServer, defer server.Shutdown, call openBrowserOrShowInstructions,
call waitForAuthCode to obtain authCode, and finally call config.Exchange to get
the token.
| }() | ||
|
|
||
| // Open the authorization URL in the user's browser. | ||
| authURL := config.AuthCodeURL("state-token", oauth2.AccessTypeOffline, oauth2.SetAuthURLParam("prompt", "consent")) |
There was a problem hiding this comment.
Security: Hardcoded OAuth state parameter defeats CSRF protection.
The state parameter is hardcoded as "state-token" instead of being a cryptographically random value per request. This defeats the purpose of CSRF protection in OAuth flows.
🛡️ Proposed fix
+import (
+ "crypto/rand"
+ "encoding/hex"
+)
+
+func generateState() string {
+ b := make([]byte, 16)
+ rand.Read(b)
+ return hex.EncodeToString(b)
+}
// In getTokenFromWeb:
- authURL := config.AuthCodeURL("state-token", oauth2.AccessTypeOffline, oauth2.SetAuthURLParam("prompt", "consent"))
+ state := generateState()
+ authURL := config.AuthCodeURL(state, oauth2.AccessTypeOffline, oauth2.SetAuthURLParam("prompt", "consent"))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| authURL := config.AuthCodeURL("state-token", oauth2.AccessTypeOffline, oauth2.SetAuthURLParam("prompt", "consent")) | |
| import ( | |
| "crypto/rand" | |
| "encoding/hex" | |
| ) | |
| func generateState() string { | |
| b := make([]byte, 16) | |
| rand.Read(b) | |
| return hex.EncodeToString(b) | |
| } | |
| // In getTokenFromWeb: | |
| state := generateState() | |
| authURL := config.AuthCodeURL(state, oauth2.AccessTypeOffline, oauth2.SetAuthURLParam("prompt", "consent")) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/auth/gemini/gemini_auth.go` at line 280, The OAuth state parameter
is hardcoded in the call to config.AuthCodeURL("state-token", ...), which breaks
CSRF protection; update the flow that builds authURL (the call to
config.AuthCodeURL in gemini_auth.go) to generate a cryptographically secure
random state per request (e.g., using crypto/rand), store that state in the user
session or a server-side store tied to the request, and pass the generated state
string into AuthCodeURL instead of "state-token", then validate the returned
state on callback to ensure it matches the stored value.
| // Token holds the raw OAuth2 token data, including access and refresh tokens. | ||
| Token any `json:"token"` |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Weak typing: Token field uses any.
Using any for the Token field loses type safety. Since this holds OAuth2 token data, consider using *oauth2.Token or a well-defined struct to enable compile-time checks and better documentation.
- // Token holds the raw OAuth2 token data, including access and refresh tokens.
- Token any `json:"token"`
+ // Token holds the raw OAuth2 token data, including access and refresh tokens.
+ Token *oauth2.Token `json:"token"`📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Token holds the raw OAuth2 token data, including access and refresh tokens. | |
| Token any `json:"token"` | |
| // Token holds the raw OAuth2 token data, including access and refresh tokens. | |
| Token *oauth2.Token `json:"token"` |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/auth/gemini/gemini_token.go` around lines 20 - 21, The Token field
is weakly typed as any; change the Token field's type from any to *oauth2.Token
(from golang.org/x/oauth2) or a dedicated struct mirroring OAuth2 fields, update
the import accordingly, and adjust any code that reads/writes the Token field
(marshal/unmarshal and access sites) to handle the pointer type (nil checks) so
you regain compile-time safety and clearer semantics for the Token field.
| func CredentialFileName(email, projectID string, includeProviderPrefix bool) string { | ||
| email = strings.TrimSpace(email) | ||
| project := strings.TrimSpace(projectID) | ||
| if strings.EqualFold(project, "all") || strings.Contains(project, ",") { | ||
| return fmt.Sprintf("gemini-%s-all.json", email) | ||
| } | ||
| prefix := "" | ||
| if includeProviderPrefix { | ||
| prefix = "gemini-" | ||
| } | ||
| return fmt.Sprintf("%s%s-%s.json", prefix, email, project) | ||
| } |
There was a problem hiding this comment.
Potential path traversal via unsanitized email in filename.
CredentialFileName uses email directly in the filename without sanitizing special characters (e.g., ../, /, \). A malicious email like ../../../etc/passwd could cause path traversal issues.
🛡️ Proposed fix to sanitize inputs
func CredentialFileName(email, projectID string, includeProviderPrefix bool) string {
email = strings.TrimSpace(email)
+ // Sanitize email to prevent path traversal
+ email = strings.ReplaceAll(email, "/", "_")
+ email = strings.ReplaceAll(email, "\\", "_")
+ email = strings.ReplaceAll(email, "..", "_")
project := strings.TrimSpace(projectID)
+ project = strings.ReplaceAll(project, "/", "_")
+ project = strings.ReplaceAll(project, "\\", "_")
+ project = strings.ReplaceAll(project, "..", "_")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func CredentialFileName(email, projectID string, includeProviderPrefix bool) string { | |
| email = strings.TrimSpace(email) | |
| project := strings.TrimSpace(projectID) | |
| if strings.EqualFold(project, "all") || strings.Contains(project, ",") { | |
| return fmt.Sprintf("gemini-%s-all.json", email) | |
| } | |
| prefix := "" | |
| if includeProviderPrefix { | |
| prefix = "gemini-" | |
| } | |
| return fmt.Sprintf("%s%s-%s.json", prefix, email, project) | |
| } | |
| func CredentialFileName(email, projectID string, includeProviderPrefix bool) string { | |
| email = strings.TrimSpace(email) | |
| // Sanitize email to prevent path traversal | |
| email = strings.ReplaceAll(email, "/", "_") | |
| email = strings.ReplaceAll(email, "\\", "_") | |
| email = strings.ReplaceAll(email, "..", "_") | |
| project := strings.TrimSpace(projectID) | |
| project = strings.ReplaceAll(project, "/", "_") | |
| project = strings.ReplaceAll(project, "\\", "_") | |
| project = strings.ReplaceAll(project, "..", "_") | |
| if strings.EqualFold(project, "all") || strings.Contains(project, ",") { | |
| return fmt.Sprintf("gemini-%s-all.json", email) | |
| } | |
| prefix := "" | |
| if includeProviderPrefix { | |
| prefix = "gemini-" | |
| } | |
| return fmt.Sprintf("%s%s-%s.json", prefix, email, project) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/auth/gemini/gemini_token.go` around lines 77 - 88,
CredentialFileName currently embeds the raw email (and projectID) into a
filename which allows path traversal characters; update CredentialFileName to
sanitize/normalize email and projectID before formatting the filename (e.g.,
strip or replace path separators "/" and "\" and any ".." sequences, or better:
apply a whitelist/regexp to allow only safe characters such as alphanumerics, @,
., -, _ and replace all others with an underscore). Ensure the sanitized values
are used in the fmt.Sprintf call, and provide a safe fallback (e.g., "unknown")
if sanitization results in an empty string so filenames are never able to
reference parent paths.
1 participant
Summary
phenotype-go-kitregistry package for auth token storage across all providersinternal/auth/{claude,copilot,gemini}/with provider-specific auth flows and token management (+1218 lines)Test plan
go build ./...succeeds with newgo.moddependencygo test ./...)🤖 Generated with Claude Code
Summary by CodeRabbit