Bug 47593 – Require a user gesture to open the file dialog

• Component: DOM
• Status: RESOLVED FIXED

[Johnny(Jianning) Ding]

<Input type=file> is represented as a file upload control in WebKit, it is a sensitive control since it can access users' local file(s). 
So ideally browser should only allow doing the file choosing behavior via user gesture, but so far only Firefox does.(Please refer to http://mxr.mozilla.org/mozilla/source/content/html/content/src/nsHTMLInputElement.cpp#1413)
A chromium bug(http://crbug.com/58319) is related to this bug.

For example, when running the following code, there is no file select dialog only in Firefox, other main-stream browsers show the file dialog.
<input type="file" id="f">
<script>setTimeout("document.getElementById('f').click();", 1);</script>

I suggest requiring a user gesture to open the file dialog in WebKit like Firefox does

To do this, there are two ways.
1) Only direct user gesture event can open the file dialog. The direct user gesture event(key/mouse) means the event must dispatch on the <input type=file>, which can ensure the file dialog is opened by users' true intention.
2) The file dialog can be opened as long as a user gesture happens at that time, which means you can open the file dialog in user gesture event dispatched on other elements, which may be more convenient

For example. when running the following code in WebKit, 
<input type="file" id="f">
<a onclick="document.getElementById('f').click();">click me</a>

In way 1, clicking the text "click me" will not open the file dialog, but the file dialog does open in way 2.

I personally prefer the way 1, since it's more safe.

Any suggestions?

[Adam Barth]

I think either would be fine.  I'd go with the general user gesture approach first, but I don't feel strongly about it.

[Alexey Proskuryakov]

Would that add any safety in the presence of FileReader? A site can read file content from a drop handler.

[Adam Barth]

The Chromium bug is marked security sensitive because it has exploit details.  The issue is that the web site can spam the user with file picker dialogs.

[Alexey Proskuryakov]

> The Chromium bug is marked security sensitive because it has exploit details.

I can't see the Chromium bug.

> The issue is that the web site can spam the user with file picker dialogs.

Is this different from spamming with alerts, confirms or modal dialogs?

[Adam Barth]

> > The issue is that the web site can spam the user with file picker dialogs.
> 
> Is this different from spamming with alerts, confirms or modal dialogs?

Yes.  For two reasons: (1) we have mitigations in place for all of those and (2) the file picker is non-modal.

Requiring a user gesture here matches Firefox as well.  We plan to put in other mitigations at the browser level, but this one seems worth doing at the WebCore level.

[Johnny(Jianning) Ding]

Created attachment 70727 [details]
patch v1

Thanks for comments! 
Patch V1 is ready.

[Abhishek Arya]

Adam, can you please review.

[Adam Barth]

Comment on attachment 70727 [details]
patch v1

View in context: https://bugs.webkit.org/attachment.cgi?id=70727&action=review

> WebCore/rendering/RenderFileUploadControl.cpp:128
> +    if (!(frame() && frame()->loader()->isProcessingUserGesture()))

We usually say !frame() || !frame() ...

[Abhishek Arya]

Thanks a lot Adam for the quick response.

Johnny, i can fix the style nit and commit it for you ? Also, did anyone nominate you for committer access yet ?

[Johnny(Jianning) Ding]

