Status

defect
RESOLVED FIXED
6 years ago
2 years ago

People

(Reporter: fabrice, Assigned: fabrice)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 5 obsolete attachments)

Assignee

Description

6 years ago
Posted patch b2g-add-ons.patch (obsolete) — Splinter Review
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

6 years ago
:fabrice,
Nice!
Don't you think it might afraid some editors/companies out there?
How would Mozilla handle that?
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.
Flags: needinfo?(fabrice)
Assignee

Comment 3

6 years ago
There's no feature page yet. Do we need that?
Flags: needinfo?(fabrice)
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.
Assignee

Updated

5 years ago
Duplicate of this bug: 911483
We should add a pref and land this turned off, so people can experiment.
Assignee

Comment 8

5 years ago
Yep, I'll do that. This need tests too...
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 ?
Assignee

Comment 10

5 years ago
(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.
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...

Updated

5 years ago
Duplicate of this bug: 1028911

Comment 13

5 years ago
Would it be possible to use regular expressions instead of an url as filter? This would enlarge the use case a lot.
Posted patch fxos-extensibility.diff (obsolete) — Splinter Review
Updated patch against current m-c.
Attachment #813929 - Attachment is obsolete: true
Attachment #8379827 - Attachment is obsolete: true
Needs some tests, and then we should land this turned off by default.
(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.
Assignee

Updated

5 years ago
Depends on: 1072090
Assignee

Comment 17

5 years ago
Posted patch b2g-addons.patch (obsolete) — Splinter Review
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
Assignee: nobody → fabrice
Attachment #8528057 - Flags: review?(ferjmoreno)
Assignee

Updated

5 years ago
Attachment #8528057 - Flags: review?(ferjmoreno)
Assignee

Comment 18

5 years ago
Emulator M1 failure, will investigate.
Assignee

Comment 19

5 years ago
Posted patch b2g-addons.patch (obsolete) — Splinter Review
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.
Attachment #8490938 - Attachment is obsolete: true
Attachment #8528057 - Attachment is obsolete: true
Attachment #8533760 - Flags: review?(ferjmoreno)
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
Attachment #8533760 - Flags: feedback?(jonas)
Attachment #8533760 - Flags: feedback?(bobbyholley)
Attachment #8533760 - Flags: review?(ferjmoreno)
Attachment #8533760 - Flags: feedback?(amac.bug)
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.
Attachment #8533760 - Flags: feedback?(bobbyholley) → feedback+
"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.
Assignee

Comment 23

5 years ago
(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.
(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 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.
Attachment #8533760 - Flags: feedback+
Assignee

Comment 26

5 years ago
Posted patch patch v2Splinter Review
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.
Attachment #8533760 - Attachment is obsolete: true
Attachment #8533760 - Flags: feedback?(jonas)
Attachment #8533760 - Flags: feedback?(amac.bug)
Attachment #8536350 - Flags: review?(ferjmoreno)
Attachment #8536350 - Flags: review?(bobbyholley)
Comment on attachment 8536350 [details] [diff] [review]
patch v2

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

r=me on the subscript loader change.
Attachment #8536350 - Flags: review?(bobbyholley) → review+
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.
Attachment #8536350 - Flags: review?(ferjmoreno) → review+
Assignee

Updated

5 years ago
Depends on: 1112391
https://hg.mozilla.org/mozilla-central/rev/060e34d41972
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Assignee

Updated

5 years ago
Depends on: 1113016
Is this in fxos2.2?
Flags: needinfo?(fabrice)
Current trunk is 2.2, so yes, I think this is in 2.2.
Assignee

Comment 33

5 years ago
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.
Flags: needinfo?(fabrice)
Blocks: spark
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?
Assignee

Comment 35

4 years ago
Adrian, can you file a new bug? This one is closed.
Blocks: 1122979
Blocks: 1122981
Blocks: spark-addons
No longer blocks: spark
You need to log in before you can comment on or make changes to this bug.