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

fix: ipv6 addresses parsing #1805

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

perrin4869
Copy link
Contributor

@perrin4869 perrin4869 commented May 13, 2024

#1803 broke the parsing of ipv6 addresses such as http://[::]:80. Before that PR, using node:url, we would get:

> const { parse } = require("node:url")
undefined
> parse("http://[::]:80")
Url {
  protocol: 'http:',
  slashes: true,
  auth: null,
  host: '[::]:80',
  port: '80',
  hostname: '::',
  hash: null,
  search: null,
  query: null,
  pathname: '/',
  path: '/',
  href: 'http://[::]:80/'
}

With the URL class, however, the hostname becomes:

> new URL("http://[::]:80")
URL {
  href: 'http://[::]/',
  origin: 'http://[::]',
  protocol: 'http:',
  username: '',
  password: '',
  host: '[::]',
  hostname: '[::]',
  port: '',
  pathname: '/',
  search: '',
  searchParams: URLSearchParams {},
  hash: ''
}

This hostname [::] is not compatible with the http.request function. I added some normalization functionality here to return the previous behavior.

Edit: hm... it is a bit hard to add regression tests here, since all the tests seem to run against a single server that doesn't seem to listen on [::]

@gramakri
Copy link

Can confirm. This broke in 9.0.2 .

@titanism
Copy link
Collaborator

If you can get the tests to pass I can merge

@perrin4869
Copy link
Contributor Author

ok, I managed to add some regression tests! they don't seem to work on http2 so I disabled those

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants