diff --git a/core/core.services.yml b/core/core.services.yml index afccb31..e28d805 100644 --- a/core/core.services.yml +++ b/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: diff --git a/core/lib/Drupal/Core/EventSubscriber/CustomPageExceptionHtmlSubscriber.php b/core/lib/Drupal/Core/EventSubscriber/CustomPageExceptionHtmlSubscriber.php index 8422db2..0aef10c 100644 --- a/core/lib/Drupal/Core/EventSubscriber/CustomPageExceptionHtmlSubscriber.php +++ b/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. @@ -28,30 +29,22 @@ 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); + } } } diff --git a/core/lib/Drupal/Core/EventSubscriber/DefaultExceptionHtmlSubscriber.php b/core/lib/Drupal/Core/EventSubscriber/DefaultExceptionHtmlSubscriber.php index 2ae2114..64f2e97 100644 --- a/core/lib/Drupal/Core/EventSubscriber/DefaultExceptionHtmlSubscriber.php +++ b/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. @@ -45,6 +43,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. * * @param \Symfony\Component\HttpKernel\HttpKernelInterface $http_kernel @@ -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); } } diff --git a/core/modules/system/src/Tests/Routing/DestinationTest.php b/core/modules/system/src/Tests/Routing/DestinationTest.php index a590d62..ce72f7b 100644 --- a/core/modules/system/src/Tests/Routing/DestinationTest.php +++ b/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('')->toString(), $this->getRawContent(), 'External URL is not allowed on 404 pages.'); diff --git a/core/modules/system/src/Tests/Routing/ExceptionHandlingTest.php b/core/modules/system/src/Tests/Routing/ExceptionHandlingTest.php index ee3b6eb..2115e8f 100644 --- a/core/modules/system/src/Tests/Routing/ExceptionHandlingTest.php +++ b/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(); } @@ -99,6 +100,42 @@ public function testHtml404() { } /** + * 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(), '
doTest404Route('/router_test/test27'); + $this->assertTrue(strpos($response->getContent(), '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. */ public function testBacktraceEscaping() { diff --git a/core/modules/system/src/Tests/System/AccessDeniedTest.php b/core/modules/system/src/Tests/System/AccessDeniedTest.php index b068802..43090eb 100644 --- a/core/modules/system/src/Tests/System/AccessDeniedTest.php +++ b/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. diff --git a/core/modules/system/system.module b/core/modules/system/system.module index d3b55f9..3901a86 100644 --- a/core/modules/system/system.module +++ b/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, diff --git a/core/modules/system/tests/modules/router_test_directory/router_test.routing.yml b/core/modules/system/tests/modules/router_test_directory/router_test.routing.yml index e0c91dd..ad2d418 100644 --- a/core/modules/system/tests/modules/router_test_directory/router_test.routing.yml +++ b/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: diff --git a/core/modules/system/tests/modules/router_test_directory/src/TestControllers.php b/core/modules/system/tests/modules/router_test_directory/src/TestControllers.php index 40fad0b..d603de8 100644 --- a/core/modules/system/tests/modules/router_test_directory/src/TestControllers.php +++ b/core/modules/system/tests/modules/router_test_directory/src/TestControllers.php @@ -106,6 +106,15 @@ public function test24() { throw new \Exception('Escaped content:


'); } + public function test25() { + return [ + '#cache' => [ + 'url', + ], + '#markup' => \Drupal::requestStack()->getCurrentRequest()->getUri(), + ]; + } + /** * Throws an exception. * diff --git a/core/tests/Drupal/Tests/Core/EventSubscriber/CustomPageExceptionHtmlSubscriberTest.php b/core/tests/Drupal/Tests/Core/EventSubscriber/CustomPageExceptionHtmlSubscriberTest.php index a7efb7e..cdcf5b4 100644 --- a/core/tests/Drupal/Tests/Core/EventSubscriber/CustomPageExceptionHtmlSubscriberTest.php +++ b/core/tests/Drupal/Tests/Core/EventSubscriber/CustomPageExceptionHtmlSubscriberTest.php @@ -36,13 +36,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. * * @var \Psr\Log\LoggerInterface @@ -71,21 +64,31 @@ 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. @@ -100,20 +103,9 @@ protected function tearDown() { } /** - * 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) {