From 7124387b3414c41533078f14a84446e2e0a6ff95 Mon Sep 17 00:00:00 2001
From: Ben Noordhuis <info@bnoordhuis.nl>
Date: Wed, 15 May 2013 22:25:45 +0200
Subject: [PATCH] http: don't escape request path, reject bad chars

Commit 38149bb changes http.get() and http.request() to escape unsafe
characters. However, that creates an incompatibility with v0.10 that
is difficult to work around: if you escape the path manually, then in
v0.11 it gets escaped twice. Change lib/http.js so it no longer tries
to fix up bad request paths, simply reject them with an exception.

The actual check is rather basic right now. The full check for illegal
characters is difficult to implement efficiently because it requires a
few characters of lookahead. That's why it currently only checks for
spaces because those are guaranteed to create an invalid request.

Fixes #5474.
---
 doc/api/http.markdown                         |  4 +-
 lib/http.js                                   | 13 +++--
 ....js => test-http-client-unescaped-path.js} | 54 ++-----------------
 3 files changed, 15 insertions(+), 56 deletions(-)
 rename test/simple/{test-http-client-escape-path.js => test-http-client-unescaped-path.js} (50%)

diff --git a/doc/api/http.markdown b/doc/api/http.markdown
index e93aa2d2ddfd2c..d992bdffa3e35c 100644
--- a/doc/api/http.markdown
+++ b/doc/api/http.markdown
@@ -413,7 +413,9 @@ Options:
 - `socketPath`: Unix Domain Socket (use one of host:port or socketPath)
 - `method`: A string specifying the HTTP request method. Defaults to `'GET'`.
 - `path`: Request path. Defaults to `'/'`. Should include query string if any.
-  E.G. `'/index.html?page=12'`
+  E.G. `'/index.html?page=12'`. An exception is thrown when the request path
+  contains illegal characters. Currently, only spaces are rejected but that
+  may change in the future.
 - `headers`: An object containing request headers.
 - `auth`: Basic authentication i.e. `'user:password'` to compute an
   Authorization header.
diff --git a/lib/http.js b/lib/http.js
index c45af98b9deedf..a1abd1e2e0e6aa 100644
--- a/lib/http.js
+++ b/lib/http.js
@@ -52,11 +52,14 @@ var ClientRequest = exports.ClientRequest = client.ClientRequest;
 exports.request = function(options, cb) {
   if (typeof options === 'string') {
     options = url.parse(options);
-  } else if (options && options.path) {
-    options = util._extend({}, options);
-    options.path = encodeURI(options.path);
-    // encodeURI() doesn't escape quotes while url.parse() does. Fix up.
-    options.path = options.path.replace(/'/g, '%27');
+  } else if (options && options.path && / /.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
+    // why it only scans for spaces because those are guaranteed to create
+    // an invalid request.
+    throw new TypeError('Request path contains unescaped characters.');
   }
 
   if (options.protocol && options.protocol !== 'http:') {
diff --git a/test/simple/test-http-client-escape-path.js b/test/simple/test-http-client-unescaped-path.js
similarity index 50%
rename from test/simple/test-http-client-escape-path.js
rename to test/simple/test-http-client-unescaped-path.js
index 0178c823ba2936..376bca19c462b3 100644
--- a/test/simple/test-http-client-escape-path.js
+++ b/test/simple/test-http-client-unescaped-path.js
@@ -22,54 +22,8 @@
 var common = require('../common');
 var assert = require('assert');
 var http = require('http');
-var util = require('util');
 
-first();
-
-function first() {
-  test('/~username/', '/~username/', second);
-}
-function second() {
-  test('/\'foo bar\'', '/%27foo%20bar%27', third);
-}
-function third() {
-  var expected = '/%3C%3E%22%60%20%0D%0A%09%7B%7D%7C%5C%5E~%60%27';
-  test('/<>"` \r\n\t{}|\\^~`\'', expected);
-}
-
-function test(path, expected, next) {
-  function helper(arg, next) {
-    var server = http.createServer(function(req, res) {
-      assert.equal(req.url, expected);
-      res.end('OK');
-      server.close(next);
-    });
-    server.on('clientError', function(err) {
-      throw err;
-    });
-    server.listen(common.PORT, '127.0.0.1', function() {
-      http.get(arg);
-    });
-  }
-
-  // Go the extra mile to ensure that the behavior of
-  // http.get("http://example.com/...") matches http.get({ path: ... }).
-  test1();
-
-  function test1() {
-    console.log('as url: ' + util.inspect(path));
-    helper('http://127.0.0.1:' + common.PORT + path, test2);
-  }
-  function test2() {
-    var options = {
-      host: '127.0.0.1',
-      port: common.PORT,
-      path: path
-    };
-    console.log('as options: ' + util.inspect(options));
-    helper(options, done);
-  }
-  function done() {
-    if (next) next();
-  }
-}
+assert.throws(function() {
+  // Path with spaces in it should throw.
+  http.get({ path: 'bad path' }, assert.fail);
+}, /contains unescaped characters/);