Make JS callers of ios.newChannel call ios.newChannel2 in mobile/

RESOLVED FIXED in mozilla37

Status

()

Core
DOM: Security
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: ckerschb, Assigned: ckerschb)

Tracking

unspecified
mozilla37
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

Comment hidden (empty)
(Assignee)

Updated

3 years ago
Blocks: 1087720
(Assignee)

Comment 1

3 years ago
Created attachment 8538864 [details] [diff] [review]
js_10_mobile.patch

And there are more to come where we have to make sure we pass the right arguments
Attachment #8538864 - Flags: feedback?(tanvi)
(Assignee)

Updated

3 years ago
Assignee: nobody → mozilla
Status: NEW → ASSIGNED
(Assignee)

Comment 2

3 years ago
Created attachment 8540867 [details] [diff] [review]
js_10_mobile.patch

Wesley, in Bug 1038756 we started attaching a loadInfo to every channel whenever the channel gets created through NS_NewChannel in nsNetUtil.h. Now we are expanding the loadInfo attachment to also include channels that get created within JS to ultimately end up having a loadInfo on every channel.  Please find a description of all the required arguments to create a channel here [1] or alternatively here [2].

In the attached patch we tried to pass the right arguments to newChannel() to the best of our knowledge. It's quite complicated to provide the most accurate arguments (node, principal, contentType, etc.) because we are not experts within mobile/ code. Probably we have to change some more function signatures to pass the correct principal/node around so we finally end up having the right arguments in the function that finally creates the channel.

Please take a look at the patch. Since this is security critical code please look closely and let us know if we need additional reviewers/help for certain sub components/files.

[1] http://mxr.mozilla.org/mozilla-central/source/netwerk/base/public/nsNetUtil.h#209
[2] http://mxr.mozilla.org/mozilla-central/source/netwerk/base/public/nsIIOService.idl#73
Attachment #8538864 - Attachment is obsolete: true
Attachment #8538864 - Flags: feedback?(tanvi)
Attachment #8540867 - Flags: review?(wjohnston)
Comment on attachment 8540867 [details] [diff] [review]
js_10_mobile.patch

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

::: mobile/android/chrome/content/CastingApps.js
@@ +301,5 @@
>  
> +  _getContentTypeForURI: function(aURI, aElement, aCallback) {
> +    let channel = Services.io.newChannelFromURI2(aURI,
> +                                                 aElement,
> +                                                 null, // aLoadingPrincipal

I assume this is guessed from the element? The docs don't make that clear to me (same for triggering principal, although triggering principal notes that it could be left out).

@@ +315,5 @@
>            case 303:
>              request.cancel(0);
>              let location = channel.getResponseHeader("Location");
> +            CastingApps._getContentTypeForURI(
> +              CastingApps.makeURI(location), aElement, aCallback);

Don't wrap this. We don't use 80 line in mobile/ directories.
Attachment #8540867 - Flags: review?(wjohnston) → review+
(Assignee)

Comment 4

3 years ago
Created attachment 8544809 [details] [diff] [review]
js_10_mobile.patch

(In reply to Wesley Johnston (:wesj) from comment #3)
> > +    let channel = Services.io.newChannelFromURI2(aURI,
> > +                                                 aElement,
> > +                                                 null, // aLoadingPrincipal
> 
> I assume this is guessed from the element? The docs don't make that clear to
> me (same for triggering principal, although triggering principal notes that
> it could be left out).

Yes, that is correct, you you pass an element/node - then we query the principal from that element/node.

Carrying over r+ from wesj.
Attachment #8540867 - Attachment is obsolete: true
Attachment #8544809 - Flags: review+
(Assignee)

Comment 5

3 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/e285881c00c5
Target Milestone: --- → mozilla37
https://hg.mozilla.org/mozilla-central/rev/e285881c00c5
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.