From 42c4cbd8de44ad685ca48c5e1f1b5a8cbdce0be2 Mon Sep 17 00:00:00 2001 From: Raymond Feng Date: Tue, 28 Jan 2020 08:27:13 -0800 Subject: [PATCH] fix(rest): improves error handling for express middleware Fixes https://github.com/strongloop/loopback-next/issues/4530 --- .../integration/rest.server.integration.ts | 82 +++++++++++++++---- packages/rest/src/rest.server.ts | 54 +++++++----- 2 files changed, 100 insertions(+), 36 deletions(-) diff --git a/packages/rest/src/__tests__/integration/rest.server.integration.ts b/packages/rest/src/__tests__/integration/rest.server.integration.ts index 67708689f6e0..7c4916389495 100644 --- a/packages/rest/src/__tests__/integration/rest.server.integration.ts +++ b/packages/rest/src/__tests__/integration/rest.server.integration.ts @@ -23,6 +23,7 @@ import { BodyParser, ControllerRoute, get, + HttpErrors, post, Request, requestBody, @@ -160,25 +161,49 @@ describe('RestServer (integration)', () => { await expect(server.stop()).to.fulfilled(); }); - it('responds with 500 when Sequence fails with unhandled error', async () => { - const server = await givenAServer(); - server.handler((context, sequence) => { - return Promise.reject(new Error('unhandled test error')); + describe('unhandled error', () => { + let server: RestServer; + const consoleError = console.error; + let errorMsg = ''; + + // Patch `console.error` + before(async () => { + console.error = (format: unknown, ...args: unknown[]) => { + errorMsg = util.format(format, ...args); + }; + server = await givenAServer(); }); - // Temporarily disable Mocha's handling of uncaught exceptions - const mochaListeners = process.listeners('uncaughtException'); - process.removeAllListeners('uncaughtException'); - process.once('uncaughtException', err => { - expect(err).to.have.property('message', 'unhandled test error'); - for (const l of mochaListeners) { - process.on('uncaughtException', l); - } + // Restore `console.error` + after(() => { + console.error = consoleError; }); - return createClientForHandler(server.requestHandler) - .get('/') - .expect(500); + it('responds with 500 when Sequence fails with unhandled error', async () => { + server.handler((context, sequence) => { + return Promise.reject(new Error('unhandled test error')); + }); + await createClientForHandler(server.requestHandler) + .get('/') + .expect(500); + expect(errorMsg).to.match( + /Unhandled error in GET \/\: 500 Error\: unhandled test error/, + ); + }); + + it('hangs up socket when Sequence fails with unhandled error and headers sent', async () => { + server.handler((context, sequence) => { + context.response.writeHead(200); + return Promise.reject(new Error('unhandled test error after sent')); + }); + + await expect( + createClientForHandler(server.requestHandler).get('/'), + ).to.be.rejectedWith(/socket hang up/); + expect(errorMsg).to.match( + /Unhandled error in GET \/\: 500 Error\: unhandled test error after sent/, + ); + }); }); it('allows static assets to be mounted at /', async () => { @@ -370,6 +395,33 @@ describe('RestServer (integration)', () => { .expect('Access-Control-Max-Age', '1'); }); + it('allows CORS configuration with origin function to reject', async () => { + const server = await givenAServer({ + rest: { + port: 0, + cors: { + origin: (origin, callback) => { + process.nextTick(() => { + callback(new HttpErrors.Forbidden('Not allowed by CORS')); + }); + }, + }, + }, + }); + + server.handler(dummyRequestHandler); + + await createClientForHandler(server.requestHandler) + .options('/') + .expect(403, { + error: { + statusCode: 403, + name: 'ForbiddenError', + message: 'Not allowed by CORS', + }, + }); + }); + it('exposes "GET /openapi.json" endpoint', async () => { const server = await givenAServer(); const greetSpec = { diff --git a/packages/rest/src/rest.server.ts b/packages/rest/src/rest.server.ts index d5bef856420c..24f5dde5d5a4 100644 --- a/packages/rest/src/rest.server.ts +++ b/packages/rest/src/rest.server.ts @@ -23,12 +23,13 @@ import { import {AssertionError} from 'assert'; import cors from 'cors'; import debugFactory from 'debug'; -import express from 'express'; +import express, {ErrorRequestHandler} from 'express'; import {PathParams} from 'express-serve-static-core'; import {IncomingMessage, ServerResponse} from 'http'; import {ServerOptions} from 'https'; import {safeDump} from 'js-yaml'; import {ServeStaticOptions} from 'serve-static'; +import {writeErrorToResponse} from 'strong-error-handler'; import {BodyParser, REQUEST_BODY_PARSER_TAG} from './body-parsers'; import {HttpHandler} from './http-handler'; import {RestBindings} from './keys'; @@ -237,11 +238,34 @@ export class RestServer extends Context implements Server, HttpServerLike { }); // Mount our error handler - this._expressApp.use( - (err: Error, req: Request, res: Response, next: Function) => { - this._onUnhandledError(req, res, err); - }, - ); + this._expressApp.use(this._unexpectedErrorHandler()); + } + + /** + * Get an Express handler for unexpected errors + */ + protected _unexpectedErrorHandler(): ErrorRequestHandler { + const handleUnExpectedError: ErrorRequestHandler = ( + err, + req, + res, + next, + ) => { + // Handle errors reported by Express middleware such as CORS + // First try to use the `REJECT` action + this.get(SequenceActions.REJECT, {optional: true}) + .then(reject => { + if (reject) { + // TODO(rfeng): There is a possibility that the error is thrown + // from the `REJECT` action in the sequence + return reject({request: req, response: res}, err); + } + // Use strong-error handler directly + writeErrorToResponse(err, req, res); + }) + .catch(unexpectedErr => next(unexpectedErr)); + }; + return handleUnExpectedError; } /** @@ -734,7 +758,9 @@ export class RestServer extends Context implements Server, HttpServerLike { } /** - * Update or rebuild OpenAPI Spec object to be appropriate for the context of a specific request for the spec, leveraging both app config and request path information. + * Update or rebuild OpenAPI Spec object to be appropriate for the context of + * a specific request for the spec, leveraging both app config and request + * path information. * * @param spec base spec object from which to start * @param requestContext request to use to infer path information @@ -897,20 +923,6 @@ export class RestServer extends Context implements Server, HttpServerLike { this._httpServer = undefined; } - protected _onUnhandledError(req: Request, res: Response, err: Error) { - if (!res.headersSent) { - res.statusCode = 500; - res.end(); - } - - // It's the responsibility of the Sequence to handle any errors. - // If an unhandled error escaped, then something very wrong happened - // and it's best to crash the process immediately. - process.nextTick(() => { - throw err; - }); - } - /** * Mount an Express router to expose additional REST endpoints handled * via legacy Express-based stack.