Note: There are a few cases of duplicates in user autocompletion which are being worked on.

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

RESOLVED FIXED in mozilla2.0b7

Status

()

Core
DOM
--
major
RESOLVED FIXED
11 years ago
4 years ago

People

(Reporter: Chris Thomas (CTho) [formerly cst@andrew.cmu.edu cst@yecc.com], Assigned: mounir)

Tracking

(Blocks: 1 bug)

Trunk
mozilla2.0b7
Points:
---
Dependency tree / graph
Bug Flags:
wanted1.9.0.x -
in-testsuite +

Firefox Tracking Flags

(blocking2.0 betaN+, status1.9.2 wanted)

Details

(URL)

Attachments

(3 attachments, 3 obsolete attachments)

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

11 years ago
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
Duplicate of this bug: 369854

Comment 4

10 years ago
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

10 years ago
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

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

Updated

10 years ago
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)

Comment 7

10 years ago
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

10 years ago
Created attachment 296123 [details]
more extensive testcase
Attachment #287983 - Attachment is obsolete: true

Updated

10 years ago
Attachment #296122 - Flags: review?(jst)

Updated

9 years ago
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]

Comment 10

9 years ago
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.

Updated

9 years ago
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

Comment 12

9 years ago
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?

Updated

9 years ago
Blocks: 176958
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)

Comment 14

8 years ago
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.

Updated

8 years ago
Duplicate of this bug: 545337
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)

Updated

7 years ago
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.

Updated

7 years ago
Assignee: nobody → jst
blocking2.0: beta2+ → beta3+
status1.9.2: --- → wanted
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+
(Assignee)

Comment 24

7 years ago
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.
Attachment #296122 - Attachment is obsolete: true
Attachment #464868 - Flags: review?(jst)
(Assignee)

Updated

7 years ago
Whiteboard: [needs r jst]
(Assignee)

Comment 25

7 years ago
Windows builds can be found here:
http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/mlamouri@mozilla.com-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
(Assignee)

Comment 26

7 years ago
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
(Assignee)

Comment 27

7 years ago
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
Attachment #464868 - Attachment is obsolete: true
Attachment #472866 - Flags: review?(jst)
Attachment #464868 - Flags: review?(jst)
(Assignee)

Comment 28

7 years ago
Created attachment 472867 [details] [diff] [review]
Part 2 - Fixing broken tests and add new tests
Attachment #472867 - Flags: 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

+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-
(Assignee)

Comment 30

7 years ago
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+

Updated

7 years ago
Attachment #472867 - Attachment is patch: true
Attachment #472867 - Attachment mime type: application/octet-stream → text/plain

Updated

7 years ago
Attachment #472867 - Flags: review?(jst) → review+
(Assignee)

Comment 32

7 years ago
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
Last Resolved: 7 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b6

Updated

7 years ago
Blocks: 502824

Updated

7 years ago
Depends on: 595100

Updated

7 years ago
Duplicate of this bug: 610562
Duplicate of this bug: 644339
Duplicate of this bug: 505210
You need to log in before you can comment on or make changes to this bug.