Don't allow alert/confirm/prompt in onbeforeunload, onunload and onpagehide

RESOLVED FIXED in mozilla17

Status

()

Core
DOM
--
enhancement
RESOLVED FIXED
10 years ago
3 years ago

People

(Reporter: Volkmar Kostka, Assigned: Neil Deakin (not available until Aug 9))

Tracking

(Depends on: 1 bug, Blocks: 1 bug, {dev-doc-needed, sec-want})

unspecified
mozilla17
dev-doc-needed, sec-want
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:want P4], URL)

Attachments

(5 attachments, 6 obsolete attachments)

205 bytes, text/html
Details
107 bytes, text/html
Details
165 bytes, text/html
Details
15.78 KB, patch
smaug
: review+
Details | Diff | Splinter Review
18.85 KB, patch
Details | Diff | Splinter Review
(Reporter)

Description

10 years ago
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9a8pre) Gecko/2007081005 Minefield/3.0a8pre
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9a8pre) Gecko/2007081005 Minefield/3.0a8pre

Using onbeforeunload with a loop can render the browser useless.

As example:
<body onbeforeunload="for ( var i = 0; i < 999999; i++) { alert('Alert'); }
return false;">

If JS is enabled and you tries top close the tab you run into an endless loop with an alert which blocks the complete browser.
It may be even possible that you can trigger it automatically with a redirect clause.
It is not a bug just malicious code. IE6 is also affected but i could not get it to work with Opera.


Reproducible: Always

Steps to Reproduce:
1) Visit the mentioned site with JS on.
2) Try to close the tab.

Actual Results:  
You get an long chain of alerts and the tab is not closed.

Expected Results:  
Allow to ignore the return value from onbeforeunload and allow to close the tab nevertheless.

If you do not know how to fix it (disable JS before closing the tab) you are not even able to get rid of it when sessionstore is enabled. If the thing with redirect is possible you have to abandon your stored session to use your browser again.

Comment 1

10 years ago
Being able to get out of an infinite loop of alerts is covered by other bugs (bug 123913, bug 61098, bug 59314), so I'm changing the summary of this bug to focus on the onbeforeunload part.
Summary: Need way to forcefully close tabs → Don't allow alert/confirm/prompt in onbeforeunload
I was able to reproduce this on the Branch:

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.7pre) Gecko/20070811 BonEcho/2.0.0.7pre ID:2007081103

Nasty - a malicious hacker could really make hay with this.
I wonder in what cases websites will use this? What could be the advantage for them? After such an experience visitors with avoid the website like the plague.
(Reporter)

Comment 4

10 years ago
It is just malicious code.
Let's say someone was able to hack a web server and places this code on any page. If there are internal web pages which are required for the day-to-day use this will wreck havoc in that company.
Sort of DoS attack.

Updated

10 years ago
Duplicate of this bug: 402257

Comment 6

10 years ago
Malicious sites such as http://performanceoptimizer.com/.landing/index.php?4656530f4a0653445b59165866474a6f540b6f500646560a43480b5a5b0a551f5f576d53563c06020c053d050a030d6f0352050c3d5756673a590b695559035f69096d070a520b69595e3d115842560d004355450d0b081e150556 are already abusing onunload in this way, but we decided in bug 130719 to allow that???
Whiteboard: [sg:want P4]

Updated

10 years ago
Duplicate of this bug: 410247

Updated

10 years ago
Summary: Don't allow alert/confirm/prompt in onbeforeunload → Don't allow alert/confirm/prompt in onbeforeunload and onunload

Updated

10 years ago
Status: UNCONFIRMED → NEW
Ever confirmed: true

Updated

10 years ago
Blocks: 61098
Just an idea.
(A) For DoS by infinite dialog loop
    Button/option to "Stop (malicious) JavaScript Execution" or "Stop next
    alert/confirm/prompt" in alert/confirm/prompt dialog
(B) For DoS by infinite loop
    (B-1) "Stop (malicious) JavaScript Execution" menu or button,
          with highest task priority among all of Back-end/UI/User-Script tasks
    (B-2) Option to limit loop counter(e.g. site_script.max_loop_count),
          for site specified JavaScript(and/or for Script by extension)
(C) For DoS by "very-small/ZERO value" of setInterval/setTimeout
    Minimum interval/timeout option
