⚠️ Security / auth-adjacent — investigate the SAFE scope; do NOT auto-implement a broad refactor.
Filed by the groom sweep — Comfy-Org/comfy-cli (whole-repo) · verdict CONFIRM · run.
Verified
comfy_cli/http.py centralizes the openers, the no-redirect auth-leak policy (NoRedirectHandler) and target_auth_headers, and stops at the response object (plain_urlopen:126, no_redirect_urlopen:135, authed_urlopen:170). Every caller then re-invents read, maybe-cap, parse, and decide what a non-empty non-JSON 200 means.
Capped, each with its own exception type and wording:
- command/workflow.py:719 _userdata_request -- reads the cap plus one byte, private _ResponseTooLarge.
- command/workflow.py:816 _http_request -- the same cap-plus-one trick, _ResponseTooLarge and _ResponseUnparseable.
- command/models/search.py:141 _http_get_json -- cap into a bare ValueError.
- cql/loader.py:96 -- MAX_INPUT_BYTES plus one into CQLRuntimeError (bounded read so a misbehaving server cannot OOM us).
Unbounded, all three confirmed:
- command/jobs.py:84 _http_get_json -- json.loads(resp.read()).
- command/templates.py:66 _fetch_gallery -- resp.read() on a remote gallery URL over the network.
- comfy_client.py:269 -- resp.read().decode(utf-8, errors=replace).
Verdict -- confirm, with the scope stated precisely
Add beside the openers in http.py: read_json(resp, max_bytes) and a read_bytes twin, raising ResponseTooLarge and ResponseUnparseable defined in http.py, implementing the read-one-past-the-cap truncation detection and the explicit-UTF-8-decode plus ValueError/RecursionError handling workflow.py already got right. max_bytes must be a REQUIRED per-call argument with no module-level default, so no existing limit changes silently and no new call site can omit a cap by accident.
Adopt immediately at the three unbounded sites (jobs.py:84, templates.py:66, comfy_client.py:269), each with a cap chosen for its surface and each mapping the two new exceptions onto the envelope code it already uses for a bad body.
Migrate the four capped sites only where the mapping is provably preserved: cql/loader.py must keep raising CQLRuntimeError with its details, and workflow.py's two private exceptions feed per-operation hint tables (_LOCAL_TOO_LARGE_HINTS, _TOO_LARGE_HINTS), so either alias those names to the new ones or leave those readers alone. The retrofit is not required for the value.
Steelman
Caps and error taxonomies legitimately differ per surface, so the helper saves a handful of lines per site and can read as ceremony over the stdlib.
Counter: three sites having no cap at all -- one reading a remote URL over the network -- is exactly what each-site-decides produces, and http.py is already the module that owns how we talk to servers safely.
Risk
Low and purely additive, but security-adjacent: it is hardening against hostile or misbehaving server input, and it lands in the module that owns the auth-leak redirect policy. File as an investigation; review that no existing cap value or error code changes.
Filed by the groom sweep — Comfy-Org/comfy-cli (
whole-repo) · verdict CONFIRM · run.Verified
comfy_cli/http.py centralizes the openers, the no-redirect auth-leak policy (NoRedirectHandler) and target_auth_headers, and stops at the response object (plain_urlopen:126, no_redirect_urlopen:135, authed_urlopen:170). Every caller then re-invents read, maybe-cap, parse, and decide what a non-empty non-JSON 200 means.
Capped, each with its own exception type and wording:
Unbounded, all three confirmed:
Verdict -- confirm, with the scope stated precisely
Add beside the openers in http.py: read_json(resp, max_bytes) and a read_bytes twin, raising ResponseTooLarge and ResponseUnparseable defined in http.py, implementing the read-one-past-the-cap truncation detection and the explicit-UTF-8-decode plus ValueError/RecursionError handling workflow.py already got right. max_bytes must be a REQUIRED per-call argument with no module-level default, so no existing limit changes silently and no new call site can omit a cap by accident.
Adopt immediately at the three unbounded sites (jobs.py:84, templates.py:66, comfy_client.py:269), each with a cap chosen for its surface and each mapping the two new exceptions onto the envelope code it already uses for a bad body.
Migrate the four capped sites only where the mapping is provably preserved: cql/loader.py must keep raising CQLRuntimeError with its details, and workflow.py's two private exceptions feed per-operation hint tables (_LOCAL_TOO_LARGE_HINTS, _TOO_LARGE_HINTS), so either alias those names to the new ones or leave those readers alone. The retrofit is not required for the value.
Steelman
Caps and error taxonomies legitimately differ per surface, so the helper saves a handful of lines per site and can read as ceremony over the stdlib.
Counter: three sites having no cap at all -- one reading a remote URL over the network -- is exactly what each-site-decides produces, and http.py is already the module that owns how we talk to servers safely.
Risk
Low and purely additive, but security-adjacent: it is hardening against hostile or misbehaving server input, and it lands in the module that owns the auth-leak redirect policy. File as an investigation; review that no existing cap value or error code changes.