fix(remix): Use domains to prevent scope bleed (#5570)

This patch wraps both the express and built-in request handlers with domains, preventing scope bleeding from happening. It also updates our express request handler to match that of the `withSentry` handler from NextJS. In a follow-up patch, we'll consolidate the implementations of both these handlers. To test this, we've added an integration test.

Co-authored-by: Onur Temizkan <onur@narval.co.uk>

Author: AbhiPrasad

packages/remix/src/utils/instrumentServer.ts

@@ -1,8 +1,9 @@
 /* eslint-disable max-lines */
-import { captureException, getCurrentHub } from '@sentry/node';
-import { getActiveTransaction, hasTracingEnabled, Span } from '@sentry/tracing';
-import { WrappedFunction } from '@sentry/types';
+import { captureException, getCurrentHub, Hub } from '@sentry/node';
+import { getActiveTransaction, hasTracingEnabled } from '@sentry/tracing';
+import { Transaction, WrappedFunction } from '@sentry/types';
 import { addExceptionMechanism, fill, isNodeEnv, loadModule, logger, serializeBaggage } from '@sentry/utils';
+import * as domain from 'domain';
 
 import {
   AppData,
@@ -291,9 +292,9 @@ export function startRequestHandlerTransaction(
   url: URL,
   method: string,
   routes: ServerRoute[],
+  hub: Hub,
   pkg?: ReactRouterDomPkg,
-): Span | undefined {
-  const hub = getCurrentHub();
+): Transaction {
   const currentScope = hub.getScope();
   const matches = matchServerRoutes(routes, url.pathname, pkg);
 
@@ -311,24 +312,28 @@ export function startRequestHandlerTransaction(
     },
   });
 
-  if (transaction) {
-    currentScope?.setSpan(transaction);
-  }
+  currentScope?.setSpan(transaction);
   return transaction;
 }
 
 function wrapRequestHandler(origRequestHandler: RequestHandler, build: ServerBuild): RequestHandler {
   const routes = createRoutes(build.routes);
   const pkg = loadModule<ReactRouterDomPkg>('react-router-dom');
   return async function (this: unknown, request: Request, loadContext?: unknown): Promise<Response> {
-    const url = new URL(request.url);
+    const hub = getCurrentHub();
+    const options = hub.getClient()?.getOptions();
 
-    const transaction = startRequestHandlerTransaction(url, request.method, routes, pkg);
+    if (!options || !hasTracingEnabled(options)) {
+      return origRequestHandler.call(this, request, loadContext);
+    }
+
+    const url = new URL(request.url);
+    const transaction = startRequestHandlerTransaction(url, request.method, routes, hub, pkg);
 
     const res = (await origRequestHandler.call(this, request, loadContext)) as Response;
 
-    transaction?.setHttpStatus(res.status);
-    transaction?.finish();
+    transaction.setHttpStatus(res.status);
+    transaction.finish();
 
     return res;
   };
@@ -416,7 +421,8 @@ function makeWrappedCreateRequestHandler(
     const newBuild = instrumentBuild(build);
     const requestHandler = origCreateRequestHandler.call(this, newBuild, ...args);
 
-    return wrapRequestHandler(requestHandler, newBuild);
+    const local = domain.create();
+    return local.bind(() => wrapRequestHandler(requestHandler, newBuild))();
   };
 }
 

packages/remix/src/utils/serverAdapters/express.ts

