Closed Bug 1163028 Opened 5 years ago Closed 4 years ago

URL: stop escaping [ and ] in path

Categories

(Core :: Networking, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla45
Tracking Status
firefox45 --- fixed

People

(Reporter: annevk, Assigned: valentin)

References

()

Details

Attachments

(3 files, 2 obsolete files)

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.
Attachment #8610872 - Flags: review?(MattN+bmo)
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?
Attachment #8610872 - Flags: review?(MattN+bmo) → review-
Attachment #8610870 - Flags: review?(mcmanus) → review+
Attachment #8611425 - Flags: review?(dteller)
Attachment #8610872 - Attachment is obsolete: true
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.
Attachment #8611425 - Flags: review?(dteller) → feedback+
Attachment #8611425 - Attachment is obsolete: true
(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]',
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?
(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 on attachment 8616419 [details] [diff] [review]
stop escaping [ and ] in toFileURI

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

In that case, r+.
Attachment #8616419 - Flags: review?(dteller)
removing the b2g 2.5 flag since this commit has been reverted due to an incorrect merge, sorry for the confusion
You need to log in before you can comment on or make changes to this bug.