Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
Problem/Motivation
if (!$this->csrfToken->validate($csrf_token, self::TOKEN_KEY)
&& !$this->csrfToken->validate($csrf_token, 'rest')) {
return AccessResult::forbidden()->setReason('X-CSRF-Token request header is missing')->setCacheMaxAge(0);
}
This message notifies that a token is missing even if one is given that just isn't valid.
Proposed resolution
Provide different messages if a token is provided versus when one is given but its invalid? Just make the message more generic?
Remaining tasks
User interface changes
API changes
Data model changes
Comment | File | Size | Author |
---|---|---|---|
#12 | invalid_csrf_token_feedback-2795965-12.patch | 10.12 KB | Wim Leers |
Comments
Comment #2
neclimdulComment #3
Wim LeersOhhh! Of course :)
Comment #4
Wim LeersThis was not triaged.
Comment #5
Wim LeersAnd patch!
I wanted to assert that the appropriate message was being returned. But … turns out the existing test coverage is not testing that either, because
WTB::assertResponse()
is extremely misleading :(So in the test we were only really asserting we were getting 403 responses. I verified manually at the time (and again today) that this indeed works in real life. But in the test, we were getting not
application/hal+json
responses, but …text/html
responses!You'll never guess the root cause for that one, especially because the correct
DELETE
request works just fine. Well, it turns out that if you don't specify?_format=hal_json
, you don't get the JSON exception subscriber for handling 4xx responses… which means you getDefaultExceptionSubscriber
instead, so you get an HTML response.This was supposed to be trivial, and it is, but the test fixes make it look less trivial. The big test-only patch shows that.
(I thought this patch would take me 4 to 5 minutes. It took me 4 to 5 hours.)
Comment #7
neclimdulSo this fixes testing the original implementation.
And this change lets us test the fixes for this issue?
It looks like forget_xcsrf is no longer an accurate description of this argument... This should probably be fixed but I don't really want to block this quick win. I assume that's why you crammed it in. How did this quickfix get so complicated.
Comment #8
Wim Leers$csrf_token
being: A)TRUE
by default (for "please generate it"), B)FALSE
(for "please forget it") or C) a string (for "please use this as the CSRF token")? Does that make sense to you?Comment #9
neclimdulyucky tri-state. Yeah, I think if I was starting from scratch I'd use NULL but for what you described makes sense and doesn't break backwards compatibility.
Comment #10
Wim LeersIt does break BC, because the parameter name is no longer a negative, so the boolean values must be inverted. You'd use
NULL
instead ofFALSE
in my example?Comment #11
neclimdulA) Null default. Generate token B) Truish use value C) False don't do anything with CSRF.
Its going to be complicated regardless because we're relying on loose typing but this makes it closer to being typed so it makes a little more sense to me. As long as we document it I think its fine.
Comment #12
Wim LeersThat makes sense! Thanks for your input, it's much appreciated :)
Done.
Comment #13
neclimdulWorks for me!
Comment #14
alexpottCommitted 676f38d and pushed to 8.3.x. Thanks!
Setting to patch to ported so that once 8.2.0 is released this can be cherry-picked to 8.2.x
Comment #16
Wim LeersComment #17
gnugetDrupal 8.2.0 is now out, Can we now cherry-pick this to the 8.2.x branch?
:D
Comment #18
alexpottCommitted eb38af5 and pushed to 8.2.x. Thanks!