Closed Bug 819882 Opened 7 years ago Closed 2 years ago

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

Categories

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

defect
Not set

Tracking

(tracking-b2g:backlog)

RESOLVED WONTFIX
tracking-b2g backlog

People

(Reporter: ochameau, Assigned: olav)

References

Details

Attachments

(3 files, 2 obsolete files)

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'.
Depends on: 819884
Blocks: 807438
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
Component: Gaia → Gaia::System
Summary: Allows any app to open OOP new window throug open-remote-window permission → Allows any app to open OOP new window through open-remote-window permission
Summary: Allows any app to open OOP new window through open-remote-window permission → Allow any app which has the open-remote-window permission to open a new OOP window
Attached file github_pointer (obsolete) —
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.
Attachment #788944 - Flags: review?(poirot.alex)
Attached patch 819882.patch (obsolete) — Splinter Review
Apps of type 'app' and 'privileged' allowed to use 'open-remote-window' permission.

PS: First time looking at gecko.
Attachment #788947 - Flags: review?(poirot.alex)
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.
Attachment #788944 - Flags: review?(poirot.alex) → review?(alive)
Assignee: nobody → olav
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.
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.
(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 on attachment 788944 [details] [review]
github_pointer

Good clean, r=me, thanks.
Attachment #788944 - Flags: review?(alive) → review+
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.
Don't land the gecko part before we make a decision on letting non-certified apps use this permission. Jonas, Paul?
Flags: needinfo?(ptheriault)
Flags: needinfo?(jonas)
>+    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().
Attached file github_pointer
Switched from & to , string separator. Both are encoded away from keys and values by encodeURIComponent so should be safe.
Attachment #790109 - Flags: review?(alive)
Comment on attachment 790109 [details] [review]
github_pointer

r+, You still need to wait for platform permission check.
Attachment #790109 - Flags: review?(alive) → review+
Attachment #788944 - Attachment is obsolete: true
(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.
(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.
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.
I am not completely sure but I guess that there is a regression with this patch bug 905447 comment 3
This patch hasn't landed; have you been applying this patch yourself?
Oh, I see; the Gaia bits have landed.  Usually we paste that into the bug...
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
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?
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
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
   };
 }());
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.
for sure
Depends on: 905447
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.
Attachment #788947 - Flags: review?(poirot.alex) → review?(fabrice)
I'll do a formal review once Jonas and/or Paul chime in.
(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.
Depends on: 912340
(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 :)
> 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?
(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.
Attachment #788947 - Attachment is obsolete: true
Attachment #788947 - Flags: review?(fabrice)
Attachment #803738 - Flags: review?(21)
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
Attachment #803741 - Flags: review?(21) → review-
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.
Attachment #803738 - Flags: review?(21) → review?(fabrice)
(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.
Flags: needinfo?(ptheriault)
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.
> 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.
Attachment mime type: text/plain → text/x-github-pull-request
Attachment mime type: text/plain → text/x-github-pull-request
Attachment #803738 - Flags: review?(fabrice)
Flags: needinfo?(ehsan)
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?
Flags: needinfo?(ehsan) → needinfo?(gene.lian)
(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.
Flags: needinfo?(gchen)
Blocks: 983001
Flags: needinfo?(gene.lian)
(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.
Flags: needinfo?(gchen)
No longer blocks: 983001
Blocks: 980768
(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.
Note that my "OK" here is for the TV project only, not the general idea.  :-)
(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
> 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...)
Needed for privileged home-screen
blocking-b2g: --- → 2.0+
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.
blocking-b2g: 2.0+ → backlog
No longer blocks: vertical-homescreen
blocking-b2g: backlog → ---
Privileged homescreens are now allowed to open remote windows when they have the homescreen-webapps-manage permission, removing dependent bug.
No longer blocks: homescreen-apps
Blocks: TV_FxOS2.5
No longer blocks: TV_FxOS2.5
Clearing out old requests...

Is this still relevant? It's not clear to me what the goal is here UX-wise.
Flags: needinfo?(jonas)
Firefox OS is not being worked on
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.