fix(node): Ensure express requests are properly handled (#14850)

Fixes #14847

After some digging, I figured out that the `req.user` handling on
express is not working anymore, because apparently express does not
mutate the actual http `request` object, but clones/forkes it (??)
somehow. So since we now set the request in the
SentryHttpInstrumentation generally, it would not pick up
express-specific things anymore.

IMHO this is not great and we do not want this anymore in v9 anyhow, but
it was the behavior before. This PR fixes this by setting the express
request again on the isolation scope in an express middleware, which is
registered by `Sentry.setupExpressErrorHandler(app)`.

Note that we plan to change this behavior here:
#14806 but I figured
it still makes sense to fix this on develop first, so we have a proper
history/tests for this. I will backport this to v8 too. Then, in PR
#14806 I will remove the middleware again.

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://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);

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

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

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