What makes this file-upload handler dangerous, and how would you make it safe?
Vulnerable code
app.post('/upload', requireAuth, (req, res) => {
const file = req.files.upload;
// BUG: trusts user filename + extension, writes into web-served dir
const dest = path.join(__dirname, 'public/uploads', file.name);
file.mv(dest, (err) => {
if (err) return res.sendStatus(500);
res.json({ url: '/uploads/' + file.name });
});
});Fixed
import { fileTypeFromBuffer } from 'file-type';
import { randomUUID } from 'crypto';
const ALLOWED = new Map([
['image/png', 'png'],
['image/jpeg', 'jpg'],
['application/pdf', 'pdf'],
]);
app.post('/upload', requireAuth, async (req, res) => {
const file = req.files.upload;
if (file.size > 10 * 1024 * 1024) return res.sendStatus(413);
// Verify true type by magic bytes, not extension/Content-Type
const type = await fileTypeFromBuffer(file.data);
if (!type || !ALLOWED.has(type.mime)) return res.sendStatus(415);
// Random name; store OUTSIDE the web root (here: private dir / object storage)
const id = randomUUID();
const ext = ALLOWED.get(type.mime);
await storage.put(`${req.user.id}/${id}.${ext}`, file.data); // not web-executable
res.json({ id }); // served later via an auth-gated download route
});The handler trusts the user-supplied filename and extension and writes the file into a web-served directory using that name. This enables multiple attacks: an attacker uploads shell.php (or .jsp/.aspx) which the server may then execute on request — remote code execution; a filename like ../../app/config.js triggers path traversal; and there's no content-type verification, so the Content-Type/extension are attacker-controlled lies. The fix: validate the file type by magic bytes, not extension or client Content-Type; generate a random server-side filename (UUID) and never use the user's name; store uploads outside the web root / in object storage so they can't be executed; serve them through an auth-gated handler with a forced safe Content-Type and Content-Disposition: attachment; and enforce size limits. AV scanning and a separate no-exec storage domain are good additions.