Preserve Azure authentication boundaries across transports - #3684
Preserve Azure authentication boundaries across transports#3684apcha-oai wants to merge 3 commits into
Conversation
jbeckwith-oai
left a comment
jbeckwith-oai
left a comment
There was a problem hiding this comment.
P1: The new HTTPX origin guard does not protect stable or beta async Azure Realtime connections. Both construct WebSocket connections directly with Azure api-key or bearer headers, and the supported websockets 15.0.1 client follows cross-origin handshake redirects while retaining those authentication headers. Two-server testing confirms both credential types are forwarded to the redirected origin, violating the same origin boundary this change establishes for HTTP. Reject cross-origin WebSocket redirects before reconnecting or strip all Azure authentication headers, and add stable/beta regressions for both API-key and bearer authentication.
| self._azure_endpoint = httpx2.URL(azure_endpoint) if azure_endpoint else None | ||
|
|
||
| hooks = self._client.event_hooks["request"] | ||
| if _async_strip_azure_api_key_on_redirect not in hooks: |
There was a problem hiding this comment.
[P1] This HTTP-only hook leaves stable and beta async Azure Realtime WebSockets unprotected. Supported websockets 15 follows cross-origin handshake redirects and reuses both api-key and Authorization headers; a real two-server reproduction confirms credential forwarding. Reject cross-origin WebSocket redirects or strip credentials before reconnecting, with coverage for both clients and auth methods.
jbeckwith-oai
left a comment
jbeckwith-oai
left a comment
There was a problem hiding this comment.
Independent re-review confirms the prior High Azure Realtime redirect credential leak is fixed for stable/beta clients, API-key/bearer authentication, and supported websockets versions. One small required test cleanup remains: remove the newly introduced, avoidable A002 suppression.
| self.end_headers() | ||
|
|
||
| @override | ||
| def log_message(self, format: str, *args: object) -> None: # noqa: A002 |
There was a problem hiding this comment.
[Low] Remove the avoidable A002 suppression. This new test override does not need a parameter named format: def log_message(self, *_args: object, **_kwargs: object) -> None: preserves positional/keyword compatibility, keeps the no-op behavior, and passes Ruff without # noqa: A002. Please use that straightforward lint-compliant form.
jbeckwith-oai
left a comment
jbeckwith-oai
left a comment
There was a problem hiding this comment.
Independent two-pass security re-review confirms the prior High Azure Realtime credential leak is fixed: stable and beta asynchronous WebSockets reject cross-origin redirects before forwarding API keys or bearer tokens while preserving legitimate same-origin redirects across supported websockets versions. The requested A002 suppression is removed using the straightforward variadic no-op test override. Real-server regressions, substantive CI, and CodeQL pass; no actionable findings.
## Summary - Resolve one Azure authentication mode consistently for synchronous and asynchronous clients. Explicit credentials take precedence over credential environment variables; conflicting explicit modes raise the existing `MutuallyExclusiveAuthError`. - Preserve environment-only AD-token precedence, client-copy behavior, and callable API-key refresh. - Keep module-level explicit Azure configuration separate from environment fallback, including import-time configuration and automatic Azure selection. - Add focused regressions for explicit and ambient credential selection, client copies, HTTP and Realtime configuration, and callable keys. No dependency or exported client API changes. This is independent of the Azure redirect-transport changes in openai#3684.
2 participants
Keep Azure authentication scoped to the original request origin across HTTP and Realtime connections. Supported HTTP clients remove the Azure API key when following an off-origin redirect, and async Realtime connections reject off-origin handshake redirects. Same-origin authentication and normal connection behavior remain intact.
The change is limited to Azure transport handling and focused synchronous/asynchronous coverage for stable and beta Realtime. No public API or dependency changes.
Review follow-up
The requested test logging-override cleanup is addressed by
6eb081861d715eee668cd46bffca44b3cd0227ec. It removes the unnecessary A002 suppression without changing runtime behavior. All 22 focused WebSocket redirect tests, Ruff, targeted Pyright, andgit diff --checkpass. Human re-review is requested; auto-merge remains disabled.