url: refactor truncating long hostname #9292

jun-oka · 2016-10-26T11:44:41Z

Checklist

make -j8 test (UNIX), or vcbuild test nosign (Windows) passes
commit message follows commit guidelines

Affected core subsystem(s)

lib url
test url

Description of change

I deleted and refactored the line around 417 in url.js which was truncating hostname if hostname length after . is more than 63. Thus url parse behavior for hostname should be same as browser's behavior. Now it doesn’t truncate hostname.

cjihrig

LGTM I suppose.

Trott

The moving-part-of-the-hostname-to-the-path bit is really surprising and, I would argue, buggy behavior. Is that documented anywhere? If not, I wonder if the correct thing to do is a (hopefully semver-patch) change that treats such URLs as invalid. (A hostname longer than 63 chars is invalid per RFC 1035.)

EDIT: It doesn't appear to be documented anywhere.

Trott · 2016-10-26T20:23:03Z

/cc @nodejs/http-parser I guess, although I suppose this isn't http-specific...

jun-oka · 2016-10-26T20:29:20Z

@Trott OK. I agree. I'll look into the document again.
The only way I could pass this URL on test was this way, so I thought it was true.
However, according to your opinion, it seems buggy.

Trott · 2016-10-26T20:30:23Z

Looks like this behavior was introduced (or at least refactored?) in db912813 by @mscdex. I'm not sure if it was unintentional and the intended behavior was something else, or if this is totally intended behavior on the theory that "garbage input results in garbage output". So maybe @mscdex can let us know?

jasnell · 2016-10-28T16:06:26Z

ping @mscdex

mscdex · 2016-10-28T16:43:48Z

I honestly don't remember, that was awhile ago. All I could go by is whatever tests we had at the time.

Trott · 2016-10-28T18:01:14Z

Looking at what new URL(url) does in the browser, it does not truncate the hostname component. I would say we should follow suit: Don't try to validate the hostname length.

I would say the path forward is:

rewrite the expected test result to preserve the hostname
update url.js to not truncate long hostnames
update the subsystem in the PR name and the commit message to be url
Get some input from @nodejs/lts on the semver implications of this. I think it's a bug fix, personally, but if anyone wants to argue that it should be treated as semver-major out of an abundance of caution, I'm OK with that.

What do others think?

Trott · 2016-10-28T18:02:35Z

I should add that, to me at least, the real objectionable thing here is not the truncation but the moving of the remainder of the hostname into the path. That just makes no sense to me. I cannot think of a situation where that is helpful. And I can think of situations where that can introduce bugs, for sure.

jun-oka · 2016-11-04T17:51:35Z

@Trott Thanks.
I will do this part.

rewrite the expected test result to preserve the hostname
update url.js to not truncate long hostnames
update the subsystem in the PR name and the commit message to be url

sam-github · 2016-11-10T17:48:10Z

re: #9521 (comment), I think truncation is bizarre.

I don't believe that the URL RFCs say that host names have to be _DNS_ host names, there are lots of ways of resolving names, so I think truncation is a bug.

Here's an example:

% tail /etc/hosts

127.0.0.1 ThisIsAnAbsurdlyLongHostNameButSurelyNoneOfThisShouldEndUpAsPartOfThePathBecauseWhoCouldPossiblyWantThatAMIRITE.com
% ping -c1 ThisIsAnAbsurdlyLongHostNameButSurelyNoneOfThisShouldEndUpAsPartOfThePathBecauseWhoCouldPossiblyWantThatAMIRITE.com
PING ThisIsAnAbsurdlyLongHostNameButSurelyNoneOfThisShouldEndUpAsPartOfThePathBecauseWhoCouldPossiblyWantThatAMIRITE.com (127.0.0.1) 56(84) bytes of data.
64 bytes from localhost (127.0.0.1): icmp_seq=1 ttl=64 time=0.065 ms

host names clearly are not the same as DNS names, they can be longer than DNS allows and still be resolveable.

Trott

LGTM if new CI is ✅

Trott · 2016-12-01T04:28:29Z

@cjihrig @jasnell This is a bit different (and, in my opinion, better) than what was originally submitted. Can you re-check the code to make sure it is still good by you?

Trott · 2016-12-01T04:29:06Z

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

Trott · 2016-12-01T04:29:23Z

/cc @sam-github

jun-oka · 2016-12-07T19:03:52Z

@cjihrig @jasnell @sam-github It would be nice if you can check it!

sam-github · 2016-12-07T19:08:43Z

