Fix Memory Exhaustion and DoS for PDF uploads (#1549) - #1550
Conversation
|
Warning Review limit reached
Next review available in: 19 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughResume and notes PDF uploads now use temporary disk storage. Validation accepts PDF files from disk, controllers read and Base64-encode file contents, and cleanup runs after processing or validation failure. ChangesPDF upload processing
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant uploadMiddleware
participant Controller
participant Filesystem
participant Gemini
Client->>uploadMiddleware: Upload PDF
uploadMiddleware->>Filesystem: Store temporary PDF
uploadMiddleware->>Controller: Pass temporary file path
Controller->>Filesystem: Read PDF
Filesystem-->>Controller: Return PDF buffer
Controller->>Gemini: Send Base64 PDF content
Controller->>Filesystem: Delete temporary PDF
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
backend/middlewares/uploadMiddleware.js (1)
161-166: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winRemove the unreachable Word MIME types from
validateResumeMagicBytes.
resumeFileFilterrejects Word uploads before the validator runs, so accepted files are consistently stored and sent to Gemini as PDFs. The current Word allowlist and error message are misleading.🤖 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 `@backend/middlewares/uploadMiddleware.js` around lines 161 - 166, Update validateResumeMagicBytes to remove the unreachable Word MIME types from its allowlist and revise its error message to describe only supported PDF files; leave resumeFileFilter and the PDF upload flow unchanged.
🤖 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 `@backend/controllers/notesSummaryController.js`:
- Around line 244-247: Extend temporary-file cleanup beyond the controller
finally blocks so files are removed when validation middleware short-circuits:
update backend/controllers/notesSummaryController.js lines 244-247 around
summarizeNotes/validateSummarizeNotes, and
backend/controllers/resumeController.js lines 169-172 around
analyzeResume/validateResumeMagicBytes/validateAnalyzeResume. Ensure each
uploaded temporary file is deleted exactly once across both validation-error and
normal post-upload paths, preserving the existing error handling for unlink
failures.
- Around line 114-116: Bound PDF memory usage in notesSummaryController.js
(lines 114-116) and resumeController.js (lines 110-112 and 141) by using Gemini
file-based input or acquiring a shared byte/concurrency budget before each
fs.promises.readFile call and releasing it in finally; ensure the full Buffer
and base64 representations are not simultaneously retained without this bound.
In `@backend/middlewares/uploadMiddleware.js`:
- Around line 161-166: Update the filename callbacks in both upload storage
instances, including uploadResume, to generate a server-side unique temporary
filename for every upload rather than combining Date.now() with
sanitizeBase(file.originalname). Keep file.originalname available only as
metadata and preserve the .pdf extension.
- Around line 161-166: Update the upload storage configuration around
uploadResume and the corresponding notes Multer instance so resumes and notes
are stored outside the directory exposed by express.static; keep profile images
in a separately served public location. Ensure rejected uploads are deleted
during validation, and make cleanup failures propagate or otherwise receive
explicit handling beyond logging.
---
Nitpick comments:
In `@backend/middlewares/uploadMiddleware.js`:
- Around line 161-166: Update validateResumeMagicBytes to remove the unreachable
Word MIME types from its allowlist and revise its error message to describe only
supported PDF files; leave resumeFileFilter and the PDF upload flow unchanged.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 4e110586-4bca-498a-bc1c-c4f9d9382df1
📒 Files selected for processing (3)
backend/controllers/notesSummaryController.jsbackend/controllers/resumeController.jsbackend/middlewares/uploadMiddleware.js
| } finally { | ||
| if (uploadedFilePath) { | ||
| fs.promises.unlink(uploadedFilePath).catch(err => console.error("Failed to delete temp notes PDF:", err)); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Make temporary-file cleanup cover the complete post-upload lifecycle.
The controller finally blocks do not run when earlier route middleware returns a validation error. Those files remain on disk and can create a disk-exhaustion DoS.
backend/controllers/notesSummaryController.js#L244-L247: clean the file whenvalidateSummarizeNotesshort-circuits beforesummarizeNotes.backend/controllers/resumeController.js#L169-L172: clean the file whenvalidateResumeMagicBytesorvalidateAnalyzeResumeshort-circuits beforeanalyzeResume.
📍 Affects 2 files
backend/controllers/notesSummaryController.js#L244-L247(this comment)backend/controllers/resumeController.js#L169-L172
🤖 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 `@backend/controllers/notesSummaryController.js` around lines 244 - 247, Extend
temporary-file cleanup beyond the controller finally blocks so files are removed
when validation middleware short-circuits: update
backend/controllers/notesSummaryController.js lines 244-247 around
summarizeNotes/validateSummarizeNotes, and
backend/controllers/resumeController.js lines 169-172 around
analyzeResume/validateResumeMagicBytes/validateAnalyzeResume. Ensure each
uploaded temporary file is deleted exactly once across both validation-error and
normal post-upload paths, preserving the existing error handling for unlink
failures.
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/controllers/resumeController.js (1)
172-175: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftAwait and harden temporary-file cleanup.
unlink()is started but not awaited. The request can complete before deletion finishes, and deletion failures only produce a log entry. Persistent failures can accumulate PDFs in temporary storage and exhaust disk space. Await cleanup infinally, and add bounded retries or a TTL-based janitor for files that remain.🤖 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 `@backend/controllers/resumeController.js` around lines 172 - 175, Update the finally cleanup in the resume controller to await deletion of uploadedFilePath before the request completes, and add bounded retry handling or an existing TTL-based janitor for files that cannot be removed. Preserve cleanup only when uploadedFilePath is present, and ensure persistent failures remain visible without leaving unbounded temporary files.
♻️ Duplicate comments (1)
backend/controllers/notesSummaryController.js (1)
114-116:⚠️ Potential issue | 🟠 MajorBound active PDF-processing memory, not only upload memory.
Line 116 still reads the complete PDF into memory. Lines 145-147 then retain a second Base64 representation before releasing
buffer. At the 15 MiB limit, the two representations use roughly 35 MiB per active request before parser and Gemini overhead. Unless a separate global budget exists, the per-IP limiter does not bound concurrent memory usage. Use Gemini file-based input or a shared byte/concurrency budget aroundreadFile.#!/bin/bash set -euo pipefail rg -n -C 8 \ 'aiLimiter|readFile|inlineData|base64|Semaphore|p-limit|concurr|active|budget' \ backend --glob '*.js'Also applies to: 145-147, 177-177
🤖 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 `@backend/controllers/notesSummaryController.js` around lines 114 - 116, Change the PDF processing flow in the notes summary controller around the upload read and subsequent Base64/inlineData handling so it does not retain multiple full-file representations per request. Prefer Gemini file-based input; otherwise wrap readFile and related processing in an existing shared byte/concurrency budget, ensuring the budget covers the full active processing interval and is released on every path.
🧹 Nitpick comments (1)
backend/Input_validators/ValidateNotesSummary.js (1)
31-31: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winRemove the unreachable uploaded-file cleanup checks.
Both checks run only after the code has established that
req.fileis absent. They cannot delete a temporary upload.
backend/Input_validators/ValidateNotesSummary.js#L31-L31: remove the dead check from the missing-source branch.backend/Input_validators/ValidateResume.js#L53-L53: remove the dead check from the no-resume branch.🤖 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 `@backend/Input_validators/ValidateNotesSummary.js` at line 31, Remove the unreachable uploaded-file cleanup checks from the missing-source branch in backend/Input_validators/ValidateNotesSummary.js (line 31) and the no-resume branch in backend/Input_validators/ValidateResume.js (line 53). No other behavior in these validation branches needs to change.
🤖 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 `@backend/controllers/resumeController.js`:
- Around line 111-116: Update uploadResume so concurrent resume analyses are
bounded before fs.promises.readFile() allocates the file Buffer and Base64
string; use supported file-backed or streaming Gemini input when available,
otherwise acquire an in-flight concurrency slot before readFile() and release it
in all success and error paths. Do not rely on aiLimiter, which only limits
hourly request volume.
In `@backend/Input_validators/ValidateNotesSummary.js`:
- Line 39: Update the temporary-file cleanup in ValidateNotesSummary.js:39-39
and ValidateResume.js:58-58 to ignore unlink failures only when the error code
is ENOENT, while logging or reporting all other errors. Apply the same handling
to both handlers and preserve cleanup execution when req.file.path is present.
In `@backend/middlewares/uploadMiddleware.js`:
- Line 137: Update the temporary-file cleanup calls in the upload middleware at
both `req.file.path` deletion sites to record unlink failures instead of
silently discarding them. Preserve the existing response status and cleanup
flow, but log or otherwise report the caught error through the middleware’s
established observability mechanism.
- Around line 4-6: Replace the CommonJS file-type destructuring in
uploadMiddleware with a dynamic import("file-type"), then update every call site
using fileTypeFromBuffer or fileTypeFromFile to access the dynamically loaded
module’s exports while preserving their existing behavior.
---
Outside diff comments:
In `@backend/controllers/resumeController.js`:
- Around line 172-175: Update the finally cleanup in the resume controller to
await deletion of uploadedFilePath before the request completes, and add bounded
retry handling or an existing TTL-based janitor for files that cannot be
removed. Preserve cleanup only when uploadedFilePath is present, and ensure
persistent failures remain visible without leaving unbounded temporary files.
---
Duplicate comments:
In `@backend/controllers/notesSummaryController.js`:
- Around line 114-116: Change the PDF processing flow in the notes summary
controller around the upload read and subsequent Base64/inlineData handling so
it does not retain multiple full-file representations per request. Prefer Gemini
file-based input; otherwise wrap readFile and related processing in an existing
shared byte/concurrency budget, ensuring the budget covers the full active
processing interval and is released on every path.
---
Nitpick comments:
In `@backend/Input_validators/ValidateNotesSummary.js`:
- Line 31: Remove the unreachable uploaded-file cleanup checks from the
missing-source branch in backend/Input_validators/ValidateNotesSummary.js (line
31) and the no-resume branch in backend/Input_validators/ValidateResume.js (line
53). No other behavior in these validation branches needs to change.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: b48ae651-9cc9-417f-a4b1-255be52aec48
📒 Files selected for processing (5)
backend/Input_validators/ValidateNotesSummary.jsbackend/Input_validators/ValidateResume.jsbackend/controllers/notesSummaryController.jsbackend/controllers/resumeController.jsbackend/middlewares/uploadMiddleware.js
| let fileBuffer = await fs.promises.readFile(uploadedFilePath); | ||
|
|
||
| const targetRole = req.body.targetRole || "General Professional"; | ||
|
|
||
| const base64Data = fileBuffer.toString("base64"); | ||
| fileBuffer = null; // Release Buffer memory |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 6 'readFile|inlineData|uploadFile|fileSize|uploadResume|uploadNotes' backendRepository: Canopus-Labs/PrepPilot
Length of output: 15047
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- upload limits and resume route ---'
sed -n '1,210p' backend/middlewares/uploadMiddleware.js
sed -n '1,80p' backend/routes/resumeRoutes.js
printf '%s\n' '--- resume controller and fallback helper references ---'
sed -n '90,165p' backend/controllers/resumeController.js
rg -n -C 8 'function generateWithFallback|const generateWithFallback|generateWithFallback|aiLimiter' backendRepository: Canopus-Labs/PrepPilot
Length of output: 38065
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- fallback implementation ---'
sed -n '1,120p' backend/utils/geminiHelper.js
printf '%s\n' '--- controller cleanup and response path ---'
sed -n '150,245p' backend/controllers/resumeController.js
printf '%s\n' '--- exact memory multipliers at the configured resume limit ---'
python3 - <<'PY'
size = 5 * 1024 * 1024
base64_size = 4 * ((size + 2) // 3)
print({
"resume_limit_bytes": size,
"base64_bytes": base64_size,
"peak_buffer_plus_base64_bytes": size + base64_size,
"peak_buffer_plus_base64_mib": (size + base64_size) / (1024 * 1024),
"peak_for_15_concurrent_uploads_mib": 15 * (size + base64_size) / (1024 * 1024),
})
PYRepository: Canopus-Labs/PrepPilot
Length of output: 7990
Bound concurrent resume analyses before readFile().
uploadResume limits resumes to 5 MiB, not 15 MiB. Each active request can retain about 11.67 MiB for the Buffer and Base64 string, so 15 concurrent requests can retain about 175 MiB before SDK overhead. aiLimiter limits requests per hour, not in-flight analyses. Use file-backed or streaming Gemini input when supported; otherwise add a concurrency limit before readFile().
🤖 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 `@backend/controllers/resumeController.js` around lines 111 - 116, Update
uploadResume so concurrent resume analyses are bounded before
fs.promises.readFile() allocates the file Buffer and Base64 string; use
supported file-backed or streaming Gemini input when available, otherwise
acquire an in-flight concurrency slot before readFile() and release it in all
success and error paths. Do not rely on aiLimiter, which only limits hourly
request volume.
…ean validation paths, dynamic file-type import
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 `@backend/Input_validators/ValidateNotesSummary.js`:
- Line 40: Make validateSummarizeNotes in
backend/Input_validators/ValidateNotesSummary.js asynchronous and await unlink
cleanup before calling handleValidationError. Apply the same change to
validateAnalyzeResume at backend/Input_validators/ValidateResume.js, ensuring
each validation response is sent only after deletion settles.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: e2f85e59-b9b3-4cde-b170-fa4cd3a659c1
📒 Files selected for processing (5)
backend/Input_validators/ValidateNotesSummary.jsbackend/Input_validators/ValidateResume.jsbackend/controllers/notesSummaryController.jsbackend/controllers/resumeController.jsbackend/middlewares/uploadMiddleware.js
🚧 Files skipped from review as they are similar to previous changes (3)
- backend/controllers/notesSummaryController.js
- backend/controllers/resumeController.js
- backend/middlewares/uploadMiddleware.js
2 participants
📝 Pull Request Description
Related Issue
Closes #1549
Summary
This PR fixes a critical Denial of Service (DoS) vulnerability caused by memory exhaustion during large PDF uploads.
The
uploadNotesanduploadResumemulter middlewares were previously configured to usememoryStorage(), which buffered the entire file into Node.js RAM before reaching the route controllers. This allowed attackers to crash the server by sending multiple concurrent 15MB file uploads, quickly exceeding the Node.js heap limit.We have migrated these upload handlers to use
diskStorage()instead. The files are now streamed to anuploads/directory during the request, keeping the memory footprint low and predictable. The route controllers (notesSummaryControllerandresumeController) have been updated to securely read the file from disk, process it with the Gemini API, and clean up (delete) the temporary file immediately afterward in afinallyblock to prevent disk space leaks.Type of Change
How Has This Been Tested?
uploads/directory are successfully deleted after processing completes, regardless of success or failure.Screenshots (if applicable)
N/A
Checklist
Summary
Replaces Multer memory storage with disk storage for PDF uploads. This prevents large concurrent uploads from exhausting Node.js memory.
The controllers process uploaded PDFs from disk and delete temporary files after successful or failed processing.
Image uploads continue to use memory storage.