[FILE]Drag and drop for file upload form control

RESOLVED FIXED in mozilla7

Status

()

Core
Layout: Form Controls
--
enhancement
RESOLVED FIXED
17 years ago
5 years ago

People

(Reporter: Steve Halasz, Assigned: Michael Ventnor)

Tracking

(Depends on: 1 bug)

Trunk
mozilla7
Points:
---
Dependency tree / graph
Bug Flags:
sec-review +
in-testsuite ?

Firefox Tracking Flags

(firefox7-)

Details

(Whiteboard: p-safari)

Attachments

(1 attachment, 1 obsolete attachment)

10.44 KB, patch
jst
: review+
Neil Deakin
: review+
mounir
: feedback+
Details | Diff | Splinter Review
(Reporter)

Description

17 years ago
It would be swell if dragging and dropping a file onto a file upload form 
control would insert the file path into the form.

Comment 1

17 years ago
Sounds like a very reasonable request. Will mark future for now until after the 
first release.

Adding pinkerton to see if he has any ideas.
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Target Milestone: --- → Future
should be pretty easy, we already have a flavor for files (application/x-moz-
file) which is just an nsILocalFile object. Rod, if you want to look at code that 
handles drops of files on the nav window, check out navigatorDD.js.

also look at http://www.mozilla.org/xpfe/xptoolkit/DataFlavors.html

Comment 3

17 years ago
Updating QA contact.
QA Contact: ckritzer → bsharma

Updated

17 years ago
Summary: Drag and drop for file upload form control → [FILE]Drag and drop for file upload form control

Updated

17 years ago
Target Milestone: Future → ---
(Reporter)

Comment 4

17 years ago
The code for handling files dropped on the content area has moved to 
/chrome/comm/content/communicator/contentAreaDD.js.

It appears that bug 36573 blocks this one. When the file is dropped on an input 
element of type Text or File aEvent.target.nodeName is empty. However, dropping 
the file on the 'Browse' button associated with the file input field causes 
aEvent.target.nodeName to become 'INPUT' as it should.
Depends on: 36573

Comment 5

16 years ago
QA Contact Update
QA Contact: bsharma → vladimire

Updated

16 years ago
Target Milestone: --- → Future

Comment 6

16 years ago
As of build 20010801708, dragging a file onto the "browse" button attepts to
download the file, which is definitely very unexpected behavior.  Dragging to
the text field does nothing.  Perhaps it should be upgraded from "enhancement"
to "trivial".  The non-functionality is a bit disappointing given that file drag
and drop works so well on mail attachments.
(Reporter)

Comment 7

15 years ago
Dragging and dropping in Linux build 2002022015 from Nautilus works! Dropping on
the text entry part of the file form control fills it in with
'file:/home/mozilla/test.html'. Shouldn't that be file:///home...?

I'm not wise in the ways of C++ but /content/base/src/nsContentAreaDragDrop.cpp
looks like its set up to just load the dropped item as a URL. This happens when
the file is dropped on the Browse... button in the file upload control. So
apparently the drop event is getting dealt with somewhere else first.

Updated

15 years ago
Priority: P3 → --

Comment 8

15 years ago
Under MacOS X (like I reported for OS 9 last August), dragging a file to a file
upload widget does the following:
 * on the text area: nothing
 * on the browser button: file download popup appears
The latter behavior is clearly incorrect.

Comment 9

15 years ago
I got bitten a bit by the current behaviour.  On linux/gnome I dragged a file
from the desktop to the file upload text area, and the filename appeared.  I
could only see the end of it.  When I hit the submit button, a zero length
upload was performed by mozilla.  The uploaded file was there in the request,
but of zero length.

The problem was the initial file: part, that I didn't see at first.  That file
wasn't there (with the file: part included), so a zero length upload was done. 
Once I removed file: and just left the filename, it uploaded correctly.

At least gnome always drops filenames as file:/ urls, I don't know about
other systems, but in this case I believe mozilla should remove the file: part
before trying to upload the file.

BTW, is a zero length upload ok for an upload of a non-existing file?  Shouldn't
the user get some kind of warning that nothing is being sent?

This is with 1.0.0 installed from Debian/testing.

Comment 10

