url: fix WHATWG URL host formatting error caused by port string by peakji · Pull Request #20493 · nodejs/node

peakji · 2018-05-03T07:32:19Z

The current url.format implementation will return an invalid URL string without the host if there is a port and unicode: true.

This unexpected behavior is caused by domainToUnicode, which expects a hostname instead of a host string according to node_url.cc.

For example: url.domainToUnicode('xn--3kq375bonc.com') will be correctly converted into Chinese '搞事情.com', but url.domainToUnicode('xn--3kq375bonc.com:80') would produce an empty string.

Code to reproduce

'use strict';const{format,URL}=require('url');constmyURL=newURL('http://examlpe.com:8080/path');console.log(format(myURL,{unicode: false}));// OK: 'http://examlpe.com:8080/path'console.log(format(myURL,{unicode: true}));// Error: 'http:///path' <-- the host is missing!

References

RFC 3986 - Uniform Resource Identifier (URI): Generic Syntax
RFC 3987 - Internationalized Resource Identifiers (IRIs)
https://url.spec.whatwg.org/#concept-host

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
commit message follows commit guidelines

peakji · 2018-05-03T08:34:22Z

This is a pretty serious bug and I'm wondering if these ambiguous names (host vs hostname) affected more stuffs in the code base?

ping @TimothyGu @jasnell @joyeecheung

AyushG3112 · 2018-05-03T08:46:00Z

cc @nodejs/url

targos · 2018-05-03T10:43:04Z

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

apapirovski

The logic needs to be fixed slightly.

apapirovski · 2018-05-03T10:54:15Z

 ret += options.unicode ?
- domainToUnicode(this.host) : this.host;
+ domainToUnicode(this.hostname) : this.hostname;
+ if (ctx.port !== '')


I think this should be ctx.port !== null?

The WHATWG URL API uses '' as the empty value, not null.

>newurl.URL("https://nodejs.org/en/")URL{href: 'https://nodejs.org/en/',origin: 'https://nodejs.org',protocol: 'https:',username: '',password: '',host: 'nodejs.org',hostname: 'nodejs.org',port: '',pathname: '/en/',search: '',searchParams: URLSearchParams{},hash: ''}

Oh my bad! This is the internal context...

apapirovski · 2018-05-03T11:09:54Z

 const ctx = this[context];
 var ret = ctx.scheme;
