Bug 220780 - Null bytes aren't percent-encoded on urlencoded over POST
Summary: Null bytes aren't percent-encoded on urlencoded over POST
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Forms (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-01-20 15:01 PST by Andreu Botella
Modified: 2022-02-04 12:51 PST (History)
7 users (show)

See Also:


Attachments
Patch (4.75 KB, patch)
2021-11-14 02:55 PST, Andreu Botella
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Andreu Botella 2021-01-20 15:01:41 PST
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)
Comment 1 Radar WebKit Bug Importer 2021-01-21 16:50:42 PST
<rdar://problem/73475504>
Comment 2 Alex Christensen 2021-01-21 20:37:55 PST
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?
Comment 3 Andreu Botella 2021-01-22 01:50:20 PST
(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.
Comment 4 Alex Christensen 2021-01-23 22:19:33 PST
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.
Comment 5 Alex Christensen 2021-01-23 22:45:31 PST
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.
Comment 6 Andreu Botella 2021-01-24 08:55:53 PST
(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.
Comment 7 Andreu Botella 2021-05-21 05:21:30 PDT
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.
Comment 8 Andreu Botella 2021-11-14 02:55:32 PST
Created attachment 444175 [details]
Patch
Comment 9 EWS 2021-11-15 10:36:51 PST
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].
Comment 10 Alex Christensen 2021-11-15 11:11:34 PST
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
Comment 11 Brent Fulgham 2022-02-04 12:51:10 PST
This change should be present in STP 139, iOS 15.4 Beta, and macOS 12.3 Beta.