Right now, there are two unsafe places to take actions (form builder, form validate handler) and one safe place (form submit handler). If we would abort the rest of the form validation after the token validation fails, we would automatically protect our validation handlers from CSRF as well, and we'd have TWO safe places, one unsafe.
The arguments for continuing validation are not strong; we can show errors on more fields. Okay, but the user will not be able to submit anyway, as the token is incorrect.
Comment | File | Size | Author |
---|---|---|---|
#19 | token-validation-fapi-576276-19.patch | 3.71 KB | tim.plunkett |
Comments
Comment #1
sun+1
Comment #2
Heine CreditAttribution: Heine commentedA bit late in the game now, imo :)
Comment #3
sunHeine, I've incorporated this change into my patch for #240828: "This form is outdated. Reload the page and try again" is incorrect and confuses users
Comment #4
gregglesI don't think the patch that made it in via #240828: "This form is outdated. Reload the page and try again" is incorrect and confuses users included a change to address this, right?
Comment #5
Heine CreditAttribution: Heine commentedThis has been changed in 7.x and 6.x with SA-CORE-2013-003.
Comment #6
larowlanForward port of fix from SA
Comment #7
larowlanComment #8
larowlanComment #10
scor CreditAttribution: scor commentedComment #11
amateescu CreditAttribution: amateescu commentedSA followups are critical, marking as such.
Comment #12
tim.plunkettOnce #2131851: Form errors must be specific to a form and not a global is in this could be a nice unit test.
I have no other complaints otherwise.
Comment #13
tim.plunkettI was looking at FormBuilder tonight and noticed this:
So maybe that is a better option here?
Comment #14
larowlanWorks for me
Comment #15
tim.plunkettThere are some issues with the current unit tests that shouldn't hold up this one, leaving the simpletest in place and making the #token => FALSE change.
Comment #17
larowlan15: token-validation-fapi-576276-15.patch queued for re-testing.
Comment #19
tim.plunkettOkay, let's do that separately as well: #2143349: Submitting a form as an anonymous user when $form['#token'] = FALSE results in a notice
Comment #20
larowlanThen we're good
Comment #21
catchCommitted/pushed to 8.x, thanks!