fix(responses): reject function-based API keys in WebSocket constructors - #2586
fix(responses): reject function-based API keys in WebSocket constructors#2586sylvesterkaczmarek wants to merge 5 commits into
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 395125547e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| 'ResponsesWS from "openai/resources/responses/ws" requires the "ws" package but it could not be loaded.', | ||
| ); | ||
| } | ||
| if (typeof client._options.apiKey === 'function') { |
There was a problem hiding this comment.
When a function-backed client has already run _callApiKey(), client.apiKey contains the resolved token while _options.apiKey intentionally remains a function. This condition therefore rejects both stable and beta constructors before _authHeaders() can send that token, breaking the rotating Bedrock Responses WebSocket path already exercised in tests/bedrock-websocket-security.test.ts:251-269, as well as similarly pre-resolved OpenAI credentials. Gate on the unresolved state or provide an async factory/bypass analogous to the Realtime surfaces.
AGENTS.md reference: AGENTS.md:L50-L57
Useful? React with 👍 / 👎.
| 'ResponsesWS from "openai/resources/responses/ws" requires the "ws" package but it could not be loaded.', | ||
| ); | ||
| } | ||
| if (typeof client._options.apiKey === 'function') { |
There was a problem hiding this comment.
This guard covers only the two concrete Node constructors, but ResponsesWSBase remains available through the published openai/resources/{,beta/}responses/ws-base import subpaths and exposes _connectInitial() to subclasses. A custom transport subclass can therefore pass an unresolved function-backed client to the base and still reach _authHeaders(), which returns no authorization header, preserving the unauthenticated-handshake bug this change is meant to prevent. Enforce the unresolved-provider invariant in the shared base/authentication boundary instead of only these callers.
AGENTS.md reference: AGENTS.md:L31-L35
Useful? React with 👍 / 👎.
HAYDEN-OAI
left a comment
HAYDEN-OAI
left a comment
There was a problem hiding this comment.
The fail-fast behavior is useful, but please preserve already-resolved function credentials before merging. I confirmed the regression described in the existing inline finding: _callApiKey() stores the token in client.apiKey and leaves _options.apiKey as a function, so the new check rejects a client that the existing WebSocket authentication path can use successfully.
Please enforce the unresolved-credential condition at the shared authentication boundary, preserving already-resolved credentials and supported provider authentication on both stable and beta surfaces. An async construction path that resolves credentials before connecting is another option. The important behavior is to prevent an unauthenticated connection without rejecting a usable resolved token.
Static review of 395125547ef1fce61aa1213bd32bf0345b4f707d; no tests run.
HAYDEN-OAI
left a comment
HAYDEN-OAI
left a comment
There was a problem hiding this comment.
Requesting changes for two issues affecting both stable and beta Responses WebSocket constructors: the new protected-member access does not compile, and the guard rejects provider clients even when their credential has already been resolved. A focused TypeScript compiler check confirmed TS2445 in both changed files; the credential regression was traced through the client and WebSocket authentication paths. No tests were run.
| 'ResponsesWS from "openai/resources/responses/ws" requires the "ws" package but it could not be loaded.', | ||
| ); | ||
| } | ||
| if (typeof client._options.apiKey === 'function') { |
There was a problem hiding this comment.
[P1] Avoid accessing the protected client options directly
OpenAI._options is declared protected in src/client.ts, and ResponsesWS does not extend OpenAI. This new access therefore produces TS2445 and prevents the source from compiling. A focused compiler check confirmed the same error at line 23 in both the stable and beta files. Use an accessible client method or helper to inspect credential configuration, and apply the correction to both constructors.
| 'ResponsesWS from "openai/resources/beta/responses/ws" requires the "ws" package but it could not be loaded.', | ||
| ); | ||
| } | ||
| if (typeof client._options.apiKey === 'function') { |
There was a problem hiding this comment.
[P2] Preserve already-resolved provider credentials
After a normal REST request or await client._callApiKey(), a function-backed client has the resolved token in client.apiKey, while _options.apiKey remains a function. Reusing that client for Responses WebSocket previously authenticated successfully through _authHeaders(), but this guard now always throws. This also affects BedrockOpenAI configured with bedrockTokenProvider. Unlike Realtime, Responses has no async factory that preserves this path. Restrict rejection to unresolved credentials or provide an async factory that supports provider clients; the stable constructor needs the same correction.
|
Addressed both review issues on the current head. The concrete constructors no longer access protected client options. Resolved credential validation now lives in the shared ResponsesWSBase authentication boundary, so unresolved function credentials are rejected there while a credential already resolved through _callApiKey() remains usable by both stable and beta WebSockets. Focused WebSocket tests pass (4/4), and TypeScript compilation succeeds. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d2d038ff61
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (!apiKey) { | ||
| throw new OpenAIError( | ||
| 'Cannot open a Responses WebSocket without a resolved string apiKey. Resolve function-based credentials before constructing the WebSocket.', | ||
| ); |
There was a problem hiding this comment.
When a client has no apiKey because it uses workload identity or another caller-managed credential, this throws before ResponsesWS._createSocket() can merge options.headers. That breaks the documented new ResponsesWS(client, { headers: { Authorization: ... } }) path in docs/responses.md:157-160 and the existing stable/beta custom-header tests. Reject unresolved credentials only when the final handshake has no caller-supplied authentication, rather than requiring every client to expose an API key.
AGENTS.md reference: AGENTS.md:L52-L57
Useful? React with 👍 / 👎.
| const client = new OpenAI({ apiKey }); | ||
| expect(await client._callApiKey()).toBe(true); | ||
|
|
||
| expect(() => new Responses(client)).not.toThrow(); |
There was a problem hiding this comment.
No ws mock is installed in this test, so after _callApiKey() resolves the token, each new Responses(client) immediately starts a real connection to wss://api.openai.com. The socket is discarded without an error listener or close(), meaning a normal authentication or network failure reaches _onError() as an unhandled rejection, while a successful connection leaves an open handle; mock the transport and assert the emitted authorization header instead.
Useful? React with 👍 / 👎.
|
Addressed both new findings. The shared Responses WebSocket boundary now rejects only an unresolved function-backed apiKey, while clients with caller-managed Authorization headers or other non-apiKey authentication remain allowed. Added an internal client predicate for the unresolved-function state. The acceptance test now uses a mocked |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5199e017f5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| protected _authHeaders(): Record<string, string> { | ||
| if (this._client.apiKey) { | ||
| return { Authorization: `Bearer ${this._client.apiKey}` }; | ||
| if (this._client._hasUnresolvedApiKey()) { |
There was a problem hiding this comment.
When apiKey is a function and the caller follows docs/responses.md:157-160 by passing a resolved Authorization header in the ResponsesWS options, this unconditional guard throws before _createSocket() merges wsOptions.headers, so the documented workaround cannot open either the stable or beta socket. The fresh final tree still performs this check before the caller headers are merged, while the added custom-header test only covers a client without a function-based key. Make the validation consider the final caller-supplied authentication rather than rejecting solely from client state.
AGENTS.md reference: AGENTS.md:L52-L57
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in b22c606. The unresolved function-key check now happens at the final Node WebSocket header boundary, so an explicit caller Authorization header satisfies authentication while an unresolved key without explicit auth still fails closed. Added stable/beta coverage for both Authorization and authorization. Focused suite: 10/10 passed; tsc --noEmit passed.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b22c606afa
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const hasExplicitAuthorization = Object.entries(this._wsOptions?.headers ?? {}).some( | ||
| ([name, value]) => name.toLowerCase() === 'authorization' && value != null, |
There was a problem hiding this comment.
When headers.Authorization is accessor-backed, the spread above can snapshot an empty string while this Object.entries() read invokes the accessor again and sees a non-null token. The guard then passes, but new WS.WebSocket receives the already-built empty authorization value, so an unresolved function-key client still attempts the unauthenticated handshake this change is intended to prevent. Validate the final headers snapshot passed to ws and require a usable nonempty value; the beta copy has the same defect.
AGENTS.md reference: AGENTS.md:L98-L110
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in d1165a9. Credential validation now inspects the exact final socketOptions snapshot passed to ws; accessor-backed headers are read only during that snapshot, and empty/whitespace credential values do not satisfy the guard. Added a regression where Authorization returns empty on the snapshot and a token on a hypothetical second read. Focused suite: 20/20 passed; tsc --noEmit passed.
| const hasExplicitAuthorization = Object.entries(this._wsOptions?.headers ?? {}).some( | ||
| ([name, value]) => name.toLowerCase() === 'authorization' && value != null, | ||
| ); | ||
| if (this._client._hasUnresolvedApiKey() && !hasExplicitAuthorization) { |
There was a problem hiding this comment.
When a function-backed client authenticates a custom Responses endpoint through another supported ws mechanism, such as auth: 'user:pass' or an X-API-Key header, this check throws solely because there is no Authorization header, even though the parent revision passed those credentials through to ws. Fresh evidence in the final tree is tests/realtime-websocket-redirects.test.ts:401-426, which explicitly treats Basic auth, proxy authorization, cookies, and X-API-Key as caller credentials on these Responses transports. Check the final credential-bearing WebSocket options rather than only this one header; the beta copy has the same regression.
AGENTS.md reference: AGENTS.md:L52-L57
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in d1165a9. The unresolved-key guard now accepts the supported caller credential mechanisms already exercised by the Responses transport: Basic auth, Authorization, Proxy-Authorization, Cookie, and X-API-Key, all checked on the final socket options. Added stable/beta regressions for each. Focused suite: 20/20 passed; tsc --noEmit passed.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d1165a9d0d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const CREDENTIAL_HEADERS = new Set([ | ||
| 'authorization', | ||
| 'proxy-authorization', | ||
| 'cookie', | ||
| 'x-api-key', |
There was a problem hiding this comment.
When a function-backed client authenticates a custom endpoint with X-Auth-Token, X-Amz-Security-Token, or another caller-defined credential header, this four-name allowlist rejects the connection even though the parent revision forwarded it. Fresh evidence beyond the earlier credential comment is src/internal/ws.ts:47-74, which already classifies credential-bearing WebSocket headers, while tests/realtime-websocket-custom-credential-redirects.test.ts:11-49 explicitly exercises these additional names. Reuse that shared classification rather than maintaining a narrower stable/beta list.
AGENTS.md reference: AGENTS.md:L31-L35
Useful? React with 👍 / 👎.
|
|
||
| function hasCredentialValue(value: unknown): boolean { | ||
| if (Array.isArray(value)) return value.some(hasCredentialValue); | ||
| return value != null && String(value).trim().length > 0; |
There was a problem hiding this comment.
When JavaScript supplies an object-valued credential header with a stateful toString(), this conversion can return a nonempty token here, allowing the guard to pass, while the unchanged object passed to ws is stringified again by Node and can emit an empty or different header. Fresh evidence relative to the earlier accessor finding is that the final code snapshots the header property but not its serialized value. Normalize the credential value into the same primitive snapshot dispatched to ws; the beta copy has the same issue.
AGENTS.md reference: AGENTS.md:L106-L110
Useful? React with 👍 / 👎.
| followRedirects: false, | ||
| }); | ||
| }; | ||
| if (this._client._hasUnresolvedApiKey() && !hasExplicitWebSocketCredential(socketOptions)) { |
There was a problem hiding this comment.
When client.apiKey is accessor-backed, _authHeaders() reads it once to build the outgoing header and this call reads it again through _hasUnresolvedApiKey(). A stateful accessor can return null on the first read and a string on the second, making the unresolved check return false while the already-built header set remains empty. Fresh evidence beyond the prior header-accessor finding is this separate second read of the client credential; carry the original API-key/header snapshot into validation instead.
AGENTS.md reference: AGENTS.md:L106-L110
Useful? React with 👍 / 👎.
2 participants
Summary
Responses WebSocket authentication is synchronous, so a client with an unresolved function-based
apiKeycannot authenticate the socket. At the same time, a function-backed client that has already run_callApiKey()has a resolved string inclient.apiKeyand should remain usable.The current fix enforces that invariant at the shared stable and beta
ResponsesWSBase._authHeaders()boundary instead of only in the concrete Node constructors:client.apiKeyis a resolved string, use it for theAuthorizationheader regardless of whether the original client option was a function;Review follow-up
This addresses both review findings on the original head:
ResponsesWSBasesubclasses.Tests
Stable and beta regression coverage verifies both states:
_callApiKey()resolves the provider, constructing the same WebSocket surface no longer fails the credential guard.Fresh code/security review is running on commit
d2d038ff615b3e693abf8e1a01aba0a2bbd9c140.