1334918 - Allow modifying privileged sites like AMO with an explicit host permission, separate from <all_urls> or activeTab

Status: NEW

(WebExtensions :: General, defect, P5)

Description

[Kris Maglione [:kmag]]

This will require modifying the AMO validator to require admin review before signing such extensions.

Can someone please file an AMO bug to implement that behavior? The current list of sites protected by this policy are[1]:

  addons.mozilla.org
  discovery.addons.mozilla.org
  testpilot.firefox.com

[1]: http://searchfox.org/mozilla-central/rev/8fa84ca6444e2e01fb405e078f6d2c8da0e55723/toolkit/mozapps/extensions/AddonManagerWebAPI.cpp#23-25

Comment 1

[Andreas Wagner [:TheOne] [use NI]]

So, whenever a webextension uses one or more of the mentioned urls in their host permissions, the review should automatically be flagged for admin review?
What do we do with unlisted add-ons?

Comment 2

[Andrew Swan [:aswan]]

I think this would be more maintainable if we made it a regular named permission instead of overloading host permissions.

Comment 3

[Kris Maglione [:kmag]]

(In reply to Andreas Wagner [:TheOne] from comment #1)
> So, whenever a webextension uses one or more of the mentioned urls in their
> host permissions, the review should automatically be flagged for admin
> review?

Correct.

> What do we do with unlisted add-ons?

Probably the same. Or just reject the add-on. I don't have a particularly strong opinion on which.

(In reply to Andrew Swan [:aswan] from comment #2)
> I think this would be more maintainable if we made it a regular named
> permission instead of overloading host permissions.

I don't think this would be tantamount to overloading host permissions. We'd essentially just be excluding those hosts from <all_urls> and activeTab, and require them to be explicitly specified.

Comment 4

[Andrew Swan [:aswan]]

(In reply to Kris Maglione [:kmag] from comment #3)
> I don't think this would be tantamount to overloading host permissions. We'd
> essentially just be excluding those hosts from <all_urls> and activeTab, and
> require them to be explicitly specified.

Well I have a few concerns:
- in permissions prompts we collapse redundant permissions so for instance if you request "<all_urls>" and "https://site.com", the prompt just says "all sites".  I could see somebody reasonably wanting to do similar collapsing elsewhere, but doing this with host permissions would prevent doing that.
- related to the above, when computing changes in permissions between updates, we don't currently do anything very clever with host permissions but i could see wanting to do so (e.g., going from <all_urls> in version 1 to "https://site.com" in version 2 is actually not requesting new permissions).  if some individual host permissions are treated specially, that would become more complicated
- now we have to keep the list of affected sites in sync between the browser and AMO, that's a drag

Comment 5

[Kris Maglione [:kmag]]

(In reply to Andrew Swan [:aswan] from comment #4)
> Well I have a few concerns:
> - in permissions prompts we collapse redundant permissions so for instance
> if you request "<all_urls>" and "https://site.com", the prompt just says
> "all sites".  I could see somebody reasonably wanting to do similar
> collapsing elsewhere, but doing this with host permissions would prevent
> doing that.

Given that these extensions will require special review, I think that continuing to collapse them in the permission prompt is fine. Users shouldn't need to be aware that this is a special case, but reviewers and internal code will.

> - related to the above, when computing changes in permissions between
> updates, we don't currently do anything very clever with host permissions
> but i could see wanting to do so (e.g., going from <all_urls> in version 1
> to "https://site.com" in version 2 is actually not requesting new
> permissions).  if some individual host permissions are treated specially,
> that would become more complicated

Again, this shouldn't be an issue as far as updates or prompts are concerned.

> - now we have to keep the list of affected sites in sync between the browser
> and AMO, that's a drag

It is, but I can live with it.

Comment 7

[Col]

My bug has been m a duplicate of this one, but also concerns about:* pages.

Is the permission being discussed here intended to allow modification of about:* pages as well as AMO? Or is this marked as duplicate because there's not a "related" button?

Comment 8

[Col]

Phone keyboard lost some characters:

> My bug has been marked as a duplicate of this one, but also concerns about:* pages...

Is what I meant to say.

Comment 9

[Andrew Swan [:aswan]]

about: pages were covered separately in bug 1270412

Comment 12

[Andy McKay]

One use case was from reviewers, however the review tools on AMO have moved to a seperate domain, that's no longer relevant.

Most of the other requests seem to be around the ability to alter AMO (e.g. gestures) or the fact that people install an add-on from AMO and then test it on AMO because it was the place they were when they installed it.

It would be nice to fix, but I don't think it warrants a P2 and I'm dropping down the priority, although I'm tempted to close.

I'm also strongly against anything that requires "special review", any API needing that should provide a large amount of functionality to justify the work it entails.

Noting that Chrome can't run content scripts on the Chrome store either.

Comment 13

[Philipp Kewisch [:Fallen][📱📆]]

(In reply to Andy McKay [:andym] from comment #12)
> Most of the other requests seem to be around the ability to alter AMO (e.g.
> gestures) or the fact that people install an add-on from AMO and then test
> it on AMO because it was the place they were when they installed it.
Could this be solved by user messaging, e.g. a note somewhere that some add-ons will not work on AMO?

Comment 14

[BugBot [:suhaib / :marco/ :calixte]]

The severity field for this bug is relatively low, S3. However, the bug has 3 duplicates.
:robwu, could you consider increasing the bug severity?

For more information, please visit auto_nag documentation.

Comment 15

[BugBot (nomail) [:suhaib / :marco/ :calixte]]

The last needinfo from me was triggered in error by recent activity on the bug. I'm clearing the needinfo since this is a very old bug and I don't know if it's still relevant.