Last Comment Bug 698949 - Refuse untrusted keypress events in editor
: Refuse untrusted keypress events in editor
Status: RESOLVED FIXED
[inbound]
: dev-doc-complete
Product: Core
Classification: Components
Component: Editor (show other bugs)
: Trunk
: All All
: -- enhancement (vote)
: mozilla12
Assigned To: Masayuki Nakano [:masayuki] (Mozilla Japan)
:
Mentors:
Depends on: 303713 700199 749185 754529 817377
Blocks: 622245
  Show dependency treegraph
 
Reported: 2011-11-01 17:15 PDT by Masayuki Nakano [:masayuki] (Mozilla Japan)
Modified: 2012-12-02 05:50 PST (History)
5 users (show)
masayuki: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch part.1 Editor should refuse untrusted keypress event (10.18 KB, patch)
2011-11-02 18:00 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan)
bugs: review+
Details | Diff | Review
Patch part.2 Use real key event path for sendKey(), sendChar() and sendString() in EventUtils.js (68.06 KB, patch)
2011-11-02 18:15 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan)
no flags Details | Diff | Review
Patch part.2 Use real key event path for sendKey(), sendChar() and sendString() in EventUtils.js (71.32 KB, patch)
2011-11-04 04:43 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan)
no flags Details | Diff | Review

Description Masayuki Nakano [:masayuki] (Mozilla Japan) 2011-11-01 17:15:08 PDT
Now, our editor accepts untrusted keypress events which was implemented in bug 303713. This has some problems:

1. We need to dispatch textinput event if a keypress event causes inserting text. However, if script is blocked by some reasons at that time, textinput event would be dispatched asynchronously. I think that web developers who use untrusted events for modifying editor content must expect synchronous modification. So, anyway, such script might be broken by implementing textinput event.

2. No other browser accepts untrusted keypress event in its editor. Web applications should use another way for cross browser application.

3. There is no spec about untrusted keypress and textinput events for editor in D3E. Even if we reimplement untrusted keypress event handling, we should do it after it's documented in the spec.

I think that we should remove to support it for now at least.
Comment 1 Masayuki Nakano [:masayuki] (Mozilla Japan) 2011-11-02 18:00:30 PDT
Created attachment 571526 [details] [diff] [review]
Patch part.1 Editor should refuse untrusted keypress event
Comment 2 Masayuki Nakano [:masayuki] (Mozilla Japan) 2011-11-02 18:15:06 PDT
Created attachment 571530 [details] [diff] [review]
Patch part.2 Use real key event path for sendKey(), sendChar() and sendString() in EventUtils.js

This makes sendKey(), sendChar() and sendString() in EventUtils.js use trusted key events which is dispatched by realistic code path. I means each test needs to set focus to the target before dispatching key events. It might change selection by focus event handling.

I mean using untrusted events for testing trusted event's behavior is wrong thing. We should use actual code path as far as possible.

I don't change most tests' logic except:

> docshell/test/navigation/test_bug430723.html

tests both down and up keys' results rather than only up key's.

> editor/libeditor/text/tests/test_bug641466.html

There is no backspace key. There is only back_space key.

> toolkit/components/passwordmgr/test/test_basic_form_autocomplete.html

This test has a bug. Test 700 doesn't set focus before dispatching key events. It causes different result than actual behavior. Test 700 should cause showing up the autocomplete popup. See bug 497541 comment 12 and later comments.
Comment 3 Masayuki Nakano [:masayuki] (Mozilla Japan) 2011-11-04 04:43:55 PDT
Created attachment 571932 [details] [diff] [review]
Patch part.2 Use real key event path for sendKey(), sendChar() and sendString() in EventUtils.js

The previous patch didn't fix all of them. This patch is the latest version.
Comment 4 Masayuki Nakano [:masayuki] (Mozilla Japan) 2011-11-06 18:32:24 PST
Comment on attachment 571932 [details] [diff] [review]
Patch part.2 Use real key event path for sendKey(), sendChar() and sendString() in EventUtils.js

I misunderstood the DOM3 textinput event. That is fired *after* modification in editor, so, the untrusted textinput events cannot be a trigger for modifying editor contents. Therefore, we don't need to fix this bug for now.

However, I still worry about that is editor to be able to be modified by untrusted key events. It can be a cause of security issue like bug 497541. And other browsers don't support this feature. So, there is not compatibility issues except with our older versions.
Comment 5 Masayuki Nakano [:masayuki] (Mozilla Japan) 2011-11-08 17:02:08 PST
(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #4)
> I misunderstood the DOM3 textinput event. That is fired *after* modification
> in editor, so, the untrusted textinput events cannot be a trigger for
> modifying editor contents. Therefore, we don't need to fix this bug for now.

This is wrong per bug 622245 comment 33.
Comment 6 Masayuki Nakano [:masayuki] (Mozilla Japan) 2011-12-21 04:14:34 PST
https://hg.mozilla.org/integration/mozilla-inbound/rev/147be2eb7800
Comment 7 Ed Morley [:emorley] 2011-12-21 12:54:13 PST
https://hg.mozilla.org/mozilla-central/rev/147be2eb7800
Comment 8 Eric Shepherd [:sheppy] 2012-04-24 08:22:36 PDT
Is the gist of this that keypress events now have to come from trusted sources (ie, chrome) now? That seems to be all there is to it.
Comment 9 :Ehsan Akhgari (out sick) 2012-04-24 11:33:50 PDT
(In reply to Eric Shepherd [:sheppy] from comment #8)
> Is the gist of this that keypress events now have to come from trusted
> sources (ie, chrome) now? That seems to be all there is to it.

Precisely.
Comment 10 Eric Shepherd [:sheppy] 2012-04-25 13:43:36 PDT
Documentation revised:

https://developer.mozilla.org/en/Using_the_Editor_from_XUL#Editor_event_handling

Mentioned on Firefox 12 for developers.

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