Change adblockpluschrome permissions to '<all_urls>'

[jsonesen]

Background

Due to the changes introduced as a result of issue #5027, the upload to the Chrome Web Store currently fails. While Chrome requires adding permissions for the ws:// and wss:// protocol schemes, the ​validation in the Chrome Web Store still rejects extensions that use them.

What to change

replace the current permissions in metadata.chrome with <all_urls>

[greiner]

It'd be great if we could check beforehand whether changing this will cause a permission warning to be shown when updating from an older version. Because if that's the case the extension will be disabled and we'd lose a significant amount of users.

When the extension autoupdates, the increased permissions cause the extension to be disabled until the user re-enables it.

Source: ​https://developer.chrome.com/extensions/permission_warnings

On that note it'd be helpful to introduce some automated mechanism to warn of permission changes since those can be quite devastating.

[sebastian]

There is no way to check that beforehand. We have to wait at least until the devbuild in the Web Store has been updated, and therefore this issue must be fixed first. Otherwise, we won't be able to upload anything to the Chrome Web Store at all.

It's not clear whether the change from http://*/* and https:/*/* to <all_urls> will be considered an "increased permission" as the message shown seems to be the same for both sets of permissions. But we are aware of this possibility and have it covered in 5027#Howtotest.

[kzar]

To test beforehand someone™ could create a new dummy extension with the same permissions, publish it, install it on a few computers, then update the permissions and version, publish again and then see what happens on upgrade. That would be a pain to do - and I don't volunteer - but perhaps it's worth it given the risk?

(Careful publishing an extension with a similar name to "Adblock Plus", I had a dummy ad blocking extension called "Adblocker" which wasn't even publicly available get forcibly removed by Google once.)

[greiner]

Replying to sebastian:

There is no way to check that beforehand.

We could, for instance, have warnings when certain parts of the manifest, which might cause a permission warning, change and we could even prevent changes that we have identified to reliably cause such a permission warning.

There's no expectation of such a system covering all cases.

[sebastian]

Replying to kzar:

That would be a pain to do - and I don't volunteer - but perhaps it's worth it given the risk?

I don't have access to our Chrome Web Store account, so only Felix or Wladimir could do that, or somebody has to do it with their private developer account (if you don't have one yet, getting one is quite a process, involving paperwork and a payment of 10 USD). And in the end that dummy extension will remain forever in the dashboard of whatever account was used for this test.

On the other hand what risk do we face, if we just test it with our devbuild? When the extension gets disabled on update due to permission changes, I will just backout the change, and in the worst case we just lose some devbuild users which got the update before I undid the change.

Replying to greiner:

We could, for instance, have warnings when certain parts of the manifest, which might cause a permission warning, change and we could even prevent changes that we have identified to reliably cause such a permission warning.

There's no expectation of such a system covering all cases.

First of all how would you even identify a change? The only way to do so at build time (and later would be pointless) would be by comparing the current metadata against some previous revision, and this is just a mess. We would need support for both hg and git. Then which revision should we compare against anyway? If you compare against the previous revision you might not see the warning if there is already a commit on top, if you compare against the last release tag, we would now spam every developer with a warning until we had the next release. Then the metadata format and even the filename is changing occasionally. Also permission changes could potentially be caused by a change to buildtools. So eventually we would have to compare the generated manifest.json files, which makes it even more complicated.

Then there is no way to tell which permissions changes are critical and which not. The logic implemented by the Chrome Web Store is vastly undocumented and matter to change. Adblock Plus is already requesting most permissions that are known to cause a warning. And in the rare situations we further increase the permissions of Adblock Plus it is unlikely that an algorithm we would come up with, would prevent us from mistakes. But even if we ignore all the issues above, what would have it helped us here? Not at all. We were aware of the potential consequences from the beginning, and have a plan to make sure that we don't release this change to all users if it will cause the extension to be disabled. There are code reviews, and devbuilds on the Chrome Web Store, which so far have been sufficient in order to prevent us from messing up stuff like this.

[greiner]

Replying to sebastian:

First of all how would you even identify a change? The only way to do so at build time (and later would be pointless) would be by comparing the current metadata against some previous revision, and this is just a mess.

I don't like to make assumptions on when or where to check that because others might be able to think of better solutions than I could come up with. Presumably, we could verify commits when they are pushed to the server, for instance.

Also permission changes could potentially be caused by a change to buildtools. So eventually we would have to compare the generated manifest.json files, which makes it even more complicated.

Adding tests for buildtools would be a good idea either way.

Then there is no way to tell which permissions changes are critical and which not. The logic implemented by the Chrome Web Store is vastly undocumented and matter to change.

Again, there's no expectation of such a system covering all cases so relying on the list at ​https://developer.chrome.com/extensions/permission_warnings should already be quite valuable.

But even if we ignore all the issues above, what would have it helped us here? Not at all. We were aware of the potential consequences from the beginning, and have a plan to make sure that we don't release this change to all users if it will cause the extension to be disabled. There are code reviews, and devbuilds on the Chrome Web Store, which so far have been sufficient in order to prevent us from messing up stuff like this.

Both code reviews as well as devbuilds have failed at some point in the past which is why we also have tests to verify that critical functionality doesn't break.

[sebastian]

When the commit lands on the server it is too late. Unless the server will reject the push it will already cause a new devbuild to be generated. Blocking the push until checks like this pass, might be a bad idea. Either way, if this warning is any meaningful you should get it even before you upload a patch for review, just like test and linter output. Anyway, if you are not willing to do such considerations, we are not discussing a possible solution, but just dream up wild ideas.

Besides all the technical problems that might render such a mechanism unfeasible (or at least extremely elaborate), I'm still missing a case in which it would be helpful. This issue is none such case, as explained above. While code reviews and devbuilds not always catch every issue, I think that they work much more reliable than an automated check for something like this. Anyway, this discussion is off-topic here. If you want to invest more time pursuing this idea, please file a separate issue, or move it to other channels.

We must move forward with this change, otherwise our development builds remain outdated, and we neither will be able to publish a new release version.

[abpbot]

A commit referencing this issue has landed:
​Issue 5130 - Changes url protocol permissions to all urls

[sebastian]

The devbuilds are updated on the Chrome Web Store again, and after it was automatically updated on my computer running Chrome 57, the extension was NOT disabled and no warning was shown. So it looks good, but I leave it up to QA to test this more thorough.

[jsonesen]

Looks like explicitly listing websocket protocol patterns when registering the listener is causing an error on chrome 57 that prevents request blocking. So, I will open a new review for this.

[abpbot]

A commit referencing this issue has landed:
​Issue 5130 - Changes listener to register <all_urls> and filter non ws http requests

[Ross]

Done. Change seems fine. Upgrading from older versions before the change works with no extra warnings.

ABP 1.13.2.1785
Chrome 49 / 59 / Windows 7
Opera 36 / 45 / Windows 7