fregante / webext-permissions

WebExtensions module: Get any optional permissions that users have granted you.
MIT License
15 stars 1 forks source link

Support for Chrome's native site access UI #22

Closed aspiers closed 8 months ago

aspiers commented 9 months ago

As mentioned in https://github.com/fregante/webext-domain-permission-toggle/issues/29#issuecomment-1897606529, this module needs support for Chrome's native UI for controlling site access, which looks like this when accessed from an extension's context menu:

image

and also this bit of the extension's details page:

image

fregante commented 9 months ago

Thank you for opening this.

What's your manifest.json? I don't have that dropdown in Refined GitHub (it also has *://*/* as the optional permission). Do you have *://*/* in host_permissions or optional_host_permissions?

Here "Twitter" was granted manually with this module:

Screenshot 2
fregante commented 9 months ago

Anyway I think the solution would be to not assume that we still have all the static permissions. It's kinda complicated probably because I just refactored the module around that assumption.

fregante commented 9 months ago

Also in https://github.com/fregante/webext-domain-permission-toggle/blob/56cbac98b545036a8686ccb592516bba73ef16ef/index.ts#L39

aspiers commented 9 months ago

Thank you for opening this.

What's your manifest.json?

  permissions: ['activeTab', 'contextMenus', 'scripting', 'sidePanel', 'storage'],
  optional_permissions: [],
  optional_host_permissions: ['*://*/*'],

I have a list of allowed sites in my content_scripts.matches, but am also experimenting both with and without <all_urls> in there.

I don't have that dropdown in Refined GitHub (it also has *://*/* as the optional permission). Do you have *://*/* in host_permissions or optional_host_permissions?

Here "Twitter" was granted manually with this module:

It only appears when you click on the main toggle at the top.

I think it would be useful if webext-permissions could detect when the extension is in the "enabled for all sites" mode and offer a function so that extensions and other libraries can detect this mode. In particular, I think webext-domain-permission-toggle should disable (i.e. grey out) the context menu item when it's in this mode - but only when <all_urls> is in the manifest. When it's not, I think Chrome interprets "for all sites" to mean "for all sites listed in the manifest", and then you still need to request additional permission for other sites. But of course you already know this, because this is the default use case which webext-domain-permission-toggle currently supports.

I'm compiling this spreadsheet to try to understand Chrome's behaviour better, and how webext-permissions currently handles the various combinations:

https://docs.google.com/spreadsheets/d/175IfrjTVqzCMTqklR55FyDtugdU_it1FGZyR4epwf5s/edit?usp=sharing

fregante commented 9 months ago

I have a list of allowed sites in my content_scripts.matches, but am also experimenting both with and without <all_urls> in there.

Note that any host specified in content_scripts will be considered a permission by the store, so you'll get the "access all sites" message. This module also reads from that:

Chrome does treat these permissions as "half-assed" so for me anything that is in matches must also be in host_permissions

It only appears when you click on the main toggle at the top.

That one is grayed out

detect when the extension is in the "enabled for all sites" mode

I still don't know how to repro that. I assume that option becomes available when you have the all_urls permission and therefore Chrome lets the user decide where to run the extension. But then again we're back to square one, you're asking "permission to read all sites" on install.

fregante commented 9 months ago

I'm compiling this spreadsheet

That's really good, thank you the research!

It seems that the "On all sites" on the extension context sub-menu is greyed out when <all_urls> isn't in the manifest.

So yes it seems to confirm my last paragraph.

aspiers commented 9 months ago

Note that any host specified in content_scripts will be considered a permission by the store, so you'll get the "access all sites" message.

You mean on install, or at another point? I don't recall seeing an "access all sites" message, but it's been a while since I paid attention while installing an extension from the Web Store.

This module also reads from that:

Chrome does treat these permissions as "half-assed" so for me anything that is in matches must also be in host_permissions

Ugh, another variable to consider. I only just noticed that I don't have host_permissions in my manifest, only optional_host_permissions. This doesn't seem to have caused issues so far though; I can still inject a content script. Perhaps it would be needed if I wanted to do any of the other things listed in https://developer.chrome.com/docs/extensions/develop/concepts/declare-permissions ?

It only appears when you click on the main toggle at the top.

That one is grayed out

Hrm, weird.

detect when the extension is in the "enabled for all sites" mode

I still don't know how to repro that. I assume that option becomes available when you have the all_urls permission and therefore Chrome lets the user decide where to run the extension.

I see this option even without <all_urls>. Currently on Chrome 119.0.6045.159 (Official Build) (64-bit), in case that makes any difference.

I had started to take a stab at writing code to detect this and handle it gracefully:

https://github.com/aspiers/webext-domain-permission-toggle/commit/412c07089ca95efdf4573adb892b72dc9c72ad91

but I don't think I got it working yet.

But then again we're back to square one, you're asking "permission to read all sites" on install.

Yeah exactly.

aspiers commented 9 months ago

I should probably explain what I am trying to achieve, because it may seem odd that I'm simultaneously experimenting with <all_urls> and trying to get per-site access under the user's control. My goals for my extension are:

Probably I'm aiming too high and need to simplify my design goals, but this is how I ended up down this particular rabbit hole. Any advice on the best way forward is welcome :)

fregante commented 9 months ago

You mean on install, or at another point?

Both. See your second screenshot on this page, it says: "Permissions. Read and change all your data on all websites"

I can still inject a content script

Yeah, Chrome doesn't force you to specify them, it already treats matches as permissions. However it fails some tests, for example permissions.contains() will fail )(I think) unless you specifically used request() on that host.

Allow the less security-conscious user to enable the extension across all sites if they prefer that. AFAICS, this can only be done if is in the manifest, but maybe I got that wrong.

You got this right but that permission must be exclusively in optional_host_permissions, not in matches. You can also use request() and pass all_urls to grant permission everywhere at once.

allow the user to disable the extension on any of the built-in sites.

They can already do that via chrome's UI.

Include a user-friendly page in the extension's settings which shows which sites it's enabled for, and allows easy control of that list.

Chrome already offers that.

Any advice on the best way forward is welcome :)

