fix: sanitize the download filename extension derived from ?filename=

[mattmillerai]

ELI-5

When comfy download saves a file, it decides the file's extension (.png, .mp4, …) by reading the ?filename= bit of the URL the output server gave it. Nobody was checking what came back. A hostile or compromised server could answer ?filename=out.png<ESC>[31mHACK and those invisible escape bytes would end up in the real filename on your disk — and get printed straight to your terminal when the saved path is echoed. This makes the extension go through a "letters and numbers only" check first, and falls back to .png when it doesn't pass.

What changed

• New _sanitize_ext() helper next to the existing _sanitize_item_name(), mirroring its style: an extension is kept only if it fullmatches \.[A-Za-z0-9]{1,16}, otherwise it falls back to .png — the same default the callers already used for a suffix-less name.
• execute_download now derives the remote extension as _sanitize_ext(Path(remote_name).suffix). The item token was already sanitized; the extension was the asymmetry.
• Tests covering the happy path, the ANSI/control-byte case, every fallback case, and a regression pin that traversal stays impossible.

Scope — what this is and is not

This is Low severity and cosmetic: junk filenames plus terminal injection into human (non-JSON) output. It is not an arbitrary-write bug. Path traversal was never possible and still is not — Path(remote_name).suffix only reads the last path component's extension, so ?filename=x.../../../etc/passwd yields .png, not a traversal. test_traversal_stays_impossible pins that as a regression guard rather than implying it was ever broken.

Judgment calls

1. The length cap is 16, not the proposed 10. .safetensors is 12 characters — a 10-cap would silently rename a legitimate output to .png. The security property here is the character class (alphanumeric only, which is what excludes control/ANSI bytes); the length bound just stops absurd junk, so widening it costs nothing. I verified empirically that no real output extension the product emits is mangled: .png .webp .jpg .jpeg .gif .mp4 .webm .flac .mp3 .wav .ogg .safetensors .latent .json .txt .glb .avif .mkv .mov .tiff .exr .pt .ckpt all pass through unchanged, and test_long_real_extension_is_preserved locks that in. Over-denial is the real risk of a whitelist, so it is worth pinning.
2. The local_source.suffix branch is deliberately NOT sanitized — CORRECTED: it is now sanitized too. I originally argued a locally-produced filename comes from the job's own on-disk output, so it is not a trust boundary. That was wrong, and review caught it. _local_source_path only parses a Path out of an outputs entry — it never proves the string came from a real local render — and that entry arrives from untrusted metadata (a piped stdin envelope's data.outputs, or a cloud/remote record), exactly as execute_download's own is_local_job comment says two lines above the call site. Verified: an outputs entry ending evil.png<ESC>[31mHACK produced a real on-disk file carrying those bytes. Both branches now go through _sanitize_ext; the or ".png" fallback is preserved because _sanitize_ext("") already returns .png. Pinned by test_copied_output_extension_is_sanitized, with test_copied_output_keeps_real_extension_not_png still green so legitimate suffixes like .webp are untouched.
3. Deliberately built on main, not stacked on refactor(download): extract execute_download per-URL loop into _download_one_url / _copy_local_one / _stream_http_one (BE-3273) #531. This bug is pre-existing on main (transfer.py:641); refactor(download): extract execute_download per-URL loop into _download_one_url / _copy_local_one / _stream_http_one (BE-3273) #531 is a byte-identical mechanical extraction whose whole reviewability argument is that runtime behavior is unchanged, so a real behavior change does not belong in it. The two touch the same lines and will need a trivial merge resolution whichever lands second — the sanitization is one call site either way.

Noted, not fixed (out of scope)

The download error paths also echo the raw url back in details / the human-mode message, which is the same class of terminal-injection surface but a wider one than this ticket scopes. Not touched here; flagging it rather than silently expanding the diff.

Testing

• uv run --extra dev pytest tests/comfy_cli/command/test_transfer_download.py — 74 passed, 1 skipped.
• uv run ruff format --check — clean (235 files).
• uv run ruff check — 14 pre-existing UP038 errors in untouched modules (workflow_to_api.py, oauth.py, …); zero in the files this PR changes.

[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: 39 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: 82e6618b-5549-4176-867d-ec17f5c33202

📥 Commits

Reviewing files that changed from the base of the PR and between a732f5c and d53fe66.

📒 Files selected for processing (2)

• comfy_cli/command/transfer.py
• tests/comfy_cli/command/test_transfer_download.py

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch matt/be-3300-sanitize-download-ext

✨ Simplify code

• Create PR with simplified code
• Commit simplified code in branch matt/be-3300-sanitize-download-ext

---

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

[github-actions]

🔍 Cursor Review — Consolidated panel

Triggered by @mattmillerai.

Found 2 finding(s).

Severity	Count
🟢 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]

