Closed Bug 90392 Opened 23 years ago Closed 23 years ago

Form submitted multiple times if 'Insecure' dialog dismissed by hitting Enter. [form sub]

Categories

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

Product:
Core
Component:
DOM: Core & HTML
Type:
defect
Priority:
Not set
Severity:
major

Tracking

()

Status:
VERIFIED FIXED

People

(Reporter: basil, Assigned: alexsavulov)

References

(
URL
)

Details

(Whiteboard: [PDT+] fixed on branch [which one?])

Attachments

(4 files)

reduced test case (insecure form submit warning must be on)
168 bytes, text/html
Details
patch: only trap KEY_PRESS not KEY_UP on submit inputs (back out fix for bug 68846)
662 bytes, patch
Details | Diff | Splinter Review
patch 2: Trap KEY_PRESS for ENTER and KEY_UP for SPACE
1.34 KB, patch
Details | Diff | Splinter Review
Patch 2 with cleanup, per jag's review...
1.36 KB, patch
Details | Diff | Splinter Review 
When submitting insecure forms and the insecure form alert pops, if I click on 
OK, the alert clears; if I hit enter, the alert pops again. 

On some form submissions, like the Yahoo! mail URL above, hitting enter 
multiple times does not clear the alert. On others, like the poll on 
http://home.netscape.com/, the alert clears after two strikes of the enter key.

I know that each strike of the enter key is sending the same information, 
because this bug in another web email client will send as many copies of mail 
as the number of times I hit enter.

I am seeing this bug on build 2001071104 for Win32. I believe I began seeing it 
Monday (20010709xx), but I may have begun seeing it with Friday's build 
(20010706xx). This appears to be a regression.
bug 76605 and bug 89888 seem to be similar issues; possible dupes.

Since this bug is seen on Mac builds, changing platform and OS to all.
OS: Windows 98 → All
Hardware: PC → All
Test with build 2001071004 on Win2K to try to reproduce this bug.
Confirming... Bugzilla generated the mid-air collision with my first submit as
expected after I pressed Enter the second time to the insecure submission prompt.

To reproduce:
1 Login to bugzilla if not already
2 Set the browser to show warning when posting to insecure pages:
Edit->Preferences->Privacy and Security->SSL and check "Sending form data from
an insecure page to an insecure page.
3 Add comments and click Commit button
4 Press enter to the insecure form submission warning (don't click ok)
5 Press enter again when it reappears

Expected:
The insecure form submission should only appear once and be dismissed when enter
is pressed.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Dupe of Bug 89888

*** This bug has been marked as a duplicate of 89888 ***
Status: NEW → RESOLVED
Closed: 23 years ago
Resolution: --- → DUPLICATE
Reopening... Although this does show up as duplicate behavior, I believe this 
bug is more general and better explains the problem. The problem is not 
specific to just Bugzilla; it is visible anytime you do an insecure submission. 
If anything, bug 89888 should be marked a duplicate of this one.
Status: RESOLVED → REOPENED
Resolution: DUPLICATE → ---
After looking more closely at bug 76605, it is very likely a different issue.
Bug 76605 is almost certainly page-related and possibly even invalid, whereas
this issue happens on all pages that have insecure form submission.
Wow, didn't notice this bug until just now...  It is easily reproducible for any
form submission with a branch build.  The action of pressing Enter to dismiss
warning dialogs is very common user behavior.

Double submission of forms is extrememly severe as it could result in actions
taking place twice (online purchases, ...)  Grabbing this one.  I'll try to come
up with a quick fix for the branch!
Assignee: rods → pollmann
Status: REOPENED → NEW
Keywords: nsBranch
Whiteboard: PDT
CC'ing Steve - potential stop ship bug.
Status: NEW → ASSIGNED
Do you have a patch coming soon?  This is worth working on, but we need to see
the actual fix to make an evaluation for taking it.  Do we need any other people
helping on this?
No patch yet - I am in the middle of a clobber build on Window (unfortunately I
had clobbered before I noticed this bug...)

I am not seeing this problem on Linux (which is too bad because I was going to
use my Linux build to debug while my Windows build finishes...)  I'll continue
work on this in an hour or so after my build finishes and I grab a bite to
eat...  :)

