diff --git a/contributors/emails/justin@actual.inc b/contributors/emails/justin@actual.inc new file mode 100644 index 000000000000..d09d2f84ccc2 --- /dev/null +++ b/contributors/emails/justin@actual.inc @@ -0,0 +1,2 @@ +somewheresy +# PR #83554 (Actual authenticated model discovery) diff --git a/hermes_cli/urllib_security.py b/hermes_cli/urllib_security.py index 616ff7ade48e..ac34411ef7a6 100644 --- a/hermes_cli/urllib_security.py +++ b/hermes_cli/urllib_security.py @@ -3,15 +3,28 @@ from __future__ import annotations import copy +import logging +import os +import ssl +import sys import urllib.parse import urllib.request from collections.abc import Callable, Iterable +from pathlib import Path from typing import Any +logger = logging.getLogger(__name__) + # Headers safe to forward to a different origin. Everything else is dropped: # custom provider headers routinely carry credentials under arbitrary names. _CROSS_ORIGIN_SAFE_HEADERS = frozenset({"accept", "user-agent"}) _DEFAULT_PORTS = {"http": 80, "https": 443} +_CA_BUNDLE_ENV_VARS = ( + "HERMES_CA_BUNDLE", + "SSL_CERT_FILE", + "REQUESTS_CA_BUNDLE", + "CURL_CA_BUNDLE", +) def url_origin(url: str) -> tuple[str, str, int | None]: @@ -83,17 +96,67 @@ def _sanitize(self, request: urllib.request.Request): https_request = _sanitize +def _resolved_https_context() -> ssl.SSLContext | None: + """Return the explicit CA context for Hermes-owned urllib openers.""" + ca_bundle = next( + ( + value + for name in _CA_BUNDLE_ENV_VARS + if (value := os.getenv(name, "").strip()) + ), + "", + ) + if ca_bundle: + ca_path = Path(ca_bundle).expanduser() + if ca_path.is_file(): + try: + return ssl.create_default_context(cafile=str(ca_path)) + except (OSError, ssl.SSLError) as exc: + logger.warning( + "CA bundle could not be loaded from %s: %s — falling back to default certificates", + ca_bundle, + exc, + ) + else: + logger.warning( + "CA bundle path does not exist: %s — falling back to default certificates", + ca_bundle, + ) + + if sys.platform != "darwin": + return None + + try: + import certifi + + return ssl.create_default_context(cafile=certifi.where()) + except (ImportError, OSError, ssl.SSLError) as exc: + logger.warning( + "Could not load certifi for urllib HTTPS verification: %s — falling back to default certificates", + exc, + ) + return None + + def _secure_opener_from_installed_policy(original_url: str, *, ssl_context=None): """Clone the installed opener's handlers, replacing redirect policy only. When ``ssl_context`` is provided, the cloned HTTPS handler is replaced with one bound to that context so per-provider TLS settings (``ssl_ca_cert`` / - ``ssl_verify``) apply to this request. When it is None, the installed - opener's TLS policy is preserved unchanged (env / certifi default). + ``ssl_verify``) apply to this request. When it is None, Hermes-owned + openers get an explicit CA default via ``_resolved_https_context`` (env + bundle first, certifi on macOS); an application-installed opener's TLS + policy is preserved unchanged. """ installed = getattr(urllib.request, "_opener", None) if installed is None: - installed = urllib.request.build_opener() + context = _resolved_https_context() + if context is None: + installed = urllib.request.build_opener() + else: + installed = urllib.request.build_opener( + urllib.request.HTTPSHandler(context=context) + ) _https_handler_cls = getattr(urllib.request, "HTTPSHandler", None) handlers = [ diff --git a/plugins/model-providers/actual/__init__.py b/plugins/model-providers/actual/__init__.py index 123892d4a686..0141dd8175d4 100644 --- a/plugins/model-providers/actual/__init__.py +++ b/plugins/model-providers/actual/__init__.py @@ -61,8 +61,10 @@ def fetch_models( req.add_header("Accept", "application/json") req.add_header("User-Agent", _profile_user_agent()) + from hermes_cli.urllib_security import open_credentialed_url + try: - with urllib.request.urlopen(req, timeout=timeout) as resp: + with open_credentialed_url(req, timeout=timeout) as resp: data = json.loads(resp.read().decode()) items = data if isinstance(data, list) else data.get("data", []) return [m["id"] for m in items if isinstance(m, dict) and "id" in m] diff --git a/tests/hermes_cli/test_actual_provider.py b/tests/hermes_cli/test_actual_provider.py index 2e9116c08e90..e373b1eb18c5 100644 --- a/tests/hermes_cli/test_actual_provider.py +++ b/tests/hermes_cli/test_actual_provider.py @@ -48,11 +48,25 @@ def test_actual_aliases_and_profile_metadata(): def test_actual_base_url_normalization(): - assert normalize_actual_base_url("https://api.actual.inc") == DEFAULT_ACTUAL_BASE_URL - assert normalize_actual_base_url("https://api.actual.inc/v1") == DEFAULT_ACTUAL_BASE_URL - assert normalize_actual_base_url("http://127.0.0.1:8080") == DEFAULT_ACTUAL_LOCAL_BASE_URL - assert normalize_actual_base_url("http://127.0.0.1:8080/v1") == DEFAULT_ACTUAL_LOCAL_BASE_URL - assert normalize_actual_base_url("http://localhost:8080/") == "http://localhost:8080/v1" + assert ( + normalize_actual_base_url("https://api.actual.inc") == DEFAULT_ACTUAL_BASE_URL + ) + assert ( + normalize_actual_base_url("https://api.actual.inc/v1") + == DEFAULT_ACTUAL_BASE_URL + ) + assert ( + normalize_actual_base_url("http://127.0.0.1:8080") + == DEFAULT_ACTUAL_LOCAL_BASE_URL + ) + assert ( + normalize_actual_base_url("http://127.0.0.1:8080/v1") + == DEFAULT_ACTUAL_LOCAL_BASE_URL + ) + assert ( + normalize_actual_base_url("http://localhost:8080/") + == "http://localhost:8080/v1" + ) def test_actual_credentials_default_to_hosted_api(monkeypatch): @@ -173,13 +187,13 @@ def __exit__(self, *args): def read(self): return json.dumps({"data": [{"id": "actual/local-model"}]}).encode() - def _urlopen(req, timeout=0): + def _open(req, timeout=0): seen["url"] = req.full_url seen["auth"] = req.get_header("Authorization") seen["timeout"] = timeout return _Response() - monkeypatch.setattr("urllib.request.urlopen", _urlopen) + monkeypatch.setattr("hermes_cli.urllib_security.open_credentialed_url", _open) assert profile.fetch_models(api_key=None, timeout=1.5) == ["actual/local-model"] assert seen["url"] == DEFAULT_ACTUAL_LOCAL_BASE_URL + "/models" @@ -187,12 +201,93 @@ def _urlopen(req, timeout=0): assert seen["timeout"] == 1.5 +def test_actual_profile_fetch_models_sends_credential_only_to_original_origin( + monkeypatch, +): + """fetch_models must route through the shared redirect-credential guard. + + ActualProfile overrides ProviderProfile.fetch_models with its own + base_url resolution, and previously called raw urllib.request.urlopen + directly instead of the base class's open_credentialed_url — losing the + protection that strips the Authorization header when a redirect leaves + the original host. Exercises the real SafeCredentialRedirectHandler + (no mocking of open_credentialed_url itself) against a local HTTP + server that 302s to a different origin, mirroring + test_urllib_security.py's end-to-end redirect tests. + """ + import http.server + import threading + + _clear_actual_env(monkeypatch) + profile = get_provider_profile("actual") + + source_auth_headers: list[str | None] = [] + target_auth_headers: list[str | None] = [] + + class _RedirectTargetHandler(http.server.BaseHTTPRequestHandler): + def do_GET(self): + target_auth_headers.append(self.headers.get("Authorization")) + body = json.dumps({"data": [{"id": "should-not-be-trusted"}]}).encode() + self.send_response(200) + self.send_header("Content-Type", "application/json") + self.send_header("Content-Length", str(len(body))) + self.end_headers() + self.wfile.write(body) + + def log_message(self, *_args): + pass + + target_server = http.server.HTTPServer(("127.0.0.1", 0), _RedirectTargetHandler) + target_thread = threading.Thread(target=target_server.serve_forever, daemon=True) + target_thread.start() + target_port = target_server.server_address[1] + + class _RedirectingHandler(http.server.BaseHTTPRequestHandler): + def do_GET(self): + source_auth_headers.append(self.headers.get("Authorization")) + self.send_response(302) + self.send_header("Location", f"http://127.0.0.1:{target_port}/models") + self.end_headers() + + def log_message(self, *_args): + pass + + redirect_server = http.server.HTTPServer(("127.0.0.1", 0), _RedirectingHandler) + redirect_thread = threading.Thread( + target=redirect_server.serve_forever, daemon=True + ) + redirect_thread.start() + redirect_port = redirect_server.server_address[1] + + try: + result = profile.fetch_models( + api_key="actual-secret-token", + base_url=f"http://127.0.0.1:{redirect_port}", + timeout=5.0, + ) + finally: + redirect_server.shutdown() + target_server.shutdown() + redirect_thread.join(timeout=2.0) + target_thread.join(timeout=2.0) + + assert result == ["should-not-be-trusted"], ( + "sanity check: the redirect must actually have been followed" + ) + assert source_auth_headers == ["Bearer actual-secret-token"] + assert target_auth_headers == [None], ( + "Authorization header leaked to a different origin after a redirect" + ) + + def test_actual_provider_model_ids_use_local_profile_catalog(monkeypatch): _clear_actual_env(monkeypatch) monkeypatch.setenv("ACTUAL_BASE_URL", "http://127.0.0.1:8080") profile = get_provider_profile("actual") - with patch.object(profile, "fetch_models", return_value=["actual/local-model"]) as fetch: + with patch.object( + profile, "fetch_models", return_value=["actual/local-model"] + ) as fetch: assert provider_model_ids("actual") == ["actual/local-model"] fetch.assert_called_once_with( @@ -201,6 +296,32 @@ def test_actual_provider_model_ids_use_local_profile_catalog(monkeypatch): ) +def test_actual_hosted_model_ids_send_resolved_credential(monkeypatch): + _clear_actual_env(monkeypatch) + monkeypatch.setenv("ACTUAL_API_KEY", "actual-test-key") + profile = get_provider_profile("actual") + + with patch.object( + profile, "fetch_models", return_value=["actual/hosted-model"] + ) as fetch: + assert provider_model_ids("actual") == ["actual/hosted-model"] + + fetch.assert_called_once_with( + api_key="actual-test-key", + base_url=DEFAULT_ACTUAL_BASE_URL, + ) + + +def test_actual_hosted_model_ids_do_not_probe_without_credentials(monkeypatch): + _clear_actual_env(monkeypatch) + profile = get_provider_profile("actual") + + with patch.object(profile, "fetch_models") as fetch: + assert provider_model_ids("actual") == [] + + fetch.assert_not_called() + + def test_actual_codex_transport_clamps_reasoning_effort(): """Actual's SGLang/vLLM backends only accept none/low/medium/high/max. diff --git a/tests/hermes_cli/test_urllib_security.py b/tests/hermes_cli/test_urllib_security.py index 679491e56ef0..f79379e04a1d 100644 --- a/tests/hermes_cli/test_urllib_security.py +++ b/tests/hermes_cli/test_urllib_security.py @@ -4,6 +4,7 @@ import json from http.server import BaseHTTPRequestHandler, ThreadingHTTPServer +import ssl from threading import Thread import urllib.error import urllib.request @@ -37,9 +38,10 @@ class _RecordingHandler(BaseHTTPRequestHandler): requests: list[tuple[str, dict[str, str]]] = [] def _record(self) -> None: - type(self).requests.append( - (self.command, {name.lower(): value for name, value in self.headers.items()}) - ) + type(self).requests.append(( + self.command, + {name.lower(): value for name, value in self.headers.items()}, + )) def do_GET(self): if self.path.startswith("/redirect"): @@ -86,8 +88,6 @@ def _credential_headers() -> dict[str, str]: } - - def test_cross_host_redirect_drops_arbitrary_credentials_on_wire(): source = _server() sink = _server() @@ -140,10 +140,6 @@ def test_same_host_different_port_drops_credentials_on_wire(): assert "cf-access-client-secret" not in headers - - - - def test_post_307_remains_rejected_by_urllib(): request = urllib.request.Request( "https://models.example.test/load", @@ -183,10 +179,6 @@ def factory(*handlers): assert calls == [("https://models.example.test/models", 7)] - - - - def test_installed_request_processor_cannot_resurrect_cross_origin_secret( monkeypatch, ): @@ -238,9 +230,7 @@ def test_multihop_redirects_never_resurrect_credentials(): "https://a.example.test/step-two", ) assert same_origin is not None - same_headers = { - name.lower(): value for name, value in same_origin.header_items() - } + same_headers = {name.lower(): value for name, value in same_origin.header_items()} assert "authorization" in same_headers cross_origin = handler.redirect_request( @@ -252,9 +242,7 @@ def test_multihop_redirects_never_resurrect_credentials(): "https://b.example.test/step-three", ) assert cross_origin is not None - cross_headers = { - name.lower(): value for name, value in cross_origin.header_items() - } + cross_headers = {name.lower(): value for name, value in cross_origin.header_items()} assert "authorization" not in cross_headers assert "cf-access-client-secret" not in cross_headers @@ -267,9 +255,7 @@ def test_multihop_redirects_never_resurrect_credentials(): "https://a.example.test/final", ) assert returned is not None - returned_headers = { - name.lower(): value for name, value in returned.header_items() - } + returned_headers = {name.lower(): value for name, value in returned.header_items()} assert "authorization" not in returned_headers assert "cf-access-client-secret" not in returned_headers @@ -395,3 +381,133 @@ def test_azure_anthropic_probe_drops_api_key_and_bearer_on_redirect(): assert "api-key" not in headers +def _clear_ca_bundle_env(monkeypatch) -> None: + for name in ( + "HERMES_CA_BUNDLE", + "SSL_CERT_FILE", + "REQUESTS_CA_BUNDLE", + "CURL_CA_BUNDLE", + ): + monkeypatch.delenv(name, raising=False) + + +def test_hermes_owned_opener_uses_resolved_https_context(monkeypatch): + import hermes_cli.urllib_security as urllib_security + + context = ssl.create_default_context() + monkeypatch.setattr(urllib.request, "_opener", None) + monkeypatch.setattr(urllib_security, "_resolved_https_context", lambda: context) + + opener = urllib_security._secure_opener_from_installed_policy( + "https://models.example.test/catalog" + ) + + https_handlers = [ + handler + for handler in opener.handlers + if isinstance(handler, urllib.request.HTTPSHandler) + ] + assert len(https_handlers) == 1 + assert getattr(https_handlers[0], "_context", None) is context + + +def test_resolved_https_context_prefers_configured_ca_bundle(monkeypatch, tmp_path): + import hermes_cli.urllib_security as urllib_security + + _clear_ca_bundle_env(monkeypatch) + ca_bundle = tmp_path / "corporate-ca.pem" + ca_bundle.touch() + expected_context = ssl.create_default_context() + seen: list[str | None] = [] + + def create_default_context(*, cafile=None): + seen.append(cafile) + return expected_context + + monkeypatch.setenv("HERMES_CA_BUNDLE", str(ca_bundle)) + monkeypatch.setattr(ssl, "create_default_context", create_default_context) + + assert urllib_security._resolved_https_context() is expected_context + assert seen == [str(ca_bundle)] + + +def test_resolved_https_context_uses_certifi_on_macos(monkeypatch): + import certifi + import hermes_cli.urllib_security as urllib_security + + _clear_ca_bundle_env(monkeypatch) + expected_context = ssl.create_default_context() + seen: list[str | None] = [] + + def create_default_context(*, cafile=None): + seen.append(cafile) + return expected_context + + monkeypatch.setattr(urllib_security.sys, "platform", "darwin") + monkeypatch.setattr(certifi, "where", lambda: "/certifi/cacert.pem") + monkeypatch.setattr(ssl, "create_default_context", create_default_context) + + assert urllib_security._resolved_https_context() is expected_context + assert seen == ["/certifi/cacert.pem"] + + +def test_invalid_ca_bundle_falls_back_to_certifi_on_macos(monkeypatch, tmp_path): + import certifi + import hermes_cli.urllib_security as urllib_security + + _clear_ca_bundle_env(monkeypatch) + missing_bundle = tmp_path / "missing-ca.pem" + expected_context = ssl.create_default_context() + seen: list[str | None] = [] + + def create_default_context(*, cafile=None): + seen.append(cafile) + return expected_context + + monkeypatch.setenv("HERMES_CA_BUNDLE", str(missing_bundle)) + monkeypatch.setattr(urllib_security.sys, "platform", "darwin") + monkeypatch.setattr(certifi, "where", lambda: "/certifi/cacert.pem") + monkeypatch.setattr(ssl, "create_default_context", create_default_context) + + assert urllib_security._resolved_https_context() is expected_context + assert seen == ["/certifi/cacert.pem"] + + +def test_resolved_https_context_keeps_stdlib_default_off_macos(monkeypatch): + import hermes_cli.urllib_security as urllib_security + + _clear_ca_bundle_env(monkeypatch) + monkeypatch.setattr(urllib_security.sys, "platform", "linux") + + assert urllib_security._resolved_https_context() is None + + +def test_installed_https_context_is_preserved(monkeypatch): + import hermes_cli.urllib_security as urllib_security + + context = ssl.create_default_context() + installed = urllib.request.build_opener( + urllib.request.HTTPSHandler(context=context) + ) + monkeypatch.setattr(urllib.request, "_opener", installed) + + def unexpected_context_resolution(): + raise AssertionError("installed TLS policy must remain authoritative") + + monkeypatch.setattr( + urllib_security, + "_resolved_https_context", + unexpected_context_resolution, + ) + + opener = urllib_security._secure_opener_from_installed_policy( + "https://models.example.test/catalog" + ) + + https_handlers = [ + handler + for handler in opener.handlers + if isinstance(handler, urllib.request.HTTPSHandler) + ] + assert len(https_handlers) == 1 + assert getattr(https_handlers[0], "_context", None) is context