@@ -1,4 +1,9 @@
-import { extractRequestData, loadModule } from '@sentry/utils';
+import { getCurrentHub } from '@sentry/hub';
+import { flush } from '@sentry/node';
+import { hasTracingEnabled } from '@sentry/tracing';
+import { Transaction } from '@sentry/types';
+import { extractRequestData, loadModule, logger } from '@sentry/utils';
+import * as domain from 'domain';
 
 import {
   createRoutes,
@@ -35,20 +40,26 @@ function wrapExpressRequestHandler(
     res: ExpressResponse,
     next: ExpressNextFunction,
   ): Promise<void> {
-    const request = extractRequestData(req);
+    // eslint-disable-next-line @typescript-eslint/unbound-method
+    res.end = wrapEndMethod(res.end);
 
-    if (!request.url || !request.method) {
-      return origRequestHandler.call(this, req, res, next);
-    }
+    const local = domain.create();
+    local.add(req);
+    local.add(res);
 
-    const url = new URL(request.url);
+    local.run(async () => {
+      const request = extractRequestData(req);
+      const hub = getCurrentHub();
+      const options = hub.getClient()?.getOptions();
 
-    const transaction = startRequestHandlerTransaction(url, request.method, routes, pkg);
+      if (!options || !hasTracingEnabled(options) || !request.url || !request.method) {
+        return origRequestHandler.call(this, req, res, next);
+      }
 
-    await origRequestHandler.call(this, req, res, next);
-
-    transaction?.setHttpStatus(res.statusCode);
-    transaction?.finish();
+      const url = new URL(request.url);
+      startRequestHandlerTransaction(url, request.method, routes, hub, pkg);
+      await origRequestHandler.call(this, req, res, next);
+    });
   };
 }
 
