Scope for Drupal 8:

This adds test coverage to ensure that the following bug that is currently affecting Drupal 7 will not regress in Drupal 8 and later.

The bug:

In Drupal 7 it is currently impossible to submit a form as an anonymous user when the CSRF tokens are disabled. When such a form is submitted the following validation error is always shown:

The form has become outdated. Copy any unsaved work in the form below and then reload this page.

Original report by tea.time

I wanted to bypass token-based validation in a custom form, so I followed the note in drupal_prepare_form():

// Form constructors may explicitly set #token to FALSE when cross site
// request forgery is irrelevant to the form, such as search forms.

... and set $form['#token'] = FALSE in my form constructor function.

However, drupal_prepare_form() doesn't touch #token at all if the user is anonymous, so it remains set with the value of FALSE and then drupal_validate_form() passes that to drupal_valid_token(), which obviously fails every time. This was quite a surprising fallout until I read the code again carefully.

I understand from the comments in the code that #token-based validation is only intended for authenticated users. In most cases, then, the problem for anonymous users could be avoided by instead doing:

global $user;
if ($user->uid > 0) $form['#token'] = FALSE;

----------------------------------------------------------------------------------------

I realize this could be working as designed; so, I suggest either of the following:

1. Add a warning in either the API docs or code comments for drupal_prepare_form() that $form['#token'] = FALSE consistently breaks validation for anon users (and therefore a check must be made somehow).

2. Adjust drupal_validate_form() to check for this case and therefore make $form['#token'] = FALSE a non-breaking/functional option for anon users as well, by changing the line:

