From 7ae570490302b0b6a748b8ef174aa6b9cdfa8902 Mon Sep 17 00:00:00 2001
From: Patrik Oldsberg <poldsberg@gmail.com>
Date: Mon, 19 Feb 2024 13:07:43 +0100
Subject: [PATCH] backend-app-api: filter internal errors

Signed-off-by: Patrik Oldsberg <poldsberg@gmail.com>
---
 .changeset/heavy-cameras-provide.md           |  5 ++
 .../src/http/MiddlewareFactory.test.ts        | 30 ++++++++++
 .../src/http/MiddlewareFactory.ts             | 10 +++-
 .../src/http/applyInternalErrorFilter.ts      | 56 +++++++++++++++++++
 4 files changed, 100 insertions(+), 1 deletion(-)
 create mode 100644 .changeset/heavy-cameras-provide.md
 create mode 100644 packages/backend-app-api/src/http/applyInternalErrorFilter.ts

diff --git a/.changeset/heavy-cameras-provide.md b/.changeset/heavy-cameras-provide.md
new file mode 100644
index 0000000000..1d86f20592
--- /dev/null
+++ b/.changeset/heavy-cameras-provide.md
@@ -0,0 +1,5 @@
+---
+'@backstage/backend-app-api': patch
+---
+
+Updated the default error handling middleware to filter out certain known error types that should never be returned in responses. The errors are instead logged along with a correlation ID, which is also returned in the response. Initially only PostgreSQL protocol errors from the `pg-protocol` package are filtered out.
diff --git a/packages/backend-app-api/src/http/MiddlewareFactory.test.ts b/packages/backend-app-api/src/http/MiddlewareFactory.test.ts
index 5dbb63ec59..f0506f11de 100644
--- a/packages/backend-app-api/src/http/MiddlewareFactory.test.ts
+++ b/packages/backend-app-api/src/http/MiddlewareFactory.test.ts
@@ -192,6 +192,36 @@ describe('MiddlewareFactory', () => {
       );
     });
 
+    it('should filter out internal errors', async () => {
+      const app = express();
+
+      class DatabaseError extends Error {}
+      const thrownError = new DatabaseError('some error');
+
+      app.use('/breaks', () => {
+        throw thrownError;
+      });
+      app.use(middleware.error());
+
+      await request(app).get('/breaks');
+
+      expect(childLogger.error).toHaveBeenCalledTimes(2);
+      expect(childLogger.error).toHaveBeenCalledWith(
+        'Request failed with status 500',
+        expect.objectContaining({
+          message: expect.stringMatching(
+            /^An internal error occurred logId=[0-9a-f]+$/,
+          ),
+        }),
+      );
+      expect(childLogger.error).toHaveBeenCalledWith(
+        expect.stringMatching(
+          /^Filtered internal error with logId=[0-9a-f]+ from response$/,
+        ),
+        thrownError,
+      );
+    });
+
     it('does not log 400 errors', async () => {
       const app = express();
 
diff --git a/packages/backend-app-api/src/http/MiddlewareFactory.ts b/packages/backend-app-api/src/http/MiddlewareFactory.ts
index bd78d77045..77136fd7e5 100644
--- a/packages/backend-app-api/src/http/MiddlewareFactory.ts
+++ b/packages/backend-app-api/src/http/MiddlewareFactory.ts
@@ -43,6 +43,7 @@ import {
   serializeError,
 } from '@backstage/errors';
 import { NotImplementedError } from '@backstage/errors';
+import { applyInternalErrorFilter } from './applyInternalErrorFilter';
 
 /**
  * Options used to create a {@link MiddlewareFactory}.
@@ -209,7 +210,14 @@ export class MiddlewareFactory {
       type: 'errorHandler',
     });
 
-    return (error: Error, req: Request, res: Response, next: NextFunction) => {
+    return (
+      rawError: Error,
+      req: Request,
+      res: Response,
+      next: NextFunction,
+    ) => {
+      const error = applyInternalErrorFilter(rawError, logger);
+
       const statusCode = getStatusCode(error);
       if (options.logAllErrors || statusCode >= 500) {
         logger.error(`Request failed with status ${statusCode}`, error);
diff --git a/packages/backend-app-api/src/http/applyInternalErrorFilter.ts b/packages/backend-app-api/src/http/applyInternalErrorFilter.ts
new file mode 100644
index 0000000000..e2e2f60e73
--- /dev/null
+++ b/packages/backend-app-api/src/http/applyInternalErrorFilter.ts
@@ -0,0 +1,56 @@
+/*
+ * Copyright 2024 The Backstage Authors
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+import { LoggerService } from '@backstage/backend-plugin-api';
+import { assertError } from '@backstage/errors';
+import { randomBytes } from 'crypto';
+
+function handleBadError(error: Error, logger: LoggerService) {
+  const logId = randomBytes(10).toString('hex');
+  logger.error(
+    `Filtered internal error with logId=${logId} from response`,
+    error,
+  );
+  const newError = new Error(`An internal error occurred logId=${logId}`);
+  delete newError.stack; // Trim the stack since it's not particularly useful
+  return newError;
+}
+
+/**
+ * Filters out certain known error types that should never be returned in responses.
+ *
+ * @internal
+ */
+export function applyInternalErrorFilter(
+  error: unknown,
+  logger: LoggerService,
+): Error {
+  try {
+    assertError(error);
+  } catch (assertionError: unknown) {
+    assertError(assertionError);
+    return handleBadError(assertionError, logger);
+  }
+
+  const constructorName = error.constructor.name;
+
+  // DatabaseError are thrown by the pg-protocol module
+  if (constructorName === 'DatabaseError') {
+    return handleBadError(error, logger);
+  }
+
+  return error;
+}