The csrf form token is a necessary thing for CSRF protection.

However, a csrf token is usually not needed for search and filter forms, that do not trigger any changes on the server.
In such forms, typically implemented with GET, the token just looks ugly on the url.

The views guys managed to somehow remove the token in the views filter form views_exposed_form(). If you submit those filters, there is no ugly token in the url. Unfortunately I was unable to figure out what they did to avoid the token.

There was also a suggestion on stackoverflow, see http://stackoverflow.com/questions/1497298/preventing-form-token-from-re..., setting $form['#token'] = FALSE;. Unfortunately, this solution does not seem to work.

Any ideas? There must be a way...
Maybe use a custom function instead of drupal_get_form() ?

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

donquixote’s picture

donquixote’s picture

It seems the views exposed form does not remove the token at all. Instead, it does a redirect.
Is this necessary?

tmckeown’s picture

This can be a really bad idea. But if you insist, I've updated the stack overflow question with my solution. It's the third answer down.

Cheers,

Trevor

pwolanin’s picture

Title: Disable csrf form token on a GET search / filter form » Optionally disable form_build_id, form_id, and form_token on a GET search / filter form
Version: 6.x-dev » 8.x-dev
Category: Support request » Bug report
Issue summary: View changes

Bumping to 8.x. Related to: #1366020: Overhaul SearchQuery; make search redirects use GET query params for keywords

For a case like search pages, we already strip out these and redirect to a GET request, so it would be more efficient to have the option to skip the redirect all together and yet keep the URL clean.

pwolanin’s picture

Category: Bug report » Feature request
Status: Active » Needs review
FileSize
2.46 KB

Here's a quick 1st pass at enabling this.

a GET method form, with #token and #build_id set to false skip all these hidden values.

tim.plunkett’s picture

Issue tags: +Needs tests

We have unit tests for FormBuilder, we should add coverage for this.

Status: Needs review » Needs work

The last submitted patch, 5: 1191278-5.patch, failed testing.

The last submitted patch, 5: 1191278-5.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
5.84 KB

Let's just hide the rendering, I think for the sake of people altering forms it should be still there.
This patch also removes the checking for the #build_id as it does not make sense to opt out with two keys.
#token is already described with:

// Form constructors may explicitly set #token to FALSE when cross site
 // request forgery is irrelevant to the form, such as search forms.
      if (isset($form['#token']) && $form['#token'] === FALSE) {
        unset($form['#token']);
      }

This patch also uses the information in views.

pwolanin’s picture

Status: Needs review » Needs work

@dawehner - are there cases where wee might have a multi-step form but not want a token? I

This logic looks wrong - you can't ever set a 'get' form?

-    if ($form_state['method'] == 'get' && !isset($form['#method'])) {
+    if (isset($form_state['method']) && $form_state['method'] == 'get' && !isset($form['#method'])) {
       $form['#method'] = 'get';
     }
+    else {
+      $form['#method'] = 'post';
+    }

Also, this doesn't make total sense - there is no token to controll access to if $form['#token'] === FALSE

+    // For certain forms like a search for or exposed filters, we want to make a
+    // clean GET request without hidden values as query parameters.
+    $clean_get = $form['#method'] == 'get' && isset($form['#token']) && $form['#token'] === FALSE;
+    if ($clean_get) {
+      $form['form_build_id']['#access'] = FALSE;
+      $form['form_token']['#access'] = FALSE;
+      $form['form_id']['#access'] = FALSE;
+    }

I don't think it makes sense to leave them in the form (and spend the effort building them just to hide them)?

pwolanin’s picture

Status: Needs work » Needs review
Issue tags: -Needs tests
FileSize
9.03 KB
9.78 KB

Merging Daniel's patch with mine. Still need to clean up the mock user in one of the tests, but this should be good otherwise.

The incremental diff is relative to my 1st patch.

pwolanin’s picture

FileSize
4.59 KB
11.71 KB

With test fixes

jhodgdon’s picture

Coming over here from search module, and having my first look at this patch...

In FormBuilder::prepareForm(), what is $form_state['always_process'] -- I've never seen it before, and have no idea what it means or why this should be enough to say we want a "clean GET"?

tim.plunkett’s picture

Guess what! It came from Views in D7, to make exposed filters work :)
#322344: (notice) Form improvements from Views

That way D7's view_plugin_exposed_form::render_exposed_form() could set 'always_process' => TRUE, for their custom $form_state.

jhodgdon’s picture

