108104 - Key events from script can execute menu commands

Status: VERIFIED FIXED

(Core :: DOM: Events, defect)

Description

[Mitchell Stoltz (not reading bugmail)]

Sending key events from a script can activate commands in the menus, just as if
the user had chosen a menu item. This allows scripts to add bookmarks without
user confirmation, to name just one example. Events initiated by scripts in
content should not be able to call menu key-shortcuts.

The example below sends Alt-B Alt-B, which corresponds to Add Bookmark. Some
Ctrl-key menu shortcuts work as well.

Comment 1

[Mitchell Stoltz (not reading bugmail)]

Attachment: Example HTML - adds a bookmark

Comment 2

[Ben Bucksch (:BenB)]

Adding [discoverable] - this is a bug which is easy to figure out.

Comment 3

[Ben Bucksch (:BenB)]

Testcase doesn't work for me.

It might be possible to use this to open/write arbitary files. (That's what I
wanted to try.)

Comment 4

[georgi - hopefully not receiving bugspam]

My testcase works for me under both Linux and windows.
Mitchell's description of the testcase is slightly incorrect - the script sends
"ALT-B B" 
NOT "ALT-B ---ALT--- B".

Comment 5

[Jaime Rodriguez, Jr.]

Adding PDT for tracking and 6.2 branch review purposes.

Comment 6

[Jaime Rodriguez, Jr.]

What are the chances we'd have a patch for this one by this afternoon?

Comment 7

[joki (gone)]

Pretty good, though I need to talk it over with some people to make sure we're
picking up XUL key commands at the best spot.  I'll attach the patch in a mintue.

Comment 8

[joki (gone)]

Attachment: First proposed patch

Comment 9

[saari (gone)]

looks good generally. Is getting the script security service manager an
expensive call for every event dispatch? Should we just cache it once we have it?

Comment 10

[chris hofmann]

who else can we get to review?

Comment 11

[Jaime Rodriguez, Jr.]

Tentative PDT+, pending positive reviews. We'd like to see this one land on the
6.2 branch by 9 am PST (if possible).

Comment 12

[Johnny Stenback (:jst)]

r/sr=jst, but hyatt should have a look too and see if there are any other places
where we need to add similar checks.

Comment 13

[joki (gone)]

Saari, I don't think we have to worry too much about performance at the location 
of the security check.  Its only called in the dispatchEvent case which is only 
used for user created events and some of our own internal dispatches, which to 
my knowledge, are not high frequency.  None of the native events hit it.  So due 
to the push to get this in more or less immediately I'm going to take that for 
the r on this and use jst for the sr and get this in tonight.

Comment 14

[joki (gone)]

Attachment: Modified part of patch for nsEventStateManager.cpp

Replaces nsEventStateManager.cpp portion of previous patch

Comment 15

[Johnny Stenback (:jst)]

Comment on attachment 58514 [details] [diff] [review]
Modified part of patch for nsEventStateManager.cpp

sr=jst

Comment 16

[joki (gone)]

Fix checked into main trunk and 6.2.1 branch.  Still want to talk to hyatt about 
possible optimization point but fix as is works just fine.

Comment 17

[Jimmy Lee]

Adding Antonio to Cc.  He is helping with Mac testing.

Comment 18

[bsharma]

Verified on 2001-11-26-6.2.1 build on WinNT, Linux (7.2), Mac OS X, Mac OS 9.1.

The above test case passes.

Comment 19

[Mitchell Stoltz (not reading bugmail)]

Why is this bug still open? Are waiting for checkin approval on the 0.9.4
branch? Or the 6.2 branch?

Comment 20

[bsharma]

Verified on 2001-11-29-Trunk build on WinNT.

Above test case passes.

Comment 21

[bsharma]

Marking as verified FIXED.

Comment 22

[Marek Z. Jeziorek]

edt0.9.4+ per chofmann's e-mail:
"
yes, go ahead.

thanks

chris h."

Comment 23

[Mitchell Stoltz (not reading bugmail)]

I'm having trouble modifying this patch for the 0.9.4 branch. joki, can you
help? reopening bug.

Comment 24

[Mitchell Stoltz (not reading bugmail)]

Attachment: First try of patch for 0.9.4 - doesn't quite work

Here's my best guess at applying this patch to the 0.9.4 branch. When I run the
testcase (the first attachment), it opens the menu but doesn't choose a menu
item. I think something's slightly wrong.

Comment 25

[joki (gone)]

Mitch, I don't have a 0.9.4 branch set up right now.  Can you take a look at 
this and try it out?

http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&su
bdir=mozilla/layout/xul/base/src&command=DIFF_FRAMESET&file=nsMenuBarListener.cp
p&rev1=1.55&rev2=1.55.22.1&root=/cvsroot

Comment 26

[Mitchell Stoltz (not reading bugmail)]

Attachment: Complete patch for 0.9.4

Ah, that's what I was missing. With that addition, the patch works for 0.9.4.
As it's already been reviewed and approved, I'll check it in ASAP.

Comment 27

[Mitchell Stoltz (not reading bugmail)]

Checked in on 0.9.4 branch. Bindu, can you please verify against the above 
testcase (first attachment) on the 0.9.4 embedding branch?

Comment 28

[Vladimir Ermakov]

Doesnt happen on 2002-01-08-22-0.9.4ec windows 98
and 2002-01-04-08-trunk on linux RedHat 6.2

Verifying

Comment 29

[Mitchell Stoltz (not reading bugmail)]

Arrgh, somehow part of the fix was never checked into the trunk! Reopening.

Comment 30

[Mitchell Stoltz (not reading bugmail)]

Attachment: Rest of the fix for the trunk

The changes for nsMenuBarListener.cpp never got checked in on the trunk -
here's the diff for the current trunk.

Comment 31

[Mitchell Stoltz (not reading bugmail)]

Setting milestone to Moz1.0 - needs approval.

Comment 32

[Johnny Stenback (:jst)]

Comment on attachment 73593 [details] [diff] [review]
Rest of the fix for the trunk

sr=jst

Comment 33

[Asa Dotzler [:asa]]

Comment on attachment 73593 [details] [diff] [review]
Rest of the fix for the trunk

a=asa (on behalf of drivers) for checkin to the 1.0 trunk

Comment 34

[bsharma]

Verified on 6.2.2 on WinNT.

Added AOLTWOK in the Status Whiteboard.

Comment 35

[Jimmy Lee]

Adding Michael.

Comment 36

[joki (gone)]

Fix checked in.

Comment 37

[Vladimir Ermakov]

verifying on build 2002-04-01-03-trunk win98