Fixing uncaught exception when using limitError (#1600)

The `limitHandler` of `express-fileupload` called asynchronously for a
failed file before finishing the upload.
In the end `express-fileupload` calls `next()`.
All that leads to calling `next()` multiple times which leads to
uncaught exception due to the way the error handling in express is
organised.
The `next()` must be called once after completing the file processing.

CHANGELOG.md

@@ -2,6 +2,17 @@
 
 ## Version 17
 
+### v17.1.2
+
+- Fixed Uncaught Exception when using `limitError` feature.
+  - The exception was caused by excessive `next()` call from `express-fileupload` after handling the `limitError`.
+  - The issue did not affect the actual response since it had already been sent.
+  - In general, the problem arose due to asynchronous processing.
+  - The version introduces an upload failure handler instead of relying on the `limitHandler` of `express-fileupload`.
+  - Thus, handling the failed uploads is carried out after completing them.
+  - The specified `limitError` is only applicable to the `fileSize` limit, other limits do not trigger errors.
+  - The `limitError` feature introduced in v17.1.0.
+
 ### v17.1.1
 
 - Fixed wrong status code sending in case of upload failures when `limitError` is `HttpError`.

src/config-type.ts

@@ -88,7 +88,7 @@ type UploadOptions = Pick<
   | "limits"
 > & {
   /**
-   * @desc The error to throw when the file exceeds the configured limits (handled by errorHandler).
+   * @desc The error to throw when the file exceeds the configured fileSize limit (handled by errorHandler).
    * @see limits
    * @override limitHandler
    * @example createHttpError(413, "The file is too large")

src/server-helpers.ts

@@ -68,3 +68,15 @@ export const createNotFoundHandler =
       });
     }
   };
+
+export const createUploadFailueHandler =
+  (error: Error): RequestHandler =>
+  (req, {}, next) => {
+    const failedFile = Object.values(req?.files || [])
+      .flat()
+      .find(({ truncated }) => truncated);
+    if (failedFile) {
+      return next(error);
+    }
+    next();
+  };

src/server.ts

@@ -15,6 +15,7 @@ import { Routing, initRouting } from "./routing";
 import {
   createNotFoundHandler,
   createParserFailureHandler,
+  createUploadFailueHandler,
 } from "./server-helpers";
 
 const makeCommonEntities = async (config: CommonConfig) => {
@@ -58,9 +59,11 @@ export const createServer = async (config: ServerConfig, routing: Routing) => {
         ...derivedConfig,
         abortOnLimit: false,
         parseNested: true,
-        limitHandler: limitError && (({}, {}, next) => next(limitError)),
       }),
     );
+    if (limitError) {
+      app.use(createUploadFailueHandler(limitError));
+    }
   }
   if (config.server.rawParser) {
     app.use(config.server.rawParser);

tests/unit/server-helpers.spec.ts

@@ -1,6 +1,7 @@
 import {
   createNotFoundHandler,
   createParserFailureHandler,
+  createUploadFailueHandler,
 } from "../../src/server-helpers";
 import { describe, expect, test, vi } from "vitest";
 import winston from "winston";
@@ -137,4 +138,34 @@ describe("Server helpers", () => {
       );
     });
   });
+
+  describe("createUploadFailueHandler()", () => {
+    const error = new Error("Too heavy");
+
+    test.each([
+      {
+        files: { one: { truncated: true } },
+      },
+      {
+        files: { one: [{ truncated: false }, { truncated: true }] },
+      },
+    ])("should handle truncated files by calling next with error %#", (req) => {
+      const handler = createUploadFailueHandler(error);
+      const next = vi.fn();
+      handler(req as unknown as Request, {} as Response, next);
+      expect(next).toHaveBeenCalledWith(error);
+    });
+
+    test.each([
+      {},
+      { files: {} },
+      { files: { one: { truncated: false } } },
+      { file: { one: [{ truncated: false }] } },
+    ])("should call next when all uploads succeeded %#", (req) => {
+      const handler = createUploadFailueHandler(error);
+      const next = vi.fn();
+      handler(req as unknown as Request, {} as Response, next);
+      expect(next).toHaveBeenCalledWith();
+    });
+  });
 });

tests/unit/server.spec.ts

@@ -222,18 +222,13 @@ describe("Server", () => {
         },
       };
       await createServer(configMock, routingMock);
-      expect(appMock.use).toHaveBeenCalledTimes(4);
+      expect(appMock.use).toHaveBeenCalledTimes(5);
       expect(fileUploadMock).toHaveBeenCalledTimes(1);
       expect(fileUploadMock).toHaveBeenCalledWith({
         abortOnLimit: false,
         parseNested: true,
         limits: { fileSize: 1024 },
-        limitHandler: expect.any(Function),
       });
-      // limitHandler and limitError test
-      const nextMock = vi.fn();
-      fileUploadMock.mock.calls[0][0].limitHandler({}, {}, nextMock);
-      expect(nextMock).toHaveBeenCalledWith(new Error("Too heavy"));
     });
 
     test("should enable raw on request", async () => {