1116278 - Make JS callers of ios.newChannel call ios.newChannel2 in browser/components/feeds

Status: RESOLVED FIXED

(Core :: DOM: Security, defect)

Comment 1

[Christoph Kerschbaumer [:ckerschb]]

Attachment: js_3_browser_components_feeds.patch

Comment 2

[Christoph Kerschbaumer [:ckerschb]]

What do all these arguments do? Find a description here:
https://bugzilla.mozilla.org/show_bug.cgi?id=1087726#c2

Comment 3

[[e-mail]]

Comment on attachment 8542278 [details] [diff] [review]
js_3_browser_components_feeds.patch

>       if (result.doc) {
> 
>         // Store the result in the result service so that the display
>         // page can access it.
>         feedService.addFeedResult(result);
> 
>         // Now load the actual XUL document.
>         var aboutFeedsURI = ios.newURI("about:feeds", null, null);
>-        chromeChannel = ios.newChannelFromURI(aboutFeedsURI, null);
>+        chromeChannel = ios.newChannelFromURI2(aboutFeedsURI,
>+                                               result.doc,
>+                                               null, // aLoadingPrincipal
>+                                               null, // aTriggeringPrincipal
>+                                               Ci.nsILoadInfo.SEC_NORMAL,
>+                                               Ci.nsIContentPolicy.TYPE_OTHER);
>         chromeChannel.originalURI = result.uri;
>         chromeChannel.owner =
>           Services.scriptSecurityManager.getNoAppCodebasePrincipal(aboutFeedsURI);
>       } else {
>-        chromeChannel = ios.newChannelFromURI(result.uri, null);
>+        chromeChannel = ios.newChannelFromURI2(result.uri,
>+                                               null,      // aLoadingNode
>+                                               Services.scriptSecurityManager.getSystemPrincipal(),
>+                                               null,      // aTriggeringPrincipal
>+                                               Ci.nsILoadInfo.SEC_NORMAL,
>+                                               Ci.nsIContentPolicy.TYPE_OTHER);
>       }
So, what this is doing is a sort of redirect.
When the original page loaded, we detected that it was a feed based on its URI, content type or XML content, and diverted the load to the feed processor.
The feed processor then generates a Feed object and passes that to this function. The naming of this Feed object as result.doc is rather unfortunate.
If the parsing succeeds then about:feeds is loaded which then retrieves the original Feed object and displays it in the browser.
If the parsing fails then the code simply allows the original document to load normally.
The original channel is saved in this._request so it may be possible to access the appropriate parameters that way.
Either way I suspect the content should be TYPE_DOCUMENT.

