Problem/Motivation
- Enable REST module
- 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!
- 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? - 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
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.- 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:
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.- 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
Better feedback when missingDone in #2659070: REST requests without Content-Type header: unhelpful response significantly hinders DX, should receive a 415 response.Content-Type
request header (and 415 instead of 405 response).Better feedback when missing/invalidX-CSRF-Token
request header.Test coverage.
User interface changes
None.
API changes
TBD
Data model changes
None.
Comment | File | Size | Author |
---|---|---|---|
#44 | interdiff.txt | 745 bytes | garphy |
#44 | rest_xcsrftoken_error_message-2681911-44.patch | 14.7 KB | garphy |
Comments
Comment #2
Wim LeersTo 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.
Comment #4
Wim LeersAnother 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.
Comment #5
Wim LeersComment #6
gabesulliceI'm working on this issue with valthebald to triage this.
Comment #7
gabesulliceCan confirm, error messages remain ambiguous.
Comment #8
xjmThanks @gabesullice for confirming the issue! Updating issue credit.
Comment #9
Wim LeersRetitled to be consistent with the title of #2659070: REST requests without Content-Type header: unhelpful response significantly hinders DX, should receive a 415 response.
Comment #10
Wim LeersPer http://tools.ietf.org/html/rfc7235#section-4.2 & https://www.w3.org/Protocols/rfc2616/rfc2616-sec10.html#sec10.4.2:
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.Comment #11
Wim Leersd.o--, it lost my issue summary changes.
Comment #12
Wim LeersComment #13
Wim LeersProblem analysis + possible solutions, from #2659070-15: REST requests without Content-Type header: unhelpful response significantly hinders DX, should receive a 415 response:
Comment #14
Wim LeersFor now I've opted to let only
AccessResultForbidden
contain a message.Test before vs after by using
Comment #15
dawehnerComment #16
neclimdulNice work! Obviously this is really NW because of tests and the interface but in terms of discussing the approach a big +1.
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.Comment #17
Wim LeersCool :) I think this may actually be a good novice task, perhaps for Drupal Dev Days Milan!? :)
Comment #18
xjm@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.
Comment #19
Wim LeersClearly nobody did. So I'll take it on again.
Comment #20
Wim LeersThese tags are still relevant. Unassigning until I really start working on it. Anybody, feel free to step up!
Comment #21
garphy CreditAttribution: garphy at ICI LA LUNE commentedLet me give this a try
Comment #22
garphy CreditAttribution: garphy at ICI LA LUNE commentedRebasing to take care of #2753681: Move CSRF header token out of REST module so that user module can use it, as well as any contrib module.
Let's add some tests now.
Comment #23
garphy CreditAttribution: garphy at ICI LA LUNE commentedNow there's an interface and a basic test.
Let's see what the bot thinks.
Comment #24
garphy CreditAttribution: garphy at ICI LA LUNE commentedComment #26
garphy CreditAttribution: garphy at ICI LA LUNE commentedMy bad. Forgot to make the $reason constructor parameter optional.
Comment #27
dawehnerIs there a reason for this super strictness here?
Maybe giving an example what a reason might be would help to know about the possible things people could put into it.
Nitpick: This should start with an uppercase character and end with a dot, because its a sentence.
It feels like some test for this part could be useful
Comment #29
Wim LeersThanks, @garphy!
#27:
getReason()
Nit: s/Construct/Constructs/
What about
?Should extend
AccessResultInterface
.And I wonder if it should be called
AccessResultWithReasonInterface
?s/Get/Gets/
s/Set/Sets/
s/detailed reason/reason/
s/result/access result/
Missing capitalization at the beginning.
Missing period at the end.
s/detail/reason/
Missing
@return
documentation.One unnecessary blank line.
Should be
assertSame()
. (For stricter checking.)Let's use a random string here:
$this->randomGenerator->string();
.Comment #30
garphy CreditAttribution: garphy at ICI LA LUNE commented#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.
Comment #31
garphy CreditAttribution: garphy at ICI LA LUNE commentedOk... I missed Wim's post.
Stay tuned for the next patch
Comment #32
garphy CreditAttribution: garphy at ICI LA LUNE commentedThanks @dawehner & @Wim Leers for your reviews.
I think I addressed everything.
Comment #33
Wim Leers@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…
Comment #34
Wim LeersAccessAwareRouter
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!s/an access rejection :/for forbidden access:/
Let's say "you are" so you don't need the backslash.
Also: hah! :D
Needs FQCN.
s/of/for/
Needs FQCN.
You don't need these anymore :)
X-CSRF-Token
request header) to:\Drupal\rest\Tests\UpdateTest::testPatchUpdate()
\Drupal\rest\Tests\CreateTest::testCreateEntityTest()
\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.
Comment #35
garphy CreditAttribution: garphy at ICI LA LUNE commentedI adressed #34.[1-7]
I trigger the bot for checking the test of AccessAwareRouter
I will adress #34.8 later on.
Comment #36
Wim LeersNit: Omit "specific", so it fits within 80 cols.
Perfect! :)
That leaves just #34.8, then this is ready! Woot!
@garphy++
@garphy++
@garphy++
@garphy++
Comment #37
Wim LeersTo be clear, this needs work for #34.8.
Comment #38
dawehnerThis should use @code and @endcode
I guess we could also put @covers ::getReason on here
Nice!
Comment #39
Wim Leers#38++ — great review, thanks!
Comment #40
garphy CreditAttribution: garphy at ICI LA LUNE commentedGreat reviews, thanks !
Adressed #34.8, #38.1 and #38.2
Comment #42
Wim LeersGreat work! But I think it can be done in a much more simple way :)
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 :)
Rather than doing this, I'd add a
$forget_xcsrf_token
boolean parameter that defaults toFALSE
to\Drupal\rest\Tests\RESTTestBase::httpRequest()
.In that method, I'd then change
to:
etc
And then you can update
\Drupal\rest\Tests\CreateTest::assertCreateEntityOverRestApi()
to once do a request with that new boolean parameter set toTRUE
, followed by$this->assertResponse(403, 'X-CSRF-Token request header is missing');
.That means we avoid:
It also means you end up with a realistic flow: "oops I forgot, let me fix that in my next request".
Similar observation for this one. In this case, I'd do the "oops I forgot" request before these lines:
And same here, but in this case, let's do the "oops I forgot the X-CSRF-Token" request just before
Comment #43
garphy CreditAttribution: garphy at ICI LA LUNE commentedWim Leers++ for your patient & detailed reviews :)
Here's a new patch which hopefully solves all points listed in #42.
Comment #44
garphy CreditAttribution: garphy at ICI LA LUNE commentedErr... previous patch won't pass. Stupid mistake.
This one should make it.
Comment #46
Wim LeersPerfect :)
Comment #47
alexpottI think we need a change record for the new interface and stuff introduced by this issue. Code and tests look solid.
Comment #48
Wim LeersAbsolutely right!
Created change record: https://www.drupal.org/node/2775521.
Comment #49
alexpottCommitted 3de7999 and pushed to 8.2.x. Thanks!
Comment #54
Wim LeersFYI: 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 :)
Comment #55
Wim LeersThe follow-up #2795965: REST requests with invalid X-CSRF-Token header get "missing " mesage was created and solved.
Comment #56
Wim LeersAnd just now #2826391: CsrfAccessCheck should have proper error feedback for invalid/missing CSRF token query argument just like CsrfRequestHeaderAccessCheck was created.
Comment #57
Wim Leers