753978 - Be able to know if a window is part of an application

Status: RESOLVED FIXED

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

Description

[Mounir Lamouri (:mounir)]

For some security model, we need to know if a window is part of an application whatever the application is.

To make that possible, the plan is to add a 'mozApp' attribute to <iframe mozBrowser> when the iframe should actually be an application. The only case where both attribute will not be present is when <iframe mozBrowser> is used for the browser app content/tabs.

Comment 1

[Mounir Lamouri (:mounir)]

Attachment: Patch v1

Comment 2

[Justin Lebar (not reading bugmail)]

Comment on attachment 622858 [details] [diff] [review]
Patch v1

>@@ -932,17 +932,20 @@ nsFrameLoader::ShowRemoteFrame(const nsI
>     mRemoteBrowser->Show(size);
>     mRemoteBrowserShown = true;
> 
>     EnsureMessageManager();
> 
>     nsCOMPtr<nsIObserverService> os = services::GetObserverService();
>     if (OwnerIsBrowserFrame() && os) {
>       os->NotifyObservers(NS_ISUPPORTS_CAST(nsIFrameLoader*, this),
>-                          "remote-browser-frame-shown", NULL);
>+                          "remote-browser-frame-shown",
>+                          mOwnerContent->HasAttr(kNameSpaceID_None, nsGkAtoms::mozapp)
>+                            ? NS_LITERAL_STRING("true").get()
>+                            : NS_LITERAL_STRING("false").get());

Instead of passing "true" and "false" through the observers, can you pass "is-moz-app:{true,false}", or something?  Also, can we do "true" instead of NS_LITERAL_STRING("true").get()?

>diff --git a/content/html/content/src/nsGenericHTMLFrameElement.cpp b/content/html/content/src/nsGenericHTMLFrameElement.cpp
>--- a/content/html/content/src/nsGenericHTMLFrameElement.cpp
>+++ b/content/html/content/src/nsGenericHTMLFrameElement.cpp
>@@ -284,15 +284,25 @@ nsGenericHTMLFrameElement::GetReallyIsBr
>   }
> 
>   // Fail if the node principal isn't trusted.
>   nsIPrincipal *principal = NodePrincipal();
>   nsCOMPtr<nsIURI> principalURI;
>   principal->GetURI(getter_AddRefs(principalURI));
>   if (!nsContentUtils::URIIsChromeOrInPref(principalURI,
>                                            "dom.mozBrowserFramesWhitelist")) {
>-    return NS_OK;
>+    if (!HasAttr(kNameSpaceID_None, nsGkAtoms::mozapp)) {
>+      return NS_OK;
>+    }
>+
>+    // When @mozApp is set, having mozApps permission is enough.
>+    // TODO: that means <iframe mozApp mozBrowser> can work with only mozBrowser
>+    // permissions.
>+    if (!nsContentUtils::URIIsChromeOrInPref(principalURI,
>+                                             "dom.mozApps.whitelist")) {
>+      return NS_OK;
>+    }
>   }
> 
>   // Otherwise, succeed.
>   *aOut = true;
>   return NS_OK;
> }

I don't think this extra whitelist adds much, since <iframe mozapp> by itself doesn't do anything.  I'd rather add this new whitelist once we do <iframe mozapp> properly.

>diff --git a/dom/base/BrowserAppsElement.js b/dom/base/BrowserAppsElement.js
>new file mode 100644
>--- /dev/null
>+++ b/dom/base/BrowserAppsElement.js
>@@ -0,0 +1,9 @@

BrowserAppElement.js?

But you could just load this as an inline script:

      mm.loadFrameScript("data:,content.QueryInterface...",
                         /* allowDelayedLoad = */ true);

Also, could you do me a favor and check whether allowDelayedLoad = true is
right?  I think that may be loading into every frame that's created after.  If
so, both this and the other one should be false.

