Skip to content

Commit

Permalink
http{s}: don't connect to localhost on invalid URL
Browse files Browse the repository at this point in the history
If the URL passed to `http{s}.request` or `http{s}.get` is not properly
parsable by `url.parse`, we fall back to use `localhost` and port 80.
This creates confusing error messages like in this question
http://stackoverflow.com/q/32675907/1903116.

This patch throws an error message, if `url.parse` fails to parse the
URL properly.

Previous Discussion: #2966
PR-URL: #2967

Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
  • Loading branch information
thefourtheye authored and rvagg committed Oct 29, 2015
1 parent 6a04cc0 commit c9786bb
Show file tree
Hide file tree
Showing 3 changed files with 25 additions and 0 deletions.
3 changes: 3 additions & 0 deletions lib/_http_client.js
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,9 @@ function ClientRequest(options, cb) {

if (typeof options === 'string') {
options = url.parse(options);
if (!options.hostname) {
throw new Error('Unable to determine the domain name');
}
} else {
options = util._extend({}, options);
}
Expand Down
3 changes: 3 additions & 0 deletions lib/https.js
Original file line number Diff line number Diff line change
Expand Up @@ -163,6 +163,9 @@ exports.Agent = Agent;
exports.request = function(options, cb) {
if (typeof options === 'string') {
options = url.parse(options);
if (!options.hostname) {
throw new Error('Unable to determine the domain name');
}
} else {
options = util._extend({}, options);
}
Expand Down
19 changes: 19 additions & 0 deletions test/parallel/test-http-invalid-urls.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
'use strict';

require('../common');
const assert = require('assert');
const http = require('http');
const https = require('https');
const error = 'Unable to determine the domain name';

function test(host) {
['get', 'request'].forEach((method) => {
[http, https].forEach((module) => {
assert.throws(() => module[method](host, () => {
throw new Error(`${module}.${method} should not connect to ${host}`);
}), error);
});
});
}

['www.nodejs.org', 'localhost', '127.0.0.1', 'http://:80/'].forEach(test);

0 comments on commit c9786bb

Please sign in to comment.