REST requests without X-CSRF-Token header: unhelpful response significantly hinders DX, should receive a 401 response

To be clear: this was split off from #2659070: REST requests without Content-Type header: unhelpful response significantly hinders DX, should receive a 415 response to keep the scope of that issue manageable.

Another person who ran into this: #2667186: Can't make a post with cookie authentication.

And yet another: http://wimleers.com/comment/2504#comment-2504.

I think that's sufficient justification to keep this at major.

Retitled to be consistent with the title of #2659070: REST requests without Content-Type header: unhelpful response significantly hinders DX, should receive a 415 response.

Per http://tools.ietf.org/html/rfc7235#section-4.2 & https://www.w3.org/Protocols/rfc2616/rfc2616-sec10.html#sec10.4.2:

The response MUST include a WWW-Authenticate header field

That header field can only be used for Basic or Digest Auth. Neither of which is relevant to X-CSRF-Token. So the current 403 response is in fact correct. What's missing, is a meaningful message.

d.o--, it lost my issue summary changes.

Problem analysis + possible solutions, from #2659070-15: REST requests without Content-Type header: unhelpful response significantly hinders DX, should receive a 415 response:

…

3. X-CSRF-Token (when only the X-CSRF-Token request header is missing)

So now that the system is providing actually sensible errors, let's send that Content-Type request header we were missing. As the IS already says, now you get a 403, and {"message":""} as the response body.

This problem still remains to be solved. The root cause of the problem here is that the system is designed to only allow \Drupal\Core\Routing\AccessAwareRouter::checkAccess() to return a response. \Drupal\rest\Access\CSRFAccessCheck::access() is intended to be only allowed to return an access result object (\Drupal\Core\Access\AccessResultInterface). And such an object can not return a useful message.

i.e. AccessAwareRouter::checkAccess() does this:

    $access_result = $this->accessManager->checkRequest($request, $this->account, TRUE);
    …
    if (!$access_result->isAllowed()) {
      throw new AccessDeniedHttpException();
    }

Instead of this:

    $access_result = $this->accessManager->checkRequest($request, $this->account, TRUE);
    …
    if (!$access_result->isAllowed()) {
      throw new AccessDeniedHttpException((string) $access_result);
    }

i.e. if AccessResult implemented __toString() and would allow a helpful indicator message to be set, we could allow a sensible message to be sent.

Alternatively, we could change CSRFAccessCheck::access() to throw new AccessDeniedHttpException('Missing or invalid X-CSRF-Token'), but I'm not sure if that would break/violate the existing API/architecture. I think this will not be acceptable because it would break in case of checking access to e.g. menu links, because in that context you're checking access not for the current request's route, but just for some route.

---

Conclusion

1. …
2. Forgetting to send the X-CSRF-Token (or sending an invalid one) is not yet solved by this patch. Two possible solutions are given: allow AccessResultInterface objects to contain a message, or allow AccessCheckInterface implementations to not return an AccessResultInterface object but throw an exception (questionable.

For now I've opted to let only AccessResultForbidden contain a message.

Test before vs after by using

jQuery.ajax({
      url: Drupal.url('node/1') + '?_format=hal_json',
      headers: {
        "Content-Type": "application/hal+json"
      },
      method: 'PATCH',
      data: {},
    });

Cool :) I think this may actually be a good novice task, perhaps for Drupal Dev Days Milan!? :)

Clearly nobody did. So I'll take it on again.

These tags are still relevant. Unassigning until I really start working on it. Anybody, feel free to step up!

Thanks, @garphy!

#27:

1. Yes: to prevent other data structures from being passed in, which would lead to unexpected behavior when using the return value of getReason()
2. +1
3. +1
4. +1!

1. +++ b/core/lib/Drupal/Core/Access/AccessResultForbidden.php
   @@ -5,7 +5,25 @@
   +   * Construct a new AccessResultForbidden instance.
   

   Nit: s/Construct/Constructs/

2. +++ b/core/lib/Drupal/Core/Access/AccessResultReasonInterface.php
   @@ -0,0 +1,26 @@
   + * Interface for access result detailed reason
   

   What about Interface for access result value objects with stored reason for developers.?