Just follow the advice in the readme for webext-dynamic-content-scripts. Your use case is the same as mine. The only requirement is that you have at least one pre-defined site.

Works:

Don't overcomplicate it 😃

fregante commented 9 months ago

I had started to take a stab at writing code to detect this and handle it gracefully:

aspiers/webext-domain-permission-toggle@412c070

That's only fine if the permission is granted via request(). If it's not optional, we have no API to remove it, so this toggle is completely useless/inert.

If it sees all_urls in the manifest, this module should throw an error.

aspiers commented 9 months ago

Makes sense.

BTW, I did a bunch more experimenting and it looks like this is going to be horrible to support. I've seen so much confusing behaviour from Chrome which I can't really explain. For example, behaviour following on from when you select "When you click the extension" from the submenu:

fregante commented 9 months ago

so much confusing behaviour from Chrome which I can't really explain

Welcome to Web Extensions development 😂 it's basically a continuous research.

  • the extension becomes active!!

I think you're talking about activeTab, see: https://github.com/fregante/webext-domain-permission-toggle/blob/ba697453ee5de2b8889aa2c76a2798d90addca16/index.ts#L42-L43

In short, if the user clicks the action, the extension receives temporary access until the origin is changed. This permission is not part of chrome.permissions.contains(), but executeScript will (temporarily) work.

Off topic, but I also have logic to optionally support this use case: https://github.com/fregante/webext-dynamic-content-scripts#activetab-tracking

That's concerning. Either way it's something to discuss in:

The code is: https://github.com/fregante/webext-tools/blob/386d385d8a0449bf32a466197f9a658a37a40991/index.ts#L18-L35

I call them overlapping permissions:

In short if you have https://github.com/*' and later grant *://*/*, the former is gone. You can't see it, you can't remove it. You can only remove *://*/* now:

  • both appear in the array returned from permissions.getAll

That's not an issue because of what I just said

aspiers commented 9 months ago

so much confusing behaviour from Chrome which I can't really explain

Welcome to Web Extensions development 😂 it's basically a continuous research.

😱

  • the extension becomes active!!

I think you're talking about activeTab, see: fregante/webext-domain-permission-toggle@ba69745/index.ts#L42-L43

Thanks for the hint, I hadn't properly looked into activeTab before. This raises some interesting questions.

In short, if the user clicks the action

If by this you mean the extension action icon, this was not quite the scenario I was describing. Here I'm specifically talking about the scenario where the first interaction they have with the extension on a site in the manifest is to right-click the action icon, and then click the context menu item.

However, I note that https://developer.chrome.com/docs/extensions/develop/concepts/activeTab says:

The following user gestures enable the "activeTab" permission:

Executing an action Executing a context menu item ...

At first sight, this could explain why clicking on the toggle menu item would be enough to activate the extension, even before the user clicks "Allow" on the native permissions popup. However, my extension is using the normal import rather than including-active-tab.js, so I'm still struggling to understand how the content script could have been injected in this scenario. Any ideas?

The natural follow-on question is whether in our extensions' use cases it makes sense to include activeTab in the manifest. I only put it in there on the suggestion of https://github.com/fregante/webext-domain-permission-toggle/#manifestjson-v3, but what is it actually needed for when using the combination of webext-domain-permission-toggle and webext-dynamic-content-scripts? Will anything break if I remove it?

Off topic, but I also have logic to optionally support this use case: fregante/webext-dynamic-content-scripts#activetab-tracking

That's concerning. Either way it's something to discuss in:

But this is happening even when the extension is set to "On all sites"; it's not specific to when site access is restricted via native UI. Maybe I need to file a separate issue for that.