Review pass — one finding fixed, one declined, CI triaged.

Fixed (3176abe): the where == "local" branch is now sanitized too. The adversarial reviewer was right and my PR description was wrong. I'd claimed the local branch isn't a trust boundary because the filename "comes from the job's own on-disk output" — but _local_source_path only parses a Path out of an outputs entry, and that entry arrives from untrusted metadata (a piped stdin envelope's data.outputs, or a cloud/remote record), which execute_download's own is_local_job comment states two lines above the call site. Nothing proved the string came from a real local render. Reproduced it before fixing (an outputs entry ending evil.png<ESC>[31mHACK produced a real on-disk file with that suffix), then routed both branches through _sanitize_ext. The or ".png" fallback is preserved — _sanitize_ext("") already returns .png. I've corrected judgment call #2 in the description above.

Declined: allowlisting output extensions. download writes bytes into --out-dir and never opens or executes them, and the content is server-controlled regardless of the name — a hostile server wanting a user to open HTML can already serve HTML bytes under .png. An allowlist changes the label, not the payload, while silently corrupting every output format the product grows next. Full reasoning on the thread.

CI: build and test are red, and it isn't this PR. Both fail identically on #540 and #541, which don't touch transfer.py — it's ambient repo-wide breakage: a newer tomlkit now rejects the multi-line comment comfy node init writes (test_node_init.py, ValueError: Comment cannot contain line breaks), and the Windows test job hits ImportError: cannot import name '__version__' from 'pydantic_core'. This PR's diff is transfer.py + its test only, so neither can be caused by it. Flagged for a separate fix rather than pinning deps inside a narrow security change. ruff_check, CodeQL, the GPU runner, and the ubuntu/macos/windows test matrix are all green.

Local: 1300 passed, 2 skipped across tests/comfy_cli/command/ + tests/comfy_cli/cql/; ruff format --check clean; ruff check clean on both changed files.

[mattmillerai]

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

• https://linear.app/comfyorg/issue/BE-3309|BE-3309 — Strip terminal control bytes from the raw url echoed in comfy download error output
• https://linear.app/comfyorg/issue/BE-3310|BE-3310 — Fix repo-wide CI breakage: tomlkit regression in test_node_init.py and Windows pydantic_core ImportError

[mattmillerai]

Second review pass — no code changes needed; PR stands as-is. Three things triaged:

1. The failing Windows Specific Commands check is pre-existing and repo-wide, not from this PR. It fails at the Install Dependencies step with ImportError: cannot import name '__version__' from 'pydantic_core' — before any of this PR's tests run. It also failed on this PR's first commit (16a08eb), which predates the uv.lock change, and it is currently failing on ~17 of the last 20 runs across unrelated branches (be-3285, be-3276, be-3297, be-3274, be-3000, …). Already tracked as BE-3310; nothing to fix here. It is not a required check, hence mergeStateStatus: UNSTABLE rather than blocked.

2. The uv.lock churn is a real fix, not accidental scope creep — I checked, because 287 lines of dep churn in a filename-sanitizing PR deserves suspicion. main's lock is genuinely stale: pyproject.toml on main declares optional-dependencies.bench = ["anthropic>=0.40"] (landed with the BE-2309 bench runner) but main's uv.lock has no anthropic entry. Concretely, uv lock --check fails on main and passes on this branch. That staleness is why any uv invocation on main spontaneously dirties uv.lock. Dropping it here would leave main broken, so I kept it; the added packages (anthropic, pydantic, jiter, …) are all confined to the optional bench extra and are not installed by default.

3. Both call sites are covered. Swept every .suffix use in the repo: the two in transfer.py both route through _sanitize_ext, and the rest are tmp/backup-file suffix appends on already-local paths (auth/store.py, cql/loader.py, skills/__init__.py, workflow.py) — not untrusted-derived.

One follow-up filed from self-review. _ext_from_url in comfy_cli/command/generate/output.py:28 has the same bug class in a different command: it copies a server-controlled URL suffix straight into the on-disk filename. Verified reachable from comfy generate --download (generate/app.py:149/:159) and reproduced the primitive — a URL ending evil.png<ESC>[31mHACK yields filename req_0.png\x1b[31mhack. Out of scope for this PR (separate command, separate call path, and this diff is already reviewed), so it's going to its own ticket rather than expanding this one. _ext_from_response is unaffected — it maps content-types through a fixed dict.

Local verification on this branch: full suite 2583 passed, 37 skipped; ruff check and ruff format --check clean; no merge conflict with main.

[mattmillerai]

Pushed d53fe66 — dropped the unrelated uv.lock churn, and diagnosed the red CI (it is not from this PR).

uv.lock removed from this diff

The previous head carried a 287-line uv.lock refresh that the description never mentioned. A local uv run swept it in: main's pyproject.toml declares optional-dependencies.bench = ["anthropic>=0.40"] but main's uv.lock has no anthropic entry, so uv regenerates the lock on any run and dirties the tree. That is a real bug, but it is already the subject of dedicated PRs #541 and #543, and a dependency change has no business hiding inside a security fix. Reverted to main's lockfile — this PR is now just transfer.py (+24/-3) and its tests.

The failing CI is a pre-existing, repo-wide break — not this PR

Both red jobs are unrelated to this diff.

build / Run pytest (9 failures, ValueError: Comment cannot contain line breaks) — caused by the upstream tomlkit 0.15 release, not by any repo change. config_parser.py:75 and :250 pass a multi-line string to .comment(), which 0.15 now rejects. Verified directly rather than assumed:

tomlkit	multi-line .comment()
0.15.1	ValueError: Comment cannot contain line breaks
0.13.2	accepted

pyproject.toml:49 pins tomlkit with no upper bound and CI installs via pip install -e ., so CI resolves the newest tomlkit and breaks. Worth noting this also means CI never reads uv.lock — so the lockfile churn above was never the cause either way. main was last green 2026-07-13 and these same 9 tests fail on every open PR. Not fixable from here; I have filed it as a separate high-priority follow-up so it can land on main and unblock the whole queue.

test (windows-latest) — ImportError: cannot import name '__version__' from 'pydantic_core' during Install Dependencies, i.e. a dependency-install failure before any of this code runs. Infrastructure, not the diff.

This PR cannot go green until the tomlkit fix lands on main; the code here is green locally (75 passed, 1 skipped; ruff format + check clean).

Self-review

Re-verified the two things a whitelist fix can get wrong. Over-denial: every real output extension passes through unchanged — .png .webp .jpg .jpeg .gif .mp4 .webm .flac .mp3 .wav .ogg .safetensors .latent .json .txt .glb .avif .mkv .mov .tiff .exr .pt .ckpt .sft .gguf — zero mangled, which is why the cap is 16 and not 10 (.safetensors is 12). Missed call sites: both extension-derivation sites in transfer.py (the remote ?filename= branch and the local local_source.suffix branch) now go through _sanitize_ext, and a repo-wide grep found no others. The filename at transfer.py:334 is a dict key for node lookup, never an on-disk name, so it is correctly untouched.

[mattmillerai]

Correction to my comment above: the tomlkit fix already has a PR — #536 (fix(registry): emit pyproject hints as standalone comments for tomlkit 0.15), which takes exactly the call-site approach I described. I withdrew the follow-up ticket I had filed rather than duplicate it.

So the state of this PR is unchanged but the unblock path is concrete: #539 goes green once #536 lands on main; no action needed here. Everything else in the previous comment stands.

[mattmillerai]

Status: no changes needed here — both red checks are pre-existing repo-wide breakages, not regressions from this PR. This diff touches only comfy_cli/command/transfer.py.

build — 9 failures in tests/comfy_cli/registry/test_config_parser.py and test_node_init.py, all ValueError: Comment cannot contain line breaks. Root cause is config_parser.py:75 and :250 passing triple-quoted multi-line strings to tomlkit's .comment(), which tomlkit 0.15 rejects. The build job installs unpinned (pip install -e .) so it resolves tomlkit 0.15.1, while uv.lock pins 0.13.3 — which is why the uv-based jobs are green. Verified by reproducing on pristine origin/main with --with 'tomlkit>=0.14' (8 failed), and confirming every other recent open PR fails build identically. Already fixed by #536.

test (windows) — dependency-install failure, ImportError: cannot import name '__version__' from 'pydantic_core' during comfy install. Infra/import-order, unrelated to this diff. Already fixed by #535.

Locally on this branch: pytest tests/comfy_cli/command/test_transfer_download.py → 75 passed, 1 skipped; ruff format --check clean; ruff check on both changed files clean. Both review threads are resolved. Nothing to defer — the two root causes each already have a dedicated open PR.