1256282 - options_ui browsers should have access to the full WebExtension API

Status: RESOLVED FIXED

(WebExtensions :: Untriaged, defect)

Description

[wyohknott]

In Chrome, if a script is loaded in a page belonging to the extension, this script has permission to access all of Chrome APIs, like a background script.

In Firefox this is not the case.

For example, if my extension has an option page 'options.html' with a script tag containing 'options.js' :

 * in Chrome, I can call chrome.tabs.onUpdated
 * in Firefox, the console gives me 'chrome.tabs is undefined'

Comment 1

[Kris Maglione [:kmag]]

How are you loading options.html? We have tests that extension pages loaded in tabs have access to the tabs API.

Comment 2

[wyohknott]

It's an options V2 page.
In manifest.json :

> "options_ui": {
>   "chrome_style": false,
>   "page": "options.html"
>  },

Comment 3

[wyohknott]

Okay, I loaded the options page in a new tab, I get access to chrome.tabs

But now I got another similar problem, my options page is divided into subpages loaded into an iframe (took the idea from uBlock), and I don't have access to chrome.tabs in there.

Comment 4

[Kris Maglione [:kmag]]

So the sub-frames in tab pages also have access to the entire API in Chrome? I was under the impression that that wasn't the case.

Comment 5

[wyohknott]

Sub-frames problem apart (should I file another bug for this?), I ran the debugger step by step on Extension.jsm and the options_ui problem seems located here:

https://hg.mozilla.org/mozilla-central/file/tip/toolkit/components/extensions/Extension.jsm#l437

>   // We don't inject privileged APIs into sub-frames of a UI page.
>    const {FULL_PRIVILEGES} = ExtensionManagement.API_LEVELS;
>    if (ExtensionManagement.getAPILevelForWindow(contentWindow, id) !== FULL_PRIVILEGES) {
>      return;
>    }

When loading the inline options_ui:
ExtensionManagement.getAPILevelForWindow(contentWindow, id) == 1 whereas FULL_PRIVILEGES == 2

thus it returns and doesn't reach the code you added the other day for options_ui.

Comment 6

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

I'm still traveling home, but I took a deeper look into how Chrome behaves with addon pages running into sub-frames and it seems to use the following strategy:

- options UI sub-frame is full privileged
- sub-frames of an addon context (e.g. a tab window, background window etc.) with an chrome-extension url from the same addon
  has the same privileges of their parent context

I'm pushing a proposed change on the |getAPILevelForWindow| internal helper, which should make our implementation to behave like described above.

I'm going to turn it into an f?, to get a preliminary feedback, before proceeding to create new tests for the new scenarios.

Comment 7

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

Attachment: MozReview Request: Bug 1256282 - [webext] options_ui browsers should have access to the full WebExtension API. r=kmag

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

Comment 8

[Kris Maglione [:kmag]]

https://reviewboard.mozilla.org/r/41097/#review37753

This mostly looks good, but can you create a separate bug for the handling of sub-frame privileges, and let me handle the specific case of `options_ui` browsers in this one?

::: toolkit/components/extensions/ExtensionManagement.jsm:254
(Diff revision 1)
> -    return CONTENTSCRIPT_PRIVILEGES;
> +    let parentWindow = docShell.sameTypeParent.QueryInterface(Ci.nsIInterfaceRequestor)
> +          .getInterface(Ci.nsIDOMWindow);
> +
> +    // The option page iframe embedded in the about:addons tab should have full API level privileges.
> +    // (see Bug 1256282 for rationale)
> +    if (parentWindow.document.nodePrincipal.origin == "[System Principal]" &&

This should be `Services.scriptSecurityManager.isSystemPrincipal`

Comment 9

[wyohknott]

May I also bring to your attention that, in the case of options_ui:

https://hg.mozilla.org/mozilla-central/file/tip/toolkit/components/extensions/Extension.jsm#l449
>    let docShell = contentWindow.QueryInterface(Ci.nsIInterfaceRequestor)
>                                .getInterface(Ci.nsIWebNavigation)
>                                .QueryInterface(Ci.nsIDocShellTreeItem)
>                                .sameTypeRootTreeItem
>                                .QueryInterface(Ci.nsIDocShell);
>  
>    let browser = docShell.chromeEventHandler;

docShell.chromeEventHandler returns the browser containing entire addons page, not the browser containing the options_ui.

Thus, the following check can never be true:

https://hg.mozilla.org/mozilla-central/file/tip/toolkit/components/extensions/Extension.jsm#l473
>      } else if (browser.classList.contains("inline-options-browser")) {
>        // Options pages are currently displayed inline, but in Chrome
>        // and in our UI mock-ups for a later milestone, they're
>        // pop-ups.
>        type = "popup";
>      }

