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:

This page simply demonstrates the behavior. It does not attempt to demonstrate an 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.

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.

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.

(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.

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.

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.

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.

> 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.

"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

> 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.

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.

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)...

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.

(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.

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.

This version makes the text field clickable, so when you click on it we pop up the file dialog.

Comment on attachment 197487 [details] [diff] [review] fix #2 r+sr=dbaron, although I'm not happy about this.

checked in. Let the flaming commence!

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 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 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.

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?

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.

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.

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.

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.

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)

(In reply to comment #47) >I'll try to get it working on trunk

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?

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.

(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.

That's a good point. Perhaps 'cancel' in the filepicker dialog should clear the field?

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?

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.

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.

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

*** Bug 325717 has been marked as a duplicate of this bug. ***

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.

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

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.

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

*** 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.

Plussing for 1.8.1, but we probably want something with slightly different UI from the current patch.

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.

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.

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.

(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?

> 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".

(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...

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.

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.

Would be better to file followup bugs on the other issues that need to be fixed, and nominate them for 1.9.

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.

- 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.

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.

Biju, we need to do the advocacy in the newsgroups. I think mozilla.dev.security might be the right one.

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).

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.)

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

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)

(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.

> 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?

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.

> 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 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.

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

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.

(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)

(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?

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!

> 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.

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.

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!

Filed bug 399917 on item 2.

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.

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?

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.

roc, was this latest patch (https://bugzilla.mozilla.org/attachment.cgi?id=282340) meant to be checked in?

No. See comment #134.

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.