1115193 - Make JS callers of ios.newChannel call ios.newChannel2 in browser/devtools

Status: RESOLVED FIXED

(DevTools :: General, defect)

Comment 1

[Christoph Kerschbaumer [:ckerschb]]

Attachment: js_3_a_browser_devtools.patch

Comment 2

[Christoph Kerschbaumer [:ckerschb]]

Attachment: js_3_b_browser_devtools_tests.patch

Comment 3

[Christoph Kerschbaumer [:ckerschb]]

Whoever ends up reviewing those patches, please find useful documentation here:
https://bugzilla.mozilla.org/show_bug.cgi?id=1087726#c2

Comment 4

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 8541000 [details] [diff] [review]
js_3_a_browser_devtools.patch

Joe, can you suggest appropriate reviewers for these patches?

Comment 5

[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 6

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8541000 [details] [diff] [review]
js_3_a_browser_devtools.patch

Let's try Victor.

Comment 7

[Victor Porof [:vporof][:vp]]

Comment on attachment 8541000 [details] [diff] [review]
js_3_a_browser_devtools.patch

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

This looks ugly as hell, but ok.

::: browser/devtools/canvasdebugger/canvasdebugger.js
@@ +417,5 @@
>  
> +    let channel = NetUtil.newChannel2(fp.file,
> +                                      null,
> +                                      null,
> +                                      window.document,

Why is the document sent here but not in the other cases? A document is indeed unavailable in modules, but I'd like to understand this change a bit more.

Comment 8

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Victor Porof [:vporof][:vp] from comment #7)
> ::: browser/devtools/canvasdebugger/canvasdebugger.js
> > +    let channel = NetUtil.newChannel2(fp.file,
> > +                                      null,
> > +                                      null,
> > +                                      window.document,

Victor, in the attached patch we tried to pass the right arguments to newChannel2() 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 browser/devtools/ 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.

Some background: 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].

Since this is security critical code please look closely and let us know if we need additional reviewers/help for certain sub components/files.
Keep in mind that URIs coming from a webpage should *never* use the systemPrincipal as the loadingPrincipal.

> Why is the document sent here but not in the other cases? A document is
> indeed unavailable in modules, but I'd like to understand this change a bit
> more.

Probably, we have to send the document in other cases too. I don't know. Sorry for not providing the above information earlier, but patches get split and assigned to different reviewers. Total, we have to change roughly ~400 callsites to pass the right arguments.

[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 9

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8541001 [details] [diff] [review]
js_3_b_browser_devtools_tests.patch

Victor, since you are already reviewing the patches here, can you also review the tests. Tests are not as critical, so I guess passing the systemPrincipal is fine. If you think we can pass more appropriate arguments however, please let me know.

Comment 10

[Victor Porof [:vporof][:vp]]

Comment on attachment 8541001 [details] [diff] [review]
js_3_b_browser_devtools_tests.patch

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

Looks mechanical enough.

Comment 11

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8541000 [details] [diff] [review]
js_3_a_browser_devtools.patch

Victor, have you seen what I have posted in comment 8? Since we don't know the codebase, we would rather you have provide answers for us regarding what arguments to pass to the newChannel-API. Given your question in comment 7, I am not sure whether this patch provides the correct arguments at the moment.

Since this is security critical code, I was wondering if you could take another pass over the patch and make sure we pass the right arguments. If we need additional reviewers, please let us also know. Thanks!

Comment 12

[Victor Porof [:vporof][:vp]]

Comment on attachment 8541000 [details] [diff] [review]
js_3_a_browser_devtools.patch

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

Sorry for the long wait.

In all of these cases except AppCacheUtils.jsm, it is correct to have aLoadingNode (third argument) pointing to a certain node, however I'm not sure how important that actually is. Especially since these methods are used in tests as well, for example by calling them directly instead of synthesizing a click on a button, I would say it's ok to use `document` for now.

More granular comments below; r+ with the change to ui-recordings.js and comments addressed.

::: browser/devtools/canvasdebugger/canvasdebugger.js
@@ +417,5 @@
>  
> +    let channel = NetUtil.newChannel2(fp.file,
> +                                      null,
> +                                      null,
> +                                      window.document,

If I'm understanding the documentation correctly, aLoadingPrincipal should always be passed, regardless of whether aLoadingNode is null or not. Why is that not the case here (or everywhere else).

::: browser/devtools/profiler/ui-recordings.js
@@ +328,5 @@
>  
> +  let channel = NetUtil.newChannel2(file,
> +                                    null,
> +                                    null,
> +                                    null,      // aLoadingNode

Note that this code will be entirely removed after bug 1123815 is resolved (in a week or so). 

This file is included as a xul script, so it's possible to reach the document directly. Simply pass window.document here for `aLoadingNode`.

@@ +329,5 @@
> +  let channel = NetUtil.newChannel2(file,
> +                                    null,
> +                                    null,
> +                                    null,      // aLoadingNode
> +                                    Services.scriptSecurityManager.getSystemPrincipal(),

This should either be null like in the other cases (see comment for canvasdebugger.js), or it should not be null in every other case. Let's stay consistent, since `aLoadingNode` doesn't have to be null.

::: browser/devtools/styleeditor/StyleEditorUI.jsm
@@ +349,5 @@
>            this._onStyleSheetCreated(styleSheet, file);
>          });
> +      },
> +      parentWindow ? parentWindow.document : null,
> +      parentWindow ? null : Services.scriptSecurityManager.getSystemPrincipal(),

As far as I can tell, `parentWindow` can ever be null here, so we shouldn't have these conditionals.

Comment 13

[Christoph Kerschbaumer [:ckerschb]]

Attachment: js_3_b_browser_devtools.patch

(In reply to Victor Porof [:vporof][:vp] from comment #12)
> Comment on attachment 8541000 [details] [diff] [review]
> js_3_a_browser_devtools.patch
> 
> Sorry for the long wait.

No problem at all.

> In all of these cases except AppCacheUtils.jsm, it is correct to have
> aLoadingNode (third argument) pointing to a certain node, however I'm not
> sure how important that actually is. Especially since these methods are used
> in tests as well, for example by calling them directly instead of
> synthesizing a click on a button, I would say it's ok to use `document` for
> now.

I agree, let's use window.document consistently with the exception of AppCacheutils.jsm where it's not available.
 
> ::: browser/devtools/canvasdebugger/canvasdebugger.js
> @@ +417,5 @@
> >  
> > +    let channel = NetUtil.newChannel2(fp.file,
> > +                                      null,
> > +                                      null,
> > +                                      window.document,
> 
> If I'm understanding the documentation correctly, aLoadingPrincipal should
> always be passed, regardless of whether aLoadingNode is null or not. Why is
> that not the case here (or everywhere else).

If you pass a document then we internally query the principal of that node (document). So we should pass null for aLoadingPrincipal in case we pass a node.
 
> ::: browser/devtools/profiler/ui-recordings.js
> @@ +328,5 @@
> >  
> > +  let channel = NetUtil.newChannel2(file,
> > +                                    null,
> > +                                    null,
> > +                                    null,      // aLoadingNode
> 
> Note that this code will be entirely removed after bug 1123815 is resolved
> (in a week or so). 

Ok, until then we can use the document.
 
> @@ +329,5 @@
> > +  let channel = NetUtil.newChannel2(file,
> > +                                    null,
> > +                                    null,
> > +                                    null,      // aLoadingNode
> > +                                    Services.scriptSecurityManager.getSystemPrincipal(),
> 
> This should either be null like in the other cases (see comment for
> canvasdebugger.js), or it should not be null in every other case. Let's stay
> consistent, since `aLoadingNode` doesn't have to be null.

I agree, let's be consistent and pass window.document.

> ::: browser/devtools/styleeditor/StyleEditorUI.jsm
> @@ +349,5 @@
> >            this._onStyleSheetCreated(styleSheet, file);
> >          });
> > +      },
> > +      parentWindow ? parentWindow.document : null,
> > +      parentWindow ? null : Services.scriptSecurityManager.getSystemPrincipal(),
> 
> As far as I can tell, `parentWindow` can ever be null here, so we shouldn't
> have these conditionals.

The documentation on top of the function indicates that a parentWindow is optional, so I would rather play it save here, see:
http://mxr.mozilla.org/mozilla-central/source/browser/devtools/styleeditor/StyleEditorUI.jsm#334

Comment 14

[Christoph Kerschbaumer [:ckerschb]]

Attachment: js_3_b_browser_devtools.patch

As discussed with Jonas over IRC and also as Victor indicates in Comment 12 we concluded to use the SystemPrincipal in all of these cases.

Carrying over r+.

Comment 15

[Tanvi Vyas[:tanvi]]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #14)
> Created attachment 8561508 [details] [diff] [review]
> js_3_b_browser_devtools.patch
> 
> As discussed with Jonas over IRC and also as Victor indicates in Comment 12
> we concluded to use the SystemPrincipal in all of these cases.
> 

I disagree; if we have the document and have already identified what it is, we should use that instead of system.  More information is better than less.  If we run into problems in the future where a security check blocks something it shouldn't, switching to system is an easy fix.

Comment 16

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Tanvi Vyas [:tanvi] from comment #15)
> (In reply to Christoph Kerschbaumer [:ckerschb] from comment #14)
> > Created attachment 8561508 [details] [diff] [review]
> > js_3_b_browser_devtools.patch
> > 
> > As discussed with Jonas over IRC and also as Victor indicates in Comment 12
> > we concluded to use the SystemPrincipal in all of these cases.
> > 
> 
> I disagree; if we have the document and have already identified what it is,
> we should use that instead of system.  More information is better than less.
> If we run into problems in the future where a security check blocks
> something it shouldn't, switching to system is an easy fix.

Alright, initially I was with Tanvi, because in general it makes sense to make the loadingNode available for consumers of loadinfo. In the particular case of devtools, e.g. canvasdebugger, IMHO it doesn't make a whole lot of a difference whether we use the document or systemPrincipal.

Jonas, I am willing to update again but I don't want to go back and forth all the time. How do you feel about it?

Comment 17

[Tanvi Vyas[:tanvi]]

Not just canvasdebugger...

browser/devtools/canvasdebugger/canvasdebugger.js
browser/devtools/profiler/ui-recordings.js 
browser/devtools/scratchpad/scratchpad.js 
browser/devtools/styleeditor/StyleEditorUI.jsm

Comment 18

[:Gavin Sharp [email: [e-mail]]]

It would be ideal if the semantics of these arguments were very clearly defined. The IDL documentation clearly isn't sufficient because we keep running into disagreements about when using the system principal makes sense.

Debugging tools are privileged and don't operate on behalf of the page being debugged, so I don't see how it makes sense for their requests to be associated with elements from that page.

Comment 19

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I think Gavin is asking the right question. It's not *just* a matter of "more information" or "less information", but also about "right information".

Let's use canvasdebugger.js as an example so that we have something concrete to talk about.

The file that is being loaded appears to be a local file on the user's harddrive, picked by the user.

That to me means that the triggeringPrincipal is the system principal. It was not a webpage that caused the given URL to be loaded.


I'm also guessing (though Gavin should confirm) that the resulting file is not being made available to the webpage. Instead it is being made available only to the canvasdebugger.xul page.

So to me, the webpage doesn't fit the description of neither the loadingNode or the loadingPrincipal. Since the webpage isn't getting access to the result of the load.


However we could use the canvasdebugger.xul page as the loadingNode. In which case the loadingPrincipal and the triggeringPrincipal would be the system principal since that's the principal of canvasdebugger.xul.

The other proposal on the table is to use the system principal as loadingPrincipal and triggeringPrincipal.

Either way it seems like loadingPrincipal and triggeringPrincipal should both be the system principal.


The only thing that can be debated is if we should use null or the canvasdebugger.xul page as the loadingNode. The difference is somewhat academic right now since we don't really apply any security checks when doing loads using the system principal.


However it has been debated allowing chrome pages to use CSP. The way that we'd likely implement that is by grabbing the CSP policy using the loadingNode. So if we want this load to be subject to CSP policies of canvasdebugger.xul, then we should probably pass that page as the loadingNode.


Either way, I definitely think that passing the webpage which is currently being debugged as the loadingNode would be wrong. And likely would result in the load getting blocked once we move security checks into AsyncOpen.

Comment 20

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Jonas Sicking (:sicking) from comment #19)
> However it has been debated allowing chrome pages to use CSP. The way that
> we'd likely implement that is by grabbing the CSP policy using the
> loadingNode. So if we want this load to be subject to CSP policies of
> canvasdebugger.xul, then we should probably pass that page as the
> loadingNode.

That's a little down the road. I don't think we should worry about that at the moment. Potentially the CSP object ends up being stored somewhere else. Let's worry about that once we get there.

> Either way, I definitely think that passing the webpage which is currently
> being debugged as the loadingNode would be wrong. And likely would result in
> the load getting blocked once we move security checks into AsyncOpen.

I agree, thanks for the detailed explanation Jonas. And as you mentioned above, Gavin puts it perfectly "not only more information, but also the correct information".

Comment 21

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Hmm.. maybe I misunderstood earlier comments and no one was actually proposing to use the principal or node from the webpage.

It sounds like maybe we're all in violent agreement that using the principal/node from the webpage would be the wrong thing to do. And that the loadingPrincipal and triggeringPrincipal both should be the system principal.

If so, the only thing begin debated is if we should use canvasdebugger.xul or null as loadingNode. Like I said, I think the difference is somewhat academic. But it seems beneficial to use the canvasdebugger.xul page if that's easy to do.

Comment 22

[Tanvi Vyas[:tanvi]]

Thanks for your comments Gavin and Jonas.  Using the canvasdebugger.xul seems like the right thing to do for the canvasdebugger.js case.  That will result in a system loading and triggering principal and pass security checks, while still giving some node information to consumers of loadinfo (given that there may be other non-security consumers in the future who would appreciate the information).

What about the other 3 cases?
browser/devtools/profiler/ui-recordings.js 
browser/devtools/scratchpad/scratchpad.js 
browser/devtools/styleeditor/StyleEditorUI.jsm

We can use the xul documents for scrathpad and styleeditor.  Not sure what ui-recordings.js is.  Perhaps Victor can comment on what the chrome documents are for each of these and if they are relatively easy to get in the code.

Thanks!

Comment 23

[Tanvi Vyas[:tanvi]]

Needinfo'ing Victor for comment 22.

Comment 24

[Victor Porof [:vporof][:vp]]

browser/devtools/profiler/ui-recordings.js - included in profiler.xul
browser/devtools/scratchpad/scratchpad.js - included in scratchpad.js
browser/devtools/styleeditor/StyleEditorUI.jsm - has access to the styleeditor.xul document

Comment 25

[Victor Porof [:vporof][:vp]]

(In reply to Victor Porof [:vporof][:vp] from comment #24)
> browser/devtools/scratchpad/scratchpad.js - included in scratchpad.js

I mean "included in scratchpad.xul"

Comment 26

[Christoph Kerschbaumer [:ckerschb]]

Important parts from victors and my IRC conversation:

(03:33:21 PM) victorporof: ui-recordings.js: yes, you can use window.document
(03:33:27 PM) victorporof: scratchpad.js: ditto
(03:34:19 PM) victorporof: StyleEditorUI.jsm: access via this._window.document
(03:34:29 PM) victorporof: canvasdebugger.js: window.document

Comment 27

[Christoph Kerschbaumer [:ckerschb]]

Attachment: js_3_a_browser_devtools.patch

Thanks Victor for your help and time!

Using the xul document in as the loadingNode in all cases except for AppCachutils.jsm where we don't have a node available and use systemPrincipal instead.

Comment 28

[Christoph Kerschbaumer [:ckerschb]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2f8e6c5d7188
https://hg.mozilla.org/integration/mozilla-inbound/rev/d4a6a7ae9109

Comment 29

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/2f8e6c5d7188
https://hg.mozilla.org/mozilla-central/rev/d4a6a7ae9109