923897 - Extensibility support for b2g

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect)

Description

[[:fabrice] Fabrice Desré]

Attachment: b2g-add-ons.patch

This wip adds initial support for css and js script injection in pages and apps.
These customizations are shipped as apps themselves, and have to be declared in the app manifest this way:
"customization": [
    {
      "filter": "http://youtube.com",
      "css": ["file1.css", "file2.css"],
      "scripts": ["script1.js", "script2.js"]
    }
   ]

'filter' is treated as a prefix match. The security model is such that the customization shipped in an app will only be applied to content that is of equal or lesser privilege.

Comment 1

[sonny]

:fabrice,
Nice!
Don't you think it might afraid some editors/companies out there?
How would Mozilla handle that?

Comment 2

[David Chan [:dchan]]

Is there a feature page for this?

My understanding is that this patch would allow apps to declare a customization array in the manifest. Sites which match "filter" will have the listed "css" and "scripts" files injected into them.

Comment 3

[[:fabrice] Fabrice Desré]

There's no feature page yet. Do we need that?

Comment 4

[David Chan [:dchan]]

The change looks small enough that I don't think we need a full fledged feature page. However I wanted to make sure our team had a clear understanding of the security implications when we review this bug.

Comment 6

[Dietrich Ayala (:dietrich)]

Attachment: Link to Github pull-request: https://github.com/mozilla/gecko-dev/pull/7

rebased to master

Comment 7

[Dietrich Ayala (:dietrich)]

We should add a pref and land this turned off, so people can experiment.

Comment 8

[[:fabrice] Fabrice Desré]

Yep, I'll do that. This need tests too...

Comment 9

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Comment on attachment 813929 [details] [diff] [review]
b2g-add-ons.patch

Review of attachment 813929 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/apps/src/UserCustomization.jsm
@@ +176,5 @@
> +    aWindow.addEventListener("unload", () => {
> +      Cu.nukeSandbox(sandbox);
> +      sandbox = null;
> +    });
> +  },

As a comment I think you want to do something different than injectItems.

Why ?
 - For stylesheets it makes it hard to order them the way you want, and to override styles.
 - For scripts, it makes it hard to load the script only when you really want it.

Instead I suggest that you hack around the CSP policy, so the files that are listed in the manifest are allowed to be loaded by the CSP. That would makes it much closer to real web development world and offer greater flexibility to the app authors.

Also I'm curious about how this is supposed to work for packaged app ? My understanding is that in order to make things a bit faster when we try to open them is that a FD is sent as soon as we know what content will be loaded, and this FD is then used by the content process to load the file. Do you plan to send the FD of the other app to the content process in order to boost the startup process ? And how do we make sure that the content process does not try to access other resources of the other app ?

Comment 10

[[:fabrice] Fabrice Desré]

