Dragging an image into designMode iframe can't be prevented

RESOLVED FIXED

Status

()

Firefox
General
RESOLVED FIXED
8 years ago
8 years ago

People

(Reporter: Michael Davidson, Assigned: Neil Deakin)

Tracking

unspecified
x86
Windows XP
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(blocking2.0 beta1+)

Details

(Whiteboard: [evang-wanted], URL)

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

8 years ago
User-Agent:       Mozilla/5.0 (X11; U; Linux x86_64; en-US) AppleWebKit/532.8 (KHTML, like Gecko) Chrome/4.0.288.1 Safari/532.8
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2) Gecko/20100105 Firefox/3.6 GTB6 (.NET CLR 3.5.30729)

Using the new drag/drop file handling in FF 3.6, if you drag an image file into an iframe in designMode, the HTML is modified to include an <img> with src of the file:/// URL of the image you dragged. This cannot be prevented. 

Reproducible: Always

Steps to Reproduce:
1. Drag an image from the FS into the frame.
2. Notice the alert (which is the custom drag/drop handling)

Actual Results:  
There is an <img> added to the HTML of the frame (verifiable in Firebug).

Expected Results:  
There should be no <img> added since the script calls preventDefault()
(Reporter)

Updated

8 years ago
blocking2.0: --- → ?
Neil, is part of your drag'n'drop code? Or is this a bug somewhere in editor? This is sort of a big deal since it means that you can't use our new-fangled File drag'n'drop support doesn't work with rich text editing.
(Assignee)

Comment 2

8 years ago
Two issues here:

1. The editor drop event listener is firing before the one added by the testcase.
2. The editor drop event listener doesn't check if the event has been cancelled.

The editor should really be handing everything as part of the default phase of the event handling, or it should add the listener in the system event group.

As a workaround, change:
  target.addEventListener('drop', drop, false);
to:
  target.documentElement.addEventListener('drop', drop, false);

such that this listener occurs before the editor one.
(Reporter)

Comment 3

8 years ago
In the code I'm working on we have a contentEditable body in an iframe, so the timing might be different there. I'll try the workaround in the meantime.
Neil, could you take this bug on?
(Assignee)

Updated

8 years ago
Assignee: nobody → enndeakin
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Whiteboard: [evang-wanted]
(Assignee)

Updated

8 years ago
Depends on: 545714
(Assignee)

Comment 6

8 years ago
Created attachment 427639 [details] [diff] [review]
Fix bug. The patch is dependent on other bugs
(Assignee)

Updated

8 years ago
Blocks: 554400
(Assignee)

Comment 7

8 years ago
Created attachment 434921 [details] [diff] [review]
version 2
Attachment #427639 - Attachment is obsolete: true
(Assignee)

Updated

8 years ago
Attachment #434921 - Flags: review?(Olli.Pettay)

Comment 8

8 years ago
Comment on attachment 434921 [details] [diff] [review]
version 2

