security: validate file uploads by magic bytes, not client MIME type#565
security: validate file uploads by magic bytes, not client MIME type#565advikdivekar wants to merge 4 commits into
Conversation
…E type Replaces the Multer fileFilter MIME string check — which trusted the client's Content-Type header — with server-side magic byte inspection using the file-type library. The storage layer is switched from diskStorage to memoryStorage so the buffer is available before any Cloudinary interaction. Key changes: - upload.middleware.js: memoryStorage, no fileFilter (validation moved downstream) - validateFileType.js: new utility — fileTypeFromBuffer against JPEG/PNG/WebP allowlist - uploadCleanup.js: rewritten to use cloudinary.uploader.upload_stream with a buffer - user.controller.js / post.controller.js (createPost, updatePost): call validateImageBuffer after Multer, return HTTP 415 on mismatch - upload.test.js: mock updated to upload_stream; eight new magic-byte acceptance/rejection tests added - post.test.js: Cloudinary mock updated to upload_stream; fake image buffers replaced with valid PNG magic byte sequences Closes Shivayan09#540
|
@advikdivekar is attempting to deploy a commit to the Shivayan's projects Team on Vercel. A member of the Team first needs to authorize it. |
|
@Shivayan09 please review it and let me know if any improvements to be done or its ready to get merged |
|
@advikdivekar Mention/attach the issue it fixes in the description |
|
@Shivayan09 please review it now, i just mentioned the issue as well |
Shivayan09
left a comment
Shivayan09
left a comment
There was a problem hiding this comment.
Code fails CI pipeline workflows
Yes working on it rn |
|
@advikdivekar are you working on the changes? |
Yes I’m working on it, it failed on my machine so I was looking onto it, I’ll make sure to ship a clean pr by eod |
|
@Shivayan09 almost done |
|
@Shivayan09 please review it now, if anything do let me know, thank you assigning me this issue |
|
The upload security fix is not complete yet: validateImageBuffer() correctly checks magic bytes in the controllers, but the shared upload validation still reads file.path and still enforces file.mimetype, which means it remains tied to client-provided metadata and will likely break with the current multer.memoryStorage() setup. Please refactor validateImageUpload() to validate the in-memory buffer only, remove any MIME-based allowlist from the enforcement path, and clean up the middleware so there is a single source of truth for image validation. Also, the updated post upload tests need a pass, because several .attach() chains are split in a way that looks syntactically broken and may prevent the suite from running. |
…lidation - Remove dead storage/fileFilter/ALLOWED_MIME_TYPES from upload.middleware.js; middleware now cleanly wraps memoryStorage with only a size limit - Refactor validateImageUpload() to read file.buffer instead of readFile(file.path), which was broken under multer.memoryStorage() (no disk path exists); also drop the MIME cross-check so validation is based solely on magic bytes - Remove four duplicate .attach() chains in post.test.js that caused a parse error - Update two test assertions to expect the magic-byte rejection message instead of the old middleware MIME-filter message - Replace incomplete validPng buffer with PNG_MAGIC in the oversized-image test so file-type can positively identify the format before the size check fires
|
@Shivayan09 please review it now, let me know if any changes to be made |
2 participants
What is the problem
The shared Multer upload middleware accepted files based solely on the client-declared Content-Type header. Because that header is attacker-controlled, any file — an SVG with embedded script tags, an HTML page, a polyglot binary — could be uploaded by forging its declared MIME type. Both the avatar endpoint and post image endpoints were affected.
What was changed
Why this approach
Magic byte inspection is the only reliable server-side method for determining a file's true type. file-type reads the actual binary signature in the file rather than trusting the HTTP header the client supplies. Switching Multer to memoryStorage makes the buffer available immediately after upload without temporary files.
How to test
Edge cases covered
Verification
fixes #540
Please review and merge this under GSSoC 2026.