Bug 233725 – File inputs in non-multipart form submissions show up as string values in the formdata event

• Status: RESOLVED
• Resolution: FIXED
• Component: Forms

[Andreu Botella]

When a form's entry list is constructed, file inputs are being added to the DOMFormData object as entries with File values only if the entry list is being constructed as part of a multipart/form-data form submission or through the FormData constructor – for urlencoded and text/plain form submissions, file inputs are added as entries with a string value consisting of the file's filename. Before r280310, this followed the spec: this filename conversion is part of "convert to a list of name-value pairs" (https://html.spec.whatwg.org/multipage/form-control-infrastructure.html#convert-to-a-list-of-name-value-pairs), except done earlier in the form submission than the rest of the normalization in that algorithm, and there was no way to observe the difference because the DOMFormData object was never exposed to the user halfway through a form submission.

r280310 changed this by adding the formdata event, which fires at the end of the "construct the entry list" algorithm and can both inspect and modify the DOMFormData object. This means that it is now observable that file inputs in urlencoded and text/plain form submissions are encoded as their filenames, which goes against the spec. Additionally, any new File entries added to the DOMFormData object in that event will be skipped when building the form payload, which also shouldn't happen.

It looks like file inputs should always be added to DOMFormData as File values, and the conversion to a filename should happen in FormData::appendNonMultipartKeyValuePairItems, along with the rest of normalizations in "convert to a list of name-value pairs".

[Andreu Botella]

Created attachment 445620 [details]
Patch

[EWS Watchlist]

This patch modifies the imported WPT tests. Please ensure that any changes on the tests (not coming from a WPT import) are exported to WPT. Please see https://trac.webkit.org/wiki/WPTExportProcess

[Chris Dumez]

Comment on attachment 445620 [details]
Patch

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

> Source/WebCore/ChangeLog:10
> +        filename. Before r280310, this optimizaton was unobservable, since the DOMFormData object

typo: optimizaton

> Source/WebCore/html/FileInputType.cpp:161
> +bool FileInputType::appendFormData(DOMFormData& formData, bool) const

Can we drop the boolean parameter completely now or is it still required for some reason?

> Source/WebCore/platform/network/FormData.cpp:277
> +        if (std::holds_alternative<String>(item.data))

May be nice to use WTF::switchOn() here.

[Chris Dumez]

Comment on attachment 445620 [details]
Patch

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

> LayoutTests/imported/w3c/web-platform-tests/html/semantics/forms/form-submission-0/constructing-form-data-set.html:80
> +for (const enctype of ["application/x-www-form-urlencoded", "multipart/form-data", "text/plain"]) {

Is this a resync from upstream? or is there a corresponding PR on upstream WPT?

The change log doesn't say and we cannot make changes to WPT downstream unless these changes are also made upstream.

[Andreu Botella]

Comment on attachment 445620 [details]
Patch

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

>> Source/WebCore/html/FileInputType.cpp:161
>> +bool FileInputType::appendFormData(DOMFormData& formData, bool) const
> 
> Can we drop the boolean parameter completely now or is it still required for some reason?

It's no longer required, but it involves changes in every subclass of InputType and FormAssociatedElement, as well as removing the third parameter to HTMLFormElement::constructEntryList, so it's probably best to leave for a follow-up patch.

[Andreu Botella]

Comment on attachment 445620 [details]
Patch

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

>> LayoutTests/imported/w3c/web-platform-tests/html/semantics/forms/form-submission-0/constructing-form-data-set.html:80
>> +for (const enctype of ["application/x-www-form-urlencoded", "multipart/form-data", "text/plain"]) {
> 
> Is this a resync from upstream? or is there a corresponding PR on upstream WPT?
> 
> The change log doesn't say and we cannot make changes to WPT downstream unless these changes are also made upstream.

I opened https://github.com/web-platform-tests/wpt/pull/31821 upstream.

[Andreu Botella]

Created attachment 445650 [details]
Patch

[Chris Dumez]

Comment on attachment 445650 [details]
Patch

r=me

[EWS]

Committed r286427 (244773@main): <https://commits.webkit.org/244773@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 445650 [details].

[Radar WebKit Bug Importer]

<rdar://problem/85970598>