1227462 - (CVE-2016-2817) chrome.tabs.update/create APIs should call checkLoadURI with DISALLOW_INHERIT_PRINCIPAL

Status: VERIFIED FIXED

(WebExtensions :: Untriaged, defect, P1)

Description

[Muneaki Nishimura]

Attachment: uxss.xpi

User Agent: Mozilla/5.0 (Windows NT 6.3; WOW64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/46.0.2490.86 Safari/537.36

Steps to reproduce:

The 'chrome.tabs.update' API allows to navigate tabs to javascript: URLs without any permissions. It can be used for universal XSS attacks to all documents that are opening. It affects not only to ordinary websites but also other extensions. If an other extension'd page is shown in a tab, then script can be  injected from the malicious extension with no permissions, i.e., obtain the target extension's privilege.
Attached is a sample extension that can reproduce the issue.


Actual results:

1. Open arbitrary web sites and extension's page on tabs
2. Click the button of this extension
3. Alert dialog with their current document.location is shown on each tab



Expected results:

The 'chrome.tabs.update' should prohibits navigation to harmful URLs e.g., javascript: and data:. Chrome do so.

Comment 1

[Muneaki Nishimura]

Attachment: Screenshots on my environment

I attached screenshots when the issue reproduced on my PC.

Comment 2

[Daniel Veditz [:dveditz]]

Regular add-ons can do this an more already, of course, hopefully bad stuff caught in review. WebExtensions have stricter limits, this is somewhere between sec-moderate and sec-high.

Comment 3

[Kris Maglione [:kmag]]

There are probably a few other places we need to do this as well. And it should technically be `checkLoadURIStrWithPrincipal`, but that's not as summary-friendly.

Comment 4

[Tyler Steiman]

I'd love to work on this if I could get some more information and what part of the source code is involved here.

Comment 5

[Kris Maglione [:kmag]]

For tabs.update and tabs.create, the checks need to happen somewhere near:

https://dxr.mozilla.org/mozilla-central/source/browser/components/extensions/ext-tabs.js#337
https://dxr.mozilla.org/mozilla-central/source/browser/components/extensions/ext-tabs.js#267

The principal for checkLoadURI should be `context.contentWindow.document.nodePrincipal`. There are a few places we do similar checks in ext-utils.js, if you need examples.

Comment 6

[Muneaki Nishimura]

It seems that an similar issue was fixed on the Firefox 43 and that severity is critical.
https://www.mozilla.org/en-US/security/advisories/mfsa2015-148/
I'm not sure what was the difference between that issue and this, anyway if needed please rate the severity of this bug again.

Comment 7

[Kris Maglione [:kmag]]

That issue exposed elevated privileges to web content. This exposes elevated privileges to extension code, which is more trusted than web code, and currently comes with no promises of privilege separation. At the time this bug was reported, we still hadn't implemented permissions checking for `tabs.executeScript`, for instance.

Comment 8

[Andy McKay]

(In reply to Tyler Steiman from comment #4)
> I'd love to work on this if I could get some more information and what part
> of the source code is involved here.

Have you had chance to look at this yet?

Comment 9

[Kris Maglione [:kmag]]

Luca, this might be a good bug for you. We have a `checkLoadURL` method in the `context` objects (which are ExtensionPage instances) now.

Comment 10

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

(In reply to Kris Maglione [:kmag] from comment #9)
> Luca, this might be a good bug for you. We have a `checkLoadURL` method in
> the `context` objects (which are ExtensionPage instances) now.

Thanks Kris, I've done a couple of tests about how Chromium behaves in this scenarios and it looks that "javascript:" URLs are checked like javascript evaluation requests on the tabs.update target (mostly like in tabs.executeScript).

tabs.create
- "javascript:" URLs execute successfully in a new "about:blank" tab
- "about:" or "chrome:" URLs do load correctly in a new tab

tabs.update
- "javascript:" URLs
  - logged error: Cannot access a chrome:// URL
  - logged error: Cannot access a chrome-extension:// URL of different extension
  - logged error: Cannot access contents of url "http...". 
                  Extension manifest must request permission to access this host.
- "about:" or "chrome:" URLs do load correctly in the existent tab

checkLoadURL seems to intended to check if a defined URL can be loaded by the current context (the ExtensionPage, but I didn't find any current usage of it) instead of doing the evaluation based on the principal of the target tab.

I'm going to upload a proposed patch which:
- define a new "checkAccessTargetPrincipal" helper in the ExtensionPage prototype
- introduce activeTab and whitelists's MatchPattern behavior in the tabs.update API method
- introduce more tests in the tabs.update test file

In the test cases I have to add a group which tests the error logged by trying accessing 
a tab from a different extension (currently the raise an exception if I try to load a second
extension)

Comment 11

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

Attachment: 0001-Bug-1227462-tabs.update-should-check-it-has-permissi.patch

This draft patch contains the proposed changes to the ExtensionPage and tabs.update.

Comment 12

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

Attachment: 0002-Bug-1227462-Test-tabs.update-on-javascript-URLs.-r-k.patch

This patch introduces more tests on tabs.update (in particular its behaviour on "javascript:" url applied to different kinds of target tabs)

Comment 13

[Kris Maglione [:kmag]]

Comment on attachment 8706412 [details] [diff] [review]
0001-Bug-1227462-tabs.update-should-check-it-has-permissi.patch

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

I'm not sure we need to do anything this complicated.

The checkLoadURI function is the correct way to check if a principal is allowed to load a URI. It already has special logic for dealing with extension principals, and if that's insufficient, it should be fixed at the platform level.

The principal of the target tab doesn't really matter. If we do the check with the DISALLOW_INHERIT_PRINCIPAL flag, javascript: and data: URLs which inherit the page's principal won't be accepted, no matter what the principal is.

I suppose we could add a special case to allow it when the extension has access to that page, but it doesn't seem necessary. They can already execute scripts via `executeScript`, which already has the appropriate checks in place.

::: browser/components/extensions/ext-tabs.js
@@ +331,5 @@
>          let tabbrowser = tab.ownerDocument.defaultView.gBrowser;
> +
> +        let matchesHost;
> +
> +        TabManager.for(extension).addActiveTabPermission(TabManager.activeTab);

I don't understand why we would do this.

@@ +347,3 @@
>          if (updateProperties.url !== null) {
> +          let updateURI = Services.io.newURI(updateProperties.url, null, null);
> +          let targetPrincipal = tab.linkedBrowser.contentDocument.nodePrincipal;

This won't work under e10s.

@@ +350,5 @@
> +
> +          // TODO: checkAccessTarget should set runtime.lastError
> +          if (updateURI.scheme !== "javascript" ||
> +              context.checkAccessTargetPrincipal(targetPrincipal, matchesHost)) {
> +            tab.linkedBrowser.loadURI(updateProperties.url);

This check isn't safe under e10s. The tab may have loaded a different document by the time we load a URL into it.

::: toolkit/components/extensions/Extension.jsm
@@ +247,5 @@
>    get principal() {
>      return this.contentWindow.document.nodePrincipal;
>    },
>  
> +  checkAccessTargetPrincipal(targetPrincipal, matchesHost) {

I don't think this is necessary.

As far as cross-origin requests go, we already check this at the platform layer.

For load access, we do the same, though it's probably somewhat more restrictive than this check at the moment.

Comment 14

[Kris Maglione [:kmag]]

Comment on attachment 8706414 [details] [diff] [review]
0002-Bug-1227462-Test-tabs.update-on-javascript-URLs.-r-k.patch

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

f+ on the general implementation, but I think the behavior we're testing for should be different.

::: browser/components/extensions/test/browser/browser_ext_tabs_update.js
@@ +51,5 @@
> +function* testAccessTarget(url, permissions) {
> +  let extension = ExtensionTestUtils.loadExtension({
> +    manifest: {
> +      "permissions": ["tabs"].concat(permissions),
> +      "webAccessibleResources": [ "tab.html" ],

No spaces around square brackets.

@@ +89,5 @@
> +      });
> +    },
> +  });
> +
> +  let awaitExtensionTabURL = extension.awaitMessage("ready");

There's no need to do this ahead of time. Just use `yield extension.awaitMessage("ready")` when you need it.

@@ +101,5 @@
> +  info(`Testing on tab with URL ${url}`);
> +
> +
> +  let tab1 = yield BrowserTestUtils.openNewForegroundTab(gBrowser, url);
> +  gBrowser.selectedTab = tab1;

This isn't necessary. `openNewForegroundTab` already does it.

@@ +109,5 @@
> +
> +  yield BrowserTestUtils.removeTab(tab1);
> +  yield extension.unload();
> +
> +  let errors = Services.console.getMessageArray() || [];

This is problematic for a few reasons. You need to use `SimpleTest.monitorConsole` before you start your test and `endMonitorConsole` when you're done. There are a few other tests that do this.

@@ +125,5 @@
> +  info("Start testing tabs.update on javascript URLs");
> +
> +  let testCases = [{
> +    tabURL: "http://example.net",
> +    extensionSpecialPermissions: ["activeTab"],

The active tab permission should only grant permission for the active tab after the user has interacted with it (e.g., by clicking the extension's toolbar button), so it shouldn't apply here.

Comment 15

[Frederik Braun [:freddy] (reo duty for Fx124)]

(In reply to Luca Greco [:rpl] from comment #10)
> (In reply to Kris Maglione [:kmag] from comment #9)
> > Luca, this might be a good bug for you. We have a `checkLoadURL` method in
> > the `context` objects (which are ExtensionPage instances) now.
> 
> Thanks Kris, I've done a couple of tests about how Chromium behaves in this
> scenarios and it looks that "javascript:" URLs are checked like javascript
> evaluation requests on the tabs.update target (mostly like in
> tabs.executeScript).


This comparison should include tests for data: URLs as their behavior is very different between Chrome and Firefox (i.e., data: inherits context in Firefox, but gets a blank context in Chrome)

Comment 16

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

(In reply to Kris Maglione [:kmag] from comment #13)
> Comment on attachment 8706412 [details] [diff] [review]
> 0001-Bug-1227462-tabs.update-should-check-it-has-permissi.patch
> 
> Review of attachment 8706412 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I'm not sure we need to do anything this complicated.
> 
> The checkLoadURI function is the correct way to check if a principal is
> allowed to load a URI. It already has special logic for dealing with
> extension principals, and if that's insufficient, it should be fixed at the
> platform level.
> 
> The principal of the target tab doesn't really matter. If we do the check
> with the DISALLOW_INHERIT_PRINCIPAL flag, javascript: and data: URLs which
> inherit the page's principal won't be accepted, no matter what the principal
> is.
> 
> I suppose we could add a special case to allow it when the extension has
> access to that page, but it doesn't seem necessary. They can already execute
> scripts via `executeScript`, which already has the appropriate checks in
> place.

I agree, as tabs.executeScript is a better and more explicit way to execute javascript code in the context of a target tab window, I think we can be more restrictive on what kind of url we can load in a tab using tabs.update (and put a note about this in the "WebExtensions - Chrome incompatibilities" MDN doc page).

I'm going to rewrite the patch to simply use the checkLoadURL to test the url and change the tests accordingly (e.g. the different expected error messages, and use different combinations of existent tab url and url to be loaded using tabs.update, like "about:", regular webpages url, "javascript:" and "data:" url, as suggested by Frederik)

Comment 17

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

Attachment: 0001-Bug-1227462-tabs.update-should-call-checkLoadURL.-r-.patch

This patch is an update version of the Attachment 8706412 [details] [diff] which implements the more restricted tabs.update.

This new "tabs.update" version may load into a target tab:

- regular web pages URLs
- non privileged "about:" URLs (e.g. about:blank)
- pages from the same extension

it cannot load into a target tab:
- privileged "about:" URLs (e.g. about:config)
- javascript URLs
- data URLs

Comment 18

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

Attachment: 0002-Bug-1227462-Test-tabs.update-URLs.-r-kmag.patch

This patch is a rework of the new test from Attachment 0706414 [details] [diff]

It checks which kind of URL can be loaded in different types of existing tabs and when it will log an error
(and it checks that successes and failures only depend from the URL that is going to be loaded, and do not depend from the url that is currently loaded in the target tab)

Comment 19

[Kris Maglione [:kmag]]

Comment on attachment 8707641 [details] [diff] [review]
0001-Bug-1227462-tabs.update-should-call-checkLoadURL.-r-.patch

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

We need to do this for tabs.create as well.

Comment 20

[Kris Maglione [:kmag]]

Comment on attachment 8707651 [details] [diff] [review]
0002-Bug-1227462-Test-tabs.update-URLs.-r-kmag.patch

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

Let's move this to its own test file. It's probably going to trigger timeouts on debug builds if it runs with the other update tests.

::: browser/components/extensions/test/browser/browser_ext_tabs_update.js
@@ +52,5 @@
> +  let extension = ExtensionTestUtils.loadExtension({
> +    manifest: {
> +      "backgound": { scripts: ["background.js"] },
> +      "permissions": ["tabs"],
> +      "webAccessibleResources": [ "tab.html" ],

No spaces inside of square brackets.

@@ +78,5 @@
> +          for (let tab of tabs.slice(1)) {
> +            updates.push(new Promise((resolve, reject) => {
> +              browser.tabs.update(tab.id, {
> +                url: tabsUpdateURL,
> +              }, resolve);

We're only intending to update one tab, right?

@@ +178,5 @@
> +          .map((check) => Object.assign({}, check, { existentTabURL }))
> +          .filter(el => el.tabsUpdateURL != el.existentTabURL);
> +
> +    return acc.concat(testSet);
> +  }, []);

I don't think we need to do this anymore. We're not checking what's currently loaded into the tab, so it shouldn't make a difference.

Comment 21

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

Attachment: 0001-Bug-1227462-tabs.update-should-call-checkLoadURL.-r-.patch

Same patch as Attachment 8707641 [details] [diff] (just rebased)

Comment 22

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

Attachment: 0002-Bug-1227462-Test-tabs.update-URLs.-r-kmag.patch

Updated test case for the new tabs.update behavior:

- split the test case in a separated test file
- simplified as suggested in Comment 20

Comment 23

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

Attachment: 0003-Bug-1227462-tabs.create-should-call-checkLoadURL.-r-.patch

This patch introduces the checkLoadURL on tabs.create as well.

Comment 24

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

Attachment: 0004-Bug-1227462-Test-tabs.create-URLs.-r-kmag.patch

This patch introduces a new test file which tests the new tabs.create behaviour.

Comment 25

[Kris Maglione [:kmag]]

Comment on attachment 8709382 [details] [diff] [review]
0002-Bug-1227462-Test-tabs.update-URLs.-r-kmag.patch

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

::: browser/components/extensions/test/browser/browser_ext_tabs_update_url.js
@@ +47,5 @@
> +
> +  info(`tab.update URL "${tabsUpdateURL}" on tab with URL "${existentTabURL}"`);
> +
> +  let tab1 = yield BrowserTestUtils.openNewForegroundTab(gBrowser, existentTabURL);
> +  gBrowser.selectedTab = tab1;

|openNewForegroundTab| should take care of this.

@@ +52,5 @@
> +
> +  extension.sendMessage("start", tabsUpdateURL);
> +  yield extension.awaitMessage("done");
> +
> +  yield BrowserTestUtils.removeTab(tab1);

It would be good to check the final URL of the tab before we remove it.

@@ +57,5 @@
> +  yield extension.unload();
> +
> +  let errors = Services.console.getMessageArray() || [];
> +
> +  Services.console.reset();

I mentioned this before: We should really use |SimpleTest.monitorConsole| and |SimpleTest.endMonitorConsole| for this.

Console messages are handled asynchronously, so unless you take special precautions, there's a race here. Clearing the console is also not really good test etiquette.

Comment 26

[Kris Maglione [:kmag]]

Comment on attachment 8709383 [details] [diff] [review]
0003-Bug-1227462-tabs.create-should-call-checkLoadURL.-r-.patch

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

::: browser/components/extensions/ext-tabs.js
@@ +264,5 @@
>          let url = createProperties.url || aboutNewTabService.newTabURL;
>          url = context.uri.resolve(url);
>  
> +        if (!context.checkLoadURL(url)) {
> +          Cu.reportError(`Permission denied on tabs.create with url "${url}"`);

Let's skip this for now. checkLoadURI already reports the error. We can add more specific reporting when we add lastError support.

Comment 27

[Kris Maglione [:kmag]]

Comment on attachment 8709385 [details] [diff] [review]
0004-Bug-1227462-Test-tabs.create-URLs.-r-kmag.patch

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

r=me with the same caveats as the tabs.update tests.

::: browser/components/extensions/test/browser/browser_ext_tabs_create_url.js
@@ +31,5 @@
> +        browser.tabs.create({ url: tabsCreateURL }, (tab) => {
> +          if (tab) {
> +            browser.tabs.remove(tab.id);
> +          }
> +          browser.test.sendMessage("done");

Please check that the tab is undefined when we're expecting an error.

Comment 28

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

Attachment: 0001-Bug-1227462-tabs.update-should-call-checkLoadURL.-r-.patch

To cover some of the review comments on the test cases, I had to apply some of the changes on this first patch (and I reworked the method to be
more readable and to better reproduce some of the expected behaviour, based on more tests on other Chromium based browsers): 

- removed Cu.reportError (I reported the error there mainly to prevents them to become new silent errors sources, but the runtime.lastError is not so far so, I agree and prefer to leave a comment in the mean time and then update the test case to check it in the callbacks, and remove completely any console service usage in the tests)

- reworked tabs.update to ensure to better reproduce some details of the API behaviour on Chromium:

  - the callback tab parameter should be null when the request is totally invalid

  - the URL in the callback tab parameter should be the requested one 
    (Chromium don't care if the loading is not yet started or completed, 
    on the contrary the other tab fields, e.g. the title, are unmodified)

The new version of this patch introduces a small tweak in ext-utils.js:
    
- small tweak on TabManager.convert(...) to ensure it accepts and returns 
  undefined when its tab parameter is undefined or null

NOTE: added f? due to the reworked tab.update method and the small change to ext-utils.js

Comment 29

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

Attachment: 0002-Bug-1227462-Test-tabs.update-URLs.-r-kmag.patch

This new version introduces the suggested changes, the result is a simplified and cleaner version of the new test file introduced by this patch.

- removed any Service.console usage (it is definitely better to check the runtime.lastError in this test case, once it lands and the tabs API methods support it)
    
- removed gBrowser.selectedTab (ouch! sorry, I forgot to removed it from this patch)

- test the tab in the tabs.update callback:
  - on error, tab should be null 
  - on success, tab should be defined and the tab.url should be url in the tabs.update request

Comment 30

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

Attachment: 0003-Bug-1227462-tabs.create-should-call-checkLoadURL.-r-.patch

- fixed bug (aboutNewTabService.newTabURL is usually an "about:" or a "chrome:" URL and it will not pass checkLoadURL)

- fixed nit, moved the "TODO: runtime.lastError" out of the "if (callback)" branch

- minor rework of the tabs.create method, which better reproduces some details of the chrome behavior:

  - the URL in the tab Details should be pre-populated with the requested URL,
  - the callback tab parameter should be undefined when the request is invalid

NOTE: added f? due to the rework on the tabs.create method and the fix applied to do not break the "aboutNewTabService.newTabURL" scenario

Comment 31

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

Attachment: 0004-Bug-1227462-Test-tabs.create-URLs.-r-kmag.patch

This new version of the last patch introduces the suggested changes, plus the same simplifications and clean up introduced in the last version of the other test file (the tabs.update test file from Attachment 8710004 [details] [diff]).

Comment 32

[Kris Maglione [:kmag]]

Comment on attachment 8710003 [details] [diff] [review]
0001-Bug-1227462-tabs.update-should-call-checkLoadURL.-r-.patch

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

Let's save this for a separate bug. Most of it isn't especially related to the issue here.

(In reply to Luca Greco [:rpl] from comment #28)
>   - the URL in the callback tab parameter should be the requested one 
>     (Chromium don't care if the loading is not yet started or completed, 
>     on the contrary the other tab fields, e.g. the title, are unmodified)

I'm on the fence about this. I've run into it before, but I'm not sure the
solution is to special case the URL in the callback arguments. Ideally we
should maintain some kind of internal state so queries also return the updated
URL, but that requires clearing it when we know the load failed.

Can you file a separate bug, and we can discuss it further there?

::: browser/components/extensions/ext-tabs.js
@@ +325,5 @@
>          let tabbrowser = tab.ownerDocument.defaultView.gBrowser;
> +
> +        // An update is valid if at least one of the supported updateProperties is valid
> +        // and the request is going to be handled.
> +        let isValidTabUpdate = false;

I don't think emulating this behavior is necessary. It's not documented, and it doesn't seem useful enough to justify the added complexity.

::: browser/components/extensions/ext-utils.js
@@ +421,5 @@
>    },
>  
>    convert(tab) {
> +    if (!tab) {
> +      return undefined;

I'd rather not do this if we can avoid it. If there are places where we're expecting the tab to be null, we can do |tab && tabManager.convert(...)|.

Comment 33

[Kris Maglione [:kmag]]

Comment on attachment 8710004 [details] [diff] [review]
0002-Bug-1227462-Test-tabs.update-URLs.-r-kmag.patch

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

f+, but let's save the callback URL checks for a follow-up, if we decide to make those changes.

In the mean time, let's just use an onUpdated observer. You can steal the helper function I added here, if it helps: https://hg.mozilla.org/integration/fx-team/file/8fc9a5eeede3/browser/components/extensions/test/browser/browser_ext_pageAction_context.js#l180

Comment 34

[Kris Maglione [:kmag]]

Comment on attachment 8710008 [details] [diff] [review]
0003-Bug-1227462-tabs.create-should-call-checkLoadURL.-r-.patch

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

Let's also save this for a follow-up.

::: browser/components/extensions/ext-tabs.js
@@ +265,5 @@
>          url = context.uri.resolve(url);
>  
> +        // Skip checkLoadURL if the URL has been populated from the aboutNewTabService,
> +        // (it is usually an "about:" or "chrome:" URL).
> +        if (url != aboutNewTabService.newTabURL && !context.checkLoadURL(url)) {

I'd rather not special case the new tab URL. Extensions can already load it by simply not passing a URL. If we decide to add a separate special case for it, it should be on the platform side rather than here.

Comment 35

[Kris Maglione [:kmag]]

Comment on attachment 8710011 [details] [diff] [review]
0004-Bug-1227462-Test-tabs.create-URLs.-r-kmag.patch

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

Same as the other patch. f+, but let's save the URL property checks for a separate bug.

Comment 36

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

Attachment: 0001-Bug-1227462-tabs.update-should-call-checkLoadURL.-r-.patch

Updated the patch with the following changes:

- removed the change on ext-utils.js

- changed the detection of invalid tabs.update requests
  (I think that we have to handle this new scenario, 
  the tabs.update apparently can only fail on invalid tabId on Chromium,
  so this is a Firefox special case, and I think that returning 
  the tab details to the callback when we have basically discarded
  the request can be misleading)

Comment 37

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

Attachment: 0002-Bug-1227462-Test-tabs.update-URLs.-r-kmag.patch

Updated "tabs.update test case" patch:

- removed assertion on the tab.url (which will be discussed and eventually fixed in a follow up)

Comment 38

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

Attachment: 0003-Bug-1227462-tabs.create-should-call-checkLoadURL.-r-.patch

Updated patch on tabs.create:

- cleaned up the tabs.create changes

I had to handle the empty url differently because on empty URLs:

- context.uri.resolve will resolve it as an URL relative to the
  context base URI
- gBrowser.addTab will open a tab on "about:blank", instead of opening
  it on the expected URL

Currently, if we do not use aboutNewTabService.newTabURL, we break a number
of test cases that are counting on this.

Nevertheless, I think that the current implementation is wrong because,
I think it is supposed to behave like the keyboard shortcut or the menu item
that let the user opening a new tab, and:

- the "chrome://..." URL is visible in the location bar (and it shouldn't)
- on a private window it should show "about:privatebrowsing" instead of
  "about:newtab"

I've some ideas (and a preliminary patch for it, I need to complete to fix the test cases),
but it would be better to move the discussion in a separate bugzilla issue as a follow-up.

Comment 39

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

Attachment: 0004-Bug-1227462-Test-tabs.create-URLs.-r-kmag.patch

Updated tabs.create test:

- removed all the kind of URLs which are already tested by the other tabs.create test file (now it tests only invalid URLs)

Maybe I should rename the file accordingly.

Comment 40

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

Attachment: 0005-Bug-1227462-fix-tabs.onUpdated-test-case.-r-kmag.patch

Small change on the test file for tabs.onUpdated, which cannot use anymore "about:preferences" as a valid url for the tabs.update method call
that is used to generate the update events.

Comment 41

[Kris Maglione [:kmag]]

Comment on attachment 8710579 [details] [diff] [review]
0001-Bug-1227462-tabs.update-should-call-checkLoadURL.-r-.patch

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

::: browser/components/extensions/ext-tabs.js
@@ +323,5 @@
>        update: function(tabId, updateProperties, callback) {
>          let tab = tabId !== null ? TabManager.getTab(tabId) : TabManager.activeTab;
>          let tabbrowser = tab.ownerDocument.defaultView.gBrowser;
> +
> +        if (tab) {

Let's just return early here if there's no matching tab.

@@ +353,5 @@
> +              let keyIsNotURL = (k) => k !== "url";
> +              let valueIsNotNull = (k) => updateProperties[k] !== null;
> +              let otherUpdatedProperties = Object.keys(updateProperties)
> +                                                 .filter(keyIsNotURL)
> +                                                 .some(valueIsNotNull);

I don't think there's any reason to try to exactly mimic Chrome's behavior here. Let's just check this first. If the URL is illegal, we should return immediately with an error, rather than trying to handle cases where we update other properties but then reject the URL.

Comment 42

[Kris Maglione [:kmag]]

Comment on attachment 8710580 [details] [diff] [review]
0002-Bug-1227462-Test-tabs.update-URLs.-r-kmag.patch

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

::: browser/components/extensions/test/browser/browser_ext_tabs_update_url.js
@@ +4,5 @@
> +
> +function* testTabsUpdateURL(existentTabURL, tabsUpdateURL, isErrorExpected) {
> +  let extension = ExtensionTestUtils.loadExtension({
> +    manifest: {
> +      "backgound": { scripts: ["background.js"] },

This line is unnecessary (and should probably be an error).

@@ +31,5 @@
> +            if (isErrorExpected) {
> +              // TODO: check runtime.lastError once supported
> +              browser.test.assertEq(undefined, tab, "on error tab should be undefined");
> +            } else {
> +              browser.test.assertTrue(!!tab, "on success the tab should be defined");

No need for the !!

Comment 43

[Kris Maglione [:kmag]]

Comment on attachment 8710588 [details] [diff] [review]
0003-Bug-1227462-tabs.create-should-call-checkLoadURL.-r-.patch

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

::: browser/components/extensions/ext-tabs.js
@@ +278,4 @@
>  
>          function createInWindow(window) {
> +          if (!url) {
> +            url = aboutNewTabService.newTabURL;

Let's just set this in the `else` of `if (createProperties.url)`

@@ +299,5 @@
>              window.gBrowser.pinTab(tab);
>            }
>  
>            if (callback) {
> +            runSafe(context, callback, tab && TabManager.convert(extension, tab));

`tab` should never be null here, so let's skip the `tab && `

Comment 44

[Kris Maglione [:kmag]]

Comment on attachment 8710594 [details] [diff] [review]
0004-Bug-1227462-Test-tabs.create-URLs.-r-kmag.patch

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

::: browser/components/extensions/test/browser/browser_ext_tabs_create_url.js
@@ +4,5 @@
> +
> +function* testTabsCreateInvalidURL(tabsCreateURL) {
> +  let extension = ExtensionTestUtils.loadExtension({
> +    manifest: {
> +      "backgound": { scripts: ["background.js"] },

This isn't necessary.

Comment 45

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

Attachment: 0001-Bug-1227462-tabs.update-should-call-checkLoadURL.-r-.patch

First patch of the queue, updated with the suggested tweaks:

- return early if there isn't any matching tab
- return early if the URL is invalid (even if it isn't the only property that the add-on wants to update)

In both the above cases the callback |tab| parameter will be |undefined|.

Thanks a lot Kris for your feedback on this:

this "invalid URL" scenario is "Firefox"-specific, so I wasn't really try to mimic any particular Chrome behaviour, but I wanted to find a behaviour for Firefox which could be reasonable (and I think that the last suggested behaviour is pretty good and that it will not be misleading)

Comment 46

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

Attachment: 0002-Bug-1227462-Test-tabs.update-URLs.-r-kmag.patch

Fixed the two small nits on the new tabs.update test file.

Comment 47

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

Attachment: 0003-Bug-1227462-tabs.create-should-call-checkLoadURL.-r-.patch

Updated the patch on tabs.create, applying the following suggested tweaks:

- set the "newtab" fallback url in the else branch
- remove the |tab && ...| because tab should never be null

Set the previous r+ flag.

Comment 48

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

Attachment: 0004-Bug-1227462-Test-tabs.create-URLs.-r-kmag.patch

Fixed the nit on the new tabs.create test file, and renamed it to "browser_ext_tabs_create_invalid_url.js".

Set the previous f+ flag.

Comment 49

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

Attachment: 1-Bug_1227462___tabs_create_and_tabs_update_should_check_URLs_using_checkLoadURL__r_kmag.patch

Comment 50

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

Attachment: 2-Bug_1227462___Test_tabs_create_update_on_allowed_and_disallowed_URLs__r_kmag.patch

Comment 51

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

Comment on attachment 8717298 [details] [diff] [review]
1-Bug_1227462___tabs_create_and_tabs_update_should_check_URLs_using_checkLoadURL__r_kmag.patch

This patch is a rebased and updated version of the past main two patches attached to this issue:

- squashed the two patches for tabs.update and tabs.create in a single patch
- ported to the new Promise based API

- checkLoadURL:
  - the new |dontReportErrors| option prevents it from logging into the console errors which we are already reporting using a rejected promise / runtime.lastError

- tabs.create
  - checks the request url, if specified, and do not open a new tab if the url 
    is not allowed (and return a rejected Promise to set the lastError accordingly)
  - preserved the changes introduce by Bug 1242588 (new tab url if url is missing)

- tabs.update
  - rejects and set lastError if no tab is found with the requested tabId
  - rejects and set lastError if the requested url is not allowed
  - the requested url is resolved using context.uri.resolved (as tabs.create already does)

Comment 52

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

Comment on attachment 8717299 [details] [diff] [review]
2-Bug_1227462___Test_tabs_create_update_on_allowed_and_disallowed_URLs__r_kmag.patch

Updated version of the patches with the new two test cases "browser_ext_tabs_create_invalid_url.js" and "browser_ext_tabs_update_url.js" (ported to the new Promise based API, added assertion related to the Promise rejected error message) and the small tweak on the "browser_ext_tabs_onUpdated.js" test.

Comment 53

[Kris Maglione [:kmag]]

Comment on attachment 8717298 [details] [diff] [review]
1-Bug_1227462___tabs_create_and_tabs_update_should_check_URLs_using_checkLoadURL__r_kmag.patch

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

::: browser/components/extensions/ext-tabs.js
@@ +293,5 @@
>  
> +          let url;
> +
> +          if (createProperties.url !== null) {
> +            url = context.uri.resolve(createProperties.url);

I'd rather keep this check in the createInWindow function. I know we technically don't need to wait for the window to load before checking if the URL is legal, but I'd find it easier to follow if all of URL processing was in one place.

Comment 54

[Kris Maglione [:kmag]]

Comment on attachment 8717299 [details] [diff] [review]
2-Bug_1227462___Test_tabs_create_update_on_allowed_and_disallowed_URLs__r_kmag.patch

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

::: browser/components/extensions/test/browser/browser_ext_tabs_create_invalid_url.js
@@ +41,5 @@
> +add_task(function* () {
> +  info("Start testing tabs.create on invalid URLs");
> +
> +  let dataURLPage = `
> +    data:text/html,<!DOCTYPE html>

Please make sure to trim the leading whitespace from this.

::: browser/components/extensions/test/browser/browser_ext_tabs_update_url.js
@@ +5,5 @@
> +function* testTabsUpdateURL(existentTabURL, tabsUpdateURL, isErrorExpected) {
> +  let extension = ExtensionTestUtils.loadExtension({
> +    manifest: {
> +      "permissions": ["tabs"],
> +      "webAccessibleResources": ["tab.html"],

This shouldn't be necessary. Also, if it is necessary it needs to be "web_accessible_resources".

@@ +25,5 @@
> +    background: function() {
> +      browser.test.sendMessage("ready", browser.runtime.getURL("tab.html"));
> +
> +      browser.test.onMessage.addListener((msg, tabsUpdateURL, isErrorExpected) => {
> +        let onTabsUpdated = (tab) => isErrorExpected ?

Please use if-else statements for these.

Comment 55

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

Attachment: 1-Bug_1227462___tabs_create_and_tabs_update_should_check_URLs_using_checkLoadURL__r_kmag.patch

Updated patch (applied changes based on the last review comments)

Comment 56

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

Attachment: 2-Bug_1227462___Test_tabs_create_update_on_allowed_and_disallowed_URLs__r_kmag.patch

Updated patch (applied minor changes based on the last review comments)

Comment 57

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

Attachment: 1-Bug_1227462___tabs_create_and_tabs_update_should_check_URLs_using_checkLoadURL__r_kmag.patch

Patch rebased, previous review flag state set.

Comment 58

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

Attachment: 2-Bug_1227462___Test_tabs_create_update_on_allowed_and_disallowed_URLs__r_kmag.patch

Patch rebased, previous review flag state set.

Comment 59

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

Comment on attachment 8721380 [details] [diff] [review]
1-Bug_1227462___tabs_create_and_tabs_update_should_check_URLs_using_checkLoadURL__r_kmag.patch

[Security approval request comment]
How easily could an exploit be constructed based on the patch?

   Any webextension addon can trivially exploit this issue to gain access to
   higher privileges provided by tabs with higher privileged origins, 
   by targeting them using chrome.tabs.create/chrome.tabs.update with 
   "data:" or "javascript:" URLs.
   
Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?

   
   The attached xpi file, the new tests included and the early comments 
   describe the issue with enough details:

   In the initial version of the webextension sandbox mechanisms,
   the chrome.tabs.create and chrome.tabs.update API methods
   didn't check the URL which the addon wants to load in a new or
   existent tab.

   "data:" and "javascript:" URLs inherit the existent principal,
   which means that the addon will be able to run arbitrary javascript code
   with the privileges of the principal currently associated with the document
   loaded in the tab (and some of the tabs could have higher privileges than the
   webextension addons, e.g. about:addons, "about:config")

Which older supported branches are affected by this flaw?

   Firefox >= 42, because this issue has been introduced in the initial import 
   of the WebExtension implementation.

   it is worth to mention that the new security sandboxing for WebExtensions
   was still in a prototype stage in those versions, and this level of sandbox
   restriction isn't enforced on no one of the other existent extensions types.

If not all supported branches, which bug introduced the flaw?

   N/A

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?

   Backport this fix is possible but a rewrite is needed, because some of the
   internals are changed over the time.

How likely is this patch to cause regressions; how much testing does it need?

   This patch can only affect addons based on webextensions APIs, 

   we have added new test cases to test the new restrictions and fixed tests 
   that were based on wrong assumptions.

   It's still possible that this would also cause problems for webextensions 
   which might depend on being able to load data: or javascript: URLs, 
   which they may not be doing for malicious reasons (especially those who
   have been ported from Chrome which allows the use of this kind of URLs).

Comment 60

[Al Billings [:abillings - ex-MoCo]]

Comment on attachment 8721380 [details] [diff] [review]
1-Bug_1227462___tabs_create_and_tabs_update_should_check_URLs_using_checkLoadURL__r_kmag.patch

sec-approval+.

We should take this on Aurora and Beta. Please make and/or nominate patches today.

Comment 61

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

Attachment: Bug_1227462___Backport_chrome_tabs_update_create_APIs_DISALLOW_INHERIT_PRINCIPAL_to_Aurora__r_kmag.patch

Backport to Aurora of the changes needed to fix this issue (and an existent test that needs to be fixed once the fix is applied).

Try push:
- https://treeherder.mozilla.org/#/jobs?repo=try&revision=d9050889c35b

Comment 62

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

Attachment: Bug_1227462___Backport_chrome_tabs_update_create_APIs_DISALLOW_INHERIT_PRINCIPAL_to_Beta__r_kmag.patch

Backport to Beta of the changes needed to fix this issue (and an existent test that needs to be fixed once the fix is applied).

Try push:
- https://treeherder.mozilla.org/#/jobs?repo=try&revision=ed65761212cc

Comment 63

[Kris Maglione [:kmag]]

Luca, do your tests apply cleanly to beta and aurora?

Comment 64

[Kris Maglione [:kmag]]

Comment on attachment 8721494 [details] [diff] [review]
Bug_1227462___Backport_chrome_tabs_update_create_APIs_DISALLOW_INHERIT_PRINCIPAL_to_Beta__r_kmag.patch

Approval Request Comment
See comment 59.

[String/UUID change made/needed]:

None.

Comment 65

[Kris Maglione [:kmag]]

Comment on attachment 8721493 [details] [diff] [review]
Bug_1227462___Backport_chrome_tabs_update_create_APIs_DISALLOW_INHERIT_PRINCIPAL_to_Aurora__r_kmag.patch

Approval Request Comment
See comment 59.

[String/UUID change made/needed]:

None.

Comment 66

[Al Billings [:abillings - ex-MoCo]]

Comment on attachment 8721494 [details] [diff] [review]
Bug_1227462___Backport_chrome_tabs_update_create_APIs_DISALLOW_INHERIT_PRINCIPAL_to_Beta__r_kmag.patch

sec-approval is only needed for trunk.

Comment 67

[Kris Maglione [:kmag]]

Sorry, I thought I'd cleared that flag.

Comment 68

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

Attachment: Bug_1227462___Backport_test_tabs_create_update_on_allowed_and_disallowed_URLs_to_Aurora__r_kmag.patch

Backport to Aurora of new test cases which explicitly tests the allowed and disallowed URLS.

Unfortunately the original patch couldn't be used like it was because in Aurora the WebExtensions APIs do not support lastError and the Promise-based behaviour.

Try push:
- https://treeherder.mozilla.org/#/jobs?repo=try&revision=51e428bf9654

Comment 69

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

Comment on attachment 8721711 [details] [diff] [review]
Bug_1227462___Backport_test_tabs_create_update_on_allowed_and_disallowed_URLs_to_Aurora__r_kmag.patch

Wrong patch attached by mistake, set as obsolete.

Comment 70

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

Attachment: Bug_1227462___Backport_test_tabs_create_update_on_allowed_and_disallowed_URLs_to_Aurora__r_kmag.patch

Backport of the new test cases on Aurora.

Pushed to try:
- https://treeherder.mozilla.org/#/jobs?repo=try&revision=c3ec9328bfb8

Comment 71

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

Attachment: Bug_1227462___Backport_test_tabs_create_update_on_allowed_and_disallowed_URLs_to_Beta__r_kmag.patch

Backport of the new tests on Beta (the only difference between this patch and the patch applied on Aurora is related to the changes on the browser.ini file).

Pushed to try:
- https://treeherder.mozilla.org/#/jobs?repo=try&revision=d42d3c66e6d9

Comment 72

[Sylvestre Ledru [:Sylvestre]]

For now, I am not planning to take this uplift:
* not landed in m-c yet (on purpose? in this case 47 should not be marked as affected)
* I don't have the risk evaluation
* it is a sec moderate

Comment 73

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

Attachment: 1-Bug_1227462___tabs_create_and_tabs_update_should_check_URLs_using_checkLoadURL__r_kmag.patch

Unfortunately I had to update this patch (the one which is going to land on m-c), because the previous version will not pass one of newly added eslint rules.

Re-applied the previous r+ flag, 'cause it differs only in the fixed eslint errors (unneeded spaces inside object's curly brackets).

Comment 74

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

Attachment: 2-Bug_1227462___Test_tabs_create_update_on_allowed_and_disallowed_URLs__r_kmag.patch

Unfortunately, as for the previous one, I had to update this patch (the one which is going to land on m-c), because the previous version will not pass one of the newly added eslint rules.

Re-applied the previous r+ flag, 'cause it differs only in the fixed eslint errors (unneeded spaces inside object's curly brackets).

Comment 75

[Kris Maglione [:kmag]]

I didn't realize this hadn't landed on m-c yet.

Luca, is there a reason you haven't requested checkin yet?

Comment 76

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

(In reply to Kris Maglione [:kmag] from comment #75)
> I didn't realize this hadn't landed on m-c yet.
> 
> Luca, is there a reason you haven't requested checkin yet?

Fixed eslint errors in the patches that will be merged in m-c (and added in the whiteboard comment about which attachments should to land on m-c)

Comment 77

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/cc5fbcbece94
https://hg.mozilla.org/integration/fx-team/rev/5efb021d57d9

Comment 78

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/cc5fbcbece94
https://hg.mozilla.org/mozilla-central/rev/5efb021d57d9

Comment 79

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8721494 [details] [diff] [review]
Bug_1227462___Backport_chrome_tabs_update_create_APIs_DISALLOW_INHERIT_PRINCIPAL_to_Beta__r_kmag.patch

I am sorry but this is too late. This is RC week and we only take critical fixes.

Comment 80

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8721493 [details] [diff] [review]
Bug_1227462___Backport_chrome_tabs_update_create_APIs_DISALLOW_INHERIT_PRINCIPAL_to_Aurora__r_kmag.patch

Fix for regression from 42. May risk regressions in web extensions. We are a few days away from 46 moving to beta, so we should keep an eye on this in testing.

Comment 81

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/09ca754e5fb7

Comment 82

[Will Bamberg [:wbamberg]]

docs:
https://developer.mozilla.org/en-US/Add-ons/WebExtensions/Chrome_incompatibilities#tabs
https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/tabs/create
https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/tabs/update

does this cover it, Luca?

Comment 83

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

(In reply to Will Bamberg [:wbamberg] from comment #82)
> docs:
> https://developer.mozilla.org/en-US/Add-ons/WebExtensions/
> Chrome_incompatibilities#tabs
> https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/tabs/create
> https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/tabs/update
> 
> does this cover it, Luca?

Yes, I think that the changes in the docs cover this Chrome incompatibility, Thanks Will!

If you think that it could be an useful addition: starting from Firefox 47 the chrome.runtime.lastError is set when an privileged URL is used, with `URL not allowed: ${url}` as the error message,  and, if the addon doesn't check it in the callback, then it will be logged to the console (which are currently visible in the Browser Console).

Comment 84

[Will Bamberg [:wbamberg]]

This is a good call! I've not described the exact error message, as I'd worry that might be too volatile, but I've noted this in:
https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/tabs/create
https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/tabs/update
...and also beefed up the lastError docs: https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/Runtime/lastError to be more helpful.

Comment 85

[Vasilica Tamas, QA [:vtamas]]

I was able to reproduce the initial issue mentioned in Description on Firefox 44.0a1 (2015-09-22) under Windows 10 64-bit.

Manually tested using the uxss.xpi on Firefox 48.0a1 (2016-03-29), Firefox 47.0a2 (2016-03-29) and Firefox 46 beta 6 under Windows 10 64-bit, Ubuntu 12.04 32-bit and Mac OS X 10.11 and noticed the following:
- The alert dialog is no longer displayed across all Firefox versions
- A browser console error is thrown on Firefox 47.0a2 and Firefox 48.0a1 while clicking on the webextension’s icon:     
Unchecked lastError value: Error: URL not allowed: javascript:alert(document.location);   ExtensionUtils.jsm:240


Luca,any thoughts about this error? Could you please confirm that this is the intended behaviour?

Comment 86

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

(In reply to Vasilica Mihasca, QA [:vasilica_mihasca] from comment #85)
> I was able to reproduce the initial issue mentioned in Description on
> Firefox 44.0a1 (2015-09-22) under Windows 10 64-bit.
> 
> Manually tested using the uxss.xpi on Firefox 48.0a1 (2016-03-29), Firefox
> 47.0a2 (2016-03-29) and Firefox 46 beta 6 under Windows 10 64-bit, Ubuntu
> 12.04 32-bit and Mac OS X 10.11 and noticed the following:
> - The alert dialog is no longer displayed across all Firefox versions
> - A browser console error is thrown on Firefox 47.0a2 and Firefox 48.0a1
> while clicking on the webextension’s icon:     
> Unchecked lastError value: Error: URL not allowed:
> javascript:alert(document.location);   ExtensionUtils.jsm:240
>  
> Luca,any thoughts about this error? Could you please confirm that this is
> the intended behaviour?

Sure, intended behaviour confirmed.

This behavior is now documented in the Chrome incompatibilities:

- https://developer.mozilla.org/en-US/Add-ons/WebExtensions/Chrome_incompatibilities#tabs

In Chromium-based browsers some of these URLs are currently allowed, and so, starting from Firefox 47 where runtime.lastError has been introduced, an Error message is logged in the browser console if not explicitly checked in the addon source code, as now documented on MDN:

-  https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/Runtime/lastError

Comment 87

[Vasilica Tamas, QA [:vtamas]]

Thanks Luca!

Based on Comment 85 and Comment 86, I am marking this bug as Verified.

Comment 89

[joony kohe]

Best information unfortunatelY I had to update this patch (the one which is going to land on m-c), because the previous version will not pass one of newly added eslint The best Bugzilla Mozilla Browser best information blogs

Hi Marie and many thanks for your great interview of Rick Hanson regarding inner strength. Will certainly be watching the interview again. You did a wonderful job I
<a href="https://www.sattamatka.company/">satta matka, indian matka, boss matka</a>

Comment 90

[joony kohe]

Best information unfortunatelY I had to update this patch (the one which is going to land on m-c), because the previous version will not pass one of newly added eslint The best Bugzilla Mozilla Browser best information blogs

Hi Marie and many thanks for your great interview of Rick Hanson regarding inner strength. Will certainly be watching the interview again. You did a wonderful job I https://www.sattamatka.company/

Comment 91

[BugBot [:suhaib / :marco/ :calixte]]

Bugbug thinks this bug is a regression, but please revert this change in case of error.