Do not depend on event subscribers for security: Tighten routing security by access checking in matchRequest

So... the idea here is to make it harder to do route matching without running access checks? That seems counter-productive. They are separate tasks in my mind. If someone wants to assemble these components together in such a way as to not have access checks at all in the system, they should be able to. Tying things together in order to make it less flexible in the name of protecting people from themselves doesn't have a great track record in Drupal. I would need to see something more than a "gut feeling" to reduce decoupling here.

Some code-related comments while I'm at it:

1. +++ b/core/lib/Drupal/Core/Routing/Router.php
   @@ -0,0 +1,126 @@
   +class Router implements RouterInterface, RequestMatcherInterface {
   

   AccessAwareRouter?

2. +++ b/core/lib/Drupal/Core/Routing/Router.php
   @@ -0,0 +1,126 @@
   +    $parameters = $this->chainRouter->matchRequest($request);
   +    // The controller is being handled by the HTTP kernel, so add an attribute
   +    // to tell us this is the controller request.
   +    $parameters['_controller_request'] = TRUE;
   +    $request->attributes->add($parameters);
   

   Even if this patch does move forward, the access check parts are still a separate task from the match itself. Move that to a separate method within the class.

3. +++ b/core/lib/Drupal/Core/Routing/Router.php
   @@ -0,0 +1,126 @@
   +    try {
   +      if (!$this->accessManager->check($parameters[RouteObjectInterface::ROUTE_OBJECT], $request, $this->account)) {
   +        $e = new AccessDeniedHttpException();
   +      }
   +    }
   +    catch (\Exception $e) {
   +    }
   +
   +    $request->attributes->remove('_controller_request');
   +
   +    if ($e) {
   +      throw $e;
   +    }
   +    return $parameters;
   

   What we really want here is finally {}, but that wasn't added until PHP 5.5. :-( This is a really screwy way of emulating that, though. What's wrong with the approach in the code being replaced?

Correct me if I'm wrong..

My first thought when reading @crell's comment is that I'd expect that if a particular use case didn't need access checks it would be easy enough to write a custom module that returned true in all instances where it wasn't already handled by the permissions system.

Personally I'm in favor of having security as a priority especially where it may protect me from making a site insecure accidentally.

@crell I'd be really interested to hear and example of where this may be counter productive.

+++ b/core/core.services.yml
@@ -424,6 +424,9 @@ services:
   router:
+    class: Drupal\Core\Routing\AccessAwareRouter
+    arguments: ['@router.chain', '@access_manager', '@current_user']
+  router.unsafe:
     class: Symfony\Cmf\Component\Routing\ChainRouter
     calls:
       - [setContext, ['@router.request_context']]

This patch has changed since @Crell's comments. It now leaves the original router intact, but with a different service name.

I think this is a sufficient allowance for code that wants to run matching without applying access checks.

1. +++ b/core/lib/Drupal/Core/Routing/AccessAwareRouter.php
   @@ -0,0 +1,128 @@
   +class AccessAwareRouter implements AccessAwareRouterInterface, RequestMatcherInterface {
   
   +++ b/core/lib/Drupal/Core/Routing/AccessAwareRouterInterface.php
   @@ -0,0 +1,31 @@
   +interface AccessAwareRouterInterface extends RouterInterface {
   

   I think this hierarchy is a little strange. Why not have AccessAwareRouterInterface extend RequestMatcherInterface as well?

2. +++ b/core/lib/Drupal/Core/Routing/AccessAwareRouter.php
   @@ -0,0 +1,128 @@
   +  /**
   +   * @var ChainRouter
   +   */
   +  protected $chainRouter;
   +
   +  /**
   +   * @var AccessManagerInterface
   +   */
   +  protected $accessManager;
   +
   +  /**
   +   * @var AccountInterface
   +   */
   +  protected $account;
   

   These need full namespaces and one-liners

3. +++ b/core/lib/Drupal/Core/Routing/AccessAwareRouter.php
   @@ -0,0 +1,128 @@
   +   * Constructs a router for drupal with access and upcasting.
   

   Drupal

4. +++ b/core/lib/Drupal/Core/Routing/AccessAwareRouter.php
   @@ -0,0 +1,128 @@
   +   * @param \Symfony\Cmf\Component\Routing\ChainRouter $router
   +   *   The router.
   +   */
   +  public function __construct(ChainRouter $router, AccessManagerInterface $access_manager, AccountInterface $account) {
   

   Missing docs for the other params. And this should clarify why there is a second router.

5. +++ b/core/lib/Drupal/Core/Routing/AccessAwareRouterInterface.php
   @@ -0,0 +1,31 @@
   +   * @throws \Symfony\Component\HttpKernel\Exception\AccessDeniedHttpException
   +   *   Thrown when access checking failed.
   +   */
   +  public function matchRequest(Request $request);
   ...
   +   * @throws \Symfony\Component\HttpKernel\Exception\AccessDeniedHttpException
   +   *   Thrown when $access_check is enabled and access checking failed.
   +   */
   +  public function match($pathinfo);
   

   So now this interface is exactly the same as far methods and params.

   The only difference is the documented exceptions. Do you think a new interface is needed, or can they be on the implementing class?

6. +++ b/core/lib/Drupal/Core/Url.php
   @@ -13,6 +13,7 @@
   +use Symfony\Component\Process\Exception\LogicException;
   

   Not needed

7. +++ b/core/lib/Drupal/Core/Url.php
   @@ -123,7 +124,7 @@ public static function createFromPath($path) {
   -        $result = \Drupal::service('router')->match('/' . $path);
   +        $result = \Drupal::service('router.insecure')->match('/' . $path);
   

   This deserves a comment about why we purposefully use router.insecure

8. +++ b/core/modules/config_translation/src/Controller/ConfigTranslationController.php
   @@ -167,15 +167,7 @@ public function itemPage(Request $request, $plugin_id) {
   -          $route_name = $route_request->attributes->get(RouteObjectInterface::ROUTE_NAME);
   
   @@ -228,35 +220,4 @@ public function itemPage(Request $request, $plugin_id) {
   -    catch (ParamNotConvertedException $e) {
   ...
   -    catch (ResourceNotFoundException $e) {
   

   You remove some classes here, but not their use statements.

9. +++ b/core/modules/system/src/PathBasedBreadcrumbBuilder.php
   @@ -201,13 +202,12 @@ protected function getRequestForPath($path, array $exclude) {
   -      return NULL;
   ...
   -      return NULL;
   ...
   -      return NULL;
   

   Well, these were nice for clarity.

10. +++ b/core/tests/Drupal/Tests/Core/Routing/AccessAwareRouterTest.php
    @@ -0,0 +1,86 @@
    +   * Tests the matchrRequest() function for access allowed.
    ...
    +   * Tests the matchrRequest() function for access denied.
    

    This says matchrR, with an extra "r"

I think this adequately addresses @Crell's concerns, and I like where we ended up. Thanks for working through this @chx!

Cool

+++ b/core/lib/Drupal/Core/Routing/AccessAwareRouterInterface.php
@@ -0,0 +1,32 @@
+
+interface AccessAwareRouterInterface extends RouterInterface, RequestMatcherInterface {

this. Comitters will see this anyway.

No it didn't, yes it did, not it didn't!

1. +++ b/core/core.services.yml
   @@ -418,12 +418,15 @@ services:
   +  router.insecure:
   

   Pretty sure it should be unsecure, not insecure.

   Also it isn't unsecure if you use it properly, could do with a more descriptive name.

2. +++ b/core/lib/Drupal/Core/Url.php
   @@ -123,7 +123,9 @@ public static function createFromPath($path) {
   +        $result = \Drupal::service('router.insecure')->match('/' . $path);
   

   While it's true that Url objects might be created and stored for different users, this is also the most common place where you might or might not want access checks. Are you supposed to check access to the route before using this? I don't see any docs in the patch on how to do that.

Thanks for clarifying this!

Trivial fix.

While reading this I realized that we potentially introduced a non-trivial regression.

That expectation shouldn't be in setUp, but in the methods that need it.

Sometimes phpunit is quite lovely!

Ah, yours is better.

Oh I didn't actually wanted to do a issue-queue push --force but well, this happened :(

Committed 450cc98 and pushed to 8.0.x. Thanks!

diff --git a/core/lib/Drupal/Core/Path/PathValidator.php b/core/lib/Drupal/Core/Path/PathValidator.php
index 0c1580b..d363417 100644
--- a/core/lib/Drupal/Core/Path/PathValidator.php
+++ b/core/lib/Drupal/Core/Path/PathValidator.php
@@ -50,10 +50,6 @@ class PathValidator implements PathValidatorInterface {
    *   The route provider.
    * @param \Symfony\Component\HttpFoundation\RequestStack $request_stack
    *   The request stack.
-   * @param \Drupal\Core\Access\AccessManagerInterface $access_manager
-   *   The access manager.
-   * @param \Drupal\Core\Session\AccountInterface $account
-   *   The user account.
    */
   public function __construct(RequestMatcherInterface $request_matcher, RouteProviderInterface $route_provider, RequestStack $request_stack) {
     $this->requestMatcher = $request_matcher;
@@ -76,7 +72,7 @@ public function isValid($path) {
       return FALSE;
     }
 
-    // We can not use $this->requestMatcher->match() beccause we need to set
+    // We can not use $this->requestMatcher->match() because we need to set
     // the _menu_admin attribute to indicate a menu administrator is running
     // the menu access check.
     $request = RequestHelper::duplicate($this->requestStack->getCurrentRequest(), '/' . $path);

Fixed on commit.

Tagging a parent issue and adding a title related to the new security initiative