CVE-2023-50249: Inefficient Regular Expression Complexity (CWE-1333)
value: Sentry-Javascript is official Sentry SDKs for JavaScript. A ReDoS (Regular expression Denial of Service) vulnerability has been identified in Sentry's Astro SDK 7.78.0-7.86.0. Under certain conditions, this vulnerability allows an attacker to cause excessive computation times on the server, leading to denial of service (DoS). This vulnerability has been patched in sentry/astro version 7.87.0.
CWE description: The product uses a regular expression with an inefficient, possibly exponential worst-case computational complexity that consumes excessive CPU cycles.
CVSS v3 base score: 7.5
Published: 2023-12-20T14:15Z
Repository: https://github.com/getsentry/sentry-javascript
Language: TypeScript
Files changed: packages/astro/src/server/middleware.ts, packages/astro/test/server/middleware.test.ts
Commit date: 2023-12-13 13:18:14 +0100
Commit message: fix(astro): Avoid RegExp creation during route interpolation (#9815)

- iterate over route segments to replace parameter values
- decode raw url to match previously unmatched param values
- prioritize multi-segment [rest
parameters](https://docs.astro.build/en/core-concepts/routing/#rest-parameters)
before iterating over individual segments
Diff: diff --git a/packages/astro/src/server/middleware.ts b/packages/astro/src/server/middleware.ts
index 8ff33382d..37603a2cd 100644
--- a/packages/astro/src/server/middleware.ts
+++ b/packages/astro/src/server/middleware.ts
@@ -5,16 +5,11 @@ import {
   getCurrentHub,
   runWithAsyncContext,
   startSpan,
 } from '@sentry/node';
 import type { Hub, Span } from '@sentry/types';
-import {
-  addNonEnumerableProperty,
-  objectify,
-  stripUrlQueryAndFragment,
-  tracingContextFromHeaders,
-} from '@sentry/utils';
+import { addNonEnumerableProperty, objectify, stripUrlQueryAndFragment } from '@sentry/utils';
 import type { APIContext, MiddlewareResponseHandler } from 'astro';
 
 import { getTracingMetaTags } from './meta';
 
 type MiddlewareOptions = {
@@ -62,11 +57,15 @@ function sendErrorToSentry(e: unknown): unknown {
 type AstroLocalsWithSentry = Record<string, unknown> & {
   __sentry_wrapped__?: boolean;
 };
 
 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
     // we don't create a new domain for it. If we created one, nested server calls would
     // create new transactions instead of adding a child span to the currently active span.
@@ -111,22 +110,23 @@ async function instrumentRequest(
       scope.setUser({ ip_address: ctx.clientAddress });
     });
   }
 
   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,
           url: stripUrlQueryAndFragment(ctx.url.href),
           ...(ctx.url.search && { 'http.query': ctx.url.search }),
@@ -200,12 +200,78 @@ function addMetaTagToHead(htmlChunk: string, hub: Hub, span?: Span): string {
 /**
  * Interpolates the route from the URL and the passed params.
  * Best we can do to get a route name instead of a raw URL.
  *
  * exported for testing
+ *
+ * @param rawUrlPathname - The raw URL pathname, e.g. '/users/123/details'
+ * @param params - The params object, e.g. `{ userId: '123' }`
+ *
+ * @returns The interpolated route, e.g. '/users/[userId]/details'
  */
-export function interpolateRouteFromUrlAndParams(rawUrl: string, params: APIContext['params']): string {
-  return Object.entries(params).reduce((interpolateRoute, value) => {
-    const [paramId, paramValue] = value;
-    return interpolateRoute.replace(new RegExp(`(/|-)${paramValue}(/|-|$)`), `$1[${paramId}]$2`);
-  }, rawUrl);
+export function interpolateRouteFromUrlAndParams(
+  rawUrlPathname: string,
+  params: APIContext['params'],
+): string | undefined {
+  const decodedUrlPathname = tryDecodeUrl(rawUrlPathname);
+  if (!decodedUrlPathname) {
+    return undefined;
+  }
+
+  // Invert params map so that the param values are the keys
+  // differentiate between rest params spanning multiple url segments
+  // and normal, single-segment params.
+  const valuesToMultiSegmentParams: Record<string, string> = {};
+  const valuesToParams: Record<string, string> = {};
+  Object.entries(params).forEach(([key, value]) => {
+    if (!value) {
+      return;
+    }
+    if (value.includes('/')) {
+      valuesToMultiSegmentParams[value] = key;
+      return;
+    }
+    valuesToParams[value] = key;
+  });
+
+  function replaceWithParamName(segment: string): string {
+    const param = valuesToParams[segment];
+    if (param) {
+      return `[${param}]`;
+    }
+    return segment;
+  }
+
+  // before we match single-segment params, we first replace multi-segment params
+  const urlWithReplacedMultiSegmentParams = Object.keys(valuesToMultiSegmentParams).reduce((acc, key) => {
+    return acc.replace(key, `[${valuesToMultiSegmentParams[key]}]`);
+  }, decodedUrlPathname);
+
+  return urlWithReplacedMultiSegmentParams
+    .split('/')
+    .map(segment => {
+      if (!segment) {
+        return '';
+      }
+
+      if (valuesToParams[segment]) {
+        return replaceWithParamName(segment);
+      }
+
+      // astro permits multiple params in a single path segment, e.g. /[foo]-[bar]/
+      const segmentParts = segment.split('-');
+      if (segmentParts.length > 1) {
+        return segmentParts.map(part => replaceWithParamName(part)).join('-');
+      }
+
+      return segment;
+    })
+    .join('/');
+}
+
+function tryDecodeUrl(url: string): string | undefined {
+  try {
+    return decodeURI(url);
+  } catch {
+    return undefined;
+  }
 }
diff --git a/packages/astro/test/server/middleware.test.ts b/packages/astro/test/server/middleware.test.ts
index 19ee0ef1b..dc3b0139b 100644
--- a/packages/astro/test/server/middleware.test.ts
+++ b/packages/astro/test/server/middleware.test.ts
@@ -67,10 +67,47 @@ describe('sentryMiddleware', () => {
 
     expect(next).toHaveBeenCalled();
     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');
 
     const middleware = handleRequest();
     const ctx = {
@@ -297,19 +334,35 @@ describe('sentryMiddleware', () => {
   });
 });
 
 describe('interpolateRouteFromUrlAndParams', () => {
   it.each([
+    ['/', {}, '/'],
     ['/foo/bar', {}, '/foo/bar'],
     ['/users/123', { id: '123' }, '/users/[id]'],
     ['/users/123', { id: '123', foo: 'bar' }, '/users/[id]'],
     ['/lang/en-US', { lang: 'en', region: 'US' }, '/lang/[lang]-[region]'],
     ['/lang/en-US/posts', { lang: 'en', region: 'US' }, '/lang/[lang]-[region]/posts'],
+    // edge cases that astro doesn't support
+    ['/lang/-US', { region: 'US' }, '/lang/-[region]'],
+    ['/lang/en-', { lang: 'en' }, '/lang/[lang]-'],
   ])('interpolates route from URL and params %s', (rawUrl, params, expectedRoute) => {
     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(
       interpolateRouteFromUrlAndParams('/someroute/catchall-123/params/foo/bar', {
         catchall: 'catchall-123/params/foo',
@@ -322,6 +375,13 @@ describe('interpolateRouteFromUrlAndParams', () => {
     const rawUrl = '/usernames/username';
     const params = { name: 'username' };
     const expectedRoute = '/usernames/[name]';
     expect(interpolateRouteFromUrlAndParams(rawUrl, params)).toEqual(expectedRoute);
   });
+
+  it('handles set but undefined params', () => {
+    const rawUrl = '/usernames/user';
+    const params = { name: undefined, name2: '' };
+    const expectedRoute = '/usernames/user';
+    expect(interpolateRouteFromUrlAndParams(rawUrl, params)).toEqual(expectedRoute);
+  });
 });

Vulnerable code: import {
  addNonEnumerableProperty,
  objectify,
  stripUrlQueryAndFragment,
  tracingContextFromHeaders,
} from '@sentry/utils';
  const handlerOptions = { trackClientIp: false, trackHeaders: false, ...options };
        name: `${method} ${interpolateRouteFromUrlAndParams(ctx.url.pathname, ctx.params)}`,
          source: 'route',
export function interpolateRouteFromUrlAndParams(rawUrl: string, params: APIContext['params']): string {
  return Object.entries(params).reduce((interpolateRoute, value) => {
    const [paramId, paramValue] = value;
    return interpolateRoute.replace(new RegExp(`(/|-)${paramValue}(/|-|$)`), `$1[${paramId}]$2`);
  }, rawUrl);
Fixed code: import { addNonEnumerableProperty, objectify, stripUrlQueryAndFragment } from '@sentry/utils';
  const handlerOptions = {
    trackClientIp: false,
    trackHeaders: false,
    ...options,
  };
    const interpolatedRoute = interpolateRouteFromUrlAndParams(ctx.url.pathname, ctx.params);
        name: `${method} ${interpolatedRoute || ctx.url.pathname}`,
          source: interpolatedRoute ? 'route' : 'url',
 *
 * @param rawUrlPathname - The raw URL pathname, e.g. '/users/123/details'
 * @param params - The params object, e.g. `{ userId: '123' }`
 *
 * @returns The interpolated route, e.g. '/users/[userId]/details'
export function interpolateRouteFromUrlAndParams(
  rawUrlPathname: string,
  params: APIContext['params'],
): string | undefined {
  const decodedUrlPathname = tryDecodeUrl(rawUrlPathname);
  if (!decodedUrlPathname) {
    return undefined;
  }

  // Invert params map so that the param values are the keys
  // differentiate between rest params spanning multiple url segments
  // and normal, single-segment params.
  const valuesToMultiSegmentParams: Record<string, string> = {};
  const valuesToParams: Record<string, string> = {};
  Object.entries(params).forEach(([key, value]) => {
    if (!value) {
      return;
    }
    if (value.includes('/')) {
      valuesToMultiSegmentParams[value] = key;
      return;
    }
    valuesToParams[value] = key;
  });

  function replaceWithParamName(segment: string): string {
    const param = valuesToParams[segment];
    if (param) {
      return `[${param}]`;
    }
    return segment;
  }

  // before we match single-segment params, we first replace multi-segment params
  const urlWithReplacedMultiSegmentParams = Object.keys(valuesToMultiSegmentParams).reduce((acc, key) => {
    return acc.replace(key, `[${valuesToMultiSegmentParams[key]}]`);
  }, decodedUrlPathname);

  return urlWithReplacedMultiSegmentParams
    .split('/')
    .map(segment => {
      if (!segment) {
        return '';
      }

      if (valuesToParams[segment]) {
        return replaceWithParamName(segment);
      }

      // astro permits multiple params in a single path segment, e.g. /[foo]-[bar]/
      const segmentParts = segment.split('-');
      if (segmentParts.length > 1) {
        return segmentParts.map(part => replaceWithParamName(part)).join('-');
      }

      return segment;
    })
    .join('/');
}

function tryDecodeUrl(url: string): string | undefined {
  try {
    return decodeURI(url);
  } catch {
    return undefined;
  }
  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);
  });

    ['/', {}, '/'],
    // edge cases that astro doesn't support
    ['/lang/-US', { region: 'US' }, '/lang/-[region]'],
    ['/lang/en-', { lang: 'en' }, '/lang/[lang]-'],
  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 set but undefined params', () => {
    const rawUrl = '/usernames/user';
    const params = { name: undefined, name2: '' };
    const expectedRoute = '/usernames/user';
    expect(interpolateRouteFromUrlAndParams(rawUrl, params)).toEqual(expectedRoute);
  });