1232901 - Use channel.asyncOpen2 within dom/browser-element/BrowserElementParent.js

Status: RESOLVED FIXED

(Core :: DOM: Security, defect)

Comment 1

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1232901_asyncopen2_dom_browser_element.patch

Taking https://bugzilla.mozilla.org/show_bug.cgi?id=1232430#c4 and splitting the patch into several subproblems.

(In reply to Aus Lacroix [:aus] from comment #4)
> The goal of grabbing the loadingPrincipal is to have access to the correct
> cookie jar and other session related information so that you can download
> content properly because, often, sites require sign-in which is verified via
> session data.

Thanks Aus, but in what circumstances do we end up with a 'null' principal. Can that ever be the case? Probably we can simplify that code.

As the code is right now we never end up with a nullPrincipal (at least not in debug code) because we have assertions in place that don't allow creating of a channel without a loadingPrincipal.

Comment 2

[Aus Lacroix [:aus]]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #1)
> Created attachment 8698795 [details] [diff] [review]
> bug_1232901_asyncopen2_dom_browser_element.patch
> 
> Taking https://bugzilla.mozilla.org/show_bug.cgi?id=1232430#c4 and splitting
> the patch into several subproblems.
> 
> (In reply to Aus Lacroix [:aus] from comment #4)
> > The goal of grabbing the loadingPrincipal is to have access to the correct
> > cookie jar and other session related information so that you can download
> > content properly because, often, sites require sign-in which is verified via
> > session data.
> 
> Thanks Aus, but in what circumstances do we end up with a 'null' principal.
> Can that ever be the case? Probably we can simplify that code.
> 
> As the code is right now we never end up with a nullPrincipal (at least not
> in debug code) because we have assertions in place that don't allow creating
> of a channel without a loadingPrincipal.

Sounds like based on the fact that we have this assertion we could completely remove the extra code and assume we'll have a loadingPrincipal. Awesome! :)

Comment 3

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1232901_asyncopen2_dom_browser_element.patch

Hey Aus, thanks for the update. One last question. Unfortunately, I don't fully understand what the code is doing but it seems we do not perform any kind of network security checks. Since we are going to change the callsite to use asyncOpen2(), do we still want no security checks to be performed? Or let me rephrase that, what kind of security checks would be appropriate for that channel?

Comment 4

[Aus Lacroix [:aus]]

Comment on attachment 8700204 [details] [diff] [review]
bug_1232901_asyncopen2_dom_browser_element.patch

Looks fantastic! I love ditching old code. :)

Comment 5

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #3)
> Created attachment 8700204 [details] [diff] [review]
> bug_1232901_asyncopen2_dom_browser_element.patch
> 
> Hey Aus, thanks for the update. One last question. Unfortunately, I don't
> fully understand what the code is doing but it seems we do not perform any
> kind of network security checks. Since we are going to change the callsite
> to use asyncOpen2(), do we still want no security checks to be performed? Or
> let me rephrase that, what kind of security checks would be appropriate for
> that channel?

Thanks for the r+ but I would really like you to weigh in what kind of security checks we need to perform before opening that channel. Maybe you could explain what this code is doing, then we can easily come up with the right policy. Thanks!

Comment 6

[Aus Lacroix [:aus]]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #5)
> (In reply to Christoph Kerschbaumer [:ckerschb] from comment #3)
> > Created attachment 8700204 [details] [diff] [review]
> > bug_1232901_asyncopen2_dom_browser_element.patch
> > 
> > Hey Aus, thanks for the update. One last question. Unfortunately, I don't
> > fully understand what the code is doing but it seems we do not perform any
> > kind of network security checks. Since we are going to change the callsite
> > to use asyncOpen2(), do we still want no security checks to be performed? Or
> > let me rephrase that, what kind of security checks would be appropriate for
> > that channel?
> 
> Thanks for the r+ but I would really like you to weigh in what kind of
> security checks we need to perform before opening that channel. Maybe you
> could explain what this code is doing, then we can easily come up with the
> right policy. Thanks!

Since this functionality in the browser element is used to support things such as 'save link as' and 'save image as' and the likes, I would imagine that SEC_ALLOW_CROSS_ORIGIN_DATA_INHERITS would be most correct while being the safest.

Comment 7

[Christoph Kerschbaumer [:ckerschb]]

Those look all like intermittents to me:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=43811d303d19&selectedJob=15651127

Comment 8

[Pulsebot]

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

Comment 9

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/a6c0026d360b as being the more likely of the two to have caused https://treeherder.mozilla.org/logviewer.html#?job_id=20163348&repo=mozilla-inbound

Comment 10

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0f2a2b6bbf62

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/0f2a2b6bbf62