fix: 500 server error when files.create request is closed by client (#5878)

plugins/storage/server/api/files.ts

@@ -41,7 +41,7 @@ router.post(
       rejectOnEmpty: true,
     });
 
-    if (attachment?.userId !== actor.id) {
+    if (attachment.userId !== actor.id) {
       throw AuthorizationError("Invalid key");
     }
 

server/errors.ts

@@ -26,11 +26,9 @@ export function InvalidAuthenticationError(
   });
 }
 
-export function AuthorizationError(
-  message = "You do not have permission to access this resource"
-) {
+export function AuthorizationError(message = "Authorization error") {
   return httpErrors(403, message, {
-    id: "permission_required",
+    id: "authorization_error",
   });
 }
 
@@ -200,3 +198,11 @@ export function AuthenticationProviderDisabledError(
     id: "authentication_provider_disabled",
   });
 }
+
+export function ClientClosedRequestError(
+  message = "Client closed request before response was received"
+) {
+  return httpErrors(499, message, {
+    id: "client_closed_request",
+  });
+}

server/onerror.ts

@@ -1,20 +1,164 @@
 import fs from "fs";
 import http from "http";
 import path from "path";
-import Koa, { Context } from "koa";
+import formidable from "formidable";
+import Koa from "koa";
 import escape from "lodash/escape";
 import isNil from "lodash/isNil";
 import snakeCase from "lodash/snakeCase";
-import { ValidationError, EmptyResultError } from "sequelize";
+import {
+  ValidationError as SequelizeValidationError,
+  EmptyResultError as SequelizeEmptyResultError,
+} from "sequelize";
 import env from "@server/env";
-import { InternalError } from "@server/errors";
+import {
+  AuthorizationError,
+  ClientClosedRequestError,
+  InternalError,
+  NotFoundError,
+  ValidationError,
+} from "@server/errors";
 import { requestErrorHandler } from "@server/logging/sentry";
 
 const isDev = env.ENVIRONMENT === "development";
 const isProd = env.ENVIRONMENT === "production";
 let errorHtmlCache: Buffer | undefined;
 
