Closed Bug 1679688 Opened 4 years ago Closed 4 years ago

host permissions or <all_urls> permission should grant access to privileged parts of the tabs API

Categories

(WebExtensions :: Untriaged, enhancement, P3)

Product:
WebExtensions
Component:
Untriaged
Version:
Firefox 83
Type:
enhancement

Tracking

(firefox86 fixed)

Status:
RESOLVED FIXED
Milestone:
86 Branch
Tracking Flags:
Tracking Status
firefox86 --- fixed

People

(Reporter: robbendebiene, Assigned: robbendebiene, Mentored)

References

(Blocks 1 open bug)

Details

(Keywords: dev-doc-complete, good-first-bug)

Attachments

(3 files)

Bug 1679688 - make host permissions grant access to privileged parts of the tabs API and fix Bug 1686443 r=robwu
47 bytes, text/x-phabricator-request
Details | Review
test_ext_tabs_permissions.html
10.67 KB, text/html
Details
Bug 1679688 - Fix a mozlint warning (File does not end with newline character)
48 bytes, text/x-phabricator-request
Details | Review

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.

Severity: -- → N/A
Priority: -- → P3

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 .

Mentor: rob
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: good-first-bug

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.

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.

Assignee: nobody → robbendebiene
Status: NEW → ASSIGNED

(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.

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

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.

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);
});
Attachment #9190818 - Attachment description: Bug 1679688 - make host permissions grant access to privileged parts of the tabs API r=robwu → Bug 1679688 - make host permissions grant access to privileged parts of the tabs API and fix Bug 1686443 r=robwu
See Also: → 1686080
Blocks: 1688388
Pushed by nerli@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/a5b3fa068d5e make host permissions grant access to privileged parts of the tabs API and fix Bug 1686443 r=robwu,geckoview-reviewers,agi
Keywords: dev-doc-needed
Pushed by sledru@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/8218893f48c1 Fix a mozlint warning (File does not end with newline character) DONTBUILD

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

Status: ASSIGNED → RESOLVED
Closed: 4 years ago
status-firefox86: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → 86 Branch
Regressions: 1688440
Blocks: 1688476

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! 😎

Blocks: 1690611
Blocks: 1690613
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: