User Agent: Mozilla/5.0 (Windows NT 6.1) AppleWebKit/537.36 (KHTML, like Gecko) Iron/28.0.1550.1 Chrome/28.0.1550.1 Safari/537.36 Steps to reproduce: 1. Go on http://tim-philipp-schaefers.de/poc/ 2. Try to close the box! 3. You are not able to do that. A normal User would not find a way to close the browser (with process explorer/task manager), so you are able to execute javascript for a long time. So you are able to calculate bitcoin or sth. like that. Actual results: Instead of creating the dialog which allows you to prevent additional dialogs, the page refreshs all the time and creates a new "js-alert" box. So for an attacker would it be possible to execute javascript for a long time (perhaps calculate bitcoin or crack hashes ;because some BDU don't know how to kill a process), if someone goes on an effected domain. Expected results: Normally if a "js-alert" of a website appears you are able to close it. If you try it two times there is a dialog: "Prevent this page from creating additional dialogs" with the checkbox - but not in this case. Possible fix: There should be implemented a control which points out how often a js-alert was shown and if it is several times there should be add the normal dialog: "Prevent this page from creating additional dialogs" with the checkbox.

It could also be work on mobile versions of the browser.

This code: <script>onbeforeunload=function(){location.reload();return "internetwache.org"};onload=function(){location.reload();}</script> is making it happend. Thanks in advance to fix it, Tim Philipp Schäfers Team of internetwache.org

Gijs, I assume your beforeunload changes don't help here and that we need bug 616853 to fix this?

