From e2298b25a47dcb5b6c4d24fbf2c7fb287d217ad9 Mon Sep 17 00:00:00 2001
From: Matt Miller <mattmiller@comfy.org>
Date: Tue, 28 Jul 2026 18:39:00 -0700
Subject: [PATCH 1/2] refactor: converge capped-read JSON helpers onto shared
 http.request_json
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Two near-identical capped-read JSON helpers had drifted apart:
workflow._http_request and models.search._http_get_json. Converge them onto
one shared comfy_cli.http.request_json, preserving each caller's exception
surface exactly.

This also fixes two latent search.py crashes. An oversize response raised a
bare ValueError and a non-UTF-8 body let UnicodeDecodeError escape json.loads
— neither is in the callers' (URLError, OSError, JSONDecodeError) tuple, so
both took down the CLI with a traceback instead of emitting a --json
envelope. Both now route through ResponseTooLarge / JSONDecodeError to the
existing cloud_http_error / server_not_running envelope path.

- http.py: add ResponseTooLarge + request_json (keyword-required max_bytes so
  each caller keeps owning its own cap constant).
- workflow.py: _http_request becomes a thin wrapper (name, signature, and
  call-time _HTTP_MAX_BYTES read preserved); _ResponseTooLarge becomes a
  top-level alias of the shared class so all ten catch sites keep working.
- search.py: _http_get_json wraps request_json and raises JSONDecodeError for
  an empty/unparseable body; ResponseTooLarge added to all four except-tuples.
---
 comfy_cli/command/models/search.py            |  39 ++---
 comfy_cli/command/workflow.py                 |  38 ++---
 comfy_cli/http.py                             |  49 ++++++
 tests/comfy_cli/command/models/test_search.py |  82 +++++++++-
 tests/comfy_cli/test_http.py                  | 151 +++++++++++++++++-
 5 files changed, 308 insertions(+), 51 deletions(-)

diff --git a/comfy_cli/command/models/search.py b/comfy_cli/command/models/search.py
index 0afdaf9b..e3257c2f 100644
--- a/comfy_cli/command/models/search.py
+++ b/comfy_cli/command/models/search.py
@@ -28,12 +28,12 @@
 import re
 import urllib.error
 import urllib.parse
-import urllib.request
 from typing import Annotated, Any, NoReturn
 
 import typer
 
 from comfy_cli import tracking
+from comfy_cli.http import ResponseTooLarge
 from comfy_cli.output import get_renderer, rprint
 
 app = typer.Typer(no_args_is_help=True, help="Discover models — folders, files, and the cloud asset catalog.")
@@ -98,30 +98,21 @@ def _models_path_parts(target) -> tuple[str, ...]:
     return ("experiment", "models") if target.is_cloud else ("models",)
 
 
-def _authed_request(url: str, target) -> urllib.request.Request:
-    from comfy_cli.http import target_auth_headers
-
-    req = urllib.request.Request(url)
-    for k, v in target_auth_headers(target).items():
-        req.add_header(k, v)
-    return req
-
-
 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.
     """
-    req = _authed_request(url, target)
-    with urllib.request.urlopen(req, 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:
@@ -177,7 +168,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}",
@@ -262,7 +253,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}",
@@ -485,7 +476,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}",
@@ -573,7 +564,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
 
diff --git a/comfy_cli/command/workflow.py b/comfy_cli/command/workflow.py
index 6727f98b..abaadd87 100644
--- a/comfy_cli/command/workflow.py
+++ b/comfy_cli/command/workflow.py
@@ -23,6 +23,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.")
@@ -438,10 +444,6 @@ def vary_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.
@@ -619,30 +621,18 @@ def _authed_request(
 
 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
-    oversize body must not masquerade as an unparseable one."""
-    import urllib.request
+    oversize body must not masquerade as an unparseable one.
 
