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

file: URL with a port number through the host setter #97

Closed
SimonSapin opened this issue Mar 1, 2016 · 14 comments
Closed

file: URL with a port number through the host setter #97

SimonSapin opened this issue Mar 1, 2016 · 14 comments

Comments

@SimonSapin
Copy link
Contributor

When the Url::host setter is invoked for an URL in the file scheme, the parser starts in the "host state" (not the "file host state") so the URL can (per spec) end up with a non-null port number, which otherwise never happens.

In this test case: http://software.hixie.ch/utilities/js/live-dom-viewer/?saved=3927 Firefox doesn’t change the URL at all, Chromium changes the host but not the port number.

@annevk
Copy link
Member

annevk commented Dec 29, 2016

@achristensen07 Safari appears to allow for host:port somehow through the host setter for file URLs. Would you consider that a bug?

I tend to think we should fix this by making file URLs use the file host state.

annevk added a commit to web-platform-tests/wpt that referenced this issue Dec 29, 2016
@achristensen07
Copy link
Collaborator

This is indeed a bug in my implementation. I had overlooked the fact that file URLs can't have ports and used the same host-and-port parser for file URLs as non-file URLs. We should also add a test like "new URL('file://host:45/path')" and make sure it fails.

@achristensen07
Copy link
Collaborator

We should definitely either change this or allow file URLs to have a port. I'm not sure why Safari allows ports on file URLs

@annevk
Copy link
Member

annevk commented Dec 30, 2016

The other question is what the setter should do. Should we fail on ":" or just end tokenizing there (as hostname does). And setting host and hostname should probably do the same thing.

If we special case file URLs in host and hostname they'd end up failing on ":", but it seems Chrome just ends tokenizing there, which is somewhat different behavior.

@annevk
Copy link
Member

annevk commented Jan 3, 2017

To be clear, I don't have strong opinions here on what is best. Allowing ports to end up in file URLs doesn't seem to be the end of the world, nor would failing on ":" be.

@annevk
Copy link
Member

annevk commented Jan 4, 2017

@sleevi do you have any thoughts here?

@annevk
Copy link
Member

annevk commented Jan 4, 2017

Note that Firefox doesn't appear to allow setting the host of file URLs at all.

@jasnell
Copy link
Collaborator

jasnell commented Jan 4, 2017

This is a bug in the Node.js implementation also. I'll get it fixed.
nodejs/node#10608

@jasnell
Copy link
Collaborator

jasnell commented Jan 4, 2017

Thinking it over a bit, the spec should likely be updated to treat specifying a port for the host in a file URL as an error. Parsing a URL such as file://example.net:81/foo or setting url.host = 'example.net:81' should likely throw.

@sleevi
Copy link

sleevi commented Jan 4, 2017

@annevk File URLs & ports/authority has a storied history in Chrome... attached are my notes from the last time I dug into this, to at least hopefully explain the behaviour to figure out where to align:

Can "file" have a host?

RFC 3986 Section 3.2.2 notes (in passing) that

For example, the "file" URI
scheme is defined so that no authority, an empty host, and
"localhost" all mean the end-user's machine, whereas the "http"
scheme considers a missing authority or empty host invalid.

If we dig back to RFC 1630, Page 18:

There is clearly a danger of confusion that a link made to a local
file should be followed by someone on a different system, with
unexpected and possibly harmful results. Therefore, the convention
is that even a "file" URL is provided with a host part. This allows
a client on another system to know that it cannot access the file
system, or perhaps to use some other local mecahnism to access the
file.

and

A void host field is equivalent to "localhost".

Can "file" have a port?

RFC 1738, Section 3.10, which updates RFC 1630 (and became the basis for RFC 3986) notes the file scheme as:

A file URL takes the form:
file:///

Unlike other schemes (such as prospero or wais), which explicitly list the :<port> construction in their ABNF, file:// lacks this.

So to what Chrome's behaviour is:

To your question about what's the right behaviour: I suspect failing on : would probably be ideal, but I wouldn't be in a place to change anytime soon, simply because I don't have the time to own any fallout/regressions that it might cause (however unlikely). It might be one of my colleagues can own this, if it's believed to be important for compat. Not allowing port is definitely a good thing (... and seems like it'd require no work on Chrome's side, since that's what we do).

Allowing ports on file URLs seems to have the largest back-compat issues, at least re: spec precedent - it's seemingly long been forbidden - and it's also something probably unlikely for Chrome, if only because that would require a lot more monkey-ing about with the UNC & drive-letter sniffing logic.

@annevk
Copy link
Member

annevk commented Jan 5, 2017

@sleevi if you do file://y/.hostname = "x:123" in Chrome you get file://x:123/. So the issues you allude to might already occur.

@annevk
Copy link
Member

annevk commented Jan 5, 2017

Using "file host state" as override is not a good idea as it doesn't really deal with the possibility of being a state override well.

Since a file host can be empty, setting to the empty string should work as well I guess if we care about making this fully functional...

@sleevi
Copy link

sleevi commented Jan 5, 2017

@annevk I'd be happy to treat that behaviour as a bug and see about fixing it in Chrome, if only because what the URL API exposes, the underlying implementation won't preserve (e.g. if you copy the object or send it across IPC boundaries, it will serialize/reparse differently)

annevk added a commit that referenced this issue Feb 1, 2017
This:

* Stops the username/password/port APIs from functioning when host is
the empty string.
* Makes the host/hostname APIs work better with file URLs and adjusts
the file host state accordingly.
* Make setting host/hostname to the empty string impossible when they
have a username/password/port.
* Fixes #97.
@annevk
Copy link
Member

annevk commented Feb 1, 2017

I ended up finding quite a few issues while working through this. I hope I addressed them all in the PR. Have yet to write test coverage for all that.

annevk added a commit to web-platform-tests/wpt that referenced this issue Feb 1, 2017
annevk added a commit that referenced this issue Feb 8, 2017
This:

* Stops the username/password/port APIs from functioning when host is the empty string.
* Makes the host/hostname APIs work better with file URLs and adjusts the file host state accordingly.
* Make setting host/hostname to the empty string impossible when they have a username/password/port.
* Fixes #97.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

No branches or pull requests

5 participants