Issue #2595695 by Wim Leers, alexpott, mfb, Crell, cilefen, dawehner: 4xx...

Issue #2595695 by Wim Leers, alexpott, mfb, Crell, cilefen, dawehner: 4xx handling using subrequests: no longer able to vary by URL

core/core.services.yml

@@ -1144,7 +1144,7 @@ services:
     class: Drupal\Core\EventSubscriber\DefaultExceptionHtmlSubscriber
     tags:
       - { name: event_subscriber }
-    arguments: ['@http_kernel', '@logger.channel.php', '@redirect.destination']
+    arguments: ['@http_kernel', '@logger.channel.php', '@redirect.destination', '@router.no_access_checks']
   exception.default:
     class: Drupal\Core\EventSubscriber\DefaultExceptionSubscriber
     tags:
@@ -1164,7 +1164,7 @@ services:
     class: Drupal\Core\EventSubscriber\CustomPageExceptionHtmlSubscriber
     tags:
       - { name: event_subscriber }
-    arguments: ['@config.factory', '@path.alias_manager', '@http_kernel', '@logger.channel.php', '@redirect.destination']
+    arguments: ['@config.factory', '@http_kernel', '@logger.channel.php', '@redirect.destination', '@router.no_access_checks']
   exception.fast_404_html:
     class: Drupal\Core\EventSubscriber\Fast404ExceptionHtmlSubscriber
     tags:

core/lib/Drupal/Core/EventSubscriber/CustomPageExceptionHtmlSubscriber.php

@@ -14,6 +14,7 @@
 use Symfony\Component\HttpFoundation\Response;
 use Symfony\Component\HttpKernel\Event\GetResponseForExceptionEvent;
 use Symfony\Component\HttpKernel\HttpKernelInterface;
+use Symfony\Component\Routing\Matcher\UrlMatcherInterface;
 
 /**
  * Exception subscriber for handling core custom HTML error pages.
@@ -27,31 +28,23 @@ class CustomPageExceptionHtmlSubscriber extends DefaultExceptionHtmlSubscriber {
    */
   protected $configFactory;
 
-  /**
-   * The page alias manager.
-   *
-   * @var \Drupal\Core\Path\AliasManagerInterface
-   */
-  protected $aliasManager;
-
   /**
    * Constructs a new CustomPageExceptionHtmlSubscriber.
    *
    * @param \Drupal\Core\Config\ConfigFactoryInterface $config_factory
    *   The configuration factory.
-   * @param \Drupal\Core\Path\AliasManagerInterface $alias_manager
-   *   The alias manager service.
    * @param \Symfony\Component\HttpKernel\HttpKernelInterface $http_kernel
    *   The HTTP Kernel service.
    * @param \Psr\Log\LoggerInterface $logger
    *   The logger service.
    * @param \Drupal\Core\Routing\RedirectDestinationInterface $redirect_destination
    *   The redirect destination service.
+   * @param \Symfony\Component\Routing\Matcher\UrlMatcherInterface $access_unaware_router
+   *   A router implementation which does not check access.
    */
-  public function __construct(ConfigFactoryInterface $config_factory, AliasManagerInterface $alias_manager, HttpKernelInterface $http_kernel, LoggerInterface $logger, RedirectDestinationInterface $redirect_destination) {
-    parent::__construct($http_kernel, $logger, $redirect_destination);
+  public function __construct(ConfigFactoryInterface $config_factory, HttpKernelInterface $http_kernel, LoggerInterface $logger, RedirectDestinationInterface $redirect_destination, UrlMatcherInterface $access_unaware_router) {
+    parent::__construct($http_kernel, $logger, $redirect_destination, $access_unaware_router);
     $this->configFactory = $config_factory;
-    $this->aliasManager = $alias_manager;
   }
 
   /**
@@ -65,16 +58,20 @@ protected static function getPriority() {
    * {@inheritdoc}
    */
   public function on403(GetResponseForExceptionEvent $event) {
-    $path = $this->aliasManager->getPathByAlias($this->configFactory->get('system.site')->get('page.403'));
-    $this->makeSubrequest($event, trim($path, '/'), Response::HTTP_FORBIDDEN);
+    $custom_403_path = $this->configFactory->get('system.site')->get('page.403');
+    if (!empty($custom_403_path)) {
+      $this->makeSubrequest($event, $custom_403_path, Response::HTTP_FORBIDDEN);
+    }
   }
 
   /**
    * {@inheritdoc}
    */
   public function on404(GetResponseForExceptionEvent $event) {
-    $path = $this->aliasManager->getPathByAlias($this->configFactory->get('system.site')->get('page.404'));
-    $this->makeSubrequest($event, trim($path, '/'), Response::HTTP_NOT_FOUND);
+    $custom_404_path = $this->configFactory->get('system.site')->get('page.404');
+    if (!empty($custom_404_path)) {
+      $this->makeSubrequest($event, $custom_404_path, Response::HTTP_NOT_FOUND);
+    }
   }
 
 }