It seems like we should either use !empty($form_state['always_process'] or use $form['#token'] === FALSE, but not both, in that case?

Since you set the method and action in $form, I would vote for also setting the "please don't add these stupid hidden form elements" value on $form as well. I'm not sure I love the idea of using $form['#token'] having a FALSE value to do that though. Maybe we could use a different #key that is called something like "no_form_tokens' so you would not have to do all that convoluted setting to false and then checking if it's null etc. that is in this code, which seems really pretty messy?

pwolanin’s picture

@jhodgdon - in general, I agree that this isn't the most intuitive, but setting #token to FALSE has been present for several versions of Drupal, so I didn't want to lightly change that.

jhodgdon’s picture

RE #16, OK, then what's new here is that if #token is FALSE, and it's a GET form, you are now omitting all three of those hidden fields?

Can we get rid of the $form_state['always_process'] thing then, since setting $form[#token] to FALSE is playing the same role, and it was only for Views, and we have Views in Core now?

jhodgdon’s picture

And I still say the code is *very* convoluted as it is.

pwolanin’s picture

@jhogdon - probably we could get rid of always_process, though perhaps you'd want to have a form that didn't process?

pwolanin’s picture

FileSize
1.02 KB
12.51 KB

Yes, this is a bit convoluted. However, here's why the 2 things are different.

For the search block form, we do NOT set always_process. Hence the keywords are NOT taken from ?keys= in the URL and populated into the form when on e.g. /search/node

Views exposed filters forms behave differently.

Here's an updated patch that tries to simplify the always_process code a little.

jhodgdon’s picture

OK, I went and looked up FormBuilderInterface::buildForm(), which is where $form_state['always_process'] is documented:

  *   - always_process: If TRUE and the method is GET, a form_id is not
   *     necessary. This should only be used on RESTful GET forms that do NOT
   *     write data, as this could lead to security issues. It is useful so that
   *     searches do not need to have a form_id in their query arguments to
   *     trigger the search.

Given the current patch, this documentation may need an update, since there are more consequences.

I also don't see documentation for $form[#token] ==> FALSE anywhere (outside of a code comment deep in the FormBuilder class) as being something form building functions can set -- perhaps we should not be doing that, and should use 'always_process' as the trigger that a form building function/method should use? If we're allowing that, we should document it on the FormBuilderInterface somewhere, shouldn't we?

I guess I still really don't understand the distinction of when you want to use #token and when you want to use always_process?

donquixote’s picture

If I understand correctly, there are 3 cases to consider:

  1. GET or POST, regular form validation + redirect, do NOT pre-fill elements with parameters from the request except on failed validation (right?).
    This distinction is only relevant if the same form (that is, a new instance of the same form) shows up after the submit redirect.
    An example is the taxonomy term create form, where after submit you get to the same form page to create yet another term. Or any form in a block, e.g. to send a contact message, where the same block will show up again in the next request.
  2. GET, no form validation or processing on submit, no form build id or token, no submit handler, but pre-fill element values from the request.
    Example: Main search field on a page, or a views exposed form.
  3. GET, no form validation or processing on submit, no form build id or token, no submit handler, do NOT pre-fill element values.
    Example: Search form in a block that leads to a search page.

Is this correct?

tim.plunkett’s picture

$form[#token] = FALSE is rather internal, and I would prefer to avoid it and use the documented $form_state['always_process']

pwolanin’s picture

@tim.plunkett - I've certainly known about #token for many years, so maybe it just needs better docs? Also, as above they do not have the same effect.

We do not want the values processed on the search block form, so $form_state['always_process'] is not the correct thing to use.

however, I'm certainly open to defining another flag on the form if you prefer that instead of suggesting people mess with #token directly.

jhodgdon’s picture

pwolanin: Maybe you could propose the better docs for $form[#token] so we could look at them (in the form of a patch)? I don't know anything about this, and I'm still not clear on what the differences are between the effects of these two ways of doing things. Plus I think the existing docs for the form_state always_process functionality are incomplete and/or innacurate, or at least not understandable because I don't understand them. :)

Luxian’s picture

Updated the patch from #20 to use "#clean_get" instead of #token. We don't have a test for views exposed forms to check for unwanted GET parameters, should we write one?

jhodgdon’s picture

I don't understand why in the processForm() method, these lines were completely removed:

-      if (!isset($form_state['input']['form_build_id'])) {
-        $form_state['input']['form_build_id'] = $form['#build_id'];
-      }
...
-      if (!isset($form_state['input']['form_token']) && isset($form['#token'])) {
-        $form_state['input']['form_token'] = $this->csrfToken->get($form['#token']);
-      }

and these lines were added:

+      $form_state['no_cache'] = TRUE;
+      unset($form['#token']);

I don't think we want these changes done in the case where #clean-get isn't being used, right? Or maybe I'm confused.

The rest of the patch looks good to me. There was one nitpick... one of the test doc block lines doesn't end in .:

+   *
+   * You can explicitly remove form token checking by using #clean_get = TRUE
+   */

Also these lines in the test method doc blocks that say "You can ...", as they are written, are declarative statements about Drupal saying what you can do with the API, when probably they should say "Tests that you can..." (to tell what the method is testing).

pwolanin’s picture

@jhodgdon - if we are ignoring the token and build ID, there is not need to populate them into the form values.

jhodgdon’s picture

@pwolanin - right, but aren't we only ignoring them if #clean_get is used?

jhodgdon’s picture

Priority: Normal » Major

This is holding up a major issue, so I'm upping the priority. Either that or we need to get #1366020: Overhaul SearchQuery; make search redirects use GET query params for keywords reviewed/committed and then patch it again after this issue is ready to go.

pwolanin’s picture

Yes, we should add a test.

@jhodgdon - regarding removing the duplicate/dubious setting of the token, etc, it's rather unrelated to #clean_get, rather it's related to the pre-existing always_process

 if ($form_state['method'] == 'get' && !empty($form_state['always_process'])) {

The existing code is doing a bunch of unneeded work, and the same next logic is achieved by making #token NULL.

This addition can probably be omitted:

+      $form_state['no_cache'] = TRUE
dokumori’s picture

The test 'testPrepareFormGetWithoutTokenChecking' in the previous patch did not specify whether the test was run for anonymous or authenticated user accounts. The test has been modified to run for both anon and auth.

Also made a couple of very minor changes to FormBuilder.php which @pwolanin suggested.

pwolanin’s picture

we'll need a follow-up documentation issue filed at https://drupal.org/project/documentation

jhodgdon’s picture

What is the follow-up issue for, to update the FAPI reference? And thanks for the explanation in #31.

pwolanin’s picture

Yes, a follow-up to update the FAPI reference docs in git

Nick_vh’s picture

Status: Needs review » Reviewed & tested by the community

Reviewed all of the code and it's looking great. The only thing I find a little tricky is our special case when making a clean get request. I just don't like special cases for stuff like this as it adds a certain complexity.

Marking as RTBC.

sun’s picture

Status: Reviewed & tested by the community » Needs review

As this patch touches on some essential Form API security functionality, please give me + other Form API maintainers some time to review and respond.

In other words, pinging @effulgentsia, @fago, @sun, @Frando.

pwolanin’s picture

@sun - it touches it, but I'm pretty sure it didn't break any of it. Tests were added, but not removed.

effulgentsia’s picture

I don't yet see what makes this issue major or what makes it a feature request. It looks like it's taking what can now be accomplished with a hook_form_alter() that sets #access = FALSE on the elements it wants not rendered plus a $form_state['always_process'] = TRUE to allow the form to be processed despite lacking a submission of those element values, and instead adding a new #clean_get property to accomplish more or less the same thing. Maybe that's a DX improvement, but that seems like a normal priority task to me, and not something worth holding up #1366020: Overhaul SearchQuery; make search redirects use GET query params for keywords on. Maybe there's something I'm just not getting though, so I left a comment on #1366020-90: Overhaul SearchQuery; make search redirects use GET query params for keywords.

pwolanin’s picture

For search we don't want always_processed.

This is mostly an improvement to DX (a developer feature) so you can do this in a simple way, which I'd rather see us using for the search use case.

In IRC jhodgdon indicated she's neutral on this, but wants it to get in if it makes fixing the search issue easier. It's mostly major since it's blocking that now.

fago’s picture

I'm not sure I see the point in adding the #clean_get key, in particular as it's not really obvious of what it does for me. I agree it's too hard too make nice GET forms right now though.

So first off, when there is always processed is set to TRUE, I guess it should automatically remove form id, form build id and disable caching. About the form token I'm not so sure as it's security related, I'd prefer keeping that enabled by default and having people disable that explicitly.

For the use case of having a form that creates a clean GET request without processing later on (different action), I can see as adding a flag like #clean_get key plus token. So, what about adding something a 'never_process' option alongside with "always_process", which then results in clean GET and POST requests by default? The documentation can point that out, but having the flag should help making clear what the form is supposed to do.

Given that, we'd have
- never_process for creating the clean_get
- always_process for the form that always processes
- explicit deactivation of security feature #token

The always_process documentation should probably mentioned that one might want to disable #token as well. Or is it even common sense that always processing should skip tokens also?

pwolanin’s picture

Per our discussion in person - perhaps never_process should disable all these hidden elements and could potentially apply to either GET or POST.

always_process I think should only apply to GET and since it already disables the token, I'm not sure we need more?

I can see wanting to keep the form_id for always_process, in case you have more than 1 on a page.

fago’s picture

But what's the point of always process if you have to pass the right form id to be processed? It doesn't sound like always-process then ;) But always_process does already skip form id by default, so it should not be output by default as well.

Per our discussion in person - perhaps never_process should disable all these hidden elements and could potentially apply to either GET or POST.

Yep - that was the I way would have thought of it also. Auto-hide all the stuff added, never process the form + do it that way regardless for the form method.

effulgentsia’s picture

I can see wanting to keep the form_id for always_process, in case you have more than 1 on a page

I agree with #43. 'always_process' can only be used by 1 GET form per action URL. If you use it for two, you'll have bugs, as each form will assume it's the one that the query string is meant for. That is its inherent limitation. However, you can already disable token via $form['#token']. And in a separate issue, perhaps we can make FAPI not output a 'form_build_id' element if the form wasn't cached. In which case, if you want a form that only submits a $form_id and not the other two, then you just make it a form that isn't cached, and set $form['#token'] = FALSE.

effulgentsia’s picture

Title: Optionally disable form_build_id, form_id, and form_token on a GET search / filter form » Simplify DX of removing form_id, form_build_id, and token from RESTful GET forms
Category: Feature request » Task
Priority: Major » Normal
jhodgdon’s picture

Status: Needs review » Needs work

#1366020: Overhaul SearchQuery; make search redirects use GET query params for keywords was committed! So I think this needs an additional piece to apply the fix here to the function search.module :: search_form_search_block_form_alter() and SearchBlockForm (same fix as Views in this patch).

ufku’s picture

As a workaround i use the below code in hook_form_alter:

// Make the query cleaner for GET forms that do not use a token
if (!isset($form['form_token']) && $form['#method'] === 'get') {
  $form['form_id']['#access'] = FALSE;
  $form['form_build_id']['#access'] = FALSE;
}

I guess altering the #access property is a cleaner and more backward-compatible way.
Core can do the same in FormBuilder::prepareForm() before calling alterers.

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.

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

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should 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.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should 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.3.x-dev » 8.4.x-dev

Drupal 8.3.6 was released on August 2, 2017 and is the final full bugfix release for the Drupal 8.3.x series. Drupal 8.3.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.4.0 on October 4, 2017. (Drupal 8.4.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.4.x-dev branch from now on, and new development or disruptive changes should 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.4.x-dev » 8.5.x-dev

Drupal 8.4.4 was released on January 3, 2018 and is the final full bugfix release for the Drupal 8.4.x series. Drupal 8.4.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.5.0 on March 7, 2018. (Drupal 8.5.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.5.x-dev branch from now on, and new development or disruptive changes should 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.5.x-dev » 8.6.x-dev

Drupal 8.5.6 was released on August 1, 2018 and is the final bugfix release for the Drupal 8.5.x series. Drupal 8.5.x will not receive any further development aside from security fixes. Sites should prepare to update to 8.6.0 on September 5, 2018. (Drupal 8.6.0-rc1 is available for testing.)

Bug reports should be targeted against the 8.6.x-dev branch from now on, and new development or disruptive changes should 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.6.x-dev » 8.8.x-dev

Drupal 8.6.x will not receive any further development aside from security fixes. Bug reports should be targeted against the 8.8.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.9.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: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.7 was released on June 3, 2020 and is the final full bugfix release for the Drupal 8.8.x series. Drupal 8.8.x will not receive any further development aside from security fixes. Sites should prepare to update to Drupal 8.9.0 or Drupal 9.0.0 for ongoing support.

Bug reports should be targeted against the 8.9.x-dev branch from now on, and new development or disruptive changes should 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.

andypost’s picture

Version: 8.9.x-dev » 9.2.x-dev
Category: Task » Feature request

Still makes sense but as feature (API addition)

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.