diff --git a/core/core.services.yml b/core/core.services.yml index 32fefbc281..c842acf78d 100644 --- a/core/core.services.yml +++ b/core/core.services.yml @@ -1221,7 +1221,7 @@ services: class: Drupal\Core\Access\CsrfAccessCheck tags: - { name: access_check, applies_to: _csrf_token, needs_incoming_request: TRUE } - arguments: ['@csrf_token'] + arguments: ['@csrf_token', '@session_configuration'] access_check.header.csrf: class: Drupal\Core\Access\CsrfRequestHeaderAccessCheck arguments: ['@session_configuration', '@csrf_token'] @@ -1367,7 +1367,7 @@ services: class: Drupal\Core\Access\RouteProcessorCsrf tags: - { name: route_processor_outbound } - arguments: ['@csrf_token'] + arguments: ['@csrf_token', '@session_configuration', '@request_stack'] transliteration: class: Drupal\Core\Transliteration\PhpTransliteration arguments: [null, '@module_handler'] diff --git a/core/lib/Drupal/Core/Access/CsrfAccessCheck.php b/core/lib/Drupal/Core/Access/CsrfAccessCheck.php index 48e75acb58..2863091b2f 100644 --- a/core/lib/Drupal/Core/Access/CsrfAccessCheck.php +++ b/core/lib/Drupal/Core/Access/CsrfAccessCheck.php @@ -4,6 +4,7 @@ use Drupal\Core\Routing\Access\AccessInterface as RoutingAccessInterface; use Drupal\Core\Routing\RouteMatchInterface; +use Drupal\Core\Session\SessionConfigurationInterface; use Symfony\Component\Routing\Route; use Symfony\Component\HttpFoundation\Request; @@ -23,14 +24,24 @@ class CsrfAccessCheck implements RoutingAccessInterface { */ protected $csrfToken; + /** + * The session configuration. + * + * @var \Drupal\Core\Session\SessionConfigurationInterface + */ + protected $sessionConfiguration; + /** * Constructs a CsrfAccessCheck object. * * @param \Drupal\Core\Access\CsrfTokenGenerator $csrf_token * The CSRF token generator. + * @param \Drupal\Core\Session\SessionConfigurationInterface $session_configuration + * The session configuration. */ - public function __construct(CsrfTokenGenerator $csrf_token) { + public function __construct(CsrfTokenGenerator $csrf_token, SessionConfigurationInterface $session_configuration) { $this->csrfToken = $csrf_token; + $this->sessionConfiguration = $session_configuration; } /** @@ -54,7 +65,9 @@ public function access(Route $route, Request $request, RouteMatchInterface $rout $path = str_replace("{{$param}}", $value, $path); } - if ($this->csrfToken->validate($request->query->get('token', ''), $path)) { + // Per https://www.drupal.org/node/2319205 anonymous users do not need + // CSRF checking. + if (!$this->sessionConfiguration->hasSession($request) || $this->csrfToken->validate($request->query->get('token', ''), $path)) { $result = AccessResult::allowed(); } else { diff --git a/core/lib/Drupal/Core/Access/RouteProcessorCsrf.php b/core/lib/Drupal/Core/Access/RouteProcessorCsrf.php index d0aec30185..f06dd543e8 100644 --- a/core/lib/Drupal/Core/Access/RouteProcessorCsrf.php +++ b/core/lib/Drupal/Core/Access/RouteProcessorCsrf.php @@ -6,6 +6,8 @@ use Drupal\Core\Render\BubbleableMetadata; use Drupal\Core\Security\TrustedCallbackInterface; use Drupal\Core\RouteProcessor\OutboundRouteProcessorInterface; +use Drupal\Core\Session\SessionConfigurationInterface; +use Symfony\Component\HttpFoundation\RequestStack; use Symfony\Component\Routing\Route; /** @@ -20,21 +22,43 @@ class RouteProcessorCsrf implements OutboundRouteProcessorInterface, TrustedCall */ protected $csrfToken; + /** + * The session configuration. + * + * @var \Drupal\Core\Session\SessionConfigurationInterface + */ + protected $sessionConfiguration; + + /** + * The request stack. + * + * @var \Symfony\Component\HttpFoundation\RequestStack + */ + protected $requestStack; + /** * Constructs a RouteProcessorCsrf object. * * @param \Drupal\Core\Access\CsrfTokenGenerator $csrf_token * The CSRF token generator. + * @param \Drupal\Core\Session\SessionConfigurationInterface $session_configuration + * The session configuration. + * @param \Symfony\Component\HttpFoundation\RequestStack $request_stack + * The request stack. */ - public function __construct(CsrfTokenGenerator $csrf_token) { + public function __construct(CsrfTokenGenerator $csrf_token, SessionConfigurationInterface $session_configuration, RequestStack $request_stack) { $this->csrfToken = $csrf_token; + $this->sessionConfiguration = $session_configuration; + $this->requestStack = $request_stack; } /** * {@inheritdoc} */ public function processOutbound($route_name, Route $route, array &$parameters, BubbleableMetadata $bubbleable_metadata = NULL) { - if ($route->hasRequirement('_csrf_token')) { + // Per https://www.drupal.org/node/2319205 anonymous users do not need + // CSRF checking. + if ($route->hasRequirement('_csrf_token') && $this->sessionConfiguration->hasSession($this->requestStack->getCurrentRequest())) { $path = ltrim($route->getPath(), '/'); // Replace the path parameters with values from the parameters array. foreach ($parameters as $param => $value) { @@ -49,7 +73,10 @@ public function processOutbound($route_name, Route $route, array &$parameters, B // Generate a placeholder and a render array to replace it. $placeholder = Crypt::hashBase64($path); $placeholder_render_array = [ - '#lazy_builder' => ['route_processor_csrf:renderPlaceholderCsrfToken', [$path]], + '#lazy_builder' => [ + 'route_processor_csrf:renderPlaceholderCsrfToken', + (array) $path, + ], ]; // Instead of setting an actual CSRF token as the query string, we set @@ -62,6 +89,8 @@ public function processOutbound($route_name, Route $route, array &$parameters, B } /** + * Gets a CSRF token for the given path. + * * #lazy_builder callback; gets a CSRF token for the given path. * * @param string $path diff --git a/core/modules/menu_link_content/tests/src/Kernel/MenuLinkContentCacheabilityBubblingTest.php b/core/modules/menu_link_content/tests/src/Kernel/MenuLinkContentCacheabilityBubblingTest.php index 27b49b27ee..f22de3db77 100644 --- a/core/modules/menu_link_content/tests/src/Kernel/MenuLinkContentCacheabilityBubblingTest.php +++ b/core/modules/menu_link_content/tests/src/Kernel/MenuLinkContentCacheabilityBubblingTest.php @@ -15,6 +15,8 @@ use Symfony\Component\Routing\Route; /** + * The menu link content cache bubbling kernel tests. + * * Ensures that rendered menu links bubble the necessary bubbleable metadata * for outbound path/route processing. * @@ -61,6 +63,8 @@ public function testOutboundPathAndRouteProcessing() { $request = Request::create('/'); $request->attributes->set(RouteObjectInterface::ROUTE_NAME, ''); $request->attributes->set(RouteObjectInterface::ROUTE_OBJECT, new Route('/')); + // Fake a started session. + $request->cookies->add(['SESS' . substr(hash('sha256', $this->getDatabasePrefix()), 0, 32) => '']); $request_stack->push($request); $request_context->fromRequest($request); @@ -70,7 +74,11 @@ public function testOutboundPathAndRouteProcessing() { $default_menu_cacheability = (new BubbleableMetadata()) ->setCacheMaxAge(Cache::PERMANENT) ->setCacheTags(['config:system.menu.tools']) - ->setCacheContexts(['languages:' . LanguageInterface::TYPE_INTERFACE, 'theme', 'user.permissions']); + ->setCacheContexts([ + 'languages:' . LanguageInterface::TYPE_INTERFACE, + 'theme', + 'user.permissions', + ]); User::create(['uid' => 1, 'name' => $this->randomString()])->save(); User::create(['uid' => 2, 'name' => $this->randomString()])->save(); @@ -85,7 +93,8 @@ public function testOutboundPathAndRouteProcessing() { // cacheability metadata of the same type is working (two links with cache // tags). $test_cases = [ - // \Drupal\Core\RouteProcessor\RouteProcessorCurrent: 'route' cache context. + // \Drupal\Core\RouteProcessor\RouteProcessorCurrent: 'route' cache + // context. [ 'uri' => 'route:', 'cacheability' => (new BubbleableMetadata())->setCacheContexts(['route']), diff --git a/core/modules/system/tests/src/Kernel/Common/UrlTest.php b/core/modules/system/tests/src/Kernel/Common/UrlTest.php index 76c5ffc45c..57bcfed73e 100644 --- a/core/modules/system/tests/src/Kernel/Common/UrlTest.php +++ b/core/modules/system/tests/src/Kernel/Common/UrlTest.php @@ -12,6 +12,8 @@ use Drupal\KernelTests\KernelTestBase; /** + * Url kernel tests. + * * Confirm that \Drupal\Core\Url, * \Drupal\Component\Utility\UrlHelper::filterQueryParameters(), * \Drupal\Component\Utility\UrlHelper::buildQuery(), and @@ -22,12 +24,17 @@ */ class UrlTest extends KernelTestBase { + /** + * The modules list. + * + * @var array + */ protected static $modules = ['common_test', 'url_alter_test']; /** * Confirms that invalid URLs are filtered in link generating functions. */ - public function testLinkXSS() { + public function testLinkXss() { // Test link generator. $text = $this->randomMachineName(); $path = ""; @@ -48,19 +55,84 @@ public function testLinkXSS() { */ public function testLinkBubbleableMetadata() { \Drupal::service('module_installer')->install(['user']); + // Fake a started session. + \Drupal::request()->cookies->add(['SESS' . substr(hash('sha256', $this->getDatabasePrefix()), 0, 32) => '']); $cases = [ - ['Regular link', 'internal:/user', [], ['contexts' => [], 'tags' => [], 'max-age' => Cache::PERMANENT], []], - ['Regular link, absolute', 'internal:/user', ['absolute' => TRUE], ['contexts' => ['url.site'], 'tags' => [], 'max-age' => Cache::PERMANENT], []], - ['Route processor link', 'route:system.run_cron', [], ['contexts' => ['session'], 'tags' => [], 'max-age' => Cache::PERMANENT], ['placeholders' => []]], - ['Route processor link, absolute', 'route:system.run_cron', ['absolute' => TRUE], ['contexts' => ['url.site', 'session'], 'tags' => [], 'max-age' => Cache::PERMANENT], ['placeholders' => []]], - ['Path processor link', 'internal:/user/1', [], ['contexts' => [], 'tags' => ['user:1'], 'max-age' => Cache::PERMANENT], []], - ['Path processor link, absolute', 'internal:/user/1', ['absolute' => TRUE], ['contexts' => ['url.site'], 'tags' => ['user:1'], 'max-age' => Cache::PERMANENT], []], + [ + 'Regular link', + 'internal:/user', + [], + [ + 'contexts' => [], + 'tags' => [], + 'max-age' => Cache::PERMANENT, + ], + [], + ], + [ + 'Regular link, absolute', + 'internal:/user', + ['absolute' => TRUE], + [ + 'contexts' => ['url.site'], + 'tags' => [], + 'max-age' => Cache::PERMANENT, + ], + [], + ], + [ + 'Route processor link', + 'route:system.run_cron', + [], + [ + 'contexts' => ['session'], + 'tags' => [], + 'max-age' => Cache::PERMANENT, + ], + ['placeholders' => []], + ], + [ + 'Route processor link, absolute', + 'route:system.run_cron', + ['absolute' => TRUE], + [ + 'contexts' => ['url.site', 'session'], + 'tags' => [], + 'max-age' => Cache::PERMANENT, + ], + ['placeholders' => []], + ], + [ + 'Path processor link', + 'internal:/user/1', + [], + [ + 'contexts' => [], + 'tags' => ['user:1'], + 'max-age' => Cache::PERMANENT, + ], + [], + ], + [ + 'Path processor link, absolute', + 'internal:/user/1', + ['absolute' => TRUE], + [ + 'contexts' => ['url.site'], + 'tags' => ['user:1'], + 'max-age' => Cache::PERMANENT, + ], + [], + ], ]; foreach ($cases as $case) { [$title, $uri, $options, $expected_cacheability, $expected_attachments] = $case; - $expected_cacheability['contexts'] = Cache::mergeContexts($expected_cacheability['contexts'], ['languages:language_interface', 'theme', 'user.permissions']); + $expected_cacheability['contexts'] = Cache::mergeContexts( + $expected_cacheability['contexts'], + ['languages:language_interface', 'theme', 'user.permissions'] + ); $link = [ '#type' => 'link', '#title' => $title, @@ -137,6 +209,7 @@ public function testLinkRenderArrayText() { $l = Link::fromTextAndUrl('foo', Url::fromUri('https://www.drupal.org'))->toString(); // Test a renderable array passed to the link generator. + // @codingStandardsIgnoreLine $renderer->executeInRenderContext(new RenderContext(), function () use ($renderer, $l) { $renderable_text = ['#markup' => 'foo']; $l_renderable_text = \Drupal::service('link_generator')->generate($renderable_text, Url::fromUri('https://www.drupal.org')); @@ -165,11 +238,11 @@ public function testLinkRenderArrayText() { /** * Checks for class existence in link. * - * @param $attribute + * @param string $attribute * Attribute to be checked. - * @param $link + * @param \Drupal\Core\Render\RenderableInterface|string $link * URL to search. - * @param $class + * @param string $class * Element class to search for. * * @return bool @@ -212,7 +285,12 @@ public function testDrupalGetQueryParameters() { // Multiple exclusions. $result = $original; unset($result['a'], $result['b']['e'], $result['c']); - $this->assertEquals(UrlHelper::filterQueryParameters($original, ['a', 'b[e]', 'c']), $result, "'a', 'b[e]', 'c' were removed."); + $this->assertEquals( + UrlHelper::filterQueryParameters( + $original, + ['a', 'b[e]', 'c'] + ), $result, "'a', 'b[e]', 'c' were removed." + ); } /** @@ -244,11 +322,13 @@ public function testDrupalParseUrl() { ]; $this->assertEquals($result, UrlHelper::parse($url), 'Relative URL parsed correctly.'); - // Test that drupal can recognize an absolute URL. Used to prevent attack vectors. + // Test that drupal can recognize an absolute URL. Used to prevent attack + // vectors. $url = 'https://www.drupal.org/foo/bar?foo=bar&bar=baz&baz#foo'; $this->assertTrue(UrlHelper::isExternal($url), 'Correctly identified an external URL.'); - // Test that UrlHelper::parse() does not allow spoofing a URL to force a malicious redirect. + // Test that UrlHelper::parse() does not allow spoofing a URL to force a + // malicious redirect. $parts = UrlHelper::parse('forged:http://cwe.mitre.org/data/definitions/601.html'); $this->assertFalse(UrlHelper::isValid($parts['path'], TRUE), '\Drupal\Component\Utility\UrlHelper::isValid() correctly parsed a forged URL.'); } diff --git a/core/tests/Drupal/Tests/Core/Access/CsrfAccessCheckTest.php b/core/tests/Drupal/Tests/Core/Access/CsrfAccessCheckTest.php index a52d335f19..d79734385e 100644 --- a/core/tests/Drupal/Tests/Core/Access/CsrfAccessCheckTest.php +++ b/core/tests/Drupal/Tests/Core/Access/CsrfAccessCheckTest.php @@ -6,6 +6,9 @@ use Symfony\Component\HttpFoundation\Request; use Symfony\Component\Routing\Route; use Drupal\Core\Access\CsrfAccessCheck; +use Drupal\Core\Access\CsrfTokenGenerator; +use Drupal\Core\Routing\RouteMatchInterface; +use Drupal\Core\Session\SessionConfigurationInterface; use Drupal\Tests\UnitTestCase; /** @@ -35,20 +38,35 @@ class CsrfAccessCheckTest extends UnitTestCase { */ protected $routeMatch; + /** + * The session configuration. + * + * @var \Drupal\Core\Session\SessionConfigurationInterface|\PHPUnit_Framework_MockObject_MockObject + */ + protected $sessionConfiguration; + + /** + * Sets up the necessary mocks. + */ protected function setUp(): void { - $this->csrfToken = $this->getMockBuilder('Drupal\Core\Access\CsrfTokenGenerator') + $this->csrfToken = $this->getMockBuilder(CsrfTokenGenerator::class) ->disableOriginalConstructor() ->getMock(); - $this->routeMatch = $this->createMock('Drupal\Core\Routing\RouteMatchInterface'); + $this->routeMatch = $this->createMock(RouteMatchInterface::class); - $this->accessCheck = new CsrfAccessCheck($this->csrfToken); + $this->sessionConfiguration = $this->createMock(SessionConfigurationInterface::class); + $this->accessCheck = new CsrfAccessCheck($this->csrfToken, $this->sessionConfiguration); } /** * Tests the access() method with a valid token. */ public function testAccessTokenPass() { + $this->sessionConfiguration->expects($this->once()) + ->method('hasSession') + ->willReturn(TRUE); + $this->csrfToken->expects($this->once()) ->method('validate') ->with('test_query', 'test-path/42') @@ -64,10 +82,32 @@ public function testAccessTokenPass() { $this->assertEquals(AccessResult::allowed()->setCacheMaxAge(0), $this->accessCheck->access($route, $request, $this->routeMatch)); } + /** + * Tests the access() method for anonymous users. + */ + public function testAccessTokenAnonymousPass() { + $this->sessionConfiguration->expects($this->once()) + ->method('hasSession') + ->willReturn(FALSE); + $this->routeMatch->expects($this->once()) + ->method('getRawParameters') + ->will($this->returnValue(['node' => 42])); + + $route = new Route('/test-path/{node}', [], ['_csrf_token' => 'TRUE']); + $request = Request::create('/test-path/42?token=test_query'); + $this->csrfToken->expects($this->never()) + ->method('validate'); + $this->assertEquals(AccessResult::allowed()->setCacheMaxAge(0), $this->accessCheck->access($route, $request, $this->routeMatch)); + } + /** * @covers ::access */ public function testCsrfTokenInvalid() { + $this->sessionConfiguration->expects($this->once()) + ->method('hasSession') + ->willReturn(TRUE); + $this->csrfToken->expects($this->once()) ->method('validate') ->with('test_query', 'test-path') @@ -92,6 +132,10 @@ public function testCsrfTokenMissing() { ->with('', 'test-path') ->will($this->returnValue(FALSE)); + $this->sessionConfiguration->expects($this->once()) + ->method('hasSession') + ->willReturn(TRUE); + $this->routeMatch->expects($this->once()) ->method('getRawParameters') ->will($this->returnValue([])); diff --git a/core/tests/Drupal/Tests/Core/Access/RouteProcessorCsrfTest.php b/core/tests/Drupal/Tests/Core/Access/RouteProcessorCsrfTest.php index cadd623b5d..efc4b6f8d0 100644 --- a/core/tests/Drupal/Tests/Core/Access/RouteProcessorCsrfTest.php +++ b/core/tests/Drupal/Tests/Core/Access/RouteProcessorCsrfTest.php @@ -3,9 +3,13 @@ namespace Drupal\Tests\Core\Access; use Drupal\Component\Utility\Crypt; +use Drupal\Core\Access\CsrfTokenGenerator; use Drupal\Core\Render\BubbleableMetadata; use Drupal\Tests\UnitTestCase; use Drupal\Core\Access\RouteProcessorCsrf; +use Drupal\Core\Session\SessionConfigurationInterface; +use Symfony\Component\HttpFoundation\Request; +use Symfony\Component\HttpFoundation\RequestStack; use Symfony\Component\Routing\Route; /** @@ -28,18 +32,39 @@ class RouteProcessorCsrfTest extends UnitTestCase { */ protected $processor; + /** + * The session configuration. + * + * @var \Drupal\Core\Session\SessionConfigurationInterface|\PHPUnit_Framework_MockObject_MockObject + */ + protected $sessionConfiguration; + + /** + * Sets up the necessary mocks. + */ protected function setUp(): void { - $this->csrfToken = $this->getMockBuilder('Drupal\Core\Access\CsrfTokenGenerator') + $this->csrfToken = $this->getMockBuilder(CsrfTokenGenerator::class) ->disableOriginalConstructor() ->getMock(); - $this->processor = new RouteProcessorCsrf($this->csrfToken); + $this->sessionConfiguration = $this->createMock(SessionConfigurationInterface::class); + $request_stack = $this->createMock(RequestStack::class); + // The number this is called differs between tests and is completely + // irrelevant, the sessionConfiguration mock object will have the exact + // number of calls. + $request_stack->expects($this->atMost(1)) + ->method('getCurrentRequest') + ->willReturn($this->createMock(Request::class)); + + $this->processor = new RouteProcessorCsrf($this->csrfToken, $this->sessionConfiguration, $request_stack); } /** * Tests the processOutbound() method with no _csrf_token route requirement. */ public function testProcessOutboundNoRequirement() { + $this->sessionConfiguration->expects($this->never()) + ->method('hasSession'); $this->csrfToken->expects($this->never()) ->method('get'); @@ -59,6 +84,9 @@ public function testProcessOutboundNoRequirement() { * Tests the processOutbound() method with a _csrf_token route requirement. */ public function testProcessOutbound() { + $this->sessionConfiguration->expects($this->once()) + ->method('hasSession') + ->willReturn(TRUE); $route = new Route('/test-path', [], ['_csrf_token' => 'TRUE']); $parameters = []; @@ -71,16 +99,40 @@ public function testProcessOutbound() { $path = 'test-path'; $placeholder = Crypt::hashBase64($path); $placeholder_render_array = [ - '#lazy_builder' => ['route_processor_csrf:renderPlaceholderCsrfToken', [$path]], + '#lazy_builder' => [ + 'route_processor_csrf:renderPlaceholderCsrfToken', + (array) $path, + ], ]; $this->assertSame($parameters['token'], $placeholder); $this->assertEquals((new BubbleableMetadata())->setAttachments(['placeholders' => [$placeholder => $placeholder_render_array]]), $bubbleable_metadata); } + /** + * Tests the processOutbound() method for anonymous users. + */ + public function testProcessOutboundForAnonymous() { + $this->sessionConfiguration->expects($this->once()) + ->method('hasSession') + ->willReturn(FALSE); + $this->csrfToken->expects($this->never()) + ->method('get'); + $route = new Route('/test-path', [], ['_csrf_token' => 'TRUE']); + $parameters = []; + + $bubbleable_metadata = new BubbleableMetadata(); + $this->processor->processOutbound('test', $route, $parameters, $bubbleable_metadata); + $this->assertEmpty($parameters); + $this->assertEquals((new BubbleableMetadata()), $bubbleable_metadata); + } + /** * Tests the processOutbound() method with a dynamic path and one replacement. */ public function testProcessOutboundDynamicOne() { + $this->sessionConfiguration->expects($this->once()) + ->method('hasSession') + ->willReturn(TRUE); $route = new Route('/test-path/{slug}', [], ['_csrf_token' => 'TRUE']); $parameters = ['slug' => 100]; @@ -91,7 +143,10 @@ public function testProcessOutboundDynamicOne() { $path = 'test-path/100'; $placeholder = Crypt::hashBase64($path); $placeholder_render_array = [ - '#lazy_builder' => ['route_processor_csrf:renderPlaceholderCsrfToken', [$path]], + '#lazy_builder' => [ + 'route_processor_csrf:renderPlaceholderCsrfToken', + (array) $path, + ], ]; $this->assertEquals((new BubbleableMetadata())->setAttachments(['placeholders' => [$placeholder => $placeholder_render_array]]), $bubbleable_metadata); } @@ -100,6 +155,9 @@ public function testProcessOutboundDynamicOne() { * Tests the processOutbound() method with two parameter replacements. */ public function testProcessOutboundDynamicTwo() { + $this->sessionConfiguration->expects($this->once()) + ->method('hasSession') + ->willReturn(TRUE); $route = new Route('{slug_1}/test-path/{slug_2}', [], ['_csrf_token' => 'TRUE']); $parameters = ['slug_1' => 100, 'slug_2' => 'test']; @@ -110,7 +168,10 @@ public function testProcessOutboundDynamicTwo() { $path = '100/test-path/test'; $placeholder = Crypt::hashBase64($path); $placeholder_render_array = [ - '#lazy_builder' => ['route_processor_csrf:renderPlaceholderCsrfToken', [$path]], + '#lazy_builder' => [ + 'route_processor_csrf:renderPlaceholderCsrfToken', + (array) $path, + ], ]; $this->assertEquals((new BubbleableMetadata())->setAttachments(['placeholders' => [$placeholder => $placeholder_render_array]]), $bubbleable_metadata); }