14 years ago
This still doesn't work, on Mozilla 1.5b and WinXP.

Same thing; when you drag a file to the textarea it does nothing (well it shows
a blocking cursor), but when on the button (which gets highlighted) or anywhere
else it just tries to download the draged file.
The fact that we do not allowed dropping into a file upload control is very much
intentional to prevent attacks, triggered by a synthetic drag and drop event
combination (actually I think just the drop event is needed).  By allowing this,
we would open a huge can of worms which I think we'd be better off avoiding.
Assignee: rods → nobody
Status: ASSIGNED → NEW
QA Contact: vladimire → core.layout.form-controls
Whiteboard: WONTFIX?
(Reporter)

Comment 12

14 years ago
I can see that allowing JavaScript to change the contents of a file upload field
could allow a script to put a sensitive file in there. But is there really no
way to tell the difference between that and a drop? Or is the vulnerability
you're referencing something else. Can you explain it in a little more detail
please?

Comment 13

14 years ago
unfortunately some web developers whine about not being able to do real drag and
drop from web js. there's no guarantee caillon can prevent that from changing.

Comment 14

13 years ago
We don't allow dragging text into file upload controls; that's intentional (see
bug 206859).  But why shouldn't you be able to drag a file into a file upload
control?
*** Bug 243704 has been marked as a duplicate of this bug. ***

Comment 16

13 years ago
*** Bug 262106 has been marked as a duplicate of this bug. ***
*** Bug 263663 has been marked as a duplicate of this bug. ***

Comment 18

12 years ago
What's going on with this bug? Will it ever be fixed?

Comment 19

12 years ago
*** Bug 310220 has been marked as a duplicate of this bug. ***

Comment 20

12 years ago
I think this would be better UI than forcing users to go through a file picker if they already have the file visible on the desktop or a folder that's open in Finder or Explorer.

I also think this feature would be pretty safe.  Some attacks that might be worth worrying about:

* Tricking a user into thinking that dragging a file to the web page will do something desirable but not reveal its contents.  Seems unlikely to me.

* Depending on settings, a malicious site might be able to move a browser window out from under the mouse pointer just as you're about to drag something in the web page, causing you to start dragging a somewhat random file from your desktop, and then move the browser window back before you drop.

* If you have both http://www.evil.com/ and https://bugzilla.mozilla.org/ open, and http://www.evil.com/ is able to reference the domwindow in which https://bugzilla.mozilla.org/ is open, http://www.evil.com/ could replace the https://bugzilla.mozilla.org/ tab/window with itself just as you're about to drop.

All of those attacks would be hard to pull off successfully.  They could all be mitigated with a confirmation dialog: "Do you want to upload a copy of testcase.html to https://bugzilla.mozilla.org/?".  Most of them could be mitigated without a dialog, by requiring that there have been no big changes (URL channges, tab switches, window moves/resizes/focuses) in the 2 seconds prior to receiving a dropped file.

Comment 21

11 years ago
*** Bug 324484 has been marked as a duplicate of this bug. ***

Comment 22

11 years ago
i would expect drag to be supported.

a malicious site could set the value of the field to C://WINNT/foo.bar without simulating a drop event anyway right?

Comment 23

11 years ago
No, web sites are not allowed to change the text in file upload controls.  If they are able to, it's considered a high-severity security hole.
*** Bug 344828 has been marked as a duplicate of this bug. ***

Comment 25

11 years ago
See also bug 347178, which asks for a JavaScript "onDragFile" event.

Comment 26

11 years ago
*** Bug 357991 has been marked as a duplicate of this bug. ***
+100.

Well that feature would be cool. Last decade, I every time i have to send a file over web (and as active user and developer I do that all the time) my mind clunks why nobody has implemented that.

P.S. I think I should send that report to Apple - Safari would pick the feature in instant ;-)

Comment 28

10 years ago
This basic requirement has been fixed by an add-in https://addons.mozilla.org/mozilla/2190/ or (for seamonkey the related http://xsidebar.mozdev.org/modifiedmisc.html#dragdropupload )

Updated

10 years ago
Duplicate of this bug: 395083

Comment 30

