From 856014347bd226cbc568546df294cfe186f6ae85 Mon Sep 17 00:00:00 2001 From: Walter Moar Date: Wed, 5 Jun 2024 10:36:57 -0700 Subject: [PATCH 1/5] fix: FORMS-1139 handle express errors (#1380) --- app/src/forms/common/middleware/dataErrors.js | 11 ++++- .../common/middleware/dataErrors.spec.js | 43 +++++++++++++------ 2 files changed, 41 insertions(+), 13 deletions(-) diff --git a/app/src/forms/common/middleware/dataErrors.js b/app/src/forms/common/middleware/dataErrors.js index 593906ec6..b313d90fa 100644 --- a/app/src/forms/common/middleware/dataErrors.js +++ b/app/src/forms/common/middleware/dataErrors.js @@ -2,7 +2,8 @@ const Problem = require('api-problem'); const Objection = require('objection'); module.exports.dataErrors = async (err, _req, res, next) => { - let error = err; + let error; + if (err instanceof Objection.DataError) { error = new Problem(422, { detail: 'Sorry... the database does not like the data you provided :(', @@ -20,6 +21,14 @@ module.exports.dataErrors = async (err, _req, res, next) => { detail: 'Validation Error', errors: err.data, }); + } else if (!(err instanceof Problem) && (err.status || err.statusCode)) { + // Express throws Errors that are not Problems, but do have an HTTP status + // code. For example, 400 is thrown when POST bodies are malformed JSON. + error = new Problem(err.status || err.statusCode, { + detail: err.message, + }); + } else { + error = err; } if (error instanceof Problem && error.status !== 500) { diff --git a/app/tests/unit/forms/common/middleware/dataErrors.spec.js b/app/tests/unit/forms/common/middleware/dataErrors.spec.js index eb4b479ee..fb6369516 100644 --- a/app/tests/unit/forms/common/middleware/dataErrors.spec.js +++ b/app/tests/unit/forms/common/middleware/dataErrors.spec.js @@ -9,8 +9,7 @@ describe('test data errors middleware', () => { const error = new Objection.DataError({ nativeError: { message: 'This is a DataError' }, }); - const { res } = getMockRes(); - const next = jest.fn(); + const { res, next } = getMockRes(); middleware.dataErrors(error, {}, res, next); @@ -22,8 +21,7 @@ describe('test data errors middleware', () => { const error = new Objection.NotFoundError({ nativeError: { message: 'This is a NotFoundError' }, }); - const { res } = getMockRes(); - const next = jest.fn(); + const { res, next } = getMockRes(); middleware.dataErrors(error, {}, res, next); @@ -35,8 +33,7 @@ describe('test data errors middleware', () => { const error = new Objection.UniqueViolationError({ nativeError: { message: 'This is a UniqueViolationError' }, }); - const { res } = getMockRes(); - const next = jest.fn(); + const { res, next } = getMockRes(); middleware.dataErrors(error, {}, res, next); @@ -48,8 +45,7 @@ describe('test data errors middleware', () => { const error = new Objection.ValidationError({ nativeError: { message: 'This is a ValidationError' }, }); - const { res } = getMockRes(); - const next = jest.fn(); + const { res, next } = getMockRes(); middleware.dataErrors(error, {}, res, next); @@ -57,10 +53,33 @@ describe('test data errors middleware', () => { expect(res.end).toBeCalledWith(expect.stringContaining('422')); }); + it('should handle non-problem errors with a status', () => { + const error = new Error('This is a 400 status.'); + error.status = 400; + const { res, next } = getMockRes(); + + middleware.dataErrors(error, {}, res, next); + + expect(next).not.toBeCalled(); + expect(res.end).toBeCalledWith(expect.stringContaining('400')); + expect(res.end).toBeCalledWith(expect.stringContaining('This is a 400 status.')); + }); + + it('should handle non-problem errors with a status code', () => { + const error = new Error('This is a 400 status code.'); + error.statusCode = 400; + const { res, next } = getMockRes(); + + middleware.dataErrors(error, {}, res, next); + + expect(next).not.toBeCalled(); + expect(res.end).toBeCalledWith(expect.stringContaining('400')); + expect(res.end).toBeCalledWith(expect.stringContaining('This is a 400 status code.')); + }); + it('should handle any non-500 Problems', () => { const error = new Problem(429); - const { res } = getMockRes(); - const next = jest.fn(); + const { res, next } = getMockRes(); middleware.dataErrors(error, {}, res, next); @@ -70,7 +89,7 @@ describe('test data errors middleware', () => { it('should pass through any 500s', () => { const error = new Problem(500); - const next = jest.fn(); + const { next } = getMockRes(); middleware.dataErrors(error, {}, {}, next); @@ -79,7 +98,7 @@ describe('test data errors middleware', () => { it('should pass through any Errors', () => { const error = new Error(); - const next = jest.fn(); + const { next } = getMockRes(); middleware.dataErrors(error, {}, {}, next); From 505cd25941b296f73f2bbc4f0f98b958b196b169 Mon Sep 17 00:00:00 2001 From: Walter Moar Date: Wed, 5 Jun 2024 12:56:23 -0700 Subject: [PATCH 2/5] fix: FORMS-1139 update api-problem (#1382) --- app/package-lock.json | 14 +++++++------- app/package.json | 2 +- 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/app/package-lock.json b/app/package-lock.json index 66e4fc761..614f8d760 100644 --- a/app/package-lock.json +++ b/app/package-lock.json @@ -11,7 +11,7 @@ "dependencies": { "@json2csv/node": "^6.1.3", "@json2csv/transforms": "^6.1.3", - "api-problem": "^7.0.4", + "api-problem": "^9.0.2", "aws-sdk": "^2.1376.0", "axios": "^0.28.1", "axios-oauth-client": "^1.5.0", @@ -1981,9 +1981,9 @@ } }, "node_modules/api-problem": { - "version": "7.0.4", - "resolved": "https://registry.npmjs.org/api-problem/-/api-problem-7.0.4.tgz", - "integrity": "sha512-8f/Dg1LY26YSMzhguscyJWw2qVafdczyK82+X7zcnv73iN1mh4KmVEm2w7xAL3ttpWddPE7lTKMRFdidH/dFYA==", + "version": "9.0.2", + "resolved": "https://registry.npmjs.org/api-problem/-/api-problem-9.0.2.tgz", + "integrity": "sha512-Xr4TyFkyvTEkgL8zUhyoqeK2Oxx2GQaFIPNiuhVRfop34gNl5r5gk1jYMsQhtPWSpwavROVweNIyL677ibE4rw==", "engines": { "node": ">=6" }, @@ -12619,9 +12619,9 @@ } }, "api-problem": { - "version": "7.0.4", - "resolved": "https://registry.npmjs.org/api-problem/-/api-problem-7.0.4.tgz", - "integrity": "sha512-8f/Dg1LY26YSMzhguscyJWw2qVafdczyK82+X7zcnv73iN1mh4KmVEm2w7xAL3ttpWddPE7lTKMRFdidH/dFYA==" + "version": "9.0.2", + "resolved": "https://registry.npmjs.org/api-problem/-/api-problem-9.0.2.tgz", + "integrity": "sha512-Xr4TyFkyvTEkgL8zUhyoqeK2Oxx2GQaFIPNiuhVRfop34gNl5r5gk1jYMsQhtPWSpwavROVweNIyL677ibE4rw==" }, "append-field": { "version": "1.0.0", diff --git a/app/package.json b/app/package.json index d05db7cc4..be5311708 100644 --- a/app/package.json +++ b/app/package.json @@ -49,7 +49,7 @@ "dependencies": { "@json2csv/node": "^6.1.3", "@json2csv/transforms": "^6.1.3", - "api-problem": "^7.0.4", + "api-problem": "^9.0.2", "aws-sdk": "^2.1376.0", "axios": "^0.28.1", "axios-oauth-client": "^1.5.0", From 1d69f8007329c45e8b52d34367abc9a8a96f630e Mon Sep 17 00:00:00 2001 From: Walter Moar Date: Wed, 5 Jun 2024 14:09:36 -0700 Subject: [PATCH 3/5] refactor: FORMS-1139 rename dataErrors to errorHandler (#1383) --- app/app.js | 2 +- .../{dataErrors.js => errorHandler.js} | 2 +- app/src/forms/common/middleware/index.js | 2 +- ...ataErrors.spec.js => errorHandler.spec.js} | 22 +++++++++---------- 4 files changed, 14 insertions(+), 14 deletions(-) rename app/src/forms/common/middleware/{dataErrors.js => errorHandler.js} (96%) rename app/tests/unit/forms/common/middleware/{dataErrors.spec.js => errorHandler.spec.js} (83%) diff --git a/app/app.js b/app/app.js index b2385b2fa..82ce5a787 100644 --- a/app/app.js +++ b/app/app.js @@ -75,7 +75,7 @@ apiRouter.get('/api', (_req, res) => { // Host API endpoints apiRouter.use(config.get('server.apiPath'), v1Router); app.use(config.get('server.basePath'), apiRouter); -app.use(middleware.dataErrors); +app.use(middleware.errorHandler); // Host the static frontend assets const staticFilesPath = config.get('frontend.basePath'); diff --git a/app/src/forms/common/middleware/dataErrors.js b/app/src/forms/common/middleware/errorHandler.js similarity index 96% rename from app/src/forms/common/middleware/dataErrors.js rename to app/src/forms/common/middleware/errorHandler.js index b313d90fa..7595f661b 100644 --- a/app/src/forms/common/middleware/dataErrors.js +++ b/app/src/forms/common/middleware/errorHandler.js @@ -1,7 +1,7 @@ const Problem = require('api-problem'); const Objection = require('objection'); -module.exports.dataErrors = async (err, _req, res, next) => { +module.exports.errorHandler = async (err, _req, res, next) => { let error; if (err instanceof Objection.DataError) { diff --git a/app/src/forms/common/middleware/index.js b/app/src/forms/common/middleware/index.js index 7cb497cd3..e953aaa3b 100644 --- a/app/src/forms/common/middleware/index.js +++ b/app/src/forms/common/middleware/index.js @@ -1,4 +1,4 @@ module.exports = { - ...require('./dataErrors'), + ...require('./errorHandler'), ...require('./rateLimiter'), }; diff --git a/app/tests/unit/forms/common/middleware/dataErrors.spec.js b/app/tests/unit/forms/common/middleware/errorHandler.spec.js similarity index 83% rename from app/tests/unit/forms/common/middleware/dataErrors.spec.js rename to app/tests/unit/forms/common/middleware/errorHandler.spec.js index fb6369516..9d0c2476a 100644 --- a/app/tests/unit/forms/common/middleware/dataErrors.spec.js +++ b/app/tests/unit/forms/common/middleware/errorHandler.spec.js @@ -4,14 +4,14 @@ const Objection = require('objection'); const middleware = require('../../../../../src/forms/common/middleware'); -describe('test data errors middleware', () => { +describe('test error handler middleware', () => { it('should handle an objection data error', () => { const error = new Objection.DataError({ nativeError: { message: 'This is a DataError' }, }); const { res, next } = getMockRes(); - middleware.dataErrors(error, {}, res, next); + middleware.errorHandler(error, {}, res, next); expect(next).not.toBeCalled(); expect(res.end).toBeCalledWith(expect.stringContaining('422')); @@ -23,7 +23,7 @@ describe('test data errors middleware', () => { }); const { res, next } = getMockRes(); - middleware.dataErrors(error, {}, res, next); + middleware.errorHandler(error, {}, res, next); expect(next).not.toBeCalled(); expect(res.end).toBeCalledWith(expect.stringContaining('404')); @@ -35,7 +35,7 @@ describe('test data errors middleware', () => { }); const { res, next } = getMockRes(); - middleware.dataErrors(error, {}, res, next); + middleware.errorHandler(error, {}, res, next); expect(next).not.toBeCalled(); expect(res.end).toBeCalledWith(expect.stringContaining('422')); @@ -47,7 +47,7 @@ describe('test data errors middleware', () => { }); const { res, next } = getMockRes(); - middleware.dataErrors(error, {}, res, next); + middleware.errorHandler(error, {}, res, next); expect(next).not.toBeCalled(); expect(res.end).toBeCalledWith(expect.stringContaining('422')); @@ -58,7 +58,7 @@ describe('test data errors middleware', () => { error.status = 400; const { res, next } = getMockRes(); - middleware.dataErrors(error, {}, res, next); + middleware.errorHandler(error, {}, res, next); expect(next).not.toBeCalled(); expect(res.end).toBeCalledWith(expect.stringContaining('400')); @@ -70,7 +70,7 @@ describe('test data errors middleware', () => { error.statusCode = 400; const { res, next } = getMockRes(); - middleware.dataErrors(error, {}, res, next); + middleware.errorHandler(error, {}, res, next); expect(next).not.toBeCalled(); expect(res.end).toBeCalledWith(expect.stringContaining('400')); @@ -81,7 +81,7 @@ describe('test data errors middleware', () => { const error = new Problem(429); const { res, next } = getMockRes(); - middleware.dataErrors(error, {}, res, next); + middleware.errorHandler(error, {}, res, next); expect(next).not.toBeCalled(); expect(res.end).toBeCalledWith(expect.stringContaining('429')); @@ -91,16 +91,16 @@ describe('test data errors middleware', () => { const error = new Problem(500); const { next } = getMockRes(); - middleware.dataErrors(error, {}, {}, next); + middleware.errorHandler(error, {}, {}, next); expect(next).toBeCalledWith(error); }); - it('should pass through any Errors', () => { + it('should pass through any Errors without statuses', () => { const error = new Error(); const { next } = getMockRes(); - middleware.dataErrors(error, {}, {}, next); + middleware.errorHandler(error, {}, {}, next); expect(next).toBeCalledWith(error); }); From 753abdab14dc036ae898e6180978ca2cdb6e44dc Mon Sep 17 00:00:00 2001 From: Walter Moar Date: Wed, 5 Jun 2024 14:43:45 -0700 Subject: [PATCH 4/5] refactor: FORMS-1139 add db error handler (#1384) --- .../forms/common/middleware/errorHandler.js | 92 +++++++++++++------ .../common/middleware/errorHandler.spec.js | 13 +++ 2 files changed, 75 insertions(+), 30 deletions(-) diff --git a/app/src/forms/common/middleware/errorHandler.js b/app/src/forms/common/middleware/errorHandler.js index 7595f661b..084aa213d 100644 --- a/app/src/forms/common/middleware/errorHandler.js +++ b/app/src/forms/common/middleware/errorHandler.js @@ -1,45 +1,77 @@ const Problem = require('api-problem'); const Objection = require('objection'); -module.exports.errorHandler = async (err, _req, res, next) => { - let error; - - if (err instanceof Objection.DataError) { - error = new Problem(422, { +/** + * Given a subclass of DBError will create and throw the corresponding Problem. + * If the error is of an unknown type it will not be converted. + * + * @param {DBError} error the error to convert to a Problem. + * @returns nothing + */ +const _throwObjectionProblem = (error) => { + if (error instanceof Objection.DataError) { + throw new Problem(422, { detail: 'Sorry... the database does not like the data you provided :(', }); - } else if (err instanceof Objection.NotFoundError) { - error = new Problem(404, { + } + + if (error instanceof Objection.NotFoundError) { + throw new Problem(404, { detail: "Sorry... we still haven't found what you're looking for :(", }); - } else if (err instanceof Objection.UniqueViolationError) { - error = new Problem(422, { - detail: 'Unique Validation Error', + } + + if (error instanceof Objection.ConstraintViolationError) { + throw new Problem(422, { + detail: 'Constraint Violation Error', }); - } else if (err instanceof Objection.ValidationError) { - error = new Problem(422, { + } + + if (error instanceof Objection.ValidationError) { + throw new Problem(422, { detail: 'Validation Error', - errors: err.data, + errors: error.data, }); - } else if (!(err instanceof Problem) && (err.status || err.statusCode)) { + } +}; + +/** + * Send an error response for all errors except 500s, which are handled by the + * code in app.js. + * + * @param {*} err the Error that occurred. + * @param {*} _req the Express object representing the HTTP request - unused. + * @param {*} res the Express object representing the HTTP response. + * @param {*} next the Express chaining function. + * @returns nothing + */ +module.exports.errorHandler = async (err, _req, res, next) => { + try { + if (err instanceof Objection.DBError || err instanceof Objection.NotFoundError || err instanceof Objection.ValidationError) { + _throwObjectionProblem(err); + } + // Express throws Errors that are not Problems, but do have an HTTP status // code. For example, 400 is thrown when POST bodies are malformed JSON. - error = new Problem(err.status || err.statusCode, { - detail: err.message, - }); - } else { - error = err; - } + if (!(err instanceof Problem) && (err.status || err.statusCode)) { + throw new Problem(err.status || err.statusCode, { + detail: err.message, + }); + } - if (error instanceof Problem && error.status !== 500) { - // Handle here when not an internal error. These are mostly from buggy - // systems using API Keys, but could also be from frontend bugs. Save the - // ERROR level logs (below) for only the things that need investigation. - error.send(res); - } else { - // HTTP 500 Problems and all other exceptions should be handled by the error - // handler in app.js. It will log them at the ERROR level and include a full - // stack trace. - next(error); + // Not sure what it is, so also handle it in the catch block. + throw err; + } catch (error) { + if (error instanceof Problem && error.status !== 500) { + // Handle here when not an internal error. These are mostly from buggy + // systems using API Keys, but could also be from frontend bugs. Note that + // this does not log the error (see below). + error.send(res); + } else { + // HTTP 500 Problems and all other exceptions should be handled by the + // error handler in app.js. It will log them at the ERROR level and + // include a full stack trace. + next(error); + } } }; diff --git a/app/tests/unit/forms/common/middleware/errorHandler.spec.js b/app/tests/unit/forms/common/middleware/errorHandler.spec.js index 9d0c2476a..508d1df29 100644 --- a/app/tests/unit/forms/common/middleware/errorHandler.spec.js +++ b/app/tests/unit/forms/common/middleware/errorHandler.spec.js @@ -87,6 +87,19 @@ describe('test error handler middleware', () => { expect(res.end).toBeCalledWith(expect.stringContaining('429')); }); + it('should pass through unknown objection errors', () => { + const error = new Objection.DBError({ + nativeError: { + message: 'This base class is never actually instantiated', + }, + }); + const { res, next } = getMockRes(); + + middleware.errorHandler(error, {}, res, next); + + expect(next).toBeCalledWith(error); + }); + it('should pass through any 500s', () => { const error = new Problem(500); const { next } = getMockRes(); From c0ba2855045f6c705da6db5e26131cb9fcc399bd Mon Sep 17 00:00:00 2001 From: Walter Moar Date: Wed, 5 Jun 2024 16:02:33 -0700 Subject: [PATCH 5/5] revert: FORMS-1139 urlencoded extended setting (#1385) --- app/app.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/app.js b/app/app.js index 82ce5a787..007d4c69a 100644 --- a/app/app.js +++ b/app/app.js @@ -24,7 +24,7 @@ let probeId; const app = express(); app.use(compression()); app.use(express.json({ limit: config.get('server.bodyLimit') })); -app.use(express.urlencoded()); +app.use(express.urlencoded({ extended: true })); // Express needs to know about the OpenShift proxy. With this setting Express // pulls the IP address from the headers, rather than use the proxy IP address.