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.

CommentFileSizeAuthor
#104 1798296-typo.patch489 bytesandypost
#96 interdiff-93-96.txt781 bytesDavid_Rothstein
#96 1798296-96.patch36.29 KBDavid_Rothstein
#93 interdiff-1798296-93.txt7.59 KBdamiankloip
#93 1798296-93.patch36.28 KBdamiankloip
#88 1798296-87.patch35.73 KBdamiankloip
#83 1798296-83.patch28.09 KBdamiankloip
#82 1798296-82.patch28.09 KBdamiankloip
#81 1798296-81.patch29.4 KBdamiankloip
#81 interdiff-1798296-81.txt14.74 KBdamiankloip
#79 Screen Shot 2013-10-16 at 09.44.43.png266.2 KBdamiankloip
#72 1798296-72.patch35.74 KBdamiankloip
#72 interdiff-1798296-72.txt1.99 KBdamiankloip
#67 1798296-67.patch36.22 KBDavid_Rothstein
#67 interdiff-1798296-67.txt1.43 KBDavid_Rothstein
#65 1798296-65.patch36.22 KBdamiankloip
#65 interdiff-1798296-65.txt7.19 KBdamiankloip
#63 csrf-1798296-63-do-not-test.patch3.31 KBDavid_Rothstein
#60 1798296-60.patch32.91 KBdamiankloip
#60 interdiff-1798296-60.txt613 bytesdamiankloip
#57 1798296-57.patch32.92 KBdamiankloip
#57 interdiff-1798296-57.txt8.45 KBdamiankloip
#55 1798296-55.patch28.71 KBdamiankloip
#55 interdiff-1798296-55.txt2.17 KBdamiankloip
#53 1798296-53.patch26.54 KBdamiankloip
#53 interdiff-1798296-53.txt10.21 KBdamiankloip
#50 1798296-50.patch19.76 KBdamiankloip
#50 interdiff-1798296-50.txt1.63 KBdamiankloip
#49 1798296-49.patch19.65 KBdamiankloip
#49 interdiff-1798296-49.txt1.07 KBdamiankloip
#48 1798296-48.patch19.34 KBdamiankloip
#48 interdiff-1798296-48.txt2.11 KBdamiankloip
#47 1798296-47.patch17.74 KBdamiankloip
#47 interdiff-1798296-47.txt20.67 KBdamiankloip
#45 1798296-45.patch18.22 KBdamiankloip
#45 interdiff-1798296-45.txt611 bytesdamiankloip
#43 1798296-43.patch18.22 KBdamiankloip
#43 interdiff-1798296-43.txt452 bytesdamiankloip
#42 1798296-42.patch18.21 KBdamiankloip
#38 drupal_1798296_38.patch15.52 KBXano
#36 1798296-36.patch15.52 KBdamiankloip
#32 drupal_1798296_32.patch4.16 KBXano
#26 drupal_1798296_26.patch4.2 KBXano
#26 interdiff.txt1.62 KBXano
#23 1798296-23.patch4.91 KBdamiankloip
#23 interdiff-1798296-23.txt706 bytesdamiankloip
#20 drupal-1798296-20.patch4.19 KBdawehner
#20 interdiff.txt2.79 KBdawehner
#18 drupal_1798296_18.patch3.67 KBXano
#6 drupal_1798296_00.patch3.94 KBXano
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

sun’s picture

Component: base system » routing system
Issue tags: +Security improvements

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.

Crell’s picture

Title: Integrate XSRF link token directly into routing system » Integrate CSRF link token directly into routing system

I knew there was an existing issue, but couldn't find it. Thanks, sun. (Also, I've heard the terms interchangeably. Meh.)

Crell’s picture

Issue tags: +WSCCI

Tagging

Gaelan’s picture

Crell’s picture

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.

Xano’s picture

Assigned: Unassigned » Xano
Status: Active » Needs review
FileSize
3.94 KB

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.

Status: Needs review » Needs work

The last submitted patch, drupal_1798296_00.patch, failed testing.

Crell’s picture

+++ 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 the UrlGenerator. Hm...

Xano’s picture

We've standardized to CsrfAccessCheck, I believe.

Uh, we don't properly write abbreviations in all caps? Alright.

The token system needs to become a service that we can inject.

Shouldn't we do that in a follow-up?

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.

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.

andypost’s picture

Would like to test in current masquerade 8.x-2.x module we need this to get rid of hook menu

Crell’s picture

andypost: grammar parse error. Wha?

andypost’s picture

@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?

andypost’s picture

Category: feature » task

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

Xano’s picture

token-service

