User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:83.0) Gecko/20100101 Firefox/83.0

Steps to reproduce:

The whole tabs API can be used without any permission except tabs.executeScript() and tabs.insertCSS() which require a host permission. The only thing the "tabs" permission does is granting access to the Tab.url, Tab.title, and Tab.favIconUrl properties. This is a good design in my opinion. However extensions with the appropriate host permission should also be able to access this data, since they can workaround this limitation via content scripts and the message API any way.

My use case is a simple blocklist which disables the add-on/content script on specific websites. Since there is no pure way to detect URL changes made by history push or replace without monkey patching I'm using the tabs.onUpdated events. In order to listen to URL changes where the content script is running I would need the "tabs" permission, which seems odd. For extensions with specific host permissions the "tabs" permission also reveals more data then they actually need (the tab data of all tabs and not only for tabs matching their host permissions).
The only way around this is listening to the tab "status" property, send a message to the content script and check the url. This however leads to a lot of unnecessary checks/messages since "status" often changes without a tab URL change.

I guess this sounds a bit nit picky and at the end I could just ask for the tabs permission. But since it is another permission which the user needs to accept I try to avoid it, especially when the extension theoretically already has access to everything it needs.

This can be fixed by adding a check for host permissions at https://searchfox.org/mozilla-central/rev/168c45a7acc44e9904cfd4eebcb9eb080e05699c/toolkit/components/extensions/parent/ext-tabs-base.js#172

Should be relatively easy to do - are you interested in contributing a patch? I'll guide you through its development if you are interested.
The guide to getting started is at https://wiki.mozilla.org/WebExtensions/Contribution_Onramp .

Sure, I would like to give it a try.

To give a little update on my side.
So far I successfully built Firefox using Artifact builds.

To get started I tried replacing the getter function you showed me like this

get hasTabPermission() { return true; }

I expected that now every extension would get access to restricted tab properties regardless if it has the "tabs" permission or not.
But using the code below in a webextension generates outputs without the url, title etc. properties.

browser.tabs.onUpdated.addListener((tabId, changeInfo, tab) => {
  console.log(changeInfo);
});

So either my understanding of the function is wrong or I'm building Firefox incorrectly.

My quick guess: it's because https://searchfox.org/mozilla-central/rev/168c45a7acc44e9904cfd4eebcb9eb080e05699c/browser/components/extensions/parent/ext-tabs.js
uses extension.hasPermission("tabs"); to check if the user is allowed to retrieve the properties.

Right. There are multiple places where a "tabs" check is relevant.

• Exposing via tabs.Tab type from various APIs (e.g. result from tabs.get)
  • This is what I referred to in the link from comment 1, i.e. https://searchfox.org/mozilla-central/rev/168c45a7acc44e9904cfd4eebcb9eb080e05699c/toolkit/components/extensions/parent/ext-tabs-base.js#172
• changeInfo in tabs.onUpdated event.
  • https://searchfox.org/mozilla-central/rev/168c45a7acc44e9904cfd4eebcb9eb080e05699c/browser/components/extensions/parent/ext-tabs.js#222
  • Code for Android: https://searchfox.org/mozilla-central/rev/168c45a7acc44e9904cfd4eebcb9eb080e05699c/mobile/android/components/extensions/ext-tabs.js#238
• Adding a URL filter to the tabs.onUpdated event.
  • https://searchfox.org/mozilla-central/rev/168c45a7acc44e9904cfd4eebcb9eb080e05699c/browser/components/extensions/parent/ext-tabs.js#429-430
• Using tabs.query with a URL parameter.
  • https://searchfox.org/mozilla-central/rev/168c45a7acc44e9904cfd4eebcb9eb080e05699c/browser/components/extensions/parent/ext-tabs.js#955
  • Code for Android: https://searchfox.org/mozilla-central/rev/168c45a7acc44e9904cfd4eebcb9eb080e05699c/mobile/android/components/extensions/ext-tabs.js#458
  • Note that this only checks url and title, not faviconUrl because the latter is not supported by tabs.query.

To unlock the requested functionality in all places where it makes sense, you need to add checks to the above places.

