fix(server): stop the global catch-all filter from shadowing typed GraphQL exception filters (#23508)
## 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.
This commit is contained in:
@@ -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[] {
|
||||
|
||||
-17
@@ -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;
|
||||
}
|
||||
}
|
||||
+2
@@ -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 {
|
||||
|
||||
+2
@@ -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 {
|
||||
|
||||
@@ -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<YogaDriverConfig>({
|
||||
driver: YogaDriver,
|
||||
autoSchemaFile: true,
|
||||
}),
|
||||
FeatureModule,
|
||||
],
|
||||
providers: [{ provide: APP_FILTER, useClass: UnhandledExceptionFilter }],
|
||||
})
|
||||
class RootModuleWithAppFilter {}
|
||||
|
||||
@Module({
|
||||
imports: [
|
||||
GraphQLModule.forRoot<YogaDriverConfig>({
|
||||
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<GraphQLSchemaHostType>(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();
|
||||
});
|
||||
});
|
||||
@@ -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,
|
||||
|
||||
+10
-2
@@ -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,
|
||||
}
|
||||
`;
|
||||
|
||||
@@ -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)
|
||||
|
||||
Reference in New Issue
Block a user