Follow-up to #2681911: REST requests without X-CSRF-Token header: unhelpful response significantly hinders DX, should receive a 401 response

Problem/Motivation

      if (!$this->csrfToken->validate($csrf_token, self::TOKEN_KEY)
        && !$this->csrfToken->validate($csrf_token, 'rest')) {
        return AccessResult::forbidden()->setReason('X-CSRF-Token request header is missing')->setCacheMaxAge(0);
      }

This message notifies that a token is missing even if one is given that just isn't valid.

Proposed resolution

Provide different messages if a token is provided versus when one is given but its invalid? Just make the message more generic?

Remaining tasks

User interface changes

API changes

Data model changes

CommentFileSizeAuthor
#12 invalid_csrf_token_feedback-2795965-12.patch10.12 KBwim leers
#12 interdiff.txt6.72 KBwim leers
#5 invalid_csrf_token_feedback-2795965-5.patch9.25 KBwim leers
#5 invalid_csrf_token_feedback-2795965-5-test-only.patch7.59 KBwim leers

Comments

neclimdul created an issue. See original summary.

neclimdul’s picture

neclimdul
He/Him
commented
Title: REST requests without X-CSRF-Token header: unhelpful response significantly hinders DX, should receive a 401 response » REST requests with invalid X-CSRF-Token header get "missing " mesage
wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

Ohhh! Of course :)

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Issue tags: -Triaged for D8 major current state, -Triaged core major, -Novice, -php-novice

This was not triaged.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Active » Needs review
StatusFileSize
new invalid_csrf_token_feedback-2795965-5-test-only.patch7.59 KB
new invalid_csrf_token_feedback-2795965-5.patch9.25 KB

And patch!

I wanted to assert that the appropriate message was being returned. But … turns out the existing test coverage is not testing that either, because WTB::assertResponse() is extremely misleading :(

So in the test we were only really asserting we were getting 403 responses. I verified manually at the time (and again today) that this indeed works in real life. But in the test, we were getting not application/hal+json responses, but … text/html responses!
You'll never guess the root cause for that one, especially because the correct DELETE request works just fine. Well, it turns out that if you don't specify ?_format=hal_json, you don't get the JSON exception subscriber for handling 4xx responses… which means you get DefaultExceptionSubscriber instead, so you get an HTML response.

This was supposed to be trivial, and it is, but the test fixes make it look less trivial. The big test-only patch shows that.

(I thought this patch would take me 4 to 5 minutes. It took me 4 to 5 hours.)

The last submitted patch, 5: invalid_csrf_token_feedback-2795965-5-test-only.patch, failed testing.

neclimdul’s picture

neclimdul
He/Him
commented 
  1. +++ b/core/modules/rest/src/Tests/CreateTest.php
@@ -366,10 +366,16 @@ public function assertCreateEntityOverRestApi($entity_type, $serialized = NULL)
+    $url = Url::fromUri('internal:/entity/' . $entity_type)->setOption('query', ['_format' => $this->defaultFormat]);
+    $this->httpRequest($url, 'POST', $serialized, $this->defaultMimeType, TRUE);
+    $this->assertResponse(403);

    So this fixes testing the original implementation. 

  2. +++ b/core/modules/rest/src/Tests/CreateTest.php
@@ -366,10 +366,16 @@ public function assertCreateEntityOverRestApi($entity_type, $serialized = NULL)
+    $this->httpRequest($url, 'POST', $serialized, $this->defaultMimeType, 'invalid-csrf-token');
+    $this->assertResponse(403);
+    $this->assertRaw('X-CSRF-Token request header is invalid');

+++ b/core/modules/rest/src/Tests/RESTTestBase.php
@@ -132,9 +134,9 @@ protected function httpRequest($url, $method, $body = NULL, $mime_type = NULL, $
+            'X-CSRF-Token: ' . (is_string($forget_xcsrf_token) ? $forget_xcsrf_token : $token),

    And this change lets us test the fixes for this issue? 

  3. +++ b/core/modules/rest/src/Tests/RESTTestBase.php
@@ -86,7 +86,9 @@ protected function setUp() {
    * @param bool $forget_xcsrf_token
-   *   If TRUE, the CSRF token won't be included in request.
+   *   If TRUE, the CSRF token won't be included in request. If a string, that
+   *   string will be used as the CSRF token (allows for testing invalid CSRF
+   *   tokens).

@@ -132,9 +134,9 @@ protected function httpRequest($url, $method, $body = NULL, $mime_type = NULL, $
-          CURLOPT_HTTPHEADER => !$forget_xcsrf_token ? array(
+          CURLOPT_HTTPHEADER => $forget_xcsrf_token !== TRUE ? array(
             'Content-Type: ' . $mime_type,
-            'X-CSRF-Token: ' . $token,
+            'X-CSRF-Token: ' . (is_string($forget_xcsrf_token) ? $forget_xcsrf_token : $token),

    It looks like forget_xcsrf is no longer an accurate description of this argument... This should probably be fixed but I don't really want to block this quick win. I assume that's why you crammed it in. How did this quickfix get so complicated.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
  1. Yep!
  2. Yep again!
  3. Indeed. I'd be happy to rename it though. Perhaps something like just $csrf_token being: A) TRUE by default (for "please generate it"), B) FALSE (for "please forget it") or C) a string (for "please use this as the CSRF token")? Does that make sense to you?
neclimdul’s picture

neclimdul
He/Him
commented

yucky tri-state. Yeah, I think if I was starting from scratch I'd use NULL but for what you described makes sense and doesn't break backwards compatibility.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

It does break BC, because the parameter name is no longer a negative, so the boolean values must be inverted. You'd use NULL instead of FALSE in my example?

neclimdul’s picture

neclimdul
He/Him
commented

A) Null default. Generate token B) Truish use value C) False don't do anything with CSRF.

Its going to be complicated regardless because we're relying on loose typing but this makes it closer to being typed so it makes a little more sense to me. As long as we document it I think its fine.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
StatusFileSize
new interdiff.txt6.72 KB
new invalid_csrf_token_feedback-2795965-12.patch10.12 KB

That makes sense! Thanks for your input, it's much appreciated :)

Done.

neclimdul’s picture

neclimdul
He/Him
commented
Status: Needs review » Reviewed & tested by the community

Works for me!

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Patch (to be ported)

Committed 676f38d and pushed to 8.3.x. Thanks!

Setting to patch to ported so that once 8.2.0 is released this can be cherry-picked to 8.2.x

  • alexpott committed 676f38d on 8.3.x 
    Issue #2795965 by Wim Leers, neclimdul: REST requests with invalid X-...
wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Related issues: +#2811133: Confusing error response set by ContentTypeHeaderMatcher when forgetting the Content-Type request header
gnuget’s picture

gnuget
Primary language Spanish
commented

Drupal 8.2.0 is now out, Can we now cherry-pick this to the 8.2.x branch?

:D

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Patch (to be ported) » Fixed

Committed eb38af5 and pushed to 8.2.x. Thanks!

  • alexpott committed eb38af5 on 8.2.x 
    Issue #2795965 by Wim Leers, neclimdul: REST requests with invalid X-...

Status: Fixed » Closed (fixed)

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