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

can't parse a domain with underscore #347

Closed
vzaidman opened this issue Aug 9, 2017 · 9 comments
Closed

can't parse a domain with underscore #347

vzaidman opened this issue Aug 9, 2017 · 9 comments

Comments

@vzaidman
Copy link
Contributor

vzaidman commented Aug 9, 2017

banner

probably because of this code

  // allowed hostname characters according to RFC 3986
  // ALPHA DIGIT "-" "." "_" "~" "!" "$" "&" "'" "(" ")" "*" "+" "," ";" "=" %encoded
  // I've never seen a (non-IDN) hostname other than: ALPHA DIGIT . -
  URI.invalid_hostname_characters = /[^a-zA-Z0-9\.-]/;
@rodneyrehm
Copy link
Member

Do you want to send a PR for that? (maybe also adding an URL with a _ in its domain to urls.js for testing)

@vzaidman
Copy link
Contributor Author

vzaidman commented Aug 9, 2017

sure ill send a pr

@vzaidman
Copy link
Contributor Author

vzaidman commented Aug 9, 2017

submitted a PR with a relevant test:
#348

@vzaidman
Copy link
Contributor Author

vzaidman commented Aug 9, 2017

@rodneyrehm is there a possibility to have

host:name.something.com

?

@rodneyrehm
Copy link
Member

the hostname could be an IPv6 address and that may contain :. A domain name may not - as far as I know - but then, I wasn't aware _ was legal either…

@vzaidman
Copy link
Contributor Author

vzaidman commented Aug 9, 2017

host:name.something.com currently fails in tests because name.something.com is considered the port
so the hostname i test is now:

    var u = new URI('http://some.complex_host-name123.org/');

@rodneyrehm
Copy link
Member

When you opened the PR I thought I'd immediately close the issue because _ is not legal in domain names per RFC 1034. However, the URL spec seems to permit it and Chrome's implementation parses it. That said, : is considered an illegal code point

@vzaidman
Copy link
Contributor Author

vzaidman commented Aug 9, 2017

gotcha. so the PR can be merged i think.

@rodneyrehm
Copy link
Member

fixed in v1.18.12

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

No branches or pull requests

2 participants