Protection against forgery of input selection value doesn't work with checkboxes

To reproduce:

function scratch_form(&$form_state) {
	$form = array();

	$options = array(
		'one' => 'One',
		'two' => 'Two',
	);

	$form['secissue'] = array(
		'#type' => 'checkboxes',
		'#options' => $options,
	);

	$form['submit'] = array(
		'#type' => 'submit',
		'#value' => t('Submit'),
	);
	return $form;
}

function scratch_form_submit($form, &$form_state) {
	drupal_set_message('<pre>'. check_plain(print_r($form_state['values'], TRUE)) .'</pre>');
}

When checking the checkboxes, results in (only relevant key given):

[secissue] => Array
        (
            [one] => one
            [two] => two
        )

Whe modifying the value of $_POSTed secissue[two] to FOO:

[secissue] => Array
        (
            [one] => one
            [two] => FOO
        )

Only the value is affected; when you try to add another key, the "Illegal option" error message is given. This is not an issue on Drupal 6.

See #426056: Server-side enforcement of #disabled is inconsistent for more info about #disabled.

I've traced this back to #414424: Introduce Form API #type 'text_format'.

In Drupal 6, checkboxes behaviour results in:

array(
  'key' => 'key', // For checked boxes
  'key' => 0,     // For unchecked boxes
);

regardless of the 'value' posted for the element.

What happens in D6 is that the "input" from the checkboxes element is added to form_state, and later overwritten by the child checkboxes; they always return the correct return_value. The following check that was added by the referenced issue prevents this overwriting by the child elements:

3f587416 (webchick 2010-03-07 23:14:20 +0000 1561)   // Set the element's value in $form_state['values'], but only, if its key
3f587416 (webchick 2010-03-07 23:14:20 +0000 1562)   // does not exist yet (a #value_callback may have already populated it).
3f587416 (webchick 2010-03-07 23:14:20 +0000 1563)   $values = $form_state['values'];
3f587416 (webchick 2010-03-07 23:14:20 +0000 1564)   foreach ($element['#parents'] as $key) {
3f587416 (webchick 2010-03-07 23:14:20 +0000 1565)     $values = (isset($values[$key]) ? $values[$key] : NULL);
3f587416 (webchick 2010-03-07 23:14:20 +0000 1566)   }
3f587416 (webchick 2010-03-07 23:14:20 +0000 1567)   if (!isset($values)) {
3f587416 (webchick 2010-03-07 23:14:20 +0000 1568)     form_set_value($element, $element['#value'], $form_state);
3f587416 (webchick 2010-03-07 23:14:20 +0000 1569)   }

This _could_ be handled by the checkboxes value callback, or process handler, but I doubt that it is the correct approach; the behaviour of the processed element should depend on the children, and should not be coded entirely in the parent process / value callbacks.

Related issue #335035: drupalPost() incorrectly submits input for disabled elements.

Thanks. This fixes form_type_checkboxes_value() to be consistent with form_type_select_value().

This also fixes drupal_map_assoc() to handle empty arrays.

subscribe

@arpeggio: In the original issue description, you write:

Same with disabled input elements, making the "#disabled" property of any element to TRUE should prohibit the user from submitting the form but by editing the HTML code to remove the disabled="disabled" property of any input element and submit this form, Drupal does not validate this kind of forgery and still accept the submitted disabled input form.

This is true for D6, but was fixed for D7: #426056: Server-side enforcement of #disabled is inconsistent. If you can find a use-case where that bug remains, please post the steps to reproduce it. Thanks. Note, for D6, since #disabled isn't enforced server-side, if your module needs such enforcement, you need to set #value. See the linked issue.

Can we please discuss the disabled issue elsewhere? edit; Maybe reopen #426056: Server-side enforcement of #disabled is inconsistent ?

As of #9; I find it odd that an element type processed into simpler types has no access to values of that type, but has to reimplement such functionality on raw $_POST data.

I would expect the following flow of information:

- type checkboxes -> processed into multiple checkbox elements
- value of all checkbox elements is set _individually_ in form_type_checkbox_value
- form_type_checkboxes_value runs later & receives these values, then returns the appropriate value for the checkboxes element.
- this value lands in form_state['values']

Unfortunately, it seems to late to do this in D7.

Thanks for #12. #736298: Refactor file.module to use proper button click detection, enabling FAPI to improve button click detection security fixes that problem (see comment #46 on that issue): I'll try to roll in a test into that issue's patch with your use-case. So as per #13, this issue is now strictly about checkbox value forgery.

Re #14:

Unfortunately, it seems too late to do this in D7.

I agree that it is too late for that for D7. _form_builder_handle_input_element() runs in pre-order traversal (i.e., form_builder() calls it before recursing). This has been the case since FAPI was first introduced, and changing it to post-order traversal will have many side effects that are way out of scope for D7.

anyone available to slay this critical?

#9 slays this as far as I'm concerned. Comments after it have been answered or relegated to other issues. Anyone available to RTBC this issue?

Patch is only a couple of lines and the test looks good, it would be nice to see another FAPI maintainer or Heine take a look at this before commit, maybe marking RTBC will encourage that second look.

Patch itself is RTBC would it not for the options checker allowing the insertion of 0 keys:

$value = $elements['#type'] == 'checkboxes' ? array_keys($elements['#value']) : $elements['#value'];

I think I agree with the change in #20, but I'm not sure it belongs in this issue. #654796: Identify fix bugs with checkbox element is still a critical issue for D7, and if the line in #20 has unwanted side-effects, that would be the issue to catch it. @Heine: what's your thinking for including it here?

I reviewed this patch -- it fixes the problem. Committed to CVS HEAD. Thanks Heine and effulgentsia -- great job.

The change introduced in #20 did indeed have an unwanted side effect: It prevents drupal_form_submit() from programmatically turning off a checkbox by submitting a NULL value for it.

I posted a patch which fixes it here (not a simple rollback, since for everything but checkboxes I think #20 is probably correct):

#827430: drupal_form_submit() no longer works with checkboxes

It could be pulled into #654796: Identify fix bugs with checkbox element instead, but I think probably not, since this really is just about NULL values rather than other things which also evaluate to empty.