Closed
Bug 92955
Opened 24 years ago
Closed 23 years ago
Disable window.open from onload and onunload
Categories
(Core :: DOM: Core & HTML, enhancement, P1)
Core
DOM: Core & HTML
Tracking
()
VERIFIED
FIXED
mozilla0.9.5
People
(Reporter: rginda, Assigned: security-bugs)
References
(Blocks 1 open bug, )
Details
(Whiteboard: fixinhand)
Attachments
(5 files)
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review |
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.
Reporter | ||
Comment 1•24 years ago
|
||
Reporter | ||
Comment 2•24 years ago
|
||
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.
Status: NEW → ASSIGNED
Reporter | ||
Comment 3•24 years ago
|
||
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"
Reporter | ||
Comment 4•24 years ago
|
||
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.
Reporter | ||
Comment 5•24 years ago
|
||
Assignee | ||
Comment 6•24 years ago
|
||
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?
Reporter | ||
Comment 7•24 years ago
|
||
Reporter | ||
Comment 8•24 years ago
|
||
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?
Assignee | ||
Comment 9•23 years ago
|
||
I'm taking this over.
Assignee: rginda → mstoltz
Status: ASSIGNED → NEW
Target Milestone: --- → mozilla0.9.4
Assignee | ||
Comment 10•23 years ago
|
||
Assignee | ||
Comment 11•23 years ago
|
||
I think patch 4 is check-in-able. The question is, do we want it on by default in Mozilla? Needs reviews and approval.
Status: NEW → ASSIGNED
Priority: -- → P1
Whiteboard: patch
Comment 12•23 years ago
|
||
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.
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).
Assignee | ||
Comment 14•23 years ago
|
||
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."
"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•23 years ago
|
||
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.
Assignee | ||
Comment 17•23 years ago
|
||
> 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.
Assignee | ||
Comment 18•23 years ago
|
||
Assignee | ||
Comment 19•23 years ago
|
||
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.
Whiteboard: patch → fixinhand
Sweet. Thanks, Mitch. (I guess we'll just leave this bug open, and mutate the summary a little bit when you check in?)
Assignee | ||
Comment 24•23 years ago
|
||
The fix is checked in. Bug 64737 will track continuing efforts to generalize this mechanism.
Status: ASSIGNED → RESOLVED
Closed: 23 years ago
Resolution: --- → FIXED
Target Milestone: mozilla0.9.4 → mozilla0.9.5
Comment 26•23 years ago
|
||
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 29•23 years ago
|
||
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•23 years ago
|
||
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee | ||
Comment 31•23 years ago
|
||
Slice, bug 64737. Jeff, embedding may be out of luck on this one but I'll take a look.
Status: REOPENED → ASSIGNED
Comment 32•23 years ago
|
||
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.
Assignee | ||
Comment 34•23 years ago
|
||
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).
Status: ASSIGNED → RESOLVED
Closed: 23 years ago → 23 years ago
Resolution: --- → FIXED
Comment 35•23 years ago
|
||
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.
Status: RESOLVED → VERIFIED
Updated•6 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•