Fix directory downloads failing with "object not found" - #59
Conversation
Co-authored-by: dentifrag <65632734+dentifrag@users.noreply.github.com>
Co-authored-by: dentifrag <65632734+dentifrag@users.noreply.github.com>
Co-authored-by: dentifrag <65632734+dentifrag@users.noreply.github.com>
Co-authored-by: dentifrag <65632734+dentifrag@users.noreply.github.com>
There was a problem hiding this comment.
🟡 Changes recommended
The new relativePath slicing can allow .. segments or absolute paths in entry.Path to escape the intended destination subdirectory unless additional validation/sanitization is added before enqueuing transfers.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Fixes folder (directory) downloads failing with “object not found” by correcting how rclone list results are converted into copy jobs, normalizing directory paths before listing, and adding a fail-closed containment guard. This aligns expandDownload’s assumptions with rclone’s operations/list output shape and prevents silent no-op downloads for malformed directory paths.
Changes:
- Adjusted
expandDownloadto usesrcFs: <jobRemote>:with fullentry.PathassrcRemote, while stripping the directory prefix only for destination layout. - Normalized
dirPathbefore callinglistRecursiveFiles, fixing redundant/trailing/leading separator cases. - Updated tests to match rclone’s real returned
Pathshape and added coverage for normalization + containment guard behavior.
File summaries
| File | Description |
|---|---|
src/server/routes/downloads.ts |
Fixes directory expansion logic (src/dst pairing, normalization, containment guard). |
test/downloadRoute.test.ts |
Updates mocks to rclone-shaped paths and adds new test cases for normalization and containment rejection. |
CHANGELOG.md |
Documents the folder download fix and the redundant-separator no-op behavior. |
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
🟢 Approval recommended
The changes align with the described rclone behavior, add fail-closed safety checks, and include targeted test coverage for the regression and key edge cases.
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 0 new
- Review effort level: Lite
What this fixes
Directory downloads were completely broken. Every file in a folder download failed.
client.listRecursiveFiles()wraps rclone'soperations/list. Its returnedentry.Pathis relative to thefsroot and includes the listedremoteprefix:expandDownloadassumed the opposite, thatPathwas relative to the listed directory. It pairedsrcFs: "<job>:media/movies"withsrcRemote: "media/movies/a.mkv", doubling the prefix. Verified against rclone v1.74.4,operations/copyfilewith that pairing returns:{"error": "object not found"}downloadManagerturns that rejection intostatus: 'error', so users saw every file in a folder download fail. Only a root-level download (dirPath === '') worked, by accident.The fix
srcFsand keep the fullentry.PathassrcRemote, so the pair resolves. Strip the prefix only when building the destination path, preserving the on-disk layout.media//moviespreviously hit thefiles.length === 0early return and silently downloaded nothing.Normalizing before the listing call also removes a latent trap: rclone echoes a leading slash back in
Path, butfilter(Boolean)strips it from the prefix, so a/-prefixeddirPathwould have false-rejected legitimate entries.Tests
test/downloadRoute.test.tspreviously mockedlistRecursiveFileswith directory-relative paths (a.mkv), encoding the wrong assumption. That is why the bug survived. The mocks now use real rclone shape (media/movies/a.mkv), plus new cases for the containment guard and for redundant, trailing, and leading separators.Verification
Probed rclone v1.74.4 directly rather than relying on the mocks:
remotepassed tooperations/listmedia//movies(pre-fix, raw)media/movies(post-fix, normalized)operations/copyfilewith the newsrcFs/srcRemotepairing succeeds and lands the file at the expectedout/movies/a.mkv.Full suite green: typecheck, lint, format:check, 189 tests, build.