Comment 10

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

Comment on attachment 8732323 [details]
MozReview Request: Bug 1256282 - [webext] options_ui browsers should have access to the full WebExtension API. r=kmag

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

Comment 11

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

https://reviewboard.mozilla.org/r/41097/#review37753

I've just pushed this updated version with the following changes:

- handle only the options_ui scenario (I'll move the general "handle the sub-frame special case" in a separate issue which will depend from this one)

- fix the options_ui test cases accordingly (once we turn options_ui browser into a full privileged extension page, it will not be reachable using the browser.tabs.sendMessage because it will not be a "content script" based context anymore and it will not receive messages sent to a tab, I'll look on Chromium if we need to tweak this behavior)

- check the pong message and handle the rejection (e.g. if no one is listening the lastError will be set) to prevent false success

Do you mind if I handle this under your guidance so that we can keep higher amount of shared knowledge on the evolution of this part of the WebExtensions internals?

Comment 12

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

Comment on attachment 8732323 [details]
MozReview Request: Bug 1256282 - [webext] options_ui browsers should have access to the full WebExtension API. r=kmag

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

Comment 13

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

https://reviewboard.mozilla.org/r/41095/#review37781

::: toolkit/components/extensions/ExtensionManagement.jsm:235
(Diff revision 3)
>  function getAPILevelForWindow(window, addonId) {
>    const {NO_PRIVILEGES, CONTENTSCRIPT_PRIVILEGES, FULL_PRIVILEGES} = API_LEVELS;
>  
>    // Non WebExtension URLs and WebExtension URLs from a different extension
>    // has no access to APIs.
> -  if (!addonId && getAddonIdForWindow(window) != addonId) {
> +  if (!addonId || getAddonIdForWindow(window) != addonId) {

Kris, can you confirm that I'm reading this right and that the previous check was wrong?

I think that we want to return not privileged if the the page doesn't have an addonId OR it doesn't have the expected addonId.

Comment 14

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

As from discussion on IRC, I opened and assigned to me Bug 1258347, which is going to fix the general issue of sub-frames privileges level (e.g. by introducing the additional check in the |getAPILevelForWindow| helper, as it was in the first draft of the patch attached to this ticket).

Kris will continue to handle this issue (eventually changing the patch attached if needed), because this issue is related to other issues that he is already working on.

Comment 15

[Kris Maglione [:kmag]]

Comment on attachment 8732323 [details]
MozReview Request: Bug 1256282 - [webext] options_ui browsers should have access to the full WebExtension API. r=kmag

https://reviewboard.mozilla.org/r/41097/#review38313

Were you planning to submit the subframe and `addonId` test changes as part of another patch?

::: browser/components/extensions/test/browser/browser_ext_runtime_openOptionsPage.js:101
(Diff revision 3)
>          browser.test.assertEq(optionsTab, tab.id, "Tab is the same as the previous options tab");
>          browser.test.assertEq("about:addons", tab.url, "Tab contains AddonManager");
>  
>          browser.test.log("Ping options page.");
> -        return new Promise(resolve => browser.tabs.sendMessage(optionsTab, "ping", resolve));
> -      }).then(() => {
> +        return new Promise((resolve, reject) => {
> +          browser.runtime.sendMessage("ping", (msg) => {

No need for the `new Promise(...)`. Just `return browser.runtime.sendMessage("ping")`

Comment 16

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

Comment on attachment 8732323 [details]
MozReview Request: Bug 1256282 - [webext] options_ui browsers should have access to the full WebExtension API. r=kmag

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

Comment 17

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

Attachment: MozReview Request: Bug 1256282 - [webext] Fix addonId checks in getAPILevelForWindow and add an xpcshell test unit. r=kmag

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

Comment 18

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

(In reply to Kris Maglione [:kmag] from comment #15) 
> Were you planning to submit the subframe and `addonId` test changes as part
> of another patch?

I added a new patch which adds a test unit over the getAPILevelForWindow (which fails without the fix introduced by the changes in that internal helper and pass when the fix is applied).

> No need for the `new Promise(...)`. Just `return
> browser.runtime.sendMessage("ping")`

fixed in the updated version of the same patch.

Comment 19

[Kris Maglione [:kmag]]

Comment on attachment 8734471 [details]
MozReview Request: Bug 1256282 - [webext] Fix addonId checks in getAPILevelForWindow and add an xpcshell test unit. r=kmag

https://reviewboard.mozilla.org/r/42271/#review38899

Looks good, but please make it an xpcshell test. Also, this would probably be better as part of bug 1258347.

::: toolkit/components/extensions/test/mochitest/test_chrome_ext_getAPILevelForWindow.html:1
(Diff revision 1)
> +<!DOCTYPE HTML>

This should be an xpcshell test.

::: toolkit/components/extensions/test/mochitest/test_chrome_ext_getAPILevelForWindow.html:24
(Diff revision 1)
> +
> +function createWindowWithAddonId(addonId) {
> +  let baseURI = Services.io.newURI("about:blank", null, null);
> +  let originAttributes = {addonId};
> +  let principal = Services.scriptSecurityManager
> +                           .createCodebasePrincipal(baseURI, originAttributes);

Extra space.

Comment 20

[Kris Maglione [:kmag]]

Comment on attachment 8732323 [details]
MozReview Request: Bug 1256282 - [webext] options_ui browsers should have access to the full WebExtension API. r=kmag

https://reviewboard.mozilla.org/r/41097/#review38903

Please add a test that the options page has access to something like `browser.tabs.getCurrent`.

I suppose this will do for now, but we need other changes too, so please set the leave-open keyword before landing.

Comment 21

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

Comment on attachment 8732323 [details]
MozReview Request: Bug 1256282 - [webext] options_ui browsers should have access to the full WebExtension API. r=kmag

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

Comment 22

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

Comment on attachment 8734471 [details]
MozReview Request: Bug 1256282 - [webext] Fix addonId checks in getAPILevelForWindow and add an xpcshell test unit. r=kmag

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

Comment 23

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

https://reviewboard.mozilla.org/r/42271/#review38899

In the last review of this patches, I logically splitted the changes and their related tests into the two commits in this queue:

- the first patch applies the needed changes to give full privileges to the options ui page and adds a new test that checks that an options ui page can use the tabs privileged apis as expected
- the second patch fix the addonId checks and adds a new xpcshell test case to check that getAPILevelForWindow returns the expected privilege level in the different cases related to the addonId check.

Comment 25

[Kris Maglione [:kmag]]

Comment on attachment 8734471 [details]
MozReview Request: Bug 1256282 - [webext] Fix addonId checks in getAPILevelForWindow and add an xpcshell test unit. r=kmag

https://reviewboard.mozilla.org/r/42271/#review40593

::: toolkit/components/extensions/test/xpcshell/test_getAPILevelForWindow.js:23
(Diff revision 2)
> +
> +add_task(function* test_eventpages() {
> +  const {getAPILevelForWindow, getAddonIdForWindow} = ExtensionManagement;
> +  const {NO_PRIVILEGES, FULL_PRIVILEGES} = ExtensionManagement.API_LEVELS;
> +
> +  let apiLevel;

Per style guide: Always declare variables as close to their first use or assignment as possible. In this case, `let apiLevel = ...`

::: toolkit/components/extensions/test/xpcshell/test_getAPILevelForWindow.js:25
(Diff revision 2)
> +  const {getAPILevelForWindow, getAddonIdForWindow} = ExtensionManagement;
> +  const {NO_PRIVILEGES, FULL_PRIVILEGES} = ExtensionManagement.API_LEVELS;
> +
> +  let apiLevel;
> +
> +  let fakeAddonIdWindow = createWindowWithAddonId("fake_id");

Can we use a constant instead of the string "fake_id"?

::: toolkit/components/extensions/test/xpcshell/test_getAPILevelForWindow.js:27
(Diff revision 2)
> +
> +  let apiLevel;
> +
> +  let fakeAddonIdWindow = createWindowWithAddonId("fake_id");
> +  equal(getAddonIdForWindow(fakeAddonIdWindow),
> +     "fake_id", "the window has the expected addonId");

Please align after the opening paren on the previous line.

::: toolkit/components/extensions/test/xpcshell/test_getAPILevelForWindow.js:37
(Diff revision 2)
> +
> +  apiLevel = getAPILevelForWindow(fakeAddonIdWindow, "different_fake_id");
> +  equal(apiLevel, NO_PRIVILEGES,
> +        "apiLevel for the window with a different addonId should be NO_PRIVILEGES");
> +
> +  fakeAddonIdWindow.close();

`.close()` needs to be called on the object returned by `createWindowlessBrowser`, rather than on the content window.

::: toolkit/components/extensions/test/xpcshell/test_getAPILevelForWindow.js:39
(Diff revision 2)
> +  equal(apiLevel, NO_PRIVILEGES,
> +        "apiLevel for the window with a different addonId should be NO_PRIVILEGES");
> +
> +  fakeAddonIdWindow.close();
> +
> +  let emptyAddonIdWindow = createWindowWithAddonId("");

An empty string isn't a valid add-on ID. The behavior in this case should be undefined, so we shouldn't be testing it.

Comment 26

[Kris Maglione [:kmag]]

Comment on attachment 8732323 [details]
MozReview Request: Bug 1256282 - [webext] options_ui browsers should have access to the full WebExtension API. r=kmag

https://reviewboard.mozilla.org/r/41097/#review40595

::: browser/components/extensions/test/browser/browser_ext_optionsPage_privileges.js:37
(Diff revision 5)
> +          <head>
> +            <meta charset="utf-8">
> +            <script src="options.js" type="text/javascript"></script>
> +          </head>
> +        </html>`,
> +      "options.js": `(${optionsScript})()`,

`"options.js": optionsScript,`

::: browser/components/extensions/test/browser/browser_ext_optionsPage_privileges.js:39
(Diff revision 5)
> +            <script src="options.js" type="text/javascript"></script>
> +          </head>
> +        </html>`,
> +      "options.js": `(${optionsScript})()`,
> +    },
> +    background: `(${backgroundScript})()`,

`background: backgroundScript,`

::: browser/components/extensions/test/browser/browser_ext_optionsPage_privileges.js:47
(Diff revision 5)
> +  let tab = yield BrowserTestUtils.openNewForegroundTab(gBrowser, "http://example.com/");
> +
> +  yield extension.startup();
> +
> +  yield extension.awaitFinish("options-ui-privileges");
> +  yield extension.unload();

We need to remove the add-on manager tab at the end of this test.

Comment 27

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

Comment on attachment 8732323 [details]
MozReview Request: Bug 1256282 - [webext] options_ui browsers should have access to the full WebExtension API. r=kmag

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41097/diff/5-6/

Comment 28

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

Comment on attachment 8734471 [details]
MozReview Request: Bug 1256282 - [webext] Fix addonId checks in getAPILevelForWindow and add an xpcshell test unit. r=kmag

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

Comment 29

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

https://reviewboard.mozilla.org/r/42271/#review40593

> An empty string isn't a valid add-on ID. The behavior in this case should be undefined, so we shouldn't be testing it.

An empty addonId strings seems to be what regular web pages get as their originAttributes.addonId, and as it is not a valid addonId for sure, I'd like to test that we always return NO_PRIVILEGES for such an invalid add-on ID.

Comment 30

[Kris Maglione [:kmag]]

https://reviewboard.mozilla.org/r/42271/#review40853

Comment 31

[Kris Maglione [:kmag]]

Comment on attachment 8732323 [details]
MozReview Request: Bug 1256282 - [webext] options_ui browsers should have access to the full WebExtension API. r=kmag

https://reviewboard.mozilla.org/r/41097/#review40855

::: browser/components/extensions/test/browser/browser_ext_optionsPage_privileges.js:8
(Diff revisions 5 - 6)
>  "use strict";
>  
>  add_task(function* test_tab_options_privileges() {
>    function backgroundScript() {
> +    browser.runtime.onMessage.addListener(tabId => {
> +      browser.tabs.remove(tabId).then(() => {

We should probably send a message name along with the tab ID.

Comment 32

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

Comment on attachment 8732323 [details]
MozReview Request: Bug 1256282 - [webext] options_ui browsers should have access to the full WebExtension API. r=kmag

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41097/diff/6-7/

Comment 33

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

Comment on attachment 8734471 [details]
MozReview Request: Bug 1256282 - [webext] Fix addonId checks in getAPILevelForWindow and add an xpcshell test unit. r=kmag

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

Comment 34

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

https://reviewboard.mozilla.org/r/41097/#review40855

> We should probably send a message name along with the tab ID.

Fixed in the last revision of this patch.

Comment 35

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

Last rebased and tweaked patches pushed to try:

- https://treeherder.mozilla.org/#/jobs?repo=try&revision=8c574e683e2f

Comment 36

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/2677bb5bffff
https://hg.mozilla.org/integration/fx-team/rev/9dc2be94c435

Comment 37

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/2677bb5bffff
https://hg.mozilla.org/mozilla-central/rev/9dc2be94c435