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?

[CSenshi]

For now to quickly fix this issue, this approach is acceptable for me :)

[mydea]

Ok, thanks for the feedback! We'll look into this in the near future 🙏

[nicohrubec]

Hey,

I looked into this today. The issue seems to be resolvable by using UseFilters annotation on the controllers directly instead of injecting the exception filters in the module providers. Does that solution work for you?

[SaizFerri]

Same issue here. UseFilters is a bit verbose if you have quite some filters.

[lforst]

I agree, having to do UseFilters everywhere is too much work. We'll find a solution to make this nice to use and functional. We will update you here on the process.

[nicohrubec]

Hey everyone,

We are about to merge a PR that allows to setup the Nest SDK by adding a root module: SentryModule.forRoot()

From the e2e tests we observed that this new setup also seems to resolve this issue. This will go out with the next release. Let us know if anything doesn't work as expected.

[AymaneAit]

Hey,

Just updated to the last version of the package and used the new approach with SentryModule however, in my case, the issue is still persisting.

I have two filters, one that catch BadRequestException, and a catch all. It works fine without registering SentryModule, but they are not called once I register it. My filters are registered using APP_FILTER in the main app module.

Let me know if you need more context.

[nicohrubec]

Hey, thanks for writing in. Would you mind sharing a small reproduction sample?

[AymaneAit]

Sure, here are the main files.

Main.ts

import './instrument';

import { NestFactory } from '@nestjs/core';
import {
  ExpressAdapter,
  NestExpressApplication,
} from '@nestjs/platform-express';
import Express from 'express';
import { onRequest } from 'firebase-functions/v2/https';
import { ReplaySubject, firstValueFrom } from 'rxjs';
import { AppModule } from './app.module';
import { InternalDisabledLogger } from './shared/utils/logger';

const serverSubject = new ReplaySubject<Express.Express>();

const bootstrap = async (): Promise<any> => {
  const app = await NestFactory.create<NestExpressApplication>(
    AppModule,
    new ExpressAdapter(),
    {
      rawBody: true,
      logger: new InternalDisabledLogger(),
      bodyParser: true,
    },
  );

  app.enableCors();
  await app.init();

  return app.getHttpAdapter().getInstance();
};

bootstrap().then((server) => serverSubject.next(server));

export const api = onRequest(
  {
    timeoutSeconds: 60 * 2,
    memory: '4GiB',
    cpu: 4,
    region: 'europe-west1',
    maxInstances: 10,
  },
  async (request, response) => {
    const server = await firstValueFrom(serverSubject);
    return server(request, response);
  },
);

Main app module

@Module({
  providers: [
    {
      provide: APP_FILTER,
      useClass: GenericExceptionFilter,
    },
    {
      provide: APP_FILTER,
      useClass: BadRequestExceptionFilter,
    },
    {
      provide: APP_INTERCEPTOR,
      useClass: GenericResponseInterceptor,
    },
  ],
  imports: [
    SentryModule.forRoot(),
    // ... my other modules
  ],
})
export class AppModule {}

Error filters

@Catch(BadRequestException)
export class BadRequestExceptionFilter implements ExceptionFilter {
  catch(exception: BadRequestException, host: ArgumentsHost): void {
    // custom logic, basically formatting the error response
  }
}

@Catch()
export class GenericExceptionFilter implements ExceptionFilter {
  catch(
    exception:
      | InternalServerErrorException
      | ConflictException
      | ForbiddenException
      | ServiceException
      | NotFoundException
      | UnauthorizedException,
    host: ArgumentsHost,
  ): void {
    // custom logic, basically formatting the error response
  }
}

[nicohrubec]

Thanks! I'll try to investigate this this week. Will keep you posted.

[nicohrubec]

Hey everyone,

We are about to merge a PR that should resolve this issue. Be careful, we had to change the SDK setup a bit to make this work properly. The root module remains, but now you have to either add the SentryGlobalFilter to your app providers or decorate your existing global filters with the WithSentry decorator (check the README for specific instructions).

The fix will go out with version 8.27. If anything does not work as expected, let us know.

[ysuhaas]

Hey @nicohrubec - we are using GraphQL/NestJS and it looks like the newest version of the Sentry SDK for NestJS doesn't work for this. Specifically - I think it is somewhat related to this change regressing from #12128, or this comment #12128 (comment)

It doesn't look like there is a way to specify a different base filter in the new version of the NestJS/Sentry SDK like in setupNestErrorHandler - which btw, should that be considered deprecated?

I'm wondering if SentryGlobalFilter needs to allow configuring a custom base filter here https://github.com/getsentry/sentry-javascript/blob/develop/packages/nestjs/src/setup.ts#L63 as you are able to do here

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

Line 55 in 615c670

export function setupNestErrorHandler(app: MinimalNestJsApp, baseFilter: NestJsErrorFilter): void {

With the current impl - we are consistently getting

{
      "query": "query {\n  triggerError\n}",
      "variables": {},
      "errors": [
        {
          "message": "response.status is not a function",
          "locations": [
            {
              "line": 2,
              "column": 3
            }
          ],
          "path": [
            "triggerError"
          ]
        }
      ]
    }

Let me know if you'd like me to open a new issue for this!

Edit - I confirmed monkey-patching the BaseExceptionFilter to ExternalExceptionFilter fixed the issue on GraphQL requests for us

diff --git a/build/cjs/setup.js b/build/cjs/setup.js
index 119735398bb49a01b1c941045a809edd45b32fdc..b06228c7d71288ef53f140de1890e9e4fefe7477 100644
--- a/build/cjs/setup.js
+++ b/build/cjs/setup.js
@@ -6,6 +6,7 @@ Object.defineProperty(exports, '__esModule', { value: true });
 
 const common = require('@nestjs/common');
 const core = require('@nestjs/core');
+const filter = require('@nestjs/core/exceptions/external-exception-filter');
 const microservices = require('@nestjs/microservices');
 const core$1 = require('@sentry/core');
 const utils = require('@sentry/utils');
@@ -47,7 +48,7 @@ common.Injectable()(SentryTracingInterceptor);
 /**
  * Global filter to handle exceptions and report them to Sentry.
  */
-class SentryGlobalFilter extends core.BaseExceptionFilter {
+class SentryGlobalFilter extends filter.ExternalExceptionFilter {
    static  __initStatic2() {this.__SENTRY_INTERNAL__ = true;}
 
   /**

[nicohrubec]

Hey @ysuhaas, thanks for writing in!

If you could open an issue for this with the information above and the SDK version you are using that would be ideal. I'll have a look next week!

[rohitkhatri]

I'm facing the same issue waiting for 8.27 release, any tentative time for this release?

[nicohrubec]

@rohitkhatri Hopefully today or tomorrow but definitely this week.