core/lib/Drupal/Core/EventSubscriber/DefaultExceptionHtmlSubscriber.php

@@ -7,16 +7,14 @@
 
 namespace Drupal\Core\EventSubscriber;
 
-use Drupal\Core\Routing\AccessAwareRouterInterface;
 use Drupal\Core\Routing\RedirectDestinationInterface;
-use Drupal\Core\Url;
 use Drupal\Core\Utility\Error;
 use Psr\Log\LoggerInterface;
-use Symfony\Component\HttpFoundation\Request;
 use Symfony\Component\HttpFoundation\Response;
 use Symfony\Component\HttpKernel\Event\GetResponseForExceptionEvent;
 use Symfony\Component\HttpKernel\HttpKernelInterface;
 use Symfony\Component\HttpKernel\Exception\HttpExceptionInterface;
+use Symfony\Component\Routing\Matcher\UrlMatcherInterface;
 
 /**
  * Exception subscriber for handling core default HTML error pages.
@@ -44,6 +42,13 @@ class DefaultExceptionHtmlSubscriber extends HttpExceptionSubscriberBase {
    */
   protected $redirectDestination;
 
+  /**
+   * A router implementation which does not check access.
+   *
+   * @var \Symfony\Component\Routing\Matcher\UrlMatcherInterface
+   */
+  protected $accessUnawareRouter;
+
   /**
    * Constructs a new DefaultExceptionHtmlSubscriber.
    *
@@ -53,11 +58,14 @@ class DefaultExceptionHtmlSubscriber extends HttpExceptionSubscriberBase {
    *   The logger service.
    * @param \Drupal\Core\Routing\RedirectDestinationInterface $redirect_destination
    *   The redirect destination service.
+   * @param \Symfony\Component\Routing\Matcher\UrlMatcherInterface $access_unaware_router
+   *   A router implementation which does not check access.
    */