(D) For Dos by recursive&infinite call/include
    (D-1) Option of max-depth for iframe, link-rel-style, object,
          Script-generated-Script-Tag, object etc. 
    (D-2) Loop check for recursive include

Comment 9

10 years ago
Making it possible to abort scripts from the alert() dialog is bug 61098.  That's mostly orthogonal to this bug.

Updated

10 years ago
Depends on: 411855

Comment 10

10 years ago
alert() should be allowed in on(before)unload code. Many sites use it to prevent the user from losing data by accidentally closing the tab.

The decision of what to do with abusive loops is the subject of bug 61098.

Comment 11

10 years ago
Many sites *return a string* from onbeforeunload, which triggers a dialog that lets users stay on the page.  alert() doesn't let users stay on the page, so I don't see why sites would do that.  Do you know of sites that do?

Comment 12

10 years ago
@Jesse: D'oh! Okay, in that case I have no objection to disabling all modal interface components in onunload/onbeforeunload.
The "rickroll" bug. We should fix this. :-)

Updated

9 years ago
Blocks: 432687
No longer depends on: 411855

Comment 14

8 years ago
Onunload/onbeforeunload can have legitimate uses, yes, but they're also popular (very popular?) vectors for malicious websites to launch Javascript code and/or XSS with.

If the latter potential outweighs the former (and I would suggest it does, if not clearly so) then this is a definite hazard that must be made a user configurable option, e.g. (Tools > Options > Javascript > Advanced) or at least via about:config.

I was recently hit by a random prompt("Are you sure you want to navigate away from this page?") (exact words) while browsing across the Deviantart domain (normally a safe domain in this respect), which looped until I hit the "Cancel" (as in "stay on this page") button, upon which it redirected my tab to somewhere in the MySpace domain.  I shut off Javascript using another window to mitigate any further damage, but being unable to prevent this sort of attack in the first place is more frustrating than the attack itself.
Component: Tabbed Browser → DOM
Product: Firefox → Core
QA Contact: tabbed.browser → general

Updated

7 years ago
No longer blocks: 61098

Updated

7 years ago
Blocks: 588292

Comment 15

7 years ago
Created attachment 477005 [details]
onunload.html

Updated

7 years ago
Summary: Don't allow alert/confirm/prompt in onbeforeunload and onunload → Don't allow alert/confirm/prompt in onbeforeunload, onunload and onpagehide
(In reply to comment #11)
> Many sites *return a string* from onbeforeunload, which triggers a dialog that
> lets users stay on the page.  

For example another product: JIRA (similar to bugzilla) uses this when I use Chrome. The dialog shows up when I try to leave a page that I was updating to do something else, but didn't want to lose my changes.

Comment 17

7 years ago
This bug isn't about removing the return-a-string "are you sure you want to leave" dialog. It's about removing the ability to pop up *other* dialogs from the beforeunload event, which are problematic.

If you want to argue about whether we should have onbeforeunload at all, go to bug 578828 ;)

Comment 18

7 years ago
Created attachment 492119 [details]
OnlyOnBeforeUnload.html

Updated

7 years ago
Blocks: 613800

Comment 19

