From 05709dc5859eb151596ad7a6e2b5f6439e401bdd Mon Sep 17 00:00:00 2001 From: Katie Byers Date: Thu, 15 Sep 2022 23:23:52 -0700 Subject: [PATCH] move request data functions back to node --- packages/node/src/handlers.ts | 3 +- packages/node/src/index.ts | 13 +- packages/node/src/requestDataDeprecated.ts | 9 +- packages/node/src/requestdata.ts | 318 +++++++++++++++++++++ packages/node/src/sdk.ts | 55 +--- packages/node/test/requestdata.test.ts | 46 +-- packages/utils/src/index.ts | 1 + packages/utils/src/requestdata.ts | 2 + 8 files changed, 339 insertions(+), 108 deletions(-) create mode 100644 packages/node/src/requestdata.ts diff --git a/packages/node/src/handlers.ts b/packages/node/src/handlers.ts index c29d5d836efe..68155cb9559a 100644 --- a/packages/node/src/handlers.ts +++ b/packages/node/src/handlers.ts @@ -14,10 +14,11 @@ import * as domain from 'domain'; import * as http from 'http'; import { NodeClient } from './client'; +import { addRequestDataToEvent, extractRequestData } from './requestdata'; // TODO (v8 / XXX) Remove these imports import type { ParseRequestOptions } from './requestDataDeprecated'; import { parseRequest } from './requestDataDeprecated'; -import { addRequestDataToEvent, extractRequestData, flush, isAutoSessionTrackingEnabled } from './sdk'; +import { flush, isAutoSessionTrackingEnabled } from './sdk'; /** * Express-compatible tracing handler. diff --git a/packages/node/src/index.ts b/packages/node/src/index.ts index 29f5da167782..a637e6c44865 100644 --- a/packages/node/src/index.ts +++ b/packages/node/src/index.ts @@ -46,17 +46,8 @@ export { export { NodeClient } from './client'; export { makeNodeTransport } from './transports'; -export { - addRequestDataToEvent, - extractRequestData, - defaultIntegrations, - init, - defaultStackParser, - lastEventId, - flush, - close, - getSentryRelease, -} from './sdk'; +export { defaultIntegrations, init, defaultStackParser, lastEventId, flush, close, getSentryRelease } from './sdk'; +export { addRequestDataToEvent, extractRequestData } from './requestdata'; export { deepReadDirSync } from './utils'; import { Integrations as CoreIntegrations } from '@sentry/core'; diff --git a/packages/node/src/requestDataDeprecated.ts b/packages/node/src/requestDataDeprecated.ts index 75f1cca7b65f..7e7ce5c5d241 100644 --- a/packages/node/src/requestDataDeprecated.ts +++ b/packages/node/src/requestDataDeprecated.ts @@ -7,13 +7,12 @@ /* eslint-disable deprecation/deprecation */ /* eslint-disable @typescript-eslint/no-explicit-any */ import { Event, ExtractedNodeRequestData, PolymorphicRequest } from '@sentry/types'; + import { addRequestDataToEvent, AddRequestDataToEventOptions, extractRequestData as _extractRequestData, -} from '@sentry/utils'; -import * as cookie from 'cookie'; -import * as url from 'url'; +} from './requestdata'; /** * @deprecated `Handlers.ExpressRequest` is deprecated and will be removed in v8. Use `PolymorphicRequest` instead. @@ -30,7 +29,7 @@ export type ExpressRequest = PolymorphicRequest; * @returns An object containing normalized request data */ export function extractRequestData(req: { [key: string]: any }, keys?: string[]): ExtractedNodeRequestData { - return _extractRequestData(req, { include: keys, deps: { cookie, url } }); + return _extractRequestData(req, { include: keys }); } /** @@ -55,5 +54,5 @@ export type ParseRequestOptions = AddRequestDataToEventOptions['include'] & { * @hidden */ export function parseRequest(event: Event, req: ExpressRequest, options: ParseRequestOptions = {}): Event { - return addRequestDataToEvent(event, req, { include: options, deps: { cookie, url } }); + return addRequestDataToEvent(event, req, { include: options }); } diff --git a/packages/node/src/requestdata.ts b/packages/node/src/requestdata.ts new file mode 100644 index 000000000000..058f2ea75c68 --- /dev/null +++ b/packages/node/src/requestdata.ts @@ -0,0 +1,318 @@ +import { Event, ExtractedNodeRequestData, PolymorphicRequest, Transaction, TransactionSource } from '@sentry/types'; +import { isPlainObject, isString, normalize, stripUrlQueryAndFragment } from '@sentry/utils/'; +import * as cookie from 'cookie'; +import * as url from 'url'; + +const DEFAULT_INCLUDES = { + ip: false, + request: true, + transaction: true, + user: true, +}; +const DEFAULT_REQUEST_INCLUDES = ['cookies', 'data', 'headers', 'method', 'query_string', 'url']; +const DEFAULT_USER_INCLUDES = ['id', 'username', 'email']; + +/** + * Options deciding what parts of the request to use when enhancing an event + */ +export interface AddRequestDataToEventOptions { + /** Flags controlling whether each type of data should be added to the event */ + include?: { + ip?: boolean; + request?: boolean | Array; + transaction?: boolean | TransactionNamingScheme; + user?: boolean | Array; + }; +} + +type TransactionNamingScheme = 'path' | 'methodPath' | 'handler'; + +/** + * Sets parameterized route as transaction name e.g.: `GET /users/:id` + * Also adds more context data on the transaction from the request + */ +export function addRequestDataToTransaction(transaction: Transaction | undefined, req: PolymorphicRequest): void { + if (!transaction) return; + if (!transaction.metadata.source || transaction.metadata.source === 'url') { + // Attempt to grab a parameterized route off of the request + transaction.setName(...extractPathForTransaction(req, { path: true, method: true })); + } + transaction.setData('url', req.originalUrl || req.url); + if (req.baseUrl) { + transaction.setData('baseUrl', req.baseUrl); + } + transaction.setData('query', extractQueryParams(req)); +} + +/** + * Extracts a complete and parameterized path from the request object and uses it to construct transaction name. + * If the parameterized transaction name cannot be extracted, we fall back to the raw URL. + * + * Additionally, this function determines and returns the transaction name source + * + * eg. GET /mountpoint/user/:id + * + * @param req A request object + * @param options What to include in the transaction name (method, path, or a custom route name to be + * used instead of the request's route) + * + * @returns A tuple of the fully constructed transaction name [0] and its source [1] (can be either 'route' or 'url') + */ +export function extractPathForTransaction( + req: PolymorphicRequest, + options: { path?: boolean; method?: boolean; customRoute?: string } = {}, +): [string, TransactionSource] { + const method = req.method && req.method.toUpperCase(); + + let path = ''; + let source: TransactionSource = 'url'; + + // Check to see if there's a parameterized route we can use (as there is in Express) + if (options.customRoute || req.route) { + path = options.customRoute || `${req.baseUrl || ''}${req.route && req.route.path}`; + source = 'route'; + } + + // Otherwise, just take the original URL + else if (req.originalUrl || req.url) { + path = stripUrlQueryAndFragment(req.originalUrl || req.url || ''); + } + + let name = ''; + if (options.method && method) { + name += method; + } + if (options.method && options.path) { + name += ' '; + } + if (options.path && path) { + name += path; + } + + return [name, source]; +} + +/** JSDoc */ +function extractTransaction(req: PolymorphicRequest, type: boolean | TransactionNamingScheme): string { + switch (type) { + case 'path': { + return extractPathForTransaction(req, { path: true })[0]; + } + case 'handler': { + return (req.route && req.route.stack && req.route.stack[0] && req.route.stack[0].name) || ''; + } + case 'methodPath': + default: { + return extractPathForTransaction(req, { path: true, method: true })[0]; + } + } +} + +/** JSDoc */ +function extractUserData( + user: { + [key: string]: unknown; + }, + keys: boolean | string[], +): { [key: string]: unknown } { + const extractedUser: { [key: string]: unknown } = {}; + const attributes = Array.isArray(keys) ? keys : DEFAULT_USER_INCLUDES; + + attributes.forEach(key => { + if (user && key in user) { + extractedUser[key] = user[key]; + } + }); + + return extractedUser; +} + +/** + * Normalize data from the request object + * + * @param req The request object from which to extract data + * @param options.include An optional array of keys to include in the normalized data. Defaults to + * DEFAULT_REQUEST_INCLUDES if not provided. + * @param options.deps Injected, platform-specific dependencies + * + * @returns An object containing normalized request data + */ +export function extractRequestData( + req: PolymorphicRequest, + options?: { + include?: string[]; + }, +): ExtractedNodeRequestData { + const { include = DEFAULT_REQUEST_INCLUDES } = options || {}; + const requestData: { [key: string]: unknown } = {}; + + // headers: + // node, express, koa, nextjs: req.headers + const headers = (req.headers || {}) as { + host?: string; + cookie?: string; + }; + // method: + // node, express, koa, nextjs: req.method + const method = req.method; + // host: + // express: req.hostname in > 4 and req.host in < 4 + // koa: req.host + // node, nextjs: req.headers.host + const host = req.hostname || req.host || headers.host || ''; + // protocol: + // node, nextjs: + // express, koa: req.protocol + const protocol = req.protocol === 'https' || (req.socket && req.socket.encrypted) ? 'https' : 'http'; + // url (including path and query string): + // node, express: req.originalUrl + // koa, nextjs: req.url + const originalUrl = req.originalUrl || req.url || ''; + // absolute url + const absoluteUrl = `${protocol}://${host}${originalUrl}`; + include.forEach(key => { + switch (key) { + case 'headers': { + requestData.headers = headers; + break; + } + case 'method': { + requestData.method = method; + break; + } + case 'url': { + requestData.url = absoluteUrl; + break; + } + case 'cookies': { + // cookies: + // node, express, koa: req.headers.cookie + // vercel, sails.js, express (w/ cookie middleware), nextjs: req.cookies + // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access + requestData.cookies = + // TODO (v8 / #5257): We're only sending the empty object for backwards compatibility, so the last bit can + // come off in v8 + req.cookies || (headers.cookie && cookie.parse(headers.cookie)) || {}; + break; + } + case 'query_string': { + // query string: + // node: req.url (raw) + // express, koa, nextjs: req.query + // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access + requestData.query_string = extractQueryParams(req); + 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]: unknown })[key]; + } + } + } + }); + + return requestData; +} + +/** + * Add data from the given request to the given event + * + * @param event The event to which the request data will be added + * @param req Request object + * @param options.include Flags to control what data is included + * + * @returns The mutated `Event` object + */ +export function addRequestDataToEvent( + event: Event, + req: PolymorphicRequest, + options?: AddRequestDataToEventOptions, +): Event { + const include = { + ...DEFAULT_INCLUDES, + ...options?.include, + }; + + if (include.request) { + const extractedRequestData = Array.isArray(include.request) + ? extractRequestData(req, { include: include.request }) + : extractRequestData(req); + + event.request = { + ...event.request, + ...extractedRequestData, + }; + } + + if (include.user) { + const extractedUser = req.user && isPlainObject(req.user) ? extractUserData(req.user, include.user) : {}; + + if (Object.keys(extractedUser).length) { + event.user = { + ...event.user, + ...extractedUser, + }; + } + } + + // client ip: + // node, nextjs: req.socket.remoteAddress + // express, koa: req.ip + if (include.ip) { + const ip = req.ip || (req.socket && req.socket.remoteAddress); + if (ip) { + event.user = { + ...event.user, + ip_address: ip, + }; + } + } + + if (include.transaction && !event.transaction) { + // TODO do we even need this anymore? + // TODO make this work for nextjs + event.transaction = extractTransaction(req, include.transaction); + } + + return event; +} + +function extractQueryParams(req: PolymorphicRequest): string | Record | undefined { + // url (including path and query string): + // node, express: req.originalUrl + // koa, nextjs: req.url + let originalUrl = req.originalUrl || req.url || ''; + + if (!originalUrl) { + return; + } + + // The `URL` constructor can't handle internal URLs of the form `/some/path/here`, so stick a dummy protocol and + // hostname on the beginning. Since the point here is just to grab the query string, it doesn't matter what we use. + if (originalUrl.startsWith('/')) { + originalUrl = `http://dogs.are.great${originalUrl}`; + } + + return ( + req.query || + (typeof URL !== undefined && new URL(originalUrl).search.replace('?', '')) || + // In Node 8, `URL` isn't in the global scope, so we have to use the built-in module from Node + url.parse(originalUrl).query || + undefined + ); +} diff --git a/packages/node/src/sdk.ts b/packages/node/src/sdk.ts index bb4c3bad7018..9f4a7eb3ee3c 100644 --- a/packages/node/src/sdk.ts +++ b/packages/node/src/sdk.ts @@ -1,20 +1,15 @@ /* eslint-disable max-lines */ import { getCurrentHub, getIntegrationsToSetup, initAndBind, Integrations as CoreIntegrations } from '@sentry/core'; import { getMainCarrier, setHubOnCarrier } from '@sentry/hub'; -import { Event, ExtractedNodeRequestData, PolymorphicRequest, SessionStatus, StackParser } from '@sentry/types'; +import { SessionStatus, StackParser } from '@sentry/types'; import { - addRequestDataToEvent as _addRequestDataToEvent, - AddRequestDataToEventOptions, createStackParser, - extractRequestData as _extractRequestData, getGlobalObject, logger, nodeStackLineParser, stackParserFromStackParserOptions, } from '@sentry/utils'; -import * as cookie from 'cookie'; import * as domain from 'domain'; -import * as url from 'url'; import { NodeClient } from './client'; import { @@ -278,51 +273,3 @@ function startSessionTracking(): void { if (session && !terminalStates.includes(session.status)) hub.endSession(); }); } - -/** - * Add data from the given request to the given event - * - * (Note that there is no sister function to this one in `@sentry/browser`, because the whole point of this wrapper is - * to pass along injected dependencies, which isn't necessary in a browser context. Isomorphic packages like - * `@sentry/nextjs` should export directly from `@sentry/utils` in their browser index file.) - * - * @param event The event to which the request data will be added - * @param req Request object - * @param options.include Flags to control what data is included - * @hidden - */ -export function addRequestDataToEvent( - event: Event, - req: PolymorphicRequest, - options?: Omit, -): Event { - return _addRequestDataToEvent(event, req, { - ...options, - // We have to inject these node-only dependencies because we can't import them in `@sentry/utils`, where the - // original function lives - deps: { cookie, url }, - }); -} - -/** - * Normalize data from the request object, accounting for framework differences. - * - * (Note that there is no sister function to this one in `@sentry/browser`, because the whole point of this wrapper is - * to inject dependencies, which isn't necessary in a browser context. Isomorphic packages like `@sentry/nextjs` should - * export directly from `@sentry/utils` in their browser index file.) - * - * @param req The request object from which to extract data - * @param options.keys An optional array of keys to include in the normalized data. Defaults to DEFAULT_REQUEST_KEYS if - * not provided. - * @returns An object containing normalized request data - */ -export function extractRequestData( - req: PolymorphicRequest, - options?: { - include?: string[]; - }, -): ExtractedNodeRequestData { - // We have to inject these node-only dependencies because we can't import them in `@sentry/utils`, where the original - // function lives - return _extractRequestData(req, { ...options, deps: { cookie, url } }); -} diff --git a/packages/node/test/requestdata.test.ts b/packages/node/test/requestdata.test.ts index 94b1ee939286..9008aff782f6 100644 --- a/packages/node/test/requestdata.test.ts +++ b/packages/node/test/requestdata.test.ts @@ -1,32 +1,24 @@ /* eslint-disable deprecation/deprecation */ -/* Note: These tests (except for the ones related to cookies) should eventually live in `@sentry/utils`, and can be - * moved there once the the backwards-compatibility-preserving wrappers in `handlers.ts` are removed. - */ - -// TODO (v8 / #5257): Remove everything above +// TODO (v8 / #5257): Remove everything related to the deprecated functions import { Event, PolymorphicRequest, TransactionSource, User } from '@sentry/types'; +import * as net from 'net'; + import { addRequestDataToEvent, AddRequestDataToEventOptions, extractPathForTransaction, extractRequestData as newExtractRequestData, -} from '@sentry/utils'; -import * as cookie from 'cookie'; -import * as net from 'net'; -import * as url from 'url'; - +} from '../src/requestdata'; import { ExpressRequest, extractRequestData as oldExtractRequestData, parseRequest, } from '../src/requestDataDeprecated'; -const mockCookieModule = { parse: jest.fn() }; - -// TODO (v8 / #5257): Remove `describe.each` wrapper, remove `formatArgs` wrapper, reformat args in tests, use only -// `addRequestDataToEvent`, and move these tests to @sentry/utils +// TODO (v8 / #5257): Remove `describe.each` wrapper, remove `formatArgs` wrapper, reformat args in tests, and use only +// `addRequestDataToEvent` describe.each([parseRequest, addRequestDataToEvent])( 'backwards compatibility of `parseRequest` rename and move', fn => { @@ -40,17 +32,7 @@ describe.each([parseRequest, addRequestDataToEvent])( if (fn.name === 'parseRequest') { return [event, req as ExpressRequest, include]; } else { - return [ - event, - req as PolymorphicRequest, - { - include, - deps: { - cookie: mockCookieModule, - url, - }, - }, - ]; + return [event, req as PolymorphicRequest, { include }]; } } @@ -255,8 +237,7 @@ describe.each([parseRequest, addRequestDataToEvent])( ); // TODO (v8 / #5257): Remove `describe.each` wrapper, remove `formatArgs` wrapper, reformat args in tests, use only -// `newExtractRequestData`, rename `newExtractRequestData` to just `extractRequestData`, and move these tests (except -// the ones involving cookies) to @sentry/utils (use `mockCookieModule` for others) +// `newExtractRequestData`, and rename `newExtractRequestData` to just `extractRequestData` Object.defineProperty(oldExtractRequestData, 'name', { value: 'oldExtractRequestData', }); @@ -275,16 +256,7 @@ describe.each([oldExtractRequestData, newExtractRequestData])( if (fn.name === 'oldExtractRequestData') { return [req as ExpressRequest, include] as Parameters; } else { - return [ - req as PolymorphicRequest, - { - include, - deps: { - cookie: include?.includes('cookies') ? cookie : mockCookieModule, - url, - }, - }, - ] as Parameters; + return [req as PolymorphicRequest, { include }] as Parameters; } } diff --git a/packages/utils/src/index.ts b/packages/utils/src/index.ts index b221bef38c91..b55e3b0303be 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'; +// TODO: Remove requestdata export once equivalent integration is used everywhere export * from './requestdata'; export * from './severity'; export * from './stacktrace'; diff --git a/packages/utils/src/requestdata.ts b/packages/utils/src/requestdata.ts index 3c8c11589575..4f914bfe16c0 100644 --- a/packages/utils/src/requestdata.ts +++ b/packages/utils/src/requestdata.ts @@ -1,3 +1,5 @@ +// TODO: Remove this file once equivalent integration is used everywhere + /* eslint-disable complexity */ /** * The functions here, which enrich an event with request data, are mostly for use in Node, but are safe for use in a