diff --git a/lib/request.js b/lib/request.js index c97afb96..19c12c32 100644 --- a/lib/request.js +++ b/lib/request.js @@ -458,6 +458,17 @@ function patch(Request) { if (this._cacheURL !== this.url) { var protocol = this.isSecure() ? 'https://' : 'http://'; var base = protocol + (this.headers.host || 'localhost'); + + // The Host header and the request target are both client-supplied + // and Node's HTTP parser validates neither as a URL, so either can + // be something new URL() rejects - a "foo|bar" host, or an + // absolute-form target like "http://a:99999/x". getUrl() is called + // from Router.lookup on every request, where nothing catches, so + // throwing here takes down the process. Fall back instead. + if (!URL.canParse(base)) { + base = protocol + 'localhost'; + } + // For origin-form targets (starting with "/"), concatenate into // one URL string instead of new URL(this.url, base), which would // treat a leading "//" as a network-path reference and drop part @@ -467,10 +478,16 @@ function patch(Request) { // path as "//evil.com/x" on the original host. Other forms // (e.g. "*" for "OPTIONS *") don't start with "/" and keep the // two-arg form. - this._url = - this.url.charAt(0) === '/' + if (this.url.charAt(0) === '/') { + this._url = URL.canParse(base + this.url) ? new URL(base + this.url) - : new URL(this.url, base); + : new URL(base + '/'); + } else { + this._url = URL.canParse(this.url, base) + ? new URL(this.url, base) + : new URL(base + '/'); + } + this._cacheURL = this.url; } return this._url; diff --git a/test/request.test.js b/test/request.test.js index 31dd3db3..a4eaaec0 100644 --- a/test/request.test.js +++ b/test/request.test.js @@ -1,6 +1,9 @@ 'use strict'; /* eslint-disable func-names */ +var http = require('http'); +var net = require('net'); + var restifyClients = require('restify-clients'); var validator = require('validator'); @@ -374,3 +377,57 @@ test(module, 'getUrl result is cached across calls', function(t) { t.end(); }); }); + +test(module, 'should not crash when the Host header is not a valid URL host', function(t) { + SERVER.get('/hosthdr', function(req, res, next) { + res.send({ pathname: req.path() }); + return next(); + }); + + // The Host header is client-supplied and Node does not validate it as a + // URL authority. getUrl() must not throw on it: it is called from + // Router.lookup on every request, where nothing catches, so a throw + // here takes down the process. + var opts = { + agent: false, + headers: { Host: 'foo|bar' }, + hostname: '127.0.0.1', + method: 'GET', + path: '/hosthdr', + port: PORT + }; + + http.request(opts, function(res) { + t.equal(res.statusCode, 200); + res.resume(); + res.on('end', function() { + t.end(); + }); + }).end(); +}); + +test(module, 'should not crash when the request target is not a valid URL', function(t) { + // An absolute-form request target is client-supplied and Node's HTTP + // parser does not validate it as a URL either, so getUrl() must not + // throw on it. Needs a raw socket: http.request only emits + // origin-form targets. + var response = ''; + var socket = net.connect(PORT, '127.0.0.1', function() { + socket.write( + 'GET http://a:99999/x HTTP/1.1\r\n' + + 'Host: 127.0.0.1:' + + PORT + + '\r\n' + + 'Connection: close\r\n\r\n' + ); + }); + + socket.on('data', function(chunk) { + response += chunk; + }); + + socket.on('close', function() { + t.ok(/^HTTP\/1\.1 \d{3}/.test(response), 'server sent a response'); + t.end(); + }); +}); \ No newline at end of file