fix(remix): Don't inject trace/baggage to redirect and catch responses (#8467)

Skips `trace` and `baggage` injections to `redirect` and `catch`
responses.

For `redirect` responses: It was breaking the behaviour of redirection.
Internal redirection targets should already have their `trace` and
`baggage`, so I assume this should not break the connection between
services at the end.

`catch` responses do not have bodies, and they are thrown by Remix, so
skipping injection as well not to potentially break the internal catch
behaviour of Remix.

Author: onurtemizkan

packages/remix/src/utils/instrumentServer.ts

@@ -236,18 +236,26 @@ function makeWrappedRootLoader(origLoader: DataFunction): DataFunction {
       };
     }
 
-    // Note: `redirect` and `catch` responses do not have bodies to extract
-    if (isResponse(res) && !isRedirectResponse(res) && !isCatchResponse(res)) {
-      const data = await extractData(res);
-
-      if (typeof data === 'object') {
-        return json(
-          { ...data, ...traceAndBaggage },
-          { headers: res.headers, statusText: res.statusText, status: res.status },
-        );
-      } else {
-        __DEBUG_BUILD__ && logger.warn('Skipping injection of trace and baggage as the response body is not an object');
+    if (isResponse(res)) {
+      // Note: `redirect` and `catch` responses do not have bodies to extract.
+      // We skip injection of trace and baggage in those cases.
+      // For `redirect`, a valid internal redirection target will have the trace and baggage injected.
+      if (isRedirectResponse(res) || isCatchResponse(res)) {
+        __DEBUG_BUILD__ && logger.warn('Skipping injection of trace and baggage as the response does not have a body');
         return res;
+      } else {
+        const data = await extractData(res);
+
+        if (typeof data === 'object') {
+          return json(
+            { ...data, ...traceAndBaggage },
+            { headers: res.headers, statusText: res.statusText, status: res.status },
+          );
+        } else {
+          __DEBUG_BUILD__ &&
+            logger.warn('Skipping injection of trace and baggage as the response body is not an object');
+          return res;
+        }
       }
     }
 

packages/remix/test/integration/app_v1/root.tsx

@@ -34,6 +34,10 @@ export const loader: LoaderFunction = async ({ request }) => {
       throw redirect('/?type=plain');
     case 'returnRedirect':
       return redirect('/?type=plain');
+    case 'throwRedirectToExternal':
+      throw redirect('https://example.com');
+    case 'returnRedirectToExternal':
+      return redirect('https://example.com');
     default: {
       return {};
     }

packages/remix/test/integration/app_v2/root.tsx

@@ -34,6 +34,10 @@ export const loader: LoaderFunction = async ({ request }) => {
       throw redirect('/?type=plain');
     case 'returnRedirect':
       return redirect('/?type=plain');
+    case 'throwRedirectToExternal':
+      throw redirect('https://example.com');
+    case 'returnRedirectToExternal':
+      return redirect('https://example.com');
     default: {
       return {};
     }

packages/remix/test/integration/test/client/root-loader.test.ts

@@ -125,6 +125,9 @@ test('should inject `sentry-trace` and `baggage` into root loader throwing a red
 }) => {
   await page.goto('/?type=throwRedirect');
 
+  // We should be successfully redirected to the path.
+  expect(page.url()).toEqual(expect.stringContaining('/?type=plain'));
+
   const { sentryTrace, sentryBaggage } = await extractTraceAndBaggageFromMeta(page);
 
   expect(sentryTrace).toEqual(expect.any(String));
@@ -138,11 +141,14 @@ test('should inject `sentry-trace` and `baggage` into root loader throwing a red
   });
 });
 
-test('should inject `sentry-trace` and `baggage` into root loader returning a redirection to a plain object', async ({
+test('should inject `sentry-trace` and `baggage` into root loader returning a redirection to valid path.', async ({
   page,
 }) => {
   await page.goto('/?type=returnRedirect');
 
+  // We should be successfully redirected to the path.
+  expect(page.url()).toEqual(expect.stringContaining('/?type=plain'));
+
   const { sentryTrace, sentryBaggage } = await extractTraceAndBaggageFromMeta(page);
 
   expect(sentryTrace).toEqual(expect.any(String));
@@ -155,3 +161,27 @@ test('should inject `sentry-trace` and `baggage` into root loader returning a re
     sentryBaggage: sentryBaggage,
   });
 });
+
+test('should return redirect to an external path with no baggage and trace injected.', async ({ page }) => {
+  await page.goto('/?type=returnRedirectToExternal');
+
+  // We should be successfully redirected to the external path.
+  expect(page.url()).toEqual(expect.stringContaining('https://example.com'));
+
+  const { sentryTrace, sentryBaggage } = await extractTraceAndBaggageFromMeta(page);
+
+  expect(sentryTrace).toBeUndefined();
+  expect(sentryBaggage).toBeUndefined();
+});
+
+test('should throw redirect to an external path with no baggage and trace injected.', async ({ page }) => {
+  await page.goto('/?type=throwRedirectToExternal');
+
+  // We should be successfully redirected to the external path.
+  expect(page.url()).toEqual(expect.stringContaining('https://example.com'));
+
+  const { sentryTrace, sentryBaggage } = await extractTraceAndBaggageFromMeta(page);
+
+  expect(sentryTrace).toBeUndefined();
+  expect(sentryBaggage).toBeUndefined();
+});