craftcms / element-api

Create a JSON API/Feed for your elements in Craft.
MIT License
498 stars 57 forks source link

errors with newlines #115

Closed drifteaur closed 3 years ago

drifteaur commented 5 years ago

If Element API catches an exception, it will output the exception's message as a HTTP header. The exception message can contain newlines, in which case Yii will print an unhelpful message instead of the exception's message:

PHP Warning 'yii\base\ErrorException' with message 'Header may not contain more than a single header, new line detected' 

in /Users/drifter/www/project/vendor/yiisoft/yii2/web/Response.php:380

Stack trace:
#0 /Users/drifter/www/project/vendor/yiisoft/yii2/web/Response.php(380): ::header()
#1 /Users/drifter/www/project/vendor/yiisoft/yii2/web/Response.php(339): craft\web\Response->sendHeaders()
#2 /Users/drifter/www/project/vendor/yiisoft/yii2/base/Application.php(392): craft\web\Response->send()
#3 /Users/drifter/www/project/web/index.php(86): craft\web\Application->run()
#4 {main}

Proposed resolution: don't include the error message in the header text (it's already included in the content of the returned data). Or, remove the newlines.

It's in DefaultController.php at around line 170.

brandonkelly commented 3 years ago

The only header that Element API is setting is the Content-Type header, via the JsonResponseHeader here:

https://github.com/craftcms/element-api/blob/33da444d49950e5adc7aaa3b8e5b917993e6a40b/src/controllers/DefaultController.php#L101-L102

And I just tested locally and couldn’t reproduce. Exception messages are not leaking into the response headers for me. Not sure how that would be happening.

jlawrence-yellostudio commented 3 years ago

@brandonkelly I was just able to replicate this by adding an invalid 'orderBy' to the getElementQuery() in a custom ElementResource class e.g:

protected function getElementQuery(): ElementQueryInterface
{
        $query = parent::getElementQuery();
        $query->orderBy('boo');
        return $query;
}

Screenshot 2021-08-31 at 18 36 22-clean

brandonkelly commented 3 years ago

What version of PHP are you running? In the case of #138, this ended up being an issue with PHP 7.3.

drifteaur commented 3 years ago

@brandonkelly OK let me clarify, it's not a header like Content-Type, it's the status text sent along with the status code (500 etc).

The caught exception pushes the exception message into the status text; in DefaultController::actionIndex:

    } catch (\Throwable $e) {
        $data = [
            'error' => [
                'code' => $e instanceof HttpException ? $e->statusCode : $e->getCode(),
                'message' => $e->getMessage(),
            ]
        ];
        $statusCode = $e instanceof HttpException ? $e->statusCode : 500;
        $statusText = $e->getMessage();
    }

Later, the response is set:

$response->setStatusCode($statusCode, $statusText);

The Yii Response will set the status code text. If it contains newlines, it will trigger an error.

Proposed resolution: don't include the message in the statusText, it's there in the response body anyway. Or, sanitize it to remove newlines/limit length.

jlawrence-yellostudio commented 3 years ago

@brandonkelly using php7.4 can be replicated using the instructions i provided. Screenshot 2021-09-01 at 11 07 05

brandonkelly commented 3 years ago

@drifteaur @jlawrence-yellostudio Thanks for the clarification! Just released v2.8.1 with a fix for this.