Push CSRF tokens for forms to placeholders + #lazy_builder

Taking this on.

Super hacky super rough initial version.

This causes an error upon submitting a form: The form has become outdated. Copy any unsaved work in the form below and then reload this page.

We'll need to update the form building/tracking of the token in form state stuff too. But I've mostly forgotten about Form API's internals. (i.e. why it works the way it works…) Pointers are most welcome, so I don't need to spend hours re-learning FAPI.

The (majority of the) 199 failures are caused by the tricky way GET forms' input processing is handled by the form builder: their input is only processed in certain scenarios, and because Views happens to $form['form_token']['#access'] = FALSE, Views' exposed filters (GET) form happens to not be one of the scenarios where processing happens, causing these test failures.

This was @effulgentsia's clever work-around :)

Pair-programmed with @effulgentsia.

The remaining failures are caused because the "form_token" is in a different format than expected: <input value="{value}" type="hidden" name="form_token" /> instead of <input type="hidden" name="form_token" value="{value}">

The attached patch fixes the test for QuickEditAutocompleteTermTest so it uses the actual format of the input. I'm pretty sure this is a bad way of fixing this.

There are 2 options:
- Make the regex more flexibile, so it doesn't matter in what order the attributes of the input are.
- Fix the actual input to have the expected order of attributes.

What is the preferred way forward?

Now doing exactly this for links: #2512132: Make CSRF links cacheable. Postponing this on that.

#2512132: Make CSRF links cacheable landed. We can apply the same solution.

The implementation in the patch in #7 was using #post_render and that didn't work anymore.

I've changed this to use a #lazy_builder callback, this is till WIP and doesn't work yet.

