Problem/Motivation

  1. Enable REST module
  2. Try doing a GET request, that works:
        jQuery.ajax({
          url: Drupal.url('node/' + nodeID) + '?_format=hal_json',
          method: 'GET',
          success: function(data, status, xhr) {
            var json = data;
            console.log(json);
          }
        });
    

    Success!

  3. Now let's try to do a PATCH request:
        jQuery.ajax({
          url: Drupal.url('node/' + nodeID) + '?_format=hal_json',
          method: 'PATCH',
          data: {},
        });
    

    Now you get a 405 (Method Not Allowed) response, with this body: {"message":"No route found for \u0022PATCH \/node\/2\u0022: Method Not Allowed (Allow: GET, POST, DELETE)"}. This makes no sense at all: why is the PATCH method not listed?

  4. Clearly, the previous thing is also wrong because the Content-Type request header is missing. So, let's add that:
        jQuery.ajax({
          url: Drupal.url('node/' + nodeID) + '?_format=hal_json',
          headers: {
            "Content-Type": "application/hal+json"
          },
          method: 'PATCH',
          data: {},
        });
    

    Now you get a 403, and {"message":""} as the response body.

Proposed resolution

  1. Upon debugging this, you end up deep in the routing system, where it will only have matched the "GET" and "DELETE" routes. The "PATCH" and "POST" routes are both missing. So if the "POST" route is missing, why is the "POST" method allowed? Because apparently the "GET" route allows both the "GET" and "POST" methods. This is fixed by #2659070: REST requests without Content-Type header: unhelpful response significantly hinders DX, should receive a 415 response.
  2. Oh, apparently you need to specify the X-CSRF-Token request header with a valid CSRF token as a value. Upon doing that, routing does work correctly. This will be solved in this issue.

Improve routing + response:

  1. Response should be a 415, not a 405. This points to a deep routing problem. Done in #2659070: REST requests without Content-Type header: unhelpful response significantly hinders DX, should receive a 415 response.
  2. Response should still be a 403, but its body should say Required "X-CSRF-Token" request header missing, see https://www.drupal.org/docs/somewhere/useful/info.. Not an empty message, which is just infuriating.

Remaining tasks

  1. Better feedback when missing Content-Type request header (and 415 instead of 405 response). Done in #2659070: REST requests without Content-Type header: unhelpful response significantly hinders DX, should receive a 415 response.
  2. Better feedback when missing/invalid X-CSRF-Token request header.
  3. Test coverage.

User interface changes

None.

API changes

TBD

Data model changes

None.

Support from Pantheon helps power Drupal and open source. Pantheon Learn more

Comments

Wim Leers created an issue. See original summary.

Wim Leers’s picture

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Wim Leers’s picture

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.

Wim Leers’s picture

Issue tags: +DrupalWTF
gabesullice’s picture

Issue tags: +neworleans2016

I'm working on this issue with valthebald to triage this.

gabesullice’s picture

Version: 8.1.x-dev » 8.2.x-dev
Status: Active » Needs work
Issue tags: -neworleans2016 +Triaged for D8 major current state

Can confirm, error messages remain ambiguous.

xjm’s picture

Thanks @gabesullice for confirming the issue! Updating issue credit.

Wim Leers’s picture

Title: REST requests without X-CSRF-Token header: unhelpful responses significantly hinder DX » REST requests without X-CSRF-Token header: unhelpful response significantly hinders DX, should receive a 401 response
Wim Leers’s picture

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.

Wim Leers’s picture

Issue summary: View changes

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

Wim Leers’s picture

Issue summary: View changes
Wim Leers’s picture

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. 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.
Wim Leers’s picture

Status: Needs work » Needs review
Issue tags: +Needs tests
FileSize
4.2 KB

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: {},
    });
dawehner’s picture

neclimdul’s picture

Nice work! Obviously this is really NW because of tests and the interface but in terms of discussing the approach a big +1.

