Key events from script can execute menu commands

VERIFIED FIXED in mozilla1.0

Status

()

Core
DOM: Events
--
critical
VERIFIED FIXED
16 years ago
15 years ago

People

(Reporter: Mitchell Stoltz (not reading bugmail), Assigned: joki (gone))

Tracking

Trunk
mozilla1.0
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: AOLTWOK)

Attachments

(5 attachments, 1 obsolete attachment)

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.
(Reporter)

Comment 1

16 years ago
Created attachment 56184 [details]
Example HTML - adds a bookmark
(Assignee)

Updated

16 years ago
Target Milestone: --- → mozilla0.9.7

Comment 2

16 years ago
Adding [discoverable] - this is a bug which is easy to figure out.
Whiteboard: [discoverable]

Comment 3

16 years ago
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.)

Updated

16 years ago
Whiteboard: [discoverable]
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

16 years ago
Adding PDT for tracking and 6.2 branch review purposes.
Whiteboard: PDT

Comment 6

16 years ago
What are the chances we'd have a patch for this one by this afternoon?
Whiteboard: PDT → [PDT] [ETA ?]
(Assignee)

Comment 7

16 years ago
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.
Status: NEW → ASSIGNED
Whiteboard: [PDT] [ETA ?] → PDT
(Assignee)

Comment 8

16 years ago
Created attachment 58412 [details] [diff] [review]
First proposed patch

Comment 9

16 years ago
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

16 years ago
who else can we get to review?

Comment 11

16 years ago
Tentative PDT+, pending positive reviews. We'd like to see this one land on the
6.2 branch by 9 am PST (if possible).
Whiteboard: PDT → [PDT+]
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.
(Assignee)

Comment 13

16 years ago
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.
(Assignee)

Updated

16 years ago
Attachment #58412 - Flags: superreview+
Attachment #58412 - Flags: review+
(Assignee)

Comment 14

16 years ago
Created attachment 58514 [details] [diff] [review]
Modified part of patch for nsEventStateManager.cpp

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

sr=jst
Attachment #58514 - Flags: superreview+
(Assignee)

Comment 16

16 years ago
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

16 years ago
Adding Antonio to Cc.  He is helping with Mac testing.

Comment 18

16 years ago
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.


(Reporter)

Comment 19

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

Comment 20

16 years ago
Verified on 2001-11-29-Trunk build on WinNT.

Above test case passes.
Status: ASSIGNED → RESOLVED
Last Resolved: 16 years ago
Resolution: --- → FIXED

Comment 21

16 years ago
Marking as verified FIXED.
Status: RESOLVED → VERIFIED

Comment 22

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

thanks

chris h."
Keywords: edt0.9.4+
(Reporter)

Comment 23

16 years ago
I'm having trouble modifying this patch for the 0.9.4 branch. joki, can you
help? reopening bug.
Status: VERIFIED → REOPENED
Resolution: FIXED → ---
(Reporter)

Comment 24

16 years ago
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.
(Assignee)

Comment 25

16 years ago
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
(Reporter)

Comment 26

16 years ago
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.
Attachment #60758 - Attachment is obsolete: true
(Reporter)

Comment 27

16 years ago
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?
Status: REOPENED → RESOLVED
Last Resolved: 16 years ago16 years ago
Resolution: --- → FIXED

Updated

16 years ago
Keywords: fixed0.9.4

Comment 28

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

Verifying
Status: RESOLVED → VERIFIED
(Reporter)

Comment 29

15 years ago
Arrgh, somehow part of the fix was never checked into the trunk! Reopening.
Status: VERIFIED → REOPENED
Resolution: FIXED → ---
(Reporter)

Comment 30

15 years ago
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.
(Reporter)

Comment 31

15 years ago
Setting milestone to Moz1.0 - needs approval.
Status: REOPENED → ASSIGNED
Keywords: edt0.9.4+, fixed0.9.4 → mozilla1.0, nsbeta1
Whiteboard: [PDT+]
Target Milestone: mozilla0.9.7 → mozilla1.0
Keywords: nsbeta1 → nsbeta1+
Comment on attachment 73593 [details] [diff] [review]
Rest of the fix for the trunk

sr=jst
Attachment #73593 - Flags: superreview+

Comment 33

15 years ago
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
Attachment #73593 - Flags: approval+

Comment 34

15 years ago
Verified on 6.2.2 on WinNT.

Added AOLTWOK in the Status Whiteboard.
Whiteboard: AOLTWOK

Comment 35

15 years ago
Adding Michael.
(Assignee)

Comment 36

15 years ago
Fix checked in.
Status: ASSIGNED → RESOLVED
Last Resolved: 16 years ago15 years ago
Resolution: --- → FIXED

Comment 37

15 years ago
verifying on build 2002-04-01-03-trunk win98
Status: RESOLVED → VERIFIED
(Reporter)

Updated

15 years ago
Group: security?
You need to log in before you can comment on or make changes to this bug.