| Project: | Drupal core |
| Version: | 8.x-dev |
| Component: | routing system |
| Category: | task |
| Priority: | normal |
| Assigned: | Xano |
| Status: | needs work |
| Issue tags: | Security improvements, WSCCI |
Issue Summary
Problem/Motivation
The secure way to make "links that do things" is to have the link include a token in the URL in a GET parameter, and then check that parameter as part of the access callback on the menu item.
Of course, that is something everyone has to do themselves, which means developers frequently forget to do so.
Proposed resolution
Whether or not a route needs an XSRF token on it should be centralized. In the new routing system, we can flag a route as needing such a token. Then we can always access check for it, and when generating URLs to that route we can always inject the token. That makes it a one-line task for developers.
This issue depends on:
#1606794: Implement new routing system
#1874500: CMF-based Routing system
and (maybe) #1888424: Make Drupal's URL generation logic available to HttpKernel, and minimize code repetition/divergence
Once we have a generator, this should be super easy to do:
1) Add an "xsrf" => TRUE option to a Route.
2) In the Generator, check and see if the route has an xsrf option set. If so, add a token=[token value goes here] GET parameter to it.3) Add a new request listener (after the matcher listener) that checks if a route requires an XSRF token. If so, check if the incoming request has one and a valid one. If not, throw a 403 exception.
3) Add a new Access Checker that runs if the route has an xsrf tag, and if so check that the incoming request has a valid one. If not, return FALSE to let the access system throw a 403.
Remaining tasks
Get the above issues committed, then implement this. :-)
User interface changes
None.
API changes
Adds a new option to Route object definitions.
Comments
#1
Prior art: #755584: Built-in support for csrf tokens in links and menu router
Related: #961508: Dual http/https session cookies interfere with drupal_valid_token()
I also think you wanted to say CSRF wherever you said XSRF. While dictionaries surprisingly yield it as a synonym, CSRF is the proper industry term.
#2
I knew there was an existing issue, but couldn't find it. Thanks, sun. (Also, I've heard the terms interchangeably. Meh.)
#3
Tagging
#4
Possible duplicate of #755584: Built-in support for csrf tokens in links and menu router.
#5
I think that issue should close in favor of this one, which we should be able to work on as soon as #1874500: CMF-based Routing system happens. (That introduces the generator, which is where we would do this sort of work.
#6
The patch implements Crell's original idea, with the addition that it allows routes to use a custom parameter that is passed on to drupal_get_token(). This value may be left empty. The access check simply responds to the presence of a _csrf key in the route requirements.
#7
The last submitted patch, drupal_1798296_00.patch, failed testing.
#8
+++ b/core/lib/Drupal/Core/Access/CSRFAccessCheck.php@@ -0,0 +1,35 @@
+class CSRFAccessCheck implements AccessCheckInterface {
We've standardized to CsrfAccessCheck, I believe.
+++ b/core/lib/Drupal/Core/Access/CSRFAccessCheck.php@@ -0,0 +1,35 @@
+ return drupal_get_token($route->getRequirement('_csrf')) == $request->query->get('token');
The token system needs to become a service that we can inject.
To really do this right, though, we should also implement an OutboundPathProcessor, perhaps one for just routes, that will add a token to an outgoing link automagically. Then there's no involvement from the user.
That requires OutboundPathProcessors, which are coming as part of #1888424: Make Drupal's URL generation logic available to HttpKernel, and minimize code repetition/divergence. Although, that may cause problems for #1965074: Add cache wrapper to Generator. Hm...
#9
Uh, we don't properly write abbreviations in all caps? Alright.
Shouldn't we do that in a follow-up?
I'm not familiar with those, but how do CSRF tokens apply to outbound links only?
I just found Drupal\rest\Access\CSRFAccessCheck, which does a similar job, although it is more restrictive in terms of when it applies.
#10
Would like to test in current masquerade 8.x-2.x module we need this to get rid of hook menu
#11
andypost: grammar parse error. Wha?
#12
@Crell, I mean that this hook_menu implementation needs token but the way Xano pointed looks nice.
So probably deal with injection in follow-ups?
#13
This kind of access is needed for #1978948: Convert comment_approve() to a Controller
Actually there's no need to implement token-service here because we already have the same implementation and scrope is different
#14
FYI: the token API now is a service, so another name should probably be used for CSRF tokens to prevent confusion.