652186 - Implement URL Standard's backslash replacement ("\" is treated as "/" when parsing paths)

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[Masatoshi Kimura [:emk]]

HTML5 defines replacing backslashes to forward slashes as a "willful violation" of RFC 3986 and RFC 3987, motivated by a desire to handle legacy content.

Comment 1

[Boris Zbarsky [:bzbarsky]]

I don't think we should do this, and we should push back on the spec here....  We don't do this now, we have very few compat problems as a result, and doing this breaks some existing use cases.

Comment 2

[Masatoshi Kimura [:emk]]

Hmm, could you escalate this to public-html lists or W3C bugzilla?

Comment 3

[Boris Zbarsky [:bzbarsky]]

OK, filed http://www.w3.org/Bugs/Public/show_bug.cgi?id=12543

Comment 4

[Masatoshi Kimura [:emk]]

The current spec had a serious flaw anyway...
http://www.w3.org/Bugs/Public/show_bug.cgi?id=12543#c4

Comment 5

[Masatoshi Kimura [:emk]]

https://www.w3.org/Bugs/Public/show_bug.cgi?id=15684 (cloned from https://www.w3.org/Bugs/Public/show_bug.cgi?id=12543 ) has been closed as WONTFIX.

Comment 6

[Masatoshi Kimura [:emk]]

Interesting.
https://plus.google.com/+AlexisImperialLegrandGoogle/posts/EQXTzsBVS7L

Comment 7

[Boris Zbarsky [:bzbarsky]]

Anne, see comment 6?

Comment 8

[Anne (:annevk)]

Yes, not having a common understanding of what a URL is can be problematic. That's why I'm trying to standardize it. This is not restricted to backslash handling, but can also be observed by e.g. the slash removal we do after a scheme. E.g. a server might consider http:///google.com/ to be fine as it has no host per the original RFC, but we consider it to have a host of google.com.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Yes, but the issue I'm worried about is that until servers move to the new spec us moving to it can introduce security bugs... and I'm not sure we have a sane way to mitigate that.

> a server might consider http:///google.com/ to be fine as it has no host per the original
> RFC

That's fair.  A strict reading of the RFCs would consider that an invalid URI and refuse to load it at all.

Comment 10

[Anne (:annevk)]

I think we have to accept that. Or convince the other browsers to remove this behavior, which seems unlikely (and we have it too, albeit limited to file URL schemes at the moment).

Comment 11

[Anne (:annevk)]

We should really fix this. This is causing issues such as bug 1166874 and the use case argument seems bogus as "\" is not a valid code point within a URL. %5C will remain working fine.

Comment 14

[Valentin Gosu [:valentin] (he/him) {{ AFK in July }}]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=736de6c2c34e

Comment 15

[Valentin Gosu [:valentin] (he/him) {{ AFK in July }}]

