Bug 220780 – Null bytes aren't percent-encoded on urlencoded over POST

• Component: Forms
• Status: RESOLVED FIXED

[Andreu Botella]

The application/x-www-form-urlencoded serializer per the URL spec percent-encodes every byte in the urlencoded percent-encode set, which includes all controls. But WebKit doesn't seem to be percent-encoding the null byte on urlencoded over POST. It is percent-encoding it over GET, which I assume is because the normal url query string percent-encoding is being applied on top.

This bug doesn't happen with URLSearchParams, though (https://wpt.fyi/results/url/urlsearchparams-stringifier.any.html?label=pr_head&max-count=1&pr=26740).

Tests: https://wpt.fyi/results/html/semantics/forms/form-submission-0/urlencoded2.window.html?label=pr_head&max-count=1&pr=26740
(ignore the "formdata event" entries, since those are meant to test entries added to the form entry list through the formdata event, which WebKit doesn't yet implement)

[Radar WebKit Bug Importer]

<rdar://problem/73475504>

[Alex Christensen]

I'm quite interested in this bug.  Why do the links from wpt.fyi to run it in my browser or view the source on GitHub just result in HTTP 404 errors?

[Andreu Botella]

(In reply to Alex Christensen from comment #2)
> I'm quite interested in this bug.  Why do the links from wpt.fyi to run it
> in my browser or view the source on GitHub just result in HTTP 404 errors?

Those tests are from a PR that hasn't been merged yet because it's primarily meant to test changes in the spec that are still being discussed (and that don't affect WebKit atm, but might in the final version), and the tests for null bytes are incidental: https://github.com/web-platform-tests/wpt/pull/26740

I thought the fact that those wpt.fyi links didn't work for PRs was a known bug, but it's not listed on the https://github.com/web-platform-tests/wpt.fyi issue list.

[Alex Christensen]

We're currently using FormData::appendNonMultiPartKeyValuePairItems in these tests and it appears we need to use something more like URLParser::serialize to be compatible with other browsers.

[Alex Christensen]

We'll need more test coverage than that PR has.  Right now we use the encoding of the DOMFormData but other browsers seem to always use UTF-8.

[Andreu Botella]

(In reply to Alex Christensen from comment #5)
> We'll need more test coverage than that PR has.  Right now we use the
> encoding of the DOMFormData but other browsers seem to always use UTF-8.

The two "characters not in encoding..." tests set the form's accept-charset to windows-1252 and pass in all browsers. If Firefox and Chrome used UTF-8, they would instead fail with "a%C9%99b=c%EF%BF%BDd" and "%C3%A1=%F0%9F%92%A9".

But yeah, that PR was initially only meant to test newline normalizations on filenames in the urlencoded enctype, and since there were no other tests for urlencoded forms, I thought I'd add some corner cases as well. But in the time since we realized that that bug affected more than we initially thought. But feel free to comment on that PR to propose more test cases.

[Andreu Botella]

The WPT PR has now been merged for over a month, and as I explained in comment #6, the issue is not with Firefox and Chrome using only UTF-8. I'm not very familiar with WebKit code, but it seems like FormData::appendNonMultiPartKeyValuePairItems ultimately defers to FormDataBuilder::appendFormURLEncoded, which aims to implement the application/x-www-form-urlencoded percent-encode set (https://url.spec.whatwg.org/#application-x-www-form-urlencoded-percent-encode-set) but apparently misses the fact that the strchr in line 93 can also match a null character.

[Andreu Botella]

Created attachment 444175 [details]
Patch

[EWS]

Committed r285810 (244254@main): <https://commits.webkit.org/244254@main>

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

[Alex Christensen]

This was certainly a small step in the right direction, but I feel like it was probably incomplete.  We probably still need logic more like serializeURLEncodedForm

[Brent Fulgham]

This change should be present in STP 139, iOS 15.4 Beta, and macOS 12.3 Beta.