Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

The deprecated url.format formats ipv6 localhost address wrongly #36654

Closed
chenxsan opened this issue Dec 28, 2020 · 3 comments
Closed

The deprecated url.format formats ipv6 localhost address wrongly #36654

chenxsan opened this issue Dec 28, 2020 · 3 comments
Labels
url Issues and PRs related to the legacy built-in url module.

Comments

@chenxsan
Copy link

chenxsan commented Dec 28, 2020

  • Version: I've tested it against Node.js 11, 12, 13, 14, 15
  • Platform: I've tested it on macOS Big Sur.
  • Subsystem:

What steps will reproduce the bug?

Create a new format.js file with content below:

const url = require('url')
console.log(url.format({
    protocol: 'http',
    hostname: '[::]'
}))

Then run it with node format.js.

How often does it reproduce? Is there a required condition?

Always.

What is the expected behavior?

Should output http://[::] instead.

What do you see instead?

It will output http://[[::]] which I believe is wrong.

Additional information

  1. I know url.format(urlobject) was deprecated as per https://nodejs.org/api/url.html#url_url_format_urlobject, but there're still libraries using it.
  2. url.format(new URL('http://[::]')) would output http://[::]/ as I expected.
@lpinca
Copy link
Member

lpinca commented Dec 28, 2020

The problem as I see it is not with url.format() but with using '[::]' as hostname. You can use '[::]' as host and it works as expected.

console.log(url.format({ protocol: 'http', host: '[::]' }));

Also both

url.format(new URL('http://[::]'));

and

url.format(url.parse('http://[::]'));

work as expected.

When using '[::]' as hostname, url.format() assumes that it is a literal IPv6 address and encloses it in the [ and ] characters.

We can document this behavior or make url.format() smarter making it use the hostname as is if it is already enclosed in [ and ] characters.

@lpinca lpinca added the url Issues and PRs related to the legacy built-in url module. label Dec 28, 2020
@chenxsan
Copy link
Author

If we recognize [::] as host, then we would fail to handle cases with port:

> const url = require('url');
> console.log(url.format({ protocol: 'http', host: '[::]', port: 3000 }));
http://[::]

It would be more reasonable to make url.format() smarter in my opinion according to https://tools.ietf.org/html/rfc2732:

To use a literal IPv6 address in a URL, the literal address should be
enclosed in "[" and "]" characters

@lpinca
Copy link
Member

lpinca commented Dec 28, 2020

host is the concatenation of hostname and port so it should be [::]:3000 and url.format() would handle that correctly. Also if you use :: or any other literal IPv6 address as hostname, url.format() would handle that correctly.

I find it a bit surprising that the WHATWG URL hostname getter returns [::] instead of :: but that is consistent with the specification.

Lxxyx added a commit to Lxxyx/node that referenced this issue Dec 30, 2020
danielleadams pushed a commit that referenced this issue Jan 12, 2021
Fixes: #36654

PR-URL: #36665
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Robert Nagy <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Daijiro Wachi <[email protected]>
Reviewed-By: Yash Ladha <[email protected]>
targos pushed a commit that referenced this issue May 1, 2021
Fixes: #36654

PR-URL: #36665
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Robert Nagy <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Daijiro Wachi <[email protected]>
Reviewed-By: Yash Ladha <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
url Issues and PRs related to the legacy built-in url module.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants