diff --git a/packages/node-integration-tests/suites/express/multiple-routers/complex-router/server.ts b/packages/node-integration-tests/suites/express/multiple-routers/complex-router/server.ts new file mode 100644 index 000000000000..6083d1a85f91 --- /dev/null +++ b/packages/node-integration-tests/suites/express/multiple-routers/complex-router/server.ts @@ -0,0 +1,35 @@ +import * as Sentry from '@sentry/node'; +import * as Tracing from '@sentry/tracing'; +import express from 'express'; + +const app = express(); + +Sentry.init({ + dsn: 'https://public@dsn.ingest.sentry.io/1337', + release: '1.0', + // eslint-disable-next-line deprecation/deprecation + integrations: [new Sentry.Integrations.Http({ tracing: true }), new Tracing.Integrations.Express({ app })], + tracesSampleRate: 1.0, +}); + +app.use(Sentry.Handlers.requestHandler()); +app.use(Sentry.Handlers.tracingHandler()); + +const APIv1 = express.Router(); + +APIv1.use( + '/users/:userId', + APIv1.get('/posts/:postId', (_req, res) => { + Sentry.captureMessage('Custom Message'); + return res.send('Success'); + }), +); + +const router = express.Router(); + +app.use('/api', router); +app.use('/api/api/v1', APIv1.use('/sub-router', APIv1)); + +app.use(Sentry.Handlers.errorHandler()); + +export default app; diff --git a/packages/node-integration-tests/suites/express/multiple-routers/complex-router/test.ts b/packages/node-integration-tests/suites/express/multiple-routers/complex-router/test.ts new file mode 100644 index 000000000000..b8079bfdc0ac --- /dev/null +++ b/packages/node-integration-tests/suites/express/multiple-routers/complex-router/test.ts @@ -0,0 +1,27 @@ +import { assertSentryEvent, TestEnv } from '../../../../utils/index'; + +test('should construct correct url with multiple parameterized routers, when param is also contain in middle layer route and express used multiple middlewares with route', async () => { + const env = await TestEnv.init(__dirname, `${__dirname}/server.ts`); + const event = await env.getEnvelopeRequest({ + url: env.url.replace('test', 'api/api/v1/sub-router/users/123/posts/456'), + envelopeType: 'transaction', + }); + // parse node.js major version + const [major] = process.versions.node.split('.').map(Number); + // Split test result base on major node version because regex d flag is support from node 16+ + if (major >= 16) { + assertSentryEvent(event[2] as any, { + transaction: 'GET /api/api/v1/sub-router/users/:userId/posts/:postId', + transaction_info: { + source: 'route', + }, + }); + } else { + assertSentryEvent(event[2] as any, { + transaction: 'GET /api/api/v1/sub-router/users/123/posts/:postId', + transaction_info: { + source: 'route', + }, + }); + } +}); diff --git a/packages/node-integration-tests/suites/express/multiple-routers/middle-layer-parameterized/server.ts b/packages/node-integration-tests/suites/express/multiple-routers/middle-layer-parameterized/server.ts new file mode 100644 index 000000000000..19cb3c544bde --- /dev/null +++ b/packages/node-integration-tests/suites/express/multiple-routers/middle-layer-parameterized/server.ts @@ -0,0 +1,35 @@ +import * as Sentry from '@sentry/node'; +import * as Tracing from '@sentry/tracing'; +import express from 'express'; + +const app = express(); + +Sentry.init({ + dsn: 'https://public@dsn.ingest.sentry.io/1337', + release: '1.0', + // eslint-disable-next-line deprecation/deprecation + integrations: [new Sentry.Integrations.Http({ tracing: true }), new Tracing.Integrations.Express({ app })], + tracesSampleRate: 1.0, +}); + +app.use(Sentry.Handlers.requestHandler()); +app.use(Sentry.Handlers.tracingHandler()); + +const APIv1 = express.Router(); + +APIv1.use( + '/users/:userId', + APIv1.get('/posts/:postId', (_req, res) => { + Sentry.captureMessage('Custom Message'); + return res.send('Success'); + }), +); + +const root = express.Router(); + +app.use('/api/v1', APIv1); +app.use('/api', root); + +app.use(Sentry.Handlers.errorHandler()); + +export default app; diff --git a/packages/node-integration-tests/suites/express/multiple-routers/middle-layer-parameterized/test.ts b/packages/node-integration-tests/suites/express/multiple-routers/middle-layer-parameterized/test.ts new file mode 100644 index 000000000000..3527cce5c3a6 --- /dev/null +++ b/packages/node-integration-tests/suites/express/multiple-routers/middle-layer-parameterized/test.ts @@ -0,0 +1,28 @@ +import { assertSentryEvent, TestEnv } from '../../../../utils/index'; + +test('should construct correct url with multiple parameterized routers, when param is also contain in middle layer route', async () => { + const env = await TestEnv.init(__dirname, `${__dirname}/server.ts`); + const event = await env.getEnvelopeRequest({ + url: env.url.replace('test', 'api/v1/users/123/posts/456'), + envelopeType: 'transaction', + }); + + // parse node.js major version + const [major] = process.versions.node.split('.').map(Number); + // Split test result base on major node version because regex d flag is support from node 16+ + if (major >= 16) { + assertSentryEvent(event[2] as any, { + transaction: 'GET /api/v1/users/:userId/posts/:postId', + transaction_info: { + source: 'route', + }, + }); + } else { + assertSentryEvent(event[2] as any, { + transaction: 'GET /api/v1/users/123/posts/:postId', + transaction_info: { + source: 'route', + }, + }); + } +}); diff --git a/packages/node/src/requestdata.ts b/packages/node/src/requestdata.ts index bc07fcf92f8b..e746db088a95 100644 --- a/packages/node/src/requestdata.ts +++ b/packages/node/src/requestdata.ts @@ -109,7 +109,9 @@ function extractTransaction(req: PolymorphicRequest, type: boolean | Transaction } case 'methodPath': default: { - return extractPathForTransaction(req, { path: true, method: true })[0]; + // if exist _reconstructedRoute return that path instead of route.path + const customRoute = req._reconstructedRoute ? req._reconstructedRoute : undefined; + return extractPathForTransaction(req, { path: true, method: true, customRoute })[0]; } } } diff --git a/packages/tracing-internal/src/node/integrations/express.ts b/packages/tracing-internal/src/node/integrations/express.ts index 047236381269..4327f11c5ea7 100644 --- a/packages/tracing-internal/src/node/integrations/express.ts +++ b/packages/tracing-internal/src/node/integrations/express.ts @@ -64,6 +64,8 @@ type Layer = { handle_request: (req: PatchedRequest, res: ExpressResponse, next: () => void) => void; route?: { path: RouteType | RouteType[] }; path?: string; + regexp?: RegExp; + keys?: { name: string; offset: number; optional: boolean }[]; }; type RouteType = string | RegExp; @@ -318,7 +320,24 @@ function instrumentRouter(appOrRouter: ExpressRouter): void { } // Otherwise, the hardcoded path (i.e. a partial route without params) is stored in layer.path - const partialRoute = layerRoutePath || layer.path || ''; + let partialRoute; + + if (layerRoutePath) { + partialRoute = layerRoutePath; + } else { + /** + * prevent duplicate segment in _reconstructedRoute param if router match multiple routes before final path + * example: + * original url: /api/v1/1234 + * prevent: /api/api/v1/:userId + * router structure + * /api -> middleware + * /api/v1 -> middleware + * /1234 -> endpoint with param :userId + * final _reconstructedRoute is /api/v1/:userId + */ + partialRoute = preventDuplicateSegments(req.originalUrl, req._reconstructedRoute, layer.path) || ''; + } // Normalize the partial route so that it doesn't contain leading or trailing slashes // and exclude empty or '*' wildcard routes. @@ -370,6 +389,79 @@ type LayerRoutePathInfo = { numExtraSegments: number; }; +/** + * Recreate layer.route.path from layer.regexp and layer.keys. + * Works until express.js used package path-to-regexp@0.1.7 + * or until layer.keys contain offset attribute + * + * @param layer the layer to extract the stringified route from + * + * @returns string in layer.route.path structure 'router/:pathParam' or undefined + */ +export const extractOriginalRoute = ( + path?: Layer['path'], + regexp?: Layer['regexp'], + keys?: Layer['keys'], +): string | undefined => { + if (!path || !regexp || !keys || Object.keys(keys).length === 0 || !keys[0]?.offset) { + return undefined; + } + + const orderedKeys = keys.sort((a, b) => a.offset - b.offset); + + // add d flag for getting indices from regexp result + const pathRegex = new RegExp(regexp, `${regexp.flags}d`); + /** + * use custom type cause of TS error with missing indices in RegExpExecArray + */ + const execResult = pathRegex.exec(path) as (RegExpExecArray & { indices: [number, number][] }) | null; + + if (!execResult || !execResult.indices) { + return undefined; + } + /** + * remove first match from regex cause contain whole layer.path + */ + const [, ...paramIndices] = execResult.indices; + + if (paramIndices.length !== orderedKeys.length) { + return undefined; + } + let resultPath = path; + let indexShift = 0; + + /** + * iterate param matches from regexp.exec + */ + paramIndices.forEach(([startOffset, endOffset], index: number) => { + /** + * isolate part before param + */ + const substr1 = resultPath.substring(0, startOffset - indexShift); + /** + * define paramName as replacement in format :pathParam + */ + const replacement = `:${orderedKeys[index].name}`; + + /** + * isolate part after param + */ + const substr2 = resultPath.substring(endOffset - indexShift); + + /** + * recreate original path but with param replacement + */ + resultPath = substr1 + replacement + substr2; + + /** + * calculate new index shift after resultPath was modified + */ + indexShift = indexShift + (endOffset - startOffset - replacement.length); + }); + + return resultPath; +}; + /** * Extracts and stringifies the layer's route which can either be a string with parameters (`users/:id`), * a RegEx (`/test/`) or an array of strings and regexes (`['/path1', /\/path[2-5]/, /path/:id]`). Additionally @@ -382,11 +474,24 @@ type LayerRoutePathInfo = { * if the route was an array (defaults to 0). */ function getLayerRoutePathInfo(layer: Layer): LayerRoutePathInfo { - const lrp = layer.route?.path; + let lrp = layer.route?.path; const isRegex = isRegExp(lrp); const isArray = Array.isArray(lrp); + if (!lrp) { + // parse node.js major version + const [major] = process.versions.node.split('.').map(Number); + + // allow call extractOriginalRoute only if node version support Regex d flag, node 16+ + if (major >= 16) { + /** + * If lrp does not exist try to recreate original layer path from route regexp + */ + lrp = extractOriginalRoute(layer.path, layer.regexp, layer.keys); + } + } + if (!lrp) { return { isRegex, isArray, numExtraSegments: 0 }; } @@ -424,3 +529,28 @@ function getLayerRoutePathString(isArray: boolean, lrp?: RouteType | RouteType[] } return lrp && lrp.toString(); } + +/** + * remove duplicate segment contain in layerPath against reconstructedRoute, + * and return only unique segment that can be added into reconstructedRoute + */ +export function preventDuplicateSegments( + originalUrl?: string, + reconstructedRoute?: string, + layerPath?: string, +): string | undefined { + const originalUrlSplit = originalUrl?.split('/').filter(v => !!v); + let tempCounter = 0; + const currentOffset = reconstructedRoute?.split('/').filter(v => !!v).length || 0; + const result = layerPath + ?.split('/') + .filter(segment => { + if (originalUrlSplit?.[currentOffset + tempCounter] === segment) { + tempCounter += 1; + return true; + } + return false; + }) + .join('/'); + return result; +} diff --git a/packages/tracing-internal/test/node/express.test.ts b/packages/tracing-internal/test/node/express.test.ts new file mode 100644 index 000000000000..e9f4df236b33 --- /dev/null +++ b/packages/tracing-internal/test/node/express.test.ts @@ -0,0 +1,91 @@ +import { extractOriginalRoute, preventDuplicateSegments } from '../../src/node/integrations/express'; + +/** + * prevent duplicate segment in _reconstructedRoute param if router match multiple routes before final path + * example: + * original url: /api/v1/1234 + * prevent: /api/api/v1/:userId + * router structure + * /api -> middleware + * /api/v1 -> middleware + * /1234 -> endpoint with param :userId + * final _reconstructedRoute is /api/v1/:userId + */ +describe('unit Test for preventDuplicateSegments', () => { + it('should return api segment', () => { + const originalUrl = '/api/v1/1234'; + const reconstructedRoute = ''; + const layerPath = '/api'; + const result = preventDuplicateSegments(originalUrl, reconstructedRoute, layerPath); + expect(result).toBe('api'); + }); + + it('should prevent duplicate segment api', () => { + const originalUrl = '/api/v1/1234'; + const reconstructedRoute = '/api'; + const layerPath = '/api/v1'; + const result = preventDuplicateSegments(originalUrl, reconstructedRoute, layerPath); + expect(result).toBe('v1'); + }); + + it('should prevent duplicate segment v1', () => { + const originalUrl = '/api/v1/1234'; + const reconstructedRoute = '/api/v1'; + const layerPath = '/v1/1234'; + const result1 = preventDuplicateSegments(originalUrl, reconstructedRoute, layerPath); + expect(result1).toBe('1234'); + }); +}); +describe('preventDuplicateSegments should handle empty input gracefully', () => { + it('Empty input values', () => { + expect(preventDuplicateSegments()).toBeUndefined(); + }); + + it('Empty originalUrl', () => { + expect(preventDuplicateSegments('', '/api/v1/1234', '/api/api/v1/1234')).toBe(''); + }); + + it('Empty reconstructedRoute', () => { + expect(preventDuplicateSegments('/api/v1/1234', '', '/api/api/v1/1234')).toBe('api/v1/1234'); + }); + + it('Empty layerPath', () => { + expect(preventDuplicateSegments('/api/v1/1234', '/api/v1/1234', '')).toBe(''); + }); +}); + +// parse node.js major version +const [major] = process.versions.node.split('.').map(Number); +// Test this funciton only if node is 16+ because regex d flag is support from node 16+ +if (major >= 16) { + describe('extractOriginalRoute', () => { + it('should return undefined if path, regexp, or keys are missing', () => { + expect(extractOriginalRoute('/example')).toBeUndefined(); + expect(extractOriginalRoute('/example', /test/)).toBeUndefined(); + }); + + it('should return undefined if keys do not contain an offset property', () => { + const path = '/example'; + const regex = /example/; + const key = { name: 'param1', offset: 0, optional: false }; + expect(extractOriginalRoute(path, regex, [key])).toBeUndefined(); + }); + + it('should return the original route path when valid inputs are provided', () => { + const path = '/router/123'; + const regex = /^\/router\/(\d+)$/; + const keys = [{ name: 'pathParam', offset: 8, optional: false }]; + expect(extractOriginalRoute(path, regex, keys)).toBe('/router/:pathParam'); + }); + + it('should handle multiple parameters in the route', () => { + const path = '/user/42/profile/username'; + const regex = /^\/user\/(\d+)\/profile\/(\w+)$/; + const keys = [ + { name: 'userId', offset: 6, optional: false }, + { name: 'username', offset: 17, optional: false }, + ]; + expect(extractOriginalRoute(path, regex, keys)).toBe('/user/:userId/profile/:username'); + }); + }); +} diff --git a/packages/types/src/polymorphics.ts b/packages/types/src/polymorphics.ts index e6fcac8a682f..88abf7770b2c 100644 --- a/packages/types/src/polymorphics.ts +++ b/packages/types/src/polymorphics.ts @@ -75,4 +75,5 @@ type ExpressRequest = NodeRequest & { user?: { [key: string]: any; }; + _reconstructedRoute?: string; };