fix(transfer): verify Content-Length and download atomically via .part rename (BE-2513)#494
Merged
Merged
Conversation
…t rename (BE-2513) http.client returns EOF instead of raising IncompleteRead when a Content-Length body is cut short and read in chunks, so a connection dropped mid-transfer looked like a completed download: exit 0, ok:true envelope, corrupt file. Stream to a sibling .part file, compare received bytes against the declared length, and rename into place only on success — the final path never holds a partial file. Refuse over-cap declared sizes before reading the body, and route a length-less body overrunning the cap through the structured envelope instead of an uncaught ValueError. Signed-off-by: Alexander Piskun <bigcat88@icloud.com>
dosubot
Bot
added
size:M
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds a shared 10 GB download cap, streams local and HTTP downloads through exclusive ChangesDownload size limits and streaming integrity
Sequence Diagram(s)sequenceDiagram
participant Client
participant TransferCommand
participant HTTPResponse
participant FileSystem
Client->>TransferCommand: execute_download
TransferCommand->>HTTPResponse: open(..., timeout)
HTTPResponse-->>TransferCommand: headers + body chunks
TransferCommand->>TransferCommand: _declared_content_length check
alt declared size over cap
TransferCommand-->>Client: download_failed
else stream allowed
TransferCommand->>FileSystem: write to .part file
HTTPResponse-->>TransferCommand: read() chunks
alt bytes exceed cap or truncation detected
TransferCommand->>FileSystem: unlink .part file
TransferCommand-->>Client: download_failed
else complete transfer
TransferCommand->>FileSystem: rename .part to final path
TransferCommand-->>Client: success
end
end
A safer download path, neat and tidy—no half-finished files slipping by. 🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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 @bigcat88.
Found 6 finding(s).
| Severity | Count |
|---|---|
| 🟠 High | 2 |
| 🟡 Medium | 2 |
| 🟢 Low | 2 |
Panel: 6/8 reviewers contributed findings.
Reviewers that did not contribute: kimi-k2.5:adversarial (empty), kimi-k2.5:edge-case (empty)
A deterministic <name>.part sibling was unconditionally unlinked and
then reopened with open("wb") after a full HTTP round-trip: a symlink
planted in that window redirected the write to an arbitrary file, a
legitimate pre-existing .part file was silently destroyed, and two
concurrent downloads of the same job clobbered each other's temp file.
mkstemp's O_EXCL random name closes all three; the 0600 mode is widened
to the process umask so downloaded files keep their previous
permissions.
Signed-off-by: Alexander Piskun <bigcat88@icloud.com>
…stem errors The download block caught only urllib.error.HTTPError, so a connection refused/DNS/TLS failure (URLError), a timeout or reset mid-read, or an OSError from the temp-file create/write/rename escaped as a raw traceback, breaking the machine-mode/NDJSON envelope contract. Catch OSError — the common ancestor of all of those — after the HTTPError arm and emit the structured envelope with the underlying reason. Signed-off-by: Alexander Piskun <bigcat88@icloud.com>
The download opener passed no timeout, so a server that accepts the connection but never sends (or stalls mid-body) hung the command forever. Pass the house 30-second per-socket-op timeout; it bounds connect and each read, so steadily-flowing bodies of any size are unaffected while a stalled transfer aborts into the download_failed envelope. Signed-off-by: Alexander Piskun <bigcat88@icloud.com>
http.client clips reads on a plain Content-Length body, but ignores the header entirely when the response is chunked — a malicious chunked+Content-Length pairing could stream up to the 10 GB cap to disk before the post-loop byte-count check fired. Abort inside the read loop as soon as the received total passes the declared size. Signed-off-by: Alexander Piskun <bigcat88@icloud.com>
The local-copy branch checked the cap only against a pre-copy stat(); shutil.copyfile then read to EOF unbounded, so a source that grows mid-copy or a regular pseudo-file that under-reports st_size could exhaust the disk. Copy through the same exclusive temp-file machinery as the HTTP branch, counting bytes against the cap while reading and renaming into place on success, so the destination never holds a partial copy either. Signed-off-by: Alexander Piskun <bigcat88@icloud.com>
_copy_local_output_capped opened the source before wrapping the mkstemp fd, so a source unlinked between the caller's stat() and the copy (TOCTOU) raised in open(src) with the fd never wrapped or closed — a descriptor leak that exhausts the fd table across many outputs. The temp helper now returns an already-open file object (the raw fd never crosses back to a caller) and cleans up the fd plus the on-disk temp if fchmod fails; the copy enters that file's context before opening the source so an open(src) failure still closes it. The umask is read once at import instead of per file, closing the brief process-wide umask=0 window, and the stream/copy chunk grows to 1 MiB to cut syscalls on large outputs. Signed-off-by: Alexander Piskun <bigcat88@icloud.com>
A Content-Length body cut short returns a silent EOF (handled by the byte-count check), but a truncated chunked body raises http.client.IncompleteRead — an HTTPException, not an OSError — which escaped both except arms and printed a raw traceback with nothing on stdout, the exact machine-mode break this series exists to prevent. Broaden the download failure arm to (OSError, http.client.HTTPException). Signed-off-by: Alexander Piskun <bigcat88@icloud.com>
Three machine-mode robustness gaps in the download error paths:
- the truncation envelope reported the server-declared length under
expected_bytes while the over-length and over-cap envelopes used
declared_bytes; standardize on declared_bytes so a consumer reads one
key for one concept.
- a folded duplicate Content-Length ("123, 123" from a misconfigured
proxy) made int() raise, which _declared_content_length swallowed as
None and silently disabled truncation/cap verification; now a folded
value is accepted when its parts agree and treated as absent otherwise.
- a bare TimeoutError/IncompleteRead stringifies to "", which would emit
a reason-less envelope; fall back to the exception type name.
Signed-off-by: Alexander Piskun <bigcat88@icloud.com>
_FakeResp returned the whole body on a single read regardless of n, so the streaming loop, the running byte total, and the over-length/cap aborts were only ever exercised with one read — and the over-length abort was reached via a plain Content-Length body over-delivering, which a real clipped response cannot do. Make read(n) honour n and advance through the body, add a chunk_size to force multi-read streaming that models genuine chunked over-delivery, and assert the aborts fire mid-stream plus a large multi-chunk body reassembles byte-exact. Signed-off-by: Alexander Piskun <bigcat88@icloud.com>
dosubot
Bot
added
size:L
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/comfy_cli/command/test_transfer_download.py`:
- Around line 622-626: The test test_fchmod_failure_closes_fd_and_removes_temp
should be skipped on platforms that do not provide os.fchmod, since
monkeypatch.setattr on os.fchmod will fail during setup before the cleanup
behavior can be asserted. Add the same POSIX-only guard used by the neighboring
transfer download tests so the _open_part_file failure path is only exercised
where os.fchmod exists.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 2e2b518d-bc28-49e1-9af0-31a36a63989a
📒 Files selected for processing (2)
comfy_cli/command/transfer.pytests/comfy_cli/command/test_transfer_download.py
monkeypatch.setattr(os, "fchmod", ...) raises AttributeError during setup on Windows (os.fchmod is POSIX-only), so the test errored there instead of skipping. _open_part_file only calls fchmod under hasattr(os, "fchmod") anyway, so guard the test with the same condition. Signed-off-by: Alexander Piskun <bigcat88@icloud.com>
skishore23
approved these changes
Jul 10, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Content-Lengthbody cut short mid-transfer was treated as a completed download —http.clientreturns EOF instead of raisingIncompleteReadfor chunked reads of a Content-Length response — socomfy downloadexited 0 with anok:trueenvelope and a corrupt file on disk..partsibling (tempfile.mkstemp, O_EXCL) and rename it into place only after the received byte count matches the declaredContent-Length; on mismatch it emits adownload_failedenvelope withdeclared_bytes/received_bytesand exits 1, leaving nothing in the out-dir..part/concurrent-download clobber that a deterministic.partname would have; a bystander.partand parallel runs are now safe.http.client.IncompleteRead, which is not anOSError) all surface as adownload_failedenvelope instead of a raw traceback, preserving the machine-mode/NDJSON contract._MAX_DOWNLOAD_BYTES, refused up-front from the declared size, enforced mid-stream (so a chunked body can't stream to the cap past a small declared length), and applied to local-output copies by streaming through the same capped temp-file path instead of an unboundedshutil.copyfile.fchmodor a source unlinked mid-copy (TOCTOU) can't leak a file descriptor or orphan a.part; the umask is read once at import, closing a brief process-wideumask=0window._declared_content_lengthdegrades a folded/duplicate or unparseableContent-Lengthto "unknown" instead of lettingint()raise and silently skip verification..partsafety, multi-chunk reassembly, and Content-Length parsing.