NestJs - setupNestErrorHandler causes exceptions filters to not being called, therefore errors are not being handled

[CSenshi]

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.7.0

Framework Version

10.0.0

Link to Sentry event

No response

SDK Setup

import * as Sentry from '@sentry/node';
import { nodeProfilingIntegration } from '@sentry/profiling-node';

// Ensure to call this before importing any other modules!
Sentry.init({
  dsn: 'https://examplePublicKey@o0.ingest.sentry.io/0',
  integrations: [
    // Add our Profiling integration
    nodeProfilingIntegration(),
  ],

  // Add Performance Monitoring by setting tracesSampleRate
  // We recommend adjusting this value in production
  tracesSampleRate: 1.0,

  // Set sampling rate for profiling
  // This is relative to tracesSampleRate
  profilesSampleRate: 1.0,
});

Steps to Reproduce

1. Add 2 separate modules with each having their own exception filter
2. Integrate sentry as said in the docs (https://docs.sentry.io/platforms/javascript/guides/nestjs/)
3. Old exceptions are not being handled

Reproducable github code: https://github.com/CSenshi/nest-sentry-exception-filter-bug

Expected Result

After integration exceptions should be handled/filtered as they were before

Actual Result

Errors are not being handled and return
{"statusCode":500,"message":"Internal server error"}

[CSenshi]

If I change

Sentry.setupNestErrorHandler(app, new BaseExceptionFilter(httpAdapter));

to:

Sentry.setupNestErrorHandler(app, new DragonExceptionFilter(httpAdapter));

It will handle DragonModule exceptions and not HeroModule.

One possible solution: Not pass second argument (BaseExceptionFilter) and automatically iterate over all filters that are registered in nest and apply proxying of exception filter to all of them

[sambernard]

I'm seeing this as well- I have a custom Exception Filter that catches authentication errors on routes with Basic Auth, and responds with a WWW-Authenticate header. I started getting the following response instead of a basic auth prompt:

{"message":"Unauthorized","statusCode":401}

I've found a non-ideal workaround, which is to register the filter globally in my main.ts file, after running Sentry.setupNestErrorHandler. Unfortunately this means that we lose automatic dependency injection on these filters.

Sentry.setupNestErrorHandler(
    app,
    new GlobalExceptionFilter(app.getHttpAdapter()),
);

// Sentry's exception filter prevents any module-defined filters from running, so register here. 
// Must be _after_ Sentry.setupNestErrorHandler
app.useGlobalFilters(new BasicAuthErrorFilter());

[mydea]

So if you have a custom filter, you should wrap that filter instead of new BaseExceptionFilter(httpAdapter). This is probably not documented ideally, we'll try to clarify this:

1. If you had no custom error filter defined before, doing this should be fine:

Sentry.setupNestErrorHandler(app, new BaseExceptionFilter(httpAdapter));

2. If you had some other error handler defined before, wrap this instead, e.g.

Sentry.setupNestErrorHandler(app, new DragonExceptionFilter(httpAdapter));

I guess we do not really support multiple filters today, but we could provide a utility that allows to wrap any error filter, and/or a utility to add sentry support to your own error filter - would that make sense to you? E.g. (this is theoretical, just how this could work):

// Use this instead of setupNestErrorHandler()
Sentry.setupNestApp(app);

app.useGlobalFilters(wrapNestFilter(new BasicAuthErrorFilter());

[CSenshi]

It might make sense but in your solution there still is 1 problem and that is registration of filters using modules. Common practice is that you create exception filter per module and provide it in module's providers, something like this:

// hero.module.ts
@Module({
  providers: [
    { provide: APP_FILTER, useClass: HeroExceptionFilter }
  ],
})
export class HeroModule {}

// dragon.module.ts
@Module({
  providers: [
    { provide: APP_FILTER, useClass: DragonExceptionFilter }
  ],
})
export class DragonModule {}

Writing code like this does not require you to register exception filters in the bootstrap function.

Better solution for me would be having SentryModule which will be configurable and you would just need to import it to main module and it will automatically catch the exceptionFilters and use proxy with new instance or change exception filter's prototype.

Following code represents what I am saying (tested and it works well):

async function bootstrap() {
  const app = await NestFactory.create(AppModule); 

  Sentry.setupNestErrorHandler(app); // need to remove second argument and remove internal use of global prefixes

  // New Code Start
  const discoveryService = app.get(DiscoveryService);
  const providers = discoveryService.getProviders();
  const exceptionFilters = providers.filter(
    ({ metatype }) =>
      metatype && Reflect.getMetadata(CATCH_WATERMARK, metatype),
  );

  exceptionFilters.map((mod) => {
    const instance = mod.instance;
    const originalCatch = instance.catch;

    instance.catch = function (exception, host) {
      Sentry.captureException(exception);
      return originalCatch.apply(this, [exception, host]);
    };

    return mod;
  });
  // New Code End

  await app.listen(3000);
}

Here I am using DiscoveryService provided by nestjs (which is exported from DiscoveryModule), filtering out exceptionFilter providers and changing it's instance catch function. Maybe not the best practice but this would be nice to have.

Extension and thing to do would be to create SentryModule which will have one service with onModuleInit function which will do the new code snippet automatically so user would only import SentryModule into main.module

[mydea]

Thanks for elaborating!

I think we could look into providing such a thing. I'll put this in the backlog for now, as we are still following up on some other issues, but we could provide some opt-in mechanism there.

The challenge we have here is that we cannot (easily) depend on nestjs as proper dependency, so we need to pass in all things that come from nest. Also stuff like the watermark, for example 🤔 So could we provide an API like this:

async function bootstrap() {
  const app = await NestFactory.create(AppModule); 

  Sentry.setupNestErrorHandler(app); // need to remove second argument and remove internal use of global prefixes

  const discoveryService = app.get(DiscoveryService);
  const providers = discoveryService.getProviders();
  const exceptionFilters = providers.filter(
    ({ metatype }) =>
      metatype && Reflect.getMetadata(CATCH_WATERMARK, metatype),
  );

  Sentry.wrapNestExceptionFilters(exceptionFilters);

  await app.listen(3000);
}

would that make sense from your POV?