Skip to content

Commit d777da2

Browse files
bnoordhuisMylesBorins
authored andcommitted
http: reject control characters in http.request()
Unsanitized paths containing line feed characters can be used for header injection and request splitting so reject them with an exception. There seems to be no reasonable use case for allowing control characters (characters <= 31) while there are several scenarios where they can be used to exploit software bugs so reject control characters altogether. PR-URL: #8923 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Evan Lucas <[email protected]> Reviewed-By: Fedor Indutny <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: not-an-aardvark <[email protected]>
1 parent 1b85989 commit d777da2

File tree

2 files changed

+15
-11
lines changed

2 files changed

+15
-11
lines changed

lib/_http_client.js

Lines changed: 3 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -43,13 +43,12 @@ function ClientRequest(options, cb) {
4343
if (self.agent && self.agent.protocol)
4444
expectedProtocol = self.agent.protocol;
4545

46-
if (options.path && / /.test(options.path)) {
46+
if (options.path && /[\u0000-\u0020]/.test(options.path)) {
4747
// The actual regex is more like /[^A-Za-z0-9\-._~!$&'()*+,;=/:@]/
4848
// with an additional rule for ignoring percentage-escaped characters
4949
// but that's a) hard to capture in a regular expression that performs
50-
// well, and b) possibly too restrictive for real-world usage. That's
51-
// why it only scans for spaces because those are guaranteed to create
52-
// an invalid request.
50+
// well, and b) possibly too restrictive for real-world usage.
51+
// Restrict the filter to control characters and spaces.
5352
throw new TypeError('Request path contains unescaped characters');
5453
} else if (protocol !== expectedProtocol) {
5554
throw new Error('Protocol "' + protocol + '" not supported. ' +
Lines changed: 12 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1,9 +1,14 @@
11
'use strict';
2-
var common = require('../common');
3-
var assert = require('assert');
4-
var http = require('http');
2+
const common = require('../common');
3+
const assert = require('assert');
4+
const http = require('http');
55

6-
assert.throws(function() {
7-
// Path with spaces in it should throw.
8-
http.get({ path: 'bad path' }, common.fail);
9-
}, /contains unescaped characters/);
6+
function* bad() {
7+
for (let i = 0; i <= 32; i += 1)
8+
yield 'bad' + String.fromCharCode(i) + 'path';
9+
}
10+
11+
for (const path of bad()) {
12+
assert.throws(() => http.get({ path }, common.fail),
13+
/contains unescaped characters/);
14+
}

0 commit comments

Comments
 (0)