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 8 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 👍 / 👎.
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! |
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 👍 / 👎.
359113a to
decddbc
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: decddbc9a1
ℹ️ 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".
| @@ -0,0 +1,275 @@ | |||
| # Regression tests for NO_PROXY newline sanitization (issue #3303). | |||
There was a problem hiding this comment.
This introduces a 275-line handwritten test module with 15 standalone cases and substantial repeated imports, setup, and assertions for one small sanitizer. Consolidate the transport and line-ending variants with parameterization while retaining separate concurrency cases so the regression coverage complies with the repository requirement that handwritten tests remain small.
AGENTS.md reference: AGENTS.md:L5-L8
Useful? React with 👍 / 👎.
|
|
||
| def test_aiohttp_client_construction_with_newline_no_proxy(monkeypatch: pytest.MonkeyPatch) -> None: | ||
| """The aiohttp transport client also sanitizes NO_PROXY newlines.""" | ||
| pytest.importorskip("httpx_aiohttp") |
There was a problem hiding this comment.
In the normal development and aiohttp-extra environments, the SDK installs aiohttp and uses its vendored openai._vendor.httpx_aiohttp adapter; it deliberately does not install the external legacy httpx_aiohttp package. Consequently this importorskip skips both new aiohttp sanitizer tests in the configurations they are meant to cover, allowing regressions in _InstalledAioHttpClient to pass CI. Gate on aiohttp instead, as the existing aiohttp transport test does.
Useful? React with 👍 / 👎.
| return f"stainless-python-retry-{uuid.uuid4()}" | ||
|
|
||
|
|
||
| _no_proxy_sanitizer_lock = threading.Lock() |
There was a problem hiding this comment.
When a POSIX process forks while another thread is inside a default-client constructor, the child inherits this lock in the acquired state but not the thread that can release it. Any subsequent OpenAI() or default HTTP client construction in that child then blocks permanently at _sanitized_no_proxy, even when NO_PROXY needs no sanitization. Register an after_in_child fork handler that replaces the lock, or otherwise make the lock process-aware.
Useful? React with 👍 / 👎.
|
Hi @jbeckwith-oai — the two new Codex P2s from the Aug 22 review are addressed in the latest push (decddbc):
Plus a regression test for the concurrent-update case. All 13 sanitization tests and 202 client tests pass. Would appreciate a re-review when you have a moment. Thanks! |
) 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.
…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.
The _mounts attribute is private on httpx transports and not visible to Pyright. The os.environ assertion already verifies trust_env=False is respected, so the _mounts check is redundant.
…n tests Codex P2 review (Aug 22): 1. The sanitizer's restore clobbered NO_PROXY updates made by application code while a client was being constructed. Restore now only fires when the current value is still the one we sanitized. 2. Tests could leak a mutated NO_PROXY into the next test when a construction failed mid-way. An autouse fixture snapshots the inherited NO_PROXY/no_proxy and restores them after each test. Regression test: test_concurrent_no_proxy_update_not_clobbered.
b5cc07b to
d433a18
Compare
|
Hi @jbeckwith-oai — rebased this branch onto the latest main (post-HTTPX2/uv migration) and re-ran the full validation: 13/15 proxy-sanitize tests pass (2 skipped), ruff and mypy clean on the changed files. The branch is now 0 commits behind main. Requesting a re-review — the earlier CHANGES_REQUESTED blockers (env mutation, #3325 split, type checks) were all addressed in the Aug 13-25 commits. Thanks! |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d433a18a63
ℹ️ 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".
| try: | ||
| for key in ("NO_PROXY", "no_proxy"): | ||
| val = os.environ.get(key) | ||
| if val and any(c in val for c in "\n\r"): |
There was a problem hiding this comment.
When NO_PROXY contains only another line boundary recognized by str.splitlines()—such as vertical tab (\x0b), form feed, NEL, or U+2028—this condition remains false, so splitlines() is never reached and both default sync and async clients still pass an invalid hostname to HTTPX2. I reproduced InvalidURL with these separators; trigger sanitization for every supported boundary and add focused sync/async proxy regression cases.
AGENTS.md reference: AGENTS.md:L41-L45
Useful? React with 👍 / 👎.
…ight Pyright's reportDeprecated flags @contextmanager decorated functions annotated -> Iterator[...]: the decorator returns a Generator, so type the return as Generator[None, None, None] instead. Clears the lint failure that reds the branch CI (src/openai/_base_client.py:869 reportDeprecated).
|
Pushed Fork CI on the branch is fully green after the fix (lint + all test legs): https://github.com/rkfshakti/openai-python/actions/runs/33830874394 |
|
Status update on the three blockers from the 2026-08-03 review (branch has moved a lot since):
Given four competing PRs for #3303 (#3511, #3519, #3450, #3631), happy to defer to whichever shape maintainers prefer — just let me know. |
|
Addressed the remaining Codex review threads on the current head (
25 tests pass in |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ca7116bf4e
ℹ️ 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 hasattr(os, "register_at_fork"): | ||
| os.register_at_fork(after_in_child=_reset_no_proxy_sanitizer_lock) |
There was a problem hiding this comment.
When one thread forks while another is inside _sanitized_no_proxy(), the child inherits the temporarily comma-normalized environment, while the thread whose finally block would restore the original values no longer exists there. The new after_in_child callback resets only the lock, so the child—and any process it subsequently execs—permanently receives modified NO_PROXY/no_proxy values. Track the active originals and restore them in the child callback as well as replacing the lock.
Useful? React with 👍 / 👎.
| # Simulate the child state: acquire the lock in this process, then fork. | ||
| base_client._no_proxy_sanitizer_lock.acquire() | ||
| try: | ||
| ctx = multiprocessing.get_context("fork") |
There was a problem hiding this comment.
On Windows, which this package declares as supported, multiprocessing.get_context("fork") raises ValueError because that start method does not exist. This makes the test suite fail before exercising the assertion; guard the test with a platform/start-method skip so the POSIX-only regression test does not break supported Windows development environments.
Useful? React with 👍 / 👎.
|
Thank you for the contribution. We’re closing this PR based on the decision explained in #3303. |
3 participants
