Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Exceptions are too public #13

Open
chekalsky opened this issue Nov 13, 2020 · 4 comments
Open

Exceptions are too public #13

chekalsky opened this issue Nov 13, 2020 · 4 comments
Labels
enhancement New feature or request help wanted Extra attention is needed

Comments

@chekalsky
Copy link

I am concerned with random exception messages being revealed in the details field by default.

For example, today I've met Symfony's exception was thrown by internal logic of Translations engine with the text Unable to write to the "/var/task/var/cache/lambda/translations" directory. which reveals the internal structure of the project (and the fact project uses Symfony) which could be considered as a security breach.

My proposition is to not show exception's message in details field by default in production environment.

@veewee
Copy link
Contributor

veewee commented Nov 27, 2020

Yeah, that would make sense ....

The best option IMO is to put this logic inside the exception apiproblem:
https://github.com/phpro/api-problem/blob/master/src/Http/ExceptionApiProblem.php

An other option is to add an exception transformation that does this job.

Care to create a PR?

@veewee veewee added enhancement New feature or request help wanted Extra attention is needed labels Nov 27, 2020
@chekalsky
Copy link
Author

@veewee Prepared PR but I don't feel good about it, looks like a big change in core functionality people can rely on. Maybe better create default Transformer for Symfony where we will not reveal exception message?

@veewee
Copy link
Contributor

veewee commented Apr 30, 2021

Sorry for the delay @chekalsky and thanks for picking up the PR.

I feel the same way about the PR and am going to decline it in a moment.
After some thinking, it turns out that it is already possible to achieve what you want with the tools we currently provide.
Let me share you my vision:

You can create a new transformer and specify a good priority for it.
This transformer takes a list of exception classes it either makes visible or otherwise obfuscates.
Something along these lines:

<?php

use Phpro\ApiProblem\ApiProblemInterface;
use Phpro\ApiProblem\Http\ExceptionApiProblem;
use Phpro\ApiProblemBundle\Transformer\ExceptionTransformerInterface;

class PublicExceptionTransformer implements ExceptionTransformerInterface
{
    public function __constuct(
        private string ... $publicExceptionClasses
    ) {
    }

    public function transform(\Throwable $exception): ApiProblemInterface
    {
        $exceptionClass = get_class($exception);
        if (in_array($exceptionClass, $this->publicExceptionClasses, true)) {
            return new ExceptionApiProblem($exception);
        }

        // We could add a constructor variable to overwrite the message or add a withDetails() immutable method.
        return new ExceptionApiProblem($exception, $exceptionClass);
    }

    public function accepts(\Throwable $exception): bool
    {
        return true;
    }
}

(You could make the code even smarter and specify a PublicExceptionInterface inside your own codebase. That interface could then be used to determine wether the exception is public or not.)

We could accept a PR for a default implementation of this listener.

What do you think?

@mesilov
Copy link

mesilov commented Jun 10, 2022

i think this feature must depend with env - dev (more detailed) prod (hide trace and other sensitive information)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

3 participants