REST requests without Content-Type header: unhelpful response significantly hinders DX, should receive a 415 response

neclimdul and I both struggled with this for hours.

Both of us have lots of Drupal 8 experience.

Hence this is a significant DX problem.

#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.

+1 for a tag, but let's call it RX then, not RE. That'd be in sync with DX, TX …

Let's all cheerlead for @dawehner!

\o/ \o/ \o/ dawehner \o/ \o/ \o/

Thanks so much for debugging that, Daniel!

+1, #8 explains why this is primarily a routing system problem.

Discovered #2412485: Unable to sent PATCH request to node/xx, which is a duplicate of this.

1. MethodFilter (when both Content-Type and X-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:

throw new UnsupportedMediaTypeHttpException('No route found that matches the Content-Type header.');

Note the message there. Sadly, the body you actually get is {}. The tremendously helpful message is gone!

2. ExceptionJsonSubscriber (when both Content-Type and X-CSRF-Token request headers are missing)

The root cause for that helpful message to have disappeared is that ExceptionHalJsonSubscriber doesn't implement on415, and therefore it says I don't know how to generate a response body for 415 HTTP responses with Content-Type HAL+JSON. Implementing it in ExceptionJsonSubscriber 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:

    $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 Content-Type request header is fully solved by this patch. (Tests still needed.)
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.

Updating IS to reflect current status/remaining tasks.

What about adding a message support for access result objects and use that? So like a new interface with a new method?

That's what I thought too. But then I also realized we have __toString(). So we could literally just add a setMessage() method to the existing AccessResult 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.

1. +++ b/core/lib/Drupal/Core/Routing/MethodFilter.php
   @@ -25,7 +25,7 @@ public function filter(RouteCollection $collection, Request $request) {
   -    foreach ($collection as $name => $route) {
   +    foreach ($collection->all() as $name => $route) {
   

   Interesting. Why is this better? \Drupal\Core\Routing\ContentTypeHeaderMatcher doesn't use ->all() either.

2. +++ b/core/modules/rest/src/Tests/UpdateTest.php
   @@ -55,6 +56,13 @@ public function testPatchUpdate() {
   +    return;
   

   This early return means later tests don't run.

3. +++ b/core/modules/system/src/Tests/Routing/ExceptionHandlingTest.php
   @@ -35,6 +35,19 @@ protected function setUp() {
   +   * Tests on a route with a non supported HTTP method.
   

   s/non supported/non-supported/

4. +++ b/core/tests/Drupal/Tests/Core/Routing/MethodFilterTest.php
   @@ -7,6 +7,98 @@
   +    $collection->add('test_route3.get', new Route('/test', [], [], [], '', [], ['PUSH']));
   

   PUSH? :P

Just had a look at the standard, and it seems to be totally possible to come with your own ones

:D Works for me!

So, now only this remains to be done:

2. Better feedback when missing/invalid X-CSRF-Token request header.

IS & tags updated.

dawehner++

Actually, 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_401

Yeah 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?

Oops, 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.

I'll get this done somewhere in the next few days, unless somebody beats me to it.

Per #31, splitting up this issue.

But first, #38 introduced this:

+++ b/core/lib/Drupal/Core/Routing/MethodFilter.php
@@ -27,6 +27,11 @@ public function filter(RouteCollection $collection, Request $request) {
+      // If the GET method is allowed we also need to allow the HEAD method
+      // since HEAD is a GET method that doesn't return the body.
+      if (in_array('GET', $supported_methods)) {
+        $supported_methods[] = 'HEAD';
+      }

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.

Split 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.

#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.

+++ b/core/modules/rest/src/Tests/UpdateTest.php
@@ -52,6 +53,12 @@ public function testPatchUpdate() {
+    // Update the entity over the REST API but forget to specify a Content-Type
+    // header, this should throw the proper exception.
+    $this->httpRequest($entity->toUrl(), 'PATCH', $serialized, 'none');
+    $this->assertResponse(Response::HTTP_UNSUPPORTED_MEDIA_TYPE);
+    $this->assertRaw('No route found that matches "Content-Type: none"');

This 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.

A REST route: http://tres/node/1?_format=hal_json

XHProf (best of 3): from 9864 function calls in HEAD to 9888 function calls with this patch: +24 function calls, or +0.2%

ab -c 1 -n 1000 -g ab-rest.tsv http://tres/node/1?_format=hal_json (best of 3)

• HEAD:
  

  Requests per second:    27.99 [#/sec] (mean)
  Time per request:       35.722 [ms] (mean)
  Time per request:       35.722 [ms] (mean, across all concurrent requests)
  Transfer rate:          90.27 [Kbytes/sec] received
  
  Connection Times (ms)
                min  mean[+/-sd] median   max
  Connect:        0    0   0.0      0       0
  Processing:    30   36   1.4     36      42
  Waiting:       30   36   1.4     36      42
  Total:         30   36   1.4     36      42
  

• patch:
  

  Requests per second:    28.03 [#/sec] (mean)
  Time per request:       35.679 [ms] (mean)
  Time per request:       35.679 [ms] (mean, across all concurrent requests)
  Transfer rate:          90.38 [Kbytes/sec] received
  
  Connection Times (ms)
                min  mean[+/-sd] median   max
  Connect:        0    0   0.0      0       1
  Processing:    32   36   1.5     35      53
  Waiting:       32   35   1.5     35      53
  Total:         33   36   1.5     35      53
  

• Histogram:

The frontpage: http://tres/

XHProf (best of 3): from 57394 function calls in HEAD to 57453 function calls with this patch: +59 function calls, or +0.1%

ab -c 1 -n 1000 -g ab-front.tsv http://tres/ (best of 3)

• HEAD:
  

  Requests per second:    21.57 [#/sec] (mean)
  Time per request:       46.353 [ms] (mean)
  Time per request:       46.353 [ms] (mean, across all concurrent requests)
  Transfer rate:          647.39 [Kbytes/sec] received
  
  Connection Times (ms)
                min  mean[+/-sd] median   max
  Connect:        0    0   0.0      0       0
  Processing:    40   46   1.6     46      56
  Waiting:       35   40   1.5     40      51
  Total:         40   46   1.6     46      56
  

• patch:
  

  Requests per second:    21.77 [#/sec] (mean)
  Time per request:       45.929 [ms] (mean)
  Time per request:       45.929 [ms] (mean, across all concurrent requests)
  Transfer rate:          653.37 [Kbytes/sec] received
  
  Connection Times (ms)
                min  mean[+/-sd] median   max
  Connect:        0    0   0.0      0       0
  Processing:    39   46   1.8     46      53
  Waiting:       35   40   1.6     40      47
  Total:         39   46   1.8     46      53
  

• Histogram:

Conclusion

A very small increase in number of functional calls.

A performance difference that falls within the error margin.

Therefore this is acceptable, so I RTBC'd.

Better title.

#51:

1. That sounds great.
2. I had to look this up: the CR is https://www.drupal.org/node/2405829. Sounds great.

First, 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.

Addressed #51.2.

After looking into it more, I agree with Crell's reply to #51.1:

Potentially, but that's a separate matter from this fix so I'd say out of scope.

Especially because it's the standard in Symfony to not explicitly mention HEAD. See for example \Symfony\Component\Routing\Matcher\Dumper\PhpMatcherDumper::compileRoute():

        // GET and HEAD are equivalent
        if (in_array('GET', $methods) && !in_array('HEAD', $methods)) {
            $methods[] = 'HEAD';
        }

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 both GET and POST). That runs with priority 5000. We'd need to be extremely careful with the priority we choose for a new RoutingEvents::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.

1. I keep forgetting in_array() has a "strict" parameter. Thanks, done!
2. Good point. Done :)

Back to RTBC, because this contains only trivial changes.

I 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! :)