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.
getsentry · Dec 30, 2024 · 9cd0e8e · 9cd0e8e
1 parent 6f0251d
commit 9cd0e8e
Show file tree

Hide file tree

Showing 4 changed files with 123 additions and 5 deletions.
diff --git a/dev-packages/node-integration-tests/suites/express/requestUser/server.js 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
@@ -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
@@ -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
@@ -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');
 }