The code is: fregante/webext-tools@386d385/index.ts#L18-L35

Yeah I already took a quick look at that. From memory the URL just wasn't there but I'll double-check.

I call them overlapping permissions:

In short if you have https://github.com/*' and later grant *://*/*, the former is gone. You can't see it, you can't remove it.

My point was that it looks like it's gone forever, but as I said:

if you enable it from there, both appear in the array returned from permissions.getAll()

so it seems that the narrower permission isn't truly gone forever, it's just hidden and can reappear in getAll().

You can only remove *://*/* now:

Ahah! I had stumbled across this problem myself but didn't realise you already had an issue for it.

  • both appear in the array returned from permissions.getAll

That's not an issue because of what I just said

Yeah, dropOverlappingPermissions() should take care of that in practice.

But anyway, I've totally exhausted my energy for worrying about Chrome's native UI right now, and totally agree with you that it makes sense to park this and focus on getting the other stuff fully working first. As I said, the activeTab topic is not limited to the scope of this issue, so I guess that discussion should be moved elsewhere.

fregante commented 9 months ago

Will anything break if I remove it?

I think you can't access the URL unless you have permission to that tab, either via activeTab, via tabs or other host permissions. Since webext-dynamic-content-scripts exists specifically to avoid tabs and broad host permissions, activeTab is exactly what we want.

I'm still struggling to understand how the content script could have been injected in this scenario. Any ideas?

Do you still have all_urls in content_scripts?

focus on getting the other stuff fully working first

Heh yeah, good idea. I'd start by matching the suggested manifest without further permissions in matches

https://github.com/fregante/webext-dynamic-content-scripts/blob/main/how-to-add-github-enterprise-support-to-web-extensions.md

aspiers commented 9 months ago

Will anything break if I remove it?

I think you can't access the URL unless you have permission to that tab, either via activeTab, via tabs or other host permissions.

But if it's covered by specific host permissions, which as you said is the main point of webext-dynamic-content-scripts, why do we need activeTab as well?

I'm still struggling to understand how the content script could have been injected in this scenario. Any ideas?

Do you still have all_urls in content_scripts?

No.

focus on getting the other stuff fully working first

Heh yeah, good idea. I'd start by matching the suggested manifest without further permissions in matches

fregante/webext-dynamic-content-scripts@main/how-to-add-github-enterprise-support-to-web-extensions.md

I just had a big realisation in https://github.com/fregante/webext-permissions/issues/23#issuecomment-1902662517 which has solved a bunch of problems in one go. I think pretty much everything is working great now except perhaps for the native site access UI "on click", and I don't have the energy to revisit that right now - need to get back to actually hacking on my extension!!

fregante commented 9 months ago

why do we need activeTab as well?

For the toggle. If we don't know what site we're on, how can we ask the permission to it?

If you avoid the toggle and just let the user input the host, you don't need activeTab, but then you'd lose the two-click ease of use.

aspiers commented 9 months ago

Ah, I see! But anyway, I think I can confirm that the behaviour I saw before of the content script being injected even before clicking "Allow" in the popup was indeed due to activeTab. Here's how I reproduce:

This behaviour is surprising and very counter-intuitive, but it does match what is documented at https://developer.chrome.com/docs/extensions/develop/concepts/activeTab, if you interpret the bullet point "Executing a context menu item" to mean "Clicking on a context menu item" rather than "Clicking on a context menu item and waiting for its event handler to complete".

fregante commented 8 months ago

I checked, there's nothing to do here.

"Support" is actually pretty straightforward:

Since this package at the moment exclusively deals with "additional" permissions, the output of the current methods is unchanged. Just because the user removes a permission, that doesn't change the list of additional permissions.

The only method that is "affected" is isUrlPermittedByManifest, but just because the users might "assume that manifest permissions are actually granted." So again nothing to change here.

The only suggestion is to use chrome.permissions.contains() instead of isUrlPermittedByManifest, unless you really want to just check the manifest.

aspiers commented 6 months ago

why do we need activeTab as well?

For the toggle. If we don't know what site we're on, how can we ask the permission to it?

@fregante I'm really confused because I've just found that after removing activeTab from my manifest, rebuilding, and reloading my extension, AFAICS the toggle functionality still works perfectly. I've done my best to break it or find some deficiency but so far failed. Please could you point me in the direction of what exactly needs this permission?

BTW, my extension is finally public and Open Source, in case you're curious: https://github.com/aspiers/rolod0x/

fregante commented 6 months ago

Do you have the tabs permission or have already granted extensive host permissions?

Just open the webext-permission-toggle demo extension from its repo and try there without the activeTab permission. I don’t want to debug your usage like last time.

Without tab or host permissions, the extension should not be able to know the URL of the current tab, so how can it ask the browser to grant permission to it?

fregante commented 6 months ago

Anyway, this issue is done, activeTab is unrelated to this package.