Disable direct input of filename into file upload controls

RESOLVED FIXED in mozilla1.9alpha8

Status

()

Core
Layout: Form Controls
--
critical
RESOLVED FIXED
13 years ago
4 years ago

People

(Reporter: Chris Cook, Assigned: roc)

Tracking

(Depends on: 1 bug, 5 keywords)

Trunk
mozilla1.9alpha8
csectype-spoof, css-moz, css3, sec-high, testcase
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.8b5 -
blocking1.9 +
blocking1.8.1 -
blocking1.8.1.1 -
wanted1.8.1.x -

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: See bug 56236 and bug 57770 for more exploits, URL)

Attachments

(4 attachments, 5 obsolete attachments)

(Reporter)

Description

13 years ago
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:
(Reporter)

Comment 1

13 years ago
Created attachment 158546 [details]
Basic test page

This page simply demonstrates the behavior. It does not attempt to demonstrate
an exploit.
(Reporter)

Comment 2

13 years ago
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.
Confirming.
Status: UNCONFIRMED → NEW
Ever confirmed: true
(Reporter)

Comment 4

13 years ago
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.
Keywords: css-moz, css3, testcase
(Reporter)

Comment 5

13 years ago
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.
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.
(Reporter)

Comment 7

13 years ago
(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
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.

Updated

12 years ago
Summary: CSS "opacity" affects file input elements → File upload control can be obscured (e.g. using low opacity)

Comment 9

12 years ago
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.
Group: security
Flags: blocking1.8b4?
Whiteboard: [sg:fix]
Assignee: dbaron → nobody
Component: Style System (CSS) → Layout: Form Controls
QA Contact: ian → layout.form-controls
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?
Safari does the second option.
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.
Assignee: nobody → roc
Status: NEW → ASSIGNED
Attachment #191442 - Flags: superreview?(dbaron)
Attachment #191442 - Flags: review?

Updated

12 years ago
Flags: blocking1.8b4? → blocking-aviary1.5+

Updated

12 years ago
Flags: blocking-aviary1.5+ → blocking-aviary1.5?

Comment 13

12 years ago
> 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?").
1) and 2) are pretty similar as far as the user is concerned.
(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. 
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.
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).
(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.
> 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

12 years ago
"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.
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 :)
(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).
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).
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
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.
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

12 years ago
> 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.
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

