From 0a7a6a6b35948fa1d7d892f45483b5631757c136 Mon Sep 17 00:00:00 2001 From: isaacs Date: Tue, 20 Oct 2026 19:00:02 -0700 Subject: [PATCH] fix(node): Guard user callbacks in the `node:http` server instrumentation An exception in a user callback must never crash the host app. The web `Request` handlers (Deno.serve, Bun.serve, Cloudflare) already guard their callbacks, but the `node:http` server code paths in core and Node did not. A throwing `onSpanCreated` left the span open and kept the request from reaching the user's handler. A throwing `onSpanEnd` skipped `span.end()` and surfaced as an uncaught exception from the response `close` event. Each callback now runs through `safeCallback`, which logs the error in debug builds and uses a fallback. `ignoreIncomingRequests` falls back to `false`, so the span is still created. `ignoreRequestBody` falls back to `true`, so a broken filter cannot leak the body it was meant to hide. These match the web `Request` handler fallbacks. Fixes #25590 Fixes JS-3976 Co-Authored-By: Claude Opus 5.5 (1M context) --- .../instrument-throwing-callbacks.mjs | 20 +++++ .../suites/tracing/httpIntegration/test.ts | 28 +++++++ .../integrations/http/server-subscription.ts | 30 +++++++- .../http/server-subscription.test.ts | 76 ++++++++++++++++++- packages/deno/test/deno-http.test.ts | 43 +++++++++++ .../http/httpServerSpansIntegration.ts | 15 +++- 6 files changed, 204 insertions(+), 8 deletions(-) create mode 100644 dev-packages/node-integration-tests/suites/tracing/httpIntegration/instrument-throwing-callbacks.mjs diff --git a/dev-packages/node-integration-tests/suites/tracing/httpIntegration/instrument-throwing-callbacks.mjs b/dev-packages/node-integration-tests/suites/tracing/httpIntegration/instrument-throwing-callbacks.mjs new file mode 100644 index 000000000000..c2d7c4763781 --- /dev/null +++ b/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'); + }, + }), + ], +}); diff --git a/dev-packages/node-integration-tests/suites/tracing/httpIntegration/test.ts b/dev-packages/node-integration-tests/suites/tracing/httpIntegration/test.ts index 32f8f7ff9eed..28ae77d23617 100644 --- a/dev-packages/node-integration-tests/suites/tracing/httpIntegration/test.ts +++ b/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() diff --git a/packages/core/src/integrations/http/server-subscription.ts b/packages/core/src/integrations/http/server-subscription.ts index 8427c699e031..72e96535c4cd 100644 --- a/packages/core/src/integrations/http/server-subscription.ts +++ b/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; } diff --git a/packages/core/test/lib/integrations/http/server-subscription.test.ts b/packages/core/test/lib/integrations/http/server-subscription.test.ts index 8f8addf7e78a..0cef917bf5b9 100644 --- a/packages/core/test/lib/integrations/http/server-subscription.test.ts +++ b/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 = {}, + body?: string, ): Promise { const { port } = server.address() as AddressInfo; return new Promise((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 = {}): 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 { + 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(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(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(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(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 diff --git a/packages/deno/test/deno-http.test.ts b/packages/deno/test/deno-http.test.ts index e06ca0529418..5fcae6be0793 100644 --- a/packages/deno/test/deno-http.test.ts +++ b/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(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() { diff --git a/packages/node/src/integrations/http/httpServerSpansIntegration.ts b/packages/node/src/integrations/http/httpServerSpansIntegration.ts index b690854c87d2..5765629bdee9 100644 --- a/packages/node/src/integrations/http/httpServerSpansIntegration.ts +++ b/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; }