Skip to content

Commit

Permalink
Enhance server side error handling (outline#4537)
Browse files Browse the repository at this point in the history
* 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 <tom.moor@gmail.com>
  • Loading branch information
apoorv-mishra and tommoor committed Dec 9, 2022
1 parent 4f67437 commit 053d10d
Show file tree
Hide file tree
Showing 17 changed files with 319 additions and 78 deletions.
3 changes: 1 addition & 2 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down Expand Up @@ -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",
Expand Down
4 changes: 1 addition & 3 deletions server/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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";
Expand All @@ -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.
Expand Down Expand Up @@ -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"));
Expand Down
55 changes: 0 additions & 55 deletions server/middlewares/errorHandling.ts

This file was deleted.

194 changes: 194 additions & 0 deletions server/onerror.ts
Original file line number Diff line number Diff line change
@@ -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";
}
5 changes: 5 additions & 0 deletions server/routes/api/__snapshots__/collections.test.ts.snap
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ Object {
"error": "authorization_error",
"message": "Authorization error",
"ok": false,
"status": 403,
}
`;

Expand All @@ -13,6 +14,7 @@ Object {
"error": "authorization_error",
"message": "Authorization error",
"ok": false,
"status": 403,
}
`;

Expand All @@ -21,6 +23,7 @@ Object {
"error": "authorization_error",
"message": "Authorization error",
"ok": false,
"status": 403,
}
`;

Expand Down Expand Up @@ -119,6 +122,7 @@ Object {
"error": "authorization_error",
"message": "Authorization error",
"ok": false,
"status": 403,
}
`;

Expand All @@ -127,6 +131,7 @@ Object {
"error": "authorization_error",
"message": "Authorization error",
"ok": false,
"status": 403,
}
`;

Expand Down
3 changes: 3 additions & 0 deletions server/routes/api/__snapshots__/groups.test.ts.snap
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ Object {
"error": "authorization_error",
"message": "Authorization error",
"ok": false,
"status": 403,
}
`;

Expand Down Expand Up @@ -67,6 +68,7 @@ Object {
"error": "authorization_error",
"message": "Authorization error",
"ok": false,
"status": 403,
}
`;

Expand All @@ -84,5 +86,6 @@ Object {
"error": "",
"message": "The name of this group is already in use (isUniqueNameInTeam)",
"ok": false,
"status": 400,
}
`;
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ Object {
"error": "authorization_error",
"message": "Authorization error",
"ok": false,
"status": 403,
}
`;

Expand Down
2 changes: 0 additions & 2 deletions server/routes/api/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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";
Expand Down Expand Up @@ -41,7 +40,6 @@ const api = new Koa<
const router = new Router();

// middlewares
api.use(errorHandling());
api.use(
bodyParser({
multipart: true,
Expand Down
Loading

0 comments on commit 053d10d

Please sign in to comment.