fix(output): strip ANSI/control sequences from server-supplied text at the pretty renderer boundary (BE-4794)

[mattmillerai]

ELI-5

When something goes wrong, the CLI prints the error message it got back from the ComfyUI server. If that server is a stranger's machine (comfy run --host/--port can point anywhere), it gets to choose the text — and terminals treat some text as commands, not characters. A message like job <ESC>[2Jevil running doesn't say "clear my screen", it clears your screen. It can also rewrite your window title, or repaint lines that already scrolled past so the CLI appears to have said something it never said.

This PR adds one small function that removes those invisible command characters, and calls it in the one place where human-facing text becomes terminal output. Machine-readable JSON output is untouched — it already escapes them safely, and stripping there would corrupt data that agents parse.

What changed

• New comfy_cli/output/sanitize.py — strips C0/C1 control characters and ANSI escape sequences (CSI, OSC, DCS, and the shorter two- and three-character forms). Tab and newline survive (legitimate layout in multi-line messages); carriage return does not (it lets a message repaint an already-printed line). Rich markup is deliberately left alone — call sites use [yellow]…[/yellow] for styling and escape untrusted text with rich.markup.escape.
• error_panel (the only pretty path Renderer.error takes) now sanitizes code, message, hint, and the stringified details keys and values.
• Renderer.warn / info / success sanitize their message and hint.
• The JSON/NDJSON envelope paths are untouched. json.dumps already encodes ESC as a \u escape, so machine output was never exposed, and sanitizing there would silently mutate the payload agents read.

Why here and not at the call sites: the pattern is repo-wide (every renderer.error(...) interpolates {e} or a server-chosen id the same way), so fixing individual commands would leave the identical hazard on the adjacent lines and give a false sense of coverage. One boundary, applied once.

This is the same class of defense the repo already applies to untrusted download filenames in _sanitize_ext (comfy_cli/command/transfer.py) — that one guards the on-disk name, this one guards the rendered line.

Verification

• pytest tests/ --ignore=tests/e2e — 2813 passed, 18 skipped.
• ruff check and ruff format --check (pinned v0.15.15, matching .pre-commit-config.yaml) clean on every changed file. The 15 pre-existing UP038 findings elsewhere in the repo come from a newer ruff and are untouched by this PR.
• New tests: tests/comfy_cli/output/test_sanitize.py (28 cases across CSI/OSC/DCS/APC, 8-bit C1 forms, unterminated sequences, idempotence, and the text that must survive), plus panel and renderer boundary tests. Both boundary tests are genuine regression tests — I confirmed rich.print passes \x1b through unchanged, so they fail without this change.
• The JSON side is asserted two ways: no raw ESC on the wire, and the parsed envelope values round-trip byte-for-byte as they went in.
• Not ReDoS-prone: the regex has no nested quantifiers, and 400 KB adversarial inputs (unterminated OSC runs, 200k-parameter CSI, ESC-then-long-tail) all sanitize in under 5 ms.

Judgment calls

