From bcd6f8d234eb68472a350c3132dbc71de5e2e8b6 Mon Sep 17 00:00:00 2001 From: Katie Byers Date: Fri, 10 Jun 2022 15:38:08 -0700 Subject: [PATCH 1/7] move node-version- and server-name-adding code to client --- packages/node/src/client.ts | 13 ++- packages/node/src/handlers.ts | 19 ---- packages/node/test/client.test.ts | 89 +++++++++++++++++++ packages/node/test/handlers.test.ts | 21 +---- packages/serverless/src/awslambda.ts | 5 -- .../src/gcpfunction/cloud_events.ts | 9 +- packages/serverless/src/gcpfunction/events.ts | 4 +- .../serverless/src/gcpfunction/general.ts | 18 ---- packages/serverless/test/awslambda.test.ts | 16 ++-- packages/serverless/test/gcpfunction.test.ts | 15 +--- 10 files changed, 114 insertions(+), 95 deletions(-) diff --git a/packages/node/src/client.ts b/packages/node/src/client.ts index 144abaec240b..d8a98ee3bfd5 100644 --- a/packages/node/src/client.ts +++ b/packages/node/src/client.ts @@ -2,6 +2,7 @@ import { BaseClient, Scope, SDK_VERSION } from '@sentry/core'; import { SessionFlusher } from '@sentry/hub'; import { Event, EventHint, Severity, SeverityLevel } from '@sentry/types'; import { logger, resolvedSyncPromise } from '@sentry/utils'; +import * as os from 'os'; import { TextEncoder } from 'util'; import { eventFromMessage, eventFromUnknownInput } from './eventbuilder'; @@ -139,9 +140,15 @@ export class NodeClient extends BaseClient { */ protected _prepareEvent(event: Event, hint: EventHint, scope?: Scope): PromiseLike { event.platform = event.platform || 'node'; - if (this.getOptions().serverName) { - event.server_name = this.getOptions().serverName; - } + event.contexts = { + ...event.contexts, + runtime: event.contexts?.runtime || { + name: 'node', + version: global.process.version, + }, + }; + event.server_name = + event.server_name || this.getOptions().serverName || global.process.env.SENTRY_NAME || os.hostname(); return super._prepareEvent(event, hint, scope); } diff --git a/packages/node/src/handlers.ts b/packages/node/src/handlers.ts index 23114c917dc1..2f822efe66b5 100644 --- a/packages/node/src/handlers.ts +++ b/packages/node/src/handlers.ts @@ -14,7 +14,6 @@ import { import * as cookie from 'cookie'; import * as domain from 'domain'; import * as http from 'http'; -import * as os from 'os'; import * as url from 'url'; import { NodeClient } from './client'; @@ -292,10 +291,8 @@ export function extractRequestData( export interface ParseRequestOptions { ip?: boolean; request?: boolean | string[]; - serverName?: boolean; transaction?: boolean | TransactionNamingScheme; user?: boolean | string[]; - version?: boolean; } /** @@ -311,23 +308,11 @@ export function parseRequest(event: Event, req: ExpressRequest, options?: ParseR options = { ip: false, request: true, - serverName: true, transaction: true, user: true, - version: true, ...options, }; - if (options.version) { - event.contexts = { - ...event.contexts, - runtime: { - name: 'node', - version: global.process.version, - }, - }; - } - if (options.request) { // if the option value is `true`, use the default set of keys by not passing anything to `extractRequestData()` const extractedRequestData = Array.isArray(options.request) @@ -339,10 +324,6 @@ export function parseRequest(event: Event, req: ExpressRequest, options?: ParseR }; } - if (options.serverName && !event.server_name) { - event.server_name = global.process.env.SENTRY_NAME || os.hostname(); - } - if (options.user) { const extractedUser = req.user && isPlainObject(req.user) ? extractUserData(req.user, options.user) : {}; diff --git a/packages/node/test/client.test.ts b/packages/node/test/client.test.ts index 2aafb8a2544e..fc098854c0e9 100644 --- a/packages/node/test/client.test.ts +++ b/packages/node/test/client.test.ts @@ -1,4 +1,6 @@ import { Scope, SessionFlusher } from '@sentry/hub'; +import { Event, EventHint } from '@sentry/types'; +import * as os from 'os'; import { NodeClient } from '../src'; import { getDefaultNodeClientOptions } from './helper/node-client-options'; @@ -187,6 +189,93 @@ describe('NodeClient', () => { expect(requestSession!.status).toEqual('ok'); }); }); + + describe('_prepareEvent', () => { + test('adds platform to event', () => { + const options = getDefaultNodeClientOptions({ dsn: PUBLIC_DSN }); + client = new NodeClient(options); + + const event: Event = {}; + const hint: EventHint = {}; + (client as any)._prepareEvent(event, hint); + + expect(event.platform).toEqual('node'); + }); + + test('adds runtime context to event', () => { + const options = getDefaultNodeClientOptions({ dsn: PUBLIC_DSN }); + client = new NodeClient(options); + + const event: Event = {}; + const hint: EventHint = {}; + (client as any)._prepareEvent(event, hint); + + expect(event.contexts?.runtime).toEqual({ + name: 'node', + version: process.version, + }); + }); + + test('adds server name to event when value passed in options', () => { + const options = getDefaultNodeClientOptions({ dsn: PUBLIC_DSN, serverName: 'foo' }); + client = new NodeClient(options); + + const event: Event = {}; + const hint: EventHint = {}; + (client as any)._prepareEvent(event, hint); + + expect(event.server_name).toEqual('foo'); + }); + + test('adds server name to event when value given in env', () => { + const options = getDefaultNodeClientOptions({ dsn: PUBLIC_DSN }); + client = new NodeClient(options); + process.env.SENTRY_NAME = 'foo'; + + const event: Event = {}; + const hint: EventHint = {}; + (client as any)._prepareEvent(event, hint); + + expect(event.server_name).toEqual('foo'); + + delete process.env.SENTRY_NAME; + }); + + test('adds hostname as event server name when no value given', () => { + const options = getDefaultNodeClientOptions({ dsn: PUBLIC_DSN }); + client = new NodeClient(options); + + const event: Event = {}; + const hint: EventHint = {}; + (client as any)._prepareEvent(event, hint); + + expect(event.server_name).toEqual(os.hostname()); + }); + + test("doesn't clobber existing runtime data", () => { + const options = getDefaultNodeClientOptions({ dsn: PUBLIC_DSN, serverName: 'bar' }); + client = new NodeClient(options); + + const event: Event = { contexts: { runtime: { name: 'foo', version: '1.2.3' } } }; + const hint: EventHint = {}; + (client as any)._prepareEvent(event, hint); + + expect(event.contexts?.runtime).toEqual({ name: 'foo', version: '1.2.3' }); + expect(event.contexts?.runtime).not.toEqual({ name: 'node', version: process.version }); + }); + + test("doesn't clobber existing server name", () => { + const options = getDefaultNodeClientOptions({ dsn: PUBLIC_DSN, serverName: 'bar' }); + client = new NodeClient(options); + + const event: Event = { server_name: 'foo' }; + const hint: EventHint = {}; + (client as any)._prepareEvent(event, hint); + + expect(event.server_name).toEqual('foo'); + expect(event.server_name).not.toEqual('bar'); + }); + }); }); describe('flush/close', () => { diff --git a/packages/node/test/handlers.test.ts b/packages/node/test/handlers.test.ts index 0ac9b0ef43bd..753642a58303 100644 --- a/packages/node/test/handlers.test.ts +++ b/packages/node/test/handlers.test.ts @@ -2,7 +2,7 @@ import * as sentryCore from '@sentry/core'; import * as sentryHub from '@sentry/hub'; import { Hub } from '@sentry/hub'; import { Transaction } from '@sentry/tracing'; -import { Baggage, Runtime } from '@sentry/types'; +import { Baggage } from '@sentry/types'; import { isBaggageEmpty, isBaggageMutable, SentryError } from '@sentry/utils'; import * as http from 'http'; import * as net from 'net'; @@ -55,25 +55,6 @@ describe('parseRequest', () => { }; }); - describe('parseRequest.contexts runtime', () => { - test('runtime name must contain node', () => { - const parsedRequest: Event = parseRequest({}, mockReq as ExpressRequest); - expect((parsedRequest.contexts!.runtime as Runtime).name).toEqual('node'); - }); - - test('runtime version must contain current node version', () => { - const parsedRequest: Event = parseRequest({}, mockReq as ExpressRequest); - expect((parsedRequest.contexts!.runtime as Runtime).version).toEqual(process.version); - }); - - test('runtime disbaled by options', () => { - const parsedRequest: Event = parseRequest({}, mockReq as ExpressRequest, { - version: false, - }); - expect(parsedRequest).not.toHaveProperty('contexts.runtime'); - }); - }); - describe('parseRequest.user properties', () => { const DEFAULT_USER_KEYS = ['id', 'username', 'email']; const CUSTOM_USER_KEYS = ['custom_property']; diff --git a/packages/serverless/src/awslambda.ts b/packages/serverless/src/awslambda.ts index cfd9482e5611..c7c29f7efb2a 100644 --- a/packages/serverless/src/awslambda.ts +++ b/packages/serverless/src/awslambda.ts @@ -180,11 +180,6 @@ function enhanceScopeWithEnvironmentData(scope: Scope, context: Context, startTi scope.setTag('server_name', process.env._AWS_XRAY_DAEMON_ADDRESS || process.env.SENTRY_NAME || hostname()); scope.setTag('url', `awslambda:///${context.functionName}`); - scope.setContext('runtime', { - name: 'node', - version: global.process.version, - }); - scope.setContext('aws.lambda', { aws_request_id: context.awsRequestId, function_name: context.functionName, diff --git a/packages/serverless/src/gcpfunction/cloud_events.ts b/packages/serverless/src/gcpfunction/cloud_events.ts index 82b941840d8d..8ca0bff5dc16 100644 --- a/packages/serverless/src/gcpfunction/cloud_events.ts +++ b/packages/serverless/src/gcpfunction/cloud_events.ts @@ -2,12 +2,7 @@ import { captureException, flush, getCurrentHub, startTransaction } from '@sentr import { logger } from '@sentry/utils'; import { domainify, getActiveDomain, proxyFunction } from '../utils'; -import { - CloudEventFunction, - CloudEventFunctionWithCallback, - configureScopeWithContext, - WrapperOptions, -} from './general'; +import { CloudEventFunction, CloudEventFunctionWithCallback, WrapperOptions } from './general'; export type CloudEventFunctionWrapperOptions = WrapperOptions; @@ -44,7 +39,7 @@ function _wrapCloudEventFunction( // since functions-framework creates a domain for each incoming request. // So adding of event processors every time should not lead to memory bloat. getCurrentHub().configureScope(scope => { - configureScopeWithContext(scope, context); + scope.setContext('gcp.function.context', { ...context }); // We put the transaction on the scope so users can attach children to it scope.setSpan(transaction); }); diff --git a/packages/serverless/src/gcpfunction/events.ts b/packages/serverless/src/gcpfunction/events.ts index ac0de3e825c9..1b4efaf19a11 100644 --- a/packages/serverless/src/gcpfunction/events.ts +++ b/packages/serverless/src/gcpfunction/events.ts @@ -2,7 +2,7 @@ import { captureException, flush, getCurrentHub, startTransaction } from '@sentr import { logger } from '@sentry/utils'; import { domainify, getActiveDomain, proxyFunction } from '../utils'; -import { configureScopeWithContext, EventFunction, EventFunctionWithCallback, WrapperOptions } from './general'; +import { EventFunction, EventFunctionWithCallback, WrapperOptions } from './general'; export type EventFunctionWrapperOptions = WrapperOptions; @@ -39,7 +39,7 @@ function _wrapEventFunction( // since functions-framework creates a domain for each incoming request. // So adding of event processors every time should not lead to memory bloat. getCurrentHub().configureScope(scope => { - configureScopeWithContext(scope, context); + scope.setContext('gcp.function.context', { ...context }); // We put the transaction on the scope so users can attach children to it scope.setSpan(transaction); }); diff --git a/packages/serverless/src/gcpfunction/general.ts b/packages/serverless/src/gcpfunction/general.ts index 9c5f95615506..f819bd5aaaf3 100644 --- a/packages/serverless/src/gcpfunction/general.ts +++ b/packages/serverless/src/gcpfunction/general.ts @@ -1,7 +1,4 @@ -import { Scope } from '@sentry/node'; -import { Context as SentryContext } from '@sentry/types'; import type { Request, Response } from 'express'; -import { hostname } from 'os'; export interface HttpFunction { (req: Request, res: Response): any; // eslint-disable-line @typescript-eslint/no-explicit-any @@ -47,19 +44,4 @@ export interface WrapperOptions { flushTimeout: number; } -/** - * Enhances the scope with additional event information. - * - * @param scope scope - * @param context event context - */ -export function configureScopeWithContext(scope: Scope, context: Context): void { - scope.setContext('runtime', { - name: 'node', - version: global.process.version, - }); - scope.setTag('server_name', process.env.SENTRY_NAME || hostname()); - scope.setContext('gcp.function.context', { ...context } as SentryContext); -} - export type { Request, Response }; diff --git a/packages/serverless/test/awslambda.test.ts b/packages/serverless/test/awslambda.test.ts index 82eeaed5584d..20eb3cb410c0 100644 --- a/packages/serverless/test/awslambda.test.ts +++ b/packages/serverless/test/awslambda.test.ts @@ -46,8 +46,6 @@ function expectScopeSettings() { // @ts-ignore see "Why @ts-ignore" note expect(Sentry.fakeScope.setTag).toBeCalledWith('url', 'awslambda:///functionName'); // @ts-ignore see "Why @ts-ignore" note - expect(Sentry.fakeScope.setContext).toBeCalledWith('runtime', { name: 'node', version: expect.anything() }); - // @ts-ignore see "Why @ts-ignore" note expect(Sentry.fakeScope.setContext).toBeCalledWith( 'aws.lambda', expect.objectContaining({ @@ -181,7 +179,7 @@ describe('AWSLambda', () => { describe('wrapHandler() on sync handler', () => { test('successful execution', async () => { - expect.assertions(10); + expect.assertions(9); const handler: Handler = (_event, _context, callback) => { callback(null, 42); @@ -201,7 +199,7 @@ describe('AWSLambda', () => { }); test('unsuccessful execution', async () => { - expect.assertions(10); + expect.assertions(9); const error = new Error('sorry'); const handler: Handler = (_event, _context, callback) => { @@ -273,7 +271,7 @@ describe('AWSLambda', () => { }); test('capture error', async () => { - expect.assertions(10); + expect.assertions(9); const error = new Error('wat'); const handler: Handler = (_event, _context, _callback) => { @@ -304,7 +302,7 @@ describe('AWSLambda', () => { describe('wrapHandler() on async handler', () => { test('successful execution', async () => { - expect.assertions(10); + expect.assertions(9); const handler: Handler = async (_event, _context) => { return 42; @@ -335,7 +333,7 @@ describe('AWSLambda', () => { }); test('capture error', async () => { - expect.assertions(10); + expect.assertions(9); const error = new Error('wat'); const handler: Handler = async (_event, _context) => { @@ -377,7 +375,7 @@ describe('AWSLambda', () => { describe('wrapHandler() on async handler with a callback method (aka incorrect usage)', () => { test('successful execution', async () => { - expect.assertions(10); + expect.assertions(9); const handler: Handler = async (_event, _context, _callback) => { return 42; @@ -408,7 +406,7 @@ describe('AWSLambda', () => { }); test('capture error', async () => { - expect.assertions(10); + expect.assertions(9); const error = new Error('wat'); const handler: Handler = async (_event, _context, _callback) => { diff --git a/packages/serverless/test/gcpfunction.test.ts b/packages/serverless/test/gcpfunction.test.ts index 7b33ef13b57d..e9667adfd81d 100644 --- a/packages/serverless/test/gcpfunction.test.ts +++ b/packages/serverless/test/gcpfunction.test.ts @@ -218,7 +218,7 @@ describe('GCPFunction', () => { }); test('wrapHttpFunction request data', async () => { - expect.assertions(7); + expect.assertions(6); const handler: HttpFunction = (_req, res) => { res.end(); @@ -229,7 +229,6 @@ describe('GCPFunction', () => { Sentry.fakeScope.addEventProcessor.mockImplementation(cb => cb(event)); await handleHttp(wrappedHandler); expect(event.transaction).toEqual('POST /path'); - expect(event.contexts?.runtime).toEqual({ name: 'node', version: expect.anything() }); expect(event.request?.method).toEqual('POST'); expect(event.request?.url).toEqual('http://hostname/path?q=query'); expect(event.request?.query_string).toEqual('q=query'); @@ -368,16 +367,12 @@ describe('GCPFunction', () => { }); test('wrapEventFunction scope data', async () => { - expect.assertions(3); + expect.assertions(1); const handler: EventFunction = (_data, _context) => 42; const wrappedHandler = wrapEventFunction(handler); await handleEvent(wrappedHandler); // @ts-ignore see "Why @ts-ignore" note - expect(Sentry.fakeScope.setContext).toBeCalledWith('runtime', { name: 'node', version: expect.anything() }); - // @ts-ignore see "Why @ts-ignore" note - expect(Sentry.fakeScope.setTag).toBeCalledWith('server_name', expect.anything()); - // @ts-ignore see "Why @ts-ignore" note expect(Sentry.fakeScope.setContext).toBeCalledWith('gcp.function.context', { eventType: 'event.type', resource: 'some.resource', @@ -472,16 +467,12 @@ describe('GCPFunction', () => { }); test('wrapCloudEventFunction scope data', async () => { - expect.assertions(3); + expect.assertions(1); const handler: CloudEventFunction = _context => 42; const wrappedHandler = wrapCloudEventFunction(handler); await handleCloudEvent(wrappedHandler); // @ts-ignore see "Why @ts-ignore" note - expect(Sentry.fakeScope.setContext).toBeCalledWith('runtime', { name: 'node', version: expect.anything() }); - // @ts-ignore see "Why @ts-ignore" note - expect(Sentry.fakeScope.setTag).toBeCalledWith('server_name', expect.anything()); - // @ts-ignore see "Why @ts-ignore" note expect(Sentry.fakeScope.setContext).toBeCalledWith('gcp.function.context', { type: 'event.type' }); }); From 49e063e7bd60a4bed44f2ed1d8f0c312eb39351f Mon Sep 17 00:00:00 2001 From: Katie Byers Date: Fri, 10 Jun 2022 15:53:24 -0700 Subject: [PATCH 2/7] move request data functions to utils package --- packages/node/src/handlers.ts | 303 +--------------------- packages/utils/src/index.ts | 1 + packages/utils/src/requestdata.ts | 417 ++++++++++++++++++++++++++++++ 3 files changed, 427 insertions(+), 294 deletions(-) create mode 100644 packages/utils/src/requestdata.ts diff --git a/packages/node/src/handlers.ts b/packages/node/src/handlers.ts index 2f822efe66b5..297687f159db 100644 --- a/packages/node/src/handlers.ts +++ b/packages/node/src/handlers.ts @@ -1,49 +1,20 @@ -/* eslint-disable max-lines */ /* eslint-disable @typescript-eslint/no-explicit-any */ import { captureException, getCurrentHub, startTransaction, withScope } from '@sentry/core'; -import { Event, ExtractedNodeRequestData, Span, Transaction } from '@sentry/types'; +import { Event, Span } from '@sentry/types'; import { + AddRequestDataToEventOptions, + addRequestDataToTransaction, + extractPathForTransaction, extractTraceparentData, - isPlainObject, isString, logger, - normalize, parseBaggageSetMutability, - stripUrlQueryAndFragment, } from '@sentry/utils'; -import * as cookie from 'cookie'; import * as domain from 'domain'; import * as http from 'http'; -import * as url from 'url'; import { NodeClient } from './client'; -import { flush, isAutoSessionTrackingEnabled } from './sdk'; - -export interface ExpressRequest { - baseUrl?: string; - connection?: { - remoteAddress?: string; - }; - ip?: string; - method?: string; - originalUrl?: string; - route?: { - path: string; - stack: [ - { - name: string; - }, - ]; - }; - query?: { - // It can be: undefined | string | string[] | ParsedQs | ParsedQs[] (from `qs` package), but we dont want to pull it. - [key: string]: unknown; - }; - url?: string; - user?: { - [key: string]: any; - }; -} +import { addRequestDataToEvent, extractRequestData, flush, isAutoSessionTrackingEnabled } from './sdk'; /** * Express-compatible tracing handler. @@ -68,7 +39,7 @@ export function tracingHandler(): ( const transaction = startTransaction( { - name: extractExpressTransactionName(req, { path: true, method: true }), + name: extractPathForTransaction(req, { path: true, method: true }), op: 'http.server', ...traceparentData, metadata: { baggage: baggage }, @@ -91,7 +62,7 @@ export function tracingHandler(): ( // Push `transaction.finish` to the next event loop so open spans have a chance to finish before the transaction // closes setImmediate(() => { - addExpressReqToTransaction(transaction, req); + addRequestDataToTransaction(transaction, req); transaction.setHttpStatus(res.statusCode); transaction.finish(); }); @@ -101,263 +72,7 @@ export function tracingHandler(): ( }; } -/** - * Set parameterized as transaction name e.g.: `GET /users/:id` - * Also adds more context data on the transaction from the request - */ -function addExpressReqToTransaction(transaction: Transaction | undefined, req: ExpressRequest): void { - if (!transaction) return; - transaction.name = extractExpressTransactionName(req, { path: true, method: true }); - transaction.setData('url', req.originalUrl); - transaction.setData('baseUrl', req.baseUrl); - transaction.setData('query', req.query); -} - -/** - * Extracts complete generalized path from the request object and uses it to construct transaction name. - * - * eg. GET /mountpoint/user/:id - * - * @param req The ExpressRequest object - * @param options What to include in the transaction name (method, path, or both) - * - * @returns The fully constructed transaction name - */ -function extractExpressTransactionName( - req: ExpressRequest, - options: { path?: boolean; method?: boolean } = {}, -): string { - const method = req.method?.toUpperCase(); - - let path = ''; - if (req.route) { - path = `${req.baseUrl || ''}${req.route.path}`; - } else if (req.originalUrl || req.url) { - path = stripUrlQueryAndFragment(req.originalUrl || req.url || ''); - } - - let info = ''; - if (options.method && method) { - info += method; - } - if (options.method && options.path) { - info += ' '; - } - if (options.path && path) { - info += path; - } - - return info; -} - -type TransactionNamingScheme = 'path' | 'methodPath' | 'handler'; - -/** JSDoc */ -function extractTransaction(req: ExpressRequest, type: boolean | TransactionNamingScheme): string { - switch (type) { - case 'path': { - return extractExpressTransactionName(req, { path: true }); - } - case 'handler': { - return req.route?.stack[0].name || ''; - } - case 'methodPath': - default: { - return extractExpressTransactionName(req, { path: true, method: true }); - } - } -} - -/** Default user keys that'll be used to extract data from the request */ -const DEFAULT_USER_KEYS = ['id', 'username', 'email']; - -/** JSDoc */ -function extractUserData( - user: { - [key: string]: any; - }, - keys: boolean | string[], -): { [key: string]: any } { - const extractedUser: { [key: string]: any } = {}; - const attributes = Array.isArray(keys) ? keys : DEFAULT_USER_KEYS; - - attributes.forEach(key => { - if (user && key in user) { - extractedUser[key] = user[key]; - } - }); - - return extractedUser; -} - -/** Default request keys that'll be used to extract data from the request */ -const DEFAULT_REQUEST_KEYS = ['cookies', 'data', 'headers', 'method', 'query_string', 'url']; - -/** - * Normalizes data from the request object, accounting for framework differences. - * - * @param req The request object from which to extract data - * @param keys An optional array of keys to include in the normalized data. Defaults to DEFAULT_REQUEST_KEYS if not - * provided. - * @returns An object containing normalized request data - */ -export function extractRequestData( - req: { [key: string]: any }, - keys: string[] = DEFAULT_REQUEST_KEYS, -): ExtractedNodeRequestData { - const requestData: { [key: string]: any } = {}; - - // headers: - // node, express, nextjs: req.headers - // koa: req.header - const headers = (req.headers || req.header || {}) as { - 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 - const host = req.hostname || req.host || headers.host || ''; - // protocol: - // node, nextjs: - // express, koa: req.protocol - const protocol = - req.protocol === 'https' || req.secure || ((req.socket || {}) as { encrypted?: boolean }).encrypted - ? 'https' - : 'http'; - // url (including path and query string): - // node, express: req.originalUrl - // koa, nextjs: req.url - const originalUrl = (req.originalUrl || req.url || '') as string; - // absolute url - const absoluteUrl = `${protocol}://${host}${originalUrl}`; - - keys.forEach(key => { - switch (key) { - case 'headers': - requestData.headers = headers; - 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 - // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access - requestData.cookies = req.cookies || cookie.parse(headers.cookie || ''); - break; - case 'query_string': - // query string: - // node: req.url (raw) - // express, koa, nextjs: req.query - // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access - requestData.query_string = req.query || url.parse(originalUrl || '', false).query; - break; - case 'data': - if (method === 'GET' || method === 'HEAD') { - break; - } - // 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 - if (req.body !== undefined) { - requestData.data = isString(req.body) ? req.body : JSON.stringify(normalize(req.body)); - } - break; - default: - if ({}.hasOwnProperty.call(req, key)) { - requestData[key] = (req as { [key: string]: any })[key]; - } - } - }); - - return requestData; -} - -/** - * Options deciding what parts of the request to use when enhancing an event - */ -export interface ParseRequestOptions { - ip?: boolean; - request?: boolean | string[]; - transaction?: boolean | TransactionNamingScheme; - user?: boolean | string[]; -} - -/** - * Enriches passed event with request data. - * - * @param event Will be mutated and enriched with req data - * @param req Request object - * @param options object containing flags to enable functionality - * @hidden - */ -export function parseRequest(event: Event, req: ExpressRequest, options?: ParseRequestOptions): Event { - // eslint-disable-next-line no-param-reassign - options = { - ip: false, - request: true, - transaction: true, - user: true, - ...options, - }; - - if (options.request) { - // if the option value is `true`, use the default set of keys by not passing anything to `extractRequestData()` - const extractedRequestData = Array.isArray(options.request) - ? extractRequestData(req, options.request) - : extractRequestData(req); - event.request = { - ...event.request, - ...extractedRequestData, - }; - } - - if (options.user) { - const extractedUser = req.user && isPlainObject(req.user) ? extractUserData(req.user, options.user) : {}; - - if (Object.keys(extractedUser)) { - event.user = { - ...event.user, - ...extractedUser, - }; - } - } - - // client ip: - // node, nextjs: req.connection.remoteAddress - // express, koa: req.ip - if (options.ip) { - const ip = req.ip || (req.connection && req.connection.remoteAddress); - if (ip) { - event.user = { - ...event.user, - ip_address: ip, - }; - } - } - - if (options.transaction && !event.transaction) { - // TODO do we even need this anymore? - // TODO make this work for nextjs - event.transaction = extractTransaction(req, options.transaction); - } - - return event; -} - -export type RequestHandlerOptions = ParseRequestOptions & { +export type RequestHandlerOptions = AddRequestDataToEventOptions & { flushTimeout?: number; }; @@ -409,7 +124,7 @@ export function requestHandler( const currentHub = getCurrentHub(); currentHub.configureScope(scope => { - scope.addEventProcessor((event: Event) => parseRequest(event, req, options)); + scope.addEventProcessor((event: Event) => addRequestDataToEvent(event, req, options)); const client = currentHub.getClient(); if (isAutoSessionTrackingEnabled(client)) { const scope = currentHub.getScope(); diff --git a/packages/utils/src/index.ts b/packages/utils/src/index.ts index fd627db6f2e5..7c8943da813f 100644 --- a/packages/utils/src/index.ts +++ b/packages/utils/src/index.ts @@ -12,6 +12,7 @@ export * from './normalize'; export * from './object'; export * from './path'; export * from './promisebuffer'; +export * from './requestdata'; export * from './severity'; export * from './stacktrace'; export * from './string'; diff --git a/packages/utils/src/requestdata.ts b/packages/utils/src/requestdata.ts new file mode 100644 index 000000000000..3bcc9b3aa306 --- /dev/null +++ b/packages/utils/src/requestdata.ts @@ -0,0 +1,417 @@ +/* eslint-disable complexity */ +/** + * The functions here, which enrich an event with request data, are mostly for use in Node, but are safe for use in a + * browser context. They live here in `@sentry/utils` rather than in `@sentry/node` so that they can be used in + * frameworks (like nextjs), which, because of SSR, run the same code in both Node and browser contexts. + * + * TODO (v8 / #5257): Remove the note below + * Note that for now, the tests for this code have to live in `@sentry/node`, since they test both these functions and + * the backwards-compatibility-preserving wrappers which still live in `handlers.ts` there. + */ + +/* eslint-disable max-lines */ +/* eslint-disable @typescript-eslint/no-explicit-any */ + +import { Event, ExtractedNodeRequestData, Transaction } from '@sentry/types'; + +import { isPlainObject, isString } from './is'; +import { stripUrlQueryAndFragment } from './misc'; +import { normalize } from './normalize'; + +const DEFAULT_INCLUDES = { + ip: false, + request: true, + transaction: true, + user: true, +}; +const DEFAULT_REQUEST_INCLUDES = ['cookies', 'data', 'headers', 'method', 'query_string', 'url']; +const DEFAULT_USER_INCLUDES = ['id', 'username', 'email']; + +type BaseRequest = { + method?: string; + url?: string; +}; + +type BrowserRequest = BaseRequest; + +type NodeRequest = BaseRequest & { + headers?: { + [key: string]: string | string[] | undefined; + }; + protocol?: string; + socket?: { + encrypted?: boolean; + remoteAddress?: string; + }; +}; + +type KoaRequest = NodeRequest & { + host?: string; + hostname?: string; + ip?: string; + originalUrl?: string; +}; + +type NextjsRequest = NodeRequest & { + cookies?: { + [key: string]: string; + }; + query?: { + [key: string]: any; + }; +}; + +type ExpressRequest = NodeRequest & { + baseUrl?: string; + body?: string | { [key: string]: any }; + host?: string; + hostname?: string; + ip?: string; + originalUrl?: string; + route?: { + path: string; + stack: [ + { + name: string; + }, + ]; + }; + query?: { + [key: string]: any; + }; + user?: { + [key: string]: any; + }; +}; + +/** A `Request` type compatible with Node, Express, browser, etc., because everything is optional */ +export type CrossPlatformRequest = BaseRequest & + BrowserRequest & + NodeRequest & + ExpressRequest & + KoaRequest & + NextjsRequest; + +type InjectedNodeDeps = { + cookie: { + parse: (cookieStr: string) => Record; + }; + url: { + parse: (urlStr: string) => { + query: string | null; + }; + }; +}; + +/** + * Sets parameterized route as transaction name e.g.: `GET /users/:id` + * Also adds more context data on the transaction from the request + */ +export function addRequestDataToTransaction( + transaction: Transaction | undefined, + req: CrossPlatformRequest, + deps?: InjectedNodeDeps, +): void { + if (!transaction) return; + transaction.name = extractPathForTransaction(req, { path: true, method: true }); + transaction.setData('url', req.originalUrl || req.url); + if (req.baseUrl) { + transaction.setData('baseUrl', req.baseUrl); + } + transaction.setData('query', extractQueryParams(req, deps)); +} + +/** + * Extracts complete generalized path from the request object and uses it to construct transaction name. + * + * eg. GET /mountpoint/user/:id + * + * @param req A request object + * @param options What to include in the transaction name (method, path, or both) + * + * @returns The fully constructed transaction name + */ +export function extractPathForTransaction( + req: CrossPlatformRequest, + options: { path?: boolean; method?: boolean } = {}, +): string { + const method = req.method && req.method.toUpperCase(); + + let path = ''; + // Check to see if there's a parameterized route we can use (as there is in Express) + if (req.route) { + path = `${req.baseUrl || ''}${req.route.path}`; + } + // Otherwise, just take the original URL + else if (req.originalUrl || req.url) { + path = stripUrlQueryAndFragment(req.originalUrl || req.url || ''); + } + + let info = ''; + if (options.method && method) { + info += method; + } + if (options.method && options.path) { + info += ' '; + } + if (options.path && path) { + info += path; + } + + return info; +} + +type TransactionNamingScheme = 'path' | 'methodPath' | 'handler'; + +/** JSDoc */ +function extractTransaction(req: CrossPlatformRequest, type: boolean | TransactionNamingScheme): string { + switch (type) { + case 'path': { + return extractPathForTransaction(req, { path: true }); + } + case 'handler': { + return (req.route && req.route.stack && req.route.stack[0] && req.route.stack[0].name) || ''; + } + case 'methodPath': + default: { + return extractPathForTransaction(req, { path: true, method: true }); + } + } +} + +/** JSDoc */ +function extractUserData( + user: { + [key: string]: any; + }, + keys: boolean | string[], +): { [key: string]: any } { + const extractedUser: { [key: string]: any } = {}; + const attributes = Array.isArray(keys) ? keys : DEFAULT_USER_INCLUDES; + + attributes.forEach(key => { + if (user && key in user) { + extractedUser[key] = user[key]; + } + }); + + 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 + */ +export function extractRequestData( + req: CrossPlatformRequest, + options?: { + include?: string[]; + deps?: InjectedNodeDeps; + }, +): ExtractedNodeRequestData { + const { include = DEFAULT_REQUEST_INCLUDES, deps } = options || {}; + const requestData: { [key: string]: any } = {}; + + // headers: + // node, express, koa, nextjs: req.headers + const headers = (req.headers || {}) as { + 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 + const host = req.hostname || req.host || headers.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 = `${protocol}://${host}${originalUrl}`; + include.forEach(key => { + switch (key) { + case 'headers': { + requestData.headers = headers; + 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 + // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access + 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 && deps && deps.cookie && deps.cookie.parse(headers.cookie)) || {}; + break; + } + case 'query_string': { + // query string: + // node: req.url (raw) + // express, koa, nextjs: req.query + // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access + requestData.query_string = extractQueryParams(req, deps); + break; + } + case 'data': { + if (method === 'GET' || method === 'HEAD') { + break; + } + // 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 + if (req.body !== undefined) { + requestData.data = isString(req.body) ? req.body : JSON.stringify(normalize(req.body)); + } + break; + } + default: { + if ({}.hasOwnProperty.call(req, key)) { + requestData[key] = (req as { [key: string]: any })[key]; + } + } + } + }); + + return requestData; +} + +/** + * Options deciding what parts of the request to use when enhancing an event + */ +export interface AddRequestDataToEventOptions { + /** Flags controlling whether each type of data should be added to the event */ + include?: { + ip?: boolean; + request?: boolean | string[]; + transaction?: boolean | TransactionNamingScheme; + user?: boolean | string[]; + }; + + /** Injected platform-specific dependencies */ + deps?: { + cookie: { + parse: (cookieStr: string) => Record; + }; + url: { + parse: (urlStr: string) => { + query: string | null; + }; + }; + }; +} + +/** + * 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 + * @hidden + */ +export function addRequestDataToEvent( + event: Event, + req: CrossPlatformRequest, + options?: AddRequestDataToEventOptions, +): Event { + const include = { + ...DEFAULT_INCLUDES, + ...options?.include, + }; + + if (include.request) { + const extractedRequestData = Array.isArray(include.request) + ? extractRequestData(req, { include: include.request, deps: options?.deps }) + : extractRequestData(req, { deps: options?.deps }); + + 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 + if (include.ip) { + const ip = req.ip || (req.socket && req.socket.remoteAddress); + if (ip) { + event.user = { + ...event.user, + ip_address: ip, + }; + } + } + + if (include.transaction && !event.transaction) { + // TODO do we even need this anymore? + // TODO make this work for nextjs + event.transaction = extractTransaction(req, include.transaction); + } + + return event; +} + +function extractQueryParams( + req: CrossPlatformRequest, + deps?: InjectedNodeDeps, +): 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}`; + } + + return ( + req.query || + (typeof URL !== undefined && new URL(originalUrl).search.replace('?', '')) || + // In Node 8, `URL` isn't in the global scope, so we have to use the built-in module from Node + (deps && deps.url && deps.url.parse(originalUrl).query) || + undefined + ); +} From 74b5b9c5c490b252c3010733ae50fc4fb97bf8fc Mon Sep 17 00:00:00 2001 From: Katie Byers Date: Mon, 13 Jun 2022 11:10:08 -0700 Subject: [PATCH 3/7] create node wrapper for functions to pass node deps automatically --- packages/node/src/index.ts | 13 +++++++- packages/node/src/sdk.ts | 64 ++++++++++++++++++++++++++++++++++++-- 2 files changed, 74 insertions(+), 3 deletions(-) diff --git a/packages/node/src/index.ts b/packages/node/src/index.ts index 96e2383c6ec0..a0724035533a 100644 --- a/packages/node/src/index.ts +++ b/packages/node/src/index.ts @@ -15,6 +15,7 @@ export type { Thread, User, } from '@sentry/types'; +export type { AddRequestDataToEventOptions, CrossPlatformRequest } from '@sentry/utils'; export type { NodeOptions } from './types'; @@ -44,7 +45,17 @@ export { export { NodeClient } from './client'; export { makeNodeTransport } from './transports'; -export { defaultIntegrations, init, defaultStackParser, lastEventId, flush, close, getSentryRelease } from './sdk'; +export { + addRequestDataToEvent, + extractRequestData, + defaultIntegrations, + init, + defaultStackParser, + lastEventId, + flush, + close, + getSentryRelease, +} from './sdk'; export { deepReadDirSync } from './utils'; import { Integrations as CoreIntegrations } from '@sentry/core'; diff --git a/packages/node/src/sdk.ts b/packages/node/src/sdk.ts index e5993aa0d116..0746fb35ac7f 100644 --- a/packages/node/src/sdk.ts +++ b/packages/node/src/sdk.ts @@ -1,8 +1,20 @@ +/* eslint-disable max-lines */ import { getCurrentHub, getIntegrationsToSetup, initAndBind, Integrations as CoreIntegrations } from '@sentry/core'; import { getMainCarrier, setHubOnCarrier } from '@sentry/hub'; -import { SessionStatus, StackParser } from '@sentry/types'; -import { createStackParser, getGlobalObject, logger, stackParserFromStackParserOptions } from '@sentry/utils'; +import { Event, ExtractedNodeRequestData, SessionStatus, StackParser } from '@sentry/types'; +import { + addRequestDataToEvent as _addRequestDataToEvent, + AddRequestDataToEventOptions, + createStackParser, + CrossPlatformRequest, + extractRequestData as _extractRequestData, + getGlobalObject, + logger, + stackParserFromStackParserOptions, +} from '@sentry/utils'; +import * as cookie from 'cookie'; import * as domain from 'domain'; +import * as url from 'url'; import { NodeClient } from './client'; import { Console, ContextLines, Http, LinkedErrors, OnUncaughtException, OnUnhandledRejection } from './integrations'; @@ -255,3 +267,51 @@ function startSessionTracking(): void { if (session && !terminalStates.includes(session.status)) hub.endSession(); }); } + +/** + * Add data from the given request to the given event + * + * (Note that there is no sister function to this one in `@sentry/browser`, because the whole point of this wrapper is + * to pass along injected dependencies, which isn't necessary in a browser context. Isomorphic packages like + * `@sentry/nextjs` should export directly from `@sentry/utils` in their browser index file.) + * + * @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 + * @hidden + */ +export function addRequestDataToEvent( + event: Event, + req: CrossPlatformRequest, + options?: Omit, +): Event { + return _addRequestDataToEvent(event, req, { + ...options, + // We have to inject these node-only dependencies because we can't import them in `@sentry/utils`, where the + // original function lives + deps: { cookie, url }, + }); +} + +/** + * Normalize data from the request object, accounting for framework differences. + * + * (Note that there is no sister function to this one in `@sentry/browser`, because the whole point of this wrapper is + * to inject dependencies, which isn't necessary in a browser context. Isomorphic packages like `@sentry/nextjs` should + * export directly from `@sentry/utils` in their browser index file.) + * + * @param req The request object from which to extract data + * @param options.keys An optional array of keys to include in the normalized data. Defaults to DEFAULT_REQUEST_KEYS if + * not provided. + * @returns An object containing normalized request data + */ +export function extractRequestData( + req: CrossPlatformRequest, + options?: { + include?: string[]; + }, +): ExtractedNodeRequestData { + // We have to inject these node-only dependencies because we can't import them in `@sentry/utils`, where the original + // function lives + return _extractRequestData(req, { ...options, deps: { cookie, url } }); +} From bdd1daa1a97d6e7845a020e259161b672ed4b8de Mon Sep 17 00:00:00 2001 From: Katie Byers Date: Mon, 13 Jun 2022 11:16:57 -0700 Subject: [PATCH 4/7] add backwards-compatibility-preserving wrappers to `handlers` --- packages/node/src/handlers.ts | 6 +++ packages/node/src/requestDataDeprecated.ts | 60 ++++++++++++++++++++++ 2 files changed, 66 insertions(+) create mode 100644 packages/node/src/requestDataDeprecated.ts diff --git a/packages/node/src/handlers.ts b/packages/node/src/handlers.ts index 297687f159db..81496ce2a66f 100644 --- a/packages/node/src/handlers.ts +++ b/packages/node/src/handlers.ts @@ -241,3 +241,9 @@ export function errorHandler(options?: { next(error); }; } + +// TODO (v8 / #5257): Remove this +// eslint-disable-next-line deprecation/deprecation +export type { ParseRequestOptions, ExpressRequest } from './requestDataDeprecated'; +// eslint-disable-next-line deprecation/deprecation +export { parseRequest, extractRequestData } from './requestDataDeprecated'; diff --git a/packages/node/src/requestDataDeprecated.ts b/packages/node/src/requestDataDeprecated.ts new file mode 100644 index 000000000000..e5f562626fd5 --- /dev/null +++ b/packages/node/src/requestDataDeprecated.ts @@ -0,0 +1,60 @@ +/** + * Deprecated functions which are slated for removal in v8. When the time comes, this entire file can be deleted. + * + * See https://github.com/getsentry/sentry-javascript/pull/5257. + */ + +/* eslint-disable deprecation/deprecation */ +/* eslint-disable @typescript-eslint/no-explicit-any */ +import { Event, ExtractedNodeRequestData } from '@sentry/types'; +import { + addRequestDataToEvent, + AddRequestDataToEventOptions, + CrossPlatformRequest, + extractRequestData as _extractRequestData, +} from '@sentry/utils'; +import * as cookie from 'cookie'; +import * as url from 'url'; + +/** + * @deprecated `Handlers.ExpressRequest` is deprecated and will be removed in v8. Use `CrossPlatformRequest` instead. + */ +export type ExpressRequest = CrossPlatformRequest; + +/** + * Normalizes data from the request object, accounting for framework differences. + * + * @deprecated `Handlers.extractRequestData` is deprecated and will be removed in v8. Use `extractRequestData` instead. + * + * @param req The request object from which to extract data + * @param keys An optional array of keys to include in the normalized data. + * @returns An object containing normalized request data + */ +export function extractRequestData(req: { [key: string]: any }, keys?: string[]): ExtractedNodeRequestData { + return _extractRequestData(req, { include: keys, deps: { cookie, url } }); +} + +/** + * Options deciding what parts of the request to use when enhancing an event + * + * @deprecated `Handlers.ParseRequestOptions` is deprecated and will be removed in v8. Use + * `AddRequestDataToEventOptions` in `@sentry/utils` instead. + */ +export type ParseRequestOptions = AddRequestDataToEventOptions['include'] & { + serverName?: boolean; + version?: boolean; +}; + +/** + * Enriches passed event with request data. + * + * @deprecated `Handlers.parseRequest` is deprecated and will be removed in v8. Use `addRequestDataToEvent` instead. + * + * @param event Will be mutated and enriched with req data + * @param req Request object + * @param options object containing flags to enable functionality + * @hidden + */ +export function parseRequest(event: Event, req: ExpressRequest, options: ParseRequestOptions = {}): Event { + return addRequestDataToEvent(event, req, { include: options, deps: { cookie, url } }); +} From c88b08a6737c02693b069dcd5ed1393e32b07360 Mon Sep 17 00:00:00 2001 From: Katie Byers Date: Mon, 13 Jun 2022 11:18:02 -0700 Subject: [PATCH 5/7] use new functions instead of old ones in nextjs SDK --- packages/nextjs/src/utils/instrumentServer.ts | 6 ++---- packages/nextjs/src/utils/withSentry.ts | 6 ++---- 2 files changed, 4 insertions(+), 8 deletions(-) diff --git a/packages/nextjs/src/utils/instrumentServer.ts b/packages/nextjs/src/utils/instrumentServer.ts index f6a072455371..0106acfb8123 100644 --- a/packages/nextjs/src/utils/instrumentServer.ts +++ b/packages/nextjs/src/utils/instrumentServer.ts @@ -1,10 +1,10 @@ /* eslint-disable max-lines */ import { + addRequestDataToEvent, captureException, configureScope, deepReadDirSync, getCurrentHub, - Handlers, startTransaction, } from '@sentry/node'; import { extractTraceparentData, getActiveTransaction, hasTracingEnabled } from '@sentry/tracing'; @@ -22,8 +22,6 @@ import { default as createNextServer } from 'next'; import * as querystring from 'querystring'; import * as url from 'url'; -const { parseRequest } = Handlers; - // eslint-disable-next-line @typescript-eslint/no-explicit-any type PlainObject = { [key: string]: T }; @@ -246,7 +244,7 @@ function makeWrappedReqHandler(origReqHandler: ReqHandler): WrappedReqHandler { const currentScope = getCurrentHub().getScope(); if (currentScope) { - currentScope.addEventProcessor(event => parseRequest(event, nextReq)); + currentScope.addEventProcessor(event => addRequestDataToEvent(event, nextReq)); // We only want to record page and API requests if (hasTracingEnabled() && shouldTraceRequest(nextReq.url, publicDirFiles)) { diff --git a/packages/nextjs/src/utils/withSentry.ts b/packages/nextjs/src/utils/withSentry.ts index f2decaf01cc1..1684b8323215 100644 --- a/packages/nextjs/src/utils/withSentry.ts +++ b/packages/nextjs/src/utils/withSentry.ts @@ -1,4 +1,4 @@ -import { captureException, flush, getCurrentHub, Handlers, startTransaction } from '@sentry/node'; +import { addRequestDataToEvent, captureException, flush, getCurrentHub, startTransaction } from '@sentry/node'; import { extractTraceparentData, hasTracingEnabled } from '@sentry/tracing'; import { Transaction } from '@sentry/types'; import { @@ -12,8 +12,6 @@ import { import * as domain from 'domain'; import { NextApiHandler, NextApiRequest, NextApiResponse } from 'next'; -const { parseRequest } = Handlers; - // This is the same as the `NextApiHandler` type, except instead of having a return type of `void | Promise`, it's // only `Promise`, because wrapped handlers are always async export type WrappedNextApiHandler = (req: NextApiRequest, res: NextApiResponse) => Promise; @@ -43,7 +41,7 @@ export const withSentry = (origHandler: NextApiHandler): WrappedNextApiHandler = const currentScope = getCurrentHub().getScope(); if (currentScope) { - currentScope.addEventProcessor(event => parseRequest(event, req)); + currentScope.addEventProcessor(event => addRequestDataToEvent(event, req)); if (hasTracingEnabled()) { // If there is a trace header set, extract the data from it (parentSpanId, traceId, and sampling decision) From 066970e761ae088abef771551c484911977d9c55 Mon Sep 17 00:00:00 2001 From: Katie Byers Date: Mon, 13 Jun 2022 11:18:50 -0700 Subject: [PATCH 6/7] use new functions (in backwards-compatible way for types) in serverless SDK --- packages/serverless/src/gcpfunction/http.ts | 32 +++++++++++++++------ 1 file changed, 24 insertions(+), 8 deletions(-) diff --git a/packages/serverless/src/gcpfunction/http.ts b/packages/serverless/src/gcpfunction/http.ts index 96745bc054cc..2666cefc4e87 100644 --- a/packages/serverless/src/gcpfunction/http.ts +++ b/packages/serverless/src/gcpfunction/http.ts @@ -1,17 +1,29 @@ -import { captureException, flush, getCurrentHub, Handlers, startTransaction } from '@sentry/node'; +import { + addRequestDataToEvent, + AddRequestDataToEventOptions, + captureException, + flush, + getCurrentHub, + startTransaction, +} from '@sentry/node'; import { extractTraceparentData } from '@sentry/tracing'; import { isString, logger, parseBaggageSetMutability, stripUrlQueryAndFragment } from '@sentry/utils'; import { domainify, getActiveDomain, proxyFunction } from './../utils'; import { HttpFunction, WrapperOptions } from './general'; -type ParseRequestOptions = Handlers.ParseRequestOptions; - -export interface HttpFunctionWrapperOptions extends WrapperOptions { - parseRequestOptions: ParseRequestOptions; +// TODO (v8 / #5257): Remove this whole old/new business and just use the new stuff +interface OldHttpFunctionWrapperOptions extends WrapperOptions { + /** + * @deprecated Use `addRequestDataToEventOptions` instead. + */ + parseRequestOptions: AddRequestDataToEventOptions; +} +interface NewHttpFunctionWrapperOptions extends WrapperOptions { + addRequestDataToEventOptions: AddRequestDataToEventOptions; } -const { parseRequest } = Handlers; +export type HttpFunctionWrapperOptions = OldHttpFunctionWrapperOptions | NewHttpFunctionWrapperOptions; /** * Wraps an HTTP function handler adding it error capture and tracing capabilities. @@ -40,9 +52,13 @@ export function wrapHttpFunction( /** */ function _wrapHttpFunction(fn: HttpFunction, wrapOptions: Partial = {}): HttpFunction { + // TODO (v8 / #5257): Switch to using `addRequestDataToEventOptions` + // eslint-disable-next-line deprecation/deprecation + const { parseRequestOptions } = wrapOptions as OldHttpFunctionWrapperOptions; + const options: HttpFunctionWrapperOptions = { flushTimeout: 2000, - parseRequestOptions: {}, + addRequestDataToEventOptions: parseRequestOptions ? parseRequestOptions : {}, ...wrapOptions, }; return (req, res) => { @@ -72,7 +88,7 @@ function _wrapHttpFunction(fn: HttpFunction, wrapOptions: Partial { - scope.addEventProcessor(event => parseRequest(event, req, options.parseRequestOptions)); + scope.addEventProcessor(event => addRequestDataToEvent(event, req, options.addRequestDataToEventOptions)); // We put the transaction on the scope so users can attach children to it scope.setSpan(transaction); }); From eecf76834a9c74a4f166b8a755c3c6f070a60b23 Mon Sep 17 00:00:00 2001 From: Katie Byers Date: Mon, 13 Jun 2022 11:19:08 -0700 Subject: [PATCH 7/7] fix tests --- packages/node/test/handlers.test.ts | 344 +------------ packages/node/test/requestdata.test.ts | 487 ++++++++++++++++++ .../serverless/test/__mocks__/@sentry/node.ts | 1 + 3 files changed, 494 insertions(+), 338 deletions(-) create mode 100644 packages/node/test/requestdata.test.ts diff --git a/packages/node/test/handlers.test.ts b/packages/node/test/handlers.test.ts index 753642a58303..9263e1082871 100644 --- a/packages/node/test/handlers.test.ts +++ b/packages/node/test/handlers.test.ts @@ -2,174 +2,15 @@ import * as sentryCore from '@sentry/core'; import * as sentryHub from '@sentry/hub'; import { Hub } from '@sentry/hub'; import { Transaction } from '@sentry/tracing'; -import { Baggage } from '@sentry/types'; +import { Baggage, Event } from '@sentry/types'; import { isBaggageEmpty, isBaggageMutable, SentryError } from '@sentry/utils'; import * as http from 'http'; -import * as net from 'net'; -import { Event, Request, User } from '../src'; import { NodeClient } from '../src/client'; -import { - errorHandler, - ExpressRequest, - extractRequestData, - parseRequest, - requestHandler, - tracingHandler, -} from '../src/handlers'; +import { errorHandler, requestHandler, tracingHandler } from '../src/handlers'; import * as SDK from '../src/sdk'; import { getDefaultNodeClientOptions } from './helper/node-client-options'; -describe('parseRequest', () => { - let mockReq: { [key: string]: any }; - - beforeEach(() => { - mockReq = { - baseUrl: '/routerMountPath', - body: 'foo', - cookies: { test: 'test' }, - headers: { - host: 'mattrobenolt.com', - }, - 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('parseRequest.user properties', () => { - const DEFAULT_USER_KEYS = ['id', 'username', 'email']; - const CUSTOM_USER_KEYS = ['custom_property']; - - test('parseRequest.user only contains the default properties from the user', () => { - const parsedRequest: Event = parseRequest({}, mockReq as ExpressRequest); - expect(Object.keys(parsedRequest.user as User)).toEqual(DEFAULT_USER_KEYS); - }); - - test('parseRequest.user only contains the custom properties specified in the options.user array', () => { - const parsedRequest: Event = parseRequest({}, mockReq as ExpressRequest, { - user: CUSTOM_USER_KEYS, - }); - expect(Object.keys(parsedRequest.user as User)).toEqual(CUSTOM_USER_KEYS); - }); - - test('parseRequest.user doesnt blow up when someone passes non-object value', () => { - const parsedRequest: Event = parseRequest( - {}, - { - ...mockReq, - // @ts-ignore user is not assignable to object - user: 'wat', - }, - ); - expect(Object.keys(parsedRequest.user as User)).toEqual([]); - }); - }); - - describe('parseRequest.ip property', () => { - test('can be extracted from req.ip', () => { - const parsedRequest: Event = parseRequest( - {}, - { - ...mockReq, - ip: '123', - } as ExpressRequest, - { - ip: true, - }, - ); - expect(parsedRequest.user!.ip_address).toEqual('123'); - }); - - test('can extract from req.connection.remoteAddress', () => { - const parsedRequest: Event = parseRequest( - {}, - { - ...mockReq, - connection: { - remoteAddress: '321', - } as net.Socket, - } as ExpressRequest, - { - ip: true, - }, - ); - expect(parsedRequest.user!.ip_address).toEqual('321'); - }); - }); - - describe('parseRequest.request properties', () => { - test('parseRequest.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 = parseRequest({}, mockReq as ExpressRequest); - expect(Object.keys(parsedRequest.request as Request)).toEqual(DEFAULT_REQUEST_PROPERTIES); - }); - - test('parseRequest.request only contains the specified properties in the options.request array', () => { - const INCLUDED_PROPERTIES = ['data', 'headers', 'query_string', 'url']; - const parsedRequest: Event = parseRequest({}, mockReq as ExpressRequest, { - request: INCLUDED_PROPERTIES, - }); - expect(Object.keys(parsedRequest.request as Request)).toEqual(INCLUDED_PROPERTIES); - }); - - test('parseRequest.request skips `body` property for GET and HEAD requests', () => { - expect(parseRequest({}, mockReq as ExpressRequest, {}).request).toHaveProperty('data'); - expect(parseRequest({}, { ...mockReq, method: 'GET' } as ExpressRequest, {}).request).not.toHaveProperty('data'); - expect(parseRequest({}, { ...mockReq, method: 'HEAD' } as ExpressRequest, {}).request).not.toHaveProperty('data'); - }); - }); - - describe('parseRequest.transaction property', () => { - test('extracts method and full route path by default`', () => { - const parsedRequest: Event = parseRequest({}, mockReq as ExpressRequest); - expect(parsedRequest.transaction).toEqual('POST /routerMountPath/subpath/:parameterName'); - }); - - test('extracts method and full path by default when mountpoint is `/`', () => { - mockReq.originalUrl = mockReq.originalUrl.replace('/routerMountpath', ''); - mockReq.baseUrl = ''; - const parsedRequest: Event = parseRequest({}, mockReq as ExpressRequest); - // "sub"path is the full path here, because there's no router mount path - expect(parsedRequest.transaction).toEqual('POST /subpath/:parameterName'); - }); - - test('fallback to method and `originalUrl` if route is missing', () => { - delete mockReq.route; - const parsedRequest: Event = parseRequest({}, mockReq as ExpressRequest); - expect(parsedRequest.transaction).toEqual('POST /routerMountPath/subpath/specificValue'); - }); - - test('can extract path only instead if configured', () => { - const parsedRequest: Event = parseRequest({}, mockReq as ExpressRequest, { transaction: 'path' }); - expect(parsedRequest.transaction).toEqual('/routerMountPath/subpath/:parameterName'); - }); - - test('can extract handler name instead if configured', () => { - const parsedRequest: Event = parseRequest({}, mockReq as ExpressRequest, { transaction: 'handler' }); - expect(parsedRequest.transaction).toEqual('parameterNameRouteHandler'); - }); - }); -}); - describe('requestHandler', () => { const headers = { ears: 'furry', nose: 'wet', tongue: 'spotted', cookie: 'favorite=zukes' }; const method = 'wagging'; @@ -270,7 +111,7 @@ describe('requestHandler', () => { }); }); - it('patches `res.end` when `flushTimeout` is specified', () => { + it('patches `res.end` when `flushTimeout` is specified', done => { const flush = jest.spyOn(SDK, 'flush').mockResolvedValue(true); const sentryRequestMiddleware = requestHandler({ flushTimeout: 1337 }); @@ -280,10 +121,11 @@ describe('requestHandler', () => { setImmediate(() => { expect(flush).toHaveBeenCalledWith(1337); expect(res.finished).toBe(true); + done(); }); }); - it('prevents errors thrown during `flush` from breaking the response', async () => { + it('prevents errors thrown during `flush` from breaking the response', done => { jest.spyOn(SDK, 'flush').mockRejectedValue(new SentryError('HTTP Error (429)')); const sentryRequestMiddleware = requestHandler({ flushTimeout: 1337 }); @@ -292,6 +134,7 @@ describe('requestHandler', () => { setImmediate(() => { expect(res.finished).toBe(true); + done(); }); }); }); @@ -537,181 +380,6 @@ describe('tracingHandler', () => { }); }); -describe('extractRequestData()', () => { - describe('default behaviour', () => { - test('node', () => { - expect( - extractRequestData({ - headers: { host: 'example.com' }, - method: 'GET', - secure: true, - originalUrl: '/', - }), - ).toEqual({ - cookies: {}, - headers: { - host: 'example.com', - }, - method: 'GET', - query_string: null, - url: 'https://example.com/', - }); - }); - - test('degrades gracefully without request data', () => { - expect(extractRequestData({})).toEqual({ - cookies: {}, - headers: {}, - method: undefined, - query_string: null, - url: 'http://', - }); - }); - }); - - describe('cookies', () => { - it('uses `req.cookies` if available', () => { - expect( - extractRequestData( - { - cookies: { foo: 'bar' }, - }, - ['cookies'], - ), - ).toEqual({ - cookies: { foo: 'bar' }, - }); - }); - - it('parses the cookie header', () => { - expect( - extractRequestData( - { - headers: { - cookie: 'foo=bar;', - }, - }, - ['cookies'], - ), - ).toEqual({ - cookies: { foo: 'bar' }, - }); - }); - - it('falls back if no cookies are defined', () => { - expect(extractRequestData({}, ['cookies'])).toEqual({ - cookies: {}, - }); - }); - }); - - describe('data', () => { - it('includes data from `req.body` if available', () => { - expect( - extractRequestData( - { - method: 'POST', - headers: { 'Content-Type': 'application/x-www-form-urlencoded' }, - body: 'foo=bar', - }, - ['data'], - ), - ).toEqual({ - data: 'foo=bar', - }); - }); - - it('encodes JSON body contents back to a string', () => { - expect( - extractRequestData( - { - method: 'POST', - headers: { 'Content-Type': 'application/json' }, - body: { foo: 'bar' }, - }, - ['data'], - ), - ).toEqual({ - data: '{"foo":"bar"}', - }); - }); - }); - - describe('query_string', () => { - it('parses the query parms from the url', () => { - expect( - extractRequestData( - { - headers: { host: 'example.com' }, - secure: true, - originalUrl: '/?foo=bar', - }, - ['query_string'], - ), - ).toEqual({ - query_string: 'foo=bar', - }); - }); - - it('gracefully degrades if url cannot be determined', () => { - expect(extractRequestData({}, ['query_string'])).toEqual({ - query_string: null, - }); - }); - }); - - describe('url', () => { - test('express/koa', () => { - expect( - extractRequestData( - { - host: 'example.com', - protocol: 'https', - url: '/', - }, - ['url'], - ), - ).toEqual({ - url: 'https://example.com/', - }); - }); - - test('node', () => { - expect( - extractRequestData( - { - headers: { host: 'example.com' }, - secure: true, - originalUrl: '/', - }, - ['url'], - ), - ).toEqual({ - url: 'https://example.com/', - }); - }); - }); - - describe('custom key', () => { - it('includes the custom key if present', () => { - expect( - extractRequestData( - { - httpVersion: '1.1', - }, - ['httpVersion'], - ), - ).toEqual({ - httpVersion: '1.1', - }); - }); - - it('gracefully degrades if the custom key is missing', () => { - expect(extractRequestData({}, ['httpVersion'])).toEqual({}); - }); - }); -}); - describe('errorHandler()', () => { const headers = { ears: 'furry', nose: 'wet', tongue: 'spotted', cookie: 'favorite=zukes' }; const method = 'wagging'; diff --git a/packages/node/test/requestdata.test.ts b/packages/node/test/requestdata.test.ts new file mode 100644 index 000000000000..063d30d505e2 --- /dev/null +++ b/packages/node/test/requestdata.test.ts @@ -0,0 +1,487 @@ +/* eslint-disable deprecation/deprecation */ + +/* Note: These tests (except for the ones related to cookies) should eventually live in `@sentry/utils`, and can be + * moved there once the the backwards-compatibility-preserving wrappers in `handlers.ts` are removed. + */ + +// TODO (v8 / #5257): Remove everything above + +import { Event, User } from '@sentry/types'; +import { + addRequestDataToEvent, + AddRequestDataToEventOptions, + CrossPlatformRequest, + extractRequestData as newExtractRequestData, +} from '@sentry/utils'; +import * as cookie from 'cookie'; +import * as net from 'net'; +import * as url from 'url'; + +import { + ExpressRequest, + extractRequestData as oldExtractRequestData, + parseRequest, +} from '../src/requestDataDeprecated'; + +const mockCookieModule = { parse: jest.fn() }; + +// TODO (v8 / #5257): Remove `describe.each` wrapper, remove `formatArgs` wrapper, reformat args in tests, use only +// `addRequestDataToEvent`, and move these tests to @sentry/utils +describe.each([parseRequest, addRequestDataToEvent])( + 'backwards compatibility of `parseRequest` rename and move', + fn => { + /** Rearrage and cast args correctly for each version of the function */ + function formatArgs( + fn: typeof parseRequest | typeof addRequestDataToEvent, + event: Event, + req: any, + include?: AddRequestDataToEventOptions['include'], + ): Parameters | Parameters { + if (fn.name === 'parseRequest') { + return [event, req as ExpressRequest, include]; + } else { + return [ + event, + req as CrossPlatformRequest, + { + include, + deps: { + cookie: mockCookieModule, + url, + }, + }, + ]; + } + } + + describe(fn, () => { + let mockEvent: Event; + let mockReq: { [key: string]: any }; + + beforeEach(() => { + mockEvent = {}; + mockReq = { + baseUrl: '/routerMountPath', + body: 'foo', + cookies: { test: 'test' }, + headers: { + host: 'mattrobenolt.com', + }, + 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(`${fn.name}.user properties`, () => { + const DEFAULT_USER_KEYS = ['id', 'username', 'email']; + const CUSTOM_USER_KEYS = ['custom_property']; + + test(`${fn.name}.user only contains the default properties from the user`, () => { + const [event, req, options] = formatArgs(fn, mockEvent, mockReq); + const parsedRequest: Event = fn(event, req, options); + + expect(Object.keys(parsedRequest.user as User)).toEqual(DEFAULT_USER_KEYS); + }); + + test(`${fn.name}.user only contains the custom properties specified in the options.user array`, () => { + const optionsWithCustomUserKeys = { + user: CUSTOM_USER_KEYS, + }; + + const [event, req, options] = formatArgs(fn, mockEvent, mockReq, optionsWithCustomUserKeys); + const parsedRequest: Event = fn(event, req, options); + + expect(Object.keys(parsedRequest.user as User)).toEqual(CUSTOM_USER_KEYS); + }); + + test(`${fn.name}.user doesnt blow up when someone passes non-object value`, () => { + const reqWithUser = { + ...mockReq, + // @ts-ignore user is not assignable to object + user: 'wat', + }; + + const [event, req, options] = formatArgs(fn, mockEvent, reqWithUser); + const parsedRequest: Event = fn(event, req, options); + + expect(parsedRequest.user).toBeUndefined(); + }); + }); + + describe(`${fn.name}.ip property`, () => { + test('can be extracted from req.ip', () => { + const mockReqWithIP = { + ...mockReq, + ip: '123', + }; + const optionsWithIP = { + ip: true, + }; + + const [event, req, options] = formatArgs(fn, mockEvent, mockReqWithIP, optionsWithIP); + const parsedRequest: Event = fn(event, req, options); + + 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 = { + ip: true, + }; + + const [event, req, options] = formatArgs(fn, mockEvent, reqWithIPInSocket, optionsWithIP); + const parsedRequest: Event = fn(event, req, options); + + expect(parsedRequest.user!.ip_address).toEqual('321'); + }); + }); + + describe(`${fn.name}.request properties`, () => { + test(`${fn.name}.request only contains the default set of properties from the request`, () => { + const DEFAULT_REQUEST_PROPERTIES = ['cookies', 'data', 'headers', 'method', 'query_string', 'url']; + + const [event, req, options] = formatArgs(fn, mockEvent, mockReq); + const parsedRequest: Event = fn(event, req, options); + + expect(Object.keys(parsedRequest.request!)).toEqual(DEFAULT_REQUEST_PROPERTIES); + }); + + test(`${fn.name}.request only contains the specified properties in the options.request array`, () => { + const INCLUDED_PROPERTIES = ['data', 'headers', 'query_string', 'url']; + const optionsWithRequestIncludes = { + request: INCLUDED_PROPERTIES, + }; + + const [event, req, options] = formatArgs(fn, mockEvent, mockReq, optionsWithRequestIncludes); + const parsedRequest: Event = fn(event, req, options); + + expect(Object.keys(parsedRequest.request!)).toEqual(INCLUDED_PROPERTIES); + }); + + test.each([ + [undefined, true], + ['GET', false], + ['HEAD', false], + ])( + `${fn.name}.request skips \`body\` property for GET and HEAD requests - %s method`, + (method, shouldIncludeBodyData) => { + const reqWithMethod = { ...mockReq, method }; + + const [event, req, options] = formatArgs(fn, mockEvent, reqWithMethod); + const parsedRequest: Event = fn(event, req, options); + + if (shouldIncludeBodyData) { + expect(parsedRequest.request).toHaveProperty('data'); + } else { + expect(parsedRequest.request).not.toHaveProperty('data'); + } + }, + ); + }); + + describe(`${fn.name}.transaction property`, () => { + test('extracts method and full route path by default`', () => { + const [event, req, options] = formatArgs(fn, mockEvent, mockReq); + const parsedRequest: Event = fn(event, req, options); + + expect(parsedRequest.transaction).toEqual('POST /routerMountPath/subpath/:parameterName'); + }); + + test('extracts method and full path by default when mountpoint is `/`', () => { + mockReq.originalUrl = mockReq.originalUrl.replace('/routerMountpath', ''); + mockReq.baseUrl = ''; + + const [event, req, options] = formatArgs(fn, mockEvent, mockReq); + const parsedRequest: Event = fn(event, req, options); + + // `subpath/` is the full path here, because there's no router mount path + expect(parsedRequest.transaction).toEqual('POST /subpath/:parameterName'); + }); + + test('fallback to method and `originalUrl` if route is missing', () => { + delete mockReq.route; + + const [event, req, options] = formatArgs(fn, mockEvent, mockReq); + const parsedRequest: Event = fn(event, req, options); + + expect(parsedRequest.transaction).toEqual('POST /routerMountPath/subpath/specificValue'); + }); + + test('can extract path only instead if configured', () => { + const optionsWithPathTransaction = { transaction: 'path' } as const; + + const [event, req, options] = formatArgs(fn, mockEvent, mockReq, optionsWithPathTransaction); + const parsedRequest: Event = fn(event, req, options); + + expect(parsedRequest.transaction).toEqual('/routerMountPath/subpath/:parameterName'); + }); + + test('can extract handler name instead if configured', () => { + const optionsWithHandlerTransaction = { transaction: 'handler' } as const; + + const [event, req, options] = formatArgs(fn, mockEvent, mockReq, optionsWithHandlerTransaction); + const parsedRequest: Event = fn(event, req, options); + + expect(parsedRequest.transaction).toEqual('parameterNameRouteHandler'); + }); + }); + }); + }, +); + +// TODO (v8 / #5257): Remove `describe.each` wrapper, remove `formatArgs` wrapper, reformat args in tests, use only +// `newExtractRequestData`, rename `newExtractRequestData` to just `extractRequestData`, and move these tests (except +// the ones involving cookies) to @sentry/utils (use `mockCookieModule` for others) +Object.defineProperty(oldExtractRequestData, 'name', { + value: 'oldExtractRequestData', +}); +Object.defineProperty(newExtractRequestData, 'name', { + value: 'newExtractRequestData', +}); +describe.each([oldExtractRequestData, newExtractRequestData])( + 'backwards compatibility of `extractRequestData` move', + fn => { + /** Rearrage and cast args correctly for each version of the function */ + function formatArgs( + fn: typeof oldExtractRequestData | typeof newExtractRequestData, + req: any, + include?: string[], + ): Parameters | Parameters { + if (fn.name === 'oldExtractRequestData') { + return [req as ExpressRequest, include] as Parameters; + } else { + return [ + req as CrossPlatformRequest, + { + include, + deps: { + cookie: include?.includes('cookies') ? cookie : mockCookieModule, + url, + }, + }, + ] as Parameters; + } + } + + describe(fn, () => { + describe('default behaviour', () => { + test('node', () => { + const mockReq = { + headers: { host: 'example.com' }, + method: 'GET', + socket: { encrypted: true }, + originalUrl: '/', + }; + + const [req, options] = formatArgs(fn, mockReq); + + expect(fn(req, options as any)).toEqual({ + cookies: {}, + headers: { + host: 'example.com', + }, + method: 'GET', + query_string: undefined, + url: 'https://example.com/', + }); + }); + + test('degrades gracefully without request data', () => { + const mockReq = {}; + + const [req, options] = formatArgs(fn, mockReq); + + expect(fn(req, options as any)).toEqual({ + cookies: {}, + headers: {}, + method: undefined, + query_string: undefined, + url: 'http://', + }); + }); + }); + + describe('cookies', () => { + it('uses `req.cookies` if available', () => { + const mockReq = { + cookies: { foo: 'bar' }, + }; + const optionsWithCookies = ['cookies']; + + const [req, options] = formatArgs(fn, mockReq, optionsWithCookies); + + expect(fn(req, options as any)).toEqual({ + cookies: { foo: 'bar' }, + }); + }); + + it('parses the cookie header', () => { + const mockReq = { + headers: { + cookie: 'foo=bar;', + }, + }; + const optionsWithCookies = ['cookies']; + + const [req, options] = formatArgs(fn, mockReq, optionsWithCookies); + + expect(fn(req, options as any)).toEqual({ + cookies: { foo: 'bar' }, + }); + }); + + it('falls back if no cookies are defined', () => { + const mockReq = {}; + const optionsWithCookies = ['cookies']; + + const [req, options] = formatArgs(fn, mockReq, optionsWithCookies); + + expect(fn(req, options as any)).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 = ['data']; + + const [req, options] = formatArgs(fn, mockReq, optionsWithData); + + expect(fn(req, options as any)).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 = ['data']; + + const [req, options] = formatArgs(fn, mockReq, optionsWithData); + + expect(fn(req, options as any)).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 = ['query_string']; + + const [req, options] = formatArgs(fn, mockReq, optionsWithQueryString); + + expect(fn(req, options as any)).toEqual({ + query_string: 'foo=bar', + }); + }); + + it('gracefully degrades if url cannot be determined', () => { + const mockReq = {}; + const optionsWithQueryString = ['query_string']; + + const [req, options] = formatArgs(fn, mockReq, optionsWithQueryString); + + expect(fn(req, options as any)).toEqual({ + query_string: undefined, + }); + }); + }); + + describe('url', () => { + test('express/koa', () => { + const mockReq = { + host: 'example.com', + protocol: 'https', + url: '/', + }; + const optionsWithURL = ['url']; + + const [req, options] = formatArgs(fn, mockReq, optionsWithURL); + + expect(fn(req, options as any)).toEqual({ + url: 'https://example.com/', + }); + }); + + test('node', () => { + const mockReq = { + headers: { host: 'example.com' }, + socket: { encrypted: true }, + originalUrl: '/', + }; + const optionsWithURL = ['url']; + + const [req, options] = formatArgs(fn, mockReq, optionsWithURL); + + expect(fn(req, options as any)).toEqual({ + url: 'https://example.com/', + }); + }); + }); + + describe('custom key', () => { + it('includes the custom key if present', () => { + const mockReq = { + httpVersion: '1.1', + }; + const optionsWithCustomKey = ['httpVersion']; + + const [req, options] = formatArgs(fn, mockReq, optionsWithCustomKey); + + expect(fn(req, options as any)).toEqual({ + httpVersion: '1.1', + }); + }); + + it('gracefully degrades if the custom key is missing', () => { + const mockReq = {}; + const optionsWithCustomKey = ['httpVersion']; + + const [req, options] = formatArgs(fn, mockReq, optionsWithCustomKey); + + expect(fn(req, options as any)).toEqual({}); + }); + }); + }); + }, +); diff --git a/packages/serverless/test/__mocks__/@sentry/node.ts b/packages/serverless/test/__mocks__/@sentry/node.ts index 769d22f6e2f6..14043efdd42c 100644 --- a/packages/serverless/test/__mocks__/@sentry/node.ts +++ b/packages/serverless/test/__mocks__/@sentry/node.ts @@ -1,6 +1,7 @@ const origSentry = jest.requireActual('@sentry/node'); export const defaultIntegrations = origSentry.defaultIntegrations; // eslint-disable-line @typescript-eslint/no-unsafe-member-access export const Handlers = origSentry.Handlers; // eslint-disable-line @typescript-eslint/no-unsafe-member-access +export const addRequestDataToEvent = origSentry.addRequestDataToEvent; export const SDK_VERSION = '6.6.6'; export const Severity = { Warning: 'warning',