FYI: the token API now is a service, so another name should probably be used for CSRF tokens to prevent confusion.

andypost’s picture

So how to make progress here? Maybe open new issue to bikeshed about name for service to replace drupal_get_token()

ParisLiakos’s picture

i dont think we have to do this here..but if we really have to, session_token should be called, no need to bikeshed it, its pretty obvious

Xano’s picture

I'll reroll the patch. #2036351: Convert CSRF tokens to a service was made to convert CSRF token generation and validation to a service, so let's not do that here.

Xano’s picture

Assigned: Xano » Unassigned
Status: Needs work » Needs review
FileSize
3.67 KB

I changed the GET parameter name from token to csrf as to prevent confusion with the token API.

Status: Needs review » Needs work

The last submitted patch, drupal_1798296_18.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
2.79 KB
4.19 KB

Just fixed a couple of c&p problems.

Crell’s picture

This still needs to have an outbound path processor that adds the token to the URL.

Status: Needs review » Needs work

The last submitted patch, drupal-1798296-20.patch, failed testing.

damiankloip’s picture

Status: Needs work » Needs review
FileSize
706 bytes
4.91 KB

Oops.

damiankloip’s picture

Status: Needs review » Needs work

Sorry, completely posted this on the wrong issue!! This was meant for #2042487: Token checking for Views enable/disable is missing. Sorry for un-needed noise.

andypost’s picture

Now we have service for token #2036351: Convert CSRF tokens to a service so needs re-roll

Suppose all usages of token in controllers should be fixed in the issue as well

Xano’s picture

Status: Needs work » Needs review
FileSize
1.62 KB
4.2 KB

Status: Needs review » Needs work

The last submitted patch, drupal_1798296_26.patch, failed testing.

damiankloip’s picture

Status: Needs work » Needs review
+++ b/core/lib/Drupal/Core/Access/CsrfAccessCheck.phpundefined
@@ -0,0 +1,36 @@
+ * drupal_get_token(), for which the parameter has the same value as the

This can reference the new CSRF generator service too.

+++ b/core/lib/Drupal/Core/Access/CsrfAccessCheck.phpundefined
@@ -0,0 +1,36 @@
+class CsrfAccessCheck implements AccessCheckInterface {

This can implement StaticAccessCheckInterface.

+++ b/core/lib/Drupal/Core/Access/CsrfAccessCheck.phpundefined
@@ -0,0 +1,36 @@
+    return \Drupal::csrfToken()->validate($request->query->get('csrf'), $route->getRequirement('_csrf')) ? static::ALLOW : static::KILL;

Access checkers are services, so we can inject this instead.

+++ b/core/tests/Drupal/Tests/Core/Access/CsrfAccessCheckTest.phpundefined
@@ -0,0 +1,80 @@
+    $this->assertTrue(FALSE);

I guess we don;t want this anymore :)

+++ b/core/tests/Drupal/Tests/Core/Access/CsrfAccessCheckTest.phpundefined
@@ -0,0 +1,80 @@
+    $route = $this->getMockBuilder('Symfony\Component\Routing\Route')
+      ->disableOriginalConstructor()
+      ->getMock();
+    $route->expects($this->any())
+      ->method('getRequirements')
+      ->will($this->returnValue(array('_csrf' => '')));
+    $res = $applies_check->applies($route);
+    $this->assertEquals(TRUE, $res);
+
+    $route = $this->getMockBuilder('Symfony\Component\Routing\Route')
+      ->disableOriginalConstructor()
+      ->getMock();
+    $route->expects($this->any())
+      ->method('getRequirements')
+      ->will($this->returnValue(array()));
+    $res = $applies_check->applies($route);

This will have to change per comment abot about interface change.

andypost’s picture

Status: Needs review » Needs work

Is there ability to inject the service?

@@ -0,0 +1,36 @@
+    return \Drupal::csrfToken()->validate($request->query->get('csrf'), $route->getRequirement('_csrf')) ? static::ALLOW : static::KILL;

should be

$service = \Drupal::csrfToken();
$service->setRequest($request);
$service->validate();
Crell’s picture

Yes it should be injected, but no we don't set the request on it directly. The request should be implicit in the service as a dependency, or else provided directly in the validate call. Services should be treated as stateless. Vis, #28 is correct, #29 is not. :-)

@@ -0,0 +1,80 @@
+    $route = new Route('/foo', array(), array('_csrf: ' . $token_value));

This is entirely wrong. We only tag a route as needing CSRF. We should NOT put a static token on the route. That's a security hole.

Rather, we need an outbound path processor that will add a token to the path when it gets generated, and *that* is the value that gets checked by the access checker.

