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

@@ -51,11 +51,16 @@ abstract class AccessResult implements AccessResultInterface, RefinableCacheable
   /**
    * 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 @@ abstract class AccessResult implements AccessResultInterface, RefinableCacheable
     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();

core/lib/Drupal/Core/Access/AccessResultForbidden.php

@@ -5,7 +5,25 @@ namespace Drupal\Core\Access;
 /**
  * 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 @@ class AccessResultForbidden extends AccessResult {
     return TRUE;
   }
 
+  /**
+   * {@inheritdoc}
+   */
+  public function getReason() {
+    return $this->reason;
+  }
+
+  /**
+   * {@inheritdoc}
+   */
+  public function setReason($reason) {
+    $this->reason = $reason;
+    return $this;
+  }
+
 }

core/lib/Drupal/Core/Access/AccessResultReasonInterface.php

@@ -0,0 +1,36 @@
+<?php
+
+namespace Drupal\Core\Access;
+
+/**
+ * Interface for access result value objects with stored reason for developers.
+ *
+ * For example, a developer can specify the reason for forbidden access:
+ * @code
+ * new AccessResultForbidden('You are not authorized to hack core');
+ * @endcode
+ *
+ * @see \Drupal\Core\Access\AccessResultInterface
+ */
+interface AccessResultReasonInterface extends AccessResultInterface {
+
+  /**
+   * Gets the reason for this access result.
+   *
+   * @return string|NULL
+   *   The reason of this access result or NULL if no reason is provided.
+   */
+  public function getReason();
+
+  /**
+   * Sets the reason for this access result.
+   *
+   * @param $reason string|NULL
+   *   The reason of this access result or NULL if no reason is provided.
+   *
+   * @return \Drupal\Core\Access\AccessResultInterface
+   *   The access result instance.
+   */
+  public function setReason($reason);
+
+}

core/lib/Drupal/Core/Access/CsrfRequestHeaderAccessCheck.php

@@ -102,7 +102,7 @@ class CsrfRequestHeaderAccessCheck implements AccessCheckInterface {
       //   Kept here for sessions active during update.
       if (!$this->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.

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 @@ class AccessAwareRouter implements AccessAwareRouterInterface {
       $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);
     }
   }
 

core/modules/rest/src/Tests/CreateTest.php

@@ -356,6 +356,10 @@ class CreateTest extends RESTTestBase {
   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);
 

core/modules/rest/src/Tests/DeleteTest.php

@@ -38,6 +38,9 @@ class DeleteTest extends RESTTestBase {
       // 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

core/modules/rest/src/Tests/RESTTestBase.php

@@ -85,11 +85,13 @@ abstract class RESTTestBase extends WebTestBase {
    *   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 @@ abstract class RESTTestBase extends WebTestBase {
           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 @@ abstract class RESTTestBase extends WebTestBase {
           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 @@ abstract class RESTTestBase extends WebTestBase {
           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 @@ abstract class RESTTestBase extends WebTestBase {
           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;
     }

core/modules/rest/src/Tests/UpdateTest.php

@@ -374,6 +374,11 @@ class UpdateTest extends RESTTestBase {
     }
     $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);
   }

core/tests/Drupal/Tests/Core/Access/AccessResultForbiddenTest.php

@@ -0,0 +1,45 @@
+<?php
+
+namespace Drupal\Tests\Core\Access;
+
+use Drupal\Core\Access\AccessResultForbidden;
+use Drupal\Tests\UnitTestCase;
+
+/**
+ * @coversDefaultClass \Drupal\Core\Access\AccessResultForbidden
+ * @group Access
+ */
+class AccessResultForbiddenTest extends UnitTestCase {
+
+  /**
+   * Tests the construction of an AccessResultForbidden object.
+   *
+   * @covers ::__construct
+   * @covers ::getReason
+   */
+  public function testConstruction() {
+
+    $a = new AccessResultForbidden();
+    $this->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);
+  }
+
+}

core/tests/Drupal/Tests/Core/Access/AccessResultTest.php

@@ -10,6 +10,7 @@ namespace Drupal\Tests\Core\Access;
 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 @@ class AccessResultTest extends UnitTestCase {
     $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

core/tests/Drupal/Tests/Core/Routing/AccessAwareRouterTest.php

@@ -8,6 +8,7 @@ use Drupal\Core\Routing\AccessAwareRouterInterface;
 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 @@ class AccessAwareRouterTest extends UnitTestCase {
     $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.
    *