I'm nearly done with the code changes, but I struggle with the tabs.query function.
https://searchfox.org/mozilla-central/rev/168c45a7acc44e9904cfd4eebcb9eb080e05699c/browser/components/extensions/parent/ext-tabs.js#955
Should only "url" queries be allowed or also "title" queries which "url" matches the host permission?
I think the latter is the right way, since with enough effort one can query by "title" via content scripts. However in this case I would need to drop the
The "tabs" permission is required to use the query API with the "url" or "title" parameters error message, since every tab needs to be checked if it matches the host permission, but negative matches shouldn't throw an error then.

You can remove the upfront check, and instead check the permissions whenever a tab is matched. This does mean that there won't be an error.
It also offers a way for extensions to feature-detect whether the proposed functionality is available.

Side note, for this kind of change it may be useful to write unit tests together or even before the implementation. There are several edge cases that you'll probably encounter and fix along the way, and having unit tests for them makes it easier to have a correct implementation. You can find existing browser-chrome tests for the desktop implementation of the tabs API at https://searchfox.org/mozilla-central/source/browser/components/extensions/test/browser (browser_ext_tabs_ prefix by convention, registered in browser.ini).

Mobile tests are at https://searchfox.org/mozilla-central/source/mobile/android/components/extensions/test/mochitest (these tests are registered in the mochitest.ini file). The duplication of tests is historical and we'd like to consolidate that at some point (bug 1381237).

Since your tests do not really need any platform-specific APIs, I suggest to write a mochitest, because it will automatically run on desktop and mobile. Code shared between all platforms goes in toolkit/, and the location for tests is at https://searchfox.org/mozilla-central/source/toolkit/components/extensions/test/mochitest
There are already some tests with the test_ext_tabs_ prefix (registered in mochitest-common.ini), you can create a new test file with a similar convention.

• Extensions with the <all_urls> host permission don't get access to restricted tab properties for restricted urls like about:.
  The tabs permission does grant this access. Probably the correct behaviour?
• https://searchfox.org/mozilla-central/rev/168c45a7acc44e9904cfd4eebcb9eb080e05699c/browser/components/extensions/parent/ext-tabs.js#258
  This also checks for the activeTab permission, but I would assume it shouldn't check this even if it probably doesn't hurt?
  I decided to use extension.hasPermission("tabs") || tab.matchesHostPermission; in the rest of the code.
• https://searchfox.org/mozilla-central/rev/168c45a7acc44e9904cfd4eebcb9eb080e05699c/browser/components/extensions/parent/ext-tabs.js#429-430 I had to remove this error message as well.
• I wasn't able to cover the favIconUrl property in my tests because it's only available after the "load" event of the tab/document has fired.

(In reply to robbendebiene from comment #9)

• Extensions with the <all_urls> host permission don't get access to restricted tab properties for restricted urls like about:.
  The tabs permission does grant this access. Probably the correct behaviour?

Yes. Being able to see the URL is fine, being able to run code in them is not.

• https://searchfox.org/mozilla-central/rev/168c45a7acc44e9904cfd4eebcb9eb080e05699c/browser/components/extensions/parent/ext-tabs.js#258
  This also checks for the activeTab permission, but I would assume it shouldn't check this even if it probably doesn't hurt?

The checks are correct and not redundant with the current structure.

• If the extension doesn't have activeTab or tabs, then the filter creation is rejected at https://searchfox.org/mozilla-central/rev/168c45a7acc44e9904cfd4eebcb9eb080e05699c/browser/components/extensions/parent/ext-tabs.js#429-430
• Otherwise the logic at https://searchfox.org/mozilla-central/rev/de782976bf97669f1e8edee59e7a2398154efe06/browser/components/extensions/parent/ext-tabs.js#257-258 applies:
• If the extension has "tabs", then the filter is always accepted.
• If the extension has "activeTab", and the activeTab bit is granted for the tab, then the filter is applied.
• If the extension has "activeTab", but the activeTab bit is not granted for the tab, then the filter is not checked.

Note that if there is no filter, that the onUpdated event may still be fired for tabs. The tab's URL is just not visible, so extensions without the right permissions won't learn anything potentially sensitive about the tabs.

I decided to use extension.hasPermission("tabs") || tab.matchesHostPermission; in the rest of the code.

This is incorrect. An extension without host permissions or tabs permission, but with activeTab is still allowed to see tab.url for the active tab. If there aren't any failures in existing tests, then we need more test coverage. mochitests can use extension.grantActiveTab(tabId) (where tabId is from an extension) to simulate the addition. There is no revokeActiveTab counterpart, so in the test you could start with triggering the onUpdated events by navigating in a tab (and checking that filtering by URL doesn't work, that filters without URL do see the event but without tab.url) and then repeat the test after calling extension.grantActiveTab.

