Closed Bug 1256282 Opened 4 years ago Closed 4 years ago

options_ui browsers should have access to the full WebExtension API

Categories

(WebExtensions :: Untriaged, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: clare.tor86, Assigned: rpl)

References

Details

(Whiteboard: triaged)

Attachments

(2 files)

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'
How are you loading options.html? We have tests that extension pages loaded in tabs have access to the tabs API.
It's an options V2 page.
In manifest.json :

> "options_ui": {
>   "chrome_style": false,
>   "page": "options.html"
>  },
Assignee: nobody → kmaglione+bmo
Status: UNCONFIRMED → NEW
Ever confirmed: true
Summary: Scripts loaded in the extension pages should have access to all Chrome api → options_ui browsers should have access to the full WebExtension API
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.
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.
Flags: needinfo?(lgreco)
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.
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.
Flags: needinfo?(lgreco)
Attachment #8732323 - Flags: review?(kmaglione+bmo) → feedback?(kmaglione+bmo)
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`
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";
>      }
Attachment #8732323 - Attachment description: MozReview Request: Bug 1256282 - [webext] Special handling ExtensionPages running in sub-frames. r?kmag → MozReview Request: Bug 1256282 - [webext] options_ui browsers should have access to the full WebExtension API. r?kmag
Attachment #8732323 - Flags: feedback?(kmaglione+bmo) → review?(kmaglione+bmo)
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/
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 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/
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.
Blocks: 1258347
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 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")`
Attachment #8732323 - Flags: review?(kmaglione+bmo)
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/
Attachment #8732323 - Flags: review?(kmaglione+bmo)
(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 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.
Attachment #8734471 - Flags: review?(kmaglione+bmo)
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.
Attachment #8732323 - Flags: review?(kmaglione+bmo)
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/
Attachment #8734471 - Attachment description: MozReview Request: Bug 1256282 - [webext] Add test unit for the internal getAPILevelForWindow helper. r?kmag → MozReview Request: Bug 1256282 - [webext] Fix addonId checks in getAPILevelForWindow and add an xpcshell test unit. r?kmag
Attachment #8732323 - Flags: review?(kmaglione+bmo)
Attachment #8734471 - Flags: review?(kmaglione+bmo)
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/
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.
Keywords: leave-open
Duplicate of this bug: 1261297
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.
Attachment #8734471 - Flags: review?(kmaglione+bmo) → review+
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.
Attachment #8732323 - Flags: review?(kmaglione+bmo)
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/
Attachment #8732323 - Flags: review?(kmaglione+bmo)
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/
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 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.
Attachment #8732323 - Flags: review?(kmaglione+bmo) → review+
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/
Attachment #8732323 - Attachment description: MozReview Request: Bug 1256282 - [webext] options_ui browsers should have access to the full WebExtension API. r?kmag → MozReview Request: Bug 1256282 - [webext] options_ui browsers should have access to the full WebExtension API. r=kmag
Attachment #8734471 - Attachment description: MozReview Request: Bug 1256282 - [webext] Fix addonId checks in getAPILevelForWindow and add an xpcshell test unit. r?kmag → MozReview Request: Bug 1256282 - [webext] Fix addonId checks in getAPILevelForWindow and add an xpcshell test unit. r=kmag
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/
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.
Last rebased and tweaked patches pushed to try:

- https://treeherder.mozilla.org/#/jobs?repo=try&revision=8c574e683e2f
Duplicate of this bug: 1263147
Whiteboard: triaged
Assignee: kmaglione+bmo → lgreco
Status: NEW → RESOLVED
Closed: 4 years ago
Keywords: leave-open
Resolution: --- → FIXED
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.