Closed Bug 391834 Opened 17 years ago Closed 12 years ago

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

Categories

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

Product:
Core
Component:
DOM: Core & HTML
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla17

People

(Reporter: volkmarkostka, Assigned: enndeakin)

References

(Blocks 1 open bug,
URL
)

Details

(Keywords: dev-doc-needed, sec-want, Whiteboard: [sg:want P4])

Attachments

(5 files, 6 obsolete files)

onunload.html
205 bytes, text/html
Details
OnlyOnBeforeUnload.html
107 bytes, text/html
Details
test page
165 bytes, text/html
Details
Part 1.2 - cleanup of dialog abuse handling
15.78 KB, patch
smaug
: review+
Details | Diff | Splinter Review
Part 2.3 - add cabability to prevent further dialogs while unloading, with additional iframe test
18.85 KB, patch
Details | Diff | Splinter Review
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.
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.
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.
Summary: Don't allow alert/confirm/prompt in onbeforeunload → Don't allow alert/confirm/prompt in onbeforeunload and onunload
Status: UNCONFIRMED → NEW
Ever confirmed: true
Blocks: alertloops
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
Making it possible to abort scripts from the alert() dialog is bug 61098. That's mostly orthogonal to this bug.
Depends on: 411855
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.
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?
@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. :-)
Blocks: eviltraps
No longer depends on: 411855
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
No longer blocks: alertloops
Blocks: 588292
Attached file onunload.html —
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.
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 ;)
Blocks: 613800
Making this worse, these alerts are old-style modal rather than tab-modal :( data:text/html,<body onbeforeunload=alert(5)>
(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)
(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.
It's that possible to disable some specific sites or all onbeforeunload pop-up? Sometime found it's really annoying
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.
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.
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).
Attached file test page —
Assignee: nobody → gavin.sharp
Status: NEW → ASSIGNED
Attached patch WIP (obsolete) — — Splinter Review
This has some other stuff in it that may not be necessary, but I wanted to back this up.
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
(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.
Assignee: nobody → enndeakin
These are the patches so far, with the remaining issue being the broken tests as described above.
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
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.
Attachment #637197 - Attachment is obsolete: true
Attachment #650138 - Flags: review?(bugs)
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.
https://hg.mozilla.org/mozilla-central/rev/a2b13e2f75ec https://hg.mozilla.org/mozilla-central/rev/bc02fa8b7849
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
Keywords: dev-doc-needed
Hooray! \o/
Depends on: 784142
Blocks: 792458
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.
Blocks: 803022
No longer blocks: 803022
Depends on: 803022
Depends on: 827084
Depends on: 856977
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.
Depends on: 969787
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: