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

URL: Don't accept port numbers for file urls #10608

Closed
jasnell opened this issue Jan 4, 2017 · 6 comments
Closed

URL: Don't accept port numbers for file urls #10608

jasnell opened this issue Jan 4, 2017 · 6 comments
Assignees
Labels
whatwg-url Issues and PRs related to the WHATWG URL implementation.

Comments

@jasnell
Copy link
Member

jasnell commented Jan 4, 2017

Per: whatwg/url#97

Given the test case:

var m = new url.URL('file:///example/foo');
m.host = 'example.net:81';

The resulting URL is:

URL {
  href: file://example.net:81/example/foo
  protocol: file:
  hostname: example.net
  port: 81
  pathname: /example/foo
}

The host setter needs to take into consideration the fact that this is a file URL. PR incoming shortly.

@jasnell jasnell added the url Issues and PRs related to the legacy built-in url module. label Jan 4, 2017
@jasnell jasnell self-assigned this Jan 4, 2017
@domenic
Copy link
Contributor

domenic commented Jan 4, 2017

It seems like the spec isn't completely decided here though, from the discussions in whatwg/url#97, so it might be worth holding off?

@jasnell
Copy link
Member Author

jasnell commented Jan 4, 2017

Ok. I can prepare the basic PR but I'll keep an eye out for the spec changes

@domenic
Copy link
Contributor

domenic commented Jan 4, 2017

Awesome. I just want to say that I really love your responsiveness to spec updates; it's important that living standards have living implementations to benefit from these kind of bug fixes, and you've done a great job ensuring that stays the case.

@jasnell
Copy link
Member Author

jasnell commented Jan 4, 2017

Definitely wanting to make sure we keep up. One favor I would ask: while the new URL parser in Node.js is still experimental, significant changes are easier to make. Once it moves up to a fully supported API, we will need to be more careful about potentially breaking changes. We'll need to make sure that we can go through a proper deprecation cycle for old behavior if necessary which means getting as much advance notice of breaking changes as possible. I know that breaking changes are generally avoided as much as possible in the WHATWG specs but if we can continue to get active heads up when changes are being considered, it would be extremely helpful! I really appreciate the help you've already provided!!

@joyeecheung joyeecheung added the whatwg-url Issues and PRs related to the WHATWG URL implementation. label Jan 5, 2017
@mscdex mscdex removed the url Issues and PRs related to the legacy built-in url module. label Jan 5, 2017
@TimothyGu
Copy link
Member

cc @nodejs/url

@joyeecheung
Copy link
Member

This has landed in the spec.
Spec: whatwg/url#224
Test: web-platform-tests/wpt#4696

jasnell added a commit to jasnell/node that referenced this issue Apr 21, 2017
Technically, file URLs are not permitted to have a port. There is
currently an ambiguity in the URL spec. In the current spec
having a port in a file URL is undefined behavior. In the current
implementation, the port is ignored and handled as if it were part
of the host name. This will be changing once the ambiguity is
resolved in the spec. The spec change may involve either ignoring the
port if specified or throwing with an Invalid URL error if the port
is specified. For now, this test verifies the currently implemented
behavior.

Fixes: nodejs#10608
TimothyGu added a commit to TimothyGu/node that referenced this issue Apr 23, 2017
- Update to spec
  - Add opaque hosts
  - File state did not correctly deal with lack of base URL
  - Cleanup API for file and non-special URLs
  - Allow % and IPv6 addresses in non-special URL hosts
  - Use specific names for percent-encode sets
  - Add empty host concept for file and non-special URLs
  - Clarify IPv6 serializer
- Fix existing mistakes
  - Add missing ':' to forbidden host code point list.
  - Correct IPv4 parser empty label behavior
- Maintain type equivalence in URLContext with spec
  - scheme, username, and password should always be strings
  - host, port, query, and fragment may be strings or null
  - Align scheme state more closely with the spec
  - Make sure the `special` variable is always synced with
    URL_FLAG_SPECIAL.

PR-URL: nodejs#12523
Fixes: nodejs#10608
Fixes: nodejs#10634
Refs: whatwg/url#185
Refs: whatwg/url#225
Refs: whatwg/url#224
Refs: whatwg/url#218
Refs: whatwg/url#243
Refs: whatwg/url#260
Refs: whatwg/url#268
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
evanlucas pushed a commit that referenced this issue Apr 25, 2017
- Update to spec
  - Add opaque hosts
  - File state did not correctly deal with lack of base URL
  - Cleanup API for file and non-special URLs
  - Allow % and IPv6 addresses in non-special URL hosts
  - Use specific names for percent-encode sets
  - Add empty host concept for file and non-special URLs
  - Clarify IPv6 serializer
- Fix existing mistakes
  - Add missing ':' to forbidden host code point list.
  - Correct IPv4 parser empty label behavior
- Maintain type equivalence in URLContext with spec
  - scheme, username, and password should always be strings
  - host, port, query, and fragment may be strings or null
  - Align scheme state more closely with the spec
  - Make sure the `special` variable is always synced with
    URL_FLAG_SPECIAL.

PR-URL: #12523
Fixes: #10608
Fixes: #10634
Refs: whatwg/url#185
Refs: whatwg/url#225
Refs: whatwg/url#224
Refs: whatwg/url#218
Refs: whatwg/url#243
Refs: whatwg/url#260
Refs: whatwg/url#268
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
evanlucas pushed a commit that referenced this issue May 1, 2017
- Update to spec
  - Add opaque hosts
  - File state did not correctly deal with lack of base URL
  - Cleanup API for file and non-special URLs
  - Allow % and IPv6 addresses in non-special URL hosts
  - Use specific names for percent-encode sets
  - Add empty host concept for file and non-special URLs
  - Clarify IPv6 serializer
- Fix existing mistakes
  - Add missing ':' to forbidden host code point list.
  - Correct IPv4 parser empty label behavior
- Maintain type equivalence in URLContext with spec
  - scheme, username, and password should always be strings
  - host, port, query, and fragment may be strings or null
  - Align scheme state more closely with the spec
  - Make sure the `special` variable is always synced with
    URL_FLAG_SPECIAL.

PR-URL: #12523
Fixes: #10608
Fixes: #10634
Refs: whatwg/url#185
Refs: whatwg/url#225
Refs: whatwg/url#224
Refs: whatwg/url#218
Refs: whatwg/url#243
Refs: whatwg/url#260
Refs: whatwg/url#268
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
evanlucas pushed a commit that referenced this issue May 2, 2017
- Update to spec
  - Add opaque hosts
  - File state did not correctly deal with lack of base URL
  - Cleanup API for file and non-special URLs
  - Allow % and IPv6 addresses in non-special URL hosts
  - Use specific names for percent-encode sets
  - Add empty host concept for file and non-special URLs
  - Clarify IPv6 serializer
- Fix existing mistakes
  - Add missing ':' to forbidden host code point list.
  - Correct IPv4 parser empty label behavior
- Maintain type equivalence in URLContext with spec
  - scheme, username, and password should always be strings
  - host, port, query, and fragment may be strings or null
  - Align scheme state more closely with the spec
  - Make sure the `special` variable is always synced with
    URL_FLAG_SPECIAL.

PR-URL: #12523
Fixes: #10608
Fixes: #10634
Refs: whatwg/url#185
Refs: whatwg/url#225
Refs: whatwg/url#224
Refs: whatwg/url#218
Refs: whatwg/url#243
Refs: whatwg/url#260
Refs: whatwg/url#268
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
whatwg-url Issues and PRs related to the WHATWG URL implementation.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants