Last Comment Bug 50660 - [FILE]Drag and drop for file upload form control
: [FILE]Drag and drop for file upload form control
Status: RESOLVED FIXED
p-safari
:
Product: Core
Classification: Components
Component: Layout: Form Controls (show other bugs)
: Trunk
: All All
: -- enhancement with 30 votes (vote)
: mozilla7
Assigned To: Michael Ventnor
:
Mentors:
: 243704 262106 263663 310220 324484 344828 357991 395083 438907 505012 507347 544854 (view as bug list)
Depends on: 662267 36573
Blocks:
  Show dependency treegraph
 
Reported: 2000-08-29 07:40 PDT by Steve Halasz
Modified: 2012-08-15 08:10 PDT (History)
68 users (show)
dveditz: sec‑review+
mounir: in‑testsuite?
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
-


Attachments
Patch (10.44 KB, patch)
2011-05-20 00:28 PDT, Michael Ventnor
no flags Details | Diff | Splinter Review
Patch 2 (10.44 KB, patch)
2011-05-22 23:25 PDT, Michael Ventnor
jst: review+
enndeakin: review+
mounir: feedback+
Details | Diff | Splinter Review

Description Steve Halasz 2000-08-29 07:40:54 PDT
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 rods (gone) 2000-08-29 10:09:31 PDT
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.
Comment 2 Mike Pinkerton (not reading bugmail) 2000-08-29 12:16:18 PDT
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 bsharma 2000-09-26 18:01:13 PDT
Updating QA contact.
Comment 4 Steve Halasz 2000-12-13 18:07:30 PST
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.
Comment 5 Vladimir Ermakov 2001-03-06 16:55:00 PST
QA Contact Update
Comment 6 Chris Dolan 2001-08-17 13:42:16 PDT
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.
Comment 7 Steve Halasz 2002-02-21 11:43:14 PST
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.
Comment 8 Chris Dolan 2002-02-28 07:33:30 PST
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 René Seindal 2002-08-12 16:23:10 PDT
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 erik johansson 2003-09-16 01:05:02 PDT
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.
Comment 11 Christopher Aillon (sabbatical, not receiving bugmail) 2004-01-14 22:49:41 PST
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.
Comment 12 Steve Halasz 2004-01-15 08:54:53 PST
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 timeless 2004-01-15 21:43:37 PST
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 Jesse Ruderman 2004-02-17 00:56:27 PST
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?
Comment 15 Christian :Biesinger (don't email me, ping me on IRC) 2004-05-15 14:25:36 PDT
*** Bug 243704 has been marked as a duplicate of this bug. ***
Comment 16 José Jeria 2004-09-29 07:48:59 PDT
*** Bug 262106 has been marked as a duplicate of this bug. ***
Comment 17 Boris Zbarsky [:bz] (TPAC) 2004-10-16 16:06:51 PDT
*** Bug 263663 has been marked as a duplicate of this bug. ***
Comment 18 Noam Tamim 2005-02-14 00:40:35 PST
What's going on with this bug? Will it ever be fixed?
Comment 19 Jesse Ruderman 2005-09-27 22:34:51 PDT
*** Bug 310220 has been marked as a duplicate of this bug. ***
Comment 20 Jesse Ruderman 2005-12-25 14:39:38 PST
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 Peter Weilbacher 2006-01-24 10:11:47 PST
*** Bug 324484 has been marked as a duplicate of this bug. ***
Comment 22 paul stanton 2006-05-24 16:56:14 PDT
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 Jesse Ruderman 2006-05-24 17:40:35 PDT
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.
Comment 24 Dave Townsend [:mossop] 2006-07-16 06:52:07 PDT
*** Bug 344828 has been marked as a duplicate of this bug. ***
Comment 25 Jesse Ruderman 2006-08-03 22:45:34 PDT
See also bug 347178, which asks for a JavaScript "onDragFile" event.
Comment 26 Jesse Ruderman 2006-10-27 06:56:35 PDT
*** Bug 357991 has been marked as a duplicate of this bug. ***
Comment 27 Ihar 'Philips' Filipau 2006-11-18 05:12:36 PST
+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 Francis 2007-03-01 07:34:40 PST
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 )

Comment 29 Stuart Morgan 2007-09-05 15:03:53 PDT
*** Bug 395083 has been marked as a duplicate of this bug. ***
Comment 30 Emanuele Ruffaldi 2007-11-12 00:49:00 PST
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?
Comment 31 Boris Zbarsky [:bz] (TPAC) 2007-11-12 09:04:39 PST
> In FF 3.0 the bug #258875 breaks the extension

