fix(astro): Escape param values during route interpolation

Author: Lms24

packages/astro/src/server/middleware.ts

@@ -7,12 +7,7 @@ import {
   startSpan,
 } from '@sentry/node';
 import type { Hub, Span } from '@sentry/types';
-import {
-  addNonEnumerableProperty,
-  objectify,
-  stripUrlQueryAndFragment,
-  tracingContextFromHeaders,
-} from '@sentry/utils';
+import { addNonEnumerableProperty, escapeStringForRegex, objectify, stripUrlQueryAndFragment } from '@sentry/utils';
 import type { APIContext, MiddlewareResponseHandler } from 'astro';
 
 import { getTracingMetaTags } from './meta';
@@ -64,7 +59,11 @@ type AstroLocalsWithSentry = Record<string, unknown> & {
 };
 
 export const handleRequest: (options?: MiddlewareOptions) => MiddlewareResponseHandler = options => {
-  const handlerOptions = { trackClientIp: false, trackHeaders: false, ...options };
+  const handlerOptions = {
+    trackClientIp: false,
+    trackHeaders: false,
+    ...options,
+  };
 
   return async (ctx, next) => {
     // if there is an active span, we know that this handle call is nested and hence
@@ -113,18 +112,19 @@ async function instrumentRequest(
   }
 
   try {
+    const interpolatedRoute = interpolateRouteFromUrlAndParams(ctx.url.pathname, ctx.params);
     // storing res in a variable instead of directly returning is necessary to
     // invoke the catch block if next() throws
     const res = await startSpan(
       {
         ...traceCtx,
-        name: `${method} ${interpolateRouteFromUrlAndParams(ctx.url.pathname, ctx.params)}`,
+        name: `${method} ${interpolatedRoute || ctx.url.pathname}`,
         op: 'http.server',
         origin: 'auto.http.astro',
         status: 'ok',
         metadata: {
           ...traceCtx?.metadata,
-          source: 'route',
+          source: interpolatedRoute ? 'route' : 'url',
         },
         data: {
           method,
@@ -203,9 +203,28 @@ function addMetaTagToHead(htmlChunk: string, hub: Hub, span?: Span): string {
  *
  * exported for testing
  */
-export function interpolateRouteFromUrlAndParams(rawUrl: string, params: APIContext['params']): string {
+export function interpolateRouteFromUrlAndParams(
+  rawUrlPathname: string,
+  params: APIContext['params'],
+): string | undefined {
+  const decodedUrlPathname = tryDecodeUrl(rawUrlPathname);
+  if (!decodedUrlPathname) {
+    return undefined;
+  }
+
   return Object.entries(params).reduce((interpolateRoute, value) => {
     const [paramId, paramValue] = value;
-    return interpolateRoute.replace(new RegExp(`(/|-)${paramValue}(/|-|$)`), `$1[${paramId}]$2`);
-  }, rawUrl);
+    if (!paramValue) {
+      return interpolateRoute;
+    }
+    return interpolateRoute.replace(new RegExp(`(/|-)${escapeStringForRegex(paramValue)}(/|-|$)`), `$1[${paramId}]$2`);
+  }, decodedUrlPathname);
+}
+
+function tryDecodeUrl(url: string): string | undefined {
+  try {
+    return decodeURI(url);
+  } catch {
+    return undefined;
+  }
 }

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

@@ -69,6 +69,43 @@ describe('sentryMiddleware', () => {
     expect(resultFromNext).toStrictEqual(nextResult);
   });
 
+  it("sets source route if the url couldn't be decoded correctly", async () => {
+    const middleware = handleRequest();
+    const ctx = {
+      request: {
+        method: 'GET',
+        url: '/a%xx',
+        headers: new Headers(),
+      },
+      url: { pathname: 'a%xx', href: 'http://localhost:1234/a%xx' },
+      params: {},
+    };
+    const next = vi.fn(() => nextResult);
+
+    // @ts-expect-error, a partial ctx object is fine here
+    const resultFromNext = middleware(ctx, next);
+
+    expect(startSpanSpy).toHaveBeenCalledWith(
+      {
+        data: {
+          method: 'GET',
+          url: 'http://localhost:1234/a%xx',
+        },
+        metadata: {
+          source: 'url',
+        },
+        name: 'GET a%xx',
+        op: 'http.server',
+        origin: 'auto.http.astro',
+        status: 'ok',
+      },
+      expect.any(Function), // the `next` function
+    );
+
+    expect(next).toHaveBeenCalled();
+    expect(resultFromNext).toStrictEqual(nextResult);
+  });
+
   it('throws and sends an error to sentry if `next()` throws', async () => {
     const captureExceptionSpy = vi.spyOn(SentryNode, 'captureException');
 
@@ -308,6 +345,18 @@ describe('interpolateRouteFromUrlAndParams', () => {
     expect(interpolateRouteFromUrlAndParams(rawUrl, params)).toEqual(expectedRoute);
   });
 
+  it.each([
+    ['/(a+)+/aaaaaaaaa!', { id: '(a+)+', slug: 'aaaaaaaaa!' }, '/[id]/[slug]'],
+    ['/([a-zA-Z]+)*/aaaaaaaaa!', { id: '([a-zA-Z]+)*', slug: 'aaaaaaaaa!' }, '/[id]/[slug]'],
+    ['/(a|aa)+/aaaaaaaaa!', { id: '(a|aa)+', slug: 'aaaaaaaaa!' }, '/[id]/[slug]'],
+    ['/(a|a?)+/aaaaaaaaa!', { id: '(a|a?)+', slug: 'aaaaaaaaa!' }, '/[id]/[slug]'],
+    // with URL encoding
+    ['/(a%7Caa)+/aaaaaaaaa!', { id: '(a|aa)+', slug: 'aaaaaaaaa!' }, '/[id]/[slug]'],
+    ['/(a%7Ca?)+/aaaaaaaaa!', { id: '(a|a?)+', slug: 'aaaaaaaaa!' }, '/[id]/[slug]'],
+  ])('handles regex characters in param values correctly %s', (rawUrl, params, expectedRoute) => {
+    expect(interpolateRouteFromUrlAndParams(rawUrl, params)).toEqual(expectedRoute);
+  });
+
   it('handles params across multiple URL segments in catchall routes', () => {
     // Ideally, Astro would let us know that this is a catchall route so we can make the param [...catchall] but it doesn't
     expect(
@@ -324,4 +373,11 @@ describe('interpolateRouteFromUrlAndParams', () => {
     const expectedRoute = '/usernames/[name]';
     expect(interpolateRouteFromUrlAndParams(rawUrl, params)).toEqual(expectedRoute);
   });
+
+  it('handles set but undefined params', () => {
+    const rawUrl = '/usernames/user';
+    const params = { name: undefined };
+    const expectedRoute = '/usernames/user';
+    expect(interpolateRouteFromUrlAndParams(rawUrl, params)).toEqual(expectedRoute);
+  });
 });