diff --git a/packages/node/src/integrations/requestdata.ts b/packages/node/src/integrations/requestdata.ts index 472caf88fd43..0d9d7d92662e 100644 --- a/packages/node/src/integrations/requestdata.ts +++ b/packages/node/src/integrations/requestdata.ts @@ -1,44 +1,36 @@ // TODO (v8 or v9): Whenever this becomes a default integration for `@sentry/browser`, move this to `@sentry/core`. For // now, we leave it in `@sentry/integrations` so that it doesn't contribute bytes to our CDN bundles. -import { EventProcessor, Hub, Integration, Transaction } from '@sentry/types'; +import { Event, EventProcessor, Hub, Integration, PolymorphicRequest, Transaction } from '@sentry/types'; import { extractPathForTransaction } from '@sentry/utils'; -import { - addRequestDataToEvent, - AddRequestDataToEventOptions, - DEFAULT_USER_INCLUDES, - TransactionNamingScheme, -} from '../requestdata'; +import { addRequestDataToEvent, AddRequestDataToEventOptions, TransactionNamingScheme } from '../requestdata'; -type RequestDataOptions = { +export type RequestDataIntegrationOptions = { /** * Controls what data is pulled from the request and added to the event */ - include: { + include?: { cookies?: boolean; data?: boolean; headers?: boolean; ip?: boolean; query_string?: boolean; url?: boolean; - user?: boolean | Array; + user?: + | boolean + | { + id?: boolean; + username?: boolean; + email?: boolean; + }; }; /** Whether to identify transactions by parameterized path, parameterized path with method, or handler name */ - transactionNamingScheme: TransactionNamingScheme; - - /** - * Function for adding request data to event. Defaults to `addRequestDataToEvent` from `@sentry/node` for now, but - * left injectable so this integration can be moved to `@sentry/core` and used in browser-based SDKs in the future. - * - * @hidden - */ - addRequestData: typeof addRequestDataToEvent; + transactionNamingScheme?: TransactionNamingScheme; }; const DEFAULT_OPTIONS = { - addRequestData: addRequestDataToEvent, include: { cookies: true, data: true, @@ -46,7 +38,11 @@ const DEFAULT_OPTIONS = { ip: false, query_string: true, url: true, - user: DEFAULT_USER_INCLUDES, + user: { + id: true, + username: true, + email: true, + }, }, transactionNamingScheme: 'methodpath', }; @@ -64,12 +60,20 @@ export class RequestData implements Integration { */ public name: string = RequestData.id; - private _options: RequestDataOptions; + /** + * Function for adding request data to event. Defaults to `addRequestDataToEvent` from `@sentry/node` for now, but + * left as a property so this integration can be moved to `@sentry/core` as a base class in case we decide to use + * something similar in browser-based SDKs in the future. + */ + protected _addRequestData: (event: Event, req: PolymorphicRequest, options?: { [key: string]: unknown }) => Event; + + private _options: Required; /** * @inheritDoc */ - public constructor(options: Partial = {}) { + public constructor(options: RequestDataIntegrationOptions = {}) { + this._addRequestData = addRequestDataToEvent; this._options = { ...DEFAULT_OPTIONS, ...options, @@ -79,6 +83,14 @@ export class RequestData implements Integration { method: true, ...DEFAULT_OPTIONS.include, ...options.include, + user: + options.include && typeof options.include.user === 'boolean' + ? options.include.user + : { + ...DEFAULT_OPTIONS.include.user, + // Unclear why TS still thinks `options.include.user` could be a boolean at this point + ...((options.include || {}).user as Record), + }, }, }; } @@ -91,7 +103,7 @@ export class RequestData implements Integration { // the moment it lives here, though, until https://github.com/getsentry/sentry-javascript/issues/5718 is addressed. // (TL;DR: Those functions touch many parts of the repo in many different ways, and need to be clened up. Once // that's happened, it will be easier to add this logic in without worrying about unexpected side effects.) - const { include, addRequestData, transactionNamingScheme } = this._options; + const { transactionNamingScheme } = this._options; addGlobalEventProcessor(event => { const hub = getCurrentHub(); @@ -104,7 +116,9 @@ export class RequestData implements Integration { return event; } - const processedEvent = addRequestData(event, req, { include: formatIncludeOption(include) }); + const addRequestDataOptions = convertReqDataIntegrationOptsToAddReqDataOpts(this._options); + + const processedEvent = this._addRequestData(event, req, addRequestDataOptions); // Transaction events already have the right `transaction` value if (event.type === 'transaction' || transactionNamingScheme === 'handler') { @@ -138,12 +152,15 @@ export class RequestData implements Integration { } } -/** Convert `include` option to match what `addRequestDataToEvent` expects */ +/** Convert this integration's options to match what `addRequestDataToEvent` expects */ /** TODO: Can possibly be deleted once https://github.com/getsentry/sentry-javascript/issues/5718 is fixed */ -function formatIncludeOption( - integrationInclude: RequestDataOptions['include'] = {}, -): AddRequestDataToEventOptions['include'] { - const { ip, user, ...requestOptions } = integrationInclude; +function convertReqDataIntegrationOptsToAddReqDataOpts( + integrationOptions: Required, +): AddRequestDataToEventOptions { + const { + transactionNamingScheme, + include: { ip, user, ...requestOptions }, + } = integrationOptions; const requestIncludeKeys: string[] = []; for (const [key, value] of Object.entries(requestOptions)) { @@ -152,10 +169,28 @@ function formatIncludeOption( } } + let addReqDataUserOpt; + if (user === undefined) { + addReqDataUserOpt = true; + } else if (typeof user === 'boolean') { + addReqDataUserOpt = user; + } else { + const userIncludeKeys: string[] = []; + for (const [key, value] of Object.entries(user)) { + if (value) { + userIncludeKeys.push(key); + } + } + addReqDataUserOpt = userIncludeKeys; + } + return { - ip, - user, - request: requestIncludeKeys.length !== 0 ? requestIncludeKeys : undefined, + include: { + ip, + user: addReqDataUserOpt, + request: requestIncludeKeys.length !== 0 ? requestIncludeKeys : undefined, + transaction: transactionNamingScheme, + }, }; } diff --git a/packages/node/src/requestdata.ts b/packages/node/src/requestdata.ts index 8df2e4558a76..ace3ee5e7526 100644 --- a/packages/node/src/requestdata.ts +++ b/packages/node/src/requestdata.ts @@ -15,7 +15,7 @@ export const DEFAULT_USER_INCLUDES = ['id', 'username', 'email']; /** * Options deciding what parts of the request to use when enhancing an event */ -export interface AddRequestDataToEventOptions { +export type AddRequestDataToEventOptions = { /** Flags controlling whether each type of data should be added to the event */ include?: { ip?: boolean; @@ -23,7 +23,7 @@ export interface AddRequestDataToEventOptions { transaction?: boolean | TransactionNamingScheme; user?: boolean | Array; }; -} +}; export type TransactionNamingScheme = 'path' | 'methodPath' | 'handler'; diff --git a/packages/node/test/integrations/requestdata.test.ts b/packages/node/test/integrations/requestdata.test.ts new file mode 100644 index 000000000000..e1fbaa88b556 --- /dev/null +++ b/packages/node/test/integrations/requestdata.test.ts @@ -0,0 +1,103 @@ +import { getCurrentHub, Hub, makeMain } from '@sentry/core'; +import { Event, EventProcessor } from '@sentry/types'; +import * as http from 'http'; + +import { NodeClient } from '../../src/client'; +import { RequestData, RequestDataIntegrationOptions } from '../../src/integrations/requestdata'; +import * as requestDataModule from '../../src/requestdata'; +import { getDefaultNodeClientOptions } from '../helper/node-client-options'; + +const addRequestDataToEventSpy = jest.spyOn(requestDataModule, 'addRequestDataToEvent'); +const requestDataEventProcessor = jest.fn(); + +const headers = { ears: 'furry', nose: 'wet', tongue: 'spotted', cookie: 'favorite=zukes' }; +const method = 'wagging'; +const protocol = 'mutualsniffing'; +const hostname = 'the.dog.park'; +const path = '/by/the/trees/'; +const queryString = 'chase=me&please=thankyou'; + +function initWithRequestDataIntegrationOptions(integrationOptions: RequestDataIntegrationOptions): void { + const setMockEventProcessor = (eventProcessor: EventProcessor) => + requestDataEventProcessor.mockImplementationOnce(eventProcessor); + + const requestDataIntegration = new RequestData({ + ...integrationOptions, + }); + + const client = new NodeClient( + getDefaultNodeClientOptions({ + dsn: 'https://dogsarebadatkeepingsecrets@squirrelchasers.ingest.sentry.io/12312012', + integrations: [requestDataIntegration], + }), + ); + client.setupIntegrations = () => requestDataIntegration.setupOnce(setMockEventProcessor, getCurrentHub); + client.getIntegration = () => requestDataIntegration as any; + + const hub = new Hub(client); + + makeMain(hub); +} + +describe('`RequestData` integration', () => { + let req: http.IncomingMessage, event: Event; + + beforeEach(() => { + req = { + headers, + method, + protocol, + hostname, + originalUrl: `${path}?${queryString}`, + } as unknown as http.IncomingMessage; + event = { sdkProcessingMetadata: { request: req } }; + }); + + afterEach(() => { + jest.clearAllMocks(); + }); + + describe('option conversion', () => { + it('leaves `ip` and `user` at top level of `include`', () => { + initWithRequestDataIntegrationOptions({ include: { ip: false, user: true } }); + + requestDataEventProcessor(event); + + const passedOptions = addRequestDataToEventSpy.mock.calls[0][2]; + + expect(passedOptions?.include).toEqual(expect.objectContaining({ ip: false, user: true })); + }); + + it('moves `transactionNamingScheme` to `transaction` include', () => { + initWithRequestDataIntegrationOptions({ transactionNamingScheme: 'path' }); + + requestDataEventProcessor(event); + + const passedOptions = addRequestDataToEventSpy.mock.calls[0][2]; + + expect(passedOptions?.include).toEqual(expect.objectContaining({ transaction: 'path' })); + }); + + it('moves `true` request keys into `request` include, but omits `false` ones', async () => { + initWithRequestDataIntegrationOptions({ include: { data: true, cookies: false } }); + + requestDataEventProcessor(event); + + const passedOptions = addRequestDataToEventSpy.mock.calls[0][2]; + + expect(passedOptions?.include?.request).toEqual(expect.arrayContaining(['data'])); + expect(passedOptions?.include?.request).not.toEqual(expect.arrayContaining(['cookies'])); + }); + + it('moves `true` user keys into `user` include, but omits `false` ones', async () => { + initWithRequestDataIntegrationOptions({ include: { user: { id: true, email: false } } }); + + requestDataEventProcessor(event); + + const passedOptions = addRequestDataToEventSpy.mock.calls[0][2]; + + expect(passedOptions?.include?.user).toEqual(expect.arrayContaining(['id'])); + expect(passedOptions?.include?.user).not.toEqual(expect.arrayContaining(['email'])); + }); + }); +});