Skip to content
Open
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
Original file line number Diff line number Diff line change
Expand Up @@ -7,11 +7,17 @@ Sentry.init({
release: '1.0',
tracesSampleRate: 1.0,
transport: loggingTransport,
dataCollection: {
httpHeaders: {
request: { deny: ['x-tenant-id'] },
response: { deny: ['content-length'] },
},
},
integrations: [
Sentry.nativeNodeFetchIntegration({
headersToSpanAttributes: {
requestHeaders: ['x-test-header'],
responseHeaders: ['x-powered-by'],
requestHeaders: ['x-test-header', 'authorization', 'x-tenant-id'],
responseHeaders: ['x-powered-by', 'content-length'],
},
}),
],
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,5 +2,7 @@ import * as Sentry from '@sentry/node';

// eslint-disable-next-line @typescript-eslint/no-floating-promises
Sentry.startSpan({ name: 'test_transaction' }, async () => {
await fetch(`${process.env.SERVER_URL}/api/v0`, { headers: { 'x-test-header': 'test-value' } });
await fetch(`${process.env.SERVER_URL}/api/v0`, {
headers: { 'x-test-header': 'test-value', authorization: 'Bearer super-secret', 'x-tenant-id': 'acme-corp' },
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,12 @@ describe('outgoing fetch spans - headers to span attributes', () => {
origin: 'auto.http.node_fetch',
data: expect.objectContaining({
'http.request.header.x-test-header': ['test-value'],
// Listed in `headersToSpanAttributes`, but the built-in denylist still wins.
'http.request.header.authorization': ['[Filtered]'],
// Listed in `headersToSpanAttributes`, but denied via `dataCollection.httpHeaders`.
'http.request.header.x-tenant-id': ['[Filtered]'],
'http.response.header.x-powered-by': ['Express'],
'http.response.header.content-length': ['[Filtered]'],
}),
}),
]),
Expand Down
12 changes: 3 additions & 9 deletions packages/browser/src/integrations/httpclient.ts
Original file line number Diff line number Diff line change
Expand Up @@ -93,16 +93,12 @@ function _fetchResponseHandler(
const reqCookieStr = request.headers.get('Cookie') || undefined;
if (reqCookieStr) {
const filtered = _INTERNAL_filterCookies(reqCookieStr, dc.cookies);
if (typeof filtered === 'object') {
requestCookies = filtered;
}
requestCookies = typeof filtered === 'string' ? { cookie: filtered } : filtered;
}
const resCookieStr = response.headers.get('Set-Cookie') || undefined;
if (resCookieStr) {
const filtered = _INTERNAL_filterCookies(resCookieStr, dc.cookies);
if (typeof filtered === 'object') {
responseCookies = filtered;
}
responseCookies = typeof filtered === 'string' ? { 'set-cookie': filtered } : filtered;
}
}

Expand Down Expand Up @@ -146,9 +142,7 @@ function _xhrResponseHandler(
const cookieString = xhr.getResponseHeader('Set-Cookie') || xhr.getResponseHeader('set-cookie') || undefined;
if (cookieString) {
const filtered = _INTERNAL_filterCookies(cookieString, dc.cookies);
if (typeof filtered === 'object') {
responseCookies = filtered;
}
responseCookies = typeof filtered === 'string' ? { 'set-cookie': filtered } : filtered;
}
} catch {
// ignore it if parsing fails
Expand Down
5 changes: 4 additions & 1 deletion packages/core/src/utils/data-collection/filterCookies.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,8 @@ import { filterKeyValueData } from './filterKeyValueData';
*
* When individual cookies can be parsed, each key-value pair is filtered
* independently. When parsing fails, the entire string is replaced with `[Filtered]`.
* A nameless segment inside an otherwise parseable string (`"opaque-blob; theme=dark"`) is
* dropped, since a record key cannot carry a `[Filtered]` marker without leaking the token.
*/
export function filterCookies(cookieString: string, behavior: CollectBehavior): Record<string, string> | string {
if (behavior === false) {
Expand All @@ -17,8 +19,9 @@ export function filterCookies(cookieString: string, behavior: CollectBehavior):
try {
const parsed = parseCookie(cookieString);

// A non-empty string we cannot parse may still hold a session token, so it counts as sensitive.
if (Object.keys(parsed).length === 0) {
return {};
return cookieString ? FILTERED : {};

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.

l: This might be a no-op, both httpclient.ts call sites check for an object

const filtered = _INTERNAL_filterCookies(reqCookieStr, dc.cookies);
if (typeof filtered === 'object') {
  requestCookies = filtered;
}

So it would be dropped rather than show up as [FILTERED], maybe we need to adjust those checks as well?

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.

Yeah, came to say a similar thing:

filterCookies('opaque-blob; theme=dark', true) => {"theme":"dark"}
httpHeadersToSpanAttributes({Cookie: 'opaque-blob; theme=dark'}) => ["[Filtered]","theme=dark"]

It seems like we should maybe call out that non-key=value-parseable cookie segments are just dropped?

}

return filterKeyValueData(parsed, behavior, SENSITIVE_COOKIE_NAME_SNIPPETS);
Expand Down
38 changes: 26 additions & 12 deletions packages/core/src/utils/request.ts
Original file line number Diff line number Diff line change
Expand Up @@ -305,11 +305,16 @@ export function httpHeadersToSpanAttributes(

const cookies = parseCookieHeader(value, lowerKey === 'set-cookie');
spanAttributes[`${prefix}${lowerKey}`] = cookies.length
? cookies.map(([cookieKey, cookieValue]) =>
shouldFilterDataKey(cookieKey, cookieBehavior, SENSITIVE_COOKIE_NAME_SNIPPETS)
? cookies.map(([cookieKey, cookieValue]) => {
// A nameless cookie's bare token is its value; no denylist could match it, so it is
// always filtered.
if (cookieKey === '') {
return FILTERED_VALUE;
}
return shouldFilterDataKey(cookieKey, cookieBehavior, SENSITIVE_COOKIE_NAME_SNIPPETS)
? `${cookieKey}=${FILTERED_VALUE}`
: `${cookieKey}=${cookieValue}`,
)
: `${cookieKey}=${cookieValue}`;
})
: [FILTERED_VALUE];
} else {
if (headerBehavior === false) {
Expand Down Expand Up @@ -338,22 +343,31 @@ export function httpHeadersToSpanAttributes(
return spanAttributes;
}

/**

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.

I think it's probably out of scope for this PR, and can definitely be filed as a followup. But, as of this, we have 2 cookie parsers that disagree subtly.

parseCookieHeader (this file) and parseCookie (packages/core/src/utils/cookie.ts line 34, used by filterCookies) differ on nameless segments, URL-decoding, quote-stripping, and Set-Cookie attributes.

One parser returning [name, value][] would let filterCookies and httpHeadersToSpanAttributes share the same rules. Recommend filing it as follow-up rather than growing this patch.

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.

Also, this is kind of weird:

filterCookies('sid=1; Max-Age=3600; Path=/', true)
  => {"sid":"[Filtered]","Max-Age":"3600","Path":"/"}

(Max-Age and Path are reported as if they're cookie key/value pairs.)

Not introduced here, but it probably should get fixed either now or in a cookie parsing consolidation follow-up issue.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I created an issue for that: #24501

* Splits a `Cookie` / `Set-Cookie` header into its name-value pairs.
*
* A segment without an `=` is a nameless cookie, so the bare token is its value (RFC 6265bis):
* it is returned as a pair with an empty name.
*/
function parseCookieHeader(value: string | string[], isSetCookie: boolean): [string, string][] {
// Set-Cookie: one cookie per value, with attributes ("name=value; HttpOnly; Secure")
// Cookie: multiple cookies separated by "; " ("cookie1=value1; cookie2=value2")
// Cookie: multiple cookies separated by ";" (the space after ";" is not guaranteed on the wire)
const cookies = (Array.isArray(value) ? value : [value]).flatMap(headerValue => {
if (typeof headerValue !== 'string' || headerValue === '') {
return [];
}
return isSetCookie ? [headerValue.split(';')[0]!] : headerValue.split('; ');
return isSetCookie ? [headerValue.split(';')[0]!] : headerValue.split(';');
});

return cookies.map(cookie => {
const equalSignIndex = cookie.indexOf('=');
return equalSignIndex !== -1
? [cookie.substring(0, equalSignIndex), cookie.substring(equalSignIndex + 1)]
: [cookie, ''];
});
return cookies
.map(cookie => cookie.trim())
.filter(cookie => cookie !== '')
.map(cookie => {
const equalSignIndex = cookie.indexOf('=');
return equalSignIndex !== -1
? [cookie.substring(0, equalSignIndex), cookie.substring(equalSignIndex + 1)]
: ['', cookie];
});
}

/** Extract the query params from an URL. */
Expand Down
20 changes: 18 additions & 2 deletions packages/core/test/lib/utils/data-collection/filterCookies.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -78,8 +78,24 @@ describe('filterCookies', () => {
expect(filterCookies('', true)).toEqual({});
});

it('returns empty record for string with no key-value pairs', () => {
expect(filterCookies(';;;', true)).toEqual({});
it('filters the whole string when no key-value pairs can be extracted', () => {
expect(filterCookies(';;;', true)).toBe('[Filtered]');
expect(filterCookies('opaque-session-blob', true)).toBe('[Filtered]');
});
});

// Intended behavior for the cookie parsing consolidation follow-up: `Set-Cookie` attributes are
// metadata, not cookies, so they must not show up as key-value pairs. Marked `fails` until the
// shared parser handles them.
describe('Set-Cookie attribute handling (known gaps)', () => {
it.fails('does not report Set-Cookie attributes as cookie pairs', () => {
expect(filterCookies('sid=1; Max-Age=3600; Path=/', true)).toEqual({ sid: '[Filtered]' });
});

it.fails('does not report Expires/Domain attributes as cookie pairs', () => {
expect(filterCookies('theme=dark; Expires=Wed, 21 Oct 2026 07:28:00 GMT; Domain=example.com', true)).toEqual({
theme: 'dark',
});
});
});

Expand Down
36 changes: 32 additions & 4 deletions packages/core/test/lib/utils/request.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -650,8 +650,7 @@ describe('request utils', () => {

it('attaches and filters sensitive cookie headers', () => {
const headers = {
Cookie:
'session=abc123; tracking=enabled; cookie-authentication-key-without-value; theme=dark; lang=en; user_session=xyz789; pref=1',
Cookie: 'session=abc123; tracking=enabled; theme=dark; lang=en; user_session=xyz789; pref=1',
};

const result = httpHeadersToSpanAttributes(headers, resolveDataCollectionOptions({}));
Expand All @@ -660,7 +659,6 @@ describe('request utils', () => {
'http.request.header.cookie': [
'session=[Filtered]',
'tracking=enabled',
'cookie-authentication-key-without-value=[Filtered]',
'theme=dark',
'lang=en',
'user_session=[Filtered]',
Expand All @@ -669,6 +667,35 @@ describe('request utils', () => {
});
});

it('filters cookie segments that are not a name=value pair', () => {
// The bare token is a nameless cookie's value, so it must be filtered.
const headers = { Cookie: 'session=abc123; theme=dark; y7Uu0Rk2QpLmXv3' };

const result = httpHeadersToSpanAttributes(headers, resolveDataCollectionOptions({}));

expect(result).toEqual({
'http.request.header.cookie': ['session=[Filtered]', 'theme=dark', '[Filtered]'],
});
});

it('filters a cookie header that holds no name=value pair', () => {
const headers = { Cookie: 'y7Uu0Rk2QpLmXv3' };

const result = httpHeadersToSpanAttributes(headers, resolveDataCollectionOptions({}));

expect(result).toEqual({ 'http.request.header.cookie': ['[Filtered]'] });
});

it('splits cookies on ";" without a following space', () => {
const headers = { Cookie: 'theme=dark;__Secure-session=abc123' };

const result = httpHeadersToSpanAttributes(headers, resolveDataCollectionOptions({}));

expect(result).toEqual({
'http.request.header.cookie': ['theme=dark', '__Secure-session=[Filtered]'],
});
});

it('filters common framework and provider session-style cookie names', () => {
const headers = {
Cookie:
Expand Down Expand Up @@ -728,7 +755,8 @@ describe('request utils', () => {
['pref=1; Max-Age=3600', { 'http.request.header.set-cookie': ['pref=1'] }],
['color=blue; Path=/dashboard', { 'http.request.header.set-cookie': ['color=blue'] }],
['token=eyJhbGc=.eyJzdWI=.SflKxw; Secure', { 'http.request.header.set-cookie': ['token=[Filtered]'] }],
['auth_required; HttpOnly', { 'http.request.header.set-cookie': ['auth_required=[Filtered]'] }],
// A set-cookie string without "=" is a nameless cookie: the bare token is its value.
['auth_required; HttpOnly', { 'http.request.header.set-cookie': ['[Filtered]'] }],
['empty=; Secure', { 'http.request.header.set-cookie': ['empty='] }],
])('should parse and filter Set-Cookie header: %s', (setCookieValue, expected) => {
const headers = { 'Set-Cookie': setCookieValue };
Expand Down
11 changes: 10 additions & 1 deletion packages/node/src/integrations/node-fetch/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,16 @@ export interface UndiciInstrumentationConfig<RequestType = UndiciRequest, Respon
requestHook?: RequestHookFunction<RequestType>;
/** Function called once response headers have been received */
responseHook?: ResponseHookFunction<RequestType, ResponseType>;
/** Map the following HTTP headers to span attributes. */
/**
* Capture the listed HTTP headers as span attributes
* (`http.request.header.<name>` / `http.response.header.<name>`).
*
* Privacy filtering still applies to every header listed here. A header keeps its value only if
* `dataCollection.httpHeaders` permits it:
* - Sensitive names (`authorization`, `cookie`, ...) always show up as `[Filtered]`.
* - Names on the `deny` list show up as `[Filtered]`.
* - If an `allow` list is configured, a header must appear there as well, or it shows up as `[Filtered]`.
*/
headersToSpanAttributes?: {
requestHeaders?: string[];
responseHeaders?: string[];
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ import {
getUrlQuery,
filterCollectedUrl,
filterCollectedUrlQuery,
httpHeadersToSpanAttributes,
} from '@sentry/core';
import { addFetchRequestBreadcrumb, addTracePropagationHeadersToFetchRequest } from '../../utils/outgoingFetchRequest';
import {
Expand Down Expand Up @@ -312,16 +313,21 @@ function onRequestHeaders(config: NodeFetchOptions, { request, socket }: Request

// After hooks have been processed (which may modify request headers)
// we can collect the headers based on the configuration
if (config.headersToSpanAttributes?.requestHeaders) {
const client = getClient();
if (config.headersToSpanAttributes?.requestHeaders && client) {
const headersToAttribs = new Set(config.headersToSpanAttributes.requestHeaders.map(n => n.toLowerCase()));
const headersMap = parseRequestHeaders(request);

const allowlisted: Record<string, string | string[]> = {};
for (const [name, value] of headersMap.entries()) {
if (headersToAttribs.has(name)) {
const attrValue = Array.isArray(value) ? value : [value];
spanAttributes[`http.request.header.${name}`] = attrValue;
allowlisted[name] = value;
}
}

// An entry in `headersToSpanAttributes` does not exempt a header from the `dataCollection`
// filtering, so the allowlisted subset goes through the same pipeline as any other header.
Object.assign(spanAttributes, httpHeadersToSpanAttributes(allowlisted, client.getDataCollectionOptions()));

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Avoidable getClient in later callbacks

Low Severity

This is more an "is this necessary" check than a hard violation. The new getClient() calls run in later undici channel callbacks, where this file already notes the active context is no longer correct. That can yield no client or the wrong one, so headersToSpanAttributes is skipped or filtered with another client's dataCollection options. Flagged because the review rules ask to call out avoidable getClient() usage in production code.

Additional Locations (1)
Fix in Cursor Fix in Web

Triggered by project rule: PR Review Guidelines for Cursor Bot

Reviewed by Cursor Bugbot for commit bf324ae. Configure here.

}

span.setAttributes(spanAttributes);
Expand Down Expand Up @@ -354,28 +360,31 @@ function onResponseHeaders(config: NodeFetchOptions, { request, response }: Resp
() => undefined,
);

if (config.headersToSpanAttributes?.responseHeaders) {
const client = getClient();
if (config.headersToSpanAttributes?.responseHeaders && client) {
const headersToAttribs = new Set<string>();
config.headersToSpanAttributes?.responseHeaders.forEach(name => headersToAttribs.add(name.toLowerCase()));

const allowlisted: Record<string, string[]> = {};
for (let idx = 0; idx < response.headers.length; idx = idx + 2) {
const nameBuf = response.headers[idx];
const valueBuf = response.headers[idx + 1];
if (nameBuf === undefined || valueBuf === undefined) {
continue;
}
const name = nameBuf.toString().toLowerCase();
const value = valueBuf;

if (headersToAttribs.has(name)) {
const attrName = `http.response.header.${name}`;
if (!Object.prototype.hasOwnProperty.call(spanAttributes, attrName)) {
spanAttributes[attrName] = [value.toString()];
} else {
(spanAttributes[attrName] as string[]).push(value.toString());
}
(allowlisted[name] ??= []).push(valueBuf.toString());
}
}

// An entry in `headersToSpanAttributes` does not exempt a header from the `dataCollection`
// filtering, so the allowlisted subset goes through the same pipeline as any other header.
Object.assign(
spanAttributes,
httpHeadersToSpanAttributes(allowlisted, client.getDataCollectionOptions(), 'response'),
);
}

span.setAttributes(spanAttributes);
Expand Down
Loading