text/plain form encoding ignored and incorrectly specified in request header

• Component: Forms
• Status: RESOLVED FIXED

[Carsten Orthbandt]

Set up a HTML form like this:

		<form action="" method="post" enctype="text/plain">
			test1:<input name="test1" size="10">
			test2:<input name="test2" size="10">
			<input type="submit">
		</form>

The request header will say:
Content-Type: text/plain; boundary=
but the actual content is encoded as "application/x-www-form-urlencoded".

It's bad enough that the form enctype parameter is more or less ignored, but specifying a different encoding in the header than is used in the body is a serious problem.

Yes, I can change my forms not to use "text/plain". But it's still a bug.

[Alexey Proskuryakov]

Confirmed with r35843 on Mac OS X 10.4.11 as a difference with Firefox.

Note however that text/plain form serialization is not defined in HTML4 (http://www.w3.org/TR/html401/interact/forms.html#h-17.13.4), and is not yet defined in HTML5 (http://www.w3.org/html/wg/html5/#textplain).

[Carsten Orthbandt]

(In reply to comment #1)
> Confirmed with r35843 on Mac OS X 10.4.11 as a difference with Firefox.
> Note however that text/plain form serialization is not defined in HTML4
> (http://www.w3.org/TR/html401/interact/forms.html#h-17.13.4), and is not yet
> defined in HTML5 (http://www.w3.org/html/wg/html5/#textplain).

Well, especially the HTML5 doc _does_ list text/plain as a valid form encoding (which is a change compared to HTML4). Calling it "not defined" looks like an excuse though, since none of the other encodings are defined there either (yet).
While surprisingly there seems to be no official standard on name/value encoding in text/plain, all browsers I've used back to stone-age Mosaic used the same format.
So I'd expect the upcoming definition to match exactly that.

[Alexey Proskuryakov]

HTML5 now defines this, <http://dev.w3.org/html5/spec/Overview.html#text-plain-encoding-algorithm>.

[Philip Tellis]

I've written a patch to fix this bug, but I don't know how to write a regression test for it.  How do I go about testing what a form would POST to the server without actually having a server side script?

[Philip Tellis]

Created attachment 73350 [details]
Patch that handles enctype="text/plain" correctly

The attached patch changes the behaviour of form submissions when the enctype is set to text/plain and the action is a non mailto: URL

[Alexey Proskuryakov]

> How do I go about testing what a form would POST to the server without actually having a server side
> script?

Actually, you can have a server side script - tests in LayoutTests/http/tests are served by Apache.

[Philip Tellis]

Thanks, will work on the tests today then.

[Alexey Proskuryakov]

Thanks! 

Apache is launched automatically by run-webkit-tests, or you could use the run-webkit-httpd script for debugging in Safari.

[Philip Tellis]

Created attachment 73453 [details]
Patch that handles enctype="text/plain" correctly

Updated patch that also includes test cases, and handles GET requests correctly.

[Alexey Proskuryakov]

Comment on attachment 73453 [details]
Patch that handles enctype="text/plain" correctly

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

Please clarify what exactly this patch implements, and add test coverage. Besides what is mentioned below, there should be a test with non-ASCII data.

> WebCore/ChangeLog:13
> +        Implementation algorithm defined here:
> +        http://dev.w3.org/html5/spec/Overview.html#text-plain-encoding-algorithm

You reference the HTML5 algorithm, but is it fully implemented? We don't implement accept-charset, and we don't implement _charset_ entries. There is no test for sending a file input as text/plain.

> WebCore/loader/FormSubmission.cpp:168
> +        formData = FormData::create(*(static_cast<FormDataList*>(domFormData.get())), domFormData->encoding(), isMailtoForm || attributes.method() == GetMethod ? "application/x-www-form-urlencoded" : encodingType);

This doesn't seem to match what HTML5 says. GET/mailto is always "application/x-www-form-urlencoded", but POST/mailto uses "appropriate form encoding algorithm".

> WebCore/platform/network/FormData.h:98
> +    static PassRefPtr<FormData> create(const FormDataList&, const TextEncoding&, const String encodingType = "application/x-www-form-urlencoded");

The encoding type definitely shouldn't be passed as string copy. Maybe String reference, maybe an AtomicString, or maybe an enum constant. The constant is probably most appropriate.

> WebCore/platform/network/FormDataBuilder.cpp:190
> +        // http://dev.w3.org/html5/spec/Overview.html#text-plain-encoding-algorithm

I don't think that a link to an interim version of HTML5 is useful. That's where one would look by default anyway.

> LayoutTests/http/tests/misc/form-post-textplain.html:13
> +    <input type="hidden" name="f1" value="This is field #1 &!@$%'<>">
> +    <input type="hidden" name="f2" value='This is field #2 ""'>

Interesting characters to test should include '=' and newlines.

[Philip Tellis]

Thanks for the comments, will fix what I can.  As far as I know, this has been a standard as far back as HTML 3.0, but I can't find a reference to the algorithm in the specifications.

[Kent Tamura]

Comment on attachment 73453 [details]
Patch that handles enctype="text/plain" correctly

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

>> WebCore/platform/network/FormData.h:98
>> +    static PassRefPtr<FormData> create(const FormDataList&, const TextEncoding&, const String encodingType = "application/x-www-form-urlencoded");
> 
> The encoding type definitely shouldn't be passed as string copy. Maybe String reference, maybe an AtomicString, or maybe an enum constant. The constant is probably most appropriate.

I agree with Alexey.  We had better introduce an enum set.

[Vineet Chaudhary (vineetc)]

Created attachment 112317 [details]
initial approach to fix this issue.

This is just code patch incorporating above review comments.
Please let me know your review comments on this. 
I will update ChangeLog too once code review is done.

[Kent Tamura]

Comment on attachment 112317 [details]
initial approach to fix this issue.

Please read the style guide: http://www.webkit.org/coding/coding-style.html
> Enum members should user InterCaps with an initial capital letter.

Also, I feel application_x_www_form_urlencoded is too long :-)  FormURLEncoded is enough.

[Vineet Chaudhary (vineetc)]

Created attachment 112559 [details]
updated patch

Modified Enum members as per style guidelines.

[Darin Adler]

Comment on attachment 112559 [details]
updated patch

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

Is this correct behavior? Do other browsers do this? What websites use this feature?

> Source/WebCore/platform/network/FormData.cpp:102
> +PassRefPtr<FormData> FormData::create(const FormDataList& list, const TextEncoding& encoding, const EncodingType encodingType)

This should just be EncodingType, not const EncodingType.

> Source/WebCore/platform/network/FormData.cpp:183
> +void FormData::appendKeyValuePairItems(const FormDataList& list, const TextEncoding& encoding, bool isMultiPartForm, Document* document, const EncodingType encodingType)

No const here either.

> Source/WebCore/platform/network/FormData.h:104
> +    static PassRefPtr<FormData> create(const FormDataList&, const TextEncoding&, const EncodingType encodingType = FormURLEncoded);

This should be:

    EncodingType = FormURLEncoded

The const is not needed or helpful, and the argument name is not helpful.

> Source/WebCore/platform/network/FormData.h:138
> +    static EncodingType getEncodingType(const String& type)

Functions like this are not named with the word “get” in WebKit. This is covered in the coding style guide.

This should probably be named “parseEncodingType”.

> Source/WebCore/platform/network/FormData.h:143
> +             return MultipartFormData;

Extra space here.

> Source/WebCore/platform/network/FormData.h:151
> +    void appendKeyValuePairItems(const FormDataList&, const TextEncoding&, bool isMultiPartForm, Document*, const EncodingType encodingType = FormURLEncoded);

Same comment as above.

> Source/WebCore/platform/network/FormDataBuilder.cpp:184
> +void FormDataBuilder::addKeyValuePairAsFormData(Vector<char>& buffer, const CString& key, const CString& value, const FormData::EncodingType encodingType)

Same comment as above.

> Source/WebCore/platform/network/FormDataBuilder.h:47
> +    static void addKeyValuePairAsFormData(Vector<char>&, const CString& key, const CString& value, FormData::EncodingType encodingType = FormData::FormURLEncoded);

Should omit the argument name here.

[Darin Adler]

(In reply to comment #4)
> I've written a patch to fix this bug, but I don't know how to write a regression test for it.  How do I go about testing what a form would POST to the server without actually having a server side script?

We do need a test for this patch. As Alexey pointed out, we have many tests that do involve server side scripts.

[Vineet Chaudhary (vineetc)]

Thank you Darin for review comments,

Yes this is correct behavior because in webkit forms with post method and "text/plain" enctype are encoded as "application/x-www-form-urlencoded".

This can be verified with latest firefox.

(In reply to comment #17)
> We do need a test for this patch. As Alexey pointed out, we have many tests that do involve server side scripts.

I have added form-post-textplain.html and form-get-textplain.html to test this  behavior, do you want me to add more tests for this?

I will upload patch again incorporating all your comments, thanks!

[WebKit Review Bot]

Attachment 112559 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1

Source/WebCore/platform/network/FormDataBuilder.h:47:  The parameter name "encodingType" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/platform/network/FormData.h:104:  The parameter name "encodingType" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/platform/network/FormData.h:151:  The parameter name "encodingType" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 3 in 12 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Kent Tamura]

(In reply to comment #16)
> (From update of attachment 112559 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=112559&action=review
> 
> Is this correct behavior? Do other browsers do this? What websites use this feature?

According to http://crbug.com/73979, IE, Firefox, and Opera support text/plain.

[Vineet Chaudhary (vineetc)]

Created attachment 112695 [details]
updated_patch

updated patch contains form-post-textplain.html and form-get-textplain.html to test this  behavior, 
please let me know if I need add more tests for this?

[WebKit Review Bot]

Comment on attachment 112695 [details]
updated_patch

Attachment 112695 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10226774

New failing tests:
fast/forms/mailto/formenctype-attribute-input-html.html
fast/forms/mailto/post-text-plain-with-accept-charset.html
fast/forms/mailto/formenctype-attribute-button-html.html
fast/forms/mailto/post-text-plain.html
fast/forms/mailto/post-multiple-items-text-plain.html

[Vineet Chaudhary (vineetc)]

Created attachment 112740 [details]
updated_patch

(In reply to comment #22)
> (From update of attachment 112695 [details])
> Attachment 112695 [details] did not pass chromium-ews (chromium-xvfb):
> Output: http://queues.webkit.org/results/10226774
> 
> New failing tests:
> fast/forms/mailto/formenctype-attribute-input-html.html
> fast/forms/mailto/post-text-plain-with-accept-charset.html
> fast/forms/mailto/formenctype-attribute-button-html.html
> fast/forms/mailto/post-text-plain.html
> fast/forms/mailto/post-multiple-items-text-plain.html

Hello Darin/Alexey,
could you please review this patch once more, sorry for failing test cases :(.
As Alexey mentioned earlier
> GET/mailto is always "application/x-www-form-urlencoded", but POST/mailto uses "appropriate form encoding algorithm".
Above test cases uses post method and test/plain enctype so I have to modify the expected results for above test cases.

[Darin Adler]

Comment on attachment 112740 [details]
updated_patch

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

> Source/WebCore/platform/network/FormData.h:145
> +    static EncodingType parseEncodingType(const String& type)
> +    {
> +        if (equalIgnoringCase(type, "text/plain"))
> +            return TextPlain;
> +        if (equalIgnoringCase(type, "multipart/form-data"))
> +            return MultipartFormData;
> +        return FormURLEncoded;
> +    }

Seems a little ugly to include this entire function body here in the class definition.

[WebKit Review Bot]

Comment on attachment 112740 [details]
updated_patch

Clearing flags on attachment: 112740

Committed r98896: <http://trac.webkit.org/changeset/98896>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Ryosuke Niwa]

Two forms/mailto tests are failing after this patch on SL, Qt, Cr Win, & Cr Linux:
http://build.webkit.org/results/SnowLeopard%20Intel%20Release%20(Tests)/r98926%20(34333)/results.html

http://test-results.appspot.com/dashboards/flakiness_dashboard.html#tests=fast%2Fforms%2Fmailto%2Fformenctype-attribute-button-html.html&group=%40ToT%20-%20webkit.org

[Vineet Chaudhary (vineetc)]

(In reply to comment #27)
> Two forms/mailto tests are failing after this patch on SL, Qt, Cr Win, & Cr Linux:
> http://build.webkit.org/results/SnowLeopard%20Intel%20Release%20(Tests)/r98926%20(34333)/results.html
> 
> http://test-results.appspot.com/dashboards/flakiness_dashboard.html#tests=fast%2Fforms%2Fmailto%2Fformenctype-attribute-button-html.html&group=%40ToT%20-%20webkit.org

Failing tests were platform specific so I have added those to all test_expectations.txt except gtk so those could be rebaselined. Can you please correct me to fix this?

[Ryosuke Niwa]

(In reply to comment #28)
> (In reply to comment #27)
> > Two forms/mailto tests are failing after this patch on SL, Qt, Cr Win, & Cr Linux:
> > http://build.webkit.org/results/SnowLeopard%20Intel%20Release%20(Tests)/r98926%20(34333)/results.html
> > 
> > http://test-results.appspot.com/dashboards/flakiness_dashboard.html#tests=fast%2Fforms%2Fmailto%2Fformenctype-attribute-button-html.html&group=%40ToT%20-%20webkit.org
> 
> Failing tests were platform specific so I have added those to all test_expectations.txt except gtk so those could be rebaselined. Can you please correct me to fix this?

When did this happen (which revision)? Also, no port except Chromium port uses test_expectations.txt for this purpose. You typically grab the results out of bots (build.webkit.org) and check that in instead of skiping/adding failing expectations.

[Vineet Chaudhary (vineetc)]

(In reply to comment #29)
> (In reply to comment #28)
> > (In reply to comment #27)
> > > Two forms/mailto tests are failing after this patch on SL, Qt, Cr Win, & Cr Linux:
> > > http://build.webkit.org/results/SnowLeopard%20Intel%20Release%20(Tests)/r98926%20(34333)/results.html
> > > 
> > > http://test-results.appspot.com/dashboards/flakiness_dashboard.html#tests=fast%2Fforms%2Fmailto%2Fformenctype-attribute-button-html.html&group=%40ToT%20-%20webkit.org
> > 
> > Failing tests were platform specific so I have added those to all test_expectations.txt except gtk so those could be rebaselined. Can you please correct me to fix this?
> 
> When did this happen (which revision)? Also, no port except Chromium port uses test_expectations.txt for this purpose. You typically grab the results out of bots (build.webkit.org) and check that in instead of skiping/adding failing expectations.

In the revision http://trac.webkit.org/changeset/98979 itself I have added failing tests to test_expectations.txt and even build bot didn't give error on attached patch :(. Please let me know if you want me to rebaseline the results for the ports.

[Ryosuke Niwa]

Rebaseline done in http://trac.webkit.org/changeset/98984 (fixed change log in http://trac.webkit.org/changeset/98986).