Skip to content

test: fix static file tests for supertest 7+ URL normalization #6856

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
+91 −17
Open

test: fix static file tests for supertest 7+ URL normalization #6856

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,8 @@
"morgan": "1.10.1",
"nyc": "^17.1.0",
"pbkdf2-password": "1.2.1",
"supertest": "^6.3.0",
"superagent": "^10.2.3",
Copy link
Member

@jonchurch jonchurch Oct 30, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
"superagent": "^10.2.3",

Unused?

"supertest": "^7.1.4",
"vhost": "~3.0.2"
},
"engines": {
Expand Down
22 changes: 18 additions & 4 deletions test/acceptance/downloads.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@

var app = require('../../examples/downloads')
, request = require('supertest');
, request = require('supertest')
, http = require('node:http')
, assert = require('node:assert');

describe('downloads', function(){
describe('GET /', function(){
Expand Down Expand Up @@ -39,9 +41,21 @@ describe('downloads', function(){

describe('GET /files/../index.js', function () {
it('should respond with 403', function (done) {
request(app)
.get('/files/../index.js')
.expect(403, done)
var server = app.listen(function () {
var port = server.address().port
// Use http.request to avoid URL normalization by superagent
var req = http.request({
hostname: 'localhost',
port: port,
path: '/files/../index.js',
method: 'GET'
}, function (res) {
assert.strictEqual(res.statusCode, 403)
server.close(done)
})
req.on('error', done)
req.end()
})
})
})
})
83 changes: 71 additions & 12 deletions test/express.static.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ var assert = require('node:assert')
var express = require('..')
var path = require('node:path')
const { Buffer } = require('node:buffer');
var http = require('node:http')

var request = require('supertest')
var utils = require('./support/utils')
Expand Down Expand Up @@ -269,9 +270,26 @@ describe('express.static()', function () {
})

it('should fall-through when traversing past root', function (done) {
request(this.app)
.get('/users/../../todo.txt')
.expect(404, 'Not Found', done)
var server = this.app.listen(function () {
var port = server.address().port
// Use http.request to avoid URL normalization by superagent
var req = http.request({
hostname: 'localhost',
port: port,
path: '/users/../../todo.txt',
method: 'GET'
}, function (res) {
assert.strictEqual(res.statusCode, 404)
var body = ''
res.on('data', function (chunk) { body += chunk })
res.on('end', function () {
assert.strictEqual(body, 'Not Found')
server.close(done)
})
Comment on lines +281 to +288
Copy link
Member

@jonchurch jonchurch Oct 30, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When the assertions throw, they prevent cleanup (server.close) so the suite hangs forever with an open server.

They're also throwing in callbacks, so mocha only sees them due to its global uncaught exception handler.
They'd need to be try/catch'd, otherwise we can never clean up or reliaby associate the failure w/ the test.

(ideally we assert in only one place, so moved the first one)

This feedback applies to all of the updated test cases in the PR

Suggested change
}, function (res) {
assert.strictEqual(res.statusCode, 404)
var body = ''
res.on('data', function (chunk) { body += chunk })
res.on('end', function () {
assert.strictEqual(body, 'Not Found')
server.close(done)
})
}, function (res) {
var body = ''
res.on('data', function (chunk) { body += chunk })
res.on('end', function () {
try {
assert.strictEqual(res.statusCode, 404)
assert.strictEqual(body, 'Not Found')
server.close(done)
} catch(err) {
server.close(() => done(err))
}
})

})
req.on('error', done)
req.end()
})
})

it('should fall-through when URL too long', function (done) {
Expand Down Expand Up @@ -344,9 +362,26 @@ describe('express.static()', function () {
})

it('should 403 when traversing past root', function (done) {
request(this.app)
.get('/users/../../todo.txt')
.expect(403, /ForbiddenError/, done)
var server = this.app.listen(function () {
var port = server.address().port
// Use http.request to avoid URL normalization by superagent
var req = http.request({
hostname: 'localhost',
port: port,
path: '/users/../../todo.txt',
method: 'GET'
}, function (res) {
assert.strictEqual(res.statusCode, 403)
var body = ''
res.on('data', function (chunk) { body += chunk })
res.on('end', function () {
assert.match(body, /ForbiddenError/)
server.close(done)
})
})
req.on('error', done)
req.end()
})
})

it('should 404 when URL too long', function (done) {
Expand Down Expand Up @@ -578,15 +613,39 @@ describe('express.static()', function () {
})

it('should catch urlencoded ../', function (done) {
request(this.app)
.get('/users/%2e%2e/%2e%2e/todo.txt')
.expect(403, done)
var server = this.app.listen(function () {
var port = server.address().port
// Use http.request to avoid URL normalization by superagent
var req = http.request({
hostname: 'localhost',
port: port,
path: '/users/%2e%2e/%2e%2e/todo.txt',
method: 'GET'
}, function (res) {
assert.strictEqual(res.statusCode, 403)
server.close(done)
})
req.on('error', done)
req.end()
})
})

it('should not allow root path disclosure', function (done) {
request(this.app)
.get('/users/../../fixtures/todo.txt')
.expect(403, done)
var server = this.app.listen(function () {
var port = server.address().port
// Use http.request to avoid URL normalization by superagent
var req = http.request({
hostname: 'localhost',
port: port,
path: '/users/../../fixtures/todo.txt',
method: 'GET'
}, function (res) {
assert.strictEqual(res.statusCode, 403)
server.close(done)
})
req.on('error', done)
req.end()
})
})
})

Expand Down