-    data = json.dumps(body).encode("utf-8") if body is not None else None
-    ct = "application/json" if data is not None else None
-    req = _authed_request(url, target, method=method, data=data, content_type=ct)
-    with urllib.request.urlopen(req, timeout=timeout) as resp:
-        status = resp.status
-        # Read one byte past the cap so we can tell a full body from a truncated one.
-        raw = resp.read(_HTTP_MAX_BYTES + 1)
-    if len(raw) > _HTTP_MAX_BYTES:
-        raise _ResponseTooLarge()
-    if not raw:
-        return status, None
-    try:
-        return status, json.loads(raw)
-    except (json.JSONDecodeError, UnicodeDecodeError):
-        # UnicodeDecodeError is a ValueError but *not* a JSONDecodeError, so a
-        # body that isn't valid UTF-8 needs naming here or it escapes uncaught.
-        return status, None
+    Thin wrapper over the shared ``comfy_cli.http.request_json``; kept as a
+    named function so call sites (and tests) keep a stable entry point, and so
+    ``_HTTP_MAX_BYTES`` is read from the module global at call time."""
+    from comfy_cli.http import request_json
+
+    return request_json(url, target, method=method, body=body, timeout=timeout, max_bytes=_HTTP_MAX_BYTES)
 
 
 def _handle_cloud_http_error(renderer, e, *, operation: str, workflow_id: str | None = None) -> typer.Exit:
diff --git a/comfy_cli/http.py b/comfy_cli/http.py
index 0f0f4a21..8fb34ae2 100644
--- a/comfy_cli/http.py
+++ b/comfy_cli/http.py
@@ -1,5 +1,6 @@
 """Shared HTTP helpers with an auth-leak-safe redirect policy."""
 
+import json
 import urllib.error
 import urllib.request
 
@@ -42,3 +43,51 @@ def target_auth_headers(target) -> dict[str, str]:
         elif target.auth_token:
             headers["Authorization"] = f"Bearer {target.auth_token}"
     return headers
+
+
+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) 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. Redirects follow the default opener, matching
+    both helpers this replaced — attaching ``NoRedirectHandler`` here would be a
+    behavior change for those call sites.
+    """
+    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 target_auth_headers(target).items():
+        req.add_header(k, v)
+    if data is not None:
+        req.add_header("Content-Type", "application/json")
+    with urllib.request.urlopen(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):
+        return status, None
diff --git a/tests/comfy_cli/command/models/test_search.py b/tests/comfy_cli/command/models/test_search.py
index 8316896b..19c6f7ff 100644
--- a/tests/comfy_cli/command/models/test_search.py
+++ b/tests/comfy_cli/command/models/test_search.py
@@ -175,7 +175,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 = []
 
@@ -186,7 +188,7 @@ 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}")
 
@@ -240,6 +242,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
 # ---------------------------------------------------------------------------
diff --git a/tests/comfy_cli/test_http.py b/tests/comfy_cli/test_http.py
index 43fa04ad..b0de8e08 100644
--- a/tests/comfy_cli/test_http.py
+++ b/tests/comfy_cli/test_http.py
@@ -1,10 +1,11 @@
 import http.client
+import json
 import urllib.error
 import urllib.request
 
 import pytest
 
-from comfy_cli.http import NoRedirectHandler, target_auth_headers
+from comfy_cli.http import NoRedirectHandler, ResponseTooLarge, request_json, target_auth_headers
 from comfy_cli.target import Target
 
 
@@ -71,3 +72,151 @@ def test_target_auth_headers_cloud_auth_token_only():
 def test_target_auth_headers_cloud_both_api_key_wins():
     target = Target(kind="cloud", base_url="https://cloud.example", auth_token="t", api_key="k")
     assert target_auth_headers(target) == {"X-API-Key": "k"}
