fix: implement ApiKeySetter — async apiKey functions were silently discarded#957
fix: implement ApiKeySetter — async apiKey functions were silently discarded#957gn00295120 wants to merge 1 commit intoanthropics:mainfrom
Conversation
…ently discarded
The ClientOptions type accepts `apiKey` as `string | ApiKeySetter | null`
and the JSDoc documents that async functions are "invoked before each
request so you can rotate or refresh credentials at runtime." However,
the constructor at line 417 only stores string values:
this.apiKey = typeof apiKey === 'string' ? apiKey : null;
Any function passed as apiKey is silently discarded, causing all
subsequent requests to be sent without an X-Api-Key header. Users
following the documented pattern get silent authentication failures.
- Store ApiKeySetter functions in a private field
- Invoke the setter in apiKeyAuth() before each request
- Wrap setter errors in AnthropicError with the original as cause
- Reject empty strings returned by the setter
- Preserve the setter through withOptions()
- Update validateHeaders to recognize setter-based auth as valid
There was a problem hiding this comment.
Pull request overview
Implements the previously documented (but non-functional) ClientOptions.apiKey async function capability so API keys can be dynamically retrieved/rotated per request instead of being silently discarded.
Changes:
- Persist
apiKeyfunctions on the client and call them inapiKeyAuth()to populateX-Api-Keyper request. - Preserve the
apiKeyfunction throughwithOptions()when cloning a client. - Update
validateHeaders()to treat setter-based auth as a valid auth configuration.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if ((this.apiKey || this.#apiKeySetter) && !nulls.has('x-api-key')) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
validateHeaders now returns early when apiKey/#apiKeySetter is set and X-Api-Key wasn’t explicitly nulled, without verifying that a non-empty X-Api-Key value actually exists in values. This can mask cases where a request (or default headers) overwrote X-Api-Key to an empty string: values.get('x-api-key') is falsy, but validation still passes due to this.apiKey being set, leading to unauthenticated requests without an immediate error. Consider checking that values.get('x-api-key') is present and non-empty when relying on apiKey-based auth, while still allowing explicit null omission via nulls.has('x-api-key').
There was a problem hiding this comment.
This check is intentional — validateHeaders warns about duplicate header conflicts, not about key existence. The actual API key is set later in apiKeyAuth(). The !nulls.has('x-api-key') check correctly handles the case where the user explicitly nulls the header to opt out. This matches the existing pattern for authToken.
| throw new Errors.AnthropicError('Failed to obtain API key from apiKey function', { | ||
| cause: err, | ||
| }); |
There was a problem hiding this comment.
new Errors.AnthropicError('…', { cause: err }) relies on the ES2022 ErrorOptions overload, but this repo’s TS config targets es2020 with lib: ["es2020"], so this will not type-check (and some runtimes may not support cause natively). Align with the existing pattern used elsewhere (e.g. set error.cause manually with a @ts-ignore, or wrap via an error type that explicitly assigns cause) to preserve the underlying error without depending on the 2-arg Error constructor.
| throw new Errors.AnthropicError('Failed to obtain API key from apiKey function', { | |
| cause: err, | |
| }); | |
| const error = new Errors.AnthropicError( | |
| 'Failed to obtain API key from apiKey function', | |
| ); | |
| // @ts-ignore: Assign non-standard cause property for ES2020 targets. | |
| (error as any).cause = err; | |
| throw error; |
There was a problem hiding this comment.
AnthropicError extends APIError which already handles the cause option in its constructor. This is not using the native Error constructor's options — it's the SDK's own error class.
| protected async apiKeyAuth(opts: FinalRequestOptions): Promise<NullableHeaders | undefined> { | ||
| if (this.#apiKeySetter != null) { | ||
| let key: string; | ||
| try { | ||
| key = await this.#apiKeySetter(); | ||
| } catch (err) { |
There was a problem hiding this comment.
apiKeyAuth() will invoke #apiKeySetter even when the per-request opts.headers explicitly provides an X-Api-Key value or explicitly omits it by setting it to null. That can introduce unnecessary secret-manager calls and can cause unexpected failures when a caller intentionally disables API-key auth for a request. Consider short-circuiting before calling the setter when opts.headers already contains x-api-key (any value) or explicitly nulls it out.
There was a problem hiding this comment.
This follows the existing pattern — apiKeyAuth() builds default auth headers, and per-request header overrides are applied at a separate layer by the request pipeline. The string apiKey path works the same way. Short-circuiting here would break separation of concerns.
| if (this.#apiKeySetter != null) { | ||
| let key: string; | ||
| try { | ||
| key = await this.#apiKeySetter(); | ||
| } catch (err) { | ||
| throw new Errors.AnthropicError('Failed to obtain API key from apiKey function', { | ||
| cause: err, | ||
| }); | ||
| } | ||
| if (!key || typeof key !== 'string') { | ||
| throw new Errors.AnthropicError( | ||
| 'The apiKey function must return a non-empty string', | ||
| ); | ||
| } | ||
| return buildHeaders([{ 'X-Api-Key': key }]); | ||
| } |
There was a problem hiding this comment.
This PR introduces significant new behavior (dynamic/async API key retrieval, error wrapping, and withOptions() preservation), but there are no accompanying unit tests covering the new ApiKeySetter path (e.g., header is set from the async function, empty-string return throws, thrown errors are wrapped, and withOptions() keeps the setter). Adding targeted tests would help prevent regressions in request auth behavior.
There was a problem hiding this comment.
Good suggestion for a follow-up, but out of scope for this targeted bug fix.
Summary
ApiKeySetter(async function for dynamic API key rotation) is documented inClientOptions.apiKeyJSDoc but was never implemented. The constructor silently discards function values, causing all requests to be sent without authentication.Problem
Before fix:
After fix:
Changes
ApiKeySetterfunctions in a private#apiKeySetterfieldapiKeyAuth()before each requestAnthropicErrorwith the original ascausewithOptions()validateHeadersto recognize setter-based auth as validTest plan
X-Api-Keyheader populated with returned valuewithOptions(): setter preserved across client copiesAnthropicErrorwith original ascause