feat(node): Ensure request bodies are reliably captured for http requests (#13746)

This PR started out as trying to fix capturing request bodies for Koa.

Some investigation later, we found out that the fundamental problem was
that we relied on the request body being on `request.body`, which is
non-standard and thus does not necessarily works. It seems that in
express this works because it under the hood writes the body there, but
this is non-standard and rather undefined behavior. For other frameworks
(e.g. Koa and probably more) this did not work, the body was not on the
request and thus never captured. We also had no test coverage for this
overall.

This PR ended up doing a few things:

* Add tests for this for express and koa
* Streamline types for `sdkProcessingMetadata` - this used to be `any`,
which lead to any usage of this not really being typed at all. I added
proper types for this now.
* Generic extraction of the http request body in the http
instrumentation - this should now work for any node framework

Most importantly, I opted to not force this into the existing, rather
complicated and hard to follow request data integration flow. This used
to take an IsomorphicRequest and then did a bunch of conversion etc.

Since now in Node, we always have the same, proper http request (for any
framework, because this always goes through http instrumentation), we
can actually streamline this and normalize this properly at the time
where we set this.

So with this PR, we set a `normalizedRequest` which already has the url,
headers etc. set in a way that we need it/it makes sense.
Additionally, the parsed & stringified request body will be set on this
too.

If this normalized request is set in sdkProcessingMetadata, we will use
it as source of truth instead of the plain `request`. (Note that we
still need the plain request for some auxiliary data that is
non-standard, e.g. `request.user`).

For the body parsing itself, we monkey-patch `req.on('data')`. this way,
we ensure to not add more handlers than a user has, and we only extract
the body if the user is extracting it anyhow, ensuring we do not alter
behavior.

Closes #13722

---------

Co-authored-by: Luca Forstner <[email protected]>

Author: mydea

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);

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 || *",

dev-packages/e2e-tests/test-applications/node-koa/tests/transactions.test.ts

@@ -46,76 +46,129 @@ 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({
+    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);
 });

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",

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);

dev-packages/node-integration-tests/suites/express/tracing/test.ts

@@ -137,5 +137,106 @@ 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', { data: { 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', {
+          headers: { 'Content-Type': 'text/plain' },
+          data: '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', {
+          headers: { 'Content-Type': 'application/octet-stream' },
+          data: 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', {
+          headers: { 'Content-Type': 'application/octet-stream' },
+          data: body,
+        });
+      });
+    });
   });
 });