fix(v8/node): Ensure express requests are properly handled (#14851)

Backport of #14850

Fixes #14847

Author: mydea

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://[email protected]/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: '[email protected]',
+  };
+
+  next();
+});
+
+app.get('/test1', (_req, _res) => {
+  throw new Error('error_1');
+});
+
+app.use((_req, _res, next) => {
+  Sentry.setUser({
+    id: '2',
+    email: '[email protected]',
+  });
+
+  next();
+});
+
+app.get('/test2', (_req, _res) => {
+  throw new Error('error_2');
+});
+
+Sentry.setupExpressErrorHandler(app);
+
+startExpressServerAndSendPortToRunner(app);

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: '[email protected]',
+          },
+          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: '[email protected]',
+          },
+          exception: {
+            values: [
+              {
+                value: 'error_2',
+              },
+            ],
+          },
+        },
+      })
+      .start(done)
+      .makeRequest('get', '/test2', { expectError: true });
+  });
+});

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,
       };
     }
   }

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');
 }