Xano’s picture

Assigned: Unassigned » Xano
@@ -0,0 +1,36 @@
+    return \Drupal::csrfToken()->validate($request->query->get('csrf'), $route->getRequirement('_csrf')) ? static::ALLOW : static::KILL;

should be

$service = \Drupal::csrfToken();
$service->setRequest($request);
$service->validate();

No. The csrf_token service uses the request for the account (which is set when instantiating the service), but the token needs to be passed every time it is validated.

This is entirely wrong. We only tag a route as needing CSRF. We should NOT put a static token on the route. That's a security hole.

This is not the token, but the value (like a salt) used to generate the actual token. See \Drupal::csrfToken()->get().

Yes it should be injected, but no we don't set the request on it directly. The request should be implicit in the service as a dependency, or else provided directly in the validate call. Services should be treated as stateless. Vis, #28 is correct, #29 is not. :-)

Could you rephrase this in layman's terms?

Xano’s picture

Assigned: Xano » Unassigned
Status: Needs work » Needs review
FileSize
4.16 KB

Status: Needs review » Needs work

The last submitted patch, drupal_1798296_32.patch, failed testing.

Crell’s picture

Per #28, Do not put \Drupal::csrfToken() in a class. Ever. Instead, pass that service in via the constructor of the object using the appropriate services.yml file.

damiankloip’s picture

Assigned: Unassigned » damiankloip

Doing a bit of work on this.

damiankloip’s picture

Assigned: damiankloip » Unassigned
Status: Needs work » Needs review
FileSize
15.52 KB

ok, did a couple of things:

- Added $route parameter to the outboundPathProcessorInterface
- Added a check for query in the options in UrlGenerator::generateFromRoute() (same as generateFormPath() does)

There is possibly a big issue though, If we add a token to the query string for a link, the access will be checked to determine whether to show the link, but this will fail as in the access check, $request->attributes->get('csrf') will not return anything as it's not for that request.

Haven't worked on the tests yet. So will still be very broken.

Status: Needs review » Needs work

The last submitted patch, 1798296-36.patch, failed testing.

Xano’s picture

Status: Needs work » Needs review
FileSize
15.52 KB

Status: Needs review » Needs work

The last submitted patch, drupal_1798296_38.patch, failed testing.

damiankloip’s picture

This really has no chance of passing yet. There is a much bigger problem before this can can work as things like menu links will always be access false :/

Are you going to be in Prague? Maybe we could talk about it there?

Xano’s picture

Yes. I am at the extended sprints right now.

damiankloip’s picture

Status: Needs work » Needs review
FileSize
18.21 KB

I was speaking to klausi yesterday, and the discussion turned away from access checks to just having an event subscriber that does this for us instead. This was it is more 'baked in' to the routing system, kind of. But also gets around the fact that things like menu links use access checkers with a mocked request object etc...

Thoughts on this approach instead?

damiankloip’s picture

FileSize
452 bytes
18.22 KB

Meh.

Status: Needs review » Needs work

The last submitted patch, 1798296-43.patch, failed testing.

damiankloip’s picture

Status: Needs work » Needs review
FileSize
611 bytes
18.22 KB

Let's try this.

Status: Needs review » Needs work

The last submitted patch, 1798296-45.patch, failed testing.

damiankloip’s picture

FileSize
20.67 KB
17.74 KB

Ok, after more discussion, the access checker could be the better plan, as it doesn't run on every request (even though the outbound processor would anyway). I've added a RouteProcesserManager, OutboundRouteProcessorInterface and changed the previous implementation to use that. So this basically allows you to alter the route BEFORE it's compiled and the path is created etc...

This patch is still a work in progress, I will work some more on it later on.

damiankloip’s picture

Status: Needs work » Needs review
FileSize
2.11 KB
19.34 KB

After talking to dawehner a bit, we were thinking something along the lines of this; So just for access checkers we can have a way of determining if the request is for reals of not.

damiankloip’s picture

FileSize
1.07 KB
19.65 KB

And we need to take into account the access mode there so we don't get unwanted access disasters happening.

damiankloip’s picture

FileSize
1.63 KB
19.76 KB

Logic was totally the wrong way round.

Berdir’s picture

Re last interdiff: Is this still a problem with https://drupal.org/node/2063401?

Seems that when the default is ALL, this works as expected, and if the csrf requirement and another one and set the access mode to ANY, your fault?

Or is there some magic that applies that to routes automatically? I haven't looked at what all the other parts of the patch are doing...

Status: Needs review » Needs work

The last submitted patch, 1798296-50.patch, failed testing.

damiankloip’s picture