My hope/guess is that I'll be able to come up with a couple-line patch that
disables form submit requests when one is currently being processed - which
might fix this problem *fingers crossed*.
CC'ing some people who might know why (presumably) the keypress event in a modal
dialog is getting sent to a button in the main window.
*** Bug 89888 has been marked as a duplicate of this bug. ***
First two attempts at a patch failed.  The problem is not that the keypress
comes through *during* the first submit, but that it comes immediately after. 
Thus, from the perspective of the form submission logic, this bug is
indistinguishable from bug 72906 (rapid double click on submit causes two 
submits).

We will probably need to do something like:

1) Find out why this rogue keypress event gets to the submit element to begin 
with, and stop it from happening.  This seems like the "right" thing to do.

  I don't really know where to begin here other than going through checkins.
  I've gone as far back as 24-June-2001 and still see the problem.
  Tom or Chris, do you have any ideas here?

2) Prevent submit that occurs within x time of previous submit from causing an
actual submit.  If x time happened to be the doubleclick interval, we would also
be fixing bug 72906.

3) Don't allow the insecure form submit warning dialog to be dismissed with a 
keypress.
  I also don't have a clue where to begin here
  David, Ben, ??? any XUL people know if this is practical?

Okay, nevermind - figured it out:

nsHTMLInputElement.cpp:

1.175 blakeross%telocity.com Jun 19 23:20   Keypress event bubbles up to 
alerts, meaning alerts were dismissed without the user's consent (68846). 
r=kerz sr=ben a=asa

Since this warning dialog is dismissed on Enter->KEY_DOWN and the form submit 
is triggered by Enter->KEY_UP, this was just bad, bad luck...  I'll have to 
look at bug 68846, but submitting form on KEY_UP instead of KEY_PRESS seems 
like a bad idea to me...
I am not able to reproduce bug 68846, even after backing out this part of the 
fix.  CC'ing Blake... 
Patch 2 makes our HTML control behaviour (button clicks, checkboxes, radios, 
and image inputs) consistent with our XUL dialog behavior, and consistent with 
what IE does.  It will activate the control on KEY_PRESS (immediately) when 
Enter is pressed, but on KEY_UP (waiting until the key is released) when the 
spacebar is pressed.

