777135 - Nix windowUtils::setApp and friends; convert users to principal methods

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Justin Lebar (not reading bugmail)]

We don't currently have tests for <iframe mozapp>, which is why this was missed.

Mounir, what are we supposed to do here?  I can't keep track of all the changes we've made.  It's Very Bad that this code is broken right now!

Comment 1

[Mounir Lamouri (:mounir)]

If we can change screen orientation and fullscreen to use a better security model, we could remove that.
The better security model would be:
- use principal to know if in an installed app, instead of using this hack;
- use permission to know if the action is allowed, instead of using this hack.

The former would be very easy to do. The later a bit more complex but we might need that in the long run.

Comment 2

[Justin Lebar (not reading bugmail)]

I'm in the process of ripping out the code on the window and converting everything to calling the new docshell methods.  Those should be right OOP, right?

Comment 3

[Mounir Lamouri (:mounir)]

(In reply to Justin Lebar [:jlebar] (jury duty 7/25) from comment #2)
> I'm in the process of ripping out the code on the window and converting
> everything to calling the new docshell methods.  Those should be right OOP,
> right?

Yes. But I would prefer to use nsIPrincipal.appStatus if you want to go with solution 1. AppStatus != NON_INSTALLED should be equivalent as what we are checking right now.

Comment 4

[Justin Lebar (not reading bugmail)]

Attachment: Part 1: Add nsIPrincipal::IsApp.

This doesn't need a review by sicking and mounir; I'd just prefer that this get done sooner rather than later to ameliorate the confusion it's causing.  Can whoever starts reviewing first cancel the other review?

Comment 5

[Justin Lebar (not reading bugmail)]

Attachment: Bug 777135 - Part 2: Stop using nsDOMWindowUtils::GetIsApp and friends, and instead use the relevant methods on the principal.

Comment 6

[Justin Lebar (not reading bugmail)]

Attachment: Bug 777135 - Part 3: Remove nsDOMWindowUtils::GetIsApp and friends.

Comment 7

[Mounir Lamouri (:mounir)]

Comment on attachment 646281 [details] [diff] [review]
Part 1: Add nsIPrincipal::IsApp.

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

Given that checking if a principal is part of an application is a hack for the moment, I would prefer not to add a method in nsIPrincipal to do that. Ideally, we should use permissions and give those to installed apps instead of allowing some calls to iframe inside installed apps.

Comment 8

[Mounir Lamouri (:mounir)]

Comment on attachment 646282 [details] [diff] [review]
Bug 777135 - Part 2: Stop using nsDOMWindowUtils::GetIsApp and friends, and instead use the relevant methods on the principal.

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

Looks good but I would like to see the updated patch.

::: content/base/src/nsDocument.cpp
@@ +9014,5 @@
>    // check to see if the user has granted the fullscreen access
>    // to the document's principal's host, if it has one.
>    if (!mIsApprovedForFullscreen) {
>      mIsApprovedForFullscreen =
> +      NodePrincipal()->IsApp() ||

Use AppType and add a TODO that says we should use a permission for that.

::: content/base/src/nsGenericElement.cpp
@@ +4760,5 @@
>  
>  static const char*
>  GetFullScreenError(nsIDocument* aDoc)
>  {
> +  if (aDoc->NodePrincipal()->IsApp()) {

ditto

::: docshell/base/nsIDocShell.idl
@@ +611,5 @@
> +   * Returns true iif the docshell is inside an application.  However, it will
> +   * return false if the docshell is inside a browser element that is inside
> +   * an application.
> +   *
> +   * Note: Do not use this method for permissions checks!  An app may contain

... consumers should use the permission manager for permissions checks anyway.

@@ +622,5 @@
> +   * case, it would be /incorrect/ to show the permission prompt if
> +   * !isInApp().)
> +   *
> +   * If you're doing a security check, use the content's principal instead of
> +   * this method.

Principal's will not return anything different. Permission manager should be used ideally.

::: dom/base/nsGlobalWindow.cpp
@@ +9147,5 @@
>  
> +  // Popups from apps are never blocked.
> +  bool isApp = false;
> +  if (mDoc) {
> +    isApp = mDoc->NodePrincipal()->IsApp();

As previously, should use AppStatus.

Comment 9

[Mounir Lamouri (:mounir)]

Comment on attachment 646283 [details] [diff] [review]
Bug 777135 - Part 3: Remove nsDOMWindowUtils::GetIsApp and friends.

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

r- because you are breaking some code, see:
https://mxr.mozilla.org/mozilla-central/ident?i=getApp&tree=mozilla-central&filter=&strict=1

Comment 10

[Justin Lebar (not reading bugmail)]

> r- because you are breaking some code, see:
> https://mxr.mozilla.org/mozilla-central/ident?i=getApp&tree=mozilla-central&filter=&strict=1

I think it was bitrot.  In other words, r- for not reviewing quickly enough on my vacation.  :)

Comment 11

[Justin Lebar (not reading bugmail)]

(In reply to Mounir Lamouri (:mounir) (on VACATION until August 5th) from comment #9)
> https://mxr.mozilla.org/mozilla-central/ident?i=getApp&tree=mozilla-
> central&filter=&strict=1

Oh, nevermind.  I didn't even open this link.  That was pretty dumb of me.  :(

Comment 12

[Justin Lebar (not reading bugmail)]

Do you have a preference between:

 1. changing these files to use the appId instead of the manifest URL (a non-trivial change at least to the alarm service, which is doing something, I'm not sure what, with the URL as a URL)
 2. adding a function to go from appId --> app manifest URL to nsIAppsService
 3. adding a function to go from appId --> app to nsIAppsService
 4. adding a function to nsIPrincipal to get the manifest URL

?

Comment 13

[Justin Lebar (not reading bugmail)]

Attachment: Part 1, v2: Add nice C++ version of nsIPrincipal::GetAppStatus.

Comment 14

[Justin Lebar (not reading bugmail)]

Attachment: Part 2, v2: Stop using nsDOMWindowUtils::GetIsApp and friends, and instead use the relevant methods on the principal.

We'll still need a part 2a which fixes the JS callers you identified.

Comment 15

[Justin Lebar (not reading bugmail)]

Attachment: Part 3, v2: Remove nsDOMWindowUtils::GetIsApp and friends.

Comment 16

[Mounir Lamouri (:mounir)]

Comment on attachment 649183 [details] [diff] [review]
Part 1, v2: Add nice C++ version of nsIPrincipal::GetAppStatus.

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

nsExpandedPrincipal doesn't return NS_OK...

Comment 17

[Justin Lebar (not reading bugmail)]

(In reply to Mounir Lamouri (:mounir) (on VACATION until August 5th) from comment #16)
> nsExpandedPrincipal doesn't return NS_OK...

But if an implementation doesn't return NS_OK, we should return 0 for GetAppStatus, right?

Comment 18

[Justin Lebar (not reading bugmail)]

(In reply to Justin Lebar [:jlebar] from comment #17)
> But if an implementation doesn't return NS_OK, we should return 0 for
> GetAppStatus, right?

I can check for an error code returned, but assuming that the implementation either sets the out value to 0 or doesn't touch it on error (these seem like the only sane options), the net result should be the same.

Comment 19

[Justin Lebar (not reading bugmail)]

> 3. adding a function to go from appId --> app to nsIAppsService

This is bug 779935, so I guess if you r+ that patch, I'll take this approach.

Comment 20

[Justin Lebar (not reading bugmail)]

Gah, complete Bugzilla fail.  Sorry for the spam.  :(

Comment 21

[Justin Lebar (not reading bugmail)]

Attachment: Part 1, v3: Add nice C++ version of nsIPrincipal::GetAppStatus.

Comment 22

[Mounir Lamouri (:mounir)]

Comment on attachment 649470 [details] [diff] [review]
Part 1, v3: Add nice C++ version of nsIPrincipal::GetAppStatus.

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

r=me with the comments below fixed.

::: caps/idl/nsIPrincipal.idl
@@ +247,5 @@
> +      nsresult rv = GetAppStatus(&appStatus);
> +      if (NS_SUCCEEDED(rv)) {
> +        return appStatus;
> +      }
> +      return APP_STATUS_NOT_INSTALLED;

I would prefer the opposite: if (NS_FAILED(rv)) { ...

Comment 23

[Mounir Lamouri (:mounir)]

Comment on attachment 649184 [details] [diff] [review]
Part 2, v2: Stop using nsDOMWindowUtils::GetIsApp and friends, and instead use the relevant methods on the principal.

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

I think we should make sure to open follow ups to replace appStatus usage with permissions in most places. Using appStatus isn't really secure.

I r-'d this patch because I don't like the change in nsScreen. Having a CanLockOrientation() method that actually checks if the orientation can be locked but also register a listener in some cases seem error-prone.

::: content/base/src/nsDocument.cpp
@@ +9112,5 @@
>    // in web apps which are the same origin as the web app are considered
>    // trusted and so are automatically approved.
>    if (!mIsApprovedForFullscreen) {
>      mIsApprovedForFullscreen =
> +      NodePrincipal()->GetAppStatus() >= nsIPrincipal::APP_STATUS_INSTALLED ||

That scares me a bit but I guess between >= and != NOT_INSTALLED, >= might be nice.

::: dom/base/nsGlobalWindow.cpp
@@ +9147,5 @@
>    }
>  
>    NS_ASSERTION(mDocShell, "Must have docshell here");
>  
> +  // Popups from apps are never blocked.

We should really use permissions here...

Comment 24

[Mounir Lamouri (:mounir)]

(In reply to Justin Lebar [:jlebar] from comment #19)
> > 3. adding a function to go from appId --> app to nsIAppsService
> 
> This is bug 779935, so I guess if you r+ that patch, I'll take this approach.

I was thinking about something like that, indeed. appId -> manifestURL would be enough though ;)

Comment 25

[Mounir Lamouri (:mounir)]

Comment on attachment 649185 [details] [diff] [review]
Part 3, v2: Remove nsDOMWindowUtils::GetIsApp and friends.

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

Cancelling the review because you didn't update the js callers.

Comment 26

[Justin Lebar (not reading bugmail)]

> I think we should make sure to open follow ups to replace appStatus usage with permissions in most 
> places. Using appStatus isn't really secure.

I'm not convinced that the permission manager, at least in its current state, is powerful enough to enforce the conditions we want.

For example, any app can lock the screen orientation.  If you're not an app, you can lock the screen orientation only if you're in full-screen.  Oh, and as soon as you leave full-screen, the orientation lock must die.

It therefore doesn't make sense to me to have a "can-implicitly-go-full-screen" permission.  If you're not comfortable using appStatus, you should think about how to implement complex, domain-specific permissions checks without it.  :)

Comment 27

[Justin Lebar (not reading bugmail)]

Mounir, can I land the code removal in BrowserElementChild.js, which is currently breaking every app frame?  It looks like I'm blocked here until we finish bug 779935.

Comment 28

[Justin Lebar (not reading bugmail)]

Comment on attachment 649184 [details] [diff] [review]
Part 2, v2: Stop using nsDOMWindowUtils::GetIsApp and friends, and instead use the relevant methods on the principal.

r? just on the BrowserElementChild.js code removal, which is blocking all sorts of stuff.

Comment 29

[Mounir Lamouri (:mounir)]

Comment on attachment 649184 [details] [diff] [review]
Part 2, v2: Stop using nsDOMWindowUtils::GetIsApp and friends, and instead use the relevant methods on the principal.

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

Let's land this whith all callers fixed so we don't create new breakage. We can afford 24 more hours with that known bug.

Comment 30

[Justin Lebar (not reading bugmail)]

> I r-'d this patch because I don't like the change in nsScreen.

You're going to love my alternative.  You just wait.  :)

Comment 31

[Justin Lebar (not reading bugmail)]

Attachment: Part 2, v3: Stop using nsDOMWindowUtils::GetIsApp and friends, and instead use the relevant methods on the principal.

Comment 32

[Justin Lebar (not reading bugmail)]

https://tbpl.mozilla.org/?tree=Try&rev=358522df4f4b

Comment 33

[Mounir Lamouri (:mounir)]

Comment on attachment 650687 [details] [diff] [review]
Part 2, v3: Stop using nsDOMWindowUtils::GetIsApp and friends, and instead use the relevant methods on the principal.

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

Cancelling the review because you were assuming stuff that are not correct AFAIK. I would be okay to r+ a similar patch if we agree to fix the other issues in follow-ups.

::: content/base/src/nsGenericElement.cpp
@@ +2793,5 @@
>  static const char*
>  GetFullScreenError(nsIDocument* aDoc)
>  {
>    nsCOMPtr<nsPIDOMWindow> win = aDoc->GetWindow();
> +  if (aDoc->NodePrincipal()->GetAppStatus() >= nsIPrincipal::APP_STATUS_INSTALLED) {

Isn't that different from IsInAppOrigin()? Basically, we are going to all <iframe src='example.com'> to use fullscreen when inserted in <iframe mozapp='http://myapp.com/manifest.webapp'>.
Using permission would prevent that because the app + the origin would be used.

::: docshell/base/nsIDocShell.idl
@@ +622,5 @@
> +   * case, it would be /incorrect/ to show the permission prompt if
> +   * !isInApp().)
> +   *
> +   * If you're doing a security check, use the content's principal instead of
> +   * this method.

Sorry, I should have seen that before: there is a misunderstanding here. Principal and docshell will say the same thing. Maybe we should change that?

::: dom/base/nsScreen.cpp
@@ +376,4 @@
>      }
>  
> +    if (doc->NodePrincipal()->GetAppStatus() >=
> +          nsIPrincipal::APP_STATUS_INSTALLED) {

It seems that you are adding a lot of noise in that patch just to prevent GetOwner() calls, right? I don't think that's needed (GetOwner() is fast) and that makes things hard to understand. Could you just add the appStatus call and maybe a different patch if you think that code is too ugly.

Comment 34

[Justin Lebar (not reading bugmail)]

> It seems that you are adding a lot of noise in that patch just to prevent GetOwner() calls, right?

The problem is that the logic for when we allow or don't allow full-screen is highly nested.  If I don't do it in a separate function or a loop I can break out of, I'd create an unreadable tree of nested if's, and I do not want to do that.  Is that OK with you?

> Sorry, I should have seen that before: there is a misunderstanding here. Principal and docshell will 
> say the same thing.

We established on IRC that this is a bug and we're going to change it.  So I think we can land this code as-is, and then it will magically work properly when nsIPrincipal::AppStatus is correct.

Comment 35

[Justin Lebar (not reading bugmail)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/22d637d39566
https://hg.mozilla.org/integration/mozilla-inbound/rev/dcb9d2f694eb

(I folded parts 2, 2a, and 3 together.)

Thanks a lot for the review, bz.  \o/

Comment 36

[Justin Lebar (not reading bugmail)]

Comment on attachment 650687 [details] [diff] [review]
Part 2, v3: Stop using nsDOMWindowUtils::GetIsApp and friends, and instead use the relevant methods on the principal.

Re-flagging r? on this, per IRC discussion.

Comment 37

[Justin Lebar (not reading bugmail)]

> https://hg.mozilla.org/integration/mozilla-inbound/rev/22d637d39566
> https://hg.mozilla.org/integration/mozilla-inbound/rev/dcb9d2f694eb

Oops, wrong bug.  :(

Comment 38

[Mounir Lamouri (:mounir)]

Comment on attachment 650687 [details] [diff] [review]
Part 2, v3: Stop using nsDOMWindowUtils::GetIsApp and friends, and instead use the relevant methods on the principal.

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

::: dom/base/nsScreen.cpp
@@ +366,5 @@
> +      canLockOrientation = true;
> +      break;
> +    }
> +
> +    // Apps can always lock the screen orientation.

nit: can you move that comment to the block below?

Comment 39

[Justin Lebar (not reading bugmail)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/58910754c255
https://hg.mozilla.org/integration/mozilla-inbound/rev/3a51e992d7f1
https://hg.mozilla.org/integration/mozilla-inbound/rev/356689768e3f

Thanks a lot, Mounir.

Comment 40

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 650687 [details] [diff] [review]
Part 2, v3: Stop using nsDOMWindowUtils::GetIsApp and friends, and instead use the relevant methods on the principal.

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

::: content/base/src/nsGenericElement.cpp
@@ +2793,5 @@
>  static const char*
>  GetFullScreenError(nsIDocument* aDoc)
>  {
>    nsCOMPtr<nsPIDOMWindow> win = aDoc->GetWindow();
> +  if (aDoc->NodePrincipal()->GetAppStatus() >= nsIPrincipal::APP_STATUS_INSTALLED) {

I've tested the latest inbound with this change, and this has caused fullscreen requests that are inside an app but which are not in the app's origin to be approved if they're not inside user generated event handlers. This means script in non-app-origins can request fullscreen.

This is not what we want. As we agreed earlier, fullscreen requests in apps outside of the app origin should be allowed provided they're in user-generated event handlers, and provided we show approval UI (which is waiting for merge in https://github.com/mozilla-b2g/gaia/pull/3200 ...). Fullscreen requests in-app-origin should be approved without showing approval UI...

Please restore the old behaviour, and in nsDocument too else you'll regress behaviour in apps on desktop.

I'd be happy to use the principal for security checks over the nsPIDOMWindow methods, but not at the expense of functionality.

Also I would very much appreciate it if you could please consult me before making changes that impact the behaviour of the fullscreen API in future. Thanks.

Comment 41

[Justin Lebar (not reading bugmail)]

> I've tested the latest inbound with this change, and this has caused fullscreen requests that are 
> inside an app but which are not in the app's origin to be approved if they're not inside user 
> generated event handlers

Correct, this was an understood regression, and will be fixed by bug 781620.

I'm sorry for temporarily regressing this, but we really needed to remove these methods, and I had a long queue of important fixes blocked on this bug.

Comment 42

[Chris Pearce [:cpearce (Not reading bugmail)]]

OK, thanks for clarifying.

Comment 43

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/58910754c255
https://hg.mozilla.org/mozilla-central/rev/3a51e992d7f1
https://hg.mozilla.org/mozilla-central/rev/356689768e3f