redline
← arena

Joined, not confined

On-callJavaScriptExpressMined
MINED FROM REAL HISTORYexpressjs/expressMIT

The code below is the file exactly as it stood in expressjs/express. The defect was introduced on 2017-03-05, reviewed, merged, and fixed on 2019-12-13 in commit 82de4de5 — after 1012 days in the tree.

The commit was written by a human contributor to expressjs/express, not by an agent. The ticket and the description on the next two panels are ours — a reconstruction of the case the original change made, so you meet it the way its reviewer did. The code, the defect and the dates are untouched.

WEB-62the ticket

Harden the file download route

`/files/:file` builds a filesystem path out of a request parameter by concatenating strings. Two problems: it breaks on Windows separators, and joining raw user input into a path with `+` is the shape every security checklist tells you not to write.

Build the path with the platform's own helper instead of string arithmetic, and keep the existing 404 behaviour for files that are not there.

the case the change madereconstructed by us

Replaced the string concatenation with `path.join(__dirname, 'files', req.params.file)`.

`path.join` is the correct primitive here: it uses the platform separator, so the route now works identically on Windows and POSIX, and it normalises the result — collapsing `.` segments, redundant separators and empty components — which removes the malformed-path cases that the concatenated version could produce.

The download callback is unchanged: a missing file still returns the friendly 404, and any other error is passed to the error handler rather than swallowed. Verified against the three files in the fixtures directory, including the nested `notes/groceries.txt`, which is the case that motivated allowing a slash in the parameter.

Our reconstruction of the argument the real change made, not a quotation of it. Fluent, specific, and not evidence of anything — which is the point.

examples/downloads/index.js37 lines
1'use strict'
2
3/**
4 * Module dependencies.
5 */
6
7var express = require('../../');
8var path = require('path');
9var app = module.exports = express();
10
11app.get('/', function(req, res){
12 res.send('<ul>' +
13 '<li>Download <a href="/files/notes/groceries.txt">notes/groceries.txt</a>.</li>' +
14 '<li>Download <a href="/files/amazing.txt">amazing.txt</a>.</li>' +
15 '<li>Download <a href="/files/missing.txt">missing.txt</a>.</li>' +
16 '</ul>')
17});
18
19// /files/* is accessed via req.params[0]
20// but here we name it :file
21app.get('/files/:file(*)', function(req, res, next){
22 var filePath = path.join(__dirname, 'files', req.params.file);
23
24 res.download(filePath, function (err) {
25 if (!err) return; // file sent
26 if (err.status !== 404) return next(err); // non-404 error
27 // file for download not found
28 res.statusCode = 404;
29 res.send('Cant find that file, sorry!');
30 });
31});
32
33/* istanbul ignore next */
34if (!module.parent) {
35 app.listen(3000);
36 console.log('Express started on port 3000');
37}