12 years ago
Btw, to paste a filename into the Mac file picker, press Cmd+Shift+G.
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?
(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.

Updated

12 years ago
Attachment #191442 - Flags: review? → review?(dbaron)

Updated

12 years ago
Flags: blocking-aviary1.5? → blocking1.8b4+

Comment 32

12 years ago
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

12 years ago
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.
(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

12 years ago
(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.
That is a platform problem, not our problem, and we're going to disable the text
box as indicated in comment #28.

Updated

12 years ago
Whiteboard: [sg:fix] → [sg:fix][needs review+SR dbaron]

Comment 37

12 years ago
Konqueror has an nice solution by simply letting the user know what files will
be uploaded when the form (and files) are submitted/uploaded.
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.

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.

Updated

12 years ago
Flags: blocking1.8b5+ → blocking1.8b5-
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.
Attachment #191442 - Attachment is obsolete: true
Attachment #197487 - Flags: superreview?(dbaron)
Attachment #197487 - Flags: review?(dbaron)
Attachment #191442 - Flags: superreview?(dbaron)
Attachment #191442 - Flags: review?(dbaron)
Comment on attachment 197487 [details] [diff] [review]
fix #2

r+sr=dbaron, although I'm not happy about this.
Attachment #197487 - Flags: superreview?(dbaron)
Attachment #197487 - Flags: superreview+
Attachment #197487 - Flags: review?(dbaron)
Attachment #197487 - Flags: review+
checked in. Let the flaming commence!
Status: ASSIGNED → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED
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.
Attachment #197487 - Flags: approval1.8b5?

Comment 44

12 years ago
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

12 years ago
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

12 years ago
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?
Attachment #197487 - Flags: approval1.8b5?
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

12 years ago
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

12 years ago
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

12 years ago
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.

Updated

12 years ago
Depends on: 314365

Updated

12 years ago
Depends on: 314367

Comment 51

12 years ago
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)
No longer depends on: 314365, 314367

Updated

12 years ago
Flags: blocking-aviary2?

Comment 52

12 years ago
(In reply to comment #47)
>I'll try to get it working on trunk
Status: RESOLVED → REOPENED
Resolution: FIXED → ---

Updated

12 years ago
Whiteboard: [sg:fix][needs review+SR dbaron] → [sg:high][needs review+SR dbaron] See bug 56236 and bug 57770 for more exploits
Whiteboard: [sg:high][needs review+SR dbaron] See bug 56236 and bug 57770 for more exploits → [sg:high] See bug 56236 and bug 57770 for more exploits
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

12 years ago
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

12 years ago
(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)
Sure, I'll do that.
middle-clicking the file control on linux doesn't do anything (other than put a non-blinking cursor in the field)
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.

Updated

12 years ago
Flags: blocking-firefox2? → blocking1.8.1?
That's a good point. Perhaps 'cancel' in the filepicker dialog should clear the field?

Comment 60

12 years ago
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... ].
How about, clicking on the textfield clears the file name before launching the filepicker?

Comment 62

12 years ago
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.
That doesn't sound like something anyone actually does. I think I'll try the click-to-clear.

Comment 64

12 years ago
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

12 years ago
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.
(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)
A little further research shows this would probably be fairly easy to do.  Here are the relevant files:

SeaMonkey:
http://lxr.mozilla.org/mozilla1.8/source/xpfe/communicator/resources/content/contentAreaContextOverlay.xul#53
http://lxr.mozilla.org/seamonkey/source/xpfe/communicator/resources/content/nsContextMenu.js
http://lxr.mozilla.org/seamonkey/source/mailnews/base/resources/content/mailWindowOverlay.xul#743

Firefox:
http://lxr.mozilla.org/seamonkey/source/browser/base/content/browser-context.inc#39
http://lxr.mozilla.org/seamonkey/source/browser/base/content/browser.js#4074

Thunderbird:
http://lxr.mozilla.org/seamonkey/source/mail/base/content/nsContextMenu.js
http://lxr.mozilla.org/seamonkey/source/mail/base/content/mailWindowOverlay.xul#761
Blocks: 314365
*** Bug 325717 has been marked as a duplicate of this bug. ***

Comment 69

12 years ago
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

12 years ago
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

12 years ago
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

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

Comment 73

11 years ago
*** Bug 56236 has been marked as a duplicate of this bug. ***
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.
Flags: blocking1.9a2?
Flags: blocking1.8.1? → blocking1.8.1+
Plussing for 1.8.1, but we probably want something with slightly different UI from the current patch.

Comment 76

11 years ago
If this is going to make FF2 beta1, then it needs to happen ASAP.
I don't really have the cycles to get this into b1, since I'm helping brettw get his inline spellchecking stuff done.
*** Bug 343047 has been marked as a duplicate of this bug. ***
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.
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.
What about potential attacks involving drag/drop or pasting into the textfield?

Also, we still need a solution for clearing the textfield.
(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?
(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.
(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. 
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.
Target Milestone: --- → mozilla1.8.1beta2

Comment 86

11 years ago
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.
Flags: blocking1.8.1+ → blocking1.8.1?
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

11 years ago
(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?
Depends on: 345195
Flags: blocking1.8.1? → blocking1.8.1+
> 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

11 years ago
(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

11 years ago
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.
(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.
Target Milestone: mozilla1.8.1beta2 → mozilla1.9beta

Comment 93

11 years ago
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.
Minusing, can't take this sort of change at this point.
Flags: blocking1.8.1+ → blocking1.8.1-

Updated

11 years ago
Flags: blocking1.9a2? → blocking1.9+
Would be better to file followup bugs on the other issues that need to be fixed, and nominate them for 1.9.

Updated

11 years ago
Flags: blocking1.8.1.1?

Updated

11 years ago
Depends on: 355362

Comment 96

11 years ago
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.
Flags: blocking1.8.1.1? → blocking1.8.1.1-

Updated

11 years ago
Summary: File upload control can be obscured (e.g. using low opacity) → Disable direct input of filename into file upload controls

Updated

10 years ago
Depends on: 374011

Comment 97

10 years ago
-

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

10 years ago
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

10 years ago
Biju, we need to do the advocacy in the newsgroups. I think mozilla.dev.security might be the right one.
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).
Attachment #272444 - Flags: superreview?(mats.palmgren)
Attachment #272444 - Flags: review?(mats.palmgren)

Comment 101

10 years ago
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.
That approach was already rejected. I've implemented the best consensus we have.
(I actually agree with you, Jesse; but we can't oscillate forever based on who spoke loudest last.)

Comment 104

10 years ago
Hmm, it was (comment 80 and bug 314365 comment 8).  It's not clear why, though.

Comment 105

10 years ago
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.
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 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)
Attachment #272444 - Flags: superreview?(mats.palmgren)
Attachment #272444 - Flags: superreview-
Attachment #272444 - Flags: review?(mats.palmgren)
Attachment #272444 - Flags: review-
(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.
(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

10 years ago
> 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?
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.
Attachment #272444 - Attachment is obsolete: true
Attachment #273686 - Flags: superreview?(mats.palmgren)
Attachment #273686 - Flags: review?(mats.palmgren)
> 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.

Created attachment 273698 [details] [diff] [review]
updated again
Attachment #273686 - Attachment is obsolete: true
Attachment #273698 - Flags: superreview?(mats.palmgren)
Attachment #273698 - Flags: review?(mats.palmgren)
Attachment #273686 - Flags: superreview?(mats.palmgren)
Attachment #273686 - Flags: review?(mats.palmgren)
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.
Attachment #273698 - Flags: superreview?(mats.palmgren)
Attachment #273698 - Flags: superreview-
Attachment #273698 - Flags: review?(mats.palmgren)
Attachment #273698 - Flags: review-
Hmm, did something change on trunk since my first patch landed, then?
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
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?
Looks like in nsHTMLInputElement::HandleDOMEvent at

http://lxr.mozilla.org/mozilla1.8/source/content/html/content/src/nsHTMLInputElement.cpp#1431
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.
Attachment #273698 - Attachment is obsolete: true
Attachment #273720 - Flags: superreview?(mats.palmgren)
Attachment #273720 - Flags: review?(jonas)
(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
(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 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)
Attachment #273720 - Flags: superreview?(mats.palmgren) → superreview+
(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.
I don't see that patch checking for trusted input events.  Couldn't a malicious page use synthesized input events to trigger this code?
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?
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!
Attachment #273720 - Attachment is obsolete: true
Attachment #282340 - Flags: superreview+
Attachment #282340 - Flags: review?
Attachment #273720 - Flags: review?(jonas)
Attachment #282340 - Flags: review? → review?(jonas)
> 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.
> 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.
It matters if the attacker can somehow affect the frame's filename.  But I guess in that case we're in trouble anyway.
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 on attachment 282340 [details] [diff] [review]
updated yet again

Looks good, but you do want to call SetValueChanged.

r=me with that fixed.
Attachment #282340 - Flags: review?(jonas) → review+
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...
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.
Whiteboard: [sg:high] See bug 56236 and bug 57770 for more exploits → [sg:high] See bug 56236 and bug 57770 for more exploits [need feedback]
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.
alright, we're done here. Direct any remaining complaints to beltzner!
Status: REOPENED → RESOLVED
Last Resolved: 12 years ago10 years ago
Resolution: --- → FIXED
Filed bug 399917 on item 2.
Whiteboard: [sg:high] See bug 56236 and bug 57770 for more exploits [need feedback] → [sg:high] See bug 56236 and bug 57770 for more exploits
After all that, my comment #45 never did get addressed...
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.

Updated

10 years ago
Depends on: 399968

Comment 139

10 years ago
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?
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.
Privileged code can just set input.value, no?
Flags: wanted1.8.1.x-

Updated

10 years ago
Depends on: 420251

Comment 142

10 years ago
Thanks for everybody, now the extension works with FF3.0b3. I am fixing the issue of the gray box drop event.
Was this change ever documented on DevMo? I can't find anything detailing exactly what is and isn't allowed anymore.
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?
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.
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.
Depends on: 431098
roc, was this latest patch (https://bugzilla.mozilla.org/attachment.cgi?id=282340) meant to be checked in?
No. See comment #134.

Updated

9 years ago
Depends on: 314367
Depends on: 469280

Comment 149

5 years ago
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.
(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.

Updated

4 years ago
Keywords: csec-spoof, sec-high
Whiteboard: [sg:high] See bug 56236 and bug 57770 for more exploits → See bug 56236 and bug 57770 for more exploits
You need to log in before you can comment on or make changes to this bug.