(In reply to Boris Zbarsky [:bz] from comment #3) > Gijs, I assume your beforeunload changes don't help here and that we need > bug 616853 to fix this? Investigating (needed to patch-mess + clobber), but I would assume you're right. Note that while bug 616853 would fix this, so would forbidding onbeforeunload handlers to mess with the location of anything in their doctree, I think? In fact, I thought there was code that already granted unload permission if a dialog was pending. Then again, maybe that's part of the problem instead of the solution here...

(In reply to :Gijs Kruitbosch from comment #4) > (In reply to Boris Zbarsky [:bz] from comment #3) > > Gijs, I assume your beforeunload changes don't help here and that we need > > bug 616853 to fix this? > > Investigating (needed to patch-mess + clobber), but I would assume you're > right. My fiancée and the second episode of Sherlock intervened, but yes, I can confirm you're right, the patch doesn't help. :-(

So this helps in that you can use the 'stay' option to stop the onload->reload flood (which I don't think is easily fixable without breaking vaguely legitimate usecases), and then close the tab/window (and click the 'leave' option) - unintuitive, but a lot better than the current situation. Don't know if we want this patch or if we think we can do the per-tab dialogs quickly (and, if we can do per-tab dialogs quickly, if that entirely eliminates this issue). I've not tried my hand at migrating the dialogs to per-tab-prompts because AFAICT we also use the prompts on shutdown, and for that to work them being per-tab isn't good enough, as it were. So that is slightly more complicated. It might be worth rethinking some of this code more completely in view of the multitude of issues, however... Boris, thoughts?

Comment on attachment 8355934 [details] [diff] [review] 956524-no-navigation-during-beforeunload This doesn't seem insane, but: 1) What does the spec say? 2) What do other UAs do?

(In reply to Boris Zbarsky [:bz] from comment #7) > Comment on attachment 8355934 [details] [diff] [review] > 956524-no-navigation-during-beforeunload > > This doesn't seem insane, but: > > 1) What does the spec say? > 2) What do other UAs do? I can't find anything in the spec about this, and as far as I can tell location.whatever is always supposed to alter the location as instructed. To be fair, that is something we already don't do in print preview (or something?) and after the unload event has been fired, as far as I can tell. Chrome (31) shows an app-modal dialog and doesn't let me close the dialog or the app either way, so presumably it allows navigation. IE does the same. I think the tab-modal dialogs may be with us sooner than I thought in comment #6, so perhaps we can get away with not doing this...

> and as far as I can tell location.whatever is always supposed to alter the location as > instructed No, there are all sorts of cases in which http://www.whatwg.org/specs/web-apps/current-work/multipage/history.html#navigate bails out. For example, the unload event case is covered by step 4 of that algorithm. Now that I've gone and read it, I guess the relevant part of the spec here is step 6: If the prompt to unload a document algorithm is being run for the active document of the browsing context being navigated, then abort these steps without affecting the prompt to unload a document algorithm. As in, no navigation from inside beforeunload handlers, per spec as currently written. I'm not sure what dialogs have to do with this; what you want to do is just perform a new navigation (e.g. set location.href to some other value) in the beforeunload handler in IE or Chrome and see what happens. Do you want to write up the testcase, or should I?

This is just a manual testcase, wasn't sure if you meant a testcase for the spec (does HTML5 do those or is that just the JS-related specs that have a spec test suite?) or something for our own testsuites (not sure how to handle the actual dialogs) or exactly this. Either way. Just stuck this in chrome, it seems it ignores the location.href called in the beforeunload handler, so accepting navigation executes the navigation the user enacted instead of the location.href, and not accepting navigation stays on the page. Either way, the location.href setter does not have any effect. Interestingly, if you replace the location.href setting with location.reload(), and you click "stay on page" you get a new dialog, until you finally agree and say "Reload the page", which seems to indicate that it *does* let the page call location.reload from within the beforeunload handler. I don't know why they (at least AFAICT) seem to treat these two cases differently (and/or if we should do the same).

Comment on attachment 8360726 [details] 956524-reload-beforeunload.html There is an HTML test suite, in theory, but for now I just wanted a manual one so we can tell what other browsers are doing. When you did the href bit, was the href same-origin or different origin? What does IE do?

Ugh. So this was my original .href testcase (kind of; now adjusted for IE8 compat... :-\ )

... and this is a same-origin test for the same. I'm now seeing Chrome produce two onbeforeunload dialogs, which I don't think I was seeing yesterday night... either the testcase changes made a difference wrt quirks mode or something, or my sleepiness made me miss it. It does this for both the same-origin and the different-origin testcase. Still, you cannot actually get the browser to navigate to the URL provided by the page, your original navigation (ie what you typed in the address bar) will be where you end up (if you don't stay on the page). I tested IE8 (which is the only thing I have to hand in my XP VM - at a work week so my win7 desktop machine is far away; I can probably find someone to test this in a more recent version if necessary). There both navigations (ie same-origin or different-origin) happen and you end up on the page that the testcase sets location.href to, not on the one you typed in the address bar.

(In reply to :Gijs Kruitbosch from comment #13) > I'm now seeing Chrome produce two onbeforeunload dialogs, which I don't > think I was seeing yesterday night... either the testcase changes made a > difference wrt quirks mode or something, or my sleepiness made me miss it. > It does this for both the same-origin and the different-origin testcase. Ignore this bit; it seems this was because it was caching an outdated version and my attempts to reload it hadn't worked (possibly because of the onbeforeunload dialogs, possibly because I'm still sleepy this morning...)

So the real question we're trying to answer is: is doing what the spec says web-compatible? In particular, if the beforeunload handler sets href but does NOT return anything (so there is no dialog involved), what happens in IE and Chrome?

(In reply to Boris Zbarsky [:bz] from comment #15) > So the real question we're trying to answer is: is doing what the spec says > web-compatible? In particular, if the beforeunload handler sets href but > does NOT return anything (so there is no dialog involved), what happens in > IE and Chrome? Both IE and Chrome end up on the page you're navigating to instead of the location.href-set-value if there's no dialog shown.

Great. Sounds like disallowing navigation in beforeunload should work fine, then.

Comment on attachment 8355934 [details] [diff] [review] 956524-no-navigation-during-beforeunload So how does "not insane" translate when it comes to a review? :-)

Comment on attachment 8355934 [details] [diff] [review] 956524-no-navigation-during-beforeunload r=me

So I don't know the process here, what will be next? Cheers, Tim Schäfers

(In reply to TPS from comment #20) > So I don't know the process here, what will be next? > > Cheers, Tim Schäfers I'll land this on mozilla-inbound, after which it will hopefully get merged to mozilla-central (assuming it doesn't break anything), after which it will appear in a nightly (nightly.mozilla.org). After which we might consider uplifting this to Aurora. Not sure it's severe enough to consider beta, also because there's an interface change in here.

remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/94f825fa6750 (added a uuid rev. Oops.)

https://hg.mozilla.org/mozilla-central/rev/94f825fa6750 Gijs, can you please set the proper affected status for the branches I've marked '?'? Thanks! Also, should we have a test for this?

(In reply to Ryan VanderMeulen [:RyanVM UTC-5] from comment #23) > https://hg.mozilla.org/mozilla-central/rev/94f825fa6750 > > Gijs, can you please set the proper affected status for the branches I've > marked '?'? Thanks! I have no idea how the b2g branches work, if they have onbeforeunload dialogs, what landing on m-c implies for them, or even how I'd check any of that. :-( Do you? > Also, should we have a test for this? Not sure how we would automate checking for this...

I don't know. We'll leave it for the security team to triage.

[Approval Request Comment] Bug caused by (feature/regressing bug #): n/a User impact if declined: users can be kept on a page by a barrage of onbeforeunload dialogs in combination with location.reload, causing DoS (see also bug 636374 and so on) Testing completed (on m-c, etc.): Been on m-c for several days, no known issues. Manual testing of behaviour in Chrome and IE to ensure web compatibility. Risk to taking this patch (and alternatives if risky): Could potentially break sites which are trying to navigate during onbeforeunload. This isn't supported per spec, nor would it really have worked reliably in the past. We did manual testing to ensure our behaviour isn't radically different from Chrome or IE (it isn't). Furthermore, if we were to discover that we do break the web once this hits beta or similar, the fix could be disabled in a very simple patch by not having the docshell check the onbeforeunload state of the contentviewer, which would be a 1-2-line patch that doesn't touch either idl or strings. String or IDL/UUID changes made by this patch: one idl addition and corresponding uuid rev.

Comment on attachment 8362448 [details] [diff] [review] 956524-no-navigation-during-beforeunload dveditz should chime in if we have time to promote up to beta.

Gijs, what do IE/Chrome do with form submits in beforeunload? See bug 652069...

(In reply to Boris Zbarsky [:bz] from comment #29) > Gijs, what do IE/Chrome do with form submits in beforeunload? See bug > 652069... Oh dear. So, I tested by navigating away off to some other page. Interestingly, if no dialog is requested by the webpage, chrome submits the form. Submitting to the same page does that too, and the user's requested navigation to bar.com doesn't happen. IE9, looking at the network devtool they ship, seems to attempt to submit the form, and then immediately aborts the submit in deference to the user's navigation request.

That might be a web compat issue, then, if we prevent submits during beforeunload saving page state or something. :( Note that the IE behavior will still send the form data to the server, just not show the resulting page....

(In reply to Boris Zbarsky [:bz] from comment #32) > That might be a web compat issue, then, if we prevent submits during > beforeunload saving page state or something. :( Note that the IE behavior > will still send the form data to the server, just not show the resulting > page.... But that bug was filed well before this fix landed - it claims our behaviour was intermittent beforehand. Do we intend to fix that? If we do, the question becomes if we would still want to stop navigation generally but not form submits. That gets a little weird because you could submit an empty get form with the same url as the location.href and that'd work, but navigation using location.[whatever] wouldn't... Which might end up ultimately meaning that we should just fix bug 616853. :-\

I wouldn't worry too much about the intermittent behavior per se... I think the simplest thing to do is to allow the form submit but set a flag on the channel (probably in the property bag on it) that indicates it came from beforeunload and then just have content dispatch in the uriloader look for that flag and if found ignore the response and abort the channel.

(In reply to Boris Zbarsky [:bz] from comment #34) > I wouldn't worry too much about the intermittent behavior per se... > > I think the simplest thing to do is to allow the form submit but set a flag > on the channel (probably in the property bag on it) that indicates it came > from beforeunload and then just have content dispatch in the uriloader look > for that flag and if found ignore the response and abort the channel. I'm confused. Are you arguing we should add more fixes here specifically wrt forms? I can't reproduce them interfering with user-induced navigation, but perhaps I've not tested enough. Considering this seems to not have been functional before this patch, I'm not sure we should be trying to fix it here... I'm probably missing something, just not sure what at this point.

Comment on attachment 8362448 [details] [diff] [review] 956524-no-navigation-during-beforeunload This isn't really a sec-approval? issue since it is a sec-low. If we want this on beta, we should just nominate it for beta. The risk here isn't the security side but the stability and the release management team reviews the nominations often. I'm on the fence because of the outlined unknowns and risks around the new behavior and how it could affect web compatbility.

Adding Dan since Doug specifically called him out by name for an opinion (Dan doesn't necessarily review sec-approvals since I look at them daily).

> Are you arguing we should add more fixes here specifically wrt forms? I think there's a good chance we need to for web compat, yes. > Considering this seems to not have been functional before this patch Before this patch, there was a race between the form submit from beforeunload and the response from the server for the page load that triggered the beforeunload. If the former finished sending the data before the server responded, it worked. Otherwise, we killed off the form submit. After this patch the form submit in beforeunload never works. So there may well be sites where the form submit always worked: ones where the submitted data is small and the response that would kill it off is slow. I'm worried about us breaking such sites...

Comment on attachment 8362448 [details] [diff] [review] 956524-no-navigation-during-beforeunload (In reply to Boris Zbarsky [:bz] from comment #38) > > Are you arguing we should add more fixes here specifically wrt forms? > > I think there's a good chance we need to for web compat, yes. > > > Considering this seems to not have been functional before this patch > > Before this patch, there was a race between the form submit from > beforeunload and the response from the server for the page load that > triggered the beforeunload. If the former finished sending the data before > the server responded, it worked. Otherwise, we killed off the form submit. > > After this patch the form submit in beforeunload never works. > > So there may well be sites where the form submit always worked: ones where > the submitted data is small and the response that would kill it off is slow. > I'm worried about us breaking such sites... OK. In that case, it sounds like I should at least withdraw the approval request for beta (which I've just done) which is released in 2 weeks. I don't think I have cycles in the coming two weeks to shepherd what sounds to me like a complicated followup change (how do we exclude them from the current change in docshell?) wrt form submissions into Aurora/beta. I can leave this assigned and see if I stumble into free time, but what with the push to get Australis into 29, I don't think it'll happen, so please reassign as necessary. Furthermore, please also cancel approval for aurora and/or back out if you think the current patch will do more harm than good... I'm not sure either way; I've not been in web-compat land from the browser side before, only from the other side (as a web dev) where I used onbeforeunload but never for something like this. I don't know how many sites do, and how badly they break, and how that compares to the abuse to which onbeforeunload is put, and what the tradeoff is for us here.

Just a short question (because I don't know the rules here): Does the flag "sec-bounty" indicate that I'll receive a (low) reward for this issue? Cheers, Tim Schäfers

(In reply to TPS from comment #40) > Just a short question (because I don't know the rules here): > Does the flag "sec-bounty" indicate that I'll receive a (low) reward for > this issue? It is a flag with a state so sec-bounty? means the bug is nominated for bounty consideration by our bounty committee. sec-bounty+ means the bounty committee approved the nomination and it will receive a bounty. sec-bounty- means it was considered but won't get a bounty payment. This bug is currently sec-bounty- because it is a sec-low rated issue, on which we do not pay bounties because they are considered minor security issues and not seriously exploitable or dangerous.

Gijs, unfortunately the only quick way to judge web compat impact is to ship the software and see what bug reports you get. We could add telemetry for navigation attempts in beforeunload, but it would take several release cycles to get any useful data from that. What we really need here is someone who has time to figure out what Blink and Trident really do here. Do they special-case forms? Do they allow href sets during beforeunload to send a GET but then just ignore the server response? Do they treat GET and POST forms differently? What happens if the server response is "Content-Disposition: attachment"? Johnny, any idea who might have time to investigate this? In terms of implementation, the simplest thing that sorta-allows form submission, if we discover that it's needed after all, would be to back this patch out, flag all loads started in beforeunload, whether from form submission or not, and not react to the responses...

Sec-low so marking wontfix for ESR24.

https://hg.mozilla.org/releases/mozilla-aurora/rev/e1181448cd9b sec-lows are wontfix for b2g anyway, so meh :)

Clearing needinfo flag, Matt took on investigating what other browsers do here. Thanks Matt!

As Matt is absent till Monday I did a quick check: IE 11 on Win 7, Safari 7 and Chrome 32 on Mac OSX all behave the same when visiting URL of this bug: After loading the page they all show the pop-up with the two buttons for stay or leave. As long as the user has not clicked on any of the two options he can't close the tab, close the browser or navigate in any other in the browser. If the user clicks on "leave" these browsers all send a new GET request which gets answered with a 304 from the server. If the user clicks "stay" the browsers do NOT send any requests, but the dialog pops up immediately again. Thus the user is caught in an endless loop. But all other browsers have the same issue that user has only two options to get out of the endless loop: a) close the tab while the page is reloading b) terminate the whole browser from the OS None of the other browsers implements some form of loop detection or option for the user to dis-allow further dialogs from the URL. FF26 biggest issue in my opinion is that it reloads the page in the background while the pop-up dialog is presented resulting in quite some traffic. FF28 now also sends new requests only when the user clicks on "leave". But when the user clicks on "stay" no further dialogs and no further requests are send, which actually breaks the endless loop. So our solution seems to implement the same behavior (in terms of requests being send) as the others browser have implemented already. With the one advantage that our solution now breaks the endless loop and allows the user to close the tab (the only confusing part for the user is that he has to click on "leave" on more time in this case which does not result in a reload). Please let me or Matt know if more testing of JS functions is needed.

Nils, I'm not sure any of that answers my question from comment 29 or the questions from comment 42. That is, if the user clicks "stay", what happens to various load requests that might have been dispatched during the beforeunload handler?

Reassigning to Nils to answer comment 47.

I've distilled comment 42 into three discrete questions. Please feel free to ask for clarification. 1. Can href sets during onbeforeunload be allowed to send a GET but then ignore a server response? Sample code: onbeforeunload=function() { location.href="anotherpage.htm"; }; onload=function() { location.reload(); } Firefox 28+: sends GET request but seems to ignore response, although we get a 200 response only upon closing window Blink: sends GET request and seems to ignore response, always Trident: sends GET request and a 200 response is received about half of the time 2. What happens if server response contains header "Content-Disposition:attachment; filename='foo.txt'"? Firefox 28+ and Blink: sends GET request and seems to ignore response, always Blink: Same as the previous answer None of the browsers download the file or prompt with a dialog. 3a. Do browsers special-case form submission in the onbeforeunload event? Is it different for GET/POST? This one is tricky. Let me explain. Consider this sample code: onbeforeunload=function() { submitMyForm(); }; onload=function() { location.reload(); } For GET - Firefox 28+ and Blink - they behave identically. The form is not submitted. (One quirk, though, for both. Depending on how long you allow the page to run, it may or may not submit the form upon closing the window. In this case, the form is usually submitted exactly once, at the very last time.) Trident allows the form to be submitted most of the time. I say most because they appear to be dropping requests due to the high number and rapidity. I see HTTP status 200 responses for most of the outgoing GETs. For POST, Firefox 28+ does NOT behave as it does for GET. It submits the form successfully with every POST request. And with Blink/Trident, their behaviors are the same as they are with GET. Blink blocks the POST form submission, and Trident usually allows it. 3b. Comment 47 adds more nuance to the question, as it becomes more of "what happens if the form is submitted in the context of the original example (comment 2) - and user also decides to stay on the page?" In essence: onbeforeunload=function() { submitMyForm(); location.reload(); return "internetwache.org"; }; onload=function() { location.reload(); } Firefox 28+ GET: Form is submitted once Blink GET: Endless page requests and dialogs, no form submission Trident GET: Endless page requests, dialogs and form submissions (whether you stay on page or not) Firefox 28+ POST: Same as GET Blink POST: Page is requested only initially, endless dialogs, no form submission Trident POST: Same as GET

The case I'm _really_ interested in is the case without the "onload=function() { location.reload(); }" thing. That is, just a page that does: onbeforeunload=function() { submitMyForm(); }; When the user navigates away from the page, does the form get submitted from the point of view of the server? I don't care what the server response is so much, just whether the server sees a _request_. It sounds like the answer is that in Blink it doesn't and in IE it sometimes does and sometimes doesn't, based on the 3a case in comment 49, but I'd like us to make very sure that's the case.

Also, I'm curious: what was used to determine what requests are sent or not sent in comment 49?

RE: comment 50. That changes the scenario - and results - quite a bit, since we are now navigating to a different page instead of just reloading the existing page. Firefox 28+ / Blink: Form is submitted, and a response is returned, both GET and POST. Trident: Form is not submitted, both GET and POST. RE: comment 51. I'm using Charles proxy. I've tried to figure out what you were asking for vs what was in the original case, so sorry if I did not understand your initial request.

> Firefox 28+ / Blink: Form is submitted, and a response is returned, both GET and POST. Er... Even in Firefox 29? The point of the patch in this bug was to make that case not work, I thought.

Boris, I don't know what I saw yesterday, but re-running my tests today in Firefox confirm your statement. Firefox does *not* submit the form in the onbeforeunload handler, in the simplified case. So, that wonkyness is my fault. I have no idea why I don't see today what I thought I saw yesterday. Results from Blink/Trident are the same as before. However, running this particular test across versions of Firefox from 24 through 30, I see no difference. In the simple test within comment 50, all versions of Firefox do not send the form data. It seems we can achieve different results across browsers when we add the onload/reload brute force, which was part of the original PoC here.

> In the simple test within comment 50, all versions of Firefox do not send the form data. Hmm. I guess we do the Stop() call in nsDocShell::InternalLoad after firing the beforeunload handler... So it's possible we're canceling the necko channel before we actually hit the network. Alright, thanks. Sounds like this patch isn't really making things any worse, so we can just go ahead with it.

Ioana, please make sure this gets verified fixed before Firefox 28 is released.

Tested this on Windows 7 32bit using Firefox 27.0.1 (wontfix), Firefox 28.0b4, Firefox 29.0a2 (19/02), Chrome 32 and IE 11. http://tim-philipp-schaefers.de/poc opens this dialog in Firefox: "This page is asking you to confirm that you want to leave - data you have entered may not be saved" and similar dialogs in IE and Chrome. The dialog can't be closed any way in Firefox 27.0.1, IE and Chrome. Firefox 28 and 29 let you close the dialog by clicking Stay on Page or the [X] button. The Leave Page button only closes the dialog (and the tab) if the dialog was opened by trying to close the tab or load another page in the same tab (when opened by loading the page it just reopens the dialog with each click on the button). All the testcases attached by Gijs show a confirmation dialog when reloading them or trying to close the tab (except for Testcase - form submit beforeunload ) on Chrome and IE. None of the Firefox versions display any dialogs in these cases (the pages just get reloaded). "Testcase - form submit beforeunload" works the same on all the browsers I tested. When submitting, the value in the input field gets reset. Are you guys sure this doesn't submit the form? When reloading the page or trying to load another page in the same tab, I get this in the Browser Console (same as when clicking the Submit button): A form was submitted in the windows-1252 encoding which cannot encode all Unicode characters, so user input may get corrupted. To avoid this problem, the page should be changed so that the form is submitted in the UTF-8 encoding either by changing the encoding of the page itself to UTF-8 or by specifying accept-charset=utf-8 on the form element. attachment.cgi:16 GET https://bug956524.bugzilla.mozilla.org/attachment.cgi [HTTP/1.1 302 Found 775ms] GET https://bugzilla.mozilla.org/attachment.cgi [HTTP/1.1 302 Found 974ms] GET https://bug956524.bugzilla.mozilla.org/attachment.cgi [HTTP/1.1 200 OK 226ms]

Ioana, I can reproduce the behavior you see with that testcase, by viewing the Browser Console. The console implies that the form is being sent. However, when I use Charles to inspect the HTTP traffic - outside of Firefox - I don't see the network request being made. I think this is an erroneous message.

(In reply to Matt Wobensmith from comment #58) > Ioana, I can reproduce the behavior you see with that testcase, by viewing > the Browser Console. The console implies that the form is being sent. > > However, when I use Charles to inspect the HTTP traffic - outside of Firefox > - I don't see the network request being made. > > I think this is an erroneous message. Cool. I'll file a bug against the Console tomorrow.

(In reply to Ioana Budnar, QA [:ioana] from comment #59) > Cool. I'll file a bug against the Console tomorrow. Bug 974840.