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 1 commit 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 👍 / 👎.
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 👍 / 👎.
|
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 |
955d36d to
5ac81c9
Compare
|
Thanks for the detailed review @jbeckwith-oai. All blockers are addressed in the latest push (64c7f60):
Tests cover sync/async, uppercase/lowercase, trust_env=True/False, and concurrent construction. |
64c7f60 to
a82bffc
Compare
|
Hi @jbeckwith-oai — just a friendly nudge for a re-review. All three blockers from your review are addressed (context manager instead of global mutation, #3325 split out, Pyright and Ruff clean), and the branch is rebased on the latest main. Would appreciate a re-review when you have a moment. Thanks! |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a82bffcdd1
ℹ️ 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 follow the documented DefaultHttpx2Client(...) / DefaultAsyncHttpx2Client(...) path for a custom SDK HTTP client, those factory functions in src/openai/_httpx2.py still return httpx2.Client / httpx2.AsyncClient directly, so an environment with NO_PROXY containing a newline still raises during httpx environment proxy parsing. This new guard only covers the _base_client default subclasses, leaving a public SDK-provided client path with the same failure the fix is meant to eliminate.
Useful? React with 👍 / 👎.
a82bffc to
38dc8ee
Compare
|
Hi @jbeckwith-oai — all three blockers from your review are addressed in the latest push (64c7f60): (1) global env mutation replaced with a context manager that restores NO_PROXY afterwards, (2) the #3325 streaming changes were split out into a separate PR (#3521), (3) Pyright and Ruff are clean. The branch is rebased on latest main. Would appreciate a re-review when you have a moment. |
|
Hi @jbeckwith-oai — all three blockers from your review (2026-08-03) have been resolved on the current head (
Added focused regression coverage: CI is green. Could you take another look when you get a chance? |
|
Hi @jbeckwith-oai — I've addressed all review points since the initial review (Aug 3):
Could you take another look when time allows? Thanks! |
Docker/.env files can leave newline or carriage return characters in NO_PROXY. httpx splits the value on commas only, so a trailing newline or CR becomes part of the hostname and httpx raises InvalidURL (issue openai#3303). The fix normalizes line separators in NO_PROXY/no_proxy to commas before httpx reads the environment during client construction. The sanitization is temporary: the original env value is saved before and restored after httpx init completes, so the process environment is not permanently mutated. A threading lock prevents concurrent client constructions from seeing a partially sanitized value. When trust_env=False is explicitly passed, httpx does not read proxy environment variables at all, so the sanitization is skipped entirely — no side effect on the process environment for users who opt out of env-based proxy configuration. Closes openai#3303
38dc8ee to
359113a
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 359113a768
ℹ️ 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".
| _sanitize_no_proxy_env(trust_env=trust_env) | ||
| super().__init__(**kwargs) | ||
| finally: | ||
| _restore_no_proxy_env(_saved) |
There was a problem hiding this comment.
When application code updates NO_PROXY or no_proxy on another thread while this client is inside super().__init__(), the finally block unconditionally writes the pre-initialization snapshot back and silently discards the new configuration. The newly added lock only serializes SDK default-client constructors, so it does not protect arbitrary os.environ writers; restore only when the current value is still the sanitizer's temporary value, or avoid mutating the process environment.
Useful? React with 👍 / 👎.
| for var in ("NO_PROXY", "no_proxy"): | ||
| os.environ.pop(var, None) |
There was a problem hiding this comment.
When pytest starts with an inherited NO_PROXY or no_proxy value, this fixture deletes it before the first test and deletes it again during teardown instead of restoring the original value. The rest of the test process therefore permanently loses its proxy bypass list, so later localhost tests or session hooks can unexpectedly route through an inherited proxy depending on collection order; snapshot and restore the original values or use monkeypatch.
Useful? React with 👍 / 👎.
2 participants
