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("
Looks like something broke!
+
+
+//inject-stack//
+
+
+ Looks like something broke!
+