fix(doctor): sanitize bracketed-IPv6 NO_PROXY for --live probes (#118159)
Carried-over finding from #124768 (credit: @Wenfengcheng for the discovery, @Enough1122 for the review): hermes doctor --live's _http_get (hermes_cli/doctor_live.py) still called bare httpx.get with no proxy seam, so the same bracketed-IPv6 NO_PROXY entry ([::1], Clash Verge/ mihomo) that #131802 fixes for the connectivity table raised InvalidURL: Invalid port ':1]' at client construction for every live metadata probe; _run_one's catch-all dressed the crash up as '(Invalid port: :1])' fail rows — the user is told their backends are broken when the only defect is a curl-style bypass entry. Route _http_get — the single HTTP GET seam behind _keyed_probe and _probe_audio — through the same normalize_proxy_env_vars() seam the PR already uses (no new config, idempotent, byte-identical on a clean env), mirroring run_probes' sanitize-at-the-seam pattern. Red on the PR head: 7 new tests fail with the exact InvalidURL and the '✗ Firecrawl (Invalid port: ':1]')' row against a healthy local origin that received zero requests; green after: 26/26 in test_doctor_live.py.
This commit is contained in:
committed by
brooklyn!
parent
cf98cef46a
commit
f4a9bcab98
@@ -13,6 +13,7 @@ from typing import Callable, List, Optional
|
||||
from hermes_cli.browser_runtime import chromium_executable
|
||||
from hermes_cli.doctor import _section, check_info
|
||||
from hermes_cli.doctor_report import check_fail, check_ok, check_warn
|
||||
from utils import normalize_proxy_env_vars
|
||||
|
||||
DEFAULT_PROBE_TIMEOUT = 10.0
|
||||
|
||||
@@ -44,6 +45,10 @@ class ProbeResult:
|
||||
def _http_get(url: str, headers: Optional[dict] = None, timeout: Optional[float] = None):
|
||||
"""Single HTTP GET seam for all metadata probes."""
|
||||
import httpx
|
||||
# Sanitize at the seam (idempotent, no-op on a clean env): a bracketed-IPv6 NO_PROXY entry
|
||||
# ([::1], Clash Verge/mihomo) makes this bare trust_env client raise InvalidURL at
|
||||
# construction (#118159), which _run_one's catch-all would dress up as backend downtime.
|
||||
normalize_proxy_env_vars()
|
||||
return httpx.get(url, headers=headers or {}, timeout=timeout)
|
||||
|
||||
|
||||
|
||||
@@ -6,6 +6,9 @@ All probes are mocked at the HTTP/client layer; no real network calls are made.
|
||||
from __future__ import annotations
|
||||
|
||||
import argparse
|
||||
import os
|
||||
import threading
|
||||
from http.server import BaseHTTPRequestHandler, HTTPServer
|
||||
from types import SimpleNamespace
|
||||
|
||||
import pytest
|
||||
@@ -21,6 +24,30 @@ from tools import browser_tool_install as bt_install
|
||||
# to a constant, so TestBrowserAvailableNpxRung can exercise the real function.
|
||||
_real_browser_available = doctor_live._browser_available
|
||||
|
||||
# The exact environment Clash Verge / mihomo exports (issue #118159): a bracketed-IPv6
|
||||
# NO_PROXY entry that httpx 0.28.1 cannot parse into a proxy mount.
|
||||
_CLASH_NO_PROXY = "127.0.0.1,localhost,::1,[::1]"
|
||||
|
||||
|
||||
@pytest.fixture
|
||||
def clash_env(monkeypatch):
|
||||
for key in ("NO_PROXY", "no_proxy", "HTTP_PROXY", "HTTPS_PROXY", "ALL_PROXY",
|
||||
"http_proxy", "https_proxy", "all_proxy"):
|
||||
monkeypatch.delenv(key, raising=False)
|
||||
monkeypatch.setenv("NO_PROXY", _CLASH_NO_PROXY)
|
||||
monkeypatch.setenv("no_proxy", _CLASH_NO_PROXY)
|
||||
|
||||
|
||||
class _Probes(BaseHTTPRequestHandler):
|
||||
"""Minimal 200-origin for live-path tests; every request is a bare metadata GET."""
|
||||
|
||||
def do_GET(self):
|
||||
self.send_response(200)
|
||||
self.end_headers()
|
||||
|
||||
def log_message(self, format, *args):
|
||||
pass # keep test output clean
|
||||
|
||||
|
||||
def _args(live: bool = True) -> argparse.Namespace:
|
||||
return argparse.Namespace(live=live)
|
||||
@@ -154,6 +181,80 @@ class TestConfiguredOnlySelection:
|
||||
assert results["Browser"].status == "pass"
|
||||
|
||||
|
||||
@pytest.mark.parametrize("kind", ["NO_PROXY", "no_proxy"])
|
||||
@pytest.mark.parametrize("entry", ["[::1]", "[::1]:8080", "::1/128"])
|
||||
def test_http_get_sanitizes_bracketed_ipv6_no_proxy(monkeypatch, kind, entry):
|
||||
# The Clash Verge / mihomo export from #118159: httpx 0.28.1 turns a bracketed or
|
||||
# CIDR IPv6 NO_PROXY entry into an unparseable mount pattern and raises InvalidURL
|
||||
# at client construction, before any request. doctor --live's _http_get must not.
|
||||
for key in ("NO_PROXY", "no_proxy", "HTTP_PROXY", "HTTPS_PROXY", "ALL_PROXY",
|
||||
"http_proxy", "https_proxy", "all_proxy"):
|
||||
monkeypatch.delenv(key, raising=False)
|
||||
monkeypatch.setenv(kind, f"127.0.0.1,localhost,::1,{entry}")
|
||||
# A bare local-socket GET proves the client was constructible; 127.0.0.1's own
|
||||
# NO_PROXY membership keeps it out of any proxy's reach.
|
||||
with HTTPServer(("127.0.0.1", 0), _Probes) as httpd:
|
||||
thread = threading.Thread(target=httpd.serve_forever, daemon=True)
|
||||
thread.start()
|
||||
try:
|
||||
resp = doctor_live._http_get(
|
||||
f"http://127.0.0.1:{httpd.server_port}/", timeout=5.0)
|
||||
assert resp.status_code == 200
|
||||
finally:
|
||||
httpd.shutdown()
|
||||
thread.join(timeout=5)
|
||||
|
||||
|
||||
def test_run_live_checks_sanitizes_clash_env_and_probes_pass(clash_env, monkeypatch):
|
||||
# Red-on-base end-to-end: _keyed_probe -> _http_get -> bare httpx.get raised
|
||||
# InvalidURL under this env; _run_one's catch-all dressed it up as
|
||||
# "(Invalid port: ':1]')" — a backend-failure verdict for a healthy service.
|
||||
monkeypatch.setenv("FIRECRAWL_API_KEY", "fc-test")
|
||||
|
||||
# Real HTTP origin: a 200 from an actual socket proves both client construction
|
||||
# and the request reaching a reachable origin. The remote-credential probes would
|
||||
# send real auth material or 401 without proving either.
|
||||
with HTTPServer(("127.0.0.1", 0), _Probes) as httpd:
|
||||
thread = threading.Thread(target=httpd.serve_forever, daemon=True)
|
||||
thread.start()
|
||||
try:
|
||||
monkeypatch.setitem(
|
||||
doctor_live._KEYED_PROBES, "Firecrawl",
|
||||
(f"http://127.0.0.1:{httpd.server_port}/v2/team/credit-usage",
|
||||
"FIRECRAWL_API_KEY", "Bearer"))
|
||||
issues: list[str] = []
|
||||
results = {r.name: r for r in run_live_checks(issues)}
|
||||
assert results["Firecrawl"].status == "pass"
|
||||
assert issues == []
|
||||
assert "Invalid port" not in (results["Firecrawl"].detail or "")
|
||||
finally:
|
||||
httpd.shutdown()
|
||||
thread.join(timeout=5)
|
||||
# The seam rewrote the entry in place, the established pattern of this seam
|
||||
# (chat transports and run_probes already do exactly this).
|
||||
assert "[::1]" not in os.environ["NO_PROXY"]
|
||||
assert os.environ["no_proxy"] == "127.0.0.1,localhost,::1"
|
||||
|
||||
|
||||
def test_run_live_checks_leaves_a_clean_environment_untouched(monkeypatch):
|
||||
for key in ("NO_PROXY", "no_proxy", "HTTP_PROXY", "HTTPS_PROXY", "ALL_PROXY",
|
||||
"http_proxy", "https_proxy", "all_proxy"):
|
||||
monkeypatch.delenv(key, raising=False)
|
||||
monkeypatch.setenv("NO_PROXY", "example.com,10.0.0.0/8")
|
||||
# _http_get is untouched by the sanitize-once design; a socket-level 200 proves
|
||||
# the live path still works end to end when there is nothing to sanitize.
|
||||
with HTTPServer(("127.0.0.1", 0), _Probes) as httpd:
|
||||
thread = threading.Thread(target=httpd.serve_forever, daemon=True)
|
||||
thread.start()
|
||||
try:
|
||||
assert doctor_live._http_get(
|
||||
f"http://127.0.0.1:{httpd.server_port}/", timeout=5.0).status_code == 200
|
||||
finally:
|
||||
httpd.shutdown()
|
||||
thread.join(timeout=5)
|
||||
assert os.environ["NO_PROXY"] == "example.com,10.0.0.0/8"
|
||||
|
||||
|
||||
class TestBrowserAvailable:
|
||||
"""Live probes use the same passive selection as browser execution."""
|
||||
|
||||
|
||||
Reference in New Issue
Block a user