GET forms shouldn't have CSRF tokens by default

We should prevent GET forms from having CSRF tokens and add test coverage that verifies that.

+++ b/core/lib/Drupal/Core/Form/FormBuilder.php
@@ -679,6 +679,11 @@ public function prepareForm($form_id, &$form, FormStateInterface &$form_state) {
+    // Get forms should not get CSRF Tokens.
+    if (isset($form['#method']) && $form['#method'] === 'get') {
+      $form['#token'] = FALSE;
+    }

Ideally we should check whether $form['#token'] was overridden by the form itself

Or just merge in a default:

$form += [
  '#token' => FALSE,
];

Regarding the security aspect of not providing CSRF protection on GET requests:

From OWASP Cross-Site Request Forgery (CSRF) Prevention Cheat Sheet:

The ideal solution is to only include the CSRF token in POST requests and modify server-side actions that have state changing affect to only respond to POST requests. This is in fact what the RFC 2616 requires for GET requests. If sensitive server-side actions are guaranteed to only ever respond to POST requests, then there is no need to include the token in GET requests.

The problem might be that RFC 2616 refers to this as "convention", meaning that there's bound to be implementations that rely on CSRF protection for GET requests.

I think it is acceptable to remove this feature if

1. properly documented (both in a change record and on Create forms in a safe way to avoid cross-site request forgeries (CSRF);
2. there is a way for developers to still provide CSRF protected on GET requests (either by allowing the default behaviour implemented here to be overridden, or by documenting how to manually add CSRF protection).

If you can hit the GET #action URL directly then the CSRF protection for the form is useless anyway, so I think it's much clearer to remove it.

Agree with #20!

Small nitpick on the wording of the comment.

+++ b/core/lib/Drupal/Core/Form/FormBuilder.php
@@ -679,6 +679,11 @@ public function prepareForm($form_id, &$form, FormStateInterface &$form_state) {
+    // Get forms should not get CSRF Tokens.

GET forms should not use a CSRF Tokens.

Small nitpick on #23

GET forms should not use a CSRF Tokens.

GET forms should not use a CSRF token.

1. +++ b/core/lib/Drupal/Core/Form/FormBuilder.php
   @@ -679,9 +679,13 @@ public function prepareForm($form_id, &$form, FormStateInterface &$form_state) {
   +    // GET forms should not use a CSRF token..
   

   Two periods, should be one.

2. +++ b/core/lib/Drupal/Core/Form/FormBuilder.php
   @@ -679,9 +679,13 @@ public function prepareForm($form_id, &$form, FormStateInterface &$form_state) {
   +      // Merges in a default, this means if you've explicitly have set #token to
   

   "you've […] have" -> two "haves".

3. +++ b/core/modules/system/src/Tests/Form/FormTest.php
   @@ -295,6 +295,18 @@ public function testInputWithInvalidToken() {
   +   * CSRF Tokens for GET forms should not be added.
   

   s/Tokens/tokens/

   And "not added by default".

4. +++ b/core/modules/system/src/Tests/Form/FormTest.php
   @@ -295,6 +295,18 @@ public function testInputWithInvalidToken() {
   +  public function testGetFormsPreventCsrfToken() {
   

   We don't prevent it, they're just not there by default.

5. +++ b/core/modules/system/tests/modules/form_test/src/Form/FormTestGetForm.php
   @@ -0,0 +1,44 @@
   + * Form to test the validation of forms with a disabled CSRF token.
   

   Form to test whether GET forms have a CSRF token.

6. +++ b/core/tests/Drupal/Tests/Core/Form/FormBuilderTest.php
   @@ -820,10 +821,11 @@ function testFormTokenCacheability($token, $is_authenticated, $expected_form_cac
   +      'token:none,authenticated:false,method:get' => [NULL, FALSE, ['contexts' => ['user.roles:authenticated']], NULL, 'get'],
   

   I think we should also have a test case where #token is set to the form ID.

Fixed all nits, which means only the last point still needs to be addressed.

NW for #25.6.

From the DrupalCon Barcelona Hard Problems Meeting on performance:

Wim: GET forms shouldn't have CSRF tokens — https://www.drupal.org/node/2571995
Alex: I have a use case
Crell: CSRF token in the URL is a bad thing, just like a session ID in there is a bad thing
Crell: we should make it an opt-in thing (i.e. default GET forms to #token = FALSE)
Alex: Oh, now I realized that I actually don't have a use case, we found that to be wrong.
Catch: we should verify that it actually offers any protection, if it’s not, then we should not even make it opt-in, we should make it impossible, and document it

So, this issue is step 1. I've just opened an issue for step 2: #2575429: GET forms MUST NOT have CSRF tokens.

Per #19.

Splendid, thanks!

Committed/pushed to 8.0.x, thanks!

See you in #2575429: GET forms MUST NOT have CSRF tokens.

PIFR, you so slow.

This has also unblocked #2575429: GET forms MUST NOT have CSRF tokens.