From 521824b46e1009c656b0afe106770c0799c588a1 Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Tue, 22 Apr 2025 12:15:21 +0200 Subject: [PATCH 1/7] fix(node): Make body capturing more robust --- .../http/SentryHttpInstrumentation.ts | 30 +++++-------------- 1 file changed, 7 insertions(+), 23 deletions(-) diff --git a/packages/node/src/integrations/http/SentryHttpInstrumentation.ts b/packages/node/src/integrations/http/SentryHttpInstrumentation.ts index 0e0502b6fd1f..49b72f80299a 100644 --- a/packages/node/src/integrations/http/SentryHttpInstrumentation.ts +++ b/packages/node/src/integrations/http/SentryHttpInstrumentation.ts @@ -400,29 +400,6 @@ function patchRequestToCaptureBody(req: IncomingMessage, isolationScope: Scope): 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'); - - if (body) { - const normalizedRequest = { data: body } satisfies RequestEventData; - isolationScope.setSDKProcessingMetadata({ normalizedRequest }); - } - } 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); }, }); @@ -445,6 +422,13 @@ function patchRequestToCaptureBody(req: IncomingMessage, isolationScope: Scope): return Reflect.apply(target, thisArg, args); }, }); + + req.on('end', () => { + const body = Buffer.concat(chunks).toString('utf-8'); + if (body) { + isolationScope.setSDKProcessingMetadata({ normalizedRequest: { data: body } }); + } + }); } catch { // ignore errors if we can't patch stuff } From 07d85cebb100b77c72dc48e2afafecd77b2e4947 Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Tue, 22 Apr 2025 13:05:51 +0200 Subject: [PATCH 2/7] . From 793812b6eaf6d6b34306b3a2b9456949b7f427e7 Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Tue, 22 Apr 2025 13:27:43 +0200 Subject: [PATCH 3/7] Bufferify --- .../http/SentryHttpInstrumentation.ts | 30 ++++++++++--------- 1 file changed, 16 insertions(+), 14 deletions(-) diff --git a/packages/node/src/integrations/http/SentryHttpInstrumentation.ts b/packages/node/src/integrations/http/SentryHttpInstrumentation.ts index d0bc0b1bea84..a5a55a4c1b71 100644 --- a/packages/node/src/integrations/http/SentryHttpInstrumentation.ts +++ b/packages/node/src/integrations/http/SentryHttpInstrumentation.ts @@ -360,12 +360,9 @@ 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 @@ -386,16 +383,21 @@ function patchRequestToCaptureBody(req: IncomingMessage, isolationScope: Scope): if (event === 'data') { const callback = new Proxy(listener, { apply: (target, thisArg, args: Parameters) => { - // If we have already read more than the max body length, we stop adding chunks - // To avoid growing the memory indefinitely if a response is e.g. streamed - if (getChunksSize() < MAX_BODY_BYTE_LENGTH) { - const chunk = args[0] as Buffer; - chunks.push(chunk); - } else if (DEBUG_BUILD) { - logger.log( - INSTRUMENTATION_NAME, - `Dropping request body chunk because maximum body length of ${MAX_BODY_BYTE_LENGTH}b is exceeded.`, - ); + try { + const chunk = args[0] as Buffer | string; + const bufferifiedChunk = Buffer.from(chunk); + + if (bodyByteLength < MAX_BODY_BYTE_LENGTH) { + chunks.push(bufferifiedChunk); + bodyByteLength += bufferifiedChunk.length; + } else if (DEBUG_BUILD) { + logger.log( + INSTRUMENTATION_NAME, + `Dropping request body chunk because maximum body length of ${MAX_BODY_BYTE_LENGTH}b is exceeded.`, + ); + } + } catch { + // noop } return Reflect.apply(target, thisArg, args); From de8072b95d39ab76cf2a662ab76fb94d21364069 Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Tue, 22 Apr 2025 13:28:35 +0200 Subject: [PATCH 4/7] lint --- .../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 a5a55a4c1b71..0747011cae18 100644 --- a/packages/node/src/integrations/http/SentryHttpInstrumentation.ts +++ b/packages/node/src/integrations/http/SentryHttpInstrumentation.ts @@ -3,7 +3,7 @@ import { context, propagation } from '@opentelemetry/api'; import { VERSION } from '@opentelemetry/core'; import type { InstrumentationConfig } from '@opentelemetry/instrumentation'; import { InstrumentationBase, InstrumentationNodeModuleDefinition } from '@opentelemetry/instrumentation'; -import type { AggregationCounts, Client, RequestEventData, SanitizedRequestData, Scope } from '@sentry/core'; +import type { AggregationCounts, Client, SanitizedRequestData, Scope } from '@sentry/core'; import { addBreadcrumb, generateSpanId, From c60142d99c8fcecca04d5444457c3b972363e4b3 Mon Sep 17 00:00:00 2001 From: Francesco Gringl-Novy Date: Tue, 22 Apr 2025 13:29:38 +0200 Subject: [PATCH 5/7] test(fastify): Add test for request body parsing in fastify (#16107) Adding test for https://github.com/getsentry/sentry-javascript/issues/16090 --- .../test-applications/node-fastify/src/app.ts | 4 ++ .../node-fastify/tests/transactions.test.ts | 37 +++++++++++++++++++ 2 files changed, 41 insertions(+) diff --git a/dev-packages/e2e-tests/test-applications/node-fastify/src/app.ts b/dev-packages/e2e-tests/test-applications/node-fastify/src/app.ts index 0f37bc33b90a..7f7ac390b4b3 100644 --- a/dev-packages/e2e-tests/test-applications/node-fastify/src/app.ts +++ b/dev-packages/e2e-tests/test-applications/node-fastify/src/app.ts @@ -119,6 +119,10 @@ app.get('/test-outgoing-http-external-disallowed', async function (req, res) { res.send(data); }); +app.post('/test-post', function (req, res) { + res.send({ status: 'ok', body: req.body }); +}); + app.listen({ port: port }); // A second app so we can test header propagation between external URLs diff --git a/dev-packages/e2e-tests/test-applications/node-fastify/tests/transactions.test.ts b/dev-packages/e2e-tests/test-applications/node-fastify/tests/transactions.test.ts index 01e07538dc72..f7c0aa7f5b0e 100644 --- a/dev-packages/e2e-tests/test-applications/node-fastify/tests/transactions.test.ts +++ b/dev-packages/e2e-tests/test-applications/node-fastify/tests/transactions.test.ts @@ -124,3 +124,40 @@ test('Sends an API route transaction', async ({ baseURL }) => { origin: 'manual', }); }); + +test('Captures request metadata', async ({ baseURL }) => { + const transactionEventPromise = waitForTransaction('node-fastify', 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({ + cookies: {}, + 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 fe7973aec00155e36d34fc5914923c01c691c599 Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Tue, 22 Apr 2025 15:18:45 +0200 Subject: [PATCH 6/7] bytelength --- .../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 0747011cae18..a382fffaab17 100644 --- a/packages/node/src/integrations/http/SentryHttpInstrumentation.ts +++ b/packages/node/src/integrations/http/SentryHttpInstrumentation.ts @@ -389,7 +389,7 @@ function patchRequestToCaptureBody(req: IncomingMessage, isolationScope: Scope): if (bodyByteLength < MAX_BODY_BYTE_LENGTH) { chunks.push(bufferifiedChunk); - bodyByteLength += bufferifiedChunk.length; + bodyByteLength += bufferifiedChunk.byteLength; } else if (DEBUG_BUILD) { logger.log( INSTRUMENTATION_NAME, From 331bcabd72f2246e68fdeebc47f863f93f3f5cb0 Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Tue, 22 Apr 2025 15:20:54 +0200 Subject: [PATCH 7/7] msg --- .../node/src/integrations/http/SentryHttpInstrumentation.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/node/src/integrations/http/SentryHttpInstrumentation.ts b/packages/node/src/integrations/http/SentryHttpInstrumentation.ts index a382fffaab17..d9ef31fa579b 100644 --- a/packages/node/src/integrations/http/SentryHttpInstrumentation.ts +++ b/packages/node/src/integrations/http/SentryHttpInstrumentation.ts @@ -396,8 +396,8 @@ function patchRequestToCaptureBody(req: IncomingMessage, isolationScope: Scope): `Dropping request body chunk because maximum body length of ${MAX_BODY_BYTE_LENGTH}b is exceeded.`, ); } - } catch { - // noop + } catch (err) { + DEBUG_BUILD && logger.error(INSTRUMENTATION_NAME, 'Encountered error while storing body chunk.'); } return Reflect.apply(target, thisArg, args);