If a page loads another page right after calling navigator.mozApps.install, the Install App doorhanger doesn't disappear when the browser tab's location changes, which could enable a malicious page to fool a user into thinking they're installing an app from a different page than the one that actually triggered the install. Steps To Reproduce: 1. go to http://www.mykzilla.org/app/ 2. press the "install, then redirect immediately" button Expected Results: the doorhanger appears and then immediately disappears (perhaps too quickly to see it at all) as the tab starts loading about:blank. Actual Results: the doorhanger appears, the tab loads about:blank, and the doorhanger remains visible. In theory, the doorhanger should disappear when the location changes, because XULBrowserWindow.onLocationChange calls PopupNotifications.locationChange, which closes doorhangers unless they set persistWhileVisible, and webappsUI doesn't do that <http://hg.mozilla.org/mozilla-central/file/b39f4007be5a/browser/modules/webappsUI.jsm#l139>. So perhaps the location change races the notification? If the installing page waits a second before redirecting the user to about:blank, the doorhanger closes as expected (see the "call install, then redirect after one second" button in my test app for an example of this).

It doesn't even race, the location change always arrives before the webapps-ask-install notification. I've got a patch and I'm writing a test for it.

(In reply to Felipe Gomes (:felipe) from comment #1) > It doesn't even race, the location change always arrives before the > webapps-ask-install notification. Erm, that's what I meant by racing! The web page calls install() first, but the browser observes the location change first. > I've got a patch and I'm writing a test for it. Awesome!

Copy and paste fail on my part on comment 3. Meant to say let's due a k9o review of this bug and then reassess if this blocks the 1st release of the runtime or not.

So fixing this bug was more involved than initially thought. Since the location change arrives before the ask-install notification, the approach I was taking was to compare the URL from the original request to the current browser URL. This however would prevent mozApps from working inside iframes [1], so I couldn't use that. The solution was to get the innerWindowId from the original request and check if that window still exists, because if it navigates away the id will change. GetInnerWindowID existed in nsGlobalWindow but wasn't exposed in nsIDOMWindowUtils like it's sibling function GetOuterWindowID, so I had to add that, and also send the inner id with the install request. Patches incoming. [1] It's possible that we in fact want to block that, but that would be a discussion for a different bug, and certainly not a block to be done by the UI code (as this is a bug about how doorhangers interact with the ask-install notification).

This is the bug fix, where we get the window using the inner id and check if it still exists and also do a sanity check for the URL. I still find it a bit disturbing that the location change happens first and that we need to do this check since we don't have an original reference to the window that requested this (because the messages go through the message manager), but it seems a proper fix.

This adds nsIDOMWindowUtils.getInnerWindowId which we need to use to fix this problem (to get a reference to the iframe that requested the app install). That function already existed in nsGlobalWindow, like getOuter..., but it wasn't exposed in domwinutils

Myk, can you take a look at these tests change? The new test is similar to the one you wrote for bug 765063. I also had to modify your test, because this bug superseded the test for the other one (because a redirect won't show up the install doorhanger anymore). However, the change from bug 765063 is still valid by itself, so I found a different way to test it, using history.pushState instead of a redirect. Also I changed the listener from the observer to a message listener because the object received by the listener has more info than the one received by the observer.

Comment on attachment 640969 [details] [diff] [review] Part 3 - tests Review of attachment 640969 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/tests/mochitest/webapps/test_bug_771294.xul @@ +21,5 @@ > + > +popupNotifications.panel.addEventListener("popupshown", panelListener, false); > + > +setTimeout(function verify() { > + ok(true, "Install panel was blocked after immediate redirect"); This test should directly observe the panel being blocked (or shown), as timeouts like these have a history of unreliability. Based on your change to webappsUI.jsm and my reading of webappsUI.doInstall(), PopupNotifications.show(), and various bits of platform code, it looks like the test should be able to observe "webapps-ask-install", spin the event loop once (i.e. set a zero millisecond timeout), and then fail if a "popupshowing" event has been dispatched. (If that doesn't work, then another route would be to add support to PopupNotifications.jsm for determining that a doorhanger is going to be shown, just as it produces backgroundShow and updateNotShowing notifications for help in testing.)

(In reply to Myk Melez [:myk] [@mykmelez] from comment #9) > This test should directly observe the panel being blocked (or shown), as > timeouts like these have a history of unreliability. Based on your change > to webappsUI.jsm and my reading of webappsUI.doInstall(), > PopupNotifications.show(), and various bits of platform code, it looks like > the test should be able to observe "webapps-ask-install", spin the event > loop once (i.e. set a zero millisecond timeout), and then fail if a > "popupshowing" event has been dispatched. I tried to do this, but spinning the event loop once doesn't work, presumably because there are more events queued during a page redirect. I had to spin it 3 times to make the test work but then that becomes fragile. The problem is that we are testing that nothing happened, so that's why I chose the 1sec timeout over calling setTimeout 3 times. > > (If that doesn't work, then another route would be to add support to > PopupNotifications.jsm for determining that a doorhanger is going to be > shown, just as it produces backgroundShow and updateNotShowing notifications > for help in testing.) This won't work either because with the fix, doInstall() and consequently PopupNotifications.show() won't even be called, so there's no information available in PopupNotifications.jsm that will tell that the popup is about to be displayed.

Comment on attachment 640966 [details] [diff] [review] Part 2 - add nsIDOMWindowUtils.getInnerWindowId One of the core properties of inner windows as designed when we originally implemented them was that an inner window itself is *never* referenceable from JS. Any time we wrap an inner window and expose it to JS, we "outerize", and expose its outer window. This patch attempts to violate that, and the fact that this the fix for this bug (that uses this patch) "works", assuming it does, means we're violating that assumption, and that's not something I want to change w/o significant reason. Granted this is a chrome only API, but still... Blake/Bobby, whether or not we take this fix as is, and I'm arguing we don't, does this show we have a problem where we're able to wrap an inner window w/o actually outerizing in the wrapping process? Or is that no longer necessary with the new wrappers etc? Felipe, could we fix this some other way? If nothing else, a narrower API seems like it would work here, i.e. windowUtils.isWindowIdFrom(windowID, url), or something like that.

jst is right - what this patch attempts to do is verboten. But I don't think it actually does anything, and I'm pretty sure that we properly outerize the window when it gets reflected into JS in NativeInterface2JSObject: http://mxr.mozilla.org/mozilla-central/source/js/xpconnect/src/XPCConvert.cpp#1014 The reason I don't believe that this "works" is that the only thing the patch does with the inner window is access the location object, and the location object actually corresponds to the _outer_ window. There's one location object per inner window, but at any given time they all behave the exact same way (because window.location is a property of navigation, which corresponds to the outer window). So I'm pretty sure that the check in this patch could be identically written as: if (data.from == chromeWin.location.href)

Comment on attachment 640965 [details] [diff] [review] Part 1 - fix the prob Sounds like we need to address jst/bholley's comments here. I think you mentioned that this approach was intended to properly handle installation prompts inside subframes - do we need to support those at all? It's a bit of a spoofing risk, and I'm not sure that we support it for other types of permission prompts.

Thanks for the explanation, turns out I didn't need the inner window. That chromeWin variable confused me so I was associating outer window = top level, inner window = iframe, which is obviously not the case. The outer window id received in the message is still the id for the subframe, so I can use that. I simplified the code around here a bit and removed the _getBrowserForId function

Just to elaborate: I had tried that exact line as bholley suggested, but it didn't work. I too was expecting it to work, but looking more into it, chromeWin, as now I realize the name clearly says, is the chrome window (i.e. browser.xul) not the window object from where navigator.mozApps was called. Myk: any ideas of alternate approaches for the test, or would you reconsider review given comment 10?

Comment on attachment 641576 [details] [diff] [review] New fix Might be a bit more readable to keep a _getWindowForId helper. two other nits: - not related to this bug, but bundle.getFormattedString doesn't take a third argument (<stringbundle>.getFormattedString takes two arguments, nsIStringBundle.formatStringFromName takes 3 - horribly confusing) - could adjust the indentation to go along with the chromeWin.PopupNotifications change Nice cleanup! One stupid idea for testing this is that you can have the bailout in webappsUI_observe (when win.location.href != data.from) fire an observer event, and have the test watch for that. A little bit gross to have test-only code like that, but perhaps the benefits of reliable test coverage outweigh the grossness :)

(In reply to Felipe Gomes (:felipe) from comment #10) > (In reply to Myk Melez [:myk] [@mykmelez] from comment #9) > > (If that doesn't work, then another route would be to add support to > > PopupNotifications.jsm for determining that a doorhanger is going to be > > shown, just as it produces backgroundShow and updateNotShowing notifications > > for help in testing.) > > This won't work either because with the fix, doInstall() and consequently > PopupNotifications.show() won't even be called, so there's no information > available in PopupNotifications.jsm that will tell that the popup is about > to be displayed. But that's ok, because then you can test that PopupNotifications.show() wasn't called. Here's a patch that demonstrates what I'm suggesting. In my tests, it correctly fails without the fix. I haven't yet confirmed that it passes with the fix, although I would expect it to.

Comment on attachment 641660 [details] [diff] [review] alternative test I verified that the test works. Gavin, Myk's test is similar to what you suggested, but instead the notification is dispatched from PopupNotifications.jsm instead of webappsUI.jsm. Does it sound fine to you?

Patch that I will land, fixed all the nits. Carrying forward r+.

Comment on attachment 641660 [details] [diff] [review] alternative test r=me on the popupnotifications.jsm change, and the general approach. I didn't look at the rest of the test.

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/20a09c21d3d5 due to making test_cross_domain.xul fail. The test probably needs a fix similar to the fix to test_bug_765063.xul that was included in this patch.

Relanded. The test failure that happened on the previous push was actually an existing orange. I pushed the same patch to try and ran the tests twice and the random failure didn't happen, so I don't think it is the patch that caused it. https://hg.mozilla.org/integration/mozilla-inbound/rev/1924fe55fb6e

(In reply to :Felipe Gomes from comment #23) > Relanded. The test failure that happened on the previous push was actually > an existing orange. Unfortunately it wasn't an existing orange, the only bug that shows up in TBPL is: "Bug 754578 - [SeaMonkey] mochitest-chrome: "test_cross_domain.xul | Test timed out" (+ 3+ more)" Which is for seamonkey only and for a different failure. The failure we're seeing due to this bug is: 9590 ERROR TEST-UNEXPECTED-FAIL | chrome://mochitests/content/chrome/dom/tests/mochitest/webapps/test_cross_domain.xul | #1343972016077# is expected to be true per template # > Date.now() - 3000# > I pushed the same patch to try and ran the tests twice > and the random failure didn't happen, so I don't think it is the patch that > caused it. The failures seem more sporadic than that, so I suspect there would need to be half a dozen retriggers on {osx 10.6 debug,osx 10.7 debug, winxp opt} (judging by the most frequently failing platforms) on try before this can reland. Some logs from failing runs: https://tbpl.mozilla.org/php/getParsedLog.php?id=14085274&tree=Mozilla-Inbound https://tbpl.mozilla.org/php/getParsedLog.php?id=14084611&tree=Mozilla-Inbound https://tbpl.mozilla.org/php/getParsedLog.php?id=14087171&tree=Mozilla-Inbound https://tbpl.mozilla.org/php/getParsedLog.php?id=14086812&tree=Mozilla-Inbound https://tbpl.mozilla.org/php/getParsedLog.php?id=14087030&tree=Mozilla-Inbound https://tbpl.mozilla.org/php/getParsedLog.php?id=14088291&tree=Mozilla-Inbound https://tbpl.mozilla.org/php/getParsedLog.php?id=14088746&tree=Mozilla-Inbound WinXP: https://tbpl.mozilla.org/php/getParsedLog.php?id=14089886&tree=Mozilla-Inbound Backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/f633b07099d9

Just spotted that one of the tests that landed in this bug appears to be intermittent: { 10216 ERROR TEST-UNEXPECTED-FAIL | chrome://mochitests/content/chrome/dom/tests/mochitest/webapps/test_bug_771294.xul | Install panel was blocked after immediate redirect } https://tbpl.mozilla.org/php/getParsedLog.php?id=14090474&tree=Mozilla-Inbound

Felipe: are you planning to pick this back up at some point? (Just wondering whether or not to look for someone else to do so.)

Marco: can you look at this?

I've basically used Fabrice's patch, I've triggered a try run and it works. I've just modified the tests a bit.

Comment on attachment 783205 [details] [diff] [review] Patch (In reply to Marco Castelluccio [:marco] from comment #28) > I've basically used Fabrice's patch, I've triggered a try run and it works. > I've just modified the tests a bit. Erm, do you mean Felipe's patch? This patch doesn't apply; does it depend on a patch in another bug? We'll need a toolkit peer to review the PopupNotifications.jsm change.

> Erm, do you mean Felipe's patch? Yes, lapsus :D > This patch doesn't apply; does it depend on a patch in another bug? Probably the changes in webappsUI.jsm, I've a really long local queue and I'm basically working as if bug 777402 already landed. I'll post an updated patch soon. > We'll need a toolkit peer to review the PopupNotifications.jsm change. In the meantime a toolkit peer could review that small change.

Comment on attachment 783205 [details] [diff] [review] Patch Felipe: you reviewed the last three changes to PopupNotifications.jsm; perhaps you can review this change to that file? I can review the rest of the patch; I just need review of that toolkit/ change from a Toolkit peer.

Why is the PopupNotifications.jsm change needed? Can't you just use a popupshowing observer on the relevant panel instead?

(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #32) > Why is the PopupNotifications.jsm change needed? Can't you just use a > popupshowing observer on the relevant panel instead? You're right, thank you! So this doesn't need a toolkit peer review anymore.

(Note that the patch still depends on other changes, so it doesn't cleanly apply)

Comment on attachment 783880 [details] [diff] [review] Patch v2 Review of attachment 783880 [details] [diff] [review]: ----------------------------------------------------------------- (In reply to Marco Castelluccio [:marco] from comment #34) > (Note that the patch still depends on other changes, so it doesn't cleanly > apply) Those other changes appear to have landed, as it now applies cleanly. And it looks good and works as expected: the test fails without the change and passes with it. r=myk ::: dom/tests/mochitest/webapps/test_bug_765063.xul @@ +28,3 @@ > > + var msg = aMessage.json; > + var iOService = Components.classes["@mozilla.org/network/io-service;1"] Nit: iOService -> ioService per convention.

Carrying r+

I assume ESR24 and ESR17 are unaffected by this? How about 24 and 25 in general?

They should all be affected.

This is sec-moderate, so we're going to pass for ESR24. If there's any reason to reconsider, please let us know.

navigator.mozApps is not enabled on ESR-24