From 3de79990b339537404813f053c23a8b5a795e801 Mon Sep 17 00:00:00 2001 From: Alex Pott Date: Fri, 29 Jul 2016 16:11:36 +0100 Subject: [PATCH] Issue #2681911 by garphy, Wim Leers, gabesullice: REST requests without X-CSRF-Token header: unhelpful response significantly hinders DX, should receive a 401 response --- core/lib/Drupal/Core/Access/AccessResult.php | 17 ++++++- .../Core/Access/AccessResultForbidden.php | 35 ++++++++++++++- .../Access/AccessResultReasonInterface.php | 36 +++++++++++++++ .../Access/CsrfRequestHeaderAccessCheck.php | 2 +- .../Drupal/Core/Routing/AccessAwareRouter.php | 3 +- core/modules/rest/src/Tests/CreateTest.php | 4 ++ core/modules/rest/src/Tests/DeleteTest.php | 3 ++ core/modules/rest/src/Tests/RESTTestBase.php | 18 +++++--- core/modules/rest/src/Tests/UpdateTest.php | 5 +++ .../Core/Access/AccessResultForbiddenTest.php | 45 +++++++++++++++++++ .../Tests/Core/Access/AccessResultTest.php | 18 ++++++++ .../Core/Routing/AccessAwareRouterTest.php | 17 +++++++ 12 files changed, 193 insertions(+), 10 deletions(-) create mode 100644 core/lib/Drupal/Core/Access/AccessResultReasonInterface.php create mode 100644 core/tests/Drupal/Tests/Core/Access/AccessResultForbiddenTest.php diff --git a/core/lib/Drupal/Core/Access/AccessResult.php b/core/lib/Drupal/Core/Access/AccessResult.php index 8148d4ca58..f67b9f5296 100644 --- a/core/lib/Drupal/Core/Access/AccessResult.php +++ b/core/lib/Drupal/Core/Access/AccessResult.php @@ -51,11 +51,16 @@ public static function allowed() { /** * Creates an AccessResultInterface object with isForbidden() === TRUE. * + * @param string|null $reason + * (optional) The reason why access is forbidden. Intended for developers, + * hence not translatable. + * * @return \Drupal\Core\Access\AccessResult * isForbidden() will be TRUE. */ - public static function forbidden() { - return new AccessResultForbidden(); + public static function forbidden($reason = NULL) { + assert('is_string($reason) || is_null($reason)'); + return new AccessResultForbidden($reason); } /** @@ -334,8 +339,16 @@ public function andIf(AccessResultInterface $other) { if ($this->isForbidden() || $other->isForbidden()) { $result = static::forbidden(); if (!$this->isForbidden()) { + if ($other instanceof AccessResultReasonInterface) { + $result->setReason($other->getReason()); + } $merge_other = TRUE; } + else { + if ($this instanceof AccessResultReasonInterface) { + $result->setReason($this->getReason()); + } + } } elseif ($this->isAllowed() && $other->isAllowed()) { $result = static::allowed(); diff --git a/core/lib/Drupal/Core/Access/AccessResultForbidden.php b/core/lib/Drupal/Core/Access/AccessResultForbidden.php index 4dc0120a9d..2dc913761c 100644 --- a/core/lib/Drupal/Core/Access/AccessResultForbidden.php +++ b/core/lib/Drupal/Core/Access/AccessResultForbidden.php @@ -5,7 +5,25 @@ /** * Value object indicating a forbidden access result, with cacheability metadata. */ -class AccessResultForbidden extends AccessResult { +class AccessResultForbidden extends AccessResult implements AccessResultReasonInterface { + + /** + * The reason why access is forbidden. For use in error messages. + * + * @var string|null + */ + protected $reason; + + /** + * Constructs a new AccessResultForbidden instance. + * + * @param null|string $reason + * (optional) a message to provide details about this access result + */ + public function __construct($reason = NULL) { + $this->reason = $reason; + } + /** * {@inheritdoc} @@ -14,4 +32,19 @@ public function isForbidden() { return TRUE; } + /** + * {@inheritdoc} + */ + public function getReason() { + return $this->reason; + } + + /** + * {@inheritdoc} + */ + public function setReason($reason) { + $this->reason = $reason; + return $this; + } + } diff --git a/core/lib/Drupal/Core/Access/AccessResultReasonInterface.php b/core/lib/Drupal/Core/Access/AccessResultReasonInterface.php new file mode 100644 index 0000000000..468d998896 --- /dev/null +++ b/core/lib/Drupal/Core/Access/AccessResultReasonInterface.php @@ -0,0 +1,36 @@ +csrfToken->validate($csrf_token, self::TOKEN_KEY) && !$this->csrfToken->validate($csrf_token, 'rest')) { - return AccessResult::forbidden()->setCacheMaxAge(0); + return AccessResult::forbidden()->setReason('X-CSRF-Token request header is missing')->setCacheMaxAge(0); } } // Let other access checkers decide if the request is legit. diff --git a/core/lib/Drupal/Core/Routing/AccessAwareRouter.php b/core/lib/Drupal/Core/Routing/AccessAwareRouter.php index 80e16ebb6f..9346b0ae1f 100644 --- a/core/lib/Drupal/Core/Routing/AccessAwareRouter.php +++ b/core/lib/Drupal/Core/Routing/AccessAwareRouter.php @@ -3,6 +3,7 @@ namespace Drupal\Core\Routing; use Drupal\Core\Access\AccessManagerInterface; +use Drupal\Core\Access\AccessResultReasonInterface; use Drupal\Core\Session\AccountInterface; use Symfony\Cmf\Component\Routing\ChainRouter; use Symfony\Component\HttpFoundation\Request; @@ -105,7 +106,7 @@ protected function checkAccess(Request $request) { $request->attributes->set(AccessAwareRouterInterface::ACCESS_RESULT, $access_result); } if (!$access_result->isAllowed()) { - throw new AccessDeniedHttpException(); + throw new AccessDeniedHttpException($access_result instanceof AccessResultReasonInterface ? $access_result->getReason() : NULL); } } diff --git a/core/modules/rest/src/Tests/CreateTest.php b/core/modules/rest/src/Tests/CreateTest.php index 28c8af6917..d71b9e4a0f 100644 --- a/core/modules/rest/src/Tests/CreateTest.php +++ b/core/modules/rest/src/Tests/CreateTest.php @@ -356,6 +356,10 @@ public function createAccountPerEntity($entity_type) { public function assertCreateEntityOverRestApi($entity_type, $serialized = NULL) { // Note: this will fail with PHP 5.6 when always_populate_raw_post_data is // set to something other than -1. See https://www.drupal.org/node/2456025. + // Try first without the CSRF token, which should fail. + $this->httpRequest('entity/' . $entity_type, 'POST', $serialized, $this->defaultMimeType, TRUE); + $this->assertResponse(403, 'X-CSRF-Token request header is missing'); + // Then try with the CSRF token. $response = $this->httpRequest('entity/' . $entity_type, 'POST', $serialized, $this->defaultMimeType); $this->assertResponse(201); diff --git a/core/modules/rest/src/Tests/DeleteTest.php b/core/modules/rest/src/Tests/DeleteTest.php index 4cc80162ec..7de3fb48e4 100644 --- a/core/modules/rest/src/Tests/DeleteTest.php +++ b/core/modules/rest/src/Tests/DeleteTest.php @@ -38,6 +38,9 @@ public function testDelete() { // Create an entity programmatically. $entity = $this->entityCreate($entity_type); $entity->save(); + // Try first to delete over REST API without the CSRF token. + $this->httpRequest($entity->urlInfo(), 'DELETE', NULL, NULL, TRUE); + $this->assertResponse(403, 'X-CSRF-Token request header is missing'); // Delete it over the REST API. $response = $this->httpRequest($entity->urlInfo(), 'DELETE'); // Clear the static cache with entity_load(), otherwise we won't see the diff --git a/core/modules/rest/src/Tests/RESTTestBase.php b/core/modules/rest/src/Tests/RESTTestBase.php index 4e660c3561..a3f6a5fde1 100644 --- a/core/modules/rest/src/Tests/RESTTestBase.php +++ b/core/modules/rest/src/Tests/RESTTestBase.php @@ -85,11 +85,13 @@ protected function setUp() { * The body for POST and PUT. * @param string $mime_type * The MIME type of the transmitted content. + * @param bool $forget_xcsrf_token + * If TRUE, the CSRF token won't be included in request. * * @return string * The content returned from the request. */ - protected function httpRequest($url, $method, $body = NULL, $mime_type = NULL) { + protected function httpRequest($url, $method, $body = NULL, $mime_type = NULL, $forget_xcsrf_token = FALSE) { if (!isset($mime_type)) { $mime_type = $this->defaultMimeType; } @@ -130,9 +132,11 @@ protected function httpRequest($url, $method, $body = NULL, $mime_type = NULL) { CURLOPT_POSTFIELDS => $body, CURLOPT_URL => $url, CURLOPT_NOBODY => FALSE, - CURLOPT_HTTPHEADER => array( + CURLOPT_HTTPHEADER => !$forget_xcsrf_token ? array( 'Content-Type: ' . $mime_type, 'X-CSRF-Token: ' . $token, + ) : array( + 'Content-Type: ' . $mime_type, ), ); break; @@ -144,9 +148,11 @@ protected function httpRequest($url, $method, $body = NULL, $mime_type = NULL) { CURLOPT_POSTFIELDS => $body, CURLOPT_URL => $url, CURLOPT_NOBODY => FALSE, - CURLOPT_HTTPHEADER => array( + CURLOPT_HTTPHEADER => !$forget_xcsrf_token ? array( 'Content-Type: ' . $mime_type, 'X-CSRF-Token: ' . $token, + ) : array( + 'Content-Type: ' . $mime_type, ), ); break; @@ -158,9 +164,11 @@ protected function httpRequest($url, $method, $body = NULL, $mime_type = NULL) { CURLOPT_POSTFIELDS => $body, CURLOPT_URL => $url, CURLOPT_NOBODY => FALSE, - CURLOPT_HTTPHEADER => array( + CURLOPT_HTTPHEADER => !$forget_xcsrf_token ? array( 'Content-Type: ' . $mime_type, 'X-CSRF-Token: ' . $token, + ) : array( + 'Content-Type: ' . $mime_type, ), ); break; @@ -171,7 +179,7 @@ protected function httpRequest($url, $method, $body = NULL, $mime_type = NULL) { CURLOPT_CUSTOMREQUEST => 'DELETE', CURLOPT_URL => $url, CURLOPT_NOBODY => FALSE, - CURLOPT_HTTPHEADER => array('X-CSRF-Token: ' . $token), + CURLOPT_HTTPHEADER => !$forget_xcsrf_token ? array('X-CSRF-Token: ' . $token) : array(), ); break; } diff --git a/core/modules/rest/src/Tests/UpdateTest.php b/core/modules/rest/src/Tests/UpdateTest.php index b8f1b54494..dcab1ff76b 100644 --- a/core/modules/rest/src/Tests/UpdateTest.php +++ b/core/modules/rest/src/Tests/UpdateTest.php @@ -374,6 +374,11 @@ protected function patchEntity(EntityInterface $entity, array $read_only_fields, } $serialized = $serializer->serialize($normalized, $format, $context); + // Try first without CSRF token which should fail. + $this->httpRequest($url, 'PATCH', $serialized, $mime_type, TRUE); + $this->assertResponse(403, 'X-CSRF-Token request header is missing'); + + // Then try with CSRF token. $this->httpRequest($url, 'PATCH', $serialized, $mime_type); $this->assertResponse(200); } diff --git a/core/tests/Drupal/Tests/Core/Access/AccessResultForbiddenTest.php b/core/tests/Drupal/Tests/Core/Access/AccessResultForbiddenTest.php new file mode 100644 index 0000000000..3409f37971 --- /dev/null +++ b/core/tests/Drupal/Tests/Core/Access/AccessResultForbiddenTest.php @@ -0,0 +1,45 @@ +assertEquals(NULL, $a->getReason()); + + $reason = $this->getRandomGenerator()->string(); + $b = new AccessResultForbidden($reason); + $this->assertEquals($reason, $b->getReason()); + } + + /** + * Test setReason() + * + * @covers ::setReason + */ + public function testSetReason() { + $a = new AccessResultForbidden(); + + $reason = $this->getRandomGenerator()->string(); + $return = $a->setReason($reason); + + $this->assertSame($reason, $a->getReason()); + $this->assertSame($a, $return); + } + +} diff --git a/core/tests/Drupal/Tests/Core/Access/AccessResultTest.php b/core/tests/Drupal/Tests/Core/Access/AccessResultTest.php index 287c344d5c..ca262c7e18 100644 --- a/core/tests/Drupal/Tests/Core/Access/AccessResultTest.php +++ b/core/tests/Drupal/Tests/Core/Access/AccessResultTest.php @@ -10,6 +10,7 @@ use Drupal\Core\Access\AccessResult; use Drupal\Core\Access\AccessResultInterface; use Drupal\Core\Access\AccessResultNeutral; +use Drupal\Core\Access\AccessResultReasonInterface; use Drupal\Core\Cache\Cache; use Drupal\Core\Cache\CacheableDependencyInterface; use Drupal\Core\DependencyInjection\ContainerBuilder; @@ -112,6 +113,23 @@ public function testAccessForbidden() { $verify($b); } + /** + * @covers ::forbidden + */ + public function testAccessForbiddenReason() { + $verify = function (AccessResult $access, $reason) { + $this->assertInstanceOf(AccessResultReasonInterface::class, $access); + $this->assertSame($reason, $access->getReason()); + }; + + $b = AccessResult::forbidden(); + $verify($b, NULL); + + $reason = $this->getRandomGenerator()->string(); + $b = AccessResult::forbidden($reason); + $verify($b, $reason); + } + /** * @covers ::allowedIf * @covers ::isAllowed diff --git a/core/tests/Drupal/Tests/Core/Routing/AccessAwareRouterTest.php b/core/tests/Drupal/Tests/Core/Routing/AccessAwareRouterTest.php index df3b580c82..0b7bcc6730 100644 --- a/core/tests/Drupal/Tests/Core/Routing/AccessAwareRouterTest.php +++ b/core/tests/Drupal/Tests/Core/Routing/AccessAwareRouterTest.php @@ -8,6 +8,7 @@ use Drupal\Tests\UnitTestCase; use Symfony\Cmf\Component\Routing\RouteObjectInterface; use Symfony\Component\HttpFoundation\Request; +use Symfony\Component\HttpKernel\Exception\AccessDeniedHttpException; use Symfony\Component\Routing\Route; /** @@ -105,6 +106,22 @@ public function testMatchRequestDenied() { $this->assertSame($expected, $parameters); } + /** + * Tests the matchRequest() function for access denied with reason message. + */ + public function testCheckAccessResultWithReason() { + $this->setupRouter(); + $request = new Request(); + $reason = $this->getRandomGenerator()->string(); + $access_result = AccessResult::forbidden($reason); + $this->accessManager->expects($this->once()) + ->method('checkRequest') + ->with($request) + ->willReturn($access_result); + $this->setExpectedException(AccessDeniedHttpException::class, $reason); + $this->router->matchRequest($request); + } + /** * Ensure that methods are passed to the wrapped router. * -- GitLab