-const readErrorFile = (): Buffer => {
+export default function onerror(app: Koa) {
+  // eslint-disable-next-line @typescript-eslint/no-explicit-any
+  app.context.onerror = function (err: any) {
+    // Don't do anything if there is no error, this allows you to pass `this.onerror` to node-style callbacks.
+    if (isNil(err)) {
+      return;
+    }
+
+    err = wrapInNativeError(err);
+
+    if (err instanceof SequelizeValidationError) {
+      if (err.errors && err.errors[0]) {
+        err = ValidationError(
+          `${err.errors[0].message} (${err.errors[0].path})`
+        );
+      } else {
+        err = ValidationError();
+      }
+    }
+
+    // Client aborted errors are a 500 by default, but 499 is more appropriate
+    if (err instanceof formidable.errors.FormidableError) {
+      if (err.internalCode === 1002) {
+        err = ClientClosedRequestError();
+      }
+    }
+
+    if (
+      err.code === "ENOENT" ||
+      err instanceof SequelizeEmptyResultError ||
+      /Not found/i.test(err.message)
+    ) {
+      err = NotFoundError();
+    }
+
+    if (
+      !(err instanceof AuthorizationError) &&
+      /Authorization error/i.test(err.message)
+    ) {
+      err = AuthorizationError();
+    }
+
+    if (typeof err.status !== "number" || !http.STATUS_CODES[err.status]) {
+      err = InternalError();
+    }
+
+    // Push only unknown 500 errors to sentry
+    if (err.status === 500) {
+      requestErrorHandler(err, this);
+
+      if (!(err instanceof InternalError)) {
+        err = InternalError();
+      }
+    }
+
+    const headerSent = this.headerSent || !this.writable;
+    if (headerSent) {
+      err.headerSent = true;
+    }
+
+    // Nothing we can do here other than delegate to the app-level handler and log.
+    if (headerSent) {
+      return;
+    }
+
+    this.set(err.headers);
+    this.status = err.status;
+    this.type = this.accepts("json", "html") || "json";
+
+    if (this.type === "text/html") {
+      this.body = readErrorFile()
+        .toString()
+        .replace(/\/\/inject-message\/\//g, escape(err.message))
+        .replace(/\/\/inject-status\/\//g, escape(err.status))
+        .replace(/\/\/inject-stack\/\//g, escape(err.stack));
+    } else {
+      this.body = JSON.stringify({
+        ok: false,
+        error: snakeCase(err.id),
+        status: err.status,
+        message: err.message || err.name,
+        data: err.errorData ?? undefined,
+      });
+    }
+
+    this.res.end(this.body);
+  };
+
+  return app;
+}
+
+// eslint-disable-next-line @typescript-eslint/no-explicit-any
+function wrapInNativeError(err: any): Error {
+  // When dealing with cross-globals a normal `instanceof` check doesn't work properly.
+  // See https://github.com/koajs/koa/issues/1466
+  // We can probably remove it once jest fixes https://github.com/facebook/jest/issues/2549.
+  const isNativeError =
+    Object.prototype.toString.call(err) === "[object Error]" ||
+    err instanceof Error;
+
+  if (isNativeError) {
+    return err as Error;
+  }
+
+  let errMsg = err;
+  if (typeof err === "object") {
+    try {
+      errMsg = JSON.stringify(err);
+      // eslint-disable-next-line no-empty
+    } catch (e) {}
+  }
+  const newError = InternalError(`Non-error thrown: ${errMsg}`);
+  // err maybe an object, try to copy the name, message and stack to the new error instance
+  if (err) {
+    if (err.name) {
+      newError.name = err.name;
+    }
+    if (err.message) {
+      newError.message = err.message;
+    }
+    if (err.stack) {
+      newError.stack = err.stack;
+    }
+    if (err.status) {
+      newError.status = err.status;
+    }
+    if (err.headers) {
+      newError.headers = err.headers;
+    }
+  }
+
+  return newError;
+}
+
+function readErrorFile(): Buffer {
   if (isDev) {
     return fs.readFileSync(path.join(__dirname, "error.dev.html"));
   }
@@ -34,168 +178,4 @@ const readErrorFile = (): Buffer => {
       path.join(__dirname, "static/error.dev.html")
     ))
   );
-};
-
-export default function onerror(app: Koa) {
-  app.context.onerror = function (err: any) {
-    // Don't do anything if there is no error, this allows you to pass `this.onerror` to node-style callbacks.
-    if (isNil(err)) {
-      return;
-    }
-
-    // When dealing with cross-globals a normal `instanceof` check doesn't work properly.
-    // See https://github.com/koajs/koa/issues/1466
-    // We can probably remove it once jest fixes https://github.com/facebook/jest/issues/2549.
-    const isNativeError =
-      Object.prototype.toString.call(err) === "[object Error]" ||
-      err instanceof Error;
-
-    // wrap non-error object
-    if (!isNativeError) {
-      let errMsg = err;
-      if (typeof err === "object") {
-        try {
-          errMsg = JSON.stringify(err);
-          // eslint-disable-next-line no-empty
-        } catch (e) {}
-      }
-      const newError = InternalError(`non-error thrown: ${errMsg}`);
-      // err maybe an object, try to copy the name, message and stack to the new error instance
-      if (err) {
-        if (err.name) {
-          newError.name = err.name;
-        }
-        if (err.message) {
-          newError.message = err.message;
-        }
-        if (err.stack) {
-          newError.stack = err.stack;
-        }
-        if (err.status) {
-          newError.status = err.status;
-        }
-        if (err.headers) {
-          newError.headers = err.headers;
-        }
-      }
-      err = newError;
-    }
-
-    if (err instanceof ValidationError) {
-      // @ts-expect-error status is not a property on ValidationError
-      err.status = 400;
-
-      if (err.errors && err.errors[0]) {
-        err.message = `${err.errors[0].message} (${err.errors[0].path})`;
-      }
-    }
-
-    if (
-      err.code === "ENOENT" ||
-      err instanceof EmptyResultError ||
-      /Not found/i.test(err.message)
-    ) {
-      err.status = 404;
-    }
-
-    if (/Authorization error/i.test(err.message)) {
-      err.status = 403;
-    }
-
-    if (typeof err.status !== "number" || !http.STATUS_CODES[err.status]) {
-      err.status = 500;
-    }
-
-    // Push only unknown 500 errors to sentry
-    if (err.status === 500) {
-      requestErrorHandler(err, this);
-    }
-
-    const headerSent = this.headerSent || !this.writable;
-    if (headerSent) {
-      err.headerSent = true;
-    }
-
-    // Nothing we can do here other than delegate to the app-level handler and log.
-    if (headerSent) {
-      return;
-    }
-
-    this.status = err.status;
-
-    this.set(err.headers);
-    const type = this.accepts("json", "html") || "json";
-    if (type === "html") {
-      html.call(this, err, this);
-    } else {
-      json.call(this, err, this);
-    }
-    this.type = type;
-
-    if (type === "json") {
-      this.body = JSON.stringify(this.body);
-    }
-    this.res.end(this.body);
-  };
-
-  return app;
-}
-
-/**
- * Handle errors for json requests.
- *
- * @param err The error being handled.
- * @param ctx The request context.
- */
-
-function json(err: any, ctx: Context) {
-  ctx.status = err.status;
-  let message;
-  let id;
-
-  if (ctx.status === 400) {
-    message = "Validation error";
-    id = "validation_error";
-  }
-  if (ctx.status === 401) {
-    message = "Authentication error";
-    id = "authentication_error";
-  }
-  if (ctx.status === 403) {
-    message = "Authorization error";
-    id = "authorization_error";
-  }
-  if (ctx.status === 404) {
-    message = "Resource not found";
-    id = "not_found";
-  }
-  if (ctx.status === 500) {
-    message = "Internal server error";
-    id = "internal_server_error";
-  }
-
-  ctx.body = {
-    ok: false,
-    error: snakeCase(err.id || id),
-    status: ctx.status,
-    message: err.message || message || err.name,
-    data: err.errorData ?? undefined,
-  };
-}
-
-/**
- * Handle errors for html requests.
- *
- * @param err The error being handled.
- * @param ctx The request context.
- */
-
-function html(err: any, ctx: Context) {
-  const page = readErrorFile();
-  ctx.body = page
-    .toString()
-    .replace(/\/\/inject-message\/\//g, escape(err.message))
-    .replace(/\/\/inject-status\/\//g, escape(err.status))
-    .replace(/\/\/inject-stack\/\//g, escape(err.stack));
-  ctx.type = "html";
 }

server/routes/errors.test.ts

@@ -9,7 +9,7 @@ describe("/test/error", () => {
     });
     const body = await res.json();
     expect(res.status).toBe(500);
-    expect(body.message).toBe("Internal server error");
+    expect(body.message).toBe("Internal error");
   });
 
   it("should return error response as html", async () => {
@@ -33,6 +33,6 @@ describe("/test/error", () => {
     });
     const body = await res.json();
     expect(res.status).toBe(500);
-    expect(body.message).toBe("Internal server error");
+    expect(body.message).toBe("Internal error");
   });
 });