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

オープン
  • 何か問題がありますか？
#746 コメント 0 件 リアクション 0 件 担当者 1 名 GitHub で見る

メンテナーはふだん 1 日以内に返信

@robinjhuang がすでに取り組んでいます。

2026年8月21日 から。

評価

この issue はまだ評価されていません。

説明

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

主要言語
Python
スター
1k
フォーク
176
平均マージ
1日 11時間
マージ済み PR（30日）
87

環境構築

はじめの一歩

  1. issue を最後まで読み、次にプロジェクトのコントリビューションガイドを読みます。
  2. 着手することを issue にコメントします — 二人が同じ作業をするのを防げます。
  3. リポジトリをフォークし、ブランチを切って変更します。
  4. issue 番号を参照したプルリクエストを送ります。

Comfy-Org/comfy-cli のほかの issue

Comfy-Org/comfy-cli の issue をすべて見る

似ている issue

Python の issue をもっと見る

新しい issue をメールで受け取る

初心者向けの GitHub issue を短くまとめたダイジェスト。