3. +++ b/core/lib/Drupal/Core/Access/AccessResultReasonInterface.php
   @@ -0,0 +1,26 @@
   +interface AccessResultReasonInterface {
   

   Should extend AccessResultInterface.

   And I wonder if it should be called AccessResultWithReasonInterface?

4. +++ b/core/lib/Drupal/Core/Access/AccessResultReasonInterface.php
   @@ -0,0 +1,26 @@
   +   * Get the detailed reason of this result.
   ...
   +   * Set the detailed reason of this result.
   

   s/Get/Gets/
   s/Set/Sets/

   s/detailed reason/reason/
   s/result/access result/

5. +++ b/core/lib/Drupal/Core/Access/AccessResultReasonInterface.php
   @@ -0,0 +1,26 @@
   +   *   the reason of this result or NULL if no detail is provided
   ...
   +   *   the reason of this result or NULL if no detail is provided
   

   Missing capitalization at the beginning.

   Missing period at the end.

   s/detail/reason/

6. +++ b/core/lib/Drupal/Core/Access/AccessResultReasonInterface.php
   @@ -0,0 +1,26 @@
   +  public function setReason($reason);
   

   Missing @return documentation.

7. +++ b/core/tests/Drupal/Tests/Core/Access/AccessResultTest.php
   @@ -110,6 +111,23 @@ public function testAccessForbidden() {
        $verify($b);
   +
   +  }
   

   One unnecessary blank line.

8. +++ b/core/tests/Drupal/Tests/Core/Access/AccessResultTest.php
   @@ -110,6 +111,23 @@ public function testAccessForbidden() {
   +      $this->assertEquals($reason, $access->getReason());
   

   Should be assertSame(). (For stricter checking.)

9. +++ b/core/tests/Drupal/Tests/Core/Access/AccessResultTest.php
   @@ -110,6 +111,23 @@ public function testAccessForbidden() {
   +    $b = AccessResult::forbidden('Reason');
   +    $verify($b, 'Reason');
   

   Let's use a random string here: $this->randomGenerator->string();.

@garphy: can you provide interdiffs in the future? https://www.drupal.org/documentation/git/interdiff Thanks :)

Thanks again for working on this! Next review upcoming, stay tuned…

1. #30: I think you misinterpreted #27.4. @dawehner meant he'd like to see a test verifying that if access is forbidden to a route, that AccessAwareRouter throws this exception, and that the exception is indeed displayed to the end user. i.e. a test that verifies that if you make a request to a route that you don't have access to, that you then get to see this error message.

   You can add a new test method to \Drupal\Tests\Core\Routing\AccessAwareRouterTest for this.

   However, your addition of AccessResultForbiddenTest is also great!

2. +++ b/core/lib/Drupal/Core/Access/AccessResultReasonInterface.php
   @@ -0,0 +1,36 @@
   + * For example, a developer can specify the reason of an  access rejection :
   

   s/an access rejection :/for forbidden access:/

3. +++ b/core/lib/Drupal/Core/Access/AccessResultReasonInterface.php
   @@ -0,0 +1,36 @@
   + * new AccessResultForbidden('You\'re not authorized to hack core');
   

   Let's say "you are" so you don't need the backslash.

   Also: hah! :D

4. +++ b/core/lib/Drupal/Core/Access/AccessResultReasonInterface.php
   @@ -0,0 +1,36 @@
   + * @see AccessResultForbidden
   

   Needs FQCN.

5. +++ b/core/lib/Drupal/Core/Access/AccessResultReasonInterface.php
   @@ -0,0 +1,36 @@
   +   * Gets the reason of this access result.
   ...
   +   * Sets the reason of this access result.
   

   s/of/for/

6. +++ b/core/lib/Drupal/Core/Access/AccessResultReasonInterface.php
   @@ -0,0 +1,36 @@
   +   * @return AccessResultInterface
   

   Needs FQCN.

7. +++ b/core/tests/Drupal/Tests/Core/Access/AccessResultForbiddenTest.php
   @@ -0,0 +1,49 @@
   +/**
   + * @file
   + * Contains \Drupal\Tests\Core\Access\AccessResultTest.
   + */
   

   You don't need these anymore :)

8. Finally, we're also going to need an integration test to prove that the problem described in the issue summary has actually been solved. Please add a similarly "faulty" request (without an X-CSRF-Token request header) to:
   1. \Drupal\rest\Tests\UpdateTest::testPatchUpdate()
   2. \Drupal\rest\Tests\CreateTest::testCreateEntityTest()
   3. \Drupal\rest\Tests\DeleteTest::testDelete()

   That test should verify that the response you get back is a 403, and that the expected message is present.

1. +++ b/core/tests/Drupal/Tests/Core/Routing/AccessAwareRouterTest.php
   @@ -106,6 +107,22 @@ public function testMatchRequestDenied() {
   +   * Tests the matchRequest() function for access denied with specific reason message.
   

   Nit: Omit "specific", so it fits within 80 cols.

2. +++ b/core/tests/Drupal/Tests/Core/Routing/AccessAwareRouterTest.php
   @@ -106,6 +107,22 @@ public function testMatchRequestDenied() {
   +  public function testCheckAccessResultWithReason() {
   

   Perfect! :)

That leaves just #34.8, then this is ready! Woot!

@garphy++
@garphy++
@garphy++
@garphy++

To be clear, this needs work for #34.8.

#38++ — great review, thanks!

Great work! But I think it can be done in a much more simple way :)

1. +++ b/core/modules/rest/src/Tests/CreateTest.php
   @@ -99,6 +99,60 @@ public function testCreateWithoutPermission() {
   +  public function testCreateWithoutCSRFToken() {
   

   This is an entirely new test method (which means setting up an entirely separate testing environment etc too).

   This is overkill. We can do it much simpler :)

2. +++ b/core/modules/rest/src/Tests/CreateTest.php
   @@ -99,6 +99,60 @@ public function testCreateWithoutPermission() {
   +    // Attempt to create the entity over the REST API without providing CSRF token.
   +    $url = $this->buildUrl('entity/' . $entity_type);
   +
   +    $curl_options = array(
   +      CURLOPT_HTTPGET => FALSE,
   +      CURLOPT_CUSTOMREQUEST => 'POST',
   +      CURLOPT_POSTFIELDS => $serialized,
   +      CURLOPT_URL => $url,
   +      CURLOPT_NOBODY => FALSE,
   +      CURLOPT_HTTPHEADER => array(
   +        'Content-Type: ' . $this->defaultMimeType,
   +      ),
   +    );
   +
   +    $this->responseBody = $this->curlExec($curl_options);
   

   Rather than doing this, I'd add a $forget_xcsrf_token boolean parameter that defaults to FALSE to \Drupal\rest\Tests\RESTTestBase::httpRequest().

   In that method, I'd then change

             CURLOPT_HTTPHEADER => array(
               'Content-Type: ' . $mime_type,
               'X-CSRF-Token: ' . $token,
             ),
   

   to:

             CURLOPT_HTTPHEADER => !$forget_xcsrf_token ? array(
               'Content-Type: ' . $mime_type,
               'X-CSRF-Token: ' . $token,
             ) : array('Content-Type: ' . $mime_type),
   

   etc

   And then you can update \Drupal\rest\Tests\CreateTest::assertCreateEntityOverRestApi() to once do a request with that new boolean parameter set to TRUE, followed by $this->assertResponse(403, 'X-CSRF-Token request header is missing');.

   That means we avoid:

   • a new test environment (slow)
   • a lot of new test code, that mostly duplicates existing test code
   • a lot of new CURL code

   It also means you end up with a realistic flow: "oops I forgot, let me fix that in my next request".

3. +++ b/core/modules/rest/src/Tests/DeleteTest.php
   @@ -73,4 +73,50 @@ public function testDelete() {
   +  public function testDeleteWithoutCSRFToken(){
   

   Similar observation for this one. In this case, I'd do the "oops I forgot" request before these lines:

         // Delete it over the REST API.
         $response = $this->httpRequest($entity->urlInfo(), 'DELETE');
   

4. +++ b/core/modules/rest/src/Tests/UpdateTest.php
   @@ -186,6 +186,68 @@ public function testPatchUpdate() {
   +  public function testPatchUpdateWithoutCSRFToken() {
   

   And same here, but in this case, let's do the "oops I forgot the X-CSRF-Token" request just before

       $this->httpRequest($url, 'PATCH', $serialized, $mime_type);
       $this->assertResponse(200);
   

Perfect :)

Absolutely right!

Created change record: https://www.drupal.org/node/2775521.

FYI: The foundation laid here will also be used by #2808233: REST 403 responses don't tell the user *why* access is not granted: requires deep Drupal understanding to figure out :)

The follow-up #2795965: REST requests with invalid X-CSRF-Token header get "missing " mesage was created and solved.

And just now #2826391: CsrfAccessCheck should have proper error feedback for invalid/missing CSRF token query argument just like CsrfRequestHeaderAccessCheck was created.