>   newChannel: function GPH_newChannel(aUri) {
>     var inner = aUri.QueryInterface(Ci.nsINestedURI).innerURI;
>     var channel = Cc["@mozilla.org/network/io-service;1"].
>-                  getService(Ci.nsIIOService).newChannelFromURI(inner, null);
>+                  getService(Ci.nsIIOService).newChannelFromURI2(inner,
>+                                                                 null,      // aLoadingNode
>+                                                                 Services.scriptSecurityManager.getSystemPrincipal(),
>+                                                                 null,      // aTriggeringPrincipal
>+                                                                 Ci.nsILoadInfo.SEC_NORMAL,
>+                                                                 Ci.nsIContentPolicy.TYPE_OTHER);
Again, this is a sort of redirect. If you have a URI of the form feed://planet.mozilla.org/atom.xml then it attempts to force the page to load as a feed. The feed:// prefix is replaced with http:// first of course. I had a poke around but it wasn't clear what (if anything) protocol handlers had to do to their channel.

>     var resolvedURI = Cc["@mozilla.org/network/io-service;1"].
>                       getService(Ci.nsIIOService).
>-                      newChannel("about:feeds", null, null).URI;
>+                      newChannel2("about:feeds",
>+                                  null,
>+                                  null,
>+                                  aWindow.document,
>+                                  null, // aLoadingPrincipal
>+                                  null, // aTriggeringPrincipal
>+                                  Ci.nsILoadInfo.SEC_NORMAL,
>+                                  Ci.nsIContentPolicy.TYPE_OTHER).URI;
This channel is never loaded, it's just a security check to ensure that the code only runs on a page that was redirected to about:feeds. It has to jump through these hoops because about:feeds is itself a redirect to a chrome:// URI which itself is a redirect to a jar: URI (in a packaged build) or a file: URI (in a local build that you run directly from dist/bin). Since (NEEDINFO) ehsan has changed the way the feed writer loads this check may not be necessary any more.

Comment 4

[(no longer active)]

(In reply to [e-mail] from comment #3)
> >     var resolvedURI = Cc["@mozilla.org/network/io-service;1"].
> >                       getService(Ci.nsIIOService).
> >-                      newChannel("about:feeds", null, null).URI;
> >+                      newChannel2("about:feeds",
> >+                                  null,
> >+                                  null,
> >+                                  aWindow.document,
> >+                                  null, // aLoadingPrincipal
> >+                                  null, // aTriggeringPrincipal
> >+                                  Ci.nsILoadInfo.SEC_NORMAL,
> >+                                  Ci.nsIContentPolicy.TYPE_OTHER).URI;
> This channel is never loaded, it's just a security check to ensure that the
> code only runs on a page that was redirected to about:feeds. It has to jump
> through these hoops because about:feeds is itself a redirect to a chrome://
> URI which itself is a redirect to a jar: URI (in a packaged build) or a
> file: URI (in a local build that you run directly from dist/bin). Since
> (NEEDINFO) ehsan has changed the way the feed writer loads this check may
> not be necessary any more.

Yes, FeedWriterEnabled::IsEnabled() added in bug 983845 essentially does this check before this API is ever invoked, so we can remove this check completely.

Comment 5

[Christoph Kerschbaumer [:ckerschb]]

Attachment: js_3_browser_components_feeds.patch

Thanks for helping, here is what I've updated:

* FeedConverter.js handles some sort of redirect, hence we should query the loadInfo from the old channel and pass it along to the newChannel.

* FeedWriter.js, the check can be removed, right? So for now we can pass the nullPrincipal, which is not going to pass any security checks once we have moved them into channel::AsyncOpen(). It's overly conservative and what we have done in other places as well, so we can move on for now.

Comment 6

[Tanvi Vyas[:tanvi]]

Comment on attachment 8547741 [details] [diff] [review]
js_3_browser_components_feeds.patch

>Bug 1116278 - Make JS callers of ios.newChannel call ios.newChannel2 in browser/components/feeds (r=neil)
>
>diff --git a/browser/components/feeds/FeedWriter.js b/browser/components/feeds/FeedWriter.js
>--- a/browser/components/feeds/FeedWriter.js
>+++ b/browser/components/feeds/FeedWriter.js
>@@ -1123,19 +1123,29 @@ FeedWriter.prototype = {
>     var resolvedURI = Cc["@mozilla.org/network/io-service;1"].
>                       getService(Ci.nsIIOService).
>-                      newChannel("about:feeds", null, null).URI;
>+                      newChannel2("about:feeds",
>+                                  null,
>+                                  null,
>+                                  nullPrincipal,
>+                                  null, // aLoadingPrincipal
It looks like you have passed in nullPrincipal as the loadingNode instead of the loadingPrincipal.

>+                                  null, // aTriggeringPrincipal
>+                                  Ci.nsILoadInfo.SEC_NORMAL,
>+                                  Ci.nsIContentPolicy.TYPE_OTHER).URI;
>

Comment 7

[Tanvi Vyas[:tanvi]]

Comment on attachment 8547741 [details] [diff] [review]
js_3_browser_components_feeds.patch

Christoph, this part of the patch is also in FeedConverter.js.  Neil mentioned that this is also a redirect in comment 3.  Should it be setting system or should we try and pass in the loadInfo from the previous channel (which may require passing it into newChannel() first)?

diff --git a/browser/components/feeds/FeedConverter.js b/browser/components/feeds/FeedConverter.js
--- a/browser/components/feeds/FeedConverter.js
+++ b/browser/components/feeds/FeedConverter.js
@@ -541,17 +546,22 @@ GenericProtocolHandler.prototype = {
     var uri = netutil.newSimpleNestedURI(inner);
     uri.spec = inner.spec.replace(prefix, scheme);
     return uri;
   },
   
   newChannel: function GPH_newChannel(aUri) {
     var inner = aUri.QueryInterface(Ci.nsINestedURI).innerURI;
     var channel = Cc["@mozilla.org/network/io-service;1"].
-                  getService(Ci.nsIIOService).newChannelFromURI(inner, null);
+                  getService(Ci.nsIIOService).newChannelFromURI2(inner,
+                                                                 null,      // aLoadingNode
+                                                                 Services.scriptSecurityManager.getSystemPrincipal(),
+                                                                 null,      // aTriggeringPrincipal
+                                                                 Ci.nsILoadInfo.SEC_NORMAL,
+                                                                 Ci.nsIContentPolicy.TYPE_OTHER);
     if (channel instanceof Components.interfaces.nsIHttpChannel)
       // Set this so we know this is supposed to be a feed
       channel.setRequestHeader("X-Moz-Is-Feed", "1", false);
     channel.originalURI = aUri;
     return channel;
   },
   
   QueryInterface: function GPH_QueryInterface(iid) {

Comment 8

[Christoph Kerschbaumer [:ckerschb]]

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

Comment 9

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Tanvi Vyas [:tanvi] from comment #7)
> Comment on attachment 8547741 [details] [diff] [review]
> js_3_browser_components_feeds.patch
> 
> Christoph, this part of the patch is also in FeedConverter.js.  Neil
> mentioned that this is also a redirect in comment 3.  Should it be setting
> system or should we try and pass in the loadInfo from the previous channel
> (which may require passing it into newChannel() first)?
> 
> diff --git a/browser/components/feeds/FeedConverter.js
> b/browser/components/feeds/FeedConverter.js
> --- a/browser/components/feeds/FeedConverter.js
> +++ b/browser/components/feeds/FeedConverter.js
> @@ -541,17 +546,22 @@ GenericProtocolHandler.prototype = {
>      var uri = netutil.newSimpleNestedURI(inner);
>      uri.spec = inner.spec.replace(prefix, scheme);
>      return uri;
>    },
>    
>    newChannel: function GPH_newChannel(aUri) {
>      var inner = aUri.QueryInterface(Ci.nsINestedURI).innerURI;
>      var channel = Cc["@mozilla.org/network/io-service;1"].
> -                  getService(Ci.nsIIOService).newChannelFromURI(inner,
> null);
> +                  getService(Ci.nsIIOService).newChannelFromURI2(inner,
> +                                                                 null,     
> // aLoadingNode
> +                                                                
> Services.scriptSecurityManager.getSystemPrincipal(),
> +                                                                 null,     
> // aTriggeringPrincipal
> +                                                                
> Ci.nsILoadInfo.SEC_NORMAL,
> +                                                                
> Ci.nsIContentPolicy.TYPE_OTHER);
>      if (channel instanceof Components.interfaces.nsIHttpChannel)
>        // Set this so we know this is supposed to be a feed
>        channel.setRequestHeader("X-Moz-Is-Feed", "1", false);
>      channel.originalURI = aUri;
>      return channel;
>    },
>    
>    QueryInterface: function GPH_QueryInterface(iid) {

Hey Tanvi, I saw your comment only after pushing to inbound. I marked the bug as 'leave open' and we can follow up on it - probably you are right - I will investigate.

Comment 10

[Ryan VanderMeulen [:RyanVM]]

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

Comment 11

[Christoph Kerschbaumer [:ckerschb]]

Neil, in Comment 3 you mentioned that both channel creations within FeedConverter.js handle some kind of redirect. For some reason I only updated the first one, but not the second one:
> https://hg.mozilla.org/mozilla-central/rev/f15991d2f3b3#l1.50

Tanvi pointed that out in Comment 7. Anyway, I went back to update callsites of newChannel to also pass a LoadInfo from the original channel, but I can't find any. Is that code still in use? If not, we should potentially use the nullPrincipal for now. I put the following exception in newChannel:

>    newChannel: function GPH_newChannel(aUri) {
> +
> +    let exception = new Components.Exception(
> +        "newChannel in FeedConverter",
> +        Cr.NS_ERROR_INVALID_ARG,
> +        Components.stack.caller
> +    );
> +    throw exception;
> +
>      var inner = aUri.QueryInterface(Ci.nsINestedURI).innerURI;
>      var channel = Cc["@mozilla.org/network/io-service;1"].

and ran tests within browser/components/feeds. I also tried to grep for the contract/service id, but I couldn't find any consumers.
You know the codebase better than I do, am I missing something?

Comment 12

[Christoph Kerschbaumer [:ckerschb]]

Attachment: browser_components_feeds_followup.patch

As discussed in our meeting today, we should use the newChannel2 API. Adding r+ from Jonas for this follow up patch.

Comment 13

[Christoph Kerschbaumer [:ckerschb]]

We can close this bug once this follow up patch landed:
 https://hg.mozilla.org/integration/mozilla-inbound/rev/fe823d167766

Comment 14

[Ryan VanderMeulen [:RyanVM]]

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

Comment 15

[Christoph Kerschbaumer [:ckerschb]]

The follow up landed; removing 'leave-open' keyword and close this bug as fixed.