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
: Andrew Overholt [:overholt]
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 | Splinter Review
Modified part of patch for nsEventStateManager.cpp (1.64 KB, patch)
2001-11-20 00:29 PST, joki (gone)
jst: superreview+
Details | Diff | Splinter 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 | Splinter 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 | Splinter 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 | Splinter Review

Description User image 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 User image Mitchell Stoltz (not reading bugmail) 2001-11-01 16:34:54 PST
Created attachment 56184 [details]
Example HTML - adds a bookmark
Comment 2 User image Ben Bucksch (:BenB) 2001-11-12 06:09:39 PST
Adding [discoverable] - this is a bug which is easy to figure out.
Comment 3 User image 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 User image 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 User image Jaime Rodriguez, Jr. 2001-11-15 22:28:56 PST
Adding PDT for tracking and 6.2 branch review purposes.
Comment 6 User image 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 User image 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 User image joki (gone) 2001-11-19 13:38:52 PST
Created attachment 58412 [details] [diff] [review]
First proposed patch
Comment 9 User image 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 User image chris hofmann 2001-11-19 17:41:01 PST
who else can we get to review?
Comment 11 User image 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 User image 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 User image 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 User image 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 User image 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 User image 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 User image Jimmy Lee 2001-11-28 10:41:59 PST
Adding Antonio to Cc.  He is helping with Mac testing.

Comment 18 User image 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 User image 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 User image bsharma 2001-11-29 15:58:40 PST
Verified on 2001-11-29-Trunk build on WinNT.

Above test case passes.
Comment 21 User image bsharma 2001-12-03 09:49:00 PST
Marking as verified FIXED.
Comment 22 User image 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 User image 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 User image 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 User image 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 User image 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 User image 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 User image 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 User image 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 User image 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 User image Mitchell Stoltz (not reading bugmail) 2002-03-11 16:16:39 PST
Setting milestone to Moz1.0 - needs approval.
Comment 32 User image 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 User image 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 User image bsharma 2002-03-18 13:44:04 PST
Verified on 6.2.2 on WinNT.

Added AOLTWOK in the Status Whiteboard.
Comment 35 User image Jimmy Lee 2002-03-18 16:04:23 PST
Adding Michael.
Comment 36 User image joki (gone) 2002-03-27 01:28:12 PST
Fix checked in.
Comment 37 User image 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.