Issue #2571995 by borisson_, Wim Leers: GET forms shouldn't have CSRF tokens by default

core/lib/Drupal/Core/Form/FormBuilder.php

@@ -679,6 +679,15 @@ class FormBuilder implements FormBuilderInterface, FormValidatorInterface, FormS
       $form['#method'] = 'get';
     }
 
+    // GET forms should not use a CSRF token.
+    if (isset($form['#method']) && $form['#method'] === 'get') {
+      // Merges in a default, this means if you've explicitly set #token to the
+      // the $form_id on a GET form, which we don't recommend, it will work.
+      $form += [
+        '#token' => FALSE,
+      ];
+    }
+
     // Generate a new #build_id for this form, if none has been set already.
     // The form_build_id is used as key to cache a particular build of the form.
     // For multi-step forms, this allows the user to go back to an earlier

core/modules/search/src/Form/SearchBlockForm.php

@@ -89,7 +89,6 @@ class SearchBlockForm extends FormBase {
 
     $route = 'search.view_' . $entity_id;
     $form['#action'] = $this->url($route);
-    $form['#token'] = FALSE;
     $form['#method'] = 'get';
 
     $form['keys'] = array(

core/modules/system/src/Tests/Form/FormTest.php

@@ -294,6 +294,18 @@ class FormTest extends WebTestBase {
     $this->assertFieldByName('url', $edit['url']);
   }
 
+  /**
+   * CSRF tokens for GET forms should not be added by default.
+   */
+  public function testGetFormsCsrfToken() {
+    // We need to be logged in to have CSRF tokens.
+    $account = $this->createUser();
+    $this->drupalLogin($account);
+
+    $this->drupalGet(Url::fromRoute('form_test.get_form'));
+    $this->assertNoRaw('form_token');
+  }
+
   /**
    * Tests validation for required textfield element without title.
    *

core/modules/system/tests/modules/form_test/form_test.routing.yml

@@ -473,3 +473,10 @@ form_test.form_storage_page_cache:
     _title: 'Form storage with page cache test'
   requirements:
     _access: 'TRUE'
+
+form_test.get_form:
+  path: '/form-test/get-form'
+  defaults:
+    _form: '\Drupal\form_test\Form\FormTestGetForm'
+  requirements:
+    _access: 'TRUE'

core/modules/system/tests/modules/form_test/src/Form/FormTestGetForm.php

@@ -0,0 +1,44 @@
+<?php
+
+/**
+ * @file
+ * Contains \Drupal\form_test\Form\FormTestGetForm.
+ */
+
+namespace Drupal\form_test\Form;
+
+use Drupal\Core\Form\FormBase;
+use Drupal\Core\Form\FormStateInterface;
+
+/**
+ * Form to test whether GET forms have a CSRF token.
+ */
+class FormTestGetForm extends FormBase {
+
+  /**
+   * {@inheritdoc}
+   */
+  public function getFormId() {
+    return 'form_test_get_form';
+  }
+
+  /**
+   * {@inheritdoc}
+   */
+  public function buildForm(array $form, FormStateInterface $form_state) {
+    $form['#method'] = 'get';
+    $form['submit'] = [
+      '#type' => 'submit',
+      '#value' => 'Save',
+    ];
+    return $form;
+  }
+
+  /**
+   * {@inheritdoc}
+   */
+  public function submitForm(array &$form, FormStateInterface $form_state) {
+    drupal_set_message('The form_test_get_form form has been submitted successfully.');
+  }
+
+}

core/modules/views/src/Tests/Plugin/ExposedFormTest.php

@@ -232,7 +232,6 @@ class ExposedFormTest extends ViewTestBase {
       'entity_test_view_grants',
       'theme',
       'url.query_args',
-      'user.roles:authenticated',
       'languages:language_content'
     ];
 

core/tests/Drupal/Tests/Core/Form/FormBuilderTest.php

@@ -773,7 +773,7 @@ class FormBuilderTest extends FormTestBase {
    *
    * @dataProvider providerTestFormTokenCacheability
    */
-  function testFormTokenCacheability($token, $is_authenticated, $expected_form_cacheability, $expected_token_cacheability) {
+  public function testFormTokenCacheability($token, $is_authenticated, $expected_form_cacheability, $expected_token_cacheability, $method) {
     $user = $this->prophesize(AccountProxyInterface::class);
     $user->isAuthenticated()
       ->willReturn($is_authenticated);
@@ -782,6 +782,7 @@ class FormBuilderTest extends FormTestBase {
 
     $form_id = 'test_form_id';
     $form = $form_id();
+    $form['#method'] = $method;
 
     if (isset($token)) {
       $form['#token'] = $token;
@@ -797,7 +798,7 @@ class FormBuilderTest extends FormTestBase {
 
     $form_state = new FormState();
     $built_form = $this->formBuilder->buildForm($form_arg, $form_state);
-    if (!isset($expected_form_cacheability)) {
+    if (!isset($expected_form_cacheability) || ($method == 'get' && !is_string($token))) {
       $this->assertFalse(isset($built_form['#cache']));
     }
     else {
@@ -820,10 +821,12 @@ class FormBuilderTest extends FormTestBase {
    */
   function providerTestFormTokenCacheability() {
     return [
-      'token:none,authenticated:true' => [NULL, TRUE, ['contexts' => ['user.roles:authenticated']], ['max-age' => 0]],
+      'token:none,authenticated:true' => [NULL, TRUE, ['contexts' => ['user.roles:authenticated']], ['max-age' => 0], 'post'],
-      'token:false,authenticated:true' => [FALSE, TRUE, NULL, NULL],
+      'token:none,authenticated:false' => [NULL, FALSE, ['contexts' => ['user.roles:authenticated']], NULL, 'post'],
-      'token:none,authenticated:false' => [NULL, FALSE, ['contexts' => ['user.roles:authenticated']], NULL],
+      'token:false,authenticated:false' => [FALSE, FALSE, NULL, NULL, 'post'],
-      'token:false,authenticated:false' => [FALSE, FALSE, NULL, NULL],
+      'token:false,authenticated:true' => [FALSE, TRUE, NULL, NULL, 'post'],
+      'token:none,authenticated:false,method:get' => [NULL, FALSE, ['contexts' => ['user.roles:authenticated']], NULL, 'get'],
+      'token:test_form_id,authenticated:false,method:get' => ['test_form_id', TRUE, ['contexts' => ['user.roles:authenticated']], ['max-age' => 0], 'get'],
     ];
   }