From 52e945a29afbb4ea8193d3ecd7213bd389d15185 Mon Sep 17 00:00:00 2001 From: Abhijeet Prasad Date: Tue, 7 Jun 2022 19:31:05 -0400 Subject: [PATCH] Revert "ref(node): Move non-handler code out of `handlers` module (#5190)" (#5223) This reverts commit 7c521e3cc4a6a8caa685e2f1316a133b3acae212. This PR causes issues with users as reported here: #5222. This is because we added Node library imports in the utils package - which can be consumed in browser contexts. --- 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, 718 insertions(+), 843 deletions(-) delete mode 100644 packages/node/test/requestdata.test.ts delete mode 100644 packages/utils/src/requestdata.ts diff --git a/packages/nextjs/src/utils/instrumentServer.ts b/packages/nextjs/src/utils/instrumentServer.ts index 414d99e5c771..d56e8d735f71 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,6 +22,8 @@ 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 }; @@ -244,7 +246,7 @@ function makeWrappedReqHandler(origReqHandler: ReqHandler): WrappedReqHandler { const currentScope = getCurrentHub().getScope(); if (currentScope) { - currentScope.addEventProcessor(event => addRequestDataToEvent(event, nextReq)); + currentScope.addEventProcessor(event => parseRequest(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 e9cd6b1e5d7e..fa56bbaf2867 100644 --- a/packages/nextjs/src/utils/withSentry.ts +++ b/packages/nextjs/src/utils/withSentry.ts @@ -1,4 +1,4 @@ -import { addRequestDataToEvent, captureException, flush, getCurrentHub, startTransaction } from '@sentry/node'; +import { captureException, flush, getCurrentHub, Handlers, startTransaction } from '@sentry/node'; import { extractTraceparentData, hasTracingEnabled } from '@sentry/tracing'; import { Transaction } from '@sentry/types'; import { @@ -12,6 +12,8 @@ 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; @@ -41,7 +43,7 @@ export const withSentry = (origHandler: NextApiHandler): WrappedNextApiHandler = const currentScope = getCurrentHub().getScope(); if (currentScope) { - currentScope.addEventProcessor(event => addRequestDataToEvent(event, req)); + currentScope.addEventProcessor(event => parseRequest(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 c222bbe08b91..144abaec240b 100644 --- a/packages/node/src/client.ts +++ b/packages/node/src/client.ts @@ -2,7 +2,6 @@ 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'; @@ -140,14 +139,9 @@ export class NodeClient extends BaseClient { */ protected _prepareEvent(event: Event, hint: EventHint, scope?: Scope): PromiseLike { event.platform = event.platform || 'node'; - event.contexts = { - ...event.contexts, - runtime: { - name: 'node', - version: global.process.version, - }, - }; - event.server_name = this.getOptions().serverName || global.process.env.SENTRY_NAME || os.hostname(); + if (this.getOptions().serverName) { + event.server_name = this.getOptions().serverName; + } return super._prepareEvent(event, hint, scope); } diff --git a/packages/node/src/handlers.ts b/packages/node/src/handlers.ts index da744154b19d..9bd7c6035fd1 100644 --- a/packages/node/src/handlers.ts +++ b/packages/node/src/handlers.ts @@ -1,24 +1,51 @@ +/* eslint-disable max-lines */ /* eslint-disable @typescript-eslint/no-explicit-any */ import { captureException, getCurrentHub, startTransaction, withScope } from '@sentry/core'; -import { Event, ExtractedNodeRequestData, Span } from '@sentry/types'; +import { Event, ExtractedNodeRequestData, Span, Transaction } 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` @@ -46,7 +73,6 @@ export function tracingHandler(): ( ...(baggage && { metadata: { baggage: baggage } }), }, // extra context passed to the tracesSampler - // eslint-disable-next-line deprecation/deprecation { request: extractRequestData(req) }, ); @@ -74,7 +100,281 @@ export function tracingHandler(): ( }; } -export type RequestHandlerOptions = AddRequestDataToEventOptions & { +/** + * 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 & { flushTimeout?: number; }; @@ -126,7 +426,7 @@ export function requestHandler( const currentHub = getCurrentHub(); currentHub.configureScope(scope => { - scope.addEventProcessor((event: Event) => addRequestDataToEvent(event, req, options)); + scope.addEventProcessor((event: Event) => parseRequest(event, req, options)); const client = currentHub.getClient(); if (isAutoSessionTrackingEnabled(client)) { const scope = currentHub.getScope(); @@ -243,45 +543,3 @@ 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 ab456a9cb089..96e2383c6ec0 100644 --- a/packages/node/src/index.ts +++ b/packages/node/src/index.ts @@ -15,7 +15,6 @@ export type { Thread, User, } from '@sentry/types'; -export type { AddRequestDataToEventOptions, ExpressRequest } from '@sentry/utils'; export type { NodeOptions } from './types'; @@ -42,7 +41,6 @@ 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 584bf74de257..2aafb8a2544e 100644 --- a/packages/node/test/client.test.ts +++ b/packages/node/test/client.test.ts @@ -1,6 +1,4 @@ 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'; @@ -189,69 +187,6 @@ 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 22ef95fd6fd9..8cee24f678fd 100644 --- a/packages/node/test/handlers.test.ts +++ b/packages/node/test/handlers.test.ts @@ -2,15 +2,193 @@ import * as sentryCore from '@sentry/core'; import * as sentryHub from '@sentry/hub'; import { Hub } from '@sentry/hub'; import { Transaction } from '@sentry/tracing'; -import { Baggage, Event } from '@sentry/types'; +import { Baggage, Runtime } 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, requestHandler, tracingHandler } from '../src/handlers'; +import { + errorHandler, + ExpressRequest, + extractRequestData, + parseRequest, + 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'; @@ -111,7 +289,7 @@ describe('requestHandler', () => { }); }); - it('patches `res.end` when `flushTimeout` is specified', done => { + it('patches `res.end` when `flushTimeout` is specified', () => { const flush = jest.spyOn(SDK, 'flush').mockResolvedValue(true); const sentryRequestMiddleware = requestHandler({ flushTimeout: 1337 }); @@ -121,11 +299,10 @@ describe('requestHandler', () => { setImmediate(() => { expect(flush).toHaveBeenCalledWith(1337); expect(res.finished).toBe(true); - done(); }); }); - it('prevents errors thrown during `flush` from breaking the response', done => { + it('prevents errors thrown during `flush` from breaking the response', async () => { jest.spyOn(SDK, 'flush').mockRejectedValue(new SentryError('HTTP Error (429)')); const sentryRequestMiddleware = requestHandler({ flushTimeout: 1337 }); @@ -134,7 +311,6 @@ describe('requestHandler', () => { setImmediate(() => { expect(res.finished).toBe(true); - done(); }); }); }); @@ -373,6 +549,181 @@ 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 deleted file mode 100644 index d1bd486c47a5..000000000000 --- a/packages/node/test/requestdata.test.ts +++ /dev/null @@ -1,360 +0,0 @@ -/* 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 5e980233e2f2..8fce09c87dbf 100644 --- a/packages/serverless/src/awslambda.ts +++ b/packages/serverless/src/awslambda.ts @@ -180,6 +180,11 @@ 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 bb3573a5d7fd..9c5f95615506 100644 --- a/packages/serverless/src/gcpfunction/general.ts +++ b/packages/serverless/src/gcpfunction/general.ts @@ -1,6 +1,7 @@ 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 @@ -53,6 +54,11 @@ 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 d405eaa7cadc..8596774299e1 100644 --- a/packages/serverless/src/gcpfunction/http.ts +++ b/packages/serverless/src/gcpfunction/http.ts @@ -1,29 +1,17 @@ -import { - addRequestDataToEvent, - AddRequestDataToEventOptions, - captureException, - flush, - getCurrentHub, - startTransaction, -} from '@sentry/node'; +import { captureException, flush, getCurrentHub, Handlers, 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'; -// TODO (v8 / #5190): Remove this -interface OldHttpFunctionWrapperOptions extends WrapperOptions { - /** - * @deprecated Use `addRequestDataToEventOptions` instead. - */ - parseRequestOptions: AddRequestDataToEventOptions; -} -interface NewHttpFunctionWrapperOptions extends WrapperOptions { - addRequestDataToEventOptions: AddRequestDataToEventOptions; +type ParseRequestOptions = Handlers.ParseRequestOptions; + +export interface HttpFunctionWrapperOptions extends WrapperOptions { + parseRequestOptions: ParseRequestOptions; } -export type HttpFunctionWrapperOptions = OldHttpFunctionWrapperOptions | NewHttpFunctionWrapperOptions; +const { parseRequest } = Handlers; /** * Wraps an HTTP function handler adding it error capture and tracing capabilities. @@ -52,13 +40,9 @@ 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, - addRequestDataToEventOptions: parseRequestOptions ? parseRequestOptions : {}, + parseRequestOptions: {}, ...wrapOptions, }; return (req, res) => { @@ -88,7 +72,7 @@ function _wrapHttpFunction(fn: HttpFunction, wrapOptions: Partial { - scope.addEventProcessor(event => addRequestDataToEvent(event, req, options.addRequestDataToEventOptions)); + scope.addEventProcessor(event => parseRequest(event, req, options.parseRequestOptions)); // 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 14043efdd42c..769d22f6e2f6 100644 --- a/packages/serverless/test/__mocks__/@sentry/node.ts +++ b/packages/serverless/test/__mocks__/@sentry/node.ts @@ -1,7 +1,6 @@ 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 66f2cfb5343b..078eff338ea9 100644 --- a/packages/serverless/test/awslambda.test.ts +++ b/packages/serverless/test/awslambda.test.ts @@ -46,6 +46,8 @@ 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({ @@ -179,7 +181,7 @@ describe('AWSLambda', () => { describe('wrapHandler() on sync handler', () => { test('successful execution', async () => { - expect.assertions(9); + expect.assertions(10); const handler: Handler = (_event, _context, callback) => { callback(null, 42); @@ -195,7 +197,7 @@ describe('AWSLambda', () => { }); test('unsuccessful execution', async () => { - expect.assertions(9); + expect.assertions(10); const error = new Error('sorry'); const handler: Handler = (_event, _context, callback) => { @@ -262,7 +264,7 @@ describe('AWSLambda', () => { }); test('capture error', async () => { - expect.assertions(9); + expect.assertions(10); const error = new Error('wat'); const handler: Handler = (_event, _context, _callback) => { @@ -292,7 +294,7 @@ describe('AWSLambda', () => { describe('wrapHandler() on async handler', () => { test('successful execution', async () => { - expect.assertions(9); + expect.assertions(10); const handler: Handler = async (_event, _context) => { return 42; @@ -319,7 +321,7 @@ describe('AWSLambda', () => { }); test('capture error', async () => { - expect.assertions(9); + expect.assertions(10); const error = new Error('wat'); const handler: Handler = async (_event, _context) => { @@ -357,7 +359,7 @@ describe('AWSLambda', () => { describe('wrapHandler() on async handler with a callback method (aka incorrect usage)', () => { test('successful execution', async () => { - expect.assertions(9); + expect.assertions(10); const handler: Handler = async (_event, _context, _callback) => { return 42; @@ -384,7 +386,7 @@ describe('AWSLambda', () => { }); test('capture error', async () => { - expect.assertions(9); + expect.assertions(10); 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 2d96e13faea7..89b6aa084b9b 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(6); + expect.assertions(7); const handler: HttpFunction = (_req, res) => { res.end(); @@ -223,6 +223,7 @@ 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'); @@ -361,12 +362,16 @@ describe('GCPFunction', () => { }); test('wrapEventFunction scope data', async () => { - expect.assertions(1); + expect.assertions(3); 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', @@ -461,12 +466,16 @@ describe('GCPFunction', () => { }); test('wrapCloudEventFunction scope data', async () => { - expect.assertions(1); + expect.assertions(3); 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 29a13d9747cb..0f16ed335917 100644 --- a/packages/utils/package.json +++ b/packages/utils/package.json @@ -24,9 +24,6 @@ "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 7c8943da813f..fd627db6f2e5 100644 --- a/packages/utils/src/index.ts +++ b/packages/utils/src/index.ts @@ -12,7 +12,6 @@ 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 deleted file mode 100644 index 1398c3839631..000000000000 --- a/packages/utils/src/requestdata.ts +++ /dev/null @@ -1,306 +0,0 @@ -/** - * 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; -}