From 4ff9cba76d2d3d126830514e7e297c5a82a9daf5 Mon Sep 17 00:00:00 2001 From: Thomas Trompette Date: Thu, 30 Jul 2026 09:13:00 +0200 Subject: [PATCH] fix(server): stop the global catch-all filter from shadowing typed GraphQL exception filters (#23508) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Context Sentry [TWENTY-SERVER-60Y](https://twenty-v7.sentry.io/issues/6633503406) ("Permission Denied: Entity performing the request does not have permission") is still firing at full rate on `v2.25.0`: ~10.8k events in the last 7 days, 24k total. #23104 tried to fix it by registering `PermissionsGraphqlApiExceptionFilter` globally via `APP_FILTER`. That registration is correct but **inert in production**, and the integration test added alongside it passes for a reason unrelated to prod behaviour. ## Root cause `main.ts` registered a catch-all filter after bootstrap: ```ts app.useGlobalFilters(new UnhandledExceptionFilter()); ``` Nest builds each resolver's filter list as `[...global, ...class, ...method]`, reverses it, and selects **exactly one** matching filter — there is no chaining. `APP_FILTER` providers are collected during module scan; `useGlobalFilters` appends after that, so the catch-all ended up at the head of the list: ``` 1. UnhandledExceptionFilter @Catch() <- matches everything, wins 2. PermissionsGraphqlApiExceptionFilter <- never reached 3. BillingGraphqlApiExceptionFilter <- never reached ``` On a GraphQL host `UnhandledExceptionFilter` then no-ops: `host.switchToHttp().getResponse()` returns the GraphQL args object, `response.header` is undefined, so it hits `return;`. Nest treats a falsy return as unhandled and rethrows the original `PermissionsException`, which reaches the Yoga error hook as a non-`BaseGraphQLError`, is serialized `INTERNAL_SERVER_ERROR`, and is reported by `shouldCaptureException`. The 28 resolvers carrying `@UseFilters(PermissionsGraphqlApiExceptionFilter)` were unaffected — method-level filters are evaluated before globals. Only the resolvers relying on the global registration leaked, which is exactly the set showing up in Sentry (`findOneApplication`, `uploadFilesFieldFileByUniversalIdentifier`, `UpdatePageLayoutWithTabsAndWidgets`, ...). Two other global filters were shadowed the same way and have never run: `BillingGraphqlApiExceptionFilter` and `FlatEntityMapsGraphqlApiExceptionFilter`. ## Why the existing test did not catch it `test/integration/utils/create-app.ts` builds the app from `AppModule` directly and never executes `main.ts`, so `useGlobalFilters` does not exist in the test process. It registered `MockedUnhandledExceptionFilter` as an `APP_FILTER` on the root testing module, which is collected *first* and therefore evaluated *last* — the exact inverse of production precedence. The `findOneApplication` denial test passed while the same query kept reporting to Sentry. ## Fix Register `UnhandledExceptionFilter` through `APP_FILTER` on `AppModule`. Root-module providers are scanned first, so it is collected first and evaluated last. The filter stays global, stays catch-all, and keeps its CORS-header role for HTTP; it simply no longer cuts in front of the typed filters. Un-shadowing the other two global filters means they now actually run, so `FileStorageExceptionFilter` and `FlatEntityMapsGraphqlApiExceptionFilter` get the `host.getType() !== 'graphql'` rethrow that `Billing` and `Permissions` already had. Without it they would start throwing GraphQL error objects into the REST pipeline. `MockedUnhandledExceptionFilter` is removed: `AppModule` now supplies the real filter in the same position, so the mock was dead weight. ## Test Verified against a real server (not the integration harness), calling the exact document from Sentry event `8d19eb7c` as a member with no permission flags: ``` query ($v1:UUID){findOneApplication(id:$v1){applicationVariables{key,value}}} ``` | | response code | exceptions captured | |---|---|---| | before | `INTERNAL_SERVER_ERROR` | 1 | | after | `FORBIDDEN` | 0 | Capture count measured through the console exception-handler driver, i.e. the same `captureExceptions` call site that is the Sentry driver in production. New unit spec `src/filters/__tests__/unhandled-exception.filter.spec.ts` boots a Nest + Yoga app both ways: it asserts `FORBIDDEN` with the `APP_FILTER` registration, and pins the shadowing behaviour of `app.useGlobalFilters` so the pattern cannot come back unnoticed. `granular-settings-permissions.integration-spec.ts` passes (10/10). Note it also passes *without* this fix — the harness cannot observe bootstrap-only configuration, which is the underlying reason #23104 shipped green. Closing that gap properly means sharing the post-`create` bootstrap between `main.ts` and `create-app.ts`; left as a follow-up. `file-storage-exception-filter.spec.ts` extended with a non-GraphQL host case. ## CI follow-up `failing-file-by-id-download.integration-spec.ts` snapshots were updated. That REST endpoint's 403 body changed in tests from `{}` to `{"statusCode":403,"error":"Forbidden","message":"Forbidden resource"}`. The old `{}` was an artifact of the mock: `MockedUnhandledExceptionFilter` rethrew, the exception escaped Nest's handler into Express's default error handler, and supertest saw an empty body. Production has always run the real `UnhandledExceptionFilter`, which writes `response.status(status).json(exception.response)` — the new snapshot. Production HTTP behaviour is unchanged by this PR: no other global filter matches an `HttpException` (the typed ones rethrow outside GraphQL), so the same filter handles it whether it is evaluated first or last. --- packages/twenty-server/src/app.module.ts | 8 + .../mocks/mock-unhandled-exception.filter.ts | 17 --- .../controllers/field-metadata.controller.ts | 2 + .../controllers/object-metadata.controller.ts | 2 + .../unhandled-exception.filter.spec.ts | 144 ++++++++++++++++++ packages/twenty-server/src/main.ts | 3 - ...le-by-id-download.integration-spec.ts.snap | 12 +- .../test/integration/utils/create-app.ts | 8 - 8 files changed, 166 insertions(+), 30 deletions(-) delete mode 100644 packages/twenty-server/src/engine/core-modules/exception-handler/mocks/mock-unhandled-exception.filter.ts create mode 100644 packages/twenty-server/src/filters/__tests__/unhandled-exception.filter.spec.ts diff --git a/packages/twenty-server/src/app.module.ts b/packages/twenty-server/src/app.module.ts index 4b61f6e9bc..ada929e25b 100644 --- a/packages/twenty-server/src/app.module.ts +++ b/packages/twenty-server/src/app.module.ts @@ -4,6 +4,7 @@ import { Module, RequestMethod, } from '@nestjs/common'; +import { APP_FILTER } from '@nestjs/core'; import { GraphQLModule } from '@nestjs/graphql'; import { ServeStaticModule } from '@nestjs/serve-static'; @@ -31,6 +32,7 @@ import { RestCoreMiddleware } from 'src/engine/middlewares/rest-core.middleware' import { GlobalWorkspaceDataSourceModule } from 'src/engine/twenty-orm/global-workspace-datasource/global-workspace-datasource.module'; import { TwentyORMModule } from 'src/engine/twenty-orm/twenty-orm.module'; import { WorkspaceCacheStorageModule } from 'src/engine/workspace-cache-storage/workspace-cache-storage.module'; +import { UnhandledExceptionFilter } from 'src/filters/unhandled-exception.filter'; import { ModulesModule } from 'src/modules/modules.module'; import { ClickHouseModule } from './database/clickHouse/clickHouse.module'; @@ -76,6 +78,12 @@ const MIGRATED_REST_METHODS = [ // Conditional modules ...AppModule.getConditionalModules(), ], + providers: [ + { + provide: APP_FILTER, + useClass: UnhandledExceptionFilter, + }, + ], }) export class AppModule { private static getConditionalModules(): DynamicModule[] { diff --git a/packages/twenty-server/src/engine/core-modules/exception-handler/mocks/mock-unhandled-exception.filter.ts b/packages/twenty-server/src/engine/core-modules/exception-handler/mocks/mock-unhandled-exception.filter.ts deleted file mode 100644 index e108157999..0000000000 --- a/packages/twenty-server/src/engine/core-modules/exception-handler/mocks/mock-unhandled-exception.filter.ts +++ /dev/null @@ -1,17 +0,0 @@ -import { - type ArgumentsHost, - Catch, - type ExceptionFilter, -} from '@nestjs/common'; -import { BaseExceptionFilter } from '@nestjs/core'; - -@Catch() -export class MockedUnhandledExceptionFilter - extends BaseExceptionFilter - implements ExceptionFilter -{ - // oxlint-disable-next-line typescript/no-explicit-any - catch(exception: any, _host: ArgumentsHost) { - throw exception; - } -} diff --git a/packages/twenty-server/src/engine/metadata-modules/field-metadata/controllers/field-metadata.controller.ts b/packages/twenty-server/src/engine/metadata-modules/field-metadata/controllers/field-metadata.controller.ts index 3981db3b13..62b29bfb34 100644 --- a/packages/twenty-server/src/engine/metadata-modules/field-metadata/controllers/field-metadata.controller.ts +++ b/packages/twenty-server/src/engine/metadata-modules/field-metadata/controllers/field-metadata.controller.ts @@ -53,6 +53,7 @@ import { toLegacyFieldMetadataListResponse, toLegacyFieldMetadataUpdateResponse, } from 'src/engine/metadata-modules/field-metadata/utils/to-legacy-field-metadata-response.util'; +import { FlatEntityMapsRestApiExceptionFilter } from 'src/engine/metadata-modules/flat-entity/filters/flat-entity-maps-rest-api-exception.filter'; import { WorkspaceManyOrAllFlatEntityMapsCacheService } from 'src/engine/metadata-modules/flat-entity/services/workspace-many-or-all-flat-entity-maps-cache.service'; import { fromFlatFieldMetadataToFieldMetadataDto } from 'src/engine/metadata-modules/flat-field-metadata/utils/from-flat-field-metadata-to-field-metadata-dto.util'; import { computeUniqueFieldMetadataIdsFromFlatIndexMaps } from 'src/engine/metadata-modules/index-metadata/utils/compute-unique-field-metadata-ids-from-flat-index-maps.util'; @@ -68,6 +69,7 @@ import { PermissionsRestApiExceptionFilter } from 'src/engine/metadata-modules/p PermissionsRestApiExceptionFilter, FieldMetadataRestApiExceptionFilter, ApplicationRestApiExceptionFilter, + FlatEntityMapsRestApiExceptionFilter, ) @UsePipes(new ValidationPipe()) export class FieldMetadataController { diff --git a/packages/twenty-server/src/engine/metadata-modules/object-metadata/controllers/object-metadata.controller.ts b/packages/twenty-server/src/engine/metadata-modules/object-metadata/controllers/object-metadata.controller.ts index c22ddd0db7..56c846fc5e 100644 --- a/packages/twenty-server/src/engine/metadata-modules/object-metadata/controllers/object-metadata.controller.ts +++ b/packages/twenty-server/src/engine/metadata-modules/object-metadata/controllers/object-metadata.controller.ts @@ -37,6 +37,7 @@ import { SettingsPermissionGuard } from 'src/engine/guards/settings-permission.g import { WorkspaceAuthGuard } from 'src/engine/guards/workspace-auth.guard'; import { FieldMetadataEntity } from 'src/engine/metadata-modules/field-metadata/field-metadata.entity'; import { fromFieldMetadataEntityToFieldMetadataDto } from 'src/engine/metadata-modules/field-metadata/utils/from-field-metadata-entity-to-field-metadata-dto.util'; +import { FlatEntityMapsRestApiExceptionFilter } from 'src/engine/metadata-modules/flat-entity/filters/flat-entity-maps-rest-api-exception.filter'; import { WorkspaceManyOrAllFlatEntityMapsCacheService } from 'src/engine/metadata-modules/flat-entity/services/workspace-many-or-all-flat-entity-maps-cache.service'; import { fromFlatObjectMetadataToObjectMetadataDto } from 'src/engine/metadata-modules/flat-object-metadata/utils/from-flat-object-metadata-to-object-metadata-dto.util'; import { computeUniqueFieldMetadataIdsFromFlatIndexMaps } from 'src/engine/metadata-modules/index-metadata/utils/compute-unique-field-metadata-ids-from-flat-index-maps.util'; @@ -70,6 +71,7 @@ import { PermissionsRestApiExceptionFilter } from 'src/engine/metadata-modules/p PermissionsRestApiExceptionFilter, ObjectMetadataRestApiExceptionFilter, ApplicationRestApiExceptionFilter, + FlatEntityMapsRestApiExceptionFilter, ) @UsePipes(new ValidationPipe()) export class ObjectMetadataController { diff --git a/packages/twenty-server/src/filters/__tests__/unhandled-exception.filter.spec.ts b/packages/twenty-server/src/filters/__tests__/unhandled-exception.filter.spec.ts new file mode 100644 index 0000000000..88e64f76e0 --- /dev/null +++ b/packages/twenty-server/src/filters/__tests__/unhandled-exception.filter.spec.ts @@ -0,0 +1,144 @@ +// oxlint-disable twenty/graphql-resolvers-should-be-guarded +import { + type CanActivate, + Injectable, + Module, + UseGuards, +} from '@nestjs/common'; +import { APP_FILTER } from '@nestjs/core'; +import { + GraphQLModule, + GraphQLSchemaHost, + type GraphQLSchemaHost as GraphQLSchemaHostType, + Mutation, + Query, + Resolver, +} from '@nestjs/graphql'; +import { Test } from '@nestjs/testing'; + +import { YogaDriver, type YogaDriverConfig } from '@graphql-yoga/nestjs'; +import { msg } from '@lingui/core/macro'; +import { type GraphQLSchema, graphql } from 'graphql'; + +import { ErrorCode } from 'src/engine/core-modules/graphql/utils/graphql-errors.util'; +import { + PermissionsException, + PermissionsExceptionCode, + PermissionsExceptionMessage, +} from 'src/engine/metadata-modules/permissions/permissions.exception'; +import { PermissionsGraphqlApiExceptionFilter } from 'src/engine/metadata-modules/permissions/utils/permissions-graphql-api-exception.filter'; +import { UnhandledExceptionFilter } from 'src/filters/unhandled-exception.filter'; + +@Injectable() +class DenyPermissionGuard implements CanActivate { + canActivate(): boolean { + throw new PermissionsException( + PermissionsExceptionMessage.PERMISSION_DENIED, + PermissionsExceptionCode.PERMISSION_DENIED, + { userFriendlyMessage: msg`denied` }, + ); + } +} + +@Resolver() +class TestResolver { + @Query(() => String) + ping(): string { + return 'pong'; + } + + @Mutation(() => String) + @UseGuards(DenyPermissionGuard) + guardedMutation(): string { + return 'ok'; + } +} + +// mirrors CoreEngineModule / MetadataEngineModule: a feature module that +// registers a typed GraphQL exception filter +@Module({ + providers: [ + TestResolver, + { provide: APP_FILTER, useClass: PermissionsGraphqlApiExceptionFilter }, + ], +}) +class FeatureModule {} + +@Module({ + imports: [ + GraphQLModule.forRoot({ + driver: YogaDriver, + autoSchemaFile: true, + }), + FeatureModule, + ], + providers: [{ provide: APP_FILTER, useClass: UnhandledExceptionFilter }], +}) +class RootModuleWithAppFilter {} + +@Module({ + imports: [ + GraphQLModule.forRoot({ + driver: YogaDriver, + autoSchemaFile: true, + }), + FeatureModule, + ], +}) +class RootModuleWithoutAppFilter {} + +const buildSchema = async ( + rootModule: unknown, + registerFilterAtBootstrap: boolean, +) => { + const moduleRef = await Test.createTestingModule({ + // oxlint-disable-next-line typescript/no-explicit-any + imports: [rootModule as any], + }).compile(); + + const app = moduleRef.createNestApplication(); + + if (registerFilterAtBootstrap) { + app.useGlobalFilters(new UnhandledExceptionFilter()); + } + + await app.init(); + + const schema = app.get(GraphQLSchemaHost).schema; + + return { app, schema }; +}; + +const runGuardedMutation = (schema: GraphQLSchema) => + graphql({ schema, source: 'mutation { guardedMutation }' }); + +describe('UnhandledExceptionFilter global registration', () => { + it('lets typed GraphQL filters convert the exception when registered through APP_FILTER on the root module', async () => { + const { app, schema } = await buildSchema(RootModuleWithAppFilter, false); + + const result = await runGuardedMutation(schema); + + expect(result.errors?.[0]?.extensions?.code).toBe(ErrorCode.FORBIDDEN); + expect(result.errors?.[0]?.message).toBe( + PermissionsExceptionMessage.PERMISSION_DENIED, + ); + + await app.close(); + }); + + // guards against reintroducing app.useGlobalFilters(new UnhandledExceptionFilter()): + // Nest checks global filters in reverse registration order and selects a single + // one, so a catch-all registered after bootstrap shadows every typed filter + it('shadows typed GraphQL filters when registered through app.useGlobalFilters', async () => { + const { app, schema } = await buildSchema(RootModuleWithoutAppFilter, true); + + const result = await runGuardedMutation(schema); + + expect(result.errors?.[0]?.extensions?.code).toBeUndefined(); + expect(result.errors?.[0]?.originalError).toBeInstanceOf( + PermissionsException, + ); + + await app.close(); + }); +}); diff --git a/packages/twenty-server/src/main.ts b/packages/twenty-server/src/main.ts index 75eba83ae9..0521f2bda6 100644 --- a/packages/twenty-server/src/main.ts +++ b/packages/twenty-server/src/main.ts @@ -18,7 +18,6 @@ import { getSessionStorageOptions } from 'src/engine/core-modules/session-storag import { TwentyConfigService } from 'src/engine/core-modules/twenty-config/twenty-config.service'; import { configTransformers } from 'src/engine/core-modules/twenty-config/utils/config-transformers.util'; import { shouldCaptureException } from 'src/engine/utils/global-exception-handler.util'; -import { UnhandledExceptionFilter } from 'src/filters/unhandled-exception.filter'; import { AppModule } from './app.module'; import './instrument'; @@ -76,8 +75,6 @@ const bootstrap = async () => { // Use our logger app.useLogger(logger); - app.useGlobalFilters(new UnhandledExceptionFilter()); - app.useBodyParser('json', { limit: settings.storage.maxFileSize }); app.useBodyParser('urlencoded', { limit: settings.storage.maxFileSize, diff --git a/packages/twenty-server/test/integration/metadata/suites/file/__snapshots__/failing-file-by-id-download.integration-spec.ts.snap b/packages/twenty-server/test/integration/metadata/suites/file/__snapshots__/failing-file-by-id-download.integration-spec.ts.snap index 59a670e5d6..0c40d7e0a9 100644 --- a/packages/twenty-server/test/integration/metadata/suites/file/__snapshots__/failing-file-by-id-download.integration-spec.ts.snap +++ b/packages/twenty-server/test/integration/metadata/suites/file/__snapshots__/failing-file-by-id-download.integration-spec.ts.snap @@ -2,14 +2,22 @@ exports[`File-by-id controller download should fail should respond 403 when the URL fileId does not match the token payload 1`] = ` { - "body": {}, + "body": { + "error": "Forbidden", + "message": "Forbidden resource", + "statusCode": 403, + }, "status": 403, } `; exports[`File-by-id controller download should fail should respond 403 when the request has no token query parameter 1`] = ` { - "body": {}, + "body": { + "error": "Forbidden", + "message": "Forbidden resource", + "statusCode": 403, + }, "status": 403, } `; diff --git a/packages/twenty-server/test/integration/utils/create-app.ts b/packages/twenty-server/test/integration/utils/create-app.ts index eabf12acc6..f53204ea25 100644 --- a/packages/twenty-server/test/integration/utils/create-app.ts +++ b/packages/twenty-server/test/integration/utils/create-app.ts @@ -1,4 +1,3 @@ -import { APP_FILTER } from '@nestjs/core'; import { type NestExpressApplication } from '@nestjs/platform-express'; import { Test, @@ -16,7 +15,6 @@ import { StripeSDKService } from 'src/engine/core-modules/billing/stripe/stripe- import { CaptchaDriverFactory } from 'src/engine/core-modules/captcha/captcha-driver.factory'; import { ExceptionHandlerService } from 'src/engine/core-modules/exception-handler/exception-handler.service'; import { ExceptionHandlerMockService } from 'src/engine/core-modules/exception-handler/mocks/exception-handler-mock.service'; -import { MockedUnhandledExceptionFilter } from 'src/engine/core-modules/exception-handler/mocks/mock-unhandled-exception.filter'; import { JobsModule } from 'src/engine/core-modules/message-queue/jobs.module'; import { MessageQueueModule } from 'src/engine/core-modules/message-queue/message-queue.module'; @@ -44,12 +42,6 @@ export const createApp = async ( const mockExceptionHandlerService = new ExceptionHandlerMockService(); let moduleBuilder: TestingModuleBuilder = Test.createTestingModule({ imports: [AppModule, JobsModule, MessageQueueModule.registerExplorer()], - providers: [ - { - provide: APP_FILTER, - useClass: MockedUnhandledExceptionFilter, - }, - ], }) .overrideProvider(StripeSDKService) .useValue(stripeSDKMockService)