>diff --git a/dom/base/BrowserElementChild.js b/dom/base/BrowserElementChild.js
>--- a/dom/base/BrowserElementChild.js
>+++ b/dom/base/BrowserElementChild.js
>@@ -36,16 +36,20 @@ BrowserElementChild.prototype = {
>     debug("Starting up.");
>     sendAsyncMsg("hello");
> 
>     docShell.QueryInterface(Ci.nsIWebProgress)
>             .addProgressListener(this._progressListener,
>                                  Ci.nsIWebProgress.NOTIFY_LOCATION |
>                                  Ci.nsIWebProgress.NOTIFY_STATE_WINDOW);
> 
>+    content.QueryInterface(Ci.nsIInterfaceRequestor)
>+           .getInterface(Components.interfaces.nsIDOMWindowUtils)
>+           .setApp(false);

Ah, this is tricky.  If I have an app which contains a browser, the content in
the browser is not "in an app"?  We should document that here and on the app
attribute.

>diff --git a/dom/base/BrowserElementParent.js b/dom/base/BrowserElementParent.js
>--- a/dom/base/BrowserElementParent.js
>+++ b/dom/base/BrowserElementParent.js
>@@ -62,27 +62,27 @@ BrowserElementParent.prototype = {
>     try {
>       return prefs.getBoolPref(BROWSER_FRAMES_ENABLED_PREF);
>     }
>     catch(e) {
>       return false;
>     }
>   },
> 
>-  _observeInProcessBrowserFrameShown: function(frameLoader) {
>+  _observeInProcessBrowserFrameShown: function(frameLoader, data) {
>     debug("In-process browser frame shown " + frameLoader);
>-    this._setUpMessageManagerListeners(frameLoader);
>+    this._setUpMessageManagerListeners(frameLoader, data);
>   },
> 
>-  _observeRemoteBrowserFrameShown: function(frameLoader) {
>+  _observeRemoteBrowserFrameShown: function(frameLoader, data) {
>     debug("Remote browser frame shown " + frameLoader);
>-    this._setUpMessageManagerListeners(frameLoader);
>+    this._setUpMessageManagerListeners(frameLoader, data);
>   },
> 
>-  _setUpMessageManagerListeners: function(frameLoader) {
>+  _setUpMessageManagerListeners: function(frameLoader, data) {
>     let frameElement = frameLoader.QueryInterface(Ci.nsIFrameLoader).ownerElement;
>     if (!frameElement) {
>       debug("No frame element?");
>       return;
>     }
> 
>     let mm = frameLoader.messageManager;
> 
>@@ -96,16 +96,20 @@ BrowserElementParent.prototype = {
>     addMessageListener("hello", this._recvHello);
>     addMessageListener("locationchange", this._fireEventFromMsg);
>     addMessageListener("loadstart", this._fireEventFromMsg);
>     addMessageListener("loadend", this._fireEventFromMsg);
>     addMessageListener("titlechange", this._fireEventFromMsg);
> 
>     mm.loadFrameScript("chrome://global/content/BrowserElementChild.js",
>                        /* allowDelayedLoad = */ true);
>+    if (data) {
>+      mm.loadFrameScript("chrome://global/content/BrowserAppsElement.js",
>+                         /* allowDelayedLoad = */ true);
>+    }

Please don't call it "data"; we use that for mm data params, which this isn't.

>+NS_IMETHODIMP
>+nsDOMWindowUtils::SetApp(bool aValue)
>+{
>+  nsCOMPtr<nsPIDOMWindow> window = do_QueryReferent(mWindow);
>+  NS_ENSURE_TRUE(window, NS_ERROR_FAILURE);
>+
>+  static_cast<nsGlobalWindow*>(window.get())->SetIsApp(aValue);
>+
>+  return NS_OK;
>+}

I think you can call this SetIsApp and it'll all work out OK.

>diff --git a/dom/base/nsGlobalWindow.h b/dom/base/nsGlobalWindow.h
>--- a/dom/base/nsGlobalWindow.h
>+++ b/dom/base/nsGlobalWindow.h
>@@ -553,20 +553,28 @@ public:
> 
>   void SizeOfIncludingThis(nsWindowSizes* aWindowSizes) const;
> 
>   void UnmarkGrayTimers();
> 
>   void AddEventTargetObject(nsDOMEventTargetHelper* aObject);
>   void RemoveEventTargetObject(nsDOMEventTargetHelper* aObject);
> 
>+  bool IsPartOfApp();

Comment, please, specifically about how TriState_Unknown factors into this.

> protected:
>   friend class HashchangeCallback;
>   friend class nsBarProp;
> 
>+  enum TriState {
>+    State_Unknown = -1,
>+    State_False,
>+    State_True
>+  };

Maybe TriState_{Unknown,False,True}?

>@@ -806,16 +814,18 @@ protected:
> 
>   inline PRInt32 DOMMinTimeoutValue() const;
> 
>   nsresult CreateOuterObject(nsGlobalWindow* aNewInner);
>   nsresult SetOuterObject(JSContext* aCx, JSObject* aOuterObject);
>   nsresult CloneStorageEvent(const nsAString& aType,
>                              nsCOMPtr<nsIDOMStorageEvent>& aEvent);
> 
>+  void SetIsApp(bool aValue);
>+
>   // When adding new member variables, be careful not to create cycles
>   // through JavaScript.  If there is any chance that a member variable
>   // could own objects that are implemented in JavaScript, then those
>   // objects will keep the global object (this object) alive.  To prevent
>   // these cycles, ownership of such members must be released in
>   // |CleanUp| and |SetDocShell|.
> 
>   // This member is also used on both inner and outer windows, but
>@@ -873,16 +883,19 @@ protected:
> 
>   // true if tab navigation has occurred for this window. Focus rings
>   // should be displayed.
>   bool                   mFocusByKeyOccurred : 1;
> 
>   // whether we've sent the destroy notification for our window id
>   bool                   mNotifiedIDDestroyed : 1;
> 
>+  // Whether the window is the window of an application frame.
>+  TriState               mIsApp : 2;

"This can be TriState_Unknown if...".

>diff --git a/dom/interfaces/base/nsIDOMWindowUtils.idl b/dom/interfaces/base/nsIDOMWindowUtils.idl
>--- a/dom/interfaces/base/nsIDOMWindowUtils.idl
>+++ b/dom/interfaces/base/nsIDOMWindowUtils.idl
>@@ -65,17 +65,17 @@ interface nsIDOMEvent;
> interface nsITransferable;
> interface nsIQueryContentEventResult;
> interface nsIDOMWindow;
> interface nsIDOMBlob;
> interface nsIDOMFile;
> interface nsIFile;
> interface nsIDOMTouch;
> 
>-[scriptable, uuid(f75d0a14-e278-4716-a151-637862451a2f)]
>+[scriptable, uuid(2e5a1f37-786b-4a52-b0e3-f711ee2268a8)]
> interface nsIDOMWindowUtils : nsISupports {
> 
>   /**
>    * Image animation mode of the window. When this attribute's value
>    * is changed, the implementation should set all images in the window
>    * to the given value. That is, when set to kDontAnimMode, all images
>    * will stop animating. The attribute's value must be one of the
>    * animationMode values from imgIContainer.
>@@ -1144,9 +1144,15 @@ interface nsIDOMWindowUtils : nsISupport
> 
>   /**
>    * Set the scrollport size for the purposes of clamping scroll positions for
>    * the root scroll frame of this document to be (aWidth,aHeight) in CSS pixels.
>    *
>    * The caller of this method must have UniversalXPConnect privileges.
>    */
>   void setScrollPositionClampingScrollPortSize(in float aWidth, in float aHeight);
>+
>+  /**
>+   * Mark if the window is an application window or not.
>+   * This should not be set for regular iframe's inside the app.
>+   */
>+  void setApp(in boolean value);

s/iframe's/iframes/.  Also, make the statement positive.  For example: "This should be set only for top-level mozapp frames.  Leave it unset for other frames, unless you want to break the mozapp-ness of this frame and all child frames (e.g. if this is a mozbrowser frame)."

I'd also expose isInApp here.

Comment 3

[Justin Lebar (not reading bugmail)]

Yeah, I'm pretty sure, from talking to smaug and looking at the code, that this should be loadFrameScript(uri, false), not true.

[3:16pm] jlebar|mac: smaug: loadFrameScript(uri, true) -- that means that URI is loaded into every frame from then on?
[3:16pm] jlebar|mac: smaug: So…it makes no sense to call loadFrameScript(uri, true) twice with the same URI, for example?
[3:16pm] smaug: jlebar|mac: right

Comment 4

[Justin Lebar (not reading bugmail)]

smaug is now telling me that we should use true.  You shouldn't do loadFrameScript(uri, true) for the same URI *and the same mm* twice, but I don't think we do.

smaug: jlebar|mac: when you have a remotebrowser, we may create messagemanager before we have really the remove stuff
[3:35pm] smaug: jlebar|mac: so script loading is delayed
[3:36pm] jlebar|mac: smaug: But suppose my <iframe mozbrowser> content itself contains an iframe mozbrowser.  That's not a child mm in this setup, so my content doesn't get injected in there?
hat room.
[3:38pm] smaug: jlebar|mac: right

There's some complication with this, bug 754089, but we should apparently use |true|.

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

bug 754089 has nothing to do with this. Just that if you load framescript in the chrome
window level mm, they get loaded also in mozbrowser mm. I'm surprised we don't have
assertions because of that.

Comment 6

[Mounir Lamouri (:mounir)]

Attachment: Patch v2

Comment 7

[Mounir Lamouri (:mounir)]

(In reply to Justin Lebar [:jlebar] from comment #2)
> Comment on attachment 622858 [details] [diff] [review]
> Patch v1
> 
> >@@ -932,17 +932,20 @@ nsFrameLoader::ShowRemoteFrame(const nsI
> >     mRemoteBrowser->Show(size);
> >     mRemoteBrowserShown = true;
> > 
> >     EnsureMessageManager();
> > 
> >     nsCOMPtr<nsIObserverService> os = services::GetObserverService();
> >     if (OwnerIsBrowserFrame() && os) {
> >       os->NotifyObservers(NS_ISUPPORTS_CAST(nsIFrameLoader*, this),
> >-                          "remote-browser-frame-shown", NULL);
> >+                          "remote-browser-frame-shown",
> >+                          mOwnerContent->HasAttr(kNameSpaceID_None, nsGkAtoms::mozapp)
> >+                            ? NS_LITERAL_STRING("true").get()
> >+                            : NS_LITERAL_STRING("false").get());
> 
> Instead of passing "true" and "false" through the observers, can you pass
> "is-moz-app:{true,false}", or something?  Also, can we do "true" instead of
> NS_LITERAL_STRING("true").get()?

No, I have to pass a PRUnichar*. AFAIK, this is the way to do that.

> >diff --git a/dom/base/BrowserAppsElement.js b/dom/base/BrowserAppsElement.js
> >new file mode 100644
> >--- /dev/null
> >+++ b/dom/base/BrowserAppsElement.js
> >@@ -0,0 +1,9 @@
> 
> BrowserAppElement.js?
> 
> But you could just load this as an inline script:
> 
>       mm.loadFrameScript("data:,content.QueryInterface...",
>                          /* allowDelayedLoad = */ true);

I should have think about that :)

> Also, could you do me a favor and check whether allowDelayedLoad = true is
> right?  I think that may be loading into every frame that's created after. 
> If
> so, both this and the other one should be false.

It's working well with 'true'.

> >diff --git a/dom/base/BrowserElementChild.js b/dom/base/BrowserElementChild.js
> >--- a/dom/base/BrowserElementChild.js
> >+++ b/dom/base/BrowserElementChild.js
> >@@ -36,16 +36,20 @@ BrowserElementChild.prototype = {
> >     debug("Starting up.");
> >     sendAsyncMsg("hello");
> > 
> >     docShell.QueryInterface(Ci.nsIWebProgress)
> >             .addProgressListener(this._progressListener,
> >                                  Ci.nsIWebProgress.NOTIFY_LOCATION |
> >                                  Ci.nsIWebProgress.NOTIFY_STATE_WINDOW);
> > 
> >+    content.QueryInterface(Ci.nsIInterfaceRequestor)
> >+           .getInterface(Components.interfaces.nsIDOMWindowUtils)
> >+           .setApp(false);
> 
> Ah, this is tricky.  If I have an app which contains a browser, the content
> in
> the browser is not "in an app"?  We should document that here and on the app
> attribute.

Comment added.

> >diff --git a/dom/interfaces/base/nsIDOMWindowUtils.idl b/dom/interfaces/base/nsIDOMWindowUtils.idl
> >--- a/dom/interfaces/base/nsIDOMWindowUtils.idl
> >+++ b/dom/interfaces/base/nsIDOMWindowUtils.idl
> >+  /**
> >+   * Mark if the window is an application window or not.
> >+   * This should not be set for regular iframe's inside the app.
> >+   */
> >+  void setApp(in boolean value);
> 
> I'd also expose isInApp here.

I would prefer not. This solution is weak and might be replaced soon so as long as we don't need a getter, better to not have one.

Comment 8

[Justin Lebar (not reading bugmail)]

Comment on attachment 622963 [details] [diff] [review]
Patch v2

>diff --git a/dom/base/BrowserElementChild.js b/dom/base/BrowserElementChild.js
>--- a/dom/base/BrowserElementChild.js
>+++ b/dom/base/BrowserElementChild.js
>@@ -36,16 +36,22 @@ BrowserElementChild.prototype = {
>     debug("Starting up.");
>     sendAsyncMsg("hello");
> 
>     docShell.QueryInterface(Ci.nsIWebProgress)
>             .addProgressListener(this._progressListener,
>                                  Ci.nsIWebProgress.NOTIFY_LOCATION |
>                                  Ci.nsIWebProgress.NOTIFY_STATE_WINDOW);
> 
>+    // A mozBrowser iframe is an app iif mozApp is set so it is not an app by
>+    // default.
>+    content.QueryInterface(Ci.nsIInterfaceRequestor)
>+           .getInterface(Components.interfaces.nsIDOMWindowUtils)
>+           .setIsApp(false);

Sorry to be picky here, but can you say what the reason is for the first
predicate?  This is an important, subtle point.  For example:

  A mozbrowser iframe contained inside a mozapp iframe should return false for
  nsWindowUtils::IsPartOfApp (unless the mozbrowser iframe is itself also a
  mozapp).  That is, mozapp is transitive down to its children, but mozbrowser
  serves as a barrier.

  This is because mozapp iframes have some privileges which we don't want to
  extend to untrusted mozbrowser content.

>-  _observeInProcessBrowserFrameShown: function(frameLoader) {
>+  _observeInProcessBrowserFrameShown: function(frameLoader, isMozApp) {
>     debug("In-process browser frame shown " + frameLoader);
>-    this._setUpMessageManagerListeners(frameLoader);
>+    this._setUpMessageManagerListeners(frameLoader, isMozApp);
>   },
> 
>-  _observeRemoteBrowserFrameShown: function(frameLoader) {
>+  _observeRemoteBrowserFrameShown: function(frameLoader, isMozApp) {
>     debug("Remote browser frame shown " + frameLoader);
>-    this._setUpMessageManagerListeners(frameLoader);
>+    this._setUpMessageManagerListeners(frameLoader, isMozApp);
>   },

Nit: Instead of passing the argument down to setUpMessageManagerListeners, can
you have |if (isMozApp) { this._markFrameAsMozApp(frameLoader) }| or something?
I don't like replicating the if statement, but that seems better than passing
this argument which isn't really needed by setUpMessageManagerListeners.

>+bool
>+nsGlobalWindow::IsPartOfApp()
>+{
>+  FORWARD_TO_OUTER(IsPartOfApp, (), TriState_False);
>+
>+  // We go trough all window parents until we find one with |mIsApp| set to
>+  // something. If none is found, we are not part of an application.
>+  for (nsGlobalWindow* w = this; w;
>+       w = static_cast<nsGlobalWindow*>(w->GetParentInternal())) {
>+    if (w->mIsApp == TriState_True) {
>+      return true;
>+    } else if (w->mIsApp == TriState_False) {
>+      return false;
>+    }
>+  }
>+
>+  return TriState_False;
>+}

Whoa, return false!

>+  // Whether the window is the window of an application frame.
>+  // This can be TriState_Unknown if the object is the content window of an
>+  // iframe which is neither mozBrowser nor mozApp.
>+  TriState               mIsApp : 2;

Nit: s/can be/is/?

>+  /**
>+   * Mark if the window is an application window or not.
>+   * This should only be set for top-level mozApp or mozBrowser frames.
>+   * It should not be set for other frames unless you want to break the app
>+   * status of the frame and its children.
>+   */
>+  void setIsApp(in boolean value);

s/want to break the app status of the frame and its children/want a frame (and its children) to have a different value for IsPartOfApp than the frame's parent/.

Comment 9

[Mounir Lamouri (:mounir)]

http://hg.mozilla.org/mozilla-central/rev/9f2edf9a549a

Comment 10

[Johnny Stenback (:jst)]

What is the purpose of nsDOMWindowUtils::SetIsApp() here? As coded here any webpage can set their window to be an app. That seems a bit loose to me, is that the intent here?

Comment 11

[Justin Lebar (not reading bugmail)]

(In reply to Johnny Stenback (:jst, [e-mail]) from comment #10)
> What is the purpose of nsDOMWindowUtils::SetIsApp() here? As coded here any
> webpage can set their window to be an app. That seems a bit loose to me, is
> that the intent here?

He fixed this in a later bug.  See bug 754141 comment 7.