1328865 - moz-extension: URLs with fragment identifiers not resolved correctly in packed extensions

Status: RESOLVED FIXED

(Core :: Networking: JAR, defect)

Description

[Anton]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_12_2) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/54.0.2840.98 Safari/537.36

Steps to reproduce:

My extension is a single web-page application created with angular js.

The problem is that i've got "404 File not found" error when i'm using link with angular route

for example i'll get an error if I will use this link

moz-extension://76322ca6-cae1-c54b-8484-d69fea2c788f/index.html#!/dashboard
but it works if i'll use it without route

moz-extension://76322ca6-cae1-c54b-8484-d69fea2c788f/index.html

This problem takes place only with packed extension. 
Unpacked extension works fine.




Actual results:

i'll get an "404 File not found" error if I will use hash link in packed extension

example:

moz-extension://76322ca6-cae1-c54b-8484-d69fea2c788f/index.html#!/dashboard

Reproduced in FF Developer Edition 52.0a2  and FF Nightly 53.0a1 (2017-01-05) 



Expected results:

links similar to "index.html#!/dashboard" should work in packed extension

Comment 2

[Shane Caraveo (:mixedpuppy)]

I think that the jar file resolution isn't handling this.  The url in a packaged addon gets resolved to jar:

jar:file:///Users/shanec/tmp/play.xpi!/index.html#!/foobar

I'm thinking the !/ is causing the problem.  If you encode the fragment it works fine.

jar:file:///Users/shanec/tmp/play.xpi!/index.html#!%2Ffoobar

I'm still not sure where exactly the failure is occurring though.

Comment 3

[Shane Caraveo (:mixedpuppy)]

A little futher digging, I don't think there is a way around this.  !/ is used to delineate entries in jar files, and they can be nested as well.  With your url, nsJARURI is looking for dashboard inside a jar called index.html which is inside the XPI.  I think you'll have to encode the fragment part of the url.

Comment 4

[Kris Maglione [:kmag]]

We should be able to fix that by resolving to a jar: URL with the fragment ID stripped. But the jar: URI handler should really ignore anything after hash...

Comment 5

[Masatoshi Kimura [:emk]]

Yeah, this is a Core/Networking matter.

> location.hash = '!/foobar';

succeeds and location becomes 'jar:file:///Users/shanec/tmp/play.xpi!/index.html#!/foobar', but

> location = 'jar:file:///Users/shanec/tmp/play.xpi!/index.html#!/foobar';

fails. The jar: URI handler is totally inconsistent.

Comment 6

[Honza Bambas (:mayhemer)]

Jason, I'm not sure who is responsible for JAR code these days.  Can you find an assignee please?

Comment 7

[Gary Chen [:xeonchen]]