+
+
+# ---------------------------------------------------------------------------
+# request_json — the shared capped-read JSON helper
+# ---------------------------------------------------------------------------
+
+
+def _fake_resp(body: bytes, status: int = 200):
+    """Minimal urlopen-compatible response. ``read(n)`` truncates like the real one."""
+
+    class _Resp:
+        def __init__(self):
+            self.status = status
+
+        def __enter__(self):
+            return self
+
+        def __exit__(self, *args):
+            return False
+
+        def read(self, n: int | None = None):
+            return body if n is None else body[:n]
+
+    return _Resp()
+
+
+def _patch_urlopen(monkeypatch: pytest.MonkeyPatch, payload, status: int = 200):
+    """Route every urlopen call to ``payload`` (bytes) and record the Requests seen."""
+    seen: list[urllib.request.Request] = []
+
+    def _fake(req, timeout=None):
+        seen.append(req)
+        if isinstance(payload, Exception):
+            raise payload
+        return _fake_resp(payload, status)
+
+    monkeypatch.setattr("urllib.request.urlopen", _fake)
+    return seen
+
+
+@pytest.fixture
+def cloud_target():
+    return Target(kind="cloud", base_url="https://cloud.example", path_prefix="/api", api_key="test-api-key")
+
+
+@pytest.fixture
+def local_target():
+    # Stray credentials on purpose: a local target must never emit them.
+    return Target(kind="local", base_url="http://127.0.0.1:8188", path_prefix="", api_key="stray", auth_token="stray")
+
+
+def test_request_json_oversize_raises_response_too_large(monkeypatch, cloud_target):
+    # An oversize body must NOT masquerade as an unparseable one (which would
+    # silently degrade to ``None`` and look like an empty response).
+    _patch_urlopen(monkeypatch, b'{"data": []}')
+    with pytest.raises(ResponseTooLarge) as exc_info:
+        request_json("https://cloud.example/api/thing", cloud_target, max_bytes=4)
+    # The message is interpolated into search's envelope, so it must stay descriptive.
+    msg = str(exc_info.value)
+    assert "https://cloud.example/api/thing" in msg
+    assert "4" in msg
+
+
+def test_request_json_body_exactly_at_cap_still_parses(monkeypatch, cloud_target):
+    # Boundary: len(raw) == cap is a *complete* body, not a truncated one.
+    body = b'{"a": 1}'
+    _patch_urlopen(monkeypatch, body)
+    assert request_json("https://cloud.example/api/thing", cloud_target, max_bytes=len(body)) == (200, {"a": 1})
+
+
+def test_request_json_empty_body_returns_none(monkeypatch, cloud_target):
+    _patch_urlopen(monkeypatch, b"", status=204)
+    assert request_json("https://cloud.example/api/thing", cloud_target, max_bytes=1024) == (204, None)
+
+
+def test_request_json_non_utf8_body_returns_none(monkeypatch, cloud_target):
+    # UnicodeDecodeError is a ValueError but *not* a JSONDecodeError; if it were
+    # not named in the except clause it would escape request_json uncaught.
+    _patch_urlopen(monkeypatch, b"\xff\xfe\x00not json")
+    assert request_json("https://cloud.example/api/thing", cloud_target, max_bytes=1024) == (200, None)
+
+
+def test_request_json_unparseable_body_returns_none(monkeypatch, cloud_target):
+    _patch_urlopen(monkeypatch, b"<html>not json</html>")
+    assert request_json("https://cloud.example/api/thing", cloud_target, max_bytes=1024) == (200, None)
+
+
+def test_request_json_list_body_parses(monkeypatch, cloud_target):
+    # Both model-listing endpoints return top-level JSON arrays.
+    _patch_urlopen(monkeypatch, b'["checkpoints", "loras"]')
+    assert request_json("https://cloud.example/api/thing", cloud_target, max_bytes=1024) == (
+        200,
+        ["checkpoints", "loras"],
+    )
+
+
+def test_request_json_get_sends_no_body_and_no_content_type(monkeypatch, cloud_target):
+    seen = _patch_urlopen(monkeypatch, b"{}")
+    request_json("https://cloud.example/api/thing", cloud_target, max_bytes=1024)
+    req = seen[0]
+    assert req.get_method() == "GET"
+    assert req.data is None
+    assert req.get_header("Content-type") is None
+
+
+def test_request_json_post_attaches_json_body_and_content_type(monkeypatch, cloud_target):
+    seen = _patch_urlopen(monkeypatch, b"{}", status=201)
+    status, _ = request_json(
+        "https://cloud.example/api/thing", cloud_target, method="POST", body={"name": "wf"}, max_bytes=1024
+    )
+    assert status == 201
+    req = seen[0]
+    assert req.get_method() == "POST"
+    assert json.loads(req.data) == {"name": "wf"}
+    assert req.get_header("Content-type") == "application/json"
+
+
+def test_request_json_attaches_auth_headers_for_cloud(monkeypatch, cloud_target):
+    seen = _patch_urlopen(monkeypatch, b"{}")
+    request_json("https://cloud.example/api/thing", cloud_target, max_bytes=1024)
+    # urllib title-cases header names on the Request object.
+    assert seen[0].get_header("X-api-key") == "test-api-key"
+
+
+def test_request_json_attaches_no_auth_headers_for_local(monkeypatch, local_target):
+    # Same defense-in-depth gate as target_auth_headers: a local target never
+    # gets a credential, so a stray token can't leak to a plaintext server.
+    seen = _patch_urlopen(monkeypatch, b"{}")
+    request_json("http://127.0.0.1:8188/models", local_target, max_bytes=1024)
+    req = seen[0]
+    # Assert on the whole header bag, not two named keys: a named-key check
+    # would pass vacuously if urllib ever changed how it cases header names.
+    assert req.headers == {}
+
+
+def test_request_json_raises_urllib_errors_verbatim(monkeypatch, cloud_target):
+    # Callers map these to envelope codes themselves, so they must not be swallowed.
+    err = urllib.error.HTTPError("https://cloud.example/api/thing", 503, "boom", http.client.HTTPMessage(), None)
+    _patch_urlopen(monkeypatch, err)
+    with pytest.raises(urllib.error.HTTPError) as exc_info:
+        request_json("https://cloud.example/api/thing", cloud_target, max_bytes=1024)
+    assert exc_info.value.code == 503
+
+
+def test_request_json_max_bytes_is_keyword_required(cloud_target):
+    # No default: each caller keeps owning its own cap constant.
+    with pytest.raises(TypeError):
+        request_json("https://cloud.example/api/thing", cloud_target)

