Undocumented errorHandler behavior results in unreported 500s

[rclmenezes]

tl;dr: our users ran into a 500 that wasn't reported to Sentry! Sentry.Handlers.errorHandler() has some undocumented behavior that caused this.

Our users reported a 500 when they were hitting an API that involved uploading a file to S3. We were confused because Sentry didn't send us a notification. Turns out, the server's IAM role lacked the necessary permissions, so the aws-sdk S3 client threw an error like this:

{
  "message": "Access Denied",
  "code": "AccessDenied",
  "region": null,
  "time": "2019-06-28T14:18:17.152Z",
  "requestId": "3CA03D8E9C4E133E",
  "extendedRequestId": "gsZ5D8CZKcisHAua4GaMOyTNvpj/owuKH8XaTBu++6A17WmUNUjxSukM9HzQIEjG7j/qv/ql36Y=",
  "statusCode": 403,
  "retryable": false,
  "retryDelay": 87.60119265079014
}

After a lot of time spent debugging, we decided to read the Sentry.Handlers.errorHandler() source and we discovered:

export function errorHandler(): (
  error: MiddlewareError,
  req: http.IncomingMessage,
  res: http.ServerResponse,
  next: (error: MiddlewareError) => void,
) => void {
    const status = getStatusCodeFromResponse(error);
    if (status < 500) {
      next(error);
      return;
    }

    ...
}

function getStatusCodeFromResponse(error: MiddlewareError): number {
  const statusCode = error.status || error.statusCode || error.status_code || (error.output && error.output.statusCode);
  return statusCode ? parseInt(statusCode as string, 10) : 500;
}

This means that errorHandler is ignoring errors if they have a statusCode property set to < 500.

This isn't great for a variety of reasons:

• It's not documented behavior 😬
• @sentry/node shouldn't assume that error properties are explicitly made by the user. In this case, it was aws-sdk's decision to add statusCode as a descriptive attribute.
• While it's common for Express developers to make an error handler that handles statusCode elegantly (e.g. raise a 404 if the error has a statusCode of 404), many do not choose to do this because they prefer HTTP codes to be sent explicitly.

I would recommend either:

• Making this a configurable option in errorHandler(). Maybe add filterErrors as a parameter:

  Sentry.Handlers.errorHandler({
    filterErrors: (e: error) => e.statusCode && e.statusCode < 500
  })

• Removing this behavior altogether. People can avoid sending errors to Sentry by invoking errorHandler manually. For example:

   server.use((error: any, req: Request, res: Response, next: NextFunction) => {
     if (["status", "statusCode", "status_code", "output"].some(k => error[k])) {
         next(error);
     }
     Sentry.Handlers.errorHandler()(err, req, res, next);
   });

As a temporary patch, we're including code that does this:

if (USE_SENTRY) {
  server.use((error: any, req: Request, res: Response, next: NextFunction) => {
    ["status", "statusCode", "status_code", "output"].forEach(k => {
      if (error[k]) {
        delete error[k];
      }
    });
    next(error);
  });
  server.use(Sentry.Handlers.errorHandler());
}

[kamilogorek]

@rclmenezes I like the idea of filtering, PR is ready #2146 :)
Also updated relevant docs getsentry/sentry-docs#1093

[rclmenezes]

Thanks @kamilogorek !