defaultShouldHandleError filters out common library errors with no documentation

[Jaakkonen]

Is there an existing issue for this?

• I have checked for existing issues https://github.com/getsentry/sentry-javascript/issues
• I have reviewed the documentation https://docs.sentry.io/
• I am using the latest SDK release https://github.com/getsentry/sentry-javascript/releases

How do you use Sentry?

Sentry Saas (sentry.io)

Which SDK are you using?

@sentry/node

SDK Version

8.47.0

Framework Version

Express 5.0.1

Link to Sentry event

No response

Reproduction Example/SDK Setup

import express from 'express'
import {setupExpressErrorHandler} from '@sentry/node'
import { google } from 'googleapis'

const app = express()
app.get('/test', async (req, res) => {
  const userEmail = 'user-in-google-workspace-without-admin-access@mycompany.example.com'
  const auth = new google.auth.GoogleAuth({
    credentials: {
      private_key: 'xxx',
      client_email: 'xxx',
    },
    scopes: [
      'https://www.googleapis.com/auth/admin.directory.user.readonly',
    ],
    clientOptions: {
      subject: userEmail,
    },
  })
  const caller = await google.admin({ version: 'directory_v1', auth }).users.get({ userKey: userEmail })
  if (caller.data.isAdmin) {
    res.status(200).send({ status: 'user is admin' })
  } else {
    res.status(200).send({status: 'user is not admin'})
  }
})
setupExpressErrorHandler(app)
app.listen()

Steps to Reproduce

When calling the /test endpoint with a domain for which domain delegation permissions have been granted but with a user who doesn't have access to the admin directory endpoint, the upstream error thrown by googleapis (at directory.users.get() call) is a object containing .status < 500 value.

Error JSON

  {
    "config": { ... },
    "response": {
      "config": {...},
      "data": {
        "error": {
          "code": 403,
          "message": "Not Authorized to access this resource/api",
          "errors": [
            {
              "message": "Not Authorized to access this resource/api",
              "domain": "global",
              "reason": "forbidden"
            }
          ]
        }
      },
      "headers": {...},
      "status": 403,
      "statusText": "Forbidden",
      "request": {
        "responseURL": "https://admin.googleapis.com/admin/directory/v1/users/xxx"
      }
    },
    "status": 403,
    "code": 403,
    "errors": [
      {
        "message": "Not Authorized to access this resource/api",
        "domain": "global",
        "reason": "forbidden"
      }
    ]
  }

When this error enters the Sentry middleware if no shouldHandleError was provided in the setupExpressErrorHandler call, the defaultShouldHandleError gets invoked

sentry-javascript/packages/node/src/integrations/tracing/express.ts

Lines 115 to 133 in 1048a43

	return function sentryErrorMiddleware(
	error: MiddlewareError,
	request: http.IncomingMessage,
	res: http.ServerResponse,
	next: (error: MiddlewareError) => void,
	): void {
	// Ensure we use the express-enhanced request here, instead of the plain HTTP one
	// When an error happens, the `expressRequestHandler` middleware does not run, so we set it here too
	getIsolationScope().setSDKProcessingMetadata({ request });
	const shouldHandleError = options?.shouldHandleError || defaultShouldHandleError;
	if (shouldHandleError(error)) {
	const eventId = captureException(error, { mechanism: { type: 'middleware', handled: false } });
	(res as { sentry?: string }).sentry = eventId;
	}
	next(error);
	};

Now this checks whether the exception has any of status, statusCode, status_code or output.statusCode properties, and only handles the error if either any of those don't exist or the value there is equal or over 500.
This has a field status set which then

sentry-javascript/packages/node/src/integrations/tracing/express.ts

Lines 182 to 191 in 1048a43

	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;
	}
	/** Returns true if response code is internal server error */
	function defaultShouldHandleError(error: MiddlewareError): boolean {
	const status = getStatusCodeFromResponse(error);
	return status >= 500;
	}

Since the error thrown by googleapis contains status field, the error is not logged to Sentry.

On Express side the error propagates through next(err) calls until it reaches finalhandler (express source). In finalhandler if this last callback was called with an error, it gets the status code from .status or .statusCode properties, sets that to the response. If those fields don't exist it either uses the current status code in the response object or sets it to 500.

The Sentry logic of also looking into .status_code in the Error object and ignoring errors based on it is inconsistent with this.

Expected Result

Error to be captured.

Actual Result

Error not captured

Suggested fix

I do see the background of this default behavior in allowing application developers to throw all kinds of objects enabling application logic with middlewares responding with authentication errors/etc later on based on the status code. However a vast majority of errors thrown by 3rd party libraries are not well documented and thus should be able to be considered as opaque. With this default logic in place Sentry is quietly ignoring some of these errors often unexpectedly from the developers point of view.

I see the solution being

1. Default to capturing all errors. Recommend application developers to throw their own class MyAppError extends Error {} instances and have shouldHandleError: (err) => !(err instanceof MyAppError) when they have ES6 support (or some other property magic if they're not)
2. Building allowlist/blocklist logic to filter more specifically which errors won't be captured (this is already kinda in place with shouldHandleError). For a blocklist this issue would probably be repeating itself for emerging new libraries and the 3rd party library error detection logic would constantly need to be maintained
3. Documenting this behavior better and say that you may be randomly missing some errors if those happen to contain one of the fields with value under 500. Currently https://docs.sentry.io/platforms/javascript/guides/express/ doesn't even mention the shouldHandleError option or this behavior. Then the users could implement workaround like the one in 1. themselves.

My favorite would be option 1. but you do you <3

[chargome]

Hey @Jaakkonen thanks for the detailed explanation. While I understand the motivation to capture all errors, I would not go down this route by default. Capturing all errors below 500 would create significant noise in your error tracking, making it harder to identify and respond to actual system failures (for the avg user). So if you would want to capture these I would rather recommend you to trycatch these calls and report it using Sentry.captureException.

[mydea]

IMHO we should do 3., which is document this behavior a bit more explicitly. This probably applies to all node error handlers we have - it should be clear which errors are captured (or not) as well as explaining how to change that!

[mydea]

I created getsentry/sentry-docs#12326 to track this in docs - thank you for raising this! Closing this in favor of the docs issue :)