From 3778e043e538d0c1c2afbb43b6d725b68dfba968 Mon Sep 17 00:00:00 2001
From: Matt Miller <mattmiller@comfy.org>
Date: Tue, 28 Jul 2026 22:49:53 -0700
Subject: [PATCH 2/2] fix(http): refuse redirects + enforce https/loopback on
 request_json auth headers (BE-4363)

Addresses cursor-review findings on PR #623: request_json now opens
through the shared NoRedirectHandler-backed opener (matching every
other authenticated call site) instead of the default redirect-following
opener, and gates on assert_safe_url before attaching auth headers
(moved from comfy_client into http.py so both share it). Also guards
non-dict response bodies at the models/search.py and workflow.py call
sites that were indexing with .get() unconditionally, catches
RecursionError alongside the other unparseable-body cases, and
validates max_bytes >= 1.

Also fixes an unrelated pre-existing CI failure: two independent PRs
each registered an ErrorCode("server_died", ...) entry, tripping the
no-duplicate-codes registry test; merged into one entry covering both
call sites (both already emit the same "server_died" code at runtime).

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
---
 comfy_cli/comfy_client.py                     | 21 +------
 comfy_cli/command/models/search.py            |  7 +++
 comfy_cli/command/workflow.py                 |  4 +-
 comfy_cli/error_codes.py                      | 13 ++--
 comfy_cli/http.py                             | 49 ++++++++++++---
 tests/comfy_cli/command/models/test_search.py |  5 +-
 .../comfy_cli/command/test_workflow_saved.py  |  6 ++
 tests/comfy_cli/test_http.py                  | 63 ++++++++++++++++++-
 8 files changed, 127 insertions(+), 41 deletions(-)

