1258347 - ExtensionPage sub-frames should have the same privileges of the parent window

Status: RESOLVED FIXED

(WebExtensions :: Untriaged, defect)

Description

[Luca Greco [:rpl] [:luca] [:lgreco]]

If a WebExtensions' ExtensionPage contains sub-frames which are pointed to an moz-extension URL from the same addon, the sub-frames get the same privileges of the parent window, e.g.:

- sub-frames of a background page has "full priviles"
- sub-frames of a tab page has "full privileges"
- sub-frames of a popup page has "full privileges"
- sub-frames of an injected extension iframe in a regulat web-page has "content script privileges"

Comment 1

[Luca Greco [:rpl] [:luca] [:lgreco]]

Added dependency on Bug 1256282, because this issue needs to change the same helper (|getAPILevelForWindow| [1]) which we are going to change to give to the options_ui iframe full privileges APIs (instead of the current privilege level, which is the same of content scripts).

[1]: https://dxr.mozilla.org/mozilla-central/source/toolkit/components/extensions/ExtensionManagement.jsm#230

Comment 2

[Luca Greco [:rpl] [:luca] [:lgreco]]

Attachment: MozReview Request: Bug 1258347 - [webext] ExtensionContext sub-frames should have the same privileges of the parent window. r=kmag

Review commit: https://reviewboard.mozilla.org/r/49995/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/49995/

Comment 4

[Kris Maglione [:kmag]]

Comment on attachment 8747694 [details]
MozReview Request: Bug 1258347 - [webext] ExtensionContext sub-frames should have the same privileges of the parent window. r=kmag

https://reviewboard.mozilla.org/r/49995/#review46841

::: toolkit/components/extensions/test/mochitest/test_ext_subframes_privileges.html:18
(Diff revision 1)
> +<script type="text/javascript">
> +"use strict";
> +
> +add_task(function* test_webext_tab_subframe_privileges() {
> +  function backgroundScript() {
> +    browser.tabs.create({url: browser.runtime.getURL("/tab.html")});

It would probably be a good idea to explicitly close this tab when we're done. That would also give us an opportunity to test `tabs.getCurrent()` in this case.

::: toolkit/components/extensions/test/mochitest/test_ext_subframes_privileges.html:22
(Diff revision 1)
> +  function backgroundScript() {
> +    browser.tabs.create({url: browser.runtime.getURL("/tab.html")});
> +  }
> +
> +  function tabSubframeScript() {
> +    browser.test.assertTrue(browser.tabs,

`assertTrue` is only supposed to accept booleans. Please go with something like `browser.tabs != undefined`.

::: toolkit/components/extensions/test/mochitest/test_ext_subframes_privileges.html:100
(Diff revision 1)
> +  function backgroundScript() {
> +    browser.runtime.onMessage.addListener((msg, {hasTabsAPI, hasRuntimeAPI}) => {
> +      if (msg == "contentscript-iframe-loaded") {
> +        browser.test.assertFalse(hasTabsAPI,
> +                                 "Subframe of a content script privileged iframes has no access to privileged APIs");
> +        browser.test.assertFalse(hasRuntimeAPI,

Shouldn't this be `assertTrue`?

Also, I'm not really sure what the point of this test is, since if it didn't have access to the runtime API, it wouldn't be able to send the message.

::: toolkit/components/extensions/test/mochitest/test_ext_subframes_privileges.html:110
(Diff revision 1)
> +    });
> +  }
> +
> +  function subframeScript() {
> +    browser.runtime.sendMessage("contentscript-iframe-loaded", {
> +      hasTabsAPI: browser && !!browser.tabs,

We already know that `browser` is defined here, since we wouldn't be able to call browser.runtime.sendMessage if it wasn't.

Comment 5

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8747694 [details]
MozReview Request: Bug 1258347 - [webext] ExtensionContext sub-frames should have the same privileges of the parent window. r=kmag

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/49995/diff/1-2/

Comment 6

[Luca Greco [:rpl] [:luca] [:lgreco]]

https://reviewboard.mozilla.org/r/49995/#review46841

> It would probably be a good idea to explicitly close this tab when we're done. That would also give us an opportunity to test `tabs.getCurrent()` in this case.

yep, test that `tabs.getCurrent()` works as expected seems like a very good idea, I added it in the last version pushed.

As a side note, I'm removing the tab from the main background page context just in case something goes (partially) wrong in the sub-frame (I really hate when a test fails for timeout reasons and doesn't give any useful log/clue, and so I tried to prevent it as much as possible and to ensure that, if it fails, then it is going to produce some meaningful log as much as possible)

There are obviously scenarios in which this test can still fail with a timeout and no useful log messages, but they should be reduced to the worst scenario (e.g. there's no API at all available in sub-frames)

> Shouldn't this be `assertTrue`?
> 
> Also, I'm not really sure what the point of this test is, since if it didn't have access to the runtime API, it wouldn't be able to send the message.

yep, this check was definitely wrong... luckily, because it helped to spot a real issue
(because the test was passing instead of generate a failure as expected):

The code in the sub-frame is using the second parameter of `runtime.sendMessage` as it was `test.sendMessage`, and unfortunately they have different conventions and `runtime.sendMessage` is one of the few API methods that doesn't fully validate its parameters.

Fixed in the last pushed version (by sending all the data with a message in json format).

Comment 7

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8747694 [details]
MozReview Request: Bug 1258347 - [webext] ExtensionContext sub-frames should have the same privileges of the parent window. r=kmag

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/49995/diff/2-3/

Comment 8

[Kris Maglione [:kmag]]

https://reviewboard.mozilla.org/r/49995/#review47527

::: toolkit/components/extensions/test/mochitest/test_ext_subframes_privileges.html:89
(Diff revisions 1 - 3)
>    yield extension.unload();
>  });
>  
>  add_task(function* test_webext_background_subframe_privileges() {
>    function backgroundSubframeScript() {
> -    browser.test.assertTrue(browser.tabs,
> +    browser.test.assertTrue(typeof browser.tabs != "undefined",

The typeof isn't necessary. `browser.tabs != undefined` (same for the other tests)

Comment 9

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8747694 [details]
MozReview Request: Bug 1258347 - [webext] ExtensionContext sub-frames should have the same privileges of the parent window. r=kmag

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/49995/diff/3-4/

Comment 10

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/b6a36fa869a3

Comment 11

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/b6a36fa869a3