Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Problem/Motivation
In #2463567: Push CSRF tokens for forms to placeholders + #lazy_builder, we've found that there were GET forms with CSRF tokens.
A CSRF token is only useful when changing data, and changing state during a GET request is something that the HTTP spec says SHOULD NOT be done. (#2502785: Remove support for $form_state->setCached() for GET requests)
Proposed resolution
Make sure that GET forms never have CSRF tokens by setting $form['#token'] = FALSE;
in the form builder.
Remaining tasks
User interface changes
API changes
Data model changes
Comment | File | Size | Author |
---|---|---|---|
#30 | get_forms_shouldn_t-2571995-30.patch | 7.19 KB | borisson_ |
|
Comments
Comment #2
borisson_Attached a patch that will make sure GET forms never have a CSRF token. Also removed setting the token explicitly to false in
SearchBlockForm
andElementsTableSelectTest
.Comment #4
borisson_ElementsTableSelectTest
was a post form, I made an oopsie.Comment #8
borisson_Comment #9
borisson_Comment #10
Wim LeersWe should prevent GET forms from having CSRF tokens and add test coverage that verifies that.
Comment #11
borisson_Added tests.
Comment #14
borisson_So, I added a new form in the test in #13. I didn't add it to the patch though, I forgot. This is fixed now.
Comment #15
dawehnerIdeally we should check whether $form['#token'] was overridden by the form itself
Comment #16
XanoOr just merge in a default:
Comment #17
borisson_Comment #18
borisson_Comment #19
mr.baileysRegarding the security aspect of not providing CSRF protection on GET requests:
From OWASP Cross-Site Request Forgery (CSRF) Prevention Cheat Sheet:
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
Comment #20
catchIf 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.
Comment #21
Fabianx CreditAttribution: Fabianx as a volunteer commentedAgree with #20!
Comment #22
attiks CreditAttribution: attiks at Attiks commentedSmall nitpick on the wording of the comment.
GET forms should not use a CSRF Tokens.
Comment #23
mr.baileysSmall nitpick on #23
GET forms should not use a CSRF token.
Comment #24
borisson_Attached patch fixes #15 by implementing #16. It also fixes #22, #23. The solution mentioned in #16 also fixes #19.1
Comment #25
Wim LeersTwo periods, should be one.
"you've […] have" -> two "haves".
s/Tokens/tokens/
And "not added by default".
We don't prevent it, they're just not there by default.
Form to test whether GET forms have a CSRF token.
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.
Comment #26
Wim LeersNW for #25.6.
Comment #27
Wim LeersFrom the DrupalCon Barcelona Hard Problems Meeting on performance:
So, this issue is step 1. I've just opened an issue for step 2: #2575429: GET forms MUST NOT have CSRF tokens.
Comment #28
Wim LeersPer #19.
Comment #30
borisson_Comment #31
Wim LeersSplendid, thanks!
Comment #33
catchCommitted/pushed to 8.0.x, thanks!
See you in #2575429: GET forms MUST NOT have CSRF tokens.
Comment #34
borisson_This also unblocks #2463567: Push CSRF tokens for forms to placeholders + #lazy_builder.
Comment #36
Wim LeersPIFR, you so slow.
Comment #37
Wim LeersThis has also unblocked #2575429: GET forms MUST NOT have CSRF tokens.