(In reply to Anne (:annevk) from comment #10)
> I think we have to accept that. Or convince the other browsers to remove
> this behavior, which seems unlikely (and we have it too, albeit limited to
> file URL schemes at the moment).

Do we want to limit the replacement to certain schemes? nsDefaultURIFixup only does it for http/https/ftp

Comment 16

[Valentin Gosu [:valentin] (he/him) {{ AFK in July }}]

comment 15

Comment 17

[Anne (:annevk)]

Per the URL Standard it is limited to https://url.spec.whatwg.org/#special-scheme. Whenever the URL parser talks about "\" it also checks if the URL (or scheme) is special. So I think it has changed since May last year to be more limited, but not as limited as http/https/ftp.

Comment 18

[Valentin Gosu [:valentin] (he/him) {{ AFK in July }}]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=dc29d425f055

Comment 19

[Valentin Gosu [:valentin] (he/him) {{ AFK in July }}]

Attachment: MozReview Request: Bug 652186 - Implement URL Standard's backslash replacement - web-platform-tests r?annevk

Review commit: https://reviewboard.mozilla.org/r/37389/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/37389/

Comment 20

[Valentin Gosu [:valentin] (he/him) {{ AFK in July }}]

Attachment: MozReview Request: Bug 652186 - Remove unneeded backslash replacement from nsDefaultURIFixup r?bz

Review commit: https://reviewboard.mozilla.org/r/37391/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/37391/

Comment 21

[Valentin Gosu [:valentin] (he/him) {{ AFK in July }}]

Attachment: MozReview Request: Bug 652186 - Implement URL Standard's backslash replacement r?mcmanus

Review commit: https://reviewboard.mozilla.org/r/37393/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/37393/

Comment 22

[Valentin Gosu [:valentin] (he/him) {{ AFK in July }}]

Attachment: MozReview Request: Bug 652186 - Fix tests involving URL backslash replacement r?bz

Review commit: https://reviewboard.mozilla.org/r/37395/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/37395/

Comment 23

[Valentin Gosu [:valentin] (he/him) {{ AFK in July }}]

Attachment: MozReview Request: Bug 652186 - Fix test with URL containing backslashes r?mak

Review commit: https://reviewboard.mozilla.org/r/37397/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/37397/

Comment 24

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8725260 [details]
MozReview Request: Bug 652186 - Remove unneeded backslash replacement from nsDefaultURIFixup r?bz

https://reviewboard.mozilla.org/r/37391/#review33967

r=me assuming you've verified that the schemeless "foo\bar" case works right.

Comment 25

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8725262 [details]
MozReview Request: Bug 652186 - Fix tests involving URL backslash replacement r?bz

https://reviewboard.mozilla.org/r/37395/#review33969

This probably needs to have a commit message that's not identical to the preceding changeset.

r=me with that fixed.

Comment 26

[Marco Bonardo [:mak]]

Comment on attachment 8725263 [details]
MozReview Request: Bug 652186 - Fix test with URL containing backslashes r?mak

https://reviewboard.mozilla.org/r/37397/#review34103

Comment 27

[Patrick McManus [:mcmanus]]

Comment on attachment 8725261 [details]
MozReview Request: Bug 652186 - Implement URL Standard's backslash replacement r?mcmanus

https://reviewboard.mozilla.org/r/37393/#review34157

::: netwerk/base/nsStandardURL.cpp:1199
(Diff revision 1)
> +        iterator++;

braces

::: netwerk/base/nsStandardURL.cpp:1201
(Diff revision 1)
> +    nsAutoCString protocol(Substring(start, iterator));

can you use a dependent string here?

::: netwerk/base/nsStandardURL.cpp:1203
(Diff revision 1)
> +    return protocol.LowerCaseEqualsLiteral("ftp") ||

lets check https and http first as the common cases

Comment 28

[Anne (:annevk)]

Comment on attachment 8725258 [details]
MozReview Request: Bug 652186 - Implement URL Standard's backslash replacement - web-platform-tests r?annevk

https://reviewboard.mozilla.org/r/37389/#review34375

Less failures, yay.

Comment 29

[Valentin Gosu [:valentin] (he/him) {{ AFK in July }}]

Comment on attachment 8725258 [details]
MozReview Request: Bug 652186 - Implement URL Standard's backslash replacement - web-platform-tests r?annevk

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/37389/diff/1-2/

Comment 30

[Valentin Gosu [:valentin] (he/him) {{ AFK in July }}]

Comment on attachment 8725260 [details]
MozReview Request: Bug 652186 - Remove unneeded backslash replacement from nsDefaultURIFixup r?bz

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/37391/diff/1-2/

Comment 31

[Valentin Gosu [:valentin] (he/him) {{ AFK in July }}]

Comment on attachment 8725261 [details]
MozReview Request: Bug 652186 - Implement URL Standard's backslash replacement r?mcmanus

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/37393/diff/1-2/

Comment 32

[Valentin Gosu [:valentin] (he/him) {{ AFK in July }}]

Comment on attachment 8725262 [details]
MozReview Request: Bug 652186 - Fix tests involving URL backslash replacement r?bz

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/37395/diff/1-2/

Comment 33

[Valentin Gosu [:valentin] (he/him) {{ AFK in July }}]

Comment on attachment 8725263 [details]
MozReview Request: Bug 652186 - Fix test with URL containing backslashes r?mak

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/37397/diff/1-2/

Comment 34

[Valentin Gosu [:valentin] (he/him) {{ AFK in July }}]

Comment on attachment 8725261 [details]
MozReview Request: Bug 652186 - Implement URL Standard's backslash replacement r?mcmanus

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/37393/diff/2-3/

Comment 35

[Valentin Gosu [:valentin] (he/him) {{ AFK in July }}]

Comment on attachment 8725262 [details]
MozReview Request: Bug 652186 - Fix tests involving URL backslash replacement r?bz

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/37395/diff/2-3/

Comment 36

[Valentin Gosu [:valentin] (he/him) {{ AFK in July }}]

Comment on attachment 8725263 [details]
MozReview Request: Bug 652186 - Fix test with URL containing backslashes r?mak

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/37397/diff/2-3/

Comment 37

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5f236850a583
https://hg.mozilla.org/integration/mozilla-inbound/rev/737316e9f6a2
https://hg.mozilla.org/integration/mozilla-inbound/rev/ac54c03b1f99
https://hg.mozilla.org/integration/mozilla-inbound/rev/49b3509fa00a
https://hg.mozilla.org/integration/mozilla-inbound/rev/170a1aa5791b

Comment 38

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/5f236850a583
https://hg.mozilla.org/mozilla-central/rev/737316e9f6a2
https://hg.mozilla.org/mozilla-central/rev/ac54c03b1f99
https://hg.mozilla.org/mozilla-central/rev/49b3509fa00a
https://hg.mozilla.org/mozilla-central/rev/170a1aa5791b

Comment 40

[Kohei Yoshino [:kohei]]

This won't cause site compat issues...