From 7ae570490302b0b6a748b8ef174aa6b9cdfa8902 Mon Sep 17 00:00:00 2001 From: Patrik Oldsberg Date: Mon, 19 Feb 2024 13:07:43 +0100 Subject: [PATCH 1/2] backend-app-api: filter internal errors Signed-off-by: Patrik Oldsberg --- .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; +} From 3fffd8a2cbcd3ea45f1ec2ed7ebb6ed13f695a0b Mon Sep 17 00:00:00 2001 From: Patrik Oldsberg Date: Mon, 19 Feb 2024 13:24:48 +0100 Subject: [PATCH 2/2] backend-app-api: include logId in filtered error log Signed-off-by: Patrik Oldsberg --- .../src/http/MiddlewareFactory.test.ts | 15 +++++++++++---- .../src/http/applyInternalErrorFilter.ts | 7 +++---- 2 files changed, 14 insertions(+), 8 deletions(-) diff --git a/packages/backend-app-api/src/http/MiddlewareFactory.test.ts b/packages/backend-app-api/src/http/MiddlewareFactory.test.ts index f0506f11de..418d170365 100644 --- a/packages/backend-app-api/src/http/MiddlewareFactory.test.ts +++ b/packages/backend-app-api/src/http/MiddlewareFactory.test.ts @@ -195,6 +195,11 @@ describe('MiddlewareFactory', () => { it('should filter out internal errors', async () => { const app = express(); + const grandChildLogger = { + error: jest.fn(), + }; + childLogger.child.mockReturnValue(grandChildLogger); + class DatabaseError extends Error {} const thrownError = new DatabaseError('some error'); @@ -205,18 +210,20 @@ describe('MiddlewareFactory', () => { await request(app).get('/breaks'); - expect(childLogger.error).toHaveBeenCalledTimes(2); + const [{ logId }] = childLogger.child.mock.calls[0]; + + expect(logId).toMatch(/^[0-9a-f]+$/); expect(childLogger.error).toHaveBeenCalledWith( 'Request failed with status 500', expect.objectContaining({ message: expect.stringMatching( - /^An internal error occurred logId=[0-9a-f]+$/, + `An internal error occurred logId=${logId}`, ), }), ); - expect(childLogger.error).toHaveBeenCalledWith( + expect(grandChildLogger.error).toHaveBeenCalledWith( expect.stringMatching( - /^Filtered internal error with logId=[0-9a-f]+ from response$/, + `Filtered internal error with logId=${logId} from response`, ), thrownError, ); diff --git a/packages/backend-app-api/src/http/applyInternalErrorFilter.ts b/packages/backend-app-api/src/http/applyInternalErrorFilter.ts index e2e2f60e73..d1d1e0e4d9 100644 --- a/packages/backend-app-api/src/http/applyInternalErrorFilter.ts +++ b/packages/backend-app-api/src/http/applyInternalErrorFilter.ts @@ -20,10 +20,9 @@ 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, - ); + logger + .child({ logId }) + .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;