Clean-up error handler & fix message for unknown API errors

.changeset/flat-bottles-unite.md

@@ -0,0 +1,5 @@
+---
+'@directus/api': patch
+---
+
+Ensured the error message of unknown errors is returned correctly for admin users

api/src/app.ts

@@ -55,7 +55,7 @@ import authenticate from './middleware/authenticate.js';
 import cache from './middleware/cache.js';
 import { checkIP } from './middleware/check-ip.js';
 import cors from './middleware/cors.js';
-import errorHandler from './middleware/error-handler.js';
+import { errorHandler } from './middleware/error-handler.js';
 import extractToken from './middleware/extract-token.js';
 import getPermissions from './middleware/get-permissions.js';
 import rateLimiterGlobal from './middleware/rate-limiter-global.js';

api/src/middleware/error-handler.ts

@@ -1,105 +1,125 @@
-import { ErrorCode, MethodNotAllowedError, isDirectusError } from '@directus/errors';
-import { isObject, toArray } from '@directus/utils';
+import { ErrorCode, isDirectusError } from '@directus/errors';
+import { isObject } from '@directus/utils';
 import { getNodeEnv } from '@directus/utils/node';
-import type { ErrorRequestHandler } from 'express';
+import type { ErrorRequestHandler, NextFunction, Request, Response } from 'express';
 import getDatabase from '../database/index.js';
 import emitter from '../emitter.js';
 import { useLogger } from '../logger.js';
 
-// Note: keep all 4 parameters here. That's how Express recognizes it's the error handler, even if
-// we don't use next
-const errorHandler: ErrorRequestHandler = (err, req, res, _next) => {
-	const logger = useLogger();
-
-	let payload: any = {
-		errors: [],
+type ApiError = {
+	message: string;
+	extensions: {
+		code: string;
+		[key: string]: any;
 	};
+};
 
-	const errors = toArray<unknown>(err);
+const FALLBACK_ERROR = {
+	status: 500,
+	message: 'An unexpected error occurred.',
+	extensions: {
+		code: 'INTERNAL_SERVER_ERROR',
+	},
+};
 
+export const errorHandler = asyncErrorHandler(async (err, req, res) => {
+	const logger = useLogger();
+
+	let errors: ApiError[] = [];
 	let status: number | null = null;
 
-	for (const error of errors) {
+	// It can be assumed that at least one error is given
+	const receivedErrors: unknown[] = Array.isArray(err) ? err : [err];
+
+	for (const error of receivedErrors) {
 		if (getNodeEnv() === 'development') {
-			if (isObject(error)) {
-				error['extensions'] = {
-					...(error['extensions'] || {}),
-					stack: error['stack'],
-				};
+			// If available, expose stack trace under error's extensions data
+			if (isObject(error) && error['stack']) {
+				(error['extensions'] ??= {} as any).stack = error['stack'];
 			}
 		}
 
 		if (isDirectusError(error)) {
 			logger.debug(error);
 
-			if (!status) {
+			if (status === null) {
+				// Use current error status as response status
 				status = error.status;
 			} else if (status !== error.status) {
-				status = 500;
+				// Fallback if status has already been set by a preceding error
+				// and doesn't match the current one
+				status = FALLBACK_ERROR.status;
 			}
 
-			payload.errors.push({
+			errors.push({
 				message: error.message,
 				extensions: {
+					// Expose error code under error's extensions data
 					code: error.code,
 					...(error.extensions ?? {}),
 				},
 			});
 
 			if (isDirectusError(error, ErrorCode.MethodNotAllowed)) {
-				res.header('Allow', (error as InstanceType<typeof MethodNotAllowedError>).extensions.allowed.join(', '));
+				res.header('Allow', error.extensions.allowed.join(', '));
 			}
 		} else {
 			logger.error(error);
 
-			status = 500;
+			status = FALLBACK_ERROR.status;
 
 			if (req.accountability?.admin === true) {
 				const localError = isObject(error) ? error : {};
-				const message = localError['message'] ?? typeof error === 'string' ? error : null;
-
-				payload = {
-					errors: [
-						{
-							message: message || 'An unexpected error occurred.',
-							extensions: {
-								code: 'INTERNAL_SERVER_ERROR',
-								...(localError['extensions'] ?? {}),
-							},
+
+				// Use 'message' prop if available, otherwise if 'error' is a string use that
+				const message =
+					(typeof localError['message'] === 'string' ? localError['message'] : null) ??
+					(typeof error === 'string' ? error : null);
+
+				errors = [
+					{
+						message: message || FALLBACK_ERROR.message,
+						extensions: {
+							code: FALLBACK_ERROR.extensions.code,
+							...(localError['extensions'] ?? {}),
 						},
-					],
-				};
+					},
+				];
 			} else {
-				payload = {
-					errors: [
-						{
-							message: 'An unexpected error occurred.',
-							extensions: {
-								code: 'INTERNAL_SERVER_ERROR',
-							},
-						},
-					],
-				};
+				// Don't expose unknown errors to non-admin users
+				errors = [FALLBACK_ERROR];
 			}
 		}
 	}
 
-	res.status(status ?? 500);
-
-	emitter
-		.emitFilter(
-			'request.error',
-			payload.errors,
-			{},
-			{
-				database: getDatabase(),
-				schema: req.schema,
-				accountability: req.accountability ?? null,
-			},
-		)
-		.then((updatedErrors) => {
-			return res.json({ ...payload, errors: updatedErrors });
-		});
-};
+	res.status(status ?? FALLBACK_ERROR.status);
+
+	const updatedErrors = await emitter.emitFilter(
+		'request.error',
+		errors,
+		{},
+		{
+			database: getDatabase(),
+			schema: req.schema,
+			accountability: req.accountability ?? null,
+		},
+	);
 
-export default errorHandler;
+	return res.json({ errors: updatedErrors });
+});
+
+function asyncErrorHandler(fn: ErrorRequestHandler) {
+	return (err: any, req: Request, res: Response, next: NextFunction) =>
+		Promise.resolve(fn(err, req, res, next)).catch((error) => {
+			// To be on the safe side and ensure the response call is reached in any case
+			try {
+				const logger = useLogger();
+				logger.error(error, 'Unexpected error in error handler');
+			} catch {
+				// Ignore
+			}
+
+			res.status(FALLBACK_ERROR.status);
+			return res.json({ errors: [FALLBACK_ERROR] });
+		});
+}