Closed Bug 1087741 Opened 7 years ago Closed 7 years ago

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

Categories

(Core :: DOM: Security, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla38

People

(Reporter: ckerschb, Assigned: ckerschb)

References

Details

Attachments

(1 file, 2 obsolete files)

No description provided.
Blocks: 1087720
Attached patch js_13_services.patch (obsolete) — Splinter Review
Yep, that many patches that need to be reviewed that we pass the right arguments :-)
Attachment #8538867 - Flags: feedback?(tanvi)
Assignee: nobody → mozilla
Status: NEW → ASSIGNED
Attached patch js_13_services.patch (obsolete) — Splinter Review
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+
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-
Created pull request for mozmill:
  https://github.com/mozilla/mozmill/pull/120
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.
Depends on: 1121393
https://hg.mozilla.org/mozilla-central/rev/3bcebea804d5
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Depends on: 1131455
You need to log in before you can comment on or make changes to this bug.