92955 - Disable window.open from onload and onunload

Status: VERIFIED FIXED

(Core :: DOM: Core & HTML, enhancement, P1)

Description

[Robert Ginda]

Our disable window.open functionality is nice, but not very practical.  With the
threat of pop-unders looming, "don't allow domain X to window.open" will become
less of an option, as more sites will have pop-up/unders.  Maintaining the
domain list will become a hassle, and some sites with pop-foo ads may have
other, legitimate uses for window.open.  My proposed solution is to add the
ability to deny window.open when called from onload and onunload events.  This
"good enough" solution should eliminate most of the bogus uses of window.open,
without breaking legitimate uses.

My proposed implementation (patch coming up) involves searching the JS stack
(via XPConnect) for the top JS frame before allowing the window.open to proceed.
 If the top JS frame is a function named "onload" or "onunload" (case
insensitive match), bail out, otherwise continue as planned.  The problems I see
with this approach are:

 * Any script with a top level function named "onload" or "onunload" that is not
attached to window, that also calls window.open, will be blocked.  This false
positive is in all probability rare, and should not cause problems in the real
world.  
 * Sites can get around this detection by using setTimeout (or, even worse,
setInterval) which has no constant function name to match against.

An alternative, and possibly cleaner solution would be to add a member to the
window implementation that represents the current "in commonly abused function"
counter.  This counter, initially 0, would increment by one each time the
window.onload, window.onunload, and setTimeout/Interval calls were about to be
made, and decrement by one as these routines completed.  Window.open could then
verify that the counter was 0 before allowing the new window to be created. 
This method is much more robust, but involves some ideas that I'm not sure how
to implement.  Specifically, is it possible to hook onload, etc. in this way?  I
am under the impression that this code is all generic, and hooking 2 specific
events (onload and onunload) is nontrivial.

Comment 1

[Robert Ginda]

Attachment: proposed patch; take one

Comment 2

[Robert Ginda]

Here is a proposed fix.  It needs to be wrapped in a pref for sure, but I'd like
some feedback on the basic idea before I spend too much time on this.

Comment 3

[Robert Ginda]

Here are some related bugs:
Bug 29346 "Prevent repeating pop-up windows"
Bug 33448 "disable 'new window' when close box clicked (no window.open in onUnload)"
Bug 64737 "Capabilities configurable by trigger"

Comment 4

[Robert Ginda]

New patch coming up.

This patch hooks into HandleDOMEvent, in order maintain a mIsLoadComplete
boolean member.  mIsLoadComplete is PR_TRUE between the completion of
window.onload and the start of window.onunload.  This gives us the ability to
block top level scripts, as well as onload and onunload, and eliminates the JS
stack walk.

The patch also blocks window.open if mRunningTimeout is non-null, to keep
designers from getting around this stuff with a setTimeout() in onload.

Comment 5

[Robert Ginda]

Attachment: proposed patch; take two

Comment 6

[Mitchell Stoltz (not reading bugmail)]

This will probably block the majority of popups/popunders right now, until the
advertisers get wise to it and/or we regain enough market share to make it worth
their while. Then, they will open popups on mouseover, on focus, etc. In the
meantime, I think this is a good idea. We'll need to make this controllable by
pref, because Netscape will probably want it off.

Also, we could potentially tie this into caps rather than hardcoding it into
window.open. Caps could call CheckForAbusePoint() and use that as another piece
of input to the security check for property access. That way, we or the users
could block other properties besides window.open from running in the "abuse
points" simply by adding a line to prefs. I can do this if people think there's
a value to it. Thoughts?

Comment 7

[Robert Ginda]

Attachment: proposed patch; take three

Comment 8

[Robert Ginda]

The last patch was too conservative in checking for onload, it never saw chrome
documents load, so you couldn't open new windows from chrome, like say,
mailnews.  I'm running with this patch in my day to day opt build, with no
obvious problems yet.

It'd be nice to integrate this into caps, but I don't know caps well enough to
understand how it'd work.  This patch adds CheckForAbusePoint() as a protected
member of nsGlobalWindowImpl, how will caps call it?

Comment 9

[Mitchell Stoltz (not reading bugmail)]

