fix(http): pin the remaining credential-carrying urllib openers to http(s) handlers only (BE-3298)

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:

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:

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

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):

comfy_cli/cql/loader.py

@@ -30,15 +30,15 @@
 
 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
 # server and would just OOM the CLI before json.loads even fails.
 MAX_INPUT_BYTES = 256 * 1024 * 1024
 
 
-_LOADER_OPENER = urllib.request.build_opener(NoRedirectHandler())
+_LOADER_OPENER = build_http_only_opener(NoRedirectHandler())
 
 
 def load_graph(

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
+    ``<scheme>_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(

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 ``<scheme>_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]