fix: sanitize NO_PROXY env var to handle newline-separated values#3511
fix: sanitize NO_PROXY env var to handle newline-separated values#3511zerafachris wants to merge 3 commits into
Conversation
When NO_PROXY (or other proxy env vars) contain newline characters, httpx raises InvalidURL because it only splits by comma. This is common in Docker/.env files where values span multiple lines. Fixes openai#3303
![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: e4b0b30dbc
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
|
||
| When `provider` is supplied, authentication and the base URL are configured by that provider instead. | ||
| """ | ||
| _sanitize_proxy_env() |
There was a problem hiding this comment.
When a caller supplies http_client (particularly one created with trust_env=False), this SDK never reads the proxy environment because _base_client retains that already-created client. Constructing OpenAI nevertheless rewrites the process-wide proxy variables, so subsequent subprocesses or other HTTP clients can observe a changed proxy configuration even though this OpenAI client does not use it. Limit sanitization to default-client construction, or restore the environment after creating the default client.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch — fixed in aa7ae3b.
The sanitization is now gated on http_client is None, so it only runs when the SDK is about to construct the default httpx client (the only case where the proxy environment is actually read via trust_env). When a caller supplies their own client the environment is left untouched.
I went with limiting the scope rather than save/restore: restoring after construction would still leave a window where a concurrently-constructed client in another thread observes the mutated value, and it would also undo the normalization for the default client, which needs the sanitized value to persist for trust_env to work on subsequent requests.
Added coverage for both branches, sync and async: test_proxy_env_sanitized_for_default_client and test_proxy_env_untouched_when_http_client_supplied (the latter uses trust_env=False, the case you called out — it fails against the previous commit and passes now).
Verified locally: pytest tests/test_client.py 178 passed / 2 skipped, ruff check and ruff format --check clean, mypy clean.
Addresses Codex review (P2): _sanitize_proxy_env() mutated the process-wide proxy environment unconditionally, including when the caller supplies their own http_client. Such a client may deliberately ignore the environment (trust_env=False), and the SDK never reads it in that case, so the mutation was both unnecessary and observable by unrelated code in the process. Gate the call on http_client is None, and add sync + async coverage for both branches. Also applies ruff format to the helper, which was unformatted.
1 participant
Problem
When the
NO_PROXYenvironment variable contains newline characters (common in Docker/.env files), httpx raisesInvalidURLbecause it only splits by comma, not newline. The newline character becomes part of the hostname pattern, causing the URL parse failure.Solution
Add a
_sanitize_proxy_env()helper that normalizes proxy-related environment variables before httpx reads them. It replaces newlines with commas and strips whitespace from each segment.Changes
_sanitize_proxy_env()function insrc/openai/_client.pyOpenAI.__init__()andAsyncOpenAI.__init__()Fixes #3303