From 136ee0ad1dc63bb7a913cd114693146334e9ce44 Mon Sep 17 00:00:00 2001 From: Tom Moor Date: Sun, 24 Sep 2023 16:30:52 -0400 Subject: [PATCH] fix: 500 server error when `files.create` request is closed by client (#5878) --- plugins/storage/server/api/files.ts | 2 +- server/errors.ts | 14 +- server/onerror.ts | 316 +++++++++++++--------------- server/routes/errors.test.ts | 4 +- 4 files changed, 161 insertions(+), 175 deletions(-) diff --git a/plugins/storage/server/api/files.ts b/plugins/storage/server/api/files.ts index eccdfed20..a5c9e2b84 100644 --- a/plugins/storage/server/api/files.ts +++ b/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"); } diff --git a/server/errors.ts b/server/errors.ts index 10d47b9ed..f8cc57381 100644 --- a/server/errors.ts +++ b/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", + }); +} diff --git a/server/onerror.ts b/server/onerror.ts index 97f0f2972..0a32e4cc0 100644 --- a/server/onerror.ts +++ b/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"; } diff --git a/server/routes/errors.test.ts b/server/routes/errors.test.ts index 947af8a1f..c8f7c2dfa 100644 --- a/server/routes/errors.test.ts +++ b/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"); }); });