Closed Bug 652186 Opened 9 years ago Closed 4 years ago

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

Categories

(Core :: Networking, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla47
Tracking Status
firefox47 --- fixed

People

(Reporter: emk, Assigned: valentin)

References

(Depends on 1 open bug, Blocks 1 open bug, )

Details

(Keywords: sec-low, Whiteboard: [necko-active][adv-main47-])

Attachments

(5 files)

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.
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.
Hmm, could you escalate this to public-html lists or W3C bugzilla?
The current spec had a serious flaw anyway...
http://www.w3.org/Bugs/Public/show_bug.cgi?id=12543#c4
Blocks: url
Keywords: sec-low
Anne, see comment 6?
Flags: needinfo?(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.
Flags: needinfo?(annevk)
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.
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).
See Also: → 1026938
Blocks: 1116516
See Also: 1026938
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.
Summary: Implement HTML5 URL backslash replacement → Implement URL Standard's backslash replacement ("\" is treated as "/" when parsing paths)
Blocks: 1166874
Duplicate of this bug: 1190948
Duplicate of this bug: 1231880
Whiteboard: [necko-backlog]
Assignee: nobody → valentin.gosu
Whiteboard: [necko-backlog] → [necko-active]
(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 15
Flags: needinfo?(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.
Flags: needinfo?(annevk)
Attachment #8725260 - Flags: review?(bzbarsky) → review+
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 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.
Attachment #8725262 - Flags: review?(bzbarsky) → review+
Comment on attachment 8725263 [details]
MozReview Request: Bug 652186 - Fix test with URL containing backslashes r?mak

https://reviewboard.mozilla.org/r/37397/#review34103
Attachment #8725263 - Flags: review?(mak77) → review+
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
Attachment #8725261 - Flags: review?(mcmanus) → review+
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.
Attachment #8725258 - Flags: review?(annevk) → review+
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 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 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 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/
Attachment #8725262 - Attachment description: MozReview Request: Bug 652186 - Implement URL Standard's backslash replacement r?bz → MozReview Request: Bug 652186 - Fix tests involving URL backslash replacement r?bz
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 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 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 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/
Duplicate of this bug: 241688
This won't cause site compat issues...
Whiteboard: [necko-active] → [necko-active][adv-main47-]
Depends on: 1348876
You need to log in before you can comment on or make changes to this bug.