From 0bfa4c6d105c9370bdc2d5ff6fe5998f7a66ef05 Mon Sep 17 00:00:00 2001 From: Charly Gomez Date: Fri, 3 Jan 2025 10:26:52 +0100 Subject: [PATCH 1/5] remove addRequestDataToEvent and extractRequestData --- packages/astro/src/index.server.ts | 4 - packages/aws-serverless/src/index.ts | 4 - packages/bun/src/index.ts | 4 - packages/core/src/integrations/requestdata.ts | 14 +- packages/core/src/utils-hoist/index.ts | 4 - packages/core/src/utils-hoist/requestdata.ts | 225 +----------------- packages/google-cloud-serverless/src/index.ts | 4 - packages/node/src/index.ts | 3 +- packages/remix/src/index.server.ts | 4 - packages/solidstart/src/server/index.ts | 4 - packages/sveltekit/src/server/index.ts | 4 - 11 files changed, 4 insertions(+), 270 deletions(-) diff --git a/packages/astro/src/index.server.ts b/packages/astro/src/index.server.ts index 9152f67abf62..dabd32fce530 100644 --- a/packages/astro/src/index.server.ts +++ b/packages/astro/src/index.server.ts @@ -11,8 +11,6 @@ export { addBreadcrumb, addEventProcessor, addIntegration, - // eslint-disable-next-line deprecation/deprecation - addRequestDataToEvent, amqplibIntegration, anrIntegration, disableAnrDetectionForCallback, @@ -38,8 +36,6 @@ export { endSession, expressErrorHandler, expressIntegration, - // eslint-disable-next-line deprecation/deprecation - extractRequestData, extraErrorDataIntegration, fastifyIntegration, flush, diff --git a/packages/aws-serverless/src/index.ts b/packages/aws-serverless/src/index.ts index c40266ab59a9..1041f89243e4 100644 --- a/packages/aws-serverless/src/index.ts +++ b/packages/aws-serverless/src/index.ts @@ -42,11 +42,7 @@ export { flush, close, getSentryRelease, - // eslint-disable-next-line deprecation/deprecation - addRequestDataToEvent, DEFAULT_USER_INCLUDES, - // eslint-disable-next-line deprecation/deprecation - extractRequestData, createGetModuleFromFilename, anrIntegration, disableAnrDetectionForCallback, diff --git a/packages/bun/src/index.ts b/packages/bun/src/index.ts index 27ad993bc2fc..f9b38e595d74 100644 --- a/packages/bun/src/index.ts +++ b/packages/bun/src/index.ts @@ -62,11 +62,7 @@ export { flush, close, getSentryRelease, - // eslint-disable-next-line deprecation/deprecation - addRequestDataToEvent, DEFAULT_USER_INCLUDES, - // eslint-disable-next-line deprecation/deprecation - extractRequestData, createGetModuleFromFilename, anrIntegration, disableAnrDetectionForCallback, diff --git a/packages/core/src/integrations/requestdata.ts b/packages/core/src/integrations/requestdata.ts index 8f23912c5b58..01618f948d5f 100644 --- a/packages/core/src/integrations/requestdata.ts +++ b/packages/core/src/integrations/requestdata.ts @@ -1,10 +1,6 @@ import { defineIntegration } from '../integration'; import type { IntegrationFn } from '../types-hoist'; -import { - type AddRequestDataToEventOptions, - addNormalizedRequestDataToEvent, - addRequestDataToEvent, -} from '../utils-hoist/requestdata'; +import { type AddRequestDataToEventOptions, addNormalizedRequestDataToEvent } from '../utils-hoist/requestdata'; export type RequestDataIntegrationOptions = { /** @@ -93,13 +89,7 @@ const _requestDataIntegration = ((options: RequestDataIntegrationOptions = {}) = return event; } - // TODO(v9): Eventually we can remove this fallback branch and only rely on the normalizedRequest above - if (!request) { - return event; - } - - // eslint-disable-next-line deprecation/deprecation - return addRequestDataToEvent(event, request, addRequestDataOptions); + return event; }, }; }) satisfies IntegrationFn; diff --git a/packages/core/src/utils-hoist/index.ts b/packages/core/src/utils-hoist/index.ts index fcba59fea799..1de0f2a7142a 100644 --- a/packages/core/src/utils-hoist/index.ts +++ b/packages/core/src/utils-hoist/index.ts @@ -69,11 +69,7 @@ export { DEFAULT_USER_INCLUDES, addNormalizedRequestDataToEvent, // eslint-disable-next-line deprecation/deprecation - addRequestDataToEvent, - // eslint-disable-next-line deprecation/deprecation extractPathForTransaction, - // eslint-disable-next-line deprecation/deprecation - extractRequestData, winterCGHeadersToDict, winterCGRequestToRequestData, httpRequestToRequestData, diff --git a/packages/core/src/utils-hoist/requestdata.ts b/packages/core/src/utils-hoist/requestdata.ts index 91fe5361fd01..95a39c9b2ad1 100644 --- a/packages/core/src/utils-hoist/requestdata.ts +++ b/packages/core/src/utils-hoist/requestdata.ts @@ -1,7 +1,5 @@ -/* eslint-disable max-lines */ import type { Event, - ExtractedNodeRequestData, PolymorphicRequest, RequestEventData, TransactionSource, @@ -11,11 +9,9 @@ import type { import { parseCookie } from './cookie'; import { DEBUG_BUILD } from './debug-build'; -import { isPlainObject, isString } from './is'; +import { isPlainObject } from './is'; import { logger } from './logger'; -import { normalize } from './normalize'; import { dropUndefinedKeys } from './object'; -import { truncate } from './string'; import { stripUrlQueryAndFragment } from './url'; import { getClientIPAddress, ipHeaderNames } from './vendor/getIpAddress'; @@ -121,137 +117,6 @@ function extractUserData( return extractedUser; } -/** - * Normalize data from the request object, accounting for framework differences. - * - * @param req The request object from which to extract data - * @param options.include An optional array of keys to include in the normalized data. Defaults to - * DEFAULT_REQUEST_INCLUDES if not provided. - * @param options.deps Injected, platform-specific dependencies - * @returns An object containing normalized request data - * - * @deprecated Instead manually normalize the request data into a format that fits `addNormalizedRequestDataToEvent`. - */ -export function extractRequestData( - req: PolymorphicRequest, - options: { - include?: string[]; - } = {}, -): ExtractedNodeRequestData { - const { include = DEFAULT_REQUEST_INCLUDES } = options; - const requestData: { [key: string]: unknown } = {}; - - // headers: - // node, express, koa, nextjs: req.headers - const headers = (req.headers || {}) as typeof req.headers & { - host?: string; - cookie?: string; - }; - // method: - // node, express, koa, nextjs: req.method - const method = req.method; - // host: - // express: req.hostname in > 4 and req.host in < 4 - // koa: req.host - // node, nextjs: req.headers.host - // Express 4 mistakenly strips off port number from req.host / req.hostname so we can't rely on them - // See: https://github.com/expressjs/express/issues/3047#issuecomment-236653223 - // Also: https://github.com/getsentry/sentry-javascript/issues/1917 - const host = headers.host || req.hostname || req.host || ''; - // protocol: - // node, nextjs: - // express, koa: req.protocol - const protocol = req.protocol === 'https' || (req.socket && req.socket.encrypted) ? 'https' : 'http'; - // url (including path and query string): - // node, express: req.originalUrl - // koa, nextjs: req.url - const originalUrl = req.originalUrl || req.url || ''; - // absolute url - const absoluteUrl = originalUrl.startsWith(protocol) ? originalUrl : `${protocol}://${host}${originalUrl}`; - include.forEach(key => { - switch (key) { - case 'headers': { - requestData.headers = headers; - - // Remove the Cookie header in case cookie data should not be included in the event - if (!include.includes('cookies')) { - delete (requestData.headers as { cookie?: string }).cookie; - } - - // Remove IP headers in case IP data should not be included in the event - if (!include.includes('ip')) { - ipHeaderNames.forEach(ipHeaderName => { - // eslint-disable-next-line @typescript-eslint/no-dynamic-delete - delete (requestData.headers as Record)[ipHeaderName]; - }); - } - - break; - } - case 'method': { - requestData.method = method; - break; - } - case 'url': { - requestData.url = absoluteUrl; - break; - } - case 'cookies': { - // cookies: - // node, express, koa: req.headers.cookie - // vercel, sails.js, express (w/ cookie middleware), nextjs: req.cookies - requestData.cookies = - // TODO (v8 / #5257): We're only sending the empty object for backwards compatibility, so the last bit can - // come off in v8 - req.cookies || (headers.cookie && parseCookie(headers.cookie)) || {}; - break; - } - case 'query_string': { - // query string: - // node: req.url (raw) - // express, koa, nextjs: req.query - requestData.query_string = extractQueryParams(req); - break; - } - case 'data': { - if (method === 'GET' || method === 'HEAD') { - break; - } - // NOTE: As of v8, request is (unless a user sets this manually) ALWAYS a http request - // Which does not have a body by default - // However, in our http instrumentation, we patch the request to capture the body and store it on the - // request as `.body` anyhow - // In v9, we may update requestData to only work with plain http requests - // body data: - // express, koa, nextjs: req.body - // - // when using node by itself, you have to read the incoming stream(see - // https://nodejs.dev/learn/get-http-request-body-data-using-nodejs); if a user is doing that, we can't know - // where they're going to store the final result, so they'll have to capture this data themselves - const body = req.body; - if (body !== undefined) { - const stringBody: string = isString(body) - ? body - : isPlainObject(body) - ? JSON.stringify(normalize(body)) - : truncate(`${body}`, 1024); - if (stringBody) { - requestData.data = stringBody; - } - } - break; - } - default: { - if ({}.hasOwnProperty.call(req, key)) { - requestData[key] = (req as { [key: string]: unknown })[key]; - } - } - } - }); - - return requestData; -} - /** * Add already normalized request data to an event. * This mutates the passed in event. @@ -307,94 +172,6 @@ export function addNormalizedRequestDataToEvent( } } -/** - * Add data from the given request to the given event - * - * @param event The event to which the request data will be added - * @param req Request object - * @param options.include Flags to control what data is included - * @param options.deps Injected platform-specific dependencies - * @returns The mutated `Event` object - * - * @deprecated Use `addNormalizedRequestDataToEvent` instead. - */ -export function addRequestDataToEvent( - event: Event, - req: PolymorphicRequest, - options?: AddRequestDataToEventOptions, -): Event { - const include = { - ...DEFAULT_INCLUDES, - ...(options && options.include), - }; - - if (include.request) { - const includeRequest = Array.isArray(include.request) ? [...include.request] : [...DEFAULT_REQUEST_INCLUDES]; - if (include.ip) { - includeRequest.push('ip'); - } - - // eslint-disable-next-line deprecation/deprecation - const extractedRequestData = extractRequestData(req, { include: includeRequest }); - - event.request = { - ...event.request, - ...extractedRequestData, - }; - } - - if (include.user) { - const extractedUser = req.user && isPlainObject(req.user) ? extractUserData(req.user, include.user) : {}; - - if (Object.keys(extractedUser).length) { - event.user = { - ...event.user, - ...extractedUser, - }; - } - } - - // client ip: - // node, nextjs: req.socket.remoteAddress - // express, koa: req.ip - // It may also be sent by proxies as specified in X-Forwarded-For or similar headers - if (include.ip) { - const ip = (req.headers && getClientIPAddress(req.headers)) || req.ip || (req.socket && req.socket.remoteAddress); - if (ip) { - event.user = { - ...event.user, - ip_address: ip, - }; - } - } - - return event; -} - -function extractQueryParams(req: PolymorphicRequest): string | Record | undefined { - // url (including path and query string): - // node, express: req.originalUrl - // koa, nextjs: req.url - let originalUrl = req.originalUrl || req.url || ''; - - if (!originalUrl) { - return; - } - - // The `URL` constructor can't handle internal URLs of the form `/some/path/here`, so stick a dummy protocol and - // hostname on the beginning. Since the point here is just to grab the query string, it doesn't matter what we use. - if (originalUrl.startsWith('/')) { - originalUrl = `http://dogs.are.great${originalUrl}`; - } - - try { - const queryParams = req.query || new URL(originalUrl).search.slice(1); - return queryParams.length ? queryParams : undefined; - } catch { - return undefined; - } -} - /** * Transforms a `Headers` object that implements the `Web Fetch API` (https://developer.mozilla.org/en-US/docs/Web/API/Headers) into a simple key-value dict. * The header keys will be lower case: e.g. A "Content-Type" header will be stored as "content-type". diff --git a/packages/google-cloud-serverless/src/index.ts b/packages/google-cloud-serverless/src/index.ts index ace6ff127c3d..b9a367a09a18 100644 --- a/packages/google-cloud-serverless/src/index.ts +++ b/packages/google-cloud-serverless/src/index.ts @@ -42,11 +42,7 @@ export { flush, close, getSentryRelease, - // eslint-disable-next-line deprecation/deprecation - addRequestDataToEvent, DEFAULT_USER_INCLUDES, - // eslint-disable-next-line deprecation/deprecation - extractRequestData, createGetModuleFromFilename, anrIntegration, disableAnrDetectionForCallback, diff --git a/packages/node/src/index.ts b/packages/node/src/index.ts index 92d7a367ad3f..2e44c4f992f2 100644 --- a/packages/node/src/index.ts +++ b/packages/node/src/index.ts @@ -54,8 +54,7 @@ export { cron } from './cron'; export type { NodeOptions } from './types'; -// eslint-disable-next-line deprecation/deprecation -export { addRequestDataToEvent, DEFAULT_USER_INCLUDES, extractRequestData } from '@sentry/core'; +export { DEFAULT_USER_INCLUDES } from '@sentry/core'; export { // This needs exporting so the NodeClient can be used without calling init diff --git a/packages/remix/src/index.server.ts b/packages/remix/src/index.server.ts index 193cbd072b39..41c74a4a870d 100644 --- a/packages/remix/src/index.server.ts +++ b/packages/remix/src/index.server.ts @@ -15,8 +15,6 @@ export { addBreadcrumb, addEventProcessor, addIntegration, - // eslint-disable-next-line deprecation/deprecation - addRequestDataToEvent, amqplibIntegration, anrIntegration, disableAnrDetectionForCallback, @@ -41,8 +39,6 @@ export { endSession, expressErrorHandler, expressIntegration, - // eslint-disable-next-line deprecation/deprecation - extractRequestData, extraErrorDataIntegration, fastifyIntegration, flush, diff --git a/packages/solidstart/src/server/index.ts b/packages/solidstart/src/server/index.ts index 4963104f7d4e..d09e5c86b8c2 100644 --- a/packages/solidstart/src/server/index.ts +++ b/packages/solidstart/src/server/index.ts @@ -7,8 +7,6 @@ export { addBreadcrumb, addEventProcessor, addIntegration, - // eslint-disable-next-line deprecation/deprecation - addRequestDataToEvent, amqplibIntegration, anrIntegration, disableAnrDetectionForCallback, @@ -33,8 +31,6 @@ export { endSession, expressErrorHandler, expressIntegration, - // eslint-disable-next-line deprecation/deprecation - extractRequestData, extraErrorDataIntegration, fastifyIntegration, flush, diff --git a/packages/sveltekit/src/server/index.ts b/packages/sveltekit/src/server/index.ts index cbd4934744bf..7df24ce61384 100644 --- a/packages/sveltekit/src/server/index.ts +++ b/packages/sveltekit/src/server/index.ts @@ -7,8 +7,6 @@ export { addBreadcrumb, addEventProcessor, addIntegration, - // eslint-disable-next-line deprecation/deprecation - addRequestDataToEvent, amqplibIntegration, anrIntegration, disableAnrDetectionForCallback, @@ -33,8 +31,6 @@ export { endSession, expressErrorHandler, expressIntegration, - // eslint-disable-next-line deprecation/deprecation - extractRequestData, extraErrorDataIntegration, fastifyIntegration, flush, From 02cc019406363c2dcf74e39d3b38d451d7b757b6 Mon Sep 17 00:00:00 2001 From: Charly Gomez Date: Fri, 3 Jan 2025 10:27:07 +0100 Subject: [PATCH 2/5] update tests --- .../test/lib/integrations/requestdata.test.ts | 14 +- .../core/test/utils-hoist/requestdata.test.ts | 634 +----------------- 2 files changed, 9 insertions(+), 639 deletions(-) diff --git a/packages/core/test/lib/integrations/requestdata.test.ts b/packages/core/test/lib/integrations/requestdata.test.ts index aebd140b2bf3..27dc2f466c69 100644 --- a/packages/core/test/lib/integrations/requestdata.test.ts +++ b/packages/core/test/lib/integrations/requestdata.test.ts @@ -7,7 +7,7 @@ import { TestClient, getDefaultTestClientOptions } from '../../mocks/client'; import * as requestDataModule from '../../../src/utils-hoist/requestdata'; -const addRequestDataToEventSpy = jest.spyOn(requestDataModule, 'addRequestDataToEvent'); +const addNormalizedRequestDataToEventSpy = jest.spyOn(requestDataModule, 'addNormalizedRequestDataToEvent'); const headers = { ears: 'furry', nose: 'wet', tongue: 'spotted', cookie: 'favorite=zukes' }; const method = 'wagging'; @@ -50,7 +50,7 @@ describe('`RequestData` integration', () => { hostname, originalUrl: `${path}?${queryString}`, } as unknown as IncomingMessage; - event = { sdkProcessingMetadata: { request: req } }; + event = { sdkProcessingMetadata: { request: req, normalizedRequest: {} } }; }); afterEach(() => { @@ -62,8 +62,8 @@ describe('`RequestData` integration', () => { const requestDataEventProcessor = initWithRequestDataIntegrationOptions({ include: { ip: false, user: true } }); void requestDataEventProcessor(event, {}); - - const passedOptions = addRequestDataToEventSpy.mock.calls[0]?.[2]; + expect(addNormalizedRequestDataToEventSpy).toHaveBeenCalled(); + const passedOptions = addNormalizedRequestDataToEventSpy.mock.calls[0]?.[3]; expect(passedOptions?.include).toEqual(expect.objectContaining({ ip: false, user: true })); }); @@ -73,7 +73,7 @@ describe('`RequestData` integration', () => { void requestDataEventProcessor(event, {}); - const passedOptions = addRequestDataToEventSpy.mock.calls[0]?.[2]; + const passedOptions = addNormalizedRequestDataToEventSpy.mock.calls[0]?.[3]; expect(passedOptions?.include).toEqual(expect.objectContaining({ transaction: 'path' })); }); @@ -85,7 +85,7 @@ describe('`RequestData` integration', () => { void requestDataEventProcessor(event, {}); - const passedOptions = addRequestDataToEventSpy.mock.calls[0]?.[2]; + const passedOptions = addNormalizedRequestDataToEventSpy.mock.calls[0]?.[3]; expect(passedOptions?.include?.request).toEqual(expect.arrayContaining(['data'])); expect(passedOptions?.include?.request).not.toEqual(expect.arrayContaining(['cookies'])); @@ -98,7 +98,7 @@ describe('`RequestData` integration', () => { void requestDataEventProcessor(event, {}); - const passedOptions = addRequestDataToEventSpy.mock.calls[0]?.[2]; + const passedOptions = addNormalizedRequestDataToEventSpy.mock.calls[0]?.[3]; expect(passedOptions?.include?.user).toEqual(expect.arrayContaining(['id'])); expect(passedOptions?.include?.user).not.toEqual(expect.arrayContaining(['email'])); diff --git a/packages/core/test/utils-hoist/requestdata.test.ts b/packages/core/test/utils-hoist/requestdata.test.ts index 801aa6c4a296..ed8cdf89ce4a 100644 --- a/packages/core/test/utils-hoist/requestdata.test.ts +++ b/packages/core/test/utils-hoist/requestdata.test.ts @@ -1,638 +1,8 @@ /* eslint-disable deprecation/deprecation */ -import type * as net from 'net'; -import { addRequestDataToEvent, extractPathForTransaction, extractRequestData } from '../../src'; -import type { Event, PolymorphicRequest, TransactionSource, User } from '../../src/types-hoist'; +import { extractPathForTransaction } from '../../src'; +import type { PolymorphicRequest, TransactionSource } from '../../src/types-hoist'; import { getClientIPAddress } from '../../src/utils-hoist/vendor/getIpAddress'; -describe('addRequestDataToEvent', () => { - let mockEvent: Event; - let mockReq: { [key: string]: any }; - - beforeEach(() => { - mockEvent = {}; - mockReq = { - baseUrl: '/routerMountPath', - body: 'foo', - cookies: { test: 'test' }, - headers: { - host: 'example.org', - }, - method: 'POST', - originalUrl: '/routerMountPath/subpath/specificValue?querystringKey=querystringValue', - path: '/subpath/specificValue', - query: { - querystringKey: 'querystringValue', - }, - route: { - path: '/subpath/:parameterName', - stack: [ - { - name: 'parameterNameRouteHandler', - }, - ], - }, - url: '/subpath/specificValue?querystringKey=querystringValue', - user: { - custom_property: 'foo', - email: 'tobias@mail.com', - id: 123, - username: 'tobias', - }, - }; - }); - - describe('addRequestDataToEvent user properties', () => { - const DEFAULT_USER_KEYS = ['id', 'username', 'email']; - const CUSTOM_USER_KEYS = ['custom_property']; - - test('user only contains the default properties from the user', () => { - const parsedRequest: Event = addRequestDataToEvent(mockEvent, mockReq); - expect(Object.keys(parsedRequest.user as User)).toEqual(DEFAULT_USER_KEYS); - }); - - test('user only contains the custom properties specified in the options.user array', () => { - const optionsWithCustomUserKeys = { - include: { - user: CUSTOM_USER_KEYS, - }, - }; - - const parsedRequest: Event = addRequestDataToEvent(mockEvent, mockReq, optionsWithCustomUserKeys); - - expect(Object.keys(parsedRequest.user as User)).toEqual(CUSTOM_USER_KEYS); - }); - - test('setting user doesnt blow up when someone passes non-object value', () => { - const reqWithUser = { - ...mockReq, - // intentionally setting user to a non-object value, hence the as any cast - user: 'wat', - } as any; - - const parsedRequest: Event = addRequestDataToEvent(mockEvent, reqWithUser); - - expect(parsedRequest.user).toBeUndefined(); - }); - }); - - describe('addRequestDataToEvent ip property', () => { - test('can be extracted from req.ip', () => { - const mockReqWithIP = { - ...mockReq, - ip: '123', - }; - const optionsWithIP = { - include: { - ip: true, - }, - }; - - const parsedRequest: Event = addRequestDataToEvent(mockEvent, mockReqWithIP, optionsWithIP); - - expect(parsedRequest.user!.ip_address).toEqual('123'); - }); - - test('can extract from req.socket.remoteAddress', () => { - const reqWithIPInSocket = { - ...mockReq, - socket: { - remoteAddress: '321', - } as net.Socket, - }; - const optionsWithIP = { - include: { - ip: true, - }, - }; - - const parsedRequest: Event = addRequestDataToEvent(mockEvent, reqWithIPInSocket, optionsWithIP); - - expect(parsedRequest.user!.ip_address).toEqual('321'); - }); - - test.each([ - 'X-Client-IP', - 'X-Forwarded-For', - 'Fly-Client-IP', - 'CF-Connecting-IP', - 'Fastly-Client-Ip', - 'True-Client-Ip', - 'X-Real-IP', - 'X-Cluster-Client-IP', - 'X-Forwarded', - 'Forwarded-For', - 'X-Vercel-Forwarded-For', - ])('can be extracted from %s header', headerName => { - const reqWithIPInHeader = { - ...mockReq, - headers: { - [headerName]: '123.5.6.1', - }, - }; - - const optionsWithIP = { - include: { - ip: true, - }, - }; - - const parsedRequest: Event = addRequestDataToEvent(mockEvent, reqWithIPInHeader, optionsWithIP); - - expect(parsedRequest.user!.ip_address).toEqual('123.5.6.1'); - }); - - it('can be extracted from Forwarded header', () => { - const reqWithIPInHeader = { - ...mockReq, - headers: { - Forwarded: 'by=111;for=123.5.6.1;for=123.5.6.2;', - }, - }; - - const optionsWithIP = { - include: { - ip: true, - }, - }; - - const parsedRequest: Event = addRequestDataToEvent(mockEvent, reqWithIPInHeader, optionsWithIP); - - expect(parsedRequest.user!.ip_address).toEqual('123.5.6.1'); - }); - - test('it ignores invalid IP in header', () => { - const reqWithIPInHeader = { - ...mockReq, - headers: { - 'X-Client-IP': 'invalid', - }, - }; - - const optionsWithIP = { - include: { - ip: true, - }, - }; - - const parsedRequest: Event = addRequestDataToEvent(mockEvent, reqWithIPInHeader, optionsWithIP); - - expect(parsedRequest.user!.ip_address).toEqual(undefined); - }); - - test('IP from header takes presedence over socket', () => { - const reqWithIPInHeader = { - ...mockReq, - headers: { - 'X-Client-IP': '123.5.6.1', - }, - socket: { - remoteAddress: '321', - } as net.Socket, - }; - - const optionsWithIP = { - include: { - ip: true, - }, - }; - - const parsedRequest: Event = addRequestDataToEvent(mockEvent, reqWithIPInHeader, optionsWithIP); - - expect(parsedRequest.user!.ip_address).toEqual('123.5.6.1'); - }); - - test('IP from header takes presedence over req.ip', () => { - const reqWithIPInHeader = { - ...mockReq, - headers: { - 'X-Client-IP': '123.5.6.1', - }, - ip: '123', - }; - - const optionsWithIP = { - include: { - ip: true, - }, - }; - - const parsedRequest: Event = addRequestDataToEvent(mockEvent, reqWithIPInHeader, optionsWithIP); - - expect(parsedRequest.user!.ip_address).toEqual('123.5.6.1'); - }); - - test('does not add IP if ip=false', () => { - const reqWithIPInHeader = { - ...mockReq, - headers: { - 'X-Client-IP': '123.5.6.1', - }, - ip: '123', - }; - - const optionsWithoutIP = { - include: { - ip: false, - }, - }; - - const parsedRequest: Event = addRequestDataToEvent(mockEvent, reqWithIPInHeader, optionsWithoutIP); - - expect(parsedRequest.user!.ip_address).toEqual(undefined); - }); - - test('does not add IP by default', () => { - const reqWithIPInHeader = { - ...mockReq, - headers: { - 'X-Client-IP': '123.5.6.1', - }, - ip: '123', - }; - - const optionsWithoutIP = {}; - - const parsedRequest: Event = addRequestDataToEvent(mockEvent, reqWithIPInHeader, optionsWithoutIP); - - expect(parsedRequest.user!.ip_address).toEqual(undefined); - }); - - test('removes IP headers if `ip` is not set in the options', () => { - const reqWithIPInHeader = { - ...mockReq, - headers: { - otherHeader: 'hello', - 'X-Client-IP': '123', - 'X-Forwarded-For': '123', - 'Fly-Client-IP': '123', - 'CF-Connecting-IP': '123', - 'Fastly-Client-Ip': '123', - 'True-Client-Ip': '123', - 'X-Real-IP': '123', - 'X-Cluster-Client-IP': '123', - 'X-Forwarded': '123', - 'Forwarded-For': '123', - Forwarded: '123', - 'X-Vercel-Forwarded-For': '123', - }, - }; - - const optionsWithoutIP = { - include: {}, - }; - - const parsedRequest: Event = addRequestDataToEvent(mockEvent, reqWithIPInHeader, optionsWithoutIP); - - expect(parsedRequest.request?.headers).toEqual({ otherHeader: 'hello' }); - }); - - test('keeps IP headers if `ip=true`', () => { - const reqWithIPInHeader = { - ...mockReq, - headers: { - otherHeader: 'hello', - 'X-Client-IP': '123', - 'X-Forwarded-For': '123', - 'Fly-Client-IP': '123', - 'CF-Connecting-IP': '123', - 'Fastly-Client-Ip': '123', - 'True-Client-Ip': '123', - 'X-Real-IP': '123', - 'X-Cluster-Client-IP': '123', - 'X-Forwarded': '123', - 'Forwarded-For': '123', - Forwarded: '123', - 'X-Vercel-Forwarded-For': '123', - }, - }; - - const optionsWithoutIP = { - include: { - ip: true, - }, - }; - - const parsedRequest: Event = addRequestDataToEvent(mockEvent, reqWithIPInHeader, optionsWithoutIP); - - expect(parsedRequest.request?.headers).toEqual({ - otherHeader: 'hello', - 'X-Client-IP': '123', - 'X-Forwarded-For': '123', - 'Fly-Client-IP': '123', - 'CF-Connecting-IP': '123', - 'Fastly-Client-Ip': '123', - 'True-Client-Ip': '123', - 'X-Real-IP': '123', - 'X-Cluster-Client-IP': '123', - 'X-Forwarded': '123', - 'Forwarded-For': '123', - Forwarded: '123', - 'X-Vercel-Forwarded-For': '123', - }); - }); - }); - - describe('request properties', () => { - test('request only contains the default set of properties from the request', () => { - const DEFAULT_REQUEST_PROPERTIES = ['cookies', 'data', 'headers', 'method', 'query_string', 'url']; - - const parsedRequest: Event = addRequestDataToEvent(mockEvent, mockReq); - - expect(Object.keys(parsedRequest.request!)).toEqual(DEFAULT_REQUEST_PROPERTIES); - }); - - test('request only contains the specified properties in the options.request array', () => { - const INCLUDED_PROPERTIES = ['data', 'headers', 'query_string', 'url']; - const optionsWithRequestIncludes = { - include: { - request: INCLUDED_PROPERTIES, - }, - }; - - const parsedRequest: Event = addRequestDataToEvent(mockEvent, mockReq, optionsWithRequestIncludes); - - expect(Object.keys(parsedRequest.request!)).toEqual(INCLUDED_PROPERTIES); - }); - - test.each([ - [undefined, true], - ['GET', false], - ['HEAD', false], - ])('request skips `body` property for GET and HEAD requests - %s method', (method, shouldIncludeBodyData) => { - const reqWithMethod = { ...mockReq, method }; - - const parsedRequest: Event = addRequestDataToEvent(mockEvent, reqWithMethod); - - if (shouldIncludeBodyData) { - expect(parsedRequest.request).toHaveProperty('data'); - } else { - expect(parsedRequest.request).not.toHaveProperty('data'); - } - }); - }); -}); - -describe('extractRequestData', () => { - describe('default behaviour', () => { - test('node', () => { - const mockReq = { - headers: { host: 'example.com' }, - method: 'GET', - socket: { encrypted: true }, - originalUrl: '/', - }; - - expect(extractRequestData(mockReq)).toEqual({ - cookies: {}, - headers: { - host: 'example.com', - }, - method: 'GET', - query_string: undefined, - url: 'https://example.com/', - }); - }); - - test('degrades gracefully without request data', () => { - const mockReq = {}; - - expect(extractRequestData(mockReq)).toEqual({ - cookies: {}, - headers: {}, - method: undefined, - query_string: undefined, - url: 'http://', - }); - }); - }); - - describe('headers', () => { - it('removes the `Cookie` header from requestdata.headers, if `cookies` is not set in the options', () => { - const mockReq = { - cookies: { foo: 'bar' }, - headers: { cookie: 'foo=bar', otherHeader: 'hello' }, - }; - const options = { include: ['headers'] }; - - expect(extractRequestData(mockReq, options)).toStrictEqual({ - headers: { otherHeader: 'hello' }, - }); - }); - - it('includes the `Cookie` header in requestdata.headers, if `cookies` is set in the options', () => { - const mockReq = { - cookies: { foo: 'bar' }, - headers: { cookie: 'foo=bar', otherHeader: 'hello' }, - }; - const optionsWithCookies = { include: ['headers', 'cookies'] }; - - expect(extractRequestData(mockReq, optionsWithCookies)).toStrictEqual({ - headers: { otherHeader: 'hello', cookie: 'foo=bar' }, - cookies: { foo: 'bar' }, - }); - }); - - it('removes IP-related headers from requestdata.headers, if `ip` is not set in the options', () => { - const mockReq = { - headers: { - otherHeader: 'hello', - 'X-Client-IP': '123', - 'X-Forwarded-For': '123', - 'Fly-Client-IP': '123', - 'CF-Connecting-IP': '123', - 'Fastly-Client-Ip': '123', - 'True-Client-Ip': '123', - 'X-Real-IP': '123', - 'X-Cluster-Client-IP': '123', - 'X-Forwarded': '123', - 'Forwarded-For': '123', - Forwarded: '123', - 'X-Vercel-Forwarded-For': '123', - }, - }; - const options = { include: ['headers'] }; - - expect(extractRequestData(mockReq, options)).toStrictEqual({ - headers: { otherHeader: 'hello' }, - }); - }); - - it('keeps IP-related headers from requestdata.headers, if `ip` is enabled in options', () => { - const mockReq = { - headers: { - otherHeader: 'hello', - 'X-Client-IP': '123', - 'X-Forwarded-For': '123', - 'Fly-Client-IP': '123', - 'CF-Connecting-IP': '123', - 'Fastly-Client-Ip': '123', - 'True-Client-Ip': '123', - 'X-Real-IP': '123', - 'X-Cluster-Client-IP': '123', - 'X-Forwarded': '123', - 'Forwarded-For': '123', - Forwarded: '123', - 'X-Vercel-Forwarded-For': '123', - }, - }; - const options = { include: ['headers', 'ip'] }; - - expect(extractRequestData(mockReq, options)).toStrictEqual({ - headers: { - otherHeader: 'hello', - 'X-Client-IP': '123', - 'X-Forwarded-For': '123', - 'Fly-Client-IP': '123', - 'CF-Connecting-IP': '123', - 'Fastly-Client-Ip': '123', - 'True-Client-Ip': '123', - 'X-Real-IP': '123', - 'X-Cluster-Client-IP': '123', - 'X-Forwarded': '123', - 'Forwarded-For': '123', - Forwarded: '123', - 'X-Vercel-Forwarded-For': '123', - }, - }); - }); - }); - - describe('cookies', () => { - it('uses `req.cookies` if available', () => { - const mockReq = { - cookies: { foo: 'bar' }, - }; - const optionsWithCookies = { include: ['cookies'] }; - - expect(extractRequestData(mockReq, optionsWithCookies)).toEqual({ - cookies: { foo: 'bar' }, - }); - }); - - it('parses the cookie header', () => { - const mockReq = { - headers: { - cookie: 'foo=bar;', - }, - }; - const optionsWithCookies = { include: ['cookies'] }; - - expect(extractRequestData(mockReq, optionsWithCookies)).toEqual({ - cookies: { foo: 'bar' }, - }); - }); - - it('falls back if no cookies are defined', () => { - const mockReq = {}; - const optionsWithCookies = { include: ['cookies'] }; - - expect(extractRequestData(mockReq, optionsWithCookies)).toEqual({ - cookies: {}, - }); - }); - }); - - describe('data', () => { - it('includes data from `req.body` if available', () => { - const mockReq = { - method: 'POST', - headers: { 'Content-Type': 'application/x-www-form-urlencoded' }, - body: 'foo=bar', - }; - const optionsWithData = { include: ['data'] }; - - expect(extractRequestData(mockReq, optionsWithData)).toEqual({ - data: 'foo=bar', - }); - }); - - it('encodes JSON body contents back to a string', () => { - const mockReq = { - method: 'POST', - headers: { 'Content-Type': 'application/json' }, - body: { foo: 'bar' }, - }; - const optionsWithData = { include: ['data'] }; - - expect(extractRequestData(mockReq, optionsWithData)).toEqual({ - data: '{"foo":"bar"}', - }); - }); - }); - - describe('query_string', () => { - it('parses the query parms from the url', () => { - const mockReq = { - headers: { host: 'example.com' }, - secure: true, - originalUrl: '/?foo=bar', - }; - const optionsWithQueryString = { include: ['query_string'] }; - - expect(extractRequestData(mockReq, optionsWithQueryString)).toEqual({ - query_string: 'foo=bar', - }); - }); - - it('gracefully degrades if url cannot be determined', () => { - const mockReq = {}; - const optionsWithQueryString = { include: ['query_string'] }; - - expect(extractRequestData(mockReq, optionsWithQueryString)).toEqual({ - query_string: undefined, - }); - }); - }); - - describe('url', () => { - test('express/koa', () => { - const mockReq = { - host: 'example.com', - protocol: 'https', - url: '/', - }; - const optionsWithURL = { include: ['url'] }; - - expect(extractRequestData(mockReq, optionsWithURL)).toEqual({ - url: 'https://example.com/', - }); - }); - - test('node', () => { - const mockReq = { - headers: { host: 'example.com' }, - socket: { encrypted: true }, - originalUrl: '/', - }; - const optionsWithURL = { include: ['url'] }; - - expect(extractRequestData(mockReq, optionsWithURL)).toEqual({ - url: 'https://example.com/', - }); - }); - }); - - describe('custom key', () => { - it('includes the custom key if present', () => { - const mockReq = { - httpVersion: '1.1', - } as any; - const optionsWithCustomKey = { include: ['httpVersion'] }; - - expect(extractRequestData(mockReq, optionsWithCustomKey)).toEqual({ - httpVersion: '1.1', - }); - }); - - it('gracefully degrades if the custom key is missing', () => { - const mockReq = {} as any; - const optionsWithCustomKey = { include: ['httpVersion'] }; - - expect(extractRequestData(mockReq, optionsWithCustomKey)).toEqual({}); - }); - }); -}); - describe('extractPathForTransaction', () => { it.each([ [ From a4c1a5c9b040e69664f6e885f8ab623e40d223ae Mon Sep 17 00:00:00 2001 From: Charly Gomez Date: Fri, 3 Jan 2025 10:27:35 +0100 Subject: [PATCH 3/5] add migration entry --- docs/migration/v8-to-v9.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/docs/migration/v8-to-v9.md b/docs/migration/v8-to-v9.md index 0ee920fed2c9..3ce0a6119344 100644 --- a/docs/migration/v8-to-v9.md +++ b/docs/migration/v8-to-v9.md @@ -144,6 +144,8 @@ Sentry.init({ - The `Request` type has been removed. Use `RequestEventData` type instead. - The `TransactionNamingScheme` type has been removed. There is no replacement. - The `memoBuilder` method has been removed. There is no replacement. +- The `extractRequestData` method has been removed. Manually extract relevant data off request instead. +- The `addRequestDataToEvent` method has been removed. Use `addNormalizedRequestDataToEvent` instead. ### `@sentry/browser` From 071d118cc25869632cc4ea8bb10c639cf5fe7859 Mon Sep 17 00:00:00 2001 From: Charly Gomez Date: Fri, 3 Jan 2025 10:55:27 +0100 Subject: [PATCH 4/5] remove transactionNamingScheme --- packages/core/src/integrations/requestdata.ts | 9 --------- .../core/test/lib/integrations/requestdata.test.ts | 10 ---------- 2 files changed, 19 deletions(-) diff --git a/packages/core/src/integrations/requestdata.ts b/packages/core/src/integrations/requestdata.ts index 01618f948d5f..73bc03c173f5 100644 --- a/packages/core/src/integrations/requestdata.ts +++ b/packages/core/src/integrations/requestdata.ts @@ -21,12 +21,6 @@ export type RequestDataIntegrationOptions = { email?: boolean; }; }; - - /** - * Whether to identify transactions by parameterized path, parameterized path with method, or handler name. - * @deprecated This option does not do anything anymore, and will be removed in v9. - */ - transactionNamingScheme?: 'path' | 'methodPath' | 'handler'; }; const DEFAULT_OPTIONS = { @@ -106,8 +100,6 @@ function convertReqDataIntegrationOptsToAddReqDataOpts( integrationOptions: Required, ): AddRequestDataToEventOptions { const { - // eslint-disable-next-line deprecation/deprecation - transactionNamingScheme, include: { ip, user, ...requestOptions }, } = integrationOptions; @@ -138,7 +130,6 @@ function convertReqDataIntegrationOptsToAddReqDataOpts( ip, user: addReqDataUserOpt, request: requestIncludeKeys.length !== 0 ? requestIncludeKeys : undefined, - transaction: transactionNamingScheme, }, }; } diff --git a/packages/core/test/lib/integrations/requestdata.test.ts b/packages/core/test/lib/integrations/requestdata.test.ts index 27dc2f466c69..406137ca1308 100644 --- a/packages/core/test/lib/integrations/requestdata.test.ts +++ b/packages/core/test/lib/integrations/requestdata.test.ts @@ -68,16 +68,6 @@ describe('`RequestData` integration', () => { expect(passedOptions?.include).toEqual(expect.objectContaining({ ip: false, user: true })); }); - it('moves `transactionNamingScheme` to `transaction` include', () => { - const requestDataEventProcessor = initWithRequestDataIntegrationOptions({ transactionNamingScheme: 'path' }); - - void requestDataEventProcessor(event, {}); - - const passedOptions = addNormalizedRequestDataToEventSpy.mock.calls[0]?.[3]; - - expect(passedOptions?.include).toEqual(expect.objectContaining({ transaction: 'path' })); - }); - it('moves `true` request keys into `request` include, but omits `false` ones', async () => { const requestDataEventProcessor = initWithRequestDataIntegrationOptions({ include: { data: true, cookies: false }, From b080a5301c7d36bfbeba64337850dccac7f48296 Mon Sep 17 00:00:00 2001 From: Charly Gomez Date: Fri, 3 Jan 2025 11:06:46 +0100 Subject: [PATCH 5/5] remove extractPathForTransaction --- docs/migration/v8-to-v9.md | 1 + packages/core/src/utils-hoist/index.ts | 2 - packages/core/src/utils-hoist/requestdata.ts | 62 +---------- .../core/test/utils-hoist/requestdata.test.ts | 101 ------------------ 4 files changed, 2 insertions(+), 164 deletions(-) diff --git a/docs/migration/v8-to-v9.md b/docs/migration/v8-to-v9.md index 3ce0a6119344..b78478db8e4f 100644 --- a/docs/migration/v8-to-v9.md +++ b/docs/migration/v8-to-v9.md @@ -146,6 +146,7 @@ Sentry.init({ - The `memoBuilder` method has been removed. There is no replacement. - The `extractRequestData` method has been removed. Manually extract relevant data off request instead. - The `addRequestDataToEvent` method has been removed. Use `addNormalizedRequestDataToEvent` instead. +- The `extractPathForTransaction` method has been removed. There is no replacement. ### `@sentry/browser` diff --git a/packages/core/src/utils-hoist/index.ts b/packages/core/src/utils-hoist/index.ts index 1de0f2a7142a..6f01bc13a992 100644 --- a/packages/core/src/utils-hoist/index.ts +++ b/packages/core/src/utils-hoist/index.ts @@ -68,8 +68,6 @@ export type { PromiseBuffer } from './promisebuffer'; export { DEFAULT_USER_INCLUDES, addNormalizedRequestDataToEvent, - // eslint-disable-next-line deprecation/deprecation - extractPathForTransaction, winterCGHeadersToDict, winterCGRequestToRequestData, httpRequestToRequestData, diff --git a/packages/core/src/utils-hoist/requestdata.ts b/packages/core/src/utils-hoist/requestdata.ts index 95a39c9b2ad1..60d83e218c10 100644 --- a/packages/core/src/utils-hoist/requestdata.ts +++ b/packages/core/src/utils-hoist/requestdata.ts @@ -1,18 +1,10 @@ -import type { - Event, - PolymorphicRequest, - RequestEventData, - TransactionSource, - WebFetchHeaders, - WebFetchRequest, -} from '../types-hoist'; +import type { Event, PolymorphicRequest, RequestEventData, WebFetchHeaders, WebFetchRequest } from '../types-hoist'; import { parseCookie } from './cookie'; import { DEBUG_BUILD } from './debug-build'; import { isPlainObject } from './is'; import { logger } from './logger'; import { dropUndefinedKeys } from './object'; -import { stripUrlQueryAndFragment } from './url'; import { getClientIPAddress, ipHeaderNames } from './vendor/getIpAddress'; const DEFAULT_INCLUDES = { @@ -31,9 +23,6 @@ export type AddRequestDataToEventOptions = { include?: { ip?: boolean; request?: boolean | Array<(typeof DEFAULT_REQUEST_INCLUDES)[number]>; - /** @deprecated This option will be removed in v9. It does not do anything anymore, the `transcation` is set in other places. */ - // eslint-disable-next-line deprecation/deprecation - transaction?: boolean | 'path' | 'methodPath' | 'handler'; user?: boolean | Array<(typeof DEFAULT_USER_INCLUDES)[number]>; }; @@ -50,55 +39,6 @@ export type AddRequestDataToEventOptions = { }; }; -/** - * Extracts a complete and parameterized path from the request object and uses it to construct transaction name. - * If the parameterized transaction name cannot be extracted, we fall back to the raw URL. - * - * Additionally, this function determines and returns the transaction name source - * - * eg. GET /mountpoint/user/:id - * - * @param req A request object - * @param options What to include in the transaction name (method, path, or a custom route name to be - * used instead of the request's route) - * - * @returns A tuple of the fully constructed transaction name [0] and its source [1] (can be either 'route' or 'url') - * @deprecated This method will be removed in v9. It is not in use anymore. - */ -export function extractPathForTransaction( - req: PolymorphicRequest, - options: { path?: boolean; method?: boolean; customRoute?: string } = {}, -): [string, TransactionSource] { - const method = req.method && req.method.toUpperCase(); - - let path = ''; - let source: TransactionSource = 'url'; - - // Check to see if there's a parameterized route we can use (as there is in Express) - if (options.customRoute || req.route) { - path = options.customRoute || `${req.baseUrl || ''}${req.route && req.route.path}`; - source = 'route'; - } - - // Otherwise, just take the original URL - else if (req.originalUrl || req.url) { - path = stripUrlQueryAndFragment(req.originalUrl || req.url || ''); - } - - let name = ''; - if (options.method && method) { - name += method; - } - if (options.method && options.path) { - name += ' '; - } - if (options.path && path) { - name += path; - } - - return [name, source]; -} - function extractUserData( user: { [key: string]: unknown; diff --git a/packages/core/test/utils-hoist/requestdata.test.ts b/packages/core/test/utils-hoist/requestdata.test.ts index ed8cdf89ce4a..e950fe7d5357 100644 --- a/packages/core/test/utils-hoist/requestdata.test.ts +++ b/packages/core/test/utils-hoist/requestdata.test.ts @@ -1,106 +1,5 @@ -/* eslint-disable deprecation/deprecation */ -import { extractPathForTransaction } from '../../src'; -import type { PolymorphicRequest, TransactionSource } from '../../src/types-hoist'; import { getClientIPAddress } from '../../src/utils-hoist/vendor/getIpAddress'; -describe('extractPathForTransaction', () => { - it.each([ - [ - 'extracts a parameterized route and method if available', - { - method: 'get', - baseUrl: '/api/users', - route: { path: '/:id/details' }, - originalUrl: '/api/users/123/details', - } as PolymorphicRequest, - { path: true, method: true }, - 'GET /api/users/:id/details', - 'route' as TransactionSource, - ], - [ - 'ignores the method if specified', - { - method: 'get', - baseUrl: '/api/users', - route: { path: '/:id/details' }, - originalUrl: '/api/users/123/details', - } as PolymorphicRequest, - { path: true, method: false }, - '/api/users/:id/details', - 'route' as TransactionSource, - ], - [ - 'ignores the path if specified', - { - method: 'get', - baseUrl: '/api/users', - route: { path: '/:id/details' }, - originalUrl: '/api/users/123/details', - } as PolymorphicRequest, - { path: false, method: true }, - 'GET', - 'route' as TransactionSource, - ], - [ - 'returns an empty string if everything should be ignored', - { - method: 'get', - baseUrl: '/api/users', - route: { path: '/:id/details' }, - originalUrl: '/api/users/123/details', - } as PolymorphicRequest, - { path: false, method: false }, - '', - 'route' as TransactionSource, - ], - [ - 'falls back to the raw URL if no parameterized route is available', - { - method: 'get', - baseUrl: '/api/users', - originalUrl: '/api/users/123/details', - } as PolymorphicRequest, - { path: true, method: true }, - 'GET /api/users/123/details', - 'url' as TransactionSource, - ], - ])( - '%s', - ( - _: string, - req: PolymorphicRequest, - options: { path?: boolean; method?: boolean }, - expectedRoute: string, - expectedSource: TransactionSource, - ) => { - // eslint-disable-next-line deprecation/deprecation - const [route, source] = extractPathForTransaction(req, options); - - expect(route).toEqual(expectedRoute); - expect(source).toEqual(expectedSource); - }, - ); - - it('overrides the requests information with a custom route if specified', () => { - const req = { - method: 'get', - baseUrl: '/api/users', - route: { path: '/:id/details' }, - originalUrl: '/api/users/123/details', - } as PolymorphicRequest; - - // eslint-disable-next-line deprecation/deprecation - const [route, source] = extractPathForTransaction(req, { - path: true, - method: true, - customRoute: '/other/path/:id/details', - }); - - expect(route).toEqual('GET /other/path/:id/details'); - expect(source).toEqual('route'); - }); -}); - describe('getClientIPAddress', () => { it.each([ [