Bug 14774 – Submitted data only includes first input item

• Component: Forms
• Status: RESOLVED FIXED

[David Brown]

if this form is on a web page the resulting email should have two results separated by &:
---
<form method="post" action="mailto:[e-mail]">
Name: <input type="text" size="10" maxlength="40" name="name"> <br />
Password: <input type="password" size="10" maxlength="10" name="password"><br />
<input type="submit" value="Send"> 
</form>
---
Here is the result from Firefox:      name=dlb&password=photo
and from Webkit 522.12:                name=dlb

[David Kilzer (:ddkilzer)]

Confirming the difference between Firefox 2.0.0.5 and a local debug build of WebKit r24669 with Safari 3 Public Beta v. 3.0.2 (522.12) on Mac OS X 10.4.10 (8R218).

This could be a security precaution, though, to prevent the password from being leaked.  Imagine if you walked up to a computer with the username and password filled out, and used a javascript: URL to change the action on the form to a mailto: and then submitted the form.

[David Kilzer (:ddkilzer)]

Created attachment 15706 [details]
Test case from Comment #0

Please post test cases as attachments for easier testing in the future.  Thanks!

[David Kilzer (:ddkilzer)]

To: David Kilzer
From: David Brown
Date: Sat, 28 Jul 2007 14:24:44 -0700
Subject:	 http://bugs.webkit.org/show_bug.cgi?id=14774

[...]

The example provided used password type field but you will find if you change the field type that the bug occurs for more than one text field as well; it's not a feature.  In any case I agree the password-type field has no significant security, but I would think that's more an issue for the WWW standards group and web developers who use it.

Sorry for improper etiquette.  This was my first bug report.  Feedback is welcome if I still have it wrong.

-- David

[David Kilzer (:ddkilzer)]

Created attachment 15744 [details]
Test case without password field

This test case without a password field fails as well.

[David Kilzer (:ddkilzer)]

(In reply to comment #1)
> Confirming the difference between Firefox 2.0.0.5 and a local debug build of
> WebKit r24669 with Safari 3 Public Beta v. 3.0.2 (522.12) on Mac OS X 10.4.10
> (8R218).

Note that Safari 2.0.4 (419.3) on Mac OS X 10.4.10 (8R218) behaves the same way, so this is not a regression.

[Alexey Proskuryakov]

Created attachment 18198 [details]
proposed fix

Fixes this and many other issues with submitting mailto forms.

Tested GET with Firefox and IE, but only tested POST with Firefox, as IE immediately sends such e-mails instead of opening them in a mail client.

A reconversion in text/plain case looks pretty ugly, I would appreciate suggestions on how to write it in a better way.

[Darin Adler]

Comment on attachment 18198 [details]
proposed fix

Will this work if you submit the same form multiple times?

mailtoForm is not a great name for a boolean; it sounds like it's a form. Maybe isMailtoForm would be a better name? I guess it's not your name.

     bool mailtoForm = u.protocol() == "mailto";

This one is case sensitive, but the new code is correctly folding case.

I'm not happy that this code still has to use DeprecatedString even in this line of code:

+                data = new FormData((DeprecatedString("body=") + encodeCString(body.latin1())).replace('+', "%20").latin1());

Isn't there a way to do this without DeprecatedString and without widening and re-narrowing?

r=me

[Alexey Proskuryakov]

(In reply to comment #7)
> Will this work if you submit the same form multiple times?

  Not sure how to test for this. Is calling form.submit() twice the test for it?

> mailtoForm is not a great name for a boolean; it sounds like it's a form. Maybe
> isMailtoForm would be a better name?

  Will do.

>      bool mailtoForm = u.protocol() == "mailto";
> This one is case sensitive, but the new code is correctly folding case.

  Will do.


> I'm not happy that this code still has to use DeprecatedString even in this
> line of code:
> 
> +                data = new FormData((DeprecatedString("body=") +
> encodeCString(body.latin1())).replace('+', "%20").latin1());
> 
> Isn't there a way to do this without DeprecatedString and without widening and
> re-narrowing?

  Yes, I can get rid of DeprecatedString here. Not sure what you mean by narrowing and re-narrowing (though as I mentioned in a comment, the hack for text/plain looks quite ugly, and I'd appreciate ideas about how to write this in a abetter way).

[Alexey Proskuryakov]

Created attachment 18224 [details]
proposed fix

Addressed review comments that I could address (see questions above).

[Darin Adler]

Comment on attachment 18224 [details]
proposed fix

r=me

[Alexey Proskuryakov]

Committed revision 29086.