• https://searchfox.org/mozilla-central/rev/168c45a7acc44e9904cfd4eebcb9eb080e05699c/browser/components/extensions/parent/ext-tabs.js#429-430 I had to remove this error message as well.

Looks good.

• I wasn't able to cover the favIconUrl property in my tests because it's only available after the "load" event of the tab/document has fired.

Which case were you trying to test? This test checks the opposite (https://searchfox.org/mozilla-central/rev/de782976bf97669f1e8edee59e7a2398154efe06/browser/components/extensions/test/browser/browser_ext_tabs_onUpdated.js#233-236), and a similar test should be possible with host permissions. You can extend this existing test file. That's a better place than toolkit, because the mobile implementation has significantly different favicon logic.

(In reply to Rob Wu [:robwu] from comment #10)

(In reply to robbendebiene from comment #9)

• Extensions with the <all_urls> host permission don't get access to restricted tab properties for restricted urls like about:.
  The tabs permission does grant this access. Probably the correct behaviour?

Yes. Being able to see the URL is fine, being able to run code in them is not.

What I meant was if the <all_urls> host permission should also allow viewing the url, title and favicon properties of restricted urls. Currently it doesn't, which aligns with the content scripts behaviour and therefore is probably the right way to go. However one may expect it to match every possible url (including restricted).

The checks are correct and not redundant with the current structure.

All right. I'll change all my lines with extension.hasPermission("tabs") || tab.matchesHostPermission; to tab.hasTabPermission and add additional tests for the activeTab permission.

If the extension has "activeTab", but the activeTab bit is not granted for the tab, then the filter is not checked.

Is this really the case? Currently the filter is checked if it contains any urls, if no permission is granted it returns false, which therefore doesn't fire an update event. https://searchfox.org/mozilla-central/rev/de782976bf97669f1e8edee59e7a2398154efe06/browser/components/extensions/parent/ext-tabs.js#257-258
Shouldn't the code look something like this?

if (filter.urls && tab.hasTabPermission) {
   return filter.urls.matches(tab.uri);
}

(In reply to robbendebiene from comment #11)

(In reply to Rob Wu [:robwu] from comment #10)

(In reply to robbendebiene from comment #9)

• Extensions with the <all_urls> host permission don't get access to restricted tab properties for restricted urls like about:.
  The tabs permission does grant this access. Probably the correct behaviour?

Yes. Being able to see the URL is fine, being able to run code in them is not.

What I meant was if the <all_urls> host permission should also allow viewing the url, title and favicon properties of restricted urls. Currently it doesn't, which aligns with the content scripts behaviour and therefore is probably the right way to go. However one may expect it to match every possible url (including restricted).

Let's not include restricted URLs. The "tabs" permission can be used for that.

If the extension has "activeTab", but the activeTab bit is not granted for the tab, then the filter is not checked.

Is this really the case?

Yes. The hasTabPermission evaluates to false when a tab is not viewed as an "active tab" for the "activeTab" permission. When that is the case, the filter is not checked and the event is not triggered. This prevents extensions from being able to filter tab events by URLs without the right permissions.

Shouldn't the code look something like this?

if (filter.urls && tab.hasTabPermission) {
   return filter.urls.matches(tab.uri);
}

No, then the tabs.onUpdated event would fire for tabs that don't match the given urls filter. The tabs wouldn't have a url (and title, etc.) property either. These are the potential outcomes for the decisions related to the checks:

• Is url filter given? -> no = show all events (note: no further permission checks at all)
• url filter given, does the extension have the permission for the tab? -> no = hide event
• url filter given, does the tab match the filter? -> no = hide event
• url filter given, tab matches filter -> show event

All right. I really appreciate the detailed feedback.
Currently the only things left to do are:

If there aren't any failures in existing tests, then we need more test coverage. mochitests can use extension.grantActiveTab(tabId) (where tabId is from an extension) to simulate the addition. There is no revokeActiveTab counterpart, so in the test you could start with triggering the onUpdated events by navigating in a tab (and checking that filtering by URL doesn't work, that filters without URL do see the event but without tab.url) and then repeat the test after calling extension.grantActiveTab.

How do I pass the tabId from the background script to the test environment? It looks like extension.awaitMessage("active-tab-id"); cannot get any data. Is there something like a listener I can attach?

Which case were you trying to test? This test checks the opposite (https://searchfox.org/mozilla-central/rev/de782976bf97669f1e8edee59e7a2398154efe06/browser/components/extensions/test/browser/browser_ext_tabs_onUpdated.js#233-236), and a similar test should be possible with host permissions. You can extend this existing test file. That's a better place than toolkit, because the mobile implementation has significantly different favicon logic.

The problem is that changeInfo.status == "complete" is fired before the changeInfo.favIconUrl. So I would leave the test before I was able to check for the favicon property. If I wait for the favicon property then the test may get stuck if it fails. See also: https://phabricator.services.mozilla.com/D98471#C3383165NL257

(In reply to robbendebiene from comment #13)

How do I pass the tabId from the background script to the test environment? It looks like extension.awaitMessage("active-tab-id"); cannot get any data. Is there something like a listener I can attach?

browser.test.sendMessage(msg, arg) inside the test extension can be used as
arg = await extension.awaitMessage(msg) from the test runner.

Which case were you trying to test? This test checks the opposite (https://searchfox.org/mozilla-central/rev/de782976bf97669f1e8edee59e7a2398154efe06/browser/components/extensions/test/browser/browser_ext_tabs_onUpdated.js#233-236), and a similar test should be possible with host permissions. You can extend this existing test file. That's a better place than toolkit, because the mobile implementation has significantly different favicon logic.

The problem is that changeInfo.status == "complete" is fired before the changeInfo.favIconUrl. So I would leave the test before I was able to check for the favicon property. If I wait for the favicon property then the test may get stuck if it fails. See also: https://phabricator.services.mozilla.com/D98471#C3383165NL257

In tests where there is an expected faviconUrl, you could order the events and tab creation, starting by the tab without expected changeInfo.favIconUrl, followed by opening a tab with an expected favIconUrl, and then wait for an onUpdated event with changeInfo.faviconUrl set (for the second tab). When that happens, check the assertions and unregister the events. If the favIconUrl update was unexpectedly triggered for tab1, then the test should fail.

When using the extension.grantActiveTab function I'm getting the following error TypeError: can't access property "addActiveTabPermission", tabManager is undefined. Do you have any ideas?

When useAddonManager is used with loadExtension, a MockExtension is constructed to wrap the real Extension instance. That does currently not have a tabManager getter, but it will be added in this patch: https://phabricator.services.mozilla.com/differential/changeset/?ref=3376406

You can copy that part to your patch to get it to work. Once either your or Agi's patch land, the other has to remove the snippet from their patch before landing it.

Side note: useAddonManager should ideally not be set, but it's a work-around to get Android tests to work. This work-around will be unnecessary once bug 1641735 is fixed.

I'm stuck on the "activeTab" permission test. While your proposed fix resolves the error message my test still fails (it seems like the permission isn't granted)
My test is probably wrong (see attachment), but I can't make out the problem. I've extended the test_ext_tabs_permissions.html tests to work for the activeTab permission as well. But I'm not able to query the active tab via url nor can I read its restricted properties. Can you please have a look?

Could you update Phabricator rather than attaching attachments here? Then it's easier to put inline comments. Until the patch lands, the Phabricator revision is a work-in-progress, and it can be used to show where you are at and where you need help.

(In reply to robbendebiene from comment #17)

My test is probably wrong (see attachment), but I can't make out the problem. I've extended the test_ext_tabs_permissions.html tests to work for the activeTab permission as well. But I'm not able to query the active tab via url nor can I read its restricted properties. Can you please have a look?

One obvious problem is that the activeTab bit is cleared upon navigation. Your test navigates the tab, so it is not a surprise that the activeTab bit is lost. It is possible to trigger onUpdated without a navigation:

• Navigate to a new reference fragment (changing the # part of the URL) = triggers changeInfo.url change.
• Update document.title = triggers changeInfo.title change. This can be done via browser.tabs.executeScript from an extension with activeTab or host permissions.

Your current construction of the test does not make it easy to implement this, because you're creating one extension at a time. As a result, it wouldn't be possible to use the tabs.executeScript method for the extension without any permissions, for example.

A way to easily achieve the desired test coverage is to split the test execution in the following parts:

• Load multiple extensions that have test.onMessage listeners waiting for test expectations, but different permissions. It is possible to also have listeners to await the tabId to filter events later, but it could also be easier to read and write if you query existing tabs and filter them out upfront.
• Test runner: Load another extension that is responsible for opening, updating and closing tabs (in response to test.onMessage).
• Test runner: trigger open tab, collect test results, close tabs.

PS. to make debugging easier you can add dump(); in the implementation (it is like a console.log(), except immediate and without automatic trailing newline). Then you would see that the activeTab bit had really disappeared for some reason.

This nearly solved every problem I had. Thanks again, now I finally understand how the activeTab permission really works.
There is still one last thing that's not working (so maybe I still don't fully understand the activeTab permission after all :D).

For some reason only the "url" property is missing on the changeInfo for the activeTab permission test. (https://phabricator.services.mozilla.com/D98471#C3391745NL409)

In your current patch you're still using the test extension to open tabs. To ensure that all tests are behaviorally identical (except for the permissions), you need to open tabs and wait for updates in the helperExtension. You'll need to have one browser.test.onMessage listener that calls different functions depending on the message, and then browser.test.sendMessage to return control once the expected events have occurred.

To quote my previous comment:

(In reply to Rob Wu [:robwu] from comment #18)

split the test execution in the following parts:

• Load multiple extensions that have test.onMessage listeners waiting for test expectations, but different permissions. It is possible to also have listeners to await the tabId to filter events later, but it could also be easier to read and write if you query existing tabs and filter them out upfront.
• Test runner: Load another extension that is responsible for opening, updating and closing tabs (in response to test.onMessage).
• Test runner: trigger open tab, collect test results, close tabs.

The helperExtension and code outside of the helperExtension / extension is responsible for all logic in the last two lines. Only "collect test results" involves one message (sendMessage + awaitMessage) to the extension.

I've update the test. I hope this time it's the correct structure.
The test still fails, because the url property on the onUpdate changeInfo object is not available for the activeTab permission.

For some reason only the "url" property is missing on the changeInfo for the activeTab permission test. (https://phabricator.services.mozilla.com/D98471#C3391745NL409)

I've updated the code again and answered/added some questions.

I saw the code update but was awaiting review until comments were added since the update only partially addressed my previous review.

I don't see any questions - have you forgotten to hit the Submit button in Phabricator?

Oh no.. I didn't know I had to submit the comments. This also means all the comments from the previous revision never got submitted.

While there are still some programming points missing, it would be great if you could review the current state, especially the comments if you find some time.

I'll take a look.

I moved all tests into one single file. Additionally I've reworked the helperExtension to be more generic. Every test now makes use of the helperExtension, which reduced the amount of code of the actual testing extensions and made them easier to understand. I guess (and hope) that this comes close to what you had in mind.

I've tried to load the helperExtension only once (before any test has started) but always ran into errors. It's also not trivial for me to detect when I can properly unload it. Because of that I'm still recreating it in every test.

I didn't exclude the favicon test for Android yet since it might work there too (I would like to leave that decision to you).

The reason why I keep the onUpdate tests separated is, because I cannot change the favicon property without navigation (while its possible with JS, it never triggered the actual update event). Changing the title and url property by navigation would mean, that I cannot test the activeTab permission.

I'm pretty happy with the current result. From my point of view there is only one problem left. The following test still fails for tab.get() for the url property.

add_task(function has_restricted_properties_with_activeTab_permission () {
  return test_restricted_properties(["activeTab"], true);
});

https://hg.mozilla.org/mozilla-central/rev/a5b3fa068d5e
https://hg.mozilla.org/mozilla-central/rev/8218893f48c1

Thanks so much for the patch, robbendebiene! 🎉 Your contribution has been added to our recognition wiki at https://wiki.mozilla.org/Add-ons/Contribute/Recognition.

Hope to keep seeing you around the project! 😎

Documentation updated by https://github.com/mdn/content/pull/2057