10 years ago
My FF extension DragDropUpload is here: http://www.teslacore.it/wiki/index.php?title=DragDropUpload and https://addons.mozilla.org/extensions/moreinfo.php?application=firefox&id=2190

In particular it is useful when dropping multiple files (e.g. GMail and Flickr) if the site supports it.

In FF 3.0 the bug #258875 breaks the extension, and several people are complaining to me. Is there any workaround to disable the protection #258875 from an extension?
> In FF 3.0 the bug #258875 breaks the extension

How, exactly?

Comment 32

10 years ago
The extension relies on dropping of files from the system shell (e.g Windows Explorer) or from the extension sidebar over the file input box. 

The disabled control of #258875 prevents this behavior, and seems from people answering my request in #258875, that an extension cannot enable back the input field. 

You could just set the .value of the input in your drop handler, no?
Whiteboard: WONTFIX? → WONTFIX? p-safari

Comment 34

10 years ago
Why do you need to install an extension to use such a basic feature?
Why isn't it possible by default?
I mean, i don't ask for a javascript to do it, but that the application should take care of a drag and drop from the user shell to an input file control(which would fill in the textbox with the right path).
Why using an external extension should be more secure?

Comment 35

10 years ago
I agree with you, the direct support of this feature in FF is something really asked

Comment 36

10 years ago
The situation is even worse in FF3.0b2 as you can't even enter anything in the textbox of a file input as it's in readonly mode now.
Say bye bye to the copy and paste of file's path from the Shell explorer.
All you can do is to use the Browse button...
You have the file right under your mouse, but no, you have to loose your time browsing your folders again and again to find this file while a simple drag & drop would be done in 1 second.
I totally agree that you must not allow the Website to do it in Javascript, but the Browser should be able to do it securely on user interaction.

Comment 37

9 years ago
+1 for this. Emanuele's DragDropUpload extension is a life-saver, I'd be really sorry to have to switch back to tedious folder browsing in FF3.

This being said, Emanuele, you did not answer to Boris's suggestion in #33 : 

> You could just set the .value of the input in your drop handler, no?

Comment 38

