fix(astro): Only track access request headers in dynamic page requests (#13306)

In Astro middleware, we're not allowed to access the `request.headers`
object if the incoming request is for a statically generated/prerendered
route. Since we accessed these headers previously, users would get a
warning as reported multiple times and tracked in
#13116.
This patch fixes that by checking for static vs dynamic route

Author: Lms24

packages/astro/src/server/middleware.ts

@@ -84,19 +84,27 @@ async function instrumentRequest(
   }
   addNonEnumerableProperty(locals, '__sentry_wrapped__', true);
 
-  const { method, headers } = ctx.request;
+  const isDynamicPageRequest = checkIsDynamicPageRequest(ctx);
+
+  const { method, headers } = isDynamicPageRequest
+    ? ctx.request
+    : // headers can only be accessed in dynamic routes. Accessing `ctx.request.headers` in a static route
+      // will make the server log a warning.
+      { method: ctx.request.method, headers: undefined };
 
   return continueTrace(
     {
-      sentryTrace: headers.get('sentry-trace') || undefined,
-      baggage: headers.get('baggage'),
+      sentryTrace: headers?.get('sentry-trace') || undefined,
+      baggage: headers?.get('baggage'),
     },
     async () => {
-      // We store this on the current scope, not isolation scope,
-      // because we may have multiple requests nested inside each other
-      getCurrentScope().setSDKProcessingMetadata({ request: ctx.request });
+      getCurrentScope().setSDKProcessingMetadata({
+        // We store the request on the current scope, not isolation scope,
+        // because we may have multiple requests nested inside each other
+        request: isDynamicPageRequest ? ctx.request : { method, url: ctx.request.url },
+      });
 
-      if (options.trackClientIp) {
+      if (options.trackClientIp && isDynamicPageRequest) {
         getCurrentScope().setUser({ ip_address: ctx.clientAddress });
       }
 
@@ -277,3 +285,16 @@ function tryDecodeUrl(url: string): string | undefined {
     return undefined;
   }
 }
+
+/**
+ * Checks if the incoming request is a request for a dynamic (server-side rendered) page.
+ * We can check this by looking at the middleware's `clientAddress` context property because accessing
+ * this prop in a static route will throw an error which we can conveniently catch.
+ */
+function checkIsDynamicPageRequest(context: Parameters<MiddlewareResponseHandler>[0]): boolean {
+  try {
+    return context.clientAddress != null;
+  } catch {
+    return false;
+  }
+}

packages/astro/test/server/middleware.test.ts

@@ -149,55 +149,117 @@ describe('sentryMiddleware', () => {
     });
   });
 
-  it('attaches client IP if `trackClientIp=true`', async () => {
-    const middleware = handleRequest({ trackClientIp: true });
-    const ctx = {
-      request: {
-        method: 'GET',
-        url: '/users',
-        headers: new Headers({
-          'some-header': 'some-value',
-        }),
-      },
-      clientAddress: '192.168.0.1',
-      params: {},
-      url: new URL('https://myDomain.io/users/'),
-    };
-    const next = vi.fn(() => nextResult);
+  describe('track client IP address', () => {
+    it('attaches client IP if `trackClientIp=true` when handling dynamic page requests', async () => {
+      const middleware = handleRequest({ trackClientIp: true });
+      const ctx = {
+        request: {
+          method: 'GET',
+          url: '/users',
+          headers: new Headers({
+            'some-header': 'some-value',
+          }),
+        },
+        clientAddress: '192.168.0.1',
+        params: {},
+        url: new URL('https://myDomain.io/users/'),
+      };
+      const next = vi.fn(() => nextResult);
 
-    // @ts-expect-error, a partial ctx object is fine here
-    await middleware(ctx, next);
+      // @ts-expect-error, a partial ctx object is fine here
+      await middleware(ctx, next);
 
-    expect(setUserMock).toHaveBeenCalledWith({ ip_address: '192.168.0.1' });
+      expect(setUserMock).toHaveBeenCalledWith({ ip_address: '192.168.0.1' });
+    });
+
+    it("doesn't attach a client IP if `trackClientIp=true` when handling static page requests", async () => {
+      const middleware = handleRequest({ trackClientIp: true });
+
+      const ctx = {
+        request: {
+          method: 'GET',
+          url: '/users',
+          headers: new Headers({
+            'some-header': 'some-value',
+          }),
+        },
+        get clientAddress() {
+          throw new Error('clientAddress.get() should not be called in static page requests');
+        },
+        params: {},
+        url: new URL('https://myDomain.io/users/'),
+      };
+
+      const next = vi.fn(() => nextResult);
+
+      // @ts-expect-error, a partial ctx object is fine here
+      await middleware(ctx, next);
+
+      expect(setUserMock).not.toHaveBeenCalled();
+      expect(next).toHaveBeenCalledTimes(1);
+    });
   });
 
-  it('attaches request as SDK processing metadata', async () => {
-    const middleware = handleRequest({});
-    const ctx = {
-      request: {
-        method: 'GET',
-        url: '/users',
-        headers: new Headers({
-          'some-header': 'some-value',
-        }),
-      },
-      clientAddress: '192.168.0.1',
-      params: {},
-      url: new URL('https://myDomain.io/users/'),
-    };
-    const next = vi.fn(() => nextResult);
+  describe('request data', () => {
+    it('attaches request as SDK processing metadata in dynamic page requests', async () => {
+      const middleware = handleRequest({});
+      const ctx = {
+        request: {
+          method: 'GET',
+          url: '/users',
+          headers: new Headers({
+            'some-header': 'some-value',
+          }),
+        },
+        clientAddress: '192.168.0.1',
+        params: {},
+        url: new URL('https://myDomain.io/users/'),
+      };
+      const next = vi.fn(() => nextResult);
 
-    // @ts-expect-error, a partial ctx object is fine here
-    await middleware(ctx, next);
+      // @ts-expect-error, a partial ctx object is fine here
+      await middleware(ctx, next);
 
-    expect(setSDKProcessingMetadataMock).toHaveBeenCalledWith({
-      request: {
-        method: 'GET',
-        url: '/users',
-        headers: new Headers({
-          'some-header': 'some-value',
-        }),
-      },
+      expect(setSDKProcessingMetadataMock).toHaveBeenCalledWith({
+        request: {
+          method: 'GET',
+          url: '/users',
+          headers: new Headers({
+            'some-header': 'some-value',
+          }),
+        },
+      });
+      expect(next).toHaveBeenCalledTimes(1);
+    });
+
+    it("doesn't attach request headers as processing metadata for static page requests", async () => {
+      const middleware = handleRequest({});
+      const ctx = {
+        request: {
+          method: 'GET',
+          url: '/users',
+          headers: new Headers({
+            'some-header': 'some-value',
+          }),
+        },
+        get clientAddress() {
+          throw new Error('clientAddress.get() should not be called in static page requests');
+        },
+        params: {},
+        url: new URL('https://myDomain.io/users/'),
+      };
+      const next = vi.fn(() => nextResult);
+
+      // @ts-expect-error, a partial ctx object is fine here
+      await middleware(ctx, next);
+
+      expect(setSDKProcessingMetadataMock).toHaveBeenCalledWith({
+        request: {
+          method: 'GET',
+          url: '/users',
+        },
+      });
+      expect(next).toHaveBeenCalledTimes(1);
     });
   });