1362993 - Add BrowserTestUtils.addTab() which provides triggeringPrincipal for params object

Status: RESOLVED FIXED

(Core :: DOM: Security, enhancement, P2)

Description

[Christoph Kerschbaumer [:ckerschb]]

Instead of rewriting all the tests to use gBrowser.addTab(uri, {triggeringPrincipal}) we should add BrowsertestUtils.addTab() and provide a triggeringPrincipal in all the cases tests don't provide a triggeringPrincipal explicitly.

Comment 1

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1362993_add_browsertestutils_addtab.patch

Florian, I wrote a script that replaces all the occurrences of gBrowser.addTab(...) to call BrowserTestUtils.addTab(...). I hope this is what you had in mind as well.

Comment 2

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1362993_rewrite_tests_to_use_browsertestutils_addtab.patch

Comment 3

[Dão Gottwald [:dao]]

Comment on attachment 8866314 [details] [diff] [review]
bug_1362993_add_browsertestutils_addtab.patch

>+   * Opens a tab with a given uri and params object. If the params object is not set
>+   * or the params parameter does not include a triggeringPricnipal then this function
>+   * provides a params object using the systemPrincipal as the default triggeringPrincipal.
>+   */
>+  addTab(browser, uri, params) {
>+    if (!params) {
>+      params = { triggeringPrincipal: Services.scriptSecurityManager.getSystemPrincipal() }
>+    }
>+    else if (!params.triggeringPrincipal) {
>+      params.triggeringPrincipal = Services.scriptSecurityManager.getSystemPrincipal();
>+    }

Why doesn't gBrowser.addTab do this by default?

Comment 4

[:Gijs (out for now; he/him)]

(In reply to Dão Gottwald [::dao] from comment #3)
> Comment on attachment 8866314 [details] [diff] [review]
> bug_1362993_add_browsertestutils_addtab.patch
> 
> >+   * Opens a tab with a given uri and params object. If the params object is not set
> >+   * or the params parameter does not include a triggeringPricnipal then this function
> >+   * provides a params object using the systemPrincipal as the default triggeringPrincipal.
> >+   */
> >+  addTab(browser, uri, params) {
> >+    if (!params) {
> >+      params = { triggeringPrincipal: Services.scriptSecurityManager.getSystemPrincipal() }
> >+    }
> >+    else if (!params.triggeringPrincipal) {
> >+      params.triggeringPrincipal = Services.scriptSecurityManager.getSystemPrincipal();
> >+    }
> 
> Why doesn't gBrowser.addTab do this by default?

Because it's an insecure default - see the discussion in bug 1362329 comment #3 and further. If we add the system principal as the default in gBrowser.addTab(), any cases where we fail to pass the right content/unprivileged principal end up being security vulnerabilities.

Comment 5

[Florian Quèze [:florian]]

Comment on attachment 8866314 [details] [diff] [review]
bug_1362993_add_browsertestutils_addtab.patch

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

::: testing/mochitest/BrowserTestUtils/BrowserTestUtils.jsm
@@ +1369,5 @@
> +   * Opens a tab with a given uri and params object. If the params object is not set
> +   * or the params parameter does not include a triggeringPricnipal then this function
> +   * provides a params object using the systemPrincipal as the default triggeringPrincipal.
> +   */
> +  addTab(browser, uri, params) {

You can give {} as the default value for 'params', so that you don't need the first 'if (!params) check.

Comment 6

[Florian Quèze [:florian]]

Comment on attachment 8866315 [details] [diff] [review]
bug_1362993_rewrite_tests_to_use_browsertestutils_addtab.patch

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

I assume your script was a simple search & replace. What I had in mind is a script using the JS parser to rewrite some of the code (see bug 1362882 for an example).

From grepping your patch ($ egrep -C 2 '^\+[^\+]+[^;]$' attachment.cgi\?id\=8866315) I see that this broke the indent in only 3 places so it'll be easier to just fix them by hand.

::: browser/base/content/test/general/browser_bug537013.js
@@ +19,1 @@
>                                 aText + "</h1>");

please fix indent here.

::: devtools/client/webconsole/test/browser_console_private_browsing.js
@@ +28,1 @@
>                                             "<p>hello world! I am not private!");

here

::: toolkit/components/passwordmgr/test/browser/browser_hasInsecureLoginForms_streamConverter.js
@@ +86,1 @@
>                              "passwordmgr/test/browser/streamConverter_content.sjs",

and here.

Comment 7

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1362993_add_browsertestutils_addtab.patch

(In reply to Florian Quèze [:florian] [:flo] from comment #5)
> You can give {} as the default value for 'params', so that you don't need
> the first 'if (!params) check.

Yeah, makes sense, thanks!

Comment 8

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1362993_rewrite_tests_to_use_browsertestutils_addtab.patch

(In reply to Florian Quèze [:florian] [:flo] from comment #6)
> I assume your script was a simple search & replace. What I had in mind is a
> script using the JS parser to rewrite some of the code (see bug 1362882 for
> an example).

Indeed, very simple string replacement script - next time we can hack up something more sophisticated.
 
> From grepping your patch ($ egrep -C 2 '^\+[^\+]+[^;]$'
> attachment.cgi\?id\=8866315) I see that this broke the indent in only 3
> places so it'll be easier to just fix them by hand.

Done - thanks for pointing that out.

Comment 9

[Christoph Kerschbaumer [:ckerschb]]

I only test so many tests locally. I suppose BrowserTestUtils is available everywhere, let's make sure:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=5022599851c713b72be1f72c61737123af722199

Comment 10

[Dão Gottwald [:dao]]

(In reply to :Gijs from comment #4)
> (In reply to Dão Gottwald [::dao] from comment #3)
> > Comment on attachment 8866314 [details] [diff] [review]
> > bug_1362993_add_browsertestutils_addtab.patch
> > 
> > >+   * Opens a tab with a given uri and params object. If the params object is not set
> > >+   * or the params parameter does not include a triggeringPricnipal then this function
> > >+   * provides a params object using the systemPrincipal as the default triggeringPrincipal.
> > >+   */
> > >+  addTab(browser, uri, params) {
> > >+    if (!params) {
> > >+      params = { triggeringPrincipal: Services.scriptSecurityManager.getSystemPrincipal() }
> > >+    }
> > >+    else if (!params.triggeringPrincipal) {
> > >+      params.triggeringPrincipal = Services.scriptSecurityManager.getSystemPrincipal();
> > >+    }
> > 
> > Why doesn't gBrowser.addTab do this by default?
> 
> Because it's an insecure default - see the discussion in bug 1362329 comment
> #3 and further. If we add the system principal as the default in
> gBrowser.addTab(), any cases where we fail to pass the right
> content/unprivileged principal end up being security vulnerabilities.

Can we have a pref to set the default only for tests? Outlawing simple addTab calls in tests is counterintuitive and seems like a footgun.

Comment 11

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Dão Gottwald [::dao] from comment #10)
> Can we have a pref to set the default only for tests? Outlawing simple
> addTab calls in tests is counterintuitive and seems like a footgun.

I am not sure if that's easily possible, but also I don't fully understand what the footgun is here? BrowserTestUtils.addTab() just forwards the call to gBrowser.addTab() setting the systemPrincipal as the triggeringPrincipal for all browser mochitests. All of our Gecko code will be updated to pass a valid triggeringPrincipal (see Bug 1333030 and dependencies) so we can reason where the load is coming from. Obviously Addons can also use whatever front end API we provide not providing that principal, but at least within gecko we can reason about 'where the load is coming from' and what security mechanisms we need to enforce.

Comment 12

[:Gijs (out for now; he/him)]

(In reply to Dão Gottwald [::dao] from comment #10)
> (In reply to :Gijs from comment #4)
> > (In reply to Dão Gottwald [::dao] from comment #3)
> > > Why doesn't gBrowser.addTab do this by default?
> > 
> > Because it's an insecure default - see the discussion in bug 1362329 comment
> > #3 and further. If we add the system principal as the default in
> > gBrowser.addTab(), any cases where we fail to pass the right
> > content/unprivileged principal end up being security vulnerabilities.
> 
> Can we have a pref to set the default only for tests?

That will mask issues with 'real' new non-test consumers calling addTab() without a principal - they will pass in tests but fail in the 'real world', which also doesn't seem desirable. Do you see another way around this problem?

Comment 13

[Florian Quèze [:florian]]

Comment on attachment 8867092 [details] [diff] [review]
bug_1362993_add_browsertestutils_addtab.patch

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

r=me, thanks!

Please have a look at your try results. I would retrigger the 'c1' failures a couple times to ensure they are actually intermittent. And you have a bunch of eslint errors to fix.

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/08e52e63564d
Add BrowserTestUtils.addTab() which provides triggeringPrincipal for params object. r=florian
https://hg.mozilla.org/integration/mozilla-inbound/rev/a2b7eaff1268
Rewrite gBrowser.addTab() to use BrowserTestUtils.addTab(). r=florian

Comment 15

[Kris Maglione [:kmag]]

I'm not sure why this hasn't broken talos tests on inbound (I suspect it's because inbound magically uses signed/packaged versions of the talos add-ons), but on try and in local runs this breaks the talos tabpaint tests, because BrowserTestUtils is not defined in its bootstrap.js scope.

Comment 16

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Kris Maglione [:kmag] (busy; behind on reviews) from comment #15)
> I'm not sure why this hasn't broken talos tests on inbound (I suspect it's
> because inbound magically uses signed/packaged versions of the talos
> add-ons), but on try and in local runs this breaks the talos tabpaint tests,
> because BrowserTestUtils is not defined in its bootstrap.js scope.

oh oh - Kris, what is the best way to fix this?

Comment 17

[Kris Maglione [:kmag]]

Most likely just backout the change to this line:

http://searchfox.org/mozilla-central/rev/ae24a3c83d22e0e35aecfd9049c2b463ca7e045b/testing/talos/talos/tests/tabpaint/bootstrap.js#159

But it would probably be a good idea to make sure nothing else accidentally got broken the same way. I only noticed this because it broke my try runs.

Comment 18

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1362993_tabpaint_followup.patch

Kris, thanks for pointing that out. I suppose we can just push that as a follow up, passing an explicit params object. What do you think? Should work, because Services is used elsewhere in that file.

Comment 19

[Kris Maglione [:kmag]]

Comment on attachment 8868016 [details] [diff] [review]
bug_1362993_tabpaint_followup.patch

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

r=me

Someone will need to repackage and sign this so the new version is used for non-try builds, though.

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d479f76dc5e8
Followup: Use gBrowser.addTab with explicit triggeringPricnipal instead of BrowserTestUtils. r=kmaglione

Comment 21

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Kris Maglione [:kmag] (busy; behind on reviews) from comment #19)
> Someone will need to repackage and sign this so the new version is used for
> non-try builds, though.

Kris, I pushed to that inbound so that all changes can land together on central. If you tell me how to package then I'll file a follow up and get that ready to land.

Comment 22

[Kris Maglione [:kmag]]

It needs to be done by someone with privileges to sign those add-ons.

Comment 23

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/08e52e63564d
https://hg.mozilla.org/mozilla-central/rev/a2b7eaff1268
https://hg.mozilla.org/mozilla-central/rev/d479f76dc5e8

Comment 24

[Joel Maher ( :jmaher ) (UTC -8)]

let me sign the tabpaint addon

Comment 25

[Joel Maher ( :jmaher ) (UTC -8)]

I will sign the tabpaint addon in bug 1365289