diff --git a/comfy_cli/comfy_client.py b/comfy_cli/comfy_client.py
index 02b8c8eb..28683726 100644
--- a/comfy_cli/comfy_client.py
+++ b/comfy_cli/comfy_client.py
@@ -24,10 +24,9 @@
 from typing import Any
 
 from comfy_cli.http import NoRedirectHandler
+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 = urllib.request.build_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
diff --git a/comfy_cli/command/models/search.py b/comfy_cli/command/models/search.py
index e3257c2f..cdb7f6a3 100644
--- a/comfy_cli/command/models/search.py
+++ b/comfy_cli/command/models/search.py
@@ -377,6 +377,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))
@@ -556,6 +561,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",
diff --git a/comfy_cli/command/workflow.py b/comfy_cli/command/workflow.py
index abaadd87..4e893e2e 100644
--- a/comfy_cli/command/workflow.py
+++ b/comfy_cli/command/workflow.py
@@ -956,7 +956,9 @@ def list_cmd(
 
     try:
         _, body = _http_request(url, target)
-    except (urllib.error.HTTPError, urllib.error.URLError, OSError, _ResponseTooLarge) as e:
+        if body is not None and not isinstance(body, dict):
+            raise json.JSONDecodeError(f"unexpected response shape (not an object) from {url}", "", 0)
+    except (urllib.error.HTTPError, urllib.error.URLError, OSError, _ResponseTooLarge, json.JSONDecodeError) as e:
         raise _handle_cloud_http_error(renderer, e, operation="list") from e
 
     rows = (body or {}).get("data") or []
diff --git a/comfy_cli/error_codes.py b/comfy_cli/error_codes.py
index d92e2008..146e3f75 100644
--- a/comfy_cli/error_codes.py
+++ b/comfy_cli/error_codes.py
@@ -364,20 +364,17 @@ class ErrorCode:
     ),
     ErrorCode(
         "server_died",
-        "Server connection dropped while a foreground (`--wait`) job was in flight; recorded on the job state file.",
-        "check the server (it may have been OOM-killed); the prompt_id is in `comfy jobs status <id>`",
+        "The local ComfyUI server became unreachable while a job was in flight — either a "
+        "foreground (`--wait`) connection dropped or the background watcher's probes failed — "
+        "the server likely crashed, restarted, or was OOM-killed; recorded on the job state file.",
+        "check the ComfyUI server log (it may have been OOM-killed), then `comfy launch` and "
+        "re-submit the workflow — the prompt_id is in `comfy jobs status <id>`",
     ),
     ErrorCode(
         "watcher_poll_error",
         "Background watcher encountered a transient error polling the server.",
         "transient — the job is likely still running; re-run `comfy jobs watch <id>`",
     ),
