332285 - nsGlobalWindow.cpp and nsCommandManager.cpp should use nsContentUtils::IsCallerChrome

Status: RESOLVED FIXED

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

Description

[Martijn Wargers (dead)]

This is a spin-off from bug 331491.

nsGlobalWindow.cpp and nsCommandManager.cpp use their own IsCallerChrome functions. They should not do that and instead use nsContentUtils::IsCallerChrome(), nsContentUtils::IsCallerTrustedForRead() and nsContentUtils::IsCallerTrustedForWrite() where appropriate.

Comment 1

[Martijn Wargers (dead)]

Attachment: patch

So I guess something like this. I haven't tested anything yet, I only know it compiles.

Comment 2

[Martijn Wargers (dead)]

Btw, that patch is only for nsGlobalWindow.cpp

Comment 3

[Robert O'Callahan (:roc) (email my personal email if necessary)]

That looks good.

Comment 4

[Martijn Wargers (dead)]

Attachment: patchb

patch for commandmanager that compiles.

Comment 5

[Martijn Wargers (dead)]

I thought that patchb compiled, but it only compiles from /embedding/components/commandhandler/src/
not when I compile from embedding/
I guess I'm missing something, but I have no idea what.

Comment 6

[Martijn Wargers (dead)]

Attachment: patch2

Updated to recent trunk and I found another case that could make use of contentutils.

Comment 7

[Martijn Wargers (dead)]

Checking in dom/src/base/nsGlobalWindow.cpp;
/cvsroot/mozilla/dom/src/base/nsGlobalWindow.cpp,v  <--  nsGlobalWindow.cpp
new revision: 1.834; previous revision: 1.833
done

Patchb can't be used at the moment, roc and biesi at irc:
roc	it fails to build in embedding/ because the link fails, probably.
biesi	different library
biesi	and inter-library calls are hard in mozilla
roc	in non-static builds, they get put in different shared libraries (DLLs)
biesi	(unless done via vtable)
mw22	Ah, ok
roc	mw22: nsContentUtils can be accessed from content/, layout/, view/, dom/ and docshell/
roc	hmm, maybe not docshell
mw22	Why this restriction?
biesi	not docshell
biesi	docshell is its own lib
biesi	also contains exthandler uriloader etc
roc	mw22: if we avoided this restriction, we'd have to build everything as a single giant library, always

Comment 8

[[e-mail]]

Comment on attachment 217042 [details] [diff] [review]
patch2

> NS_IMETHODIMP
> nsGlobalWindow::OpenDialog(nsIDOMWindow** _retval)
> {
>-  if (!IsCallerChrome()) {
>+  if (!nsContentUtils::IsCallerTrustedForWrite()) {
>     return NS_ERROR_DOM_SECURITY_ERR;
>   }
Do we really want anyone with UBW to be able to open chrome dialogs? Note that people with UXP used to be able to fake it using the window watcher.

>-    PRBool allowClose = PR_FALSE;
>-
>-    // UniversalBrowserWrite will be enabled if it's been explicitly
>-    // enabled, or if we're called from chrome.
>-    rv = sSecMan->IsCapabilityEnabled("UniversalBrowserWrite",
>-                                      &allowClose);
>-
>-    if (NS_SUCCEEDED(rv) && !allowClose) {
>-      allowClose =
>+    if (nsContentUtils::IsCallerTrustedForWrite()) {
>+      PRBool allowClose =
>         nsContentUtils::GetBoolPref("dom.allow_scripts_to_close_windows",
>                                     PR_TRUE);
This pref check needs to be done if the caller is not trusted for write...

Comment 9

[Martijn Wargers (dead)]

(In reply to comment #8)
> Do we really want anyone with UBW to be able to open chrome dialogs? Note that
> people with UXP used to be able to fake it using the window watcher.
Well, I would like to be able to do this universalxpconnect, I was not sure about the universalbrowserwrite thing.
http://www.mozilla.org/projects/security/components/signed-scripts.html#privs-list
Personally, I always use the universalxpconnect to get something to work. 

> This pref check needs to be done if the caller is not trusted for write...
Argh! Patch coming up.

Comment 10

[Martijn Wargers (dead)]

Attachment: patch

Comment 11

[Martijn Wargers (dead)]

Attachment: patch diff -w

Comment 12

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 217134 [details] [diff] [review]
patch

oops

Comment 13

[Martijn Wargers (dead)]

Checking in dom/src/base/nsGlobalWindow.cpp;
/cvsroot/mozilla/dom/src/base/nsGlobalWindow.cpp,v  <--  nsGlobalWindow.cpp
new revision: 1.835; previous revision: 1.834
done

Thanks for the speedy review, roc!

Comment 14

[Steve England [:stevee]]

Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9a1) Gecko/20060404 Firefox/1.6a1 ID:2006040414 (non-cairo)
Things seem fine again Martijn.

Comment 15

[Martijn Wargers (dead)]

Reassigning to me, sorry for the spam.

Comment 16

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: followup to patch 2

attachment 217042 [details] [diff] [review] forgot to change the header file as well

Comment 17

[Martijn Wargers (dead)]

Comment on attachment 250228 [details] [diff] [review]
followup to patch 2

Oops, sorry about that.

Comment 18

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 250228 [details] [diff] [review]
followup to patch 2

Checked in to trunk.