Closed Bug 698949 Opened 13 years ago Closed 12 years ago

Refuse untrusted keypress events in editor

Categories

(Core :: DOM: Editor, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla12

People

(Reporter: masayuki, Assigned: masayuki)

References

Details

(Keywords: dev-doc-complete, Whiteboard: [inbound])

Attachments

(1 file, 2 obsolete files)

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.
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.
Attachment #571530 - Flags: review?(ehsan)
Attachment #571530 - Flags: review?(bugs)
The previous patch didn't fix all of them. This patch is the latest version.
Attachment #571530 - Attachment is obsolete: true
Attachment #571530 - Flags: review?(ehsan)
Attachment #571530 - Flags: review?(bugs)
Attachment #571932 - Flags: review?(ehsan)
Attachment #571932 - Flags: review?(bugs)
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.
Attachment #571932 - Flags: review?(ehsan)
Attachment #571932 - Flags: review?(bugs)
No longer blocks: 622245
Severity: normal → enhancement
Target Milestone: mozilla11 → ---
Attachment #571932 - Attachment is obsolete: true
(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.
Attachment #571526 - Flags: review?(bugs) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/147be2eb7800
Flags: in-testsuite+
Keywords: dev-doc-needed
Whiteboard: [inbound]
Target Milestone: --- → mozilla12
https://hg.mozilla.org/mozilla-central/rev/147be2eb7800
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
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.
(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.
Documentation revised:

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

Mentioned on Firefox 12 for developers.
Depends on: 749185
Depends on: 754529
Depends on: 817377
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: