fix(transfer): sanitize download extension from untrusted ?filename= param (BE-3326)#548
fix(transfer): sanitize download extension from untrusted ?filename= param (BE-3326)#548mattmillerai wants to merge 2 commits into
Conversation
…param (BE-3326) The download extension was taken straight from the untrusted `filename` query param via `Path(remote_name).suffix` and embedded into the on-disk name and echoed path. Unlike the `item` token, `ext` was never sanitized, so a malicious server returning `?filename=out.png` plus control/ANSI bytes could inject into the terminal when the path is printed in human mode. Add `_sanitize_ext` next to `_sanitize_item_name` and apply it to `ext` in both the local-source and query-param branches, whitelisting to a safe extension charset. Directory traversal remains impossible (Path.suffix drops path components). Tests pin both the control-byte scrub and the preserved no-traversal behavior.
mattmillerai
added
agent-coded
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 49 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the 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 configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughDownload extension handling now sanitizes local and remote filename-derived suffixes. Tests cover control-byte removal, fallback naming, reported paths, and traversal-shaped filenames. ChangesDownload Extension Safety
Suggested reviewers: 🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
Comment |
dosubot
Bot
added
the
size:S
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
🔍 Cursor Review — Consolidated panel
Triggered by @mattmillerai.
Found 3 finding(s).
| Severity | Count |
|---|---|
| 🟡 Medium | 2 |
| 🟢 Low | 1 |
Panel: 6/8 reviewers contributed findings.
Reviewers that did not contribute: kimi-k2.5:adversarial (empty), kimi-k2.5:edge-case (empty)
Address three cursor-review findings on the download-extension sanitizer: - Collapse an extensionless suffix (`.💥`, `.日本語`, lone control byte, or bare `.`) to "" so the caller's `or ".png"` fallback applies instead of a truthy bare-dot result that writes `<id>_000.` — invalid/normalized on Windows and desyncing the reported path from the on-disk name. - Cap the whitelisted extension length (_MAX_EXT_LEN=20) so a hostile `?filename=out.<thousands of chars>` can't push local_name past NAME_MAX and raise OSError(ENAMETOOLONG) outside the NDJSON error contract. - Assert the exact sanitized name in the control-byte tests (not a `.png` prefix) so a regression leaking control bytes after `.png` is caught; add direct unit tests for the empty-collapse and length-cap behaviors. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
dosubot
Bot
added
size:M
|
🤖 The reviews loop filed Linear follow-up ticket(s) for review thread(s) deferred as out of scope for this PR:
|
|
🤖 Reviews-loop pass: all 3 Cursor findings are resolved (fixed in ff17cd9, present in the current diff) and CodeRabbit passed with no actionable comments. The PR merges cleanly (no conflict). The two red CI checks are pre-existing, infra/dependency failures unrelated to this PR (which only touches
Transfer tests pass locally (77 passed, 1 skipped). |
|
↑ Correction: the |
1 participant
ELI-5
When
comfy downloadsaves a file, it guesses the file extension from thename the server hands back (a
?filename=...query param). That name isuntrusted — a malicious or compromised server can stuff invisible control /
ANSI escape bytes into it. Those bytes were copied straight into the saved
filename and into the path we print to the terminal, so printing the result
in human mode could let the server inject terminal escape sequences. This PR
scrubs the extension through a small whitelist so only safe characters
survive.
What changed
_sanitize_extnext to the existing_sanitize_item_nameintransfer.py. It whitelists the extension to[A-Za-z0-9._-], droppingeverything else (control bytes, ANSI escapes, etc.).
extin both derivation branches of the download loop —the local-source branch (
local_source.suffix) and the query-param branch(
Path(remote_name).suffix). The existingor ".png"fallback is kept, soan extension that sanitizes to empty still degrades to
.png.(
?filename=out.png\x1b[31mHACK) yields a clean.png-family extensionwith no control bytes in the resulting local name or echoed path —
covered for both the query-param and local-source branches; (2) the
existing no-directory-traversal behavior is preserved.
Why this is its own PR (not part of #531)
This naming/ext block is byte-identical to
origin/main, and #531 (BE-3273)is a deliberately pure mechanical extraction whose contract is that helper
bodies are verbatim with unchanged runtime behavior. Sanitizing
extis areal behavior change, so it belongs in its own small, testable diff against
the pre-existing issue on main. (BE-3326, deferred from a #531 review thread.)
Verification
pass with it; the no-traversal test passes both ways (regression guard).
tests/comfy_cli/command/suite: 1160 passed, 2 pre-existing skips.ruff check+ruff format --checkclean on both changed files.Judgment call
The sanitizer whitelists rather than truncating. For the attack input
out.png\x1b[31mHACK, the on-disk extension becomes.png31mHACK— clean,png-family, and free of control bytes — not a bare
.png. This follows theticket's proposed whitelist approach and avoids a more invasive
"truncate the extension at the first junk byte" behavior change; the
security-relevant property (no control/ANSI bytes reach the filename or the
echoed path) is fully enforced.
Not applicable
Negative-claim / capability-denial falsification: this change adds no
"not supported" / dead-end path — it only sanitizes a string — so that check
does not apply.