Bug 225299 – Constructing a FormData from a form can lead to entries with lone surrogates

• Component: Forms
• Status: RESOLVED FIXED

[Andreu Botella]

WPT test: https://wpt.fyi/results/html/semantics/forms/form-submission-0/form-data-set-usv.html?label=master&label=experimental&aligned

According to the WebIDL definition for FormData, entry names should be scalar value strings, and so should entry values when they aren't files. However, when a FormData object is constructed from a form, lone surrogates in its controls' names and values will end up in the FormData object's entry list as is. While the IDL bindings restrict incoming values to be USVStrings, meaning that surrogate-containing entry names can't be observed from the API, it is possible to observe entry values with surrogates.

In the HTML spec, the conversion into scalar value strings of names and values coming from forms happens during the entry list construction, in the "append an entry" algorithm, at the same time as newlines are normalized to CRLF. Gecko defers those conversions and normalizations until the form payload is encoded, and so does WebKit, except that the USV conversion never seems to happen. The spec and Gecko's behaviors used to be indistinguishable, until FormData was changed to allow inspection of its entry list from JS, whose consequences apparently weren't realized at the time. (See also bug 219086.)

Now in https://github.com/whatwg/html/pull/6624 (together with https://github.com/whatwg/html/pull/6287) we're standardizing on Gecko's and WebKit's behavior of deferring the newline normalization, but we're leaving the USV conversion because it wouldn't make much sense to change FormData to work with DOMStrings.

[Radar WebKit Bug Importer]

<rdar://problem/77740634>

[Andreu Botella]

With https://github.com/whatwg/html/pull/7371, the fact that entry names and values which are not File objects are scalar value strings will now become an invariant of form entries.

[Andreu Botella]

Created attachment 445352 [details]
Patch

[Chris Dumez]

Comment on attachment 445352 [details]
Patch

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

r=me with comments.

> Source/WebCore/html/DOMFormData.cpp:81
> +    return { usvName, usvValue };

We could write this in a more concise way:

return {
    replaceUnpairedSurrogatesWithReplacementCharacter(String { usvName }),
    replaceUnpairedSurrogatesWithReplacementCharacter(String { usvValue })
};

> Source/WebCore/html/DOMFormData.cpp:88
> +    usvName = replaceUnpairedSurrogatesWithReplacementCharacter(WTFMove(usvName));

I think this would look a bit nicer:
auto usvName = replaceUnpairedSurrogatesWithReplacementCharacter(String { usvName });

> Source/WebCore/html/DOMFormData.h:83
> +    Item createStringEntry(const String& name, const String& value);

It seems this can be static. Also, since it is private, no need to declare it in the header.

[Andreu Botella]

Comment on attachment 445352 [details]
Patch

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

>> Source/WebCore/html/DOMFormData.cpp:81
>> +    return { usvName, usvValue };
> 
> We could write this in a more concise way:
> 
> return {
>     replaceUnpairedSurrogatesWithReplacementCharacter(String { usvName }),
>     replaceUnpairedSurrogatesWithReplacementCharacter(String { usvValue })
> };

I still don't fully understand how && and std::move work, and I didn't realize you could write it that way.

>> Source/WebCore/html/DOMFormData.h:83
>> +    Item createStringEntry(const String& name, const String& value);
> 
> It seems this can be static. Also, since it is private, no need to declare it in the header.

I'll also be removing createFileEntry from the header then, for consistency.

[Chris Dumez]

(In reply to Andreu Botella from comment #6)
> Comment on attachment 445352 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=445352&action=review
> 
> >> Source/WebCore/html/DOMFormData.cpp:81
> >> +    return { usvName, usvValue };
> > 
> > We could write this in a more concise way:
> > 
> > return {
> >     replaceUnpairedSurrogatesWithReplacementCharacter(String { usvName }),
> >     replaceUnpairedSurrogatesWithReplacementCharacter(String { usvValue })
> > };
> 
> I still don't fully understand how && and std::move work, and I didn't
> realize you could write it that way.
> 
> >> Source/WebCore/html/DOMFormData.h:83
> >> +    Item createStringEntry(const String& name, const String& value);
> > 
> > It seems this can be static. Also, since it is private, no need to declare it in the header.
> 
> I'll also be removing createFileEntry from the header then, for consistency.

Yes, please.

[Andreu Botella]

Created attachment 445423 [details]
Patch

[Chris Dumez]

Comment on attachment 445423 [details]
Patch

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

> Source/WebCore/html/DOMFormData.cpp:73
> +auto createStringEntry(const String& name, const String& value) -> DOMFormData::Item

Please mark as static

> Source/WebCore/html/DOMFormData.cpp:82
> +auto createFileEntry(const String& name, Blob& blob, const String& filename) -> DOMFormData::Item

ditto.

[Andreu Botella]

Created attachment 445426 [details]
Patch

[EWS]

Committed r286310 (244669@main): <https://commits.webkit.org/244669@main>

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

[Darin Adler]

Comment on attachment 445426 [details]
Patch

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

> Source/WebCore/html/DOMFormData.cpp:87
> +        return { usvName, File::create(blob.scriptExecutionContext(), blob, filename.isNull() ? "blob"_s : filename) };

I believe we can slightly reduce reference count churn by using WTFMove(usvName) here.

> Source/WebCore/html/DOMFormData.cpp:90
> +        return { usvName, File::create(blob.scriptExecutionContext(), downcast<File>(blob), filename) };

Ditto.

> Source/WebCore/html/DOMFormData.cpp:92
> +    return { usvName, RefPtr<File> { &downcast<File>(blob) } };

Ditto.

One other thought: I think we can just write RefPtr instead of RefPtr<File>.