Closed Bug 369306 Opened 17 years ago Closed 14 years ago

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

Categories

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

defect
Not set
major

Tracking

()

RESOLVED FIXED
mozilla2.0b7
Tracking Status
blocking2.0 --- betaN+
status1.9.2 --- wanted

People

(Reporter: csthomas, Assigned: mounir)

References

(Blocks 1 open bug, )

Details

Attachments

(3 files, 3 obsolete files)

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).
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.   
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Windows XP → All
Hardware: PC → All
<gavin> looks to me like its just a matter of adding the pref-checking logic in nsGlobalWindow::Focus to nsGlobalWindow::Blur
Assignee: general → general
Component: General → DOM
Product: Mozilla Application Suite → Core
QA Contact: general → ian
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...
Attached file popunder testcase (obsolete) —
illustrates that blur() and self.focus() work despite dom.disable_window_flip, and allow the creation of evil popunders.
I can reproduce with James's testcase.  This isn't too surprising; see bug 355482 comment 12.
Summary: .blur() results in window being lowered and some other window being raised (regardless of value of dom.disable_window_flip) → .blur() results in window being lowered and some other window being raised (regardless of value of dom.disable_window_flip) (popunders are possible)
Attached patch proposed patch (obsolete) — Splinter Review
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.
Attachment #287983 - Attachment is obsolete: true
Attachment #296122 - Flags: review?(jst)
Flags: wanted1.9.0.x?
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.
Flags: wanted1.9.0.x? → wanted1.9.0.x-
Whiteboard: [needs r jst]
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.
Flags: wanted1.9.0.x- → wanted1.9.0.x?
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.
Flags: wanted1.9.0.x? → wanted1.9.0.x-
Version: 1.8 Branch → Trunk
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?
Blocks: popups
Assignee: general → nobody
QA Contact: ian → general
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.
Attachment #296122 - Flags: review?(jst)
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.
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.
blocking2.0: --- → beta1
Comment on attachment 296122 [details] [diff] [review]
proposed patch

should jst review then?
Attachment #296122 - Flags: review?(jst)
Comment on attachment 296122 [details] [diff] [review]
proposed patch

An updated patch is needed here.
Attachment #296122 - Flags: review?(jst)
blocking2.0: beta1+ → beta2+
(In reply to comment #8)
> Created an attachment (id=296123) [details]
> more extensive testcase

Interestingly, I get popups on Linux.  And popunders on Windows.
Assignee: nobody → jst
blocking2.0: beta2+ → beta3+
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?
(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.
jst, can you help us get the patch unbitrotted?
betaN+, not critical for beta3.
blocking2.0: beta3+ → betaN+
Attached patch Refreshed patch (obsolete) — Splinter Review
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.
Attachment #296122 - Attachment is obsolete: true
Attachment #464868 - Flags: review?(jst)
Whiteboard: [needs r jst]
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.
Assignee: jst → mounir.lamouri
Status: NEW → ASSIGNED
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.
Attachment #472866 - Flags: review?(jst) → review-
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.
Attachment #472866 - Flags: review- → review?(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.
Attachment #472866 - Flags: review?(jst) → review+
Attachment #472867 - Attachment is patch: true
Attachment #472867 - Attachment mime type: application/octet-stream → text/plain
Attachment #472867 - Flags: review?(jst) → review+
Pushed:
http://hg.mozilla.org/mozilla-central/rev/9c5a7e114cda
http://hg.mozilla.org/mozilla-central/rev/98cb25088f18

Thank you for your contribution James :)
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b6
Blocks: 502824
Depends on: 595100
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.