1124950 - Make JS callers of ios.newChannel call ios.newChannel2 in toolkit/webapps

Status: RESOLVED FIXED

(Core :: DOM: Security, defect)

Comment 1

[Christoph Kerschbaumer [:ckerschb]]

Attachment: js_15_toolkit_webapps.patch

Comment 2

[Tanvi Vyas[:tanvi]]

Fabrice, per the following comment in Bug 1087744, Gijs thinks that you might be able to get the principal of the webpage here instead of using system.  Can you take another look to confirm that we can't get a webpage principal and have to use system?

(In reply to :Gijs Kruitbosch from comment #10)
> Comment on attachment 8542255 [details] [diff] [review]
> js_15_toolkit.patch
 
> ::: toolkit/webapps/NativeApp.jsm
> @@ +449,5 @@
> > +    let channel = NetUtil.newChannel2(aIconURI,
> > +                                      null,
> > +                                      null,
> > +                                      null,      // aLoadingNode
> > +                                      Services.scriptSecurityManager.getSystemPrincipal(),
> 
> I expect that this should be the principal for the web app's origin from
> this.webappJson . It's probably wise to ask :fabrice or Marco Castelluccio
> to review the change to this file.
> 
> @@ +452,5 @@
> > +                                      null,      // aLoadingNode
> > +                                      Services.scriptSecurityManager.getSystemPrincipal(),
> > +                                      null,      // aTriggeringPrincipal
> > +                                      Ci.nsILoadInfo.SEC_NORMAL,
> > +                                      Ci.nsIContentPolicy.TYPE_OTHER);
> 
> aIconURI makes me expect that this should be TYPE_IMAGE.

Comment 3

[[:fabrice] Fabrice Desré]

Comment on attachment 8553416 [details] [diff] [review]
js_15_toolkit_webapps.patch

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

Redirecting to Marco then.

Comment 4

[Marco Castelluccio [:marco]]

There are three options here:
1) We're fetching the icon from the app ZIP package
2) We're fetching the icon from chrome://global/skin/icons/webapps-64.png
3) We're fetching the icon from the web

1 and 3 could use better security, but for 1 I wouldn't know what principal to use. So I think we should use the origin of the app in case 3

Comment 5

[Christoph Kerschbaumer [:ckerschb]]

Attachment: js_15_toolkit_webapps.patch

Marco, I am trying to read between the lines here. Is the attached patch what you are suggesting? If not, please let me know and I'll update again. Maybe you can provide a little more context of how you would like the patch to be.

Comment 6

[Christoph Kerschbaumer [:ckerschb]]

Marco, any suggestions on how to modify the patch?

Comment 7

[Marco Castelluccio [:marco]]

Comment on attachment 8558173 [details] [diff] [review]
js_15_toolkit_webapps.patch

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

When this function is called we're still in the process of installing the app, so I'm not sure if using getAppCodebasePrincipal would work in this case.
If it works, then this approach looks good to me.
If it doesn't work, then you could use getNoAppCodebasePrincipal (I assume it works for jar:// URLs as well?).

To make sure it works, you can run the unit tests in toolkit/webapps/ and you can install an hosted app with a custom icon (this will use a http:// URL), an hosted app without a custom icon (this will use a chrome:// URL) and a packaged app with a custom icon (this will use a jar:// URL).

Comment 8

[Christoph Kerschbaumer [:ckerschb]]

Attachment: js_15_toolkit_webapps.patch

I ran all the tests within toolkit/webapps, the one that is the most relevant is:
> toolkit/webapps/tests/test_hosted_icons.xul

which calls downloadIcon(aIconURI) using an aIconURI of
* chrome://global/skin/icons/webapps-64.png, as well as
* http://example.com/chrome/toolkit/webapps/tests/data/icon.png

where the second throws an exception because 'aApp is not defined'.

In other words, your hypothesis was correct, and we are still in the process of installing the app, hence we should rather use getNoAppCodebasePrincipal() instead of getAppCodeBasePrincipal.
Btw, using 'jar:' schemes as an URI and also to create a codeBasePrincipal is fine. It's even used further up in that file:
> http://mxr.mozilla.org/mozilla-central/source/toolkit/webapps/NativeApp.jsm#181

I reran all the tests within toolkit/webapps (locally) - all pass!

Comment 9

[Marco Castelluccio [:marco]]

Comment on attachment 8565640 [details] [diff] [review]
js_15_toolkit_webapps.patch

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

> I reran all the tests within toolkit/webapps (locally) - all pass!

Great!

Comment 10

[Christoph Kerschbaumer [:ckerschb]]

 https://hg.mozilla.org/integration/mozilla-inbound/rev/cebea9cb6f9b

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/cebea9cb6f9b