Problem/Motivation

If a REST plugin throws an HttpException, the request handler will catch it and output a response using the exception message and status code. The HttpException class does not however, check that the status code is valid. So when creating the response, if the code was set to 0 for example, Symfony promptly throws another exception, which overrides the expected output.

Actual output:
{"message":"A fatal error occurred: The HTTP status code \u00220\u0022 is not valid."}

Expected output:
{"error":"Exception message here."}

Proposed resolution

Let's make Drupal more robust. If the code is < 100 or >= 600, then set the HTTP status code to 500.

Additionally, it may be beneficial to add documentation reminding REST plugin authors to a) stick with HttpExceptions, not generic exceptions, and b) use valid status codes.

Remaining tasks

Needs a new patch, and review.

User interface changes

None.

API changes

None.

Data model changes

None.

CommentFileSizeAuthor
#1 2512652.patch1.55 KBMattA
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

MattA’s picture

MattA’s picture

Status: Active » Needs review
MattA’s picture

Issue summary: View changes

Cleaned up the issue summary.

MattA’s picture

Status: Needs review » Needs work
Wim Leers’s picture

Category: Bug report » Support request
Status: Needs work » Fixed

I worked on reproducing this. So I made the following change:

diff --git a/core/modules/rest/src/Plugin/rest/resource/EntityResource.php b/core/modules/rest/src/Plugin/rest/resource/EntityResource.php
index 0477982..8100d62 100644
--- a/core/modules/rest/src/Plugin/rest/resource/EntityResource.php
+++ b/core/modules/rest/src/Plugin/rest/resource/EntityResource.php
@@ -45,6 +45,7 @@ class EntityResource extends ResourceBase {
    * @throws \Symfony\Component\HttpKernel\Exception\HttpException
    */
   public function get(EntityInterface $entity) {
+    throw new \Exception('yar');
     if (!$entity->access('view')) {
       throw new AccessDeniedHttpException();
     }

This produces a HTTP 500 response. Good so far.

The response body is:

{}

This is the problem the IS complains about. Well, the reason is that by default verbose error logging is turned off, to not leak sensitive information. You must enable verbose error logging. After doing so, the HTTP 500 response's body is now:

{"message":"A fatal error occurred: yar"}

Learn how to enable verbose error logging by looking at example.settings.local.php and https://www.drupal.org/node/2259531.

MattA’s picture

Category: Support request » Bug report
Status: Fixed » Needs work

Uhhh... it has been quite some time since I filed this, but I don't think that is quite what was going on originally.

In your example, you throw an \Exception which is NOT caught by the RequestHandler, and sent further up the stack for Drupal to process. If I remember correctly, this issue was caused by throwing a Symfony\Component\HttpKernel\Exception\HttpException with an invalid status code, which IS caught by the RequestHandler, and then mangled when it tries to generate the response and fails.

Since HttpException messages should be output to clients, an empty response and {"message":"A fatal error occurred: ____"} are both invalid responses in this situation. This can be seen by clients when they then decode the response. Errors are expected to be in the 'error' key, which is not generated by the completely separate "fatal error" exception.

MattA’s picture

Issue summary: View changes

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.

dawehner’s picture

@MattA
Are you sure that setting an invalid code actually results in a Http exception?

        $this->statusCode = $code = (int) $code;
        if ($this->isInvalid()) {
            throw new \InvalidArgumentException(sprintf('The HTTP status code "%s" is not valid.', $code));
        }

I tried out the following:

diff --git a/core/modules/rest/src/RequestHandler.php b/core/modules/rest/src/RequestHandler.php
index 9efdb35..be4c0c8 100644
--- a/core/modules/rest/src/RequestHandler.php
+++ b/core/modules/rest/src/RequestHandler.php
@@ -119,6 +119,7 @@ public function handle(RouteMatchInterface $route_match, Request $request) {
       // Add rest settings config's cache tags.
       $response->addCacheableDependency($this->container->get('config.factory')->get('rest.settings'));
     }
+    $response->setStatusCode(23);
     return $response;
   }

Which results in the following response (with verbose):

$ http GET "http://d8.dev/node/1?_format=hal_json"
HTTP/1.1 500 500 Service unavailable (with message)
Cache-Control: must-revalidate, no-cache, private
Connection: close
Content-Length: 1867
Content-Type: application/hal+json
Content-language: en
Date: Thu, 05 May 2016 07:42:21 GMT
Expires: Sun, 19 Nov 1978 05:00:00 GMT
Server: Apache/2.4.16 (Unix) PHP/5.6.18
X-Content-Type-Options: nosniff
X-Content-Type-Options: nosniff
X-Frame-Options: SAMEORIGIN
X-Generator: Drupal 8 (https://www.drupal.org)
X-Powered-By: PHP/5.6.18
X-UA-Compatible: IE=edge

The website encountered an unexpected error. Please try again later.</br></br><em class="placeholder">InvalidArgumentException</em>: The HTTP status code &quot;23&quot; is not valid. in <em class="placeholder">Symfony\Component\HttpFoundation\Response-&gt;setStatusCode()</em> (line <em class="placeholder">456</em> of <em class="placeholder">vendor/symfony/http-foundation/Response.php</em>). <pre class="backtrace">Drupal\rest\RequestHandler-&gt;handle(Object, Object)
call_user_func_array(Array, Array) (Line: 123)
Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber-&gt;Drupal\Core\EventSubscriber\{closure}() (Line: 574)
Drupal\Core\Render\Renderer-&gt;executeInRenderContext(Object, Object) (Line: 124)
Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber-&gt;wrapControllerExecutionInRenderContext(Array, Array) (Line: 97)
Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber-&gt;Drupal\Core\EventSubscriber\{closure}()
call_user_func_array(Object, Array) (Line: 139)
Symfony\Component\HttpKernel\HttpKernel-&gt;handleRaw(Object, 1) (Line: 62)
Symfony\Component\HttpKernel\HttpKernel-&gt;handle(Object, 1, 1) (Line: 57)
Drupal\Core\StackMiddleware\Session-&gt;handle(Object, 1, 1) (Line: 48)
Drupal\Core\StackMiddleware\KernelPreHandle-&gt;handle(Object, 1, 1) (Line: 206)
Drupal\page_cache\StackMiddleware\PageCache-&gt;fetch(Object, 1, 1) (Line: 120)
Drupal\page_cache\StackMiddleware\PageCache-&gt;lookup(Object, 1, 1) (Line: 74)
Drupal\page_cache\StackMiddleware\PageCache-&gt;handle(Object, 1, 1) (Line: 47)
Drupal\Core\StackMiddleware\ReverseProxyMiddleware-&gt;handle(Object, 1, 1) (Line: 50)
Drupal\Core\StackMiddleware\NegotiationMiddleware-&gt;handle(Object, 1, 1) (Line: 23)
Stack\StackedHttpKernel-&gt;handle(Object, 1, 1) (Line: 628)
Drupal\Core\DrupalKernel-&gt;handle(Object) (Line: 19)

and without verbose:

$ http GET "http://d8.dev/node/1?_format=hal_json"
HTTP/1.1 500 500 Service unavailable (with message)
Cache-Control: must-revalidate, no-cache, private
Connection: close
Content-Length: 68
Content-Type: application/hal+json
Content-language: en
Date: Thu, 05 May 2016 07:43:42 GMT
Expires: Sun, 19 Nov 1978 05:00:00 GMT
Server: Apache/2.4.16 (Unix) PHP/5.6.18
X-Content-Type-Options: nosniff
X-Content-Type-Options: nosniff
X-Frame-Options: SAMEORIGIN
X-Generator: Drupal 8 (https://www.drupal.org)
X-Powered-By: PHP/5.6.18
X-UA-Compatible: IE=edge

The website encountered an unexpected error. Please try again later.
MattA’s picture

I just tried it again using this line to simulate an exception thrown by a plugin.

diff --git a/core/modules/rest/src/RequestHandler.php b/core/modules/rest/src/RequestHandler.php
index c585166..01c8ba7 100644
--- a/core/modules/rest/src/RequestHandler.php
+++ b/core/modules/rest/src/RequestHandler.php
@@ -87,6 +87,7 @@ public function handle(RouteMatchInterface $route_match, Request $request) {
     $format = $route_match->getRouteObject()->getRequirement('_format') ?: 'json';
     try {
       $response = call_user_func_array(array($resource, $method), array_merge($parameters, array($unserialized, $request)));
+      throw new HttpException(99, 'Not a valid status code');
     }
     catch (HttpException $e) {
       $error['error'] = $e->getMessage();

And got:

HTTP/1.1 500 Internal Server Error
Date: Mon, 23 May 2016 00:03:05 GMT
Server: Apache/2.4.20 (Win32) OpenSSL/1.0.2h PHP/5.5.35
x-content-type-options: nosniff, nosniff
X-Powered-By: PHP/5.5.35
Cache-Control: must-revalidate, no-cache, private
x-ua-compatible: IE=edge
Content-Language: en
X-Frame-Options: SAMEORIGIN
Expires: Sun, 19 Nov 1978 05:00:00 GMT
X-Generator: Drupal 8 (https://www.drupal.org)
Content-Length: 87
Connection: close
Content-Type: application/json

{"message":"A fatal error occurred: The HTTP status code \u002299\u0022 is not valid."}

There is a return statement in the catch block above, so the line you added would never be reached in this scenario.

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

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.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

Status: Needs work » Closed (works as designed)

\Symfony\Component\HttpFoundation\Response::setStatusCode() throws that exception.

The HttpException class does not however, check that the status code is valid. So when creating the response,

Let's make Drupal more robust. If the code is < 100 or >= 600, then set the HTTP status code to 500.

We don't babysit broken code.

Additionally, it may be beneficial to add documentation reminding REST plugin authors to a) stick with HttpExceptions, not generic exceptions, and b) use valid status codes.

a) we could document this better, BUT there are plenty of examples, and they'll quickly notice in their tests that non-HTTP exceptions are not handled, and cause fatal PHP errors
b) using valid status codes is so basic, and so easy to debug (the response you cite in #10 even says it explicitly!) that this is IMHO fine.

IOW: I don't think we need to add yet more magic here.

Wim Leers’s picture

Title: Exceptions using code 0 in REST plugins may cause fatal error. » HttpExceptions using invalid HTTP status codes in @RestResource plugins may cause fatal error

Better title.

joseph.olstad’s picture

Please re-open this issue as per rfc7231 and symfony issue 36823

Drupal is crashing currently on return codes outside of 1xx/5xx

https://tools.ietf.org/html/rfc7231#section-6

we're needing to support this for development on the linkchecker module.

I've made a pull request upstream to resolve the crashing which was promptly rebuked.
https://github.com/guzzle/psr7/pull/420