1087739 - Make JS callers of ios.newChannel call ios.newChannel2 in netwerk/

Status: RESOLVED FIXED

(Core :: DOM: Security, defect)

Comment 1

[Christoph Kerschbaumer [:ckerschb]]

Attachment: js_12_netwerk.patch

Even though there are some dependencies that haven't cleared yet, we should start getting reviews. Especially for this patch, sine netwerk/ contains the most callsites of ioservice.newChannel which need to be updated. Can you please make sure that we pass the right principal, node, triggeringPrincipal?

Comment 2

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8525688 [details] [diff] [review]
js_12_netwerk.patch

Steve volunteered to take a first peek at that monster-patch as well. Probably in the end we still want jduell to look over it since that is all security critical code.

Comment 3

[Steve Workman [:sworkman] (INACTIVE)]

Comment on attachment 8525688 [details] [diff] [review]
js_12_netwerk.patch

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

Looks fine to me. r=me.

::: netwerk/test/unit/test_NetUtil.js
@@ +12,5 @@
>  
>  Cu.import("resource://gre/modules/NetUtil.jsm");
>  Cu.import("resource://gre/modules/Task.jsm");
>  Cu.import("resource://gre/modules/Promise.jsm");
> +Cu.import("resource://gre/modules/Services.jsm");

Quick suggestion: add the following to shorten the arg lists:

  var getSystemPrincipal = Services.scriptSecurityManager.getSystemPrincipal;

You should be able to call getSystemPrincipal() directly in the rest of the script now.

Not a requirement; use it now or remember it for the future.

::: netwerk/test/unit/test_file_protocol.js
@@ +37,5 @@
>  function new_file_channel(file) {
>    var ios =
>        Cc["@mozilla.org/network/io-service;1"].
>        getService(Ci.nsIIOService);
> +  return ios.newChannelFromURI2(ios.newFileURI(file0,

s/file0/file/

Comment 4

[Tanvi Vyas[:tanvi]]

Comment on attachment 8525688 [details] [diff] [review]
js_12_netwerk.patch

Looks like these are all tests and they all use:
null for aLoadingNode
system for aLoadingPrincipal
null for aTriggeringPrincipal
SEC_NORMAL for the security flags.
TYPE_OTHER for content policy type

Couple indentation issues here:
https://bugzilla.mozilla.org/attachment.cgi?id=8525688&action=diff#a/netwerk/test/unit/test_http2.js_sec3
https://bugzilla.mozilla.org/attachment.cgi?id=8525688&action=diff#a/netwerk/test/unit/test_mismatch_last-modified.js_sec3

For the following data: uris, is SEC_NORMAL the right security flag to use?  data: inherits principal (http://mxr.mozilla.org/mozilla-central/source/netwerk/protocol/data/nsDataHandler.cpp#53) so perhaps we should use SEC_FORCE_INHERIT_PRINCIPAL.
https://bugzilla.mozilla.org/attachment.cgi?id=8525688&action=diff#a/netwerk/test/unit/test_NetUtil.js_sec6 (in test_asyncFetch_does_not_block())
https://bugzilla.mozilla.org/attachment.cgi?id=8525688&action=diff#a/netwerk/test/unit/test_bug376660.js_sec3

Comment 5

[Christoph Kerschbaumer [:ckerschb]]

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

Comment 6

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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