From d3095ec3c6d99a6fc4c09dc9224fc23cf2e7b0e7 Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Mon, 30 Dec 2024 10:00:06 +0100 Subject: [PATCH] fix(node): Ensure express requests are properly handled --- .../suites/express/requestUser/server.js | 49 +++++++++++++++++++ .../suites/express/requestUser/test.ts | 49 +++++++++++++++++++ packages/core/src/utils-hoist/requestdata.ts | 2 +- .../node/src/integrations/tracing/express.ts | 28 +++++++++-- 4 files changed, 123 insertions(+), 5 deletions(-) create mode 100644 dev-packages/node-integration-tests/suites/express/requestUser/server.js create mode 100644 dev-packages/node-integration-tests/suites/express/requestUser/test.ts diff --git a/dev-packages/node-integration-tests/suites/express/requestUser/server.js b/dev-packages/node-integration-tests/suites/express/requestUser/server.js new file mode 100644 index 000000000000..d93d22905506 --- /dev/null +++ b/dev-packages/node-integration-tests/suites/express/requestUser/server.js @@ -0,0 +1,49 @@ +const { loggingTransport } = require('@sentry-internal/node-integration-tests'); +const Sentry = require('@sentry/node'); + +Sentry.init({ + dsn: 'https://public@dsn.ingest.sentry.io/1337', + release: '1.0', + transport: loggingTransport, + debug: true, +}); + +// express must be required after Sentry is initialized +const express = require('express'); +const cors = require('cors'); +const { startExpressServerAndSendPortToRunner } = require('@sentry-internal/node-integration-tests'); + +const app = express(); + +app.use(cors()); + +app.use((req, _res, next) => { + // We simulate this, which would in other cases be done by some middleware + req.user = { + id: '1', + email: 'test@sentry.io', + }; + + next(); +}); + +app.get('/test1', (_req, _res) => { + throw new Error('error_1'); +}); + +app.use((_req, _res, next) => { + Sentry.setUser({ + id: '2', + email: 'test2@sentry.io', + }); + + next(); +}); + +app.get('/test2', (_req, _res) => { + throw new Error('error_2'); +}); + +Sentry.setupExpressErrorHandler(app); + +startExpressServerAndSendPortToRunner(app); diff --git a/dev-packages/node-integration-tests/suites/express/requestUser/test.ts b/dev-packages/node-integration-tests/suites/express/requestUser/test.ts new file mode 100644 index 000000000000..ff32e2b96c89 --- /dev/null +++ b/dev-packages/node-integration-tests/suites/express/requestUser/test.ts @@ -0,0 +1,49 @@ +import { cleanupChildProcesses, createRunner } from '../../../utils/runner'; + +describe('express user handling', () => { + afterAll(() => { + cleanupChildProcesses(); + }); + + test('picks user from request', done => { + createRunner(__dirname, 'server.js') + .expect({ + event: { + user: { + id: '1', + email: 'test@sentry.io', + }, + exception: { + values: [ + { + value: 'error_1', + }, + ], + }, + }, + }) + .start(done) + .makeRequest('get', '/test1', { expectError: true }); + }); + + test('setUser overwrites user from request', done => { + createRunner(__dirname, 'server.js') + .expect({ + event: { + user: { + id: '2', + email: 'test2@sentry.io', + }, + exception: { + values: [ + { + value: 'error_2', + }, + ], + }, + }, + }) + .start(done) + .makeRequest('get', '/test2', { expectError: true }); + }); +}); diff --git a/packages/core/src/utils-hoist/requestdata.ts b/packages/core/src/utils-hoist/requestdata.ts index bff0f3f629bd..582a8954d4c6 100644 --- a/packages/core/src/utils-hoist/requestdata.ts +++ b/packages/core/src/utils-hoist/requestdata.ts @@ -295,8 +295,8 @@ export function addNormalizedRequestDataToEvent( if (Object.keys(extractedUser).length) { event.user = { - ...event.user, ...extractedUser, + ...event.user, }; } } diff --git a/packages/node/src/integrations/tracing/express.ts b/packages/node/src/integrations/tracing/express.ts index 2c2cc5d31789..b89665844e4f 100644 --- a/packages/node/src/integrations/tracing/express.ts +++ b/packages/node/src/integrations/tracing/express.ts @@ -91,7 +91,9 @@ interface MiddlewareError extends Error { }; } -type ExpressMiddleware = ( +type ExpressMiddleware = (req: http.IncomingMessage, res: http.ServerResponse, next: () => void) => void; + +type ExpressErrorMiddleware = ( error: MiddlewareError, req: http.IncomingMessage, res: http.ServerResponse, @@ -109,13 +111,17 @@ interface ExpressHandlerOptions { /** * An Express-compatible error handler. */ -export function expressErrorHandler(options?: ExpressHandlerOptions): ExpressMiddleware { +export function expressErrorHandler(options?: ExpressHandlerOptions): ExpressErrorMiddleware { return function sentryErrorMiddleware( error: MiddlewareError, - _req: http.IncomingMessage, + request: http.IncomingMessage, res: http.ServerResponse, next: (error: MiddlewareError) => void, ): void { + // Ensure we use the express-enhanced request here, instead of the plain HTTP one + // When an error happens, the `expressRequestHandler` middleware does not run, so we set it here too + getIsolationScope().setSDKProcessingMetadata({ request }); + const shouldHandleError = options?.shouldHandleError || defaultShouldHandleError; if (shouldHandleError(error)) { @@ -127,6 +133,19 @@ export function expressErrorHandler(options?: ExpressHandlerOptions): ExpressMid }; } +function expressRequestHandler(): ExpressMiddleware { + return function sentryRequestMiddleware( + request: http.IncomingMessage, + _res: http.ServerResponse, + next: () => void, + ): void { + // Ensure we use the express-enhanced request here, instead of the plain HTTP one + getIsolationScope().setSDKProcessingMetadata({ request }); + + next(); + }; +} + /** * Add an Express error handler to capture errors to Sentry. * @@ -152,9 +171,10 @@ export function expressErrorHandler(options?: ExpressHandlerOptions): ExpressMid * ``` */ export function setupExpressErrorHandler( - app: { use: (middleware: ExpressMiddleware) => unknown }, + app: { use: (middleware: ExpressMiddleware | ExpressErrorMiddleware) => unknown }, options?: ExpressHandlerOptions, ): void { + app.use(expressRequestHandler()); app.use(expressErrorHandler(options)); ensureIsWrapped(app.use, 'express'); }