Last Comment Bug 258875 - Disable direct input of filename into file upload controls
: Disable direct input of filename into file upload controls
Status: RESOLVED FIXED
See bug 56236 and bug 57770 for more ...
: csectype-spoof, css-moz, css3, sec-high, testcase
Product: Core
Classification: Components
Component: Layout: Form Controls (show other bugs)
: Trunk
: All All
: -- critical with 4 votes (vote)
: mozilla1.9alpha8
Assigned To: Robert O'Callahan (:roc) (Exited; email my personal email if necessary)
:
Mentors:
http://www.quirksmode.org/dom/inputfi...
: 56236 57770 325717 (view as bug list)
Depends on: 431098 314367 345195 355362 374011 399968 420251 469280
Blocks: 314365
  Show dependency treegraph
 
Reported: 2004-09-11 10:20 PDT by Chris Cook
Modified: 2013-06-09 18:45 PDT (History)
57 users (show)
asa: blocking1.8b5-
pavlov: blocking1.9+
mbeltzner: blocking1.8.1-
mtschrep: blocking1.8.1.1-
dveditz: wanted1.8.1.x-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Basic test page (285 bytes, text/html)
2004-09-11 10:22 PDT, Chris Cook
no flags Details
Demonstration of exploit (2.45 KB, text/html)
2004-09-11 12:08 PDT, Chris Cook
no flags Details
fix (3.90 KB, patch)
2005-08-02 20:59 PDT, Robert O'Callahan (:roc) (Exited; email my personal email if necessary)
no flags Details | Diff | Splinter Review
fix #2 (5.43 KB, patch)
2005-09-26 16:42 PDT, Robert O'Callahan (:roc) (Exited; email my personal email if necessary)
dbaron: review+
dbaron: superreview+
Details | Diff | Splinter Review
fix (30.14 KB, patch)
2007-07-15 20:34 PDT, Robert O'Callahan (:roc) (Exited; email my personal email if necessary)
mats: review-
mats: superreview-
Details | Diff | Splinter Review
updated patch (33.12 KB, patch)
2007-07-24 16:43 PDT, Robert O'Callahan (:roc) (Exited; email my personal email if necessary)
no flags Details | Diff | Splinter Review
updated again (33.16 KB, patch)
2007-07-24 18:04 PDT, Robert O'Callahan (:roc) (Exited; email my personal email if necessary)
mats: review-
mats: superreview-
Details | Diff | Splinter Review
updated again! (36.95 KB, patch)
2007-07-24 21:33 PDT, Robert O'Callahan (:roc) (Exited; email my personal email if necessary)
mats: superreview+
Details | Diff | Splinter Review
updated yet again (37.49 KB, patch)
2007-09-25 16:34 PDT, Robert O'Callahan (:roc) (Exited; email my personal email if necessary)
jonas: review+
roc: superreview+
Details | Diff | Splinter Review

Description Chris Cook 2004-09-11 10:20:16 PDT
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7) Gecko/20040803 Firefox/0.9.3
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7) Gecko/20040803 Firefox/0.9.3

When an input of type="file" has an opacity applied to it, the element is styled
accordingly. This allows the element to be made completely invisible but still
functional, likely allowing the element to be exploited.

Reproducible: Always
Steps to Reproduce:
Comment 1 Chris Cook 2004-09-11 10:22:12 PDT
Created attachment 158546 [details]
Basic test page

This page simply demonstrates the behavior. It does not attempt to demonstrate
an exploit.
Comment 2 Chris Cook 2004-09-11 12:08:19 PDT
Created attachment 158555 [details]
Demonstration of exploit

This page demonstrates an exploit of the bug. This exploit works by misleading
the user into typing in the path to a local file and then uploading that file
without the user's knowledge.
Comment 3 Daniel Veditz [:dveditz] 2004-09-18 01:14:32 PDT
Confirming.
Comment 4 Chris Cook 2004-10-05 18:00:23 PDT
This security-sensitive bug has been open for almost a month and has been
confirmed and yet no one deems it worthy to even comment on? I have been patient
but I think this bug deserves attention.

