Do not expose sensitive information by jsamouh · Pull Request #978 · api-platform/core

jsamouh · 2017-03-10T20:53:58Z

Q	A
Bug fix?	yes
New feature?	no
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	#973 , #929
License	MIT
Doc PR	N/A

This PR fixes the display of Sensitive information (SQL request displayed, Database information) when the project is in production.
No BC according to me

dunglas · 2017-03-11T07:49:23Z

+     * @param bool                $debug             The kernel debug parameter
     */
-    public function __construct(SerializerInterface $serializer, array $errorFormats, array $exceptionToStatus = [])
+    public function __construct(SerializerInterface $serializer, array $errorFormats, array $exceptionToStatus, bool $debug)


The $exceptionToStatus default value should be kept and debug default to false to preserve backward compatibility.

dunglas · 2017-03-11T07:51:02Z

        $headers['X-Content-Type-Options'] = 'nosniff';
        $headers['X-Frame-Options'] = 'deny';

+        if ($exception->getStatusCode() == Response::HTTP_INTERNAL_SERVER_ERROR && !$this->debug) {


Please use === and Yoda style. I would also invert the order because !$this->debug is cheaper.

if (!$this->debug && Response::HTTP_INTERNAL_SERVER_ERROR === $exception->getStatusCode() ) {

Would it make sense to catch all 5xx here?

Good catch! Indeed we should handle all errors.

@jordscream what do you think?

@dunglas @dkarlovi , good point. I made a new commit to catch all server errors according the method isServerError of the Response Object.

dunglas · 2017-03-11T07:52:30Z

        $serializer->serialize($flattenException, 'jsonproblem')->willReturn();

-        $exceptionAction = new ExceptionAction($serializer->reveal(), ['jsonproblem' => ['application/problem+json'], 'jsonld' => ['application/ld+json']]);
+        $exceptionAction = new ExceptionAction($serializer->reveal(), ['jsonproblem' => ['application/problem+json'], 'jsonld' => ['application/ld+json']], [], true);


I would let it is it was previously (to test that there is no BC breaks and that the previous signature is still accepted).

dunglas · 2017-03-11T07:53:42Z

Thanks for this PR, it's greatly appreciated. I left some minor comment but 👍 to change this behavior when comments will be fixed.

jsamouh · 2017-03-11T12:20:30Z

@dunglas : reviews applied 👍 . You can check

soyuka

👍 thanks @jordscream

teohhanhui · 2017-03-13T09:55:53Z

Is this the correct way to fix it? I hope someone who's familiar with Symfony exception handling can comment...

jsamouh · 2017-03-13T10:04:18Z

@teohhanhui , check Symfony\Bundle\TwigBundle\Controller\ExceptionController.php
I tried to do the same approach... I guess :-)

teohhanhui · 2017-03-13T10:05:58Z

Ok, sounds good then.

jsamouh · 2017-03-14T20:04:02Z

@teohhanhui , as suggest @dunglas @dkarlovi , I modify the code to catch all 5XX exceptions.

jsamouh · 2017-03-15T16:37:04Z

@teohhanhui @soyuka , any news ?

soyuka · 2017-03-15T16:51:11Z

@jordscream looks fine to me, just waiting for more approval (ping @teohhanhui @meyerbaptiste ?). Anyway I'd be able to merge this but I won't publish a new release without @dunglas. Are you in a hurry to get this patch?

jsamouh · 2017-03-15T16:53:59Z

@soyuka we need it to push in production. We have a temporary solution without that. ;-)
Let me know when you want ^^

soyuka · 2017-03-15T17:26:52Z

@jordscream even if I merge this on 2.0 you'll be able to use the 2.0 branch? If that's the case you're probably using min-stability=dev, and you can set composer so that it uses your fork :).

jsamouh · 2017-03-15T17:29:46Z

@soyuka , I did not know about min-stability ! Let's do that
Do you want a target rebase on master ?

soyuka · 2017-03-15T17:31:30Z

No, I meant 2.0 sorry too many things at once. Leave this one like this it's fine! Still, if I merge it won't be released (new tag) and so composer update won't have the patch.

meyerbaptiste · 2017-03-15T17:35:27Z

+
+        $response->setContent($this->serializer->serialize($exception, $format['key']));
+
+        return $response;


return $response->setContent(...);?

