diff --git a/comfy_cli/cloud/oauth.py b/comfy_cli/cloud/oauth.py index f6459fa8..fbde81d0 100644 --- a/comfy_cli/cloud/oauth.py +++ b/comfy_cli/cloud/oauth.py @@ -52,7 +52,7 @@ CLIENT_NAME, get_base_url, ) -from comfy_cli.http import NoRedirectHandler +from comfy_cli.http import NoRedirectHandler, build_http_only_opener # --------------------------------------------------------------------------- # Error types — caller maps these to renderer.error(code=...) codes. @@ -882,7 +882,7 @@ def _assert_https_or_loopback(url: str) -> None: raise _HTTPFail(0, f"refusing plaintext HTTP for OAuth endpoint: {url}") -_OAUTH_OPENER = urllib.request.build_opener(NoRedirectHandler()) +_OAUTH_OPENER = build_http_only_opener(NoRedirectHandler()) def _send_and_parse(req: urllib.request.Request) -> dict: diff --git a/comfy_cli/comfy_client.py b/comfy_cli/comfy_client.py index 02b8c8eb..78cfdb07 100644 --- a/comfy_cli/comfy_client.py +++ b/comfy_cli/comfy_client.py @@ -23,7 +23,7 @@ from dataclasses import dataclass from typing import Any -from comfy_cli.http import NoRedirectHandler +from comfy_cli.http import NoRedirectHandler, build_http_only_opener from comfy_cli.target import Target _LOOPBACK_HOSTS = {"localhost", "127.0.0.1", "::1", "[::1]"} @@ -95,7 +95,7 @@ class Unauthenticated(Exception): """Target needs auth but no valid session is present.""" -_OPENER = urllib.request.build_opener(NoRedirectHandler()) +_OPENER = build_http_only_opener(NoRedirectHandler()) def _assert_safe_url(url: str) -> None: diff --git a/comfy_cli/command/transfer.py b/comfy_cli/command/transfer.py index 08e971fa..179c4847 100644 --- a/comfy_cli/command/transfer.py +++ b/comfy_cli/command/transfer.py @@ -27,7 +27,7 @@ from comfy_cli import jobs_state from comfy_cli.comfy_client import Client, Unauthenticated, extract_output_entries -from comfy_cli.http import NoRedirectHandler +from comfy_cli.http import NoRedirectHandler, build_http_only_opener from comfy_cli.output import get_renderer from comfy_cli.output import rprint as pprint from comfy_cli.target import resolve_target @@ -105,8 +105,8 @@ def redirect_request(self, req, fp, code, msg, headers, newurl): return new_req -_TRANSFER_OPENER = urllib.request.build_opener(NoRedirectHandler("redirect refused (auth leak prevention)")) -_DOWNLOAD_OPENER = urllib.request.build_opener(_DownloadRedirectHandler()) +_TRANSFER_OPENER = build_http_only_opener(NoRedirectHandler("redirect refused (auth leak prevention)")) +_DOWNLOAD_OPENER = build_http_only_opener(_DownloadRedirectHandler()) # Per-output safety cap, shared by the HTTP download stream and local-output copies. _MAX_DOWNLOAD_BYTES = 10 * 1024 * 1024 * 1024 # 10 GB diff --git a/comfy_cli/cql/engine.py b/comfy_cli/cql/engine.py index a8068aaa..9c4bd472 100644 --- a/comfy_cli/cql/engine.py +++ b/comfy_cli/cql/engine.py @@ -22,7 +22,7 @@ from typing import Any from comfy_cli.cql._net import is_loopback_host -from comfy_cli.http import NoRedirectHandler +from comfy_cli.http import NoRedirectHandler, build_http_only_opener # --------------------------------------------------------------------------- # Types — mirrors nodegraph/types.go @@ -1087,7 +1087,7 @@ def _check_autogrow_required( _MAX_OBJECT_INFO_BYTES = 64 * 1024 * 1024 -_opener = urllib.request.build_opener(NoRedirectHandler()) +_opener = build_http_only_opener(NoRedirectHandler()) class LoadError(Exception): diff --git a/comfy_cli/cql/loader.py b/comfy_cli/cql/loader.py index c3d0e4b1..aab3926b 100644 --- a/comfy_cli/cql/loader.py +++ b/comfy_cli/cql/loader.py @@ -30,7 +30,7 @@ from comfy_cli.cql._net import is_loopback_host from comfy_cli.cql.errors import CQLRuntimeError -from comfy_cli.http import NoRedirectHandler +from comfy_cli.http import NoRedirectHandler, build_http_only_opener # Cap raw bytes read from disk or the network. Real `object_info` dumps are a # few MB; anything past 256 MiB is almost certainly a wrong path or a hostile @@ -38,7 +38,7 @@ MAX_INPUT_BYTES = 256 * 1024 * 1024 -_LOADER_OPENER = urllib.request.build_opener(NoRedirectHandler()) +_LOADER_OPENER = build_http_only_opener(NoRedirectHandler()) def load_graph( diff --git a/comfy_cli/http.py b/comfy_cli/http.py index cb4c559b..91b0002a 100644 --- a/comfy_cli/http.py +++ b/comfy_cli/http.py @@ -26,31 +26,61 @@ def http_error_301(self, req, fp, code, msg, headers): http_error_302 = http_error_303 = http_error_307 = http_error_308 = http_error_301 -def _build_authed_opener() -> urllib.request.OpenerDirector: - """Build the credential-carrying opener with http/https handlers only. - - ``build_opener()`` would also install ``FileHandler``/``FTPHandler``. Every - call site builds its URL from a trusted ``target.base_url``, so that isn't - reachable today, but this is the opener that attaches credentials — pinning - it to http(s) means a future caller can't be steered into a ``file://`` or - ``ftp://`` fetch. Unknown schemes fall to ``UnknownHandler``, which raises +def _http_only_proxy_handler() -> urllib.request.ProxyHandler: + """A ProxyHandler that can only proxy http(s). + + ``ProxyHandler()`` defaults to ``getproxies()`` and registers a + ``_open`` method for *every* entry it finds, so an ``ftp_proxy`` in + the environment would give the opener an ``ftp_open`` — servicing + ``ftp://`` through the proxy and stepping straight past the + ``UnknownHandler`` that ``build_http_only_opener`` relies on. Filtering the + map to http(s) keeps real proxy support (``proxy_bypass``/``no_proxy`` read + the environment directly, not this dict) while leaving non-http schemes + with nowhere to go. + """ + proxies = {scheme: url for scheme, url in urllib.request.getproxies().items() if scheme in ("http", "https")} + return urllib.request.ProxyHandler(proxies) + + +def build_http_only_opener(*handlers: urllib.request.BaseHandler) -> urllib.request.OpenerDirector: + """Build an opener that speaks http(s) and nothing else. + + ``build_opener()`` would also install ``FileHandler``/``FTPHandler``/ + ``DataHandler``. Our call sites build their URLs from a trusted + ``target.base_url``, so that isn't reachable today, but these openers + attach credentials — pinning them to http(s) means a future caller can't + be steered into a ``file://``, ``ftp://`` or ``data:`` fetch. Unknown + schemes fall to ``UnknownHandler``, which raises ``URLError("unknown url type")``. + + ``handlers`` are the caller's own additions (e.g. a redirect policy). As in + ``build_opener``, a caller-supplied handler replaces the default it + subclasses rather than being appended behind it. Note that no redirect + handler is installed unless the caller passes one, so a bare opener + surfaces a 30x as an ``HTTPError`` rather than following it. """ + defaults = [ + (urllib.request.ProxyHandler, _http_only_proxy_handler), + (urllib.request.HTTPHandler, urllib.request.HTTPHandler), + (urllib.request.HTTPDefaultErrorHandler, urllib.request.HTTPDefaultErrorHandler), + (urllib.request.HTTPErrorProcessor, urllib.request.HTTPErrorProcessor), + (urllib.request.UnknownHandler, urllib.request.UnknownHandler), + ] + # urllib.request only defines HTTPSHandler on an SSL-capable build; naming it + # unconditionally would blow up at import time on one without. + if hasattr(urllib.request, "HTTPSHandler"): + defaults.append((urllib.request.HTTPSHandler, urllib.request.HTTPSHandler)) + opener = urllib.request.OpenerDirector() - for handler in ( - urllib.request.ProxyHandler(), - urllib.request.HTTPHandler(), - urllib.request.HTTPSHandler(), - urllib.request.HTTPDefaultErrorHandler(), - urllib.request.HTTPErrorProcessor(), - NoRedirectHandler(), - urllib.request.UnknownHandler(), - ): + for klass, factory in defaults: + if not any(isinstance(handler, klass) for handler in handlers): + opener.add_handler(factory()) + for handler in handlers: opener.add_handler(handler) return opener -_AUTHED_OPENER = _build_authed_opener() +_AUTHED_OPENER = build_http_only_opener(NoRedirectHandler()) def build_authed_request( diff --git a/tests/comfy_cli/test_http_only_openers.py b/tests/comfy_cli/test_http_only_openers.py new file mode 100644 index 00000000..0abb53a0 --- /dev/null +++ b/tests/comfy_cli/test_http_only_openers.py @@ -0,0 +1,124 @@ +"""Scheme-pinning guards for every credential-carrying urllib opener. + +``urllib.request.build_opener()`` implicitly installs ``FileHandler``, +``FTPHandler`` and ``DataHandler``. An opener that attaches ``Authorization``/ +``X-API-Key`` must not speak those schemes: a caller steered into a ``file://`` +or ``ftp://`` URL could read unintended local content through an opener that +carries credentials. These tests mirror ``test_http.py``'s checks on the shared +``_AUTHED_OPENER`` for each of the other openers. +""" + +import urllib.error +import urllib.request + +import pytest + +from comfy_cli import comfy_client, http +from comfy_cli.cloud import oauth +from comfy_cli.command import transfer +from comfy_cli.cql import engine, loader + +NON_HTTP_URLS = ["file:///etc/passwd", "ftp://example.com/x", "data:text/plain,hi"] + +# Every opener the CLI builds, including _DOWNLOAD_OPENER: it deliberately +# follows redirects (its own handler strips auth headers and re-checks the +# scheme), but it should be scheme-pinned like the rest. +OPENERS = [ + ("http._AUTHED_OPENER", http._AUTHED_OPENER), + ("comfy_client._OPENER", comfy_client._OPENER), + ("cql.engine._opener", engine._opener), + ("cql.loader._LOADER_OPENER", loader._LOADER_OPENER), + ("cloud.oauth._OAUTH_OPENER", oauth._OAUTH_OPENER), + ("transfer._TRANSFER_OPENER", transfer._TRANSFER_OPENER), + ("transfer._DOWNLOAD_OPENER", transfer._DOWNLOAD_OPENER), +] +OPENER_IDS = [name for name, _ in OPENERS] + + +@pytest.mark.parametrize("opener", [o for _, o in OPENERS], ids=OPENER_IDS) +@pytest.mark.parametrize("url", NON_HTTP_URLS) +def test_opener_refuses_non_http_schemes(opener, url): + """Non-http(s) schemes fall through to UnknownHandler.""" + with pytest.raises(urllib.error.URLError) as exc_info: + opener.open(url) + assert "unknown url type" in str(exc_info.value.reason) + + +@pytest.mark.parametrize("opener", [o for _, o in OPENERS], ids=OPENER_IDS) +def test_opener_handler_set(opener): + """Only http(s)-relevant handlers are installed — no File/FTP/Data.""" + names = {type(h).__name__ for h in opener.handlers} + assert {"HTTPHandler", "HTTPSHandler", "UnknownHandler"} <= names + assert not names & {"FileHandler", "FTPHandler", "DataHandler"} + + +@pytest.mark.parametrize( + ("name", "opener"), + [(n, o) for n, o in OPENERS if n != "transfer._DOWNLOAD_OPENER"], + ids=[n for n, _ in OPENERS if n != "transfer._DOWNLOAD_OPENER"], +) +def test_credentialed_openers_refuse_redirects(name, opener): + """Every opener that carries credentials keeps its NoRedirectHandler.""" + assert any(isinstance(h, http.NoRedirectHandler) for h in opener.handlers) + + +def test_download_opener_still_follows_redirects(): + """_DOWNLOAD_OPENER's redirect handler is intentional — scheme-pinning it + must not turn it into a no-redirect opener.""" + assert any(isinstance(h, transfer._DownloadRedirectHandler) for h in transfer._DOWNLOAD_OPENER.handlers) + assert not any(isinstance(h, http.NoRedirectHandler) for h in transfer._DOWNLOAD_OPENER.handlers) + + +def test_build_http_only_opener_installs_caller_handlers(): + handler = http.NoRedirectHandler("custom message") + opener = http.build_http_only_opener(handler) + assert handler in opener.handlers + + +def _proxies(opener): + return next(h.proxies for h in opener.handlers if isinstance(h, urllib.request.ProxyHandler)) + + +def test_build_http_only_opener_keeps_http_proxy_support(monkeypatch): + """Proxy support is preserved for the schemes we actually speak.""" + monkeypatch.setenv("HTTP_PROXY", "http://proxy.example:3128") + monkeypatch.setenv("HTTPS_PROXY", "http://proxy.example:8080") + + proxies = _proxies(http.build_http_only_opener()) + assert proxies == {"http": "http://proxy.example:3128", "https": "http://proxy.example:8080"} + # ...and for the schemes we speak, that is exactly what build_opener resolves. + stdlib = _proxies(urllib.request.build_opener()) + assert proxies == {k: v for k, v in stdlib.items() if k in ("http", "https")} + + +def test_build_http_only_opener_ignores_non_http_proxies(monkeypatch): + """A proxy for a non-http scheme must not smuggle that scheme back in. + + ``ProxyHandler`` registers a ``_open`` per proxy entry, so an + ``ftp_proxy`` would otherwise hand the opener an ``ftp_open`` that wins + dispatch over ``UnknownHandler``. + """ + monkeypatch.setenv("FTP_PROXY", "http://proxy.example:3128") + monkeypatch.setenv("HTTPS_PROXY", "http://proxy.example:3128") + + opener = http.build_http_only_opener() + assert "ftp" not in _proxies(opener) + assert "ftp" not in opener.handle_open + with pytest.raises(urllib.error.URLError) as exc_info: + opener.open("ftp://example.com/x") + assert "unknown url type" in str(exc_info.value.reason) + + +def test_build_http_only_opener_lets_caller_override_a_default(): + """A caller-supplied handler replaces the default it subclasses, rather + than being appended behind it where it would never win dispatch.""" + + class PinnedHTTPSHandler(urllib.request.HTTPSHandler): + pass + + handler = PinnedHTTPSHandler() + opener = http.build_http_only_opener(handler) + + https_handlers = [h for h in opener.handlers if isinstance(h, urllib.request.HTTPSHandler)] + assert https_handlers == [handler] + assert opener.handle_open["https"] == [handler]