prevent path traversal and enable global input validation#710
prevent path traversal and enable global input validation#710hitakshiA wants to merge 1 commit intotekdi:mainfrom
Conversation
WalkthroughThe pull request adds two security-focused enhancements: path traversal attack prevention in the file upload endpoint via filename sanitization with Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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.
🧹 Nitpick comments (1)
src/app.controller.ts (1)
32-36: Effective path traversal mitigation.Using
path.basename()to strip directory components and comparing against the original input is a solid defense-in-depth approach against path traversal attacks like../../etc/passwd.Minor consideration: inputs like
"."or".."would pass the check sincepath.basename("..")returns"..". While Express'ssendFilewith therootoption provides additional protection, you could add explicit rejection for these edge cases:Optional hardening for edge cases
const sanitizedFileName = path.basename(fileName); -if (sanitizedFileName !== fileName) { +if (sanitizedFileName !== fileName || sanitizedFileName === '.' || sanitizedFileName === '..') { throw new BadRequestException("Invalid file name"); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app.controller.ts` around lines 32 - 36, The current check using path.basename(fileName) == fileName allows edge inputs like "." or ".." to pass; update the validation around sanitizedFileName and fileName (used in sendFile with root "./uploads") to explicitly reject "." and ".." (and optionally any empty string) before calling res.sendFile; modify the block that throws BadRequestException to also throw for these edge values so sendFile is never called with them.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/app.controller.ts`:
- Around line 32-36: The current check using path.basename(fileName) == fileName
allows edge inputs like "." or ".." to pass; update the validation around
sanitizedFileName and fileName (used in sendFile with root "./uploads") to
explicitly reject "." and ".." (and optionally any empty string) before calling
res.sendFile; modify the block that throws BadRequestException to also throw for
these edge values so sendFile is never called with them.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: da10207d-71a9-4aa7-a145-bd242a62ff61
📒 Files selected for processing (2)
src/app.controller.tssrc/main.ts



Two security fixes addressing OWASP A01 (Broken Access Control) and A04 (Insecure Design).
What changed
No new dependencies. Build passes clean.
Summary by CodeRabbit
Release Notes
Bug Fixes
New Features