Add Anthropic OAuth support for Claude Pro/Max subscriptions#1984
Closed
loss-and-quick wants to merge 32 commits intomaximhq:mainfrom
Closed
Add Anthropic OAuth support for Claude Pro/Max subscriptions#1984loss-and-quick wants to merge 32 commits intomaximhq:mainfrom
loss-and-quick wants to merge 32 commits intomaximhq:mainfrom
Conversation
Anthropic's API requires the `anthropic-beta: oauth-2025-04-20` header when authenticating via OAuth Bearer tokens. Without it, the API returns "OAuth authentication is currently not supported." Add AnthropicOAuthBetaHeader constant and inject it in: - completeRequest (ChatCompletion, TextCompletion, Responses, CountTokens) - ChatCompletionStream (streaming headers map) - ResponsesStream (streaming headers map)
…lumn
Existing databases were missing the anthropic_oauth_config_id column in
the config_keys table because no migration was added when the OAuth
backend support was introduced.
Without this column:
- BeforeSave could not persist AnthropicOAuthKeyConfig to DB
- AfterFind could not reconstruct AnthropicOAuthKeyConfig from DB
- GET /api/providers/{provider} returned keys without anthropic_oauth_key_config
- UI showed no OAuth badge and defaulted to "API Key" tab on reload
All three key-write paths (UpdateProvidersConfig, UpdateProvider, AddProvider) were building TableKey without AnthropicOAuthKeyConfig. BeforeSave then saw nil and cleared anthropic_o_auth_config_id to NULL on every save, including startup reconciliation. As a result getProvider (which reads from SQLite, not in-memory) never returned anthropic_oauth_key_config, causing the OAuth badge to vanish on every page reload despite inference working from memory.
# Conflicts: # core/bifrost.go # core/providers/anthropic/anthropic.go # framework/configstore/migrations.go # ui/app/workspace/providers/fragments/apiKeysFormFragment.tsx
Contributor
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdd end-to-end Anthropic OAuth (PKCE): DB migration and schema fields, OAuth2 flows (initiate/exchange/refresh/revoke), per-key token resolution and auth header switching in the Anthropic provider, HTTP endpoints/handlers, frontend store/UI, and key-selection/persistence updates to accept OAuth-configured keys. Changes
Sequence DiagramsequenceDiagram
autonumber
actor User
participant UI as Client UI
participant Server as Bifrost Server
participant OAuth as Anthropic OAuth Server
participant DB as Token Store
participant API as Anthropic API
User->>UI: Click "Authorize (Anthropic)"
UI->>Server: POST /api/anthropic-oauth/initiate
Server->>OAuth: Create PKCE/state, return auth URL
Server-->>UI: authorize_url, oauth_config_id, expires_at
UI->>OAuth: Browser redirect & user consent
OAuth-->>UI: Redirect back with code + state
UI->>Server: POST /api/anthropic-oauth/exchange (code, oauth_config_id)
Server->>OAuth: Exchange code (JSON) -> tokens
OAuth-->>Server: Access + Refresh token
Server->>DB: Store tokens (linked to oauth_config_id)
UI->>Server: Request using provider key (oauth_config_id)
Server->>DB: Resolve access token for oauth_config_id
DB-->>Server: Access token
Server->>API: API call with Authorization: Bearer <token> (+ oauth beta header)
API-->>Server: Response
Server-->>UI: Forward response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 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 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 |
![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: 11
🧹 Nitpick comments (1)
core/utils.go (1)
99-106: Make this check operate on the resolved base provider.This helper can only match
schemas.Anthropic. If a caller is working with a custom-provider alias, there's no way for the OAuth exception to compose with Anthropic-backed custom providers. Consider folding this into the existing empty-credential predicate after resolvingCustomProviderConfig.BaseProviderType.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/utils.go` around lines 99 - 106, The hasAnthropicOAuthCredentials helper currently only checks providerType == schemas.Anthropic so it misses Anthropic-backed custom providers; update it to resolve the base provider when providerType is a custom provider (inspect key.CustomProviderConfig.BaseProviderType or the CustomProviderConfig type) and treat it as Anthropic if the resolved BaseProviderType equals schemas.Anthropic; keep the existing OAuth-config check (key.AnthropicOAuthKeyConfig != nil && key.AnthropicOAuthKeyConfig.OAuthConfigID != "") and return true when the resolved base is Anthropic and those fields exist so the OAuth exception composes with CustomProviderConfig.BaseProviderType.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@core/providers/anthropic/anthropic.go`:
- Around line 150-173: Update setAnthropicAuthHeader and
setAnthropicAuthHeaderMap to also add the OAuth beta header when isOAuthKey(key)
is true: keep the existing Authorization/"x-api-key" behavior but when
isOAuthKey(key) add the header "anthropic-beta" with value "oauth-2025-04-20";
preserve the early-return on empty keyValue and ensure the beta header is only
set for OAuth keys so Batch*/File* call sites get the same oauth-2025-04-20 flag
as completeRequest.
- Around line 524-533: The OAuth branch in anthropic.go overwrites any
pre-existing "anthropic-beta" values in the headers map (which may have been
populated earlier via providerUtils.SetExtraHeaders); update the isOAuthKey(...)
handling to append AnthropicOAuthBetaHeader to headers["anthropic-beta"] if any
value already exists (use a comma separator) instead of replacing it. Modify the
logic around headers manipulation in the isOAuthKey branch (and the similar
block at lines 1000-1009) so both will preserve and extend existing
headers["anthropic-beta"] rather than assigning a new value.
In `@framework/configstore/clientconfig.go`:
- Around line 394-397: GenerateKeyHash() currently omits AnthropicOAuthKeyConfig
so switching between API-key and OAuth or changing oauth_config_id doesn't
change the key hash; update GenerateKeyHash() to incorporate the
AnthropicOAuthKeyConfig into the hash input for each key (e.g., include a stable
string/ID such as AnthropicOAuthKeyConfig.OAuthConfigID or the serialized config
when key.AnthropicOAuthKeyConfig != nil, and a canonical empty marker when nil)
so the redactedConfig.Keys hashing reflects this new persisted field and
reconciliation will detect OAuth changes.
In `@framework/configstore/migrations.go`:
- Around line 4046-4055: The PostgreSQL probe against information_schema.columns
in the tx.Raw call that checks for table_name = 'config_keys' and column_name =
'anthropic_oauth_config_id' must be restricted to the current schema to avoid
false positives from other schemas; update the SQL to include "AND table_schema
= current_schema()" (so the exists check uses only the active schema) and keep
the subsequent logic that calls tx.Exec("ALTER TABLE config_keys ADD COLUMN
anthropic_oauth_config_id VARCHAR(255)").
In `@framework/configstore/rdb.go`:
- Line 297: The mapping currently overwrites existing AnthropicOAuthKeyConfig
with the incoming value (which being omitted/zero will be nil and thus
disconnect the OAuth key); update the logic in UpdateProvidersConfig and
UpdateProvider so you only modify dbKey.AnthropicOAuthKeyConfig when the caller
explicitly intends to change it — i.e., detect whether the incoming payload
includes an AnthropicOAuthKeyConfig field and if not, preserve the existing
dbKey.AnthropicOAuthKeyConfig before calling Save(&dbKey); keep
TableKey.BeforeSave behavior (which turns a nil into anthropic_oauth_config_id =
NULL) so an explicit nil from the caller still disconnects, but an absent field
does not.
In `@framework/configstore/tables/key.go`:
- Around line 638-643: When reconstructing the Anthropic OAuth config, ensure
you clear the runtime-only field when the DB column is NULL: in the block that
checks AnthropicOAuthConfigID, add an else branch that sets
k.AnthropicOAuthKeyConfig = nil so stale data doesn't carry over between reused
TableKey instances; reference the AnthropicOAuthConfigID and
AnthropicOAuthKeyConfig fields on the TableKey struct when making this change.
In `@framework/oauth2/main.go`:
- Around line 712-751: callTokenEndpointJSON currently creates HTTP requests
without context so they cannot be canceled; update its signature to accept ctx
context.Context (func (p *OAuth2Provider) callTokenEndpointJSON(ctx
context.Context, tokenURL string, data map[string]string) ...) and use
http.NewRequestWithContext(ctx, "POST", tokenURL, bytes.NewReader(jsonBody)).
Propagate the context from its caller CompleteAnthropicOAuthFlow when invoking
callTokenEndpointJSON, and keep the existing 30s http.Client timeout (or
consider removing the fixed timeout if you prefer context-driven deadlines).
Also update any other callers of callTokenEndpointJSON to pass a context. Ensure
error messages and return types remain unchanged.
- Around line 763-765: The code currently reuses the PKCE code_verifier as the
OAuth state (state := codeVerifier) and never validates it in
CompleteAnthropicOAuthFlow; generate a separate cryptographically random state
(different from codeVerifier) when starting the Anthropic OAuth flow, persist it
alongside oauthConfigID (e.g., in the same store/session used for PKCE), include
that distinct state in the authorization URL, and then in
CompleteAnthropicOAuthFlow validate the returned state parameter against the
stored state before performing the token exchange; if the state mismatches or is
missing, reject the request the same way CompleteOAuthFlow does.
In `@ui/app/workspace/providers/fragments/apiKeysFormFragment.tsx`:
- Around line 791-799: The current branch in the Tabs onValueChange handler (and
the similar branch at 887-891) clears the stored oauthConfigId (via
form.setValue('key.anthropic_oauth_key_config', undefined) / clearing key.value)
immediately when switching auth types, which can orphan refresh tokens because
logoutOAuth cannot find the config after unmount; instead, ensure you call
logoutOAuth (or another revocation routine) before clearing the oauth config and
only clear form.setValue('key.anthropic_oauth_key_config', undefined) after
logoutOAuth resolves (or move the clearing into the logout success/finally
handler), and update setAnthropicAuthType logic in the Tabs handler and the
other branch to preserve oauthConfigId until revocation completes so the
logoutOAuth function can operate correctly.
- Around line 801-804: Add data-testid attributes to all interactive Anthropic
OAuth controls so E2E tests can target them: add data-testid="auth-tab-api-key"
to the TabsTrigger with value="api_key" and
data-testid="auth-tab-anthropic-oauth" to the TabsTrigger with value="oauth";
add data-testid attributes to the connect/exchange buttons (e.g.,
"anthropic-connect-button" / "anthropic-exchange-button"), the authorization
code input (e.g., "anthropic-auth-code-input"), the refresh and disconnect
actions (e.g., "anthropic-refresh-button" / "anthropic-disconnect-button"), and
the retry/cancel controls (e.g., "anthropic-retry-button" /
"anthropic-cancel-button"); apply the same pattern to the other occurrences
mentioned (lines ~908-915, 927-929, 953-966, 982-985) so every interactive
element in the OAuth flow has a stable data-testid for tests.
- Around line 840-845: The current handleInitiate awaits initiateOAuth() then
calls window.open(result.authorize_url, '_blank'), which can be popup-blocked
and leaves window.opener set; change the flow so the click handler opens a blank
tab synchronously (const popup = window.open('about:blank', '_blank')),
immediately set popup.opener = null if popup exists, then call
initiateOAuth().unwrap(), setOauthConfigId(result.oauth_config_id) and navigate
the popup to result.authorize_url (popup.location.href = ...); if popup is null
(blocked), fall back to navigating the current window to the authorize_url; also
add a data-testid attribute to the "Connect with Claude" button element per
workspace UI conventions.
---
Nitpick comments:
In `@core/utils.go`:
- Around line 99-106: The hasAnthropicOAuthCredentials helper currently only
checks providerType == schemas.Anthropic so it misses Anthropic-backed custom
providers; update it to resolve the base provider when providerType is a custom
provider (inspect key.CustomProviderConfig.BaseProviderType or the
CustomProviderConfig type) and treat it as Anthropic if the resolved
BaseProviderType equals schemas.Anthropic; keep the existing OAuth-config check
(key.AnthropicOAuthKeyConfig != nil && key.AnthropicOAuthKeyConfig.OAuthConfigID
!= "") and return true when the resolved base is Anthropic and those fields
exist so the OAuth exception composes with
CustomProviderConfig.BaseProviderType.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5084704d-8b30-4398-9b71-aba9b0316b0c
📒 Files selected for processing (20)
core/bifrost.gocore/providers/anthropic/anthropic.gocore/providers/anthropic/types.gocore/schemas/account.gocore/schemas/provider.gocore/utils.goframework/configstore/clientconfig.goframework/configstore/migrations.goframework/configstore/rdb.goframework/configstore/tables/key.goframework/oauth2/main.gotransports/bifrost-http/handlers/anthropic_oauth.gotransports/bifrost-http/handlers/utils.gotransports/bifrost-http/server/server.goui/app/workspace/providers/fragments/apiKeysFormFragment.tsxui/app/workspace/providers/views/modelProviderKeysTableView.tsxui/lib/store/apis/anthropicOAuthApi.tsui/lib/store/apis/index.tsui/lib/types/config.tsui/lib/types/schemas.ts
Contributor
|
@loss-and-quick ❤️ for such a good feature. We will review this once code rabbit round is done |
Contributor
Author
Thanks |
setAnthropicAuthHeader and setAnthropicAuthHeaderMap now append the anthropic-beta: oauth-2025-04-20 header for OAuth keys, so Batch and File endpoints that use these helpers send the required header. Removes the now-redundant inline beta header code from ChatCompletionStream and ResponsesStream.
Switching a key between API key and OAuth auth was invisible to config reconciliation because GenerateKeyHash did not include the OAuth config.
The information_schema.columns query could match columns from other schemas, causing the migration to skip adding the column.
loss-and-quick
force-pushed
the
anthropic-oauth
branch
from
20c6d21 to
238f158
Compare
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
core/providers/anthropic/anthropic.go (2)
583-585:⚠️ Potential issue | 🟠 MajorStreaming for-loop overwrites
anthropic-betaset bySetExtraHeaders.The
setAnthropicAuthHeaderMapfix at lines 172-176 appends to the headers map, but this doesn't help because:
- The
headersmap at lines 523-528 doesn't containanthropic-betaSetExtraHeadersat line 581 may setanthropic-betaon the request directly (from context or extra headers)- This for-loop uses
req.Header.Setwhich overwrites the value already on the requestBeta headers added via
addMissingBetaHeadersToContext(e.g., for tools, citations, etc.) would be lost.🔧 Suggested fix: use appendBetaHeader for the beta header
// Set headers, merging anthropic-beta with any user-supplied values +betaHeaderFromMap := headers["anthropic-beta"] for key, value := range headers { + if key == "anthropic-beta" { + continue // Handle separately below + } req.Header.Set(key, value) } +if betaHeaderFromMap != "" { + appendBetaHeader(req, betaHeaderFromMap) +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/providers/anthropic/anthropic.go` around lines 583 - 585, The loop that applies headers to the request currently calls req.Header.Set for every key from the headers map which overwrites any existing request header values (including anthropic-beta set earlier via SetExtraHeaders or addMissingBetaHeadersToContext), so change the application logic to preserve/append the beta header: when key == "anthropic-beta" (or use the same header constant) call appendBetaHeader(req, value) instead of req.Header.Set; for all other keys continue using req.Header.Set; ensure this uses the same setAnthropicAuthHeaderMap/headers map and keeps SetExtraHeaders/addMissingBetaHeadersToContext behavior intact.
1050-1053:⚠️ Potential issue | 🟠 MajorSame
anthropic-betaoverwrite issue as inHandleAnthropicChatCompletionStreaming.The comment says "merging anthropic-beta with any user-supplied values" but
req.Header.Setreplaces rather than merges. Apply the same fix pattern as suggested for lines 583-585.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/providers/anthropic/anthropic.go` around lines 1050 - 1053, The loop that merges user-supplied headers into the request currently uses req.Header.Set(key, value), which overwrites existing headers (causing the anthropic-beta header to be replaced); change this to use req.Header.Add(key, value) (same pattern used in HandleAnthropicChatCompletionStreaming) so user headers are appended without clobbering the pre-set anthropic-beta header, updating the header merge loop that iterates over headers and references req.Header.Set.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@core/providers/anthropic/anthropic.go`:
- Around line 583-585: The loop that applies headers to the request currently
calls req.Header.Set for every key from the headers map which overwrites any
existing request header values (including anthropic-beta set earlier via
SetExtraHeaders or addMissingBetaHeadersToContext), so change the application
logic to preserve/append the beta header: when key == "anthropic-beta" (or use
the same header constant) call appendBetaHeader(req, value) instead of
req.Header.Set; for all other keys continue using req.Header.Set; ensure this
uses the same setAnthropicAuthHeaderMap/headers map and keeps
SetExtraHeaders/addMissingBetaHeadersToContext behavior intact.
- Around line 1050-1053: The loop that merges user-supplied headers into the
request currently uses req.Header.Set(key, value), which overwrites existing
headers (causing the anthropic-beta header to be replaced); change this to use
req.Header.Add(key, value) (same pattern used in
HandleAnthropicChatCompletionStreaming) so user headers are appended without
clobbering the pre-set anthropic-beta header, updating the header merge loop
that iterates over headers and references req.Header.Set.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6394ebde-e3fb-4280-875c-a5458675f8dd
📒 Files selected for processing (3)
core/providers/anthropic/anthropic.goframework/configstore/clientconfig.goframework/configstore/migrations.go
🚧 Files skipped from review as they are similar to previous changes (1)
- framework/configstore/clientconfig.go
Generate a distinct random state parameter instead of reusing the PKCE code_verifier, which was exposed through the browser redirect URL. Also validate returned state on callback for CSRF protection and thread context.Context through callTokenEndpointJSON for request cancellation support.
Without the else branch, reusing a TableKey instance for a row with NULL anthropic_oauth_config_id could carry the previous row's OAuth config forward.
The streaming for-loop used req.Header.Set which overwrote beta values already set by SetExtraHeaders. Now anthropic-beta from the headers map is appended via appendBetaHeader instead.
Open a blank tab synchronously before the async API call to prevent popup blockers. Sever window.opener for security. Fall back to same-tab navigation if the popup was blocked. Add data-testid attributes to all interactive OAuth elements.
Don't clear anthropic_oauth_key_config when switching to API Key tab — the Disconnect button in AnthropicOAuthSection handles server-side logout first. Cancel during initiated flow now calls logoutOAuth to clean up the pending server-side OAuth config.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
ui/app/workspace/providers/fragments/apiKeysFormFragment.tsx (1)
791-799:⚠️ Potential issue | 🟠 MajorRevoke the OAuth config before leaving the OAuth flow.
Switching back to API Key or pressing Cancel drops the last tracked
oauthConfigIdbeforelogoutOAuthruns. That can leave initiated configs, and even connected sessions, orphaned on the backend because the section unmounts or clears state before revocation is possible. Hoist the pending config id to a parent/ref that survives the mode switch, or calllogoutOAuthfirst and only clear form/local state after it resolves.Also applies to: 898-902
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/app/workspace/providers/fragments/apiKeysFormFragment.tsx` around lines 791 - 799, When switching auth modes or cancelling, ensure the OAuth config is revoked before you clear form or component state: don't clear 'key.anthropic_oauth_key_config' or drop the tracked oauthConfigId prior to calling logoutOAuth. Modify the Tabs onValueChange handler (where setAnthropicAuthType is called) and the Cancel/code paths referenced around the second occurrence (lines ~898-902) to either hoist the pending oauthConfigId to a ref/parent that survives the mode switch or call logoutOAuth with the current oauthConfigId and await its resolution before calling form.setValue to clear the OAuth config; use the function logoutOAuth and the oauthConfigId identifier to locate and sequence the calls so revocation runs reliably before state is wiped.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@framework/oauth2/main.go`:
- Around line 840-850: The state validation must be mandatory: always require
the incoming rawCode to include a "#state" suffix and verify it equals
oauthConfig.State; if the suffix is missing or the returnedState does not match,
set oauthConfig.Status = "failed", persist via
p.configStore.UpdateOauthConfig(ctx, oauthConfig) and return an error (e.g.,
"invalid or missing state token"); update the logic around rawCode parsing so
you do not proceed when only a bare code is supplied (use rawCode, parts :=
strings.SplitN(rawCode, "#", 2) and enforce len(parts)==2 &&
parts[1]==oauthConfig.State before accepting parts[0] as the code).
In `@ui/app/workspace/providers/fragments/apiKeysFormFragment.tsx`:
- Around line 847-855: The same-tab fallback loses required state: before doing
window.location.href= result.authorize_url in the popup-blocked branch, persist
the pending OAuth flow (oauth_config_id and flowState) so handleExchange can
retrieve it on return; update the logic around initiateOAuth().unwrap(),
setOauthConfigId(...) and setFlowState('initiated') to write the same data into
durable storage (e.g., localStorage/sessionStorage or a short-lived cookie) when
authWindow is null/closed, or alternatively replace the fallback with an
explicit “popup blocked” recovery UI that displays a recovery URL or a button to
open the auth in a new tab while still persisting oauth_config_id for
handleExchange to consume. Ensure handleExchange reads the persisted
oauth_config_id if the in-memory value is missing.
---
Duplicate comments:
In `@ui/app/workspace/providers/fragments/apiKeysFormFragment.tsx`:
- Around line 791-799: When switching auth modes or cancelling, ensure the OAuth
config is revoked before you clear form or component state: don't clear
'key.anthropic_oauth_key_config' or drop the tracked oauthConfigId prior to
calling logoutOAuth. Modify the Tabs onValueChange handler (where
setAnthropicAuthType is called) and the Cancel/code paths referenced around the
second occurrence (lines ~898-902) to either hoist the pending oauthConfigId to
a ref/parent that survives the mode switch or call logoutOAuth with the current
oauthConfigId and await its resolution before calling form.setValue to clear the
OAuth config; use the function logoutOAuth and the oauthConfigId identifier to
locate and sequence the calls so revocation runs reliably before state is wiped.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c748f021-f847-4dd6-91d8-9f54d74dcce2
📒 Files selected for processing (4)
core/providers/anthropic/anthropic.goframework/configstore/tables/key.goframework/oauth2/main.goui/app/workspace/providers/fragments/apiKeysFormFragment.tsx
UpdateProvidersConfig and UpdateProvider would set anthropic_oauth_config_id to NULL on Save when the incoming key (e.g. from config.json) had nil AnthropicOAuthKeyConfig. Now the existing OAuth config is preserved from the DB when the incoming key doesn't explicitly set it.
… on unmount - Make state validation unconditional in CompleteAnthropicOAuthFlow: reject authorization codes without #state suffix to prevent CSRF bypass - Use ref to track oauthConfigId across component lifecycle; revoke pending configs on unmount (e.g. tab switch) to prevent orphaned server-side flows - Replace same-tab fallback with explicit popup-blocked error to avoid losing pending exchange state when navigating away Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
Author
|
@akshaydeo All issues flagged by CodeRabbit have been addressed, the latest review is clean. |
Contributor
|
I do see a few more - CR sometimes is a bit slow :D. |
Contributor
Author
:D |
|
|
Contributor
Author
The Code Rabbit has already reviewed the PR. Could you please take a look? I've conducted testing over the past four days using a minimal setup focused on consolidating all my APIs into a single gateway, and I haven't encountered any issues. |
|
@loss-and-quick This is awesome! Would it be possible to setup Anthropic subscription via Bifrost API ? |
loss-and-quick
force-pushed
the
anthropic-oauth
branch
from
e03bcf2 to
7dc8bb3
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
core/providers/anthropic/anthropic.go (1)
165-179:⚠️ Potential issue | 🟠 MajorClear stale auth headers before overlaying provider auth.
Line 165, Line 609, and Line 1100 all apply provider auth after
SetExtraHeaders, but they never remove any preexistingAuthorization/x-api-key. On OAuth requests that can send both credentials upstream, which leaks the stale secret and makes auth selection ambiguous. Only when this request is supplying provider auth, delete both auth headers first and then set the chosen mechanism.🔐 Proposed fix
providerUtils.SetExtraHeaders(ctx, req, provider.networkConfig.ExtraHeaders, nil) req.SetRequestURI(url) req.Header.SetMethod(http.MethodPost) req.Header.SetContentType("application/json") if key != "" && !IsClaudeCodeMaxMode(ctx) { + req.Header.Del("Authorization") + req.Header.Del("x-api-key") if isOAuth { req.Header.Set("Authorization", "Bearer "+key) appendBetaHeader(req, AnthropicOAuthBetaHeader) } else { req.Header.Set("x-api-key", key) } }providerUtils.SetExtraHeaders(ctx, req, extraHeaders, nil) + if headers["Authorization"] != "" || headers["x-api-key"] != "" { + req.Header.Del("Authorization") + req.Header.Del("x-api-key") + } // Set headers, using append for anthropic-beta to preserve values from SetExtraHeaders betaFromMap := headers["anthropic-beta"]Also applies to: 609-621, 1100-1110
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/providers/anthropic/anthropic.go` around lines 165 - 179, The code applies provider auth after providerUtils.SetExtraHeaders without removing any preexisting Authorization/x-api-key headers, which can leak stale credentials; update each auth-application block (the conditional guarded by key != "" && !IsClaudeCodeMaxMode that follows providerUtils.SetExtraHeaders) to first delete both "Authorization" and "x-api-key" from req.Header, then set only the chosen header (for OAuth set "Authorization: Bearer <key>" and call appendBetaHeader with AnthropicOAuthBetaHeader, otherwise set "x-api-key"); apply this change to the same pattern used around providerUtils.SetExtraHeaders/IsClaudeCodeMaxMode/appendBetaHeader occurrences in this file.
♻️ Duplicate comments (1)
transports/bifrost-http/handlers/anthropic_oauth.go (1)
71-76:⚠️ Potential issue | 🔴 CriticalEnforce
oauth_config_idownership before provider calls.Line 71, Line 95, Line 131, and Line 160 accept caller-provided
oauth_config_idand perform exchange/status/refresh/revoke without an explicit workspace/user ownership check in this handler path. This is an object-level authorization gap unless ownership is enforced downstream for every call.Please gate these operations by authenticated workspace/user and reject mismatches with 403.
Also applies to: 95-96, 131-132, 160-161
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@transports/bifrost-http/handlers/anthropic_oauth.go` around lines 71 - 76, Before calling any provider methods (e.g., h.oauthProvider.CompleteAnthropicOAuthFlow, GetAnthropicOAuthStatus, RefreshAnthropicOAuthToken, RevokeAnthropicOAuthToken) verify that the authenticated workspace/user in the request context actually owns the supplied oauth_config_id; fetch the oauth config (or its owner) using the service/DB helper available in the handler, compare its workspace/user id to the authenticated principal, and if they do not match return SendError(ctx, fasthttp.StatusForbidden, "forbidden: oauth_config_id does not belong to authenticated workspace/user") without invoking the provider; perform this ownership check at each handler entry that accepts oauth_config_id (the sites where req.OAuthConfigID is used before provider calls).
🧹 Nitpick comments (1)
framework/configstore/rdb.go (1)
357-364: Optional cleanup: extract OAuth-preservation guard into a helper.The same “preserve existing
AnthropicOAuthKeyConfigwhen incoming is nil” block is repeated in three places; a small helper would reduce drift risk.♻️ Suggested refactor
+func preserveAnthropicOAuthConfigIfUnset(dst *tables.TableKey, existing tables.TableKey) { + if dst.AnthropicOAuthKeyConfig == nil { + dst.AnthropicOAuthKeyConfig = existing.AnthropicOAuthKeyConfig + } +} ... - if dbKey.AnthropicOAuthKeyConfig == nil { - dbKey.AnthropicOAuthKeyConfig = existingKey.AnthropicOAuthKeyConfig - } + preserveAnthropicOAuthConfigIfUnset(&dbKey, existingKey)Also applies to: 380-387, 532-539
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@framework/configstore/rdb.go` around lines 357 - 364, Extract the repeated "preserve existing AnthropicOAuthKeyConfig when incoming is nil" logic into a small helper (e.g., preserveAnthropicOAuthConfig(existingKey, dbKey) or a method on the key type) and replace the duplicated blocks that check and assign dbKey.AnthropicOAuthKeyConfig (currently found where dbKey.AnthropicOAuthKeyConfig == nil is tested around the existing blocks) with a single call to that helper; ensure the helper takes the incoming and existing key objects, performs the nil check and assignment, and update all three locations (the blocks currently using the explicit nil check at ~lines shown in the comment) to call it.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@core/providers/anthropic/anthropic.go`:
- Around line 315-330: The current branch treats "no keys configured" the same
as "all keys are OAuth" because it only checks len(apiKeys)==0; change the
condition so the sentinel empty response is returned only when there were
original keys but all were filtered as OAuth — i.e., after building apiKeys
compute filteredOAuthCount := len(keys)-len(apiKeys) and only return the
&schemas.BifrostListModelsResponse{Data: make([]schemas.Model,0)} when
len(apiKeys)==0 && filteredOAuthCount>0; leave the zero-original-keys path to
follow the normal misconfiguration/error handling instead of the OAuth fallback.
Use the existing symbols keys, apiKeys, and isOAuthKey to locate and implement
this change.
---
Outside diff comments:
In `@core/providers/anthropic/anthropic.go`:
- Around line 165-179: The code applies provider auth after
providerUtils.SetExtraHeaders without removing any preexisting
Authorization/x-api-key headers, which can leak stale credentials; update each
auth-application block (the conditional guarded by key != "" &&
!IsClaudeCodeMaxMode that follows providerUtils.SetExtraHeaders) to first delete
both "Authorization" and "x-api-key" from req.Header, then set only the chosen
header (for OAuth set "Authorization: Bearer <key>" and call appendBetaHeader
with AnthropicOAuthBetaHeader, otherwise set "x-api-key"); apply this change to
the same pattern used around
providerUtils.SetExtraHeaders/IsClaudeCodeMaxMode/appendBetaHeader occurrences
in this file.
---
Duplicate comments:
In `@transports/bifrost-http/handlers/anthropic_oauth.go`:
- Around line 71-76: Before calling any provider methods (e.g.,
h.oauthProvider.CompleteAnthropicOAuthFlow, GetAnthropicOAuthStatus,
RefreshAnthropicOAuthToken, RevokeAnthropicOAuthToken) verify that the
authenticated workspace/user in the request context actually owns the supplied
oauth_config_id; fetch the oauth config (or its owner) using the service/DB
helper available in the handler, compare its workspace/user id to the
authenticated principal, and if they do not match return SendError(ctx,
fasthttp.StatusForbidden, "forbidden: oauth_config_id does not belong to
authenticated workspace/user") without invoking the provider; perform this
ownership check at each handler entry that accepts oauth_config_id (the sites
where req.OAuthConfigID is used before provider calls).
---
Nitpick comments:
In `@framework/configstore/rdb.go`:
- Around line 357-364: Extract the repeated "preserve existing
AnthropicOAuthKeyConfig when incoming is nil" logic into a small helper (e.g.,
preserveAnthropicOAuthConfig(existingKey, dbKey) or a method on the key type)
and replace the duplicated blocks that check and assign
dbKey.AnthropicOAuthKeyConfig (currently found where
dbKey.AnthropicOAuthKeyConfig == nil is tested around the existing blocks) with
a single call to that helper; ensure the helper takes the incoming and existing
key objects, performs the nil check and assignment, and update all three
locations (the blocks currently using the explicit nil check at ~lines shown in
the comment) to call it.
🪄 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: CHILL
Plan: Pro
Run ID: 5e02d4a5-688d-4ed8-a072-56fe0500b45e
📒 Files selected for processing (7)
core/providers/anthropic/anthropic.gocore/providers/anthropic/utils.goframework/configstore/clientconfig.goframework/configstore/rdb.gotransports/bifrost-http/handlers/anthropic_oauth.gotransports/bifrost-http/handlers/inference.gotransports/bifrost-http/handlers/utils.go
🚧 Files skipped from review as they are similar to previous changes (2)
- transports/bifrost-http/handlers/utils.go
- transports/bifrost-http/handlers/inference.go
# Conflicts: # core/bifrost.go # core/providers/anthropic/types.go # core/providers/anthropic/utils.go # framework/configstore/migrations.go # framework/configstore/rdb.go # ui/app/workspace/providers/fragments/apiKeysFormFragment.tsx
Contributor
Confidence Score: 5/5Safe to merge; all remaining findings are P2 style/quality suggestions with no present data-loss or correctness risk The passthrough header-deletion issue from the previous review thread is addressed. The infinite-recursion bug is fixed. DB migration, ORM hooks, and key-filtering logic are all consistent. The three remaining findings — missing mutex in CompleteAnthropicOAuthFlow, per-call HTTP client, and fragile Anthropic config detection — are all low-probability edge cases or efficiency nits that do not block correctness on the happy path. framework/oauth2/main.go — mutex coverage and HTTP client allocation are the highest-priority follow-ups Important Files Changed
Reviews (5): Last reviewed commit: "fix(oauth): make CompleteAnthropicOAuthF..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
core/providers/anthropic/anthropic.go (2)
175-181:⚠️ Potential issue | 🟠 MajorClear conflicting auth headers on
req.Headerbefore applying the selected key.
providerUtils.SetExtraHeaders(...)runs before these branches, so a configuredAuthorizationorx-api-keysurvives when the selected Bifrost key uses the other auth mode. The local map helper only normalizes the map; it does not remove credentials already written ontoreq. That can send both credentials upstream on the same request and leak the stale one.🔧 Suggested fix
if key != "" && !IsClaudeCodeMaxMode(ctx) { + req.Header.Del("Authorization") + req.Header.Del("x-api-key") if isOAuth { req.Header.Set("Authorization", "Bearer "+key) appendBetaHeader(req, AnthropicOAuthBetaHeader) } else { req.Header.Set("x-api-key", key) } }+if headers["Authorization"] != "" || headers["x-api-key"] != "" { + req.Header.Del("Authorization") + req.Header.Del("x-api-key") +} + // Set headers, using append for anthropic-beta to preserve values from SetExtraHeaders betaFromMap := headers["anthropic-beta"] for key, value := range headers {Also applies to: 618-628, 1113-1123
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/providers/anthropic/anthropic.go` around lines 175 - 181, Before setting the selected credential in the Anthropic request branches (the block that checks key != "" && !IsClaudeCodeMaxMode(ctx) and the similar blocks at the other locations), clear any existing auth headers that could conflict (remove both "Authorization" and "x-api-key" from req.Header) so stale credentials added earlier by providerUtils.SetExtraHeaders(...) are not sent; then set the chosen header (either req.Header.Set("Authorization", "Bearer "+key) with appendBetaHeader(req, AnthropicOAuthBetaHeader) for OAuth or req.Header.Set("x-api-key", key) for API key). Ensure the same clear-then-set logic is applied to the other analogous blocks referenced (618-628 and 1113-1123).
320-334:⚠️ Potential issue | 🟠 MajorDon't collapse the zero-key case into the OAuth-only fallback.
len(apiKeys) == 0is also true when Anthropic has no keys configured at all. Returning the empty sentinel there triggers the HTTP-layer static catalog fallback, so an unusable provider can still look healthy. Only return the empty model list when at least one original key was actually filtered out as OAuth.🔧 Suggested fix
// Anthropic's /v1/models endpoint does not support OAuth tokens — filter them out. // OAuth-only setups will fall back to the static model catalog automatically. apiKeys := make([]schemas.Key, 0, len(keys)) + hasOAuthKeys := false for _, k := range keys { - if !isOAuthKey(k) { - apiKeys = append(apiKeys, k) + if isOAuthKey(k) { + hasOAuthKeys = true + continue } + apiKeys = append(apiKeys, k) } - if len(apiKeys) == 0 { + if len(apiKeys) == 0 && hasOAuthKeys { // OAuth keys cannot call /v1/models. Return empty response so server-level // fallback populates the static model catalog and the public HTTP endpoint // does not surface an error to clients. return &schemas.BifrostListModelsResponse{
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@core/providers/anthropic/anthropic.go`:
- Around line 2706-2712: The call to setAnthropicAuthHeader unconditionally
clears Authorization/x-api-key and thus wipes passthrough auth when no provider
key is selected; in the resolveKeyValue flow (the block calling
provider.resolveKeyValue and setAnthropicAuthHeader) modify the logic so that if
keyValue == "" you skip calling setAnthropicAuthHeader (or call a new mode/flag
on setAnthropicAuthHeader that preserves existing headers) so that
networkConfig.ExtraHeaders or req.SafeHeaders remain intact; apply the same
change to the other occurrence around the resolveKeyValue call near the
2786-2792 region to ensure empty-key paths do not remove existing auth headers.
---
Duplicate comments:
In `@core/providers/anthropic/anthropic.go`:
- Around line 175-181: Before setting the selected credential in the Anthropic
request branches (the block that checks key != "" && !IsClaudeCodeMaxMode(ctx)
and the similar blocks at the other locations), clear any existing auth headers
that could conflict (remove both "Authorization" and "x-api-key" from
req.Header) so stale credentials added earlier by
providerUtils.SetExtraHeaders(...) are not sent; then set the chosen header
(either req.Header.Set("Authorization", "Bearer "+key) with
appendBetaHeader(req, AnthropicOAuthBetaHeader) for OAuth or
req.Header.Set("x-api-key", key) for API key). Ensure the same clear-then-set
logic is applied to the other analogous blocks referenced (618-628 and
1113-1123).
🪄 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: CHILL
Plan: Pro
Run ID: 1861d253-0273-48e1-93ee-dae661054951
📒 Files selected for processing (7)
core/bifrost.gocore/providers/anthropic/anthropic.gocore/providers/anthropic/types.gocore/providers/anthropic/utils.gocore/schemas/account.gocore/schemas/provider.gocore/utils.go
✅ Files skipped from review due to trivial changes (2)
- core/providers/anthropic/types.go
- core/utils.go
🚧 Files skipped from review as they are similar to previous changes (2)
- core/schemas/provider.go
- core/bifrost.go
…elpers setAnthropicAuthHeader and setAnthropicAuthHeaderMap unconditionally deleted Authorization and x-api-key headers before checking if keyValue was empty. In passthrough mode with no configured key, this stripped pre-existing auth headers set via ExtraHeaders. Batch operations were unaffected because callers already guarded with `if keyValue != ""`. Signed-off-by: minicx <minicx@disroot.org>
ExtraHeaders from NetworkConfig are applied before auth headers in completeRequest. If ExtraHeaders contained an Authorization header and the key used x-api-key (or vice versa), both would be sent upstream. Add explicit Del calls before setting the chosen auth mechanism. Signed-off-by: minicx <minicx@disroot.org>
All OAuth handler errors returned 500 regardless of cause. Add sentinel errors (ErrOAuthConfigNotFound, ErrOAuthFlowExpired, ErrOAuthInvalidInput, ErrOAuthStateMismatch, ErrOAuthTokenNotFound, ErrOAuthNoLinkedToken) and map them to proper HTTP codes: 404 for not-found, 400 for client errors, 500 only for unexpected server failures. Signed-off-by: minicx <minicx@disroot.org>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
core/providers/anthropic/anthropic.go (1)
330-337:⚠️ Potential issue | 🟠 MajorDo not treat “no keys configured” as OAuth-only fallback.
At Line 330,
len(apiKeys) == 0also matches the “no keys exist” case, so this path can mask provider misconfiguration by returning an empty model list.🔧 Suggested fix
apiKeys := make([]schemas.Key, 0, len(keys)) + hasOAuthKeys := false for _, k := range keys { - if !isOAuthKey(k) { - apiKeys = append(apiKeys, k) - } + if isOAuthKey(k) { + hasOAuthKeys = true + continue + } + apiKeys = append(apiKeys, k) } - if len(apiKeys) == 0 { + if len(apiKeys) == 0 && hasOAuthKeys { // OAuth keys cannot call /v1/models. Return empty response so server-level // fallback populates the static model catalog and the public HTTP endpoint // does not surface an error to clients. return &schemas.BifrostListModelsResponse{ Data: make([]schemas.Model, 0), }, nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/providers/anthropic/anthropic.go` around lines 330 - 337, The current check "if len(apiKeys) == 0" treats both "no keys configured" and "OAuth-only" as the same, which can hide misconfiguration; change the condition in anthropic.go to only short-circuit and return an empty BifrostListModelsResponse when the provider is explicitly configured as OAuth-only (e.g., via an OAuth-only flag or config check), and otherwise treat an absence of apiKeys as a provider misconfiguration and return an error (or surface it) instead of silently returning an empty model list; update the branch that references apiKeys and the returned &schemas.BifrostListModelsResponse{...} accordingly so only the explicit OAuth-only path uses that empty-response behavior.framework/oauth2/main.go (1)
111-123:⚠️ Potential issue | 🔴 CriticalEnforce tenant/workspace ownership on
oauth_config_idlookups.These entrypoints still trust a bare
oauth_config_id. The currentGetOauthConfigByIDimplementation inframework/configstore/rdb.goresolves byidonly, so any authenticated caller that learns another tenant's ID can exchange, refresh, revoke, or inspect that OAuth flow. This needs an owner/workspace field on the config and scoped lookups before the new endpoints are safe to expose.Also applies to: 191-203, 837-842
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@framework/oauth2/main.go` around lines 111 - 123, The oauth handlers currently call p.configStore.GetOauthConfigByID(ctx, oauthConfigID) and trust the returned record; change to enforce workspace/tenant ownership by scoping the lookup or validating ownership: obtain the caller's workspace/tenant id from ctx (or the existing session helper), call a scoped lookup (e.g., GetOauthConfigByIDForWorkspace(ctx, oauthConfigID, workspaceID) or call GetOauthConfigByID and then assert oauthConfig.WorkspaceID/OwnerID == workspaceID), and return ErrOAuthConfigNotFound on mismatch; similarly scope or validate the token lookup (GetOauthTokenByID) to ensure the token belongs to that workspace before proceeding. Ensure references to oauthConfigID, GetOauthConfigByID, GetOauthConfigByIDForWorkspace (new), oauthConfig.WorkspaceID/OwnerID, and GetOauthTokenByID are updated consistently.transports/bifrost-http/handlers/anthropic_oauth.go (1)
111-115:⚠️ Potential issue | 🟡 Minor
/statuscan't currently distinguish invalid tokens from backend failures.
ValidateTokenreturns(false, nil)when the config or token lookup fails, so this branch never runs and a config-store outage will be reported as"invalid". Either propagate real errors fromValidateTokenor make this endpoint explicitly best-effort.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@transports/bifrost-http/handlers/anthropic_oauth.go` around lines 111 - 115, The /status handler currently calls h.oauthProvider.ValidateToken(ctx, oauthConfigID) which returns (false, nil) on backend/config lookup failures so outages are reported as "invalid"; change ValidateToken (and its implementations) to return a non-nil error for backend/config lookup failures (so it becomes `(bool, error)` with real errors for failures), then update the handler logic around h.oauthProvider.ValidateToken to treat err != nil as an internal server error (SendError StatusInternalServerError) and only treat valid==false with nil error as a true invalid-token response.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@framework/oauth2/main.go`:
- Around line 111-123: The code currently conflates store errors with "not
found" cases; update the fetch logic in the function that calls
p.configStore.GetOauthConfigByID and p.configStore.GetOauthTokenByID so you
check err first and return/propagate that error (or wrap it with context) when
err != nil, and only return ErrOAuthConfigNotFound or ErrOAuthTokenNotFound when
the returned value is nil; do the same pattern for the TokenID nil check. Apply
the identical split-of-cases fix in RevokeToken (the similar fetch/block around
lines 191-203) so store failures are not mapped to the sentinel NotFound errors.
- Around line 751-753: callTokenEndpointJSON currently returns a plain error on
any non-200 response which causes callers like CompleteAnthropicOAuthFlow to
lose status information and default oauthErrorStatus to 500; fix by introducing
a TokenEndpointError (or similar) type that carries StatusCode and Body and
implements error, change callTokenEndpointJSON (and the other similar token-call
site around the other occurrence) to return that structured error when
resp.StatusCode != http.StatusOK, and then change CompleteAnthropicOAuthFlow to
detect TokenEndpointError and map its StatusCode into oauthErrorStatus (use the
original 4xx status for client errors and 5xx fallback otherwise) so client-side
token rejections do not become 500s.
---
Duplicate comments:
In `@core/providers/anthropic/anthropic.go`:
- Around line 330-337: The current check "if len(apiKeys) == 0" treats both "no
keys configured" and "OAuth-only" as the same, which can hide misconfiguration;
change the condition in anthropic.go to only short-circuit and return an empty
BifrostListModelsResponse when the provider is explicitly configured as
OAuth-only (e.g., via an OAuth-only flag or config check), and otherwise treat
an absence of apiKeys as a provider misconfiguration and return an error (or
surface it) instead of silently returning an empty model list; update the branch
that references apiKeys and the returned &schemas.BifrostListModelsResponse{...}
accordingly so only the explicit OAuth-only path uses that empty-response
behavior.
In `@framework/oauth2/main.go`:
- Around line 111-123: The oauth handlers currently call
p.configStore.GetOauthConfigByID(ctx, oauthConfigID) and trust the returned
record; change to enforce workspace/tenant ownership by scoping the lookup or
validating ownership: obtain the caller's workspace/tenant id from ctx (or the
existing session helper), call a scoped lookup (e.g.,
GetOauthConfigByIDForWorkspace(ctx, oauthConfigID, workspaceID) or call
GetOauthConfigByID and then assert oauthConfig.WorkspaceID/OwnerID ==
workspaceID), and return ErrOAuthConfigNotFound on mismatch; similarly scope or
validate the token lookup (GetOauthTokenByID) to ensure the token belongs to
that workspace before proceeding. Ensure references to oauthConfigID,
GetOauthConfigByID, GetOauthConfigByIDForWorkspace (new),
oauthConfig.WorkspaceID/OwnerID, and GetOauthTokenByID are updated consistently.
In `@transports/bifrost-http/handlers/anthropic_oauth.go`:
- Around line 111-115: The /status handler currently calls
h.oauthProvider.ValidateToken(ctx, oauthConfigID) which returns (false, nil) on
backend/config lookup failures so outages are reported as "invalid"; change
ValidateToken (and its implementations) to return a non-nil error for
backend/config lookup failures (so it becomes `(bool, error)` with real errors
for failures), then update the handler logic around
h.oauthProvider.ValidateToken to treat err != nil as an internal server error
(SendError StatusInternalServerError) and only treat valid==false with nil error
as a true invalid-token response.
🪄 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: CHILL
Plan: Pro
Run ID: d184de3f-b27c-4634-a56b-2f017a64f39d
📒 Files selected for processing (4)
core/providers/anthropic/anthropic.gocore/providers/anthropic/utils.goframework/oauth2/main.gotransports/bifrost-http/handlers/anthropic_oauth.go
✅ Files skipped from review due to trivial changes (1)
- core/providers/anthropic/utils.go
When GetOauthConfigByID or GetOauthTokenByID returned a database error, it was collapsed with the nil-result case into ErrOAuthConfigNotFound / ErrOAuthTokenNotFound. This caused store outages to surface as 404s instead of 500s. Split the err != nil and nil checks so DB failures propagate as untyped errors (→ 500) while missing records use sentinels. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
callTokenEndpointJSON returned a plain error for all non-200 responses from the OAuth provider. When Anthropic rejected an invalid/expired authorization code (4xx), the handler fell through to 500. Add ErrOAuthTokenExchangeFailed sentinel for 4xx provider responses and map it to 400 in the HTTP handler. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
ValidateToken returned (false, nil) for both missing records and database failures, making config-store outages indistinguishable from invalid tokens. Split the error and nil cases so store failures propagate to the caller and surface as 500 instead of "invalid". Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@framework/oauth2/main.go`:
- Around line 857-930: CompleteAnthropicOAuthFlow can flip an already-authorized
oauth_config to "failed" on retries and can orphan tokens when CreateOauthToken
succeeds but UpdateOauthConfig fails; before exchanging the code (after
GetOauthConfigByID) guard against oauthConfig.Status == "authorized" and return
early, and make the token insert + config update atomic by using a transactional
operation on configStore (e.g., a RunInTx/BeginTx wrapper) that calls
CreateOauthToken and UpdateOauthConfig inside one transaction (or, if
transactions aren't supported, perform compensating cleanup by deleting the
newly created token with CreateOauthToken ID when UpdateOauthConfig fails); also
avoid setting oauthConfig.Status="failed" if it is already "authorized" so
retries do not break existing links.
🪄 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: CHILL
Plan: Pro
Run ID: 2450b84d-80e4-477a-a03f-2453080ffc9b
📒 Files selected for processing (2)
framework/oauth2/main.gotransports/bifrost-http/handlers/anthropic_oauth.go
🚧 Files skipped from review as they are similar to previous changes (1)
- transports/bifrost-http/handlers/anthropic_oauth.go
…n partial failure A retried exchange request with an already-authorized config would re-send the spent authorization code, fail, and flip the config to "failed" — bricking the existing valid token. Guard against this by returning early when the config is already authorized. Also clean up orphaned tokens when CreateOauthToken succeeds but the subsequent UpdateOauthConfig fails, preventing credential leaks in storage. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
framework/oauth2/main.go (2)
736-740: Consider adding an explicit provider type field in the future.The heuristic (
ClientSecret == "" && strings.Contains(TokenURL, "anthropic")) works for now, but could become fragile if:
- Another provider uses "anthropic" in their URL
- Anthropic changes their token URL
For robustness, consider adding an explicit
ProviderorOAuthTypefield toTableOauthConfigin a future iteration to distinguish OAuth providers unambiguously.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@framework/oauth2/main.go` around lines 736 - 740, The current heuristic in isAnthropicOAuthConfig (checking TableOauthConfig.ClientSecret == "" and strings.Contains on TableOauthConfig.TokenURL) is fragile; update TableOauthConfig to add an explicit Provider or OAuthType field and change isAnthropicOAuthConfig to check that new field (e.g., config.Provider == "anthropic") instead of relying on ClientSecret/TokenURL contents; make sure to update any code that constructs or reads TableOauthConfig to populate the new field and add migration/validation where configs are created or loaded so existing entries default to a sensible value.
56-64: Minor inconsistency in sentinel error usage.
GetAccessTokenusesschemas.ErrOAuth2ConfigNotFound(line 63) while other functions in this file use the newErrOAuthConfigNotFoundsentinel. This may be intentional for interface compatibility withschemas.OAuth2Provider, but consider aligning error types if the interface contract allows, to simplify error handling for callers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@framework/oauth2/main.go` around lines 56 - 64, GetAccessToken currently returns the old sentinel schemas.ErrOAuth2ConfigNotFound when oauthConfig is nil; align it with the rest of the file by returning the new ErrOAuthConfigNotFound sentinel (or update the file-wide usage to the schema sentinel if the schemas.OAuth2Provider interface requires the old one). Locate GetAccessToken and the nil-check after configStore.GetOauthConfigByID and replace the returned error to match the project's chosen sentinel (ErrOAuthConfigNotFound) so callers see a consistent error type across methods.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@framework/oauth2/main.go`:
- Around line 736-740: The current heuristic in isAnthropicOAuthConfig (checking
TableOauthConfig.ClientSecret == "" and strings.Contains on
TableOauthConfig.TokenURL) is fragile; update TableOauthConfig to add an
explicit Provider or OAuthType field and change isAnthropicOAuthConfig to check
that new field (e.g., config.Provider == "anthropic") instead of relying on
ClientSecret/TokenURL contents; make sure to update any code that constructs or
reads TableOauthConfig to populate the new field and add migration/validation
where configs are created or loaded so existing entries default to a sensible
value.
- Around line 56-64: GetAccessToken currently returns the old sentinel
schemas.ErrOAuth2ConfigNotFound when oauthConfig is nil; align it with the rest
of the file by returning the new ErrOAuthConfigNotFound sentinel (or update the
file-wide usage to the schema sentinel if the schemas.OAuth2Provider interface
requires the old one). Locate GetAccessToken and the nil-check after
configStore.GetOauthConfigByID and replace the returned error to match the
project's chosen sentinel (ErrOAuthConfigNotFound) so callers see a consistent
error type across methods.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6cde23a5-7ad6-4906-8c40-68ec508a2a21
📒 Files selected for processing (1)
framework/oauth2/main.go
|
Any update on this ? |
Contributor
Author
I'm tired of constantly merging. |
|
@akshaydeo Is there anything blocking this initiative ? |
Contributor
|
The legalities. Anthropic service agreement says the subscriptions are only allowed to use via their own properties like Claude desktop, Claude code. We and in-turn you will be violating those terms if we add this. We are getting in touch with them to understand it better |
Confuses me - I'm right now testing OAuth based Claude Code via bifrost without this PR here. Only thing I had to do is add the Anthrophic provider, leave out the key - and it works out of the box. It even works out of the box with Claude Code when I As long as you only use Claude Code or Claude Desktop this should be a non-issue as Anthrophic supports proxy and different API URIs dont they? However this here has the same issue that the Alternative Codex OAuth integration has against my passtrough implementation - it embeds the authentication layer inside bifrost, no? - and I guess both will have legal and maintenance issues (because they do to much and shift responsibility towards bifrost - thats at least my understanding of the problems). So maybe its rather a design issue ... and for Claude Code a documentation lack of things. |
|
Wondering if this could be refactored as a plugin and not as part of the core, making it optional for those wanting to use it. |
Contributor
Author
Anyway anthropic changed their api, i made new one version |
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
6 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds Anthropic OAuth support for Claude Pro/Max subscriptions as a provider key authentication method in Bifrost. Users can connect their Claude Pro or Max account via OAuth 2.0 flow and use it as an alternative to static API keys for Anthropic provider.
Changes
framework/oauth2,transports/bifrost-http/handlers/anthropic_oauth.go): OAuth 2.0 initiate/exchange/refresh/logout endpoints for Anthropic's authorization flowcore/providers/anthropic/anthropic.go): resolve OAuth tokens at request time viaresolveKeyValue, setAuthorization: Bearerheader + requiredanthropic-beta: oauth-2025-04-20header for OAuth keys; usex-api-keyfor static keyscore/bifrost.go,core/utils.go): allow Anthropic keys with emptyvaluewhenAnthropicOAuthKeyConfigis presentcore/schemas/account.go,core/schemas/provider.go): addAnthropicOAuthKeyConfigstruct withOAuthConfigIDfieldframework/configstore/): migration to addanthropic_oauth_config_idcolumn; preserveAnthropicOAuthKeyConfigacross all key-write paths (UpdateProvidersConfig, UpdateProvider, AddProvider)ui/app/workspace/providers/fragments/apiKeysFormFragment.tsx): authentication method tabs (API Key / OAuth) for Anthropic provider, OAuth connect/disconnect/refresh flow, OAuth badge on keys tablesetAnthropicAuthHeader/setAnthropicAuthHeaderMapwhere the else branch called itself instead of settingx-api-keyNotable design decisions:
/v1/modelslisting since the endpoint doesn't support OAuth tokens — falls back to static model cataloganthropic-beta: oauth-2025-04-20header is appended (not overwritten) to preserve other beta featuresType of change
Affected areas
How to test
Screenshots/Recordings
Breaking changes
Related issues
#1390
Security considerations
Checklist
docs/contributing/README.mdand followed the guidelines