When submitting a webform with specially crafted keys, webform_get_cid() will return the same component ID, thus causing values to be overwritten. This is due to a PHP casting issue from string to integer, causing a comparison to fail.
Take the following components:
Component 1 (form_key: 4component)
Component 2 (form_key: 4)
In _webform_client_form_submit_flatten():
function _webform_client_form_submit_flatten($node, $fieldset, $parent = 0) {
$values = array();
if (is_array($fieldset)) {
foreach ($fieldset as $form_key => $value) {
if ($cid = webform_get_cid($node, $form_key, $parent)) {
The "foreach" casts the "4" into an integer.
In webform_get_cid():
function webform_get_cid(&$node, $form_key, $pid) {
foreach ($node->webform['components'] as $cid => $component) {
if ($component['form_key'] == $form_key && $component['pid'] == $pid)
The $component['form_key'] == $form_key is problematic as it is now comparing an int to a string.
The resulting PHP runs '4component' == 4 which does evaluate to TRUE so it returns the wrong component ID.
I casted the form_key to a string which works, but I'm not sure if that's the best way. It may also be a good idea to limit form_keys to begin with a letter so that PHP's integer casting doesn't happen.
| Comment | File | Size | Author |
|---|---|---|---|
| #8 | webform-string_form_key_comparison-2747963-8-D7.patch | 3.35 KB | liam morland |
| #2 | submitted_values_lost-2747963-2.patch | 498 bytes | djdevin |
Comments
Comment #2
djdevinComment #3
djdevinComment #4
djdevinComment #5
artis.bajars commentedEncountered the same data loss issue with similarly constructed form_key values for elements. The provided patch seems to work well and I've not ran into any problems after applying it.
Comment #6
danchadwick commentedI think the right patch would include code to prevent form_keys which PHP will readily cast to numbers. The rules for this are probably byzantine (e.g. "-1e-6" is probably a number), so maybe the easiest thing is to ensure that the form_key matches something like: /[_a-zA-Z]+[_a-zA-Z0-9-]*/. Then there's the question of what a hook_update_N would do. Find invalid form_keys and updated them, possibly breaking other custom code or themes?
Comment #7
liam morlandThis is a great example of why it is best to always use
===. If casting is needed, do it yourself to ensure that you know what is going on.form_keys are supposed to be strings, so it should be possible to use
(string) $component['form_key'] === (string) $form_key. If the code is consistent about making comparisons on a string basis, it shouldn't matter if the form_key is what PHP considers a numeric string. That avoids the need to try to fix this in an update hook, which would probably break things somewhere.Comment #8
liam morlandThis patch makes all comparisons of form_keys with each other be string comparisons. Please let me know what you think.
It would be helpful if there was one or more tests which check for this problem.
Comment #10
liam morland