feat(providers): add anthropic oauth#303
Open
vfeitoza wants to merge 6 commits intoENTERPILOT:mainfrom
Open
Conversation
Adds support for OAuth-authenticated Anthropic providers configured
with api_key: "oauth". Once authenticated via the dashboard, the
provider behaves identically to a static API key provider.
New packages:
- internal/oauthstore: persists OAuth tokens (SQLite, PostgreSQL, MongoDB)
- internal/oauth: OAuth 2.0 + PKCE flow, local callback server, Anthropic provider
- internal/oauthusage: fetches and caches rate-limit usage from Anthropic OAuth API
Provider changes:
- Anthropic provider detects api_key: "oauth" and switches to Bearer token auth
- Tokens are refreshed automatically before expiry (5-minute safety margin)
- ProviderConfig gains Name field; filterEmptyProviders allows oauth sentinel
- ProviderFactory gains SetOAuthStore() and passes the store to constructors
Admin API (6 new endpoints under /admin/api/v1/oauth):
- GET /oauth/providers — list OAuth-configured providers and status
- POST /oauth/start — initiate PKCE flow, returns authorization URL
- GET /oauth/callback — receive authorization code from provider
- POST /oauth/revoke — remove stored token
- GET /oauth/usage/:name — fetch 5-hour/7-day rate-limit windows
- GET /oauth/status/:name — token status for a single provider
Dashboard UI:
- OAuth page with provider cards showing status, account info, usage bars
- Authenticate/Re-authenticate/Revoke actions with popup-based OAuth flow
- OAuth link added to sidebar
Requests use the passthrough route:
POST /p/{provider_name}/v1/chat/completions
- Convert oauth.js from ES module to IIFE global pattern to match dashboard module factory system - Register OAuth page route and module in dashboard.js - Include oauth.js script in layout.html - Add dashboard-page-oauth template to index.html - Fix OAuth page template: correct define name, add Alpine x-if wrapper, use existing CSS classes (pagination-btn-*) - Replace oauthAuthenticating map with oauthActiveProvider string so Alpine tracks reactivity correctly and Authenticate button is clickable - Replace oauthRevoking map with oauthRevokingProvider string for the same reason, fixing the Disconnect button - Fix race condition in _oauthWaitForCallback: use resolved flag with 500ms grace period so popup-close does not cancel a successful auth - Fix OAuth state format to use hex encoding matching Anthropic's expected format - Compact OAuth page layout: inline account details, smaller fonts, reduced padding for multi-provider readability
- Add dual-mode callback flow: always starts a local CallbackServer (port 54545) and also returns a manual_auth_url for remote use. No configuration required — both modes are always available. - Remote mode uses platform.claude.com/oauth/code/callback as redirect URI (accepted by Anthropic's public client ID without registration). User copies the callback URL from the browser and pastes it into the dashboard's manual input field. - Add POST /admin/api/v1/oauth/callback-manual endpoint that accepts a full callback URL, a raw authorization code, or any combination with a state token from the active flow. - Separate redirectURI (used in AuthorizationURL) from exchangeURI (used in token exchange) to handle the console→platform redirect. - Refactor oauth.Provider interface to accept redirectURI string instead of callbackPort int, making it provider-agnostic for future providers. - Add oauth.LocalCallbackURI(port) helper for local mode. - Fix post-revoke requests: cancel request context with context.WithCancelCause when OAuth token is missing so the upstream HTTP call is aborted immediately instead of proceeding without auth. - Add docs/features/oauth-authentication.md — user-facing README - Add docs/dev/oauth-implementation-guide.md — implementation reference for adding OAuth to future providers (e.g. Codex)
- Fix utilization percentage: Anthropic API returns values as 0-100 (not 0-1 fraction); normalize by dividing by 100 when value > 1, matching the cligate reference implementation behavior - Force cache invalidation on every explicit usage refresh request so the dashboard always shows fresh data when the user clicks Refresh
Contributor
📝 WalkthroughWalkthroughAdds end-to-end OAuth 2.0 (PKCE) support for Anthropic: OAuth primitives, Anthropic client, local callback server, multi-backend token store (SQLite/Postgres/MongoDB), usage fetching/caching, admin API and dashboard UI, and provider wiring to use stored OAuth tokens for authenticated requests. ChangesOAuth 2.0 Authentication Implementation
Sequence DiagramsequenceDiagram
participant Dashboard as Admin Dashboard
participant AdminAPI as Admin API Handler
participant CallbackSvr as Local Callback Server
participant Anthropic as Anthropic OAuth Service
participant Store as Token Store
participant Provider as Anthropic Provider
participant LLM as Downstream LLM Request
rect rgba(100, 200, 255, 0.5)
note over Dashboard,AdminAPI: OAuth start & completion flow
Dashboard->>AdminAPI: POST /admin/api/v1/oauth/start (provider_name)
AdminAPI->>AdminAPI: Generate PKCE pair & state
AdminAPI->>CallbackSvr: Start local callback server on 127.0.0.1:PORT
AdminAPI-->>Dashboard: Return auth_url & callback port
Dashboard->>Anthropic: Open popup to auth_url
Anthropic->>CallbackSvr: Redirect with code + state
CallbackSvr-->>Dashboard: Post success message
Dashboard->>AdminAPI: Notify completion
AdminAPI->>Anthropic: POST /oauth/token (code, verifier)
Anthropic-->>AdminAPI: access_token, refresh_token, expires_in
AdminAPI->>Anthropic: GET /oauth/profile (Bearer token)
Anthropic-->>AdminAPI: Profile data
AdminAPI->>Store: Save token metadata
AdminAPI-->>Dashboard: Flow complete
end
rect rgba(100, 255, 100, 0.5)
note over Provider,LLM: OAuth-authenticated request flow
LLM->>Provider: Send request with provider configured as "oauth"
Provider->>Store: Get token for provider
Store-->>Provider: Token (may be expired)
alt expired and refresh available
Provider->>Anthropic: POST /oauth/token (refresh_token)
Anthropic-->>Provider: new access_token, possibly new refresh_token
Provider->>Store: Save updated token
end
Provider->>Anthropic: API request with Authorization: Bearer <token>
Anthropic-->>Provider: Response
Provider-->>LLM: Return LLM response
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested labels
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. ✨ Finishing Touches🧪 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 |
|
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
Greptile SummaryThis PR adds full OAuth 2.0 + PKCE authentication for Anthropic providers, with dual-mode callback (local loopback server on port 54545, or manual code-paste for remote deployments), token persistence across SQLite/PostgreSQL/MongoDB, automatic refresh, and a new admin dashboard page with usage progress bars. The Confidence Score: 3/5Not safe to merge as-is; the reflected XSS in the OAuth callback endpoint should be fixed before shipping. One confirmed P1 security issue (reflected XSS via unsanitized internal/admin/handler_oauth.go (XSS fix required) and internal/oauth/server.go (same pattern, lower exposure).
|
| Filename | Overview |
|---|---|
| internal/admin/handler_oauth.go | New 613-line OAuth admin handler; contains an XSS vulnerability where the error query parameter is embedded unsanitized into the HTML error page. |
| internal/oauth/anthropic.go | Anthropic OAuth provider implementation (PKCE, token exchange, refresh, profile fetch); contains dead private callbackURI wrapper. |
| internal/oauth/server.go | Loopback-only OAuth callback server; errorHTML also concatenates unsanitized input, but limited to 127.0.0.1 traffic. |
| internal/oauthstore/store.go | Token store interface and helpers; normalizeProviderName comment claims lowercase normalization that isn't implemented. |
| internal/oauthstore/store_sqlite.go | SQLite token store with schema creation; contains unused isSQLiteDuplicateColumnError dead code. |
| internal/oauthstore/store_postgresql.go | PostgreSQL token store; uses parameterized queries and UPSERT correctly. |
| internal/oauthstore/store_mongodb.go | MongoDB token store; upserts via ReplaceOne with correct index setup. |
| internal/oauthusage/usage.go | HTTP usage fetcher with 5-minute cache; custom contains/stringContains helpers duplicate strings.Contains. |
| internal/providers/anthropic/oauth.go | OAuth state holder and token refresh logic for the Anthropic provider; context cancellation on token failure is correct. |
| internal/providers/anthropic/anthropic.go | Anthropic provider updated to support OAuth header injection alongside static API key; logic separation is clean. |
| internal/app/app.go | App wiring extended to bootstrap OAuth store and pass it to the admin handler and provider factory; storage errors are non-fatal (warn + skip). |
| internal/providers/config.go | Provider config extended with Name field; filterEmptyProviders correctly passes OAuth sentinel through without treating it as a missing credential. |
Sequence Diagram
sequenceDiagram
actor Admin
participant Dashboard
participant OAuthHandler
participant AnthropicProvider
participant CallbackServer
participant OAuthStore
participant AnthropicAPI
Admin->>Dashboard: Click Authenticate
Dashboard->>OAuthHandler: POST /oauth/start
OAuthHandler->>OAuthHandler: NewPKCEPair() + NewState()
OAuthHandler->>CallbackServer: TryCallbackPorts(54545)
OAuthHandler-->>Dashboard: auth_url, manual_auth_url, state
OAuthHandler->>CallbackServer: go waitForCallback(state)
alt Local flow
Admin->>AnthropicAPI: Visit auth_url
AnthropicAPI-->>CallbackServer: GET /callback?code=X&state=Y
CallbackServer-->>OAuthHandler: CallbackResult
else Manual flow
Admin->>AnthropicAPI: Visit manual_auth_url
AnthropicAPI-->>Admin: Redirect to platform.claude.com
Admin->>OAuthHandler: POST /oauth/callback-manual
end
OAuthHandler->>AnthropicAPI: ExchangeCode(code, verifier, state)
AnthropicAPI-->>OAuthHandler: TokenResponse
OAuthHandler->>AnthropicAPI: FetchProfile(access_token)
AnthropicAPI-->>OAuthHandler: Profile
OAuthHandler->>OAuthStore: Save(token)
Note over OAuthHandler: Provider now active
Admin->>AnthropicProvider: AI request
AnthropicProvider->>OAuthStore: Get(providerName)
alt Token fresh
OAuthStore-->>AnthropicProvider: token
else Token expired
AnthropicProvider->>AnthropicAPI: RefreshToken
AnthropicAPI-->>AnthropicProvider: new TokenResponse
AnthropicProvider->>OAuthStore: Save(updated token)
end
AnthropicProvider->>AnthropicAPI: Request with Bearer token
Comments Outside Diff (4)
-
internal/oauthstore/store.go, line 1280-1282 (link)normalizeProviderNamecomment claims lowercasing that isn't implementedThe function comment says the name is "trimmed and lowercased for consistent storage and lookup," but the implementation only trims whitespace. A provider configured as
"MyProvider"and looked up as"myprovider"would fail to match. Either drop the lowercase claim from the comment or addstrings.ToLower:func normalizeProviderName(name string) string { return strings.ToLower(strings.TrimSpace(name)) }
-
internal/oauthusage/usage.go, line 938-954 (link)Custom
contains/stringContainsre-implementstrings.ContainsThe
containsandstringContainshelpers replicate the behavior of the standard library'sstrings.Containswithout adding any value. Per the project's style guide (CLAUDE.md), idiomatic Go is preferred. Replace both call sites withstrings.Contains:func isUnsupportedBody(body []byte) bool { lower := strings.ToLower(string(body)) return len(lower) > 0 && (strings.Contains(lower, "not supported") || strings.Contains(lower, "oauth authentication is currently not supported")) }
Context Used: CLAUDE.md (source)
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
-
internal/oauth/anthropic.go, line 889-892 (link)callbackURIfunctioncallbackURIis a private function that simply delegates to the publicLocalCallbackURI, but it is never called anywhere in the codebase. It can be removed to keep the file clean. -
internal/oauthstore/store_sqlite.go, line 708-714 (link)isSQLiteDuplicateColumnErrorhelperisSQLiteDuplicateColumnErroris defined at the bottom of this file but is never called. Since the schema usesCREATE TABLE IF NOT EXISTS/CREATE INDEX IF NOT EXISTS, there is no code path that would need this helper. It should be removed to avoid dead-code accumulation.
Reviews (1): Last reviewed commit: "fix(oauth): fix usage utilization displa..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+214
to
+216
|
|
||
| h.flowMu.Lock() | ||
| flow, ok := h.flows[state] |
There was a problem hiding this comment.
error query parameter in HTML response
c.QueryParam("error") is user-controlled input that is string-concatenated directly into the HTML response body without HTML escaping. An attacker can craft a URL like /admin/api/v1/oauth/callback?error=<script>alert(document.cookie)</script> and, if an authenticated admin's browser follows that URL (e.g. via phishing), arbitrary JS executes in the admin dashboard's origin.
Use html.EscapeString before embedding the value:
import "html"
// …
return c.HTML(http.StatusBadRequest, oauthErrorHTML(html.EscapeString(errParam)))The same fix is needed in internal/oauth/server.go's handleCallback, though exposure there is limited to the loopback-only server.
![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: 21
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/dev/oauth-implementation-guide.md`:
- Line 120: Remove the hardcoded personal filesystem path
"/Users/vfeitoza/Projetos/cligate/src/claude-oauth.js" from the docs entry and
replace it with either a public upstream URL to the referenced reference
implementation or a concise descriptive label (e.g., "reference implementation:
claude-oauth.js in upstream repo") so the documentation does not expose local
paths and is usable by other contributors.
In `@docs/features/oauth-authentication.md`:
- Around line 44-46: Add a language identifier to the fenced code block
containing the route snippet "POST /p/{provider_name}/v1/chat/completions" so
the fence reads as an HTTP snippet (e.g., ```http). Update the existing
triple-backtick fence that wraps that route to include "http" to satisfy
markdown linting and improve readability.
In `@internal/admin/dashboard/templates/page-oauth.html`:
- Around line 97-101: Add an accessible label for the manual callback input so
screen-reader users can complete the flow: give the input a unique id (e.g.,
id="oauth-manual-callback"), add a visible or visually-hidden <label> tied to
that id with descriptive text like "OAuth callback URL" or, if a visible label
is undesirable, add aria-label="OAuth callback URL" or aria-labelledby
referencing a nearby label element; ensure the input still binds to
x-model="oauthManualInput" and the `@keydown.enter` handler
oauthSubmitManual(provider.provider_name) remains unchanged.
In `@internal/admin/handler_oauth.go`:
- Around line 518-522: The log entry currently includes the user's email
(token.AccountEmail) which writes a persistent identifier to logs; update the
slog.Info call in the oauth flow (the call referencing flow.providerName,
token.AccountEmail, token.SubscriptionType) to omit token.AccountEmail (or
replace it with a non-identifying value such as a redacted string or a hash of
the email) so only provider and outcome/subscription are logged; ensure any
replacement is deterministic (e.g., SHA256 hex) if you need correlation but do
not store the raw email in logs.
- Around line 461-466: The Wait() error path currently removes the shared flow
entry (delete(h.flows, state)) which breaks the manual/paste flow; change the
handler in handler_oauth.go so that on err != nil you log the warning as before
but do NOT delete h.flows[state] — only release h.flowMu and return; rely on the
existing cleanExpiredFlows() TTL logic to retire abandoned manual flows. Ensure
you keep the same lock/unlock sequence around h.flowMu and leave the shared
state in h.flows intact when Wait() times out.
- Around line 354-357: Remove the unconditional cache invalidation so normal
GETs don't bust the 5-minute cache: in oauthLoadProviders (where
h.usageFetcher.Invalidate(providerName) is currently called unconditionally),
only call h.usageFetcher.Invalidate(providerName) when the incoming request
contains an explicit refresh flag (e.g. query param "refresh=1") sent by the
dashboard Refresh button; ensure the manual refresh path triggers the Invalidate
and leave regular page load paths untouched so oauthLoadProviders can rely on
the existing caching behavior.
- Around line 593-610: The oauthErrorHTML function currently concatenates
unescaped errMsg into the HTML, creating a reflected XSS sink; update
oauthErrorHTML (or replace it) to render a proper HTML template and escape
errMsg before embedding (e.g., use html/template to define the HTML and pass
errMsg as a data field, or call html.EscapeString on errMsg before
concatenation) and add the corresponding html/template (or html) import; ensure
the error string is never interpolated raw into the template string in
oauthErrorHTML.
In `@internal/app/app.go`:
- Around line 102-123: The oauthStore/tmpStorage created in App.New (via
storage.New and oauthstore.NewFromStorage) are not tracked or closed causing a
leak; add fields to the App struct (e.g., oauthStore and oauthTmpStorage or
similar), assign oauthStore and tmpStorage immediately after successful creation
and cfg.Factory.SetOAuthStore(oauthStore), and ensure both are closed in
App.Shutdown() (after providers are closed); also update all early-return/error
paths in App.New() to close these resources on error so tmpStorage and
oauthStore are always cleaned up if initialization aborts.
In `@internal/oauth/anthropic.go`:
- Around line 264-267: Remove the unused helper callbackURI which simply
forwards to LocalCallbackURI: delete the callbackURI function definition so the
linter stops flagging it and update any callers to reference LocalCallbackURI
directly (search for callbackURI and replace with LocalCallbackURI where used).
In `@internal/oauth/oauth_test.go`:
- Around line 74-80: The json.NewEncoder(w).Encode(...) calls in the oauth test
handlers currently ignore returned errors (e.g., the call at the block that
writes access_token "access-abc"), which trips errcheck; update each encoder
call (the ones around the access_token block and the other occurrences
referenced) to capture the error (err := json.NewEncoder(w).Encode(...)) and
fail the test on error (for example using t.Fatalf/t.Fatal or require.NoError)
so the tests properly handle encoding failures; apply this same pattern to the
other Encode calls in oauth_test.go.
- Around line 35-42: Extend TestNewState to assert the encoding contract of
oauth.NewState in addition to uniqueness: after generating s1 and s2 (via
oauth.NewState), verify each is valid lowercase hex (e.g., matches a hex regexp
or successfully hex.DecodeString) and has the expected length (reference
oauth.NewState and any documented byte/hex length); keep the existing uniqueness
checks but add these encoding assertions so regressions to non-hex formats fail.
In `@internal/oauth/server.go`:
- Around line 171-191: The errorHTML function interpolates errMsg directly into
the HTML (XSS risk); update errorHTML to HTML-escape the errMsg before embedding
(e.g., use html.EscapeString) or rebuild the response using html/template so the
value is auto-escaped; ensure the change is applied to the errorHTML function
(and any callers that pass q.Get("error")) so user-supplied query parameters
cannot inject HTML/JS.
In `@internal/oauthstore/store_postgresql.go`:
- Around line 69-75: Remove the pre-read s.Get call and the existing.CreatedAt
logic: simply set createdAt := time.Now().UTC() (or reuse now :=
time.Now().UTC(); createdAt := now) and remove the s.Get(ctx, name) block and
error handling; the DB upsert conflict clause already preserves created_at so
the extra read and discarded lookup errors are unnecessary. Ensure you only
reference createdAt where the insert/upsert uses it and delete the s.Get usage.
In `@internal/oauthstore/store_sqlite.go`:
- Around line 211-217: The helper isSQLiteDuplicateColumnError is unused and
flagged by linters; either delete this function from
internal/oauthstore/store_sqlite.go to remove the dead code, or wire it into
relevant SQLite error handling paths by calling
isSQLiteDuplicateColumnError(err) where SQL DDL/ALTER/CREATE column errors are
checked (replace existing string-based checks around DB.Exec/tx.Exec error
handling with this helper) so the function is actually referenced.
In `@internal/oauthusage/usage.go`:
- Around line 187-202: The response body is currently read with
io.LimitReader(..., 4096) and the error is ignored, which can silently truncate
valid JSON and hide transport errors; update the body read in the function that
calls io.ReadAll (the block that returns parseUsageResponse, handles
resp.StatusCode and calls isUnsupportedBody/UnsupportedError) to use a larger
limit (e.g., 64*1024) and capture the read error (do not use the blank
identifier). On non-OK or Unauthorized paths include/return the read error (or
at least surface it in the fmt.Errorf message) so callers see transport/IO
failures, and keep using truncate(string(body), 200) for logs but avoid
truncating the actual bytes passed to parseUsageResponse.
- Around line 213-216: The isUnsupportedError function should use errors.As to
detect *UnsupportedError even when wrapped; update isUnsupportedError(err error)
to declare a var target *UnsupportedError and return errors.As(err, &target),
and add an import for the standard errors package if not already present so
wrapped errors are matched correctly.
- Around line 218-234: isUnsupportedBody currently pretends to lowercase the
payload but doesn't and uses custom contains/stringContains; make matching
actually case-insensitive by converting body to lower with strings.ToLower in
isUnsupportedBody, replace the custom contains/stringContains calls with
strings.Contains, and remove those helper functions; also ensure the package
imports "strings" and update isUnsupportedBody to call strings.Contains on the
lowercased string.
In `@internal/providers/anthropic/anthropic.go`:
- Around line 64-70: The constructor path that checks
isOAuthAPIKey(providerCfg.APIKey) currently silently leaves p.oauth nil when
opts.OAuthStore is nil, causing setHeaders to send the literal "oauth" key;
update New (the anthropic provider constructor) to detect this misconfiguration
and either return an error or at minimum log a clear warning and set a safe
state: if isOAuthAPIKey(providerCfg.APIKey) && opts.OAuthStore == nil then
log/processLogger.Warn (or return an error from New) referencing
providerCfg.Name and that OAuthStore is missing so p.oauth is not initialized,
so callers and operators see an actionable message instead of 401s from
setHeaders.
In `@internal/providers/anthropic/oauth.go`:
- Around line 37-45: The code compares the returned error to the sentinel
oauthstore.ErrNotFound with ==; change that to use errors.Is(err,
oauthstore.ErrNotFound) in the block handling the result of s.store.Get(ctx,
s.providerName) so wrapped errors are detected correctly, and ensure the package
imports "errors" if not already present; keep the existing error messages and
control flow (the branch that returns the dashboard-authentication message when
the error is ErrNotFound and the generic fmt.Errorf for other errors).
In `@internal/providers/config_test.go`:
- Around line 64-67: The test TestBuildProviderConfig_InheritsGlobal now passes
a provider name but lacks assertions for the new ProviderConfig.Name and doesn't
cover the special "oauth" sentinel keep-path behavior; update this test (or add
a new focused test) to assert that the returned config from
buildProviderConfig("test", raw, globalResilience) has ProviderConfig.Name ==
"test", and add a separate test that exercises the filtering logic using a
RawProviderConfig that includes the "oauth" sentinel to confirm the keep-path
behavior is respected by buildProviderConfig (reference symbols:
TestBuildProviderConfig_InheritsGlobal, buildProviderConfig,
ProviderConfig.Name, and the "oauth" sentinel).
In `@internal/providers/config.go`:
- Around line 425-429: The current code allows any provider with p.APIKey ==
"oauth" to be preserved (result[name] = p), which can let non-OAuth-capable
providers slip through; change the logic to only pass through the sentinel for
providers whose type supports OAuth by adding a provider-type check before
assigning result[name] = p — e.g., verify p.Type (or the concrete provider
identifier used in your config) is in an allow-list of OAuth-capable providers
(such as "anthropic", etc.) and only then continue; otherwise treat the sentinel
as invalid and skip or return an error.
🪄 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: 37924008-8bef-49ae-95e7-a3fe7e04be44
📒 Files selected for processing (31)
config/config.example.yamldocs/dev/oauth-implementation-guide.mddocs/features/oauth-authentication.mdinternal/admin/dashboard/static/js/dashboard.jsinternal/admin/dashboard/static/js/modules/oauth.jsinternal/admin/dashboard/templates/index.htmlinternal/admin/dashboard/templates/layout.htmlinternal/admin/dashboard/templates/page-oauth.htmlinternal/admin/dashboard/templates/sidebar.htmlinternal/admin/handler_oauth.gointernal/admin/routes.gointernal/app/app.gointernal/oauth/anthropic.gointernal/oauth/export_test.gointernal/oauth/oauth.gointernal/oauth/oauth_test.gointernal/oauth/server.gointernal/oauthstore/factory.gointernal/oauthstore/store.gointernal/oauthstore/store_mongodb.gointernal/oauthstore/store_postgresql.gointernal/oauthstore/store_sqlite.gointernal/oauthstore/store_test.gointernal/oauthusage/usage.gointernal/providers/anthropic/anthropic.gointernal/providers/anthropic/oauth.gointernal/providers/config.gointernal/providers/config_test.gointernal/providers/factory.gointernal/providers/provider_status.gointernal/server/http.go
| "&code_challenge_method=S256" | ||
| ``` | ||
|
|
||
| Reference implementation: `/Users/vfeitoza/Projetos/cligate/src/claude-oauth.js` |
Contributor
There was a problem hiding this comment.
Remove personal local filesystem path from shared documentation.
Line 120 contains a hardcoded absolute path from the author's machine (/Users/vfeitoza/Projetos/cligate/src/claude-oauth.js). This path is unreachable for any other contributor and leaks the developer's local directory structure. Replace it with either a URL to the upstream reference (if public) or a short description of what was referenced.
✏️ Proposed fix
-Reference implementation: `/Users/vfeitoza/Projetos/cligate/src/claude-oauth.js`
+<!-- Reference was an upstream JavaScript OAuth implementation; see Anthropic OAuth docs for details. -->📝 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.
Suggested change
| Reference implementation: `/Users/vfeitoza/Projetos/cligate/src/claude-oauth.js` | |
| <!-- Reference was an upstream JavaScript OAuth implementation; see Anthropic OAuth docs for details. --> |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/dev/oauth-implementation-guide.md` at line 120, Remove the hardcoded
personal filesystem path "/Users/vfeitoza/Projetos/cligate/src/claude-oauth.js"
from the docs entry and replace it with either a public upstream URL to the
referenced reference implementation or a concise descriptive label (e.g.,
"reference implementation: claude-oauth.js in upstream repo") so the
documentation does not expose local paths and is usable by other contributors.
Comment on lines
+44
to
+46
| ``` | ||
| POST /p/{provider_name}/v1/chat/completions | ||
| ``` |
Contributor
There was a problem hiding this comment.
Specify a language for the fenced route snippet.
At Line 44, the fence has no language identifier, which triggers markdown lint and reduces readability.
Proposed fix
-```
+```http
POST /p/{provider_name}/v1/chat/completions</details>
<!-- suggestion_start -->
<details>
<summary>📝 Committable suggestion</summary>
> ‼️ **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.
```suggestion
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 44-44: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/features/oauth-authentication.md` around lines 44 - 46, Add a language
identifier to the fenced code block containing the route snippet "POST
/p/{provider_name}/v1/chat/completions" so the fence reads as an HTTP snippet
(e.g., ```http). Update the existing triple-backtick fence that wraps that route
to include "http" to satisfy markdown linting and improve readability.
Comment on lines
+97
to
+101
| <input type="text" | ||
| x-model="oauthManualInput" | ||
| placeholder="https://console.anthropic.com/oauth/code/callback?code=...&state=..." | ||
| style="flex:1;font-size:.78rem;padding:.3rem .5rem;border:1px solid var(--border);border-radius:4px;background:var(--bg);color:var(--text)" | ||
| @keydown.enter="oauthSubmitManual(provider.provider_name)" /> |
Contributor
There was a problem hiding this comment.
Add an accessible label for the manual callback input.
This field is the only way to finish the remote flow, so leaving it unlabeled makes that path effectively unusable for screen-reader users.
Suggested fix
<input type="text"
+ aria-label="OAuth callback URL"
x-model="oauthManualInput"
placeholder="https://console.anthropic.com/oauth/code/callback?code=...&state=..."
style="flex:1;font-size:.78rem;padding:.3rem .5rem;border:1px solid var(--border);border-radius:4px;background:var(--bg);color:var(--text)"
`@keydown.enter`="oauthSubmitManual(provider.provider_name)" />📝 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.
Suggested change
| <input type="text" | |
| x-model="oauthManualInput" | |
| placeholder="https://console.anthropic.com/oauth/code/callback?code=...&state=..." | |
| style="flex:1;font-size:.78rem;padding:.3rem .5rem;border:1px solid var(--border);border-radius:4px;background:var(--bg);color:var(--text)" | |
| @keydown.enter="oauthSubmitManual(provider.provider_name)" /> | |
| <input type="text" | |
| aria-label="OAuth callback URL" | |
| x-model="oauthManualInput" | |
| placeholder="https://console.anthropic.com/oauth/code/callback?code=...&state=..." | |
| style="flex:1;font-size:.78rem;padding:.3rem .5rem;border:1px solid var(--border);border-radius:4px;background:var(--bg);color:var(--text)" | |
| `@keydown.enter`="oauthSubmitManual(provider.provider_name)" /> |
🧰 Tools
🪛 HTMLHint (1.9.2)
[warning] 97-97: No matching [ label ] tag found.
(input-requires-label)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/admin/dashboard/templates/page-oauth.html` around lines 97 - 101,
Add an accessible label for the manual callback input so screen-reader users can
complete the flow: give the input a unique id (e.g.,
id="oauth-manual-callback"), add a visible or visually-hidden <label> tied to
that id with descriptive text like "OAuth callback URL" or, if a visible label
is undesirable, add aria-label="OAuth callback URL" or aria-labelledby
referencing a nearby label element; ensure the input still binds to
x-model="oauthManualInput" and the `@keydown.enter` handler
oauthSubmitManual(provider.provider_name) remains unchanged.
Comment on lines
+354
to
+357
| // Invalidate cache so every explicit refresh fetches fresh data. | ||
| if h.usageFetcher != nil { | ||
| h.usageFetcher.Invalidate(providerName) | ||
| } |
Contributor
There was a problem hiding this comment.
Don't invalidate the usage cache on every GET.
oauthLoadProviders() automatically kicks off usage loads for authenticated providers, so this unconditional Invalidate turns normal page loads into uncached upstream requests and defeats the 5-minute cache introduced by this PR. Make cache busting explicit on the manual refresh path only.
Suggested direction
func (h *OAuthHandler) GetOAuthUsage(c *echo.Context) error {
+ forceRefresh := c.QueryParam("refresh") == "1"
providerName := strings.TrimSpace(c.Param("provider_name"))
@@
- if h.usageFetcher != nil {
+ if forceRefresh && h.usageFetcher != nil {
h.usageFetcher.Invalidate(providerName)
}Then have the dashboard send refresh=1 only from the explicit Refresh button.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/admin/handler_oauth.go` around lines 354 - 357, Remove the
unconditional cache invalidation so normal GETs don't bust the 5-minute cache:
in oauthLoadProviders (where h.usageFetcher.Invalidate(providerName) is
currently called unconditionally), only call
h.usageFetcher.Invalidate(providerName) when the incoming request contains an
explicit refresh flag (e.g. query param "refresh=1") sent by the dashboard
Refresh button; ensure the manual refresh path triggers the Invalidate and leave
regular page load paths untouched so oauthLoadProviders can rely on the existing
caching behavior.
Comment on lines
+461
to
+466
| if err != nil { | ||
| slog.Warn("oauth: callback wait failed", "provider", flow.providerName, "error", err) | ||
| h.flowMu.Lock() | ||
| delete(h.flows, state) | ||
| h.flowMu.Unlock() | ||
| return |
Contributor
There was a problem hiding this comment.
Keep the shared flow state alive when the localhost callback times out.
The same state backs both the local popup path and the manual paste flow. Deleting it here on Wait() failure makes remote/manual auth fail with “invalid or expired OAuth state” if the user takes longer than the local wait window to finish authorization and paste the callback URL.
Suggested fix
result, err := flow.server.Wait(ctx)
if err != nil {
slog.Warn("oauth: callback wait failed", "provider", flow.providerName, "error", err)
- h.flowMu.Lock()
- delete(h.flows, state)
- h.flowMu.Unlock()
return
}Let cleanExpiredFlows() retire abandoned manual flows on the normal TTL instead.
📝 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.
Suggested change
| if err != nil { | |
| slog.Warn("oauth: callback wait failed", "provider", flow.providerName, "error", err) | |
| h.flowMu.Lock() | |
| delete(h.flows, state) | |
| h.flowMu.Unlock() | |
| return | |
| if err != nil { | |
| slog.Warn("oauth: callback wait failed", "provider", flow.providerName, "error", err) | |
| return |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/admin/handler_oauth.go` around lines 461 - 466, The Wait() error
path currently removes the shared flow entry (delete(h.flows, state)) which
breaks the manual/paste flow; change the handler in handler_oauth.go so that on
err != nil you log the warning as before but do NOT delete h.flows[state] — only
release h.flowMu and return; rely on the existing cleanExpiredFlows() TTL logic
to retire abandoned manual flows. Ensure you keep the same lock/unlock sequence
around h.flowMu and leave the shared state in h.flows intact when Wait() times
out.
Comment on lines
+218
to
+234
| func isUnsupportedBody(body []byte) bool { | ||
| lower := string(body) | ||
| return len(lower) > 0 && (contains(lower, "not supported") || contains(lower, "oauth authentication is currently not supported")) | ||
| } | ||
|
|
||
| func contains(s, substr string) bool { | ||
| return len(s) >= len(substr) && (s == substr || len(s) > 0 && stringContains(s, substr)) | ||
| } | ||
|
|
||
| func stringContains(s, substr string) bool { | ||
| for i := 0; i <= len(s)-len(substr); i++ { | ||
| if s[i:i+len(substr)] == substr { | ||
| return true | ||
| } | ||
| } | ||
| return false | ||
| } |
Contributor
There was a problem hiding this comment.
Case-insensitivity not actually applied; replace hand-rolled string scan with strings.Contains.
isUnsupportedBody names the local lower, but never lowercases the body, so matching is in fact case-sensitive — variants like "Not Supported" from the API will fail detection. The custom contains/stringContains also re-implements strings.Contains non-idiomatically.
♻️ Proposed fix
+import (
+ ...
+ "strings"
+ ...
+)
+
func isUnsupportedBody(body []byte) bool {
- lower := string(body)
- return len(lower) > 0 && (contains(lower, "not supported") || contains(lower, "oauth authentication is currently not supported"))
-}
-
-func contains(s, substr string) bool {
- return len(s) >= len(substr) && (s == substr || len(s) > 0 && stringContains(s, substr))
-}
-
-func stringContains(s, substr string) bool {
- for i := 0; i <= len(s)-len(substr); i++ {
- if s[i:i+len(substr)] == substr {
- return true
- }
- }
- return false
+ lower := strings.ToLower(string(body))
+ return lower != "" && (strings.Contains(lower, "not supported") ||
+ strings.Contains(lower, "oauth authentication is currently not supported"))
}As per coding guidelines: "use idiomatic Go ... avoid premature optimization."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/oauthusage/usage.go` around lines 218 - 234, isUnsupportedBody
currently pretends to lowercase the payload but doesn't and uses custom
contains/stringContains; make matching actually case-insensitive by converting
body to lower with strings.ToLower in isUnsupportedBody, replace the custom
contains/stringContains calls with strings.Contains, and remove those helper
functions; also ensure the package imports "strings" and update
isUnsupportedBody to call strings.Contains on the lowercased string.
Comment on lines
+64
to
+70
| if isOAuthAPIKey(providerCfg.APIKey) && opts.OAuthStore != nil { | ||
| p.oauth = &oauthState{ | ||
| store: opts.OAuthStore, | ||
| providerName: providerCfg.Name, | ||
| oauthProv: oauth.NewAnthropicProvider(), | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Silent fallback when api_key == "oauth" but no OAuthStore is configured.
If isOAuthAPIKey(providerCfg.APIKey) returns true but opts.OAuthStore is nil (e.g., OAuth subsystem failed to initialize, or wiring regression in the factory), p.oauth stays nil and setHeaders will then send the literal string "oauth" as x-api-key — every Anthropic request will fail with 401 and the operator gets no actionable signal pointing to the root cause.
Recommend either (a) failing fast in New for this case, or (b) at minimum logging a warning so the misconfiguration is visible.
🛡️ Proposed fix (warn loudly; or return an error if `New` is allowed to fail)
if isOAuthAPIKey(providerCfg.APIKey) && opts.OAuthStore != nil {
p.oauth = &oauthState{
store: opts.OAuthStore,
providerName: providerCfg.Name,
oauthProv: oauth.NewAnthropicProvider(),
}
+ } else if isOAuthAPIKey(providerCfg.APIKey) {
+ slog.Error("anthropic provider configured with api_key=\"oauth\" but no OAuthStore is available; requests will fail with 401",
+ "provider", providerCfg.Name)
}As per coding guidelines: "Keep the implementation explicit and maintainable rather than relying on clever abstractions."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/providers/anthropic/anthropic.go` around lines 64 - 70, The
constructor path that checks isOAuthAPIKey(providerCfg.APIKey) currently
silently leaves p.oauth nil when opts.OAuthStore is nil, causing setHeaders to
send the literal "oauth" key; update New (the anthropic provider constructor) to
detect this misconfiguration and either return an error or at minimum log a
clear warning and set a safe state: if isOAuthAPIKey(providerCfg.APIKey) &&
opts.OAuthStore == nil then log/processLogger.Warn (or return an error from New)
referencing providerCfg.Name and that OAuthStore is missing so p.oauth is not
initialized, so callers and operators see an actionable message instead of 401s
from setHeaders.
Comment on lines
+37
to
+45
| token, err := s.store.Get(ctx, s.providerName) | ||
| if err != nil { | ||
| if err == oauthstore.ErrNotFound { | ||
| return "", fmt.Errorf( | ||
| "provider %q requires OAuth authentication — visit the dashboard to authenticate", | ||
| s.providerName, | ||
| ) | ||
| } | ||
| return "", fmt.Errorf("oauth: failed to load token for %q: %w", s.providerName, err) |
Contributor
There was a problem hiding this comment.
Use errors.Is instead of == for sentinel error comparison.
If any store backend wraps ErrNotFound (e.g., fmt.Errorf("sqlite: %w", ErrNotFound)), the equality check silently falls through to the generic error path, discarding the actionable "visit the dashboard" message. errors.Is handles the wrapped case correctly.
🛠️ Proposed fix
+import "errors"
...
- if err == oauthstore.ErrNotFound {
+ if errors.Is(err, oauthstore.ErrNotFound) {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/providers/anthropic/oauth.go` around lines 37 - 45, The code
compares the returned error to the sentinel oauthstore.ErrNotFound with ==;
change that to use errors.Is(err, oauthstore.ErrNotFound) in the block handling
the result of s.store.Get(ctx, s.providerName) so wrapped errors are detected
correctly, and ensure the package imports "errors" if not already present; keep
the existing error messages and control flow (the branch that returns the
dashboard-authentication message when the error is ErrNotFound and the generic
fmt.Errorf for other errors).
Comment on lines
64
to
67
| func TestBuildProviderConfig_InheritsGlobal(t *testing.T) { | ||
| raw := config.RawProviderConfig{Type: "openai", APIKey: "sk-test"} | ||
| got := buildProviderConfig(raw, globalResilience) | ||
| got := buildProviderConfig("test", raw, globalResilience) | ||
|
|
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Add assertions for the new behavior, not just the new signature.
At Line 66, tests were updated to pass a provider name but do not verify ProviderConfig.Name, and there’s no focused test for the new "oauth" sentinel keep-path in filtering.
As per coding guidelines: "**/*_test.go: Add or update tests for behavior changes."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/providers/config_test.go` around lines 64 - 67, The test
TestBuildProviderConfig_InheritsGlobal now passes a provider name but lacks
assertions for the new ProviderConfig.Name and doesn't cover the special "oauth"
sentinel keep-path behavior; update this test (or add a new focused test) to
assert that the returned config from buildProviderConfig("test", raw,
globalResilience) has ProviderConfig.Name == "test", and add a separate test
that exercises the filtering logic using a RawProviderConfig that includes the
"oauth" sentinel to confirm the keep-path behavior is respected by
buildProviderConfig (reference symbols: TestBuildProviderConfig_InheritsGlobal,
buildProviderConfig, ProviderConfig.Name, and the "oauth" sentinel).
Comment on lines
+425
to
+429
| // Allow OAuth sentinel value through — token is stored at runtime. | ||
| if strings.EqualFold(strings.TrimSpace(p.APIKey), "oauth") { | ||
| result[name] = p | ||
| continue | ||
| } |
Contributor
There was a problem hiding this comment.
Restrict OAuth sentinel pass-through to OAuth-capable provider types.
At Line 426, any provider with api_key: "oauth" is preserved. That can keep unsupported types (e.g., non-Anthropic) and fail later at request time instead of being filtered early.
Proposed fix
- // Allow OAuth sentinel value through — token is stored at runtime.
- if strings.EqualFold(strings.TrimSpace(p.APIKey), "oauth") {
+ // Allow OAuth sentinel only for provider types that support OAuth runtime tokens.
+ if strings.EqualFold(strings.TrimSpace(p.APIKey), "oauth") &&
+ strings.EqualFold(strings.TrimSpace(p.Type), "anthropic") {
result[name] = p
continue
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/providers/config.go` around lines 425 - 429, The current code allows
any provider with p.APIKey == "oauth" to be preserved (result[name] = p), which
can let non-OAuth-capable providers slip through; change the logic to only pass
through the sentinel for providers whose type supports OAuth by adding a
provider-type check before assigning result[name] = p — e.g., verify p.Type (or
the concrete provider identifier used in your config) is in an allow-list of
OAuth-capable providers (such as "anthropic", etc.) and only then continue;
otherwise treat the sentinel as invalid and skip or return an error.
- Remove unused callbackURI wrapper in oauth/anthropic.go - Remove unused isSQLiteDuplicateColumnError in oauthstore/store_sqlite.go - Extract scanTokenRow helper in oauthstore/store.go to eliminate duplicate scan logic between SQLite and PostgreSQL implementations - Check error return values of json.Encoder.Encode and w.Write in tests
vfeitoza
changed the title
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
internal/oauthstore/store_postgresql.go (1)
69-75: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winRemove the pre-read in
Save; upsert already preservescreated_at.The
s.Get()pre-read is redundant here and unexpected lookup errors are ignored. The currentON CONFLICTclause does not updatecreated_at, so a single upsert withcreated_at = nowon insert is enough.Proposed simplification
now := time.Now().UTC() - createdAt := now - existing, err := s.Get(ctx, name) - if err == nil { - createdAt = existing.CreatedAt - } - - _, err = s.pool.Exec(ctx, ` + _, err := s.pool.Exec(ctx, ` INSERT INTO oauth_tokens (provider_name, provider_type, access_token, refresh_token, expires_at, scopes, account_email, account_id, display_name, subscription_type, created_at, updated_at) @@ - createdAt.Unix(), + now.Unix(), now.Unix(), )As per coding guidelines: "Keep files small and follow KISS (Keep It Simple, Stupid) principles."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/oauthstore/store_postgresql.go` around lines 69 - 75, In Save, remove the pre-read call to s.Get and the existing/createdAt merging logic; instead set createdAt := time.Now().UTC() and rely on your INSERT ... ON CONFLICT ... DO UPDATE clause to preserve the original created_at on conflict (i.e., do not attempt to read or reuse existing.CreatedAt), and delete any error-ignoring branches around s.Get so the function performs a single upsert without the redundant lookup.internal/oauth/oauth_test.go (1)
158-175:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
errcheckstill flags thisEncodecall.The previous round handled the
_ = json.NewEncoder(w).Encode(...)pattern at lines 74/116/136 but missed this one. The lint job will stay red until line 163 follows the same pattern.🛠️ Proposed fix
- w.Header().Set("Content-Type", "application/json") - json.NewEncoder(w).Encode(map[string]any{ + w.Header().Set("Content-Type", "application/json") + _ = json.NewEncoder(w).Encode(map[string]any{ "account": map[string]any{🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/oauth/oauth_test.go` around lines 158 - 175, In TestAnthropicProvider_FetchProfile update the json.Encode call inside the httptest handler to handle its returned error (either assign it to the blank identifier like `_ = json.NewEncoder(w).Encode(...)` to silence errcheck or explicitly check and call t.Fatal on error) so errcheck stops flagging the json.NewEncoder(w).Encode(...) invocation in the TestAnthropicProvider_FetchProfile handler.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/oauthstore/store_sqlite.go`:
- Around line 64-70: In Save, remove the pre-read via s.Get and the existing/err
handling; always set createdAt := time.Now().UTC() for the INSERT values and
rely on the UPSERT/ON CONFLICT clause to preserve an existing created_at (i.e.
don't include created_at in the UPDATE part of the upsert). Update the Save
function (symbol Save in internal/oauthstore/store_sqlite.go) to drop the Get
call and related existing variable, keep createdAt set to now, and ensure the
upsert statement does not overwrite created_at on conflict.
In `@internal/oauthstore/store.go`:
- Around line 95-99: The comment says normalizeProviderName should trim and
lowercase but the implementation only trims; update the normalizeProviderName
function to also lowercase the input (e.g., use strings.ToLower on the trimmed
result) so storage/lookups are case-insensitive, and ensure the function comment
remains accurate; referenced symbol: normalizeProviderName(name string).
---
Duplicate comments:
In `@internal/oauth/oauth_test.go`:
- Around line 158-175: In TestAnthropicProvider_FetchProfile update the
json.Encode call inside the httptest handler to handle its returned error
(either assign it to the blank identifier like `_ =
json.NewEncoder(w).Encode(...)` to silence errcheck or explicitly check and call
t.Fatal on error) so errcheck stops flagging the json.NewEncoder(w).Encode(...)
invocation in the TestAnthropicProvider_FetchProfile handler.
In `@internal/oauthstore/store_postgresql.go`:
- Around line 69-75: In Save, remove the pre-read call to s.Get and the
existing/createdAt merging logic; instead set createdAt := time.Now().UTC() and
rely on your INSERT ... ON CONFLICT ... DO UPDATE clause to preserve the
original created_at on conflict (i.e., do not attempt to read or reuse
existing.CreatedAt), and delete any error-ignoring branches around s.Get so the
function performs a single upsert without the redundant lookup.
🪄 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: 1756ed95-3f0b-46d5-89d9-410f6c5559bc
📒 Files selected for processing (5)
internal/oauth/anthropic.gointernal/oauth/oauth_test.gointernal/oauthstore/store.gointernal/oauthstore/store_postgresql.gointernal/oauthstore/store_sqlite.go
Comment on lines
+64
to
+70
| now := time.Now().UTC() | ||
| createdAt := now | ||
| // Preserve original created_at if the record already exists. | ||
| existing, err := s.Get(ctx, name) | ||
| if err == nil { | ||
| createdAt = existing.CreatedAt | ||
| } |
Contributor
There was a problem hiding this comment.
Drop the Get pre-read in Save; preserve created_at via upsert behavior.
This extra read is unnecessary and non-nil Get errors are silently discarded. Since created_at is not part of the conflict update set, you can set it to now for inserts and rely on upsert to keep existing values.
Proposed simplification
now := time.Now().UTC()
- createdAt := now
- // Preserve original created_at if the record already exists.
- existing, err := s.Get(ctx, name)
- if err == nil {
- createdAt = existing.CreatedAt
- }
-
- _, err = s.db.ExecContext(ctx, `
+ _, err := s.db.ExecContext(ctx, `
INSERT INTO oauth_tokens
(provider_name, provider_type, access_token, refresh_token, expires_at,
scopes, account_email, account_id, display_name, subscription_type,
@@
- createdAt.Unix(),
+ now.Unix(),
now.Unix(),
)As per coding guidelines: "Keep the implementation explicit and maintainable rather than relying on clever abstractions."
📝 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.
Suggested change
| now := time.Now().UTC() | |
| createdAt := now | |
| // Preserve original created_at if the record already exists. | |
| existing, err := s.Get(ctx, name) | |
| if err == nil { | |
| createdAt = existing.CreatedAt | |
| } | |
| now := time.Now().UTC() | |
| _, err := s.db.ExecContext(ctx, ` | |
| INSERT INTO oauth_tokens | |
| (provider_name, provider_type, access_token, refresh_token, expires_at, | |
| scopes, account_email, account_id, display_name, subscription_type, | |
| created_at, updated_at) | |
| VALUES | |
| (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?) | |
| ON CONFLICT(provider_name) DO UPDATE SET | |
| provider_type = excluded.provider_type, | |
| access_token = excluded.access_token, | |
| refresh_token = excluded.refresh_token, | |
| expires_at = excluded.expires_at, | |
| scopes = excluded.scopes, | |
| account_email = excluded.account_email, | |
| account_id = excluded.account_id, | |
| display_name = excluded.display_name, | |
| subscription_type = excluded.subscription_type, | |
| updated_at = excluded.updated_at | |
| `, | |
| name, provider, token.AccessToken, token.RefreshToken, token.ExpiresAt.Unix(), | |
| token.Scopes, token.AccountEmail, token.AccountID, token.DisplayName, token.SubscriptionType, | |
| now.Unix(), | |
| now.Unix(), | |
| ) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/oauthstore/store_sqlite.go` around lines 64 - 70, In Save, remove
the pre-read via s.Get and the existing/err handling; always set createdAt :=
time.Now().UTC() for the INSERT values and rely on the UPSERT/ON CONFLICT clause
to preserve an existing created_at (i.e. don't include created_at in the UPDATE
part of the upsert). Update the Save function (symbol Save in
internal/oauthstore/store_sqlite.go) to drop the Get call and related existing
variable, keep createdAt set to now, and ensure the upsert statement does not
overwrite created_at on conflict.
Comment on lines
+95
to
+99
| // normalizeProviderName trims and lowercases the provider name for consistent | ||
| // storage and lookup. | ||
| func normalizeProviderName(name string) string { | ||
| return strings.TrimSpace(name) | ||
| } |
Contributor
There was a problem hiding this comment.
Normalize provider names consistently (comment and behavior currently diverge).
normalizeProviderName is documented to trim and lowercase, but it only trims. This can create case-variant key behavior (e.g., save under one case, lookup/delete under another).
Proposed fix
func normalizeProviderName(name string) string {
- return strings.TrimSpace(name)
+ return strings.ToLower(strings.TrimSpace(name))
}📝 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.
Suggested change
| // normalizeProviderName trims and lowercases the provider name for consistent | |
| // storage and lookup. | |
| func normalizeProviderName(name string) string { | |
| return strings.TrimSpace(name) | |
| } | |
| // normalizeProviderName trims and lowercases the provider name for consistent | |
| // storage and lookup. | |
| func normalizeProviderName(name string) string { | |
| return strings.ToLower(strings.TrimSpace(name)) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/oauthstore/store.go` around lines 95 - 99, The comment says
normalizeProviderName should trim and lowercase but the implementation only
trims; update the normalizeProviderName function to also lowercase the input
(e.g., use strings.ToLower on the trimmed result) so storage/lookups are
case-insensitive, and ensure the function comment remains accurate; referenced
symbol: normalizeProviderName(name string).
…e drivers SQLiteStore.List and PostgreSQLStore.List are structurally identical but cannot share code because database/sql and pgx use incompatible row types. Add nolint:dupl directives to both methods. Also fix errcheck: add _ = to unchecked json.Encode call in oauth_test.go.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/oauth/oauth_test.go`:
- Around line 21-33: Test currently checks randomness of oauth.NewPKCEPair but
not correctness of S256 derivation; update TestNewPKCEPair to compute
expectedChallenge = base64url(SHA256(pair1.Verifier)) and assert equality with
pair1.Challenge. Use the same algorithm as production: compute
sha256.Sum256([]byte(pair1.Verifier)) and encode with base64.RawURLEncoding (no
padding) to produce the expected value, then require/assert that pair1.Challenge
== expectedChallenge for both pair1 and pair2 checks.
In `@internal/oauthstore/store_postgresql.go`:
- Around line 20-175: Add PostgreSQL-specific unit/integration tests that
exercise NewPostgreSQLStore, Save, Get, List and Delete: initialize a transient
pgxpool-backed test database and assert NewPostgreSQLStore creates the
oauth_tokens table and indexes; test Save upsert semantics by inserting a Token,
calling Save with updated fields and verifying CreatedAt is preserved and
UpdatedAt changes (use Get to fetch); verify Get maps pgx.ErrNoRows to
ErrNotFound for missing provider names; assert List returns tokens ordered by
provider_name ascending; and confirm Delete actually removes a row (Get returns
ErrNotFound after Delete). Use the same symbols from the diff
(NewPostgreSQLStore, (*PostgreSQLStore).Save, .Get, .List, .Delete) and ensure
tests clean up state between runs.
🪄 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: d4dc54dd-24fa-44db-85bb-a9ea3c30f360
📒 Files selected for processing (3)
internal/oauth/oauth_test.gointernal/oauthstore/store_postgresql.gointernal/oauthstore/store_sqlite.go
Comment on lines
+21
to
+33
| func TestNewPKCEPair(t *testing.T) { | ||
| pair1, err := oauth.NewPKCEPair() | ||
| require.NoError(t, err) | ||
| assert.NotEmpty(t, pair1.Verifier) | ||
| assert.NotEmpty(t, pair1.Challenge) | ||
| assert.NotEqual(t, pair1.Verifier, pair1.Challenge) | ||
|
|
||
| // Two pairs must be distinct | ||
| pair2, err := oauth.NewPKCEPair() | ||
| require.NoError(t, err) | ||
| assert.NotEqual(t, pair1.Verifier, pair2.Verifier) | ||
| assert.NotEqual(t, pair1.Challenge, pair2.Challenge) | ||
| } |
Contributor
There was a problem hiding this comment.
Assert PKCE S256 derivation, not just non-empty uniqueness.
Line 21–33 validates randomness but not correctness of the PKCE contract. Please assert that pair1.Challenge equals base64url(SHA256(pair1.Verifier)) so regressions in challenge generation are caught.
Suggested test extension
import (
"context"
+ "crypto/sha256"
+ "encoding/base64"
"encoding/json"
"net/http"
"net/http/httptest"
@@
func TestNewPKCEPair(t *testing.T) {
pair1, err := oauth.NewPKCEPair()
require.NoError(t, err)
assert.NotEmpty(t, pair1.Verifier)
assert.NotEmpty(t, pair1.Challenge)
assert.NotEqual(t, pair1.Verifier, pair1.Challenge)
+ sum := sha256.Sum256([]byte(pair1.Verifier))
+ expectedChallenge := base64.RawURLEncoding.EncodeToString(sum[:])
+ assert.Equal(t, expectedChallenge, pair1.Challenge)
// Two pairs must be distinct
pair2, err := oauth.NewPKCEPair()
require.NoError(t, err)As per coding guidelines: “Add or update tests for behavior changes. Tests should cover ... provider-specific parameter mapping.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/oauth/oauth_test.go` around lines 21 - 33, Test currently checks
randomness of oauth.NewPKCEPair but not correctness of S256 derivation; update
TestNewPKCEPair to compute expectedChallenge = base64url(SHA256(pair1.Verifier))
and assert equality with pair1.Challenge. Use the same algorithm as production:
compute sha256.Sum256([]byte(pair1.Verifier)) and encode with
base64.RawURLEncoding (no padding) to produce the expected value, then
require/assert that pair1.Challenge == expectedChallenge for both pair1 and
pair2 checks.
Comment on lines
+20
to
+175
| func NewPostgreSQLStore(ctx context.Context, pool *pgxpool.Pool) (*PostgreSQLStore, error) { | ||
| if ctx == nil { | ||
| return nil, fmt.Errorf("context is required") | ||
| } | ||
| if pool == nil { | ||
| return nil, fmt.Errorf("connection pool is required") | ||
| } | ||
|
|
||
| _, err := pool.Exec(ctx, ` | ||
| CREATE TABLE IF NOT EXISTS oauth_tokens ( | ||
| provider_name TEXT PRIMARY KEY, | ||
| provider_type TEXT NOT NULL DEFAULT '', | ||
| access_token TEXT NOT NULL, | ||
| refresh_token TEXT NOT NULL DEFAULT '', | ||
| expires_at BIGINT NOT NULL, | ||
| scopes TEXT NOT NULL DEFAULT '', | ||
| account_email TEXT NOT NULL DEFAULT '', | ||
| account_id TEXT NOT NULL DEFAULT '', | ||
| display_name TEXT NOT NULL DEFAULT '', | ||
| subscription_type TEXT NOT NULL DEFAULT '', | ||
| created_at BIGINT NOT NULL, | ||
| updated_at BIGINT NOT NULL | ||
| ) | ||
| `) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to create oauth_tokens table: %w", err) | ||
| } | ||
|
|
||
| for _, index := range []string{ | ||
| `CREATE INDEX IF NOT EXISTS idx_oauth_tokens_expires ON oauth_tokens(expires_at)`, | ||
| `CREATE INDEX IF NOT EXISTS idx_oauth_tokens_type ON oauth_tokens(provider_type)`, | ||
| } { | ||
| if _, err := pool.Exec(ctx, index); err != nil { | ||
| return nil, fmt.Errorf("failed to create oauth_tokens index: %w", err) | ||
| } | ||
| } | ||
|
|
||
| return &PostgreSQLStore{pool: pool}, nil | ||
| } | ||
|
|
||
| func (s *PostgreSQLStore) Save(ctx context.Context, token *Token) error { | ||
| if token == nil { | ||
| return fmt.Errorf("token is required") | ||
| } | ||
| name := normalizeProviderName(token.ProviderName) | ||
| if name == "" { | ||
| return fmt.Errorf("provider_name is required") | ||
| } | ||
|
|
||
| now := time.Now().UTC() | ||
| createdAt := now | ||
| existing, err := s.Get(ctx, name) | ||
| if err == nil { | ||
| createdAt = existing.CreatedAt | ||
| } | ||
|
|
||
| _, err = s.pool.Exec(ctx, ` | ||
| INSERT INTO oauth_tokens | ||
| (provider_name, provider_type, access_token, refresh_token, expires_at, | ||
| scopes, account_email, account_id, display_name, subscription_type, | ||
| created_at, updated_at) | ||
| VALUES ($1, $2, $3, $4, $5, $6, $7, $8, $9, $10, $11, $12) | ||
| ON CONFLICT (provider_name) DO UPDATE SET | ||
| provider_type = EXCLUDED.provider_type, | ||
| access_token = EXCLUDED.access_token, | ||
| refresh_token = EXCLUDED.refresh_token, | ||
| expires_at = EXCLUDED.expires_at, | ||
| scopes = EXCLUDED.scopes, | ||
| account_email = EXCLUDED.account_email, | ||
| account_id = EXCLUDED.account_id, | ||
| display_name = EXCLUDED.display_name, | ||
| subscription_type = EXCLUDED.subscription_type, | ||
| updated_at = EXCLUDED.updated_at | ||
| `, | ||
| name, | ||
| strings.TrimSpace(token.ProviderType), | ||
| token.AccessToken, | ||
| token.RefreshToken, | ||
| token.ExpiresAt.UTC().Unix(), | ||
| joinScopes(token.Scopes), | ||
| strings.TrimSpace(token.AccountEmail), | ||
| strings.TrimSpace(token.AccountID), | ||
| strings.TrimSpace(token.DisplayName), | ||
| strings.TrimSpace(token.SubscriptionType), | ||
| createdAt.Unix(), | ||
| now.Unix(), | ||
| ) | ||
| if err != nil { | ||
| return fmt.Errorf("save oauth token: %w", err) | ||
| } | ||
| return nil | ||
| } | ||
|
|
||
| func (s *PostgreSQLStore) Get(ctx context.Context, providerName string) (*Token, error) { | ||
| name := normalizeProviderName(providerName) | ||
| if name == "" { | ||
| return nil, fmt.Errorf("provider_name is required") | ||
| } | ||
|
|
||
| row := s.pool.QueryRow(ctx, ` | ||
| SELECT provider_name, provider_type, access_token, refresh_token, expires_at, | ||
| scopes, account_email, account_id, display_name, subscription_type, | ||
| created_at, updated_at | ||
| FROM oauth_tokens | ||
| WHERE provider_name = $1 | ||
| `, name) | ||
|
|
||
| token, err := scanPostgreSQLToken(row) | ||
| if err != nil { | ||
| if errors.Is(err, pgx.ErrNoRows) { | ||
| return nil, ErrNotFound | ||
| } | ||
| return nil, fmt.Errorf("get oauth token: %w", err) | ||
| } | ||
| return token, nil | ||
| } | ||
|
|
||
| func (s *PostgreSQLStore) Delete(ctx context.Context, providerName string) error { | ||
| name := normalizeProviderName(providerName) | ||
| if name == "" { | ||
| return fmt.Errorf("provider_name is required") | ||
| } | ||
| _, err := s.pool.Exec(ctx, `DELETE FROM oauth_tokens WHERE provider_name = $1`, name) | ||
| if err != nil { | ||
| return fmt.Errorf("delete oauth token: %w", err) | ||
| } | ||
| return nil | ||
| } | ||
|
|
||
| //nolint:dupl // SQLite and PostgreSQL List methods are structurally identical but use incompatible driver interfaces | ||
| func (s *PostgreSQLStore) List(ctx context.Context) ([]*Token, error) { | ||
| rows, err := s.pool.Query(ctx, ` | ||
| SELECT provider_name, provider_type, access_token, refresh_token, expires_at, | ||
| scopes, account_email, account_id, display_name, subscription_type, | ||
| created_at, updated_at | ||
| FROM oauth_tokens | ||
| ORDER BY provider_name ASC | ||
| `) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("list oauth tokens: %w", err) | ||
| } | ||
| defer rows.Close() | ||
|
|
||
| result := make([]*Token, 0) | ||
| for rows.Next() { | ||
| token, err := scanPostgreSQLToken(rows) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("scan oauth token: %w", err) | ||
| } | ||
| result = append(result, token) | ||
| } | ||
| if err := rows.Err(); err != nil { | ||
| return nil, fmt.Errorf("iterate oauth tokens: %w", err) | ||
| } | ||
| return result, nil | ||
| } |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy lift
Add PostgreSQL-specific store tests for CRUD/upsert edge cases.
This backend is persistence-critical, but there’s no PostgreSQL-targeted coverage for schema init, Save upsert behavior (created_at preservation), Get not-found mapping, List ordering, and Delete. Given current patch coverage signals, this should be added before merge.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/oauthstore/store_postgresql.go` around lines 20 - 175, Add
PostgreSQL-specific unit/integration tests that exercise NewPostgreSQLStore,
Save, Get, List and Delete: initialize a transient pgxpool-backed test database
and assert NewPostgreSQLStore creates the oauth_tokens table and indexes; test
Save upsert semantics by inserting a Token, calling Save with updated fields and
verifying CreatedAt is preserved and UpdatedAt changes (use Get to fetch);
verify Get maps pgx.ErrNoRows to ErrNotFound for missing provider names; assert
List returns tokens ordered by provider_name ascending; and confirm Delete
actually removes a row (Get returns ErrNotFound after Delete). Use the same
symbols from the diff (NewPostgreSQLStore, (*PostgreSQLStore).Save, .Get, .List,
.Delete) and ensure tests clean up state between runs.
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.
This PR adds support for OAuth-authenticated Anthropic providers as an alternative to static API keys. Once authenticated via the admin dashboard, the provider behaves identically to a key-based provider — no changes needed in client applications.
Configuration
Set api_key: "oauth" for any Anthropic provider in config.yaml:
Then authenticate via the admin dashboard under OAuth Providers.
How it works
Dual-mode callback — no configuration required
New packages
New admin API endpoints under /admin/api/v1/oauth:
Dashboard
Docs
Summary by CodeRabbit
New Features
Documentation