9 years ago
(In reply to comment #36)
> The situation is even worse in FF3.0b2 as you can't even enter anything in the
> textbox of a file input as it's in readonly mode now.

bug 374011

Comment 39

9 years ago
Since FF3.0b3 it is possible to use the extension by dropping the file over the Browse Button of the input field, but not over the grayed area.

The .value property can be set.

Updated

9 years ago
Duplicate of this bug: 438907

Comment 41

9 years ago
It seems that this bug has had very little activity lately. Getting this fixed would help me out a lot.
Duplicate of this bug: 505012
Whiteboard: WONTFIX? p-safari → p-safari

Updated

8 years ago
Duplicate of this bug: 507347

Comment 44

8 years ago
I voted for this bug. It seems to me that this is a basic feature, quite safe to implement and very useful.

If the developers still consider this feature to be dangerous, I'd suggest it could be disabled by default. In order to enable it the user would have to do something like: 

Tools > Options >  Advanced > "[_] Allow drag and drop files into file upload forms?" > Mark the checkbox. 

Once the option is checked, a warning message could finally prompt the user: 

"Allowing drag and drop files into file upload forms implies such and such security risks. Do you sill want to allow drag and drop files into file upload forms? Yes No Cancel Help"
Duplicate of this bug: 544854

Updated

7 years ago

Comment 46

6 years ago
Now that we support DOM events for grabbing dropped files, there's no security reason to not implement this feature for plain <input type=file> elements.

https://developer.mozilla.org/En/DragDrop/DataTransfer

https://developer.mozilla.org/en/using_files_from_web_applications#Selecting_files_using_drag_and_drop

Comment 47

6 years ago
So very sorry to see this is *still* not implemented in 4.0b12 (:-( Aaarggghhh! Can I buy votes for this must-have feature? (;-) Kinda ironic to have to use Safari to report Firefox bugs.

Comment 48

6 years ago
(In reply to comment #47)
> So very sorry to see this is *still* not implemented in 4.0b12 (:-( Aaarggghhh!
> Can I buy votes for this must-have feature? (;-) Kinda ironic to have to use
> Safari to report Firefox bugs.

(ironic comment from a non-Mozillian)

Sure you can! Hire a programmer to create a patch :) and attach it here.
(Assignee)

Comment 49

6 years ago
I've decided to work on this, and have spent most of the day implementing a patch.
Assignee: nobody → ventnor.bugzilla
(Assignee)

Comment 50

6 years ago
Created attachment 533900 [details] [diff] [review]
Patch

Dragged this patch to the file upload control ;)

jst, I think you're the most appropriate person for this?
Attachment #533900 - Flags: review?(jst)
Cool! Needs a test :-).
(Assignee)

Comment 52

6 years ago
Created attachment 534375 [details] [diff] [review]
Patch 2

Sorry roc and jst but I won't be able to write a test for this. This is due to this line here under nsDomDataTransfer::GetFiles:

  if (mEventType != NS_DRAGDROP_DROP && mEventType != NS_DRAGDROP_DRAGDROP)
    return NS_OK;

The way we handle a dataTransfer in test drags is to capture it from a dragStart event, then use it in all other events. But, the dataTransfer saves its original event type (which is _START) and there is no way in JS to change this, so it triggers the early return.

I assume it's like this for a reason, eg. so a page can't read the files on a dragover event.

I did fix a couple bugs in finding this, but that's a whole day down the drain...
Attachment #533900 - Attachment is obsolete: true
Attachment #534375 - Flags: review?(jst)
Attachment #533900 - Flags: review?(jst)
Comment on attachment 534375 [details] [diff] [review]
Patch 2

I'd actually like to hear what Mounir has to say about this before I review.
Attachment #534375 - Flags: review?(mounir.lamouri)
Comment on attachment 534375 [details] [diff] [review]
Patch 2

Review of attachment 534375 [details] [diff] [review]:
-----------------------------------------------------------------

f=mounir with the nits fixed.
Moving the review of the dragover/drop events handling to Neil.

::: content/html/content/src/nsHTMLInputElement.cpp
@@ +1390,5 @@
> +      mFiles.AppendObject(file);
> +    }
> +  }
> +
> +  AfterSetFiles(aSetValueChanged);

Couldn't you just create a nsCOMArray<nsIDOMFile> from the nsIDOMFileList and call SetFiles() with the array in parameter? That would prevent creating another method with a name that I personally find confusing.

::: content/html/content/src/nsHTMLInputElement.h
@@ +148,5 @@
> +
> +  // Forward nsIDOMHTMLElement
> +  NS_FORWARD_NSIDOMHTMLELEMENT_NOFOCUSCLICK(nsGenericHTMLFormElement::)
> +  NS_IMETHOD Focus();
> +  NS_IMETHOD Click();

This chunk doesn't seem to change something.

::: layout/forms/nsFileControlFrame.cpp
@@ +274,5 @@
> +  NS_ENSURE_STATE(dragTarget);
> +  dragTarget->AddEventListener(NS_LITERAL_STRING("drop"),
> +                               mMouseListener, PR_FALSE);
> +  dragTarget->AddEventListener(NS_LITERAL_STRING("dragover"),
> +                               mMouseListener, PR_FALSE);

IMO, it's weird to be able to drag-n-drop a file on the "Browse" button but I see that's what Safari and Chrome do.

@@ +534,5 @@
> +    return NS_OK;
> +  }
> +  
> +  nsCOMPtr<nsIDOMDragEvent> dragEvent = do_QueryInterface(aEvent);
> +  if (dragEvent) {

Here, you should do:
if (!dragEvent || !IsValidDropData(dragEvent)) {
  return NS_OK;
}

It will save some indentation.

@@ +540,5 @@
> +    aEvent->GetType(eventType);
> +    if (eventType.EqualsLiteral("dragover") &&
> +        IsValidDropData(dragEvent)) {
> +      // Prevent default if we can accept this drag data
> +      aEvent->PreventDefault();

And here, that could be:
if (eventType.EqualsLiteral("dragover")) {
  aEvent->PreventDefault();
  return NS_OK;
}

And BTW, why do you need to prevent default on dragover?

@@ +544,5 @@
> +      aEvent->PreventDefault();
> +    } else if (eventType.EqualsLiteral("drop")) {
> +      // We probably shouldn't let any drop data propagate from a file
> +      // upload control
> +      aEvent->StopPropagation();

Why?

@@ +551,5 @@
> +        aEvent->PreventDefault();
> +
> +        nsIContent* content = mFrame->GetContent();
> +        if (!content)
> +          return NS_ERROR_FAILURE;

You can probably assert here instead of returning NS_ERROR_FAILURE.

@@ +555,5 @@
> +          return NS_ERROR_FAILURE;
> +
> +        nsHTMLInputElement* inputElement = nsHTMLInputElement::FromContent(content);
> +        if (!inputElement)
> +          return NS_ERROR_FAILURE;

For sure, you should assert here.

@@ +584,5 @@
> +
> +  // We only support dropping files onto a file upload control
> +  PRBool typeSupported;
> +  types->Contains(NS_LITERAL_STRING("Files"), &typeSupported);
> +  return typeSupported;

You can also get the file list and check if it's empty.

::: layout/forms/nsFileControlFrame.h
@@ +40,5 @@
>  
>  #include "nsBlockFrame.h"
>  #include "nsIFormControlFrame.h"
>  #include "nsIDOMMouseListener.h"
> +#include "nsIDOMDragEvent.h"

Do the include in the cpp file and do a forward declaration in the header instead.

@@ +168,5 @@
>    
>    class BrowseMouseListener: public MouseListener {
>    public:
>      BrowseMouseListener(nsFileControlFrame* aFrame) : MouseListener(aFrame) {};
> +    NS_IMETHOD MouseClick(nsIDOMEvent* aMouseEvent);

You broke the indentation here.

@@ +171,5 @@
>      BrowseMouseListener(nsFileControlFrame* aFrame) : MouseListener(aFrame) {};
> +    NS_IMETHOD MouseClick(nsIDOMEvent* aMouseEvent);
> +    NS_IMETHOD HandleEvent(nsIDOMEvent* aEvent);
> +  private:
> +    PRBool IsValidDropData(nsIDOMDragEvent* aEvent);

You don't need this method to be private it should actually be a class method (ie. static method). And you should return a |bool| instead of a |PRBool|.
Attachment #534375 - Flags: review?(mounir.lamouri)
Attachment #534375 - Flags: review?(enndeakin)
Attachment #534375 - Flags: feedback+
I don't know if it's already in this patch but it would be nice to change the cursor when hovering the drop zone. For example on Mac, this cursor is a plus on a green background.
(Assignee)

Comment 56

6 years ago
(In reply to comment #54)
> Comment on attachment 534375 [details] [diff] [review] [review]
> Patch 2
> 
> Review of attachment 534375 [details] [diff] [review] [review]:
> -----------------------------------------------------------------
> 
> f=mounir with the nits fixed.
> Moving the review of the dragover/drop events handling to Neil.
> 
> ::: content/html/content/src/nsHTMLInputElement.cpp
> @@ +1390,5 @@
> > +      mFiles.AppendObject(file);
> > +    }
> > +  }
> > +
> > +  AfterSetFiles(aSetValueChanged);
> 
> Couldn't you just create a nsCOMArray<nsIDOMFile> from the nsIDOMFileList
> and call SetFiles() with the array in parameter? That would prevent creating
> another method with a name that I personally find confusing.

I don't know about this, wouldn't it be faster to simply use the existing list rather than allocate a new one? Also, I don't see what's confusing about the name especially since it's a private method.

> ::: content/html/content/src/nsHTMLInputElement.h
> @@ +148,5 @@
> > +
> > +  // Forward nsIDOMHTMLElement
> > +  NS_FORWARD_NSIDOMHTMLELEMENT_NOFOCUSCLICK(nsGenericHTMLFormElement::)
> > +  NS_IMETHOD Focus();
> > +  NS_IMETHOD Click();
> 
> This chunk doesn't seem to change something.

I know. I couldn't figure it out either and couldn't get rid of it.

> ::: layout/forms/nsFileControlFrame.cpp
> @@ +274,5 @@
> > +  NS_ENSURE_STATE(dragTarget);
> > +  dragTarget->AddEventListener(NS_LITERAL_STRING("drop"),
> > +                               mMouseListener, PR_FALSE);
> > +  dragTarget->AddEventListener(NS_LITERAL_STRING("dragover"),
> > +                               mMouseListener, PR_FALSE);
> 
> IMO, it's weird to be able to drag-n-drop a file on the "Browse" button but
> I see that's what Safari and Chrome do.

Bigger drop area = better usability :)

> @@ +534,5 @@
> > +    return NS_OK;
> > +  }
> > +  
> > +  nsCOMPtr<nsIDOMDragEvent> dragEvent = do_QueryInterface(aEvent);
> > +  if (dragEvent) {
> 
> Here, you should do:
> if (!dragEvent || !IsValidDropData(dragEvent)) {
>   return NS_OK;
> }
> 
> It will save some indentation.
> 
> @@ +540,5 @@
> > +    aEvent->GetType(eventType);
> > +    if (eventType.EqualsLiteral("dragover") &&
> > +        IsValidDropData(dragEvent)) {
> > +      // Prevent default if we can accept this drag data
> > +      aEvent->PreventDefault();
> 
> And here, that could be:
> if (eventType.EqualsLiteral("dragover")) {
>   aEvent->PreventDefault();
>   return NS_OK;
> }

Will do.

> And BTW, why do you need to prevent default on dragover?

Because that's what other drop targets seem to do (like the browser), and apparently (looking at synthesizeDrop() in the test harness) if a dragover event is not consumed, that is interpreted as having nothing that takes a drop at that point.

> @@ +544,5 @@
> > +      aEvent->PreventDefault();
> > +    } else if (eventType.EqualsLiteral("drop")) {
> > +      // We probably shouldn't let any drop data propagate from a file
> > +      // upload control
> > +      aEvent->StopPropagation();
> 
> Why?

I couldn't think of a good reason to allow it, and thought for security reasons it's best to not let websites find out what you drag in there immediately upon drop.

> @@ +551,5 @@
> > +        aEvent->PreventDefault();
> > +
> > +        nsIContent* content = mFrame->GetContent();
> > +        if (!content)
> > +          return NS_ERROR_FAILURE;
> 
> You can probably assert here instead of returning NS_ERROR_FAILURE.
> 
> @@ +555,5 @@
> > +          return NS_ERROR_FAILURE;
> > +
> > +        nsHTMLInputElement* inputElement = nsHTMLInputElement::FromContent(content);
> > +        if (!inputElement)
> > +          return NS_ERROR_FAILURE;
> 
> For sure, you should assert here.

OK.

> @@ +584,5 @@
> > +
> > +  // We only support dropping files onto a file upload control
> > +  PRBool typeSupported;
> > +  types->Contains(NS_LITERAL_STRING("Files"), &typeSupported);
> > +  return typeSupported;
> 
> You can also get the file list and check if it's empty.

Wouldn't that be taken care of already? If we get an empty file list, we set the file upload control blank, and I think that's a good behaviour.

> ::: layout/forms/nsFileControlFrame.h
> @@ +40,5 @@
> >  
> >  #include "nsBlockFrame.h"
> >  #include "nsIFormControlFrame.h"
> >  #include "nsIDOMMouseListener.h"
> > +#include "nsIDOMDragEvent.h"
> 
> Do the include in the cpp file and do a forward declaration in the header
> instead.
> 
> @@ +168,5 @@
> >    
> >    class BrowseMouseListener: public MouseListener {
> >    public:
> >      BrowseMouseListener(nsFileControlFrame* aFrame) : MouseListener(aFrame) {};
> > +    NS_IMETHOD MouseClick(nsIDOMEvent* aMouseEvent);
> 
> You broke the indentation here.
> 
> @@ +171,5 @@
> >      BrowseMouseListener(nsFileControlFrame* aFrame) : MouseListener(aFrame) {};
> > +    NS_IMETHOD MouseClick(nsIDOMEvent* aMouseEvent);
> > +    NS_IMETHOD HandleEvent(nsIDOMEvent* aEvent);
> > +  private:
> > +    PRBool IsValidDropData(nsIDOMDragEvent* aEvent);
> 
> You don't need this method to be private it should actually be a class
> method (ie. static method). And you should return a |bool| instead of a
> |PRBool|.

OK.

I'd like some more opinion of what I discussed here, otherwise I'll fix what I can after everyone else's reviews.

Comment 57

6 years ago
Comment on attachment 534375 [details] [diff] [review]
Patch 2

@@ +584,5 @@
>> +
>> +  // We only support dropping files onto a file upload control
>> +  PRBool typeSupported;
>> +  types->Contains(NS_LITERAL_STRING("Files"), &typeSupported);
>> +  return typeSupported;

> You can also get the file list and check if it's empty.

No, you shouldn't do that during dragover, as it will unnecessarily cause the need to retrieve the data of the drag from the source application which can be slow.
Attachment #534375 - Flags: review?(enndeakin) → review+

Updated

6 years ago
Attachment #534375 - Flags: review?(jst) → review+
(Assignee)

Comment 58

6 years ago
http://hg.mozilla.org/mozilla-central/rev/5c6d107ede5a
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Maybe this should be tracked for Firefox 7 as a new feature.
tracking-firefox7: --- → ?

Updated

6 years ago
Target Milestone: Future → mozilla7
Flags: in-testsuite?

Comment 60

6 years ago
Has any part of this already landed in FF 4?  I'm asking because today I experienced windows locked file problems twice after accidentally dropping a file onto a FF4 window.  After FF captured the drop, it seemed the file was locked in the OS indefinitely until I killed the FF process.
(Assignee)

Comment 61

6 years ago
This will be in Firefox 7 at the earliest.

As there is problems with automatically testing this bug, we'd need a litmus test.

Updated

6 years ago
Flags: in-testsuite?
Flags: in-testsuite-
Flags: in-litmus?
We have some automated tests for drag-and-drop, why can't we automate a test here?
(Assignee)

Comment 63

6 years ago
(In reply to comment #62)
> We have some automated tests for drag-and-drop, why can't we automate a test
> here?

Because of GetFiles(). It blocks us getting the files if the original event of the transferData was not "drop", but the way we make a dataTransfer in script is to synthesise a "dragstart" event and use that.
That sounds fixable.
(Assignee)

Comment 65

6 years ago
That would require exposing something very sensitive to script, I thought. I did look into it while I was trying to write a test, and the only place this field is written to is the constructor and copy-constructor, so I assumed the field was kept private for security reasons.
Make it require chrome privileges and make your test a mochitest-chrome test.
Flags: in-testsuite?
Flags: in-testsuite-
Flags: in-litmus?

Updated

6 years ago
Depends on: 662267
dchan to review and comment for security team
Keywords: sec-review-needed

Comment 68

6 years ago
I talked to Michael and dveditz about the implementation. One of the main concerns was outlined by Jesse in comment 20 , tricking the user to drag/drop onto the wrong site. 

A malicious site could frame a good site which has a drag and drop. However the malicious site wouldn't be able to access the file contents due to scripting restrictions. The code prevents event propagation for a drag and drop event.

A similar attack would be if code injection was found on a good site and used to frame a bad site drag/drop control. However this is a moot point since the attacker can already inject their own code on the good site.

The last concern was if there were non-file elements in the DataTransfer object. The code retrieves a file list and ignores non-file elements.


We may want to revisit drag and drop as the HTML5 File API is implemented, but the review for this bug has been completed.
Keywords: sec-review-needed → sec-review-complete

Comment 69

6 years ago
If there are regressions from this, please nominate those issues for tracking. Thanks.
tracking-firefox7: ? → -

Comment 70

5 years ago
This broke after firefox 6. For example in confluence's drag and drop box for attachments. Works fine in Chrome & Safari.

Comment 71

5 years ago
(In reply to Sabuj Pattanayek from comment #70)
> This broke after firefox 6. For example in confluence's drag and drop box
> for attachments. Works fine in Chrome & Safari.

Please file a new bug on that with specific steps to reproduce, marking it blocking this one.

Comment 72

5 years ago
nm, https://issues.alfresco.com/jira/browse/ALF-10582 it's up to you guys to support older APIs and websites that use them. If you guys won't then I'll just have to make upgrades elsewhere.
Flags: sec-review+
Keywords: sec-review-complete
You need to log in before you can comment on or make changes to this bug.