+++ b/core/lib/Drupal/Core/Routing/AccessAwareRouter.php
@@ -105,7 +106,7 @@ protected function checkAccess(Request $request) {
+      throw new AccessDeniedHttpException($access_result instanceof AccessResultForbidden ? $access_result->getReason() : NULL);

I like this is much better then the (string). It avoids the quirks of __toString() and checking for the final interface here will be much more friendly and readable in terms of BC.

Wim Leers’s picture

Status: Needs review » Needs work
Issue tags: +Novice, +php-novice, +drupaldevdays

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

xjm’s picture

Issue tags: +Triaged D8 major

@webchick, @catch, @alexpott, @effulgentsia, @Cottser, and I followed up on the triage work above and agreed that this should be a major issue. This is a significant blocker for developer audience that makes people give up, and Drupal is giving the wrong response which makes debugging even more difficult. Thanks @gabesullice, @Wim Leers, and @neclimdul.

Wim Leers’s picture

Assigned: Unassigned » Wim Leers
Issue tags: -Novice, -php-novice, -drupaldevdays

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

Wim Leers’s picture

Assigned: Wim Leers » Unassigned
Issue tags: +Novice, +php-novice

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

garphy’s picture

Assigned: Unassigned » garphy

Let me give this a try

garphy’s picture

garphy’s picture

Now there's an interface and a basic test.
Let's see what the bot thinks.

garphy’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 23: rest_xcsrftoken_error_message-2681911-23.patch, failed testing.

garphy’s picture

My bad. Forgot to make the $reason constructor parameter optional.

dawehner’s picture

  1. +++ b/core/lib/Drupal/Core/Access/AccessResult.php
    @@ -51,11 +51,16 @@ public static function allowed() {
    +  public static function forbidden($reason = NULL) {
    +    assert('is_string($reason) || is_null($reason)');
    

    Is there a reason for this super strictness here?

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

    Maybe giving an example what a reason might be would help to know about the possible things people could put into it.

  3. +++ 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
    

    Nitpick: This should start with an uppercase character and end with a dot, because its a sentence.

  4. +++ b/core/lib/Drupal/Core/Routing/AccessAwareRouter.php
    @@ -105,7 +106,7 @@ protected function checkAccess(Request $request) {
         }
         if (!$access_result->isAllowed()) {
    -      throw new AccessDeniedHttpException();
    +      throw new AccessDeniedHttpException($access_result instanceof AccessResultReasonInterface ? $access_result->getReason() : NULL);
         }
    

    It feels like some test for this part could be useful

Status: Needs review » Needs work

The last submitted patch, 26: rest_xcsrftoken_error_message-2681911-26.patch, failed testing.

Wim Leers’s picture

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’s picture

Status: Needs work » Needs review
FileSize
8.16 KB

#27.1 : Removed.
I actually don't know why it's there. It was in Wim's original patch. Maybe he can give us some rationale about it.

#27.2 : Done

#27.3 : Done

#27.4 : Added AccessResultForbiddenTest which mainly checks that setReason() correctly returns the instance for fluid calls.

garphy’s picture

Ok... I missed Wim's post.

Stay tuned for the next patch

garphy’s picture

Thanks @dawehner & @Wim Leers for your reviews.

I think I addressed everything.

Wim Leers’s picture

@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…

Wim Leers’s picture

Status: Needs review » Needs work
  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.

garphy’s picture

Status: Needs work » Needs review
FileSize
9.48 KB
3.31 KB

I adressed #34.[1-7]
I trigger the bot for checking the test of AccessAwareRouter

I will adress #34.8 later on.

Wim Leers’s picture

  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++

Wim Leers’s picture

Status: Needs review » Needs work

To be clear, this needs work for #34.8.

dawehner’s picture

  1. +++ b/core/lib/Drupal/Core/Access/AccessResultReasonInterface.php
    @@ -0,0 +1,36 @@
    + * <code>
    ...
    + * 

    This should use @code and @endcode

  2. +++ b/core/tests/Drupal/Tests/Core/Access/AccessResultForbiddenTest.php
    @@ -0,0 +1,44 @@
    +   *
    +   * @covers ::__construct
    +   */
    +  public function testConstruction() {
    

    I guess we could also put @covers ::getReason on here

  3. +++ 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.
    +   */
    +  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);
    +  }
    

    Nice!

Wim Leers’s picture

#38++ — great review, thanks!

garphy’s picture

Status: Needs work » Needs review
FileSize
16.72 KB
9.04 KB

Great reviews, thanks !

Adressed #34.8, #38.1 and #38.2

Status: Needs review » Needs work

The last submitted patch, 40: rest_xcsrftoken_error_message-2681911-40.patch, failed testing.

Wim Leers’s picture

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);
    
garphy’s picture

Status: Needs work » Needs review
FileSize
14.69 KB
11.75 KB

Wim Leers++ for your patient & detailed reviews :)
Here's a new patch which hopefully solves all points listed in #42.

garphy’s picture

Err... previous patch won't pass. Stupid mistake.
This one should make it.

The last submitted patch, 43: rest_xcsrftoken_error_message-2681911-43.patch, failed testing.

Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs tests

Perfect :)

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs change record

I think we need a change record for the new interface and stuff introduced by this issue. Code and tests look solid.

Wim Leers’s picture

Issue summary: View changes
Status: Needs work » Reviewed & tested by the community
Issue tags: -Needs change record

Absolutely right!

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

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 3de7999 and pushed to 8.2.x. Thanks!

  • alexpott committed 3de7999 on 8.2.x
    Issue #2681911 by garphy, Wim Leers, gabesullice: REST requests without...

  • alexpott committed 3de7999 on 8.3.x
    Issue #2681911 by garphy, Wim Leers, gabesullice: REST requests without...

  • alexpott committed 3de7999 on 8.3.x
    Issue #2681911 by garphy, Wim Leers, gabesullice: REST requests without...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

Wim Leers’s picture

Wim Leers’s picture

Wim Leers’s picture