819882 - Allow any app which has the open-remote-window permission to open a new OOP window

Status: RESOLVED WONTFIX

(Firefox OS Graveyard :: Gaia::System, defect)

Description

[Alexandre Poirot [:ochameau]]

Bug 807438 introduced a way to open out-of-process windows for the homescreen,
but it makes sense to allow any app to do so.
In order to do that, we need:
- register the new `open-remote-window` permission in gecko
- cleanup the feature argument that is currently hacked and way too different from standard behavior. It is currently a stringified JSON object like '["remote": true}' instead of 'remote=true'.

Comment 1

[Justin Lebar (not reading bugmail)]

FYI, the pull request [1] for bug 807438 bastardizes the window.open features argument.

Features is a comma-separated list of key=value pairs (spaces are probably OK too in the separator, but I'd have to check).  Features is not a stringified JSON object.

This is fine if you're only exposing the remote=true feature internally to Gecko.  But this bug is about exposing it elsewhere.  Before you do so, please fix the features bit so that it conforms to web conventions and standards.

[1] https://github.com/mozilla-b2g/gaia/pull/6764

Comment 2

[Olav Nymoen [:olav]]

Attachment: github_pointer

This patch changes e.me window opening in that everything is encoded / decoded with (en/de)codeURIComponenet. I couldn't see any issues, but I don't really know what to look for.

Comment 3

[Olav Nymoen [:olav]]

Attachment: 819882.patch

Apps of type 'app' and 'privileged' allowed to use 'open-remote-window' permission.

PS: First time looking at gecko.

Comment 4

[Alexandre Poirot [:ochameau]]

Comment on attachment 788944 [details] [review]
github_pointer

I don't feel confident reviewing that as I fade away from gaia.
Alive, can you review this patch, or find someone to do it.

Comment 5

[Justin Lebar (not reading bugmail)]

Can you guys please fix comment 1 as a precondition for this work?  We should not be exposing non-idiomatic args on window.open to a larger audience.

Comment 6

[Olav Nymoen [:olav]]

Justin Lebar: If I understand comment 1 correct you mean moving from a JSON formatted argument list in features to a '&' separated string of key value pairs conforming to the style of window.open.

This patch fixes that for window.open usage in everything.me and homescreen bookmarks - The two occurrences I found. However, it does not place any restrictions on what keys can be passed.

Comment 7

[(inactive after 6/18) Alive Kuo [:alive]]

(In reply to Justin Lebar [:jlebar] (limited availability 8/9 – 8/12) from comment #5)
> Can you guys please fix comment 1 as a precondition for this work?  We
> should not be exposing non-idiomatic args on window.open to a larger
> audience.

I think he already fix the JSON.parse to a new argument parser.

Comment 8

[(inactive after 6/18) Alive Kuo [:alive]]

Comment on attachment 788944 [details] [review]
github_pointer

Good clean, r=me, thanks.

Comment 9

[Olav Nymoen [:olav]]

Thanks, Alive!

Added r=alive

Added super small check to window_manager.js:1730 to make sure detail.features isn't undefined.

Also merged it.

Comment 10

[[:fabrice] Fabrice Desré]

Don't land the gecko part before we make a decision on letting non-certified apps use this permission. Jonas, Paul?

Comment 11

[Justin Lebar (not reading bugmail)]

>+    window.open(this.url, '_blank', Object.keys(features).map(function(key) {
>+      return encodeURIComponent(key) + '=' + encodeURIComponent(features[key]);
>+    }).join('&')); 

> Justin Lebar: If I understand comment 1 correct you mean moving from a JSON formatted argument list > in features to a '&' separated string of key value pairs conforming to the style of window.open.

Close.  It is comma-separated, not '&' separated.  See nsWindowWatcher.cpp:WinHasOption().

Comment 12

[Olav Nymoen [:olav]]

Attachment: github_pointer

Switched from & to , string separator. Both are encoded away from keys and values by encodeURIComponent so should be safe.

Comment 13

[(inactive after 6/18) Alive Kuo [:alive]]

Comment on attachment 790109 [details] [review]
github_pointer

r+, You still need to wait for platform permission check.

Comment 14

[[:fabrice] Fabrice Desré]

(In reply to Alive Kuo [:alive] from comment #13)
> Comment on attachment 790109 [details] [review]
> github_pointer
> 
> r+, You still need to wait for platform permission check.

I'm pretty sure we don't want that for the web at large, though this may be ok for privileged apps.

Comment 15

[(inactive after 6/18) Alive Kuo [:alive]]

(In reply to Fabrice Desré [:fabrice] from comment #14)
> (In reply to Alive Kuo [:alive] from comment #13)
> > Comment on attachment 790109 [details] [review]
> > github_pointer
> > 
> > r+, You still need to wait for platform permission check.
> 
> I'm pretty sure we don't want that for the web at large, though this may be
> ok for privileged apps.

I think the goal of this patch is to enable 3rd party homescreen app to work.

Comment 16

[Olav Nymoen [:olav]]

Also merged in the change from &-separated to ,-separated features string.

For Sicking or Theriault: The goal is to enable third party homescreens.

So far third party homescreens will rely on access to mozApps.mgmt (Bug 899994), System app managed bookmarks DataStore (Bug 898325) and ability to open OOP windows. If any of these needs to be privileged for now, then third party homescreens in general needs to be privileged only.

Comment 17

[Cristian Rodriguez (:crdlc)]

I am not completely sure but I guess that there is a regression with this patch bug 905447 comment 3

Comment 18

[Justin Lebar (not reading bugmail)]

This patch hasn't landed; have you been applying this patch yourself?

Comment 19

[Justin Lebar (not reading bugmail)]

Oh, I see; the Gaia bits have landed.  Usually we paste that into the bug...

Comment 20

[Cristian Rodriguez (:crdlc)]

I am not pretty sure sorry if I am wrong :( I am come back today and I am very lost jeje. Maybe the string is not truncated and is the console.log who is truncating my traces :)

https://github.com/mozilla-b2g/gaia/commit/d03070587c04b9a612020ee9854937fc85fa04ab

Comment 21

[Jason Smith [:jsmith]]

If this Gaia patch caused bug 905447, then this needs to be backed out. We cannot risk having smoketest regressions in Gaia for more than a day. Can someone confirm this Gaia patch caused bug 905447?

Comment 22

[Cristian Rodriguez (:crdlc)]

OK, you can forget my truncating issue, it was due to console.lot. The regression is this one

https://github.com/mozilla-b2g/gaia/commit/d03070587c04b9a612020ee9854937fc85fa04ab#L2R1802

if (originName in features) instead of if ('originName' in features) {

and https://github.com/mozilla-b2g/gaia/commit/d03070587c04b9a612020ee9854937fc85fa04ab#L2R1807

if (searchName in features) instead of if ('searchName' in features)

and the same for if (useAsyncPanZoom in features

Comment 23

[Cristian Rodriguez (:crdlc)]

the patch is pretty easy

diff --git a/apps/system/js/window_manager.js b/apps/system/js/window_manager.js
index 73a3021..28decd3 100644
--- a/apps/system/js/window_manager.js
+++ b/apps/system/js/window_manager.js
@@ -1707,7 +1707,7 @@ var WindowManager = (function() {
       }, {});
 
     // Handles only call to window.open with `remote=true` feature.
-    if (!remote in features || features.remote !== 'true')
+    if (!'remote' in features || features.remote !== 'true')
       return;
 
     var callerIframe = evt.target;
@@ -1761,17 +1761,17 @@ var WindowManager = (function() {
     title = features.name || url;
     icon = features.icon || '';
 
-    if (originName in features) {
+    if ('originName' in features) {
       originName = features.originName;
       originURL = features.originUrl;
     }
 
-    if (searchName in features) {
+    if ('searchName' in features) {
       searchName = features.searchName;
       searchURL = features.searchUrl;
     }
 
-    if (useAsyncPanZoom in features && features.useAsyncPanZoom === 'true')
+    if ('useAsyncPanZoom' in features && features.useAsyncPanZoom === 'true')
       useAsyncPanZoom = true;
 
     // If we don't reuse an existing app, open a brand new one
@@ -2019,4 +2019,3 @@ var WindowManager = (function() {
     screenshots: screenshots
   };
 }());

Comment 24

[Justin Lebar (not reading bugmail)]

It would be helpful if you could file a separate bug, which is marked as blocking the bug which caused the regression.  A pull request, plus a pointer in the bug to the PR, plus a r? on someone (alive, maybe?), would help us get this in even faster.

Comment 25

[Cristian Rodriguez (:crdlc)]

for sure

Comment 26

[Alexandre Poirot [:ochameau]]

Comment on attachment 788947 [details] [diff] [review]
819882.patch

As for gaia patch, I don't feel being the right reviewer for this patch.
I think fabrice is a good candidate, feel free to forward to whoever makes sense.

Comment 27

[[:fabrice] Fabrice Desré]

I'll do a formal review once Jonas and/or Paul chime in.

Comment 28

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

(In reply to Fabrice Desré [:fabrice] from comment #27)
> I'll do a formal review once Jonas and/or Paul chime in.

For security folks:

This permission will basically let privileged applications open multiple <iframe mozbrowser> that will be visible in the card switcher and that would use the system app look and feel for window.[alert|confirm|prompt] that is use for regular apps for now. There is no additional privileges involved here, just look and feel closer to regular apps (no url bar for example).

I think the calls to window.open for those remote iframes can be done without any user interaction for now and I won't be against adding that as a limitation.

Also those <iframe>s runs as <iframe mozbrowser remote> as direct child of the system app (<iframe mozbrowser mozapp>).

It means that each of those <iframe>s will share the same cookie jar and so if you logged into facebook.com in one of those, then you will be loaded in facebook.com for all those iframes. You won't be logged in any other apps.

Some are also planning to move the browser app back into the system app. As a result web pages and <iframe>s opened this way will share the same cookie jar again.

Also I think the identity/pay iframes share the same cookie jar as well.

I think we already discussed that at some point back in october 2012 but I don't remember any bugs number. One of the ideas was to let the system app beeing able to specify a data jar. That would offer us the possibility to isolate those if needed, which should likely be mostly for identity/pay iframes afaict.


Lastly I don't think this permission is useful for anyone else except an homescreen application so I will personaly limit it to homescreen only by looking at the |role| attribute.See bug 912340


To summary I think we really want this feature, otherwise it will be hard to have a competitive third party homescreen that is not certified but, some additional precautions can be added before exposing it to the whole |privileged| world. My 2 cents.

Comment 29

[[:fabrice] Fabrice Desré]

(In reply to Vivien Nicolas (:vingtetun) (:21) from comment #28)

> To summary I think we really want this feature, otherwise it will be hard to
> have a competitive third party homescreen that is not certified but, some
> additional precautions can be added before exposing it to the whole
> |privileged| world. My 2 cents.

I agree, but the current patch does not limit the permission to privileged apps, but also grant it to unprivileged ones. That may be ok, or not :)

Comment 30

[Olav Nymoen [:olav]]

> Bug 807438 introduced a way to open out-of-process windows for the homescreen, but it makes sense to allow any app to do so.

> I agree, but the current patch does not limit the permission to privileged apps, but also grant it to unprivileged ones. That may be ok, or not :)

Yea, I took the 'any' literally, but I guess we've given up on having unprivileged homescreens anyway, right?

Comment 31

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

(In reply to Olav Nymoen [:olav] from comment #30)
> > Bug 807438 introduced a way to open out-of-process windows for the homescreen, but it makes sense to allow any app to do so.
> 
> > I agree, but the current patch does not limit the permission to privileged apps, but also grant it to unprivileged ones. That may be ok, or not :)
> 

I'm not saying the current patch works. I would like to figure out the data jar issue first, and then to restrict the permission to homescreen only. I'm a bit less worried about the phishing issue that can be involved since the homescreen can decide if there is a urlbar or not :)

> Yea, I took the 'any' literally, but I guess we've given up on having
> unprivileged homescreens anyway, right?

For now I will be more comfortable with privileged homescreen only. I have no opinion if this needs to change in the future or not.

Comment 32

[Olav Nymoen [:olav]]

Attachment: Edit permissions table to allow privileged apps to open windows out of process.

Comment 33

[Olav Nymoen [:olav]]

Attachment: Privileged app needs role=homescreen to open OOP window

Comment 34

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

Comment on attachment 803741 [details] [review]
Privileged app needs role=homescreen to open OOP window

I will rather prefer to enforce this security at the Gecko level somewhere in https://github.com/mozilla-b2g/gaia/pull/12173

Comment 35

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

Comment on attachment 803738 [details] [diff] [review]
Edit permissions table to allow privileged apps to open windows out of process.

I don't think I can review changes in this file. I also think the |role| should be declared somewhere in this file as well and used wisely.

Comment 36

[Paul Theriault [:pauljt] (no longer reading bugmail)]

(In reply to Vivien Nicolas (:vingtetun) (:21) from comment #34)
> Comment on attachment 803741 [details] [review]
> Privileged app needs role=homescreen to open OOP window
> 
> I will rather prefer to enforce this security at the Gecko level somewhere
> in https://github.com/mozilla-b2g/gaia/pull/12173

I am a little concerned about using the role for anything security related. I mean, how you are doing it here is ok, but by doing this we are implying that the marketplace reviewers should be checking role/permission combinations. Adding complexity to this seems like a recipe to failure. I'd like to have a discussion around the role attribute so that we can have a clear design what it should and shoulnd't be used for. 

The other issue is that ANY app can get ANY role, where as for permissions we at least enforce a model that you need to be a certain type to get a certain permission. If we need roles (and it sounds like we do) then when we enforce this in gecko, we should be enforcing restrictions such as 'homescreen' is privileged only. 

Which is maybe what you meant by "enforce this security at the Gecko level" but I just want to understand exactly what you had in mind.

Comment 37

[[:fabrice] Fabrice Desré]

It seems to me that binding permissions and roles to grant access is not giving us anything but additional complexity and potential confusion for app devs and marketplace reviewers. I expected them to only be used for other purposes in gaia.

Comment 38

[Olav Nymoen [:olav]]

> Lastly I don't think this permission is useful for anyone else except an homescreen application so I will personaly limit it to homescreen only by looking at the |role| attribute.See bug 912340

> I am a little concerned about using the role for anything security related.

> It seems to me that binding permissions and roles to grant access is not giving
us anything but additional complexity and potential confusion for app devs and
marketplace reviewers.

How about just going with the original proposal: Having the explicit permission and restricting it to privileged app. Seems like a direct way of enforcing this functionality.

With the Haida sheets (awesome!) it seems like window.open(,,'remote=false') now opens without browser chrome and even displays the new windows in the task switcher! I dunno if that will affect this in any way.

Comment 39

[(no longer active)]

My suggestion here is to make the remote=true feature argument to window.open() available to privileged apps which have a specific permission (called "open-remote-window" or something else) and not tie this to role:homescreen, so that people can for example revoke the permission if they want to, etc.

We should ignore remote=true if the caller app does not have the said permission, and just fall back to the usual way of opening windows.

Gene, does this look good for the purposes of the Stingray project?

Comment 40

[Gene Lian (I already quit Mozilla)]

(In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment #39)
> My suggestion here is to make the remote=true feature argument to
> window.open() available to privileged apps which have a specific permission
> (called "open-remote-window" or something else) and not tie this to
> role:homescreen, so that people can for example revoke the permission if
> they want to, etc.

Yeap, that makes sense to me. "role:homescreen" doesn't sound right because we shouldn't limit this to Homescreen App only. Right? Also, adding a permission is easier to control which apps are allowed to call window.open() remotely based on our current permission model. In the initiative step, we should allow the privileged apps only instead of any kinds of Web apps.

I just looked through the patches. It seems we only put the "role:homescreen" check in Gaia and still rely on the "open-remote-window" permission check in Gecko. Then, that's exactly match what we desire here from the point of view of Gecko, which is good.

> 
> We should ignore remote=true if the caller app does not have the said
> permission, and just fall back to the usual way of opening windows.

Do you mean it can still open window in-process (i.e. non-remotely) as a fall back? IMO, I'd prefer throwing exceptions or running failed event handler to explicitly notify developers about the unpermitted attempt. It sounds weird to me developers hope to open the window remotely but it's actually not and developers are not aware of that...

If you agree with the stronger error handling, we need to have more work on the current Gecko patch.

> Gene, does this look good for the purposes of the Stingray project?

Yes, it sounds good to me. Needinfo Gaia side to double check.

Comment 41

[GaryChen [:GaryChen][:PYChen][:gchen][:陳柏宇]]

(In reply to Gene Lian [:gene] (needinfo? encouraged) from comment #40)
> (In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment
> #39)
> > My suggestion here is to make the remote=true feature argument to
> > window.open() available to privileged apps which have a specific permission
> > (called "open-remote-window" or something else) and not tie this to
> > role:homescreen, so that people can for example revoke the permission if
> > they want to, etc.
> 
> Yeap, that makes sense to me. "role:homescreen" doesn't sound right because
> we shouldn't limit this to Homescreen App only. Right? Also, adding a
> permission is easier to control which apps are allowed to call window.open()
> remotely based on our current permission model. In the initiative step, we
> should allow the privileged apps only instead of any kinds of Web apps.
> 

> I just looked through the patches. It seems we only put the
> "role:homescreen" check in Gaia and still rely on the "open-remote-window"
> permission check in Gecko. Then, that's exactly match what we desire here
> from the point of view of Gecko, which is good.
> 
> > 
> > We should ignore remote=true if the caller app does not have the said
> > permission, and just fall back to the usual way of opening windows.
> 
> Do you mean it can still open window in-process (i.e. non-remotely) as a
> fall back? IMO, I'd prefer throwing exceptions or running failed event
> handler to explicitly notify developers about the unpermitted attempt. It
> sounds weird to me developers hope to open the window remotely but it's
> actually not and developers are not aware of that...
>
I agree Gene's opinion.
As a developer, we want to know what happened in platform when the behavior does not we expected.
> If you agree with the stronger error handling, we need to have more work on
> the current Gecko patch.
> 
> > Gene, does this look good for the purposes of the Stingray project?
> 
> Yes, it sounds good to me. Needinfo Gaia side to double check.


Overall is good to me, thanks.

Comment 42

[(no longer active)]

(Sorry I missed this, *please* use needinfo? to make sure I actually get back to you on time...)

(In reply to Gene Lian [:gene] (needinfo? encouraged) from comment #40)
> (In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment
> #39)
> > My suggestion here is to make the remote=true feature argument to
> > window.open() available to privileged apps which have a specific permission
> > (called "open-remote-window" or something else) and not tie this to
> > role:homescreen, so that people can for example revoke the permission if
> > they want to, etc.
> 
> Yeap, that makes sense to me. "role:homescreen" doesn't sound right because
> we shouldn't limit this to Homescreen App only. Right?

Yep.

> Also, adding a
> permission is easier to control which apps are allowed to call window.open()
> remotely based on our current permission model. In the initiative step, we
> should allow the privileged apps only instead of any kinds of Web apps.
> 
> I just looked through the patches. It seems we only put the
> "role:homescreen" check in Gaia and still rely on the "open-remote-window"
> permission check in Gecko. Then, that's exactly match what we desire here
> from the point of view of Gecko, which is good.

\o/

> > We should ignore remote=true if the caller app does not have the said
> > permission, and just fall back to the usual way of opening windows.
> 
> Do you mean it can still open window in-process (i.e. non-remotely) as a
> fall back? IMO, I'd prefer throwing exceptions or running failed event
> handler to explicitly notify developers about the unpermitted attempt. It
> sounds weird to me developers hope to open the window remotely but it's
> actually not and developers are not aware of that...
> 
> If you agree with the stronger error handling, we need to have more work on
> the current Gecko patch.

Hmm.  Right now we don't throw for any of these feature arguments that we ignore, and that's pretty much common knowledge in this API.  It's OK if you want to log to the devtools console here but this is all really a hack and you pretty much need to know exactly what you're doing anyways, so I don't think there is a good case for throwing here.

Comment 43

[(no longer active)]

Note that my "OK" here is for the TV project only, not the general idea.  :-)

Comment 44

[Paul Theriault [:pauljt] (no longer reading bugmail)]

(In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment #39)
> My suggestion here is to make the remote=true feature argument to
> window.open() available to privileged apps which have a specific permission
> (called "open-remote-window" or something else) and not tie this to
> role:homescreen, so that people can for example revoke the permission if
> they want to, etc.

Note that a users can only revoke permissions they are prompted about. I don't think prompting is appropriate for this permission, so users won't be able to change this (it won't show up in the settings app). We could change this, but that's a separate bug I think - I'll consider this as part of broader security model changes for 2.0 as this has been asked for in other areas.

Looking at this from a security review perspective, I think the main risk here is app spawning too many processes, causing the phone to be unstable. We'll have to update reviewer training to include looking for this.But apart from that, this approach sounds ok to me. 

I assume this is just a work in progess but there are two security issues I see with the current check as it stand in attachment 790109 [details] [review]:

1. Any webpage can set a mozapp attribute, so this isn't a valid security check

We do this check:
var manifestURL = callerIframe.getAttribute('mozapp');

Although mozapp attribute doesn't do anything unless the embedding page has the 'embed-apps' permission, any webpage can set the mozapp attribute on any iframe to whatever it wants. Since the gaia app origins are wellknown, any page could pretend to be embedded inside any app if we check this way.


2. We need to check the page origin, in addition to checking the containing app (or do this check in gecko?)

It's a little weird for me that the permission check here is in gaia at all - and that the check is on the App which calls window.open, rather than on checking the origin of the page which calls window.open. I'm concerned that this will lead to a security issue, but I could be wrong about this. Consider the following scenario:

1. We have a new homescreen installed which for some reason allow its window to be navigated
2. This homescreen has a link which navigates to some other site, evil.com
3. The page at evil.com call window.open(...{remote=true})
4. The system app check figures out what App made the call by looking at mozApp attribute of the .parentNode frame. It does NOT appear to check that the origin of the request actually matches the origin of the app. 

We then pull the caller origin of the app [1]. Maybe this is deliberate, but the name is confusing, because again, afaict, the origin which called window.open is not guaranteed to be that of the app itself. 

So it feels like at the very least we should be checking that the page that calls window.open is same-origin with the app, or maybe move this permission check to gecko, as it is with other permissions.

[1] https://github.com/comoyo/gaia/blob/38e653f214349b00f00c4aac90e908940c9a9de3/apps/system/js/window_manager.js#L1758

Comment 45

[Paul Theriault [:pauljt] (no longer reading bugmail)]

> 1. Any webpage can set a mozapp attribute, so this isn't a valid security
> check
> 
> We do this check:
> var manifestURL = callerIframe.getAttribute('mozapp');
> 
> Although mozapp attribute doesn't do anything unless the embedding page has
> the 'embed-apps' permission, any webpage can set the mozapp attribute on any
> iframe to whatever it wants. Since the gaia app origins are wellknown, any
> page could pretend to be embedded inside any app if we check this way.

Actually I might be confusing myself here. evt.target is the browser frame inside which the window.open was called - correct? So callerIframe is always the iframe in the system app, not some nested iframe inside the caller app (which wouldnt work anyways since its another process... derp).
So you can ignore this first issue. 

But the second one still stands I think. (unless window.open doesn't result in a mozbrowserwindowopen event being raised in the system for the case when the app has navigated away from it packaged origin...)

Comment 46

[Gregor Wagner [:gwagner]]

Needed for privileged home-screen

Comment 47

[Jason Smith [:jsmith]]

This shouldn't block - this is a feature, which is something we won't block on unless we're past FL & we're planning to still land the feature.

Comment 48

[Chris Lord [:cwiiis]]

Privileged homescreens are now allowed to open remote windows when they have the homescreen-webapps-manage permission, removing dependent bug.

Comment 49

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Clearing out old requests...

Is this still relevant? It's not clear to me what the goal is here UX-wise.

Comment 50

[BMO Automation]

Firefox OS is not being worked on