summaryrefslogtreecommitdiffstats
diff options
context:
space:
mode:
authorcatch2015-09-27 09:42:42 +0100
committercatch2015-09-27 09:42:42 +0100
commita416e9a84eeaa45fa3973d1e7184a9b9795dbbe0 (patch)
tree9b7dc5f446027069df030ed3bbacc304327f67a7
parenta1826842d6609a261af6610aebf44e9f116226da (diff)
Issue #2571995 by borisson_, Wim Leers: GET forms shouldn't have CSRF tokens by default
-rw-r--r--core/lib/Drupal/Core/Form/FormBuilder.php9
-rw-r--r--core/modules/search/src/Form/SearchBlockForm.php1
-rw-r--r--core/modules/system/src/Tests/Form/FormTest.php12
-rw-r--r--core/modules/system/tests/modules/form_test/form_test.routing.yml7
-rw-r--r--core/modules/system/tests/modules/form_test/src/Form/FormTestGetForm.php44
-rw-r--r--core/modules/views/src/Tests/Plugin/ExposedFormTest.php1
-rw-r--r--core/tests/Drupal/Tests/Core/Form/FormBuilderTest.php15
7 files changed, 81 insertions, 8 deletions
diff --git a/core/lib/Drupal/Core/Form/FormBuilder.php b/core/lib/Drupal/Core/Form/FormBuilder.php
index adf4c86..fb6ecc0 100644
--- a/core/lib/Drupal/Core/Form/FormBuilder.php
+++ b/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
diff --git a/core/modules/search/src/Form/SearchBlockForm.php b/core/modules/search/src/Form/SearchBlockForm.php
index 2345c80..243c641 100644
--- a/core/modules/search/src/Form/SearchBlockForm.php
+++ b/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(
diff --git a/core/modules/system/src/Tests/Form/FormTest.php b/core/modules/system/src/Tests/Form/FormTest.php
index 389d23e..3ce775d 100644
--- a/core/modules/system/src/Tests/Form/FormTest.php
+++ b/core/modules/system/src/Tests/Form/FormTest.php
@@ -295,6 +295,18 @@ class FormTest extends WebTestBase {
}
/**
+ * 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.
*
* Submits a test form containing a textfield form element without title.
diff --git a/core/modules/system/tests/modules/form_test/form_test.routing.yml b/core/modules/system/tests/modules/form_test/form_test.routing.yml
index a37990b..d8c5833 100644
--- a/core/modules/system/tests/modules/form_test/form_test.routing.yml
+++ b/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'
diff --git a/core/modules/system/tests/modules/form_test/src/Form/FormTestGetForm.php b/core/modules/system/tests/modules/form_test/src/Form/FormTestGetForm.php
new file mode 100644
index 0000000..9bcb55e
--- /dev/null
+++ b/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.');
+ }
+
+}
diff --git a/core/modules/views/src/Tests/Plugin/ExposedFormTest.php b/core/modules/views/src/Tests/Plugin/ExposedFormTest.php
index 5df5465..8906cfd 100644
--- a/core/modules/views/src/Tests/Plugin/ExposedFormTest.php
+++ b/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'
];
diff --git a/core/tests/Drupal/Tests/Core/Form/FormBuilderTest.php b/core/tests/Drupal/Tests/Core/Form/FormBuilderTest.php
index d95e3bc..179b7ba 100644
--- a/core/tests/Drupal/Tests/Core/Form/FormBuilderTest.php
+++ b/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:false,authenticated:true' => [FALSE, TRUE, NULL, NULL],
- 'token:none,authenticated:false' => [NULL, FALSE, ['contexts' => ['user.roles:authenticated']], NULL],
- 'token:false,authenticated:false' => [FALSE, FALSE, NULL, NULL],
+ 'token:none,authenticated:true' => [NULL, TRUE, ['contexts' => ['user.roles:authenticated']], ['max-age' => 0], 'post'],
+ 'token:none,authenticated:false' => [NULL, FALSE, ['contexts' => ['user.roles:authenticated']], NULL, 'post'],
+ 'token:false,authenticated:false' => [FALSE, FALSE, NULL, NULL, 'post'],
+ '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'],
];
}