strictOrigins catches narrower permissions as well as broader

[aspiers]

The documentation for the strictOrigins option says:

If the manifest contains the permission https://github.com/* and then you request *://*.github.com/* (#1), the latter will be considered an additional permission because technically it's broader.

However, with the way it's implemented, if the manifest contains https://*.example.com/*, and the current permissions includes https://subdomain.example.com/*, the latter will be returned as an additional permission even though it's technically narrower than what's in the manifest and not widening the set of permissions in any way.

This can occur when subdomains are explicitly added to the set of current permissions via one of Chrome's native UIs for site access (e.g. the second item in the extension's context submenu for site access).

[fregante]

Can you add some failing tests in a PR? This helps see and later fix the issue

[aspiers]

Sure, I've submitted #24 as preparation for the failing tests.

[aspiers]

OK, here's a failing test:

aspiers@subdomain-narrowing

It's on top of #24 which is a prerequisite you should be able to safely merge right away.

[fregante]

You can push it all to the same PR, making the tests fail. I'll use it as a base to fix it (or you can do it)

However the premise seems wrong:

However, if https://subdomain.hassubdomains.com/* is granted by the user as an optional permission on top of https://*.hassubdomains.com/*,
this does not change the effective set of permissions.

This is unrelated to this module, I already explained it, they're overlapping permissions. Granting subdomain. changes nothing in the extension.

If the native getAll() reports both, that's a browser bug.

[aspiers]

You can push it all to the same PR, making the tests fail. I'll use it as a base to fix it (or you can do it)

Done, but my hope was that you would merge the first commit immediately because it should be totally non-controversial and just augmenting the existing test suite with extra cases.

However the premise seems wrong:

However, if https://subdomain.hassubdomains.com/* is granted by the user as an optional permission on top of https://*.hassubdomains.com/*,
this does not change the effective set of permissions.

This is unrelated to this module, I already explained it, they're overlapping permissions. Granting subdomain. changes nothing in the extension.

I know, which is why I would not expect queryAdditionalPermissions() to claim that the subdomain is an extra permission, and the readme also heavily implies that it won't. But it does, which is the issue I'm describing.

If the native getAll() reports both, that's a browser bug.

It definitely does report both. Yes it could be argued it's a browser bug, but whether it is or isn't, we still need clarity on how queryAdditionalPermissions() should work. Currently the documented behaviour of this function is not entirely consistent with the code, so either the code should be changed or the docs. I don't particularly mind which.

One idea I had was to introduce change strictOrigins to boolean | 'onlyWider', so that the existing behaviour wouldn't change, but when set to onlyWider, when narrower permissions already covered by wider permissions are granted, they wouldn't be included in the return value. But I don't have an opinion on whether that's better than another code change or a doc change.

[fregante]

Done, but my hope was that you would merge the first commit immediately because it should be totally non-controversial and just augmenting the existing test suite with extra cases.

Good point, done. CI should run automatically on this repo when you open PRs now.

---

I took a look at the failed CI, I see both failing:

extractAdditionalPermissions after added permissions
extractAdditionalPermissions after added permissions, loose origin check

So this is not an issue with strictOrigin

It definitely does report both

Can you provide a repro? .request() calls just resolve with true in my case. Repro:

1. Run the demo extension in the toggle repo (has *.github.com in the manifest)
2. Run this code in the background:

   chrome.action.onClicked.addListener(async () => {
       console.log('approved?', await chrome.permissions.request({origins: [`https://${String(Math.random()).slice(2, 6)}.github.com/*` ]}))
   })

3. left-click the browser action (the icon)
4. Run await chrome.permissions.getAll()

[aspiers]

Trying this now. First interesting discovery - with this demo extension I don't see the same dropdown which I do with my own extension, and which you said you hadn't seen with yours:

I wonder what it is about my extension which is causing that.

[aspiers]

Done, but my hope was that you would merge the first commit immediately because it should be totally non-controversial and just augmenting the existing test suite with extra cases.

Good point, done. CI should run automatically on this repo when you open PRs now.

Thanks!

I took a look at the failed CI, I see both failing:

extractAdditionalPermissions after added permissions
extractAdditionalPermissions after added permissions, loose origin check

So this is not an issue with strictOrigin

Hmm, good point.

It definitely does report both

Can you provide a repro? .request() calls just resolve with true in my case. Repro:

1. Run the demo extension in the toggle repo (has *.github.com in the manifest)
2. Run this code in the background:

   chrome.action.onClicked.addListener(async () => {
       console.log('approved?', await chrome.permissions.request({origins: [`https://${String(Math.random()).slice(2, 6)}.github.com/*` ]}))
   })

3. left-click the browser action (the icon)
4. Run await chrome.permissions.getAll()

I also see no change on this demo. Weird. I'll try testing my extension from the background console in a similar manner.

[aspiers]

OK, this has led to a big break-through in my understanding! At some point during testing, I had moved my default list of allowed sites from manifest.host_permissions to manifest.content_scripts.matches. This was not only causing the above symptom of getAll() returning both the wildcarded host pattern (e.g. https://*.github.com/*) and the subdomain, but it was breaking other stuff too.

Most importantly, it seems that this was the cause of the failure of code to retrieve the tab URL, which in turn caused the toggle menu item to be enabled and unchecked on sites in the manifest when it should have been disabled and checked, which in turn caused broken behaviour when that toggle was clicked.

[aspiers]

I do apologise if this has wasted some of your time as well as mine! To avoid others making the same mistake, would it be worth explicitly documenting in the readmes for https://github.com/fregante/webext-domain-permission-toggle and https://github.com/fregante/webext-dynamic-content-scripts/ that host_permissions should contain the default list of allowed sites?

[fregante]

Funny you mention that, just yesterday I landed on this:

• Proposal: stop adding host_permissions from content_scripts w3c/webextensions#116

Hey, maybe one day I'll also wrap up my webext-lint to suggest that :)