fix(nextjs): Use domains to prevent scope bleed (getsentry#3788)

lobsterkatie · web-flow · commit 90eb35c92310 · 2021-07-14T16:30:09.000-07:00
Use domains to keep scope data attached to simultaneous requests from bleeding between those requests. (For example, when testing tracing on API routes deployed to Vercel, in certain cases all transactions were using the same `traceId`, even though the original transaction corresponding to that id had long since finished.)
diff --git a/packages/nextjs/src/utils/handlers.ts b/packages/nextjs/src/utils/handlers.ts
@@ -1,6 +1,7 @@
 import { captureException, flush, getCurrentHub, Handlers, startTransaction, withScope } from '@sentry/node';
 import { extractTraceparentData, getActiveTransaction, hasTracingEnabled } from '@sentry/tracing';
 import { addExceptionMechanism, isString, logger, stripUrlQueryAndFragment } from '@sentry/utils';
+import * as domain from 'domain';
 import { NextApiHandler } from 'next';
 
 import { addRequestDataToEvent, NextRequest } from './instrumentServer';
@@ -14,70 +15,82 @@ type WrappedNextApiHandler = NextApiHandler;
 export const withSentry = (handler: NextApiHandler): WrappedNextApiHandler => {
   // eslint-disable-next-line @typescript-eslint/explicit-module-boundary-types
   return async (req, res) => {
-    try {
-      const currentScope = getCurrentHub().getScope();
+    // wrap everything in a domain in order to prevent scope bleed between requests
+    const local = domain.create();
+    local.add(req);
+    local.add(res);
 
-      if (currentScope) {
-        currentScope.addEventProcessor(event => addRequestDataToEvent(event, req as NextRequest));
+    // `local.bind` causes everything to run inside a domain, just like `local.run` does, but it also lets the callback
+    // return a value. In our case, all any of the codepaths return is a promise of `void`, but nextjs still counts on
+    // getting that before it will finish the response.
+    const boundHandler = local.bind(async () => {
+      try {
+        const currentScope = getCurrentHub().getScope();
 
-        if (hasTracingEnabled()) {
-          // If there is a trace header set, extract the data from it (parentSpanId, traceId, and sampling decision)
-          let traceparentData;
-          if (req.headers && isString(req.headers['sentry-trace'])) {
-            traceparentData = extractTraceparentData(req.headers['sentry-trace'] as string);
-            logger.log(`[Tracing] Continuing trace ${traceparentData?.traceId}.`);
-          }
+        if (currentScope) {
+          currentScope.addEventProcessor(event => addRequestDataToEvent(event, req as NextRequest));
 
-          const url = `${req.url}`;
-          // pull off query string, if any
-          let reqPath = stripUrlQueryAndFragment(url);
-          // Replace with placeholder
-          if (req.query) {
-            // TODO get this from next if possible, to avoid accidentally replacing non-dynamic parts of the path if
-            // they match dynamic parts
-            for (const [key, value] of Object.entries(req.query)) {
-              reqPath = reqPath.replace(`${value}`, `[${key}]`);
+          if (hasTracingEnabled()) {
+            // If there is a trace header set, extract the data from it (parentSpanId, traceId, and sampling decision)
+            let traceparentData;
+            if (req.headers && isString(req.headers['sentry-trace'])) {
+              traceparentData = extractTraceparentData(req.headers['sentry-trace'] as string);
+              logger.log(`[Tracing] Continuing trace ${traceparentData?.traceId}.`);
             }
-          }
-          const reqMethod = `${(req.method || 'GET').toUpperCase()} `;
 
-          const transaction = startTransaction(
-            {
-              name: `${reqMethod}${reqPath}`,
-              op: 'http.server',
-              ...traceparentData,
-            },
-            // extra context passed to the `tracesSampler`
-            { request: req },
-          );
-          currentScope.setSpan(transaction);
+            const url = `${req.url}`;
+            // pull off query string, if any
+            let reqPath = stripUrlQueryAndFragment(url);
+            // Replace with placeholder
+            if (req.query) {
+              // TODO get this from next if possible, to avoid accidentally replacing non-dynamic parts of the path if
+              // they match dynamic parts
+              for (const [key, value] of Object.entries(req.query)) {
+                reqPath = reqPath.replace(`${value}`, `[${key}]`);
+              }
+            }
+            const reqMethod = `${(req.method || 'GET').toUpperCase()} `;
+
+            const transaction = startTransaction(
+              {
+                name: `${reqMethod}${reqPath}`,
+                op: 'http.server',
+                ...traceparentData,
+              },
+              // extra context passed to the `tracesSampler`
+              { request: req },
+            );
+            currentScope.setSpan(transaction);
+          }
         }
-      }
 
-      return await handler(req, res); // Call original handler
-    } catch (e) {
-      withScope(scope => {
-        scope.addEventProcessor(event => {
-          addExceptionMechanism(event, {
-            handled: false,
+        return await handler(req, res); // Call original handler
+      } catch (e) {
+        withScope(scope => {
+          scope.addEventProcessor(event => {
+            addExceptionMechanism(event, {
+              handled: false,
+            });
+            return parseRequest(event, req);
           });
-          return parseRequest(event, req);
+          captureException(e);
         });
-        captureException(e);
-      });
-      throw e;
-    } finally {
-      const transaction = getActiveTransaction();
-      if (transaction) {
-        transaction.setHttpStatus(res.statusCode);
+        throw e;
+      } finally {
+        const transaction = getActiveTransaction();
+        if (transaction) {
+          transaction.setHttpStatus(res.statusCode);
 
-        transaction.finish();
-      }
-      try {
-        await flush(2000);
-      } catch (e) {
-        // no-empty
+          transaction.finish();
+        }
+        try {
+          await flush(2000);
+        } catch (e) {
+          // no-empty
+        }
       }
-    }
+    });
+
+    return await boundHandler();
   };
 };
