url: fix WHATWG URL host formatting error caused by port string

[peakji]

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

const myURL = new URL('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

1. RFC 3986 - Uniform Resource Identifier (URI): Generic Syntax
2. RFC 3987 - Internationalized Resource Identifiers (IRIs)
3. 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]

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]

cc @nodejs/url

[targos]

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

[apapirovski]

The logic needs to be fixed slightly.

[apapirovski]

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

[peakji]

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

[peakji]

> new url.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: '' }

[peakji]

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

[apapirovski]

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

[peakji]

You're right, URLContext uses null:

const { URL } = require('url');

const myURL = new URL('https://nodejs.org/en/');

const kContext = 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]

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]

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

[domenic]

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]

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]

@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]

Throwing is fine, I think.

[trivikr]

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

[addaleax]

@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]

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

[joyeecheung]

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

[joyeecheung]

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

[apapirovski]

Landed in 2a96ee2

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