fix(client): sanitize NO_PROXY whitespace before httpx init (fixes #3303)#3309
fix(client): sanitize NO_PROXY whitespace before httpx init (fixes #3303)#3309LeSingh1 wants to merge 2 commits into
Conversation
NO_PROXY values that contain newlines or tabs (common when set from Docker, .env files, or shell scripts that wrap lines) used to break OpenAI() / AsyncOpenAI() construction with httpx.InvalidURL: the hostname token includes the whitespace and httpx rejects it during its proxy-mounts setup before any request is made. The underlying parse bug is in httpx (get_environment_proxies splits only on commas), but the encode/httpx project is not currently accepting external PRs, so the practical workaround is for the SDK to normalize the value just for the duration of the internal httpx client's __init__. Introduce a _sanitized_proxy_env() context manager that swaps any whitespace inside NO_PROXY / no_proxy for commas, wraps each of the three internal httpx wrappers (_DefaultHttpxClient, _DefaultAsyncHttpxClient, _DefaultAioHttpClient), and restores the original value on exit so user code observes the env var it set. 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: 6092238595
ℹ️ 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".
| for name, original in saved.items(): | ||
| if original is None: | ||
| os.environ.pop(name, None) | ||
| else: | ||
| os.environ[name] = original |
There was a problem hiding this comment.
The new _sanitized_proxy_env() mutates process-wide NO_PROXY/no_proxy without synchronization, so overlapping client constructions can leave the environment permanently altered. If thread A sanitizes NO_PROXY, thread B enters during that window and snapshots the already-sanitized value, then A restores the original and B later restores its stale snapshot, resulting in NO_PROXY being left sanitized instead of the caller’s original value. Because this variable is global, that leaked value can change proxy behavior for subsequent HTTP clients and subprocesses in multithreaded applications.
Useful? React with 👍 / 👎.
The contextmanager added in 6092238 mutates os.environ to clean up NO_PROXY whitespace, which is process-wide and not thread-safe. Two clients constructed on different threads can interleave: thread A saves the original and cleans the env, thread B enters during that window and snapshots the already-cleaned value as its 'original', then both threads restore in turn, leaving NO_PROXY permanently sanitized for the rest of the process. Wrap the env mutation in a module-level threading.Lock so the save/clean/restore sequence runs atomically. Add a concurrency regression test that spins up 8 threads constructing 20 clients each and asserts NO_PROXY equals the caller's original string at the end. Codex review caught this on the original PR.
1 participant
Fixes #3303.
Bug
OpenAI()andAsyncOpenAI()raisehttpx.InvalidURLat construction time when theNO_PROXYenvironment variable contains whitespace (newlines, tabs, or runs of spaces). This is common whenNO_PROXYis set from a Docker compose file with multi-line YAML, a.envfile where the value wraps, or a shell script using line continuations.Root cause is in
httpx._utils.get_environment_proxies: it splitsNO_PROXYon commas only, so whitespace inside an entry survives into the hostname and httpx rejects it during proxy-mounts setup, before any request is made. The reporter notes the fix would be trivial in httpx but the encode/httpx project is not currently accepting external PRs, so the practical workaround is for the SDK to normalize the value just for the duration of the internal httpx client's__init__.Fix
Introduce a
_sanitized_proxy_env()context manager in_base_client.pythat, while active, replaces any whitespace insideNO_PROXY/no_proxywith commas (then collapses adjacent commas). Wrapsuper().__init__(**kwargs)in each of the three internal httpx wrappers (_DefaultHttpxClient,_DefaultAsyncHttpxClient,_DefaultAioHttpClient). The original env var value is restored on exit so user code observes the env var it set.If
NO_PROXYcontains no whitespace, the context manager is a no-op.Verification
Added two regression tests in
tests/test_client.py:TestOpenAI.test_no_proxy_with_whitespace_does_not_raiseTestAsyncOpenAI.test_no_proxy_with_whitespace_does_not_raiseEach sets
NO_PROXY="localhost\nexample.com\t192.168.1.1", constructs the default client, asserts no exception, and asserts the original env var is restored.Local A/B:
Scope
40-line change to
_base_client.py(one helper + three two-linewith:wraps + three import additions) plus 26 lines of test coverage. No behavior change for users who do not setNO_PROXYor who set it with valid comma-separated values.