369306 - .blur() results in window being lowered and some other window being raised (regardless of value of dom.disable_window_flip) (popunders are possible)

Status: RESOLVED FIXED

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

Description

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.2pre) Gecko/20070111 SeaMonkey/1.1 Mnenhy/0.7.4.10000

Paste the URL in the URL bar, and open it in a new tab (ctrl+[shift+]enter).  Make sure you have another window open from the same process (e.g. chatzilla, mailnews, JS console).

Comment 1

[Rich Gray (:rbgray)]

Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8.1.2pre) Gecko/20070111 SeaMonkey/1.1

When I just plain click on the URL in this bug, I get an alert:

The URL field currently contains a URL using either a javascript or data URI scheme, which means your Bugzilla session is vulnerable to various types of attack if you click the link. The field was last changed by Chris Thomas (CTho) [formerly [e-mail]] <[e-mail]>.   ...

When I click on "proceed", I wind up in the Chatzilla window I had open with the "asdf" window underneath.   

Comment 2

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

<gavin> looks to me like its just a matter of adding the pref-checking logic in nsGlobalWindow::Focus to nsGlobalWindow::Blur

Comment 4

[James Darpinian]

I am not sure comment #2 is correct, as a call to self.focus() *also* ignores the value of dom.disable_window_flip and raises the window.  This is the source of the recently renewed plague of popunder ads (for example http://www.trafficmasterz.net/test_popunder.php, though their source is childishly obfuscated, uses self.focus() to implement the popunder).  And Firefox had come so close to eliminating them forever...

Comment 5

[James Darpinian]

Attachment: popunder testcase

illustrates that blur() and self.focus() work despite dom.disable_window_flip, and allow the creation of evil popunders.

Comment 6

[Jesse Ruderman]

I can reproduce with James's testcase.  This isn't too surprising; see bug 355482 comment 12.

Comment 7

[James Darpinian]

Attachment: proposed patch

This patch adds checking to nsGlobalWindow::Blur() as proposed in comment 2, and also adds an extra check to nsGlobalWindow::Focus() which only allows documents to focus windows they have opened.  I added a new method nsContentUtils::GetWindowFromCaller() in support of this.  

This patch fixes the testcase.  This is my first Mozilla contribution, so I'm definitely open to any feedback about better ways to do stuff, or things I've done wrong.

Comment 8

[James Darpinian]

Attachment: more extensive testcase

Comment 9

[Samuel Sidler (old account; do not CC)]

Right now, this bug looks like it only applies to 1.8, so not wanted for 1.9.0.x. If that's not the case, please re-nom.

Comment 10

[James Darpinian]

I'm not sure what you mean; this bug is reproducible in the latest nightly build with the attached testcase.  I can't re-nominate this bug myself.

Comment 11

[Samuel Sidler (old account; do not CC)]

Doesn't really meet the "wanted" criteria (security, stability, regression from maintenance release) for 1.9.0.x. However, we'll look at a reviewed and baked patch.

Comment 12

[scratch]

This is indeed a regression, and it seems like the regression itself was introduced in a maintenance release (1.8.1.1).

Seems as if you can't even use CAPS to block access to Window.blur for some reason.  Is there any way to make an extension out of the patch until it gets checked in?

Comment 13

[Johnny Stenback (:jst)]

Comment on attachment 296122 [details] [diff] [review]
proposed patch

Is this still an issue? Loading the testcase it seems like all tests produce popups, not popunders, which I believe is what we want here. Clearing review flag (which got lost and was lost for way too long, my apologies). Please re-request reviews if we still need this.

Comment 14

[James Darpinian]

This is still an issue.  Using the latest Fx nightly build all the testcases produce popunders for me (on Windows).  

I don't know how to re-request review on this; I'm not sure I have the bugzilla permissions for that.

Comment 16

[Johnny Stenback (:jst)]

We should get this fix in for 1.9.3. And as part of the fix for this, we need automated tests that check that this doesn't regress, and one part we should do in that testing is to see what happens if we not only focus a window, but the document or an element inside that window to make sure that works as expected as well.

Comment 17

[Tyler Downer [He/Him]]