Status: Needs work » Needs review
FileSize
10.21 KB
26.54 KB

Here are some unit tests. Getting there...

Status: Needs review » Needs work

The last submitted patch, 1798296-53.patch, failed testing.

damiankloip’s picture

Status: Needs work » Needs review
FileSize
2.17 KB
28.71 KB

Fix the urlGeneratorTest. Should this test use a mock instead and set expectations that processRoute() is called?

dawehner’s picture

Should this test use a mock instead and set expectations that processRoute() is called?

I would say so, otherwise you should not call it unit test.

  1. +++ b/core/lib/Drupal/Core/EventSubscriber/AccessSubscriber.php
    @@ -50,6 +56,8 @@ public function onKernelRequestAccessCheck(GetResponseEvent $event) {
    +
    +    $request->attributes->remove('_controller_request');
       }
    

    Can we please catch exceptions and remove the request variable in there and throw the exception again so it does not leak into 403/404 pages?

  2. +++ b/core/lib/Drupal/Core/RouteProcessor/RouteProcessorManager.php
    @@ -0,0 +1,85 @@
    + * Class RouteProcessorManager.
    

    Can you tell me more?

  3. +++ b/core/lib/Drupal/Core/RouteProcessor/RouteProcessorManager.php
    @@ -0,0 +1,85 @@
    +class RouteProcessorManager implements OutboundRouteProcessorInterface {
    

    I am wondering whether we should try to keep the route processors inside the path processor manager maybe and just add another interface. Having two separate systems feels kind of confusing to be honest.

  4. +++ b/core/tests/Drupal/Tests/Core/Access/CsrfAccessCheckTest.php
    @@ -0,0 +1,132 @@
    +class CsrfAccessCheckTest extends UnitTestCase {
    

    What about adding @group Drupal and @group Routing or something like that? This test is so nice separated etc.

damiankloip’s picture

FileSize
8.45 KB
32.92 KB

Thanks Daniel, how about something like this?

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

One important question though which is remaining is whether it is a good idea to have a separate routeProcessmanager ... you know removing features is harder than not adding them.

+++ b/core/lib/Drupal/Core/EventSubscriber/AccessSubscriber.php
@@ -52,7 +52,16 @@ public function onKernelRequestAccessCheck(GetResponseEvent $event) {
+    // can always be removed.
+    try {
+      $access = $this->accessManager->check($request->attributes->get(RouteObjectInterface::ROUTE_OBJECT), $request);
+    }
+    catch (\Exception $e) {
+      $request->attributes->remove('_controller_request');
+      throw $e;
+    }
+

Nice!!

Crell’s picture

+++ b/core/lib/Drupal/Core/EventSubscriber/AccessSubscriber.php
@@ -40,16 +41,32 @@ public function __construct(AccessManager $access_manager) {
-    $access = $this->accessManager->check($request->attributes->get(RouteObjectInterface::ROUTE_OBJECT), $request);
+    // Wrap this in a try/catch to ensure the '_controller_request' attribute
+    // can always be removed.
+    try {
+      $access = $this->accessManager->check($request->attributes->get(RouteObjectInterface::ROUTE_OBJECT), $request);
+    }
+    catch (\Exception $e) {
+      $request->attributes->remove('_controller_request');
+      throw $e;
+    }
+
     if (!$access) {
       throw new AccessDeniedHttpException();
     }
+
+    $request->attributes->remove('_controller_request');

Based on the comment, shouldn't the remove call happen before the throw AccessDeniedHttpException?

(This is exactly the use case that 'finally' is for in PHP 5.5... *sigh*)

damiankloip’s picture

FileSize
613 bytes
32.91 KB

Yes, this is a perfect use case for finally... :/

You're right, moved the removal to above the check for the $access variable.

damiankloip’s picture

Status: Reviewed & tested by the community » Needs review
dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Good catch in #59. Bringing back to RTBC.

David_Rothstein’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
3.31 KB
  1. This is great stuff, but with no real usage examples (including in the tests) I had a lot of trouble figuring out how a developer would use this. Eventually figured it out (I think) and did an example conversion in the attached patch, which seemed to work very nicely. Feel free to include it as part of the main patch if you want... presumably the idea of introducing this is that all route-based CSRF tokens in core should eventually use it :)
  2.    public function generateFromRoute($name, $parameters = array(), $options = array()) {
         $absolute = !empty($options['absolute']);
         $route = $this->getRoute($name);
    +    $this->processRoute($route, $parameters);
    

    Are there performance concerns with adding this? The possible performance impact of doing this for all generated URLs was a serious concern in #755584: Built-in support for csrf tokens in links and menu router, but when that issue was closed in favor of this one the discussion was not picked up here.

  3. +  public function processOutbound(Route $route, array &$parameters) {
    +    if ($route->hasRequirement('_csrf')) {
    +      // Adding this to the parameters means it will get merged into the query
    +      // string when the route is compiled.
    +      $parameters['csrf'] = $this->csrfToken->get($route->getRequirement('_csrf'));
    +    }
    +  }
    

    The current de facto standard is to use 'token' as the URL query parameter; I don't think this issue should change it to 'csrf'. People will see these tokens in the printed URL and 'token' is a lot less technical.

    Plus there is code (for example, in the Overlay module, overlay_overlay_child_initialize()) that currently assumes 'token' as the de facto standard (actually one of the nice followups of this issue is that hopefully that code could be changed to not have to assume anything anymore, since it could check the route instead).

damiankloip’s picture

Yes, that is the plan. I'm hoping almost all usages of setting the token query string param and checking for a valid token can be removed in favour of a route requirement.

Shortcut is a good example to convert, looks like it's basically done in your patch, so thanks for that. Looks like it just needs a csrf requirement for the delete route too?

I'm happy to change this back to 'token' it was just csrf for so long I haven't thought about it :)

Hmm, performance...definitely worth looking into.

I can't see this overhead really being much in the scheme of things. One thing worth mentioning though is that this patch adds functionality to the routing system that is (IMO) very much needed. As well as the massive DX improvement for having CSRF protection on routes.

This use case shows it well, but currently you cannot alter a route before if it converted to a link/url etc... You can only use the outbound path processors. Which are basically a port of the old hook_url_outbound_alter() and is very limited. For example, without being able to alter the route before it is processed, you can not really ever add anything to the query string in an outbound processor - which is a major downfall.

damiankloip’s picture

FileSize
7.19 KB
36.22 KB

Here is a new patch; rolling in David's conversion in shortcut module and changing 'csrf' to 'token' for the query string.

Crell’s picture

"Token" was renamed because it was so easily confused with the Token API string mangling system. This is specifically a CSRF check, so let's keep it at that.

I like David's update. It shows exactly the simplification we're going for. :-)

David_Rothstein’s picture

FileSize
1.43 KB
36.22 KB

I wonder if "_csrf_token" in the route and "token" in the URL would be the best combination. But in any case, #65 already started the latter conversion... It looks good but I noticed a couple places it missed (just in code comments) so I made those fixes in the attached.

Shortcut is a good example to convert, looks like it's basically done in your patch, so thanks for that. Looks like it just needs a csrf requirement for the delete route too?

Nope, delete has a confirmation form. (I only converted it to a route in this patch too since it shares the link-generation code with the other one.)

This use case shows it well, but currently you cannot alter a route before if it converted to a link/url etc... You can only use the outbound path processors. Which are basically a port of the old hook_url_outbound_alter() and is very limited.

That's a good point; it is opening up additional functionality besides the CSRF check.

I think the performance issue might come with the number of function calls involved in this (it really takes you several steps down the function stack to invoke this new processor), combined with the fact that there can easily be hundreds of URLs on a page. In addition, because core is shipping with an implementation of the system, that adds some overhead too (compared for example to hook_url_outbound_alter() which never had an implementation in core, so that made it more lightweight on many sites).

Anyway, my guess is @catch would want to see some benchmarks before committing this :) I can imagine some ways to improve performance if it becomes necessary (e.g. the CSRF check could be inlined in the generate() method, as well as a few other things).

Otherwise this is looking great.

damiankloip’s picture

Nope, delete has a confirmation form. (I only converted it to a route in this patch too since it shares the link-generation code with the other one.)

Yes, I realised that after the comment. If you notice, the patch in #65 doesn't add that.

e.g. the CSRF check could be inlined in the generate() method

Seems like a bad idea, we don't hard code any other access checks or outbound processing in the urlGenerator. This is functionality we need either way so moving any stuff inline will not help with that. This is more a patch for route processing functionality, we just haven't needed until until this use case. We certainly can't ship without being able to do this though I don't think.

Crell’s picture

I just realized something else worrisome... We were planning to cache all generated URLs for performance: #1965074: Add cache wrapper to the UrlGenerator

What does this do to cacheability? (And is it any worse than CSRF tokens already?)

damiankloip’s picture

That's a good question indeed. I would say we are no worse off than we are now with tokens? Well, maybe better, as we have a better system for doing it, plus we DO need to be able to alter outbound routes :)

dawehner’s picture

  1. +++ b/core/core.services.yml
    @@ -503,6 +510,11 @@ services:
    +      - { name: route_processor_outbound, priority: 400 }
    

    Is there any reason for the really high priority here? Note this ensures that it comes first, ... I don't see a point for that.

  2. +++ b/core/lib/Drupal/Core/DependencyInjection/Compiler/RegisterRouteProcessorsPass.php
    @@ -0,0 +1,36 @@
    +  /**
    +   * Adds services tagged 'route_processor_outbound' to route processor manager.
    +   *
    +   * @param \Symfony\Component\DependencyInjection\ContainerBuilder $container
    +   *  The container to process.
    +   */
    

    Let's just @inheritdoc it.

  3. +++ b/core/lib/Drupal/Core/DependencyInjection/Compiler/RegisterRouteProcessorsPass.php
    @@ -0,0 +1,36 @@
    +  }
    +}
    diff --git a/core/lib/Drupal/Core/EventSubscriber/AccessSubscriber.php b/core/lib/Drupal/Core/EventSubscriber/AccessSubscriber.php
    

    new line missing.

  4. +++ b/core/lib/Drupal/Core/Routing/NullGenerator.php
    @@ -9,6 +9,7 @@
    +use Symfony\Component\Routing\Route;
    
    @@ -46,7 +47,7 @@ public function getContext() {
    +  protected function processPath($path, &$options = array(), Route $route = NULL) {
         return $path;
       }
    

    I guess we don't need this change anymore.

damiankloip’s picture

FileSize
1.99 KB
35.74 KB

thanks Daniel.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Together with the potential cleanups in follow ups this seems to is ready to fly.

ParisLiakos’s picture

grep -r -E "1798296|755584" core/
core/modules/comment/lib/Drupal/comment/Controller/CommentController.php:    //   https://drupal.org/node/1798296.
core/modules/overlay/lib/Drupal/overlay/Controller/OverlayController.php:    //   http://drupal.org/node/1798296.
core/modules/aggregator/lib/Drupal/aggregator/Controller/AggregatorController.php:    //   generation. Add token support to routing: http://drupal.org/node/755584.

should we also do those here?

damiankloip’s picture

I'll open a follow up for conversions if that's ok? This patch is getting big enough :)