This fixes 90392 (it prevents multiple form submit) and bug 68846 (it prevents 
dialogs from being automatically dismissed), and does not have any 
regressions.  (The first patch caused the spacebar to no longer activate HTML 
controls at all.  It is also pretty small -> easy to understand.

The problem was an impedance mismatch between HTML controls and XUL dialogs.  
XUL dialogs were emulating IE's behaviour, but HTML controls were always 
activating on KEY_UP, regardless of which key was used.  That would be fine if 
the space bar was used to activate the control, but when Enter was used to 
dismiss the dialog box, the KEY_PRESS would come first, dismissing the dialog, 
then the KEY_UP would come later, submitting the form again.  The patch fixes 
this problem by making HTML control respond to the same key events as the XUL 
dialogs, thereby preventing them from receiving an event not consumed by the 
HTML control and acting on it.
Summary: Insecure form submission alert not cleared by hitting enter. → Form submitted multiple times if 'Insecure' dialog dismissed by hitting Enter.
Whiteboard: PDT → PDT, fix in hand, need r=/sr=/a=
r=jag
sr=jst
Checked in on the trunk.  This morning's trunk build should have the fix and can
be banged around to see if there are any regressions caused by it...  (My
assertion earlier "does not have any regressions" should have read "does not
have any *known* regressions", typing slower than the brain works...  :) )
Whiteboard: PDT, fix in hand, need r=/sr=/a= → PDT, fix in hand, need branch approval
In 2001072304 trunk for Mac, hitting return does not bring up another security
warning any more, but it in fact submits another one WITHOUT asking.
Therefore, hitting return is still captured by the form as submit in addition to
OK to the warning, and this second submission somehow bypasses the security
warning. 
Too quick. Sorry about my previous posting. I may have hit return too hard so it
produced chattering? I could not reproduce double submission this time.

Hirata, yes, a multiple-submission can still be produced by holding down the 
Enter key until the auto-repeat starts kicking in.  This would seem to me to be 
a less common problem. (Remember, without this fix, simply pressing the Enter 
key quickly will cause a multiple submit.)  Also, note that holding down the 
Enter key until the auto-repeat starts kicking in will also cause multiple-
submission in IE.  ???  Yes, this is a lesser-of-two-evils kind of thing.

One way to get around this would be to make the Enter key also trigger on 
KEY_UP for both form submission and XUL dialogs.  I looked around the XUL 
dialog code and do not know how that would be accomplished - could be a big 
change, could be small, any XUL guru care to comment?  Note that this change 
would make us behave differently than IE.

Another way to get around this would be to disable multiple submission 
altogether, or at least pop up a warning dialog (a special one that can't be 
dismissed by holding down Enter) if one was about to occur.  This would also be 
a larger change, but I will be working on it today - and will report if I make 
any progress...

In the meantime, in my opinion, this fix is at least better than our current 
behavior, and it should go in for the branch.
I agree that the current fix is OK. What I was bewildered about was the last
time I tried submitting some bug with the latest build, I got midair collision
with only one security warning after hitting Enter a bit too long. That seemed
wrong. But I have not been able to reproduce that particular result, because you
can not try this kind of behavior too many times.
Still seeing this bug in Win32 build 2001072303-trunk. Is it possible that the
fix didn't get checked in before the Win32 build?
It was checked in at 2001-07-23 at 3:30AM, so it won't be in 2001072303.   I
thought at least the trunk was still doing respins at 7AM?!
Ahh, whew!  It's there.  :)  Basil, try: 

ftp://ftp.mozilla.org/pub/mozilla/nightly/2001-07-23-09-trunk
Hirata, if you want to test this a bunch of times without messing up a bug, try
the 'reduced test case' on this bug report.  Each time you submit the form,
presuming that nobody else is messing with the test case at the exact same time,
the Request ID should increase by one.  FWIW, Internet Explorer and all previous
versions of NS 6 (and I think Nav 4.x?) dismiss that dialog immediately if you
hold down the Enter key until the auto repeat kicks in, or press Enter twice
quickly, which are essentially the same thing.
Checked in on the branch.
Status: ASSIGNED → RESOLVED
Closed: 23 years ago → 23 years ago
Resolution: --- → FIXED
Updating whiteboard so this doesn't look like rogue checkin.  ;)  [PDT gave
approval verbally at a meeting]
Whiteboard: PDT, fix in hand, need branch approval → PDT+
You did the right thing.  Updating to mention checkin has happened.
Whiteboard: PDT+ → PDT+ fixed on branch
Oops, I need to reopen this so it shows up on the PDT+ radar.  Sorry for the
spam.    Changing from nsBranch to vbranch so it gets looked at.
Status: RESOLVED → REOPENED
Keywords: nsBranch → vbranch
Resolution: FIXED → ---
This is fixed, please resolve I can verify on 
on build 2001-07-24-05-0.9.2 windows 98 build
and 2001-07-24-05-0.9.2 Linux RedHat 6.2 build
Assignee: pollmann → alexsavulov
Status: REOPENED → NEW
Bulk reassigning form bugs to Alex
Summary: Form submitted multiple times if 'Insecure' dialog dismissed by hitting Enter. → Form submitted multiple times if 'Insecure' dialog dismissed by hitting Enter. [form sub]
is this fixed on the 094 branch?
Whiteboard: PDT+ fixed on branch → [PDT+] fixed on branch [which one?]
gerardo or vladimir - pls verify.  I assume this is fixed for 094 since the fix
was in 092
resolving
Status: NEW → RESOLVED
Closed: 23 years ago → 23 years ago
Resolution: --- → FIXED
verifying
Status: RESOLVED → VERIFIED
Component: HTML: Form Submission → 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: