Closed Bug 1328865 Opened 3 years ago Closed 3 years ago

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

Categories

(Core :: Networking: JAR, defect)

53 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla56
Tracking Status
firefox56 --- fixed

People

(Reporter: anthony.brodskyi, Assigned: bagder, NeedInfo)

References

Details

(Whiteboard: [necko-active])

Attachments

(1 file, 2 obsolete files)

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
Duplicate of this bug: 1328866
Component: General → WebExtensions: Untriaged
Status: UNCONFIRMED → NEW
Component: WebExtensions: Untriaged → WebExtensions: Request Handling
Ever confirmed: true
Summary: Firefox WebExtension angular routes hash links not working → moz-extension: URLs with fragment identifiers not resolved correctly in packed extensions
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.
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.
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...
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.
Component: WebExtensions: Request Handling → Networking: JAR
Product: Toolkit → Core
Jason, I'm not sure who is responsible for JAR code these days.  Can you find an assignee please?
Assignee: nobody → jduell.mcbugs
Flags: needinfo?(jduell.mcbugs)
Whiteboard: [necko-active]
(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 :)
Assignee: jduell.mcbugs → xeonchen
Flags: needinfo?(jduell.mcbugs)
Whiteboard: [necko-active] → [necko-next]
Gary, just wondering if this is still on the radar.
Flags: needinfo?(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. :)
Flags: needinfo?(xeonchen)
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.
(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 :)
Assignee: xeonchen → nobody
Blocks: 1351411
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).
Blocks: e10s-addons
Flags: needinfo?(xeonchen)
Flags: needinfo?(sescalante)
Flags: needinfo?(jduell.mcbugs)
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?
Flags: needinfo?(xeonchen) → needinfo?(kmaglione+bmo)
> 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.
Flags: needinfo?(jduell.mcbugs) → needinfo?(daniel)
Attached file test case verifying this bug (obsolete) —
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?
Flags: needinfo?(daniel)
Attachment #8874351 - Flags: feedback?(jduell.mcbugs)
Comment on attachment 8874351 [details]
test case verifying this bug

Yes, I think this test case is right, but asking Gary too to confirm
Attachment #8874351 - Flags: feedback?(xeonchen)
Attachment #8874351 - Flags: feedback?(jduell.mcbugs)
Attachment #8874351 - Flags: feedback+
Comment on attachment 8874351 [details]
test case verifying this bug

agree :)
Attachment #8874351 - Flags: feedback?(xeonchen) → feedback+
Assignee: nobody → daniel
Whiteboard: [necko-next] → [necko-active]
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
Attachment #8878470 - Flags: review?(valentin.gosu)
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");
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!
Attachment #8874351 - Attachment is obsolete: true
Attachment #8878470 - Attachment is obsolete: true
Attachment #8878470 - Flags: review?(valentin.gosu)
Attachment #8879020 - Flags: review?(valentin.gosu)
Comment on attachment 8879020 [details] [diff] [review]
v2-0001-bug-1328865-make-nsJARURI-SetSpecWithBase-ignore-.patch

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

Thanks!
Attachment #8879020 - Flags: review?(valentin.gosu) → review+
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8cbeddfc615e
Make nsJARURI::SetSpecWithBase ignore URL fragments. r=valentin
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/8cbeddfc615e
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Duplicate of this bug: 1374610
Flags: needinfo?(kmaglione+bmo)
See Also: → 1415574
You need to log in before you can comment on or make changes to this bug.