-  public function __construct(HttpKernelInterface $http_kernel, LoggerInterface $logger, RedirectDestinationInterface $redirect_destination) {
+  public function __construct(HttpKernelInterface $http_kernel, LoggerInterface $logger, RedirectDestinationInterface $redirect_destination, UrlMatcherInterface $access_unaware_router) {
     $this->httpKernel = $http_kernel;
     $this->logger = $logger;
     $this->redirectDestination = $redirect_destination;
+    $this->accessUnawareRouter = $access_unaware_router;
   }
 
   /**
@@ -83,7 +91,7 @@ protected function getHandledFormats() {
    *   The event to process.
    */
   public function on401(GetResponseForExceptionEvent $event) {
-    $this->makeSubrequest($event, Url::fromRoute('system.401')->toString(), Response::HTTP_UNAUTHORIZED);
+    $this->makeSubrequest($event, '/system/401', Response::HTTP_UNAUTHORIZED);
   }
 
   /**
@@ -93,7 +101,7 @@ public function on401(GetResponseForExceptionEvent $event) {
    *   The event to process.
    */
   public function on403(GetResponseForExceptionEvent $event) {
-    $this->makeSubrequest($event, Url::fromRoute('system.403')->toString(), Response::HTTP_FORBIDDEN);
+    $this->makeSubrequest($event, '/system/403', Response::HTTP_FORBIDDEN);
   }
 
   /**
@@ -103,7 +111,7 @@ public function on403(GetResponseForExceptionEvent $event) {
    *   The event to process.
    */
   public function on404(GetResponseForExceptionEvent $event) {
-    $this->makeSubrequest($event, Url::fromRoute('system.404')->toString(), Response::HTTP_NOT_FOUND);
+    $this->makeSubrequest($event, '/system/404', Response::HTTP_NOT_FOUND);
   }
 
   /**
@@ -120,54 +128,46 @@ protected function makeSubrequest(GetResponseForExceptionEvent $event, $url, $st
     $request = $event->getRequest();
     $exception = $event->getException();
 
-    if (!($url && $url[0] == '/')) {
-      $url = $request->getBasePath() . '/' . $url;
-    }
-
-    $current_url = $request->getBasePath() . $request->getPathInfo();
-
-    if ($url != $request->getBasePath() . '/' && $url != $current_url) {
-      if ($request->getMethod() === 'POST') {
-        $sub_request = Request::create($url, 'POST', $this->redirectDestination->getAsArray() + ['_exception_statuscode' => $status_code] + $request->request->all(), $request->cookies->all(), [], $request->server->all());
-      }
-      else {
-        $sub_request = Request::create($url, 'GET', $request->query->all() + $this->redirectDestination->getAsArray() + ['_exception_statuscode' => $status_code], $request->cookies->all(), [], $request->server->all());
+    try {
+      // Reuse the exact same request (so keep the same URL, keep the access
+      // result, the exception, et cetera) but override the routing information.
+      // This means that aside from routing, this is identical to the master
+      // request. This allows us to generate a response that is executed on
+      // behalf of the master request, i.e. for the original URL. This is what
+      // allows us to e.g. generate a 404 response for the original URL; if we
+      // would execute a subrequest with the 404 route's URL, then it'd be
+      // generated for *that* URL, not the *original* URL.
+      $sub_request = clone $request;
+      $sub_request->attributes->add($this->accessUnawareRouter->match($url));
+
+      // Add to query (GET) or request (POST) parameters:
+      // - 'destination' (to ensure e.g. the login form in a 403 response
+      //   redirects to the original URL)
+      // - '_exception_statuscode'
+      $parameters = $sub_request->isMethod('GET') ? $sub_request->query : $sub_request->request;
+      $parameters->add($this->redirectDestination->getAsArray() + ['_exception_statuscode' => $status_code]);
+
+      $response = $this->httpKernel->handle($sub_request, HttpKernelInterface::SUB_REQUEST);
+      // Only 2xx responses should have their status code overridden; any
+      // other status code should be passed on: redirects (3xx), error (5xx)…
+      // @see https://www.drupal.org/node/2603788#comment-10504916
+      if ($response->isSuccessful()) {
+        $response->setStatusCode($status_code);
       }
 
-      try {
-        // Persist the 'exception' attribute to the subrequest.
-        $sub_request->attributes->set('exception', $request->attributes->get('exception'));
-        // Persist the access result attribute to the subrequest, so that the
-        // error page inherits the access result of the master request.
-        $sub_request->attributes->set(AccessAwareRouterInterface::ACCESS_RESULT, $request->attributes->get(AccessAwareRouterInterface::ACCESS_RESULT));
-
-        // Carry over the session to the subrequest.
-        if ($session = $request->getSession()) {
-          $sub_request->setSession($session);
-        }
-
-        $response = $this->httpKernel->handle($sub_request, HttpKernelInterface::SUB_REQUEST);
-        // Only 2xx responses should have their status code overridden; any
-        // other status code should be passed on: redirects (3xx), error (5xx)…
-        // @see https://www.drupal.org/node/2603788#comment-10504916
-        if ($response->isSuccessful()) {
-          $response->setStatusCode($status_code);
-        }
-
-        // Persist any special HTTP headers that were set on the exception.
-        if ($exception instanceof HttpExceptionInterface) {
-          $response->headers->add($exception->getHeaders());
-        }
-
-        $event->setResponse($response);
-      }
-      catch (\Exception $e) {
-        // If an error happened in the subrequest we can't do much else. Instead,
-        // just log it. The DefaultExceptionSubscriber will catch the original
-        // exception and handle it normally.
-        $error = Error::decodeException($e);
-        $this->logger->log($error['severity_level'], '%type: @message in %function (line %line of %file).', $error);
+      // Persist any special HTTP headers that were set on the exception.
+      if ($exception instanceof HttpExceptionInterface) {
+        $response->headers->add($exception->getHeaders());
       }
+
+      $event->setResponse($response);
+    }
+    catch (\Exception $e) {
+      // If an error happened in the subrequest we can't do much else. Instead,
+      // just log it. The DefaultExceptionSubscriber will catch the original
+      // exception and handle it normally.
+      $error = Error::decodeException($e);
+      $this->logger->log($error['severity_level'], '%type: @message in %function (line %line of %file).', $error);
     }
   }
 

core/modules/system/src/Tests/Routing/DestinationTest.php

@@ -74,7 +74,7 @@ public function testDestination() {
 
     // Make sure that 404 pages do not populate $_GET['destination'] with
     // external URLs.
-    \Drupal::configFactory()->getEditable('system.site')->set('page.404', 'system-test/get-destination')->save();
+    \Drupal::configFactory()->getEditable('system.site')->set('page.404', '/system-test/get-destination')->save();
     $this->drupalGet('http://example.com', ['external' => FALSE]);
     $this->assertResponse(404);
     $this->assertIdentical(Url::fromRoute('<front>')->toString(), $this->getRawContent(), 'External URL is not allowed on 404 pages.');

core/modules/system/src/Tests/Routing/ExceptionHandlingTest.php

@@ -31,6 +31,7 @@ protected function setUp() {
     parent::setUp();
 
     $this->installSchema('system', ['router']);
+    $this->installEntitySchema('date_format');
     \Drupal::service('router.builder')->rebuild();
   }
 
@@ -98,6 +99,42 @@ public function testHtml404() {
     $this->assertEqual($response->headers->get('Content-type'), 'text/html; charset=UTF-8');
   }
 
+  /**
+   * Tests that the exception response is executed in the original context.
+   */
+  public function testExceptionResponseGeneratedForOriginalRequest() {
+    // Test with 404 path pointing to a route that uses '_controller'.
+    $response = $this->doTest404Route('/router_test/test25');
+    $this->assertTrue(strpos($response->getContent(), '/not-found') !== FALSE);
+
+    // Test with 404 path pointing to a route that uses '_form'.
+    $response = $this->doTest404Route('/router_test/test26');
+    $this->assertTrue(strpos($response->getContent(), '<form class="system-logging-settings"') !== FALSE);
+
+    // Test with 404 path pointing to a route that uses '_entity_form'.
+    $response = $this->doTest404Route('/router_test/test27');
+    $this->assertTrue(strpos($response->getContent(), '<form class="date-format-add-form date-format-form"') !== FALSE);
+  }
+
+  /**
+   * Sets the given path to use as the 404 page and triggers a 404.
+   *
+   * @param string $path
+   * @return \Drupal\Core\Render\HtmlResponse
+   *
+   * @see \Drupal\system\Tests\Routing\ExceptionHandlingTest::testExceptionResponseGeneratedForOriginalRequest()
+   */
+  protected function doTest404Route($path) {
+    $this->config('system.site')->set('page.404', $path)->save();
+
+    $request = Request::create('/not-found');
+    $request->setFormat('html', ['text/html']);
+
+    /** @var \Symfony\Component\HttpKernel\HttpKernelInterface $kernel */
+    $kernel = \Drupal::getContainer()->get('http_kernel');
+    return $kernel->handle($request)->prepare($request);
+  }
+
   /**
    * Tests if exception backtraces are properly escaped when output to HTML.
    */

core/modules/system/src/Tests/System/AccessDeniedTest.php

@@ -44,6 +44,14 @@ function testAccessDenied() {
     $this->assertText(t('Access denied'), 'Found the default 403 page');
     $this->assertResponse(403);
 
+    // Ensure that users without permission are denied access and have the
+    // correct path information in drupalSettings.
+    $this->drupalLogin($this->createUser([]));
+    $this->drupalGet('admin', ['query' => ['foo' => 'bar']]);
+    $this->assertEqual($this->drupalSettings['path']['currentPath'], 'admin');
+    $this->assertEqual($this->drupalSettings['path']['currentPathIsAdmin'], TRUE);
+    $this->assertEqual($this->drupalSettings['path']['currentQuery'], ['foo' => 'bar']);
+
     $this->drupalLogin($this->adminUser);
 
     // Set a custom 404 page without a starting slash.

core/modules/system/system.module

@@ -17,6 +17,7 @@
 use Drupal\Core\PageCache\RequestPolicyInterface;
 use Drupal\Core\PhpStorage\PhpStorageFactory;
 use Drupal\Core\Routing\RouteMatchInterface;
+use Drupal\Core\Routing\StackedRouteMatchInterface;
 use Drupal\Core\Language\LanguageInterface;
 use Drupal\Core\Menu\MenuTreeParameters;
 use Drupal\Core\Extension\ModuleHandler;
@@ -646,7 +647,9 @@ function system_js_settings_build(&$settings, AttachedAssetsInterface $assets) {
  * as well as theme_token ajax state.
  */
 function system_js_settings_alter(&$settings, AttachedAssetsInterface $assets) {
-  $request = \Drupal::request();
+  // As this is being output in the final response always use the master
+  // request.
+  $request = \Drupal::requestStack()->getMasterRequest();
   $current_query = $request->query->all();
 
   // Let output path processors set a prefix.
@@ -656,8 +659,12 @@ function system_js_settings_alter(&$settings, AttachedAssetsInterface $assets) {
   $path_processor->processOutbound('/', $options);
   $pathPrefix = $options['prefix'];
 
-  $current_path = \Drupal::routeMatch()->getRouteName() ? Url::fromRouteMatch(\Drupal::routeMatch())->getInternalPath() : '';
-  $current_path_is_admin = \Drupal::service('router.admin_context')->isAdminRoute();
+  $route_match = \Drupal::routeMatch();
+  if ($route_match instanceof StackedRouteMatchInterface) {
+    $route_match = $route_match->getMasterRouteMatch();
+  }
+  $current_path = $route_match->getRouteName() ? Url::fromRouteMatch($route_match)->getInternalPath() : '';
+  $current_path_is_admin = \Drupal::service('router.admin_context')->isAdminRoute($route_match->getRouteObject());
   $path_settings = [
     'baseUrl' => $request->getBaseUrl() . '/',
     'pathPrefix' => $pathPrefix,

core/modules/system/tests/modules/router_test_directory/router_test.routing.yml

@@ -155,6 +155,29 @@ router_test.24:
   requirements:
     _access: 'TRUE'
 
+router_test.25:
+  path: '/router_test/test25'
+  defaults:
+    _controller: '\Drupal\router_test\TestControllers::test25'
+  requirements:
+    _access: 'TRUE'
+
+router_test.26:
+  path: '/router_test/test26'
+  defaults:
+    _form: 'Drupal\system\Form\LoggingForm'
+    _title: 'Cron'
+  requirements:
+    _access: 'TRUE'
+
+router_test.27:
+  path: '/router_test/test27'
+  defaults:
+    _entity_form: 'date_format.add'
+    _title: 'Add date format'
+  requirements:
+    _access: 'TRUE'
+
 router_test.hierarchy_parent:
   path: '/menu-test/parent'
   defaults:

core/modules/system/tests/modules/router_test_directory/src/TestControllers.php

@@ -106,6 +106,15 @@ public function test24() {
     throw new \Exception('Escaped content: <p> <br> <h3>');
   }
 
+  public function test25() {
+    return [
+      '#cache' => [
+        'url',
+      ],
+      '#markup' => \Drupal::requestStack()->getCurrentRequest()->getUri(),
+    ];
+  }
+
   /**
    * Throws an exception.
    *

core/tests/Drupal/Tests/Core/EventSubscriber/CustomPageExceptionHtmlSubscriberTest.php

@@ -35,13 +35,6 @@ class CustomPageExceptionHtmlSubscriberTest extends UnitTestCase {
    */
   protected $configFactory;
 
-  /**
-   * The mocked alias manager.
-   *
-   * @var \Drupal\Core\Path\AliasManagerInterface|\PHPUnit_Framework_MockObject_MockObject
-   */
-  protected $aliasManager;
-
   /**
    * The mocked logger.
    *
@@ -70,22 +63,32 @@ class CustomPageExceptionHtmlSubscriberTest extends UnitTestCase {
    */
   protected $redirectDestination;
 
+  /**
+   * The mocked access unaware router.
+   * @var \Symfony\Component\Routing\Matcher\UrlMatcherInterface|\PHPUnit_Framework_MockObject_MockObject
+   */
+  protected $accessUnawareRouter;
+
   /**
    * {@inheritdoc}
    */
   protected function setUp() {
-    $this->configFactory = $this->getConfigFactoryStub(['system.site' => ['page.403' => 'access-denied-page', 'page.404' => 'not-found-page']]);
+    $this->configFactory = $this->getConfigFactoryStub(['system.site' => ['page.403' => '/access-denied-page', 'page.404' => '/not-found-page']]);
 
-    $this->aliasManager = $this->getMock('Drupal\Core\Path\AliasManagerInterface');
     $this->kernel = $this->getMock('Symfony\Component\HttpKernel\HttpKernelInterface');
     $this->logger = $this->getMock('Psr\Log\LoggerInterface');
     $this->redirectDestination = $this->getMock('\Drupal\Core\Routing\RedirectDestinationInterface');
-
     $this->redirectDestination->expects($this->any())
       ->method('getAsArray')
       ->willReturn(['destination' => 'test']);
+    $this->accessUnawareRouter = $this->getMock('Symfony\Component\Routing\Matcher\UrlMatcherInterface');
+    $this->accessUnawareRouter->expects($this->any())
+      ->method('match')
+      ->willReturn([
+        '_controller' => 'mocked',
+      ]);
 
-    $this->customPageSubscriber = new CustomPageExceptionHtmlSubscriber($this->configFactory, $this->aliasManager, $this->kernel, $this->logger, $this->redirectDestination);
+    $this->customPageSubscriber = new CustomPageExceptionHtmlSubscriber($this->configFactory, $this->kernel, $this->logger, $this->redirectDestination, $this->accessUnawareRouter);
 
     // You can't create an exception in PHP without throwing it. Store the
     // current error_log, and disable it temporarily.
@@ -99,21 +102,10 @@ protected function tearDown() {
     ini_set('error_log', $this->errorLog);
   }
 
-  /**
-   * Sets up an alias manager that does nothing.
-   */
-  protected function setupStubAliasManager() {
-    $this->aliasManager->expects($this->any())
-      ->method('getPathByAlias')
-      ->willReturnArgument(0);
-  }
-
   /**
    * Tests onHandleException with a POST request.
    */
   public function testHandleWithPostRequest() {
-    $this->setupStubAliasManager();
-
     $request = Request::create('/test', 'POST', array('name' => 'druplicon', 'pass' => '12345'));
 
     $this->kernel->expects($this->once())->method('handle')->will($this->returnCallback(function (Request $request) {
@@ -133,8 +125,6 @@ public function testHandleWithPostRequest() {
    * Tests onHandleException with a GET request.
    */
   public function testHandleWithGetRequest() {
-    $this->setupStubAliasManager();
-
     $request = Request::create('/test', 'GET', array('name' => 'druplicon', 'pass' => '12345'));
 
     $this->kernel->expects($this->once())->method('handle')->will($this->returnCallback(function (Request $request) {