- if (ctx.host !== null) {
+ if (ctx.hostname !== null) {


This should also be ctx.host !== null. The hostname change only applies below.

You're right, URLContext uses null:
const{URL}=require('url');constmyURL=newURL('https://nodejs.org/en/');constkContext=Object.getOwnPropertySymbols(myURL)[0];console.log(myURL[kContext]);
URLContext { flags: 400, scheme: 'https:', username: '', password: '', host: 'nodejs.org', port: null, path: [ 'en', '' ], query: null, fragment: null }

apapirovski · 2018-05-03T11:16:50Z

FWIW you might want to contribute your test case back to https://github.com/w3c/web-platform-tests/tree/master/url — clearly this is a case that's not covered by the official tests.

apapirovski

Should be good to go now.

apapirovski · 2018-05-03T11:26:13Z

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

domenic · 2018-05-03T15:25:53Z

FWIW you might want to contribute your test case back to w3c/web-platform-tests:url@master — clearly this is a case that's not covered by the official tests.

Although I agree with this general sentiment, the url.format() API is nonstandard, so it doesn't make sense to test this in the official tests.

joyeecheung · 2018-05-03T17:14:48Z

Yeah, the unicode option of url.format is used to expose the internal "domain to Unicode" operation of WHATWG URL, in the context of that spec when the URL needs to be serialized the "domain to Unicode" operation is never run on the hostname, it's only used on the domain in URL rendering ~~which is probably not even a thing in the context of Node.js~~ EDIT: right it's probably a thing in say, REPLs or CLIs, or in places like electron or node-canvas.

Also as the name implies domainToUnicode runs on domains, and xn--3kq375bonc.com:80 is not a valid domain. The spec only talks about signaling validation errors and agents are encouraged to report them somewhere. In our parser we signal the error by returning an empty string.

joyeecheung · 2018-05-03T17:27:33Z

@jasnell @nodejs/url I don't know if it makes sense to throw in the non-internal part of our JS API when the non-standard methods like domainToUnicode or domainToASCII fail? At least if we throw then this bug would be more obvious.

jasnell · 2018-05-03T17:43:52Z

Throwing is fine, I think.

trivikr · 2018-05-05T11:23:51Z

CI re-run for node-test-commit-arm-fanned: https://ci.nodejs.org/job/node-test-commit-arm-fanned/977/

addaleax · 2018-05-06T19:12:49Z

@joyeecheung I can’t really tell whether your suggestion is something for this PR or not – should we wait with landing this or is it good to go?

peakji · 2018-05-07T04:50:02Z

@trivikr Could you help me to re-run node-test-commit as well?

joyeecheung · 2018-05-07T06:54:29Z

@addaleax This PR is good to go, the error handling can be changed in another PR

joyeecheung · 2018-05-07T06:56:02Z

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

apapirovski · 2018-05-07T08:41:01Z

Landed in 2a96ee2

Congrats on your first commit in Node.js core and becoming a Contributor! 🎉

The current url.format implementation will return an invalid URL string without the host if there is a port and unicode: true. This unexpected behavior is caused by domainToUnicode, which expects a hostname instead of a host string according to node_url.cc. Adds both a fix and a test for the issue. PR-URL: #20493 Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>

peakji added 2 commits May 3, 2018 15:13

url: fix WHATWG host formatting error
6135fca

test: format WHATWG URL with port
450272c

nodejs-github-bot added dont-land-on-v4.x whatwg-url Issues and PRs related to the WHATWG URL implementation. labels May 3, 2018

targos approved these changes May 3, 2018
View reviewed changes

cjihrig approved these changes May 3, 2018
View reviewed changes

jasnell approved these changes May 3, 2018
View reviewed changes

url: fix the missing semicolon
75f3f68

apapirovski reviewed May 3, 2018
View reviewed changes

apapirovski requested a review from TimothyGu May 3, 2018 11:10

url: fix WHATWG formatting logic
3db8aac

apapirovski approved these changes May 3, 2018
View reviewed changes

apapirovski added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label May 3, 2018

trivikr approved these changes May 3, 2018
View reviewed changes

joyeecheung approved these changes May 3, 2018
View reviewed changes

TimothyGu approved these changes May 3, 2018
View reviewed changes

BridgeAR approved these changes May 5, 2018
View reviewed changes

watilde approved these changes May 5, 2018
View reviewed changes

apapirovski closed this May 7, 2018

MylesBorins mentioned this pull request May 8, 2018
v10.1.0 proposal #20606
Merged

apapirovski mentioned this pull request May 22, 2018
Inconsistent documentation: hostname vs host #20892
Closed

Uh oh!

Uh oh!

Conversation

peakji commented May 3, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code to reproduce

References

Checklist

Uh oh!

peakji commented May 3, 2018

Uh oh!

AyushG3112 commented May 3, 2018

Uh oh!

targos commented May 3, 2018

Uh oh!

apapirovski left a comment

Choose a reason for hiding this comment

Uh oh!

apapirovskiMay 3, 2018

Choose a reason for hiding this comment

Uh oh!

peakjiMay 3, 2018• 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!

peakjiMay 3, 2018

Choose a reason for hiding this comment

Uh oh!

peakjiMay 3, 2018

Choose a reason for hiding this comment

Uh oh!

apapirovskiMay 3, 2018

Choose a reason for hiding this comment

Uh oh!

peakjiMay 3, 2018

Choose a reason for hiding this comment

Uh oh!

apapirovski commented May 3, 2018

Uh oh!

apapirovski left a comment

Choose a reason for hiding this comment

Uh oh!

apapirovski commented May 3, 2018

Uh oh!

domenic commented May 3, 2018

Uh oh!

joyeecheung commented May 3, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

joyeecheung commented May 3, 2018

Uh oh!

jasnell commented May 3, 2018

Uh oh!

trivikr commented May 5, 2018

Uh oh!

addaleax commented May 6, 2018

Uh oh!

peakji commented May 7, 2018

Uh oh!

joyeecheung commented May 7, 2018

Uh oh!

joyeecheung commented May 7, 2018

Uh oh!

apapirovski commented May 7, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

14 participants

peakji commented May 3, 2018•
edited
Loading

peakjiMay 3, 2018•
edited
Loading

joyeecheung commented May 3, 2018•
edited
Loading