Skip to content

Inconsistent PHP exception procesing #81

Closed
@klimov-paul

Description

@klimov-paul

At the present state this library processes "Internal Server Error" cases in the incorrect way.

Security

First of all, it exposes the actual error message to the end user.
In case there is an exception thrown within JsonRpc method, like following:

class Foo implements JsonRpcMethodInterface
{
    public function apply(array $paramList = null)
    {
        throw new \RuntimeException('Some exception message');
    }
}

the result output JSON looks like following:

{"jsonrpc":"2.0","id":"test-641844ed220e9","error":{"code":-32603,"message":"Internal error","data":{"previous":"Some exception message"}}}

Actual exception message should never be exposed to the end API consumer, as it may contain vital information, and can be used for security exploit.

So the result JSON should look like following:

{"jsonrpc":"2.0","id":"test-641844ed220e9","error":{"code":-32603,"message":"Internal error"}}

Debug

There is a case, when exception information actually should be exposed to the end API consumer.
It should be so for project local development environment and for the unit testing.

The present implementation makes the mess for such case as well. It does not shows error stack trace, which make difficult to locate actual cause of the error.

Thus there should be a parameter, which can switch the exception rendering from "secure" (hide all) mode to "verbose" (show all) mode.

Note: In most modern applications such condition is controlled via APP_ENV environment variable.

For the debug mode enabled the result error JSON should look like following:

{
    "jsonrpc": "2.0",
    "id": "test-6418478c644f8",
    "error": {
        "code": -32603,
        "message": "Internal error",
        "data": {
            "code": 0,
            "message": "Some exception message",
            "trace": [
                {
                    "file": "/path/to/project/Foo.php",
                    "line": 64,
                    "function": "apply",
                    "class": "Foo",
                    "type": "->",
                    "args": [
                        {
                            "arg1": "foo",
                            "arg2": "bar",
                        }
                    ]
                },
                ...
            ]
        }
    }
}

Original exception tracking loss

ResponseCreator::createErrorResponse() is implemented in the way the actual exception refernce is lost entirely.
To be more precise it is done at JsonRpcInternalErrorException::__construct():

public function __construct(\Exception $previousException = null)
    {
        parent::__construct(
            self::CODE,
            'Internal error',
            $previousException ? [self::DATA_PREVIOUS_KEY => $previousException->getMessage()] : []
        );
    }

Previous exception should be passed as a last argument during instantiating of the new exception. For example:

try {
    // ...
} catch (\Exception $exception) {
    throw new MyLibraryException('My library has found an error', 123, $exception);
}

In such way the actual stack trace will be preserved and can be used for debugging.
And the original exception will be available via Throwable::getPrevious().

While in present implementation the actual strack trace is lost along with the actual exception.

Dev Notes

Most likely it should be fixed at JsonRpcResponseNormalizer::normalizeError().

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions