1491272 - userScripts API should not be available without manifest key or permission

Status: VERIFIED FIXED

(WebExtensions :: General, enhancement, P1)

Description

[Rob Wu [:robwu]]

The userScripts API should require a permission or manifest key, even if there are no permission warnings. Reasons for permissions are similar to bug 1477271.

Comment 1

[Luca Greco [:rpl] [:luca] [:lgreco]]

(In reply to Rob Wu [:robwu] from comment #0)
> The userScripts API should require a permission or manifest key, even if
> there are no permission warnings. Reasons for permissions are similar to bug
> 1477271.

The userScripts API doesn't currently require a userScripts permission (nor the user_scripts manifest key, which is optionally used if the extension need to make custom APIs available to the userScript sandboxes) because the extension
that wants to register a userScript still need to have host permissions that subsume the matches pattern of the registered
userScript (and the following test verifies this behaviors: https://searchfox.org/mozilla-central/rev/dd965445ec47fbf3cee566eff93b301666bda0e1/toolkit/components/extensions/test/xpcshell/test_ext_userScripts.js#30-104).

And so the extension will have a permission warnings in the permissions doorhanger, the ones related to its host permissions.

The same approach is also used by the contentScripts.register API, which doesn't also require any permission on its own.

In my opinion the browser.search API case was different, it needs its own permission because it isn't already covered by the need of particular host permissions.

Comment 2

[Rob Wu [:robwu]]

IMO contentScripts.register should also have been behind a (warning-less) permission, because it allows extensions to dynamically register content scripts.

The userScripts API can also do that, and also create multiple sandboxes, and have some unique script execution behavior (defining an API in an API script and make that available to user scripts).

There is no disadvantage to requiring a manifest key/permission for an API, but it serves several benefits:

- Potential API consumers can be detected very easily.
  Examples of why this can be beneficial:
  - Static analysis / security audit / review.
  - Opportunity for future optimizations (e.g. delaying the load of web pages until an the background page of an extension that uses the userScripts API has loaded).
  - Showing special UI for user script add-ons.
  - Finding add-on devs if we ever introduce any breaking changes.

- Principle of least privilege.
  The userScripts API serves a very specific need that is not needed by the majority of the extensions.
  Requiring (warningless) permissions barely costs anything, but it limits the default capabilities of extensions.

If we ever find an issue with the usage of userScripts (e.g. a security flaw in our implementation, or a typical pattern by bad add-ons), then being able to detect usage by scanning the manifest.json only makes auditing easier.

Even in a permission-less add-on, add-on reviewers currently need to look for tabs.executeScript (and contentScripts.register) to see if there is any undesired code execution in web pages. Adding even more APIs with this capability makes review only more complicated.

Comment 3

[Mike Conca [:mconca]]

As Rob points out, there are very good reasons to have a permission, even if a prompt is never surfaced to the user. We should do this.

Comment 4

[Luca Greco [:rpl] [:luca] [:lgreco]]

Attachment: Bug 1491272 - Require user_scripts manifest property to have access to the userScripts API namespace. r?mixedpuppy!,robwu

Comment 5

[Luca Greco [:rpl] [:luca] [:lgreco]]

Attachment: Bug 1491272 - Lock experimental userScripts API behind a pref and make it enabled by default on Nightly. r?mixedpuppy!

Depends on D6146

Comment 6

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 9009934 [details]
Bug 1491272 - Require user_scripts manifest property to have access to the userScripts API namespace. r?mixedpuppy!,robwu

Shane Caraveo (:mixedpuppy) has approved the revision.

Comment 7

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 9009935 [details]
Bug 1491272 - Lock experimental userScripts API behind a pref and make it enabled by default on Nightly. r?mixedpuppy!

Shane Caraveo (:mixedpuppy) has approved the revision.

Comment 8

[Rob Wu [:robwu]]

Comment on attachment 9009934 [details]
Bug 1491272 - Require user_scripts manifest property to have access to the userScripts API namespace. r?mixedpuppy!,robwu

Rob Wu [:robwu] has approved the revision.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/53206bd09407
Require user_scripts manifest property to have access to the userScripts API namespace. r=robwu,mixedpuppy
https://hg.mozilla.org/integration/autoland/rev/d2ca2c0c3364
Lock experimental userScripts API behind a pref and make it enabled by default on Nightly. r=mixedpuppy

Comment 10

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/53206bd09407
https://hg.mozilla.org/mozilla-central/rev/d2ca2c0c3364

Comment 11

[Luca Greco [:rpl] [:luca] [:lgreco]]

Both these changes are tested in automation, but I guess that an additional round of verification from QA 
could still be nice (and QA may catch something that we may miss in automation).

Follows the steps to QA verify these two changes.

STR for "lock experimental userScripts API being a pref":
- install the test extension attached to Bug 1437861 comment 44
- open about:config and turn "extensions.webextensions.userScripts.enabled" to false
- try to register a userScript
- Expected behavior on the fixed version:
  - the test extension caught and printed in the sidebar the expected error message:
        userScripts APIs are currently experimental and must be enabled with the
        extensions.webextensions.userScripts.enabled preference.
  - no user script is being registered (and so it should match and run if we open a tab that is expected to match it)

STR for "require user_scripts manifest property to have access to the userScripts API namespace":

- unpack the test extension attached to Bug 1437861 comment 44
- remove (or just rename) the "user_scripts" property from its manifest
- install the modified extension
- try to register a userScript
- Expected behavior on the fixed version:
  - the test extension caught and printed in the sidebar the expected error message:
        Error: browser.userScripts is undefined; can't access its "register" property
  - no user script is being registered (and so it should match and run if we open a tab that is expected to match it)

Comment 12

[Victor Carciu]

Verified as fixed using the steps from https://bugzilla.mozilla.org/show_bug.cgi?id=1491272#c11 . I will attach postfix screenshots

Comment 13

[Victor Carciu]

Attachment: Postfix screenshot 1

Comment 14

[Victor Carciu]

Attachment: Postfix screenshot 2