if (isset($form['#token'])) {

to

if (isset($form['#token']) && $form['#token'] !== FALSE) {

Beta phase evaluation

-->

Reference: https://www.drupal.org/core/beta-changes
Issue category Task because it adds missing test coverage.
Issue priority Normal because it adds test coverage for D8 for an issue which currently only affects D7.
Unfrozen changes Unfrozen because it only changes tests.
Disruption Not disruptive
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

pfrenssen’s picture

Version: 7.14 » 8.x-dev
Category: task » bug
FileSize
586 bytes

This is a bug. I can confirm this problem and the proposed solution is valid. It is even easier if you use the empty() function, like this:

  if (!empty($form['#token'])) { 

This needs to be fixed in 8.x first and then backported to 7.x. Here's a patch for 8.x.

pfrenssen’s picture

Status: Active » Needs review

Status: Needs review » Needs work

The last submitted patch, 1617918-1-form-anonymous_no_token.patch, failed testing.

pfrenssen’s picture

Status: Needs work » Needs review
pfrenssen’s picture

Status: Needs review » Needs work
Issue tags: +Needs tests

Needs a test

oriol_e9g’s picture

Status: Needs work » Needs review
Issue tags: -Needs tests

Status: Needs review » Needs work
Issue tags: +Needs tests

The last submitted patch, 1617918-1-form-anonymous_no_token.patch, failed testing.

pfrenssen’s picture

The bots are a bit cranky :)

c960657’s picture

I agree with the fix. Successfully tested in D7.

pfrenssen’s picture

Status: Needs work » Needs review
Heine’s picture

Probably best to remove the unset in drupal_prepare_form and just check for === FALSE, right?

rfay’s picture

I *think* that the documentation could also just be changed to say that $form['#token'] should be set to NULL if you want to bypass validation. Would that be correct?

Fabianx’s picture

Status: Needs review » Needs work

This still needs tests.

rfay’s picture

BTW, the problem in our case was submitting the form via drupal_form_submit(), which is in fact incompatible with $form['#token'] = FALSE. What we did in this case (thanks to @fabian) was in the formbuilder function:

+  if (empty($form_state['programmed'])) {
+    $form['#token'] = FALSE;
+  }
TravisCarden’s picture

Issue tags: +Needs backport to D7
pfrenssen’s picture

Version: 8.0.x-dev » 7.x-dev
Issue tags: -Needs backport to D7
FileSize
2.16 KB
2.71 KB

Hit this problem again on a D7 site today. I rolled the patch for D7 and provided a test.

Temporarily setting the version to D7 so the patch can be tested. This still needs a test for D8.

pfrenssen’s picture

Status: Needs work » Needs review

The last submitted patch, 17: 1617918-17-form-anonymous_no_token-test_only.patch, failed testing.

Fabianx’s picture

Version: 7.x-dev » 8.0.x-dev
Status: Needs review » Needs work

RTBC for 7.x, back to 8.x for adding tests to the fix

pfrenssen’s picture

Assigned: Unassigned » pfrenssen
Issue tags: +Needs reroll

This needs to be rerolled, both for D7 since 7.39 touched this code, and for D8 since it has been refactored a dozen times since this patch was originally posted.

I'll take care of both rerolls, and will also write the test for D8.

pfrenssen’s picture

Version: 8.0.x-dev » 7.x-dev
Status: Needs work » Needs review
FileSize
2.9 KB
2.16 KB

Here is a rerolled patch for Drupal 7.

The last submitted patch, 22: 1617918-22-7.x.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 22: 1617918-22-7.x-test-only.patch, failed testing.

oenie queued 22: 1617918-22-7.x.patch for re-testing.

Status: Needs work » Needs review
pfrenssen’s picture

Title: $form['#token'] = FALSE in custom form always causes validation error for anonymous users » Test that no validation errors occur for anonymous users when $form['#token'] == FALSE
Version: 7.x-dev » 8.0.x-dev
Assigned: pfrenssen » Unassigned
Issue tags: -Needs tests, -Needs reroll +needs backport to 7.x
FileSize
2.73 KB

On Drupal 8 the bug no longer occurs in the meanwhile. It was fixed in #2143349: Submitting a form as an anonymous user when $form['#token'] = FALSE results in a notice, but it wasn't realized in that issue that this actually prevents tokenless forms from working, it was just considered to be a minor fix for a notice. As part of that issue a random existing test was chosen (ElementsTableSelectTest) and had the access token removed from it to prove that the notice doesn't occur any more.

I think would be useful to have an actual dedicated test in Drupal 8 that checks that a form without a token can be submitted. There is always a possibility that ElementsTableSelectTest will be refactored in the future. Having a dedicated test will ensure that we will cover this for the future.

I ported the test for Drupal 8 in this patch.

Rewording title to make it clear that for Drupal 8 this is just adding test coverage.

The last submitted patch, 22: 1617918-22-7.x.patch, failed testing.

The last submitted patch, 22: 1617918-22-7.x-test-only.patch, failed testing.

stefan.r’s picture

Status: Needs review » Reviewed & tested by the community

Looks good, yay for extra test coverage! I guess this needs a beta evaluation template?

+++ b/core/modules/system/src/Tests/Form/ValidationTest.php
@@ -76,6 +76,14 @@ function testValidate() {
+    $this->assertText(t('The form_test_validate_no_token form has been submitted successfully.'));

unnecessary t() :)

pfrenssen’s picture

Issue summary: View changes
FileSize
2.72 KB
635 bytes

Good catch with the stray t(), this will also be needed in the D7 version of the patch. Leaving at RTBC since this is a minor nitpick.

Also added beta evaluation.

pfrenssen’s picture

Issue summary: View changes
pfrenssen’s picture

Updated Drupal 7 version of the patch:

  1. Addressed the failure from #22.
  2. Removed the stray t().
  3. Use the same message as in the D8 version of the test, this is more consistent with the other related tests.

Sorry for the confusion but I need to get this patch out so I can include it in my make file for D7. Uploading patch as do-not-test so I can leave the current issue status.

People looking for the D8 patch, see #31 :)

alexpott’s picture

Version: 8.0.x-dev » 7.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)

Committed 1e1a38b and pushed to 8.0.x. Thanks!

Thanks for adding the beta evaluation to the issue summary.

  • alexpott committed 1e1a38b on 8.0.x
    Issue #1617918 by pfrenssen, tea.time: Test that no validation errors...
Ayesh’s picture

FileSize
3.54 KB

Thanks for the 7.x patch. It works great. Tests may be?

pfrenssen’s picture

Status: Patch (to be ported) » Needs review

@Ayesh, thanks for re-uploading the D7 patch. Setting to "Needs review" so that it can be tested by the bot.

pfrenssen’s picture

Restoring original issue title for D7.

pfrenssen’s picture

Title: Test that no validation errors occur for anonymous users when $form['#token'] == FALSE » $form['#token'] = FALSE in custom form always causes validation error for anonymous users

  • alexpott committed 1e1a38b on 8.1.x
    Issue #1617918 by pfrenssen, tea.time: Test that no validation errors...

  • alexpott committed 1e1a38b on 8.3.x
    Issue #1617918 by pfrenssen, tea.time: Test that no validation errors...

  • alexpott committed 1e1a38b on 8.3.x
    Issue #1617918 by pfrenssen, tea.time: Test that no validation errors...
kristiaanvandeneynde’s picture

Status: Needs review » Reviewed & tested by the community

Patch has a test which goes green, still applies cleanly and fixes the issue for me. A form with '#token' => FALSE does not work without the patch and does work with the patch.

stefan.r’s picture

Issue tags: +Pending Drupal 7 commit, +Drupal bugfix target
pfrenssen’s picture

Issue tags: -needs backport to 7.x

Great to see this still getting some love. I've needed this patch in countless D7 sites, mostly to get my anonymous search forms working.

Fabianx’s picture

Assigned: Unassigned » Fabianx

We will need a tests-only patch to prove that the test tests the right thing (it might not as the user is anonymous).

stefan.r’s picture

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 47: 1617918-testonly.patch, failed testing.

Fabianx’s picture

Status: Needs work » Reviewed & tested by the community
pfrenssen’s picture

Assigned: Fabianx » Unassigned

Awesome, thanks Stefan!

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 47: 1617918-testonly.patch, failed testing.

Fabianx’s picture

Status: Needs work » Reviewed & tested by the community

Thanks, DrupalCI. That is what we want :).

Fabianx’s picture

Status: Reviewed & tested by the community » Fixed
Issue tags: -Pending Drupal 7 commit, -Drupal bugfix target

Committed and pushed to 7.x! Thanks!

  • Fabianx committed 3677006 on 7.x
    Issue #1617918 by pfrenssen, stefan.r, Ayesh, Fabianx, rfay, tea.time: $...
David_Rothstein’s picture

FYI, Drupal 7.53 wound up just being a small bugfix release (to fix a single regression from Drupal 7.51) so this issue was not included there. It will be in the next non-security release instead.

Status: Fixed » Closed (fixed)

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