From bea2d552db82526f7b5e415e6d64bd69e280070f Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Mon, 23 Sep 2024 10:14:26 +0200 Subject: [PATCH 01/16] Add node-koa tests --- .../test-applications/node-koa/index.js | 6 + .../test-applications/node-koa/package.json | 1 + .../node-koa/tests/assert.test.ts | 1 - .../node-koa/tests/errors.test.ts | 1 - .../node-koa/tests/transactions.test.ts | 192 +++++++++++------- 5 files changed, 129 insertions(+), 72 deletions(-) diff --git a/dev-packages/e2e-tests/test-applications/node-koa/index.js b/dev-packages/e2e-tests/test-applications/node-koa/index.js index ddc17f62e6f7..9e800a4fcc99 100644 --- a/dev-packages/e2e-tests/test-applications/node-koa/index.js +++ b/dev-packages/e2e-tests/test-applications/node-koa/index.js @@ -14,10 +14,12 @@ const port1 = 3030; const port2 = 3040; const Koa = require('koa'); +const { bodyParser } = require('@koa/bodyparser'); const Router = require('@koa/router'); const http = require('http'); const app1 = new Koa(); +app1.use(bodyParser()); Sentry.setupKoaErrorHandler(app1); @@ -109,6 +111,10 @@ router1.get('/test-assert/:condition', async ctx => { ctx.assert(condition, 400, 'ctx.assert failed'); }); +router1.post('/test-post', async ctx => { + ctx.body = { status: 'ok', body: ctx.request.body }; +}); + app1.use(router1.routes()).use(router1.allowedMethods()); app1.listen(port1); diff --git a/dev-packages/e2e-tests/test-applications/node-koa/package.json b/dev-packages/e2e-tests/test-applications/node-koa/package.json index 79a4e540c089..dd8a17d0f4b5 100644 --- a/dev-packages/e2e-tests/test-applications/node-koa/package.json +++ b/dev-packages/e2e-tests/test-applications/node-koa/package.json @@ -10,6 +10,7 @@ "test:assert": "pnpm test" }, "dependencies": { + "@koa/bodyparser": "^5.1.1", "@koa/router": "^12.0.1", "@sentry/node": "latest || *", "@sentry/types": "latest || *", diff --git a/dev-packages/e2e-tests/test-applications/node-koa/tests/assert.test.ts b/dev-packages/e2e-tests/test-applications/node-koa/tests/assert.test.ts index 0f9f724ef237..5bf468fc3772 100644 --- a/dev-packages/e2e-tests/test-applications/node-koa/tests/assert.test.ts +++ b/dev-packages/e2e-tests/test-applications/node-koa/tests/assert.test.ts @@ -16,7 +16,6 @@ test('Returns 400 from failed assert', async ({ baseURL }) => { expect(errorEvent.request).toEqual({ method: 'GET', - cookies: {}, headers: expect.any(Object), url: 'http://localhost:3030/test-assert/false', }); diff --git a/dev-packages/e2e-tests/test-applications/node-koa/tests/errors.test.ts b/dev-packages/e2e-tests/test-applications/node-koa/tests/errors.test.ts index 1fbb1fd42613..03216b5cc01e 100644 --- a/dev-packages/e2e-tests/test-applications/node-koa/tests/errors.test.ts +++ b/dev-packages/e2e-tests/test-applications/node-koa/tests/errors.test.ts @@ -15,7 +15,6 @@ test('Sends correct error event', async ({ baseURL }) => { expect(errorEvent.request).toEqual({ method: 'GET', - cookies: {}, headers: expect.any(Object), url: 'http://localhost:3030/test-exception/123', }); diff --git a/dev-packages/e2e-tests/test-applications/node-koa/tests/transactions.test.ts b/dev-packages/e2e-tests/test-applications/node-koa/tests/transactions.test.ts index 4c52c932e7b4..8dcad3e01dff 100644 --- a/dev-packages/e2e-tests/test-applications/node-koa/tests/transactions.test.ts +++ b/dev-packages/e2e-tests/test-applications/node-koa/tests/transactions.test.ts @@ -46,76 +46,128 @@ test('Sends an API route transaction', async ({ baseURL }) => { origin: 'auto.http.otel.http', }); - expect(transactionEvent).toEqual( - expect.objectContaining({ - spans: [ - { - data: { - 'koa.name': '', - 'koa.type': 'middleware', - 'sentry.origin': 'auto.http.otel.koa', - 'sentry.op': 'middleware.koa', - }, - op: 'middleware.koa', - origin: 'auto.http.otel.koa', - description: '< unknown >', - parent_span_id: expect.any(String), - span_id: expect.any(String), - start_timestamp: expect.any(Number), - status: 'ok', - timestamp: expect.any(Number), - trace_id: expect.any(String), - }, - { - data: { - 'http.route': '/test-transaction', - 'koa.name': '/test-transaction', - 'koa.type': 'router', - 'sentry.origin': 'auto.http.otel.koa', - 'sentry.op': 'router.koa', - }, - op: 'router.koa', - description: '/test-transaction', - parent_span_id: expect.any(String), - span_id: expect.any(String), - start_timestamp: expect.any(Number), - status: 'ok', - timestamp: expect.any(Number), - trace_id: expect.any(String), - origin: 'auto.http.otel.koa', - }, - { - data: { - 'sentry.origin': 'manual', - }, - description: 'test-span', - parent_span_id: expect.any(String), - span_id: expect.any(String), - start_timestamp: expect.any(Number), - status: 'ok', - timestamp: expect.any(Number), - trace_id: expect.any(String), - origin: 'manual', - }, - { - data: { - 'sentry.origin': 'manual', - }, - description: 'child-span', - parent_span_id: expect.any(String), - span_id: expect.any(String), - start_timestamp: expect.any(Number), - status: 'ok', - timestamp: expect.any(Number), - trace_id: expect.any(String), - origin: 'manual', - }, - ], - transaction: 'GET /test-transaction', - type: 'transaction', - transaction_info: { - source: 'route', + expect(transactionEvent).toMatchObject({ + transaction: 'GET /test-transaction', + type: 'transaction', + transaction_info: { + source: 'route', + }, + }); + + expect(transactionEvent.spans).toEqual([ + { + data: { + 'koa.name': 'bodyParser', + 'koa.type': 'middleware', + 'sentry.op': 'middleware.koa', + 'sentry.origin': 'auto.http.otel.koa', + }, + description: 'bodyParser', + op: 'middleware.koa', + origin: 'auto.http.otel.koa', + parent_span_id: expect.any(String), + span_id: expect.any(String), + start_timestamp: expect.any(Number), + status: 'ok', + timestamp: expect.any(Number), + trace_id: expect.any(String), + }, + { + data: { + 'koa.name': '', + 'koa.type': 'middleware', + 'sentry.origin': 'auto.http.otel.koa', + 'sentry.op': 'middleware.koa', + }, + op: 'middleware.koa', + origin: 'auto.http.otel.koa', + description: '< unknown >', + parent_span_id: expect.any(String), + span_id: expect.any(String), + start_timestamp: expect.any(Number), + status: 'ok', + timestamp: expect.any(Number), + trace_id: expect.any(String), + }, + { + data: { + 'http.route': '/test-transaction', + 'koa.name': '/test-transaction', + 'koa.type': 'router', + 'sentry.origin': 'auto.http.otel.koa', + 'sentry.op': 'router.koa', }, + op: 'router.koa', + description: '/test-transaction', + parent_span_id: expect.any(String), + span_id: expect.any(String), + start_timestamp: expect.any(Number), + status: 'ok', + timestamp: expect.any(Number), + trace_id: expect.any(String), + origin: 'auto.http.otel.koa', + }, + { + data: { + 'sentry.origin': 'manual', + }, + description: 'test-span', + parent_span_id: expect.any(String), + span_id: expect.any(String), + start_timestamp: expect.any(Number), + status: 'ok', + timestamp: expect.any(Number), + trace_id: expect.any(String), + origin: 'manual', + }, + { + data: { + 'sentry.origin': 'manual', + }, + description: 'child-span', + parent_span_id: expect.any(String), + span_id: expect.any(String), + start_timestamp: expect.any(Number), + status: 'ok', + timestamp: expect.any(Number), + trace_id: expect.any(String), + origin: 'manual', + }, + ]); +}); + +test('Captures request metadata', async ({ baseURL }) => { + const transactionEventPromise = waitForTransaction('node-koa', transactionEvent => { + return ( + transactionEvent?.contexts?.trace?.op === 'http.server' && transactionEvent?.transaction === 'POST /test-post' + ); + }); + + const res = await fetch(`${baseURL}/test-post`, { + method: 'POST', + body: JSON.stringify({ foo: 'bar', other: 1 }), + headers: { + 'Content-Type': 'application/json', + }, + }); + const resBody = await res.json(); + + expect(resBody).toEqual({ status: 'ok', body: { foo: 'bar', other: 1 } }); + + const transactionEvent = await transactionEventPromise; + + expect(transactionEvent.request).toEqual({ + url: expect.stringMatching(/^http:\/\/localhost:(\d+)\/test-post$/), + method: 'POST', + headers: expect.objectContaining({ + 'user-agent': expect.stringContaining(''), + 'content-type': 'application/json', }), - ); + data: JSON.stringify({ + foo: 'bar', + other: 1, + }), + }); + + expect(transactionEvent.user).toEqual(undefined); }); From 391adf1d0401159624892235d8e75205991d2324 Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Mon, 23 Sep 2024 10:14:42 +0200 Subject: [PATCH 02/16] add integration test --- .../node-integration-tests/package.json | 1 + .../suites/express/tracing/server.js | 8 + .../suites/express/tracing/test.ts | 104 +++++++++++++ .../suites/express/without-tracing/server.ts | 11 ++ .../suites/express/without-tracing/test.ts | 143 +++++++++++++++--- 5 files changed, 249 insertions(+), 18 deletions(-) diff --git a/dev-packages/node-integration-tests/package.json b/dev-packages/node-integration-tests/package.json index e016bf6ee93b..5b62e3a8e996 100644 --- a/dev-packages/node-integration-tests/package.json +++ b/dev-packages/node-integration-tests/package.json @@ -41,6 +41,7 @@ "amqplib": "^0.10.4", "apollo-server": "^3.11.1", "axios": "^1.7.7", + "body-parser": "^1.20.3", "connect": "^3.7.0", "cors": "^2.8.5", "cron": "^3.1.6", diff --git a/dev-packages/node-integration-tests/suites/express/tracing/server.js b/dev-packages/node-integration-tests/suites/express/tracing/server.js index 81560806097e..f9b4ae24b339 100644 --- a/dev-packages/node-integration-tests/suites/express/tracing/server.js +++ b/dev-packages/node-integration-tests/suites/express/tracing/server.js @@ -13,11 +13,15 @@ Sentry.init({ // express must be required after Sentry is initialized const express = require('express'); const cors = require('cors'); +const bodyParser = require('body-parser'); const { startExpressServerAndSendPortToRunner } = require('@sentry-internal/node-integration-tests'); const app = express(); app.use(cors()); +app.use(bodyParser.json()); +app.use(bodyParser.text()); +app.use(bodyParser.raw()); app.get('/test/express', (_req, res) => { res.send({ response: 'response 1' }); @@ -35,6 +39,10 @@ app.get(['/test/arr/:id', /\/test\/arr[0-9]*\/required(path)?(\/optionalPath)?\/ res.send({ response: 'response 4' }); }); +app.post('/test-post', function (req, res) { + res.send({ status: 'ok', body: req.body }); +}); + Sentry.setupExpressErrorHandler(app); startExpressServerAndSendPortToRunner(app); diff --git a/dev-packages/node-integration-tests/suites/express/tracing/test.ts b/dev-packages/node-integration-tests/suites/express/tracing/test.ts index 44852233ed67..56826177ab48 100644 --- a/dev-packages/node-integration-tests/suites/express/tracing/test.ts +++ b/dev-packages/node-integration-tests/suites/express/tracing/test.ts @@ -137,5 +137,109 @@ describe('express tracing', () => { .start(done) .makeRequest('get', `/test/${segment}`); }) as any); + + describe('request data', () => { + test('correctly captures JSON request data', done => { + const runner = createRunner(__dirname, 'server.js') + .expect({ + transaction: { + transaction: 'POST /test-post', + request: { + url: expect.stringMatching(/^http:\/\/localhost:(\d+)\/test-post$/), + method: 'POST', + headers: { + 'user-agent': expect.stringContaining(''), + 'content-type': 'application/json', + }, + data: JSON.stringify({ + foo: 'bar', + other: 1, + }), + }, + }, + }) + .start(done); + + runner.makeRequest('post', '/test-post', {}, { foo: 'bar', other: 1 }); + }); + + test('correctly captures plain text request data', done => { + const runner = createRunner(__dirname, 'server.js') + .expect({ + transaction: { + transaction: 'POST /test-post', + request: { + url: expect.stringMatching(/^http:\/\/localhost:(\d+)\/test-post$/), + method: 'POST', + headers: { + 'user-agent': expect.stringContaining(''), + 'content-type': 'text/plain', + }, + data: 'some plain text', + }, + }, + }) + .start(done); + + runner.makeRequest( + 'post', + '/test-post', + { + 'Content-Type': 'text/plain', + }, + 'some plain text', + ); + }); + + test('correctly captures text buffer request data', done => { + const runner = createRunner(__dirname, 'server.js') + .expect({ + transaction: { + transaction: 'POST /test-post', + request: { + url: expect.stringMatching(/^http:\/\/localhost:(\d+)\/test-post$/), + method: 'POST', + headers: { + 'user-agent': expect.stringContaining(''), + 'content-type': 'application/octet-stream', + }, + data: 'some plain text in buffer', + }, + }, + }) + .start(done); + + runner.makeRequest( + 'post', + '/test-post', + { 'Content-Type': 'application/octet-stream' }, + Buffer.from('some plain text in buffer'), + ); + }); + + test('correctly captures non-text buffer request data', done => { + const runner = createRunner(__dirname, 'server.js') + .expect({ + transaction: { + transaction: 'POST /test-post', + request: { + url: expect.stringMatching(/^http:\/\/localhost:(\d+)\/test-post$/), + method: 'POST', + headers: { + 'user-agent': expect.stringContaining(''), + 'content-type': 'application/octet-stream', + }, + // This is some non-ascii string representation + data: expect.any(String), + }, + }, + }) + .start(done); + + const body = new Uint8Array([1, 2, 3, 4, 5]).buffer; + + runner.makeRequest('post', '/test-post', { 'Content-Type': 'application/octet-stream' }, body); + }); + }); }); }); diff --git a/dev-packages/node-integration-tests/suites/express/without-tracing/server.ts b/dev-packages/node-integration-tests/suites/express/without-tracing/server.ts index 2a85d39b83b8..5b96e8b1a2a3 100644 --- a/dev-packages/node-integration-tests/suites/express/without-tracing/server.ts +++ b/dev-packages/node-integration-tests/suites/express/without-tracing/server.ts @@ -8,10 +8,15 @@ Sentry.init({ }); import { startExpressServerAndSendPortToRunner } from '@sentry-internal/node-integration-tests'; +import bodyParser from 'body-parser'; import express from 'express'; const app = express(); +app.use(bodyParser.json()); +app.use(bodyParser.text()); +app.use(bodyParser.raw()); + Sentry.setTag('global', 'tag'); app.get('/test/isolationScope/:id', (req, res) => { @@ -24,6 +29,12 @@ app.get('/test/isolationScope/:id', (req, res) => { res.send({}); }); +app.post('/test-post', function (req, res) { + Sentry.captureException(new Error('This is an exception')); + + res.send({ status: 'ok', body: req.body }); +}); + Sentry.setupExpressErrorHandler(app); startExpressServerAndSendPortToRunner(app); diff --git a/dev-packages/node-integration-tests/suites/express/without-tracing/test.ts b/dev-packages/node-integration-tests/suites/express/without-tracing/test.ts index 7c304062bc22..c10d8706c42f 100644 --- a/dev-packages/node-integration-tests/suites/express/without-tracing/test.ts +++ b/dev-packages/node-integration-tests/suites/express/without-tracing/test.ts @@ -4,26 +4,133 @@ afterAll(() => { cleanupChildProcesses(); }); -test('correctly applies isolation scope even without tracing', done => { - const runner = createRunner(__dirname, 'server.ts') - .expect({ - event: { - transaction: 'GET /test/isolationScope/1', - tags: { - global: 'tag', - 'isolation-scope': 'tag', - 'isolation-scope-1': '1', +describe('express without tracing', () => { + test('correctly applies isolation scope even without tracing', done => { + const runner = createRunner(__dirname, 'server.ts') + .expect({ + event: { + transaction: 'GET /test/isolationScope/1', + tags: { + global: 'tag', + 'isolation-scope': 'tag', + 'isolation-scope-1': '1', + }, + // Request is correctly set + request: { + url: expect.stringMatching(/^http:\/\/localhost:(\d+)\/test\/isolationScope\/1$/), + method: 'GET', + headers: { + 'user-agent': expect.stringContaining(''), + }, + }, }, - // Request is correctly set - request: { - url: expect.stringContaining('/test/isolationScope/1'), - headers: { - 'user-agent': expect.stringContaining(''), + }) + .start(done); + + runner.makeRequest('get', '/test/isolationScope/1'); + }); + + describe('request data', () => { + test('correctly captures JSON request data', done => { + const runner = createRunner(__dirname, 'server.ts') + .expect({ + event: { + transaction: 'POST /test-post', + request: { + url: expect.stringMatching(/^http:\/\/localhost:(\d+)\/test-post$/), + method: 'POST', + headers: { + 'user-agent': expect.stringContaining(''), + 'content-type': 'application/json', + }, + data: JSON.stringify({ + foo: 'bar', + other: 1, + }), + }, + }, + }) + .start(done); + + runner.makeRequest('post', '/test-post', {}, { foo: 'bar', other: 1 }); + }); + + test('correctly captures plain text request data', done => { + const runner = createRunner(__dirname, 'server.ts') + .expect({ + event: { + transaction: 'POST /test-post', + request: { + url: expect.stringMatching(/^http:\/\/localhost:(\d+)\/test-post$/), + method: 'POST', + headers: { + 'user-agent': expect.stringContaining(''), + 'content-type': 'text/plain', + }, + data: 'some plain text', + }, }, + }) + .start(done); + + runner.makeRequest( + 'post', + '/test-post', + { + 'Content-Type': 'text/plain', }, - }, - }) - .start(done); + 'some plain text', + ); + }); + + test('correctly captures text buffer request data', done => { + const runner = createRunner(__dirname, 'server.ts') + .expect({ + event: { + transaction: 'POST /test-post', + request: { + url: expect.stringMatching(/^http:\/\/localhost:(\d+)\/test-post$/), + method: 'POST', + headers: { + 'user-agent': expect.stringContaining(''), + 'content-type': 'application/octet-stream', + }, + data: 'some plain text in buffer', + }, + }, + }) + .start(done); + + runner.makeRequest( + 'post', + '/test-post', + { 'Content-Type': 'application/octet-stream' }, + Buffer.from('some plain text in buffer'), + ); + }); + + test('correctly captures non-text buffer request data', done => { + const runner = createRunner(__dirname, 'server.ts') + .expect({ + event: { + transaction: 'POST /test-post', + request: { + url: expect.stringMatching(/^http:\/\/localhost:(\d+)\/test-post$/), + method: 'POST', + headers: { + 'user-agent': expect.stringContaining(''), + 'content-type': 'application/octet-stream', + }, + // This is some non-ascii string representation + data: expect.any(String), + }, + }, + }) + .start(done); + + const body = new Uint8Array([1, 2, 3, 4, 5]).buffer; - runner.makeRequest('get', '/test/isolationScope/1'); + runner.makeRequest('post', '/test-post', { 'Content-Type': 'application/octet-stream' }, body); + }); + }); }); From 0b27b94cb848d10c10284d3424b00092e5a43c13 Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Mon, 23 Sep 2024 10:19:40 +0200 Subject: [PATCH 03/16] add normalized request data in http --- packages/core/src/integrations/requestdata.ts | 20 ++- packages/node/src/transports/http-module.ts | 3 +- packages/types/src/envelope.ts | 2 +- packages/types/src/event.ts | 14 +- packages/utils/src/requestdata.ts | 124 +++++++++++++++++- yarn.lock | 25 ++++ 6 files changed, 177 insertions(+), 11 deletions(-) diff --git a/packages/core/src/integrations/requestdata.ts b/packages/core/src/integrations/requestdata.ts index f7846dec6fea..f35b1bc4db2f 100644 --- a/packages/core/src/integrations/requestdata.ts +++ b/packages/core/src/integrations/requestdata.ts @@ -1,5 +1,6 @@ import type { IntegrationFn } from '@sentry/types'; import type { AddRequestDataToEventOptions, TransactionNamingScheme } from '@sentry/utils'; +import { addNormalizedRequestDataToEvent } from '@sentry/utils'; import { addRequestDataToEvent } from '@sentry/utils'; import { defineIntegration } from '../integration'; @@ -73,15 +74,24 @@ const _requestDataIntegration = ((options: RequestDataIntegrationOptions = {}) = // that's happened, it will be easier to add this logic in without worrying about unexpected side effects.) const { sdkProcessingMetadata = {} } = event; - const req = sdkProcessingMetadata.request; + const { request, normalizedRequest } = sdkProcessingMetadata; - if (!req) { - return event; + const addRequestDataOptions = convertReqDataIntegrationOptsToAddReqDataOpts(_options); + + // If this is set, it takes precedence over the plain request object + if (normalizedRequest) { + // Some other data is not available in standard HTTP requests, but can sometimes be augmented by e.g. Express or Next.js + const ipAddress = request ? request.ip || (request.socket && request.socket.remoteAddress) : undefined; + const user = request ? request.user : undefined; + + return addNormalizedRequestDataToEvent(event, normalizedRequest, { ipAddress, user }, addRequestDataOptions); } - const addRequestDataOptions = convertReqDataIntegrationOptsToAddReqDataOpts(_options); + if (!request) { + return event; + } - return addRequestDataToEvent(event, req, addRequestDataOptions); + return addRequestDataToEvent(event, request, addRequestDataOptions); }, }; }) satisfies IntegrationFn; diff --git a/packages/node/src/transports/http-module.ts b/packages/node/src/transports/http-module.ts index f5cbe6fd35f9..65bf99349b10 100644 --- a/packages/node/src/transports/http-module.ts +++ b/packages/node/src/transports/http-module.ts @@ -10,7 +10,8 @@ export type HTTPModuleRequestOptions = HTTPRequestOptions | HTTPSRequestOptions export interface HTTPModuleRequestIncomingMessage { headers: IncomingHttpHeaders; statusCode?: number; - on(event: 'data' | 'end', listener: () => void): void; + on(event: 'data' | 'end', listener: (chunk: Buffer) => void): void; + off(event: 'data' | 'end', listener: (chunk: Buffer) => void): void; setEncoding(encoding: string): void; } diff --git a/packages/types/src/envelope.ts b/packages/types/src/envelope.ts index 29e8fc123b16..20c67b8857fe 100644 --- a/packages/types/src/envelope.ts +++ b/packages/types/src/envelope.ts @@ -101,7 +101,7 @@ export type ProfileItem = BaseEnvelopeItem; export type ProfileChunkItem = BaseEnvelopeItem; export type SpanItem = BaseEnvelopeItem>; -export type EventEnvelopeHeaders = { event_id: string; sent_at: string; trace?: DynamicSamplingContext }; +export type EventEnvelopeHeaders = { event_id: string; sent_at: string; trace?: Partial }; type SessionEnvelopeHeaders = { sent_at: string }; type CheckInEnvelopeHeaders = { trace?: DynamicSamplingContext }; type ClientReportEnvelopeHeaders = BaseEnvelopeHeaders; diff --git a/packages/types/src/event.ts b/packages/types/src/event.ts index c8c16b8ce514..bdccaa2b58dd 100644 --- a/packages/types/src/event.ts +++ b/packages/types/src/event.ts @@ -2,13 +2,15 @@ import type { Attachment } from './attachment'; import type { Breadcrumb } from './breadcrumb'; import type { Contexts } from './context'; import type { DebugMeta } from './debugMeta'; +import type { DynamicSamplingContext } from './envelope'; import type { Exception } from './exception'; import type { Extras } from './extra'; import type { Measurements } from './measurement'; import type { Mechanism } from './mechanism'; import type { Primitive } from './misc'; +import type { PolymorphicRequest } from './polymorphics'; import type { Request } from './request'; -import type { CaptureContext } from './scope'; +import type { CaptureContext, Scope } from './scope'; import type { SdkInfo } from './sdkinfo'; import type { SeverityLevel } from './severity'; import type { MetricSummary, SpanJSON } from './span'; @@ -51,7 +53,15 @@ export interface Event { measurements?: Measurements; debug_meta?: DebugMeta; // A place to stash data which is needed at some point in the SDK's event processing pipeline but which shouldn't get sent to Sentry - sdkProcessingMetadata?: { [key: string]: any }; + // Note: This is considered internal and is subject to change in minors + sdkProcessingMetadata?: { [key: string]: unknown } & { + request?: PolymorphicRequest; + normalizedRequest?: Request; + dynamicSamplingContext?: Partial; + capturedSpanScope?: Scope; + capturedSpanIsolationScope?: Scope; + spanCountBeforeProcessing?: number; + }; transaction_info?: { source: TransactionSource; }; diff --git a/packages/utils/src/requestdata.ts b/packages/utils/src/requestdata.ts index a4eae547edb1..5b5d71a93723 100644 --- a/packages/utils/src/requestdata.ts +++ b/packages/utils/src/requestdata.ts @@ -1,7 +1,9 @@ +/* eslint-disable max-lines */ import type { Event, ExtractedNodeRequestData, PolymorphicRequest, + Request, TransactionSource, WebFetchHeaders, WebFetchRequest, @@ -12,6 +14,7 @@ import { DEBUG_BUILD } from './debug-build'; import { isPlainObject, isString } from './is'; import { logger } from './logger'; import { normalize } from './normalize'; +import { truncate } from './string'; import { stripUrlQueryAndFragment } from './url'; import { getClientIPAddress, ipHeaderNames } from './vendor/getIpAddress'; @@ -228,14 +231,27 @@ export function extractRequestData( if (method === 'GET' || method === 'HEAD') { break; } + // NOTE: As of v8, request is (unless a user sets this manually) ALWAYS a http request + // Which does not have a body by default + // However, in our http instrumentation, we patch the request to capture the body and store it on the + // request as `.body` anyhow + // In v9, we may update requestData to only work with plain http requests // 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)); + const body = req.body; + if (body !== undefined) { + const stringBody: string = isString(body) + ? body + : isPlainObject(body) + ? JSON.stringify(normalize(body)) + : truncate(`${body}`, 1024); + if (stringBody) { + requestData.data = stringBody; + } } break; } @@ -250,6 +266,62 @@ export function extractRequestData( return requestData; } +/** + * Add already normalized request data to an event. + */ +export function addNormalizedRequestDataToEvent( + event: Event, + req: Request, + // This is non-standard data that is not part of the regular HTTP request + additionalData: { ipAddress?: string; user?: Record }, + options: AddRequestDataToEventOptions, +): Event { + const include = { + ...DEFAULT_INCLUDES, + ...(options && options.include), + }; + + if (include.request) { + const includeRequest = Array.isArray(include.request) ? [...include.request] : [...DEFAULT_REQUEST_INCLUDES]; + if (include.ip) { + includeRequest.push('ip'); + } + + const extractedRequestData = extractNormalizedRequestData(req, { include: includeRequest }); + + event.request = { + ...event.request, + ...extractedRequestData, + }; + } + + if (include.user) { + const extractedUser = + additionalData.user && isPlainObject(additionalData.user) + ? extractUserData(additionalData.user, include.user) + : {}; + + if (Object.keys(extractedUser).length) { + event.user = { + ...event.user, + ...extractedUser, + }; + } + } + + if (include.ip) { + const ip = (req.headers && getClientIPAddress(req.headers)) || additionalData.ipAddress; + if (ip) { + event.user = { + ...event.user, + ip_address: ip, + }; + } + } + + return event; +} + /** * Add data from the given request to the given event * @@ -374,3 +446,51 @@ export function winterCGRequestToRequestData(req: WebFetchRequest): PolymorphicR headers, }; } + +function extractNormalizedRequestData(normalizedRequest: Request, { include }: { include: string[] }): Request { + const includeKeys = include ? (Array.isArray(include) ? include : DEFAULT_REQUEST_INCLUDES) : []; + + const requestData: Request = {}; + + const { headers } = normalizedRequest; + + if (includeKeys.includes('headers')) { + requestData.headers = headers; + + // Remove the Cookie header in case cookie data should not be included in the event + if (!include.includes('cookies')) { + delete (headers as { cookie?: string }).cookie; + } + + // Remove IP headers in case IP data should not be included in the event + if (!include.includes('ip')) { + ipHeaderNames.forEach(ipHeaderName => { + // eslint-disable-next-line @typescript-eslint/no-dynamic-delete + delete (headers as Record)[ipHeaderName]; + }); + } + } + + if (includeKeys.includes('method')) { + requestData.method = normalizedRequest.method; + } + + if (includeKeys.includes('url')) { + requestData.url = normalizedRequest.url; + } + + if (includeKeys.includes('cookies')) { + const cookies = normalizedRequest.cookies || (headers && headers.cookie ? parseCookie(headers.cookie) : undefined); + requestData.cookies = cookies; + } + + if (includeKeys.includes('query_string')) { + requestData.query_string = normalizedRequest.query_string; + } + + if (includeKeys.includes('data')) { + requestData.data = normalizedRequest.data; + } + + return requestData; +} diff --git a/yarn.lock b/yarn.lock index 6b1ae8520693..01efab263fca 100644 --- a/yarn.lock +++ b/yarn.lock @@ -12973,6 +12973,24 @@ body-parser@1.20.3, body-parser@^1.18.3, body-parser@^1.19.0: type-is "~1.6.18" unpipe "1.0.0" +body-parser@^1.20.3: + version "1.20.3" + resolved "https://registry.yarnpkg.com/body-parser/-/body-parser-1.20.3.tgz#1953431221c6fb5cd63c4b36d53fab0928e548c6" + integrity sha512-7rAxByjUMqQ3/bHJy7D6OGXvx/MMc4IqBn/X0fcM1QUcAItpZrBEYhWGem+tzXH90c+G01ypMcYJBO9Y30203g== + dependencies: + bytes "3.1.2" + content-type "~1.0.5" + debug "2.6.9" + depd "2.0.0" + destroy "1.2.0" + http-errors "2.0.0" + iconv-lite "0.4.24" + on-finished "2.4.1" + qs "6.13.0" + raw-body "2.5.2" + type-is "~1.6.18" + unpipe "1.0.0" + body@^5.1.0: version "5.1.0" resolved "https://registry.yarnpkg.com/body/-/body-5.1.0.tgz#e4ba0ce410a46936323367609ecb4e6553125069" @@ -28336,6 +28354,13 @@ qs@^6.4.0: dependencies: side-channel "^1.0.4" +qs@6.13.0: + version "6.13.0" + resolved "https://registry.yarnpkg.com/qs/-/qs-6.13.0.tgz#6ca3bd58439f7e245655798997787b0d88a51906" + integrity sha512-+38qI9SOr8tfZ4QmJNplMUxqjbe7LKvvZgWdExBOmd+egZTtjLB67Gu0HRX3u/XOq7UU2Nx6nsjvS16Z9uwfpg== + dependencies: + side-channel "^1.0.6" + query-string@^4.2.2: version "4.3.4" resolved "https://registry.yarnpkg.com/query-string/-/query-string-4.3.4.tgz#bbb693b9ca915c232515b228b1a02b609043dbeb" From 484bef89115e9eb93997a9249343dd9ab55a8c55 Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Tue, 8 Oct 2024 13:45:25 +0200 Subject: [PATCH 04/16] PR feedback --- packages/core/src/integrations/requestdata.ts | 3 ++- packages/utils/src/requestdata.ts | 5 ++--- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/packages/core/src/integrations/requestdata.ts b/packages/core/src/integrations/requestdata.ts index f35b1bc4db2f..80b01ea4861f 100644 --- a/packages/core/src/integrations/requestdata.ts +++ b/packages/core/src/integrations/requestdata.ts @@ -84,7 +84,8 @@ const _requestDataIntegration = ((options: RequestDataIntegrationOptions = {}) = const ipAddress = request ? request.ip || (request.socket && request.socket.remoteAddress) : undefined; const user = request ? request.user : undefined; - return addNormalizedRequestDataToEvent(event, normalizedRequest, { ipAddress, user }, addRequestDataOptions); + addNormalizedRequestDataToEvent(event, normalizedRequest, { ipAddress, user }, addRequestDataOptions); + return event; } if (!request) { diff --git a/packages/utils/src/requestdata.ts b/packages/utils/src/requestdata.ts index 5b5d71a93723..0e09ecb8cb31 100644 --- a/packages/utils/src/requestdata.ts +++ b/packages/utils/src/requestdata.ts @@ -268,6 +268,7 @@ export function extractRequestData( /** * Add already normalized request data to an event. + * This mutates the passed in event. */ export function addNormalizedRequestDataToEvent( event: Event, @@ -275,7 +276,7 @@ export function addNormalizedRequestDataToEvent( // This is non-standard data that is not part of the regular HTTP request additionalData: { ipAddress?: string; user?: Record }, options: AddRequestDataToEventOptions, -): Event { +): void { const include = { ...DEFAULT_INCLUDES, ...(options && options.include), @@ -318,8 +319,6 @@ export function addNormalizedRequestDataToEvent( }; } } - - return event; } /** From 660ace385c2c3934039b6d0b33b24f5174709b6a Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Mon, 28 Oct 2024 10:14:36 +0100 Subject: [PATCH 05/16] fix after rebase --- .../http/SentryHttpInstrumentation.ts | 153 +++++++++++++++++- packages/types/src/request.ts | 5 +- 2 files changed, 155 insertions(+), 3 deletions(-) diff --git a/packages/node/src/integrations/http/SentryHttpInstrumentation.ts b/packages/node/src/integrations/http/SentryHttpInstrumentation.ts index 090c0783507a..23594a138d8a 100644 --- a/packages/node/src/integrations/http/SentryHttpInstrumentation.ts +++ b/packages/node/src/integrations/http/SentryHttpInstrumentation.ts @@ -1,18 +1,20 @@ import type * as http from 'node:http'; -import type { RequestOptions } from 'node:http'; +import type { IncomingMessage, RequestOptions } from 'node:http'; import type * as https from 'node:https'; import { VERSION } from '@opentelemetry/core'; import type { InstrumentationConfig } from '@opentelemetry/instrumentation'; import { InstrumentationBase, InstrumentationNodeModuleDefinition } from '@opentelemetry/instrumentation'; import { getRequestInfo } from '@opentelemetry/instrumentation-http'; import { addBreadcrumb, getClient, getIsolationScope, withIsolationScope } from '@sentry/core'; -import type { SanitizedRequestData } from '@sentry/types'; +import type { PolymorphicRequest, Request, SanitizedRequestData } from '@sentry/types'; import { getBreadcrumbLogLevelFromHttpStatusCode, getSanitizedUrlString, + logger, parseUrl, stripUrlQueryAndFragment, } from '@sentry/utils'; +import { DEBUG_BUILD } from '../../debug-build'; import type { NodeClient } from '../../sdk/client'; import { getRequestUrl } from '../../utils/getRequestUrl'; @@ -128,6 +130,28 @@ export class SentryHttpInstrumentation extends InstrumentationBase'; + const protocol = request.socket && (request.socket as { encrypted?: boolean }).encrypted ? 'https' : 'http'; + const originalUrl = request.url || ''; + const absoluteUrl = originalUrl.startsWith(protocol) ? originalUrl : `${protocol}://${host}${originalUrl}`; + + // This is non-standard, but may be set on e.g. Next.js or Express requests + const cookies = (request as PolymorphicRequest).cookies; + + const normalizedRequest: Request = { + url: absoluteUrl, + method: request.method, + query_string: extractQueryParams(request), + headers: headersToDict(request.headers), + cookies, + }; + + patchRequestToCaptureBody(request, normalizedRequest); + + // Update the isolation scope, isolate this request + isolationScope.setSDKProcessingMetadata({ request, normalizedRequest }); + // Update the isolation scope, isolate this request isolationScope.setSDKProcessingMetadata({ request }); @@ -316,3 +340,128 @@ function getBreadcrumbData(request: http.ClientRequest): Partial) => { + const [event, listener, ...restArgs] = args; + + if (event === 'data') { + const callback = new Proxy(listener, { + apply: (target, thisArg, args: Parameters) => { + const chunk = args[0]; + chunks.push(chunk); + return Reflect.apply(target, thisArg, args); + }, + }); + + callbackMap.set(listener, callback); + + return Reflect.apply(target, thisArg, [event, callback, ...restArgs]); + } + + if (event === 'end') { + const callback = new Proxy(listener, { + apply: (target, thisArg, args) => { + try { + const body = Buffer.concat(chunks).toString('utf-8'); + + // We mutate the passed in normalizedRequest and add the body to it + if (body) { + normalizedRequest.data = body; + } + } catch { + // ignore errors here + } + + return Reflect.apply(target, thisArg, args); + }, + }); + + callbackMap.set(listener, callback); + + return Reflect.apply(target, thisArg, [event, callback, ...restArgs]); + } + + return Reflect.apply(target, thisArg, args); + }, + }); + + // Ensure we also remove callbacks correctly + // eslint-disable-next-line @typescript-eslint/unbound-method + req.off = new Proxy(req.off, { + apply: (target, thisArg, args: Parameters) => { + const [, listener] = args; + + const callback = callbackMap.get(listener); + if (callback) { + callbackMap.delete(listener); + + const modifiedArgs = args.slice(); + modifiedArgs[1] = callback; + return Reflect.apply(target, thisArg, modifiedArgs); + } + + return Reflect.apply(target, thisArg, args); + }, + }); + } catch { + // ignore errors if we can't patch stuff + } +} + +function extractQueryParams(req: IncomingMessage): string | undefined { + // url (including path and query string): + let 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}`; + } + + try { + const queryParams = new URL(originalUrl).search.slice(1); + return queryParams.length ? queryParams : undefined; + } catch { + return undefined; + } +} + +function headersToDict(reqHeaders: Record): Record { + const headers: Record = {}; + + try { + Object.entries(reqHeaders).forEach(([key, value]) => { + if (typeof value === 'string') { + headers[key] = value; + } + }); + } catch (e) { + DEBUG_BUILD && + logger.warn('Sentry failed extracting headers from a request object. If you see this, please file an issue.'); + } + + return headers; +} diff --git a/packages/types/src/request.ts b/packages/types/src/request.ts index 3c04a788ded9..f0c2bdb268ca 100644 --- a/packages/types/src/request.ts +++ b/packages/types/src/request.ts @@ -1,4 +1,7 @@ -/** Request data included in an event as sent to Sentry */ +/** + * Request data included in an event as sent to Sentry. + * TODO(v9): Rename this to avoid confusion, because Request is also a native type. + */ export interface Request { url?: string; method?: string; From d106d699e76a0bcf127331e4efc081a7203a5f03 Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Mon, 28 Oct 2024 10:19:43 +0100 Subject: [PATCH 06/16] add comment --- packages/core/src/integrations/requestdata.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/packages/core/src/integrations/requestdata.ts b/packages/core/src/integrations/requestdata.ts index 80b01ea4861f..9475973d8e2c 100644 --- a/packages/core/src/integrations/requestdata.ts +++ b/packages/core/src/integrations/requestdata.ts @@ -88,6 +88,7 @@ const _requestDataIntegration = ((options: RequestDataIntegrationOptions = {}) = return event; } + // TODO(v9): Eventually we can remove this fallback branch and only rely on the normalizedRequest above if (!request) { return event; } From b7b25e5c81331a19157a3dfefe80ea846820c4fc Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Mon, 28 Oct 2024 13:05:03 +0100 Subject: [PATCH 07/16] add cookies default value --- packages/utils/src/requestdata.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/utils/src/requestdata.ts b/packages/utils/src/requestdata.ts index 0e09ecb8cb31..92f4294b7681 100644 --- a/packages/utils/src/requestdata.ts +++ b/packages/utils/src/requestdata.ts @@ -480,7 +480,7 @@ function extractNormalizedRequestData(normalizedRequest: Request, { include }: { if (includeKeys.includes('cookies')) { const cookies = normalizedRequest.cookies || (headers && headers.cookie ? parseCookie(headers.cookie) : undefined); - requestData.cookies = cookies; + requestData.cookies = cookies || {}; } if (includeKeys.includes('query_string')) { From 3029a328d0f228f3749557f2014dbaad1892a50e Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Mon, 28 Oct 2024 13:27:03 +0100 Subject: [PATCH 08/16] oops fix --- .../e2e-tests/test-applications/node-koa/tests/assert.test.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/dev-packages/e2e-tests/test-applications/node-koa/tests/assert.test.ts b/dev-packages/e2e-tests/test-applications/node-koa/tests/assert.test.ts index 5bf468fc3772..0f9f724ef237 100644 --- a/dev-packages/e2e-tests/test-applications/node-koa/tests/assert.test.ts +++ b/dev-packages/e2e-tests/test-applications/node-koa/tests/assert.test.ts @@ -16,6 +16,7 @@ test('Returns 400 from failed assert', async ({ baseURL }) => { expect(errorEvent.request).toEqual({ method: 'GET', + cookies: {}, headers: expect.any(Object), url: 'http://localhost:3030/test-assert/false', }); From cd08eef57210fd1960d3e82a996ad69ca9af87c1 Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Mon, 28 Oct 2024 14:07:33 +0100 Subject: [PATCH 09/16] oops one more time --- .../e2e-tests/test-applications/node-koa/tests/errors.test.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/dev-packages/e2e-tests/test-applications/node-koa/tests/errors.test.ts b/dev-packages/e2e-tests/test-applications/node-koa/tests/errors.test.ts index 03216b5cc01e..1fbb1fd42613 100644 --- a/dev-packages/e2e-tests/test-applications/node-koa/tests/errors.test.ts +++ b/dev-packages/e2e-tests/test-applications/node-koa/tests/errors.test.ts @@ -15,6 +15,7 @@ test('Sends correct error event', async ({ baseURL }) => { expect(errorEvent.request).toEqual({ method: 'GET', + cookies: {}, headers: expect.any(Object), url: 'http://localhost:3030/test-exception/123', }); From a21f7317f2da201efecec59f9237baeebe4a43d5 Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Mon, 28 Oct 2024 14:23:28 +0100 Subject: [PATCH 10/16] is this real life?? --- .../test-applications/node-koa/tests/transactions.test.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/dev-packages/e2e-tests/test-applications/node-koa/tests/transactions.test.ts b/dev-packages/e2e-tests/test-applications/node-koa/tests/transactions.test.ts index 8dcad3e01dff..1197575d1a96 100644 --- a/dev-packages/e2e-tests/test-applications/node-koa/tests/transactions.test.ts +++ b/dev-packages/e2e-tests/test-applications/node-koa/tests/transactions.test.ts @@ -157,6 +157,7 @@ test('Captures request metadata', async ({ baseURL }) => { const transactionEvent = await transactionEventPromise; expect(transactionEvent.request).toEqual({ + cookies: {}, url: expect.stringMatching(/^http:\/\/localhost:(\d+)\/test-post$/), method: 'POST', headers: expect.objectContaining({ From ed0939ca949eb10d9e5d8025b3fb34c06c875d77 Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Wed, 13 Nov 2024 09:22:35 +0100 Subject: [PATCH 11/16] Apply suggestions from code review Co-authored-by: Luca Forstner --- .../http/SentryHttpInstrumentation.ts | 16 +++++----------- 1 file changed, 5 insertions(+), 11 deletions(-) diff --git a/packages/node/src/integrations/http/SentryHttpInstrumentation.ts b/packages/node/src/integrations/http/SentryHttpInstrumentation.ts index 23594a138d8a..a26051d9a81e 100644 --- a/packages/node/src/integrations/http/SentryHttpInstrumentation.ts +++ b/packages/node/src/integrations/http/SentryHttpInstrumentation.ts @@ -428,21 +428,15 @@ function patchRequestToCaptureBody(req: IncomingMessage, normalizedRequest: Requ } function extractQueryParams(req: IncomingMessage): string | undefined { - // url (including path and query string): - let originalUrl = req.url || ''; - - if (!originalUrl) { + // req.url is path and query string + if (!req.url) { 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}`; - } - try { - const queryParams = new URL(originalUrl).search.slice(1); + // The `URL` constructor can't handle internal URLs of the form `/some/path/here`, so stick a dummy protocol and + // hostname as the base. Since the point here is just to grab the query string, it doesn't matter what we use. + const queryParams = new URL(req.url, 'http://dogs.are.great').search.slice(1); return queryParams.length ? queryParams : undefined; } catch { return undefined; From 664a1e3d011b246a4bd97e6a9155727da774fe30 Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Wed, 13 Nov 2024 09:22:54 +0100 Subject: [PATCH 12/16] PR feedback --- .../node/src/integrations/http/SentryHttpInstrumentation.ts | 3 --- packages/utils/src/requestdata.ts | 3 +-- 2 files changed, 1 insertion(+), 5 deletions(-) diff --git a/packages/node/src/integrations/http/SentryHttpInstrumentation.ts b/packages/node/src/integrations/http/SentryHttpInstrumentation.ts index a26051d9a81e..179c63ce72f9 100644 --- a/packages/node/src/integrations/http/SentryHttpInstrumentation.ts +++ b/packages/node/src/integrations/http/SentryHttpInstrumentation.ts @@ -152,9 +152,6 @@ export class SentryHttpInstrumentation extends InstrumentationBase(); if (client && client.getOptions().autoSessionTracking) { isolationScope.setRequestSession({ status: 'ok' }); diff --git a/packages/utils/src/requestdata.ts b/packages/utils/src/requestdata.ts index 92f4294b7681..edffc6f67da7 100644 --- a/packages/utils/src/requestdata.ts +++ b/packages/utils/src/requestdata.ts @@ -450,8 +450,7 @@ function extractNormalizedRequestData(normalizedRequest: Request, { include }: { const includeKeys = include ? (Array.isArray(include) ? include : DEFAULT_REQUEST_INCLUDES) : []; const requestData: Request = {}; - - const { headers } = normalizedRequest; + const headers = { ...normalizedRequest.headers }; if (includeKeys.includes('headers')) { requestData.headers = headers; From bc41b2ae321419857000d8101ceab41cb76bfed0 Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Wed, 13 Nov 2024 09:33:06 +0100 Subject: [PATCH 13/16] limit body buffer size --- .../http/SentryHttpInstrumentation.ts | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-) diff --git a/packages/node/src/integrations/http/SentryHttpInstrumentation.ts b/packages/node/src/integrations/http/SentryHttpInstrumentation.ts index 179c63ce72f9..c275fe0cee50 100644 --- a/packages/node/src/integrations/http/SentryHttpInstrumentation.ts +++ b/packages/node/src/integrations/http/SentryHttpInstrumentation.ts @@ -41,6 +41,9 @@ type SentryHttpInstrumentationOptions = InstrumentationConfig & { ignoreOutgoingRequests?: (url: string, request: RequestOptions) => boolean; }; +// We only want to capture request bodies up to 500kb. +const MAX_BODY_BYTE_LENGTH = 1024 * 500; + /** * This custom HTTP instrumentation is used to isolate incoming requests and annotate them with additional information. * It does not emit any spans. @@ -347,6 +350,10 @@ function getBreadcrumbData(request: http.ClientRequest): Partial acc + chunk.byteLength, 0); + } + /** * We need to keep track of the original callbacks, in order to be able to remove listeners again. * Since `off` depends on having the exact same function reference passed in, we need to be able to map @@ -363,8 +370,13 @@ function patchRequestToCaptureBody(req: IncomingMessage, normalizedRequest: Requ if (event === 'data') { const callback = new Proxy(listener, { apply: (target, thisArg, args: Parameters) => { - const chunk = args[0]; - chunks.push(chunk); + // If we have already read more than the max body length, we stop addiing chunks + // To avoid growing the memory indefinitely if a respons is e.g. streamed + if (getChunksSize() < MAX_BODY_BYTE_LENGTH) { + const chunk = args[0] as Buffer; + chunks.push(chunk); + } + return Reflect.apply(target, thisArg, args); }, }); From 42c723764d2420b2dc1f4def78e40f201f2f424b Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Wed, 13 Nov 2024 10:31:19 +0100 Subject: [PATCH 14/16] add warning --- .../src/integrations/http/SentryHttpInstrumentation.ts | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/packages/node/src/integrations/http/SentryHttpInstrumentation.ts b/packages/node/src/integrations/http/SentryHttpInstrumentation.ts index c275fe0cee50..4e665d15737b 100644 --- a/packages/node/src/integrations/http/SentryHttpInstrumentation.ts +++ b/packages/node/src/integrations/http/SentryHttpInstrumentation.ts @@ -41,8 +41,8 @@ type SentryHttpInstrumentationOptions = InstrumentationConfig & { ignoreOutgoingRequests?: (url: string, request: RequestOptions) => boolean; }; -// We only want to capture request bodies up to 500kb. -const MAX_BODY_BYTE_LENGTH = 1024 * 500; +// We only want to capture request bodies up to 1mb. +const MAX_BODY_BYTE_LENGTH = 1024 * 1024; /** * This custom HTTP instrumentation is used to isolate incoming requests and annotate them with additional information. @@ -375,6 +375,10 @@ function patchRequestToCaptureBody(req: IncomingMessage, normalizedRequest: Requ if (getChunksSize() < MAX_BODY_BYTE_LENGTH) { const chunk = args[0] as Buffer; chunks.push(chunk); + } else if (DEBUG_BUILD) { + logger.log( + `Dropping request body chunk because it maximum body length of ${MAX_BODY_BYTE_LENGTH}b is exceeded.`, + ); } return Reflect.apply(target, thisArg, args); From bc42394ffce2eaa5b0f586d8ef642548e698da1f Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Wed, 13 Nov 2024 10:56:11 +0100 Subject: [PATCH 15/16] fix tests --- .../suites/express/tracing/test.ts | 29 +++++++++---------- .../suites/express/without-tracing/test.ts | 24 +++++++-------- 2 files changed, 23 insertions(+), 30 deletions(-) diff --git a/dev-packages/node-integration-tests/suites/express/tracing/test.ts b/dev-packages/node-integration-tests/suites/express/tracing/test.ts index 56826177ab48..0b56d354759c 100644 --- a/dev-packages/node-integration-tests/suites/express/tracing/test.ts +++ b/dev-packages/node-integration-tests/suites/express/tracing/test.ts @@ -160,7 +160,7 @@ describe('express tracing', () => { }) .start(done); - runner.makeRequest('post', '/test-post', {}, { foo: 'bar', other: 1 }); + runner.makeRequest('post', '/test-post', { data: { foo: 'bar', other: 1 } }); }); test('correctly captures plain text request data', done => { @@ -181,14 +181,10 @@ describe('express tracing', () => { }) .start(done); - runner.makeRequest( - 'post', - '/test-post', - { - 'Content-Type': 'text/plain', - }, - 'some plain text', - ); + runner.makeRequest('post', '/test-post', { + headers: { 'Content-Type': 'text/plain' }, + data: 'some plain text', + }); }); test('correctly captures text buffer request data', done => { @@ -209,12 +205,10 @@ describe('express tracing', () => { }) .start(done); - runner.makeRequest( - 'post', - '/test-post', - { 'Content-Type': 'application/octet-stream' }, - Buffer.from('some plain text in buffer'), - ); + runner.makeRequest('post', '/test-post', { + headers: { 'Content-Type': 'application/octet-stream' }, + data: Buffer.from('some plain text in buffer'), + }); }); test('correctly captures non-text buffer request data', done => { @@ -238,7 +232,10 @@ describe('express tracing', () => { const body = new Uint8Array([1, 2, 3, 4, 5]).buffer; - runner.makeRequest('post', '/test-post', { 'Content-Type': 'application/octet-stream' }, body); + runner.makeRequest('post', '/test-post', { + headers: { 'Content-Type': 'application/octet-stream' }, + data: body, + }); }); }); }); diff --git a/dev-packages/node-integration-tests/suites/express/without-tracing/test.ts b/dev-packages/node-integration-tests/suites/express/without-tracing/test.ts index c10d8706c42f..fdd63ad4aa4b 100644 --- a/dev-packages/node-integration-tests/suites/express/without-tracing/test.ts +++ b/dev-packages/node-integration-tests/suites/express/without-tracing/test.ts @@ -52,7 +52,7 @@ describe('express without tracing', () => { }) .start(done); - runner.makeRequest('post', '/test-post', {}, { foo: 'bar', other: 1 }); + runner.makeRequest('post', '/test-post', { data: { foo: 'bar', other: 1 } }); }); test('correctly captures plain text request data', done => { @@ -73,14 +73,12 @@ describe('express without tracing', () => { }) .start(done); - runner.makeRequest( - 'post', - '/test-post', - { + runner.makeRequest('post', '/test-post', { + headers: { 'Content-Type': 'text/plain', }, - 'some plain text', - ); + data: 'some plain text', + }); }); test('correctly captures text buffer request data', done => { @@ -101,12 +99,10 @@ describe('express without tracing', () => { }) .start(done); - runner.makeRequest( - 'post', - '/test-post', - { 'Content-Type': 'application/octet-stream' }, - Buffer.from('some plain text in buffer'), - ); + runner.makeRequest('post', '/test-post', { + headers: { 'Content-Type': 'application/octet-stream' }, + data: Buffer.from('some plain text in buffer'), + }); }); test('correctly captures non-text buffer request data', done => { @@ -130,7 +126,7 @@ describe('express without tracing', () => { const body = new Uint8Array([1, 2, 3, 4, 5]).buffer; - runner.makeRequest('post', '/test-post', { 'Content-Type': 'application/octet-stream' }, body); + runner.makeRequest('post', '/test-post', { headers: { 'Content-Type': 'application/octet-stream' }, data: body }); }); }); }); From db8c688bc6f9d2068bbafd865be391e2aa8a60ef Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Wed, 13 Nov 2024 11:42:06 +0100 Subject: [PATCH 16/16] Update packages/node/src/integrations/http/SentryHttpInstrumentation.ts Co-authored-by: Luca Forstner --- .../node/src/integrations/http/SentryHttpInstrumentation.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/node/src/integrations/http/SentryHttpInstrumentation.ts b/packages/node/src/integrations/http/SentryHttpInstrumentation.ts index 4e665d15737b..6b6fe8aaad40 100644 --- a/packages/node/src/integrations/http/SentryHttpInstrumentation.ts +++ b/packages/node/src/integrations/http/SentryHttpInstrumentation.ts @@ -457,7 +457,7 @@ function extractQueryParams(req: IncomingMessage): string | undefined { } function headersToDict(reqHeaders: Record): Record { - const headers: Record = {}; + const headers: Record = Object.create(null); try { Object.entries(reqHeaders).forEach(([key, value]) => {