1507354 - URL parser discards host for file: URLs

Status: ASSIGNED

(Core :: Networking, defect, P3)

Description

[Logan Smyth [:loganfsmyth]]

For example:

> var url = new URL("file://host/path")
> url.host
> // ""
> url.pathname
> // "/path"

Comment 1

[Anne (:annevk)]

This is due to our URL parser.

Comment 2

[Dragana Damjanovic [:dragana]]

Valentin, can you take a look?

Comment 3

[Valentin Gosu [:valentin] (he/him)]

Ugh, this is behaviour we've had since forever. While it's probably not hard to allow hosts for file URIs, I don't fully understand the security implications of doing that. We currently treat file URIs as LOCAL_FILE and potentially trustworthy. Allowing them to have a host kinda changes those assumptions.
Putting this in the backlog for now.

[1] https://searchfox.org/mozilla-central/rev/647b9eb1099e373e079e16f147f74f3d1d65eed3/netwerk/protocol/file/nsFileProtocolHandler.cpp#156

Comment 6

[Daniel Veditz [:dveditz]]

The URL spec is somewhat confusing on this point. §4.1 has a table showing the host can be a domain or IP address, and there's a lot of parsing logic in §4.4 talking about how to get the host part, and then §4.2 says

A URL cannot have a username/password/port if its host is null or the empty string, its cannot-be-a-base-URL is true, or its scheme is "file".

"file" isn't a particular protocol, so how are you supposed to talk to the remote host? Why can't it require auth or a non-standard port? RFC 1734 was pretty clear that if there was a host part it had to be one of a few synonyms for the local machine (see bug 376502). When did arbitrary domains get into the picture, and has anyone ever implemented that? I thought multiple implementations was a requirement for WHATWG specs?

After a quick look it appears Firefox and Safari simply ignore a host part, and Chrome considers it an error. I'm with Chrome on that one.

From a security POV it would be terrible to support it in today's browsers because too much code has made assumptions about "file:" meaning "local" since the earliest specs for the WWW.

Comment 8

[Anne (:annevk)]

Based on https://jsdom.github.io/whatwg-url/#url=ZmlsZTovL3Rlc3QvdGVzdA==&base=YWJvdXQ6Ymxhbms= both Chrome and Safari allow a host, Firefox does not. As I understand it this can be important on Windows.

Comment 9

[Matthew Dirks]

Just FYI, RFC 8089 is a more current standards document that solely covers the file URI scheme. It is much more thorough and detailed compared to the relatively spartan explanation offered in RFC 1734 (which was limited to a single section).

Comment 10

[Thomas Wisniewski [:twisniewski]]

This is the root cause of us failing lots of web platform tests for interop2023, based on the kinds of failures I'm seeing after bug 1347459 comment 6:

new URL("file://hi/path").protocol = "s" will keep the protocol as "file" as expected, but will drop the "hi" part of the URL and add a root slash: file:///path

I've put together a candidate patch which changes our behavior to better-match the URL spec and related url WPTs, which builds on top of the patch in bug 1347459 (passing 160 more WPT checks). I'll upload it ASAP for comments.

Comment 11

[Thomas Wisniewski [:twisniewski]]

https://treeherder.mozilla.org/jobs?repo=try&revision=43436dac259ffe28006a2702f175b079423283ff

Comment 12

[Thomas Wisniewski [:twisniewski]]

Attachment: Bug 1507354 - better follow URL spec file protocol parsing and serialization, including the hostname field; r?kershaw

Comment 13

[Thomas Wisniewski [:twisniewski]]

Attachment: Bug 1507354 - update WPT expectations; r?kershaw

Depends on D183061

Comment 14

[Thomas Wisniewski [:twisniewski]]

I just rebased the patches, with updated test expectations (including some wins in url/failure.html, which we weren't seeing before, due to that test crashing on our harness until bug 1722328 was partly addressed).

Comment 15

[Daniel Veditz [:dveditz]]

https://phabricator.services.mozilla.com/D183061#6091970 completely fell off my radar. I'm sorry! Tagging myself here in the bug because I couldn't figure out a way to get it into any of my queues in phabricator

Comment 16

[BugBot [:suhaib / :marco/ :calixte]]

Set release status flags based on info from the regressing bug 1722328

Comment 17

[Donal Meehan [:dmeehan]]

Hi :twisniewski - we are in soft code freeze for Fx112, Merge Day is on 2023-12-18. The patches for this are reviewed but from a few months ago.
I was wondering what expectations are, if this is something you plan on landing?
If it doesn't land not before merge day, will you be looking for an uplift?

Comment 18

[Thomas Wisniewski [:twisniewski]]

No, I don't mean to land this until :dveditz has time to properly consider the open comments on the patch first. When we do land it, I think we would want it to ride the release trains rather than sneaking it in last-minute or uplifting.

Comment 19

[Donal Meehan [:dmeehan]]

Thanks for the info, setting 122 to fix-optional for now. I can set it to wontfix later in the cycle.