Comment on attachment 296122 [details] [diff] [review]
proposed patch

should jst review then?

Comment 18

[Neil Deakin]

Comment on attachment 296122 [details] [diff] [review]
proposed patch

An updated patch is needed here.

Comment 19

[Jason Oster (:Parasyte)]

(In reply to comment #8)
> Created an attachment (id=296123) [details]
> more extensive testcase

Interestingly, I get popups on Linux.  And popunders on Windows.

Comment 20

[Mike Beltzner [:beltzner, not reading bugmail]]

Do we expect this to hit code freeze? Marked beta3+ 10 days ago, no activity since then.

Enn: what sort of update to the patch is needed?

Comment 21

[Neil Deakin]

(In reply to comment #20)
> Enn: what sort of update to the patch is needed?

The current patch in the bug is over two years old. The approach seems fine though.

Comment 22

[Mike Beltzner [:beltzner, not reading bugmail]]

jst, can you help us get the patch unbitrotted?

Comment 23

[Johnny Stenback (:jst)]

betaN+, not critical for beta3.

Comment 24

[Mounir Lamouri (:mounir)]

Attachment: Refreshed patch

The patch is now refreshed. I don't know if that's working because the bug can't be reproduced on GNU/Linux systems. I 'm going to send it to the try server so we will have some builds to test it.

Comment 25

[Mounir Lamouri (:mounir)]

Windows builds can be found here:
http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/[e-mail]-259126188a9f/

It looks like this patch is breaking a Cocoa-specific test (modules/plugin/test/test_cocoa_window_focus.html), see:
http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1281555871.1281558648.7526.gz
http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1281555985.1281556638.29756.gz
http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1281555250.1281556854.30772.gz

Comment 26

[Mounir Lamouri (:mounir)]

I will work on that after beta5 (fixing broken tests and writing new ones).
By the way, I tested on MacOS X and the patch is working.

Comment 27

[Mounir Lamouri (:mounir)]

Attachment: Part 1 - If dom.disable_window_flip is enable, block .blur() and .focus() if opener is different from the caller

Comment 28

[Mounir Lamouri (:mounir)]

Attachment: Part 2 - Fixing broken tests and add new tests

Comment 29

[Johnny Stenback (:jst)]

Comment on attachment 472866 [details] [diff] [review]
Part 1 - If dom.disable_window_flip is enable, block .blur() and .focus() if opener is different from the caller

+nsPIDOMWindow *
+nsContentUtils::GetWindowFromCaller()
+{
+  JSContext *cx = nsnull;
+  sThreadJSContextStack->Peek(&cx);
+
+  if (cx) {
+    nsCOMPtr<nsPIDOMWindow> win =
+      do_QueryInterface(nsJSUtils::GetDynamicScriptGlobal(cx));
+    return win;
+  }
+
+  return nsnull;
+}

This is effectively the same code as GetDocumentFromContext(), but it should really be the same as GetDocumentFromCaller(), which return the correct window in more cases than the above code. How about we rename GetDocumentFromCaller() to GetWindowFromCaller() and make a new GetDocumentFromCaller() function that calls GetWindowFromCaller() and then calls GetExtantWindow() on the returned document, assuming it's not null?

This patch looks good otherwise, but r- until the above is dealt with.

Comment 30

[Mounir Lamouri (:mounir)]

Comment on attachment 472866 [details] [diff] [review]
Part 1 - If dom.disable_window_flip is enable, block .blur() and .focus() if opener is different from the caller

It looks like GetDocumentFromCaller isn't really doing what we were expecting.
Re-asking a review then.

Comment 31

[Johnny Stenback (:jst)]

Comment on attachment 472866 [details] [diff] [review]
Part 1 - If dom.disable_window_flip is enable, block .blur() and .focus() if opener is different from the caller

Yup, GetDocumentFromCaller() isn't what we want here after all.

Comment 32

[Mounir Lamouri (:mounir)]

Pushed:
http://hg.mozilla.org/mozilla-central/rev/9c5a7e114cda
http://hg.mozilla.org/mozilla-central/rev/98cb25088f18

Thank you for your contribution James :)