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 e6cdcf514b2c..18e50edb8e4a 100644 --- a/packages/node/src/integrations/tracing/express.ts +++ b/packages/node/src/integrations/tracing/express.ts @@ -93,7 +93,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, @@ -111,13 +113,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)) { @@ -152,6 +158,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. * @@ -177,9 +196,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'); }