Closed
Bug 698949
Opened 13 years ago
Closed 12 years ago
Refuse untrusted keypress events in editor
Categories
(Core :: DOM: Editor, enhancement)
Core
DOM: Editor
Tracking
()
RESOLVED
FIXED
mozilla12
People
(Reporter: masayuki, Assigned: masayuki)
References
Details
(Keywords: dev-doc-complete, Whiteboard: [inbound])
Attachments
(1 file, 2 obsolete files)
10.18 KB,
patch
|
smaug
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•13 years ago
|
||
Attachment #571526 -
Flags: review?(bugs)
Assignee | ||
Comment 2•13 years ago
|
||
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)
Assignee | ||
Comment 3•13 years ago
|
||
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)
Assignee | ||
Comment 4•13 years ago
|
||
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)
Assignee | ||
Updated•13 years ago
|
Assignee | ||
Updated•13 years ago
|
Attachment #571932 -
Attachment is obsolete: true
Assignee | ||
Comment 5•13 years ago
|
||
(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.
Updated•13 years ago
|
Attachment #571526 -
Flags: review?(bugs) → review+
Assignee | ||
Comment 6•12 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/147be2eb7800
Comment 7•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/147be2eb7800
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Comment 8•12 years ago
|
||
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•12 years ago
|
||
(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•12 years ago
|
||
Documentation revised: https://developer.mozilla.org/en/Using_the_Editor_from_XUL#Editor_event_handling Mentioned on Firefox 12 for developers.
Keywords: dev-doc-needed → dev-doc-complete
You need to log in
before you can comment on or make changes to this bug.
Description
•