From 053d10d893a40b995c7cc9b622acc9f37af8c150 Mon Sep 17 00:00:00 2001 From: Apoorv Mishra Date: Fri, 9 Dec 2022 21:51:42 +0530 Subject: [PATCH] Enhance server side error handling (#4537) * fix: server side error handling * fix: push only unknown 500 errors to sentry * fix: use in-house onerror in favor of errorHandling middleware * fix: split error template into dev and prod envs * fix: check Error instance * fix: error routes in test env * fix: review comments * Remove koa-onerror Co-authored-by: Tom Moor --- package.json | 3 +- server/index.ts | 4 +- server/middlewares/errorHandling.ts | 55 ----- server/onerror.ts | 194 ++++++++++++++++++ .../__snapshots__/collections.test.ts.snap | 5 + .../api/__snapshots__/groups.test.ts.snap | 3 + .../__snapshots__/documents.test.ts.snap | 1 + server/routes/api/index.ts | 2 - server/routes/auth/providers/email.ts | 2 - server/routes/errors.test.ts | 38 ++++ server/routes/errors.ts | 7 + server/routes/index.ts | 4 + server/static/error.dev.html | 34 +++ server/static/error.prod.html | 29 +++ server/test/support.ts | 2 + server/typings/koa-onerror.d.ts | 1 - yarn.lock | 13 -- 17 files changed, 319 insertions(+), 78 deletions(-) delete mode 100644 server/middlewares/errorHandling.ts create mode 100644 server/onerror.ts create mode 100644 server/routes/errors.test.ts create mode 100644 server/routes/errors.ts create mode 100644 server/static/error.dev.html create mode 100644 server/static/error.prod.html delete mode 100644 server/typings/koa-onerror.d.ts diff --git a/package.json b/package.json index 881be2410..0c4b0d878 100644 --- a/package.json +++ b/package.json @@ -7,7 +7,7 @@ "clean": "rimraf build", "copy:i18n": "mkdir -p ./build/shared/i18n && cp -R ./shared/i18n/locales ./build/shared/i18n", "build:i18n": "i18next --silent 'shared/**/*.tsx' 'shared/**/*.ts' 'app/**/*.tsx' 'app/**/*.ts' 'server/**/*.ts' 'server/**/*.tsx' && yarn copy:i18n", - "build:server": "babel --extensions .ts,.tsx --quiet -d ./build/server ./server && babel --quiet --extensions .ts,.tsx -d./build/shared ./shared && cp ./server/collaboration/Procfile ./build/server/collaboration/Procfile && cp package.json ./build && ln -sf \"$(pwd)/webpack.config.dev.js\" ./build", + "build:server": "babel --extensions .ts,.tsx --quiet -d ./build/server ./server && babel --quiet --extensions .ts,.tsx -d./build/shared ./shared && cp ./server/collaboration/Procfile ./build/server/collaboration/Procfile && cp package.json ./build && cp ./server/static/error.dev.html ./build/server/error.dev.html && cp ./server/static/error.prod.html ./build/server/error.prod.html && ln -sf \"$(pwd)/webpack.config.dev.js\" ./build", "build:webpack": "webpack --config webpack.config.prod.js", "build": "yarn build:webpack && yarn build:i18n && yarn build:server", "start": "node ./build/server/index.js", @@ -123,7 +123,6 @@ "koa-helmet": "^6.1.0", "koa-logger": "^3.2.1", "koa-mount": "^3.0.0", - "koa-onerror": "^4.2.0", "koa-router": "7.4.0", "koa-send": "5.0.1", "koa-sslify": "5.0.0", diff --git a/server/index.ts b/server/index.ts index 3347eb685..9f77277d5 100644 --- a/server/index.ts +++ b/server/index.ts @@ -8,14 +8,12 @@ import https from "https"; import Koa from "koa"; import helmet from "koa-helmet"; import logger from "koa-logger"; -import onerror from "koa-onerror"; import Router from "koa-router"; import { uniq } from "lodash"; import { AddressInfo } from "net"; import stoppable from "stoppable"; import throng from "throng"; import Logger from "./logging/Logger"; -import { requestErrorHandler } from "./logging/sentry"; import services from "./services"; import { getArg } from "./utils/args"; import { getSSLOptions } from "./utils/ssl"; @@ -25,6 +23,7 @@ import { checkPendingMigrations, } from "./utils/startup"; import { checkUpdates } from "./utils/updates"; +import onerror from "./onerror"; // The default is to run all services to make development and OSS installations // easier to deal with. Separate services are only needed at scale. @@ -84,7 +83,6 @@ async function start(id: number, disconnect: () => void) { // catch errors in one place, automatically set status and response headers onerror(app); - app.on("error", requestErrorHandler); // install health check endpoint for all services router.get("/_health", (ctx) => (ctx.body = "OK")); diff --git a/server/middlewares/errorHandling.ts b/server/middlewares/errorHandling.ts deleted file mode 100644 index 1b5faa3db..000000000 --- a/server/middlewares/errorHandling.ts +++ /dev/null @@ -1,55 +0,0 @@ -import { Context, Next } from "koa"; -import { snakeCase } from "lodash"; -import { ValidationError, EmptyResultError } from "sequelize"; - -export default function errorHandling() { - return async function errorHandlingMiddleware(ctx: Context, next: Next) { - try { - await next(); - } catch (err) { - ctx.status = err.status || 500; - let message = err.message || err.name; - let error; - - if (err instanceof ValidationError) { - // super basic form error handling - ctx.status = 400; - - if (err.errors && err.errors[0]) { - message = `${err.errors[0].message} (${err.errors[0].path})`; - } - } - - if (err instanceof EmptyResultError || message.match(/Not found/i)) { - message = "Resource not found"; - ctx.status = 404; - error = "not_found"; - } - - if (message.match(/Authorization error/i)) { - ctx.status = 403; - error = "authorization_error"; - } - - if (ctx.status === 500) { - message = "Internal Server Error"; - error = "internal_server_error"; - ctx.app.emit("error", err, ctx); - } - - ctx.body = { - ok: false, - error: snakeCase(err.id || error), - status: err.status, - message, - data: err.errorData, - }; - - // @ts-expect-error ts-migrate(2571) FIXME: Object is of type 'unknown'. - if (!ctx.body.data) { - // @ts-expect-error ts-migrate(2571) FIXME: Object is of type 'unknown'. - delete ctx.body.data; - } - } - }; -} diff --git a/server/onerror.ts b/server/onerror.ts new file mode 100644 index 000000000..5846123b3 --- /dev/null +++ b/server/onerror.ts @@ -0,0 +1,194 @@ +import fs from "fs"; +import http from "http"; +import path from "path"; +import Koa, { Context } from "koa"; +import { isNil, escape, snakeCase } from "lodash"; +import { ValidationError, EmptyResultError } from "sequelize"; +import env from "@server/env"; +import { InternalError } 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 => { + if (isDev) { + return ( + errorHtmlCache ?? + (errorHtmlCache = fs.readFileSync(path.join(__dirname, "error.dev.html"))) + ); + } + + if (isProd) { + return ( + errorHtmlCache ?? + (errorHtmlCache = fs.readFileSync( + path.join(__dirname, "error.prod.html") + )) + ); + } + + return ( + errorHtmlCache ?? + (errorHtmlCache = fs.readFileSync( + 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.code === "ENOENT") { + err.status = 404; + } + + 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 = err.message || err.name; + let error; + + if (err instanceof ValidationError) { + // super basic form error handling + ctx.status = 400; + + if (err.errors && err.errors[0]) { + message = `${err.errors[0].message} (${err.errors[0].path})`; + } + } + + if (err instanceof EmptyResultError || /Not found/i.test(message)) { + message = "Resource not found"; + ctx.status = 404; + error = "not_found"; + } + + if (/Authorization error/i.test(message)) { + ctx.status = 403; + error = "authorization_error"; + } + + if (ctx.status === 500) { + message = "Internal server error"; + error = "internal_server_error"; + } + + ctx.body = { + ok: false, + error: snakeCase(err.id || error), + status: ctx.status, + message, + data: err.errorData, + }; + + // @ts-expect-error ts-migrate(2571) FIXME: Object is of type 'unknown'. + if (!ctx.body.data) { + // @ts-expect-error ts-migrate(2571) FIXME: Object is of type 'unknown'. + delete ctx.body.data; + } +} + +/** + * 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-status\/\//g, escape(err.status)) + .replace(/\/\/inject-stack\/\//g, escape(err.stack)); + ctx.type = "html"; +} diff --git a/server/routes/api/__snapshots__/collections.test.ts.snap b/server/routes/api/__snapshots__/collections.test.ts.snap index dbf330937..9b3a446c9 100644 --- a/server/routes/api/__snapshots__/collections.test.ts.snap +++ b/server/routes/api/__snapshots__/collections.test.ts.snap @@ -5,6 +5,7 @@ Object { "error": "authorization_error", "message": "Authorization error", "ok": false, + "status": 403, } `; @@ -13,6 +14,7 @@ Object { "error": "authorization_error", "message": "Authorization error", "ok": false, + "status": 403, } `; @@ -21,6 +23,7 @@ Object { "error": "authorization_error", "message": "Authorization error", "ok": false, + "status": 403, } `; @@ -119,6 +122,7 @@ Object { "error": "authorization_error", "message": "Authorization error", "ok": false, + "status": 403, } `; @@ -127,6 +131,7 @@ Object { "error": "authorization_error", "message": "Authorization error", "ok": false, + "status": 403, } `; diff --git a/server/routes/api/__snapshots__/groups.test.ts.snap b/server/routes/api/__snapshots__/groups.test.ts.snap index 323a53f14..9530d3a94 100644 --- a/server/routes/api/__snapshots__/groups.test.ts.snap +++ b/server/routes/api/__snapshots__/groups.test.ts.snap @@ -14,6 +14,7 @@ Object { "error": "authorization_error", "message": "Authorization error", "ok": false, + "status": 403, } `; @@ -67,6 +68,7 @@ Object { "error": "authorization_error", "message": "Authorization error", "ok": false, + "status": 403, } `; @@ -84,5 +86,6 @@ Object { "error": "", "message": "The name of this group is already in use (isUniqueNameInTeam)", "ok": false, + "status": 400, } `; diff --git a/server/routes/api/documents/__snapshots__/documents.test.ts.snap b/server/routes/api/documents/__snapshots__/documents.test.ts.snap index eaa8fccbd..a045cdb84 100644 --- a/server/routes/api/documents/__snapshots__/documents.test.ts.snap +++ b/server/routes/api/documents/__snapshots__/documents.test.ts.snap @@ -5,6 +5,7 @@ Object { "error": "authorization_error", "message": "Authorization error", "ok": false, + "status": 403, } `; diff --git a/server/routes/api/index.ts b/server/routes/api/index.ts index be3425435..a34e98fa8 100644 --- a/server/routes/api/index.ts +++ b/server/routes/api/index.ts @@ -4,7 +4,6 @@ import Router from "koa-router"; import userAgent, { UserAgentContext } from "koa-useragent"; import env from "@server/env"; import { NotFoundError } from "@server/errors"; -import errorHandling from "@server/middlewares/errorHandling"; import { defaultRateLimiter } from "@server/middlewares/rateLimiter"; import { AuthenticatedState } from "@server/types"; import apiKeys from "./apiKeys"; @@ -41,7 +40,6 @@ const api = new Koa< const router = new Router(); // middlewares -api.use(errorHandling()); api.use( bodyParser({ multipart: true, diff --git a/server/routes/auth/providers/email.ts b/server/routes/auth/providers/email.ts index a2a96fe59..c432ede84 100644 --- a/server/routes/auth/providers/email.ts +++ b/server/routes/auth/providers/email.ts @@ -8,7 +8,6 @@ import SigninEmail from "@server/emails/templates/SigninEmail"; import WelcomeEmail from "@server/emails/templates/WelcomeEmail"; import env from "@server/env"; import { AuthorizationError } from "@server/errors"; -import errorHandling from "@server/middlewares/errorHandling"; import { rateLimiter } from "@server/middlewares/rateLimiter"; import { User, Team } from "@server/models"; import { signIn } from "@server/utils/authentication"; @@ -24,7 +23,6 @@ export const config = { router.post( "email", - errorHandling(), rateLimiter(RateLimiterStrategy.TenPerHour), async (ctx) => { const { email, client } = ctx.request.body; diff --git a/server/routes/errors.test.ts b/server/routes/errors.test.ts new file mode 100644 index 000000000..947af8a1f --- /dev/null +++ b/server/routes/errors.test.ts @@ -0,0 +1,38 @@ +import { getTestServer } from "@server/test/support"; + +const server = getTestServer(); + +describe("/test/error", () => { + it("should return error response as json", async () => { + const res = await server.post("/test/error", { + body: {}, + }); + const body = await res.json(); + expect(res.status).toBe(500); + expect(body.message).toBe("Internal server error"); + }); + + it("should return error response as html", async () => { + const res = await server.post("/test/error", { + headers: { + accept: "text/html", + }, + body: {}, + }); + const body = await res.text(); + expect(res.status).toBe(500); + expect(body).toContain("Error - 500"); + }); + + it("should fallback to json err response for types other than html", async () => { + const res = await server.post("/test/error", { + headers: { + accept: "text/plain", + }, + body: {}, + }); + const body = await res.json(); + expect(res.status).toBe(500); + expect(body.message).toBe("Internal server error"); + }); +}); diff --git a/server/routes/errors.ts b/server/routes/errors.ts new file mode 100644 index 000000000..406755ef4 --- /dev/null +++ b/server/routes/errors.ts @@ -0,0 +1,7 @@ +import Router from "koa-router"; + +const router = new Router(); + +router.post("/test/error", (ctx) => ctx.throw(Error())); + +export default router; diff --git a/server/routes/index.ts b/server/routes/index.ts index c21d6bf51..1d15df107 100644 --- a/server/routes/index.ts +++ b/server/routes/index.ts @@ -11,6 +11,7 @@ import { opensearchResponse } from "@server/utils/opensearch"; import { robotsResponse } from "@server/utils/robots"; import apexRedirect from "../middlewares/apexRedirect"; import { renderApp, renderShare } from "./app"; +import errors from "./errors"; const isProduction = env.ENVIRONMENT === "production"; const koa = new Koa(); @@ -133,6 +134,9 @@ koa.use(async (ctx, next) => { await next(); }); koa.use(apexRedirect()); +if (env.ENVIRONMENT === "test") { + koa.use(errors.routes()); +} koa.use(router.routes()); export default koa; diff --git a/server/static/error.dev.html b/server/static/error.dev.html new file mode 100644 index 000000000..917a102eb --- /dev/null +++ b/server/static/error.dev.html @@ -0,0 +1,34 @@ + + + + Error - //inject-status// + + + + + +
+

Error

+

Looks like something broke!

+ 
+        
+//inject-stack//
+        
+
+
+ + diff --git a/server/static/error.prod.html b/server/static/error.prod.html new file mode 100644 index 000000000..d47030178 --- /dev/null +++ b/server/static/error.prod.html @@ -0,0 +1,29 @@ + + + + Error - //inject-status// + + + + + +
+

Error

+

Looks like something broke!

+
+ + diff --git a/server/test/support.ts b/server/test/support.ts index d4fa10d31..17ad9c7b1 100644 --- a/server/test/support.ts +++ b/server/test/support.ts @@ -3,6 +3,7 @@ import { v4 as uuidv4 } from "uuid"; import { CollectionPermission } from "@shared/types"; import { sequelize } from "@server/database/sequelize"; import { User, Document, Collection, Team } from "@server/models"; +import onerror from "@server/onerror"; import webService from "@server/services/web"; export const seed = async () => { @@ -102,6 +103,7 @@ export const seed = async () => { export function getTestServer() { const app = webService(); + onerror(app); const server = new TestServer(app.callback()); server.disconnect = async () => { diff --git a/server/typings/koa-onerror.d.ts b/server/typings/koa-onerror.d.ts deleted file mode 100644 index 6439340ef..000000000 --- a/server/typings/koa-onerror.d.ts +++ /dev/null @@ -1 +0,0 @@ -declare module "koa-onerror"; diff --git a/yarn.lock b/yarn.lock index 3d6cb55f3..2768d71d0 100644 --- a/yarn.lock +++ b/yarn.lock @@ -10228,14 +10228,6 @@ koa-mount@^4.0.0: debug "^4.0.1" koa-compose "^4.1.0" -koa-onerror@^4.2.0: - version "4.2.0" - resolved "https://registry.yarnpkg.com/koa-onerror/-/koa-onerror-4.2.0.tgz#c617bb71dd036f27f6ade58e70480a50ff5fd4b1" - integrity sha512-D15tp5rxevHqqcvOiEDbtQolG6z3NpBNupz3EUZz43pjYv5SGMom2Xz1FKM8oTya56+aq+hejPW/iBrNnC/UGQ== - dependencies: - escape-html "^1.0.3" - stream-wormhole "^1.1.0" - koa-router@7.4.0: version "7.4.0" resolved "https://registry.yarnpkg.com/koa-router/-/koa-router-7.4.0.tgz#aee1f7adc02d5cb31d7d67465c9eacc825e8c5e0" @@ -14238,11 +14230,6 @@ stream-shift@^1.0.0: resolved "https://registry.yarnpkg.com/stream-shift/-/stream-shift-1.0.1.tgz#d7088281559ab2778424279b0877da3c392d5a3d" integrity sha512-AiisoFqQ0vbGcZgQPY1cdP2I76glaVA/RauYR4G4thNFgkTqr90yXTo4LYX60Jl+sIlPNHHdGSwo01AvbKUSVQ== -stream-wormhole@^1.1.0: - version "1.1.0" - resolved "https://registry.yarnpkg.com/stream-wormhole/-/stream-wormhole-1.1.0.tgz#300aff46ced553cfec642a05251885417693c33d" - integrity sha512-gHFfL3px0Kctd6Po0M8TzEvt3De/xu6cnRrjlfYNhwbhLPLwigI2t1nc6jrzNuaYg5C4YF78PPFuQPzRiqn9ew== - strict-uri-encode@^2.0.0: version "2.0.0" resolved "https://registry.yarnpkg.com/strict-uri-encode/-/strict-uri-encode-2.0.0.tgz#b9c7330c7042862f6b142dc274bbcc5866ce3546"