@meyerbaptiste , Yes in fact, I need the Response object to use the method isServerError() in the Error condition just above.
It was a point I have to discuss with you. No other way to deal it

Yes but still you can return $response->setContent() (returns Response)

@meyerbaptiste good catch, I just pushed the modification

soyuka · 2017-03-16T11:02:33Z

Nice one! Merging this tomorrow if noone has comments until then :).

meyerbaptiste

LGTM

meyerbaptiste · 2017-03-16T16:11:39Z

Before merging this, what about #929 (comment)?

cc @dkarlovi

jsamouh · 2017-03-16T16:39:32Z

@meyerbaptiste it's effectively a security issue. Something to do in particular?

teohhanhui · 2017-03-16T17:02:55Z

+        $response = new Response(null, $exception->getStatusCode(), $headers);
+
+        if (!$this->debug && $response->isServerError()) {
+            $exception->setMessage(Response::$statusTexts[$response->getStatusCode()]);


We should not modify the exception.

Which means we need to look for a different way of doing this.

@teohhanhui , you mean create a new Exception object ?

@teohhanhui, or à simply response with message and 500 status code?

I'd say that we could create a new HTTPException if we shouldn't change the exception message. I didn't though about this but indeed it'd be better if (in production) you'd like to log the origin exception.

@meyerbaptiste @soyuka , Woooh, it seems better ^^, waiting for @teohhanhui approval before modification if needed

Yes, let's avoid modifying the FlattenException. We should treat it as immutable. The current implementation can be improved in that regard.

@meyerbaptiste @teohhanhui @soyuka , what about the comment of @meyerbaptiste "And avoid the FlattenException::setStatusCode() in the same time" (means move it in ErrorNormalizer) ?
The Response with status code is dealed by the ExceptionAction, not by ErrorNormalizer....

I feel confused, cause in the same time, We alter the exception status code in ExceptionAction and the exception message in ErrorNormalizer...

@jordscream Have a look at https://github.com/symfony/symfony/blob/4d48b58d197cfa76046bb23ce9c2fe7bec8fbbb2/src/Symfony/Bundle/TwigBundle/Controller/ExceptionController.php#L41-L73

I see no compelling reason to modify the exception in any way whatsoever.

For example,

core/src/Action/ExceptionAction.php

Lines 58 to 84 in 8053c55

/**

* Converts a an exception to a JSON response.

*

* @param FlattenException $exception

* @param Request $request

*

* @return Response

*/

public function __invoke(FlattenException $exception, Request $request): Response

{

$exceptionClass = $exception->getClass();

foreach ($this->exceptionToStatus as $class => $status) {

if (is_a($exceptionClass, $class, true)) {

$exception->setStatusCode($status);

break;

}

}

$headers = $exception->getHeaders();

$format = ErrorFormatGuesser::guessErrorFormat($request, $this->errorFormats);

$headers['Content-Type'] = sprintf('%s; charset=utf-8', $format['value'][0]);

$headers['X-Content-Type-Options'] = 'nosniff';

$headers['X-Frame-Options'] = 'deny';

return new Response($this->serializer->serialize($exception, $format['key']), $exception->getStatusCode(), $headers);

}

can be rewritten as:

/** * Converts a an exception to a JSON response. * * @param FlattenException $exception * @param Request $request * * @return Response */ public function __invoke(FlattenException $exception, Request $request): Response { $exceptionClass = $exception->getClass(); $statusCode = $exception->getStatusCode(); foreach ($this->exceptionToStatus as $class => $status) { if (is_a($exceptionClass, $class, true)) { $statusCode = $status; break; } } $headers = $exception->getHeaders(); $format = ErrorFormatGuesser::guessErrorFormat($request, $this->errorFormats); $headers['Content-Type'] = sprintf('%s; charset=utf-8', $format['value'][0]); $headers['X-Content-Type-Options'] = 'nosniff'; $headers['X-Frame-Options'] = 'deny'; return new Response($this->serializer->serialize($exception, $format['key']), $statusCode, $headers); }

with no change in behaviour and no BC. (You may include this change as part of this PR since it's minor.)

The Response with status code is dealed by the ExceptionAction, not by ErrorNormalizer....

The ErrorNormalizer is responsible for the body / content of the Response. (It basically replaces the content set when creating the Response.) How is it called? https://github.com/api-platform/core/blob/8053c55311b713153179f412ea294362a9997bea/src/EventListener/SerializeListener.php

So in our case, if we want to change the message, we should change it in the respective ErrorNormalizers.

👍 to do it directly in the normalizer. It's cleaner.

teohhanhui · 2017-03-16T17:06:04Z

+     * @param bool                $debug             The kernel debug parameter
     */
-    public function __construct(SerializerInterface $serializer, array $errorFormats, array $exceptionToStatus = [])
+    public function __construct(SerializerInterface $serializer, array $errorFormats, array $exceptionToStatus = [], bool $debug = false)


We should move $debug before $exceptionToStatus and remove its default value.

@teohhanhui , what about BC breaks if we do this ? I do this to prevent the bc break
thanks for your feeback

Because this is a security issue and needs to be fixed in 2.0, I think we should maintain the debug flag here.

There is no BC break because this class is final. 😄

@teohhanhui oHo ^^

@teohhanhui there is. People can (and do) instantiate this class directly.

Uhh, yes. 😞

jsamouh · 2017-03-20T13:03:17Z

@dunglas @teohhanhui , I made changes, waiting for your reviews
Also, the behat functionnal test fails on 2.0 branch... no relation with my code
failed causing by #950

teohhanhui · 2017-03-20T15:29:13Z

Also, the behat functionnal test fails on 2.0 branch... no relation with my code
failed causing by #950

That's not true. Please check Travis. There is 1 error in PHPUnit test. All Behat tests passed.

dunglas · 2017-03-20T15:52:55Z

        $message = $object->getMessage();
        if ($this->debug) {
            $trace = $object->getTrace();
+        } else {


I suggest:

if ($this->debug) { $trace = $object->getTrace(); } elseif (Response::HTTP_INTERNAL_SERVER_ERROR <= ($code = $object->getStatusCode())) { $message = Response::$statusTexts[$code]; }

We need to check $object instanceof FlattenException first. And the use of <= here is Yoda condition gone wrong (this "over-Yoda" is not part of Symfony CS).

We lose the mapping between exceptions and their HTTP status code (#797). What about passing the correct HTTP status code inside the serializer context?

If we're okay with modifying the status code in the FlattenException, like we currently do, then this is already working just fine.

But perhaps we should determine the status code here. That may be more proper.

jsamouh · 2017-03-21T10:54:22Z

@meyerbaptiste @dunglas done, let me know ^^. (I learn a lot of things with you, guys, do not hesitate to feeback me ^^)

meyerbaptiste · 2017-03-21T11:06:17Z

        if ($this->debug) {
            $trace = $object->getTrace();
+        } elseif ($object instanceof FlattenException) {
+            $statusCode = (isset($context['statusCode'])) ? $context['statusCode'] : $object->getStatusCode();


$statusCode = $context['statusCode'] ?? $object->getStatusCode();

@meyerbaptiste PHP 7 , not yet the reflex, fixed

meyerbaptiste · 2017-03-21T11:06:47Z

+        } elseif ($object instanceof FlattenException) {
+            $statusCode = (isset($context['statusCode'])) ? $context['statusCode'] : $object->getStatusCode();
+            if ($statusCode >= 500 && $statusCode < 600) {
+                $message = Response::$statusTexts[$context['statusCode']];


It should be $statusCode instead of $context['statusCode'].

@meyerbaptiste , I push too fast, it's fixed

teohhanhui · 2017-03-21T11:42:24Z

I believe we do not have to pass the HTTP status code in the serialization context, but instead moving the logic into the ErrorNormalizer itself. It'll be cleaner.

jsamouh · 2017-03-21T15:24:55Z

rebase branch with 2.0

soyuka · 2017-03-21T15:31:16Z

May you also squash the commits? Use git rebase -i HEAD~5 for interactive and change pick by fixup for every commit that's not the first one. Thanks!

jsamouh · 2017-03-21T15:40:25Z

@teohhanhui , for me is totally the opposite.

ErrorNormalizer->normalize is only here to normalize, not to modify... and only responsible to send a serialize data. (IMO)
ExceptionAction is here to catch and to call the normalize with the right data and headers (IMO)

So always IMO, I would let the satusCode and message treatment in ExceptionAction without alter FlattenException

Again, You have more experience. Tell me the right way to take. :-)

jsamouh · 2017-03-21T15:42:04Z

@soyuka done ^^

dunglas · 2017-03-21T15:45:26Z

I agree that the normalizer should not change the state of the underlying exception.

jsamouh · 2017-03-21T16:02:53Z

@teohhanhui @dunglas @meyerbaptiste , What about NativeNormalizer Symfony and CustomNormalizer used instead of ErrorNormalizer of API Platform?

Everybody has to develop this piece of code to make it work :-D

I think , we took the wrong way. Everything has to be dealed in ExceptionAction

teohhanhui · 2017-03-22T04:05:48Z

An exception being normalized into a different error message is not modifying anything in any way. 😄

jsamouh · 2017-03-22T08:26:53Z

@dunglas @teohhanhui @meyerbaptiste @soyuka

Need to continue this PR with your advice:

1/ Let the code like this (statusCode in Action and Message in ErrorNormalizer) => implies that anybody who uses his own normalizer or a Symfony normalizer cannot benefit of the error hidding in production.

2/ Move all the logic in Action. (statusCode + Message)

3/ Move all the logic in ErrorNormalizer (satusCode + Message): same remark as 1/

4/ Other choices ?

Your choice ? :-)

dunglas · 2017-03-22T08:37:54Z

I'm for merging the code as is. Can you also add a unit test checking that the message is hidden when the status code is 500 (and not hidden when it's another status)?

jsamouh · 2017-03-22T09:44:01Z

@dunglas , done tell me if you want more.
I used dataProvider and test differents status code associated to a fake original message.
If debug is true or status < 500, I should have the original message in any case, otherwise, I should have a compliant error message from the Response Foundation Class

dunglas · 2017-03-22T10:17:04Z

Thank you very much for fixing this @jordscream!

jsamouh · 2017-03-22T12:24:01Z

@dunglas , my pleasure ^^

dkarlovi · 2017-03-23T18:18:53Z

@jordscream great work following this through, thank you!

dkarlovi · 2017-03-30T14:19:58Z

Houston, we have a problem. :) /cc @dunglas @jordscream

This works as expected with Accept: application/ld+json, but not with Accept: application/json.

Accept: application/ld+json:
{
    "@context": "/api/contexts/Error",
    "@type": "hydra:Error",
    "hydra:title": "An error occurred",
    "hydra:description": "Internal Server Error"
}

Accept: application/json:
{
    "type": "https://tools.ietf.org/html/rfc2616#section-10",
    "title": "An error occurred",
    "detail": "An exception occurred while executing 'INSERT INTO images <TLDR> SQLSTATE[42S02]: Base table or view not found: 1146 Table 'images' doesn't exist"
}

dkarlovi · 2017-03-30T14:22:46Z

Also, if I trigger a connection-type error (I turn off the DB container), the same full error is thrown with either content type:

{
    "type": "https://tools.ietf.org/html/rfc2616#section-10",
    "title": "An error occurred",
    "detail": "An exception occured in driver: SQLSTATE[HY000] [2002] php_network_getaddresses: getaddrinfo failed: Try again"
}

meyerbaptiste · 2017-03-30T15:12:10Z

Yes, we forgot the ApiPlatform\Core\Problem\Serializer\ErrorNormalizer in this PR...

jsamouh · 2017-03-30T16:13:47Z

@dunglas @meyerbaptiste @dkarlovi oups!

…ive_information Do not expose sensitive information

jsamouh force-pushed the not_expose_sensitive_information branch from 1e7b0b4 to 2b32da7 Compare March 10, 2017 21:30

dunglas requested changes Mar 11, 2017

View reviewed changes

dunglas mentioned this pull request Mar 11, 2017

[Security, RFC] Streamline exception handling, prevent disclosure #929

Closed

jsamouh force-pushed the not_expose_sensitive_information branch from 2b32da7 to af28a6c Compare March 11, 2017 12:07

soyuka approved these changes Mar 11, 2017

View reviewed changes

jsamouh force-pushed the not_expose_sensitive_information branch from af28a6c to 7eba081 Compare March 14, 2017 18:19

meyerbaptiste reviewed Mar 15, 2017

View reviewed changes

jsamouh force-pushed the not_expose_sensitive_information branch from 7eba081 to a20a69f Compare March 16, 2017 09:58

meyerbaptiste approved these changes Mar 16, 2017

View reviewed changes

Simperfit approved these changes Mar 16, 2017

View reviewed changes

teohhanhui suggested changes Mar 16, 2017

View reviewed changes

jsamouh force-pushed the not_expose_sensitive_information branch from a20a69f to 598e49e Compare March 20, 2017 12:53

dunglas reviewed Mar 20, 2017

View reviewed changes

meyerbaptiste requested changes Mar 21, 2017

View reviewed changes

jsamouh force-pushed the not_expose_sensitive_information branch from ec59f31 to effb877 Compare March 21, 2017 15:23

jsamouh force-pushed the not_expose_sensitive_information branch from effb877 to ce97d9a Compare March 21, 2017 15:41

dunglas approved these changes Mar 22, 2017

View reviewed changes

soyuka added this to the 2.1.0 milestone Mar 22, 2017

meyerbaptiste approved these changes Mar 22, 2017

View reviewed changes

Catch All Server exception in production mode

0635c0f

jsamouh force-pushed the not_expose_sensitive_information branch from ce97d9a to 0635c0f Compare March 22, 2017 09:41

dunglas merged commit 1964001 into api-platform:2.0 Mar 22, 2017

soyuka mentioned this pull request Mar 30, 2017

Do not expose sensitive informations (json format) #1024

Merged

hoangnd25 pushed a commit to hoangnd25/core that referenced this pull request Feb 23, 2018

Merge pull request api-platform#978 from jordscream/not_expose_sensit…

5e4b206

…ive_information Do not expose sensitive information


		$response->setContent($this->serializer->serialize($exception, $format['key']));

		return $response;

	/**
	* Converts a an exception to a JSON response.
	*
	* @param FlattenException $exception
	* @param Request $request
	*
	* @return Response
	*/
	public function __invoke(FlattenException $exception, Request $request): Response
	{
	$exceptionClass = $exception->getClass();
	foreach ($this->exceptionToStatus as $class => $status) {
	if (is_a($exceptionClass, $class, true)) {
	$exception->setStatusCode($status);

	break;
	}
	}

	$headers = $exception->getHeaders();
	$format = ErrorFormatGuesser::guessErrorFormat($request, $this->errorFormats);
	$headers['Content-Type'] = sprintf('%s; charset=utf-8', $format['value'][0]);
	$headers['X-Content-Type-Options'] = 'nosniff';
	$headers['X-Frame-Options'] = 'deny';

	return new Response($this->serializer->serialize($exception, $format['key']), $exception->getStatusCode(), $headers);
	}

Uh oh!

Uh oh!

Conversation

jsamouh commented Mar 10, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dunglas Mar 11, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dunglas commented Mar 11, 2017

Uh oh!

jsamouh commented Mar 11, 2017

Uh oh!

soyuka left a comment

Choose a reason for hiding this comment

Uh oh!

teohhanhui commented Mar 13, 2017

Uh oh!

jsamouh commented Mar 13, 2017

Uh oh!

teohhanhui commented Mar 13, 2017

Uh oh!

jsamouh commented Mar 14, 2017

Uh oh!

jsamouh commented Mar 15, 2017

Uh oh!

soyuka commented Mar 15, 2017

Uh oh!

jsamouh commented Mar 15, 2017

Uh oh!

soyuka commented Mar 15, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jsamouh commented Mar 15, 2017

Uh oh!

soyuka commented Mar 15, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

meyerbaptiste Mar 15, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jsamouh Mar 15, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

soyuka commented Mar 16, 2017

Uh oh!

meyerbaptiste left a comment

Choose a reason for hiding this comment

Uh oh!

meyerbaptiste commented Mar 16, 2017

Uh oh!

jsamouh commented Mar 16, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

jsamouh commented Mar 10, 2017 •

edited

Loading

dunglas Mar 11, 2017 •

edited

Loading

soyuka commented Mar 15, 2017 •

edited

Loading

soyuka commented Mar 15, 2017 •

edited

Loading

meyerbaptiste Mar 15, 2017 •

edited

Loading

jsamouh Mar 15, 2017 •

edited

Loading

jsamouh commented Mar 16, 2017 •

edited

Loading

teohhanhui Mar 20, 2017 •

edited

Loading

teohhanhui Mar 20, 2017 •

edited

Loading

jsamouh commented Mar 20, 2017 •

edited

Loading

teohhanhui Mar 20, 2017 •

edited

Loading