(In reply to Kris Maglione [:kmag] from comment #4)
> We should be able to fix that by resolving to a jar: URL with the fragment
> ID stripped. But the jar: URI handler should really ignore anything after
> hash...

Looks like we can ignore all '!/'s after a '#' symbol.

(In reply to Honza Bambas (:mayhemer) from comment #6)
> Jason, I'm not sure who is responsible for JAR code these days.  Can you
> find an assignee please?

I'd like to take this bug :)

Comment 8

[Shane Caraveo (:mixedpuppy)]

Gary, just wondering if this is still on the radar.

Comment 9

[Gary Chen [:xeonchen]]

(In reply to Shane Caraveo (:mixedpuppy) from comment #8)
> Gary, just wondering if this is still on the radar.

Oh, I'm sorry but I haven't stared this bug yet, and probably not going to fix in Q1.
If it is urgent, please feel free to take it or let me know, I can make some arrangement. :)

Comment 10

[Kris Maglione [:kmag]]

I'm worried this is going to break a fair number of extensions. If you won't be able to get to it this quarter, I can probably take it.

Comment 11

[Gary Chen [:xeonchen]]

(In reply to Kris Maglione [:kmag] from comment #10)
> I'm worried this is going to break a fair number of extensions. If you won't
> be able to get to it this quarter, I can probably take it.

I'm so sorry for late reply.

Recently I really don't have much time to deal with this bug, so if anyone is willing to fix this, please take it :)

Comment 12

[:shell escalante]

Hi Gary, 
Hi Jason,

I'm following up to see if someone will be able to land this bug before 57.   

We expect it to be an issue for users, although not in large volume until 57 (when WebExtensions before the only type of add-on).

Comment 13

[Gary Chen [:xeonchen]]

This could fall into ambiguous situation, for example:

1. jar:jar://path/to/file1!/path/to/file2!/path/to/file3

will be parsed into 2 parts:
   1.1: jar://path/to/file1
   1.2: jar:<1.1>!/path/to/file2#!/path/to/file3

However, consider the following URI:

2. jar:jar://path/to/file1#!/path/to/file2#!/path/to/file3

could be:

  2.1a: jar://path/to/file1#
  2.2a: jar:<2.1a>!/path/to/file2#!/path/to/file3

or

  2.1b: jar://path/to/file1#!/path/to/file2#
  2.2b: jar:<2.1b>!/path/to/file3


(In reply to Kris Maglione [:kmag] (busy; behind on reviews) from comment #4)
> We should be able to fix that by resolving to a jar: URL with the fragment
> ID stripped. But the jar: URI handler should really ignore anything after
> hash...

Kris, do you think it is possible to avoid the above issue?
Or can we just ignore the right-most fragment part within moz-extension handler?

Comment 14

[Jason Duell]

> This could fall into ambiguous situation

Yes, but only for nested jars IIUC?  Do packed extensions ever use nested jars?

Is angular always using '#!', i.e. the '#' is part of its magic?  Perhaps we could be smart about that.

Sounds like Gary and Kris are both busy.  I've been throwing Daniel at some jar code recently--perhaps he can take this.

Comment 15

[Daniel Stenberg [:bagder]]

Attachment: test case verifying this bug

If I'm understanding this issue correctly, it happens when the fragment part contains "!/" which isn't ignored properly by jar URI handler?

The attached test case is my take at verifying this behavior - and it currently fails as expected. (If we just modify the fragment part to not contain !/ it works fine.)

Would you agree Jason?

Comment 16

[Jason Duell]

Comment on attachment 8874351 [details]
test case verifying this bug

Yes, I think this test case is right, but asking Gary too to confirm

Comment 17

[Gary Chen [:xeonchen]]

Comment on attachment 8874351 [details]
test case verifying this bug

agree :)

Comment 18

[Daniel Stenberg [:bagder]]

Attachment: 0001-bug-1328865-make-nsJARURI-SetSpecWithBase-ignore-URL.patch

Here's my take at preventing the !/-scanning code from inspecting the fragment part. The fragment is still passed on to the SetJAREntry() method as we have numerous tests that require this.

This patch includes the new test case previously shown.

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=051b34538b07&selectedJob=107638592

Comment 19

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

Comment on attachment 8878470 [details] [diff] [review]
0001-bug-1328865-make-nsJARURI-SetSpecWithBase-ignore-URL.patch

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

Just a few minor issues with the test. The rest looks good.

::: modules/libjar/test/unit/test_bug1328865.js
@@ +1,4 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +

add "use strict";

@@ +17,5 @@
> +  var goodSpec = "jar:" + outerJarBase + "inner.jar!/hello#!/ignore%20this%20part";
> +  var goodChannel = NetUtil.newChannel({uri: goodSpec, loadUsingSystemPrincipal: true});
> +
> +  try {
> +    instr = goodChannel.open2();

This variable is undeclared.

@@ +19,5 @@
> +
> +  try {
> +    instr = goodChannel.open2();
> +  } catch (e) {
> +    do_throw("Failed to open file in inner jar");

do_throw(messageText) - Deprecated since Gecko 32.0

@@ +21,5 @@
> +    instr = goodChannel.open2();
> +  } catch (e) {
> +    do_throw("Failed to open file in inner jar");
> +  }
> +

Add a check at the end of the test. Something like ok(!!instr, "Should be able to open channel");

Comment 20

[Daniel Stenberg [:bagder]]

Attachment: v2-0001-bug-1328865-make-nsJARURI-SetSpecWithBase-ignore-.patch

Thanks Valentin, funny how just duplicating and modifying an existing test case can still get so many issues =) No tests in modules/libjar/test/unit/ use strict and they all use do_throw!

Here's my updated take. Have a look!

Comment 21

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

Comment on attachment 8879020 [details] [diff] [review]
v2-0001-bug-1328865-make-nsJARURI-SetSpecWithBase-ignore-.patch

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

Thanks!

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8cbeddfc615e
Make nsJARURI::SetSpecWithBase ignore URL fragments. r=valentin

Comment 23

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

https://hg.mozilla.org/mozilla-central/rev/8cbeddfc615e