url: refactor truncating long hostname by jun-oka · Pull Request #9292 · nodejs/node

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

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

OK. Thank you for finding.

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

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

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:
constisVc=(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

+ (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 pathDec 1, 2016

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

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

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

jun-oka changed the title ~~url: stop truncating long hostname~~url: refactor truncating long hostnameDec 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 1a0f8f5Compare 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 ac7c6beCompare 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

Uh oh!

Uh oh!

Conversation

jun-oka commented Oct 26, 2016• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Affected core subsystem(s)

Description of change

Uh oh!

cjihrig left a comment

Choose a reason for hiding this comment

Uh oh!

Trott left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Trott commented Oct 26, 2016

Uh oh!

jun-oka commented Oct 26, 2016

Uh oh!

Trott commented Oct 26, 2016

Uh oh!

jasnell commented Oct 28, 2016

Uh oh!

mscdex commented Oct 28, 2016

Uh oh!

Trott commented Oct 28, 2016

Uh oh!

Trott commented Oct 28, 2016

Uh oh!

jun-oka commented Nov 4, 2016

Uh oh!

sam-github commented Nov 10, 2016

Uh oh!

Trott left a comment

Choose a reason for hiding this comment

Uh oh!

Trott commented Dec 1, 2016

Uh oh!

Trott commented Dec 1, 2016

Uh oh!

Trott commented Dec 1, 2016

Uh oh!

jun-oka commented Dec 7, 2016

Uh oh!

sam-github commented Dec 7, 2016

Uh oh!

mscdexDec 8, 2016

Choose a reason for hiding this comment

Uh oh!

jun-okaDec 9, 2016

Choose a reason for hiding this comment

Uh oh!

mscdexDec 8, 2016• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jun-okaDec 9, 2016

Choose a reason for hiding this comment

Uh oh!

mscdex commented Dec 8, 2016• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jun-oka commented Dec 9, 2016

Uh oh!

jun-oka commented Dec 23, 2016

Uh oh!

Trott commented Dec 24, 2016

Uh oh!

mscdex commented Dec 24, 2016• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mscdexDec 24, 2016

Choose a reason for hiding this comment

Uh oh!

jun-okaDec 24, 2016

Choose a reason for hiding this comment

Uh oh!

jun-oka commented Dec 24, 2016

Uh oh!

jun-oka commented Oct 26, 2016•
edited
Loading

Trott left a comment •
edited
Loading

mscdexDec 8, 2016•
edited
Loading

mscdex commented Dec 8, 2016•
edited
Loading

mscdex commented Dec 24, 2016•
edited
Loading

mscdexDec 24, 2016•
edited
Loading