ParisLiakos’s picture

sure. i opened #2109303: Convert CSRF checks in controllers to the routing system
they are just 3 of them, so one issue should be enough

Xano’s picture

#72: 1798296-72.patch queued for re-testing.

catch’s picture

Status: Reviewed & tested by the community » Needs review
Issue tags: +needs profiling

It'd be good to see what impact the extra processing has with some profiling before this goes in. Haven't reviewed the patch yet.

damiankloip’s picture

FileSize
266.2 KB

See xhprof output from comparing before/after patch, to see the effect of the route processor, this is just using the generateFromRoute() method, as this is the only one that will actually use the route processor:

Run #525e509227d04 Run #525e50b2c8316 Diff Diff%
Number of Function Calls 186 232 46 24.7%
Incl. Wall Time (microsec) 1,907 2,455 548 28.7%
Incl. CPU (microsecs) 1,439 1,807 368 25.6%
Incl. MemUse (bytes) 177,520 207,712 30,192 17.0%
Incl. PeakMemUse (bytes) 177,936 208,704 30,768 17.3%
damiankloip’s picture

We have talked about adding the path and route processors as a single processor, this should make things much better. I would prefer to do that as a critical follow up, how does that sound? If not, I can reroll and do it in this patch. I just don't want this patch to get too large.. and merging the processors will no doubt spark a whole discussion of its own.

