Oliverde8's blue Website

  • Home
  • Catching exceptions, how hard can it be

Catching exceptions, how hard can it be

Written by Oliverde8 on 06/01/2019

PHP Akeneo

I am surprised at how much people don't "catch" properly their exceptions. Just very recently I stumbled to a PR on an open source project that decided me to write this little article. 

Let's have a look to this PR of Akeneo : PIM-7915: Display a modal on deletion error

We can see here that there is a nice catch :

// ...
} catch (\Exception $exception) {
    return new JsonResponse(['message' => $exception->getMessage()], 500);
};

What bothers me with this catch? 

  • Well, the code here catches all exceptions and logs nothing. So it will catch "errors" that are expected but it will also catch errors such as a database error. 
  • It also sends the exception message to the user, so it could end up showing the database password or at the very least the database name, and user. 
  • Nothing is logged, so the user sees a message that he might send to the developers for them to fix, or he probably will simply ignore it; and even if the error message is communicated there is no stack trace. 

What should be done? 

  • Only certain exceptions should be caught, we would only catch exceptions we know we can handle
  • An exception message should never be sent to the user, except, if it's a particular exception that we know, contains a secure & user-friendly message. 
  • All other exceptions should lead to a generic error page, and errors should be logged. 

Of course this is not always possible, even some very good PHP libraries throw generic exceptions instead of defining their own. In this case, we are obliged to catch \Exception. In such cases : 

  • The exception must be logged
  • The exception message must never be sent to the user. 

This allows us to be sure no sensitive information can be leaked to the users and that developers will have the necessary information to debug the issue if needed.

So to fix the previous example we should write the following code

   $this->get('logger')->error("There was an error while deleting the user group : {$exception->getMessage()}", ['exception' => $exception])
   return new JsonResponse(['message' => 'There was an error while deleting the user group. Please contact the administrator'], 500);

So what's with the error message containing a "static" text as well as the exception message? 

Quite simple really, finding the proper logs can be sometimes more challenging than fixing the actual issue. Even with tools such as elasticsearch & kibana. The idea is that all logs should have a prefix, so from the user message we can find the corresponding log with ease in order to fix the issue.

Let's also note the `['exception' => $exception]`, This fallows the PSR-3 recommendation : 

If an Exception object is passed in the context data, it MUST be in the 'exception' key. Logging exceptions is a common pattern and this allows implementors to extract a stack trace from the exception when the log backend supports it. Implementors MUST still verify that the 'exception' key is actually an Exception before using it as such, as it MAY contain anything.

Now we have a code that : 

  • Can't leak sensitive data to the user
  • That is user-friendly
  • Logs all the details for easy troubleshooting. 

Share

Categories :