Closed Bug 1138649 Opened 7 years ago Closed 7 years ago

Update remaining callsites to use newChannel2 in toolkit/components

Categories

(Core :: DOM: Security, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla39
Tracking Status
firefox39 --- fixed

People

(Reporter: ckerschb, Assigned: ckerschb)

References

Details

Attachments

(1 file, 3 obsolete files)

No description provided.
Assignee: nobody → mozilla
Blocks: 1087744
Status: NEW → ASSIGNED
@Jonas: Within RemoteAddonsParent.jsm we create an about channel but we do not have a LoadInfo and we can also not create one from within JS. How would you like to handle that case?
Attachment #8571548 - Flags: review?(gijskruitbosch+bugs)
Attachment #8571548 - Flags: feedback?(jonas)
Comment on attachment 8571548 [details] [diff] [review]
bug_1138649_toolkit_components.patch

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

I think mconley can help with remoteaddonsparent (although my suspicion is that what you want is to pass a principal of some sort with the message from RemoteAddonsChild.jsm so that RemoteAddonsParent at least has some idea of what is going on), and MattN is probably a better reviewer for the passwordmanager tests.
Attachment #8571548 - Flags: review?(mconley)
Attachment #8571548 - Flags: review?(gijskruitbosch+bugs)
Attachment #8571548 - Flags: review?(MattN+bmo)
Comment on attachment 8571548 [details] [diff] [review]
bug_1138649_toolkit_components.patch

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

I actually suspect billm will have a better notion of what to do here.
Attachment #8571548 - Flags: review?(mconley) → review?(wmccloskey)
I agree with what Gijs says. However, we still would need a way to construct an nsILoadInfo in JS.
(In reply to Bill McCloskey (:billm) from comment #4)
> I agree with what Gijs says. However, we still would need a way to construct
> an nsILoadInfo in JS.

We do not want to expose nsILoadInfo to JS for various reasons. Let's wait till Jonas replies and provides feedback what we can/should do about that case. Maybe you can review all the other bits in the meantime, that would be great.
Comment on attachment 8571548 [details] [diff] [review]
bug_1138649_toolkit_components.patch

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

r=me on toolkit/components/passwordmgr

::: toolkit/components/passwordmgr/test/test_prompt.html
@@ +183,5 @@
> +                                null,
> +                                null,
> +                                null,      // aLoadingNode
> +                                SpecialPowers.Services.
> +                                scriptSecurityManager.getSystemPrincipal(),

Nit: could you wrap this line before the period and align the periods (or assign the system principal to a variable above this method call? It's easy to mistake the period for a comma here.

::: toolkit/components/passwordmgr/test/test_prompt_async.html
@@ +150,5 @@
> +                                          null,
> +                                          null,
> +                                          null,      // aLoadingNode
> +                                          SpecialPowers.Services.
> +                                          scriptSecurityManager.getSystemPrincipal(),

Nit: could you wrap this line before the period and align the periods like we do above? It's easy to mistake the period for a comma here.
Attachment #8571548 - Flags: review?(MattN+bmo) → review+
Attachment #8571548 - Flags: review?(wmccloskey) → review+
I adressed the nits and even though I already got r+ for the patch, there are still those missing bits at
> toolkit/components/addoncompat/RemoteAddonsParent.jsm

Bill, I think it should be fine that we are creating the channel through NetUtil.newChannel2() instead of calling module.newChannel(), right? Because then we don't have to worry about exposing any of the loadInfo bits to JS which we really should avoid.
Attachment #8571548 - Attachment is obsolete: true
Attachment #8571548 - Flags: feedback?(jonas)
Attachment #8575058 - Flags: review?(wmccloskey)
Comment on attachment 8575058 [details] [diff] [review]
bug_1138649_toolkit_components.patch

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

This looks good overall. The thing below would be nice to have but not necessary.

::: toolkit/components/addoncompat/RemoteAddonsParent.jsm
@@ +252,5 @@
> +      let channel = NetUtil.newChannel2(uri,
> +                                        null,
> +                                        null,
> +                                        null,      // aLoadingNode
> +                                        Services.scriptSecurityManager.getSystemPrincipal(),

It would be nice to pass in the right principal. You can do that as follows:

- Add a loadInfo argument to newChannel here:
http://hg.mozilla.org/mozilla-central/annotate/c42e7e3bb0a0/toolkit/components/addoncompat/RemoteAddonsChild.jsm#l330

- Pass it into the AboutProtocolChannel constructor and save it on the instance.
- In asyncOpen, send loadInfo.principal along with the URI and contractID:
http://hg.mozilla.org/mozilla-central/annotate/c42e7e3bb0a0/toolkit/components/addoncompat/RemoteAddonsChild.jsm#l228
- The principal will then be a field on msg.data and you can use it instead of the system principal.
Attachment #8575058 - Flags: review?(wmccloskey) → review+
Bill, first of all, thanks for looking so closely. I really appreciate that you are trying to pass the right principal. Speaking of, can't we just pass the loadInfo around like I do in the attached patch? Or are we going to run into some serialization issues between child and parent? If so, would it make sense to get at least, the principal, content-type and security-flags off the loadInfo and pass those bits along?

I haven't found any automatic testcase that covers/includes usage of RemoteAddonsChild/Parent - is there even one?
Attachment #8575058 - Attachment is obsolete: true
Attachment #8575659 - Flags: review?(wmccloskey)
Comment on attachment 8575659 [details] [diff] [review]
bug_1138649_toolkit_components.patch

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

This looks good except that there's no way to serialize nsILoadInfo like this. We have special code for nsIPrincipal but not for nsILoadInfo. Sending the principal, content type, and security flags sounds good.

There's a browser-chrome test for this called browser_addonShims.js. Most of the code for the test is here:
http://mxr.mozilla.org/mozilla-central/source/toolkit/components/addoncompat/tests/addon/bootstrap.js#264
Attachment #8575659 - Flags: review?(wmccloskey)
Yep - that makes sense to me. Querying the principal, securityFlags and contentPolicyType and passing those as arguments to NetUtil.newChannel2().
Attachment #8575659 - Attachment is obsolete: true
Attachment #8575758 - Flags: review?(wmccloskey)
Comment on attachment 8575758 [details] [diff] [review]
bug_1138649_toolkit_components.patch

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

Thanks!
Attachment #8575758 - Flags: review?(wmccloskey) → review+
https://hg.mozilla.org/mozilla-central/rev/e894b69587bc
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.