damiankloip’s picture

FileSize
14.74 KB
29.4 KB

Here is a patch with the outbound route processor merged into the path processor instead.

damiankloip’s picture

FileSize
28.09 KB

Oops, this patch please. Ignore the xhprof hunk I accidentally included in that last one..

damiankloip’s picture

FileSize
28.09 KB

Rerolled.

jibran’s picture

Status: Needs review » Reviewed & tested by the community

Back to @catch after #79.

katbailey’s picture

Issue tags: -Security improvements, -WSCCI
+++ b/core/lib/Drupal/Core/PathProcessor/PathProcessorManager.phpundefined
@@ -118,6 +137,30 @@ public function processOutbound($path, &$options = array(), Request $request = N
+  public function addOutboundRoute(OutboundRouteProcessorInterface $processor, $priority = 0) {

Can we call this addOutboundRouteProcessor? I know it's a bit long but this method is not adding a route, it's adding a processor. Same with getOutboundRoute and the sortedOutboundRoute property. It sounds like they're acting on the same thing as processOutboundRoute but they aren't.

It seemed odd to see this shoved into the PathProcessorManager, but if it really makes a difference to performance then fair enough. I wonder should we change the name though? Not here necessarily... but change it to something like RoutingProcessorManager. Seems more generic.

Anyway, these are minor points so leaving it RTBC.

katbailey’s picture

Issue tags: +Security improvements, +WSCCI

wat.

msonnabaum’s picture

Totally agreed with #85.

It struck me as very odd to see PathProcessorManager handling both. The design in #72 looks much better to me, and if performance was the reason we changed that, lets please change it back.

We need to stop considering the performance impact of method calls and calling "new". It's never worth compromising the design.

damiankloip’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
35.73 KB

After talking to katbailey and msonnabaum about this, having two managers, and therefore better interfaces etc.. is a better win. Merging the managers gives us nothing from a performance point of view.

This is a reroll of the patch from #72.

msonnabaum’s picture

#88 looks much better to me.

katbailey’s picture

Status: Needs review » Reviewed & tested by the community

This looks great to me and the tests are lovely :)

katbailey’s picture

Issue summary: View changes

Update for related developments.

David_Rothstein’s picture

Issue summary: View changes
Status: Reviewed & tested by the community » Needs review
FileSize
35.9 KB
4.51 KB

The patch no longer applied, so here's a reroll. I couldn't generate a good interdiff due to the conflicts, but the attached diff-of-a-diff shows the changes (which look pretty trivial).

The performance numbers in #79 look really scary (~50 millisecond performance hit for a page with 100 URLs on it!?). However, I couldn't reproduce anything that bad. Is there some caching that causes the first call to take a lot longer? What I tested was 10,000 consecutive calls to Drupal::url('user.logout'), which came out to an average of 110 microseconds per Drupal::url() call before the patch and 120 microseconds after. So a ~10% hit rather than a ~25% hit, but more important, the baseline number was low enough that the total effect would be very small (only a ~1 millisecond performance hit for a page with 100 URLs on it). Perhaps I did something wrong too though. We should probably get to the bottom of it.

What was more "interesting" was that I ran the equivalent code in Drupal 7, and the average came out around 22 microseconds per url() call. Which means that with or without the patch, URL generation is 5 times faster in Drupal 7 than Drupal 8?! Wow.

David_Rothstein’s picture

Also, per #67 I still think we should do something to standardize the terminology and improve the developer experience ("_csrf" on the route is not precise, and doesn't match "token" in the URL in any way).

I would prefer "_csfr_token" or even "_csrf_token_value" on the route, to be more explicit about what you're actually supposed to provide there.

damiankloip’s picture

Issue summary: View changes
FileSize
36.28 KB
7.59 KB

You are surprised that D8 is slower than D7?! ;) Things like this are many times slower at the moment I'm afraid :/ I guess if you went back to D6, it would be faster than D7 too :) The bottom line is that we have to be able to alter outbound routes in this way (Sorry, starting to sound like a broken record). The routing system in general need a performance boost, so this getting in I think is an essential part of that - so we can see where we are. What do you think?

