fix(core): set x-api-key alongside Authorization on Anthropic outbound (#4323)#4342
Merged
Conversation
Contributor
📋 Review SummaryThis PR fixes a compatibility issue where Anthropic-compatible servers (OpenCode-Go, Claude proxy products) that only authenticate via the canonical 🔍 General Feedback
🎯 Specific Feedback🔵 Low
✅ Highlights
|
Contributor
Code Coverage Summary
CLI Package - Full Text ReportCore Package - Full Text ReportFor detailed HTML reports, please see the 'coverage-reports-22.x-ubuntu-latest' artifact from the main CI run. |
wenshao
reviewed
May 20, 2026
| // `x-api-key` here (post-buildHeaders) so customHeaders can't override | ||
| // it and the env back-fill suppression (apiKey: null on the SDK side) | ||
| // is preserved. The value is the user's explicitly-configured key — | ||
| // never an env-resolved one — so this can't widen the #4020 leak. |
Collaborator
There was a problem hiding this comment.
[Suggestion] The comment states "The value is the user's explicitly-configured key — never an env-resolved one" but contentGeneratorConfig.apiKey CAN be env-resolved upstream via modelConfigResolver (the resolveCredentialField function falls through to process.env[envKey]). The overall security conclusion is still correct — the same credential is already sent as Authorization: Bearer on this request — but the stated reasoning is factually wrong and could mislead future maintainers about the data flow.
Suggested change
| // never an env-resolved one — so this can't widen the #4020 leak. | |
| // is preserved. The value is the same key already sent as | |
| // `Authorization: Bearer` via `authToken` on this request, so adding | |
| // it here doesn't widen the #4020 leak surface. |
— qwen-latest-series-invite-beta-v34 via Qwen Code /review
Collaborator
Author
There was a problem hiding this comment.
Fixed in a24bc7e. Verified via resolveCredentialField at packages/core/src/models/content-generator-config.ts:178-196 — it does fall through to process.env[envKey] on line 192. The original comment was misleading. Rewrote the block to anchor the security argument on the actual invariant: whatever contentGeneratorConfig.apiKey resolved to is the same value already shipped as Authorization: Bearer via authToken on this very request, so adding it as x-api-key doesn't widen the #4020 leak surface. The SDK-level env back-fill suppression (passing apiKey: null to the SDK constructor) is still in place — that's a separate concern from upstream resolution.
| // it and the env back-fill suppression (apiKey: null on the SDK side) | ||
| // is preserved. The value is the user's explicitly-configured key — | ||
| // never an env-resolved one — so this can't widen the #4020 leak. | ||
| if (useProxyIdentity && contentGeneratorConfig.apiKey) { |
Collaborator
There was a problem hiding this comment.
[Suggestion] No test covers the falsy-apiKey branch on the proxy path. All proxy-branch tests supply a truthy apiKey. Consider adding a test that verifies x-api-key is NOT injected when apiKey is undefined or '' on a proxy baseURL — a future refactor could break the truthy guard without any test catching it.
— qwen-latest-series-invite-beta-v34 via Qwen Code /review
Collaborator
Author
There was a problem hiding this comment.
Fixed in a24bc7e — added does NOT add x-api-key when apiKey is falsy on the proxy branch. Constructs with apiKey: '' on a proxy baseURL, asserts defaultHeaders['x-api-key'] is undefined. A future refactor that loosens apiKey?: string to a required field (re-enabling the empty-string ship) would now flip this test.
| expect(headers['User-Agent']).toContain('claude-cli/1.2.3'); | ||
| expect(headers['User-Agent']).toContain('(external, cli)'); | ||
| expect(headers['x-app']).toBe('cli'); | ||
| expect(headers['x-api-key']).toBe('test-key'); |
Collaborator
There was a problem hiding this comment.
[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 asserts headers['x-api-key'] equals the canonical key — this guards against a future refactor accidentally moving the x-api-key assignment before the customHeaders merge.
— qwen-latest-series-invite-beta-v34 via Qwen Code /review
Collaborator
Author
There was a problem hiding this comment.
Fixed in a24bc7e — added customHeaders cannot override x-api-key on the proxy branch (post-buildHeaders ordering invariant). Constructs with customHeaders: { 'x-api-key': 'user-override' } and apiKey: 'canonical-key', asserts the canonical key wins. A refactor that moves the injection above the customHeaders merge inside buildHeaders would now flip this test. Good catch — the comment promised the invariant but nothing pinned it.
#4323) On the IdeaLab-style proxy branch, the Anthropic SDK is constructed with `authToken: <key>, apiKey: null` so it emits `Authorization: Bearer <key>` and suppresses the ANTHROPIC_API_KEY env back-fill (the #4020 leak fix). That covers IdeaLab and CherryStudio-style proxies, but standards- compliant Anthropic-compatible servers (OpenCode-Go, Claude proxy products) authenticate only on the canonical `x-api-key` header and reject the request with "Missing API key" even though the bearer token is present. Inject `x-api-key: <key>` into `defaultHeaders` on the proxy branch (post-`buildHeaders`, so customHeaders cannot override it). The value is the user's already-configured `apiKey` — never an env-resolved one — so the #4020 env-leak vector stays closed. The Anthropic-native branch is untouched: the SDK's apiKey path already emits the header, and duplicating it via defaultHeaders would risk stale-value drift. Verified: - new unit test pins `x-api-key: <key>` on every proxy-branch case (config-baseUrl, malformed baseUrl, DeepSeek anthropic-compat, ANTHROPIC_BASE_URL env-pointed-at-proxy); a negative test pins that the native branch does NOT add the header. - E2E: spun up a local `http.createServer`, pointed the SDK at it the same way `AnthropicContentGenerator` does, and dumped the captured wire headers — `Authorization: Bearer` and `x-api-key` both arrive alongside the existing X-Stainless-* / x-app / claude-cli UA trio. Fixes #4323
LaZzyMan
force-pushed
the
lazzy/admiring-banach-bf7ca8
branch
from
880a1b2 to
f8ee4b8
Compare
…ders ordering (#4323) Address review feedback on #4342: - Source comment claimed the apiKey value was "never an env-resolved one"; that's wrong — `resolveCredentialField` in content-generator-config.ts:178 falls through to env vars when the explicit and inherited values are unset. The security reasoning doesn't actually depend on that claim (the same value already ships as `Authorization: Bearer` via `authToken` on the same request), so re-anchor the comment on that fact and drop the misleading "never env-resolved" framing. - Add test pinning the `&& contentGeneratorConfig.apiKey` guard: a falsy apiKey on the proxy branch must NOT inject `x-api-key:` (empty string would otherwise ship a meaningless header). The TypeScript signature `apiKey?: string` keeps the guard needed at the type level, but a future loosen-the-type refactor would silently re-enable the empty ship; the test catches that. - Add test pinning the post-buildHeaders ordering: a user-supplied `customHeaders: { 'x-api-key': … }` must NOT win against the canonical key. The source comment promises this invariant but no test pinned it; a refactor that moved the injection above the customHeaders merge would silently let user config swap the auth header, defeating the dual-auth contract. Declined two suggestions: - Bot suggested extracting the 3-line injection into a `buildApiKeyHeader()` helper for consistency. Declined: adds indirection without abstraction win, and the inline form keeps the post-buildHeaders ordering visible at the call site (the ordering IS the invariant the comment promises). - Bot suggested asserting `Authorization` is absent from `defaultHeaders` on the native path. Declined: the constructor-options pins (`apiKey: 'test-key'`, `authToken: null`) already document the SDK-driven auth mode; asserting on the absence of a header we never set in defaultHeaders is redundant given the existing assertions. 68 tests pass (66 + 2 new). tsc + eslint clean.
Collaborator
Author
Round-2 outcomes — commit a24bc7eInline review (@wenshao via Qwen Code /review) — all three accepted:
Review Summary feedback (github-actions bot) — two LOW-priority suggestions declined:
Verification
|
wenshao
approved these changes
May 20, 2026
Collaborator
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
No issues found. LGTM! ✅ — qwen-latest-series-invite-beta-v34 via Qwen Code /review
wenshao
approved these changes
May 20, 2026
Collaborator
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
No issues found. LGTM! ✅ — gpt-5.5 via Qwen Code /review
wenshao
approved these changes
May 21, 2026
Collaborator
Maintainer test report — PR #4342Built and validated locally in a dedicated Environment
Results
The one failing test — pre-existing, NOT caused by this PRThis test is unchanged by PR #4342 — it just shifted from line 179 to line 262 because the PR added three tests above it. I previously reproduced the same single failure on a clean E2E #2 — captured wire headers (production code path)This is exactly the IdeaLab transparent-proxy capture shape the reporter cited in #4323, plus the new Behavioral checks against the diff
Risk assessmentLow risk for merge.
Not covered locally
Reproducegit fetch origin pull/4342/head:pr-4342 && git checkout pr-4342
npm install
cd packages/core && npx vitest run \
src/core/anthropicContentGenerator/anthropicContentGenerator.test.ts
# Then for the production-path E2E (optional, no credentials needed):
# - it spins up http.createServer on 127.0.0.1 and dumps captured wire headersRecommendation: safe to merge. The pre-existing Anthropic native-UA test failure is independent and should be tracked separately (it predates this PR and is reproducible on clean |
This was referenced May 21, 2026
LaZzyMan
added a commit
that referenced
this pull request
May 21, 2026
… outbound (#4323) (#4342)" (#4385) This reverts commit 4b25f9c. PR #4342 unconditionally injected `x-api-key` alongside `Authorization: Bearer` on every proxy-branch request. This broke IdeaLab-style proxies (e.g. `idealab.alibaba-inc.com/api/anthropic`) which reject requests carrying both headers with HTTP 401: 鉴权header, x-api-key和Authorization不可以同时存在 (auth header: x-api-key and Authorization cannot coexist) The two proxy families have mutually exclusive header contracts — OpenCode-Go-style servers want `x-api-key` only, IdeaLab-style servers want `Authorization` only — so a one-size-fits-all default cannot satisfy both at once. Reverting restores the pre-#4342 default (Bearer only) so IdeaLab users are unblocked. OpenCode-Go-style users can opt in via `customHeaders`: { "customHeaders": { "x-api-key": "<key>" } } `buildHeaders` already merges customHeaders into defaultHeaders (only `anthropic-beta` is reserved for per-request handling), and on the proxy branch the SDK is constructed with `apiKey: null` so it does not emit its own `x-api-key` — the value on the wire comes solely from the user's explicit customHeaders entry, preserving the #4020 env-leak guard. Reopens #4323.
DragonnZhang
pushed a commit
that referenced
this pull request
May 21, 2026
… outbound (#4323) (#4342)" (#4385) This reverts commit 4b25f9c. PR #4342 unconditionally injected `x-api-key` alongside `Authorization: Bearer` on every proxy-branch request. This broke IdeaLab-style proxies (e.g. `idealab.alibaba-inc.com/api/anthropic`) which reject requests carrying both headers with HTTP 401: 鉴权header, x-api-key和Authorization不可以同时存在 (auth header: x-api-key and Authorization cannot coexist) The two proxy families have mutually exclusive header contracts — OpenCode-Go-style servers want `x-api-key` only, IdeaLab-style servers want `Authorization` only — so a one-size-fits-all default cannot satisfy both at once. Reverting restores the pre-#4342 default (Bearer only) so IdeaLab users are unblocked. OpenCode-Go-style users can opt in via `customHeaders`: { "customHeaders": { "x-api-key": "<key>" } } `buildHeaders` already merges customHeaders into defaultHeaders (only `anthropic-beta` is reserved for per-request handling), and on the proxy branch the SDK is constructed with `apiKey: null` so it does not emit its own `x-api-key` — the value on the wire comes solely from the user's explicit customHeaders entry, preserving the #4020 env-leak guard. Reopens #4323.
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
4 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
authToken: <key>, apiKey: null, which emitsAuthorization: Bearer <key>and suppressesANTHROPIC_API_KEYenv back-fill (the feat(core): improve Anthropic proxy compatibility and enable global prompt cache scope #4020 leak fix). That works for IdeaLab and proxies that readAuthorization, but standards-compliant Anthropic-compatible servers (OpenCode-Go, Claude proxy products) authenticate only on the canonicalx-api-keyheader and reject the request with "Missing API key" even though the bearer is present.x-api-key: <key>intodefaultHeaderson the proxy branch alongside the existing Bearer auth so either family of compatible server accepts us. The value is the user's already-configuredapiKey— never an env-resolved one — so the feat(core): improve Anthropic proxy compatibility and enable global prompt cache scope #4020 env-leak vector stays closed. The Anthropic-native branch is untouched (the SDK's apiKey path already emits the header there).x-api-keyand authenticates fine against the same server. After this change qwen-code matches that contract while keepingAuthorization: Bearerfor backward compatibility.Test plan
vitest run src/core/anthropicContentGenerator/anthropicContentGenerator.test.ts— 66 tests pass; new positive case pinsx-api-key: <key>on every proxy-branch path (config-baseUrl, malformed baseUrl, DeepSeek anthropic-compat,ANTHROPIC_BASE_URLenv-pointed-at-proxy) and new negative case pins that the native branch does NOT add the header.vitest run(full core suite) — 8602 tests pass / 3 skipped.eslint src/core/anthropicContentGenerator/— clean.tsc --noEmitinpackages/core— clean.http.createServeron 127.0.0.1, pointednew Anthropic({ baseURL, authToken, apiKey: null, defaultHeaders: { …, 'x-api-key': key } })at it (the exact shapeAnthropicContentGeneratornow emits), firedmessages.create, and dumped the captured wire headers. Result matches the issue reporter's transparent-proxy capture (claude-cli UA, X-Stainless-* trio,Authorization: Bearer,x-app: cli) plus the newx-api-key: <key>line, confirming the SDK forwards defaultHeaders to the wire and the anthropic Missing API key #4323 server's check now passes.Fixes #4323