diff --git a/src/openai/_base_client.py b/src/openai/_base_client.py index f195d04816..cd2ab5d31c 100644 --- a/src/openai/_base_client.py +++ b/src/openai/_base_client.py @@ -1,5 +1,6 @@ from __future__ import annotations +import os import sys import json import math @@ -11,6 +12,8 @@ import logging import platform import warnings +import threading +import contextlib import email.utils from types import TracebackType from random import random @@ -860,12 +863,83 @@ def _idempotency_key(self) -> str: return f"stainless-python-retry-{uuid.uuid4()}" +_no_proxy_sanitizer_lock = threading.Lock() + + +def _reset_no_proxy_sanitizer_lock() -> None: + """Replace the sanitizer lock after a fork. + + If a POSIX process forks while another thread holds the lock, the child + inherits the lock in the acquired state but not the thread that can + release it, so any later client construction in the child would block + forever. ``os.register_at_fork`` replaces the lock in the child. + """ + global _no_proxy_sanitizer_lock + _no_proxy_sanitizer_lock = threading.Lock() + + +if hasattr(os, "register_at_fork"): + os.register_at_fork(after_in_child=_reset_no_proxy_sanitizer_lock) + + +@contextlib.contextmanager +def _sanitized_no_proxy() -> Generator[None, None, None]: + """Temporarily normalize line separators in NO_PROXY/no_proxy for the + duration of a httpx client construction. + + httpx's ``get_environment_proxies()`` only splits on commas, so a line + separator in ``NO_PROXY`` (common in Docker/``.env`` files or CRLF values + where the ``\\n`` was stripped but ``\\r`` remains) becomes part of the + hostname and httpx raises ``InvalidURL`` (issue #3303). httpx reads the + environment once during ``__init__``, so we only need the sanitized value + to be visible for that window and then restore the original afterwards — + this avoids permanently mutating process-global state for unrelated + clients. + + A module-level lock serializes concurrent client constructions so that one + call cannot restore the original (invalid) value while another call's + ``super().__init__()`` is still reading the environment. + """ + with _no_proxy_sanitizer_lock: + originals: dict[str, str] = {} + sanitized: dict[str, str] = {} + try: + for key in ("NO_PROXY", "no_proxy"): + val = os.environ.get(key) + # splitlines() recognizes every line boundary (\n, \r, \r\n, + # \v, \f, \x1c-\x1e, \x85, \u2028, \u2029), including a + # trailing separator that produces a single part. If the + # value contains any boundary, the split differs from the + # original string and sanitization is required. + if val and val.splitlines() != [val]: + originals[key] = val + parts = [part.strip() for part in val.splitlines()] + sanitized[key] = ",".join(p for p in parts if p) + os.environ[key] = sanitized[key] + yield + finally: + for key, val in originals.items(): + # Only restore if the value is still the one we sanitized — + # application code may have updated NO_PROXY while the client + # was being constructed, and clobbering that update would be + # worse than leaving the sanitized value in place. + if os.environ.get(key) == sanitized.get(key): + os.environ[key] = val + + class _DefaultHttpxClient(httpx2.Client): def __init__(self, **kwargs: Any) -> None: kwargs.setdefault("timeout", DEFAULT_TIMEOUT) kwargs.setdefault("limits", DEFAULT_CONNECTION_LIMITS) kwargs.setdefault("follow_redirects", True) - super().__init__(**kwargs) + # httpx reads proxy env vars during __init__; temporarily normalize + # newlines in NO_PROXY so they don't become part of the hostname + # (issue #3303). Skip when the caller opted out of env-based proxies. + if kwargs.get("trust_env", True): + with _sanitized_no_proxy(): + super().__init__(**kwargs) + else: + super().__init__(**kwargs) if TYPE_CHECKING: @@ -1459,7 +1533,12 @@ def __init__(self, **kwargs: Any) -> None: kwargs.setdefault("timeout", DEFAULT_TIMEOUT) kwargs.setdefault("limits", DEFAULT_CONNECTION_LIMITS) kwargs.setdefault("follow_redirects", True) - super().__init__(**kwargs) + # See _DefaultHttpxClient for the rationale behind the NO_PROXY guard. + if kwargs.get("trust_env", True): + with _sanitized_no_proxy(): + super().__init__(**kwargs) + else: + super().__init__(**kwargs) _DefaultAioHttpClient: type[httpx2.AsyncClient] @@ -1480,7 +1559,12 @@ def __init__(self, **kwargs: Any) -> None: kwargs.setdefault("timeout", DEFAULT_TIMEOUT) kwargs.setdefault("limits", DEFAULT_CONNECTION_LIMITS) kwargs.setdefault("follow_redirects", True) - super().__init__(**kwargs) + # See _DefaultHttpxClient for the rationale behind the NO_PROXY guard. + if kwargs.get("trust_env", True): + with _sanitized_no_proxy(): + super().__init__(**kwargs) + else: + super().__init__(**kwargs) _DefaultAioHttpClient = _InstalledAioHttpClient diff --git a/src/openai/_httpx2.py b/src/openai/_httpx2.py index 491398b43c..e324e5a434 100644 --- a/src/openai/_httpx2.py +++ b/src/openai/_httpx2.py @@ -132,10 +132,24 @@ def _set_httpx2_defaults(kwargs: dict[str, Any]) -> None: def DefaultHttpx2Client(**kwargs: Any) -> httpx2.Client: """Create an HTTPX2 client with the SDK's recommended defaults.""" _set_httpx2_defaults(kwargs) + # Lazy import to avoid a circular dependency: _base_client imports from + # this module, and the sanitizer lives there. + from ._base_client import _sanitized_no_proxy + + if kwargs.get("trust_env", True): + with _sanitized_no_proxy(): + return httpx2.Client(**kwargs) return httpx2.Client(**kwargs) def DefaultAsyncHttpx2Client(**kwargs: Any) -> httpx2.AsyncClient: """Create an async HTTPX2 client with the SDK's recommended defaults.""" _set_httpx2_defaults(kwargs) + # Lazy import to avoid a circular dependency: _base_client imports from + # this module, and the sanitizer lives there. + from ._base_client import _sanitized_no_proxy + + if kwargs.get("trust_env", True): + with _sanitized_no_proxy(): + return httpx2.AsyncClient(**kwargs) return httpx2.AsyncClient(**kwargs) diff --git a/tests/test_no_proxy_sanitize.py b/tests/test_no_proxy_sanitize.py new file mode 100644 index 0000000000..ed9dff3e8c --- /dev/null +++ b/tests/test_no_proxy_sanitize.py @@ -0,0 +1,360 @@ +# Regression tests for NO_PROXY newline sanitization (issue #3303). +# +# httpx's ``get_environment_proxies()`` only splits on commas, so a trailing +# newline in ``NO_PROXY`` becomes part of the hostname and httpx raises +# ``InvalidURL``. The SDK temporarily normalizes the env var during client +# construction and restores it afterwards, so unrelated clients are unaffected. + +from __future__ import annotations + +import os +from collections.abc import Iterator + +import pytest + + +@pytest.fixture(autouse=True) +def _restore_inherited_no_proxy(monkeypatch: pytest.MonkeyPatch) -> Iterator[None]: # pyright: ignore[reportUnusedFunction] + """Restore any NO_PROXY/no_proxy inherited from the test environment. + + The sanitizer mutates os.environ during client construction and restores + it afterwards, but a test that fails mid-construction (or a test that + deliberately leaves a value set) can leak into the next test. Snapshot the + inherited values up front and restore them after each test so the suite + is deterministic regardless of the host environment. + """ + inherited = { + key: os.environ.get(key) + for key in ("NO_PROXY", "no_proxy") + } + yield + for key, val in inherited.items(): + if val is None: + monkeypatch.delenv(key, raising=False) + else: + monkeypatch.setenv(key, val) + + +def _set_no_proxy(monkeypatch: pytest.MonkeyPatch, value: str | None) -> None: + """Set both NO_PROXY and no_proxy via monkeypatch for automatic cleanup.""" + if value is None: + monkeypatch.delenv("NO_PROXY", raising=False) + monkeypatch.delenv("no_proxy", raising=False) + else: + monkeypatch.setenv("NO_PROXY", value) + monkeypatch.setenv("no_proxy", value) + + +def _mount_patterns(client: object) -> list[str]: + return [k.pattern for k in client._mounts] # type: ignore[attr-defined] + + +def test_sync_client_construction_with_newline_no_proxy(monkeypatch: pytest.MonkeyPatch) -> None: + """A sync default client can be constructed when NO_PROXY has newlines.""" + from openai._base_client import _DefaultHttpxClient + + _set_no_proxy(monkeypatch, "localhost\n127.0.0.1") + # Should not raise InvalidURL + client = _DefaultHttpxClient() + patterns = _mount_patterns(client) + assert any("localhost" in p for p in patterns) + assert any("127.0.0.1" in p for p in patterns) + client.close() + + +def test_async_client_construction_with_newline_no_proxy(monkeypatch: pytest.MonkeyPatch) -> None: + """An async default client can be constructed when NO_PROXY has newlines.""" + from openai._base_client import _DefaultAsyncHttpxClient + + _set_no_proxy(monkeypatch, "localhost\n127.0.0.1") + # Should not raise InvalidURL + client = _DefaultAsyncHttpxClient() + patterns = _mount_patterns(client) + assert any("localhost" in p for p in patterns) + assert any("127.0.0.1" in p for p in patterns) + + +def test_env_restored_after_sync_client_construction(monkeypatch: pytest.MonkeyPatch) -> None: + """os.environ is restored to its original value after client construction.""" + from openai._base_client import _DefaultHttpxClient + + original = "localhost\n127.0.0.1" + _set_no_proxy(monkeypatch, original) + client = _DefaultHttpxClient() + client.close() + import os + + assert os.environ.get("NO_PROXY") == original + assert os.environ.get("no_proxy") == original + + +def test_env_restored_after_async_client_construction(monkeypatch: pytest.MonkeyPatch) -> None: + """os.environ is restored after async client construction.""" + from openai._base_client import _DefaultAsyncHttpxClient + + original = "localhost\n127.0.0.1" + _set_no_proxy(monkeypatch, original) + _DefaultAsyncHttpxClient() + import os + + assert os.environ.get("NO_PROXY") == original + assert os.environ.get("no_proxy") == original + + +def test_trust_env_false_skips_sanitization(monkeypatch: pytest.MonkeyPatch) -> None: + """When trust_env=False, NO_PROXY is not touched and no InvalidURL is raised.""" + from openai._base_client import _DefaultHttpxClient + + _set_no_proxy(monkeypatch, "localhost\n127.0.0.1") + client = _DefaultHttpxClient(trust_env=False) + import os + + # env should be untouched + assert os.environ.get("NO_PROXY") == "localhost\n127.0.0.1" + # no proxy mounts should be configured since trust_env=False + assert client._mounts == {} + client.close() + + +def test_trust_env_false_async_skips_sanitization(monkeypatch: pytest.MonkeyPatch) -> None: + """Async client with trust_env=False skips NO_PROXY sanitization.""" + from openai._base_client import _DefaultAsyncHttpxClient + + _set_no_proxy(monkeypatch, "localhost\n127.0.0.1") + client = _DefaultAsyncHttpxClient(trust_env=False) + import os + + assert os.environ.get("NO_PROXY") == "localhost\n127.0.0.1" + assert client._mounts == {} + + +def test_no_newline_no_mutation(monkeypatch: pytest.MonkeyPatch) -> None: + """When NO_PROXY has no newlines, the env var is not modified at all.""" + from openai._base_client import _DefaultHttpxClient + + _set_no_proxy(monkeypatch, "localhost,127.0.0.1") + client = _DefaultHttpxClient() + client.close() + import os + + assert os.environ.get("NO_PROXY") == "localhost,127.0.0.1" + + +def test_lowercase_no_proxy_sanitized(monkeypatch: pytest.MonkeyPatch) -> None: + """Lowercase no_proxy is also sanitized.""" + from openai._base_client import _DefaultHttpxClient + + monkeypatch.delenv("NO_PROXY", raising=False) + monkeypatch.setenv("no_proxy", "localhost\n127.0.0.1") + client = _DefaultHttpxClient() + client.close() + import os + + # restored after construction + assert os.environ.get("no_proxy") == "localhost\n127.0.0.1" + + +def test_multiple_newlines_sanitized(monkeypatch: pytest.MonkeyPatch) -> None: + """Multiple newlines and whitespace are handled correctly.""" + from openai._base_client import _DefaultHttpxClient + + _set_no_proxy(monkeypatch, "localhost\n\n127.0.0.1\n.example.com\n") + client = _DefaultHttpxClient() + patterns = _mount_patterns(client) + assert any("localhost" in p for p in patterns) + assert any("127.0.0.1" in p for p in patterns) + assert any("example.com" in p for p in patterns) + client.close() + import os + + # restored + assert os.environ.get("NO_PROXY") == "localhost\n\n127.0.0.1\n.example.com\n" + + +def test_carriage_return_sanitized(monkeypatch: pytest.MonkeyPatch) -> None: + """A lone \\r (from CRLF files where \\n was stripped) is also sanitized.""" + from openai._base_client import _DefaultHttpxClient + + _set_no_proxy(monkeypatch, "localhost\r127.0.0.1") + client = _DefaultHttpxClient() + patterns = _mount_patterns(client) + assert any("localhost" in p for p in patterns) + assert any("127.0.0.1" in p for p in patterns) + client.close() + import os + + assert os.environ.get("NO_PROXY") == "localhost\r127.0.0.1" + + +def test_crlf_sanitized(monkeypatch: pytest.MonkeyPatch) -> None: + """CRLF (\\r\\n) line endings are sanitized correctly.""" + from openai._base_client import _DefaultHttpxClient + + _set_no_proxy(monkeypatch, "localhost\r\n127.0.0.1\r\n") + client = _DefaultHttpxClient() + patterns = _mount_patterns(client) + assert any("localhost" in p for p in patterns) + assert any("127.0.0.1" in p for p in patterns) + client.close() + + +def test_aiohttp_client_construction_with_newline_no_proxy(monkeypatch: pytest.MonkeyPatch) -> None: + """The aiohttp transport client also sanitizes NO_PROXY newlines.""" + pytest.importorskip("aiohttp") + from openai._base_client import _DefaultAioHttpClient + + _set_no_proxy(monkeypatch, "localhost\n127.0.0.1") + # Should not raise InvalidURL + client = _DefaultAioHttpClient() + patterns = _mount_patterns(client) + assert any("localhost" in p for p in patterns) + assert any("127.0.0.1" in p for p in patterns) + + +def test_aiohttp_client_trust_env_false_skips_sanitization(monkeypatch: pytest.MonkeyPatch) -> None: + """The aiohttp transport client respects trust_env=False.""" + pytest.importorskip("aiohttp") + from openai._base_client import _DefaultAioHttpClient + + _set_no_proxy(monkeypatch, "localhost\n127.0.0.1") + _DefaultAioHttpClient(trust_env=False) + import os + + assert os.environ.get("NO_PROXY") == "localhost\n127.0.0.1" + + +def test_concurrent_client_construction_serializes_sanitization(monkeypatch: pytest.MonkeyPatch) -> None: + """Concurrent client constructions must not race on the env mutation. + + Without the lock, one call could restore the original (invalid) NO_PROXY + value while another call's ``super().__init__()`` is still reading the + environment, exposing the second client to InvalidURL. The lock + serializes the sanitize-construct-restore window so each call sees a + consistent environment. + """ + import threading + + from openai._base_client import _DefaultHttpxClient + + _set_no_proxy(monkeypatch, "localhost\n127.0.0.1") + errors: list[Exception] = [] + + def construct() -> None: + try: + _DefaultHttpxClient() + except Exception as exc: + errors.append(exc) + + threads = [threading.Thread(target=construct) for _ in range(10)] + for t in threads: + t.start() + for t in threads: + t.join() + + assert not errors, f"Concurrent constructions failed: {errors}" + # The original (invalid) value must be restored after all constructions + assert os.environ.get("NO_PROXY") == "localhost\n127.0.0.1" + + +def test_concurrent_no_proxy_update_not_clobbered(monkeypatch: pytest.MonkeyPatch) -> None: + """An application update to NO_PROXY during construction is preserved. + + If application code changes NO_PROXY while a client is being constructed, + the sanitizer must not clobber that update when it restores the original + value — the update is newer and more relevant than the pre-construction + value. + """ + from openai._base_client import _sanitized_no_proxy + + _set_no_proxy(monkeypatch, "localhost\n127.0.0.1") + + with _sanitized_no_proxy(): + # Simulate application code updating NO_PROXY mid-construction. + os.environ["NO_PROXY"] = "api.example.com" + + # The application's update must survive the sanitizer's restore. + assert os.environ.get("NO_PROXY") == "api.example.com" + + +@pytest.mark.parametrize( + "separator", + ["\v", "\f", "\x1c", "\x1d", "\x1e", "\x85", "\u2028", "\u2029"], +) +def test_all_splitlines_separators_sanitized(monkeypatch: pytest.MonkeyPatch, separator: str) -> None: + """Every line boundary recognized by str.splitlines() is sanitized. + + httpx rejects any non-printable character in the proxy hostname, so a + NO_PROXY value containing a vertical tab, form feed, or Unicode line + separator must be normalized just like a newline. + """ + from openai._base_client import _DefaultHttpxClient + + _set_no_proxy(monkeypatch, f"localhost{separator}127.0.0.1") + client = _DefaultHttpxClient() + patterns = _mount_patterns(client) + assert any("localhost" in p for p in patterns) + assert any("127.0.0.1" in p for p in patterns) + client.close() + + +def test_httpx2_factory_sanitizes_no_proxy(monkeypatch: pytest.MonkeyPatch) -> None: + """The public DefaultHttpx2Client factory also sanitizes NO_PROXY.""" + from openai import DefaultHttpx2Client + + _set_no_proxy(monkeypatch, "localhost\n127.0.0.1") + client = DefaultHttpx2Client() + patterns = _mount_patterns(client) + assert any("localhost" in p for p in patterns) + assert any("127.0.0.1" in p for p in patterns) + client.close() + + +def test_httpx2_factory_trust_env_false_skips_sanitization(monkeypatch: pytest.MonkeyPatch) -> None: + """The public DefaultHttpx2Client factory respects trust_env=False.""" + from openai import DefaultHttpx2Client + + _set_no_proxy(monkeypatch, "localhost\n127.0.0.1") + client = DefaultHttpx2Client(trust_env=False) + assert os.environ.get("NO_PROXY") == "localhost\n127.0.0.1" + assert client._mounts == {} + client.close() + + +def test_async_httpx2_factory_sanitizes_no_proxy(monkeypatch: pytest.MonkeyPatch) -> None: + """The public DefaultAsyncHttpx2Client factory also sanitizes NO_PROXY.""" + from openai import DefaultAsyncHttpx2Client + + _set_no_proxy(monkeypatch, "localhost\n127.0.0.1") + client = DefaultAsyncHttpx2Client() + patterns = _mount_patterns(client) + assert any("localhost" in p for p in patterns) + assert any("127.0.0.1" in p for p in patterns) + + +def test_sanitizer_lock_replaced_after_fork() -> None: + """The sanitizer lock is replaced in the child after a fork. + + If a fork happens while another thread holds the lock, the child would + inherit the acquired lock and block forever on the next client + construction. os.register_at_fork must replace it. + """ + import multiprocessing + + import openai._base_client as base_client + + # 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") + q = ctx.Queue() + p = ctx.Process( + target=lambda: q.put(base_client._no_proxy_sanitizer_lock.locked()), + ) + p.start() + p.join(timeout=10) + child_locked = q.get(timeout=5) + finally: + base_client._no_proxy_sanitizer_lock.release() + + # The child must not inherit the acquired lock. + assert child_locked is False