http{s}: fix connecting to localhost if URL is invalid #2967

thefourtheye · 2015-09-20T07:40:27Z

If the URL passed to http.request 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.

Previous Discussion: #2966

ChALkeR · 2015-09-20T07:52:10Z

~~Is this really a semver-major? The old behavior looks like a bug, is there really a case when it's desired?~~

Ah, now we throw an Error immediately when before it was passed to the callback. Seems reasonable.

thefourtheye · 2015-09-20T07:55:21Z

@ChALkeR hmmm, to think of it, you may be right. Nobody would have preferred connecting to localhost when they actually meant to connect to a different URL. So, I guess this would not break a lot of code out there. I'll remove the major tag.

ChALkeR · 2015-09-20T07:57:10Z

@thefourtheye Yes, even with the «pass error to callback» → «throw immediately» change it looks more like a bugfix to me. Let's see what the others will say.

@nodejs/collaborators

targos · 2015-09-20T08:29:24Z

There are probably applications out there that request('localhost'). They get what they expect for the wrong reason but this will still break them.

thefourtheye · 2015-09-20T08:49:02Z

@targos That's a good thing to have in the tests. I included them. Thanks :-)

trevnorris · 2015-09-21T17:56:03Z

This does come closer to the wep API new URL() to throw on invalid url's, and it's the path of least surprise. Though it may currently may have been undefined behavior, but that doesn't mean it's a bug fix. I'm +1 for the change, but am going to lend towards semver-major, but am open for discussion.

targos · 2015-09-25T08:17:47Z

I also think it should be semver-major

yosuke-furukawa · 2015-09-25T13:14:48Z

+1 semver-major
+1 throw an error immediately
But if urls are invalid, url.parse should throw the error.

thefourtheye · 2015-09-28T17:12:52Z

Looks like this is seen mostly as semver-major change. @ChALkeR Are you okay with landing this in major and #2966 in master now?

ChALkeR · 2015-09-28T21:21:26Z

lib/_http_client.js

