Thomas Trompette
4ff9cba76d
## 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.
92 lines
2.8 KiB
TypeScript
92 lines
2.8 KiB
TypeScript
import { type NestExpressApplication } from '@nestjs/platform-express';
|
|
import {
|
|
Test,
|
|
type TestingModule,
|
|
type TestingModuleBuilder,
|
|
} from '@nestjs/testing';
|
|
|
|
import bytes from 'bytes';
|
|
import graphqlUploadExpress from 'graphql-upload/graphqlUploadExpress.mjs';
|
|
|
|
import { AppModule } from 'src/app.module';
|
|
import { settings } from 'src/engine/constants/settings';
|
|
import { StripeSDKMockService } from 'src/engine/core-modules/billing/stripe/stripe-sdk/mocks/stripe-sdk-mock.service';
|
|
import { StripeSDKService } from 'src/engine/core-modules/billing/stripe/stripe-sdk/services/stripe-sdk.service';
|
|
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 { JobsModule } from 'src/engine/core-modules/message-queue/jobs.module';
|
|
import { MessageQueueModule } from 'src/engine/core-modules/message-queue/message-queue.module';
|
|
|
|
interface TestingModuleCreatePreHook {
|
|
(moduleBuilder: TestingModuleBuilder): TestingModuleBuilder;
|
|
}
|
|
|
|
/**
|
|
* Hook for adding items to nest application
|
|
*/
|
|
export type TestingAppCreatePreHook = (
|
|
app: NestExpressApplication,
|
|
) => Promise<void>;
|
|
|
|
/**
|
|
* Sets basic integration testing module of app
|
|
*/
|
|
export const createApp = async (
|
|
config: {
|
|
moduleBuilderHook?: TestingModuleCreatePreHook;
|
|
appInitHook?: TestingAppCreatePreHook;
|
|
} = {},
|
|
): Promise<NestExpressApplication> => {
|
|
const stripeSDKMockService = new StripeSDKMockService();
|
|
const mockExceptionHandlerService = new ExceptionHandlerMockService();
|
|
let moduleBuilder: TestingModuleBuilder = Test.createTestingModule({
|
|
imports: [AppModule, JobsModule, MessageQueueModule.registerExplorer()],
|
|
})
|
|
.overrideProvider(StripeSDKService)
|
|
.useValue(stripeSDKMockService)
|
|
.overrideProvider(ExceptionHandlerService)
|
|
.useValue(mockExceptionHandlerService)
|
|
.overrideProvider(CaptchaDriverFactory)
|
|
.useValue({
|
|
getCurrentDriver: () => ({
|
|
validate: async () => ({ success: true }),
|
|
}),
|
|
});
|
|
|
|
if (config.moduleBuilderHook) {
|
|
moduleBuilder = config.moduleBuilderHook(moduleBuilder);
|
|
}
|
|
|
|
const moduleFixture: TestingModule = await moduleBuilder.compile();
|
|
|
|
const app = moduleFixture.createNestApplication<NestExpressApplication>({
|
|
rawBody: true,
|
|
cors: true,
|
|
});
|
|
|
|
app.use(
|
|
'/graphql',
|
|
graphqlUploadExpress({
|
|
maxFieldSize: bytes(settings.storage.maxFileSize)!,
|
|
maxFiles: 10,
|
|
}),
|
|
);
|
|
|
|
app.use(
|
|
'/metadata',
|
|
graphqlUploadExpress({
|
|
maxFieldSize: bytes(settings.storage.maxFileSize)!,
|
|
maxFiles: 10,
|
|
}),
|
|
);
|
|
|
|
if (config.appInitHook) {
|
|
await config.appInitHook(app);
|
|
}
|
|
|
|
await app.init();
|
|
|
|
return app;
|
|
};
|