From 7c521e3cc4a6a8caa685e2f1316a133b3acae212 Mon Sep 17 00:00:00 2001 From: Katie Byers Date: Fri, 3 Jun 2022 17:04:12 -0700 Subject: [PATCH] ref(node): Move non-handler code out of `handlers` module (#5190) Currently, the `handlers` module includes not just our middleware handlers but also a bunch of code for extracting context data and applying it to events. That latter code is indeed used in the handlers, but it's also used in a number of other places in the repo. This pulls that code into its own module in `@sentry/utils`, to make it possible to use in a platform-agnostic context. Key changes: - A new `requestData` module in `@sentry/utils` to hold the data extraction code (and a new test file for its tests). - Wrapper functions in the `handlers` module for backwards compatibility. (IOW, calling `Handlers.someFunc()` still works, but it's now just a passthrough to `someFunc` from the new module.) - Deprecation notices in the docstrings of the wrappers, and TODOs so we remember to make the switch in v8. - Wrapper `describe`s in the tests, to run the tests against both the real functions and their wrappers, to make sure the wrappers work identically to the real functions. (This means that for the moment, the test file needs to live in `@sentry/node`, since it depends on the wrappers as well as the real functions. Once the wrappers are removed, the test file can be moved to `@sentry/utils`.) - `parseRequest` has been renamed `addRequestDataToEvent` for greater accuracy and clarity for the reader. (The corresponding options type has also been renamed.) - Two pieces of context data which have until now been set by `parseRequest`, but which have nothing to do with the request and are Node-only (namely `runtime` and `server_name`) are now set by the Node client. As a result, they no longer need to be set separately in the serverless package. (The exception is `server_name` in the AWS integration, because it pulls from a non-standard location.) Note: The original impetus for this change is the need to call `parseRequest`/`addRequestDataToEvent` in the nextjs SDK, in code which can run either in Node or the browser, which means it can't be exported from `@sentry/node`. --- packages/nextjs/src/utils/instrumentServer.ts | 6 +- packages/nextjs/src/utils/withSentry.ts | 6 +- packages/node/src/client.ts | 12 +- packages/node/src/handlers.ts | 362 +++-------------- packages/node/src/index.ts | 2 + packages/node/test/client.test.ts | 65 ++++ packages/node/test/handlers.test.ts | 363 +----------------- packages/node/test/requestdata.test.ts | 360 +++++++++++++++++ packages/serverless/src/awslambda.ts | 5 - .../serverless/src/gcpfunction/general.ts | 6 - packages/serverless/src/gcpfunction/http.ts | 32 +- .../serverless/test/__mocks__/@sentry/node.ts | 1 + packages/serverless/test/awslambda.test.ts | 16 +- packages/serverless/test/gcpfunction.test.ts | 15 +- packages/utils/package.json | 3 + packages/utils/src/index.ts | 1 + packages/utils/src/requestdata.ts | 306 +++++++++++++++ 17 files changed, 843 insertions(+), 718 deletions(-) create mode 100644 packages/node/test/requestdata.test.ts create mode 100644 packages/utils/src/requestdata.ts diff --git a/packages/nextjs/src/utils/instrumentServer.ts b/packages/nextjs/src/utils/instrumentServer.ts index d56e8d735f71..414d99e5c771 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 fa56bbaf2867..e9cd6b1e5d7e 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) diff --git a/packages/node/src/client.ts b/packages/node/src/client.ts index 144abaec240b..c222bbe08b91 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,14 @@ 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: { + name: 'node', + version: global.process.version, + }, + }; + 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 9bd7c6035fd1..da744154b19d 100644 --- a/packages/node/src/handlers.ts +++ b/packages/node/src/handlers.ts @@ -1,51 +1,24 @@ -/* 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, ExtractedNodeRequestData, Span } from '@sentry/types'; import { + addExpressReqToTransaction, + addRequestDataToEvent, + AddRequestDataToEventOptions, + ExpressRequest as _ExpressRequest, + extractExpressTransactionName, + extractRequestData as _extractRequestData, extractTraceparentData, - isPlainObject, isString, logger, - normalize, parseBaggageString, - stripUrlQueryAndFragment, } from '@sentry/utils'; -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'; 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; - }; -} - /** * Express-compatible tracing handler. * @see Exposed as `Handlers.tracingHandler` @@ -73,6 +46,7 @@ export function tracingHandler(): ( ...(baggage && { metadata: { baggage: baggage } }), }, // extra context passed to the tracesSampler + // eslint-disable-next-line deprecation/deprecation { request: extractRequestData(req) }, ); @@ -100,281 +74,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[]; - serverName?: boolean; - transaction?: boolean | TransactionNamingScheme; - user?: boolean | string[]; - version?: boolean; -} - -/** - * 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, - 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) - ? extractRequestData(req, options.request) - : extractRequestData(req); - event.request = { - ...event.request, - ...extractedRequestData, - }; - } - - 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) : {}; - - 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; }; @@ -426,7 +126,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(); @@ -543,3 +243,45 @@ export function errorHandler(options?: { next(error); }; } + +// TODO (v8 / #5190): Remove this +/** + * Options deciding what parts of the request to use when enhancing an event + * @deprecated `Handlers.ParseRequestOptions` is deprecated. Use `AddRequestDataToEventOptions` in `@sentry/utils` instead. + */ +export type ParseRequestOptions = AddRequestDataToEventOptions; + +// TODO (v8 / #5190): Remove this +/** + * 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 + * @deprecated `Handlers.parseRequest` is deprecated. Use `addRequestDataToEvent` in `@sentry/utils` instead. + * @hidden + */ +// eslint-disable-next-line deprecation/deprecation +export function parseRequest(event: Event, req: ExpressRequest, options?: ParseRequestOptions): Event { + return addRequestDataToEvent(event, req, options); +} + +// TODO (v8 / #5190): Remove this +/** + * Normalizes data from the request object, accounting for framework differences. + * + * @deprecated `Handlers.extractRequestData` is deprecated. Use `extractRequestData` in `@sentry/utils` 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, keys); +} + +// TODO (v8 / #5190): Remove this +/** + * @deprecated `Handlers.ExpressRequest` is deprecated. Use `ExpressRequest` in `@sentry/utils` instead. + */ +// eslint-disable-next-line @typescript-eslint/no-empty-interface +export interface ExpressRequest extends _ExpressRequest {} diff --git a/packages/node/src/index.ts b/packages/node/src/index.ts index 96e2383c6ec0..ab456a9cb089 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, ExpressRequest } from '@sentry/utils'; export type { NodeOptions } from './types'; @@ -41,6 +42,7 @@ export { setUser, withScope, } from '@sentry/core'; +export { addRequestDataToEvent, extractRequestData } from '@sentry/utils'; export { NodeClient } from './client'; export { makeNodeTransport } from './transports'; diff --git a/packages/node/test/client.test.ts b/packages/node/test/client.test.ts index 2aafb8a2544e..584bf74de257 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,69 @@ 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()); + }); + }); }); describe('flush/close', () => { diff --git a/packages/node/test/handlers.test.ts b/packages/node/test/handlers.test.ts index 8cee24f678fd..22ef95fd6fd9 100644 --- a/packages/node/test/handlers.test.ts +++ b/packages/node/test/handlers.test.ts @@ -2,193 +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, Runtime } from '@sentry/types'; +import { Baggage, Event } from '@sentry/types'; import { 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.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']; - - 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'; @@ -289,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 }); @@ -299,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 }); @@ -311,6 +134,7 @@ describe('requestHandler', () => { setImmediate(() => { expect(res.finished).toBe(true); + done(); }); }); }); @@ -549,181 +373,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..d1bd486c47a5 --- /dev/null +++ b/packages/node/test/requestdata.test.ts @@ -0,0 +1,360 @@ +/* eslint-disable deprecation/deprecation */ + +/* Note: These tests 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 / #5190): Remove everything above + +import { addRequestDataToEvent, ExpressRequest, extractRequestData as newExtractRequestData } from '@sentry/utils'; +import * as net from 'net'; + +import { Event, Request, User } from '../src'; +import { extractRequestData as oldExtractRequestData, parseRequest } from '../src/handlers'; + +// TODO (v8 / #5190): Remove `describe.each` wrapper, use only `addRequestDataToEvent`, and move these tests to +// @sentry/utils +describe.each([parseRequest, addRequestDataToEvent])( + 'backwards compatibility of `parseRequest` rename and move', + fn => { + describe(fn, () => { + 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(`${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 parsedRequest: Event = fn({}, mockReq as ExpressRequest); + 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 parsedRequest: Event = fn({}, mockReq as ExpressRequest, { + user: CUSTOM_USER_KEYS, + }); + 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 parsedRequest: Event = fn( + {}, + { + ...mockReq, + // @ts-ignore user is not assignable to object + user: 'wat', + }, + ); + expect(Object.keys(parsedRequest.user as User)).toEqual([]); + }); + }); + + describe(`${fn.name}.ip property`, () => { + test('can be extracted from req.ip', () => { + const parsedRequest: Event = fn( + {}, + { + ...mockReq, + ip: '123', + } as ExpressRequest, + { + ip: true, + }, + ); + expect(parsedRequest.user!.ip_address).toEqual('123'); + }); + + test('can extract from req.connection.remoteAddress', () => { + const parsedRequest: Event = fn( + {}, + { + ...mockReq, + connection: { + remoteAddress: '321', + } as net.Socket, + } as ExpressRequest, + { + ip: true, + }, + ); + 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 parsedRequest: Event = fn({}, mockReq as ExpressRequest); + expect(Object.keys(parsedRequest.request as 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 parsedRequest: Event = fn({}, mockReq as ExpressRequest, { + request: INCLUDED_PROPERTIES, + }); + expect(Object.keys(parsedRequest.request as Request)).toEqual(INCLUDED_PROPERTIES); + }); + + test(`${fn.name}.request skips \`body\` property for GET and HEAD requests`, () => { + expect(fn({}, mockReq as ExpressRequest, {}).request).toHaveProperty('data'); + expect(fn({}, { ...mockReq, method: 'GET' } as ExpressRequest, {}).request).not.toHaveProperty('data'); + expect(fn({}, { ...mockReq, method: 'HEAD' } as ExpressRequest, {}).request).not.toHaveProperty('data'); + }); + }); + + describe(`${fn.name}.transaction property`, () => { + test('extracts method and full route path by default`', () => { + const parsedRequest: Event = fn({}, 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 = fn({}, 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 = fn({}, mockReq as ExpressRequest); + expect(parsedRequest.transaction).toEqual('POST /routerMountPath/subpath/specificValue'); + }); + + test('can extract path only instead if configured', () => { + const parsedRequest: Event = fn({}, mockReq as ExpressRequest, { transaction: 'path' }); + expect(parsedRequest.transaction).toEqual('/routerMountPath/subpath/:parameterName'); + }); + + test('can extract handler name instead if configured', () => { + const parsedRequest: Event = fn({}, mockReq as ExpressRequest, { + transaction: 'handler', + }); + expect(parsedRequest.transaction).toEqual('parameterNameRouteHandler'); + }); + }); + }); + }, +); + +// TODO (v8 / #5190): Remove `describe.each` wrapper, use only `newExtractRequestData`, rename `newExtractRequestData` +// to just `extractRequestData`, and move these tests to @sentry/utils +Object.defineProperty(oldExtractRequestData, 'name', { + value: 'oldExtractRequestData', +}); +Object.defineProperty(newExtractRequestData, 'name', { + value: 'newExtractRequestData', +}); +describe.each([oldExtractRequestData, newExtractRequestData])( + 'backwards compatibility of `extractRequestData` move', + fn => { + describe(fn, () => { + describe('default behaviour', () => { + test('node', () => { + expect( + fn({ + 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(fn({})).toEqual({ + cookies: {}, + headers: {}, + method: undefined, + query_string: null, + url: 'http://', + }); + }); + }); + + describe('cookies', () => { + it('uses `req.cookies` if available', () => { + expect( + fn( + { + cookies: { foo: 'bar' }, + }, + ['cookies'], + ), + ).toEqual({ + cookies: { foo: 'bar' }, + }); + }); + + it('parses the cookie header', () => { + expect( + fn( + { + headers: { + cookie: 'foo=bar;', + }, + }, + ['cookies'], + ), + ).toEqual({ + cookies: { foo: 'bar' }, + }); + }); + + it('falls back if no cookies are defined', () => { + expect(fn({}, ['cookies'])).toEqual({ + cookies: {}, + }); + }); + }); + + describe('data', () => { + it('includes data from `req.body` if available', () => { + expect( + fn( + { + 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( + fn( + { + 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( + fn( + { + 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(fn({}, ['query_string'])).toEqual({ + query_string: null, + }); + }); + }); + + describe('url', () => { + test('express/koa', () => { + expect( + fn( + { + host: 'example.com', + protocol: 'https', + url: '/', + }, + ['url'], + ), + ).toEqual({ + url: 'https://example.com/', + }); + }); + + test('node', () => { + expect( + fn( + { + headers: { host: 'example.com' }, + secure: true, + originalUrl: '/', + }, + ['url'], + ), + ).toEqual({ + url: 'https://example.com/', + }); + }); + }); + + describe('custom key', () => { + it('includes the custom key if present', () => { + expect( + fn( + { + httpVersion: '1.1', + }, + ['httpVersion'], + ), + ).toEqual({ + httpVersion: '1.1', + }); + }); + + it('gracefully degrades if the custom key is missing', () => { + expect(fn({}, ['httpVersion'])).toEqual({}); + }); + }); + }); + }, +); diff --git a/packages/serverless/src/awslambda.ts b/packages/serverless/src/awslambda.ts index 2514510af067..d0076c853773 100644 --- a/packages/serverless/src/awslambda.ts +++ b/packages/serverless/src/awslambda.ts @@ -182,11 +182,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/general.ts b/packages/serverless/src/gcpfunction/general.ts index 9c5f95615506..bb3573a5d7fd 100644 --- a/packages/serverless/src/gcpfunction/general.ts +++ b/packages/serverless/src/gcpfunction/general.ts @@ -1,7 +1,6 @@ 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 @@ -54,11 +53,6 @@ export interface WrapperOptions { * @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); } diff --git a/packages/serverless/src/gcpfunction/http.ts b/packages/serverless/src/gcpfunction/http.ts index 8596774299e1..d405eaa7cadc 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, parseBaggageString, 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 / #5190): Remove this +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 / #5190): Remove this + // 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); }); 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', diff --git a/packages/serverless/test/awslambda.test.ts b/packages/serverless/test/awslambda.test.ts index 078eff338ea9..66f2cfb5343b 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); @@ -197,7 +195,7 @@ describe('AWSLambda', () => { }); test('unsuccessful execution', async () => { - expect.assertions(10); + expect.assertions(9); const error = new Error('sorry'); const handler: Handler = (_event, _context, callback) => { @@ -264,7 +262,7 @@ describe('AWSLambda', () => { }); test('capture error', async () => { - expect.assertions(10); + expect.assertions(9); const error = new Error('wat'); const handler: Handler = (_event, _context, _callback) => { @@ -294,7 +292,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; @@ -321,7 +319,7 @@ describe('AWSLambda', () => { }); test('capture error', async () => { - expect.assertions(10); + expect.assertions(9); const error = new Error('wat'); const handler: Handler = async (_event, _context) => { @@ -359,7 +357,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; @@ -386,7 +384,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 89b6aa084b9b..2d96e13faea7 100644 --- a/packages/serverless/test/gcpfunction.test.ts +++ b/packages/serverless/test/gcpfunction.test.ts @@ -212,7 +212,7 @@ describe('GCPFunction', () => { }); test('wrapHttpFunction request data', async () => { - expect.assertions(7); + expect.assertions(6); const handler: HttpFunction = (_req, res) => { res.end(); @@ -223,7 +223,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'); @@ -362,16 +361,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', @@ -466,16 +461,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' }); }); diff --git a/packages/utils/package.json b/packages/utils/package.json index c5ac5df959c6..8584533aebe9 100644 --- a/packages/utils/package.json +++ b/packages/utils/package.json @@ -24,6 +24,9 @@ "array.prototype.flat": "^1.3.0", "chai": "^4.1.2" }, + "peerDependencies": { + "cookie": "^0.4.1" + }, "scripts": { "build": "run-p build:rollup build:types", "build:dev": "run-s build", 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..1398c3839631 --- /dev/null +++ b/packages/utils/src/requestdata.ts @@ -0,0 +1,306 @@ +/** + * 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 / #5190): 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 * as cookie from 'cookie'; +import * as url from 'url'; + +import { isPlainObject, isString } from './is'; +import { stripUrlQueryAndFragment } from './misc'; +import { normalize } from './normalize'; + +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; + }; +} + +/** + * Sets parameterized route as transaction name e.g.: `GET /users/:id` + * Also adds more context data on the transaction from the request + */ +export 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 + */ +export function extractExpressTransactionName( + req: ExpressRequest, + options: { path?: boolean; method?: boolean } = {}, +): string { + const method = req.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 && req.route && req.route.stack[0] && 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 AddRequestDataToEventOptions { + 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 addRequestDataToEvent( + event: Event, + req: ExpressRequest, + options?: AddRequestDataToEventOptions, +): 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; +}