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

RESOLVED FIXED in mozilla38

Status

()

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

People

(Reporter: ckerschb, Assigned: ckerschb)

Tracking

unspecified
mozilla38
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

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 8538867 [details] [diff] [review]
js_13_services.patch

Yep, that many patches that need to be reviewed that we pass the right arguments :-)
Attachment #8538867 - Flags: feedback?(tanvi)
(Assignee)

Updated

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

Comment 2

3 years ago
Created attachment 8542227 [details] [diff] [review]
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
Attachment #8538867 - Attachment is obsolete: true
Attachment #8538867 - Flags: feedback?(tanvi)
Attachment #8542227 - Flags: review?(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!
Attachment #8542227 - Flags: review?(rnewman)
Attachment #8542227 - Flags: review?(hskupin)
Attachment #8542227 - Flags: review+
(Assignee)

Comment 4

3 years ago
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.
(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 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.
Attachment #8542227 - Flags: review?(hskupin) → review-
(Assignee)

Comment 7

3 years ago
Created pull request for mozmill:
  https://github.com/mozilla/mozmill/pull/120
(Assignee)

Comment 8

3 years ago
Created attachment 8547821 [details] [diff] [review]
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?
Attachment #8542227 - Attachment is obsolete: true
Attachment #8547821 - Flags: review?(hskupin)
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 on attachment 8547821 [details] [diff] [review]
js_13_services.patch

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

r=me for the TPS part.
Attachment #8547821 - Flags: review?(hskupin) → review+
(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.
(Assignee)

Updated

3 years ago
Depends on: 1121393
(Assignee)

Comment 12

3 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/3bcebea804d5
Target Milestone: --- → mozilla38
https://hg.mozilla.org/mozilla-central/rev/3bcebea804d5
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
(Assignee)

Updated

3 years ago
Depends on: 1131455
You need to log in before you can comment on or make changes to this bug.