refactor(http): shared capped-read request_json; migrate workflow + models search, fixing search's uncaught oversize/non-UTF-8 crashes (BE-4363)

comfy_cli/comfy_client.py

@@ -24,10 +24,9 @@
 from typing import Any
 
 from comfy_cli.http import NoRedirectHandler, build_http_only_opener, target_auth_headers
+from comfy_cli.http import assert_safe_url as _assert_safe_url
 from comfy_cli.target import Target
 
-_LOOPBACK_HOSTS = {"localhost", "127.0.0.1", "::1", "[::1]"}
-
 # Transient HTTP failures during polling should back off and retry, not abort.
 # 429 (rate limit) is retried for any method — the request was rejected, not
 # processed, so even a POST is safe to repeat. Transient 5xx is retried for
@@ -98,24 +97,6 @@ class Unauthenticated(Exception):
 _OPENER = build_http_only_opener(NoRedirectHandler())
 
 
-def _assert_safe_url(url: str) -> None:
-    """Reject plaintext HTTP for non-loopback hosts.
-
-    Anything carrying a Bearer token over the wire must be HTTPS unless the
-    host is a loopback address (where there's no network to sniff).
-    """
-    parsed = urllib.parse.urlsplit(url)
-    if parsed.scheme == "https":
-        return
-    host = (parsed.hostname or "").lower()
-    if host in _LOOPBACK_HOSTS:
-        return
-    raise ValueError(
-        f"refusing to send request to non-https, non-loopback URL: {url} "
-        "(set COMFY_CLOUD_BASE_URL to an https:// endpoint)"
-    )
-
-
 @dataclass
 class SubmitResult:
     prompt_id: str

comfy_cli/command/models/search.py

@@ -34,7 +34,7 @@
 import typer
 
 from comfy_cli import tracking
-from comfy_cli.http import authed_urlopen
+from comfy_cli.http import ResponseTooLarge
 from comfy_cli.output import get_renderer, rprint
 from comfy_cli.output.sanitize import sanitize_markup
 
@@ -143,16 +143,17 @@ def _http_get_json(url: str, target, timeout: float = 30.0) -> Any:
     """Issue an authenticated GET and decode JSON. Raises urllib/JSON errors verbatim.
 
     Response body is capped at ``_MAX_RESPONSE_BYTES`` to bound memory use on a
-    misbehaving server. A ``ValueError`` is raised if the cap is exceeded.
+    misbehaving server; exceeding it raises ``ResponseTooLarge``, which every
+    caller routes to an envelope error alongside the urllib/JSON families.
     """
-    with authed_urlopen(url, target, timeout=timeout) as resp:
-        # ``read(N)`` returns up to N bytes; reading N+1 lets us distinguish
-        # "fits exactly" from "exceeds cap" without buffering the whole stream
-        # twice on the happy path.
-        body = resp.read(_MAX_RESPONSE_BYTES + 1)
-        if len(body) > _MAX_RESPONSE_BYTES:
-            raise ValueError(f"response from {url} exceeds {_MAX_RESPONSE_BYTES} byte cap")
-        return json.loads(body)
+    from comfy_cli.http import request_json
+
+    _, body = request_json(url, target, timeout=timeout, max_bytes=_MAX_RESPONSE_BYTES)
+    if body is None:
+        # Callers route JSONDecodeError to an envelope error; an empty or
+        # unparseable body must surface the same way, not crash on body.get().
+        raise json.JSONDecodeError("empty or unparseable response body", "", 0)
+    return body
 
 
 def _emit_http_error(e: urllib.error.HTTPError, *, renderer, target, message: str, hint: str) -> NoReturn:
@@ -208,7 +209,7 @@ def list_folders_cmd(
             if target.is_cloud
             else "run `comfy launch` to start a local server",
         )
