fix: repair failing test baseline (path validation, 404 vs 403, test isolation) - #82
fix: repair failing test baseline (path validation, 404 vs 403, test isolation)#82greirson wants to merge 1 commit into
Conversation
…tion) The committed suite had 18 deterministic failures plus latent flakiness on main. Root causes and fixes: - isPathWithinUploadDir rejected valid in-bounds paths when the upload dir is reached through a symlink (macOS /var, Docker bind mounts): the non-existent branch used path.resolve while the upload dir was realpath'd. Added realpathAllowingMissing to resolve symlinks in the deepest existing ancestor, falling back gracefully on ENOTDIR/EACCES/ELOOP instead of throwing. - files.js info/download/delete/rename returned 403 "path traversal" for missing but in-bounds paths. Switched the bounds check to requireExists=false so they return 404; traversal attempts are still rejected with 403. - auth.test.js set the PIN via the wrong env var (PIN, not DUMBDROP_PIN) and too late (after config froze); fixed, and corrected the empty-PIN expectation to 401 (the route's actual response). - Corrected outdated test expectations: hyphen-to-underscore sanitization and the batch-ID format contract. - Isolated each suite's UPLOAD_DIR to a unique per-process temp dir, removing cross-suite races on ./local_uploads (parallel-safe) and stopping tests from polluting the dev upload directory; pre-clean in before() guards pid reuse. - Fixed 2 pre-existing no-unused-vars lint errors in the touched file. Result: npm test 67/67 green, deterministic across parallel runs with and without a local .env; npm run lint clean. Route behavior (404 vs 403, traversal still 403) and the upload flow verified in a browser.
|
The preview deployment for DumbWareio/DumbDrop-main is ready. 🟢 Open Preview | Open Build Logs | Open Application Logs Last updated at: 2026-06-13 03:56:21 CET |
|
The preview deployment for DumbWareio/DumbDrop-demo is ready. 🟢 Open Preview | Open Build Logs | Open Application Logs Last updated at: 2026-06-13 03:56:22 CET |
WalkthroughThe PR refactors path traversal defense by introducing symlink-aware resolution for non-existent targets, updating the traversal guard to validate only path inclusion (not existence), and switching four route endpoints to this "guard-only" mode. Test suites are refactored to use isolated temp directories for parallel safety with updated assertions. ChangesPath Traversal and Route Behavior Refactoring
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install failed due to a network error. 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/routes/files.js (1)
86-103: ⚡ Quick winAcknowledged TOCTOU gap — reminder for follow-up hardening.
Look, I know you said this is "out of scope," but let me spell it out so it doesn't get lost in the shuffle: the traversal check resolves symlinks via
realpathAllowingMissing, but thenfs.accessandcreateReadStreamoperate on the originalfilePath. A race condition exists where a symlink could be created between validation and file access, pointing outside the upload directory.You've already noted this in the PR objectives for a separate hardening pass. Just making sure nobody forgets. The fix would be to use the resolved path for all subsequent filesystem operations, not just the validation.
🤖 Prompt for 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. In `@src/routes/files.js` around lines 86 - 103, The traversal check uses a resolved path helper but subsequent filesystem ops still use the original filePath, leaving a TOCTOU via symlink changes; fix by obtaining the resolved path (via realpathAllowingMissing or the same resolution used inside isPathWithinUploadDir) and then use that resolved path for fs.access and for require('fs').createReadStream (and any other fs operations) instead of the original filePath so the validation and access operate on the same canonical target.
🤖 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.
Nitpick comments:
In `@src/routes/files.js`:
- Around line 86-103: The traversal check uses a resolved path helper but
subsequent filesystem ops still use the original filePath, leaving a TOCTOU via
symlink changes; fix by obtaining the resolved path (via realpathAllowingMissing
or the same resolution used inside isPathWithinUploadDir) and then use that
resolved path for fs.access and for require('fs').createReadStream (and any
other fs operations) instead of the original filePath so the validation and
access operate on the same canonical target.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 888f7833-6fbd-438c-a6a7-08fabc112617
📒 Files selected for processing (7)
src/routes/files.jssrc/utils/fileUtils.jstest/auth.test.jstest/files.test.jstest/path-validation.test.jstest/security.test.jstest/upload.test.js
1 participant
Summary
On
main,npm testwas not green: 65 tests, 47 pass, 18 fail, plus latentparallel-run flakiness. Two of the failures turned out to be real production
bugs, not test noise. This PR fixes the bugs, corrects the outdated test
expectations, and makes the suite deterministic.
Result:
npm testis 67/67 green and deterministic (verified across repeatedparallel runs, with and without a local
.env);npm run lintis clean.Production bugs fixed
Symlinked upload directory rejected valid uploads (
src/utils/fileUtils.js)isPathWithinUploadDirresolved the upload dir withfs.realpathSyncbutresolved a non-existent candidate path with only
path.resolve. When theupload dir is reached through a symlink (macOS
/var->/private/var, andnotably Docker bind mounts), the two sides disagreed and
path.relativeproduced a
..prefix, so valid in-bounds paths were wrongly rejected. AddedrealpathAllowingMissing, which resolves symlinks in the deepest existingancestor and re-appends the missing remainder, falling back gracefully on
ENOTDIR/EACCES/ELOOPinstead of throwing.Missing in-bounds files returned 403 instead of 404 (
src/routes/files.js)The info/download/delete/rename routes used the path check as an existence
gate (
requireExists=true) and mapped itsfalseto403 "Access denied"plus a misleading "path traversal attack" log. A simply-missing file therefore
returned 403 (and a false security alert) instead of 404. Switched these to
requireExists=falseso missing in-bounds paths fall through to the fs calland return 404. Path traversal is still rejected with 403 (verified).
Test corrections
auth.test.jsconfigured the PIN viaprocess.env.PIN, but the app readsDUMBDROP_PIN, and it was set insidebefore()after config is loaded andfrozen, so the PIN was never actually configured. Fixed, and corrected the
empty-PIN expectation to 401 (the route's actual response for an invalid PIN).
and the batch-ID format contract.
Flakiness fix
All suites shared the real
./local_uploadsdirectory and ran in parallel, withsome suites deleting the whole directory in teardown. Each suite now isolates its
UPLOAD_DIRto a unique per-process temp dir (parallel-safe), which also stopstests from polluting the dev upload directory. A pre-clean in
before()guardsagainst pid reuse.
Verification
npm test: 67/67 green, deterministic across parallel runs (with and without.env).npm run lint: clean (also fixed 2 pre-existingno-unused-varserrors in the touched file).in-bounds files now return 404; path traversal still returns 403.
Not in scope (noted for follow-up)
fs.access/createReadStreamon the unresolvedpath (a pre-existing TOCTOU window, defense-in-depth only); left for a separate
hardening pass.
High-level PR Summary
This PR fixes two production bugs and makes the test suite reliable and deterministic. The first bug caused symlinked upload directories (common in Docker bind mounts and macOS
/varpaths) to incorrectly reject valid uploads due to inconsistent symlink resolution. The second bug caused missing but in-bounds files to return 403 "Access denied" instead of the correct 404 status. Additionally, the test suite had incorrect expectations (PIN configuration, filename sanitization, batch ID format) and suffered from parallel-run flakiness due to shared upload directories. All test suites now use isolated per-process temp directories, and the suite is now 67/67 green and deterministic.⏱️ Estimated Review Time: 30-90 minutes
💡 Review Order Suggestion
test/auth.test.jstest/files.test.jstest/security.test.jstest/upload.test.jssrc/utils/fileUtils.jssrc/routes/files.jstest/path-validation.test.jsSummary by CodeRabbit