@@ -22,6 +22,9 @@ function ClientRequest(options, cb) {

 if (typeof options === 'string') {
 options = url.parse(options);
+ if (!options.host && !options.hostname) {


Why not just check options.hostname? Is there a reason for allowing empty hostnames at this point?
Example: url.parse('http://:80/'): host: ':80', hostname: '', the only difference per doc between them is the presence of port in host.

Btw, how will the following code behave in such case (when the effective host includes the port, but not the domain)?

@ChALkeR Good point. I also think we need to check only the hostname here. I changed the check and included a test as well.

Btw, how will the following code behave in such case (when the effective host includes the port, but not the domain)?

I tried http.get('http://:80/') and got the error Error: getaddrinfo ENOTFOUND :80 :80:80

ChALkeR · 2015-09-28T21:30:51Z

@thefourtheye

Looks like this is seen mostly as semver-major change. @ChALkeR Are you okay with landing this in major and #2966 in master now?

LGTM (as a semver-major) with notes and if tests pass. Btw, don't semver-major entries go directly into master now? If they do, then what is the purpose for landing #2966 in master?

Not sure about #2966. If it gets merged, it would make sense to merge that directly into 4.x branch and backport to 0.12/0.10 if applicable, and put a notice that this behavior is changed in 5.x. But I see no actual need in that, not sure about others. Let's move that discussion to #2966.

ChALkeR · 2015-09-28T21:34:54Z

@targos
request\(.localhost regex:

min-request-1.4.1.tgz/test.js:29:               request('localhost:' + port, {body: data}, function(err, res, body) {
min-request-1.4.1.tgz/test.js:36:               request('localhost:' + port, {
mitm-servers-1.0.3.tgz/test/test.js:66:    return request('localhost:1234')
rest-stop-framework-1.0.0.tgz/test/server_test.js:17:    chai.request('localhost:3000')
rest-stop-framework-1.0.0.tgz/test/server_test.js:30:    chai.request('localhost:3000')
rest-stop-framework-1.0.0.tgz/test/server_test.js:42:    chai.request('localhost:3000')
rest-stop-framework-1.0.0.tgz/test/server_test.js:55:    chai.request('localhost:3000')
rest-stop-framework-1.0.0.tgz/test/server_test.js:68:    chai.request('localhost:3000')

request\(.127\.0\.0\.1 regex:

cadvisor-to-metric-server-0.0.4.tgz/demo/test.js:92:request('127.0.0.1:8080/api/v1.0/machine', function(err, machineInfo) {
cadvisor-to-metric-server-0.0.4.tgz/demo/test.js:94:    request('127.0.0.1:8080/api/v1.2/docker/' + id, function(err, data) {

thefourtheye · 2015-10-01T11:08:16Z

don't semver-major entries go directly into master now

@ChALkeR Yes, they do. But if the PR is tagged as semver-major, it will be picked only during a major release cut. Till we release v5, we may want to tell users that this is the expected behavior. That is why #2966 is needed.

ChALkeR · 2015-10-04T15:22:49Z

@thefourtheye Once again: this change LGTM, and once you decide to land it, it should be landed to master. There is no separate «major» branch, nor there is a reason for delaying landing this till the release.

Also, this change could include #2966 without the note:

 `options` can be an object or a string. If `options` is a string, it is
-automatically parsed with [url.parse()][].
+automatically parsed with [url.parse()][] and it must be a valid complete URL,
+including protocol and complete domain name or IP address.

ChALkeR · 2015-10-04T15:23:57Z

Ah, also cc @rvagg for semver-major.

Fishrock123 · 2015-10-06T03:07:49Z

lib/_http_client.js

@@ -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);


Don't we also need the check here? Why not just check after these if statements?

@Fishrock123 At this place, if user doesn't pass hostname, then the default value undefined will be used

Won't that default to localhost again? Isn't that what this is trying to prevent? I don't understand.

@Fishrock123 If the user doesn't specify any hostname in the options, then defaulting to localhost is expected. But when I give google.com (without http://), it should not take me to localhost.

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: nodejs#2966 PR-URL: nodejs#2967

Trott · 2015-10-26T20:43:09Z

cc @nodejs/http

dougwilson · 2015-10-26T20:47:05Z

I'm 👍 for this change.

evanlucas · 2015-10-26T21:31:56Z

LGTM if the CI is happy

jasnell · 2015-10-26T21:38:22Z

LGTM if CI is green

thefourtheye · 2015-10-27T02:42:06Z

CI Run: https://ci.nodejs.org/job/node-test-pull-request/624/

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>

thefourtheye · 2015-10-27T03:19:46Z

The failures seem unrelated to this change. Thanks for the review. Landed at 437930c

thefourtheye · 2015-10-27T03:21:04Z

It would be better if we get more eyes on #2966 as well. @jasnell Will this be backported to LTS as well?

jasnell · 2015-10-27T04:57:17Z

@thefourtheye .. not as a semver-major

thefourtheye · 2015-10-27T05:53:58Z

@jasnell oops right...

rvagg · 2015-10-28T01:15:04Z

@thefourtheye can we avoid http{s} in future please and stick to vanilla names in commit simmaries? It makes sorting and grokking changes a bit tricky and http almost always means https anyway.

thefourtheye · 2015-10-28T01:48:32Z

@rvagg Oh, sorry about that. I'll use only proper subsystem names now on.

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: nodejs#2966 PR-URL: nodejs#2967 Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com> Reviewed-By: Evan Lucas <evanlucas@me.com> Reviewed-By: James M Snell <jasnell@gmail.com>

chalkers · 2016-11-10T19:35:02Z

I believe this PR introduced a bug.

In the following example code in Node.js 4.0.0 or earlier...

const request = http.get("localhost", res => { });

request.on('error', e => console.error(e.message));

...requires a try catch now in 5.0.0 or later

try {
  const request = http.get("localhost", res => { });
  request.on('error', e => console.error(e.message));
} catch (e) {
  console.error(e.message);
}

I would expect all parse error would be emitted like all the other errors.

Happy to do a pull request if you agree :)

bnoordhuis · 2016-11-10T21:03:02Z

@chalkers Mmm, it's actually closer to the core design philosophy of throwing early on bad inputs. Asynchronous error events are for operational errors, not logic errors.

chalkers · 2016-11-28T17:29:02Z

@bnoordhuis Interesting. Do you have any other examples in the Node.js API that work that way?

I.E.

try {
   //some event emitter
}  catch (e) {
 
}

I ask because it's confusing a lot of students or new users to Node.js. I've been teaching Node.js on Treehouse for over two years and we started on 0.12. If people follow along they tend to have upgraded to 6+ (and we did naively in our interactive coding environment) expecting this would still be compatible.

If there are more examples of this pattern it would be good to know as an educator. I'm also refreshing the Node.js Basics course right now. If this is an unintended consequence of this patch, as I said before, I'd be more than happy to create a pull request resolving the issue.

bnoordhuis · 2016-11-28T21:43:31Z

@chalkers As a rule of thumb:

passing bad arguments to an API function throws an exception
passing good arguments that fail at run-time emits an asynchronous 'error' event.

Most APIs in Node.js work this way (although there are shades of gray, of course.) Taking this particular issue as an example:

a malformed URL throws an exception
a valid URL with a hostname that doesn't resolve to a DNS record results in an 'error' event

The fs module blatantly deviates from that rule with its 'does this path contain nul bytes?' check - it emits an asynchronous 'error' event even though it's clearly a bad input - but that is the only real outlier I can think of (and I still think that was a stupid move.)

chalkers · 2016-11-30T22:47:18Z

Thanks for clearing that up - I'll be sure to mention the philosophy in the update :)

thefourtheye added the http Issues or PRs related to the http subsystem. label Sep 20, 2015

thefourtheye force-pushed the http-fix-invalid-urls branch from 7a147ec to b51965e Compare September 20, 2015 07:49

thefourtheye added https Issues or PRs related to the https subsystem. semver-major PRs that contain breaking changes and should be released in the next major version. labels Sep 20, 2015

thefourtheye changed the title ~~http: fix connecting to localhost if URL is invalid~~ http{s}: fix connecting to localhost if URL is invalid Sep 20, 2015

thefourtheye removed the semver-major PRs that contain breaking changes and should be released in the next major version. label Sep 20, 2015

thefourtheye mentioned this pull request Sep 21, 2015

Release Proposal: v4.1.1 #2943

Closed

thefourtheye mentioned this pull request Sep 25, 2015

Discussion: LTS & v5 release planning #3000

Closed

8 tasks

targos mentioned this pull request Sep 25, 2015

doc: mention the behaviour if URL is invalid #2966

Closed

ChALkeR reviewed Sep 28, 2015
View reviewed changes

thefourtheye force-pushed the http-fix-invalid-urls branch from 157e5d2 to 7cd15da Compare October 1, 2015 09:32

ChALkeR added the semver-major PRs that contain breaking changes and should be released in the next major version. label Oct 4, 2015

Fishrock123 reviewed Oct 6, 2015
View reviewed changes

thefourtheye closed this Oct 27, 2015

thefourtheye deleted the http-fix-invalid-urls branch October 27, 2015 03:19

http{s}: fix connecting to localhost if URL is invalid #2967

http{s}: fix connecting to localhost if URL is invalid #2967

Conversation

thefourtheye commented Sep 20, 2015

ChALkeR commented Sep 20, 2015

thefourtheye commented Sep 20, 2015

ChALkeR commented Sep 20, 2015

targos commented Sep 20, 2015

thefourtheye commented Sep 20, 2015

trevnorris commented Sep 21, 2015

targos commented Sep 25, 2015

yosuke-furukawa commented Sep 25, 2015

thefourtheye commented Sep 28, 2015

ChALkeR Sep 28, 2015

Choose a reason for hiding this comment

thefourtheye Oct 1, 2015

Choose a reason for hiding this comment

ChALkeR commented Sep 28, 2015

ChALkeR commented Sep 28, 2015

thefourtheye commented Oct 1, 2015

ChALkeR commented Oct 4, 2015

ChALkeR commented Oct 4, 2015

Fishrock123 Oct 6, 2015

Choose a reason for hiding this comment

thefourtheye Oct 6, 2015

Choose a reason for hiding this comment

Fishrock123 Oct 6, 2015

Choose a reason for hiding this comment

thefourtheye Oct 6, 2015

Choose a reason for hiding this comment

Trott commented Oct 26, 2015

dougwilson commented Oct 26, 2015

evanlucas commented Oct 26, 2015

jasnell commented Oct 26, 2015

thefourtheye commented Oct 27, 2015

thefourtheye commented Oct 27, 2015

thefourtheye commented Oct 27, 2015

jasnell commented Oct 27, 2015

thefourtheye commented Oct 27, 2015

rvagg commented Oct 28, 2015

thefourtheye commented Oct 28, 2015

chalkers commented Nov 10, 2016 • edited Loading

bnoordhuis commented Nov 10, 2016

chalkers commented Nov 28, 2016

bnoordhuis commented Nov 28, 2016

chalkers commented Nov 30, 2016

chalkers commented Nov 10, 2016 •

edited

Loading