Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support for Chrome's native site access UI #22

Closed
aspiers opened this issue Jan 20, 2024 · 21 comments
Closed

Support for Chrome's native site access UI #22

aspiers opened this issue Jan 20, 2024 · 21 comments
Labels
enhancement New feature or request help wanted Extra attention is needed

Comments

@aspiers
Copy link
Contributor

aspiers commented Jan 20, 2024

As mentioned in fregante/webext-permission-toggle#29 (comment), 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
Copy link
Owner

fregante commented Jan 20, 2024

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
Copy link
Owner

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
Copy link
Owner

fregante commented Jan 20, 2024

@aspiers
Copy link
Contributor Author

aspiers commented Jan 20, 2024

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
Copy link
Owner

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
Copy link
Owner

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
Copy link
Contributor Author

aspiers commented Jan 20, 2024

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:

aspiers/webext-permission-toggle@412c070

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
Copy link
Contributor Author

aspiers commented Jan 20, 2024

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:

  • Ship with a list of sites enabled out of the box which make the most sense for the extension.

  • Allow the user to additionally enable other sites.

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

  • (Ideally, but I guess this will come later) allow the user to disable the extension on any of the built-in sites.

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

  • Support native Chrome UI's as much as possible (the built-in context sub-menu we've discussed, plus native popup dialogs asking the user's permission to enable for a site). This is to reinforce the feeling that the user is being protected by the browser (e.g. as opposed to enabling the extension for all sites, and then having per-site controls implemented purely by the extension without using chrome.permissions at all, since that would require the user to somewhat blindly trust that the extension isn't touching a site when it claims it isn't).

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
Copy link
Owner

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 <all_urls> 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:

  • the user can add hosts via my toggle
  • the user can remove built-in sites via chrome's UI

Don't overcomplicate it 😃

@fregante
Copy link
Owner

fregante commented Jan 20, 2024

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

aspiers/webext-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
Copy link
Contributor Author

aspiers commented Jan 20, 2024

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:

  • When this is done on a site in the manifest, after the browser pops up a dialog requesting a page reload, the extension icon becomes embossed indicating that the extension is currently disabled on that site awaiting a click. So far so good. But now if we request site access via the extra context menu item added by webext-domain-permission-toggle and then click Deny, the extension becomes active!! That's a pretty awful security/privacy violation as it's going directly against the wishes of the user.
  • When this is done on a site not in the manifest, the extension icon does not become embossed indicating that the extension is currently disabled on that site awaiting a click. Furthermore, the middle option on the submenu is greyed out, but I can't think of any good reason why it should be.
  • When access is granted to https://*.example.com, switching to https://example.com or https://foo.example.com triggers the tab changed handler, but getTabUrl(tabId) fails to return anything useful.
  • If https://*.example.com and https://example.com are both included in the manifest, only the former is shown in the extension details page. But then if you enable it from there, both appear in the array returned from permissions.getAll(). Furthermore, if you enable https://example.com manually, that does appear as a new entry in the details page.

@fregante
Copy link
Owner

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

  • When access is granted to https://*.example.com, switching to example.com or foo.example.com triggers the tab changed handler, but getTabUrl(tabId) fails to return anything useful.

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

  • If https://*.example.com and example.com are both included in the manifest, only the former is shown in the extension details page

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
Copy link
Contributor Author

aspiers commented Jan 21, 2024

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

  • When access is granted to https://*.example.com, switching to example.com or foo.example.com triggers the tab changed handler, but getTabUrl(tabId) fails to return anything useful.

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.

  • If https://*.example.com and example.com are both included in the manifest, only the former is shown in the extension details page

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
Copy link
Owner

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
Copy link
Contributor Author

aspiers commented Jan 21, 2024

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 #23 (comment) 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
Copy link
Owner

fregante commented Jan 21, 2024

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
Copy link
Contributor Author

aspiers commented Jan 21, 2024

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:

  • Put a site in manifest.content_scripts.matches but not host_permissions
  • Switch the extension's site access to "On click" in Chrome's native UI
  • Make sure the site in question isn't enabled in the extension details page (sometimes there can be residual permissions from previous versions of the manifest)
  • Visit the site in question
  • Observe that the extension's action icon is embossed, indicating it's not enabled yet
  • Right-click on the icon
  • Left-click on the toggle menu item and wait for the popup asking the user permission to enable the site
  • Observe that even before the user clicks "Deny" or "Allow", activeTab has been triggered, the content script has been injected, and the extension is now active.

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
Copy link
Owner

fregante commented Feb 10, 2024

I checked, there's nothing to do here.

"Support" is actually pretty straightforward:

  • don't assume that manifest permissions are actually granted.

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.

@fregante fregante closed this as not planned Won't fix, can't repro, duplicate, stale Feb 10, 2024
@aspiers
Copy link
Contributor Author

aspiers commented Apr 14, 2024

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
Copy link
Owner

fregante commented Apr 14, 2024

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
Copy link
Owner

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

Repository owner locked and limited conversation to collaborators Apr 14, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

2 participants