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: https://github.com/nodejs/node/pull/8923
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: Fedor Indutny <fedor@indutny.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: not-an-aardvark <not-an-aardvark@users.noreply.github.com>

lib/_http_client.js

@@ -43,13 +43,12 @@ function ClientRequest(options, cb) {
   if (self.agent && self.agent.protocol)
     expectedProtocol = self.agent.protocol;
 
-  if (options.path && / /.test(options.path)) {
+  if (options.path && /[\u0000-\u0020]/.test(options.path)) {
     // The actual regex is more like /[^A-Za-z0-9\-._~!$&'()*+,;=/:@]/
     // with an additional rule for ignoring percentage-escaped characters
     // but that's a) hard to capture in a regular expression that performs
-    // well, and b) possibly too restrictive for real-world usage. That's
+    // well, and b) possibly too restrictive for real-world usage.
-    // why it only scans for spaces because those are guaranteed to create
+    // Restrict the filter to control characters and spaces.
-    // an invalid request.
     throw new TypeError('Request path contains unescaped characters');
   } else if (protocol !== expectedProtocol) {
     throw new Error('Protocol "' + protocol + '" not supported. ' +

test/parallel/test-http-client-unescaped-path.js

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