userScripts API should not be available without manifest key or permission

VERIFIED FIXED in Firefox 64

Status

enhancement
P1
normal
VERIFIED FIXED
9 months ago
9 months ago

People

(Reporter: robwu, Assigned: rpl)

Tracking

(Blocks 1 bug)

unspecified
mozilla64
Dependency tree / graph

Firefox Tracking Flags

(firefox64 verified)

Details

Attachments

(4 attachments)

Reporter

Description

9 months ago
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.
Assignee

Comment 1

9 months ago
(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.
Reporter

Comment 2

9 months ago
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.
See Also: → 1470310
Flags: needinfo?(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.
Severity: normal → enhancement
Flags: needinfo?(mconca)
Priority: -- → P1
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.
Attachment #9009934 - Flags: review+
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.
Attachment #9009935 - Flags: review+
Assignee

Updated

9 months ago
Assignee: nobody → lgreco
Status: NEW → ASSIGNED
Iteration: --- → 64.2 (Sep 28)
Attachment #9009934 - Attachment description: Bug 1491272 - Added a userScripts optional permission. r?mixedpuppy!,robwu → Bug 1491272 - Require user_scripts manifest property to have access to the userScripts API namespace. r?mixedpuppy!,robwu
Reporter

Comment 8

9 months ago
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.
Attachment #9009934 - Flags: review+

Comment 9

9 months ago
Pushed by luca.greco@alcacoop.it:
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

9 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/53206bd09407
https://hg.mozilla.org/mozilla-central/rev/d2ca2c0c3364
Status: ASSIGNED → RESOLVED
Closed: 9 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
Assignee

Comment 11

9 months ago
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

9 months ago
Verified as fixed using the steps from https://bugzilla.mozilla.org/show_bug.cgi?id=1491272#c11 . I will attach postfix screenshots
Status: RESOLVED → VERIFIED

Comment 13

9 months ago
Posted image Postfix screenshot 1

Comment 14

9 months ago
Posted image Postfix screenshot 2
You need to log in before you can comment on or make changes to this bug.