fix: comfy download copies on-disk local outputs instead of refusing them (BE-2190)#485
fix: comfy download copies on-disk local outputs instead of refusing them (BE-2190)#485mattmillerai wants to merge 2 commits into
Conversation
…them (BE-2190) A LOCAL run (comfy run --where local --wait) records bare absolute output paths, but comfy download is an HTTP fetcher whose anti-SSRF guard rejected any non-http(s) URL, so downloading a local job failed with 'refusing to download from non-HTTP URL'. Detect a bare absolute path / file:// URL in the download loop and shutil.copyfile it into --out-dir before the HTTP guard runs; real URLs still go through _assert_download_url, so SSRF protection is unchanged. The copied file's extension is derived from the source file (a bare path has no ?filename= param, so the prior code mislabeled every local output .png).
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 7 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)
✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
Comment |
There was a problem hiding this comment.
🔍 Cursor Review — Consolidated panel
Triggered by @mattmillerai.
Found 6 finding(s).
| Severity | Count |
|---|---|
| 🟠 High | 2 |
| 🟡 Medium | 2 |
| 🟢 Low | 1 |
| ⚪ Nit | 1 |
Panel: 8/8 reviewers contributed findings.
…2190) Address review findings on the copy-from-disk download branch: - Gate the copy-from-disk branch on the job's own `where == "local"` marker, not on URL shape alone. `output_urls` can arrive from untrusted metadata (piped stdin envelope, cloud/remote API record), so a bare path / file:// URL from a non-local job must fall through to the SSRF guard instead of being copied off disk (was: 6/8 reviewers). - Reject UNC / network paths (\\host\share, //host/share) in _local_source_path so is_file() can't trigger an outbound NTLM-leaking SMB connection on Windows. - Use parsed.hostname (not netloc) for the loopback check so a port or IPv6 brackets (file://localhost:8080/…, file://[::1]/…) don't defeat it. - Wrap shutil.copyfile in try/except: OSError and SameFileError now surface as structured download_failed envelopes instead of tracebacks. - Apply the HTTP branch's 10 GB size cap to the local copy too. Add regression tests for the local-job gate, UNC rejection, and loopback-with-port file:// URIs. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
ELI-5
If you run a workflow locally and then try to
comfy downloadits result, ComfyUI already wrote the image to your disk — there's no web address to fetch. Butdownloadis a web-fetcher with a safety guard that refuses anything that isn't anhttp(s)://link, so it choked on the plain file path withrefusing to download from non-HTTP URL: /…/output/foo.png. This teachesdownloadto notice "hey, that's already a file on disk" and just copy it into your output folder instead of trying to fetch it — while keeping the safety guard fully in force for real remote URLs.What & why
Fixes #480.
A
comfy run --where local --waitjob records bare absolute output paths (run/execution.py::format_image_pathreturns an on-disk path for loopback hosts).comfy downloadfed those through_assert_download_url, whose anti-SSRF guard rejects any non-http(s)URL →[download_failed]: refusing to download from non-HTTP URL.The fix (additive — SSRF guard untouched for real URLs)
_local_source_path(url)returns aPathfor a bare absolute filesystem path or afile://URL with no remote host, andNonefor anything carrying a network scheme (http/https/ftp/gopher/data/…). Afile://URL pointing at a remote host returnsNonetoo, so it still hits the guard.shutil.copyfileit into--out-dir(no HTTP request, so the SSRF guard is irrelevant — there is no host to forge a request to). Everything else still runs through_assert_download_urlexactly as before.comfy downloadrefuses local runs: "refusing to download from non-HTTP URL" for on-disk output paths fromcomfy run --where local --wait#480): a bare path has no?filename=query param, so the old code mislabeled every local output.png. The copied file now takes its extension from the real source file.download_failedenvelope, not a traceback.Acceptance criteria
comfy download <prompt_id> --where localsucceeds for an on-disk output, copying it into--out-dirand returning its path._assert_download_urlstill rejects non-http(s)URLs on the remote/download path — tests cover both the local-copy branch and a rejected non-http URL (ftp://, remote-hostfile://,gopher://,data:) end-to-end through the loop..webp,.mp4, …), not hardcoded.png.Testing
TestLocalOutputCopy(bare path copy, extension preservation,file://URI, missing-source error) andTestSSRFGuardIntact(guard still rejects non-http; remote-hostfile://not treated as local; non-http rejected end-to-end through the loop).pytest tests/comfy_cli/command/→ 1066 passed, 1 skipped.ruff check+ruff format --checkclean.Judgment calls
Path(url).is_absolute()(platform-aware for POSIX/…and WindowsC:\…) after ruling out thefile://scheme. Network URLs are never absolute paths, so they always fall through to the SSRF guard. A protocol-relative//host/pathwould be treated as a local read (not a network fetch) and simply failis_file()with a clean error — not an SSRF vector, and not a form the producer emits.files[].urlfor a local output is the bare source path (provenance); the transfer schema typesurlas an unconstrained string, so this validates.🤖 Generated with Claude Code