Review: File Download Route Written by an AI Assistant

Published 10 Nov, 2026

Your team asked an AI coding assistant to let users download the files they have uploaded to a document-sharing app. It opened this pull request. You are the reviewer, and nothing merges without your approval.

The assistant's description:

Adds GET /download?file=.... Files are read from the signed-in user's own folder, so users can only ever reach their own uploads. Supports sub-folders, sets the right content type so PDFs and images open in the browser, and streams the file instead of buffering it.

Where it runs: Express 4 on Node 20, one process per container, with sessions held in a cookie on the same domain as the rest of the app. Users can upload any file type. The router is mounted without any async error wrapper.

Your task

Flag every problem you would block the merge for. Click a line number to comment, and Shift-click to cover a range.
For each one, choose the issue type and severity, and explain what fails and how to fix it.
You are scored on the problems you catch, the right issue type and a clear explanation. Missing a critical problem fails the round.

Aim for 10 minutes, as in a real review round.

The description says users can only reach their own folder. Try file=../7/payslip.pdf.

What does Express 4 do when an async handler's promise rejects? Ask for a file that does not exist.

A user uploads notes.html and shares the download link. What does the browser do with it?

pipe does not pass errors along. What happens if the file vanishes between stat and the read?

A strong reviewer blocks this on two grounds straight away: any user can read any file the process can read, and any user can take the server down with one request.

Path traversal (line 23, critical). path.join resolves .., so file=../7/payslip.pdf reads another user's file and file=../../../etc/passwd leaves the uploads folder completely. Resolve the path and check that it still starts with userDir + path.sep, or look files up by id in the database instead of by name.
Unhandled rejection crashes the process (line 25, critical). fs.promises.stat rejects with ENOENT for a missing file. Express 4 does not catch rejected promises from handlers, so the request hangs and Node 20 exits on the unhandled rejection. One mistyped name takes the container down. Wrap the handler in try/catch and return 404 for ENOENT, or forward errors with next(err).
User uploads served inline with their own type (lines 31–33, major). An uploaded .html or .svg file is served as text/html or image/svg+xml, inline, on the app's own domain. Opening a shared link runs the uploader's script with the victim's session: stored XSS. Send Content-Disposition: attachment (for example res.attachment(name)) and X-Content-Type-Options: nosniff, allow inline only for a short list of safe types, or serve uploads from a separate domain.
Read stream errors are not handled (line 35, minor). pipe does not forward errors. If the file is deleted or unreadable after the stat, the stream emits error with no listener and the process crashes. Use stream.pipeline with a callback, or res.sendFile with a root option.

const express = require("express");
const fs = require("fs");
const path = require("path");
const { requireLogin } = require("../auth");

const router = express.Router();

// Each user's uploads live in uploads/<userId>/, possibly in sub-folders.
const UPLOAD_ROOT = path.resolve(__dirname, "..", "uploads");

/**
 * GET /download?file=reports/q3.pdf
 * Sends one of the signed-in user's own files. The browser shows it
 * directly when it can (PDFs, images) and downloads it otherwise.
 */
router.get("/download", requireLogin, async (req, res) => {
  const requested = String(req.query.file || "");
  if (!requested) {
    return res.status(400).json({ error: "file is required" });
  }

const userDir = path.join(UPLOAD_ROOT, String(req.user.id));
  const filePath = path.join(userDir, requested);

const stat = await fs.promises.stat(filePath);
  if (!stat.isFile()) {
    return res.status(404).json({ error: "Not found" });
  }

const name = path.basename(filePath);
  res.type(path.extname(name));
  res.setHeader("Content-Length", stat.size);
  res.setHeader("Content-Disposition", `inline; filename="${name}"`);

fs.createReadStream(filePath).pipe(res);
});

module.exports = router;

{"issues":[
{"id": "traversal", "file": "routes/download.js", "lines": [23, 23], "category": "security", "severity": "critical", "title": "Path traversal reads other users' files and system files", "explanation": "path.join resolves ../ segments, so file=../7/x.pdf reaches another user's folder and ../../../etc/passwd escapes the uploads root entirely. The per-user folder is not a security boundary.", "keywords": ["traversal", "../", "..", "dot dot", "escape", "outside", "other user", "normalize", "normalise", "startswith", "resolve"], "accept": ["edge"], "fix": "Resolve the full path and reject it unless it starts with userDir + path.sep, or look files up by id in the database."},
{"id": "rejection", "file": "routes/download.js", "lines": [25, 25], "category": "bug", "severity": "critical", "title": "Missing file rejects the async handler and crashes the server", "explanation": "fs.promises.stat rejects with ENOENT for a missing file or folder. Express 4 does not handle rejected promises, so the request hangs and Node exits on the unhandled rejection. Any user can take the process down.", "keywords": ["unhandled", "rejection", "reject", "enoent", "try", "catch", "crash", "next(err)", "404", "async"], "accept": ["edge", "api"], "fix": "Wrap the handler in try/catch, map ENOENT to 404 and pass other errors to next(err)."},
{"id": "inline-xss", "file": "routes/download.js", "lines": [31, 33], "category": "security", "severity": "major", "title": "Uploaded HTML or SVG served inline on the app's domain", "explanation": "res.type uses the uploaded file's own extension and the header says inline, so an uploaded .html or .svg runs as script on the app's origin with the viewer's session cookie: stored XSS through a shared link.", "keywords": ["xss", "inline", "attachment", "html", "svg", "script", "content type", "content-type", "nosniff", "same origin", "domain"], "fix": "Use Content-Disposition: attachment and X-Content-Type-Options: nosniff, allow inline only for safe types, or serve uploads from a separate domain."},
{"id": "stream-error", "file": "routes/download.js", "lines": [35, 35], "category": "bug", "severity": "minor", "title": "No error handling on the read stream", "explanation": "pipe does not forward errors. If the file is removed or unreadable after stat, the read stream emits error with no listener, which crashes the process, and the response is left half-sent.", "keywords": ["pipeline", "error", "listener", "pipe", "crash", "stream", "sendfile"], "accept": ["edge"], "fix": "Use stream.pipeline(readStream, res, callback) or res.sendFile, and handle the error."}
]}

Topics #001 AI Code Review Easy Express JavaScript Node.js Security