Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

input.files need not be readonly #1391

Closed
espadrine opened this issue Jun 5, 2016 · 6 comments
Closed

input.files need not be readonly #1391

espadrine opened this issue Jun 5, 2016 · 6 comments
Labels
addition/proposal New features or enhancements

Comments

@espadrine
Copy link

https://html.spec.whatwg.org/multipage/forms.html#the-input-element:dom-fae-form

readonly attribute FileList? files;

Seaching online points to the idea that malicious authors could set input.files to include a file on the user's local file system, leaking their data. However, nowadays, if they had access to a FileList with a local file, they can already read it through FileReader and send it through XHR.

In fact, input.files is already writable on Blink and WebKit, since this commit. WebKit bug. As a result, the issue was studied by WHATWG and Mozilla. There seems to be patches being worked on for Firefox by bz and sicking. I believe there is no reason not to move forward with that change, but welcome discussing it further.

The IDL line in Blink is the following:

attribute FileList? files;

Here is a simple test that goes green if it allows setting input.files: https://output.jsbin.com/yavebam.

espadrine added a commit to espadrine/plugs that referenced this issue Jun 5, 2016
This feature does not work in Gecko, and relies on a non-standard HTML feature,
which I hope will get standardized. More on that here:
<whatwg/html#1391>.
@domenic
Copy link
Member

domenic commented Jun 5, 2016

This seems like a duplicate of https://www.w3.org/Bugs/Public/show_bug.cgi?id=22682? I don't believe anything has changed since Ian's post there; we have several different implementations of different behaviors, neither of which are very good, so it's unclear what we could possibly spec.

@espadrine
Copy link
Author

espadrine commented Jun 6, 2016

Now, there are two implementations, WebKit and Blink. Both Safari and Chrome show green. In both cases, they simply made it non-readonly. All you can do is input.files = someFileList. Ian's concerns about setters and getters that return different types are no longer valid.

I believe the only changes needed to the spec is to remove readonly and add "On setting, if the IDL attribute applies, if the object set to is a FileList, set the current selected files to those defined in the FileList, and ensure that the same object is returned upon getting until the list of selected files changes. Otherwise, throw a TypeError." (I am not a frequent contributor to the spec, which is why I'd love editorial feedback before producing a patch.)

The only reason that Gecko doesn't yet have the functionality is that they made a call to implement a much bigger set of features in the patch before shipping, and unsurprisingly three years later the patch is still unfinished. They are not arguing about the behaviour of input.files = someFileList, however.

(We can always update the standard to include the implementation of the shiny JS Array thing in the future, but the current number of implementations of that in the wild is zero, and Gecko's suggested implementation conflicts with the way the getter is already specced, so I see no reason for it to be specced, and it is not what I am discussing here.)

@domenic
Copy link
Member

domenic commented Jun 6, 2016

I see. Pinging the various Mozilla people who have been involved, @bzbarsky @smaug---- @sicking, do you all think it's a good incremental step to make the setter non-readonly? This would allow you to set it to preexisting FileLists, as in @espadrine's examples, but would not provide a way to create a new FileList, and would not allow setting it to arrays or other iterables.

@domenic domenic added the addition/proposal New features or enhancements label Jun 6, 2016
@sicking
Copy link

sicking commented Jun 7, 2016

I think we should define .files as:

partial interface HTMLInputElement {
  FrozenArray<Blob> files;
};

In the short term I hope that that allows calling the setter with a FileList. In the long term the FileList API should be removed from everywhere and replaced with FrozenArray<Blob>.

I think the only hurdle remaining is that we don't actually need FrozenArray<Blob>, but rather FrozenArrayWithItem<Blob>.

@domenic
Copy link
Member

domenic commented Jun 7, 2016

@sicking I was hoping for an initial consensus on the existing two-browser behavior, instead of continuing to try to come up with an ideal API which will get stuck for another 4 years :-/. After agreeing on removing readonly, we could try pushing for changing the type, but that would require more implementer interest beyond just Mozilla, which I haven't seen in any of the linked threads.

@TimothyGu
Copy link
Member

Fixed by #2866.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
addition/proposal New features or enhancements
Development

No branches or pull requests

4 participants