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()
?
Comment | File | Size | Author |
---|---|---|---|
#32 | form_token-1191278-25-32.interdiff.txt | 2.76 KB | dokumori |
#32 | form_token-1191278-32.patch | 13.56 KB | dokumori |
Comments
Comment #1
donquixote CreditAttribution: donquixote commentedBtw, if you want to read about csrf, here is a link.
https://www.owasp.org/index.php/Cross-Site_Request_Forgery_(CSRF)_Prevention_Cheat_Sheet
Comment #2
donquixote CreditAttribution: donquixote commentedIt seems the views exposed form does not remove the token at all. Instead, it does a redirect.
Is this necessary?
Comment #3
tmckeown CreditAttribution: tmckeown commentedThis 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
Comment #4
pwolanin CreditAttribution: pwolanin commentedBumping 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.
Comment #5
pwolanin CreditAttribution: pwolanin commentedHere'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.
Comment #6
tim.plunkettWe have unit tests for FormBuilder, we should add coverage for this.
Comment #9
dawehnerLet'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:
This patch also uses the information in views.
Comment #10
pwolanin CreditAttribution: pwolanin commented@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?
Also, this doesn't make total sense - there is no token to controll access to if $form['#token'] === FALSE
I don't think it makes sense to leave them in the form (and spend the effort building them just to hide them)?
Comment #11
pwolanin CreditAttribution: pwolanin commentedMerging 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.
Comment #12
pwolanin CreditAttribution: pwolanin commentedWith test fixes
Comment #13
jhodgdonComing 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"?
Comment #14
tim.plunkettGuess 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.Comment #15
jhodgdonIt 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?
Comment #16
pwolanin CreditAttribution: pwolanin commented@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.
Comment #17
jhodgdonRE #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?
Comment #18
jhodgdonAnd I still say the code is *very* convoluted as it is.
Comment #19
pwolanin CreditAttribution: pwolanin commented@jhogdon - probably we could get rid of always_process, though perhaps you'd want to have a form that didn't process?
Comment #20
pwolanin CreditAttribution: pwolanin commentedYes, 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.
Comment #21
jhodgdonOK, I went and looked up FormBuilderInterface::buildForm(), which is where $form_state['always_process'] is documented:
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?
Comment #22
donquixote CreditAttribution: donquixote commentedIf I understand correctly, there are 3 cases to consider:
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.
Example: Main search field on a page, or a views exposed form.
Example: Search form in a block that leads to a search page.
Is this correct?
Comment #23
tim.plunkett$form[#token] = FALSE is rather internal, and I would prefer to avoid it and use the documented $form_state['always_process']
Comment #24
pwolanin CreditAttribution: pwolanin commented@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.
Comment #25
jhodgdonpwolanin: 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. :)
Comment #26
LuxianUpdated 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?
Comment #27
jhodgdonI don't understand why in the processForm() method, these lines were completely removed:
and these lines were added:
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 .:
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).
Comment #28
pwolanin CreditAttribution: pwolanin commented@jhodgdon - if we are ignoring the token and build ID, there is not need to populate them into the form values.
Comment #29
jhodgdon@pwolanin - right, but aren't we only ignoring them if #clean_get is used?
Comment #30
jhodgdonThis 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.
Comment #31
pwolanin CreditAttribution: pwolanin commentedYes, 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
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:
Comment #32
dokumori CreditAttribution: dokumori commentedThe 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.
Comment #33
pwolanin CreditAttribution: pwolanin commentedwe'll need a follow-up documentation issue filed at https://drupal.org/project/documentation
Comment #34
jhodgdonWhat is the follow-up issue for, to update the FAPI reference? And thanks for the explanation in #31.
Comment #35
pwolanin CreditAttribution: pwolanin commentedYes, a follow-up to update the FAPI reference docs in git
Comment #36
Nick_vhReviewed 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.
Comment #37
sunAs 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.
Comment #38
pwolanin CreditAttribution: pwolanin commented@sun - it touches it, but I'm pretty sure it didn't break any of it. Tests were added, but not removed.
Comment #39
effulgentsia CreditAttribution: effulgentsia commentedI 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.Comment #40
pwolanin CreditAttribution: pwolanin commentedFor 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.
Comment #41
fagoI'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?
Comment #42
pwolanin CreditAttribution: pwolanin commentedPer 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.
Comment #43
fagoBut 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.
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.
Comment #44
effulgentsia CreditAttribution: effulgentsia commentedI 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.
Comment #45
effulgentsia CreditAttribution: effulgentsia commentedPer #1366020-99: Overhaul SearchQuery; make search redirects use GET query params for keywords.
Comment #46
jhodgdon#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).
Comment #47
ufku CreditAttribution: ufku commentedAs a workaround i use the below code in hook_form_alter:
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.
Comment #56
andypostStill makes sense but as feature (API addition)