I'm not familiar with the code, it would make more sense for @jasnell (who just wrote a URL decoder) or @mscdex (who might have implemented the truncation) looked at it.

mscdex · 2016-12-08T21:43:26Z

lib/url.js

+    const code = hostname.charCodeAt(i);
+    const isVc = code === 46/*.*/ ||
+                 code >= 48/*0*/ && code <= 57/*9*/ ||
+                 code >= 97/*a*/ && code <= 122/*\z*/ ||


There is a backslash before z here that needs to be removed.

OK. Thank you for finding.

mscdex · 2016-12-08T21:44:11Z

lib/url.js

+  for (var i = 0; i <= hostname.length; ++i) {
+    const code = hostname.charCodeAt(i);
+    const isVc = code === 46/*.*/ ||
+                 code >= 48/*0*/ && code <= 57/*9*/ ||


Please keep the parens around the groups of checks, IMHO it makes it easier to reason about.

Yes. I think so too.

mscdex · 2016-12-08T21:50:10Z

@Trott I'm just being more cautious is all, especially since this behavior has existed for a long time (even before my rewrite). If everyone else feels differently, then go ahead and remove the semver-major label.

jun-oka · 2016-12-09T05:19:32Z

@mscdex
Thank you for those reviewing.
I will look into them.

jun-oka · 2016-12-23T18:26:46Z

@mscdex @Trott
I have just updated!

Trott · 2016-12-24T18:13:56Z

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

mscdex · 2016-12-24T18:23:43Z

There's a couple of changes that still need to be made, ~~namely the for loop iteration condition and~~ the isVc variable naming.

mscdex · 2016-12-24T19:44:27Z

lib/url.js

+                 (code >= 65/*A*/ && code <= 90/*Z*/) ||
+                 code === 43/*+*/ ||
+                 code === 95/*_*/||
+                 code > 127;


While we're changing things in here, I think it might be better to re-prioritize the conditionals here to optimize for the common cases. Here is what I propose:

const isVc = (code >= 97/*a*/ && code <= 122/*z*/) || code === 46/*.*/ || (code >= 65/*A*/ && code <= 90/*Z*/) || (code >= 48/*0*/ && code <= 57/*9*/) || code === 45/*-*/ || code === 43/*+*/ || code === 95/*_*/|| code > 127;

@mscdex In my opinion, that makes more sense. It looks more common in this case. Thanks a lot.

Currently, around line 417 lib/url.js is truncating hostname and put the rest of hostname to the path if hostname length after `.` is equal or more than 63. This behavior is different from browser behavior. I changed the code so that it doesn’t truncate. I also added the test example which has more than 63 length in after `.` in hostname in test url.

…loop condition

…ostname

jun-oka · 2016-12-24T20:25:44Z

@mscdex I have applied your review which you proposed.

mscdex · 2016-12-24T20:28:43Z

lib/url.js

+                    (code >= 48/*0*/ && code <= 57/*9*/) ||
+                    code === 45/*-*/ ||
+                    code === 43/*+*/ ||
+                    code === 95/*_*/||


minor nit: there is a missing space before the ||. I missed that in my proposed example earlier.

jun-oka · 2016-12-24T20:41:02Z

@mscdex Thank you! I corrected it. I will be careful.

Trott · 2016-12-24T20:49:09Z

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

mscdex · 2016-12-25T00:39:59Z

LGTM

Trott · 2016-12-25T05:04:08Z

(arm-fanned failure on CI is infra related and unrelated to this change.)

Currently, around line 417 lib/url.js is truncating hostname and put the rest of hostname to the path if hostname length after `.` is equal or more than 63. This behavior is different from browser behavior. I changed the code so that it doesn’t truncate. I also added the test example which has more than 63 length in after `.` in hostname in test url. PR-URL: nodejs#9292 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Brian White <mscdex@mscdex.net>

Trott · 2016-12-25T05:07:19Z

Landed in c65d55f.

Two months in the making. 🎉

Currently, around line 417 lib/url.js is truncating hostname and put the rest of hostname to the path if hostname length after `.` is equal or more than 63. This behavior is different from browser behavior. I changed the code so that it doesn’t truncate. I also added the test example which has more than 63 length in after `.` in hostname in test url. PR-URL: nodejs#9292 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Brian White <mscdex@mscdex.net>

nodejs-github-bot added the test Issues and PRs related to the tests. label Oct 26, 2016

cjihrig approved these changes Oct 26, 2016

View reviewed changes

mscdex added the url Issues and PRs related to the legacy built-in url module. label Oct 26, 2016

jasnell approved these changes Oct 26, 2016

View reviewed changes

Trott requested changes Oct 26, 2016

View reviewed changes

Trott mentioned this pull request Nov 10, 2016

Changes to file: URL formatting in Node v7 cause problems for npm #9521

Closed

jun-oka changed the title ~~test: expand test coverage for url.js~~ url: stop to truncate long hostname into path Dec 1, 2016

jun-oka changed the title ~~url: stop to truncate long hostname into path~~ url: stop to truncate long hostname Dec 1, 2016

jun-oka changed the title ~~url: stop to truncate long hostname~~ url: stop truncating long hostname Dec 1, 2016

jun-oka force-pushed the test-url-coverage branch 4 times, most recently from de9563d to c186062 Compare December 1, 2016 04:24

jun-oka changed the title ~~url: stop truncating long hostname~~ url: refactor truncating long hostname Dec 1, 2016

Trott approved these changes Dec 1, 2016

View reviewed changes

mscdex added the semver-major PRs that contain breaking changes and should be released in the next major version. label Dec 7, 2016

mscdex reviewed Dec 8, 2016

View reviewed changes

jun-oka force-pushed the test-url-coverage branch from c186062 to 1a0f8f5 Compare December 23, 2016 17:53

jun-oka closed this Dec 24, 2016

jun-oka reopened this Dec 24, 2016

mscdex reviewed Dec 24, 2016

View reviewed changes

jun-oka added 3 commits December 24, 2016 12:21

test: refactor fix some typo, put parents for groups and correct for …

835a097

…loop condition

url: correct variable name, and re-prioritize conditionals to valid h…

ac7c6be

…ostname

jun-oka force-pushed the test-url-coverage branch from 1a0f8f5 to ac7c6be Compare December 24, 2016 20:23

mscdex reviewed Dec 24, 2016

View reviewed changes

url: add missing space

782e351

jasnell approved these changes Dec 24, 2016

View reviewed changes

Trott closed this Dec 25, 2016

joyeecheung mentioned this pull request Feb 2, 2017

test, url: break up test-url.js #11049

Closed

3 tasks

jasnell mentioned this pull request Apr 4, 2017

8.0.0 Release Proposal #12220

Closed

url: refactor truncating long hostname #9292

url: refactor truncating long hostname #9292

Conversation

jun-oka commented Oct 26, 2016 • edited Loading

Checklist

Affected core subsystem(s)

Description of change

cjihrig left a comment

Choose a reason for hiding this comment

Trott left a comment • edited Loading

Choose a reason for hiding this comment

Trott commented Oct 26, 2016

jun-oka commented Oct 26, 2016

Trott commented Oct 26, 2016

jasnell commented Oct 28, 2016

mscdex commented Oct 28, 2016

Trott commented Oct 28, 2016

Trott commented Oct 28, 2016

jun-oka commented Nov 4, 2016

sam-github commented Nov 10, 2016

Trott left a comment

Choose a reason for hiding this comment

Trott commented Dec 1, 2016

Trott commented Dec 1, 2016

Trott commented Dec 1, 2016

jun-oka commented Dec 7, 2016

sam-github commented Dec 7, 2016

mscdex Dec 8, 2016

Choose a reason for hiding this comment

jun-oka Dec 9, 2016

Choose a reason for hiding this comment

mscdex Dec 8, 2016 • edited Loading

Choose a reason for hiding this comment

jun-oka Dec 9, 2016

Choose a reason for hiding this comment

mscdex commented Dec 8, 2016 • edited Loading

jun-oka commented Dec 9, 2016

jun-oka commented Dec 23, 2016

Trott commented Dec 24, 2016

mscdex commented Dec 24, 2016 • edited Loading

mscdex Dec 24, 2016

Choose a reason for hiding this comment

jun-oka Dec 24, 2016

Choose a reason for hiding this comment

jun-oka commented Dec 24, 2016

mscdex Dec 24, 2016 • edited Loading

Choose a reason for hiding this comment

jun-oka commented Dec 24, 2016

Trott commented Dec 24, 2016

mscdex commented Dec 25, 2016

Trott commented Dec 25, 2016

Trott commented Dec 25, 2016

jun-oka commented Oct 26, 2016 •

edited

Loading

Trott left a comment •

edited

Loading

mscdex Dec 8, 2016 •

edited

Loading

mscdex commented Dec 8, 2016 •

edited

Loading

mscdex commented Dec 24, 2016 •

edited

Loading

mscdex Dec 24, 2016 •

edited

Loading