1087741 - Make JS callers of ios.newChannel call ios.newChannel2 in services/

Status: RESOLVED FIXED

(Core :: DOM: Security, defect)

Comment 1

[Christoph Kerschbaumer [:ckerschb]]

Attachment: js_13_services.patch

Yep, that many patches that need to be reviewed that we pass the right arguments :-)

Comment 2

[Christoph Kerschbaumer [:ckerschb]]

Attachment: js_13_services.patch

Richard, 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 services/ 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#202
[2] http://mxr.mozilla.org/mozilla-central/source/netwerk/base/public/nsIIOService.idl#76

Comment 3

[Richard Newman [:rnewman]]

Comment on attachment 8542227 [details] [diff] [review]
js_13_services.patch

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

r? to Henrik for TPS and Mozmill changes.

::: services/sync/tests/unit/test_errorhandler_filelog.js
@@ +81,5 @@
>    Svc.Obs.notify("weave:service:sync:finish");
>  });
>  
>  function readFile(file, callback) {
> +  NetUtil.asyncFetch2(file, function (inputStream, statusCode, request) {

We could/should probably reimplement this in terms of OS.File; feel free to do so if you choose, or roll on with this.

::: services/sync/tps/extensions/mozmill/resource/stdlib/utils.js
@@ +411,5 @@
>      }
>      ready = true;
>    }
>  
> +  NetUtil.asyncFetch2(dataURI, function (aInputStream, aAsyncFetchResult) {

Henrik will know if this needs to be upstreamed to mozmill, and might have opinions besides. Flagging!

Comment 4

[Christoph Kerschbaumer [:ckerschb]]

Additional review note: One thing worth mentioning is that when a uri is coming from a webpage we should not use the systemPrincipal as the loadingPrincipal when calling NewChannel2.

Comment 5

[Richard Newman [:rnewman]]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #4)
> Additional review note: One thing worth mentioning is that when a uri is
> coming from a webpage we should not use the systemPrincipal as the
> loadingPrincipal when calling NewChannel2.

Sync never loads URIs from sites -- indeed, it only fetches content from your Sync server URI.

Comment 6

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 8542227 [details] [diff] [review]
js_13_services.patch

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

(In reply to Richard Newman [:rnewman] from comment #3)
> Henrik will know if this needs to be upstreamed to mozmill, and might have
> opinions besides. Flagging!

Richard, thanks for letting me know! So for the Mozmill fixes in TPS you certainly want to file a bug in the Testing/Mozmill component and create a patch against the master branch of https://github.com/mozilla/mozmill. Once the next version of Mozmill is released, we probably will sync it with TPS. Please keep in mind that Mozmill is going away soon, so we might not add any Mozmill test to TPS.

Comment 7

[Christoph Kerschbaumer [:ckerschb]]

Created pull request for mozmill:
  https://github.com/mozilla/mozmill/pull/120

Comment 8

[Christoph Kerschbaumer [:ckerschb]]

Attachment: js_13_services.patch

Henrik, I created a pull request for mozmill and took those changes out of this patch, which can land on inbound right?

And the other changes in mozmill/ are going to be merged into mc, right? When is that going to happen?

Comment 9

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Christoph, for the Mozmill core patch please file a bug as I mentioned in my last comment. Also please attach the patch to bugzilla for review. Sadly we cannot disable PRs on Github. Thanks.

Comment 10

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 8547821 [details] [diff] [review]
js_13_services.patch

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

r=me for the TPS part.

Comment 11

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to Henrik Skupin (:whimboo) from comment #9)
> Christoph, for the Mozmill core patch please file a bug as I mentioned in my
> last comment. Also please attach the patch to bugzilla for review. Sadly we
> cannot disable PRs on Github. Thanks.

I filed bug 1121393 for the necessary changes in Mozmill.

Comment 12

[Christoph Kerschbaumer [:ckerschb]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3bcebea804d5

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/3bcebea804d5