Optimization/Architecture] Transition to AWS S3 Pre-signed URLs for Direct Client-Side File Uploads#310
Merged
Sachinchaurasiya360 merged 4 commits intoMay 21, 2026
Conversation
- Removed multer and server-side file buffering - Added /presigned-url endpoint - Created frontend utility for direct S3 PUT requests with cookie support - Refactored student, recruiter, apply, and ATS components - Closes Sachinchaurasiya360#206
Contributor
📝 WalkthroughWalkthroughThis PR migrates uploads to a presigned-URL flow: clients request an S3 PUT URL, upload files directly to S3, then notify server endpoints to persist file metadata. Client pages use ChangesPre-signed URL Upload Architecture
Sequence DiagramsequenceDiagram
participant Browser as Client Browser
participant Server as InternHack Server
participant S3 as AWS S3
Browser->>Server: POST /api/upload/presigned-url {fileName,fileType,folder}
Server->>Server: createUniqueS3Key, generatePresignedUploadUrl
Server-->>Browser: {uploadUrl, fileUrl, fileKey}
Browser->>S3: PUT uploadUrl (Content-Type: mime, file bytes)
S3-->>Browser: 200 OK
Browser->>Server: POST /api/upload{endpoint} {fileUrl, originalName, size, mimeType}
Server->>Server: validate fileUrl origin, persist to Prisma, delete previous object if any
Server-->>Browser: {user, file metadata}
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly Related PRs
Suggested Labels
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)
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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
client/src/module/student/profile/StudentProfilePage.tsx (1)
483-508:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMissing client-side validation for resume uploads.
handleResumeUploadcallsuploadDirectToS3without validating file size or type, unlike the image upload handlers which properly validate. The UI states "PDF only, max 10 MB" but this isn't enforced in code.🛡️ Proposed fix to add validation
+ const MAX_RESUME_SIZE = 5 * 1024 * 1024; // 5 MB per guidelines + const handleResumeUpload = async (e: React.ChangeEvent<HTMLInputElement>) => { const file = e.target.files?.[0]; if (!file) return; + + if (file.size > MAX_RESUME_SIZE) { + toast.error("Resume must be under 5 MB"); + if (resumeInputRef.current) resumeInputRef.current.value = ""; + return; + } + if (file.type !== "application/pdf") { + toast.error("Only PDF files are allowed"); + if (resumeInputRef.current) resumeInputRef.current.value = ""; + return; + } + setUploadingResume(true);Also update line 1254 to reflect the correct limit:
<p className="text-[10px] font-mono text-stone-500">PDF only, max 5 MB each.</p>As per coding guidelines: "File upload validation must validate client-side with size ≤ 5 MB and allowed types"
🤖 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 `@client/src/module/student/profile/StudentProfilePage.tsx` around lines 483 - 508, The resume upload handler handleResumeUpload lacks client-side validation: before calling uploadDirectToS3, validate the selected file (from e.target.files[0] / resumeInputRef) for allowed type (PDF MIME and/or .pdf extension) and enforce size ≤ 5 MB; if invalid, setUploadingResume(false), clear resumeInputRef.current.value, and surface a descriptive toast.error (do not call uploadDirectToS3). After a successful upload keep the existing setForm/syncUser flow and toast.success; also update the UI text near the resume input in StudentProfilePage to state "PDF only, max 5 MB each." to match the enforced limit.server/src/module/upload/upload.controller.ts (2)
16-24:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winReject client-supplied asset URLs and derive them server-side.
These handlers persist any
fileUrlthe caller sends, and previously stored values later flow intodeleteFile(). That lets a user point their profile data at another S3 object and trigger deletion on the next update, or store a crafted/uploads/...path and hit the local unlink branch. Persist a canonical URL derived from a validated server-issuedfileKeyinstead, and only delete objects under the current user's allowed prefix.Also applies to: 58-70, 84-96, 110-127
🤖 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/src/module/upload/upload.controller.ts` around lines 16 - 24, Reject using any client-supplied fileUrl and instead persist and operate only on a server-derived canonical URL computed from a validated server-issued fileKey; update the handlers that currently accept fileUrl (the handlers covering lines 58-70, 84-96, 110-127) to validate the incoming fileKey, construct the canonical URL/server-side key, save that canonical URL to the user record, and pass it to deleteFile. In deleteFile (and helper getS3KeyFromUrl) enforce strict deletion rules: only allow S3 deletions for keys under the app's tenant/user prefix and only allow local deletions inside the UPLOADS_DIR canonical path (resolve and verify the path is within UPLOADS_DIR); reject or log and skip any client-provided or out-of-scope paths. Ensure all places that previously persisted fileUrl now store the server-derived canonical URL derived from fileKey.
117-129:⚠️ Potential issue | 🟠 Major | ⚡ Quick winEvict the old resume only after the DB write succeeds.
deleteFile(oldest)runs beforeprisma.user.update(...). If the update fails, the user can lose the object while their profile still points at it. Capture the evicted URL first, update Prisma, then delete the old object after persistence succeeds.🤖 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/src/module/upload/upload.controller.ts` around lines 117 - 129, The code currently calls deleteFile(oldest) before persisting the new resumes array, risking data loss if prisma.user.update(...) fails; instead, determine the evicted URL (oldest) from updatedResumes (using MAX_RESUMES, fileUrl logic), perform prisma.user.update(...) with the new resumes array first, and only after the update resolves successfully call deleteFile(evictedUrl) to remove the old object; ensure you handle the case when there is no evictedUrl (i.e., updatedResumes length < MAX_RESUMES) and surface/log any deletion errors without rolling back the successful DB update.
🧹 Nitpick comments (3)
client/src/utils/upload.ts (1)
29-30: ⚡ Quick winExtract error details from failed responses.
Generic error messages make debugging difficult. The server may return useful error information (e.g., file too large, invalid type, auth failure) that gets lost.
♻️ Proposed improvement for better error messages
- if (!presignRes.ok) throw new Error('Failed to get upload URL'); + if (!presignRes.ok) { + const errBody = await presignRes.json().catch(() => ({})); + throw new Error(errBody.message || `Failed to get upload URL (${presignRes.status})`); + } const { uploadUrl, fileUrl } = await presignRes.json();Apply similar pattern to Lines 41 and 60.
🤖 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 `@client/src/utils/upload.ts` around lines 29 - 30, When a fetch response like presignRes.ok is false, capture and include the server's error body in the thrown Error instead of a generic message: read the response body (attempt JSON parse, fallback to text), extract a helpful message or fields, and include it in the Error thrown for the presign step (where const { uploadUrl, fileUrl } is used) and apply the same pattern to the other fetch responses in this file (the upload/finalize responses—e.g., uploadRes and finalizeRes or similarly named variables at the later checks) so failures surface server-provided details (status code + body) in the thrown error.client/src/module/student/ats/AtsScorePage.tsx (1)
273-273: 💤 Low valueFile size limit exceeds coding guidelines.
MAX_SIZEis set to 10 MB, but coding guidelines specify file uploads should be ≤ 5 MB. Consider aligning with the guideline or documenting the exception if 10 MB is intentional for ATS analysis.🤖 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 `@client/src/module/student/ats/AtsScorePage.tsx` at line 273, The constant MAX_SIZE in AtsScorePage.tsx is set to 10 MB which violates the ≤5 MB upload guideline; change MAX_SIZE to 5 * 1024 * 1024 (5 MB) to comply, or if 10 MB is intentional for ATS analysis add a clear comment above MAX_SIZE explaining the exception and reference the guideline ID/approval so reviewers know it's deliberate.server/src/module/upload/upload.routes.ts (1)
13-20: ⚡ Quick winAdd request validation middleware to the new JSON upload routes.
After removing multer, these endpoints only run
authMiddlewarebefore controller logic. Add route-level validation forfileName,fileType,folder, andfileUrlso malformed or cross-namespace payloads are rejected before they reach the controller.As per coding guidelines, Routes layer must contain Express router with middleware chain (auth, role, usage-limit, 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 `@server/src/module/upload/upload.routes.ts` around lines 13 - 20, The new JSON-only routes (uploadRouter.post("/profile-pic", uploadController.uploadProfilePic), uploadRouter.post("/cover-image", uploadController.uploadCoverImage), uploadRouter.post("/profile-resume", uploadController.uploadProfileResume), uploadRouter.delete("/profile-resume", uploadController.deleteProfileResume)) lack route-level request validation and only run authMiddleware; add a validation middleware into the router chain that verifies required fields (fileName: non-empty string, fileType: allowed mime-types/string, folder: non-empty string and matches the authenticated user’s namespace/owner from authMiddleware, fileUrl: well-formed URL) and rejects malformed or cross-namespace payloads before calling the controller; update the routes to use authMiddleware, then the new file-upload request validator (and existing role/usage-limit middleware if applicable) so controllers only receive validated input.
🤖 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 `@client/src/utils/upload.ts`:
- Around line 8-10: The uploadDirectToS3 function currently accepts any file
without client-side validation; add validation inside uploadDirectToS3 (which
receives UploadProfileParams) to enforce file.size ≤ 5 * 1024 * 1024 bytes and
restrict mime types to the same allowedTypes list/pattern used in
DynamicFieldRenderer.tsx, returning/rejecting with a clear validation error
before any network call if checks fail; ensure the function documents/throws a
ValidationError (or rejects the promise) so callers can handle it consistently.
In `@server/src/module/upload/upload.controller.ts`:
- Around line 34-45: Currently folder and fileType are accepted directly from
the client (variables folder, fileType) and used to build fileKey and call
generatePresignedUploadUrl, which lets clients control S3 prefixes and MIME
types; change the handler in upload.controller.ts to enforce a server-side
allowlist for folder (map allowed flow names to fixed prefixes and override the
client-supplied folder) and validate fileType against a per-flow whitelist of
acceptable MIME types (reject with res.status(400) if not allowed). Continue
sanitizing fileName and using req.user.id and crypto.randomUUID() for
uniqueness, but do not include client-supplied folder or arbitrary fileType in
the generated fileKey or presigned URL call; instead pass the canonical prefix
and a validated MIME type to generatePresignedUploadUrl.
---
Outside diff comments:
In `@client/src/module/student/profile/StudentProfilePage.tsx`:
- Around line 483-508: The resume upload handler handleResumeUpload lacks
client-side validation: before calling uploadDirectToS3, validate the selected
file (from e.target.files[0] / resumeInputRef) for allowed type (PDF MIME and/or
.pdf extension) and enforce size ≤ 5 MB; if invalid, setUploadingResume(false),
clear resumeInputRef.current.value, and surface a descriptive toast.error (do
not call uploadDirectToS3). After a successful upload keep the existing
setForm/syncUser flow and toast.success; also update the UI text near the resume
input in StudentProfilePage to state "PDF only, max 5 MB each." to match the
enforced limit.
In `@server/src/module/upload/upload.controller.ts`:
- Around line 16-24: Reject using any client-supplied fileUrl and instead
persist and operate only on a server-derived canonical URL computed from a
validated server-issued fileKey; update the handlers that currently accept
fileUrl (the handlers covering lines 58-70, 84-96, 110-127) to validate the
incoming fileKey, construct the canonical URL/server-side key, save that
canonical URL to the user record, and pass it to deleteFile. In deleteFile (and
helper getS3KeyFromUrl) enforce strict deletion rules: only allow S3 deletions
for keys under the app's tenant/user prefix and only allow local deletions
inside the UPLOADS_DIR canonical path (resolve and verify the path is within
UPLOADS_DIR); reject or log and skip any client-provided or out-of-scope paths.
Ensure all places that previously persisted fileUrl now store the server-derived
canonical URL derived from fileKey.
- Around line 117-129: The code currently calls deleteFile(oldest) before
persisting the new resumes array, risking data loss if prisma.user.update(...)
fails; instead, determine the evicted URL (oldest) from updatedResumes (using
MAX_RESUMES, fileUrl logic), perform prisma.user.update(...) with the new
resumes array first, and only after the update resolves successfully call
deleteFile(evictedUrl) to remove the old object; ensure you handle the case when
there is no evictedUrl (i.e., updatedResumes length < MAX_RESUMES) and
surface/log any deletion errors without rolling back the successful DB update.
---
Nitpick comments:
In `@client/src/module/student/ats/AtsScorePage.tsx`:
- Line 273: The constant MAX_SIZE in AtsScorePage.tsx is set to 10 MB which
violates the ≤5 MB upload guideline; change MAX_SIZE to 5 * 1024 * 1024 (5 MB)
to comply, or if 10 MB is intentional for ATS analysis add a clear comment above
MAX_SIZE explaining the exception and reference the guideline ID/approval so
reviewers know it's deliberate.
In `@client/src/utils/upload.ts`:
- Around line 29-30: When a fetch response like presignRes.ok is false, capture
and include the server's error body in the thrown Error instead of a generic
message: read the response body (attempt JSON parse, fallback to text), extract
a helpful message or fields, and include it in the Error thrown for the presign
step (where const { uploadUrl, fileUrl } is used) and apply the same pattern to
the other fetch responses in this file (the upload/finalize responses—e.g.,
uploadRes and finalizeRes or similarly named variables at the later checks) so
failures surface server-provided details (status code + body) in the thrown
error.
In `@server/src/module/upload/upload.routes.ts`:
- Around line 13-20: The new JSON-only routes (uploadRouter.post("/profile-pic",
uploadController.uploadProfilePic), uploadRouter.post("/cover-image",
uploadController.uploadCoverImage), uploadRouter.post("/profile-resume",
uploadController.uploadProfileResume), uploadRouter.delete("/profile-resume",
uploadController.deleteProfileResume)) lack route-level request validation and
only run authMiddleware; add a validation middleware into the router chain that
verifies required fields (fileName: non-empty string, fileType: allowed
mime-types/string, folder: non-empty string and matches the authenticated user’s
namespace/owner from authMiddleware, fileUrl: well-formed URL) and rejects
malformed or cross-namespace payloads before calling the controller; update the
routes to use authMiddleware, then the new file-upload request validator (and
existing role/usage-limit middleware if applicable) so controllers only receive
validated input.
🪄 Autofix (Beta)
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: defaults
Review profile: CHILL
Plan: Pro
Run ID: da01f91b-709f-4bfe-994c-17058b7a92d5
📒 Files selected for processing (9)
client/src/module/recruiter/profile/RecruiterProfilePage.tsxclient/src/module/student/applications/ApplyPage.tsxclient/src/module/student/ats/AtsScorePage.tsxclient/src/module/student/profile/StudentProfilePage.tsxclient/src/utils/upload.tsserver/src/middleware/upload.middleware.tsserver/src/module/upload/upload.controller.tsserver/src/module/upload/upload.routes.tsserver/src/utils/s3.utils.ts
💤 Files with no reviewable changes (1)
- server/src/middleware/upload.middleware.ts
Comment on lines
+8
to
+10
| export const uploadDirectToS3 = async ({ file, folder, endpoint, authToken }: UploadProfileParams) => { | ||
| try { | ||
| const apiUrl = (import.meta.env.VITE_API_URL as string | undefined) ?? "http://localhost:3000"; |
Contributor
There was a problem hiding this comment.
Missing client-side file validation before upload.
Per coding guidelines, file uploads must validate client-side with size ≤ 5 MB and allowed types. This utility accepts any file without validation, which could lead to:
- Wasted bandwidth uploading files that the server will reject
- Inconsistent validation across callers
🛡️ Proposed fix to add validation
+const MAX_FILE_SIZE = 5 * 1024 * 1024; // 5 MB
+const ALLOWED_TYPES: Record<string, string[]> = {
+ resumes: ['application/pdf'],
+ 'profile-pics': ['image/jpeg', 'image/jpg', 'image/png'],
+ 'cover-images': ['image/jpeg', 'image/jpg', 'image/png'],
+};
+
export const uploadDirectToS3 = async ({ file, folder, endpoint, authToken }: UploadProfileParams) => {
try {
+ if (file.size > MAX_FILE_SIZE) {
+ throw new Error(`File exceeds ${MAX_FILE_SIZE / (1024 * 1024)} MB limit`);
+ }
+ const allowed = ALLOWED_TYPES[folder];
+ if (allowed && !allowed.includes(file.type)) {
+ throw new Error(`Invalid file type. Allowed: ${allowed.join(', ')}`);
+ }
+
const apiUrl = (import.meta.env.VITE_API_URL as string | undefined) ?? "http://localhost:3000";As per coding guidelines: "File upload validation must validate client-side with size ≤ 5 MB and allowed types following the pattern in DynamicFieldRenderer.tsx"
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| export const uploadDirectToS3 = async ({ file, folder, endpoint, authToken }: UploadProfileParams) => { | |
| try { | |
| const apiUrl = (import.meta.env.VITE_API_URL as string | undefined) ?? "http://localhost:3000"; | |
| const MAX_FILE_SIZE = 5 * 1024 * 1024; // 5 MB | |
| const ALLOWED_TYPES: Record<string, string[]> = { | |
| resumes: ['application/pdf'], | |
| 'profile-pics': ['image/jpeg', 'image/jpg', 'image/png'], | |
| 'cover-images': ['image/jpeg', 'image/jpg', 'image/png'], | |
| }; | |
| export const uploadDirectToS3 = async ({ file, folder, endpoint, authToken }: UploadProfileParams) => { | |
| try { | |
| if (file.size > MAX_FILE_SIZE) { | |
| throw new Error(`File exceeds ${MAX_FILE_SIZE / (1024 * 1024)} MB limit`); | |
| } | |
| const allowed = ALLOWED_TYPES[folder]; | |
| if (allowed && !allowed.includes(file.type)) { | |
| throw new Error(`Invalid file type. Allowed: ${allowed.join(', ')}`); | |
| } | |
| const apiUrl = (import.meta.env.VITE_API_URL as string | undefined) ?? "http://localhost:3000"; |
🤖 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 `@client/src/utils/upload.ts` around lines 8 - 10, The uploadDirectToS3
function currently accepts any file without client-side validation; add
validation inside uploadDirectToS3 (which receives UploadProfileParams) to
enforce file.size ≤ 5 * 1024 * 1024 bytes and restrict mime types to the same
allowedTypes list/pattern used in DynamicFieldRenderer.tsx, returning/rejecting
with a clear validation error before any network call if checks fail; ensure the
function documents/throws a ValidationError (or rejects the promise) so callers
can handle it consistently.
Comment on lines
+34
to
45
| const { fileName, fileType, folder = "uploads" } = req.body; | ||
| if (!fileName || !fileType) { | ||
| return res.status(400).json({ message: "fileName and fileType are required" }); | ||
| } | ||
|
|
||
| // Validate actual file content matches claimed MIME type | ||
| await validateOrReject( | ||
| req.file.path, | ||
| ["application/pdf", "application/msword", "application/vnd.openxmlformats-officedocument.wordprocessingml.document", "image/jpeg", "image/png", "image/webp", "text/plain"], | ||
| "File content does not match allowed file types", | ||
| ); | ||
| const cleanFileName = fileName.replace(/[^a-zA-Z0-9.-]/g, "_"); | ||
| const uniqueId = crypto.randomUUID(); | ||
| const fileKey = `${folder}/${req.user.id}/${uniqueId}-${cleanFileName}`; | ||
|
|
||
| const buffer = readAndCleanup(req.file.path); | ||
| const url = await uploadWithFallback(buffer, "uploads", userId, req.file.originalname, req.file.mimetype); | ||
| const uploadUrl = await generatePresignedUploadUrl(fileKey, fileType); | ||
| const fileUrl = `https://${process.env.AWS_S3_BUCKET}.s3.${process.env.AWS_REGION}.amazonaws.com/${fileKey}`; | ||
|
|
Contributor
There was a problem hiding this comment.
Do not let the client choose arbitrary S3 prefixes and content types.
folder and fileType are fully client-controlled here, so any authenticated user can mint upload URLs for arbitrary bucket namespaces and MIME types. Restrict folder to a small server-side allowlist per use case and validate fileType against the formats each flow actually supports.
🤖 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/src/module/upload/upload.controller.ts` around lines 34 - 45,
Currently folder and fileType are accepted directly from the client (variables
folder, fileType) and used to build fileKey and call generatePresignedUploadUrl,
which lets clients control S3 prefixes and MIME types; change the handler in
upload.controller.ts to enforce a server-side allowlist for folder (map allowed
flow names to fixed prefixes and override the client-supplied folder) and
validate fileType against a per-flow whitelist of acceptable MIME types (reject
with res.status(400) if not allowed). Continue sanitizing fileName and using
req.user.id and crypto.randomUUID() for uniqueness, but do not include
client-supplied folder or arbitrary fileType in the generated fileKey or
presigned URL call; instead pass the canonical prefix and a validated MIME type
to generatePresignedUploadUrl.
…ogo sync - Structural rewrite of uploadDirectToS3 utility to provide clean conditional contracts - Linked resolved standalone S3 fileUrl records into company payload submissions - Eliminated dead authorization parameters from type files
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
client/src/utils/upload.ts (1)
7-23:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd client-side file validation before presign/upload calls.
This helper still uploads without enforcing size/type constraints, so invalid files go over the network before rejection.
As per coding guidelines: "File upload validation must validate client-side with size ≤ 5 MB and allowed types following the pattern in `DynamicFieldRenderer.tsx`."Suggested fix
interface UploadProfileParams { file: File; folder: 'resumes' | 'profile-pics' | 'cover-images' | 'company-logos'; endpoint?: '/profile-resume' | '/profile-pic' | '/cover-image'; } +const MAX_FILE_SIZE = 5 * 1024 * 1024; // 5 MB +const ALLOWED_TYPES: Record<UploadProfileParams["folder"], string[]> = { + resumes: ["application/pdf"], + "profile-pics": ["image/jpeg", "image/jpg", "image/png", "image/webp"], + "cover-images": ["image/jpeg", "image/jpg", "image/png", "image/webp"], + "company-logos": ["image/jpeg", "image/jpg", "image/png", "image/webp", "image/svg+xml"], +}; + export const uploadDirectToS3 = async ({ file, folder, endpoint }: UploadProfileParams) => { try { + if (file.size > MAX_FILE_SIZE) { + throw new Error("File size must be 5 MB or less"); + } + const allowed = ALLOWED_TYPES[folder]; + if (!allowed.includes(file.type)) { + throw new Error(`Invalid file type for ${folder}`); + } + const apiUrl = (import.meta.env.VITE_API_URL as string | undefined) ?? "http://localhost:3000";🤖 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 `@client/src/utils/upload.ts` around lines 7 - 23, The uploadDirectToS3 helper currently calls the backend presign endpoint without client-side validation; add a pre-check inside uploadDirectToS3 (before calling fetch) that enforces file.size ≤ 5 * 1024 * 1024 and that file.type matches the same allowed MIME/type pattern used in DynamicFieldRenderer.tsx, and if validation fails return/reject with a clear error (or throw) to prevent the presign/upload flow from proceeding; reference the uploadDirectToS3 function and UploadProfileParams to locate where to insert these checks.
🤖 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/src/utils/s3.utils.ts`:
- Around line 92-100: The generatePresignedUploadUrl function currently passes
process.env.AWS_S3_BUCKET directly into PutObjectCommand which can be undefined;
add a guard at the start of generatePresignedUploadUrl that checks
process.env.AWS_S3_BUCKET (or a resolved constant) and throws a clear, early
Error if missing; then use that validated bucket value when constructing the
PutObjectCommand (referencing generatePresignedUploadUrl, PutObjectCommand,
s3Client, and getSignedUrl) so signing never proceeds with an undefined Bucket.
---
Duplicate comments:
In `@client/src/utils/upload.ts`:
- Around line 7-23: The uploadDirectToS3 helper currently calls the backend
presign endpoint without client-side validation; add a pre-check inside
uploadDirectToS3 (before calling fetch) that enforces file.size ≤ 5 * 1024 *
1024 and that file.type matches the same allowed MIME/type pattern used in
DynamicFieldRenderer.tsx, and if validation fails return/reject with a clear
error (or throw) to prevent the presign/upload flow from proceeding; reference
the uploadDirectToS3 function and UploadProfileParams to locate where to insert
these checks.
🪄 Autofix (Beta)
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: defaults
Review profile: CHILL
Plan: Pro
Run ID: b6ee44cd-5d4b-4e1e-bda9-b14f3abd9a96
📒 Files selected for processing (5)
client/src/module/student/companies/AddCompanyPage.tsxclient/src/utils/upload.tsclient/tsconfig.app.jsonserver/src/module/upload/upload.controller.tsserver/src/utils/s3.utils.ts
✅ Files skipped from review due to trivial changes (1)
- client/tsconfig.app.json
🚧 Files skipped from review as they are similar to previous changes (1)
- server/src/module/upload/upload.controller.ts
Comment on lines
+92
to
+100
| export const generatePresignedUploadUrl = async (fileKey: string, fileType: string) => { | ||
| const command = new PutObjectCommand({ | ||
| Bucket: process.env.AWS_S3_BUCKET, | ||
| Key: fileKey, | ||
| ContentType: fileType, // Enforces that the client uploads the correct file type | ||
| }); | ||
|
|
||
| // URL expires in 5 minutes (300 seconds) | ||
| const uploadUrl = await getSignedUrl(s3Client, command, { expiresIn: 300 }); |
Contributor
There was a problem hiding this comment.
Guard missing S3 bucket config before signing upload URLs.
Bucket can be undefined here, which turns misconfiguration into opaque runtime failures on the upload path. Fail fast with a clear error.
Suggested fix
export const generatePresignedUploadUrl = async (fileKey: string, fileType: string) => {
+ if (!BUCKET) {
+ throw new Error("AWS_S3_BUCKET is not configured");
+ }
+
const command = new PutObjectCommand({
- Bucket: process.env.AWS_S3_BUCKET,
+ Bucket: BUCKET,
Key: fileKey,
ContentType: fileType, // Enforces that the client uploads the correct file type
});🤖 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/src/utils/s3.utils.ts` around lines 92 - 100, The
generatePresignedUploadUrl function currently passes process.env.AWS_S3_BUCKET
directly into PutObjectCommand which can be undefined; add a guard at the start
of generatePresignedUploadUrl that checks process.env.AWS_S3_BUCKET (or a
resolved constant) and throws a clear, early Error if missing; then use that
validated bucket value when constructing the PutObjectCommand (referencing
generatePresignedUploadUrl, PutObjectCommand, s3Client, and getSignedUrl) so
signing never proceeds with an undefined Bucket.
Abfa41
added
level:advanced
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
3 participants
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.
This PR implements the architectural optimization outlined in #206. It entirely removes the server-side file buffering bottleneck by transitioning all file uploads (resumes, profile pictures, cover images, and attachments) to a direct client-to-S3 flow using pre-signed URLs.
By removing
multerand preventing large files from occupying the Node.js heap, this update significantly improves the scalability and performance of the platform.Key Benefits:
saveLocally()) that could lead to data loss or storage bloating on the production server.Closes #206
Technical Changes
Backend:
generatePresignedUploadUrlleveraging the existing@aws-sdk/s3-request-presignerdependency inserver/src/utils/s3.utils.ts.POST /api/upload/presigned-url.server/src/middleware/upload.middleware.ts(Multer disk storage).upload.controller.tsto removereadAndCleanup()anduploadWithFallback(). Profile and resume endpoints now simply accept a JSON payload containing the final S3 URL to update the database.Frontend:
uploadDirectToS3utility to handle the 3-step upload flow (Get URL -> PUT to S3 -> Save to DB).credentials: 'include').FormDatauploads across the Student Profile, Recruiter Profile, ATS Score page, and Apply page to use the new direct S3 utility.CRITICAL: Maintainer Action Required (AWS CORS)
Because the browser is now attempting to execute a
PUTrequest directly to the S3 bucket frominternhack.com(andlocalhost), you must update the CORS policy in the AWS Console.If this is not added, the browser will block all user uploads. Please navigate to the S3 Bucket Permissions tab and append the following JSON to the Cross-origin resource sharing (CORS) configuration:
[ { "AllowedHeaders": [ "Content-Type" ], "AllowedMethods": [ "PUT" ], "AllowedOrigins": [ "[https://internhack.com](https://internhack.com)", "http://localhost:5173" ], "ExposeHeaders": [] } ] <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Improvements** * Direct cloud uploads for profile pictures, cover images, resumes, and attachments for faster, more reliable transfers * Unified upload behavior across recruiter and student flows, applications, company logos, and ATS resume scoring * More robust handling of returned file URLs and automatic normalization for consistent display * **Backend** * New presigned-url endpoint and server-side utilities to support secure client-to-cloud uploads * Upload endpoints now accept persisted file URLs and enforce resume limits and signed access <!-- review_stack_entry_start --> [](https://app.coderabbit.ai/change-stack/Sachinchaurasiya360/InternHack/pull/310?utm_source=github_walkthrough&utm_medium=github&utm_campaign=change_stack) <!-- review_stack_entry_end --> <!-- end of auto-generated comment: release notes by coderabbit.ai -->