What did you use to benchmark? Here is an xhprof summary of what you described above (calling \Drupal::url('user.logout') 100 times):

Run #52789f7bedb98 Run #52789fa1913fa Diff Diff%
Number of Function Calls 5,672 6,275 603 10.6%
Incl. Wall Time (microsec) 27,876 33,942 6,066 21.8%
Incl. CPU (microsecs) 27,603 33,564 5,961 21.6%
Incl. MemUse (bytes) 144,928 156,216 11,288 7.8%
Incl. PeakMemUse (bytes) 136,040 147,048 11,008 8.1%

I have changed this patch to use '_csrf_token' as the requirement instead, I think '_csrf_token_value' is a bit too far IMO.

That above patch should (I hope anyway) certainly fail, as the unit tests are not using the new access() parameter signature. The reroll contains a fix for that too.

Hopefully you're next reply won't take a month, like the frequency between the last one ;) ha. This does make core dev slightly more painful tbh.

catch’s picture

I guess if you went back to D6, it would be faster than D7 too :)

Actually no. I did quite a bit of work on url generation (path alias caching and various other things) to make it faster in Drupal 7.

Related to the caching of the CSRF tokens, this patch doesn't make it worse, it might potentially make it better if it's possible to replace the token handling with something similar to https://drupal.org/project/cacheable_csrf. For that I'd need to be able to swap out the csrf token generator when creating links to one that adds a placeholder, then remove the CSRF check that core does and insert a different one in the routing system. Not sure if that approach is at all viable for core though.