(In reply to comment #9)
> Thanks a lot Adam for the quick response.
> 
> Johnny, i can fix the style nit and commit it for you ? Also, did anyone nominate you for committer access yet ?

Abhishek, please help me fix the nit and land it. Thanks very much for the helps from you and Adam!

So far no-body nominated me for committer yet.
I have 10+ changes on WebKit, you can refer to them from 
http://trac.webkit.org/search?q=jnd%40chromium&noquickjump=1&changeset=on
http://trac.webkit.org/search?q=johnnyding.webkit%40gmail.com&noquickjump=1&changeset=on

[Abhishek Arya]

Committed r69914: <http://trac.webkit.org/changeset/69914>

[Andy Estes]

(In reply to comment #5)
> > > The issue is that the web site can spam the user with file picker dialogs.
> > 
> > Is this different from spamming with alerts, confirms or modal dialogs?
> 
> Requiring a user gesture here matches Firefox as well.  We plan to put in other mitigations at the browser level, but this one seems worth doing at the WebCore level.

Interestingly, this will no longer be true in Firefox 4. They now allow .click() to be called programmatically on input elements (see <https://bugzilla.mozilla.org/show_bug.cgi?id=61098>) without an underlying user gesture. IE also allows this.

I think we should revert this change. It makes us incompatible with other major engines and with at least one major JavaScript framework that I'm aware of (SproutCore), which I think outweighs the benefits of mitigating file chooser spamming.

[Andy Estes]

I will note that from reading the mozilla bug it seems like Firefox has mitigation against creating modal dialogs in a loop, which they apply to file chooser dialogs as well. I'm not sure if we do something similar. This is the type of exploit they wish to avoid (copied from the bug):

myFileControl = doc.getElementById('file');
do {
  myFileControl.click();
} while (!fileContainsDirectionsToSecretVolcanoLair(myFileControl.files[0]));

[Johnny(Jianning) Ding]

(In reply to comment #13)
> I will note that from reading the mozilla bug it seems like Firefox has mitigation against creating modal dialogs in a loop, which they apply to file chooser dialogs as well. I'm not sure if we do something similar. This is the type of exploit they wish to avoid (copied from the bug):
> 
> myFileControl = doc.getElementById('file');
> do {
>   myFileControl.click();
> } while (!fileContainsDirectionsToSecretVolcanoLair(myFileControl.files[0]));

Thanks Andy.

After reading mozilla bug https://bugzilla.mozilla.org/show_bug.cgi?id=61098, I think it is to deal with how to avoid creating endless model dialog like alert, like you said it applies to file chooser dialogs as well in Firefox, but this webkit change is to disallow javascript to open the file dialog without user gesture. See the following test.


<input type=button value="click" onclick="open_file_dialog()" />
<input type=file id="file">
<script>
function open_file_dialog() {
  while (1) {
    document.getElementById("file").click();
  }
}
// try to open a file dialog in load stage.
document.getElementById("file").click();
</script>

In above test, function open_file_dialog opens file dialog in infinite loop.
In Firefox4, when loading the test, there is no file dialog, which is blocked because of no user gesture (we follow it from this webkit bug). When calling the function in user gesture stage, the patch for mozilla bug 61098 can allow only a few file dialogs (model dialog) to be popped up.

I test the following test case in Firefox4 and Safari with enabling popup blocker, they have same behavior. I think we should keep this patch.
Please correct me if I am wrong.

[Andy Estes]

Hi Johnny,

(In reply to comment #14)
> (In reply to comment #13) 
> After reading mozilla bug https://bugzilla.mozilla.org/show_bug.cgi?id=61098, I think it is to deal with how to avoid creating endless model dialog like alert, like you said it applies to file chooser dialogs as well in Firefox, but this webkit change is to disallow javascript to open the file dialog without user gesture.

Sorry, I linked to the wrong bug. The bug I meant to link to was <https://bugzilla.mozilla.org/show_bug.cgi?id=36619>, which tracks adding a click() method to file input elements. <https://bugzilla.mozilla.org/show_bug.cgi?id=61098> is also relevant here, but wasn't what I was basing my analysis on.

Firefox 4 integrates their implementation with their pop-up blocker, which (if enabled) allows programatic clicks to open a file picker after user confirmation. We don't, so such attempts are blocked regardless of the state of our pop-up blocker.

Firefox 4 also allows file pickers even if the programatic click() and the user gesture are separated by a timeout. Take the following test case for example:

<script>
    window.addEventListener('load', function() {
        document.getElementById('the-link').addEventListener('click', function() {
             setTimeout(function() {
                 document.getElementById('the-file-input').click();
             },1000);
        }, false);
    }, false);
</script>
<input id="the-file-input" type="file">
<a id='the-link' href="#">Programatically click the input.</a>

Firefox 4 will display a file picker one second after clicking 'Programatically click the input'; we won't, again regardless of pop-up blocker state.

There are valid reasons for content authors to programmatically send click events to file input elements via setTimeout() and I think we shouldn't break those use cases.

[Andy Estes]

Perhaps timers could remember the user gesture state of the event loop iteration that initiated them, then set the user gesture state accordingly when the timer fires. This would allow the click-via-setTimeout use case without allowing non-gesture-initiated clicks in general. Depending on how careful we want to be we could add protections like only forwarding gestures for timers less than a certain maximum delay and/or only propagating one level deep.

[Johnny(Jianning) Ding]

(In reply to comment #16)
> Perhaps timers could remember the user gesture state of the event loop iteration that initiated them, then set the user gesture state accordingly when the timer fires. This would allow the click-via-setTimeout use case without allowing non-gesture-initiated clicks in general. Depending on how careful we want to be we could add protections like only forwarding gestures for timers less than a certain maximum delay and/or only propagating one level deep.

I see, looks like a feature for gesture management. Make sense to me for supporting it, but we should avoid contents authors to abuse this feature, I will suggest that only a timer can remember the user gesture state and the new timeouts created inside that timeout can not inherit this gesture.

Adam, what do you think about Andy's suggestion?

[Adam Barth]

> Adam, what do you think about Andy's suggestion?

Sounds reasonable.  We might want to check what other browsers do.  The long term solution here is probably to write a spec for all these cases and socialize it with other browser vendors.  Otherwise we're going to be continually chasing our tail w.r.t. what web sites expect us to do.

[Andy Estes]

I'll do a little more research into how Firefox handles the setTimeout() case. I'll also file new bugs for these enhancements.

[Andy Estes]

(In reply to comment #19)
> I'll do a little more research into how Firefox handles the setTimeout() case. I'll also file new bugs for these enhancements.

I filed <https://bugs.webkit.org/show_bug.cgi?id=55102> and <https://bugs.webkit.org/show_bug.cgi?id=55104>.

[Andy Estes]

Johnny, while looking into this I noticed that the test for this patch passes even if I comment out the early return in RenderFileUploadControl::click(). This isn't so great :(

I was thinking about how to make this test more obvious, and I think the right thing to do is to have DumpRenderTree implement the UI delegates for the open panel (webView:runOpenPanelForFileButtonWithResultListener:allowMultipleFiles: and webView:runOpenPanelForFileButtonWithResultListener on the Mac) and have them log a message. Layout test expectations could then be based around the presence/absence of this log message.

That technique could be useful for this test as well as for the tests that will be necessary for the two dependent bugs.

[Andy Estes]

(In reply to comment #21)
> Johnny, while looking into this I noticed that the test for this patch passes even if I comment out the early return in RenderFileUploadControl::click(). This isn't so great :(
> 
> I was thinking about how to make this test more obvious, and I think the right thing to do is to have DumpRenderTree implement the UI delegates for the open panel (webView:runOpenPanelForFileButtonWithResultListener:allowMultipleFiles: and webView:runOpenPanelForFileButtonWithResultListener on the Mac) and have them log a message. Layout test expectations could then be based around the presence/absence of this log message.
> 
> That technique could be useful for this test as well as for the tests that will be necessary for the two dependent bugs.

I filed <https://bugs.webkit.org/show_bug.cgi?id=55110> about this.

[Jon Leighton]

FWIW, this change causes problems with programmatically adding files in QtWebKit. See https://bugs.webkit.org/show_bug.cgi?id=75385.