Closed Bug 1124950 Opened 5 years ago Closed 5 years ago

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

Categories

(Core :: DOM: Security, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla38
Tracking Status
firefox38 --- fixed

People

(Reporter: ckerschb, Assigned: ckerschb)

References

Details

Attachments

(1 file, 2 obsolete files)

No description provided.
Assignee: nobody → mozilla
Blocks: 1087744
Status: NEW → ASSIGNED
Attached patch js_15_toolkit_webapps.patch (obsolete) — Splinter Review
Attachment #8553416 - Flags: review?(fabrice)
Attachment #8553416 - Flags: review?(fabrice) → review+
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 on attachment 8553416 [details] [diff] [review]
js_15_toolkit_webapps.patch

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

Redirecting to Marco then.
Attachment #8553416 - Flags: review+ → review?(mar.castelluccio)
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
Attachment #8553416 - Flags: review?(mar.castelluccio)
Attached patch js_15_toolkit_webapps.patch (obsolete) — Splinter Review
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.
Attachment #8553416 - Attachment is obsolete: true
Attachment #8558173 - Flags: review?(mar.castelluccio)
Marco, any suggestions on how to modify the patch?
Flags: needinfo?(mar.castelluccio)
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).
Flags: needinfo?(mar.castelluccio)
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!
Attachment #8558173 - Attachment is obsolete: true
Attachment #8558173 - Flags: review?(mar.castelluccio)
Attachment #8565640 - Flags: review?(mar.castelluccio)
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!
Attachment #8565640 - Flags: review?(mar.castelluccio) → review+
https://hg.mozilla.org/mozilla-central/rev/cebea9cb6f9b
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Blocks: 1223234
You need to log in before you can comment on or make changes to this bug.