diff --git a/packages/astro/src/server/middleware.ts b/packages/astro/src/server/middleware.ts index 8ff33382d916..37603a2cdb62 100644 --- a/packages/astro/src/server/middleware.ts +++ b/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, objectify, stripUrlQueryAndFragment } from '@sentry/utils'; import type { APIContext, MiddlewareResponseHandler } from 'astro'; import { getTracingMetaTags } from './meta'; @@ -64,7 +59,11 @@ type AstroLocalsWithSentry = Record & { }; 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, @@ -202,10 +202,76 @@ function addMetaTagToHead(htmlChunk: string, hub: Hub, span?: Span): string { * 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 = {}; + const valuesToParams: Record = {}; + 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 19ee0ef1b5c6..dc3b0139b965 100644 --- a/packages/astro/test/server/middleware.test.ts +++ b/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'); @@ -299,15 +336,31 @@ 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( @@ -324,4 +377,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, name2: '' }; + const expectedRoute = '/usernames/user'; + expect(interpolateRouteFromUrlAndParams(rawUrl, params)).toEqual(expectedRoute); + }); });