From 1450d649bdc1b75c50796499e109943368a9d26b Mon Sep 17 00:00:00 2001 From: isaacs Date: Fri, 25 Sep 2026 16:18:43 -0700 Subject: [PATCH 1/3] fix(core, node, bun, deno): Align server span client address with event IP Server spans in Node, Bun and Deno took `client.address` from the first `X-Forwarded-For` entry, with no check that the value is an IP. Events use `getClientIPAddress` for `user.ip_address`. That function checks the value and also reads other forwarding headers (`X-Real-IP`, `CF-Connecting-IP`, `Forwarded`, and more). So a span and the event for the same request could report different client IPs. Spans now use `getClientIPAddress` too. It is exported from `@sentry/core/server`. When the address comes from a forwarding header, the socket port belongs to the proxy, not to the client. `client.port` is now unset in that case. (It was previously the incorrect data, now it is blank, as it should be.) `network.peer.*` in Node still reports the socket values. Bun and Deno now pass the socket address to `sdkProcessingMetadata.ipAddress`, as Node does. Error events get `user.ip_address` when no forwarding header is present, and the `sentry.is_localhost` check works for direct loopback requests. Behavior changes: - Behind a proxy, server spans no longer have `client.port`. - A request with both `X-Client-IP` and `X-Forwarded-For` now reports the `X-Client-IP` value as `client.address`, the same as `user.ip_address` on events. This addresses the follow-up comments raised in #24523. Co-Authored-By: Claude Opus 5.5 (1M context) --- .../suites/tracing/httpIntegration/test.ts | 21 +++ packages/bun/src/integrations/bunserver.ts | 34 +++-- .../bun/test/integrations/bunserver.test.ts | 134 ++++++++++++++++-- .../integrations/http/server-subscription.ts | 18 +-- packages/core/src/server.ts | 1 + .../http/server-subscription.test.ts | 22 ++- .../deno/src/wrap-deno-request-handler.ts | 23 ++- packages/deno/test/deno-serve.test.ts | 108 ++++++++++++++ .../http/httpServerSpansIntegration.ts | 25 ++-- 9 files changed, 328 insertions(+), 58 deletions(-) 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 76c62fdf7bd3..10017949470d 100644 --- a/dev-packages/node-integration-tests/suites/tracing/httpIntegration/test.ts +++ b/dev-packages/node-integration-tests/suites/tracing/httpIntegration/test.ts @@ -171,6 +171,27 @@ describe('httpIntegration', () => { runner.makeRequest('post', '/test?a=1&b=2#hash', { data: 'test body' }); await runner.completed(); }); + + test('prefers the forwarded client address, without the socket port', async () => { + const runner = createRunner() + .expect({ + transaction: transaction => { + const data = transaction.contexts?.trace?.data; + expect(data).toEqual( + expect.objectContaining({ + 'client.address': '203.0.113.7', + 'network.peer.address': '::1', + 'network.peer.port': expect.any(Number), + }), + ); + expect(data).not.toHaveProperty('client.port'); + }, + }) + .start(); + + runner.makeRequest('get', '/test', { headers: { 'X-Forwarded-For': '203.0.113.7, 10.0.0.1' } }); + await runner.completed(); + }); }); describe('custom server.emit', () => { diff --git a/packages/bun/src/integrations/bunserver.ts b/packages/bun/src/integrations/bunserver.ts index 11977876d040..270922aafe9e 100644 --- a/packages/bun/src/integrations/bunserver.ts +++ b/packages/bun/src/integrations/bunserver.ts @@ -21,6 +21,7 @@ import { filterCollectedUrl, filterCollectedUrlQuery, } from '@sentry/core'; +import { getClientIPAddress } from '@sentry/core/server'; import type { Server, ServeOptions } from 'bun'; import { CLIENT_ADDRESS, @@ -245,17 +246,20 @@ function wrapRequestHandler( const client = getClient(); const dataCollection = client?.getDataCollectionOptions(); + const headers = request.headers.toJSON(); + // Bun passes the `Server` as the second argument to both `fetch` and route handlers, except + // when the handler runs through `server.fetch()`. + const socketAddress = getRequestIP(args[1], request); + if (dataCollection?.userInfo) { // `client.address` is the originating client, so a forwarding header wins over the socket, which - // behind a proxy holds the proxy's address. - const forwardedFor = request.headers.get('x-forwarded-for')?.split(',')[0]?.trim(); - // Bun passes the `Server` as the second argument to both `fetch` and route handlers, except - // when the handler runs through `server.fetch()`. - const socketAddress = getRequestIP(args[1], request); - if (forwardedFor || socketAddress?.address) { - attributes[CLIENT_ADDRESS] = forwardedFor || socketAddress?.address; - } - if (socketAddress?.port) { + // behind a proxy holds the proxy's address. The socket port is the proxy's too, so `client.port` + // stays unset then. + const forwardedAddress = getClientIPAddress(headers); + if (forwardedAddress) { + attributes[CLIENT_ADDRESS] = forwardedAddress; + } else if (socketAddress) { + attributes[CLIENT_ADDRESS] = socketAddress.address; attributes[CLIENT_PORT] = socketAddress.port; } } @@ -264,11 +268,12 @@ function wrapRequestHandler( attributes[NETWORK_PROTOCOL_NAME] = 'http'; if (dataCollection) { - Object.assign(attributes, httpHeadersToSpanAttributes(request.headers.toJSON(), dataCollection)); + Object.assign(attributes, httpHeadersToSpanAttributes(headers, dataCollection)); } isolationScope.setSDKProcessingMetadata({ normalizedRequest: winterCGRequestToRequestData(request), + ipAddress: socketAddress?.address, }); if (client && dataCollection) { @@ -329,12 +334,15 @@ function wrapRequestHandler( }); } -function getRequestIP(server: unknown, request: Request): { address: string; port: number } | undefined { - if (typeof (server as Partial | undefined)?.requestIP !== 'function') { +function getRequestIP( + server: Partial> | undefined, + request: Request, +): { address: string; port: number } | undefined { + if (typeof server?.requestIP !== 'function') { return undefined; } try { - return (server as Server).requestIP(request) ?? undefined; + return server.requestIP(request) ?? undefined; } catch { // Defensive: never let a failed lookup break the user's handler. return undefined; diff --git a/packages/bun/test/integrations/bunserver.test.ts b/packages/bun/test/integrations/bunserver.test.ts index d3ed401e095c..391b3c981c59 100644 --- a/packages/bun/test/integrations/bunserver.test.ts +++ b/packages/bun/test/integrations/bunserver.test.ts @@ -570,7 +570,7 @@ describe('Bun Serve Integration', () => { ); }); - test('prefers the first x-forwarded-for address over the socket address', async () => { + test('prefers the first x-forwarded-for address over the socket address, without the socket port', async () => { const server = Bun.serve({ async fetch(_req) { return new Response('Bun!'); @@ -593,6 +593,97 @@ describe('Bun Serve Integration', () => { }), expect.any(Function), ); + expect(startSpanSpy).toHaveBeenCalledWith( + expect.objectContaining({ + attributes: expect.not.objectContaining({ + 'client.port': expect.anything(), + }), + }), + expect.any(Function), + ); + }); + + test('reads the client address from other forwarding headers', async () => { + const server = Bun.serve({ + async fetch(_req) { + return new Response('Bun!'); + }, + port, + }); + + await fetch(`http://localhost:${port}/`, { + headers: { 'X-Real-IP': '203.0.113.8' }, + }); + + await server.stop(); + + expect(startSpanSpy).toHaveBeenCalledTimes(1); + expect(startSpanSpy).toHaveBeenCalledWith( + expect.objectContaining({ + attributes: expect.objectContaining({ + 'client.address': '203.0.113.8', + }), + }), + expect.any(Function), + ); + }); + + test('falls back to the socket address when the forwarding header is not an IP', async () => { + const server = Bun.serve({ + async fetch(_req) { + return new Response('Bun!'); + }, + port, + }); + + await fetch(`http://localhost:${port}/`, { + headers: { 'X-Forwarded-For': 'unknown' }, + }); + + await server.stop(); + + expect(startSpanSpy).toHaveBeenCalledTimes(1); + expect(startSpanSpy).toHaveBeenCalledWith( + expect.objectContaining({ + attributes: expect.objectContaining({ + 'client.address': expect.stringMatching(/^(127\.0\.0\.1|::1|::ffff:127\.0\.0\.1)$/), + 'client.port': expect.any(Number), + }), + }), + expect.any(Function), + ); + }); + + test('sets the socket address as the user IP on error events', async () => { + const events: SentryCore.Event[] = []; + setupClient({ + integrations: [SentryCore.requestDataIntegration()], + beforeSend: event => { + events.push(event); + return null; + }, + }); + + const server = Bun.serve({ + async fetch(_req) { + SentryCore.captureException(new Error('Boom')); + return new Response('Bun!'); + }, + port, + }); + + await fetch(`http://localhost:${port}/`); + + await server.stop(); + await SentryCore.flush(); + + expect(events).toEqual([ + expect.objectContaining({ + user: expect.objectContaining({ + ip_address: expect.stringMatching(/^(127\.0\.0\.1|::1|::ffff:127\.0\.0\.1)$/), + }), + }), + ]); }); test('does not capture client address when userInfo collection is disabled', async () => { @@ -680,8 +771,14 @@ describe('Bun Serve Integration', () => { await server.stop(); expect(startSpanSpy).toHaveBeenCalledTimes(1); - const attributes = startSpanSpy.mock.calls[0]?.[0]?.attributes; - expect(attributes?.['http.request.header.x-forwarded-for']).toEqual(['203.0.113.7']); + expect(startSpanSpy).toHaveBeenCalledWith( + expect.objectContaining({ + attributes: expect.objectContaining({ + 'http.request.header.x-forwarded-for': ['203.0.113.7'], + }), + }), + expect.any(Function), + ); }); test('filters request headers according to the dataCollection deny list', async () => { @@ -703,9 +800,15 @@ describe('Bun Serve Integration', () => { await server.stop(); expect(startSpanSpy).toHaveBeenCalledTimes(1); - const attributes = startSpanSpy.mock.calls[0]?.[0]?.attributes; - expect(attributes?.['http.request.header.x-internal']).toEqual(['[Filtered]']); - expect(attributes?.['http.request.header.x-public']).toEqual(['public-value']); + expect(startSpanSpy).toHaveBeenCalledWith( + expect.objectContaining({ + attributes: expect.objectContaining({ + 'http.request.header.x-internal': ['[Filtered]'], + 'http.request.header.x-public': ['public-value'], + }), + }), + expect.any(Function), + ); }); test('filters always-sensitive request headers even when collection is permissive', async () => { @@ -725,8 +828,14 @@ describe('Bun Serve Integration', () => { await server.stop(); expect(startSpanSpy).toHaveBeenCalledTimes(1); - const attributes = startSpanSpy.mock.calls[0]?.[0]?.attributes; - expect(attributes?.['http.request.header.authorization']).toEqual(['[Filtered]']); + expect(startSpanSpy).toHaveBeenCalledWith( + expect.objectContaining({ + attributes: expect.objectContaining({ + 'http.request.header.authorization': ['[Filtered]'], + }), + }), + expect.any(Function), + ); }); test('applies the dataCollection response header collection behavior', async () => { @@ -746,9 +855,12 @@ describe('Bun Serve Integration', () => { await server.stop(); expect(setAttributesSpy).toHaveBeenCalledTimes(1); - const responseAttributes = setAttributesSpy.mock.calls[0]?.[0]; - expect(responseAttributes?.['http.response.header.x-internal']).toEqual(['[Filtered]']); - expect(responseAttributes?.['http.response.header.x-public']).toEqual(['public-value']); + expect(setAttributesSpy).toHaveBeenCalledWith( + expect.objectContaining({ + 'http.response.header.x-internal': ['[Filtered]'], + 'http.response.header.x-public': ['public-value'], + }), + ); }); }); diff --git a/packages/core/src/integrations/http/server-subscription.ts b/packages/core/src/integrations/http/server-subscription.ts index bc09da8ba6d9..8032f50e5fde 100644 --- a/packages/core/src/integrations/http/server-subscription.ts +++ b/packages/core/src/integrations/http/server-subscription.ts @@ -69,6 +69,7 @@ import { } from '@sentry/conventions/attributes'; import { HTTP_SERVER } from '@sentry/conventions/op'; import { filterCollectedUrl, filterCollectedUrlQuery } from '../../utils/data-collection/filterCollectedUrl'; +import { getClientIPAddress } from '../../vendor/getIpAddress'; // Tree-shakable guard to remove all code related to tracing declare const __SENTRY_TRACING__: boolean; @@ -306,7 +307,6 @@ function buildServerSpanWrap( : `${method} ${httpTargetWithoutQueryFragment}`; const headers = request.headers; const userAgent = headers['user-agent']; - const ips = headers['x-forwarded-for']; const httpVersion = request.httpVersion; const host = headers.host as undefined | string; const hostname = host?.replace(/^(.*)(:[0-9]{1,5})/, '$1') || 'localhost'; @@ -315,8 +315,10 @@ function buildServerSpanWrap( const { localAddress, localPort, remoteAddress, remotePort } = socket ?? {}; const collectClientAddress = client.getDataCollectionOptions().userInfo; // `client.address` is the originating client, so a forwarding header wins over the socket, which - // behind a proxy holds the proxy's address. `network.peer.address` keeps the socket value. - const clientAddress = getForwardedClientAddress(ips) ?? remoteAddress; + // behind a proxy holds the proxy's address. The socket port is the proxy's too, so `client.port` + // stays unset then. `network.peer.*` keeps the socket values. + const forwardedAddress = getClientIPAddress(headers); + const clientAddress = forwardedAddress || remoteAddress; return startSpanManual( { @@ -333,7 +335,7 @@ function buildServerSpanWrap( [NETWORK_LOCAL_ADDRESS]: localAddress, [NETWORK_LOCAL_PORT]: localPort, [CLIENT_ADDRESS]: collectClientAddress ? clientAddress : undefined, - [CLIENT_PORT]: remotePort, + [CLIENT_PORT]: forwardedAddress ? undefined : remotePort, [NETWORK_PEER_ADDRESS]: collectClientAddress ? remoteAddress : undefined, [NETWORK_PEER_PORT]: remotePort, [SENTRY_HTTP_PREFETCH]: isKnownPrefetchRequest(request) || undefined, @@ -392,14 +394,6 @@ function buildServerSpanWrap( }; } -/** - * First entry of `X-Forwarded-For`: the client as seen by the outermost proxy. - * https://opentelemetry.io/docs/specs/semconv/registry/attributes/client/#client-address - */ -function getForwardedClientAddress(forwardedFor: string | string[] | undefined): string | undefined { - return typeof forwardedFor === 'string' ? forwardedFor.split(',')[0]?.trim() || undefined : undefined; -} - function shouldIgnoreSpansForIncomingRequest( request: HttpIncomingMessage, { diff --git a/packages/core/src/server.ts b/packages/core/src/server.ts index e48e0ea1de5f..dd301b5c7b3f 100644 --- a/packages/core/src/server.ts +++ b/packages/core/src/server.ts @@ -23,6 +23,7 @@ export { processHttpServerTransactionEvent, } from './integrations/http/server-transaction-event'; export { recordRequestSession } from './integrations/http/record-request-session'; +export { getClientIPAddress } from './vendor/getIpAddress'; export { addOutgoingRequestBreadcrumb } from './integrations/http/add-outgoing-request-breadcrumb'; export { getRequestUrl, 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 3b749134bd9d..8f8addf7e78a 100644 --- a/packages/core/test/lib/integrations/http/server-subscription.test.ts +++ b/packages/core/test/lib/integrations/http/server-subscription.test.ts @@ -153,12 +153,32 @@ describe('getHttpServerSubscriptions', () => { await makeRequest('/users/42', 'GET', { 'X-Forwarded-For': '203.0.113.7, 198.51.100.1' }); const transaction = await waitForTransaction(); - expect(transaction.contexts?.trace?.data).toEqual( + const data = transaction.contexts?.trace?.data; + expect(data).toEqual( expect.objectContaining({ // the originating client, as reported by the outermost proxy [CLIENT_ADDRESS]: '203.0.113.7', // the immediate peer stays the socket, i.e. the proxy itself [NETWORK_PEER_ADDRESS]: '127.0.0.1', + [NETWORK_PEER_PORT]: expect.any(Number), + }), + ); + // the socket port belongs to the proxy, not the forwarded client + expect(data).not.toHaveProperty(CLIENT_PORT); + }); + + it('falls back to the socket for `client.address` when the forwarding header is not an IP', async () => { + server = http.createServer((_req, res) => res.end('ok')); + await new Promise(resolve => server.listen(0, '127.0.0.1', () => resolve())); + instrument(true); + + await makeRequest('/users/42', 'GET', { 'X-Forwarded-For': 'unknown' }); + const transaction = await waitForTransaction(); + + expect(transaction.contexts?.trace?.data).toEqual( + expect.objectContaining({ + [CLIENT_ADDRESS]: '127.0.0.1', + [CLIENT_PORT]: expect.any(Number), }), ); }); diff --git a/packages/deno/src/wrap-deno-request-handler.ts b/packages/deno/src/wrap-deno-request-handler.ts index 1d53cca0db27..1ab390d3790b 100644 --- a/packages/deno/src/wrap-deno-request-handler.ts +++ b/packages/deno/src/wrap-deno-request-handler.ts @@ -24,6 +24,7 @@ import { winterCGRequestToRequestData, withIsolationScope, } from '@sentry/core'; +import { getClientIPAddress } from '@sentry/core/server'; import { streamResponse } from './utils/streaming'; export type RequestHandlerWrapperOptions = { @@ -93,24 +94,32 @@ export const wrapDenoRequestHandler = ( assignIfSet(attributes, 'http.request.body.size', contentLength && parseInt(contentLength, 10)); assignIfSet(attributes, 'user_agent.original', request.headers.get('user-agent')); + const headers = winterCGHeadersToDict(request.headers); + const socketIp = (info?.remoteAddr as Deno.NetAddr)?.hostname; const dataCollection = client.getDataCollectionOptions(); if (dataCollection.userInfo) { // `client.address` is the originating client, so a forwarding header wins over the socket, which - // behind a proxy holds the proxy's address. - const forwardedFor = request.headers.get('x-forwarded-for')?.split(',')[0]?.trim(); - const socketAddress = (info?.remoteAddr as Deno.NetAddr)?.hostname ?? (info?.remoteAddr as Deno.UnixAddr)?.path; - const clientPort = (info?.remoteAddr as Deno.NetAddr)?.port; - assignIfSet(attributes, CLIENT_ADDRESS, forwardedFor || socketAddress); - assignIfSet(attributes, CLIENT_PORT, clientPort); + // behind a proxy holds the proxy's address. The socket port is the proxy's too, so `client.port` + // stays unset then. + const forwardedAddress = getClientIPAddress(headers); + assignIfSet( + attributes, + CLIENT_ADDRESS, + forwardedAddress || socketIp || (info?.remoteAddr as Deno.UnixAddr)?.path, + ); + if (!forwardedAddress) { + assignIfSet(attributes, CLIENT_PORT, (info?.remoteAddr as Deno.NetAddr)?.port); + } } // describes the OSI application-layer protocol (http), not the scheme (might be https) attributes[NETWORK_PROTOCOL_NAME] = 'http'; - Object.assign(attributes, httpHeadersToSpanAttributes(winterCGHeadersToDict(request.headers), dataCollection)); + Object.assign(attributes, httpHeadersToSpanAttributes(headers, dataCollection)); attributes[SENTRY_OP] = HTTP_SERVER; isolationScope.setSDKProcessingMetadata({ normalizedRequest: winterCGRequestToRequestData(request), + ipAddress: socketIp, }); const configuredBodySize = client.getIntegrationByName( diff --git a/packages/deno/test/deno-serve.test.ts b/packages/deno/test/deno-serve.test.ts index 933aded91ee8..085a9914df80 100644 --- a/packages/deno/test/deno-serve.test.ts +++ b/packages/deno/test/deno-serve.test.ts @@ -552,6 +552,114 @@ Deno.test('Deno.serve should capture client address and port by default', async assertExists(transaction?.contexts?.trace?.data?.['client.port']); }); +Deno.test('Deno.serve should prefer the forwarded client address, without the socket port', async () => { + resetGlobals(); + const transactionEvents: TransactionEvent[] = []; + + init({ + dsn: 'https://username@domain/123', + tracesSampleRate: 1, + traceLifecycle: 'static', + beforeSendTransaction: (event: TransactionEvent) => { + transactionEvents.push(event); + return null; + }, + }) as DenoClient; + + const abortController = new AbortController(); + let onListen: ((_: unknown) => void) | undefined = undefined; + const p = new Promise(resolve => (onListen = resolve)); + const server = Deno.serve({ port: 0, signal: abortController.signal, onListen }, () => { + return new Response('OK'); + }); + await p; + + const res = await fetch(`http://localhost:${server.addr.port}/test`, { + headers: { 'X-Forwarded-For': '203.0.113.7, 10.0.0.1' }, + }); + assertEquals(await res.text(), 'OK'); + + abortController.abort(); + await server.finished; + + assertEquals(transactionEvents.length, 1); + const [transaction] = transactionEvents; + + assertEquals(transaction?.contexts?.trace?.data?.['client.address'], '203.0.113.7'); + assertEquals(transaction?.contexts?.trace?.data?.['client.port'], undefined); +}); + +Deno.test('Deno.serve should fall back to the socket address when the forwarding header is not an IP', async () => { + resetGlobals(); + const transactionEvents: TransactionEvent[] = []; + + init({ + dsn: 'https://username@domain/123', + tracesSampleRate: 1, + traceLifecycle: 'static', + beforeSendTransaction: (event: TransactionEvent) => { + transactionEvents.push(event); + return null; + }, + }) as DenoClient; + + const abortController = new AbortController(); + let onListen: ((_: unknown) => void) | undefined = undefined; + const p = new Promise(resolve => (onListen = resolve)); + const server = Deno.serve({ port: 0, signal: abortController.signal, onListen }, () => { + return new Response('OK'); + }); + await p; + + const res = await fetch(`http://localhost:${server.addr.port}/test`, { + headers: { 'X-Forwarded-For': 'unknown' }, + }); + assertEquals(await res.text(), 'OK'); + + abortController.abort(); + await server.finished; + + assertEquals(transactionEvents.length, 1); + const [transaction] = transactionEvents; + + assertNotEquals(transaction?.contexts?.trace?.data?.['client.address'], 'unknown'); + assertExists(transaction?.contexts?.trace?.data?.['client.address']); + assertExists(transaction?.contexts?.trace?.data?.['client.port']); +}); + +Deno.test('Deno.serve should set the socket address as the user IP on error events', async () => { + resetGlobals(); + const errorEvents: ErrorEvent[] = []; + + init({ + dsn: 'https://username@domain/123', + tracesSampleRate: 1, + traceLifecycle: 'static', + beforeSend: (event: ErrorEvent) => { + errorEvents.push(event); + return null; + }, + }) as DenoClient; + + const abortController = new AbortController(); + let onListen: ((_: unknown) => void) | undefined = undefined; + const p = new Promise(resolve => (onListen = resolve)); + const server = Deno.serve({ port: 0, signal: abortController.signal, onListen }, () => { + captureException(new Error('Boom')); + return new Response('OK'); + }); + await p; + + const res = await fetch(`http://localhost:${server.addr.port}/test`); + assertEquals(await res.text(), 'OK'); + + abortController.abort(); + await server.finished; + + assertEquals(errorEvents.length, 1); + assertExists(errorEvents[0]?.user?.ip_address); +}); + Deno.test('Deno.serve should not capture client address when userInfo collection is disabled', async () => { resetGlobals(); const transactionEvents: TransactionEvent[] = []; diff --git a/packages/node/src/integrations/http/httpServerSpansIntegration.ts b/packages/node/src/integrations/http/httpServerSpansIntegration.ts index a00d048591a0..4712bdd61e7a 100644 --- a/packages/node/src/integrations/http/httpServerSpansIntegration.ts +++ b/packages/node/src/integrations/http/httpServerSpansIntegration.ts @@ -28,7 +28,11 @@ import { import { HTTP_SERVER } from '@sentry/conventions/op'; import type { Event, Integration, IntegrationFn, Span, SpanAttributes, SpanStatus } from '@sentry/core'; import type { HttpIncomingMessage, HttpServerResponse } from '@sentry/core/server'; -import { DEFAULT_IGNORE_STATUS_CODES, processHttpServerTransactionEvent } from '@sentry/core/server'; +import { + DEFAULT_IGNORE_STATUS_CODES, + getClientIPAddress, + processHttpServerTransactionEvent, +} from '@sentry/core/server'; import { debug, getSpanStatusFromHttpCode, @@ -320,14 +324,6 @@ function shouldIgnoreSpansForIncomingRequest( return false; } -/** - * First entry of `X-Forwarded-For`: the client as seen by the outermost proxy. - * https://opentelemetry.io/docs/specs/semconv/registry/attributes/client/#client-address - */ -function getForwardedClientAddress(forwardedFor: string | string[] | undefined): string | undefined { - return typeof forwardedFor === 'string' ? forwardedFor.split(',')[0]?.trim() || undefined : undefined; -} - function getIncomingRequestAttributesOnResponse( request: HttpIncomingMessage, response: HttpServerResponse, @@ -343,11 +339,12 @@ function getIncomingRequestAttributesOnResponse( 'http.response.status_text': statusMessage?.toUpperCase(), }; + // `client.address` is the originating client, so a forwarding header wins over the socket, which + // behind a proxy holds the proxy's address. The socket port is the proxy's too, so `client.port` + // stays unset then. `network.peer.*` below keeps the socket values. + const forwardedAddress = getClientIPAddress(request.headers); if (collectClientAddress) { - // `client.address` is the originating client, so a forwarding header wins over the socket, which - // behind a proxy holds the proxy's address. `network.peer.address` below keeps the socket value. - newAttributes[CLIENT_ADDRESS] = - getForwardedClientAddress(request.headers['x-forwarded-for']) ?? socket?.remoteAddress; + newAttributes[CLIENT_ADDRESS] = forwardedAddress || socket?.remoteAddress; } if (socket) { @@ -355,7 +352,7 @@ function getIncomingRequestAttributesOnResponse( newAttributes[SERVER_PORT] = localPort; newAttributes[NETWORK_LOCAL_ADDRESS] = localAddress; newAttributes[NETWORK_LOCAL_PORT] = localPort; - newAttributes[CLIENT_PORT] = remotePort; + newAttributes[CLIENT_PORT] = forwardedAddress ? undefined : remotePort; newAttributes[NETWORK_PEER_ADDRESS] = collectClientAddress ? remoteAddress : undefined; newAttributes[NETWORK_PEER_PORT] = remotePort; } From 76f4bdb6a16ef140f0f7e151432c7ad56a8152a4 Mon Sep 17 00:00:00 2001 From: isaacs Date: Fri, 25 Sep 2026 16:36:04 -0700 Subject: [PATCH 2/3] fixup! fix(core, node, bun, deno): Align server span client address with event IP --- packages/bun/src/integrations/bunserver.ts | 39 +++++++-------- .../integrations/http/server-subscription.ts | 2 +- packages/core/src/integrations/requestdata.ts | 3 +- packages/core/src/server.ts | 2 +- packages/core/src/utils/clientIPAddress.ts | 47 +++++++++++++++++++ .../test/lib/utils/clientIPAddress.test.ts | 39 +++++++++++++++ packages/deno/test/deno-serve.test.ts | 11 +++-- 7 files changed, 116 insertions(+), 27 deletions(-) create mode 100644 packages/core/src/utils/clientIPAddress.ts create mode 100644 packages/core/test/lib/utils/clientIPAddress.test.ts diff --git a/packages/bun/src/integrations/bunserver.ts b/packages/bun/src/integrations/bunserver.ts index 270922aafe9e..3cc5406fbc05 100644 --- a/packages/bun/src/integrations/bunserver.ts +++ b/packages/bun/src/integrations/bunserver.ts @@ -246,31 +246,32 @@ function wrapRequestHandler( const client = getClient(); const dataCollection = client?.getDataCollectionOptions(); - const headers = request.headers.toJSON(); - // Bun passes the `Server` as the second argument to both `fetch` and route handlers, except - // when the handler runs through `server.fetch()`. - const socketAddress = getRequestIP(args[1], request); - - if (dataCollection?.userInfo) { - // `client.address` is the originating client, so a forwarding header wins over the socket, which - // behind a proxy holds the proxy's address. The socket port is the proxy's too, so `client.port` - // stays unset then. - const forwardedAddress = getClientIPAddress(headers); - if (forwardedAddress) { - attributes[CLIENT_ADDRESS] = forwardedAddress; - } else if (socketAddress) { - attributes[CLIENT_ADDRESS] = socketAddress.address; - attributes[CLIENT_PORT] = socketAddress.port; + let socketAddress: { address: string; port: number } | undefined; + if (dataCollection) { + const headers = request.headers.toJSON(); + // Bun passes the `Server` as the second argument to both `fetch` and route handlers, except + // when the handler runs through `server.fetch()`. + socketAddress = getRequestIP(args[1], request); + + if (dataCollection.userInfo) { + // `client.address` is the originating client, so a forwarding header wins over the socket, which + // behind a proxy holds the proxy's address. The socket port is the proxy's too, so `client.port` + // stays unset then. + const forwardedAddress = getClientIPAddress(headers); + if (forwardedAddress) { + attributes[CLIENT_ADDRESS] = forwardedAddress; + } else if (socketAddress) { + attributes[CLIENT_ADDRESS] = socketAddress.address; + attributes[CLIENT_PORT] = socketAddress.port; + } } + + Object.assign(attributes, httpHeadersToSpanAttributes(headers, dataCollection)); } // describes the OSI application-layer protocol (http), not the scheme (might be https) attributes[NETWORK_PROTOCOL_NAME] = 'http'; - if (dataCollection) { - Object.assign(attributes, httpHeadersToSpanAttributes(headers, dataCollection)); - } - isolationScope.setSDKProcessingMetadata({ normalizedRequest: winterCGRequestToRequestData(request), ipAddress: socketAddress?.address, diff --git a/packages/core/src/integrations/http/server-subscription.ts b/packages/core/src/integrations/http/server-subscription.ts index 8032f50e5fde..3ef8edbf0a49 100644 --- a/packages/core/src/integrations/http/server-subscription.ts +++ b/packages/core/src/integrations/http/server-subscription.ts @@ -69,7 +69,7 @@ import { } from '@sentry/conventions/attributes'; import { HTTP_SERVER } from '@sentry/conventions/op'; import { filterCollectedUrl, filterCollectedUrlQuery } from '../../utils/data-collection/filterCollectedUrl'; -import { getClientIPAddress } from '../../vendor/getIpAddress'; +import { getClientIPAddress } from '../../utils/clientIPAddress'; // Tree-shakable guard to remove all code related to tracing declare const __SENTRY_TRACING__: boolean; diff --git a/packages/core/src/integrations/requestdata.ts b/packages/core/src/integrations/requestdata.ts index a8e0dc14e0a2..8c641f666169 100644 --- a/packages/core/src/integrations/requestdata.ts +++ b/packages/core/src/integrations/requestdata.ts @@ -16,7 +16,8 @@ import { filterQueryParams } from '../utils/data-collection/filterQueryParams'; import { filterUrlQuery } from '../utils/data-collection/filterUrlQuery'; import { filterCookiePairs, httpHeadersToSpanAttributes } from '../utils/request'; import { getUrlQuery } from '../utils/url'; -import { getClientIPAddress, ipHeaderNames } from '../vendor/getIpAddress'; +import { getClientIPAddress } from '../utils/clientIPAddress'; +import { ipHeaderNames } from '../vendor/getIpAddress'; import { safeSetSpanJSONAttributes } from '../tracing/spans/captureSpan'; import { SENTRY_IS_LOCALHOST, URL_FULL, URL_QUERY } from '@sentry/conventions/attributes'; diff --git a/packages/core/src/server.ts b/packages/core/src/server.ts index dd301b5c7b3f..a34657db8d6a 100644 --- a/packages/core/src/server.ts +++ b/packages/core/src/server.ts @@ -23,7 +23,7 @@ export { processHttpServerTransactionEvent, } from './integrations/http/server-transaction-event'; export { recordRequestSession } from './integrations/http/record-request-session'; -export { getClientIPAddress } from './vendor/getIpAddress'; +export { getClientIPAddress } from './utils/clientIPAddress'; export { addOutgoingRequestBreadcrumb } from './integrations/http/add-outgoing-request-breadcrumb'; export { getRequestUrl, diff --git a/packages/core/src/utils/clientIPAddress.ts b/packages/core/src/utils/clientIPAddress.ts new file mode 100644 index 000000000000..eb16facb032f --- /dev/null +++ b/packages/core/src/utils/clientIPAddress.ts @@ -0,0 +1,47 @@ +import { getClientIPAddress as getBareClientIPAddress, ipHeaderNames } from '../vendor/getIpAddress'; + +const IP_HEADER_NAMES = new Set(ipHeaderNames.map(name => name.toLowerCase())); + +/** + * Get the IP address of the client sending a request, from its forwarding headers. + * + * The vendored implementation accepts only bare addresses, so the header values are + * normalized first. Some proxies (for example Azure App Service) add the client port, + * and RFC 7239 allows a quoted `Forwarded` value and any case for `for=`. + */ +export function getClientIPAddress(headers: { [key: string]: string | string[] | undefined }): string | null { + const normalized: { [key: string]: string } = {}; + + for (const [key, value] of Object.entries(headers)) { + const name = key.toLowerCase(); + if (value === undefined || !IP_HEADER_NAMES.has(name)) { + continue; + } + + const joined = Array.isArray(value) ? value.join(',') : value; + normalized[key] = + name === 'forwarded' + ? normalizeForwardedHeader(joined) + : joined + .split(',') + .map(ip => stripPort(ip.trim())) + .join(','); + } + + return getBareClientIPAddress(normalized); +} + +// The vendored parser reads the first `for=` pair only, so only that pair is kept. +function normalizeForwardedHeader(value: string): string { + const forPair = value + .split(/[,;]/) + .map(part => part.trim()) + .find(part => part.slice(0, 4).toLowerCase() === 'for='); + + return forPair ? `for=${stripPort(forPair.slice(4).replace(/^"(.*)"$/, '$1'))}` : ''; +} + +// `203.0.113.7:4711` -> `203.0.113.7`, `[2001:db8::1]:4711` -> `2001:db8::1` +function stripPort(ip: string): string { + return ip.match(/^\[([^\]]+)\](?::\d+)?$/)?.[1] ?? ip.match(/^(\d{1,3}(?:\.\d{1,3}){3}):\d+$/)?.[1] ?? ip; +} diff --git a/packages/core/test/lib/utils/clientIPAddress.test.ts b/packages/core/test/lib/utils/clientIPAddress.test.ts new file mode 100644 index 000000000000..c19c090752ec --- /dev/null +++ b/packages/core/test/lib/utils/clientIPAddress.test.ts @@ -0,0 +1,39 @@ +import { describe, expect, it } from 'vitest'; +import { getClientIPAddress } from '../../../src/utils/clientIPAddress'; + +describe('getClientIPAddress', () => { + it.each([ + ['203.0.113.7', '203.0.113.7'], + ['203.0.113.7:4711', '203.0.113.7'], + ['unknown, 203.0.113.7:4711', '203.0.113.7'], + ['[2001:db8::1]:4711', '2001:db8::1'], + ['[2001:db8::1]', '2001:db8::1'], + ['2001:db8::1', '2001:db8::1'], + ['unknown', null], + ])('parses the X-Forwarded-For value %s', (headerValue, expectedIP) => { + expect(getClientIPAddress({ 'X-Forwarded-For': headerValue })).toBe(expectedIP); + }); + + it.each([ + ['for=203.0.113.7', '203.0.113.7'], + ['For=203.0.113.7', '203.0.113.7'], + ['proto=https;for=203.0.113.7', '203.0.113.7'], + ['for="203.0.113.7:4711"', '203.0.113.7'], + ['for="[2001:db8::1]:4711"', '2001:db8::1'], + ['for=203.0.113.7;proto=https, for=198.51.100.1', '203.0.113.7'], + ['proto=https', null], + ['for=_hidden', null], + ])('parses the Forwarded value %s', (headerValue, expectedIP) => { + expect(getClientIPAddress({ Forwarded: headerValue })).toBe(expectedIP); + }); + + it('joins array header values', () => { + expect(getClientIPAddress({ 'x-forwarded-for': ['unknown', '203.0.113.7:4711'] })).toBe('203.0.113.7'); + }); + + it('keeps the header priority order', () => { + expect(getClientIPAddress({ 'X-Real-IP': '198.51.100.1', 'X-Forwarded-For': '203.0.113.7:4711' })).toBe( + '203.0.113.7', + ); + }); +}); diff --git a/packages/deno/test/deno-serve.test.ts b/packages/deno/test/deno-serve.test.ts index 085a9914df80..f85e596e631a 100644 --- a/packages/deno/test/deno-serve.test.ts +++ b/packages/deno/test/deno-serve.test.ts @@ -2,10 +2,12 @@ import type { ErrorEvent, TransactionEvent } from '@sentry/core'; import { getMainCarrier } from '@sentry/core'; -import { assertEquals, assertExists, assertNotEquals } from 'https://deno.land/std@0.212.0/assert/mod.ts'; +import { assertEquals, assertExists, assertMatch, assertNotEquals } from 'https://deno.land/std@0.212.0/assert/mod.ts'; import type { DenoClient } from '../build/esm/index.js'; import { captureException, captureMessage, init, denoServeIntegration, setTag, setUser } from '../build/esm/index.js'; +const LOOPBACK_ADDRESS = /^(127\.0\.0\.1|::1|::ffff:127\.0\.0\.1)$/; + function resetGlobals(): void { getMainCarrier().__SENTRY__ = undefined; } @@ -622,9 +624,8 @@ Deno.test('Deno.serve should fall back to the socket address when the forwarding assertEquals(transactionEvents.length, 1); const [transaction] = transactionEvents; - assertNotEquals(transaction?.contexts?.trace?.data?.['client.address'], 'unknown'); - assertExists(transaction?.contexts?.trace?.data?.['client.address']); - assertExists(transaction?.contexts?.trace?.data?.['client.port']); + assertMatch(String(transaction?.contexts?.trace?.data?.['client.address']), LOOPBACK_ADDRESS); + assertEquals(typeof transaction?.contexts?.trace?.data?.['client.port'], 'number'); }); Deno.test('Deno.serve should set the socket address as the user IP on error events', async () => { @@ -657,7 +658,7 @@ Deno.test('Deno.serve should set the socket address as the user IP on error even await server.finished; assertEquals(errorEvents.length, 1); - assertExists(errorEvents[0]?.user?.ip_address); + assertMatch(String(errorEvents[0]?.user?.ip_address), LOOPBACK_ADDRESS); }); Deno.test('Deno.serve should not capture client address when userInfo collection is disabled', async () => { From e0907c6356421c13d3c893e68f2fe829b94ab180 Mon Sep 17 00:00:00 2001 From: isaacs Date: Fri, 25 Sep 2026 16:50:26 -0700 Subject: [PATCH 3/3] fix deno e2e test --- .../deno/tests/streamed/transactions.test.ts | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/dev-packages/e2e-tests/test-applications/deno/tests/streamed/transactions.test.ts b/dev-packages/e2e-tests/test-applications/deno/tests/streamed/transactions.test.ts index cb9bb624f6bd..83bf22f3d6dc 100644 --- a/dev-packages/e2e-tests/test-applications/deno/tests/streamed/transactions.test.ts +++ b/dev-packages/e2e-tests/test-applications/deno/tests/streamed/transactions.test.ts @@ -146,6 +146,10 @@ const SEGMENT_SPAN = { type: 'string', value: 'http:', }, + 'user.ip_address': { + type: 'string', + value: expect.any(String), + }, 'user_agent.original': { type: 'string', value: 'node',