fix: add PDF-only file type validation to multer upload middleware#461
Open
ChetanSenta wants to merge 2 commits into
Open
fix: add PDF-only file type validation to multer upload middleware#461ChetanSenta wants to merge 2 commits into
ChetanSenta wants to merge 2 commits into
Conversation
|
Someone is attempting to deploy a commit to the firefistisdead's projects Team on Vercel. A member of the Team first needs to authorize it. |
Contributor
📝 WalkthroughWalkthroughReworks Multer to store uploads in ChangesUpload Validation & Error Handling
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
github-actions
Bot
added
backend
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@server.js`:
- Around line 449-450: The current multer.diskStorage filename function uses
Date.now() + '-' + file.originalname which can collide; update the filename
callback (the filename function in multer.diskStorage) to generate a
collision-resistant name by using a UUID or crypto.randomBytes (e.g.,
crypto.randomUUID() or random hex) combined with a timestamp and the file
extension from path.extname(file.originalname), then pass that string to
cb(null, ...) instead of Date.now() + '-' + file.originalname so concurrent
uploads cannot overwrite each other.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
server.js (2)
600-605:⚠️ Potential issue | 🟠 Major | ⚡ Quick winThe new 400 PDF handler is currently bypassed for
/upload.
/uploadalready runsmulterErrorHandlerright afterupload.single("file"), and that handler returns a 415 response for the same"Only PDF files are allowed."error. So callers still get the 415 payload, not the 400 JSON this new middleware is trying to standardize.Suggested fix
- if (err && err.message && err.message.includes("Only PDF files are allowed")) { - console.warn(`[upload] Invalid file type attempted: ${err.message}`); - return res.status(415).json({ - error: "Invalid file type", - message: "Only PDF files are allowed. Please upload a valid PDF document.", - }); - } + if (err?.message === "Only PDF files are allowed.") { + console.warn(`[upload] Invalid file type attempted: ${err.message}`); + return res.status(400).json({ error: err.message }); + }Then remove the duplicate app-level handler at Lines 1125-1129.
Also applies to: 1125-1129
🤖 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 `@server.js` around lines 600 - 605, The multer-level error handler multerErrorHandler currently sends a 415 for the "Only PDF files are allowed." message and prevents the new standardized 400 PDF middleware from running for the /upload route (which calls upload.single("file") then multerErrorHandler). Change multerErrorHandler so that when err.message includes "Only PDF files are allowed" it does not send the response but instead calls next(err) to let the app-level standardized PDF handler produce the 400 JSON; then remove the duplicate app-level 415 handler that also checks for that message (the duplicate currently returning 415) so only the standardized 400 PDF handler emits the response.
449-467:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRe-add the Multer size limit.
uploadno longer passeslimits, even though Line 391 still computesMAX_PDF_SIZE_BYTESandmulterErrorHandlerstill handlesLIMIT_FILE_SIZE. As written, an arbitrarily large PDF will be fully written touploads/instead of being rejected early.Suggested fix
-const upload = multer({ storage, fileFilter }); +const upload = multer({ + storage, + fileFilter, + limits: { + fileSize: MAX_PDF_SIZE_BYTES, + }, +});🤖 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 `@server.js` around lines 449 - 467, The multer instance created as upload no longer enforces a file size limit so large PDFs can be fully written; add the limits option when creating upload to reject oversized files early by passing limits: { fileSize: MAX_PDF_SIZE_BYTES } to multer({ storage, fileFilter, limits }) (ensure MAX_PDF_SIZE_BYTES referenced earlier is used), so multer will trigger LIMIT_FILE_SIZE and be handled by multerErrorHandler instead of writing huge files to uploads/.
🤖 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.
Outside diff comments:
In `@server.js`:
- Around line 600-605: The multer-level error handler multerErrorHandler
currently sends a 415 for the "Only PDF files are allowed." message and prevents
the new standardized 400 PDF middleware from running for the /upload route
(which calls upload.single("file") then multerErrorHandler). Change
multerErrorHandler so that when err.message includes "Only PDF files are
allowed" it does not send the response but instead calls next(err) to let the
app-level standardized PDF handler produce the 400 JSON; then remove the
duplicate app-level 415 handler that also checks for that message (the duplicate
currently returning 415) so only the standardized 400 PDF handler emits the
response.
- Around line 449-467: The multer instance created as upload no longer enforces
a file size limit so large PDFs can be fully written; add the limits option when
creating upload to reject oversized files early by passing limits: { fileSize:
MAX_PDF_SIZE_BYTES } to multer({ storage, fileFilter, limits }) (ensure
MAX_PDF_SIZE_BYTES referenced earlier is used), so multer will trigger
LIMIT_FILE_SIZE and be handled by multerErrorHandler instead of writing huge
files to uploads/.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
The /upload endpoint in server.js was using multer without any file type filter, allowing users to upload any file format (.txt, .png, .zip, .docx, etc.). These files were silently forwarded to the FastAPI RAG service which attempted to parse them as PDFs via PyPDFLoader, causing unhandled crashes and unhelpful 500 errors with no feedback to the user.
This PR adds a fileFilter to the multer configuration that validates both the MIME type (application/pdf) and file extension (.pdf) before accepting the upload. A dedicated Express error handler is also added to return a clean 400 Bad Request JSON response when an invalid file type is submitted.
Related issue
Closes #459
Testing
Manual test cases performed:
Uploaded a .txt file → received 400 with { "error": "Only PDF files are allowed." } ✅
Uploaded a .png file → received 400 with { "error": "Only PDF files are allowed." } ✅
Uploaded a .zip file → received 400 with { "error": "Only PDF files are allowed." } ✅
Uploaded a valid .pdf file → processed correctly as before ✅
Asked a question after valid upload → RAG service responded correctly ✅
Checklist:
Notes
Only server.js is modified — no changes to the RAG service, frontend, or dependencies
The fileFilter checks both mimetype AND file extension to handle edge cases where browsers misreport MIME types
This fix is a prerequisite for any future file management features (e.g. Issue #1 — delete uploaded PDFs after processing)
Security
Summary by CodeRabbit
New Features
Bug Fixes
Chores