Closed
Bug 1256282
Opened 9 years ago
Closed 9 years ago
options_ui browsers should have access to the full WebExtension API
Categories
(WebExtensions :: Untriaged, defect)
WebExtensions
Untriaged
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'
Comment 1•9 years ago
|
||
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"
> },
Updated•9 years ago
|
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.
Comment 4•9 years ago
|
||
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.
Assignee | ||
Comment 6•9 years ago
|
||
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)
Assignee | ||
Comment 7•9 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/41097/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/41097/
Attachment #8732323 -
Flags: review?(kmaglione+bmo)
Assignee | ||
Updated•9 years ago
|
Attachment #8732323 -
Flags: review?(kmaglione+bmo) → feedback?(kmaglione+bmo)
Comment 8•9 years ago
|
||
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";
> }
Assignee | ||
Updated•9 years ago
|
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)
Assignee | ||
Comment 10•9 years ago
|
||
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/
Assignee | ||
Comment 11•9 years ago
|
||
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?
Assignee | ||
Comment 12•9 years ago
|
||
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/
Assignee | ||
Comment 13•9 years ago
|
||
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.
Assignee | ||
Comment 14•9 years ago
|
||
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•9 years ago
|
||
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)
Assignee | ||
Comment 16•9 years ago
|
||
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)
Assignee | ||
Comment 17•9 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/42271/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/42271/
Attachment #8734471 -
Flags: review?(kmaglione+bmo)
Assignee | ||
Comment 18•9 years ago
|
||
(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•9 years ago
|
||
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 20•9 years ago
|
||
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)
Assignee | ||
Comment 21•9 years ago
|
||
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)
Assignee | ||
Comment 22•9 years ago
|
||
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/
Assignee | ||
Comment 23•9 years ago
|
||
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.
Assignee | ||
Updated•9 years ago
|
Keywords: leave-open
Comment 25•9 years ago
|
||
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 26•9 years ago
|
||
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)
Assignee | ||
Comment 27•9 years ago
|
||
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)
Assignee | ||
Comment 28•9 years ago
|
||
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/
Assignee | ||
Comment 29•9 years ago
|
||
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 31•9 years ago
|
||
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+
Assignee | ||
Comment 32•9 years ago
|
||
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
Assignee | ||
Comment 33•9 years ago
|
||
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/
Assignee | ||
Comment 34•9 years ago
|
||
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.
Assignee | ||
Comment 35•9 years ago
|
||
Last rebased and tweaked patches pushed to try:
- https://treeherder.mozilla.org/#/jobs?repo=try&revision=8c574e683e2f
Assignee | ||
Updated•9 years ago
|
Keywords: checkin-needed
Comment 36•9 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/2677bb5bffff
https://hg.mozilla.org/integration/fx-team/rev/9dc2be94c435
Keywords: checkin-needed
Comment 37•9 years ago
|
||
bugherder |
Updated•9 years ago
|
Whiteboard: triaged
Updated•9 years ago
|
Assignee: kmaglione+bmo → lgreco
Status: NEW → RESOLVED
Closed: 9 years ago
Keywords: leave-open
Resolution: --- → FIXED
Updated•7 years ago
|
Product: Toolkit → WebExtensions
You need to log in
before you can comment on or make changes to this bug.
Description
•