diff --git a/core/core.services.yml b/core/core.services.yml index 465d145..8fbe3ea 100644 --- a/core/core.services.yml +++ b/core/core.services.yml @@ -412,6 +412,11 @@ services: class: Drupal\Core\Theme\ThemeAccessCheck tags: - { name: access_check } + access_check.csrf: + class: Drupal\Core\Access\CsrfAccessCheck + tags: + - { name: access_check } + arguments: ['@csrf_token'] maintenance_mode_subscriber: class: Drupal\Core\EventSubscriber\MaintenanceModeSubscriber tags: @@ -488,6 +493,11 @@ services: - { name: path_processor_inbound, priority: 100 } - { name: path_processor_outbound, priority: 300 } arguments: ['@path.alias_manager'] + route_processer_csrf: + class: Drupal\Core\Access\RouteProcessorCsrf + tags: + - { name: route_processor_outbound } + arguments: ['@csrf_token'] transliteration: class: Drupal\Core\Transliteration\PHPTransliteration flood: diff --git a/core/lib/Drupal/Core/Access/CsrfAccessCheck.php b/core/lib/Drupal/Core/Access/CsrfAccessCheck.php new file mode 100644 index 0000000..da7dcba --- /dev/null +++ b/core/lib/Drupal/Core/Access/CsrfAccessCheck.php @@ -0,0 +1,70 @@ +get() using the same value as the + * "_csrf" parameter in the route. + */ +class CsrfAccessCheck implements StaticAccessCheckInterface { + + /** + * The CSRF token generator. + * + * @var \Drupal\Core\Access\CsrfTokenGenerator + */ + protected $csrfToken; + + /** + * Constructs a CsrfAccessCheck object. + * + * @param \Drupal\Core\Access\CsrfTokenGenerator $csrf_token + * The CSRF token generator. + */ + function __construct(CsrfTokenGenerator $csrf_token) { + $this->csrfToken = $csrf_token; + } + + /** + * {@inheritdoc} + */ + public function appliesTo() { + return array('_csrf'); + } + + /** + * {@inheritdoc} + */ + public function access(Route $route, Request $request) { + // If this is the controller request, check CSRF access as normal. + if ($request->attributes->get('_controller_request')) { + return $this->csrfToken->validate($request->query->get('token'), $route->getRequirement('_csrf')) ? static::ALLOW : static::KILL; + } + + // Otherwise, this could be another requested access check that we don't + // want to check CSRF tokens on. + $conjunction = $route->getOption('_access_mode') ?: 'ANY'; + // Return ALLOW if all access checks are needed. + if ($conjunction == 'ALL') { + return static::ALLOW; + } + // Return DENY otherwise, as another access checker should grant access + // for the route. + else { + return static::DENY; + } + } + +} diff --git a/core/lib/Drupal/Core/Access/RouteProcessorCsrf.php b/core/lib/Drupal/Core/Access/RouteProcessorCsrf.php new file mode 100644 index 0000000..b12da28 --- /dev/null +++ b/core/lib/Drupal/Core/Access/RouteProcessorCsrf.php @@ -0,0 +1,49 @@ +csrfToken = $csrf_token; + } + + /** + * {@inheritdoc} + */ + public function processOutboundRoute(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['token'] = $this->csrfToken->get($route->getRequirement('_csrf')); + } + } + +} + diff --git a/core/lib/Drupal/Core/CoreServiceProvider.php b/core/lib/Drupal/Core/CoreServiceProvider.php index 9cd8f5a..acd2a36 100644 --- a/core/lib/Drupal/Core/CoreServiceProvider.php +++ b/core/lib/Drupal/Core/CoreServiceProvider.php @@ -14,6 +14,7 @@ use Drupal\Core\DependencyInjection\Compiler\RegisterKernelListenersPass; use Drupal\Core\DependencyInjection\Compiler\RegisterAccessChecksPass; use Drupal\Core\DependencyInjection\Compiler\RegisterPathProcessorsPass; +use Drupal\Core\DependencyInjection\Compiler\RegisterRouteProcessorsPass; use Drupal\Core\DependencyInjection\Compiler\RegisterRouteFiltersPass; use Drupal\Core\DependencyInjection\Compiler\RegisterRouteEnhancersPass; use Drupal\Core\DependencyInjection\Compiler\RegisterParamConvertersPass; diff --git a/core/lib/Drupal/Core/DependencyInjection/Compiler/RegisterPathProcessorsPass.php b/core/lib/Drupal/Core/DependencyInjection/Compiler/RegisterPathProcessorsPass.php index 70de42d..ee9034c 100644 --- a/core/lib/Drupal/Core/DependencyInjection/Compiler/RegisterPathProcessorsPass.php +++ b/core/lib/Drupal/Core/DependencyInjection/Compiler/RegisterPathProcessorsPass.php @@ -37,5 +37,10 @@ public function process(ContainerBuilder $container) { $priority = isset($attributes[0]['priority']) ? $attributes[0]['priority'] : 0; $manager->addMethodCall('addOutbound', array(new Reference($id), $priority)); } + // Add outbound route processors. + foreach ($container->findTaggedServiceIds('route_processor_outbound') as $id => $attributes) { + $priority = isset($attributes[0]['priority']) ? $attributes[0]['priority'] : 0; + $manager->addMethodCall('addOutboundRoute', array(new Reference($id), $priority)); + } } } diff --git a/core/lib/Drupal/Core/EventSubscriber/AccessSubscriber.php b/core/lib/Drupal/Core/EventSubscriber/AccessSubscriber.php index 33e7274..4802012 100644 --- a/core/lib/Drupal/Core/EventSubscriber/AccessSubscriber.php +++ b/core/lib/Drupal/Core/EventSubscriber/AccessSubscriber.php @@ -9,6 +9,7 @@ use Symfony\Cmf\Component\Routing\RouteObjectInterface; use Symfony\Component\HttpKernel\KernelEvents; +use Symfony\Component\HttpKernel\HttpKernelInterface; use Symfony\Component\HttpKernel\Event\GetResponseEvent; use Symfony\Component\EventDispatcher\EventSubscriberInterface; use Symfony\Component\HttpKernel\Exception\AccessDeniedHttpException; @@ -40,13 +41,29 @@ public function __construct(AccessManager $access_manager) { */ public function onKernelRequestAccessCheck(GetResponseEvent $event) { $request = $event->getRequest(); + + // The controller is being handled by the HTTP kernel, so add an attribute + // to tell us this is the controller request. + $request->attributes->set('_controller_request', TRUE); + if (!$request->attributes->has(RouteObjectInterface::ROUTE_OBJECT)) { // If no Route is available it is likely a static resource and access is // handled elsewhere. return; } - $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; + } + + $request->attributes->remove('_controller_request'); + if (!$access) { throw new AccessDeniedHttpException(); } diff --git a/core/lib/Drupal/Core/PathProcessor/OutboundPathProcessorInterface.php b/core/lib/Drupal/Core/PathProcessor/OutboundPathProcessorInterface.php index 347a877..9e69001 100644 --- a/core/lib/Drupal/Core/PathProcessor/OutboundPathProcessorInterface.php +++ b/core/lib/Drupal/Core/PathProcessor/OutboundPathProcessorInterface.php @@ -8,6 +8,7 @@ namespace Drupal\Core\PathProcessor; use Symfony\Component\HttpFoundation\Request; +use Symfony\Component\Routing\Route; /** * Defines an interface for classes that process the outbound path. diff --git a/core/lib/Drupal/Core/PathProcessor/PathProcessorManager.php b/core/lib/Drupal/Core/PathProcessor/PathProcessorManager.php index db8b799..4eda1d0 100644 --- a/core/lib/Drupal/Core/PathProcessor/PathProcessorManager.php +++ b/core/lib/Drupal/Core/PathProcessor/PathProcessorManager.php @@ -8,6 +8,8 @@ namespace Drupal\Core\PathProcessor; use Symfony\Component\HttpFoundation\Request; +use Symfony\Component\Routing\Route; +use Drupal\Core\RouteProcessor\OutboundRouteProcessorInterface; /** * Path processor manager. @@ -15,7 +17,7 @@ * Holds an array of path processor objects and uses them to sequentially process * a path, in order of processor priority. */ -class PathProcessorManager implements InboundPathProcessorInterface, OutboundPathProcessorInterface { +class PathProcessorManager implements InboundPathProcessorInterface, OutboundPathProcessorInterface, OutboundRouteProcessorInterface { /** * Holds the array of inbound processors to cycle through. @@ -53,6 +55,23 @@ class PathProcessorManager implements InboundPathProcessorInterface, OutboundPat protected $sortedOutbound = array(); /** + * Holds the array of outbound route processors to cycle through. + * + * @var array + * An array whose keys are priorities and whose values are arrays of path + * processor objects. + */ + protected $outboundRouteProcessors = array(); + + /** + * Holds the array of outbound route processors, sorted by priority. + * + * @var array + * An array of path processor objects. + */ + protected $sortedOutboundRoute = array(); + + /** * Adds an inbound processor object to the $inboundProcessors property. * * @param \Drupal\Core\PathProcessor\InboundPathProcessorInterface $processor @@ -118,6 +137,30 @@ public function processOutbound($path, &$options = array(), Request $request = N } /** + * Adds an outbound route processor object to $outboundRouteProcessors. + * + * @param \Drupal\Core\RouteProcessor\OutboundRouteProcessorInterface $processor + * The processor object to add. + * + * @param int $priority + * The priority of the processor being added. + */ + public function addOutboundRoute(OutboundRouteProcessorInterface $processor, $priority = 0) { + $this->outboundRouteProcessors[$priority][] = $processor; + $this->sortedOutboundRoute = array(); + } + + /** + * {inheritdoc} + */ + public function processOutboundRoute(Route $route, array &$parameters) { + $processors = $this->getOutboundRoute(); + foreach ($processors as $processor) { + $processor->processOutboundRoute($route, $parameters); + } + } + + /** * Returns the sorted array of outbound processors. * * @return array @@ -132,6 +175,20 @@ protected function getOutbound() { } /** + * Returns the sorted array of outbound route processors. + * + * @return array + * An array of processor objects. + */ + protected function getOutboundRoute() { + if (empty($this->sortedOutboundRoute)) { + $this->sortedOutboundRoute = $this->sortProcessors('outboundRouteProcessors'); + } + + return $this->sortedOutboundRoute; + } + + /** * Sorts the processors according to priority. * * @param string $type diff --git a/core/lib/Drupal/Core/RouteProcessor/OutboundRouteProcessorInterface.php b/core/lib/Drupal/Core/RouteProcessor/OutboundRouteProcessorInterface.php new file mode 100644 index 0000000..e4d52bc --- /dev/null +++ b/core/lib/Drupal/Core/RouteProcessor/OutboundRouteProcessorInterface.php @@ -0,0 +1,32 @@ +getRoute($name); + $this->pathProcessor->processOutboundRoute($route, $parameters); + // Symfony adds any parameters that are not path slugs as query strings. if (isset($options['query']) && is_array($options['query'])) { $parameters = (array) $parameters + $options['query']; } + $path = $this->getInternalPathFromRoute($route, $parameters); $path = $this->processPath($path, $options); $fragment = ''; @@ -179,6 +183,7 @@ public function generateFromRoute($name, $parameters = array(), $options = array $fragment = '#' . $fragment; } } + $base_url = $this->context->getBaseUrl(); if (!$absolute || !$host = $this->context->getHost()) { return $base_url . $path . $fragment; diff --git a/core/modules/shortcut/lib/Drupal/shortcut/Controller/ShortcutSetController.php b/core/modules/shortcut/lib/Drupal/shortcut/Controller/ShortcutSetController.php index 7123ae5..d2f38eb 100644 --- a/core/modules/shortcut/lib/Drupal/shortcut/Controller/ShortcutSetController.php +++ b/core/modules/shortcut/lib/Drupal/shortcut/Controller/ShortcutSetController.php @@ -33,9 +33,8 @@ class ShortcutSetController extends ControllerBase { * @throws \Symfony\Component\HttpKernel\Exception\AccessDeniedHttpException */ public function addShortcutLinkInline(ShortcutSetInterface $shortcut_set, Request $request) { - $token = $request->query->get('token'); $link = $request->query->get('link'); - if (isset($token) && drupal_valid_token($token, 'shortcut-add-link') && shortcut_valid_link($link)) { + if (shortcut_valid_link($link)) { $item = menu_get_item($link); $title = ($item && $item['title']) ? $item['title'] : $link; $link = array( diff --git a/core/modules/shortcut/shortcut.module b/core/modules/shortcut/shortcut.module index 302d17b..f3b67a0 100644 --- a/core/modules/shortcut/shortcut.module +++ b/core/modules/shortcut/shortcut.module @@ -461,14 +461,15 @@ function shortcut_preprocess_page(&$variables) { $link_mode = isset($mlid) ? "remove" : "add"; if ($link_mode == "add") { - $query['token'] = drupal_get_token('shortcut-add-link'); $link_text = shortcut_set_switch_access() ? t('Add to %shortcut_set shortcuts', array('%shortcut_set' => $shortcut_set->label())) : t('Add to shortcuts'); - $link_path = 'admin/config/user-interface/shortcut/manage/' . $shortcut_set->id() . '/add-link-inline'; + $route_name = 'shortcut.link_add_inline'; + $route_parameters = array('shortcut_set' => $shortcut_set->id()); } else { $query['mlid'] = $mlid; $link_text = shortcut_set_switch_access() ? t('Remove from %shortcut_set shortcuts', array('%shortcut_set' => $shortcut_set->label())) : t('Remove from shortcuts'); - $link_path = 'admin/config/user-interface/shortcut/link/' . $mlid . '/delete'; + $route_name = 'shortcut.link_delete'; + $route_parameters = array('menu_link' => $mlid); } if (theme_get_setting('shortcut_module_link')) { @@ -481,7 +482,8 @@ function shortcut_preprocess_page(&$variables) { '#prefix' => '
', '#type' => 'link', '#title' => ''. t('Add or remove shortcut') .'' . $link_text . '', - '#href' => $link_path, + '#route_name' => $route_name, + '#route_parameters' => $route_parameters, '#options' => array('query' => $query, 'html' => TRUE), '#suffix' => '
', ); diff --git a/core/modules/shortcut/shortcut.routing.yml b/core/modules/shortcut/shortcut.routing.yml index 73decdd..4f902a5 100644 --- a/core/modules/shortcut/shortcut.routing.yml +++ b/core/modules/shortcut/shortcut.routing.yml @@ -40,6 +40,7 @@ shortcut.link_add_inline: _controller: 'Drupal\shortcut\Controller\ShortcutSetController::addShortcutLinkInline' requirements: _entity_access: 'shortcut_set.update' + _csrf: 'shortcut-add-link' shortcut.set_customize: path: '/admin/config/user-interface/shortcut/manage/{shortcut_set}' diff --git a/core/tests/Drupal/Tests/Core/Access/CsrfAccessCheckTest.php b/core/tests/Drupal/Tests/Core/Access/CsrfAccessCheckTest.php new file mode 100644 index 0000000..2a12387 --- /dev/null +++ b/core/tests/Drupal/Tests/Core/Access/CsrfAccessCheckTest.php @@ -0,0 +1,135 @@ + 'CSRF access checker', + 'description' => 'Tests CSRF access control for routes.', + 'group' => 'Routing', + ); + } + + public function setUp() { + $this->csrfToken = $this->getMockBuilder('Drupal\Core\Access\CsrfTokenGenerator') + ->disableOriginalConstructor() + ->getMock(); + + $this->accessCheck = new CsrfAccessCheck($this->csrfToken); + } + + /** + * Tests CsrfAccessCheck::appliesTo(). + */ + public function testAppliesTo() { + $this->assertEquals($this->accessCheck->appliesTo(), array('_csrf'), 'Access checker returned the expected appliesTo() array.'); + } + + /** + * Tests the access() method with a valid token. + */ + public function testAccessTokenPass() { + $this->csrfToken->expects($this->once()) + ->method('validate') + ->with('test_query', 'test') + ->will($this->returnValue(TRUE)); + + $route = new Route('', array(), array('_csrf' => 'test')); + $request = new Request(array( + 'token' => 'test_query', + )); + // Set the _controller_request flag so tokens are validated. + $request->attributes->set('_controller_request', TRUE); + + $this->assertSame(AccessInterface::ALLOW, $this->accessCheck->access($route, $request)); + } + + /** + * Tests the access() method with an invalid token. + */ + public function testAccessTokenFail() { + $this->csrfToken->expects($this->once()) + ->method('validate') + ->with('test_query', 'test') + ->will($this->returnValue(FALSE)); + + $route = new Route('', array(), array('_csrf' => 'test')); + $request = new Request(array( + 'token' => 'test_query', + )); + // Set the _controller_request flag so tokens are validated. + $request->attributes->set('_controller_request', TRUE); + + $this->assertSame(AccessInterface::KILL, $this->accessCheck->access($route, $request)); + } + + /** + * Tests the access() method with no _controller_request attribute set. + * + * This will default to the 'ANY' access conjuction. + */ + public function testAccessTokenMissAny() { + $this->csrfToken->expects($this->never()) + ->method('validate'); + + $route = new Route('', array(), array('_csrf' => 'test')); + $request = new Request(array( + 'token' => 'test_query', + )); + + $this->assertSame(AccessInterface::DENY, $this->accessCheck->access($route, $request)); + } + + /** + * Tests the access() method with no _controller_request attribute set. + * + * This will use the 'ALL' access conjuction. + */ + public function testAccessTokenMissAll() { + $this->csrfToken->expects($this->never()) + ->method('validate'); + + $route = new Route('', array(), array('_csrf' => 'test'), array('_access_mode' => 'ALL')); + $request = new Request(array( + 'token' => 'test_query', + )); + + $this->assertSame(AccessInterface::ALLOW, $this->accessCheck->access($route, $request)); + } + +} diff --git a/core/tests/Drupal/Tests/Core/Access/RouteProcessorCsrfTest.php b/core/tests/Drupal/Tests/Core/Access/RouteProcessorCsrfTest.php new file mode 100644 index 0000000..33bbd70 --- /dev/null +++ b/core/tests/Drupal/Tests/Core/Access/RouteProcessorCsrfTest.php @@ -0,0 +1,87 @@ + 'CSRF access checker', + 'description' => 'Tests CSRF access control for routes.', + 'group' => 'Routing', + ); + } + + public function setUp() { + $this->csrfToken = $this->getMockBuilder('Drupal\Core\Access\CsrfTokenGenerator') + ->disableOriginalConstructor() + ->getMock(); + + $this->processor = new RouteProcessorCsrf($this->csrfToken); + } + + /** + * Tests the processOutbound() method with no _csrf route requirement. + */ + public function testProcessOutboundNoRequirement() { + $this->csrfToken->expects($this->never()) + ->method('get'); + + $route = new Route(''); + $parameters = array(); + + $this->processor->processOutboundRoute($route, $parameters); + // No parameters should be added to the parameters array. + $this->assertEmpty($parameters); + } + + /** + * Tests the processOutbound() method with a _csrf route requirement. + */ + public function testProcessOutboundRoute() { + $this->csrfToken->expects($this->once()) + ->method('get') + ->with('test') + ->will($this->returnValue('test_token')); + + $route = new Route('', array(), array('_csrf' => 'test')); + $parameters = array(); + + $this->processor->processOutboundRoute($route, $parameters); + // 'token' should be added to the parameters array. + $this->assertArrayHasKey('token', $parameters); + $this->assertSame($parameters['token'], 'test_token'); + } + +} diff --git a/core/tests/Drupal/Tests/Core/PathProcessor/PathProcessorTest.php b/core/tests/Drupal/Tests/Core/PathProcessor/PathProcessorTest.php index f49184f..d48cdf6 100644 --- a/core/tests/Drupal/Tests/Core/PathProcessor/PathProcessorTest.php +++ b/core/tests/Drupal/Tests/Core/PathProcessor/PathProcessorTest.php @@ -14,6 +14,7 @@ use Drupal\Core\PathProcessor\PathProcessorManager; use Drupal\language\HttpKernel\PathProcessorLanguage; use Symfony\Component\HttpFoundation\Request; +use Symfony\Component\Routing\Route; use Drupal\Tests\UnitTestCase; @@ -150,4 +151,46 @@ function testProcessInbound() { $processed = $processor_manager->processInbound($test_path, $request); $this->assertEquals('user/1', $processed, 'Processing in the correct order resolves the system path from an alias.'); } + + /** + * Tests the Route process manager functionality. + */ + public function testProcessOutboundRoute() { + $route = new Route(''); + $parameters = array('test' => 'test'); + + $processors = array( + 10 => $this->getMockRouteProcessor($route, $parameters), + 5 => $this->getMockRouteProcessor($route, $parameters), + 0 => $this->getMockRouteProcessor($route, $parameters), + ); + + $path_processor_manager = new PathProcessorManager(); + + // Add the processors in reverse order. + foreach ($processors as $priority => $processor) { + $path_processor_manager->addOutboundRoute($processor, $priority); + } + + $path_processor_manager->processOutboundRoute($route, $parameters); + } + + /** + * Returns a mock Route processor object. + * + * @param \Symfony\Component\Routing\Route $route + * The Route to use in mock with() expectation. + * @param array $parameters + * The parameters to use in mock with() expectation. + * + * @return \Drupal\Core\RouteProcessor\OutboundRouteProcessorInterface|\PHPUnit_Framework_MockObject_MockObject + */ + protected function getMockRouteProcessor($route, $parameters) { + $processor = $this->getMock('Drupal\Core\RouteProcessor\OutboundRouteProcessorInterface'); + $processor->expects($this->once()) + ->method('processOutboundRoute') + ->with($route, $parameters); + + return $processor; + } } diff --git a/core/tests/Drupal/Tests/Core/Routing/UrlGeneratorTest.php b/core/tests/Drupal/Tests/Core/Routing/UrlGeneratorTest.php index 75d051d..cbe1899 100644 --- a/core/tests/Drupal/Tests/Core/Routing/UrlGeneratorTest.php +++ b/core/tests/Drupal/Tests/Core/Routing/UrlGeneratorTest.php @@ -8,21 +8,15 @@ namespace Drupal\Tests\Core\Routing; use Drupal\Component\Utility\Settings; -use Drupal\Core\Config\ConfigFactory; -use Drupal\Core\Config\NullStorage; -use Drupal\Core\Config\Context\ConfigContextFactory; use Drupal\Core\PathProcessor\PathProcessorAlias; use Drupal\Core\PathProcessor\PathProcessorManager; -use Symfony\Component\EventDispatcher\EventDispatcher; +use Drupal\Core\Routing\UrlGenerator; +use Drupal\Tests\UnitTestCase; use Symfony\Component\HttpFoundation\Request; use Symfony\Component\Routing\Route; use Symfony\Component\Routing\RouteCollection; use Symfony\Component\Routing\RequestContext; -use Drupal\Tests\UnitTestCase; - -use Drupal\Core\Routing\UrlGenerator; - /** * Basic tests for the Route. * @@ -44,6 +38,11 @@ class UrlGeneratorTest extends UnitTestCase { */ protected $generatorMixedMode; + /** + * The alias manager. + * + * @var \Drupal\Core\Path\AliasManager|\PHPUnit_Framework_MockObject_MockObject + */ protected $aliasManager; public static function getInfo() {