The default bug view has changed. See this FAQ.

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

RESOLVED FIXED in mozilla17

Status

()

Core
DOM
--
critical
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: Justin Lebar (not reading bugmail), Assigned: Justin Lebar (not reading bugmail))

Tracking

(Blocks: 1 bug)

Trunk
mozilla17
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 5 obsolete attachments)

14.31 KB, patch
mounir
: review+
Details | Diff | Splinter Review
1.09 KB, patch
mounir
: review+
Details | Diff | Splinter Review
14.51 KB, patch
mounir
: review+
cpearce
: feedback-
Details | Diff | Splinter Review
(Assignee)

Description

5 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!
(Assignee)

Updated

5 years ago
Blocks: 693515
(Assignee)

Updated

5 years ago
Severity: normal → critical
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.
(Assignee)

Comment 2

5 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?
(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.
(Assignee)

Updated

5 years ago
Summary: BrowserElementChild.js fails to initialize when loaded for an OOP app, due to windowUtils.setApp(appManifestURL) being unavailable in child process → Nix windowUtils::setApp and friends; convert users to principal methods
(Assignee)

Updated

5 years ago
Assignee: nobody → justin.lebar+bug
(Assignee)

Comment 4

5 years ago
Created attachment 646281 [details] [diff] [review]
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?
Attachment #646281 - Flags: review?(mounir)
Attachment #646281 - Flags: review?(jonas)
(Assignee)

Comment 5

5 years ago
Created attachment 646282 [details] [diff] [review]
Bug 777135 - Part 2: Stop using nsDOMWindowUtils::GetIsApp and friends, and instead use the relevant methods on the principal.
Attachment #646282 - Flags: review?(mounir)
Attachment #646282 - Flags: review?(jonas)
(Assignee)

Comment 6

5 years ago
Created attachment 646283 [details] [diff] [review]
Bug 777135 - Part 3: Remove nsDOMWindowUtils::GetIsApp and friends.
Attachment #646283 - Flags: review?(mounir)
Attachment #646283 - Flags: review?(jonas)
(Assignee)

Updated

5 years ago
Blocks: 768832
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.
Attachment #646281 - Flags: review?(mounir)
Attachment #646281 - Flags: review?(jonas)
Attachment #646281 - Flags: review-
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.
Attachment #646282 - Flags: review?(mounir)
Attachment #646282 - Flags: review?(jonas)
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
Attachment #646283 - Flags: review?(mounir)
Attachment #646283 - Flags: review?(jonas)
Attachment #646283 - Flags: review-
(Assignee)

Comment 10

5 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.  :)
(Assignee)

Comment 11

5 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.  :(
(Assignee)

Comment 12

5 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

?
(Assignee)

Comment 13

5 years ago
Created attachment 649183 [details] [diff] [review]
Part 1, v2: Add nice C++ version of nsIPrincipal::GetAppStatus.
Attachment #649183 - Flags: review?(mounir)
(Assignee)

Comment 14

5 years ago
Created attachment 649184 [details] [diff] [review]
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.
Attachment #649184 - Flags: review?(mounir)
(Assignee)

Comment 15

5 years ago
Created attachment 649185 [details] [diff] [review]
Part 3, v2: Remove nsDOMWindowUtils::GetIsApp and friends.
Attachment #649185 - Flags: review?(mounir)
(Assignee)

Updated

5 years ago
Attachment #646281 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Attachment #646282 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Attachment #646283 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Attachment #649183 - Attachment description: Part 1: Add nice C++ version of nsIPrincipal::GetAppStatus. → Part 1, v2: Add nice C++ version of nsIPrincipal::GetAppStatus.
(Assignee)

Updated

5 years ago
Attachment #649184 - Attachment description: Part 2: Stop using nsDOMWindowUtils::GetIsApp and friends, and instead use the relevant methods on the principal. → Part 2, v2: Stop using nsDOMWindowUtils::GetIsApp and friends, and instead use the relevant methods on the principal.
(Assignee)

Updated

5 years ago
Attachment #649185 - Attachment description: Part 3: Remove nsDOMWindowUtils::GetIsApp and friends. → Part 3, v2: Remove nsDOMWindowUtils::GetIsApp and friends.
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...
Attachment #649183 - Flags: review?(mounir) → review-
(Assignee)

Comment 17

5 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?
(Assignee)

Comment 18

5 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.
(Assignee)

Comment 19

5 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.
(Assignee)

Updated

5 years ago
Depends on: 779935
(Assignee)

Updated

5 years ago
No longer depends on: 779935
(Assignee)

Updated

5 years ago
Blocks: 779935
(Assignee)

Comment 20

5 years ago
Gah, complete Bugzilla fail.  Sorry for the spam.  :(
No longer blocks: 779935
Depends on: 779935
(Assignee)

Comment 21

5 years ago
Created attachment 649470 [details] [diff] [review]
Part 1, v3: Add nice C++ version of nsIPrincipal::GetAppStatus.
Attachment #649183 - Attachment is obsolete: true
Attachment #649470 - Flags: review?(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)) { ...
Attachment #649470 - Flags: review?(mounir) → review+
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...
Attachment #649184 - Flags: review?(mounir) → review-
(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 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.
Attachment #649185 - Flags: review?(mounir)
(Assignee)

Comment 26

5 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.  :)
(Assignee)

Comment 27

5 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.
(Assignee)

Comment 28

5 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.
Attachment #649184 - Flags: feedback?(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.
Attachment #649184 - Flags: feedback?(mounir)
(Assignee)

Comment 30

5 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.  :)
(Assignee)

Updated

5 years ago
Attachment #649185 - Flags: review?(mounir)
(Assignee)

Comment 31

5 years ago
Created attachment 650687 [details] [diff] [review]
Part 2, v3: Stop using nsDOMWindowUtils::GetIsApp and friends, and instead use the relevant methods on the principal.
Attachment #649184 - Attachment is obsolete: true
Attachment #650687 - Flags: review?(mounir)
(Assignee)

Comment 32

5 years ago
https://tbpl.mozilla.org/?tree=Try&rev=358522df4f4b
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.
Attachment #650687 - Flags: review?(mounir)
Attachment #649185 - Flags: review?(mounir) → review+
(Assignee)

Comment 34

5 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.
(Assignee)

Comment 35

5 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/
(Assignee)

Comment 36

5 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.
Attachment #650687 - Flags: review?(mounir)
(Assignee)

Comment 37

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

Oops, wrong bug.  :(
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?
Attachment #650687 - Flags: review?(mounir) → review+
(Assignee)

Comment 39

5 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.
(Assignee)

Updated

5 years ago
Blocks: 768561
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.
Attachment #650687 - Flags: feedback-
(Assignee)

Comment 41

5 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.
OK, thanks for clarifying.
https://hg.mozilla.org/mozilla-central/rev/58910754c255
https://hg.mozilla.org/mozilla-central/rev/3a51e992d7f1
https://hg.mozilla.org/mozilla-central/rev/356689768e3f
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
Component: DOM: Mozilla Extensions → DOM
Product: Core → Core
You need to log in before you can comment on or make changes to this bug.