Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 34 additions & 1 deletion packages/bun/src/integrations/bunserver.ts
Original file line number Diff line number Diff line change
Expand Up @@ -21,8 +21,11 @@ import {
filterCollectedUrl,
filterCollectedUrlQuery,
} from '@sentry/core';
import type { ServeOptions } from 'bun';
import type { Server, ServeOptions } from 'bun';
import {
CLIENT_ADDRESS,
CLIENT_PORT,
NETWORK_PROTOCOL_NAME,
SENTRY_OP,
SENTRY_SEGMENT_NAME_SOURCE,
URL_DOMAIN,
Expand Down Expand Up @@ -242,6 +245,24 @@ function wrapRequestHandler<T extends RouteHandler = RouteHandler>(
const client = getClient();
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();

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(not a blocker or suggestion for this PR, making notes for follow-up)

The span takes the first x-forwarded-for entry as-is. Core's getClientIPAddress (packages/core/src/vendor/getIpAddress.ts line 35), which sets user.ip_address on events, is different:

  • It checks the value with isIP, so values like unknown or junk are skipped. The span keeps them.
  • It also reads Forwarded, X-Real-IP, CF-Connecting-IP, Fly-Client-IP and other headers. The span ignores them, so behind Cloudflare or nginx with X-Real-IP the span reports the proxy while the event reports the client.

So client.address on the span and user.ip_address on the event can disagree. Deno has the same code, and parity with Deno was the goal, so this is not a blocker.

Suggestion (follow-up): export getClientIPAddress from @sentry/core and use it in both the Bun and Deno wrappers: getClientIPAddress(request.headers.toJSON()) || socketAddress?.address.

// 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) {
attributes[CLIENT_PORT] = socketAddress.port;
}
}
Comment on lines +248 to +261

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This section has an interesting bug (and Deno has the same one, which I'm guessing is how it got here).

When x-forwarded-for is present, client.address is the origin client from the header, but client.port is still the socket port. Behind a proxy, that is the proxy's ephemeral port. The span then has an address/port pair that does not describe one endpoint. OTel semantic conventions say that, through an intermediary, client.port SHOULD be the port behind the intermediary. A proxy port is not that, so it is better to leave the attribute unset.

Suggested change
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.
const socketAddress = getRequestIP(args[1], request);
if (forwardedFor || socketAddress?.address) {
attributes[CLIENT_ADDRESS] = forwardedFor || socketAddress?.address;
}
if (socketAddress?.port) {
attributes[CLIENT_PORT] = socketAddress.port;
}
}
let socketAddress: { address: string; port: number } | undefined = undefined;
if (dataCollection?.userInfo) {
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.
socketAddress = getRequestIP(args[1], request);
if (forwardedFor) {
attributes[CLIENT_ADDRESS] = forwardedFor;
} else if (socketAddress) {
attributes[CLIENT_ADDRESS] = socketAddress.address;
attributes[CLIENT_PORT] = socketAddress.port;
}
}

To be honest, it might be better to keep this PR focused on Bun, keep it consistent with Deno's (incorrect) behavior, and fix in a follow-up. But if you feel like updating it, we should get this and the similar fix applied to packages/deno/src/wrap-deno-request-handler.ts lines 102-104, together in this PR, so the two implementations stay consistent.


// describes the OSI application-layer protocol (http), not the scheme (might be https)
attributes[NETWORK_PROTOCOL_NAME] = 'http';

if (dataCollection) {
Object.assign(attributes, httpHeadersToSpanAttributes(request.headers.toJSON(), dataCollection));
}
Expand Down Expand Up @@ -308,6 +329,18 @@ function wrapRequestHandler<T extends RouteHandler = RouteHandler>(
});
}

function getRequestIP(server: unknown, request: Request): { address: string; port: number } | undefined {
if (typeof (server as Partial<Server> | undefined)?.requestIP !== 'function') {
return undefined;
}
try {
return (server as Server).requestIP(request) ?? undefined;
} catch {
// Defensive: never let a failed lookup break the user's handler.
return undefined;
}
}

function getSpanAttributesFromParsedUrl(
parsedUrl: ReturnType<typeof parseStringToURLObject>,
request: Request,
Expand Down
142 changes: 142 additions & 0 deletions packages/bun/test/integrations/bunserver.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -521,6 +521,148 @@ describe('Bun Serve Integration', () => {
});

describe('data collection', () => {
test('captures client address, port and protocol by default', async () => {
const server = Bun.serve({
async fetch(_req) {
return new Response('Bun!');
},
port,
});

await fetch(`http://localhost:${port}/`);

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),
'network.protocol.name': 'http',
}),
}),
expect.any(Function),
);
});

test('captures client address on route handlers', async () => {
const server = Bun.serve({
routes: {
'/users/:id': req => new Response(`User ${req.params.id}`),
},
port,
});

await fetch(`http://localhost:${port}/users/123`);

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('prefers the first x-forwarded-for address over the socket address', async () => {
const server = Bun.serve({
async fetch(_req) {
return new Response('Bun!');
},
port,
});

await fetch(`http://localhost:${port}/`, {
headers: { 'X-Forwarded-For': '203.0.113.7, 10.0.0.1' },
});

await server.stop();

expect(startSpanSpy).toHaveBeenCalledTimes(1);
expect(startSpanSpy).toHaveBeenCalledWith(
expect.objectContaining({
attributes: expect.objectContaining({
'client.address': '203.0.113.7',
}),
}),
expect.any(Function),
);
});

test('does not capture client address when userInfo collection is disabled', async () => {
setupClient({ dataCollection: { userInfo: false } });

const server = Bun.serve({
async fetch(_req) {
return new Response('Bun!');
},
port,
});

await fetch(`http://localhost:${port}/`, {
headers: { 'X-Forwarded-For': '203.0.113.7' },
});

await server.stop();

expect(startSpanSpy).toHaveBeenCalledTimes(1);
expect(startSpanSpy).toHaveBeenCalledWith(
expect.objectContaining({
attributes: expect.not.objectContaining({
'client.address': expect.anything(),
'client.port': expect.anything(),
}),
}),
expect.any(Function),
);
expect(startSpanSpy).toHaveBeenCalledWith(
expect.objectContaining({
attributes: expect.objectContaining({
'network.protocol.name': 'http',
}),
}),
expect.any(Function),
);
});

test('leaves client address unset when the handler gets no server', async () => {
const server = Bun.serve({
async fetch(_req) {
return new Response('Bun!');
},
port,
});

// `server.fetch()` calls the handler with the request only.
const response = await server.fetch(new Request(`http://localhost:${port}/`));

await server.stop();

expect(await response.text()).toBe('Bun!');
expect(startSpanSpy).toHaveBeenCalledTimes(1);
expect(startSpanSpy).toHaveBeenCalledWith(
expect.objectContaining({
attributes: expect.not.objectContaining({
'client.address': expect.anything(),
'client.port': expect.anything(),
}),
}),
expect.any(Function),
);
expect(startSpanSpy).toHaveBeenCalledWith(
expect.objectContaining({
attributes: expect.objectContaining({ 'network.protocol.name': 'http' }),
}),
expect.any(Function),
);
});

test('keeps PII request headers when dataCollection enables full header collection', async () => {
setupClient({ dataCollection: { httpHeaders: { request: true, response: true } } });

Expand Down
Loading