It's not great having both this patch and #2019123: Use the same canonical URI paths as for HTML routes adding overhead to inbound/outbound link generation respectively. We may need to bump #1805054: Cache localized, access filtered, URL resolved, and rendered menu trees and #1965074: Add cache wrapper to the UrlGenerator to critical - although those only address caching not raw performance at all.

damiankloip’s picture

Sorry, I stand corrected on the D6 > D7 url handling :)

For that I'd need to be able to swap out the csrf token generator when creating links to one that adds a placeholder, then remove the CSRF check that core does and insert a different one in the routing system.

They are all services, so you could just swap them out, you would switch the outbound csrf route processor, as well as the access checker. That would be job done? unless I missed something.

As mentioned up somewhere, we basically need to be able to alter the outbound route in this way, so I think it would be good to get this in, then atleast we know where we stand and what we need to try and optimise more. Thoughts on that?

David_Rothstein’s picture

FileSize
36.29 KB
781 bytes

#93 looks good to me after fixing the remaining instances of _csrf (they were in documentation) - see attached. Thanks for catching the remaining access() instances.

The numbers in #93 look closer to mine although still fairly different (~6 millisecond performance hit per 100 URL calls, rather than ~1 millisecond). Some of that could be computer speed, although percentage-wise they're also quite different (20% vs 10%). For what it's worth, I did my testing like this:

  $start = microtime(TRUE);
  for ($i = 0; $i < 10000; $i++) {
    Drupal::url('user.logout');
  }
  $end = microtime(TRUE);

And then compared the numbers.

I'm putting this to RTBC on the theory that a few milliseconds isn't too terrible of a hit for a security boost (especially compared to the overall situation), and that @catch is going to look more at this with a performance eye anyway. Beyond that it seems ready to go.

David_Rothstein’s picture

Hm, that's weird... it already was back to RTBC. But I don't see which comment changed it to that.

dawehner’s picture

Please try to do the following:

  $start = microtime(TRUE);
  $url_generator = \Drupal::service('url_generator');
  for ($i = 0; $i < 10000; $i++) {
    $url_generator->generate('user.logout');
  }
  $end = microtime(TRUE);
David_Rothstein’s picture

Tried that and it sped things up a tiny bit (though that's actually the wrong direction in terms of reconciling with #93).

Results: ~100 microseconds per generate() call before the patch, ~113 microseconds after

(compared to 110 and 120 during previous test with Drupal::url())

Overall makes sense to me that it's similar (since Drupal::url() and $url_generator->generate() are both wrappers around basically the same thing).

David_Rothstein’s picture

Oh also, all my tests were done on a fresh install using the Minimal profile, if it matters.

catch’s picture

Title: Integrate CSRF link token directly into routing system » Change notice: Integrate CSRF link token directly into routing system
Priority: Normal » Major
Status: Reviewed & tested by the community » Active

OK I'm still not happy about the performance hit here, but since this is going to end up contributing to making the CSRF tokens (and hence the HTML for those links) cacheable that seems a reasonable trade-off - I'm about to implement that for a 7.x site and there's no clean way to do it.

Committed/pushed to 8.x. Will need a change notice.

damiankloip’s picture

Created a change notice for the csrf token usage, shall we create a new notice or add to existing path subscriber ones for the outbound route subscriber stuff?

https://drupal.org/node/2133117

jibran’s picture

I think we should add example of dynamic value token as well.

andypost’s picture

Status: Active » Needs review
FileSize
489 bytes

Better to fix the typo

webchick’s picture

Committed and pushed to 8.x. Thanks!

I think "needs review" is still appropriate for the change notice..?

xjm’s picture

Issue tags: +Needs change record
damiankloip’s picture

I created one of the CNs, but would like #2133439: Dynamically create token value string based on route path to get committed first, as that will change it slightly anyway.

webchick’s picture

Title: Change notice: Integrate CSRF link token directly into routing system » Change notice+docs: Integrate CSRF link token directly into routing system
Issue tags: +Needs documentation

Let's make sure we also make sure to document when to use this new property and why in the routing system docs

damiankloip’s picture

Added something for that to the bottom of https://drupal.org/node/2122195, check it out.

andypost’s picture

https://drupal.org/node/2133117 should be updated too

damiankloip’s picture

Updated that.

Crell’s picture

Title: Change notice+docs: Integrate CSRF link token directly into routing system » Integrate CSRF link token directly into routing system
Status: Needs review » Fixed
Issue tags: -Needs change record, -Needs documentation

I added a note to the docs about the token only working when using the generator. Otherwise I think we're done. Thanks all!

damiankloip’s picture

Oh yeah, that.. Good call!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.