Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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.
Oh, apparently you need to specify theThis will be solved in #2681911: REST requests without X-CSRF-Token header: unhelpful response significantly hinders DX, should receive a 401 response.X-CSRF-Token
request header with a valid CSRF token as a value. Upon doing that, routing does work correctly.
Improve routing + response:
- Response should be a 415, not a 405. This points to a deep routing problem.
Response should be a 401, not a 403, and its body should sayThis will be solved in #2681911: REST requests without X-CSRF-Token header: unhelpful response significantly hinders DX, should receive a 401 response.Required "X-CSRF-Token" request header missing, see https://www.drupal.org/docs/somewhere/useful/info.
. Not an empty message, which is just infuriating.
(Note this seems to belong both in the rest.module
component and the routing system
component.)
Remaining tasks
Better feedback when missingContent-Type
request header (and 415 instead of 405 response).Better feedback when missing/invalidThis will be solved in #2681911: REST requests without X-CSRF-Token header: unhelpful response significantly hinders DX, should receive a 401 response.X-CSRF-Token
request header (and 401 instead of 403 response).Test coverage.
User interface changes
None.
API changes
TBD
Data model changes
TBD
Comment | File | Size | Author |
---|---|---|---|
#61 | 2659070-61.patch | 10.88 KB | Wim Leers |
#48 | 2659070-48.patch | 9.64 KB | Wim Leers |
Comments
Comment #2
Wim Leersneclimdul and I both struggled with this for hours.
Both of us have lots of Drupal 8 experience.
Hence this is a significant DX problem.
Comment #3
Wim LeersComment #4
Wim Leers#2449143: REST views specify HTML as a possible request format, so if there is a "regular" HTML view on the same path, it will serve JSON is quite possibly caused by the same underlying problems in the routing system that the issue summary of this issue describes in some detail.
Comment #5
dawehnerAdding other random related issue. IMHO we should just have a tag, RE (REST experience ): #2594777: Accessing a non existing format results in a 500
Comment #6
Wim Leers+1 for a tag, but let's call it
then, not . That'd be in sync with DX, TX …Comment #7
swentel CreditAttribution: swentel as a volunteer commentedI really hope one day we can tag something with BMX .. ;)
Comment #8
dawehnerFinally figured out the problem here. Unlike content type, request methods are not filtered by a route filter but after that using
\Symfony\Component\Routing\Matcher\UrlMatcher::matchCollection
. At that point in time, we don't have the list of all routes and so no longer all routes,and so no methods.
One way could be to add a route filter for request methods, in which case we could throw some form of helpful exception.
Comment #9
Wim LeersLet's all cheerlead for @dawehner!
Thanks so much for debugging that, Daniel!
Comment #10
dawehnerNo its just up to you to actually solve the problem :) Do we want to take the pill and slow down our routing by introducing another route filter?
Maybe that wouldn't be a problem because we just do things we would do anyway but just simply a bit earlier? Kinda tricky questions to ask I'd say.
Should we move this to the routing component instead?
Comment #11
Crell CreditAttribution: Crell at Palantir.net for Acquia commentedComment #12
Wim Leers+1, #8 explains why this is primarily a routing system problem.
Comment #13
Wim LeersDiscovered #2412485: Unable to sent PATCH request to node/xx, which is a duplicate of this.
Comment #14
Crell CreditAttribution: Crell at Palantir.net for Acquia commenteddawehner and I discussed this a bit in IRC, and I did a bit of quick googling (which is not a definitive answer, of course).
The problem is that we're filtering by content type before we filter by method. That is, technically, incorrect as it results in, er, this exact problem. Routes that are not content-type sensitive (like GET and DELETE) will always survive the content-type filter, so we can never get a content-type exception.
The proper fix is to move method filtering before the content type filter by adding a MethodFilter that runs earlier. That way, by the time we get to Content-Type filter we have only the methods for routes that are relevant (PATCH in this case), and so the content type filter will do its thing, end up with an empty list, and throw the appropriate exception.
That's pretty simple to do, actually. The concern is whether there's a performance implication to that, and if we can swallow it. Unfortunately I don't know of any way to figure that out other than to try and and benchmark it, then decide if we can swallow it. If not, then I'm not sure what plan B is.
Anyone want to volunteer?
Comment #15
Wim Leers1.
MethodFilter
(when bothContent-Type
andX-CSRF-Token
request headers are missing)Added
MethodFilter
. This solves point 3 in the problem description/implements point 1 in the proposed solution.So, with this, if you repeat point 3 in the problem description, you no longer 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)"}
.Instead, you get a 415 (Unsupported Media Type), which makes sense, because
\Drupal\Core\Routing\ContentTypeHeaderMatcher::filter()
now runs after (the newly added)MethodFilter
and it does:Note the message there. Sadly, the body you actually get is
{}
. The tremendously helpful message is gone!2.
ExceptionJsonSubscriber
(when bothContent-Type
andX-CSRF-Token
request headers are missing)The root cause for that helpful message to have disappeared is that
ExceptionHalJsonSubscriber
doesn't implementon415
, and therefore it says . Implementing it inExceptionJsonSubscriber
means both JSON and HAL+JSON 415 responses can be generated.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:Instead of this:
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()
tothrow 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
Content-Type
request header is fully solved by this patch. (Tests still needed.)X-CSRF-Token
(or sending an invalid one) is not yet solved by this patch. Two possible solutions are given: allowAccessResultInterface
objects to contain a message, or allowAccessCheckInterface
implementations to not return anAccessResultInterface
object but throw an exception (questionable.Comment #17
Wim LeersComment #19
Wim LeersUpdating IS to reflect current status/remaining tasks.
Comment #20
dawehnerIMHO we should add a higher priority than the other ones.
Yeah that feels odd. What about adding a message support for access result objects and use that? So like a new interface with a new method?
Comment #22
Wim LeersThat's what I thought too. But then I also realized we have
__toString()
. So we could literally just add asetMessage()
method to the existingAccessResult
and that'd be okay too. But if we can think of a better name/semantic for that interface, then +1 for such an interface.Comment #23
Wim LeersInteresting. Why is this better?
\Drupal\Core\Routing\ContentTypeHeaderMatcher
doesn't use->all()
either.This early return means later tests don't run.
s/non supported/non-supported/
PUSH? :P
Comment #24
dawehnerWell, otherwise my IDE doesn't give my autocompletion.
Ha yeah, but it also prooves I executed the tests locally :)
Just had a look at the standard, and it seems to be totally possible to come with your own ones:
, see https://www.w3.org/Protocols/rfc2616/rfc2616-sec9.html
Comment #25
Wim Leers:D Works for me!
Comment #26
dawehnerThank you for your review!
Comment #27
Wim LeersSo, now only this remains to be done:
IS & tags updated.
dawehner++
Comment #29
Wim LeersActually, the problem wrt
X-CSRF-Token
is even worse than I thought: #2573537: Confusing http status code returned if operation is failed due to authentication failed (now marked a duplicate) made me realize that the correct response is not a 403, but a 401. https://en.wikipedia.org/wiki/HTTP_401Comment #30
dawehnerWell, do you think we should all those problems in this issue or rather split it up?
Comment #31
Wim LeersYeah I was thinking about that too. Splitting up probably makes more sense. This patch solves the first problem (but still needs to pass all tests). Then we could reopen #257537: file_check_directory is verbose on success. to solve the second problem.
What do you think?
Comment #32
dawehnerEven I guess you mean a different issue, I agree.
Comment #33
Wim LeersOops, yes, I forgot to type one digit. I meant #2573537: Confusing http status code returned if operation is failed due to authentication failed of course.
Comment #34
Wim LeersI'll get this done somewhere in the next few days, unless somebody beats me to it.
Comment #35
tic2000 CreditAttribution: tic2000 at Intellix commentedThis is a reroll of the patch in #26 which didn't apply anymore. NR just so it runs the tests to be sure I got it right.
Comment #37
tic2000 CreditAttribution: tic2000 at Intellix commentedThe additional fail is because of the change introduced in #2417917: Include content type format name in error response
Comment #38
tic2000 CreditAttribution: tic2000 at Intellix commentedThis should fix the failures.
Comment #39
Crell CreditAttribution: Crell as a volunteer commentedYay, working patch. Now who wants to benchmark it to see what the cost is?
Comment #40
Wim LeersPer #31, splitting up this issue.
But first, #38 introduced this:
and I don't understand why. Removing it to see if the patch still passes. Besides, if we add that, we should also update the test coverage.
Comment #41
Wim LeersComment #42
Wim LeersSplit off the second part of this issue to #2681911: REST requests without X-CSRF-Token header: unhelpful response significantly hinders DX, should receive a 401 response.
Comment #44
tic2000 CreditAttribution: tic2000 at Intellix commentedRegarding #40 the reason is in the comment. The test that fails does a drupalHead() request and fails since the HEAD request is not allowed. If you change the test to do a drupalGet() the test passes. But we shouldn't change that. We should allow HEAD when GET is allowed cause again as the comment states, a HEAD request is a GET request that doesn't need the body.
Also the fact that the test fails proves that we already have test coverage. Unless we want another one specific for HEAD requests, not one that happens to do a HEAD request and might be changed to a GET later for whatever reason.
Comment #45
Crell CreditAttribution: Crell as a volunteer commented#40: As the comment says, the access control of HEAD is/should be the same as GET, so anywhere GET is allowed HEAD should be allowed, too. A HEAD is, essentially, just a truncated GET. We should keep that.
Comment #46
alexpottDiscussed with @xjm and @catch. We agreed that this is a major bug because the rest API is a key API for Drupal going forward and we need to make it correct in terms of the response it emits and as easy to use as possible.
Comment #48
Wim Leers#44 + #45 answered #40. Thanks for those comments, that totally makes sense, and I should've thought of that too!
This reverts #41, so this is effectively identical to #38 again. Let's see if that still passes.
Comment #49
Wim LeersThis is the test that proves the DX is now better. It proves that if you forget to specify the
Content-Type
request header, that you actually get a helpful response.I wrote the initial implementation of the solution at #15, but a test was still missing. @dawehner added this in #20.
This is what proves that the issue adequately addresses the problem described in the IS.
In fact, @dawehner wrote full unit test coverage and significantly improved what I originally wrote. The patches I posted in #41 and #48 are not rerolls, they only remove one hunk and then add it back again, to better understand why that hunk was there.
Hence I think it's fair for me to RTBC this. On the condition that Crell mentioned in #14: that this doesn't impact performance too much.
Without further ado: here's performance tests that I did. Tested with PHP 5.5.1.1, as the anonymous user, with Page Cache turned off.
http://tres/node/1?_format=hal_json
ab -c 1 -n 1000 -g ab-rest.tsv http://tres/node/1?_format=hal_json
(best of 3)http://tres/
ab -c 1 -n 1000 -g ab-front.tsv http://tres/
(best of 3)Comment #50
Wim LeersBetter title.
Comment #51
dawehnerThis could be moved to a compile level operation.
Could we for this particular usecase actually use a lazy route filter, so we skip some of the executions on runtime?
Comment #52
Crell CreditAttribution: Crell at Platform.sh for Acquia commented#51.1: Potentially, but that's a separate matter from this fix so I'd say out of scope.
#51.2: I'm unclear what you mean by lazy route filter?
Comment #53
Wim Leers#51:
Comment #54
Wim LeersFirst, a straight reroll, because this did not apply at all anymore.
Furthermore, because this affects every request/response, this is AFAICT considered too big of a change for 8.1. So, moving this to 8.2.
Comment #55
dawehnerAgreed, its also not something which blocks people from building stuff in 8.1.
Comment #57
neclimdulGosh darn it update...
Comment #58
Wim LeersAddressed #51.2.
Comment #59
Wim LeersAfter looking into it more, I agree with Crell's reply to #51.1:
Especially because it's the standard in Symfony to not explicitly mention
HEAD
. See for example\Symfony\Component\Routing\Matcher\Dumper\PhpMatcherDumper::compileRoute()
:In other words, this is just to match that same behavior. So I'm not entirely sure that #51.1 would be appropriate. But even if it is, it'd require special care, and merits a separate discussion, because it must run after
\Drupal\Core\EventSubscriber\RouteMethodSubscriber::onRouteBuilding()
(which sets the default methods of bothGET
andPOST
). That runs with priority 5000. We'd need to be extremely careful with the priority we choose for a newRoutingEvents::ALTER
subscriber, that must run after that subscriber, but before certain others. Hence the need for a separate issue.So, given that, everything in #51 is addressed, and it required only trivial changes (a one-line change), so moving this back to RTBC.
Comment #60
alexpottShould we make these strict? It always feels safer.
This can go first no? No need to do the in_array on an empty
$supported_methods
right?Comment #61
Wim Leersin_array()
has a "strict" parameter. Thanks, done!Back to RTBC, because this contains only trivial changes.
Comment #62
xjmThis issue likely has a beta deadline, so tagging for visibility in the RTBC queue.
Comment #63
alexpottCommitted af80cb4 and pushed to 8.2.x. Thanks!
Comment #65
Wim LeersI think this issue/patch shows an aspect of the Symfony routing system that is well-designed: without having to change any existing routing code and by merely registering another route filter, we are able to efficiently, elegantly, succinctly solve an edge case.
Splendid! :)