fix(downloader): allow remote downloads without a Content-Length

[dualfroz]

Fixes #133

raw.githubusercontent.com and pastebin gzip the response, Go decompresses it and reports the length as -1, so the downloader refused it. chunked responses hit the same check.

the check was there for the disk limit, so instead of dropping it Filesystem.Write now takes a negative size as unknown, writes to a temp file through the quota file and only renames it over the target once done. if it doesn't fit, the temp file goes away and the old file stays as it was. known-length downloads work like before

tests with an httptest server for gzip, chunked and over the limit

Summary by CodeRabbit

• Bug Fixes
  • Downloads now succeed when the server does not provide a file size. Progress remains at zero until the download completes in those cases.
  • Files can now be written when their final size is unknown. If a write exceeds available disk space, an existing file is preserved and incomplete new files are not left behind.

[coderabbitai]

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Advanced

Run ID: d1960583-f96d-4c90-859e-92a8cc5be92a

📥 Commits

Reviewing files that changed from the base of the PR and between 65422ff and 04487e1.

📒 Files selected for processing (4)

• router/downloader/downloader.go
• router/downloader/downloader_test.go
• server/filesystem/filesystem.go
• server/filesystem/filesystem_test.go

Included review availability: Your plan provides up to 2 included reviews per hour; 1 remains after this review.

📜 Recent review details

🔇 Additional comments (4)

server/filesystem/filesystem.go (1)

6-6: LGTM!

Also applies to: 20-20, 157-162, 191-194, 225-293

server/filesystem/filesystem_test.go (1)

300-354: LGTM!

router/downloader/downloader.go (1)

284-291: LGTM!

Also applies to: 313-314

router/downloader/downloader_test.go (1)

1-263: LGTM!

---

📝 Walkthrough

Walkthrough

The downloader now accepts responses without a positive content length. The filesystem writes unknown-size streams under disk quotas and replaces existing files only after a successful write.

Changes

Unknown-Length Downloads

Layer / File(s)	Summary
Write unknown-size files safely server/filesystem/filesystem.go, server/filesystem/filesystem_test.go	Negative write sizes use a quota-aware temporary file beside the target. The filesystem replaces the target after a successful write and removes the temporary file on failure. Tests check quota updates and preservation of existing files.
Stream responses without a known length router/downloader/downloader.go, router/downloader/downloader_test.go	The downloader streams responses with unknown or nonpositive content lengths. It tracks progress when the response length is positive. Tests cover gzip decoding, chunked responses, progress, and disk-limit failures.

Priority: ➖ Normal

Estimated code review effort: 3 (Moderate) | ~20 minutes

Change: Bug fix · Severity of issue fixed: Medium

Sequence Diagram(s)

sequenceDiagram
  participant Download as Download.Execute
  participant Filesystem as Filesystem.Write
  participant UnknownSize as writeUnknownSize
  participant QuotaFile as quota-aware file
  Download->>Filesystem: Stream response body with unknown size
  Filesystem->>UnknownSize: Route negative newSize
  UnknownSize->>QuotaFile: Copy data while checking quota
  UnknownSize->>UnknownSize: Rename completed temporary file over target

Loading

Suggested reviewers: lancepioch

Merge Risk: ⚪ Minimal · up to 04487

Unknown-length downloads appear mergeable after normal checks; no concrete issue remains that warrants holding the change.

Security Architecture Review

Security architecture risk: 🟡 Moderate · up to 04487

Unknown-length downloads remain subject to disk limits, but replacing an existing file can change its permissions. Concurrent replacements and interrupted downloads also present risks to quota accuracy and cleanup.

Retained concerns

• Medium · security · inferred: An unknown-length download replacing a restrictive existing file creates the replacement with the requested mode, currently 0644 for downloads, rather than retaining the target's permissions. Where directory access and the process umask permit, this can widen local read access.
• Medium · security · inferred: Concurrent replacements of one target can observe the same old file before either rename and each subtract that old size afterward. Depending on the replacement sizes, tracked usage can fall below actual usage, weakening subsequent quota checks. Known-length concurrent writes already had a separate accounting race; this PR adds a replacement path with that exposure.
• Low · reliability · inferred: A process interruption between temporary-file creation and replacement bypasses the returned-error cleanup path, potentially leaving quota-consuming data after restart. Whether another component removes such files is unverified.

Security review details

Security Blast Radius

• inferred — An authorized pull can write within its server filesystem. The route limits simultaneous downloads to three per server, but quota misaccounting could affect later writes against that server's limit; wider host-level effects depend on deployment.

Security Findings and Attack Paths

• inferred — A remote response without a known length can cause an authorized download to replace a restrictive file with a newly created file using mode 0644. The resulting local-read exposure depends on directory access and umask.
• inferred — Two overlapping replacements of the same target can each subtract a stale target size. An undercount can allow later quota reservations that would fail against actual usage.

Trust Boundaries and Controls

• observed — The pull route inherits authorization and requires the remote-download feature to be enabled. The downloader's connection logic rejects local and internal address ranges, while the write path resolves destinations through the filesystem's safe-path boundary.
• observed — The initial space check is permissive for a negative size, but the quota wrapper reserves growth before each write and adjusts its reservation after a short write. Unknown-length downloads therefore do not simply bypass quota enforcement.

Resilience and Maintainability Implications

• inferred — Returned failures have a cleanup path, but interruption before it runs can leave a temporary file. Periodic usage reconciliation is not itself evidence that orphaned files are removed.

Hardening Proposals

• proposed — Preserve the existing target's restrictive permissions when replacing it, including while the temporary file is being written.
• proposed — Couple target replacement to its quota adjustment under a same-target transition lock or equivalent identity-aware accounting, and define recovery ownership for interrupted temporary files.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 18.18% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 11 functions across 4 files.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and concisely describes the primary change: allowing downloads when the remote response does not provide a Content-Length.
Linked Issues check	✅ Passed	The PR addresses issue [#133]. Download.Execute no longer rejects successful responses with an unknown or nonpositive ContentLength. Unknown-length responses stream to Filesystem.Write, which wr…
Out of Scope Changes check	✅ Passed	The changed downloader and filesystem paths directly support unknown-length remote downloads. The added tests cover gzip, chunked responses, known-length behavior, disk limits, cleanup, target preserv…

• Fix all pre-merge checks with AI

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create a new PR

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{A rabbit watched the bytes flow by,
Through streams with lengths left unknown.
A quota checked each passing byte,
A safe new file took root beside,
Then hopped in place when writes were done.}

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