fix(node): Guard user callbacks in the node:http server instrumentation

dev-packages/node-integration-tests/suites/tracing/httpIntegration/instrument-throwing-callbacks.mjs

@@ -0,0 +1,20 @@
+import * as Sentry from '@sentry/node';
+import { loggingTransport } from '@sentry-internal/node-integration-tests';
+
+Sentry.init({
+  dsn: 'https://public@dsn.ingest.sentry.io/1337',
+  release: '1.0',
+  tracesSampleRate: 1.0,
+  transport: loggingTransport,
+
+  integrations: [
+    Sentry.httpIntegration({
+      ignoreIncomingRequests: () => {
+        throw new Error('ignoreIncomingRequests error');
+      },
+      onSpanCreated: () => {
+        throw new Error('onSpanCreated error');
+      },
+    }),
+  ],
+});

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

@@ -55,6 +55,34 @@ describe('httpIntegration', () => {
     });
   });
 
+  describe('user callbacks that throw', () => {
+    createEsmAndCjsTests(__dirname, 'server.mjs', 'instrument-throwing-callbacks.mjs', (createRunner, test) => {
+      test('still handles requests and sends the span when `onSpanCreated` and `ignoreIncomingRequests` throw', async () => {
+        const runner = createRunner()
+          .expect({
+            span: {
+              items: expect.arrayContaining([
+                expect.objectContaining({
+                  is_segment: true,
+                  status: 'ok',
+                  attributes: expect.objectContaining({
+                    [URL_FULL]: { type: 'string', value: expect.stringMatching(/\/test$/) },
+                    [HTTP_RESPONSE_STATUS_CODE]: { type: 'integer', value: 200 },
+                    [SENTRY_OP]: { type: 'string', value: HTTP_SERVER },
+                  }),
+                }),
+              ]),
+            },
+          })
+          .start();
+
+        expect(await runner.makeRequest('get', '/test')).toEqual({ response: 'response 1' });
+        expect(await runner.makeRequest('post', '/test')).toEqual({ response: 'response 2' });
+        await runner.completed();
+      });
+    });
+  });
+
   describe('outgoing request span hooks', () => {
     test('runs outgoingRequestHook, outgoingResponseHook and outgoingRequestApplyCustomAttributes', async () => {
       const [SERVER_URL, closeTestServer] = await createTestServer()

packages/core/src/integrations/http/server-subscription.ts

@@ -23,6 +23,7 @@ import { HTTP_ON_SERVER_REQUEST } from './constants';
 import type { HttpIncomingMessage, HttpInstrumentationOptions, HttpServer, HttpServerResponse } from './types';
 import { DEBUG_BUILD } from '../../debug-build';
 import { debug } from '../../utils/debug-logger';
+import { safeCallback } from '../../utils/safeCallback';
 import { getClient, getCurrentScope, getIsolationScope, withIsolationScope } from '../../currentScopes';
 import { hasSpansEnabled } from '../../utils/hasSpansEnabled';
 import {
@@ -147,7 +148,14 @@ export function instrumentServer(options: HttpInstrumentationOptions, server: Ht
         configuredBodySize ??
         (client.getDataCollectionOptions().httpBodies.includes('incomingRequest') ? 'medium' : 'none');
 
-      if (effectiveBodySize !== 'none' && !ignoreRequestBody?.(url, request)) {
+      if (
+        effectiveBodySize !== 'none' &&
+        !safeCallback(
+          DEBUG_BUILD ? 'The `ignoreRequestBody` callback threw an error, ignoring the request body:' : '',
+          () => !!ignoreRequestBody?.(url, request),
+          () => true,
+        )
+      ) {
         patchRequestToCaptureBody(request, isolationScope, effectiveBodySize, INTEGRATION_NAME);
       }
 
@@ -354,7 +362,11 @@ function buildServerSpanWrap(
           },
         },
         span => {
-          onSpanCreated?.(span, request, response);
+          safeCallback(
+            DEBUG_BUILD ? 'The `onSpanCreated` callback threw an error:' : '',
+            () => onSpanCreated?.(span, request, response),
+            () => undefined,
+          );
           // Ensure we only end the span once
           // E.g. error can be emitted before close is emitted
           let isEnded = false;
@@ -372,7 +384,11 @@ function buildServerSpanWrap(
               ...httpHeadersToSpanAttributes(headersToDict(response.headers), dataCollectionOptions, 'response'),
             });
             span.setStatus(status);
-            onSpanEnd?.(span, request, response);
+            safeCallback(
+              DEBUG_BUILD ? 'The `onSpanEnd` callback threw an error:' : '',
+              () => onSpanEnd?.(span, request, response),
+              () => undefined,
+            );
             span.end();
           }
 
@@ -419,7 +435,13 @@ function shouldIgnoreSpansForIncomingRequest(
     return true;
   }
 
-  if (ignoreIncomingRequests?.(urlPath, request)) {
+  if (
+    safeCallback(
+      DEBUG_BUILD ? 'The `ignoreIncomingRequests` callback threw an error, not ignoring the request:' : '',
+      () => !!ignoreIncomingRequests?.(urlPath, request),
+      () => false,
+    )
+  ) {
     return true;
   }
 

packages/core/test/lib/integrations/http/server-subscription.test.ts

@@ -24,6 +24,7 @@ import { spanToJSON } from '../../../../src/utils/spanUtils';
 import { setCurrentClient } from '../../../../src/sdk';
 import { HTTP_ON_SERVER_REQUEST } from '../../../../src/integrations/http/constants';
 import { getHttpServerSubscriptions } from '../../../../src/integrations/http/server-subscription';
+import type { HttpInstrumentationOptions } from '../../../../src/integrations/http/types';
 import type { Event } from '../../../../src/types/event';
 import { getDefaultTestClientOptions, TestClient } from '../../../mocks/client';
 
@@ -62,6 +63,7 @@ describe('getHttpServerSubscriptions', () => {
     path: string,
     method: 'GET' | 'HEAD' | 'OPTIONS' | 'POST' = 'GET',
     extraHeaders: Record<string, string> = {},
+    body?: string,
   ): Promise<void> {
     const { port } = server.address() as AddressInfo;
     return new Promise<void>((resolve, reject) => {
@@ -85,11 +87,11 @@ describe('getHttpServerSubscriptions', () => {
         },
       );
       req.on('error', reject);
-      req.end();
+      req.end(body);
     });
   }
 
-  function instrument(spans: boolean, extra: { ignoreStaticAssets?: boolean } = {}): void {
+  function instrument(spans: boolean, extra: Omit<HttpInstrumentationOptions, 'spans'> = {}): void {
     const { [HTTP_ON_SERVER_REQUEST]: onServerRequest } = getHttpServerSubscriptions({ spans, ...extra });
     // Fire the channel listener manually with the server we're about to use.
     // This avoids depending on Node's diagnostics_channel firing (only
@@ -296,6 +298,76 @@ describe('getHttpServerSubscriptions', () => {
     expect(transaction.transaction).toBe('GET /favicon.ico');
   });
 
+  describe('with user callbacks that throw', () => {
+    const throwingCallback = (): never => {
+      throw new Error('callback error');
+    };
+
+    async function expectRequestHandledTwice(): Promise<void> {
+      await makeRequest('/first');
+      await makeRequest('/second');
+    }
+
+    it('still handles the request and ends the span when `onSpanCreated` throws', async () => {
+      const handler = vi.fn((_req: http.IncomingMessage, res: http.ServerResponse) => res.end('ok'));
+      server = http.createServer(handler);
+      await new Promise<void>(resolve => server.listen(0, '127.0.0.1', () => resolve()));
+      instrument(true, { onSpanCreated: throwingCallback });
+
+      await expectRequestHandledTwice();
+      const transaction = await waitForTransaction();
+
+      expect(handler).toHaveBeenCalledTimes(2);
+      expect(transaction.transaction).toBe('GET /first');
+    });
+
+    it('still handles the request and ends the span when `onSpanEnd` throws', async () => {
+      const handler = vi.fn((_req: http.IncomingMessage, res: http.ServerResponse) => res.end('ok'));
+      server = http.createServer(handler);
+      await new Promise<void>(resolve => server.listen(0, '127.0.0.1', () => resolve()));
+      instrument(true, { onSpanEnd: throwingCallback });
+
+      await expectRequestHandledTwice();
+      const transaction = await waitForTransaction();
+
+      expect(handler).toHaveBeenCalledTimes(2);
+      expect(transaction.transaction).toBe('GET /first');
+    });
+
+    it('creates the span when `ignoreIncomingRequests` throws', async () => {
+      const handler = vi.fn((_req: http.IncomingMessage, res: http.ServerResponse) => res.end('ok'));
+      server = http.createServer(handler);
+      await new Promise<void>(resolve => server.listen(0, '127.0.0.1', () => resolve()));
+      instrument(true, { ignoreIncomingRequests: throwingCallback });
+
+      await expectRequestHandledTwice();
+      const transaction = await waitForTransaction();
+
+      expect(handler).toHaveBeenCalledTimes(2);
+      expect(transaction.transaction).toBe('GET /first');
+    });
+
+    it('skips the request body when `ignoreRequestBody` throws', async () => {
+      const receivedBodies: string[] = [];
+      server = http.createServer((req, res) => {
+        let body = '';
+        req.on('data', chunk => (body += chunk));
+        req.on('end', () => {
+          receivedBodies.push(body);
+          res.end('ok');
+        });
+      });
+      await new Promise<void>(resolve => server.listen(0, '127.0.0.1', () => resolve()));
+      instrument(true, { maxRequestBodySize: 'always', ignoreRequestBody: throwingCallback });
+
+      await makeRequest('/body', 'POST', { 'Content-Type': 'text/plain' }, 'secret');
+      const transaction = await waitForTransaction();
+
+      expect(receivedBodies).toEqual(['secret']);
+      expect(transaction.request?.data).toBeUndefined();
+    });
+  });
+
   it('re-evaluates the spans default per request based on the current client', async () => {
     // Swap in a no-tracing client. The default-from-client decision must be
     // reactive: instrumentation registered before this swap should still

packages/deno/test/deno-http.test.ts

@@ -113,6 +113,49 @@ Deno.test({
   },
 });
 
+Deno.test({
+  name: 'denoHttpIntegration: a throwing onSpanEnd still ends the span and serves later requests',
+  async fn() {
+    resetGlobals();
+    const sink = transactionSink();
+    init({
+      dsn: 'https://username@domain/123',
+      tracesSampleRate: 1,
+      beforeSendTransaction: sink.beforeSendTransaction,
+      traceLifecycle: 'static',
+      integrations: [
+        denoHttpIntegration({
+          onSpanEnd: () => {
+            throw new Error('onSpanEnd error');
+          },
+        }),
+      ],
+    });
+
+    const server = http.createServer((_req, res) => {
+      res.end('ok');
+    });
+    const port: number = await new Promise(resolve => {
+      server.listen(0, '127.0.0.1', () => {
+        resolve((server.address() as { port: number }).port);
+      });
+    });
+
+    assertEquals(await (await fetch(`http://127.0.0.1:${port}/first`)).text(), 'ok');
+    assertEquals(await (await fetch(`http://127.0.0.1:${port}/second`)).text(), 'ok');
+
+    const txn = await withTimeout(
+      sink.waitFor(t => t.transaction === 'GET /first'),
+      5000,
+      'http.server transaction for the first request',
+    );
+
+    await new Promise<void>(resolve => server.close(() => resolve()));
+
+    assertEquals(txn.contexts?.trace?.op, 'http.server');
+  },
+});
+
 Deno.test({
   name: 'denoHttpIntegration: node:http incoming request records a release-health session by default',
   async fn() {

packages/node/src/integrations/http/httpServerSpansIntegration.ts

@@ -52,6 +52,7 @@ import {
   filterCollectedUrlQuery,
   hasSpanStreamingEnabled,
   HTTP_SPAN_NAME_FALLBACK,
+  safeCallback,
 } from '@sentry/core';
 import { DEBUG_BUILD } from '../../debug-build';
 import type { NodeClient } from '../../sdk/client';
@@ -186,7 +187,11 @@ const _httpServerSpansIntegration = ((options: HttpServerSpansIntegrationOptions
             },
           });
 
-          onSpanCreated?.(span, request, response);
+          safeCallback(
+            DEBUG_BUILD ? 'The `onSpanCreated` callback threw an error:' : '',
+            () => onSpanCreated?.(span, request, response),
+            () => undefined,
+          );
 
           return withActiveSpan(span, () => {
             bindScopeToEmitter(request);
@@ -317,7 +322,13 @@ function shouldIgnoreSpansForIncomingRequest(
     return true;
   }
 
-  if (ignoreIncomingRequests?.(urlPath, request)) {
+  if (
+    safeCallback(
+      DEBUG_BUILD ? 'The `ignoreIncomingRequests` callback threw an error, not ignoring the request:' : '',
+      () => !!ignoreIncomingRequests?.(urlPath, request),
+      () => false,
+    )
+  ) {
     return true;
   }