Is access control necessary upon form submission if I've controlled access to the page containing the form?

Question

Let's say a malicious user has no access to some path (e.g. path/element/1) based on 'access callback' as set in hook_menu() or altered in hook_menu_alter(). What happens if he has access to path/element/2?

Could he simply not change the values that are POSTed in the form submission to the server? I thought they can (even though there are form tokens). If I do several access checks to determine who can view a page, and that page contains a form that can be submitted by anyone who has access to that page, do I need to do those same access checks in the #submit or #validate handler or anything like that? Or can I rely on Drupal's form API security to keep me from having to tediously duplicate all that access code twice for pretty much every custom form!

I know Drupal has some form tokens and whatnot, but doesn't this depend on the developer who output that form (assume a contrib or custom module) setting things up right? I remember a while back I created a custom form and tried changing the POSTed values of one of the elements, and the altered value is what got picked up by Drupal! There was no rejection of the form based on altered values! So clearly I did something wrong there (what might it have been?) - and if I did, then a developer of a contrib module could do the same. So what's the best practice here?

Answer 1

It is not necessary to control if the user has access to a form in its submission handler.

Drupal protects its forms from form forgery, and verifies the values contained in the submitted form. The first control is done in drupal_validate_form(), where the form token (which is automatically assigned to every form built through the form API) is verified; if the value of $form_state['values']['form_token'] is different from the value of drupal_hmac_base64($form['#token'], session_id() . drupal_get_private_key() . drupal_get_hash_salt()), the form is rejected. The submitted values are also validated in _form_validate(), where the length of the entered value is checked against the maximum length reported for the form field, and the value entered for a "checkboxes," or a "tableselect" form field is checked against its acceptable values. In both the cases, the value used to verify the submitted values is not taken from the submitted values.

To be more sure, you should not pass sensible data, such as the ID of the node being edited as hidden field; it is preferable to set it inside the $form variable, using an array index that starts with "#" and that is unique for that form (such as "#nid"), or use a "value" form field; in that way, the value will not be passed to the HTML form rendered in the browser, and a forged form would not be able to alter that value to effectuate an operation on an entity for which the user doesn't have access. For example, this is the code used for a confirmation form that allows to delete a node:

function node_delete_confirm($form, &$form_state, $node) {
  $form['#node'] = $node;
  // Always provide entity id in the same form key as in the entity edit form.
  $form['nid'] = array(
    '#type' => 'value',
    '#value' => $node->nid,
  );
  return confirm_form($form, 
    t('Are you sure you want to delete %title?', array('%title' => $node->title)), 
    'node/' . $node->nid, 
    t('This action cannot be undone.'), 
    t('Delete'), 
    t('Cancel')
  );
}

Both $form['#node'] and $form['nid'] are not contained in the form passed to the browser.

The submission handler for that form builder is the following one:

function node_delete_confirm_submit($form, &$form_state) {
  if ($form_state['values']['confirm']) {
    $node = node_load($form_state['values']['nid']);
    node_delete($form_state['values']['nid']);
    watchdog('content', '@type: deleted %title.', array('@type' => $node->type, '%title' => $node->title));
    drupal_set_message(t('@type %title has been deleted.', array('@type' => node_type_get_name($node), '%title' => $node->title)));
  }

  $form_state['redirect'] = '<front>';
}

The value of $form passed to the form handlers is not passed to the browser, but it is retrieved in the Drupal cache from drupal_build_form(), which calls form_get_cache() when the form has been submitted. The documentation of drupal_build_form() contains the following sentence:

The form may also be retrieved from the cache if the form was built in a previous page-load. The form is then passed on for processing, validation and submission if there is proper input.

If you look at Drupal code, you will notice that none of the core modules check if the user has the permission of accessing the form in the form submission; the only code you will find is the code that checks if an error message must be output, in the case that error message should be visible to a group of users that is smaller than the group of users that has access to the form.

As side note, Drupal.org has a security team that investigates on security issues reported for Drupal core code, and any third-party modules hosted on Drupal.org; for third-party modules, they don't investigate on reports for non official releases of the modules (e.g. alpha, beta, and development snapshots). If you find a security issue in Drupal, you should report it in the project queue for Drupal core.

Answer 2

The user has total control of what he sends to your server; always. That's the nature of client-server computing.

It is the responsibility of the access callback, to determine if the user has access to the URL. The access callback does usually not look at the posted values for this, although such an access callback could be implemented.

It is the responsibility of the validation and submit handlers to not update entities that the user is not allowed to update if that is so desired. This can be achieved in two ways, depending on where the entity that is to be updated comes from.

1. The entity can be determined from the drupal path and passed to the submit handler from the menu system. In this case the submit handler should not update the primary key.
2. The entity can be determined by the form values. In this case the submit handler must determine whether the user is allowed to update the entity.