I'm taking this over.

Comment 10

[Mitchell Stoltz (not reading bugmail)]

Attachment: Patch take 4 - updated & added pref

Comment 11

[Mitchell Stoltz (not reading bugmail)]

I think patch 4 is check-in-able. The question is, do we want it on by default
in Mozilla? Needs reviews and approval.

Comment 12

[Johnny Stenback (:jst)]

A couple of issues with the patch:

- Any reason to not combine the check for aEvent->message being NS_PAGE_LOAD or
NS_PAGE_UNLOAD into one place in stead of having them separated? (and yes,
please do keep the braces around the if statement)

- In GlobalWindowImpl::Open() the suggested changes are:

+
+  /* If we're in a commonly abused state (top level script, running a timeout,
+   * or onload/onunload), and the preference is enabled, block the window.open().
+   */
+  nsCOMPtr<nsIPref> prefs(do_GetService(kPrefServiceCID));
+  if (prefs)
+  {
+      PRBool blockOpenOnLoad = PR_FALSE;
+      prefs->GetBoolPref("privacy.block_open_during_load", &blockOpenOnLoad);
+    if (blockOpenOnLoad && CheckForAbusePoint()) {
+#ifdef DEBUG
+        printf ("*** Blocking window.open.\n");
+#endif
+        return NS_OK;
+    }
+  }
+    

This gets the pref service even if it's not necessarily needed, I would suggest
this instead:
+
+  /* If we're in a commonly abused state (top level script, running a
+   * timeout, or onload/onunload), and the preference is enabled, block
+   * the window.open().
+   */
+  if (CheckForAbusePoint()) {
+    nsCOMPtr<nsIPref> prefs(do_GetService(kPrefServiceCID));
+
+    if (prefs) {
+      PRBool blockOpenOnLoad = PR_FALSE;
+      prefs->GetBoolPref("privacy.block_open_during_load", &blockOpenOnLoad);
+
+      if (blockOpenOnLoad) {
+#ifdef DEBUG
+        printf ("*** Blocking window.open.\n");
+#endif
+
+        *_retval = nsnull;
+
+        return NS_OK;
+    }
+  }
+    

Especially note the setting of *_retval in the early return case, this is important.
And, a few nits on top of that:

- Both 4 and 2 space indentation used in GlobalWindowImpl::CheckForAbusePoint(),
use 2 space indentation in this file.

- Bad indentation in GlobalWindowImpl::Open(), but that's fixed in my suggested
change above.

- mIsLoadComplete could maybe be renamed to mIsDocumentLoaded to make the name
match closer with what it's actually used for.

sr=jst with the above changes.

Comment 13

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

shaver-nit: there's nothing privacy-related here, that I can think of.  Let's
not dilute the term any more than we have to (or have already: c.f. the form
manager).

Comment 14

[Mitchell Stoltz (not reading bugmail)]

Sure it's privacy-related. Pop-up ads are a violation of my privacy (in that
they are annoying, not that they steal personal info). Can you suggest another
term? It's not "security." 

Comment 15

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

"Privacy" in the internet context almost always refers to personal information:
none of the interesting legislation or lobbying is really concerned with
"annoying things on the internet", other than spam -- and there it's because an
email address is PII.  I think we'd do well to keep our "privacy" label applied
only to personal information.

