Justin Lebar (not reading bugmail) Assignee
	Description • 12 years ago

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!

	Mounir Lamouri (:mounir)
	Comment 1 • 12 years ago

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.

	Justin Lebar (not reading bugmail) Assignee
	Comment 2 • 12 years ago

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?

	Mounir Lamouri (:mounir)
	Comment 3 • 12 years ago

(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.

	Justin Lebar (not reading bugmail) Assignee
	Comment 4 • 12 years ago

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?

	Mounir Lamouri (:mounir)
	Comment 7 • 12 years ago

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.

	Mounir Lamouri (:mounir)
	Comment 8 • 12 years ago

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.

	Mounir Lamouri (:mounir)
	Comment 9 • 12 years ago

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

	Justin Lebar (not reading bugmail) Assignee
	Comment 10 • 12 years ago

> 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.  :)

	Justin Lebar (not reading bugmail) Assignee
	Comment 11 • 12 years ago

(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.  :(

	Justin Lebar (not reading bugmail) Assignee
	Comment 12 • 12 years ago

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

?

	Justin Lebar (not reading bugmail) Assignee
	Comment 14 • 12 years ago

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

	Mounir Lamouri (:mounir)
	Comment 16 • 12 years ago

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...

	Justin Lebar (not reading bugmail) Assignee
	Comment 17 • 12 years ago

(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?

	Justin Lebar (not reading bugmail) Assignee
	Comment 18 • 12 years ago

(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.

	Justin Lebar (not reading bugmail) Assignee
	Comment 19 • 12 years ago

> 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.

	Justin Lebar (not reading bugmail) Assignee
	Comment 20 • 12 years ago

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

	Mounir Lamouri (:mounir)
	Comment 22 • 12 years ago

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)) { ...

	Mounir Lamouri (:mounir)
	Comment 23 • 12 years ago

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...

	Mounir Lamouri (:mounir)
	Comment 24 • 12 years ago

(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 ;)

	Mounir Lamouri (:mounir)
	Comment 25 • 12 years ago

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.

	Justin Lebar (not reading bugmail) Assignee
	Comment 26 • 12 years ago

> 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.  :)

	Justin Lebar (not reading bugmail) Assignee
	Comment 27 • 12 years ago

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.

	Justin Lebar (not reading bugmail) Assignee
	Comment 28 • 12 years ago

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.

	Mounir Lamouri (:mounir)
	Comment 29 • 12 years ago

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.

	Justin Lebar (not reading bugmail) Assignee
	Comment 30 • 12 years ago

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

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

	Justin Lebar (not reading bugmail) Assignee
	Comment 32 • 12 years ago

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

	Mounir Lamouri (:mounir)
	Comment 33 • 12 years ago

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.

	Justin Lebar (not reading bugmail) Assignee
	Comment 34 • 12 years ago

> 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.

	Justin Lebar (not reading bugmail) Assignee
	Comment 35 • 12 years ago

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/

	Justin Lebar (not reading bugmail) Assignee
	Comment 36 • 12 years ago

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.

	Justin Lebar (not reading bugmail) Assignee
	Comment 37 • 12 years ago

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

Oops, wrong bug.  :(

	Mounir Lamouri (:mounir)
	Comment 38 • 12 years ago

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?

	Justin Lebar (not reading bugmail) Assignee
	Comment 39 • 12 years ago

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.

	Chris Pearce [:cpearce (Not reading bugmail)]
	Comment 40 • 12 years ago

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.

	Justin Lebar (not reading bugmail) Assignee
	Comment 41 • 12 years ago

> 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.

	Chris Pearce [:cpearce (Not reading bugmail)]
	Comment 42 • 12 years ago

OK, thanks for clarifying.

	Ed Morley [:emorley]
	Comment 43 • 12 years ago

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