I realize the potential for the argument that compliance with the CSS RFC must
not be a bug but I have not found any part of the RFC which states which
elements the style MUST or MUST NOT apply to.
Comment 5 Chris Cook 2004-10-05 21:18:39 PDT
Rereading the CSS3 color module
(http://www.w3.org/TR/2003/CR-css3-color-20030514/#transparency) I see that it
"Applies to: all elements" so perhaps this issue is not a CSS bug after all.

I have presented the issue to the CSS-Discuss mailing list for feedback.
Comment 6 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2004-10-05 22:53:52 PDT
CSS doesn't require this.  However, there are so many other ways to do things
like this that I'm not really sure what good fixing this would do.

I'd be perfectly happy to switch to native, completely unstylable form widgets,
but a lot of other people wouldn't.
Comment 7 Chris Cook 2004-10-05 23:13:10 PDT
(In reply to comment #6)
> CSS doesn't require this.  However, there are so many other ways to do things
> like this that I'm not really sure what good fixing this would do.

You may be right. I had already started thinking of other methods of hiding the
input with CSS.

As an alternative resolution, I would view the enhancement proposed in the
following attachment (the second option at the bottom) as an option, albeit
perhaps a naggy one.

https://bugzilla.mozilla.org/attachment.cgi?id=17860&action=view
Comment 8 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2004-10-06 06:12:42 PDT
You could do exactly the same thing even if we turned off styling of form
elements completely... just position some images on top of the control, around
the edges of the control.

I think the only reasonable way to fix this is to remove the text entry box
completely and only allow use via the filepicker. But I know that has problems
for computer savvy users on some platforms where the filepicker doesn't let you
paste in a filename. Perhaps we could have two buttons: one that launches the OS
filepicker, one which pops up an alert where you can paste the file name.
Comment 9 Jesse Ruderman 2005-07-29 16:47:13 PDT
I'm making this bug public because everything here (except for roc's idea for a
fix) has been discussed in public bugs.

Getting rid of the editable filename box would also fix bug 57770 and bug 56236,
which make it even easier to steal files, even from careful users who know about
file upload controls.
Comment 10 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2005-08-02 16:59:56 PDT
What should be done here? I can see a few options and there may be others:
-- just make the textbox non-editable (simplest, maybe the best fix for 1.8)
-- replace the textbox with a plain text element
-- remove the textbox, let the button fill the whole area and put the file name
as additional text inside the button

Any suggestions/preferences?
Comment 11 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2005-08-02 19:52:13 PDT
Safari does the second option.
Comment 12 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2005-08-02 20:59:32 PDT
Created attachment 191442 [details] [diff] [review]
fix

This patch fixes the issue with the first approach --- just turns the textfield
readonly. It also removes it from the tabbing order and makes any accesskey
point to the button instead. We also adjust the appearance so it doesn't look
so much like a textfield.

I like this fix but I'm not sure who should review it or even who should make
this sort of decision in general.
Comment 13 Torben 2005-08-03 17:15:08 PDT
> Safari does the second option.

Then why don't we do the same?
IMHO security features should be as similar across browsers as possible to avoid
user confusion ("Which browser am I using? What was it I was not supposed to do?").
Comment 14 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2005-08-03 17:28:52 PDT
1) and 2) are pretty similar as far as the user is concerned.
Comment 15 Mike Beltzner [:beltzner, not reading bugmail] 2005-08-03 17:41:19 PDT
(In reply to comment #11)
> Safari does the second option.

Likely because file paths aren't all that common in Mac OS, whereas file
browsers are. I think that in terms of getting this bug squared away, always
launching the file browser is fine. After all, the user can still paste the file
path into that file browser, and if a single click on the element launches the
browser, it makes no diffence in terms of number of keystrokes. 
Comment 16 Mike Beltzner [:beltzner, not reading bugmail] 2005-08-03 17:42:14 PDT
Er, where "always launching the file browser" means, making it read only and
interpreting a click in the field to be the same as a click on the browse
button. Sorry to be unclear.
Comment 17 Boris Zbarsky [:bz] 2005-08-04 20:20:54 PDT
A lot of users actually type in the text input of a file control...  This is
compounded by the very poor usability of some of our filepickers (the GTK2 one
comes to mind).
Comment 18 Mike Beltzner [:beltzner, not reading bugmail] 2005-08-05 00:00:05 PDT
(In reply to comment #17)
> compounded by the very poor usability of some of our filepickers (the GTK2 one
> comes to mind).

That's the one I've not seen - does the GTK2 filepicker not have a textbox in
which users can just enter a full pathname? If it does, then as I see things,
moving to an interaction where putting focus on the textbox of a file input
control launches the filepicker results in the same experience for the user;
that is to say, instead of typing directly into the textbox of the file input
area, they type directly into the textbox in the file picker. Plus, there's
zero-confusion about the fact that they are specifying a local resource.
Comment 19 Boris Zbarsky [:bz] 2005-08-05 06:29:46 PDT
> does the GTK2 filepicker not have a textbox in which users can just enter a full 
> pathname?

See screenshot at <http://primates.ximian.com/%7Efederico/news-2004-02.html#23>.
I _think_ one can launch a prompt with such a textbox from the filepicker via a
totally undiscoverable keyboard shortcut, but I don't remember what that
shortcut is, and I bet no actual users know it.
Comment 20 James Ross 2005-08-05 07:05:08 PDT
"You can press Control-L to bring up a popup dialog that will let you type a
filename directly, just like in Nautilus." -- from the page you linked to ;)

The chances of any user *knowing* this is basically 0, rendering the 'feature'
useless, as mentioned.
Comment 21 Christian :Biesinger (don't email me, ping me on IRC) 2005-08-05 07:18:53 PDT
I assume that that dialog was targeted at users who rarely type filenames in at
all... and I disagree that it's unusable, I happen to like it :)
Comment 22 Mike Beltzner [:beltzner, not reading bugmail] 2005-08-05 09:27:47 PDT
(In reply to comment #19)
> See screenshot at <http://primates.ximian.com/%7Efederico/news-2004-02.html#23>.
> I _think_ one can launch a prompt with such a textbox from the filepicker via a

Boy. That filepicker sure isn't good for people who like to type in filepaths
directly. However, I'm not so sure that we want that to be the reason we decide
to maintain direct-typing support in our file input widget. The file picker
works in ways that support the platform users for w32 (ie: quick access to type
in a file path) and os x (ie: browser only, but that's the mac way) and I'm
assuming that not all linux desktops are saddled with this UI (please correct
that assumption if it's wrong).
Comment 23 Boris Zbarsky [:bz] 2005-08-05 11:07:28 PDT
All Linux desktops on which a sufficiently new version of GTK2 is installed are
saddled with this UI, even if the user uses KDE and not GNOME (or uses neither).
Comment 24 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2005-08-07 16:41:00 PDT
The GTK2 filepicker sucks but we should not use that as an excuse to not use it.
There are two things to be done about it:
-- get the GTK2 people to fix it
-- add support to Mozilla to use the KDE filepicker when KDE is running
Comment 25 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2005-08-07 21:37:51 PDT
One thing we could do for GTK2 is try using the gtk_file_selection dialog
instead of gtk_file_chooser. gtk_file_selection has a text input directly in the
file dialog.
Comment 26 Mats Palmgren (:mats) 2005-08-08 10:12:57 PDT
Would it be possible to make the readonly bit a config option?
I paste paths directly into the text field part nearly 100% of the time I use
a file control, e.g. uploading patches and testcases to Bugzilla.

Re: comment 24
> -- add support to Mozilla to use the KDE filepicker when KDE is running

FYI, that is bug 298848
Comment 27 Jesse Ruderman 2005-08-08 12:06:13 PDT
> Would it be possible to make the readonly bit a config option?

I would hope that if you saw the attachments on bug 57770 and bug 56236, you
wouldn't want to reopen this hole for youself.

Having a config option for a security hole is generally not a good idea. As
we've seen with the XPI delay, people will re-enable a security hole without
understanding how vulnerable it will make them, if they perceive that the fix
for the security hole causes them inconvenience.

Also, having a config option for whether the file upload input is read-only
might prevent storing the file information somewhere other than in the text
input, which might be important for guarding against future security holes.
Comment 28 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2005-08-08 13:19:05 PDT
Summary of feedback:
1) GTK2 filepicker sucks
2) bug 111821 means Windows filepicker is sometimes slow
3) text controls are useful for pasting many filenames with small edits

I talked to some drivers (asa, caillon and bsmedberg) and they think none of
this justifies leaving normal users exposed to potentially serious spoofing
attacks. There was no support for enabling the obsolete GTK2 filepicker or the
XUL filepicker, even under a hidden pref. On that basis, I think we should go
ahead and do this.

For the record, if you want to paste a file name in GTK2, bring up the file
dialog and press ctrl-L ctrl-V. It's not discoverable, but absolute file paths
aren't either.

The issues with filepicker slowness are going to have to be solved in their own
bugs. Whoever was complaining about GTK2 filepicker slowness (Mats?) should try
upgrading GTK... I believe some issues were fixed in GTK 2.4.10.
Comment 29 Jesse Ruderman 2005-08-09 11:10:19 PDT
Btw, to paste a filename into the Mac file picker, press Cmd+Shift+G.
Comment 30 Mike Beltzner [:beltzner, not reading bugmail] 2005-08-09 11:17:34 PDT
What I'm seeing is that always bringing up the filepicker ..
 - makes it very clear that someone's browsing to a file location
 - puts the ease of use onus for direct entry of paths onto the OS

I'm happy with this approach, too. Roc, I'm assuming the patch takes the text
field out of the tabbing order, but keeps the button in, right?
Comment 31 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2005-08-09 13:24:26 PDT
(In reply to comment #30)
> Roc, I'm assuming the patch takes the text
> field out of the tabbing order, but keeps the button in, right?

Yes.
Comment 32 Jens Bannmann 2005-08-11 01:17:21 PDT
Why is it even possible to read the file path from the text field? This might
not be that important anymore when it's changed to be readonly, but I was
surprised that the exploit linked in comment 7 did actually work (without
clicking submit)...
Comment 33 Aaron P 2005-08-17 07:41:41 PDT
Perhaps, while the file textbox HAS focus, Firefox could do something to the
textbox. 

One idea would be to simply force the textbox on top of everything, temporarily
override the style with something usable, and (if necessary) move the viewport
to show the textbox. When the textbox looses focus, it would revert back to how
it was before.

Another idea would be to show a separate textbox (maybe at the bottom of the
screen, right above the status bar) that actually accepts user input (and has a
file picker button). As you are typing in the separate textbox, the event would
also be sent to the original textbox. When the separate textbox looses focus, it
would disappear.
Comment 34 Mike Beltzner [:beltzner, not reading bugmail] 2005-08-17 11:10:09 PDT
(In reply to comment #33)
> file picker button). As you are typing in the separate textbox, the event would
> also be sent to the original textbox. When the separate textbox looses focus, it
> would disappear.

I think I'd rather force the platform-standard UI mechanism for entering files
than insert a FFx-only half measure. I'm pretty sure that taking user focus away
from current position and moving it to a browsermessage element at the bottom of
the page would be a little more confusing/distracting than just popping up a
file picker.
Comment 35 Jens Bannmann 2005-08-17 14:15:10 PDT
(In reply to comment #34)
> I think I'd rather force the platform-standard UI mechanism for entering files
> than insert a FFx-only half measure. I'm pretty sure that taking user focus away
> from current position and moving it to a browsermessage element at the bottom of
> the page would be a little more confusing/distracting than just popping up a
> file picker.

Mike, the problem that needs to be solved is that at least on one platform, you
can't type in file paths inside the file picker (or it's not obvious how to do
it), so we should keep some way to do that in addition to the "Browse..." button
that invokes the file picker.
Comment 36 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2005-08-17 14:19:33 PDT
That is a platform problem, not our problem, and we're going to disable the text
box as indicated in comment #28.
Comment 37 Mark Janssen 2005-09-03 07:07:39 PDT
Konqueror has an nice solution by simply letting the user know what files will
be uploaded when the form (and files) are submitted/uploaded.
Comment 38 Mike Beltzner [:beltzner, not reading bugmail] 2005-09-03 09:00:25 PDT
An additional confirmation would be overkill in a normal, non-security-violation
use case. Imagining how this sol'n would look with the current file upload control:

  1. User manually enterers file path or selects using file picker widget.
  2. User fills out rest of form.
  3. User presses "Submit"
  4. Gets confirmation window about what file will be uploaded.

So the confirmation dialog basically becomes an "are you sure" dialog, at which
point a "never ask me again" checkbox becomes tempting, at which point the value
of the confirmation dialog is pretty much eliminated.

The nice thing about the implemented solution is that we simply ensure that the
user always understands that they're actually selecting a file in step 1, and we
skip step 4 altogether.

Comment 39 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2005-09-03 13:50:52 PDT
In reality confirmation dialogs are almost entirely useless. Users see them as
merely an impediment to achieving their goals, and quickly learn to click
through without reading them. If you watch yourself carefully you'll probably
see yourself doing it.
Comment 40 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2005-09-26 16:42:37 PDT
Created attachment 197487 [details] [diff] [review]
fix #2

This version makes the text field clickable, so when you click on it we pop up
the file dialog.
Comment 41 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2005-09-30 14:58:36 PDT
Comment on attachment 197487 [details] [diff] [review]
fix #2

r+sr=dbaron, although I'm not happy about this.
Comment 42 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2005-09-30 15:02:14 PDT
checked in. Let the flaming commence!
Comment 43 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2005-09-30 15:03:43 PDT
Comment on attachment 197487 [details] [diff] [review]
fix #2

shooting for branch approval. This really should be in beta2 if we're going to
do it at all.
Comment 44 neil@parkwaycc.co.uk 2005-09-30 17:51:59 PDT
Comment on attachment 197487 [details] [diff] [review]
fix #2

>+      textControl->SetDisabled(PR_TRUE);
Odd, when I break into gdb and look at the text control it's magically enabled
again, which is handy otherwise it would never get any events ;-)
Comment 45 neil@parkwaycc.co.uk 2005-10-01 07:00:17 PDT
Comment on attachment 197487 [details] [diff] [review]
fix #2

>+      textControl->SetDisabled(PR_TRUE);
This doesn't happen, as the text control's disabled attribute is synchronized
to the file control. Otherwise you would never recieve events on it ;-)

>-input[type="file"] > input[type="text"] {
>+input[type="file"] > input[type="text"][disabled] {
>   border-color: inherit;
>   background-color: inherit;
>+  -moz-user-focus: ignore !important;
I don't think -moz-user-focus applies to HTML any more, you'd have to check
with aaronlev to see if setting tabindex to -1 would get the same effect. Note
that if it did work, then "ignore" would work to leave the focus where it was
previously, while "none" would make the canvas take focus. You may find it
easier to make just the file control focusable instead.

>+  /* enable user input so that clicking on the control can bring up the
>+     file dialog. It's still read-only. */
>+  -moz-user-input: enabled;
This doesn't actually work, see above, and looks wrong for
input[type="file"][disabled] if it did.
Comment 46 Asa Dotzler [:asa] 2005-10-03 10:40:59 PDT
aaronl, dbaron, and roc, can you guys address neil's comments and give us a
better idea of why what risk is associated with taking this into 1.5?
Comment 47 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2005-10-05 13:57:35 PDT
Given that it's not right, let's not take this on branch. At least, not now.
I'll try to get it working on trunk and we can backport it to a branch point
release if it becomes necessary.
Comment 48 Felix Miata 2005-10-29 15:54:40 PDT
Since this is "not right" anyway, any chance of altering the fix so that readonly is removed once the filepicker has been used to select a file? On http://validator.w3.org/ I often want to upload a series of files in short order with filenames differing only by one character from a directory with hundreds of files. Manual editing is a lot more convenient than a filepicker in this situation.
Comment 49 Jesse Ruderman 2005-10-29 16:03:19 PDT
mrmazda, no.  Firefox should not assume that just because you uploaded a file to a web site, it's ok for the web site to steal other files without your permission.
Comment 50 Felix Miata 2005-10-29 16:28:07 PDT
This is a core bug. I use SeaMonkey. SM users could be provided a hidden pref, but filing an enhancement request bug for it would be premature before this gets verified.
Comment 51 Jesse Ruderman 2005-10-29 17:37:08 PDT
I filed bug 314365 as a followup bug to remove the textbox.

Fixing these bugs would make things easier for some of the people who complained about this change:
* Bug 50660, Drag and drop for file upload form control
* Bug 314367, Provide a (safe) way to enter a filename to upload (e.g. "Enter filename..." on context menu)
Comment 52 neil@parkwaycc.co.uk 2005-11-26 16:39:52 PST
(In reply to comment #47)
>I'll try to get it working on trunk
Comment 53 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2006-01-08 14:02:15 PST
The current situation actually isn't too bad. You can't tab to the file control. Clicking on it brings up the file dialog. If you cancel that dialog, focus remains in the file control's text field, but typing doesn't enter anything into the field. So I think all the currently submitted exploits are blocked. Is there actually any way to type into the text field, on trunk?
Comment 54 Jesse Ruderman 2006-01-08 14:10:57 PST
I don't think so.  I even tried various methods of dragging and pasting.  (Someone on Linux should test middle-click pasting and context-menu pasting.)

Removing the textbox would improve the UI and might prevent future security holes; see bug 314365 comment 0.
Comment 55 neil@parkwaycc.co.uk 2006-01-08 14:29:36 PST
(In reply to comment #53)
>The current situation actually isn't too bad.
Then would you mind patching the code to reflect it? i.e.
a) remove the useless call to SetDisabled that isn't having any effect
b) revert the forms.css changes that might be allowing the input to be styled (since it isn't being disabled)
Comment 56 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2006-01-08 15:40:06 PST
Sure, I'll do that.
Comment 57 Christian :Biesinger (don't email me, ping me on IRC) 2006-01-08 16:33:57 PST
middle-clicking the file control on linux doesn't do anything (other than put a non-blinking cursor in the field)
Comment 58 Jeff Walden [:Waldo] (remove +bmo to email) 2006-01-09 21:39:58 PST
I seem to recall some griping on IRC about this, and someone raised the point that once a file is selected this way there's no way to un-select it short of resetting the whole form via forced reload or a reset button.
Comment 59 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2006-01-10 12:29:34 PST
That's a good point. Perhaps 'cancel' in the filepicker dialog should clear the field?
Comment 60 James Ross 2006-01-10 12:42:40 PST
Clearing the box if you cancel the dialog would be hidious UI, and would be a complete PITA to use. Just put a [ Clear ] button next to [ Browse... ].
Comment 61 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2006-01-10 12:57:05 PST
How about, clicking on the textfield clears the file name before launching the filepicker?
Comment 62 James Ross 2006-01-10 13:00:16 PST
Please, no!

I have wanted to copy the text out of that damn screwy box before, and having to click it, cancel the browse dialog and then use the keyboard to select it is a bad enough experience as it is. Please do not make it any worse.
Comment 63 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2006-01-10 13:23:57 PST
That doesn't sound like something anyone actually does. I think I'll try the click-to-clear.
Comment 64 James Ross 2006-01-10 13:28:08 PST
You're saying that just because I do it that automatically no-one else has ever done it. Your logic is even worse than my cat's.

Go fuck up what little usability is left, but don't blame me if people don't like it.
Comment 65 Jesse Ruderman 2006-01-10 14:31:55 PST
I also think click-to-clear is a bad idea.  I often click trying to make sure I've attached the right file, or trying to copy the path and filename.  

I don't think people need to clear file upload controls often enough to warrant a "Clear" or "No file" button.  There should be a "Clear" context menu item, and you should be able to clear the field by pressing Delete or Backspace while the file upload control has focus.
Comment 66 Alex Vincent [:WeirdAl] 2006-01-10 14:48:40 PST
(In reply to comment #65)
>There should be a "Clear" context menu item...

Interesting you mention that; I filed bug 312869 for *correctly* adding context menu items to <xul:textbox/>.  Unfortunately, I'm not sure it would help for HTML input elements.

Also interesting, but not relevant to this bug directly:  right-clicking on a <html:input type="file"/> brings up a different context menu (browser navigation) than using the context menu button for Windows does (input contents edit menu)
Comment 68 Dave Townsend [:mossop] 2006-02-03 16:37:37 PST
*** Bug 325717 has been marked as a duplicate of this bug. ***
Comment 69 Micah Bucy 2006-02-27 05:25:17 PST
how about a browse window that pops up if the user attempts to type or paste to the input field, or on clicking the browse button.  the window could be something like in the extension Get File https://addons.mozilla.org/extensions/moreinfo.php?id=1925&application=firefox 

there would be a "browse" button that calls the filepicker, a textfield where the user can type or paste the location, an "okay" button would accept the transaction, and a "cancel" button which would cancel the transaction.  

the input field would be read-only, allowing the user to copy the text in it at his pleasure.  if there was already text in the input field, the browse window should intercept this, putting the location in the location field and browsing to the correct location if the filepicker is called.  

also if the user clicks cancel at this point, the text in the input field should remain.  if the location field is blank and the user clicks okay, the input field should be cleared.
Comment 70 Micah Bucy 2006-02-27 05:29:30 PST
if the above is done, it would then be possible to allow full styling of inputs with type="file", fixing bugs such as https://bugzilla.mozilla.org/show_bug.cgi?id=52500
Comment 71 Micah Bucy 2006-02-27 05:37:13 PST
sorry for the triple post, I just thought of something else.  right-click menus should be disabled on the input field.  

also, depending on the difficulty of the code, or if this is even implemented, the browse window could intercept pasting and keystrokes to the input field on the webpage and automatically put that in the location field in the browse window.
Comment 72 Jesse Ruderman 2006-03-17 01:57:43 PST
*** Bug 57770 has been marked as a duplicate of this bug. ***
Comment 73 Jesse Ruderman 2006-03-17 01:59:59 PST
*** Bug 56236 has been marked as a duplicate of this bug. ***
Comment 74 Boris Zbarsky [:bz] 2006-04-27 12:42:05 PDT
So the CSS changes we made do break rendering of file inputs on trunk, generally speaking.... We should really fix that before we ship anything.
Comment 75 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2006-06-14 10:54:10 PDT
Plussing for 1.8.1, but we probably want something with slightly different UI from the current patch.
Comment 76 Darin Fisher 2006-06-26 11:50:31 PDT
If this is going to make FF2 beta1, then it needs to happen ASAP.
Comment 77 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2006-06-26 15:10:56 PDT
I don't really have the cycles to get this into b1, since I'm helping brettw get his inline spellchecking stuff done.
Comment 78 Ryan Flint [:rflint] (ping via IRC for reviews) 2006-06-28 16:04:39 PDT
*** Bug 343047 has been marked as a duplicate of this bug. ***
Comment 79 Mike Beltzner [:beltzner, not reading bugmail] 2006-06-28 18:06:50 PDT
Having just played with this on Minefield on OS X and Windows, I really don't think it's that bad as is (even with the fact that on Windows the field might look disabled) and would rather have it on branch as-is than leave the exploit open.
Comment 80 Mike Beltzner [:beltzner, not reading bugmail] 2006-06-28 18:11:53 PDT
Looking at this more with darin, schrep and dbaron, I think it would be better if we listened for text input and showed the file selection dialog if we caught any of that. That way we could:

 - keep the field looking active
 - keep the field in the tab order
 - keep everything the same

in Windows we can even pass the text input into the file chooser.
Comment 81 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2006-06-28 18:24:43 PDT
What about potential attacks involving drag/drop or pasting into the textfield?

Also, we still need a solution for clearing the textfield.
Comment 82 Mike Beltzner [:beltzner, not reading bugmail] 2006-06-28 19:00:20 PDT
(In reply to comment #81)
> What about potential attacks involving drag/drop or pasting into the textfield?

Good questions. I'd say that both actions should load a file chooser, pre-populated with drag content if the chooser supports that sort of thing; the user still needs to click the "OK" button, so I think we're OK in terms of them being clear that they're actually pointing to a local file. Does that seem OK?

> Also, we still need a solution for clearing the textfield.

Delete in context menu?
Comment 83 Jeff Walden [:Waldo] (remove +bmo to email) 2006-06-28 19:46:52 PDT
(In reply to comment #82)
> Delete in context menu?

I conjecture that at best this is not good enough for Mac users with their crazy single-button, no-right-click mice, but I could be wrong, having never owned or really used a Mac.
Comment 84 Mike Beltzner [:beltzner, not reading bugmail] 2006-06-28 20:30:37 PDT
(In reply to comment #83)
> I conjecture that at best this is not good enough for Mac users with their
> crazy single-button, no-right-click mice, but I could be wrong, having never
> owned or really used a Mac.

Mac users know where their context menus are, yeah. I don't think that'll be a large issue. 
Comment 85 Mike Beltzner [:beltzner, not reading bugmail] 2006-07-06 09:46:12 PDT
This doesn't look like it will make beta1, and I fear will be considered too risky for beta2, but am moving the TM to that target.
Comment 86 Aaron Leventhal 2006-07-19 08:03:20 PDT
I object to this change. It is a usability problem for people with disabilities.

Normal file inputs that are visible should be tabbable by the user and focusable by screen readers. Turning off the ability for the user to explicitly focus a normal file input completely is a bit extreme.

I'm not sure how this major focus change to form controls got in without having been run by myself or Mark Pilgrim. That's what we're in the project to prevent.

I've filed bug 345195 to reverse this or fix it so focusability is only removed in the cases you are concerned about.
Comment 87 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2006-07-19 09:59:27 PDT
You can tab to the button and activate the platform file chooser dialog, which must be accessible of course. You just can't tab to the textbox. So why is this such a big deal?

Anyway, given comment #80, this will be modified to make the textbox focusable before we ship anything.
Comment 88 Aaron Leventhal 2006-07-19 10:13:40 PDT
(In reply to comment #87)
> You can tab to the button and activate the platform file chooser dialog, which
> must be accessible of course. You just can't tab to the textbox. So why is this
> such a big deal?

For one thing, tabbing to something is how users use copy and paste functions. How do you copy the filename unless you can tab to it?

This new feature is just going to be annoying to everyone. I've already been thanked on IRC by someone for filing the bug. Can't we check the style rules to see if anything strange is being done to the file control before activating this safeguard?
Comment 89 Jonas Sicking (:sicking) No longer reading bugmail consistently 2006-07-19 11:08:46 PDT
> For one thing, tabbing to something is how users use copy and paste functions.
> How do you copy the filename unless you can tab to it?

You can still tab to the filecontrol, you just can't type in it. So presumably if you want to copy the filename you can just focus it and then copy, if not we should fix that.

However I'm not sure why you'd want to copy the filename. I can't think of a single time where i've ever done that. Especially given that you'll be the one that has selected that filename in the first place.

> This new feature is just going to be annoying to everyone. I've already been
> thanked on IRC by someone for filing the bug. Can't we check the style rules 
> to see if anything strange is being done to the file control before activating
> this safeguard?

There are a million ways to obscure the filecontrol. Simply positioning other elements over it works just as well and is impossible to detect without doing some sort of analysis of the resulting rendering and using AI to detect when the filecontrol is "visible enough".
Comment 90 neil@parkwaycc.co.uk 2006-07-19 11:52:54 PDT
(In reply to comment #89)
>>How do you copy the filename unless you can tab to it?
>You can still tab to the filecontrol
I think he means that you can't tab to the file name to copy it...
Comment 91 Aaron Leventhal 2006-07-19 13:20:23 PDT
I'd say we're pretty good at knowing whether something is the topmost visible thing in a certain position. We do it for mouseovers all the time. If it's obscured at all, then make it so the user can't type in it.
Comment 92 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2006-07-19 14:16:47 PDT
(In reply to comment #91)
> I'd say we're pretty good at knowing whether something is the topmost visible
> thing in a certain position. We do it for mouseovers all the time. If it's
> obscured at all, then make it so the user can't type in it.

What if someone puts it inside a container element that clips off the borders and Browse button, and then places that on top of other content that provides the spoofing context?

If you say "don't allow typing when any part of the control is not visible", what if the control is partially scrolled out of the window or other scrollable container? Should we suddenly disable typing if the control gets partially scrolled out of view? It gets really complex.

Anyway, this is not relevant. We'll go with comment #80. It won't make Firefox 2 though.
Comment 93 Felix Miata 2006-07-19 14:54:11 PDT
1-to expand on comment 48, my shareware OS/2 filepicker has its own (short) file history. I don't want to pollute that history with the one character edits I commonly make uploading to the W3C HTML validator.

2-This protection ought to be disabled when I: View -> Use Style -> None.
Comment 94 Mike Beltzner [:beltzner, not reading bugmail] 2006-08-15 12:21:04 PDT
Minusing, can't take this sort of change at this point.
Comment 95 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2006-08-17 15:17:36 PDT
Would be better to file followup bugs on the other issues that need to be fixed, and nominate them for 1.9.
Comment 96 Mike Schroepfer 2006-11-01 10:30:57 PST
Not clear this is ready for the 1.8.1.x branch.  If a solution is ready and baked for that branch please re-nom.
Comment 97 Biju 2007-03-14 22:02:16 PDT
-

Fix for this is very much irritating...
Please allow direct input of filename into file upload controls (Bug 374011)


I had to switch from Minefield to BonEcho before uploading few photos.
Comment 98 Biju 2007-03-14 22:49:59 PDT
Also allow direct input due to ACCESSIBILITY reason.
For blind or less mobility people using mouse and scrolling is a nightmare.
Please just not blindly follow Safari, Apples philosophy is to make everybody to use only mouse and icons.
Comment 99 Aaron Leventhal 2007-03-15 06:35:43 PDT
Biju, we need to do the advocacy in the newsgroups. I think mozilla.dev.security might be the right one.
Comment 100 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2007-07-15 20:34:49 PDT
Created attachment 272444 [details] [diff] [review]
fix

This is a fix following the idea in comment #80.

The patch does a few things:
-- backs out the mouse listener on the text input that I added before
-- removes mTextFrame because it was used unsafely, and keeping arbitrary frame pointers around is always a bad idea
-- makes GetTextControlFrame sane
-- removes mCachedState because as far as I can tell, it is never needed; creation of frames for the anonymous children happens nearly immediately, before any calls to SetFrameProperty can happen
-- adds a key listener to the text input that detects character input events, suppresses them, and opens the file chooser dialog instead

I like this solution quite a lot. The only way I can see spoofing happening is if the attacker gets the user to paste or drag-and-drop something into the text input. If that is considered a problem, we can do a followup patch where we disable pasting and drag-and-drop into the text input (probably by adding an API to the text control frame).
Comment 101 Jesse Ruderman 2007-07-20 21:25:26 PDT
I think both the current trunk behavior (clicking the textbox launches a file picker) and roc's behavior (typing/dragging/pasting into the textbox launches a file picker) are likely to be confusing.  I think we should follow Safari and make the filename look non-editable and non-focusable.
Comment 102 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2007-07-20 23:47:02 PDT
That approach was already rejected. I've implemented the best consensus we have.
Comment 103 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2007-07-20 23:59:21 PDT
(I actually agree with you, Jesse; but we can't oscillate forever based on who spoke loudest last.)
Comment 104 Jesse Ruderman 2007-07-21 00:03:37 PDT
Hmm, it was (comment 80 and bug 314365 comment 8).  It's not clear why, though.
Comment 105 Tim Keenan 2007-07-23 12:20:36 PDT
Not allowing the control to be focused would make the control totally unusable by people using assistive technology, such as screen readers, magnifiers and on-screen keyboards.  Has this fix based on comment 80 been checked into the trunk?  If so I can play wit it using a few types of access technology to ensure that it's usable.  It sounds like it should be, though.

In general, as someone already said, doing things the way they're done in Safari is likely to result in  a much less accessible piece of software, which is not our goal.
Comment 106 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2007-07-23 20:12:30 PDT
The current trunk code --- which does not allow the text input control to be focused --- should be accessible: you can still focus the button, and open the platform file chooser, which then should be accessible.
Comment 107 Mats Palmgren (:mats) 2007-07-24 14:22:45 PDT
Comment on attachment 272444 [details] [diff] [review]
fix

> nsFileControlFrame::CreateAnonymousContent(nsTArray<nsIContent*>& aElements)
> ...
>+  nsCOMPtr<nsIDOMHTMLInputElement> fileControl = do_QueryInterface(mContent);
>+  if (fileControl) {
>+    PRInt32 tabIndex;
>+    fileControl->GetTabIndex(&tabIndex);
>+
>+    nsCOMPtr<nsIDOMHTMLInputElement> browseControl = do_QueryInterface(mBrowse);
>+    if (browseControl) {
>+      nsAutoString accessKey;
>+      fileControl->GetAccessKey(accessKey);
>+      browseControl->SetAccessKey(accessKey);    
>+      browseControl->SetTabIndex(tabIndex);

It looks like we only propagate 'accesskey' and 'tabindex' once...
sync these attrs in AttributeChanged()?


> nsFileControlFrame::SetFocus(PRBool aOn, PRBool aRepaint)
>+  // When aOn is false, we should just ignore this; the file control never
>+  // has focus normally so aOn == PR_FALSE happens only when we transfer focus
>+  // away ourselves in the SetFocus call below.
>+  if (mBrowse && aOn) {
>+    mBrowse->SetFocus(PresContext());

The comment is misleading - an explicit file.blur() will reach this
method with aOn=false.  Actually, I don't think we can get here with
aOn=true at all because a file input still isn't focusable.
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/content/html/content/src/nsHTMLInputElement.cpp&rev=1.462&root=/cvsroot&mark=2801,2806#2790

Is there a reason we shouldn't allow file.focus() to work?


>+  // Reacquire frames
>+  nsIFrame* f = presContext->PresShell()->GetPrimaryFrameFor(content);

Maybe do a "Flush_Layout" before GetPrimaryFrameFor?


> NS_IMETHODIMP nsFileControlFrame::Reflow
> ...
>+  nsTextControlFrame* textControlFrame = GetTextControlFrame();
>+  NS_ENSURE_TRUE(textControlFrame, NS_ERROR_UNEXPECTED);
> 
>   // The Areaframe takes care of all our reflow 
>   // except for when style is used to change its size.
>   // XXXbz do we care?  This setup just needs to die....  Leaving it
>   // in for now just because I don't want to regress things, but....
>   nsresult rv = nsAreaFrame::Reflow(aPresContext, aDesiredSize, aReflowState,
>                                     aStatus);
>-  if (NS_SUCCEEDED(rv) && mTextFrame != nsnull) {
>+  if (NS_SUCCEEDED(rv) && textControlFrame) {

We can't reach this line with textControlFrame == nsnull.


>+nsFileControlFrame::GetTextControlFrame()
>+  nsIFrame* frame = PresContext()->PresShell()->GetPrimaryFrameFor(mTextContent);
>+  if (frame->GetType() != nsGkAtoms::textInputFrame)

"if (frame && frame->GetType() ..."  just in case?
maybe even NS_ERROR("unexpected frame type") in this block


>+nsFileControlFrame::Listener::KeyPress(nsIDOMEvent* aKeyEvent)

Hmm, by passing through events with charCode==0 I can select text using
SHIFT+Left and delete it with DEL or BACKSPACE and paste the clipboard with
SHIFT+Insert (on Linux at least).  This doesn't seem right.


>Index: layout/forms/nsFileControlFrame.h
>+  virtual nsIAtom* GetType() const { return nsGkAtoms::fileInputFrame; }

Nit: I prefer virtual method bodies in the .cpp file.


>+    nsresult HandleKey(nsIDOMEvent* aKeyEvent);

Not implemented/used AFAICT.


>Index: layout/style/forms.css
>-input[type="file"] > input[type="text"][disabled] {
>+input[type="file"] > input[type="text"] {

Did you intend to allow the author to style text input color etc?
(seems like an important change and it wasn't mentioned in comment 100)
Comment 108 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2007-07-24 14:58:40 PDT
(In reply to comment #107)
> It looks like we only propagate 'accesskey' and 'tabindex' once...
> sync these attrs in AttributeChanged()?

We never did that before (see http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/layout/forms/nsFileControlFrame.cpp&rev=3.176), so it's not a regression from 1.8, just an old bug. But I can fix that here if you want.

> > nsFileControlFrame::SetFocus(PRBool aOn, PRBool aRepaint)
> >+  // When aOn is false, we should just ignore this; the file control never
> >+  // has focus normally so aOn == PR_FALSE happens only when we transfer focus
> >+  // away ourselves in the SetFocus call below.
> >+  if (mBrowse && aOn) {
> >+    mBrowse->SetFocus(PresContext());
> 
> The comment is misleading - an explicit file.blur() will reach this
> method with aOn=false.  Actually, I don't think we can get here with
> aOn=true at all because a file input still isn't focusable.
> http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/content/html/content/src/nsHTMLInputElement.cpp&rev=1.462&root=/cvsroot&mark=2801,2806#2790

Good point. I'll just remove that change.

> Is there a reason we shouldn't allow file.focus() to work?

Probably not, but we'd have to disentangle it from the code that detects whether the file input is tabbable --- it still shouldn't be tabbable. I'd rather not mess with that here.

> >+  // Reacquire frames
> >+  nsIFrame* f = presContext->PresShell()->GetPrimaryFrameFor(content);
> 
> Maybe do a "Flush_Layout" before GetPrimaryFrameFor?

Yes, good point.

> > NS_IMETHODIMP nsFileControlFrame::Reflow
> > ...
> >+  nsTextControlFrame* textControlFrame = GetTextControlFrame();
> >+  NS_ENSURE_TRUE(textControlFrame, NS_ERROR_UNEXPECTED);
> > 
> >   // The Areaframe takes care of all our reflow 
> >   // except for when style is used to change its size.
> >   // XXXbz do we care?  This setup just needs to die....  Leaving it
> >   // in for now just because I don't want to regress things, but....
> >   nsresult rv = nsAreaFrame::Reflow(aPresContext, aDesiredSize, aReflowState,
> >                                     aStatus);
> >-  if (NS_SUCCEEDED(rv) && mTextFrame != nsnull) {
> >+  if (NS_SUCCEEDED(rv) && textControlFrame) {
> 
> We can't reach this line with textControlFrame == nsnull.

Good point.

> >+nsFileControlFrame::GetTextControlFrame()
> >+  nsIFrame* frame = PresContext()->PresShell()->GetPrimaryFrameFor(mTextContent);
> >+  if (frame->GetType() != nsGkAtoms::textInputFrame)
> 
> "if (frame && frame->GetType() ..."  just in case?

I think "!frame || frame->GetType() ..."

> maybe even NS_ERROR("unexpected frame type") in this block

OK

> >+nsFileControlFrame::Listener::KeyPress(nsIDOMEvent* aKeyEvent)
> 
> Hmm, by passing through events with charCode==0 I can select text using
> SHIFT+Left and delete it with DEL or BACKSPACE and paste the clipboard with
> SHIFT+Insert (on Linux at least).  This doesn't seem right.

That is intended. I can't see any spoofing potential with those commands and people say that being able to paste is useful.

> >Index: layout/forms/nsFileControlFrame.h
> >+  virtual nsIAtom* GetType() const { return nsGkAtoms::fileInputFrame; }
> 
> Nit: I prefer virtual method bodies in the .cpp file.

It doesn't really make a difference (except this is less code to read, arguably), but OK.

> >+    nsresult HandleKey(nsIDOMEvent* aKeyEvent);
> 
> Not implemented/used AFAICT.

Yeah, I'll remove it.

> >Index: layout/style/forms.css
> >-input[type="file"] > input[type="text"][disabled] {
> >+input[type="file"] > input[type="text"] {
> 
> Did you intend to allow the author to style text input color etc?
> (seems like an important change and it wasn't mentioned in comment 100)

I'm just reverting the change that was made in my previous patch in this bug. But the author could already style text input color, right? Since the rules weren't !important.
Comment 109 Mats Palmgren (:mats) 2007-07-24 15:44:54 PDT
(In reply to comment #108)
> > It looks like we only propagate 'accesskey' and 'tabindex' once...
> > sync these attrs in AttributeChanged()?
> 
> ... just an old bug. But I can fix that here if you want.

Or file a new bug, your call.


> > Is there a reason we shouldn't allow file.focus() to work?
> 
> Probably not, but we'd have to disentangle it from the code that detects
> whether the file input is tabbable --- it still shouldn't be tabbable. I'd
> rather not mess with that here.

Ok.  FWIW, 1.8 branch allows file.focus().


> That is intended. I can't see any spoofing potential with those commands and
> people say that being able to paste is useful.

Ok, but it seems inconsistent that some (less frequently used) editing
commands like SHIFT+Insert works, but the more frequent CTRL+A/C/X/V
triggers the file dialog.


> I'm just reverting the change that was made in my previous patch in this bug.

Ok, I just compared before/after the patch on trunk.
If we want branch compat with respect to styling then it looks correct.
Comment 110 Jesse Ruderman 2007-07-24 16:40:31 PDT
> Ok, but it seems inconsistent that some (less frequently used) editing
> commands like SHIFT+Insert works, but the more frequent CTRL+A/C/X/V
> triggers the file dialog.

"Works" as in actually uploads the file or "works" as in makes it look like it's going to upload the file?
Comment 111 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2007-07-24 16:43:11 PDT
Created attachment 273686 [details] [diff] [review]
updated patch

Okay, here's the patch updated to comments. I did what I just said, except for one thing --- in nsFileControlFrame::SetFocus, I just removed the comment, I left the "if (mBrowse && aOn)" check in since it clearly makes no sense to call SetFocus when aOn is false.
Comment 112 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2007-07-24 18:02:49 PDT
> FWIW, 1.8 branch allows file.focus().

That's strange since the code you pointed to in nsHTMLInputElement predates the 1.8 branch.

> Ok, but it seems inconsistent that some (less frequently used) editing
> commands like SHIFT+Insert works, but the more frequent CTRL+A/C/X/V
> triggers the file dialog.

Oh, that's because control characters have charCode > 0. On Mac this isn't an issue because those shortcuts use the cmd-key and have no charCode... I'll update the patch to test charCode >= 32.

(In reply to comment #110)
> > Ok, but it seems inconsistent that some (less frequently used) editing
> > commands like SHIFT+Insert works, but the more frequent CTRL+A/C/X/V
> > triggers the file dialog.
> 
> "Works" as in actually uploads the file or "works" as in makes it look like
> it's going to upload the file?

Actually uploads the file; nothing has changed with respect to how the edit control interprets keypresses we allow it to receive.

Comment 113 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2007-07-24 18:04:17 PDT
Created attachment 273698 [details] [diff] [review]
updated again
Comment 114 Mats Palmgren (:mats) 2007-07-24 19:32:01 PDT
Comment on attachment 273698 [details] [diff] [review]
updated again

(In reply to comment #110)
> "Works" as in actually uploads the file or "works" as in makes it look like
> it's going to upload the file?

Good catch Jesse, I assumed that what-I-see-is-what-I-submit but this
is not the case.  If I choose <dir>/Makefile using the file dialog,
then select the text Makefile and paste the string README.txt over it,
the text input now contains <dir>/README.txt but it is <dir>/Makefile
that is submitted.  I don't think text input editing is reflected
into the filename value at all, the two values are kept separate:
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/content/html/content/src/nsHTMLInputElement.cpp&rev=1.462&root=/cvsroot&mark=331-345#307
We submit mFileName:
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/content/html/content/src/nsHTMLInputElement.cpp&rev=1.462&root=/cvsroot&mark=2369-2383#2361

Also, CTRL+<key> still triggers the file chooser dialog.
Comment 115 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2007-07-24 19:40:53 PDT
Hmm, did something change on trunk since my first patch landed, then?
Comment 116 Mats Palmgren (:mats) 2007-07-24 19:45:16 PDT
FWIW, this seems to work on Linux:

    if (NS_SUCCEEDED(keyEvent->GetCharCode(&charCode))) {
      PRBool b = PR_FALSE;
      keyEvent->GetCtrlKey(&b);
      if (!b)
        keyEvent->GetAltKey(&b);
      if (!b)
        keyEvent->GetMetaKey(&b);
      if (!b && charCode >= 32) { // control characters are allowed
Comment 117 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2007-07-24 19:57:22 PDT
I see, bug 334977 separated file names from values.

But I don't see how the branch code updates the file name for changes in the text input ... Jonas, how does that work?
Comment 118 Jonas Sicking (:sicking) No longer reading bugmail consistently 2007-07-24 20:20:10 PDT
Looks like in nsHTMLInputElement::HandleDOMEvent at

http://lxr.mozilla.org/mozilla1.8/source/content/html/content/src/nsHTMLInputElement.cpp#1431
Comment 119 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2007-07-24 21:33:20 PDT
Created attachment 273720 [details] [diff] [review]
updated again!

Use Mats' suggestion for modifier keys.

Also added code similar to the branch code for propagating text input changes to mFileName and asking sicking for review there. Note the XXX comment, Jonas (although if there's a bug here, there's probably also a branch regression).

This is slightly unsatisfactory in that we're reopening the possibility that if an attacker can modify the anonymous text input, they can get their file submitted. On the other hand if we're going to support pasting into the text input, that is hard to avoid.
Comment 120 neil@parkwaycc.co.uk 2007-07-25 02:39:49 PDT
(In reply to comment #116)
> FWIW, this seems to work on Linux:
> 
>     if (NS_SUCCEEDED(keyEvent->GetCharCode(&charCode))) {
>       PRBool b = PR_FALSE;
>       keyEvent->GetCtrlKey(&b);
>       if (!b)
>         keyEvent->GetAltKey(&b);
>       if (!b)
>         keyEvent->GetMetaKey(&b);
>       if (!b && charCode >= 32) { // control characters are allowed

nsPlaintextEditor::HandleKeyPress seems to use
if (character && !altKey && !ctrlKey && !metaKey)
i.e. it assumes ctrlKey will be set for charCode < 32
Comment 121 Mats Palmgren (:mats) 2007-07-26 19:58:50 PDT
(In reply to comment #120)
> nsPlaintextEditor::HandleKeyPress seems to use
> if (character && !altKey && !ctrlKey && !metaKey)

Yes, this code is better...

> i.e. it assumes ctrlKey will be set for charCode < 32

No, that is a misunderstanding.
ctrlKey is true if the Control key on the keyboard was pressed when
the event occurred.  It has nothing to do with "control character",
which generates key press events with charCode==0 and keyCode==<key>,
but ctrlKey works the same for them, e.g. a key press event for
TAB      has charCode=0   keyCode=9 ctrlKey=0
CTRL+TAB has charCode=0   keyCode=9 ctrlKey=1
CTRL+V   has charCode=118 keyCode=0 ctrlKey=1

(In reply to comment #112)
> On Mac this isn't an issue because those shortcuts use the
> cmd-key and have no charCode...

No, the reason you didn't have a problem with Cmd+V etc is that those
events never reach nsFileControlFrame::Listener::KeyPress() at all.
If they did I would expect them to have charCode='v' and metaKey=1.
Comment 122 Mats Palmgren (:mats) 2007-07-26 20:00:32 PDT
Comment on attachment 273720 [details] [diff] [review]
updated again!

> // Block key press events that are producing actual text. We allow key events
> // charcode 0 because text editing, including copying and pasting, is
> // explicitly allowed.

The second sentence is slightly wrong, I think you can remove it.
The first sentence can be moved down/merged with:

> // Block this input. Fire up the dialog instead.
For example:
// Block key press events that are producing actual text,
// fire up the file chooser dialog instead.

sr=mats  (if you improve the comments in some way)
Comment 123 neil@parkwaycc.co.uk 2007-07-27 01:11:22 PDT
(In reply to comment #121)
>> i.e. it assumes ctrlKey will be set for charCode < 32
>No, that is a misunderstanding.
Sorry, I was thinking of all the hacks the windows widget code goes through to turn control characters (1-31) back into ctrlKey + "orginal" character.
Comment 124 Boris Zbarsky [:bz] 2007-09-25 14:54:13 PDT
I don't see that patch checking for trusted input events.  Couldn't a malicious page use synthesized input events to trigger this code?
Comment 125 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2007-09-25 15:58:08 PDT
I don't think we checked for trusted input events in this code in 1.8. Why would we need to now? Aren't we relying on the native-anonymity of the text field to stop people targeting it?
Comment 126 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2007-09-25 16:34:35 PDT
Created attachment 282340 [details] [diff] [review]
updated yet again

Okay, for defense in depth I added a trusted-ness check. This is also updated to trunk since it had rotted quite a bit. Jonas, please review it this time!
Comment 127 Boris Zbarsky [:bz] 2007-09-25 17:37:51 PDT
> Aren't we relying on the native-anonymity of the text field

You never check that the target is the native anonymous text field.  You just check that it's not |this|.  It could be XBL-bound anonymous content with the XBL binding (under the control of the site) sending the event.

Though I guess it's hard to get a "textInputFileName" property on the frame in that case.  But it still seems safer to check for trustedness.

As for 1.8....  It might be buggy!

And I was talking about the PreHandleEvent code in nsHTMLInputElement, not the Listener::KeyPress code.  Though checking trustedness in the latter is also a good idea.
Comment 128 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2007-09-25 18:52:53 PDT
> And I was talking about the PreHandleEvent code in nsHTMLInputElement, not the
> Listener::KeyPress code.

The PreHandleEvent code is safe. We're only using the event as a trigger to synchronize the element's filename with the frame's filename. It doesn't matter who sends those events, they have no control over the actual value of the file name.
Comment 129 Boris Zbarsky [:bz] 2007-09-25 19:04:46 PDT
It matters if the attacker can somehow affect the frame's filename.  But I guess in that case we're in trouble anyway.
Comment 130 Jonas Sicking (:sicking) No longer reading bugmail consistently 2007-09-25 19:21:30 PDT
I'm not really happy with making this UI change, for a couple of reasons.

1. I much prefer the safari-style UI of not having an input field there at all.
   A very small percentage of our users enter file paths. But everyone suffers
   from its uglyness and quirky behavior.
2. As soon as we have the textfield there and enable any sort of input we
   increase the risk for stealing files.
   From a code-wise point of view we have to work much harder on preventing the
   web author from being able to modify the textfield through bugs in our code
   (this has happened numerous times before).
   From a users point of view it increases the risk that the user can be tricked
   into modifying the textfield. Though admittedly with the current patch
   very slightly.

That said, I'm clearly way to late to the party. But I wanted to register a complaint ;)
Comment 131 Jonas Sicking (:sicking) No longer reading bugmail consistently 2007-09-25 19:25:11 PDT
Comment on attachment 282340 [details] [diff] [review]
updated yet again

Looks good, but you do want to call SetValueChanged.

r=me with that fixed.
Comment 132 Boris Zbarsky [:bz] 2007-09-25 19:34:39 PDT
For what it's worth, I'd like to enthusiastically second what sicking says.  And I'm one of those users who types in the file upload field.  But keeping it secure is just a major pain, historically...
Comment 133 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2007-09-25 19:52:07 PDT
I hear you.

I would be happy with the state of trunk, or a Safari-ish solution. I do not care about this bug.

I will land this patch, which satisfies the request in comment #80. I will do a followup patch to disable pasting into the text field, to address bug 57770.

Then anyone who still cares passionately about this issue can file a followup bug and own it.
Comment 134 Jonas Sicking (:sicking) No longer reading bugmail consistently 2007-10-15 15:25:43 PDT
I grabbed beltzner as he walked by my cube and had a quick conversation with him about this bug. Our conclusion was:

1. The important part is that a file picker is not popped up if you tab through
   a form containing an <input type=file>
2. Ideal would be if we on pasting and dropping opened the file picker with the
   pasted/dropped filename chosen

Since 1 is already taken care of by focusing the "Browse..." button rather than the text field, we should simply close this bug as FIXED and file a new bug on 2. As not blocking the FF3 release.
Comment 135 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2007-10-15 15:39:26 PDT
alright, we're done here. Direct any remaining complaints to beltzner!
Comment 136 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2007-10-15 15:43:14 PDT
Filed bug 399917 on item 2.
Comment 137 neil@parkwaycc.co.uk 2007-10-15 16:40:33 PDT
After all that, my comment #45 never did get addressed...
Comment 138 Jonas Sicking (:sicking) No longer reading bugmail consistently 2007-10-15 18:21:00 PDT
Please file a new bug on that and mark as blocking? if you think it's bad enough. This bug has gotten big enough that stuff gets too easily lost.
Comment 139 Emanuele Ruffaldi 2007-11-12 00:52:20 PST
I am the writer of the DragDropUpload extension

http://www.teslacore.it/wiki/index.php?title=DragDropUpload
https://addons.mozilla.org/en-US/firefox/addon/2190

is there any way to re-enable the text box from an extension?
Comment 140 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2007-11-12 01:31:43 PST
Not really, no. Maybe there should be a way for privileged code to set the file name in the control. Someone would have to write a patch.
Comment 141 Boris Zbarsky [:bz] 2007-11-12 09:05:57 PST
Privileged code can just set input.value, no?
Comment 142 Emanuele Ruffaldi 2008-02-29 06:40:17 PST
Thanks for everybody, now the extension works with FF3.0b3. I am fixing the issue of the gray box drop event.
Comment 143 Wesley Johnston (:wesj) 2008-04-09 15:20:20 PDT
Was this change ever documented on DevMo? I can't find anything detailing exactly what is and isn't allowed anymore.
Comment 144 Jonas Sicking (:sicking) No longer reading bugmail consistently 2008-04-09 17:32:02 PDT
So there we're really any thing that affects developers. Only users should notice this change. Don't know where we document such things? I guess we could put something in the docs for the <input> element saying that the UI is different but that it shouldn't affect developers?
Comment 145 Wesley Johnston (:wesj) 2008-04-09 17:52:08 PDT
I've seen two threads on Mozillazine asking about what had been changed:

http://forums.mozillazine.org/viewtopic.php?t=647059
http://forums.mozillazine.org/viewtopic.php?p=3329026&sid=0a9e40a1cac837edf36851ae7cc6823e

Not sure if their complaints are "bugs" or not, but I just thought it might be nice to have some documentation they can turn to.
Comment 146 Jonas Sicking (:sicking) No longer reading bugmail consistently 2008-04-09 18:14:26 PDT
Neither of those posts are about this bug.

The first one is bug 143220. I added a dev-doc-needed request to that bug.

The second is not a change, it's a bug that has always been there. See 36619.
Comment 147 Neil Deakin 2008-05-22 08:01:18 PDT
roc, was this latest patch (https://bugzilla.mozilla.org/attachment.cgi?id=282340) meant to be checked in?
Comment 148 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2008-05-22 15:28:34 PDT
No. See comment #134.
Comment 149 davebad 2012-07-27 10:40:20 PDT
I don't think the accepted 'fix' for this issue is adequate. It requires too much real estate for users of your site to see the entire file name. That is often not possible.

Since you cannot click into the box without opening a dialog box, you cannot move the cursor, therefore prohibiting a user from seeing the entire file path
when the size of the box is narrower than the path.

Users of Firefox report that the web site is broken. I believe this should be taken up again.

If this fix is deemed acceptable by the powers that be, then maybe only the actual file name and not the entire path should show in the box.

Newest post here is older than me, so off to the requests section.
Comment 150 Cesar Eduardo Barros 2012-07-27 15:39:06 PDT
(In reply to davebad from comment #149)
> I don't think the accepted 'fix' for this issue is adequate. It requires too
> much real estate for users of your site to see the entire file name. That is
> often not possible.
> 
> Since you cannot click into the box without opening a dialog box, you cannot
> move the cursor, therefore prohibiting a user from seeing the entire file
> path
> when the size of the box is narrower than the path.
> 
> If this fix is deemed acceptable by the powers that be, then maybe only the
> actual file name and not the entire path should show in the box.

How about showing the right part of the path instead of the left part then? That is, show the path as if the user had moved the cursor to its end. This is better than showing just the file name, since it shows the file name plus as much of the path as will fit.

Please file a new bug to request that however (and add a link to the new bug here as a courtesy), since this bug is already finished.

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