Improve RestApiExceptionFilter (#12967)

RestApiExceptionFilter is used as an exception filter for the core
controller which is used for crud operations on our objects (equivalent
of our dynamic queries findManyPeople etc. on the graphql API).

Exceptions were leading a 400 / BadRequestException response status
which can be confusing to users.
By default we should actually throw a 500 if the error was not handled
priorily, but we have not implemented input validation for the REST api
so we fear to be flooded with errors that should not be 500 but 400 due
to user inputs. A solution should be brought [with this
ticket](https://github.com/twentyhq/core-team-issues/issues/1027) but it
has not been prioritized yet.
This commit is contained in:
Marie
2025-06-30 16:01:48 +02:00
committed by GitHub
parent d4fe8efd7f
commit 1b72d901a5
6 changed files with 69 additions and 16 deletions

View File

@ -1,4 +1,8 @@
import { BadRequestException, Injectable } from '@nestjs/common';
import {
BadRequestException,
Injectable,
InternalServerErrorException,
} from '@nestjs/common';
import { Request } from 'express';
import { isDefined } from 'twenty-shared/utils';
@ -72,7 +76,7 @@ export class RestApiCreateManyHandler extends RestApiBaseHandler {
});
if (records.length !== body.length) {
throw new Error(
throw new InternalServerErrorException(
`Error when creating records. ${body.length - records.length} records are missing after creation.`,
);
}

View File

@ -1,4 +1,8 @@
import { BadRequestException, Injectable } from '@nestjs/common';
import {
BadRequestException,
Injectable,
InternalServerErrorException,
} from '@nestjs/common';
import { Request } from 'express';
import { isDefined } from 'twenty-shared/utils';
@ -57,7 +61,7 @@ export class RestApiCreateOneHandler extends RestApiBaseHandler {
const record = records[0];
if (!isDefined(record)) {
throw new Error('Created record not found');
throw new InternalServerErrorException('Created record not found');
}
return this.formatResult({

View File

@ -1,4 +1,8 @@
import { BadRequestException, Injectable } from '@nestjs/common';
import {
BadRequestException,
Injectable,
InternalServerErrorException,
} from '@nestjs/common';
import { Request } from 'express';
import { isDefined } from 'twenty-shared/utils';
@ -54,7 +58,7 @@ export class RestApiUpdateOneHandler extends RestApiBaseHandler {
const record = records[0];
if (!isDefined(record)) {
throw new Error('Updated record not found');
throw new InternalServerErrorException('Updated record not found');
}
return this.formatResult({

View File

@ -1,9 +1,13 @@
import { ArgumentsHost, Catch, ExceptionFilter } from '@nestjs/common';
import {
ArgumentsHost,
Catch,
ExceptionFilter,
HttpException,
} from '@nestjs/common';
import { Response } from 'express';
import { HttpExceptionHandlerService } from 'src/engine/core-modules/exception-handler/http-exception-handler.service';
import { CustomException } from 'src/utils/custom-exception';
@Catch()
export class RestApiExceptionFilter implements ExceptionFilter {
@ -15,10 +19,13 @@ export class RestApiExceptionFilter implements ExceptionFilter {
const ctx = host.switchToHttp();
const response = ctx.getResponse<Response>();
const statusCode =
exception instanceof HttpException ? exception.getStatus() : 400; // should actually default to 500 but we dont have input validation yet and dont want to be flooded with errors from input https://github.com/twentyhq/core-team-issues/issues/1027
return this.httpExceptionHandlerService.handleError(
exception as CustomException,
exception as Error | HttpException,
response,
400,
statusCode,
);
}
}

View File

@ -1,4 +1,10 @@
import { BadRequestException, Inject, Injectable, Scope } from '@nestjs/common';
import {
BadRequestException,
HttpException,
Inject,
Injectable,
Scope,
} from '@nestjs/common';
import { REQUEST } from '@nestjs/core';
import { Response } from 'express';
@ -16,6 +22,34 @@ interface RequestAndParams {
params: any;
}
const getErrorNameFromStatusCode = (statusCode: number) => {
switch (statusCode) {
case 400:
return 'BadRequestException';
case 401:
return 'UnauthorizedException';
case 403:
return 'ForbiddenException';
case 404:
return 'NotFoundException';
case 405:
return 'MethodNotAllowedException';
case 409:
return 'ConflictException';
case 422:
return 'UnprocessableEntityException';
case 500:
return 'InternalServerErrorException';
default: {
if (statusCode >= 500) {
return 'InternalServerErrorException';
}
return 'BadRequestException';
}
}
};
@Injectable({ scope: Scope.REQUEST })
export class HttpExceptionHandlerService {
constructor(
@ -25,15 +59,13 @@ export class HttpExceptionHandlerService {
) {}
handleError = (
exception: Error,
// eslint-disable-next-line @typescript-eslint/no-explicit-any
exception: Error | HttpException,
// eslint-disable-next-line @typescript-eslint/no-explicit-any
response: Response<any, Record<string, any>>,
errorCode?: number,
user?: ExceptionHandlerUser,
workspace?: ExceptionHandlerWorkspace,
// eslint-disable-next-line @typescript-eslint/no-explicit-any
// eslint-disable-next-line @typescript-eslint/no-explicit-any
): Response<any, Record<string, any>> | undefined => {
const params = this.request?.params;
@ -62,7 +94,7 @@ export class HttpExceptionHandlerService {
return response.status(statusCode).send({
statusCode,
error: exception.name || 'BadRequestException',
error: exception.name ?? getErrorNameFromStatusCode(statusCode),
messages: [exception?.message],
});
};

View File

@ -86,7 +86,9 @@ export const shouldCaptureException = (
return true;
};
export const handleException = <T extends Error | CustomException>({
export const handleException = <
T extends Error | CustomException | HttpException,
>({
exception,
exceptionHandlerService,
user,