(Aside: Why isn't it OK to say it's "securing" the browser against popups, but
it's OK to say that it's keeping you "private" against popups?  If this isn't
security, how are Window.alert controls in our configurable "security" policies
security?  I don't get it.)

Anyway, it's a site capability.  Why isn't this in with the rest of the
configurable-"security" stuff, like general access to Window.open, &c.?  We
could either have a different capability

    user_pref("capability.policy.default.Window.open_during_load", "noAccess")

or a different "security level" value
    user_pref("capability.policy.default.Window.open", "notDuringLoad")

.  I think either would fit better with our current
what-can-this-site-do-to-my-browser? story.

Comment 16

[Jacek Piskozub]

I would argue that this is a security issue, after all. Denial of Service
attacks fall in this category. Opening pop-up windows ad infinitum crashed a lot
of browsers. This also may leads to dataloss.

Comment 17

[Mitchell Stoltz (not reading bugmail)]

> If this isn't
> security, how are Window.alert controls in our configurable "security" policies
> security?  I don't get it.

They're not security. They're just a useful side effect of the security manager.
We've had this discussion, remember? I'd rather see "content control" stuff
handled seaprately from security, at least on the frontend.

> Anyway, it's a site capability.  Why isn't this in with the rest of the
> configurable-"security" stuff
Ideally it would be. In fact, I plan to integrate this with the configurable
policies stuff, but we're obviously out of time in this milestone and I wanted
to get a basic functionality into this milestone because I think our users will
really appreciate it.

Comment 18

[Mitchell Stoltz (not reading bugmail)]

Attachment: Patch v5 with jst's suggestions.

Comment 19

[Mitchell Stoltz (not reading bugmail)]

I changed the pref name to "dom.disable_open_during_load" so we're not diluting
the meaning of 'privacy.' Eventually I will integrate this into the
"capability.policy" functionality but there's no time now.

Johnny, as to your first point, it looks like the check for NS_PAGE_UNLOAD needs
to come before the event handler is called, and NS_PAGE_LOAD afterwards.

Comment 20

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Sweet.  Thanks, Mitch.  (I guess we'll just leave this bug open, and mutate the
summary a little bit when you check in?)

Comment 21

[harishd]

r=harishd

Comment 22

[Johnny Stenback (:jst)]

Mitch, yeah, doh, I should've realized...

Comment 23

[Brendan Eich [:brendan]]

a=[e-mail] on behalf of drivers, for 0.9.4 checkin.

/be

Comment 24

[Mitchell Stoltz (not reading bugmail)]

The fix is checked in. Bug 64737 will track continuing efforts to generalize
this mechanism.

Comment 25

[Peter Lairo]

just for the record: 

What is the pref to disable popups on window close?

Comment 26

[Jacek Piskozub]

The pref is dom.disable_open_during_load. It's Boolean (1 or 0). I got this
fromm the text of the latest patch.

Comment 27

[Mitchell Stoltz (not reading bugmail)]

Actually, the value must be true or false, not 1 or 0. 

Comment 28

[Mitchell Stoltz (not reading bugmail)]

*** Bug 33448 has been marked as a duplicate of this bug. ***

Comment 29

[slice1900]

Since as others have noted, once advertisers get wise to this they'll do
something else like opening a window on focus, why not just have an option to
ONLY allow window.open when clicked.  That's the only time we ought to want it,
anything else is just annoying and rife with abuse.  Is this fix general enough
that an enhancement request could be made to add that as another option?  Or
perhaps there is another similar bug already opened?  I looked but wasn't able
to find one.

Comment 30

[Jeff Doozan]

In embedding builds, this patch seems to be a bit over zealous, and disables all
popup windows.

Steps to recreate:

Add the line
user_pref("dom.disable_open_during_load", true);
to prefs.js in you mfcembed profile directory

Launch mfcembed
go to www.collegeclub.com
click the "Log On" link

Expected results:
A popup window is created to let the user enter a username and password

Actual results:
No popup window is created.

Comment 31

[Mitchell Stoltz (not reading bugmail)]

Slice, bug 64737. Jeff, embedding may be out of luck on this one but I'll take a
look.

Comment 32

[Jeremy M. Dolan]

Why is this still open?

Re: Jeff's comment on embeded builds... not seeing that on linux 2001091305...
if it's still happening on embeded, file a seperate bug.

Comment 33

[Jeff Doozan]

This seems to be working on windows embedding now, too.

Comment 34

[Mitchell Stoltz (not reading bugmail)]

Marking Fixed again based on comments. If this is still broken in embedding,
file a separate bug. I'm not sure that embedding wants this (at least not the
AOL embeddors; maybe others do).

Comment 35

[bsharma]

Verified on 2001-09-20-Branch build on WinNT

Followed following steps and the pop up window is not opened
1. Added line user_pref("dom.disable_open_during_load", true); to prefs.js file.
2. Loaded www.collegeclub.com and clicked on the LogOn button
3. Nothing happens.