Fix path traversal in file deletion (CVE-2026-45230)#136
Conversation
The /api/delete-file endpoint and deleteAssetFileAsync() resolved
user-supplied paths with path.join() without verifying the result stayed
inside the base directory. A request like /../../../tmp/x could reach
any file the Node process could touch.
Introduce resolveDataFilePath() that:
- resolves against DATA_DIR via path.resolve (which normalizes ../)
- rejects anything that doesn't stay under DATA_DIR
- additionally restricts to the legitimate upload subdirectories
(Images/, Receipts/, Manuals/) so Assets.json, SubAssets.json,
config.json and other in-DATA_DIR files can never be targeted
deleteAssetFileAsync() now skips silently (with a [SECURITY] log) when
the path is rejected, matching the existing ENOENT-tolerant behavior
used by PUT /api/assets/:id and PUT /api/subassets/:id filesToDelete.
/api/delete-file returns 400 on rejection. As a side effect the endpoint
is also realigned with the on-disk layout (uploads live under DATA_DIR,
not __dirname) which the previous implementation got wrong; no internal
client used it so no callers break.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughThis PR adds a centralized ChangesFile Deletion Path Traversal Hardening
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 skipped: no ESLint configuration detected in root package.json. To enable, add 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 |
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Hardens two file-deletion code paths against path traversal by introducing a shared resolveDataFilePath helper that confines deletions to a fixed allow-list of subdirectories under DATA_DIR.
Changes:
- Adds
ALLOWED_ASSET_FILE_SUBDIRSandresolveDataFilePath()helper to validate/resolve incoming asset paths. - Updates
deleteAssetFileAsyncto use the helper and silently skip on rejection (with a security log). - Updates
POST /api/delete-fileto use the helper and return 400 on rejection.
| const { path: filePath } = req.body; | ||
| if (!filePath) return res.status(400).json({ error: 'No file path provided' }); | ||
| const absPath = path.join(__dirname, filePath.startsWith('/') ? filePath.substring(1) : filePath); | ||
| const absPath = resolveDataFilePath(filePath); |
|
|
||
| function resolveDataFilePath(filePath) { | ||
| if (typeof filePath !== 'string') return null; | ||
| const cleanPath = filePath.replace(/^\/+/, ''); |
|
|
||
| function resolveDataFilePath(filePath) { | ||
| if (typeof filePath !== 'string') return null; | ||
| const cleanPath = filePath.replace(/^\/+/, ''); | ||
| if (!cleanPath) return null; | ||
| const resolved = path.resolve(DATA_DIR, cleanPath); | ||
| const dataDirPrefix = DATA_DIR + path.sep; | ||
| if (resolved !== DATA_DIR && !resolved.startsWith(dataDirPrefix)) return null; | ||
| const relative = path.relative(DATA_DIR, resolved); |
| console.warn(`[SECURITY] Path traversal blocked in deleteAssetFileAsync: ${filePath}`); | ||
| return resolve(); |
|
|
||
| // Subdirectories under DATA_DIR that legitimately hold uploaded asset files. | ||
| // Any path resolved outside one of these is rejected to prevent path traversal. | ||
| const ALLOWED_ASSET_FILE_SUBDIRS = ['Images', 'Receipts', 'Manuals']; |
The /api/delete-file endpoint and deleteAssetFileAsync() resolved user-supplied paths with path.join() without verifying the result stayed inside the base directory. A request like /../../../tmp/x could reach any file the Node process could touch.
Introduce resolveDataFilePath() that:
deleteAssetFileAsync() now skips silently (with a [SECURITY] log) when the path is rejected, matching the existing ENOENT-tolerant behavior used by PUT /api/assets/:id and PUT /api/subassets/:id filesToDelete.
/api/delete-file returns 400 on rejection. As a side effect the endpoint is also realigned with the on-disk layout (uploads live under DATA_DIR, not __dirname) which the previous implementation got wrong; no internal client used it so no callers break.
High-level PR Summary
This PR fixes a critical path traversal vulnerability (CVE-2026-45230) in the file deletion functionality. The previous implementation used
path.join()without validation, allowing attackers to delete arbitrary files outside the intended directories using paths like/../../../tmp/x. The fix introduces a newresolveDataFilePath()function that resolves paths againstDATA_DIR, normalizes path traversal attempts, and restricts access to only legitimate upload subdirectories (Images/,Receipts/,Manuals/). Both thedeleteAssetFileAsync()function and the/api/delete-fileendpoint now use this validation, with the former logging security warnings and continuing silently (matching existing error-tolerant behavior) while the latter returns a 400 error for invalid paths.⏱️ Estimated Review Time: 15-30 minutes
💡 Review Order Suggestion
server.jsSummary by CodeRabbit
Bug Fixes