1163028 - URL: stop escaping [ and ] in path

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[Anne (:annevk)]

Per excellent research in bug 1152455 comment 6 we're the only browser that escapes [ and ] in the path component of a URL. We should stop doing that.

Comment 1

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

Attachment: URL: stop escaping [ and ] in path

Comment 2

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

Attachment: ospath_win.jsm: stop escaping [ and ] in toFileURI

Comment 3

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

Attachment: web-platform-tests URL: stop escaping [ and ] in path

Comment 4

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

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

Comment 5

[Matthew N. [:MattN]]

Comment on attachment 8610872 [details] [diff] [review]
ospath_win.jsm: stop escaping [ and ] in toFileURI

Review of attachment 8610872 [details] [diff] [review]:
-----------------------------------------------------------------

It looks like ospath_unix.jsm has the same problem.

Can you add a test to test_file_URL_conversion.js? If so, you probably don't need a spec/bug link in the code, only in the test.

It's not clear to me that you're only changing the path escaping… do you know if any OSs support query parameters? I think you should get :Yoric to do the final review.

::: toolkit/components/osfile/modules/ospath_win.jsm
@@ +311,5 @@
>    // URI-escape forward slashes and convert backward slashes to forward
>    path = this.normalize(path).replace(/[\\\/]/g, m => (m=='\\')? '/' : '%2F');
> +
> +  // Per bug 1163028 we should not encode [] in the path
> +  let uri = encodeURI(path).replace(/%(5b|5d)/gi,

If possible, could you link to a spec instead?

Comment 6

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

Attachment: stop escaping [ and ] in toFileURI

Comment 7

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 8611425 [details] [diff] [review]
stop escaping [ and ] in toFileURI

Review of attachment 8611425 [details] [diff] [review]:
-----------------------------------------------------------------

I don't think that you're testing the Windows version of your code.

::: toolkit/components/osfile/modules/ospath_unix.jsm
@@ +166,5 @@
>  let toFileURIExtraEncodings = {';': '%3b', '?': '%3F', '#': '%23'};
>  let toFileURI = function toFileURI(path) {
> +  // Per https://url.spec.whatwg.org we should not encode [] in the path
> +  let uri = encodeURI(this.normalize(path)).replace(/%(5b|5d)/gi,
> +    match => decodeURIComponent(match));

Calling `decodeURIComponent` just to decode %5b and %5d looks overkill, both here and in the Windows version.

Comment 8

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

Attachment: stop escaping [ and ] in toFileURI

Comment 9

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

(In reply to David Rajchenbach-Teller [:Yoric] (use "needinfo") from comment #7)
> I don't think that you're testing the Windows version of your code.

These are tested in test_file_URL_conversion.js

  // Test cases for filePathToURI
  let paths = isWindows ? [
    [...]
    'C:\\char[',
    'C:\\char]',

Comment 10

[David Teller [:Yoric] - still alive but not very active]

So there already was a test in the Windows version, and this test already passed? In that case, why do you need to patch the Windows code?

Comment 11

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

(In reply to David Rajchenbach-Teller [:Yoric] (use "needinfo") from comment #10)
> So there already was a test in the Windows version, and this test already
> passed? In that case, why do you need to patch the Windows code?

If we stop escaping [] in the C++ code, that test fails. That's why the changes in ospath_unix/win.jsm are necessary.

Comment 12

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 8616419 [details] [diff] [review]
stop escaping [ and ] in toFileURI

Review of attachment 8616419 [details] [diff] [review]:
-----------------------------------------------------------------

In that case, r+.

Comment 13

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

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

Comment 14

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/c626dc6dc4fbf2a07efe0772e85e28bf2d03674c
Bug 1163028 - URL: stop escaping [ and ] in path r=mcmanus

https://hg.mozilla.org/integration/mozilla-inbound/rev/baecbc728aabae3cd3604cd13377fe1e079ccc86
Bug 1163028 - stop escaping [ and ] in toFileURI r=yoric

https://hg.mozilla.org/integration/mozilla-inbound/rev/507c68ba9c7761ca780aa4c1fce38a4f5a53a056
Bug 1163028 - Fix WPT expectfails for urls containing [] a=testonly

Comment 15

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/c626dc6dc4fb
https://hg.mozilla.org/mozilla-central/rev/baecbc728aab
https://hg.mozilla.org/mozilla-central/rev/507c68ba9c77

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/c626dc6dc4fb
https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/baecbc728aab
https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/507c68ba9c77

Comment 17

[Carsten Book [:Tomcat]]

removing the b2g 2.5 flag since this commit has been reverted due to an incorrect merge, sorry for the confusion