>diff --git a/editor/libeditor/base/nsEditor.cpp b/editor/libeditor/base/nsEditor.cpp
>--- a/editor/libeditor/base/nsEditor.cpp
>+++ b/editor/libeditor/base/nsEditor.cpp
>@@ -364,26 +364,36 @@ nsEditor::InstallEventListeners()
>                                         NS_GET_IID(nsIDOMTextListener));
> 
>   rv |= piTarget->AddEventListenerByIID(mEventListener,
>                                         NS_GET_IID(nsIDOMCompositionListener));
> 
>   nsCOMPtr<nsIDOMEventTarget> target(do_QueryInterface(piTarget));
>   if (target) {
>     // See bug 455215, we cannot use the standard dragstart event yet
>-    rv |= target->AddEventListener(NS_LITERAL_STRING("draggesture"),
>-                                   mEventListener, PR_FALSE);
>-    rv |= target->AddEventListener(NS_LITERAL_STRING("dragenter"),
>-                                   mEventListener, PR_FALSE);
>-    rv |= target->AddEventListener(NS_LITERAL_STRING("dragover"),
>-                                   mEventListener, PR_FALSE);
>-    rv |= target->AddEventListener(NS_LITERAL_STRING("dragleave"),
>-                                   mEventListener, PR_FALSE);
>-    rv |= target->AddEventListener(NS_LITERAL_STRING("drop"),
>-                                   mEventListener, PR_FALSE);
>+    rv |= elmP->AddEventListenerByType(mEventListener, NS_LITERAL_STRING("draggesture"),
>+                                       NS_EVENT_FLAG_BUBBLE |
>+                                       NS_PRIV_EVENT_UNTRUSTED_PERMITTED,
>+                                       sysGroup);
>+    rv |= elmP->AddEventListenerByType(mEventListener, NS_LITERAL_STRING("dragenter"),
>+                                       NS_EVENT_FLAG_BUBBLE |
>+                                       NS_PRIV_EVENT_UNTRUSTED_PERMITTED,
>+                                       sysGroup);
>+    rv |= elmP->AddEventListenerByType(mEventListener, NS_LITERAL_STRING("dragover"),
>+                                       NS_EVENT_FLAG_BUBBLE |
>+                                       NS_PRIV_EVENT_UNTRUSTED_PERMITTED,
>+                                       sysGroup);
>+    rv |= elmP->AddEventListenerByType(mEventListener, NS_LITERAL_STRING("dragleave"),
>+                                       NS_EVENT_FLAG_BUBBLE |
>+                                       NS_PRIV_EVENT_UNTRUSTED_PERMITTED,
>+                                       sysGroup);
>+    rv |= elmP->AddEventListenerByType(mEventListener, NS_LITERAL_STRING("drop"),
>+                                       NS_EVENT_FLAG_BUBBLE |
>+                                       NS_PRIV_EVENT_UNTRUSTED_PERMITTED,
>+                                       sysGroup);
>   }
>
Do we really want to allow untrusted events?
I guess that depends on other browser, do they support dispatching 
drag events to contentEditable or designMode?

> nsresult
> nsEditorEventListener::DragOver(nsIDOMDragEvent* aDragEvent)
> {
I wonder if there should be a check somewhere here that whether
preventDefault() was called.

>+
>+  // If there is no source node, this is an external drag and the drop is allowed.
Always? Even if script dispatches the event?
(Assignee)

Comment 9

8 years ago
> Do we really want to allow untrusted events?

Probably not, but I'll test some more.

> > nsresult
> > nsEditorEventListener::DragOver(nsIDOMDragEvent* aDragEvent)
> > {
> I wonder if there should be a check somewhere here that whether
> preventDefault() was called.

Sounds like a good idea.


> >+
> >+  // If there is no source node, this is an external drag and the drop is allowed.
> Always? Even if script dispatches the event?

Perhaps not, but it wouldn't be a security issue. The remaining checks are only relevant when there is a source node of the drag (that is, they prevent drops on the start of the drag). I'll clarify the comment.
Comment on attachment 434921 [details] [diff] [review]
version 2

I assume there is a new patch coming.
Attachment #434921 - Flags: review?(Olli.Pettay) → review-
How does the patch in bug 512717 affect this bug?
(Assignee)

Comment 12

8 years ago
Created attachment 438078 [details] [diff] [review]
address comments
Attachment #434921 - Attachment is obsolete: true
Attachment #438078 - Flags: review?(Olli.Pettay)
Comment on attachment 438078 [details] [diff] [review]
address comments

r+, but this really needs tests.
Attachment #438078 - Flags: review?(Olli.Pettay) → review+
(Assignee)

Comment 14

8 years ago
Tests are dependent on editor using the new api (bug 499008).
OK. Please add tests somewhere.
(Assignee)

Comment 16

8 years ago
http://hg.mozilla.org/mozilla-central/rev/701fd8e1377c
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
(Assignee)

Updated

8 years ago
Flags: in-testsuite?
You need to log in before you can comment on or make changes to this bug.