• Extended past the ticket's letter to Renderer.info and success. The ticket named error and warn. Sanitizing warn while leaving the adjacent info unsanitized would reproduce the exact "false sense of coverage" the ticket argues against, and both take a plain string straight to the terminal. Zero call sites today, so no behavior risk.
• Renderer.print is deliberately NOT sanitized. It is the generic passthrough and accepts arbitrary Rich renderables, not just strings; sanitizing there would mean stringifying renderables. Callers with untrusted text should use the semantic helpers above.
• which_panel / auth_list_table / discover_panel are deliberately NOT sanitized. They render locally-sourced data (workspace paths, the local auth store, the CLI's own command help), not server-supplied text. Adding them would widen the diff without closing a reachable path.
• The helpers coerce with str() before sanitizing (sanitize_value, not sanitize). Their params are annotated str, but warn/success previously interpolated via an f-string, which accepted anything. Keeping the coercion means a loose caller gets the old behavior instead of a new TypeError; there is a test for it.
• An unterminated OSC/DCS sequence consumes the rest of the string. That is what a terminal itself does with it, so dropping the tail is the faithful reading rather than avoidable data loss. Only reachable via a real ESC byte, which never appears in legitimate message text.
• Homoglyph and bidi-override spoofing (e.g. U+202E) are out of scope, and noted as such in the module docstring. They are a different problem with a different fix; the ticket scoped this to C0/C1 and ANSI.

Capability-denial check

The change removes ANSI passthrough from four rendering helpers, so I checked empirically that nothing legitimately relies on it rather than assuming: error_panel has exactly one caller (Renderer.error); warn/info/success have exactly one call site between them (comfy_cli/command/outdated.py:512, which already wraps its input in rich.markup.escape); all 242 .error(...) call sites pass f-strings or str(e); and no .py file in comfy_cli/ contains a raw \x1b/\033/\e[ literal. No product path emits intentional ANSI through these helpers, so nothing is denied.

Closes BE-4794.

[coderabbitai]

Warning

Review limit reached

You’ve reached a temporary PR review limit under our Fair Usage Limits Policy.

Your recent review volume is higher than typical usage, so adaptive limits are currently applied.

Next review available in: 9 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 5d2a7a67-9e1f-431e-8b6f-e4bebb306a0b

📥 Commits

Reviewing files that changed from the base of the PR and between cf07841 and 53d7759.

📒 Files selected for processing (14)

• comfy_cli/command/models/search.py
• comfy_cli/command/nodes.py
• comfy_cli/command/outdated.py
• comfy_cli/command/run/__init__.py
• comfy_cli/command/run/execution.py
• comfy_cli/command/run/preflight.py
• comfy_cli/command/run/watcher.py
• comfy_cli/output/panels.py
• comfy_cli/output/renderer.py
• comfy_cli/output/sanitize.py
• tests/comfy_cli/command/test_pretty_print_sanitize.py
• tests/comfy_cli/output/test_panels.py
• tests/comfy_cli/output/test_renderer.py
• tests/comfy_cli/output/test_sanitize.py

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch matt/be-4794-sanitize-ansi

✨ Simplify code

• Create PR with simplified code
• Commit simplified code in branch matt/be-4794-sanitize-ansi

---

_{Comment @coderabbitai help to get the list of available commands.}

[github-actions]

🔍 Cursor Review — Consolidated panel

Triggered by @mattmillerai.

Found 4 finding(s).

Severity	Count
🟠 High	2
🟢 Low	2

Panel: 6/8 reviewers contributed findings.

Reviewers that did not contribute: kimi-k2.5:adversarial (empty), kimi-k2.5:edge-case (empty)

[mattmillerai]

Addressed all 4 findings from the Cursor panel (2 High, 2 Low) in f469172 + 3ce16d1. All four threads replied to and resolved.

The High findings were real, and they shared one root cause. The PR stripped the escape bytes, but Rich manufactures new ones from markup it finds in the string afterwards — so the sanitizer was bypassable. Reproduced both halves before fixing:

• renderer.warn("[link=https://attacker.example]click[/link]") emitted \x1b]8;id=...;https://attacker.example\x1b\\ — a live OSC 8 hyperlink, created after sanitizing. Same via error_panel(details=...), since Table.add_row parses markup in a str cell.
• An unbalanced [/] raised rich.errors.MarkupError, crashing the CLI while it was merely printing an info line.

Fix: new sanitize_markup() (sanitize_value + rich.markup.escape), applied at every sink that actually parses markup — Renderer.info/warn/success and their hints, plus the details rows in error_panel. Values that go to rich.text.Text (code/message/hint) keep plain sanitize_*: Text never parses markup, so escaping there would print the backslashes. That split is documented on both functions.

I moved the escaping into the boundary rather than the call sites deliberately. The original design asked every call site to remember rich.markup.escape, which is the exact discipline a boundary exists to replace — and outdated.execute was the one place doing it manually. I removed that now-redundant call, since double-escaping renders a visible stray backslash.

The tradeoff, checked rather than assumed: these helpers no longer accept caller markup. An exhaustive grep of .info|.warn|.success( across comfy_cli/ returns 7 hits — 6 are stdlib logging, and the 7th passed pre-escaped text. Nothing is denied: callers who want markup still have renderer.print(), the documented generic passthrough for arbitrary Rich renderables, and these four style their own line anyway.

Also verified no cosmetic regression: rich.markup.escape is a no-op on markup-free text, so realistic messages render byte-identically — node list[0] failed, shape [1,2,3] mismatch, [2/5] downloading model, C:\Users\me\path, URLs.

Low findings: added SOS (\x98) to the 8-bit C1 string-introducer class, so \x98payload\x9c is consumed as a unit instead of leaving payload as visible garbage — matching the 7-bit \x1bX form. And error_panel code/message now use sanitize_value; I extended the same to hint via sanitize_optional, which carried the identical TypeError hazard.

Tests: 15 new cases, each verified to fail with the source fix stashed (not vacuous). Console width is pinned in the markup tests since rich.print gives no way to pass width through and the error-panel case fit 80 columns with only ~10 to spare. Full suite green locally (2835 passed), ruff check + ruff format clean against the CI-pinned 0.15.15.

No deferrals — everything raised was fixable in scope.

[bigcat88]

The sanitizer itself is excellent and I could not break it. What I can break is the claim that it closes the hole — the headline attack from your own ELI-5 (comfy run --host/--port pointed at a stranger's machine) still clears my screen and sets my window title on this branch. Requesting changes so BE-4794 doesn't close on a threat that still works, not because anything here is wrong.

What I verified works. sanitize() neutralized all 13 vectors I threw at it — CSI 2J, OSC 0 title-set, OSC 8 hyperlink, CR repaint, 8-bit C1 introducer, DCS, APC, unterminated OSC, lone trailing ESC, two-/three-char escapes, NUL/BEL/DEL, raw C1 block — with zero leaks, idempotent, and legitimate text (newlines, tabs, unicode, literal [brackets]) untouched. ReDoS claim holds: 400 KB adversarial inputs in 0.4–6.8 ms. Your non-vacuity claim is real — I confirmed Console.print passes the CSI 2J sequence through intact for both str and Text, so the boundary tests are genuine regression tests. All four helpers strip end-to-end through the real Renderer, while JSON mode emits the � escape and round-trips the payload byte-for-byte. Suite: 2907 passed, 1 failure that is the sandbox's test_non_fast_deps_uses_global_python, identical on main. ruff clean.

The gap. I stood up a hostile ComfyUI-shaped server (answers /history with 200, returns text/plain with raw escape bytes on /prompt) and ran the real CLI against it in pretty mode on this branch:

$ COMFY_OUTPUT=pretty comfy run --workflow wf.json --host 127.0.0.1 --port 8793
raw ESC bytes: 4 | CSI-2J: True | OSC-title: True

Error running workflow (HTTP 400)
prompt <clear-screen><set-title:PWNED><red>FAKE: install succeeded
╭─ error · client_error ────────────────────────────────╮   <- this part IS sanitized

The panel is clean. The line above it is not. comfy_cli/command/run/execution.py:203 prints the server's body via pprint under its own is_pretty() gate — two lines above the renderer.error(...) your boundary does cover, interpolating the same body_text. That is exactly the "identical hazard on the adjacent lines / false sense of coverage" your body argues the call-site approach would produce; the boundary approach lands there too, because Renderer is not the only path to the terminal.

Second, independent defect at the same line: a body of boom [/] unbalanced crashes the CLI with an unhandled rich.errors.MarkupError traceback — the precise failure your sanitize_markup docstring predicts. A remote server can hard-crash the client.

Both are pre-existing on main, not introduced here — I checked, main crashes identically. So this PR makes nothing worse. But it is scoped as the fix for server-supplied ANSI, and merging it as-is means the ticket closes with comfy run against a hostile host still executing escape sequences.

The fix is one line, using your own helper. I applied it locally and re-ran the same attack:

pprint(f"[bold red]Error running workflow (HTTP {e.status})\n{sanitize_markup(body_text)}[/bold red]")

raw ESC bytes: 0 | CSI-2J: False | MarkupError: False

Error running workflow (HTTP 400)
boom  [/] unbalanced          <- renders literally, no crash

Sibling sites worth sweeping while you're in there (same shape — server-supplied text into a pretty-mode pprint/rprint, never touching Renderer):

• command/run/__init__.py:962 — w['message'] from cloud warnings
• command/run/preflight.py:111 — w['field'] / w['message'], field names derive from object_info
• command/models/search.py:606-620 — row['name'] / type / tags / source_url from the cloud asset catalog
• command/run/__init__.py:471 — the interpolated exception on lost connection

execution.py:193 and :544 are already safe — json.dumps escapes the escape byte — worth a comment saying so, so nobody "fixes" them later.

Everything else in the PR I agree with, including the judgment calls: extending to info/success is right, leaving Renderer.print alone is right, sanitize_value's str() coercion over a new TypeError is right, and consuming the tail of an unterminated OSC is the faithful reading. Re-request me once the pprint paths are covered and I will re-run the hostile-server suite against it.

[mattmillerai]

Fixed in 98665cc — you were right, and the reword alone would not have been enough.

I widened coverage rather than just narrowing the claim, then did both. sanitize_markup now runs at the direct rich.print sites across the comfy run and comfy models surfaces:

• execution.py — HTTP error body (your repro), the execution_error payload, the submit URLError reason, and the --verbose log_node line (leaves escaped before assembly, since that string builds its own markup)
• run/__init__.py — lost-connection exception, local output paths, cloud output URLs and warnings
• preflight.py — validation warning field/message
• watcher.py — output URLs off the state file
• models/search.py — show fields plus the three Table.add_row sites (add_row parses markup in a str cell, the same sink as the error panel's _kv_table)

One correction to your sweep list: execution.py:193 and :544 are not already safe. json.dumps escapes the ESC byte as a \u escape, so the ANSI half is covered as you said — but it does not escape [. A node-error message of [/red] survives the JSON encoding intact and still raises MarkupError:

rich.errors.MarkupError: closing tag '[/red]' at position 92 doesn't match any open tag

So a hostile server crashes the client through the node_errors path too. Both json.dumps sites are sanitized now. Worth noting the rendered output is unchanged: Rich consumes the escaping backslash, so the blob on screen is byte-identical to the original JSON (asserted in a test).

The docstring is reworded too — it now says automatic coverage stops at Renderer and a bare rich.print of remote text must sanitize explicitly, instead of claiming to be "the single sanitizing boundary".

15 regression tests in tests/comfy_cli/command/test_pretty_print_sanitize.py drive the real call sites in pretty mode against a force-terminal console, asserting no OSC 8 / OSC 0 / CSI 2J and no crash on [/]. Every one was verified to fail without its fix. Full suite: 2850 passed, 37 skipped; ruff clean on the touched files.

Two caveats I'd rather state than have you find:

1. The cloud-warning and local-output loops in execute_cloud / execute are fixed but not unit-tested — driving those end-to-end needs the whole cloud polling path, which I judged not worth the harness. They are the same one-value sanitize_markup(x) shape as the tested sites.
2. I scoped this to the comfy run + comfy models surfaces, which is BE-4794's threat model. The repo has ~500 other rich.print sites (install.py, setup.py, nodes.py), carrying registry and local text rather than remote-host text. Sweeping those is a separate piece of work, not a silent omission.

Re-requesting review — the hostile-server suite should come back clean now.

[mattmillerai]

Follow-up to the above — my own adversarial pass found a third surface reachable by the identical attack, so I fixed it here rather than deferring it (1a5a94f).

comfy nodes ls / show / search / types / categories / path take the same --host/--port pair as comfy run and render object_info straight from that host into markup sinks — node ids, categories, descriptions, display names, input/output types. Verified against a hostile object_info:

comfy nodes show --host <evil>   ->  CSI-2J: True | OSC-title: True
description = "boom [/] x"       ->  rich.errors.MarkupError (unhandled)

Deferring that would have closed BE-4794 with the same threat still working one command over, which is exactly the objection you raised on the run path — so it seemed wrong to file it as a follow-up.

Intentional markup is preserved: the [dim]—[/dim] empty-outputs fallback and the [bold]/[cyan] wrappers are ours, so only the joined server values inside them are escaped. The search/description cells truncate before escaping so the escapes stay balanced.

5 more regression tests (20 total in the new file), each verified failing without the fix. Full suite 2855 passed, 37 skipped; ruff clean on touched files.

On the scope boundary I mentioned: I checked the rest of the repo's rich.print surface (install.py, setup.py, cmdline.py, the registry commands) and did not find attacker-controlled remote text flowing into those — they carry local paths, versions, user input, and manager-delegated output. So I'm not proposing a follow-up ticket for them; the reworded module docstring is what guards the next reader. If you know of a remote-text path there I missed, say so and I'll take it.

[mattmillerai]

Heads-up on the red build check — it is not this PR, it is main.

build fails on a single assertion:

FAILED tests/comfy_cli/output/test_error_code_registry.py::test_no_duplicate_codes
  AssertionError: Duplicate codes in registry: ['server_died']
  1 failed, 3125 passed, 36 skipped

comfy_cli/error_codes.py registers ErrorCode("server_died", ...) twice on main — once from #605 (BE-4750) and once from #604 (BE-4751). Neither PR touched the other's line, so git merged both with no textual conflict and the collision only shows up at runtime.

Evidence it is inherited, not introduced here:

• this PR's diff does not touch error_codes.py (git diff origin/main...HEAD --name-only | grep error_codes → nothing)
• git show origin/main:comfy_cli/error_codes.py | grep -c '"server_died",' → 2
• Run pytest on main itself (e6965d0) is failing with the same assertion
• every other job here is green: ruff_check, test, Run Tests on GPU Runners (linux), Run Tests on Multiple Platforms (ubuntu/macos/windows, 3.10), CodeQL, Analyze, Socket, CLA

I deliberately did not make it green from this branch. The assertion is correct — it caught a real contract collision in the public error-code registry — and deleting or relaxing someone else's passing-by-design test to unblock my own PR would ship the breakage. Fixing it properly means collapsing the two entries into one and reconciling their descriptions/hints, which is a change to the public error-code surface and belongs in its own PR, not buried in an ANSI-sanitizing one. Filed as a separate follow-up.

So: this branch is clean on its own merits. Once the server_died duplicate is fixed on main, this check goes green here with no change to this PR.

Also note the branch now contains a merge of main (53d7759) taken to clear a textual conflict in command/run/__init__.py: main renamed jobs_state.write → _write_state and moved the pretty Outputs: block out of the try to after the finally, iterating completed_payload["outputs"]. I took main's structure wholesale and re-applied the sanitize at the block's new home, so both intents survive. Verified on the merged tree: 3039 passed, 37 skipped locally, ruff clean on the touched files.

[mattmillerai]

🤖 The reviews loop filed Linear follow-up ticket(s) for review thread(s) deferred as out of scope for this PR:

• BE-5023 — Strip ANSI (not just Rich markup) from download-server error text in comfy model download
• BE-5024 — Fix duplicate server_died error code breaking main (collides between fix(jobs): detect a dead local server and record a terminal server_died error (BE-4751) #604 and fix: comfy run --wait writes job state at submit, names prompt_id on disconnect (BE-4750) #605)

[bigcat88]

Fixed — approving. I re-ran the exact attack that broke this branch yesterday.

Before (my last review), same command, same hostile server:

raw ESC bytes: 4 | CSI-2J: True | OSC-title: True | MarkupError: crash

Now:

raw ESC bytes : 0
CSI-2J        : False
OSC title-set : False
MarkupError   : False

Error running workflow (HTTP 400)
prompt FAKE: install succeeded and [bold]markup[/bold] and [/]

Screen-clear gone, window-title rewrite gone, and the unbalanced [/] that used to raise an unhandled MarkupError now renders literally. The hostile server is the same rig: answers /history with 200, returns text/plain carrying raw ESC[2J, OSC title-set, and Rich markup.

All four sibling sites I listed are covered, plus one I'd missed:

• run/execution.py:207 — sanitize_markup(body_text), exactly the fix
• run/__init__.py:473 — lost-connection exception text
• run/__init__.py:518, 963 — cloud warnings / uploads
• run/preflight.py:113 — both field and message
• models/search.py:214, 215, 304 — catalog-supplied names and subfolders
• command/nodes.py:287, 288, 362, 364 — object_info-derived node ids, categories and display names. Good catch; I didn't flag that one and it's server-supplied text on a surface people run constantly.

The JSON contract is still intact, which is the part that would have been easy to break while fixing this. stdout is pure JSON (0 non-JSON lines), the raw escape bytes are preserved inside the payload so agents still parse exactly what the server sent, and no raw ESC byte appears on the wire. Strip at the terminal, never mutate the data — still correct.

On the suite: 3057 passed, 2 failed, and neither is yours.

• test_non_fast_deps_uses_global_python — the usual sandbox artifact, fails on plain main here too.
• test_no_duplicate_codes — main is currently broken, independently of this PR. error_codes.py has server_died registered twice: #605 (BE-4750, "connection dropped while a foreground --wait job was in flight") at line 366 and #604 (BE-4751, "local ComfyUI server became unreachable") at line 376. Both merged, git auto-merged them cleanly because they sit at different offsets in the list, and the combined tree fails. I confirmed it on a clean main checkout at e6965d0 with error_codes.py untouched — nothing to do with this branch. Flagged separately; don't let it hold this up.

ruff check / ruff format --check clean over 261 files. 18 new cases in test_pretty_print_sanitize.py covering the direct-pprint sites.

Nice turnaround — the boundary claim in the description is now actually true.