651315 - extensions.js uses a limited openDialog (it's impossible to open dialogs from a chrome-like protocol)

Status: VERIFIED FIXED

(Toolkit :: Add-ons Manager, defect)

Description

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

So, I was playing with bootstrapped extensions, and I've hit what is called problem 6.5 in this blog post: http://adblockplus.org/blog/how-many-hacks-does-it-take-to-make-your-extension-install-without-a-restart

To workaround the "chrome" protocol problem (not available to register my resources) I've registered my own system principal protocol, the problem is that using this protocol in optionsURL or aboutURL doesn't open a dialog, rather opens a browser window with xul inside it.

The problem is that about:addons is a specially privileged page, when extensions.js invokes openDialog it does with special privileges, and it ends up using the uriToLoadIsChrome check in http://hg.mozilla.org/mozilla-central/annotate/0680c776e806/embedding/components/windowwatcher/src/nsWindowWatcher.cpp#l527 (most likely ending up here http://hg.mozilla.org/mozilla-central/annotate/0680c776e806/embedding/components/windowwatcher/src/nsWindowWatcher.cpp#l1594)

A really simple workaround to this is to change in extensions.js (http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/content/extensions.js#894) from openDialog to:
var browserWin = Services.wm.getMostRecentWindow("navigator:browser");
browserWin.openDialog(optionsURL, "", features);
This way it's the browser window that opens the dialog, and all security context is in place.

But I suppose fixing properly openDialog could be better, unfortunately touching this security code looks scary.

Comment 1

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Discussed on IRC: Can't rely on having a navigator:browser window open in toolkit applications, but we can walk up the docshell tree to get the parent window. This should work in cases where the addons manager is in-content, and in its own window (in which case, its already the root parent window).

The alternative is to make windowwatcher do what we want, but that's a long rabbithole with its own set of subtle side-effects.

Comment 2

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Attachment: addon manager workaround v1.0

Not taking the bug for now since I dunno if this solution is fine for you, I've noticed that we use it in other situations (like in mobile about:home). I don't know enough about security implications of touching the windowwatcher to investigate the other approach.

Comment 3

[Dave Townsend [:mossop]]

Can we just use nsIWindowWatcher.openWindow or does that have the same problem?

Comment 4

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

hm, well openWindow wants to know the parent, then it would need chromeWindow again since it would end up again in CalculateChromeFlags. I suspect anything passing through windowwatcher would show the same bug since all calls are (correctly) going to the same code path.

Comment 5

[Wladimir Palant]

Passing dialog parameters through nsIWindowWatcher.openWindow is really ugly, navigating the docshell hierarchy is simple in comparison. And it needs the correct parent window anyway, otherwise you won't be able to open a proper modal dialog.

Comment 6

[Dave Townsend [:mossop]]

Ah I had forgotten about the new arguments for the about dialog.

Why won't the dialog be properly modal if it is parented to the content window? If we parent it to the chrome window then it will display in a different place on OSX, not a big deal I guess.

Comment 7

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

(In reply to comment #6)
> Why won't the dialog be properly modal if it is parented to the content window?

I think (but correct me if I'm wrong) he meant that parenting to the content window would hit this bug again?

Comment 8

[Dave Townsend [:mossop]]

Comment on attachment 527241 [details] [diff] [review]
addon manager workaround v1.0

Not terribly keen on it but there doesn't seem to be a better way. Can we get a test?

Comment 9

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

hm, I suppose I can make a browser-chrome test, it could take some time though.

Comment 10

[Wladimir Palant]

The hardcoded chrome protocol check that's causing this is this one: http://hg.mozilla.org/mozilla-central/annotate/357593f3abbd/embedding/components/windowwatcher/src/nsWindowWatcher.cpp#l527. CalculateChromeFlags() later removes "chrome" and "modal" flags if the target uses a non-chrome protocol and the parent isn't nsIChromeWindow. And using nsIWindowWatcher.openWindow() won't help because it takes the same code path.

Maybe this can be fixed relatively easily after all. Changing the hardcoded check for the "chrome" scheme is hard - this is a security check and URI_IS_LOCAL_RESOURCE isn't restrictive enough. Unfortunately, chrome privileges are associated with the channel, not with the URI, so detecting them at this stage is impossible. But a better logic to calculate aHasChromeParent parameter should be possible, e.g. checking whether the parent uses the system principal rather than whether it implements nsIChromeWindow. I would suggest using nsContentUtils::IsCallerTrustedForWrite() like GlobalWindow::OpenDialog but that won't work for C++ callers.

Comment 11

[Wladimir Palant]

nsContentUtils::IsChromeDoc() is apparently the function to be used here. Something along the lines of:

-  nsCOMPtr<nsIDOMChromeWindow> chromeParent(do_QueryInterface(aParent));
+  PRBool hasChromeParent = PR_TRUE;
+  if (aParent) {
+    nsCOMPtr<nsIDOMDocument> domdoc;
+    aParent->GetDocument(getter_AddRefs(domdoc));
+    nsCOMPtr<nsIDocument> doc(do_QueryInterface(mDocument));
+    hasChromeParent = doc && nsContentUtils::IsChromeDoc(parentDoc);
+  }
 
   // Make sure we call CalculateChromeFlags() *before* we push the
   // callee context onto the context stack so that
   // CalculateChromeFlags() sees the actual caller when doing its
   // security checks.
   chromeFlags = CalculateChromeFlags(features.get(), featuresSpecified,
                                      aDialog, uriToLoadIsChrome,
-                                     !aParent || chromeParent);
+                                     hasChromeParent);

Comment 12

[Wladimir Palant]

> +    nsCOMPtr<nsIDocument> doc(do_QueryInterface(mDocument));
> +    hasChromeParent = doc && nsContentUtils::IsChromeDoc(parentDoc);

Should have been "domdoc" rather than "mDocument" and "doc" rather than "parentDoc" of course. I obviously cannot write code without having the compiler check my variables :)

Comment 13

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

cool, will try this solution as well, in both cases there is probably the need for a test, luckily the test should be independent from the fix.
If we fix ww, most likely we'll want an additional review from jst.

Comment 14

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Attachment: test v1.0

This is the test only, clearly now it fails.
I'll make a patch based on Wladimir findings, then we can choose the approach, the test will stay valid for both.

Comment 15

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Attachment: windowwatcher patch v1.0

This is the patch suggested from Wladimir, it works fine, I've set him as patch author since he practically wrote it :)

I think this approach is globally better since fixes a wrong check at the source. Setting the review request to jst that reviewed other patches in the ww (alternatively sicking could review this I think).

Comment 16

[Dave Townsend [:mossop]]

Comment on attachment 527895 [details] [diff] [review]
test v1.0

Review of attachment 527895 [details] [diff] [review]:

::: toolkit/mozapps/extensions/test/browser/browser_openDialog.js
@@ +13,5 @@
+                 Ci.nsIProtocolHandler.URI_NORELATIVE |
+                 Ci.nsIProtocolHandler.URI_NOAUTH,
+
+  newURI: function CCP_newURI(aSpec, aOriginCharset, aBaseUri)
+  {

Can you put opening braces on the same line as the function declaration throughout please.

@@ +77,5 @@
+}
+XPCOMUtils.defineLazyGetter(CustomChromeProtocol.factory, "registrar", function()
+{
+  return Components.manager.QueryInterface(Ci.nsIComponentRegistrar);
+});

Doesn't seem much point in this being a lazy getter, it will always get used.

@@ +140,5 @@
+          }
+          break;
+        case "domwindowopened":
+          let win = aSubject.QueryInterface(Ci.nsIDOMEventTarget);
+          win.addEventListener("load", function ()

As I said in the other bug, we've hit problems where we must wait for focus on the new window, I think we should do it here too.

Comment 17

[Johnny Stenback (:jst)]

Comment on attachment 527902 [details] [diff] [review]
windowwatcher patch v1.0

Review of attachment 527902 [details] [diff] [review]:

Looks good, r=jst, but I'd like bz to glance over this as well.

Comment 18

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 527902 [details] [diff] [review]
windowwatcher patch v1.0

r=me, I guess.  I _really_ wish we'd just fix the pile of broken that is loading about:addons in a content docshell.  If we did that, this issue would never have arisen.  :(

Do we have anyone working on that, or are we just hoping it will happen somehow?

Comment 19

[Boris Zbarsky [:bzbarsky]]

Note, in particular, that I can't offer any guarantee that this change is not adding a new security vulnerability...

Comment 20

[Dave Townsend [:mossop]]

(In reply to comment #18)
> Comment on attachment 527902 [details] [diff] [review]
> windowwatcher patch v1.0
> 
> r=me, I guess.  I _really_ wish we'd just fix the pile of broken that is
> loading about:addons in a content docshell.  If we did that, this issue would
> never have arisen.  :(
> 
> Do we have anyone working on that, or are we just hoping it will happen
> somehow?

I've filed bug 652736 but I don't know of anyone working on it, nor do I really know how to go about it, can docshell types be changed dynamically or anything?

Comment 21

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Attachment: test v1.1

Comment 22

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Attachment: test v1.2

uups, misformatted the addons array in the previous version.

Comment 23

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Using Places as a staging area, plan to merge to central soon (today or tomorrow) if everything goes well.
http://hg.mozilla.org/projects/places/rev/e48ba6fbc57c
http://hg.mozilla.org/projects/places/rev/ad9ca556b46c

Comment 24

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

pushed a follow-up for the test.  It was failing on Mac and Linux because on these platforms there is instantApply enabled, thus the dialog is not modal.

http://hg.mozilla.org/projects/places/rev/bb1886a87fea

Comment 25

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

http://hg.mozilla.org/mozilla-central/rev/e48ba6fbc57c
http://hg.mozilla.org/mozilla-central/rev/ad9ca556b46c
http://hg.mozilla.org/mozilla-central/rev/bb1886a87fea

Comment 26

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Marco, can you verify this fix with your example jetpack or let me know where I can find it to verify it myself?

Comment 27

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

https://addons.mozilla.org/firefox/addon/places-maintenance/

You can try the difference between FF5.0 and FF6.0

Comment 28

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Thanks. Verified fixed with Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:6.0a1) Gecko/20110512 Firefox/6.0a1.

Marco, what's the coverage of the automated test? Do we also need a manual one?

Comment 29

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

The test opens about:addons, clicks on the options button, and checks the chrome flags on the opened dialog. I feel like it's enough.

Comment 30

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

oh and that is done for both a classic chrome: and a fake-chrome: schemes.

Comment 31

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Ok, declining a Litmus then. Thanks.