-    except (urllib.error.URLError, OSError, json.JSONDecodeError) as e:
+    except (urllib.error.URLError, OSError, json.JSONDecodeError, ResponseTooLarge) as e:
         renderer.error(
             code="server_not_running" if not target.is_cloud else "cloud_http_error",
             message=f"failed to fetch {url}: {e}",
@@ -301,7 +302,7 @@ def list_folder_cmd(
                 details={"status": e.code, "folder": folder},
             )
         raise typer.Exit(code=1) from e
-    except (urllib.error.URLError, OSError, json.JSONDecodeError) as e:
+    except (urllib.error.URLError, OSError, json.JSONDecodeError, ResponseTooLarge) as e:
         renderer.error(
             code="cloud_http_error" if target.is_cloud else "server_not_running",
             message=f"failed to fetch {url}: {e}",
@@ -425,6 +426,11 @@ def _cloud_search(
     qs = urllib.parse.urlencode(params)
     url = target.url("assets") + "?" + qs
     body = _http_get_json(url, target)
+    if not isinstance(body, dict):
+        # Callers route JSONDecodeError to an envelope error; a non-object
+        # top-level body (list/scalar) must surface the same way, not crash
+        # on body.get().
+        raise json.JSONDecodeError(f"unexpected response shape (not an object) from {url}", "", 0)
     assets = body.get("assets") or []
     rows = [_asset_to_row(a) for a in assets if isinstance(a, dict)]
     return rows, int(body.get("total") or len(rows))
@@ -591,7 +597,7 @@ def search_cmd(
             message=f"HTTP {e.code} during models search",
             hint="check auth (`comfy cloud whoami`) or network",
         )
-    except (urllib.error.URLError, OSError, json.JSONDecodeError) as e:
+    except (urllib.error.URLError, OSError, json.JSONDecodeError, ResponseTooLarge) as e:
         renderer.error(
             code="cloud_http_error" if target.is_cloud else "server_not_running",
             message=f"models search failed: {e}",
@@ -674,6 +680,8 @@ def show_cmd(
         url = target.url("assets") + "?" + qs
         try:
             body = _http_get_json(url, target)
+            if not isinstance(body, dict):
+                raise json.JSONDecodeError(f"unexpected response shape (not an object) from {url}", "", 0)
         except urllib.error.HTTPError as e:
             renderer.error(
                 code="cloud_http_error",
@@ -682,7 +690,7 @@ def show_cmd(
                 details={"status": e.code},
             )
             raise typer.Exit(code=1) from e
-        except (urllib.error.URLError, OSError, json.JSONDecodeError) as e:
+        except (urllib.error.URLError, OSError, json.JSONDecodeError, ResponseTooLarge) as e:
             renderer.error(code="cloud_http_error", message=f"models show failed: {e}")
             raise typer.Exit(code=1) from e
 

comfy_cli/command/workflow.py

@@ -28,6 +28,12 @@
 import typer
 
 from comfy_cli import tracking
+
+# Aliased at module scope rather than lazy-imported: a class used in ``except``
+# clauses at module scope cannot be resolved lazily. ``comfy_cli.http`` is
+# stdlib-only and tiny, and ``search.py``/``jobs.py`` already pull
+# ``urllib.request`` in at import time, so the precedent exists.
+from comfy_cli.http import ResponseTooLarge as _ResponseTooLarge
 from comfy_cli.output import get_renderer, rprint
 
 app = typer.Typer(no_args_is_help=True, help="Slot-based editing of frontend-format ComfyUI workflows.")
@@ -605,10 +611,6 @@ def notes_cmd(
 _HTTP_MAX_BYTES = 64 * 1024 * 1024
 
 
-class _ResponseTooLarge(Exception):
-    """A response exceeded the surface's byte cap — refuse to truncate."""
-
-
 # Per-operation guidance for an oversize cloud response. ``save``/``delete``
 # have already sent their request by the time the response is read, so the
 # server-side write may well have landed — say so rather than implying it did not.
@@ -815,7 +817,7 @@ def _userdata_file_url(target, key: str, query: dict | None = None) -> str:
 
 def _http_request(
     url: str, target, *, method: str = "GET", body: dict | None = None, timeout: float = 30.0
-) -> tuple[int, dict | None]:
+) -> tuple[int, dict | list | None]:
     """Authed HTTP call returning (status, parsed_json_or_none). Raises
     urllib errors verbatim so callers can surface the right error code, and
     ``_ResponseTooLarge`` when the body exceeds ``_HTTP_MAX_BYTES`` — an

comfy_cli/http.py

@@ -1,8 +1,31 @@
 """Shared HTTP helpers with an auth-leak-safe redirect policy."""
 
+import json
 import urllib.error
+import urllib.parse
 import urllib.request
 
+_LOOPBACK_HOSTS = {"localhost", "127.0.0.1", "::1", "[::1]"}
+
+
+def assert_safe_url(url: str) -> None:
+    """Reject plaintext HTTP for non-loopback hosts.
+
+    Anything carrying a credential (``X-API-Key`` / Bearer token) over the
+    wire must be HTTPS unless the host is a loopback address (where there's
+    no network to sniff).
+    """
+    parsed = urllib.parse.urlsplit(url)
+    if parsed.scheme == "https":
+        return
+    host = (parsed.hostname or "").lower()
+    if host in _LOOPBACK_HOSTS:
+        return
+    raise ValueError(
+        f"refusing to send request to non-https, non-loopback URL: {url} "
+        "(set COMFY_CLOUD_BASE_URL to an https:// endpoint)"
+    )
+
 
 class NoRedirectHandler(urllib.request.HTTPRedirectHandler):
     """Refuse to follow HTTP redirects.
@@ -180,3 +203,57 @@ def authed_urlopen(
     HTTPError instead of replaying credentials at the redirect target."""
     req = build_authed_request(url, target, method=method, data=data, content_type=content_type)
     return _AUTHED_OPENER.open(req, timeout=timeout)
+
+
+class ResponseTooLarge(Exception):
+    """A response exceeded the caller's byte cap — refuse to truncate."""
+
+
+def request_json(
+    url: str,
+    target,
+    *,
+    method: str = "GET",
+    body: dict | None = None,
+    timeout: float = 30.0,
+    max_bytes: int,
+) -> tuple[int, dict | list | None]:
+    """Authed HTTP call returning (status, parsed_json_or_none).
+
+    Raises urllib errors verbatim so callers can map them to envelope codes,
+    and ``ResponseTooLarge`` when the body exceeds ``max_bytes`` — an oversize
+    body must not masquerade as an unparseable one. An empty or unparseable
+    (bad JSON / non-UTF-8 / too-deeply-nested) body parses to ``None``;
+    ``UnicodeDecodeError`` is a ``ValueError`` but *not* a ``JSONDecodeError``,
+    so it needs naming here or it escapes uncaught.
+
+    ``max_bytes`` is keyword-required with no default so every caller keeps
+    owning its own cap constant. Auth headers never go out over the wire
+    without this: redirects are refused via the shared no-redirect opener (a
+    30x can't replay the credential at another host), and the URL itself
+    must be HTTPS or loopback before a credential is attached.
+    """
+    if max_bytes < 1:
+        raise ValueError(f"max_bytes must be >= 1, got {max_bytes}")
+    headers = target_auth_headers(target)
+    if headers:
+        assert_safe_url(url)
+    data = json.dumps(body).encode("utf-8") if body is not None else None
+    req = urllib.request.Request(url, data=data, method=method)
+    for k, v in headers.items():
+        req.add_header(k, v)
+    if data is not None:
+        req.add_header("Content-Type", "application/json")
+    with _AUTHED_OPENER.open(req, timeout=timeout) as resp:
+        status = resp.status
+        # Read one byte past the cap so a full body is distinguishable from a truncated one.
+        raw = resp.read(max_bytes + 1)
+    if len(raw) > max_bytes:
+        # Keep the message descriptive: search interpolates it into its envelope.
+        raise ResponseTooLarge(f"response from {url} exceeds {max_bytes} byte cap")
+    if not raw:
+        return status, None
+    try:
+        return status, json.loads(raw)
+    except (json.JSONDecodeError, UnicodeDecodeError, RecursionError):
+        return status, None

tests/comfy_cli/command/models/test_search.py

@@ -203,7 +203,9 @@ def _patch_urlopen(monkeypatch: pytest.MonkeyPatch, routes: dict[str, Any]):
     """Wire urlopen to a URL→body lookup. Body is JSON-encoded.
 
     Substring matching: the first registered URL substring that matches wins.
-    Unknown URLs raise so we never silently pass on a typo'd path.
+    Unknown URLs raise so we never silently pass on a typo'd path. A ``bytes``
+    payload is served verbatim, so a test can hand back a body that is not
+    valid JSON (or not valid UTF-8) at all.
     """
     calls = []
 
@@ -214,13 +216,14 @@ def _fake(req, timeout=None):
             if needle in url:
                 if isinstance(payload, Exception):
                     raise payload
-                body = json.dumps(payload).encode()
+                body = payload if isinstance(payload, bytes) else json.dumps(payload).encode()
                 return _fake_resp(body)
         raise AssertionError(f"unexpected URL hit by mock: {url}")
 
-    # ``_http_get_json`` now opens through the shared no-redirect opener in
-    # ``comfy_cli.http``; the fake still receives a ``Request`` object, so the
-    # route-matching above is unchanged.
+    # ``_http_get_json`` routes every request through the shared
+    # ``comfy_cli.http.request_json``, which opens via the module's
+    # ``_AUTHED_OPENER`` (built with NoRedirectHandler); the fake still
+    # receives a ``Request`` object, so the route-matching above is unchanged.
     import comfy_cli.http as http_mod
 
     monkeypatch.setattr(http_mod._AUTHED_OPENER, "open", _fake)
@@ -273,6 +276,82 @@ def test_local_http_error_uses_server_not_running(self, local_target, monkeypatc
         assert env["error"]["details"]["body"] == "boom"
 
 
+class TestMalformedResponseEnvelopes:
+    """Regression: an oversize or undecodable body must be an envelope, not a traceback.
+
+    Before the shared ``request_json`` migration, ``_http_get_json`` raised a
+    bare ``ValueError`` past the cap and let ``UnicodeDecodeError`` escape from
+    ``json.loads`` — neither is in the callers' ``(URLError, OSError,
+    JSONDecodeError)`` tuple, so both crashed the CLI with a traceback and no
+    ``--json`` envelope at all.
+    """
+
+    def test_oversize_response_yields_envelope_not_traceback(self, cloud_target, monkeypatch, capsys):
+        monkeypatch.setattr(search_cmd, "_MAX_RESPONSE_BYTES", 4)
+        _patch_urlopen(monkeypatch, {"/api/experiment/models": _CLOUD_FOLDERS})
+        env = _run(["list-folders", "--where", "cloud"], capsys)
+        assert env["ok"] is False
+        assert env["error"]["code"] == "cloud_http_error"
+        # The helper's message is interpolated into the envelope, so it stays informative.
+        assert "byte cap" in env["error"]["message"]
+
+    def test_oversize_response_local_uses_server_not_running(self, local_target, monkeypatch, capsys):
+        monkeypatch.setattr(search_cmd, "_MAX_RESPONSE_BYTES", 4)
+        _patch_urlopen(monkeypatch, {"127.0.0.1:8188/models": _LOCAL_FOLDERS})
+        env = _run(["list-folders", "--where", "local"], capsys)
+        assert env["ok"] is False
+        assert env["error"]["code"] == "server_not_running"
+
+    def test_non_utf8_response_yields_envelope_not_traceback(self, cloud_target, monkeypatch, capsys):
+        _patch_urlopen(monkeypatch, {"/api/experiment/models": b"\xff\xfe\x00not json"})
+        env = _run(["list-folders", "--where", "cloud"], capsys)
+        assert env["ok"] is False
+        assert env["error"]["code"] == "cloud_http_error"
+
+    def test_unparseable_response_yields_envelope_not_traceback(self, cloud_target, monkeypatch, capsys):
+        _patch_urlopen(monkeypatch, {"/api/experiment/models": b"<html>not json</html>"})
+        env = _run(["list-folders", "--where", "cloud"], capsys)
+        assert env["ok"] is False
+        assert env["error"]["code"] == "cloud_http_error"
+
+    def test_literal_null_body_yields_envelope(self, cloud_target, monkeypatch, capsys):
+        # The one intentional behavior change of the shared-helper migration:
+        # ``request_json`` cannot distinguish a literal JSON ``null`` from an
+        # unparseable body, so both become None and _http_get_json raises. Before,
+        # `list-folders` reported ok:true with count 0 for a `null` body — which
+        # silently presented a malformed response as "this backend has no folders".
+        _patch_urlopen(monkeypatch, {"/api/experiment/models": b"null"})
+        env = _run(["list-folders", "--where", "cloud"], capsys)
+        assert env["ok"] is False
+        assert env["error"]["code"] == "cloud_http_error"
+
+    def test_empty_response_yields_envelope_not_attribute_error(self, cloud_target, monkeypatch, capsys):
+        # `models show` calls body.get() on the parsed result; an empty body used
+        # to reach that as None. It now surfaces as a decode-family envelope.
+        _patch_urlopen(monkeypatch, {"/api/assets": b""})
+        env = _run(["show", "flux1-dev", "--where", "cloud"], capsys)
+        assert env["ok"] is False
+        assert env["error"]["code"] == "cloud_http_error"
+
+    @pytest.mark.parametrize(
+        ("args", "route"),
+        [
+            (["list-folders", "--where", "cloud"], "/api/experiment/models"),
+            (["list-folder", "loras", "--where", "cloud"], "/api/experiment/models/loras"),
+            (["search", "--where", "cloud"], "/api/assets"),
+            (["show", "flux1-dev", "--where", "cloud"], "/api/assets"),
+        ],
+    )
+    def test_every_call_site_routes_oversize_to_envelope(self, args, route, cloud_target, monkeypatch, capsys):
+        # Each of the four handlers wrapping _http_get_json must catch
+        # ResponseTooLarge; an uncaught one would be a traceback, not an envelope.
+        monkeypatch.setattr(search_cmd, "_MAX_RESPONSE_BYTES", 4)
+        _patch_urlopen(monkeypatch, {route: {"assets": [], "total": 0}})
+        env = _run(args, capsys)
+        assert env["ok"] is False, env
+        assert env["error"]["code"] == "cloud_http_error"
+
+
 # ---------------------------------------------------------------------------
 # list-folder
 # ---------------------------------------------------------------------------