fix(rest): improves error handling for express middleware

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 = {

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.