fix: sanitize newlines in NO_PROXY env var before httpx client init - #3519
fix: sanitize newlines in NO_PROXY env var before httpx client init#3519rkfshakti wants to merge 2 commits into
Conversation
rkfshakti
commented
Jul 20, 2026
rkfshakti
commented
- fix(parsing): guard against None response.output in parse_response
- fix(streaming): preserve accumulated output when response.completed has null output
- fix(streaming): route null-output fallback through parse_response
- fix(client): sanitize newlines in NO_PROXY env var before httpx client init
![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: 217dc74b35
ℹ️ 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".
| # only splits by comma, not by newline. When NO_PROXY contains newlines | ||
| # (common in Docker/.env files), the newline becomes part of the hostname | ||
| # and httpx raises InvalidURL. See issue #3303. | ||
| _sanitize_no_proxy() |
There was a problem hiding this comment.
This call only runs in _DefaultHttpxClient; AsyncOpenAI creates AsyncHttpxClientWrapper from _DefaultAsyncHttpxClient (src/openai/_base_client.py:1546, 1441-1446), whose constructor still enters httpx.AsyncClient.__init__ with the original environment. In async clients with NO_PROXY/no_proxy containing newlines, the same InvalidURL during httpx environment proxy parsing remains, so the fix only works for sync users.
Useful? React with 👍 / 👎.
| type_=type(event.response), | ||
| value={ | ||
| **event.response.to_dict(), | ||
| "output": [item.to_dict() for item in snapshot.output], |
There was a problem hiding this comment.
When response.completed has output: null, this fallback copies snapshot.output, but the snapshot is populated from response.output_item.added and deltas and never replaced from response.output_item.done before this branch. For streams where the added item is still in_progress and the done event carries the completed item/final annotations, the final parsed response returned here preserves the stale in-progress item instead of the finalized output.
Useful? React with 👍 / 👎.
| becomes part of the hostname and httpx raises ``InvalidURL`` (issue #3303). | ||
| """ | ||
| for key in ("NO_PROXY", "no_proxy"): | ||
| val = os.environ.get(key) |
There was a problem hiding this comment.
With the default sync client, _DefaultHttpxClient.__init__ now always calls _sanitize_no_proxy(), and this line references os even though _base_client.py does not import it. Any OpenAI()/DefaultHttpxClient() construction therefore raises NameError before httpx initialization, even when NO_PROXY is unset.
Useful? React with 👍 / 👎.
rkfshakti
changed the title
|
Friendly ping — this PR sanitizes newlines in the NO_PROXY environment variable before passing it to httpx, which otherwise crashes with an InvalidURL error when NO_PROXY contains trailing newlines (common in misconfigured shell profiles or CI secrets). Would appreciate a review when time allows. |
|
Hi maintainers — following up on this fix for #3303. Sanitizes newlines in the NO_PROXY environment variable before httpx client construction to prevent silent proxy bypass. CI is passing. Would appreciate a review when time allows. Thanks! |
|
Friendly ping — this PR has been open for over 10 days. Would appreciate a human review when time allows. |
jbeckwith-oai
left a comment
jbeckwith-oai
left a comment
There was a problem hiding this comment.
I found several blockers on the current head:
-
The null-output streaming fallback can return stale partial items.
ResponseStreamStateappends the payload fromresponse.output_item.added, but it never replaces it fromresponse.output_item.done(orresponse.content_part.done). Whenresponse.completed.response.outputis null, this branch serializes the earlier snapshot instead of the authoritative done-event payload. I reproducedcreated -> output_item.added(status=in_progress) -> output_item.done(status=completed) -> completed(output=null)returning a final item whose status is stillin_progress. Please accumulate/retain the done-event payloads and add a full streaming regression test that verifies final status/content/metadata. -
_sanitize_no_proxy()permanently mutates process-global environment state even when callers explicitly constructDefaultHttpxClient(trust_env=False). I reproducedNO_PROXY="localhost\n127.0.0.1"becominglocalhost,127.0.0.1after that constructor, despite the client being configured to ignore proxy environment variables. This is a surprising public-constructor side effect and can race with or alter unrelated clients in the same process. Please keep normalization local to the client/proxy configuration (and at minimum respecttrust_env=False) rather than unconditionally rewritingos.environ; cover sync/async and uppercase/lowercase behavior in tests. -
Required static checks fail on the diff: Ruff reports an unsorted import block in
_base_client.py, and strict Pyright reports the null comparison as impossible plus a partially unknownconstruct_type_uncheckedvalue in_responses.py. These need to be clean before merge.
The PR also bundles the unrelated #3325 response-streaming change into a PR titled/scoped for #3303, with no regression tests for either new path. Please split or rebase this to one cohesive fix and include focused coverage.
fb55f96 to
92b0f72
Compare
|
Thanks @jbeckwith-oai for the thorough review — all blockers are addressed in the rewritten head (92b0f72): 1. Split PR — unrelated #3325 streaming changes removed. The response-streaming null-output fallback ( 2. No more permanent 3. 4. Ruff + Pyright clean. The import block is sorted ( 5. Regression tests. Added Would appreciate a re-review. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 92b0f72f7e
ℹ️ 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".
| if kwargs.get("trust_env", True): | ||
| with _sanitized_no_proxy(): | ||
| super().__init__(**kwargs) |
There was a problem hiding this comment.
When users opt into the documented aiohttp transport with AsyncOpenAI(http_client=DefaultAioHttpClient()), this new guard never runs: _DefaultAioHttpClient below still delegates directly to its httpx.AsyncClient-compatible superclass, so NO_PROXY/no_proxy values containing newlines can still raise during client construction. Please wrap that constructor with the same _sanitized_no_proxy() logic, while preserving the trust_env=False skip, so the regression fix applies to all SDK-provided clients.
Useful? React with 👍 / 👎.
92b0f72 to
4b34439
Compare
|
Addressed the Codex P2 about the aiohttp client in the latest push (4b34439):
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4b344392c5
ℹ️ 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".
| if val and "\n" in val: | ||
| originals[key] = val | ||
| os.environ[key] = ",".join(part.strip() for part in val.replace("\n", ",").split(",") if part.strip()) |
There was a problem hiding this comment.
When NO_PROXY/no_proxy contains a lone carriage return, such as a value read from a CRLF file via command substitution where the trailing \n was stripped but \r remains, this guard does not run because it only looks for "\n"; httpx rejects \r as a non-printable URL character in the same proxy parsing path, so default client construction can still raise InvalidURL. Please treat all line separators as delimiters here, for example by using splitlines() before joining the entries.
Useful? React with 👍 / 👎.
) httpx's get_environment_proxies() only splits on commas, so a trailing newline in NO_PROXY (common in Docker/.env files) becomes part of the hostname and httpx raises InvalidURL. The previous implementation permanently mutated os.environ, which leaked into unrelated clients in the same process and ignored trust_env=False. Replace the unconditional mutation with a context manager that temporarily normalizes NO_PROXY/no_proxy only for the duration of httpx client construction, then restores the original values. Skip the normalization entirely when the caller passes trust_env=False. Add 9 regression tests covering sync/async construction, env restoration, trust_env=False, lowercase no_proxy, and multiple newlines.
4b34439 to
332dd5d
Compare
|
Addressed the Codex P2 about carriage returns in the latest push (332dd5d): The guard now checks for both Added two regression tests: |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 332dd5deb2
ℹ️ 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".
| afterwards — this avoids permanently mutating process-global state for | ||
| unrelated clients. | ||
| """ | ||
| originals: dict[str, str] = {} |
There was a problem hiding this comment.
When two SDK default clients are constructed concurrently while NO_PROXY contains a newline, one call can enter this context after another has already sanitized the process-wide env, record no original value, and then the first call restores the invalid value before the second super().__init__() reaches httpx's env proxy parsing. That leaves the second client exposed to the same InvalidURL this change is trying to prevent; guard this global mutation with a shared lock/refcount, or avoid mutating os.environ, so the sanitized value remains visible until all overlapping initializations finish.
Useful? React with 👍 / 👎.
…ctions Addresses Codex P2: when two SDK default clients are constructed concurrently while NO_PROXY contains a newline, one call can enter the context after another already sanitized the process-wide env, record no original value, and then the first call restores the invalid value before the second call's super().__init__() reaches httpx's env proxy parsing. That leaves the second client exposed to the same InvalidURL this change is trying to prevent. Added a module-level threading.Lock (_no_proxy_sanitizer_lock) that wraps the entire sanitize-construct-restore window in _sanitized_no_proxy, so concurrent client constructions are serialized and each call sees a consistent environment. Added test_concurrent_client_construction_serializes_sanitization that spawns 10 threads constructing clients with a newline NO_PROXY and verifies no errors and the original value is restored.
|
Addressed the Codex P2 about concurrent client construction in the latest push (955d36d): Serialize NO_PROXY sanitization during client init — Added a module-level Added |
2 participants