@@ -57,11 +68,73 @@ function wrapExpressRequestHandler(
  */
 export function wrapExpressCreateRequestHandler(
   origCreateRequestHandler: ExpressCreateRequestHandler,
+  // eslint-disable-next-line @typescript-eslint/no-explicit-any
 ): (options: any) => ExpressRequestHandler {
+  // eslint-disable-next-line @typescript-eslint/no-explicit-any
   return function (this: unknown, options: any): ExpressRequestHandler {
     const newBuild = instrumentBuild((options as ExpressCreateRequestHandlerOptions).build);
     const requestHandler = origCreateRequestHandler.call(this, { ...options, build: newBuild });
 
     return wrapExpressRequestHandler(requestHandler, newBuild);
   };
 }
+
+export type AugmentedExpressResponse = ExpressResponse & {
+  __sentryTransaction?: Transaction;
+};
+
+type ResponseEndMethod = AugmentedExpressResponse['end'];
+type WrappedResponseEndMethod = AugmentedExpressResponse['end'];
+
+/**
+ * Wrap `res.end()` so that it closes the transaction and flushes events before letting the request finish.
+ *
+ * Note: This wraps a sync method with an async method. While in general that's not a great idea in terms of keeping
+ * things in the right order, in this case it's safe, because the native `.end()` actually *is* async, and its run
+ * actually *is* awaited, just manually so (which reflects the fact that the core of the request/response code in Node
+ * by far predates the introduction of `async`/`await`). When `.end()` is done, it emits the `prefinish` event, and
+ * only once that fires does request processing continue. See
+ * https://github.com/nodejs/node/commit/7c9b607048f13741173d397795bac37707405ba7.
+ *
+ * @param origEnd The original `res.end()` method
+ * @returns The wrapped version
+ */
+function wrapEndMethod(origEnd: ResponseEndMethod): WrappedResponseEndMethod {
+  return async function newEnd(this: AugmentedExpressResponse, ...args: unknown[]) {
+    await finishSentryProcessing(this);
+
+    return origEnd.call(this, ...args);
+  };
+}
+
+/**
+ * Close the open transaction (if any) and flush events to Sentry.
+ *
+ * @param res The outgoing response for this request, on which the transaction is stored
+ */
+async function finishSentryProcessing(res: AugmentedExpressResponse): Promise<void> {
+  const { __sentryTransaction: transaction } = res;
+
+  if (transaction) {
+    transaction.setHttpStatus(res.statusCode);
+
+    // Push `transaction.finish` to the next event loop so open spans have a better chance of finishing before the
+    // transaction closes, and make sure to wait until that's done before flushing events
+    await new Promise(resolve => {
+      setImmediate(() => {
+        transaction.finish();
+        resolve();
+      });
+    });
+  }
+
+  // Flush the event queue to ensure that events get sent to Sentry before the response is finished and the lambda
+  // ends. If there was an error, rethrow it so that the normal exception-handling mechanisms can apply.
+  try {
+    __DEBUG_BUILD__ && logger.log('Flushing events...');
+    await flush(2000);
+    __DEBUG_BUILD__ && logger.log('Done flushing events');
+  } catch (e) {
+    __DEBUG_BUILD__ && logger.log('Error while flushing events:\n', e);
+  }
+}

packages/remix/test/integration/app/routes/scope-bleed/$id.tsx

@@ -0,0 +1,18 @@
+import { json, LoaderFunction } from '@remix-run/node';
+
+import * as Sentry from '@sentry/remix';
+
+export const loader: LoaderFunction = async ({ params: { id } }) => {
+  const timeTil = parseInt(id || '', 10) * 1000;
+  await new Promise(resolve => setTimeout(resolve, 3000 - timeTil));
+  Sentry.setTag(`tag${id}`, id);
+  return json({ test: 'test' });
+};
+
+export default function ScopeBleed() {
+  return (
+    <div>
+      <h1>Hello</h1>
+    </div>
+  );
+}

packages/remix/test/integration/test/server/loader.test.ts

@@ -14,13 +14,12 @@ describe.each(['builtin', 'express'])('Remix API Loaders with adapter = %s', ada
     const config = await runServer(adapter);
     const url = `${config.url}/loader-json-response/-2`;
 
-    let [transaction, event] = await getMultipleEnvelopeRequest({ ...config, url }, { count: 2 });
+    const envelopes = await getMultipleEnvelopeRequest({ ...config, url }, { count: 2 });
 
-    // The event envelope is returned before the transaction envelope when using express adapter.
-    // We can update this when we merge the envelope filtering utility.
-    adapter === 'express' && ([event, transaction] = [transaction, event]);
+    const event = envelopes[0][2].type === 'transaction' ? envelopes[1][2] : envelopes[0][2];
+    const transaction = envelopes[0][2].type === 'transaction' ? envelopes[0][2] : envelopes[1][2];
 
-    assertSentryTransaction(transaction[2], {
+    assertSentryTransaction(transaction, {
       contexts: {
         trace: {
           status: 'internal_error',
@@ -31,7 +30,7 @@ describe.each(['builtin', 'express'])('Remix API Loaders with adapter = %s', ada
       },
     });
 
-    assertSentryEvent(event[2], {
+    assertSentryEvent(event, {
       exception: {
         values: [
           {
@@ -136,4 +135,39 @@ describe.each(['builtin', 'express'])('Remix API Loaders with adapter = %s', ada
       },
     });
   });
+
+  it('makes sure scope does not bleed between requests', async () => {
+    const { url, server, scope } = await runServer(adapter);
+
+    const envelopes = await Promise.all([
+      getEnvelopeRequest({ url: `${url}/scope-bleed/1`, server, scope }, { endServer: false }),
+      getEnvelopeRequest({ url: `${url}/scope-bleed/2`, server, scope }, { endServer: false }),
+      getEnvelopeRequest({ url: `${url}/scope-bleed/3`, server, scope }, { endServer: false }),
+      getEnvelopeRequest({ url: `${url}/scope-bleed/4`, server, scope }, { endServer: false }),
+    ]);
+
+    scope.persist(false);
+    await new Promise(resolve => server.close(resolve));
+
+    assertSentryTransaction(envelopes[0][2], {
+      tags: {
+        tag4: '4',
+      },
+    });
+    assertSentryTransaction(envelopes[1][2], {
+      tags: {
+        tag3: '3',
+      },
+    });
+    assertSentryTransaction(envelopes[2][2], {
+      tags: {
+        tag2: '2',
+      },
+    });
+    assertSentryTransaction(envelopes[3][2], {
+      tags: {
+        tag1: '1',
+      },
+    });
+  });
 });