6 years ago
Making this worse, these alerts are old-style modal rather than tab-modal :(

data:text/html,<body onbeforeunload=alert(5)>

Comment 20

6 years ago
(In reply to comment #19)
> Making this worse, these alerts are old-style modal rather than tab-modal :(
culprit Bug 613800
Suggesting alternatives to this bug:

* Disallow (infinite) loops in onBeforeUnload, onUnload and onPageHide
* Disallow alert/confirm/prompt in onUnload and onPageHide only (as long as infinite loops are disallowed in onBeforeUnload)
* Disallow all infinite loops with only alert/confirm/prompt
* Don't fix this bug at all, instead relying on tab-modal prompts (bad for UX)

Comment 22

6 years ago
(In reply to comment #21)
Already disallow infinite loops implemented in Fx 4
This bug is clearly disallowing and message from web developer on page close.

Comment 23

6 years ago
It's that possible to disable some specific sites or all onbeforeunload pop-up?

Sometime found it's really annoying

Comment 24

6 years ago
The proper fix against malicious javascript is to make alert/confirm non-modal (tab-modal)!

Currently confirm is modal when called from beforeunload, and not modal in other cases. Removing confirm from onbeforeunload would break the ability of web application to ask user to save his data before leaving (which he might have spent a lot of time to enter). This would reduce appeal of recommending mozilla to users of such web application.

Comment 25

6 years ago
Sergey, I think you're misunderstanding the onbeforeunload API. There's a confirm-like dialog that's part of the onbeforeunload API itself; this is about blocking additional alerts/confirms/prompts.

I see you've also commented in bug 578828; thanks.
webkit landed it https://bugs.webkit.org/show_bug.cgi?id=56397
I tested Chrome 15.0.874.121m, Opera 11.52, IE 9, and Safari 5.1.2 (Mac).

IE 9 is the only browser that seems to support onbeforeunload.

All of them seem to ignore alert() from onunload/onpagehide, except for Safari, which ignores them on tab close but not on tab navigation, and IE 9, which allows alert() from onunload on navigation (but not tab close).
Created attachment 579471 [details]
test page
Assignee: nobody → gavin.sharp
Status: NEW → ASSIGNED
Created attachment 579473 [details] [diff] [review]
WIP

This has some other stuff in it that may not be necessary, but I wanted to back this up.
Blocks: 664586
Blocks: 700080
The HTML spec has specified how this should work in more detail since I last looked into this (see http://html5.org/r/6966 ). I think implementing that behavior exactly would require some more invasive changes, but it would probably be a cleaner and more robust solution.
Assignee: gavin.sharp → nobody
Keywords: sec-want
(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #30)
> The HTML spec has specified how this should work in more detail since I last
> looked into this (see http://html5.org/r/6966 ). I think implementing that
> behavior exactly would require some more invasive changes, but it would
> probably be a cleaner and more robust solution.

What changes?

The algorithm described in the spec seems to match what the code does, apart from the counter and state stuff which seems to have changed since this comment and is now vague about what it means.

Your patch above seems to fix the bug though. What's left to do here?
Thanks for looking that stuff over. Good to hear that it matches what's specced pretty closely.

Much of the changes to nsGlobalWindow (basically, all changes except from the addition of mDialogsPermanentlyDisabled) are cleanup, that might be best split into its own preliminary patch.

The tabbrowser/nsIContentViewer/nsDOMWindowUtils changes are the meat of the functional change (one thing I forgot in the patch: nsDOMWindowUtils::PreventFurtherDialogs will need a security check). And then we'll need some tests for this (ideally based on the HTML spec).
So I updated the patches but some tests fail when run as a batch, although not when run individually. This is because the code within nsGlobalWindow::DialogsAreBeingAbused is checking to see if the time since the last dialog was shown is less than 3 seconds, and displaying a 'prevent additional dialogs' dialog.

The last dialog was from an different earlier test that happened to run less than 3 seconds ago.

The tests worked before because showModalDialog didn't perform this timestamp check.

What is the correct behaviour here? Should the timestamp be reset when changing pages, or should the tests be changed to wait for a bit, or should the tests change the 'time between dialogs' preference temporarily to some very high value?
Neil and I discussed this on IRC - the timestamp should certainly not be persisting across page loads.

Looking over this further, though, I'm not sure how this is actually happeening: the state seems to be kept on the inner window (which shouldn't persist across different page navigations). Perhaps the problem is actually that we're using the top-most window - I don't actually understand why we do that, at least not for mLastDialogQuitTime/mDialogAbuseCount.
I would assume that the top window is used so that a series of successive dialogs cannot be opened from different child frames. In this case, the top window is the test harness page (http://mochi.test:8888/tests/dom/tests/mochitest/bugs/)

We could change this to set the time on the parent frame from the same domain, but it's not clear to me that it isn't intentionally set of the top page to avoid user annoyance, since in a situation where multiple prompts appear successively, it doesn't seem like the user would care what page or frame they were from.
Created attachment 635802 [details] [diff] [review]
Part 1 - cleanup of dialog abuse handling
Assignee: nobody → enndeakin
Created attachment 635804 [details] [diff] [review]
Part 2 - add cabability to prevent further dialogs while unloading
These are the patches so far, with the remaining issue being the broken tests as described above.
Created attachment 637197 [details] [diff] [review]
Part 1 - cleanup of dialog abuse handling

This version moves the check at the end of DialogsAreBeingAbused so that tests no longer fail. Theres a leak with these patches now, or still, that I need to investigate.
Attachment #635802 - Attachment is obsolete: true
Attachment #579473 - Attachment is obsolete: true
Created attachment 637733 [details] [diff] [review]
Part 2 - add cabability to prevent further dialogs while unloading

Add security check and fix test to remove listener so a leak doesn't occur.
Attachment #635804 - Attachment is obsolete: true
Attachment #637733 - Flags: review?(jst)
Attachment #637197 - Flags: review?(jst)
Comment on attachment 637197 [details] [diff] [review]
Part 1 - cleanup of dialog abuse handling

Gavin hinted that smaug might be willing to review this instead.
Attachment #637197 - Flags: review?(jst) → review?(bugs)
Attachment #637733 - Flags: review?(jst)
Attachment #637733 - Flags: review?(dao)
Attachment #637733 - Flags: review?(bugs)
Comment on attachment 637197 [details] [diff] [review]
Part 1 - cleanup of dialog abuse handling

> bool
>-nsGlobalWindow::DialogOpenAttempted()
>-{
>-  nsGlobalWindow *topWindow = GetScriptableTop();
>+nsGlobalWindow::DialogsAreBlocked(bool *beingAbused)
* goes with the type, and parameters should be named aParameterName


...hmm, this patch is quite tricky to review. I need some sleep before r-/+
Comment on attachment 637197 [details] [diff] [review]
Part 1 - cleanup of dialog abuse handling


> bool
>-nsGlobalWindow::DialogOpenAttempted()
>-{
>-  nsGlobalWindow *topWindow = GetScriptableTop();
>+nsGlobalWindow::DialogsAreBlocked(bool *beingAbused)
>+{
>+  *beingAbused = false;
>+
>+  nsGlobalWindow * topWindow = GetTopInnerWin();
So you change behavior here.
GetScriptableTop -> GetTopInnerWin


> 
>-  if (AreDialogsBlocked() || !ConfirmDialogAllowed())
>+  bool needToPromptForAbuse;
>+  if (DialogsAreBlocked(&needToPromptForAbuse))
>+    return NS_ERROR_NOT_AVAILABLE;
if (expr) {
  stmt;
}

>+  bool needToPromptForAbuse;
>+  if (DialogsAreBlocked(&needToPromptForAbuse))
>+    return NS_ERROR_NOT_AVAILABLE;
ditto
Attachment #637197 - Flags: review?(bugs) → review-
Comment on attachment 637733 [details] [diff] [review]
Part 2 - add cabability to prevent further dialogs while unloading

>+nsGlobalWindow::PreventFurtherDialogs(bool aPermanent)
> {
>   nsGlobalWindow *topWindow = GetScriptableTop();
>   if (!topWindow) {
>     NS_ERROR("PreventFurtherDialogs() called without a top window?");
>     return;
>   }
> 
>   topWindow = topWindow->GetCurrentInnerWindowInternal();
I don't see where aPermanent is used.
Attachment #637733 - Flags: review?(bugs) → review-
(In reply to Olli Pettay [:smaug] from comment #44)
> >   topWindow = topWindow->GetCurrentInnerWindowInternal();
> I don't see where aPermanent is used.

nsDOMWindowUtils::PreventFurtherDialogs() calls nsGlobalWindow::PreventFurtherDialogs(true). Perhaps the functions should have different names, since they slightly different things.
> >+nsGlobalWindow::PreventFurtherDialogs(bool aPermanent)
> > {
> >   nsGlobalWindow *topWindow = GetScriptableTop();
> >   if (!topWindow) {
> >     NS_ERROR("PreventFurtherDialogs() called without a top window?");
> >     return;
> >   }
> > 
> >   topWindow = topWindow->GetCurrentInnerWindowInternal();
> I don't see where aPermanent is used.

Actually, the lines that used this didn't make it into the patch. I'll make a new patch soon.
Created attachment 650138 [details] [diff] [review]
Part 1.2 - cleanup of dialog abuse handling
Attachment #637197 - Attachment is obsolete: true
Attachment #650138 - Flags: review?(bugs)
Created attachment 650139 [details] [diff] [review]
Part 2.2 - add cabability to prevent further dialogs while unloading
Attachment #637733 - Attachment is obsolete: true
Attachment #637733 - Flags: review?(dao)
Attachment #650139 - Flags: review?(bugs)
Comment on attachment 650138 [details] [diff] [review]
Part 1.2 - cleanup of dialog abuse handling


> bool
>-nsGlobalWindow::DialogOpenAttempted()
>-{
>+nsGlobalWindow::DialogsAreBlocked(bool *beingAbused)
bool* aBeingAbused


>+  bool DialogsAreBlocked(bool *beingAbused);
bool* aBeingAbused
Attachment #650138 - Flags: review?(bugs) → review+
Comment on attachment 650139 [details] [diff] [review]
Part 2.2 - add cabability to prevent further dialogs while unloading

We need tests for iframes.
Attachment #650139 - Flags: review?(bugs) → review+
(In reply to Olli Pettay [:smaug] from comment #50)
> Comment on attachment 650139 [details] [diff] [review]
> Part 2.2 - add cabability to prevent further dialogs while unloading
> 
> We need tests for iframes.

What do you think needs to be tested here?
Same things which tested for the top level content page.
Created attachment 651072 [details] [diff] [review]
Part 2.3 - add cabability to prevent further dialogs while unloading, with additional iframe test
Attachment #650139 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/a2b13e2f75ec
https://hg.mozilla.org/mozilla-central/rev/bc02fa8b7849
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17

Updated

5 years ago
Keywords: dev-doc-needed
Hooray! \o/

Updated

5 years ago
Depends on: 784142

Updated

5 years ago
Blocks: 792458

Comment 56

5 years ago
Crashes in vBulletin 4 editors. Could reproduce this on contao-community.org: I wanted to answer, entered a response but wanted to go back and check the original message (two-finger-swipe), message showed for a second, and boom. 

Shall I file a new bug?


This is what I got to see from the crash report: 

AdapterDeviceID: 0x 166
AdapterVendorID: 0x8086
Add-ons: kitsuneymg%40gmail.com:1.0.6,macos-keychain%40fitzell.ca:1.1.3,youtubeunblocker%40unblocker.yt:0.2.0,%7B45d8ff86-d909-11db-9705-005056c00008%7D:1.1.0,%7Baede9b05-c23c-479b-a90e-9146ed62d377%7D:1.2.1,firefontfamily%40firebugextensions.org:0.1.2,artur.dubovoy%40gmail.com:3.7.1,%7B73a6fe31-595d-460b-a920-fcc0f8843232%7D:2.5.7,%7B972ce4c6-7e08-4474-a285-3208198ce6fd%7D:17.0,%7Bd10d0bf8-f5b5-c8b4-a8b2-2b9879e08c5d%7D:2.1.2,firebug%40software.joehewitt.com:1.10.4
BuildID: 20121010150351
Comments: crashing when unload: when in vBulletin 4 (on contao-community.de) editor with some text entered, navigating back via gesture will show prompt() asking the user if he wants to discard his text and navigate away from the editor – message will appear and Fx immediately crashes

will post a bug. 
CrashTime: 1350512527
EMCheckCompatibility: true
Email: ***
FramePoisonBase: 7ffffffff0dea000
FramePoisonSize: 4096
InstallTime: 1350329730
Notes: AdapterVendorID: 0x8086, AdapterDeviceID: 0x 166GL Context? GL Context+ GL Layers? GL Layers+ WebGL? WebGL+ 
ProductID: {ec8030f7-c20a-464f-9b0e-13a3a9e97384}
ProductName: Firefox
ReleaseChannel: beta
StartupTime: 1350372636
Theme: classic/1.0
Throttleable: 1
URL: https://www.contao-community.de/newreply.php?p=227322&noquote=1
Vendor: Mozilla
Version: 17.0
(In reply to Florian Bender from comment #56)
> Shall I file a new bug?

Yes.

Updated

5 years ago
Blocks: 803022

Updated

5 years ago
No longer blocks: 803022
Depends on: 803022

Updated

5 years ago
Depends on: 827084

Updated

4 years ago
Depends on: 856977

Comment 58

4 years ago
I hate this feature! It is an evil function to do modal confirm dialog in page unload event.
Could Mozilla/FF provide a way to disable it? Much Much Thanks.

Updated

3 years ago
Depends on: 969787
You need to log in before you can comment on or make changes to this bug.