Last Comment Bug 369306 - .blur() results in window being lowered and some other window being raised (regardless of value of dom.disable_window_flip) (popunders are possible)
: .blur() results in window being lowered and some other window being raised (r...
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: DOM (show other bugs)
: Trunk
: All All
: -- major with 6 votes (vote)
: mozilla2.0b7
Assigned To: Mounir Lamouri (:mounir)
:
Mentors:
data:text/html,<body onload="blur();"...
: 369854 505210 545337 610562 (view as bug list)
Depends on: 595100
Blocks: popups 502824
  Show dependency treegraph
 
Reported: 2007-02-04 15:43 PST by Chris Thomas (CTho) [formerly cst@andrew.cmu.edu cst@yecc.com]
Modified: 2013-11-12 00:00 PST (History)
23 users (show)
samuel.sidler+old: wanted1.9.0.x-
mounir: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
betaN+
wanted


Attachments
popunder testcase (616 bytes, text/html)
2007-11-09 03:24 PST, James Darpinian
no flags Details
proposed patch (5.38 KB, patch)
2008-01-09 04:40 PST, James Darpinian
no flags Details | Diff | Review
more extensive testcase (1.79 KB, text/html)
2008-01-09 04:43 PST, James Darpinian
no flags Details
Refreshed patch (4.99 KB, patch)
2010-08-11 10:53 PDT, Mounir Lamouri (:mounir)
no flags Details | Diff | Review
Part 1 - If dom.disable_window_flip is enable, block .blur() and .focus() if opener is different from the caller (5.02 KB, patch)
2010-09-07 18:10 PDT, Mounir Lamouri (:mounir)
jst: review+
Details | Diff | Review
Part 2 - Fixing broken tests and add new tests (7.74 KB, patch)
2010-09-07 18:14 PDT, Mounir Lamouri (:mounir)
jst: review+
Details | Diff | Review

Description Chris Thomas (CTho) [formerly cst@andrew.cmu.edu cst@yecc.com] 2007-02-04 15:43:40 PST
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) 2007-02-04 16:13:26 PST
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 cst@andrew.cmu.edu] <cst@yecc.com>.   ...

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 cst@andrew.cmu.edu cst@yecc.com] 2007-02-04 16:52:58 PST
<gavin> looks to me like its just a matter of adding the pref-checking logic in nsGlobalWindow::Focus to nsGlobalWindow::Blur
Comment 3 :Gavin Sharp [email: gavin@gavinsharp.com] 2007-02-09 09:37:20 PST
*** Bug 369854 has been marked as a duplicate of this bug. ***
Comment 4 James Darpinian 2007-11-09 02:44:32 PST
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 2007-11-09 03:24:45 PST
Created attachment 287983 [details]
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 2007-11-09 12:06:49 PST
I can reproduce with James's testcase.  This isn't too surprising; see bug 355482 comment 12.
Comment 7 James Darpinian 2008-01-09 04:40:47 PST
Created attachment 296122 [details] [diff] [review]
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 2008-01-09 04:43:16 PST
Created attachment 296123 [details]
more extensive testcase
Comment 9 Samuel Sidler (old account; do not CC) 2008-08-17 16:13:23 PDT
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 2008-08-18 23:11:26 PDT
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) 2008-10-04 17:39:20 PDT
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 2009-01-20 02:33:20 PST
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, jst@mozilla.com) 2010-02-09 15:44:52 PST
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 2010-02-10 13:25:45 PST
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 15 Johnny Stenback (:jst, jst@mozilla.com) 2010-02-18 15:56:15 PST
*** Bug 545337 has been marked as a duplicate of this bug. ***
Comment 16 Johnny Stenback (:jst, jst@mozilla.com) 2010-02-18 16:12:25 PST
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 [:Tyler] 2010-04-19 23:54:04 PDT
Comment on attachment 296122 [details] [diff] [review]
proposed patch

should jst review then?
Comment 18 Neil Deakin 2010-04-20 09:40:33 PDT
Comment on attachment 296122 [details] [diff] [review]
proposed patch

An updated patch is needed here.
Comment 19 Jason Oster (:Parasyte) 2010-07-01 13:48:24 PDT
(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] 2010-07-29 12:31:18 PDT
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 2010-07-29 12:37:07 PDT
(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] 2010-07-30 09:34:20 PDT
jst, can you help us get the patch unbitrotted?
Comment 23 Johnny Stenback (:jst, jst@mozilla.com) 2010-07-30 17:12:04 PDT
betaN+, not critical for beta3.
Comment 24 Mounir Lamouri (:mounir) 2010-08-11 10:53:23 PDT
Created attachment 464868 [details] [diff] [review]
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 26 Mounir Lamouri (:mounir) 2010-08-20 15:49:54 PDT
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) 2010-09-07 18:10:27 PDT
Created 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
Comment 28 Mounir Lamouri (:mounir) 2010-09-07 18:14:34 PDT
Created attachment 472867 [details] [diff] [review]
Part 2 - Fixing broken tests and add new tests
Comment 29 Johnny Stenback (:jst, jst@mozilla.com) 2010-09-08 13:43:50 PDT
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) 2010-09-08 17:14:03 PDT
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, jst@mozilla.com) 2010-09-08 17:28:14 PDT
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) 2010-09-09 15:33:56 PDT
Pushed:
http://hg.mozilla.org/mozilla-central/rev/9c5a7e114cda
http://hg.mozilla.org/mozilla-central/rev/98cb25088f18

Thank you for your contribution James :)
Comment 33 Jesse Ruderman 2010-11-08 18:26:40 PST
*** Bug 610562 has been marked as a duplicate of this bug. ***
Comment 34 Matthias Versen [:Matti] 2011-03-23 16:03:41 PDT
*** Bug 644339 has been marked as a duplicate of this bug. ***
Comment 35 :Gavin Sharp [email: gavin@gavinsharp.com] 2013-02-01 11:18:21 PST
*** Bug 505210 has been marked as a duplicate of this bug. ***

Note You need to log in before you can comment on or make changes to this bug.