(In reply to Vivien Nicolas (:vingtetun) (:21) (NOT reading bugmails, needinfo? please) from comment #9)

> As a comment I think you want to do something different than injectItems.
> 
> Why ?
>  - For stylesheets it makes it hard to order them the way you want, and to
> override styles.

Yep, ordering is unaddressed in this patch. I punted on that for a v1 because of all the UI that could be involved if we really need that. How can users know how to order stylesheets? Themes authors can't know about the interactions with other themes so it's not possible to set direct dependencies at the theme level...

>  - For scripts, it makes it hard to load the script only when you really
> want it.

Why? We load scripts only if they have been declared as needed for a page.

> Instead I suggest that you hack around the CSP policy, so the files that are
> listed in the manifest are allowed to be loaded by the CSP. That would makes
> it much closer to real web development world and offer greater flexibility
> to the app authors.

But that would mean that the "injected-in" app needs to pull resources from the add-on right? The point here is that it works the other way around, where add-ons push themselves in apps or web pages without needing them to be modified.

> Also I'm curious about how this is supposed to work for packaged app ? My
> understanding is that in order to make things a bit faster when we try to
> open them is that a FD is sent as soon as we know what content will be
> loaded, and this FD is then used by the content process to load the file. Do
> you plan to send the FD of the other app to the content process in order to
> boost the startup process ? And how do we make sure that the content process
> does not try to access other resources of the other app ?

Opening and sending the FD is very cheap, I'm not worried about performance issues because of that. For sure there will be some impact as we load additional resources. That patch doesn't change the security checks for cross-package resource access (they are in necko parent code) so I don't feel we have a risk there either.

Comment 11

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

OK. I guess most of the confusion here is because I read the feature the other way around.

I read it as a way to for an app to opt-in to some external styles or scripts, and so as a way to exposed shared/ resources...

Comment 13

[sjw]

Would it be possible to use regular expressions instead of an url as filter? This would enlarge the use case a lot.

Comment 14

[Dietrich Ayala (:dietrich)]

Attachment: fxos-extensibility.diff

Updated patch against current m-c.

Comment 15

[Dietrich Ayala (:dietrich)]

Needs some tests, and then we should land this turned off by default.

Comment 16

[Dietrich Ayala (:dietrich)]

(In reply to sjw from comment #13)
> Would it be possible to use regular expressions instead of an url as filter?
> This would enlarge the use case a lot.

I like it. Let's get the base landed first though, and then can tackle more features afterwards.

Comment 17

[[:fabrice] Fabrice Desré]

Attachment: b2g-addons.patch

Rebased, and with tests that verify both css styling and script injection.
It's also checking that disabled add-ons are not applied.

try run at:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=d31978a14a72

Comment 18

[[:fabrice] Fabrice Desré]

Emulator M1 failure, will investigate.

Comment 19

[[:fabrice] Fabrice Desré]

Attachment: b2g-addons.patch

Try build at https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=4563f5c289c0

The security model is to allow privileged and certified apps to be addons, but only let certified addons to inject modifications to certified apps. This is in line with our app preloading mechanism.

Comment 20

[Fernando Jiménez Moreno [:ferjm]]

Comment on attachment 8533760 [details] [diff] [review]
b2g-addons.patch

Review of attachment 8533760 [details] [diff] [review]:
-----------------------------------------------------------------

Thank you Fabrice. The code looks good to me in general, but I have a few questions before giving it a closer look:

- Is the webAPI and security teams aware of this feature? At a first glance this looks very powerful but it also feels a bit scary to allow one app to inject JS in other apps code. I know we are allowing this to certified and privileged apps only, but it seems to me that we are putting too much responsibility in the hands of app reviewers in this case. It is not only that they have to review the code of the app adding the add-on files but also the code of the affected urls, right?. I guess it is the same scenario as for desktop add-ons anyway. In any case, I just want to be sure that this has the proper attention from the security and webAPI folks.

- Are we allowing apps to avoid being modified? Some app authors may not want their apps to be extensible for many reasons (security could be one for instance) so I think it would be good to also add the ability for apps to opt out of this feature.

- How can users manage (enable/disable/uninstall) addons? Are we going to add an "Addons manager" section in settings as we do with the themes in b2g? If so, could you file a bug for that, please? How would this work on other products? 

- Do we want this to be available on b2g only?

::: b2g/app/b2g.js
@@ +1041,5 @@
>  // Enable wakelock
>  pref("dom.wakelock.enabled", true);
>  
> +// Enable webapps add-ons
> +pref("dom.apps.customization.enabled", true);

Are we landing this enabled by default?

::: dom/apps/AppsUtils.jsm
@@ +478,5 @@
>      }
>      return true;
>    },
>  
> +  allowUnsignedAddons: false, // for testing purposes.

Could this be a pref to ease the manual testing process, please?

