fix(rest): improves error handling for express middleware

Fixes #4530
loopbackio · Jan 28, 2020 · a71a33e · a71a33e
1 parent f20f518
commit a71a33e
Show file tree

Hide file tree

Showing 2 changed files with 43 additions and 10 deletions.
diff --git a/packages/rest/src/__tests__/integration/rest.server.integration.ts b/packages/rest/src/__tests__/integration/rest.server.integration.ts
@@ -22,6 +22,7 @@ import util from 'util';
 import {
   BodyParser,
   get,
+  HttpErrors,
   post,
   Request,
   requestBody,
@@ -369,6 +370,31 @@ describe('RestServer (integration)', () => {
       .expect('Access-Control-Max-Age', '1');
   });
 
+  it('allows custom CORS configuration with origin function', 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: {
+          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
@@ -238,7 +238,7 @@ 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._onUnhandledError(req, res, err, next);
       },
     );
   }
@@ -884,18 +884,25 @@ export class RestServer extends Context implements Server, HttpServerLike {
     this._httpServer = undefined;
   }
 
-  protected _onUnhandledError(req: Request, res: Response, err: Error) {
+  protected _onUnhandledError(
+    req: Request,
+    res: Response,
+    err: Error,
+    next: Function,
+  ) {
     if (!res.headersSent) {
-      res.statusCode = 500;
-      res.end();
+      // The error can be thrown from cors middleware
+      // See https://github.com/strongloop/loopback-next/issues/4530
+      // eslint-disable-next-line @typescript-eslint/no-explicit-any
+      res.status((err as any).statusCode || 500).send({error: err});
+      return;
     }
 
-    // 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;
-    });
+    // https://expressjs.com/en/guide/error-handling.html
+    // So when you add a custom error handler, you must delegate to the
+    // default Express error handler, when the headers have already been sent
+    // to the client:
+    next(err);
   }
 
   /**