Last Comment Bug 108104 - Key events from script can execute menu commands
: Key events from script can execute menu commands
Status: VERIFIED FIXED
AOLTWOK
:
Product: Core
Classification: Components
Component: DOM: Events (show other bugs)
: Trunk
: All All
: -- critical (vote)
: mozilla1.0
Assigned To: joki (gone)
: Vladimir Ermakov
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2001-11-01 16:34 PST by Mitchell Stoltz (not reading bugmail)
Modified: 2002-05-28 17:33 PDT (History)
17 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Example HTML - adds a bookmark (551 bytes, text/html)
2001-11-01 16:34 PST, Mitchell Stoltz (not reading bugmail)
no flags Details
First proposed patch (7.20 KB, patch)
2001-11-19 13:38 PST, joki (gone)
joki: review+
joki: superreview+
Details | Diff | Review
Modified part of patch for nsEventStateManager.cpp (1.64 KB, patch)
2001-11-20 00:29 PST, joki (gone)
jst: superreview+
Details | Diff | Review
First try of patch for 0.9.4 - doesn't quite work (6.10 KB, patch)
2001-12-06 18:42 PST, Mitchell Stoltz (not reading bugmail)
no flags Details | Diff | Review
Complete patch for 0.9.4 (8.24 KB, patch)
2001-12-27 18:20 PST, Mitchell Stoltz (not reading bugmail)
no flags Details | Diff | Review
Rest of the fix for the trunk (2.05 KB, patch)
2002-03-11 16:15 PST, Mitchell Stoltz (not reading bugmail)
jst: superreview+
asa: approval+
Details | Diff | Review

Description Mitchell Stoltz (not reading bugmail) 2001-11-01 16:34:10 PST
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) 2001-11-01 16:34:54 PST
Created attachment 56184 [details]
Example HTML - adds a bookmark
Comment 2 Ben Bucksch (:BenB) 2001-11-12 06:09:39 PST
Adding [discoverable] - this is a bug which is easy to figure out.
Comment 3 Ben Bucksch (:BenB) 2001-11-12 06:27:42 PST
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 2001-11-14 23:55:04 PST
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. 2001-11-15 22:28:56 PST
Adding PDT for tracking and 6.2 branch review purposes.
Comment 6 Jaime Rodriguez, Jr. 2001-11-19 08:52:04 PST
What are the chances we'd have a patch for this one by this afternoon?
Comment 7 joki (gone) 2001-11-19 13:06:32 PST
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) 2001-11-19 13:38:52 PST
Created attachment 58412 [details] [diff] [review]
First proposed patch
Comment 9 saari (gone) 2001-11-19 16:36:43 PST
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 2001-11-19 17:41:01 PST
who else can we get to review?
Comment 11 Jaime Rodriguez, Jr. 2001-11-19 19:15:54 PST
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, jst@mozilla.com) 2001-11-19 20:58:06 PST
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) 2001-11-19 23:24:59 PST
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) 2001-11-20 00:29:27 PST
Created attachment 58514 [details] [diff] [review]
Modified part of patch for nsEventStateManager.cpp

Replaces nsEventStateManager.cpp portion of previous patch
Comment 15 Johnny Stenback (:jst, jst@mozilla.com) 2001-11-20 00:33:06 PST
Comment on attachment 58514 [details] [diff] [review]
Modified part of patch for nsEventStateManager.cpp

sr=jst
Comment 16 joki (gone) 2001-11-20 02:19:14 PST
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 2001-11-28 10:41:59 PST
Adding Antonio to Cc.  He is helping with Mac testing.

Comment 18 bsharma 2001-11-28 12:51:19 PST
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) 2001-11-28 13:14:29 PST
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 2001-11-29 15:58:40 PST
Verified on 2001-11-29-Trunk build on WinNT.

Above test case passes.
Comment 21 bsharma 2001-12-03 09:49:00 PST
Marking as verified FIXED.
Comment 22 Marek Z. Jeziorek 2001-12-05 20:18:35 PST
edt0.9.4+ per chofmann's e-mail:
"
yes, go ahead.

thanks

chris h."
Comment 23 Mitchell Stoltz (not reading bugmail) 2001-12-06 16:43:22 PST
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) 2001-12-06 18:42:54 PST
Created attachment 60758 [details] [diff] [review]
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) 2001-12-18 11:27:27 PST
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) 2001-12-27 18:20:06 PST
Created attachment 62901 [details] [diff] [review]
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) 2001-12-28 13:00:18 PST
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 2002-01-09 14:22:47 PST
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) 2002-03-11 16:03:34 PST
Arrgh, somehow part of the fix was never checked into the trunk! Reopening.
Comment 30 Mitchell Stoltz (not reading bugmail) 2002-03-11 16:15:13 PST
Created attachment 73593 [details] [diff] [review]
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) 2002-03-11 16:16:39 PST
Setting milestone to Moz1.0 - needs approval.
Comment 32 Johnny Stenback (:jst, jst@mozilla.com) 2002-03-11 16:50:22 PST
Comment on attachment 73593 [details] [diff] [review]
Rest of the fix for the trunk

sr=jst
Comment 33 Asa Dotzler [:asa] 2002-03-14 11:58:52 PST
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 2002-03-18 13:44:04 PST
Verified on 6.2.2 on WinNT.

Added AOLTWOK in the Status Whiteboard.
Comment 35 Jimmy Lee 2002-03-18 16:04:23 PST
Adding Michael.
Comment 36 joki (gone) 2002-03-27 01:28:12 PST
Fix checked in.
Comment 37 Vladimir Ermakov 2002-04-01 12:53:59 PST
verifying on build 2002-04-01-03-trunk win98

Note You need to log in before you can comment on or make changes to this bug.