1. +++ b/core/lib/Drupal/Core/Form/FormBuilder.php
   @@ -657,15 +675,22 @@ public function prepareForm($form_id, &$form, FormStateInterface &$form_state) {
   +        $placeholder = hash('sha1', $form['#token']);
   

   Just use cre32, this is what we use in other places ...

2. +++ b/core/lib/Drupal/Core/Render/Element/Token.php
   @@ -29,12 +29,18 @@ public function getInfo() {
   +  public static function something($element) {
   

   Do we have a better name for this?

#13

1. In #2512132: Make CSRF links cacheable and #2504139: Blocks containing a form include the form action in the cache, so they always submit to the first URL the form was viewed at we're also using hash('sha1', ...), so for consistency I also went for that here, I can change this if you want.
2. You're right, this should be more descriptive. I went for preRenderSetPlaceholderAsValue

Attached patch doesn't functionally change anything compared to #12, there's only the method rename.

#14.1: Yes, let's change that to crc32(). I discussed that with dawehner; crc32() is sufficient here; we should also update all other cases (i.e. the issue you linked to and \Drupal\Core\Render\Renderer::createPlaceholder() — if you open an issue for the latter, I'll RTBC it :)).

@borisson_: Thanks for taking this one on! When I saw it fly by just now, I immediately though of you, since it's related to that other issue. But clearly you're alredy working on it :P

This should a bunch of the current failures.

I removed too much code in the previous patch.

1. +++ b/core/lib/Drupal/Core/Form/FormBuilder.php
   @@ -603,6 +603,24 @@ public function processForm($form_id, &$form, FormStateInterface &$form_state) {
   +   * #post_render_cache callback; render form csrf token.
   

   s/#post_render_cache/#lazy_builder/
   s/render/renders/
   s/csrf/CSRF/

2. +++ b/core/lib/Drupal/Core/Form/FormBuilder.php
   @@ -603,6 +603,24 @@ public function processForm($form_id, &$form, FormStateInterface &$form_state) {
   +      '#cache' => [
   +        'contexts' => [
   +          'session',
   +        ],
   +        'max-age' => 0
   +      ],
   

   Just the session cache context should be sufficient.

3. +++ b/core/lib/Drupal/Core/Form/FormBuilder.php
   @@ -656,16 +674,23 @@ public function prepareForm($form_id, &$form, FormStateInterface &$form_state) {
   +        $placeholder = crc32($form_id);
   +        $form['#token'] = $placeholder;
   ...
   +        $form['#attached']['placeholders'][$placeholder] = [
   +          '#lazy_builder' => ['form_builder:renderFormTokenPlaceholder', [$placeholder]]
   

   This could use some comments, much like those in #2504139: Blocks containing a form include the form action in the cache, so they always submit to the first URL the form was viewed at.

The test failures are simple: just missing the session cache context :)

This patch contains fixes for #21 (interdiff_comments.txt) and fixes for the tests.

#25

1. fixed
2. fixed
3. I don't think it's needed, let's see what the bot says
4. I don't think so, this is a hidden field. In \Drupal\Core\Render\Element\Token::preRenderSetPlaceholderAsValue this placeholder gets copied to the value.

This also fixes the failures in QuickEditLoadingTest

This makes sure that a session is set so the test can use it, this fixes the last test failure in CommentDefaultFormatterCacheTagsTest

1. +++ b/core/lib/Drupal/Core/Form/FormBuilder.php
   @@ -603,6 +603,23 @@ public function processForm($form_id, &$form, FormStateInterface &$form_state) {
   +   * @param string $path
   +   * @return array
   

   Incomplete docblock.

2. +++ b/core/lib/Drupal/Core/Form/FormBuilder.php
   @@ -656,16 +673,28 @@ public function prepareForm($form_id, &$form, FormStateInterface &$form_state) {
   +        // Generates a placeholder based on the form id.
   

   Nit: s/id/ID/

3. +++ b/core/lib/Drupal/Core/Render/Element/Token.php
   @@ -29,12 +29,18 @@ public function getInfo() {
   +  public static function preRenderSetPlaceholderAsValue($element) {
   

   Needs docblock.

#33

1. Not sure what docblock you mean.
2. Fixed.
3. added a docblock.

1. The one I quoted! Note how it just lists the params without documentation.

+++ b/core/lib/Drupal/Core/Render/Element/Token.php
@@ -36,6 +36,12 @@ public function getInfo() {
+  /**
+   * Sets the Token's #placeholder as the #value.
+   *
+   * @param $element
+   * @return mixed
+   */

This one is also incomplete for the same reason.

(Yes, I know, our docs standards are quite silly for cases like these.)

We still have the "needs tests" issue tag. Do we really need tests for this though? We have a gazillion implicit tests anyway, right? So I think we're fine in that regard.

Updated the docblocks of both methods.

---
I agree, I think we have sufficient implicit tests for this.

1. +++ b/core/lib/Drupal/Core/Form/FormBuilder.php
   @@ -603,6 +603,25 @@ public function processForm($form_id, &$form, FormStateInterface &$form_state) {
   +   * @param string $path a string containing the path we need to get the
   +   *  CSRF token for from the CsrfTokenGenerator.
   +   * @return array
   +   *   A renderable array containing the CSRF token.
   
   +++ b/core/lib/Drupal/Core/Render/Element/Token.php
   @@ -36,6 +37,19 @@ public function getInfo() {
   +   * @param array $element A form element that needs to have it's #value
   +   *  updated.
   +   * @return array
   +   *   The updated form element.
   

   These docblocks are wrong.

   We always have a blank line between the last @param and @return.

   And we never put the documentation for a parameter on the same line, always on a new line.

   See the many thousands of examples in the rest of core :)

2. +++ b/core/lib/Drupal/Core/Form/FormBuilder.php
   @@ -603,6 +603,25 @@ public function processForm($form_id, &$form, FormStateInterface &$form_state) {
   +  public function renderFormTokenPlaceholder($path) {
   ...
   +      '#markup' => $this->csrfToken->get($path),
   

   Hrm, the $path parameter does not actually make sense.

3. +++ b/core/lib/Drupal/Core/Form/FormBuilder.php
   @@ -656,16 +675,28 @@ public function prepareForm($form_id, &$form, FormStateInterface &$form_state) {
   +          '#default_value' => $this->csrfToken->get($placeholder),
   

   Hrm, we're still using the CSRF token generator here. That seems wrong. (I know it is a remnant from my early patch in #5, but that doesn't make it right :))

   @Fabianx already asked for its removal in #25.3, you already did that in #27, but because it caused test failures, you restored it in #30.

   But that just means we need to figure out why #27 failed so badly.

   This is IMHO the biggest blocker.

4. +++ b/core/lib/Drupal/Core/Form/FormBuilder.php
   @@ -656,16 +675,28 @@ public function prepareForm($form_id, &$form, FormStateInterface &$form_state) {
   +          '#placeholder' => $placeholder,
   
   +++ b/core/lib/Drupal/Core/Render/Element/Token.php
   @@ -29,6 +29,7 @@ public function getInfo() {
   +        [$class, 'preRenderSetPlaceholderAsValue'],
   
   @@ -36,6 +37,19 @@ public function getInfo() {
   +  public static function preRenderSetPlaceholderAsValue(array $element) {
   +    $element['#attributes']['value'] = $element['#placeholder'];
   +    return $element;
   +  }
   

   This is the clever work-around invented by @effulgentsia, as I wrote in #5. We need to document why and how this works, because I sure don't understand it (anymore).

#37

1. Remaining docblock should be standard-proof now. :-)
2. It didn't make sense that this was called $path, renamed it, see .3
3. I now better understand the code so this is simplified a little bit.

   This needed to be the CSRF token because previously, this ran after the #lazy_builder and otherwise the CsrfTokenGenerator::validate call returned false.
   It is now removed, as well as the '#placeholder', so the confusion in #25.4 doesn't happen anymore.

4. Removed.

Locally, all forms still work and can be submitted, I'd like to see what happens with the test-suite.

So, the current failures are all entity listings that fail, and as far as I can see, they are failing because of the filters that are on these listings.

If I debug $token and $this->computeToken($seed, $value) in \Drupal\Core\Access\CsrfTokenGenerator::validate then it shows that they aren't the same.

Example:

string 'token: form_token_placeholder_4107625689' (length=40)
string 'computed token: CI2j2pPZ_YeGSTxEHTlenlkdTFM19qXlvyAnc6zn-CM' (length=59)

So, the #lazy_builder hasn't ran yet for the filter form. This is what needs to be solved for the failures to be resolved.

@borisson_ and I went through the code this morning and we discovered a few interesting things:
ExposedFormPluginBase::renderExposedForm() and DisplayPluginBase::elementPreRender() build complete forms using the form builder, while their return values should be embeddable forms.This means there are multiple form token elements in the final form, all of which say their value is stored in FormStateInterface::getValue('form_token'). As form tokens are based on form IDs, all elements have different token values, and as such only one of them can possibly match the token in the form state, causing the other elements to fail validation.

Attached a reroll, also changed the placeholder to be attached to the form_token element, not the parent form build array.

The reason for the test failures is that GET forms unnecessarily received form token elements, but GET forms are validated before rendering/submission, so the CSRF token was never set, because lazy builders are part of the rendering process.

Because GET forms are immutable, which means they don't need CSRF protection, we can safely prevent CSRF token elements from being added to such forms.

I had introduced the changes to QuickEditLoadingTest and QuickEditAutocompleteTermTest earlier in this patch, reverted these back to the state of HEAD and they pass again.

The current approach removes CSRF tokens for get forms completely. Is this a security concern?

If it is, we might be able to also fix the the failures of #42 by reverting the changes in #44 and setting #token to false for exposed filter forms.

I pinged @effulgentsia for review.

Patch no longer applied, this one does, let's see what kind of failures we're getting now.

FormBuilderTest will still fail, looking into fixing that.

I looked into fixing the remaining failure in FormBuilderTest, but I can't figure it out yet, I'll have another look tomorrow.

Rerolled the patch and fixes the FormBuilderTest.

+++ b/core/lib/Drupal/Core/Form/FormBuilder.php
@@ -677,6 +698,7 @@ public function prepareForm($form_id, &$form, FormStateInterface &$form_state) {
+      $form['#token'] = FALSE;

I think we should split this off into a separate issue, and block this issue on that new issue.

That's an API change in line with #2502785: Remove support for $form_state->setCached() for GET requests and fully sensible, but this issue really is only about improving how CSRF tokens are rendered. This change is something else entirely.

Also, if we do this, then we should also remove all manual calls that set $form['#token'] = FALSE in GET forms. E.g. \Drupal\search\Form\SearchBlockForm::buildForm() does this.

postponing on #2571995: GET forms shouldn't have CSRF tokens by default.

Attached patch removes the code quoted in #63.

This will not work before #2571995: GET forms shouldn't have CSRF tokens by default is in.

Moving 'needs security review' tag to #2571995: GET forms shouldn't have CSRF tokens by default.

Now that #2571995: GET forms shouldn't have CSRF tokens by default is in, we can finish this.

Attached patch is the same as in #65.

Rebased.

+++ b/core/lib/Drupal/Core/Form/FormBuilder.php
@@ -725,15 +746,29 @@ public function prepareForm($form_id, &$form, FormStateInterface &$form_state) {
           '#cache' => [
             'max-age' => 0,
           ],

This patch presents a big step forward; it opens the door towards cacheable forms. But let's keep marking forms uncacheable for now, because that requires a broader discussion.

So:

1. Let's get this in; this does not affect security in any way.
2. Let's file a follow-up issue where we try get rid of this max-age=0, but doing so requires further discussion.

Patch looks great, but a couple of nits:

1. +++ b/core/lib/Drupal/Core/Form/FormBuilder.php
   @@ -652,6 +652,27 @@ public function renderPlaceholderFormAction() {
   +   * @param string $placeholder
   +   *  A string containing a placeholder. This could've been a different value,
   +   *  as long as it matches the value of the form's #token.
   

   I don't understand this comment. "could've been different" from what?

2. +++ b/core/lib/Drupal/Core/Form/FormBuilder.php
   @@ -725,15 +746,29 @@ public function prepareForm($form_id, &$form, FormStateInterface &$form_state) {
   +        $placeholder = 'form_token_placeholder_' . crc32($form_id);
   

   Elsewhere we use hash('crc32b') instead of crc32(), possibly due to the portability warning in http://php.net/manual/en/function.crc32.php. Let's switch to that here as well.

Fixes both comments of #74.

PIFR was having MySQL troubles it seems. Re-testing. DrupalCI came back green already though. Back to RTBC.

Thanks! Pushed to 8.0.x.