How, exactly?
Comment 32 Emanuele Ruffaldi 2007-11-12 09:08:19 PST
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. 

Comment 33 Boris Zbarsky [:bz] (TPAC) 2007-11-12 09:33:51 PST
You could just set the .value of the input in your drop handler, no?
Comment 34 Dricks 2007-12-18 02:27:05 PST
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 Emanuele Ruffaldi 2007-12-19 00:46:04 PST
I agree with you, the direct support of this feature in FF is something really asked
Comment 36 Dricks 2007-12-19 08:16:38 PST
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 y.chedemois 2007-12-30 20:22:00 PST
+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 Biju 2008-02-26 19:16:04 PST
(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 Emanuele Ruffaldi 2008-02-29 06:37:30 PST
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.
Comment 40 Sylvain Pasche 2008-06-12 12:50:57 PDT
*** Bug 438907 has been marked as a duplicate of this bug. ***
Comment 41 Roger Peters 2008-11-17 12:09:04 PST
It seems that this bug has had very little activity lately. Getting this fixed would help me out a lot.
Comment 42 Phil Ringnalda (:philor) 2009-07-18 06:29:36 PDT
*** Bug 505012 has been marked as a duplicate of this bug. ***
Comment 43 Neil Deakin (away until Oct 3) 2009-07-30 08:02:10 PDT
*** Bug 507347 has been marked as a duplicate of this bug. ***
Comment 44 Carlos Alén Silva 2009-08-16 08:24:17 PDT
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"
Comment 45 Ria Klaassen (not reading all bugmail) 2010-02-08 08:03:58 PST
*** Bug 544854 has been marked as a duplicate of this bug. ***
Comment 46 Jesse Ruderman 2011-02-18 14:27:23 PST
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 Doug Grinbergs 2011-03-01 20:08:25 PST
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 Matěj Cepl 2011-03-02 00:24:28 PST
(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.
Comment 49 Michael Ventnor 2011-05-20 00:25:18 PDT
I've decided to work on this, and have spent most of the day implementing a patch.
Comment 50 Michael Ventnor 2011-05-20 00:28:27 PDT
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?
Comment 51 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-05-20 07:13:33 PDT
Cool! Needs a test :-).
Comment 52 Michael Ventnor 2011-05-22 23:25:14 PDT
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...
Comment 53 Johnny Stenback (:jst, jst@mozilla.com) 2011-05-25 16:40:24 PDT
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.
Comment 54 Mounir Lamouri (:mounir) 2011-05-26 07:58:23 PDT
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|.
Comment 55 Anthony Ricaud (:rik) 2011-05-26 08:23:18 PDT
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.
Comment 56 Michael Ventnor 2011-05-26 19:09:28 PDT
(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 Neil Deakin (away until Oct 3) 2011-05-30 04:26:38 PDT
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.
Comment 59 Mounir Lamouri (:mounir) 2011-06-01 02:49:38 PDT
Maybe this should be tracked for Firefox 7 as a new feature.
Comment 60 John M. Black 2011-06-01 06:32:03 PDT
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.
Comment 61 Michael Ventnor 2011-06-01 06:34:19 PDT
This will be in Firefox 7 at the earliest.

As there is problems with automatically testing this bug, we'd need a litmus test.
Comment 62 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-06-01 17:47:07 PDT
We have some automated tests for drag-and-drop, why can't we automate a test here?
Comment 63 Michael Ventnor 2011-06-01 19:44:25 PDT
(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.
Comment 64 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-06-01 19:47:58 PDT
That sounds fixable.
Comment 65 Michael Ventnor 2011-06-01 19:57:40 PDT
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.
Comment 66 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-06-01 21:25:34 PDT
Make it require chrome privileges and make your test a mochitest-chrome test.
Comment 67 Curtis Koenig [:curtisk-use curtis.koenig+bzATgmail.com]] 2011-06-16 15:59:39 PDT
dchan to review and comment for security team
Comment 68 David Chan [:dchan] 2011-06-27 10:29:01 PDT
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.
Comment 69 Asa Dotzler [:asa] 2011-07-05 19:08:44 PDT
If there are regressions from this, please nominate those issues for tracking. Thanks.
Comment 70 Sabuj Pattanayek 2012-05-03 14:30:22 PDT
This broke after firefox 6. For example in confluence's drag and drop box for attachments. Works fine in Chrome & Safari.
Comment 71 Chris Lawson (gone) 2012-05-03 14:35:57 PDT
(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 Sabuj Pattanayek 2012-05-03 14:38:32 PDT
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.

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