Comfy-Org / Comfy-Org/comfy-cli
`comfy generate --download`: the partner's job id is used unvalidated as the on-disk write path
- Dominant language
- Python
- Stars
- 968
- Forks
- 151
- Avg merge
- 1d 9h
- Merged PRs (30d)
- 77
Description
**What** — In `comfy generate`, the job id that comes back in the **partner's create-response body** is used unvalidated as the on-disk filename for `--download`. Both the path component and the file extension are remote-controlled, so the remote side chooses the full path the CLI writes to, and the bytes it writes there.
`extract_job_id`'s own docstring says it pulls the id "for display". It is not used for display only.
**The chain**, all on `origin/main` (`3ff9f55`):
| Step | Location |
|---|---|
| id read out of the remote body, `str(v)`, no validation | `comfy_cli/command/generate/poll.py:331-339` |
| the "for display" value becomes the write path | `comfy_cli/command/generate/app.py:669` → `app.py:723` `_emit_result(result, request_id=job_id, …)` |
| path built from it, unsanitized | `comfy_cli/command/generate/output.py:45-51` (`_resolve_template`) |
| parent dirs created, bytes written | `comfy_cli/command/generate/output.py:70-72` |
| the extension is *also* remote and unsanitized | `comfy_cli/command/generate/output.py:32-34` (`_ext_from_url`) |
No unusual flag is needed. The documented directory shorthand `--download out/` is enough, because that branch is `Path(template) / f"{request_id}_{index}.{ext}"` — a `..` in `request_id` walks straight out of `out/`.
**Attacker position** — the upstream partner model API (Kling, Luma, BFL, MiniMax, Pika, Runway, …), or `api.comfy.org`'s `/proxy/` relay of its JSON. A hostile or compromised partner response is the input; a file written outside `--download` with attacker-chosen content is the output. This is a supply-chain position, not a drive-by, which is why I'd call it high-impact / moderate-likelihood rather than critical.
**Reproduction** — runs the real `origin/main` `poll.py` and `output.py`; only `client.download_bytes` is stubbed so nothing hits the network. Controls on every row:
```
CONTROL extract_job_id: 'abc123'
ATTACK extract_job_id: '../../../../victim/.config/pwned' <- returned verbatim
--download dir: /tmp/x05-.../home/victim/images
CONTROL dir-shorthand -> .../home/victim/images/abc123_0.png inside --download dir? True
ATTACK dir-shorthand -> /tmp/victim/.config/pwned_0.png inside --download dir? False
CONTROL {request_id} -> .../home/victim/images/abc123.png inside --download dir? True
ATTACK {request_id} -> /tmp/victim/.config/pwned.png inside --download dir? False
_ext_from_url('https://evil/x.bashrc') -> 'bashrc'
_ext_from_url with ANSI bytes -> 'p\x1b[31mng'
```
The escaped files exist on disk and contain the stubbed payload bytes. Because `request_id` and `ext` are *both* remote-controlled, the final filename is chosen end-to-end by the remote side; I've left the obvious escalation out of a public issue, and can walk through it privately if useful.
**There is already a fix for this exact bug class in this repo, one command over.** `comfy download` does all three things this path omits:
- `comfy_cli/command/transfer.py:262-264` — `_sanitize_item_name` restricts the name token to `[A-Za-z0-9._-]`
- `comfy_cli/command/transfer.py:270-282` — `_sanitize_ext` whitelists and length-caps the extension
- `comfy_cli/command/transfer.py:762` — collision-safe path
- `comfy_cli/command/transfer.py:765` — refuses to overwrite a symlink
That guard landed in #548 / BE-3326. `generate/output.py` never got the same pass.
**Also still open from that same review.** In #539's self-review (2026-07-17) the `_ext_from_url` half was spotted and deferred to its own ticket — *"has the same bug class in a different command … verified reachable from `comfy generate --download` … reproduced the primitive"*. It never landed. Same file, same fix.
**Secondary, same values, same file** — `print_urls` / `print_saved` (`generate/output.py:104-113`, `:128-133`) push the remote `request_id` and the remote `url` through a bare `rich.print` with no `sanitize_markup`, so a partner can inject ANSI into human-mode output. The repo's own `comfy_cli/output/sanitize.py` docstring calls out exactly this pattern: *"Command modules also call `rich.print` directly under their own `is_pretty()` gates … treat a bare `rich.print` of remote text as unsanitized until you have checked it."* `_emit_result` two frames up already sanitizes `result.error` "because it is remote" (`app.py:371`), so the boundary is understood here — just not applied on the success path.
**Suggested fix** — validate `request_id` as a single inert path component before it reaches `_resolve_template` (the `_reject_unsafe_component` helper at `comfy_cli/command/models/models.py:130-157` is already the house pattern), route `_ext_from_url` through a `_sanitize_ext`-style whitelist, and `sanitize_markup` the remote values in `print_urls`.
**Blame** — `git log -S"request_id=job_id"` → `d81915a` (2026-05-16, *"feat(generate): add `comfy generate` for direct partner model calls (#452)"*), which introduced `_resolve_template`, `save_urls` and `_ext_from_url` together. Plain `git blame` points at `56ada5b4` instead, but that is a later tracking refactor that only moved the line.
---
Found during a cross-repo security review of `comfy-cli`, 2026-08-21. Related: #725 (the other unfiltered-write path), #539/#548 (the sibling fix).
Contributor guide
Assessment
This issue has not been assessed yet.