-    ErrorCode(
-        "server_died",
-        "The local ComfyUI server became unreachable (or restarted without the job) while it "
-        "was in flight — the server likely crashed or was killed (e.g. an out-of-memory allocation).",
-        "check the ComfyUI server log, then `comfy launch` and re-submit the workflow",
-    ),
     ErrorCode(
         "unknown_status_stall",
         "Cloud reported a status the CLI does not recognize and it did not change within the stall window.",
diff --git a/comfy_cli/http.py b/comfy_cli/http.py
index 8fb34ae2..4960de8c 100644
--- a/comfy_cli/http.py
+++ b/comfy_cli/http.py
@@ -2,8 +2,30 @@
 
 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.
@@ -49,6 +71,9 @@ class ResponseTooLarge(Exception):
     """A response exceeded the caller's byte cap — refuse to truncate."""
 
 
+_OPENER = urllib.request.build_opener(NoRedirectHandler())
+
+
 def request_json(
     url: str,
     target,
@@ -63,22 +88,28 @@ def request_json(
     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) body parses to ``None``; ``UnicodeDecodeError`` is a
-    ``ValueError`` but *not* a ``JSONDecodeError``, so it needs naming here or
-    it escapes uncaught.
+    (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. Redirects follow the default opener, matching
-    both helpers this replaced — attaching ``NoRedirectHandler`` here would be a
-    behavior change for those call sites.
+    owning its own cap constant. Auth headers never go out over the wire
+    without this: redirects are refused via ``NoRedirectHandler`` (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 target_auth_headers(target).items():
+    for k, v in headers.items():
         req.add_header(k, v)
     if data is not None:
         req.add_header("Content-Type", "application/json")
-    with urllib.request.urlopen(req, timeout=timeout) as resp:
+    with _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)
@@ -89,5 +120,5 @@ def request_json(
         return status, None
     try:
         return status, json.loads(raw)
-    except (json.JSONDecodeError, UnicodeDecodeError):
+    except (json.JSONDecodeError, UnicodeDecodeError, RecursionError):
         return status, None
diff --git a/tests/comfy_cli/command/models/test_search.py b/tests/comfy_cli/command/models/test_search.py
index 19c6f7ff..3b65f71c 100644
--- a/tests/comfy_cli/command/models/test_search.py
+++ b/tests/comfy_cli/command/models/test_search.py
@@ -192,7 +192,10 @@ def _fake(req, timeout=None):
                 return _fake_resp(body)
         raise AssertionError(f"unexpected URL hit by mock: {url}")
 
-    monkeypatch.setattr("urllib.request.urlopen", _fake)
+    # models/search.py routes every request through the shared
+    # comfy_cli.http.request_json, which opens via the module's ``_OPENER``
+    # (built with NoRedirectHandler), not the bare urlopen function.
+    monkeypatch.setattr("comfy_cli.http._OPENER.open", _fake)
     return calls
 
 
diff --git a/tests/comfy_cli/command/test_workflow_saved.py b/tests/comfy_cli/command/test_workflow_saved.py
index 2d71cef9..1fee5ad6 100644
--- a/tests/comfy_cli/command/test_workflow_saved.py
+++ b/tests/comfy_cli/command/test_workflow_saved.py
@@ -125,7 +125,13 @@ def _fake(req, timeout=None):
                 return _fake_resp(json.dumps(payload).encode())
         raise AssertionError(f"unexpected URL: {url}")
 
+    # Local (``--where local``) saved-workflow verbs go through
+    # ``_userdata_request``, which still calls the bare ``urllib.request.urlopen``.
+    # Cloud verbs go through the shared ``comfy_cli.http.request_json``, which
+    # opens via its own ``_OPENER`` (built with NoRedirectHandler). Patch both
+    # so either path is intercepted regardless of which one a given test hits.
     monkeypatch.setattr("urllib.request.urlopen", _fake)
+    monkeypatch.setattr("comfy_cli.http._OPENER.open", _fake)
     return calls
 
 
diff --git a/tests/comfy_cli/test_http.py b/tests/comfy_cli/test_http.py
index b0de8e08..3a4208a9 100644
--- a/tests/comfy_cli/test_http.py
+++ b/tests/comfy_cli/test_http.py
@@ -5,6 +5,7 @@
 
 import pytest
 
+from comfy_cli import http as comfy_http
 from comfy_cli.http import NoRedirectHandler, ResponseTooLarge, request_json, target_auth_headers
 from comfy_cli.target import Target
 
@@ -99,7 +100,12 @@ def read(self, n: int | None = None):
 
 
 def _patch_urlopen(monkeypatch: pytest.MonkeyPatch, payload, status: int = 200):
-    """Route every urlopen call to ``payload`` (bytes) and record the Requests seen."""
+    """Route every ``request_json`` call to ``payload`` (bytes) and record the Requests seen.
+
+    ``request_json`` opens through the shared ``_OPENER`` (built with
+    ``NoRedirectHandler``), not the bare ``urllib.request.urlopen`` function,
+    so the fake must patch the opener's ``open`` method.
+    """
     seen: list[urllib.request.Request] = []
 
     def _fake(req, timeout=None):
@@ -108,7 +114,7 @@ def _fake(req, timeout=None):
             raise payload
         return _fake_resp(payload, status)
 
-    monkeypatch.setattr("urllib.request.urlopen", _fake)
+    monkeypatch.setattr(comfy_http._OPENER, "open", _fake)
     return seen
 
 
@@ -220,3 +226,56 @@ def test_request_json_max_bytes_is_keyword_required(cloud_target):
     # No default: each caller keeps owning its own cap constant.
     with pytest.raises(TypeError):
         request_json("https://cloud.example/api/thing", cloud_target)
+
+
+@pytest.mark.parametrize("max_bytes", [0, -1])
+def test_request_json_rejects_non_positive_max_bytes(cloud_target, max_bytes):
+    with pytest.raises(ValueError, match="max_bytes"):
+        request_json("https://cloud.example/api/thing", cloud_target, max_bytes=max_bytes)
+
+
+def test_request_json_opens_via_shared_opener_with_no_redirect_handler():
+    # A 30x with an authenticated request in flight must not be followed —
+    # NoRedirectHandler on the shared opener is what refuses it. Assert the
+    # opener request_json actually uses carries that handler, so a future
+    # revert to the bare default opener (which follows redirects and copies
+    # headers onto the new request) is caught here rather than in production.
+    assert any(isinstance(h, NoRedirectHandler) for h in comfy_http._OPENER.handlers)
+
+
+def test_request_json_refuses_plaintext_http_for_cloud_target(monkeypatch, cloud_target):
+    # A credential must never go out over cleartext HTTP to a non-loopback
+    # host — even if some misconfiguration points COMFY_CLOUD_BASE_URL at
+    # http://. No urlopen call should happen at all in this case.
+    seen = _patch_urlopen(monkeypatch, b"{}")
+    with pytest.raises(ValueError, match="non-https"):
+        request_json("http://cloud.example/api/thing", cloud_target, max_bytes=1024)
+    assert seen == []
+
+
+def test_request_json_allows_plaintext_http_for_loopback(monkeypatch, local_target):
+    # Loopback has no network to sniff, so plaintext is fine — this is the
+    # normal case for local ComfyUI.
+    _patch_urlopen(monkeypatch, b"{}")
+    assert request_json("http://127.0.0.1:8188/models", local_target, max_bytes=1024) == (200, {})
+
+
+def test_request_json_no_auth_headers_skips_https_gate(monkeypatch):
+    # A cloud target with no credentials at all (e.g. logged out) attaches no
+    # headers, so there's nothing to protect — the https/loopback gate must
+    # not block that request.
+    target = Target(kind="cloud", base_url="http://cloud.example", path_prefix="/api")
+    _patch_urlopen(monkeypatch, b"{}")
+    assert request_json("http://cloud.example/api/thing", target, max_bytes=1024) == (200, {})
+
+
+def test_request_json_recursion_error_returns_none(monkeypatch, cloud_target):
+    # A pathologically nested body can exhaust the interpreter stack in
+    # json.loads; RecursionError is neither JSONDecodeError nor
+    # UnicodeDecodeError, so it needs naming here or it escapes uncaught like
+    # the other two. Forced directly (rather than via real deep nesting,
+    # which CPython's C-accelerated decoder tolerates to very large depths)
+    # so this test stays fast and deterministic.
+    _patch_urlopen(monkeypatch, b'{"a": 1}')
+    monkeypatch.setattr(comfy_http.json, "loads", lambda *a, **kw: (_ for _ in ()).throw(RecursionError()))
+    assert request_json("https://cloud.example/api/thing", cloud_target, max_bytes=1024) == (200, None)