@@ +492,5 @@
>      if (aRole == "theme" && aStatus !== Ci.nsIPrincipal.APP_STATUS_CERTIFIED) {
>        return false;
>      }
> +    if (!this.allowUnsignedAddons &&
> +        (aRole == "addon" &&

I guess we need a Gaia patch as well to hide apps with this role.

https://mxr.mozilla.org/gaia/source/apps/homescreen/js/grid.js#16

::: dom/apps/UserCustomization.jsm
@@ +37,5 @@
> +  * ]
> +  */
> +
> +function debug(aStr) {
> +  dump("-*-*- UserCustomization (" +

Enable this only if dom.mozApps.debug is true, please

@@ +84,5 @@
> +
> +    this._enabled = false;
> +    try {
> +      this._enabled = Services.prefs.getBoolPref("dom.apps.customization.enabled");
> +    } catch(e) {}

Why don't we do this on .init()?

@@ +88,5 @@
> +    } catch(e) {}
> +
> +    if (!this._enabled) {
> +      return;
> +    }

You can get rid of this as you have the same condition below.

@@ +134,5 @@
> +      if (item.scripts && Array.isArray(item.scripts)) {
> +        item.scripts.forEach((script) => {
> +          custom.scripts.push(base.resolve(script));
> +        });
> +      }

It would be good to look for dups before registering css or js scripts.

@@ +216,5 @@
> +    let sandbox;
> +    if (aItem.scripts.length > 0) {
> +      sandbox = Cu.Sandbox(aWindow,
> +                           { wantComponents: false,
> +                             wantXrays: true,

I read some docs, but I am still unsure about the implications of this. Bobby, could you take a look at this specific piece, please? Thanks!

::: dom/apps/Webapps.jsm
@@ +2014,5 @@
>        }
>        this._registerSystemMessages(aNewManifest, aApp);
>        this._registerActivities(aNewManifest, aApp, true);
>        this._registerInterAppConnections(aNewManifest, aApp);
> +      UserCustomization.register(aNewManifest, aApp);

I don't think this is dependent of system messages support

@@ +4025,5 @@
>      Services.obs.notifyObservers(null, "webapps-uninstall", JSON.stringify(aApp));
>  
>      if (supportSystemMessages()) {
>        this._unregisterActivities(aApp.manifest, aApp);
> +      UserCustomization.unregister(aApp.manifest, aApp);

Ditto

Comment 21

[Bobby Holley (:bholley)]

Comment on attachment 8533760 [details] [diff] [review]
b2g-addons.patch

Review of attachment 8533760 [details] [diff] [review]:
-----------------------------------------------------------------

f=bholley on sandbox initialization modulo that.

::: dom/apps/UserCustomization.jsm
@@ +214,5 @@
> +    });
> +
> +    let sandbox;
> +    if (aItem.scripts.length > 0) {
> +      sandbox = Cu.Sandbox(aWindow,

You need [aWindow] (an nsExpandedPrincipal) here to give the addon the appropriate security characteristics. That will also let you remove the |wantXrays| flag below, since same-origin Xrays are a deprecated feature.

Comment 22

[Chris Lord [:cwiiis]]

"Are we allowing apps to avoid being modified? Some app authors may not want their apps to be extensible for many reasons (security could be one for instance) so I think it would be good to also add the ability for apps to opt out of this feature."

I think it's quite important that apps *can't* opt out - it should be entirely under users' control. If as a user there's an app I want to use, but it has some heinous behaviour that I wasn't to remove, I don't want to be restricted from doing so.

Comment 23

[[:fabrice] Fabrice Desré]

(In reply to Fernando Jiménez Moreno [:ferjm] from comment #20)

> - Is the webAPI and security teams aware of this feature? At a first glance
> this looks very powerful but it also feels a bit scary to allow one app to
> inject JS in other apps code. I know we are allowing this to certified and
> privileged apps only, but it seems to me that we are putting too much
> responsibility in the hands of app reviewers in this case. It is not only
> that they have to review the code of the app adding the add-on files but
> also the code of the affected urls, right?. I guess it is the same scenario
> as for desktop add-ons anyway. In any case, I just want to be sure that this
> has the proper attention from the security and webAPI folks.

Yes, Jonas & Paul are very aware of what's happening there. And yes, we'll have the same review process as add-ons and privileged apps. Reviewer can't review code from affected urls anyway.

> - Are we allowing apps to avoid being modified? Some app authors may not
> want their apps to be extensible for many reasons (security could be one for
> instance) so I think it would be good to also add the ability for apps to
> opt out of this feature.

Like Chris, I disagree. If your app is insecure, any current greasemonkey script can hack it. We are not adding risk there compared to what already exists.

> - How can users manage (enable/disable/uninstall) addons? Are we going to
> add an "Addons manager" section in settings as we do with the themes in b2g?
> If so, could you file a bug for that, please? How would this work on other
> products? 

Yep, I have a WIP patch for gaia that:
- hides the add-on icons on the homescreen.
- adds an add-on panel where you can enable/disable add-ons.

I'm not sure yet about desktop & android, because of the way they use profiles.

> - Do we want this to be available on b2g only?

For now probably yes.

> ::: b2g/app/b2g.js
> @@ +1041,5 @@
> >  // Enable wakelock
> >  pref("dom.wakelock.enabled", true);
> >  
> > +// Enable webapps add-ons
> > +pref("dom.apps.customization.enabled", true);
> 
> Are we landing this enabled by default?

Sure, why not? :P

> ::: dom/apps/AppsUtils.jsm
> @@ +478,5 @@
> >      }
> >      return true;
> >    },
> >  
> > +  allowUnsignedAddons: false, // for testing purposes.
> 
> Could this be a pref to ease the manual testing process, please?

It could, I just wanted to make it a bit harder to turn off security bits.

> @@ +84,5 @@
> > +
> > +    this._enabled = false;
> > +    try {
> > +      this._enabled = Services.prefs.getBoolPref("dom.apps.customization.enabled");
> > +    } catch(e) {}
> 
> Why don't we do this on .init()?

Because the profile when running tests has the pref unset, and it's too late to use pushPrefEnv() for that. If there's a way to change the prefs used by the testing profile I'll switch.

Comment 24

[Andrew Sutherland [:asuth] (he/him)]

(In reply to Chris Lord [:cwiiis] from comment #22)
> "Are we allowing apps to avoid being modified? Some app authors may not want
> their apps to be extensible for many reasons (security could be one for
> instance) so I think it would be good to also add the ability for apps to
> opt out of this feature."
> 
> I think it's quite important that apps *can't* opt out - it should be
> entirely under users' control. If as a user there's an app I want to use,
> but it has some heinous behaviour that I wasn't to remove, I don't want to
> be restricted from doing so.

Bug 866522 has the meta-discussion and links to other discussions related to the interaction of CSP and bookmarklets (a very similar issue and CSP is likely the mechanism that would be used for opting out anyways).  I'm not expecting this bug to go spammy over this, but I think it's best to raise any additional discussion points on that bug or (better) on the discussion threads linked to by that bug.

Comment 25

[Fernando Jiménez Moreno [:ferjm]]

Comment on attachment 8533760 [details] [diff] [review]
b2g-addons.patch

Review of attachment 8533760 [details] [diff] [review]:
-----------------------------------------------------------------

::: b2g/app/b2g.js
@@ +1041,5 @@
>  // Enable wakelock
>  pref("dom.wakelock.enabled", true);
>  
> +// Enable webapps add-ons
> +pref("dom.apps.customization.enabled", true);

I would prefer if we land this disabled at least until we have a way to disable/uninstall addons

::: dom/apps/AppsService.js
@@ +14,5 @@
>  
>  Cu.import("resource://gre/modules/XPCOMUtils.jsm");
>  Cu.import("resource://gre/modules/Services.jsm");
>  Cu.import("resource://gre/modules/Promise.jsm");
> +Cu.import("resource://gre/modules/UserCustomization.jsm");

Can we avoid loading this module if the customizations are disabled?

::: dom/apps/AppsUtils.jsm
@@ +492,5 @@
>      if (aRole == "theme" && aStatus !== Ci.nsIPrincipal.APP_STATUS_CERTIFIED) {
>        return false;
>      }
> +    if (!this.allowUnsignedAddons &&
> +        (aRole == "addon" &&

Could you file the bugs for adding the Gaia side of this feature, please? Or if they already exist, add them to the dependency tree of this bug.

::: dom/apps/UserCustomization.jsm
@@ +27,5 @@
> +                                   "nsIConsoleService");
> +/**
> +  * Customization scripts and CSS stylesheets can be specified in an
> +  * application manifest with the following syntax:
> +  * "customization": [

nit: we use plurals for enumerations in the manifest. "activities", "messages", "redirects", "datastores-owned", "connections", etc. This should probably be "customizations"

@@ +84,5 @@
> +
> +    this._enabled = false;
> +    try {
> +      this._enabled = Services.prefs.getBoolPref("dom.apps.customization.enabled");
> +    } catch(e) {}

I think you can move this to .init() and solve the test issue that you mentioned in comment 23 by adding the pref to https://mxr.mozilla.org/mozilla-central/source/testing/profiles/prefs_general.js

@@ +209,5 @@
> +      utils.loadSheet(uri, Ci.nsIDOMWindowUtils.AUTHOR_SHEET);
> +      if (!this._loaded[aItem.manifestURL]) {
> +        this._loaded[aItem.manifestURL] = { css: [], scripts: [] };
> +      }
> +      this._loaded[aItem.manifestURL].css.push({ window: aWindow, uri: uri });

If we control dups in the registration process, we should never inject the same scripts twice, right?

::: dom/apps/Webapps.jsm
@@ +41,5 @@
>  Cu.import("resource://gre/modules/AppDownloadManager.jsm");
>  Cu.import("resource://gre/modules/osfile.jsm");
>  Cu.import("resource://gre/modules/Task.jsm");
>  Cu.import("resource://gre/modules/Promise.jsm");
> +Cu.import("resource://gre/modules/UserCustomization.jsm");

Same as above, can we avoid loading this if it is preffed off? We'll need to mock it or check before using it though, but we'll save the memory and load process.

It could also be lazy loaded.

@@ +406,5 @@
>          if (app.appStatus >= Ci.nsIPrincipal.APP_STATUS_PRIVILEGED) {
>            app.redirects = this.sanitizeRedirects(aResult.redirects);
>          }
>          app.kind = this.appKind(app, aResult.manifest);
> +        UserCustomization.register(aResult.manifest, app);

If we lazy load this module, we can check if "aResult.manifest.customizations" exists before using "UserCustomization".

@@ +1098,5 @@
>          app.kind = this.appKind(app, aResult.manifest);
>          this._registerSystemMessages(manifest, app);
>          this._registerInterAppConnections(manifest, app);
>          appsToRegister.push({ manifest: manifest, app: app });
> +        UserCustomization.register(manifest, app);

Same here.

::: dom/apps/tests/addons/manifest.webapp
@@ +3,5 @@
> +  "description": "Let me inject script and css!",
> +  "customization" : [
> +    {
> +      "filter": "http://mochi.test:8888/tests/dom/apps/tests/addons",
> +      "css": ["style.css"],

Let´s try with more than one css and script. And with invalid file names.

::: dom/apps/tests/addons/script.js
@@ +1,2 @@
> +document.addEventListener("DOMContentLoaded", function() {
> +  //alert(document.documentURI);

Remove this, please.

::: dom/apps/tests/test_app_addons.html
@@ +59,5 @@
> +  ifr.addEventListener("mozbrowsershowmodalprompt", listener, false);
> +
> +  // Open an the app url in an iframe.
> +  ifr.setAttribute("mozbrowser", "true");
> +  //ifr.setAttribute("remote", "true");

Remove the line?

@@ +92,5 @@
> +  SpecialPowers.allowUnsignedAddons();
> +  SpecialPowers.pushPrefEnv({'set': [
> +    ["dom.mozBrowserFramesEnabled", true],
> +    ["dom.apps.customization.enabled", true],
> +    ["dom.sysmsg.enabled", true] // That let us test things on desktop too.

I still can't see why is this system messages dependent.

::: dom/ipc/preload.js
@@ +26,5 @@
>    Cu.import("resource://gre/modules/NetUtil.jsm");
>    Cu.import("resource://gre/modules/Services.jsm");
>    Cu.import("resource://gre/modules/SettingsDB.jsm");
>    Cu.import("resource://gre/modules/XPCOMUtils.jsm");
> +  Cu.import("resource://gre/modules/UserCustomization.jsm")

I think it is worth not loading this if user customizations are not enabled.

::: js/xpconnect/loader/mozJSSubScriptLoader.cpp
@@ +345,5 @@
>      if (NS_FAILED(rv)) {
>          return ReportError(cx, LOAD_ERROR_NOSCHEME, uri);
>      }
>  
> +    if (!scheme.EqualsLiteral("chrome") && !scheme.EqualsLiteral("app")) {

This change needs to be reviewed by an xpconnect peer.

Comment 26

[[:fabrice] Fabrice Desré]

Attachment: patch v2

Addressed comments.

Bobby, can you check the change in mozJSSubScriptLoader.cpp ? I'm whitelisting the app:// protocol in addition of the chrome:// protocol to let script injection work.

Comment 27

[Bobby Holley (:bholley)]

Comment on attachment 8536350 [details] [diff] [review]
patch v2

Review of attachment 8536350 [details] [diff] [review]:
-----------------------------------------------------------------

r=me on the subscript loader change.

Comment 28

[Fernando Jiménez Moreno [:ferjm]]

Comment on attachment 8536350 [details] [diff] [review]
patch v2

Review of attachment 8536350 [details] [diff] [review]:
-----------------------------------------------------------------

Thank you Fabrice!

Looks good. The only thing I am missing is the check for duplicated scripts. r=me with that added and a test for it, please.

::: dom/apps/UserCustomizations.jsm
@@ +36,5 @@
> +  *  }
> +  * ]
> +  */
> +
> +let debug = Services.prefs.getBoolPref("dom.mozApps.debug")

Put this within a try/catch please. Just in case we miss the pref.

Comment 29

[[:fabrice] Fabrice Desré]

https://hg.mozilla.org/integration/b2g-inbound/rev/060e34d41972

Comment 30

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/060e34d41972

Comment 31

[David Bialer [:dbialer]]

Is this in fxos2.2?

Comment 32

[Martijn Wargers (dead)]

Current trunk is 2.2, so yes, I think this is in 2.2.

Comment 33

[[:fabrice] Fabrice Desré]

That's correct. But I've yet to finish the gaia part, and of course we need marketplace support. I'll pref off if the stars don't align in time.

Comment 34

[Adrian Aichner [:anaran]]

When I install my add-on via webide it hits https://dxr.mozilla.org/mozilla-central/source/dom/apps/AppsUtils.jsm#73 twice where `this` is my addon. First time this.role == "", second time "addon".

aIID gives this in the console:

XPCWrappedNative_NoHelper { number: "{986c11d0-f340-11d4-9075-0010a4e73d9a}", name: "", valid: true }

adb logcat:

Uninstall via Settings App-Permissions on flame device:

I/GeckoDump(  212): XXX FIXME : Got a mozContentEvent: webapps-uninstall-granted
I/Gecko   (  212): -- InterAppCommService: 1421437734685: Finish updating registered/allowed connections for an uninstalled app.
I/Gecko   ( 3362): -*-*- UserCustomizations (child): _removeItem: 02ccb1c631f89a7f2331e24da209d9bf
I/Gecko   ( 3362): -*-*- UserCustomizations (child): _unloadForManifestURL app://b1bba6b4-1790-4054-9608-064dd5220203/manifest.webapp

But function register is never called for the add-on.

How would I proceed debugging that part?

Comment 35

[[:fabrice] Fabrice Desré]

Adrian, can you file a new bug? This one is closed.