Right now, there are two unsafe places to take actions (form builder, form validate handler) and one safe place (form submit handler). If we would abort the rest of the form validation after the token validation fails, we would automatically protect our validation handlers from CSRF as well, and we'd have TWO safe places, one unsafe.

The arguments for continuing validation are not strong; we can show errors on more fields. Okay, but the user will not be able to submit anyway, as the token is incorrect.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

sun’s picture

Category: feature » task

+1

Heine’s picture

Version: 7.x-dev » 8.x-dev
Category: task » feature

A bit late in the game now, imo :)

sun’s picture

greggles’s picture

I don't think the patch that made it in via #240828: "This form is outdated. Reload the page and try again" is incorrect and confuses users included a change to address this, right?

Heine’s picture

Issue summary: View changes

This has been changed in 7.x and 6.x with SA-CORE-2013-003.

larowlan’s picture

Status: Active » Needs review
Issue tags: +SA-CORE-2013-03
FileSize
3.69 KB

Forward port of fix from SA

larowlan’s picture

Priority: Normal » Major
larowlan’s picture

Category: Feature request » Bug report

Status: Needs review » Needs work

The last submitted patch, 6: valid-token-576276.4.patch, failed testing.

scor’s picture

Issue tags: -SA-CORE-2013-03 +Security Advisory follow-up, +SA-CORE-2013-003
amateescu’s picture

Priority: Major » Critical

SA followups are critical, marking as such.

tim.plunkett’s picture

  1. +++ b/core/modules/system/lib/Drupal/system/Tests/Form/ValidationTest.php
    @@ -65,6 +65,18 @@ function testValidate() {
    +    // Verify that #validate handlers don't run if the CSRF token is invalid.
    

    Once #2131851: Form errors must be specific to a form and not a global is in this could be a nice unit test.

I have no other complaints otherwise.

tim.plunkett’s picture

+++ b/core/modules/system/lib/Drupal/system/Tests/Form/ElementsTableSelectTest.php
@@ -223,6 +223,9 @@ private function formSubmitHelper($form, $edit) {
+    $form_state['programmed'] = TRUE;

+++ b/core/modules/system/lib/Drupal/system/Tests/Form/FormTest.php
@@ -111,6 +111,9 @@ function testRequiredFields() {
+          $form_state['programmed'] = TRUE;

I was looking at FormBuilder tonight and noticed this:

      // Form constructors may explicitly set #token to FALSE when cross site
      // request forgery is irrelevant to the form, such as search forms.
      if (isset($form['#token']) && $form['#token'] === FALSE) {
        unset($form['#token']);
      }

So maybe that is a better option here?

larowlan’s picture

Works for me

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
3.71 KB

There are some issues with the current unit tests that shouldn't hold up this one, leaving the simpletest in place and making the #token => FALSE change.

Status: Needs review » Needs work

The last submitted patch, 15: token-validation-fapi-576276-15.patch, failed testing.

larowlan’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 15: token-validation-fapi-576276-15.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
3.71 KB
larowlan’s picture

Status: Needs review » Reviewed & tested by the community

Then we're good

catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.x, thanks!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.