From f319a9b05cdede51ecba0cfbd29c0a60d657e8e0 Mon Sep 17 00:00:00 2001 From: Matt Miller Date: Fri, 17 Jul 2026 01:15:37 -0700 Subject: [PATCH 1/3] fix(http): pin the remaining credential-carrying openers to http(s) (BE-3298) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `urllib.request.build_opener()` implicitly installs FileHandler, FTPHandler and DataHandler, so an opener that attaches Bearer / X-API-Key could be steered into reading unintended local content via a file:// or ftp:// URL. BE-3274 pinned `_AUTHED_OPENER`; the same pattern remained at five other credential-carrying openers. Generalize that PR's private `_build_authed_opener()` into a reusable `build_http_only_opener(*handlers)` and route every opener through it, preserving each site's existing handler argument. `_DOWNLOAD_OPENER` keeps its redirect-following `_DownloadRedirectHandler` — only its scheme set is pinned. Not known to be exploitable: every call site builds its URL from a trusted base_url or is already scheme-guarded upstream. This is the remaining scheme-restriction half of the same defense-in-depth. --- comfy_cli/cloud/oauth.py | 4 +- comfy_cli/comfy_client.py | 4 +- comfy_cli/command/transfer.py | 6 +- comfy_cli/cql/engine.py | 4 +- comfy_cli/cql/loader.py | 4 +- comfy_cli/http.py | 28 +++++--- tests/comfy_cli/test_http_only_openers.py | 87 +++++++++++++++++++++++ 7 files changed, 116 insertions(+), 21 deletions(-) create mode 100644 tests/comfy_cli/test_http_only_openers.py 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..8fb10e20 100644 --- a/comfy_cli/http.py +++ b/comfy_cli/http.py @@ -26,15 +26,23 @@ 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 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); + everything else mirrors ``build_opener``'s defaults, including + ``ProxyHandler``, which only registers a scheme when the environment + configures a proxy for 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. """ opener = urllib.request.OpenerDirector() for handler in ( @@ -43,14 +51,14 @@ def _build_authed_opener() -> urllib.request.OpenerDirector: urllib.request.HTTPSHandler(), urllib.request.HTTPDefaultErrorHandler(), urllib.request.HTTPErrorProcessor(), - NoRedirectHandler(), urllib.request.UnknownHandler(), + *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..58df2fb9 --- /dev/null +++ b/tests/comfy_cli/test_http_only_openers.py @@ -0,0 +1,87 @@ +"""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 test_build_http_only_opener_matches_build_opener_proxy_support(monkeypatch): + """Proxy support is preserved: ProxyHandler registers exactly the schemes + build_opener's own ProxyHandler would for the same environment.""" + monkeypatch.setenv("HTTPS_PROXY", "http://proxy.example:3128") + + def proxies(opener): + return next(h.proxies for h in opener.handlers if isinstance(h, urllib.request.ProxyHandler)) + + assert proxies(http.build_http_only_opener()) == proxies(urllib.request.build_opener()) + assert "https" in proxies(http.build_http_only_opener()) From 2774121813e95049c9b5dcc3d2f8393b9f8b6ea6 Mon Sep 17 00:00:00 2001 From: Matt Miller Date: Fri, 17 Jul 2026 02:58:24 -0700 Subject: [PATCH 2/3] fix(http): close the proxy scheme-smuggling hole in build_http_only_opener (BE-3298) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Per cursor-review panel on #542: - ProxyHandler() defaults to getproxies() and registers a _open for every entry, so an ftp_proxy in the environment gave the opener an ftp_open that won dispatch over UnknownHandler — servicing ftp:// through the proxy and defeating the scheme pinning this PR exists to enforce. Verified reproducible before the fix. The proxy map is now filtered to http(s); no_proxy/proxy_bypass read the environment directly, so bypass still works. - Guard HTTPSHandler on hasattr: urllib.request only defines it on an SSL-capable build, so naming it unconditionally raised AttributeError at import time on one without, taking the whole module with it. - Mirror build_opener's override semantics: a caller-supplied handler now replaces the default it subclasses instead of being appended behind it, where it would silently never be consulted. No current caller is affected: every opener still dispatches exactly http/https/unknown, and http(s) proxying is unchanged (asserted end-to-end against a live proxy). Co-Authored-By: Claude Opus 4.8 --- comfy_cli/http.py | 52 ++++++++++++++++------- tests/comfy_cli/test_http_only_openers.py | 51 +++++++++++++++++++--- 2 files changed, 81 insertions(+), 22 deletions(-) diff --git a/comfy_cli/http.py b/comfy_cli/http.py index 8fb10e20..b5a35094 100644 --- a/comfy_cli/http.py +++ b/comfy_cli/http.py @@ -26,6 +26,22 @@ 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 _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. @@ -37,23 +53,29 @@ def build_http_only_opener(*handlers: urllib.request.BaseHandler) -> urllib.requ schemes fall to ``UnknownHandler``, which raises ``URLError("unknown url type")``. - ``handlers`` are the caller's own additions (e.g. a redirect policy); - everything else mirrors ``build_opener``'s defaults, including - ``ProxyHandler``, which only registers a scheme when the environment - configures a proxy for 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. + ``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(), - urllib.request.UnknownHandler(), - *handlers, - ): + 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 diff --git a/tests/comfy_cli/test_http_only_openers.py b/tests/comfy_cli/test_http_only_openers.py index 58df2fb9..0abb53a0 100644 --- a/tests/comfy_cli/test_http_only_openers.py +++ b/tests/comfy_cli/test_http_only_openers.py @@ -75,13 +75,50 @@ def test_build_http_only_opener_installs_caller_handlers(): assert handler in opener.handlers -def test_build_http_only_opener_matches_build_opener_proxy_support(monkeypatch): - """Proxy support is preserved: ProxyHandler registers exactly the schemes - build_opener's own ProxyHandler would for the same environment.""" +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") - def proxies(opener): - return next(h.proxies for h in opener.handlers if isinstance(h, urllib.request.ProxyHandler)) + 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) - assert proxies(http.build_http_only_opener()) == proxies(urllib.request.build_opener()) - assert "https" in proxies(http.build_http_only_opener()) + https_handlers = [h for h in opener.handlers if isinstance(h, urllib.request.HTTPSHandler)] + assert https_handlers == [handler] + assert opener.handle_open["https"] == [handler] From 556d542a21eb3d0352da85c02b722f14d76f9de9 Mon Sep 17 00:00:00 2001 From: Matt Miller Date: Fri, 17 Jul 2026 03:10:52 -0700 Subject: [PATCH 3/3] docs(http): reflow _http_only_proxy_handler docstring (BE-3298) Co-Authored-By: Claude Opus 4.8 --- comfy_cli/http.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/comfy_cli/http.py b/comfy_cli/http.py index b5a35094..91b0002a 100644 --- a/comfy_cli/http.py +++ b/comfy_cli/http.py @@ -34,9 +34,9 @@ def _http_only_proxy_handler() -> urllib.request.ProxyHandler: 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. + 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)