`comfy generate --download`: the partner's job id is used unvalidated as the on-disk write path

Aperta
  • Qualcosa non va?
#746 0 commenti 0 reazioni 1 assegnatario Vedi su GitHub

I maintainer di solito rispondono entro 1 giorno

@robinjhuang ci sta già lavorando.

Dal 21/8/2026.

Valutazione

Questa issue non è ancora stata valutata.

Descrizione

bug

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).

Lingua principale
Python
Stelle
1k
Fork
176
Merge medio
1g 11h
PR unite (30g)
87

Preparare l'ambiente

Come iniziare

  1. Leggi tutta la issue e poi la guida ai contributi del progetto.
  2. Commenta sulla issue per dire che te ne occupi tu — evita che due persone facciano lo stesso lavoro.
  3. Fai un fork del repository e lavora su un branch.
  4. Apri una pull request che faccia riferimento al numero della issue.

Altre issue di Comfy-Org/comfy-cli

Tutte le issue di Comfy-Org/comfy-cli

Issue simili

Altre issue su Python

Ricevi le nuove issue nella tua casella

Un breve riepilogo di issue GitHub adatte ai principianti.