Scope for Drupal 8:
This adds test coverage to ensure that the following bug that is currently affecting Drupal 7 will not regress in Drupal 8 and later.
The bug:
In Drupal 7 it is currently impossible to submit a form as an anonymous user when the CSRF tokens are disabled. When such a form is submitted the following validation error is always shown:
The form has become outdated. Copy any unsaved work in the form below and then reload this page.
Original report by tea.time
I wanted to bypass token-based validation in a custom form, so I followed the note in drupal_prepare_form():
// Form constructors may explicitly set #token to FALSE when cross site
// request forgery is irrelevant to the form, such as search forms.
... and set $form['#token'] = FALSE
in my form constructor function.
However, drupal_prepare_form() doesn't touch #token at all if the user is anonymous, so it remains set with the value of FALSE and then drupal_validate_form() passes that to drupal_valid_token(), which obviously fails every time. This was quite a surprising fallout until I read the code again carefully.
I understand from the comments in the code that #token-based validation is only intended for authenticated users. In most cases, then, the problem for anonymous users could be avoided by instead doing:
global $user;
if ($user->uid > 0) $form['#token'] = FALSE;
----------------------------------------------------------------------------------------
I realize this could be working as designed; so, I suggest either of the following:
1. Add a warning in either the API docs or code comments for drupal_prepare_form() that $form['#token'] = FALSE consistently breaks validation for anon users (and therefore a check must be made somehow).
2. Adjust drupal_validate_form() to check for this case and therefore make $form['#token'] = FALSE a non-breaking/functional option for anon users as well, by changing the line:
if (isset($form['#token'])) {
to
if (isset($form['#token']) && $form['#token'] !== FALSE) {
Beta phase evaluation
Issue category | Task because it adds missing test coverage. |
---|---|
Issue priority | Normal because it adds test coverage for D8 for an issue which currently only affects D7. |
Unfrozen changes | Unfrozen because it only changes tests. |
Disruption | Not disruptive |
Comment | File | Size | Author |
---|---|---|---|
#47 | 1617918-testonly.patch | 2.21 KB | stefan.r |
#47 | 1617918-33-d7.patch | 3.54 KB | stefan.r |
#36 | 1617918-33-d7.patch | 3.54 KB | Ayesh |
#33 | interdiff-d7.txt | 1.88 KB | pfrenssen |
#33 | 1617918-33-d7-do-not-test.patch | 3.54 KB | pfrenssen |
Comments
Comment #1
pfrenssenThis is a bug. I can confirm this problem and the proposed solution is valid. It is even easier if you use the
empty()
function, like this:This needs to be fixed in 8.x first and then backported to 7.x. Here's a patch for 8.x.
Comment #2
pfrenssenComment #4
pfrenssen#1: 1617918-1-form-anonymous_no_token.patch queued for re-testing.
Comment #5
pfrenssenNeeds a test
Comment #6
oriol_e9g#1: 1617918-1-form-anonymous_no_token.patch queued for re-testing.
Comment #8
pfrenssenThe bots are a bit cranky :)
Comment #9
c960657 CreditAttribution: c960657 commentedI agree with the fix. Successfully tested in D7.
Comment #10
pfrenssen#1: 1617918-1-form-anonymous_no_token.patch queued for re-testing.
Comment #11
Heine CreditAttribution: Heine commentedProbably best to remove the unset in drupal_prepare_form and just check for === FALSE, right?
Comment #12
rfayI *think* that the documentation could also just be changed to say that $form['#token'] should be set to NULL if you want to bypass validation. Would that be correct?
Comment #13
Fabianx CreditAttribution: Fabianx commentedThis still needs tests.
Comment #14
rfayBTW, the problem in our case was submitting the form via drupal_form_submit(), which is in fact incompatible with $form['#token'] = FALSE. What we did in this case (thanks to @fabian) was in the formbuilder function:
Comment #15
pfrenssenSee also #2143349: Submitting a form as an anonymous user when $form['#token'] = FALSE results in a notice.
Comment #16
TravisCarden CreditAttribution: TravisCarden commentedComment #17
pfrenssenHit this problem again on a D7 site today. I rolled the patch for D7 and provided a test.
Temporarily setting the version to D7 so the patch can be tested. This still needs a test for D8.
Comment #18
pfrenssenComment #20
Fabianx CreditAttribution: Fabianx commentedRTBC for 7.x, back to 8.x for adding tests to the fix
Comment #21
pfrenssenThis needs to be rerolled, both for D7 since 7.39 touched this code, and for D8 since it has been refactored a dozen times since this patch was originally posted.
I'll take care of both rerolls, and will also write the test for D8.
Comment #22
pfrenssenHere is a rerolled patch for Drupal 7.
Comment #27
pfrenssenOn Drupal 8 the bug no longer occurs in the meanwhile. It was fixed in #2143349: Submitting a form as an anonymous user when $form['#token'] = FALSE results in a notice, but it wasn't realized in that issue that this actually prevents tokenless forms from working, it was just considered to be a minor fix for a notice. As part of that issue a random existing test was chosen (
ElementsTableSelectTest
) and had the access token removed from it to prove that the notice doesn't occur any more.I think would be useful to have an actual dedicated test in Drupal 8 that checks that a form without a token can be submitted. There is always a possibility that
ElementsTableSelectTest
will be refactored in the future. Having a dedicated test will ensure that we will cover this for the future.I ported the test for Drupal 8 in this patch.
Rewording title to make it clear that for Drupal 8 this is just adding test coverage.
Comment #30
stefan.r CreditAttribution: stefan.r commentedLooks good, yay for extra test coverage! I guess this needs a beta evaluation template?
unnecessary
t()
:)Comment #31
pfrenssenGood catch with the stray
t()
, this will also be needed in the D7 version of the patch. Leaving at RTBC since this is a minor nitpick.Also added beta evaluation.
Comment #32
pfrenssenComment #33
pfrenssenUpdated Drupal 7 version of the patch:
t()
.Sorry for the confusion but I need to get this patch out so I can include it in my make file for D7. Uploading patch as do-not-test so I can leave the current issue status.
People looking for the D8 patch, see #31 :)
Comment #34
alexpottCommitted 1e1a38b and pushed to 8.0.x. Thanks!
Thanks for adding the beta evaluation to the issue summary.
Comment #36
Ayesh CreditAttribution: Ayesh commentedThanks for the 7.x patch. It works great. Tests may be?
Comment #37
pfrenssen@Ayesh, thanks for re-uploading the D7 patch. Setting to "Needs review" so that it can be tested by the bot.
Comment #38
pfrenssenRestoring original issue title for D7.
Comment #39
pfrenssenComment #43
kristiaanvandeneyndePatch has a test which goes green, still applies cleanly and fixes the issue for me. A form with
'#token' => FALSE
does not work without the patch and does work with the patch.Comment #44
stefan.r CreditAttribution: stefan.r commentedComment #45
pfrenssenGreat to see this still getting some love. I've needed this patch in countless D7 sites, mostly to get my anonymous search forms working.
Comment #46
Fabianx CreditAttribution: Fabianx as a volunteer commentedWe will need a tests-only patch to prove that the test tests the right thing (it might not as the user is anonymous).
Comment #47
stefan.r CreditAttribution: stefan.r commentedAnd a test-only patch
Comment #49
Fabianx CreditAttribution: Fabianx as a volunteer commentedComment #50
pfrenssenAwesome, thanks Stefan!
Comment #52
Fabianx CreditAttribution: Fabianx as a volunteer commentedThanks, DrupalCI. That is what we want :).
Comment #53
Fabianx CreditAttribution: Fabianx as a volunteer commentedCommitted and pushed to 7.x! Thanks!
Comment #55
David_Rothstein CreditAttribution: David_Rothstein as a volunteer commentedFYI, Drupal 7.53 wound up just being a small bugfix release (to fix a single regression from Drupal 7.51) so this issue was not included there. It will be in the next non-security release instead.