Problem/Motivation

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, step 1 is #2571995: GET forms shouldn't have CSRF tokens by default, this is step 2.

Proposed resolution

Prevent GET forms from ever having a CSRF token.

Remaining tasks

  1. Investigate
  2. If conclusion supports our hypothesis, implement

User interface changes

None.

API changes

None.

Data model changes

None.

CommentFileSizeAuthor
#11 2575429-11.patch2.67 KBDanny.Wouters
#11 interdiff-2575429-4-11.txt1.65 KBDanny.Wouters
#5 2575429-4.patch1.24 KBkylebrowning

Issue fork drupal-2575429

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

Wim Leers created an issue. See original summary.

catch’s picture

catch
he/him
Primary language English
commented

So one thing to double check here would be whether there's a difference between submitting the form and just visiting the URL - i.e. what happens with validate/submit handlers in those cases.

borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
commented
Status: Postponed » Needs work

Now that #2571995: GET forms shouldn't have CSRF tokens by default is in, this is no longer postponed.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Needs work » Active

No patch yet, so setting to Active.

kylebrowning’s picture

kylebrowning commented
Status: Active » Needs review
StatusFileSize
new 2575429-4.patch1.24 KB

SO im not sure I understand why this wasn't done in the first place, but heres the first run at a patch, and probably will fail tests.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented 
+++ b/core/lib/Drupal/Core/Form/FormBuilder.php
@@ -546,9 +546,6 @@ public function processForm($form_id, &$form, FormStateInterface &$form_state) {
-      if (!isset($input['form_token']) && isset($form['#token'])) {
-        $input['form_token'] = $this->csrfToken->get($form['#token']);
-      }

Eh… isn't this going to remove CSRF tokens for all forms? :P

kylebrowning’s picture

kylebrowning commented

No because its inside this, 

<?php
  // With GET, these forms are always submitted if requested.
    if ($form_state->isMethodType('get') && $form_state->getAlwaysProcess()) {
?>

Status: Needs review » Needs work

The last submitted patch, 5: 2575429-4.patch, failed testing.

The last submitted patch, 5: 2575429-4.patch, failed testing.

Danny.Wouters’s picture

Danny.Wouters commented
Assigned: Unassigned » Danny.Wouters
Issue tags: +DUGBE2410

I will try to find a solution.

Danny.Wouters’s picture

Danny.Wouters commented
Status: Needs work » Needs review
StatusFileSize
new interdiff-2575429-4-11.txt1.65 KB
new 2575429-11.patch2.67 KB

I added extra checks on the form method.

Status: Needs review » Needs work

The last submitted patch, 11: 2575429-11.patch, failed testing.

Danny.Wouters’s picture

Danny.Wouters commented
Assigned: Danny.Wouters » Unassigned

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

joachim’s picture

joachim commented
Version: 8.1.x-dev » 8.2.x-dev

Search module has to do hook_form_alter() on its own form because of this, so that can be cleaned up as part of this:

/**
 * Implements hook_form_FORM_ID_alter() for the search_block_form form.
 *
 * Since the exposed form is a GET form, we don't want it to send the form
 * tokens. However, you cannot make this happen in the form builder function
 * itself, because the tokens are added to the form after the builder function
 * is called. So, we have to do it in a form_alter.
 *
 * @see \Drupal\search\Form\SearchBlockForm
 */
function search_form_search_block_form_alter(&$form, FormStateInterface $form_state) {
  $form['form_build_id']['#access'] = FALSE;
  $form['form_token']['#access'] = FALSE;
  $form['form_id']['#access'] = FALSE;
}

Should removing the form_id and form_build_id be handled in this issue too, or as a follow-up?

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.0-beta1 was released on August 3, 2016, which means new developments and disruptive changes should now be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.0-alpha1 will be released the week of July 31, 2017, which means new developments and disruptive changes should now be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.0-alpha1 will be released the week of January 17, 2018, which means new developments and disruptive changes should now be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.6.x-dev » 8.7.x-dev

Drupal 8.6.0-alpha1 will be released the week of July 16, 2018, which means new developments and disruptive changes should now be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.7.x-dev » 8.8.x-dev

Drupal 8.7.0-alpha1 will be released the week of March 11, 2019, which means new developments and disruptive changes should now be targeted against the 8.8.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.0-alpha1 will be released the week of October 14th, 2019, which means new developments and disruptive changes should now be targeted against the 8.9.x-dev branch. (Any changes to 8.9.x will also be committed to 9.0.x in preparation for Drupal 9’s release, but some changes like significant feature additions will be deferred to 9.1.x.). For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 8.9.x-dev » 9.1.x-dev

Drupal 8.9.0-beta1 was released on March 20, 2020. 8.9.x is the final, long-term support (LTS) minor release of Drupal 8, which means new developments and disruptive changes should now be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 9.1.x-dev » 9.2.x-dev

Drupal 9.1.0-alpha1 will be released the week of October 19, 2020, which means new developments and disruptive changes should now be targeted for the 9.2.x-dev branch. For more information see the Drupal 9 minor version schedule and the Allowed changes during the Drupal 9 release cycle.

Version: 9.2.x-dev » 9.3.x-dev

Drupal 9.2.0-alpha1 will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.0-rc1 was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.0-alpha1 was released on May 6, 2022, which means new developments and disruptive changes should now be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.5.x-dev » 10.1.x-dev

Drupal 9.5.0-beta2 and Drupal 10.0.0-beta2 were released on September 29, 2022, which means new developments and disruptive changes should now be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 10.1.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

quietone’s picture

quietone commented
Issue tags: -DrupalCon Barcelona 2015 +Barcelona2015

bhanu951 made their first commit to this issue’s fork.

bhanu951 opened merge request !11231

Version: 11.x-dev » main

Drupal core is now using the main branch as the primary development branch. New developments and disruptive changes should now be targeted to the main branch.

Read more in the announcement.