-
Notifications
You must be signed in to change notification settings - Fork 2.4k
fix(core): set x-api-key alongside Authorization on Anthropic outbound (#4323) #4342
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -175,6 +175,22 @@ export class AnthropicContentGenerator implements ContentGenerator { | |
| // half of the bundle without the other. | ||
| const useProxyIdentity = !isAnthropicNativeBaseUrl(contentGeneratorConfig); | ||
| const defaultHeaders = this.buildHeaders(useProxyIdentity); | ||
| // On the proxy branch the SDK is constructed with `authToken` so it | ||
| // emits `Authorization: Bearer <key>` natively, but some | ||
| // Anthropic-compatible servers (OpenCode-Go, Claude proxy products — | ||
| // see #4323) authenticate only on the canonical `x-api-key` header. | ||
| // Ship both shapes side-by-side so either family accepts us. We add | ||
| // `x-api-key` here (post-buildHeaders) so customHeaders can't override | ||
| // it and the SDK-level env back-fill suppression (apiKey: null on the | ||
| // SDK side, suppressing the SDK's own ANTHROPIC_API_KEY destructuring | ||
| // default) is preserved. `contentGeneratorConfig.apiKey` itself may | ||
| // have been env-resolved upstream by `resolveCredentialField`, but | ||
| // that's the same value already shipped as `Authorization: Bearer` | ||
| // via `authToken` on this very request — adding it as `x-api-key` | ||
| // doesn't widen the #4020 leak surface. | ||
| if (useProxyIdentity && contentGeneratorConfig.apiKey) { | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Suggestion] No test covers the falsy- — qwen-latest-series-invite-beta-v34 via Qwen Code /review
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed in a24bc7e — added |
||
| defaultHeaders['x-api-key'] = contentGeneratorConfig.apiKey; | ||
| } | ||
| const baseURL = contentGeneratorConfig.baseUrl; | ||
| // Configure fetch options for proxy support and timeout handling. | ||
| // With proxy, dispatcher timeouts are disabled so SDK timeout controls the | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[Suggestion] The code comment in the source explicitly states "customHeaders can't override it" but no test verifies this invariant. Consider adding a test that constructs with
customHeaders: { 'x-api-key': 'user-override' }on a proxy baseURL and assertsheaders['x-api-key']equals the canonical key — this guards against a future refactor accidentally moving thex-api-keyassignment before the customHeaders merge.— qwen-latest-series-invite-beta-v34 via Qwen Code /review
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in a24bc7e — added
customHeaders cannot override x-api-key on the proxy branch (post-buildHeaders ordering invariant). Constructs withcustomHeaders: { 'x-api-key': 'user-override' }andapiKey: 'canonical-key', asserts the canonical key wins. A refactor that moves the injection above the customHeaders merge insidebuildHeaderswould now flip this test. Good catch — the comment promised the invariant but nothing pinned it.