Skip to content

Commit cef1d49

Browse files
authored
ref(core): Consolidate cookie parsing into one parser (#24536)
`parseCookie` (used for event cookie records) and `parseCookieHeader` (used for span attributes) had a different implementation for nameless segments, `Set-Cookie` attributes, and decoding. `parseCookieHeader` is the new parser for both and returns ordered `[name, value]` pairs, with a `set-cookie` mode that ignores cookie attributes (like Max-Age). `Set-Cookie` attributes (`Path`, `Domain`, `Max-Age`, ...) are dropped. They can carry PII and are not cookies. ### Changes for event attributes | Case | Input | Before | After | | --- | --- | --- | --- | | `Set-Cookie` attributes (e.g. Max-Age) | `filterCookies('sid=1; Max-Age=3600; Path=/', true, 'set-cookie')` | `{ sid: '[Filtered]', 'Max-Age': '3600', Path: '/' }` | `{ sid: '[Filtered]' }` | | Nameless cookie, `=token` form | `filterCookies('=s3cr3t; theme=dark', true, 'cookie')` | `{ '': 's3cr3t', theme: 'dark' }`, so the token leaks | `{ '': '[Filtered]', theme: 'dark' }` | | Nameless cookie, bare token | `filterCookies('s3cr3t; theme=dark', true, 'cookie')` | `{ theme: 'dark' }`, the token is dropped | `{ '': '[Filtered]', theme: 'dark' }` | | No cookie at all | `filterCookies(';;;', true, 'cookie')` | `'[Filtered]'` | `{}` | ### What stays the same | Case | Input | Event record | Span attribute | | --- | --- | --- | --- | | Encoded value | `email=jane%40example.com` | `{ email: 'jane@example.com' }` (decoded) | `['email=jane%40example.com']` (raw, as sent) | | Repeated name | `lang=en; lang=de` | `{ lang: 'en' }` (first wins) | `['lang=en', 'lang=de']` | | Header with no cookie | `;;;` | `{}` | `['[Filtered]']` | Fixes #24501 Added a changelog contribution entry because of this PR: #24525
1 parent 8b31bf1 commit cef1d49

11 files changed

Lines changed: 367 additions & 164 deletions

File tree

‎CHANGELOG.md‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,7 @@
44

55
- "You miss 100 percent of the chances you don't take. — Wayne Gretzky" — Michael Scott
66

7-
Work in this release was contributed by @psh4607, @thijsw, @trinitiwowka, @nehaprasad-dev, @JealousGx, @Jxxunnn, @eddie333016, @davidmurdoch, @yashschandra, @atharv-sys32, @AG0708, @birkskyum, @mkly, @mcbbugu, @suhailopensource, @zkasuran, @mohd-akram, @RealBhupesh, @halillusion, @psang39, @hafzism, @JosephDoUrden, @Tyagiquamar, @Andarist, @msnelling, @oesnuj, @chiliec, @ihsraham, and @matthewbjones. Thank you for your contributions!
7+
Work in this release was contributed by @psh4607, @thijsw, @trinitiwowka, @nehaprasad-dev, @JealousGx, @Jxxunnn, @eddie333016, @davidmurdoch, @yashschandra, @atharv-sys32, @AG0708, @birkskyum, @mkly, @mcbbugu, @suhailopensource, @zkasuran, @mohd-akram, @RealBhupesh, @halillusion, @psang39, @hafzism, @JosephDoUrden, @Tyagiquamar, @Andarist, @msnelling, @oesnuj, @chiliec, @ihsraham, @Dextheking1, and @matthewbjones. Thank you for your contributions!
88

99
- ref(browser)!: LCP and CLS spans no longer set `browser.web_vital.lcp.report_event` and `browser.web_vital.cls.report_event`. With per-navigation web vitals (the default) the attribute was already never set; it is now also gone when `softNavigations` and `bfcacheNavigations` are turned off. When the values are finalized is unchanged.
1010
- feat(browser)!: `browser.navigation.type` on web vital and bfcache navigation spans now carries the navigation type exactly as web-vitals reports it. `bfcache` is now `back-forward-cache`, and a back/forward navigation that missed the bfcache (`back-forward`) or a discarded-tab restore (`restore`) is no longer folded into `navigate`. Update any dashboards or alerts filtering on `bfcache`.

‎packages/browser/src/integrations/httpclient.ts‎

Lines changed: 3 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -92,13 +92,11 @@ function _fetchResponseHandler(
9292
if (dc.cookies !== false) {
9393
const reqCookieStr = request.headers.get('Cookie') || undefined;
9494
if (reqCookieStr) {
95-
const filtered = _INTERNAL_filterCookies(reqCookieStr, dc.cookies);
96-
requestCookies = typeof filtered === 'string' ? { cookie: filtered } : filtered;
95+
requestCookies = _INTERNAL_filterCookies(reqCookieStr, dc.cookies, 'cookie');
9796
}
9897
const resCookieStr = response.headers.get('Set-Cookie') || undefined;
9998
if (resCookieStr) {
100-
const filtered = _INTERNAL_filterCookies(resCookieStr, dc.cookies);
101-
responseCookies = typeof filtered === 'string' ? { 'set-cookie': filtered } : filtered;
99+
responseCookies = _INTERNAL_filterCookies(resCookieStr, dc.cookies, 'set-cookie');
102100
}
103101
}
104102

@@ -141,8 +139,7 @@ function _xhrResponseHandler(
141139
try {
142140
const cookieString = xhr.getResponseHeader('Set-Cookie') || xhr.getResponseHeader('set-cookie') || undefined;
143141
if (cookieString) {
144-
const filtered = _INTERNAL_filterCookies(cookieString, dc.cookies);
145-
responseCookies = typeof filtered === 'string' ? { 'set-cookie': filtered } : filtered;
142+
responseCookies = _INTERNAL_filterCookies(cookieString, dc.cookies, 'set-cookie');
146143
}
147144
} catch {
148145
// ignore it if parsing fails

‎packages/browser/test/integrations/httpclient.test.ts‎

Lines changed: 15 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -146,7 +146,7 @@ describe('httpClientIntegration', () => {
146146

147147
triggerFetch(fetchHandler, {
148148
requestHeaders: { Authorization: 'Bearer x', Accept: 'application/json', Cookie: 'theme=dark; session=secret' },
149-
responseHeaders: { 'Content-Type': 'text/html', 'Set-Cookie': 'locale=en; session=secret' },
149+
responseHeaders: { 'Content-Type': 'text/html', 'Set-Cookie': 'session=secret; Path=/; HttpOnly' },
150150
});
151151

152152
expect(captureEventSpy).toHaveBeenCalledTimes(1);
@@ -158,7 +158,7 @@ describe('httpClientIntegration', () => {
158158
});
159159
expect(event.request?.cookies).toEqual({ theme: 'dark', session: '[Filtered]' });
160160
expect(event.contexts?.response?.headers).toEqual({ 'content-type': 'text/html', 'set-cookie': '[Filtered]' });
161-
expect(event.contexts?.response?.cookies).toEqual({ locale: 'en', session: '[Filtered]' });
161+
expect(event.contexts?.response?.cookies).toEqual({ session: '[Filtered]' });
162162
});
163163

164164
it('filters PII headers when an explicit deny list is configured', () => {
@@ -244,30 +244,36 @@ describe('httpClientIntegration', () => {
244244
const { xhrHandler, captureEventSpy } = setup();
245245

246246
triggerXhr(xhrHandler, {
247-
setCookie: 'session=abc123; theme=dark; connect.sid=secret',
247+
setCookie: 'connect.sid=s3cr3t; Path=/; HttpOnly',
248248
});
249249

250-
expect(getEvent(captureEventSpy).contexts?.response?.cookies).toEqual({
251-
session: '[Filtered]',
252-
theme: 'dark',
253-
'connect.sid': '[Filtered]',
250+
expect(getEvent(captureEventSpy).contexts?.response?.cookies).toEqual({ 'connect.sid': '[Filtered]' });
251+
});
252+
253+
it('does not report Set-Cookie attributes as response cookies', () => {
254+
const { xhrHandler, captureEventSpy } = setup();
255+
256+
triggerXhr(xhrHandler, {
257+
setCookie: 'theme=dark; Max-Age=3600; Path=/; Domain=example.com',
254258
});
259+
260+
expect(getEvent(captureEventSpy).contexts?.response?.cookies).toEqual({ theme: 'dark' });
255261
});
256262

257263
it('collects response headers and filters response cookies by default', () => {
258264
const { xhrHandler, captureEventSpy } = setup();
259265

260266
triggerXhr(xhrHandler, {
261267
requestHeaders: { Authorization: 'Bearer x' },
262-
setCookie: 'session=abc123; theme=dark',
268+
setCookie: 'session=abc123; Path=/',
263269
allResponseHeaders: 'content-type: text/html',
264270
});
265271

266272
expect(captureEventSpy).toHaveBeenCalledTimes(1);
267273
const event = getEvent(captureEventSpy);
268274
expect(event.request?.headers).toEqual({ Authorization: '[Filtered]' });
269275
expect(event.contexts?.response?.headers).toEqual({ 'content-type': 'text/html' });
270-
expect(event.contexts?.response?.cookies).toEqual({ session: '[Filtered]', theme: 'dark' });
276+
expect(event.contexts?.response?.cookies).toEqual({ session: '[Filtered]' });
271277
});
272278
});
273279
});

‎packages/core/src/integrations/requestdata.ts‎

Lines changed: 19 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -7,12 +7,12 @@ import type { Event } from '../types/event';
77
import type { IntegrationFn } from '../types/integration';
88
import type { QueryParams, RequestEventData } from '../types/request';
99
import type { StreamedSpanJSON } from '../types/span';
10-
import { parseCookie } from '../utils/cookie';
10+
import { cookiePairsToRecord, parseCookieHeader } from '../utils/cookie';
1111
import { SENSITIVE_COOKIE_NAME_SNIPPETS } from '../utils/data-collection/filtering-snippets';
1212
import { filterKeyValueData } from '../utils/data-collection/filterKeyValueData';
1313
import { filterQueryParams } from '../utils/data-collection/filterQueryParams';
1414
import { filterUrlQuery } from '../utils/data-collection/filterUrlQuery';
15-
import { httpHeadersToSpanAttributes } from '../utils/request';
15+
import { filterCookiePairs, httpHeadersToSpanAttributes } from '../utils/request';
1616
import { getUrlQuery } from '../utils/url';
1717
import { getClientIPAddress, ipHeaderNames } from '../vendor/getIpAddress';
1818
import { safeSetSpanJSONAttributes } from '../tracing/spans/captureSpan';
@@ -185,12 +185,20 @@ function addNormalizedRequestDataToSpan(
185185

186186
// Process cookies before headers so normalizedRequest.cookies takes precedence
187187
// over the raw cookie header (matching the processEvent path).
188-
if (requestData.cookies && Object.keys(requestData.cookies).length > 0) {
189-
const cookieString = Object.entries(requestData.cookies)
190-
.map(([name, value]) => `${name}=${value}`)
191-
.join('; ');
192-
const cookieAttributes = httpHeadersToSpanAttributes({ cookie: cookieString }, dataCollection, 'request');
193-
safeSetSpanJSONAttributes(span, cookieAttributes);
188+
if (include.cookies) {
189+
// Cookies are not serialized to a string and re-parsed: a decoded value could contain ";" and
190+
// split into a second, differently named cookie that escapes the denylist.
191+
const cookieHeader = normalizedRequest.headers?.cookie;
192+
const cookiePairs = normalizedRequest.cookies
193+
? Object.entries(normalizedRequest.cookies)
194+
: cookieHeader
195+
? parseCookieHeader(cookieHeader, 'cookie')
196+
: [];
197+
if (cookiePairs.length > 0) {
198+
safeSetSpanJSONAttributes(span, {
199+
'http.request.header.cookie': filterCookiePairs(cookiePairs, dataCollection.cookies),
200+
});
201+
}
194202
}
195203

196204
if (requestData.headers) {
@@ -245,7 +253,9 @@ function extractNormalizedRequestData(
245253
}
246254

247255
if (include.cookies) {
248-
const cookies = normalizedRequest.cookies || (headers?.cookie ? parseCookie(headers.cookie) : undefined);
256+
const cookies =
257+
normalizedRequest.cookies ||
258+
(headers?.cookie ? cookiePairsToRecord(parseCookieHeader(headers.cookie, 'cookie')) : undefined);
249259
requestData.cookies = cookies || {};
250260
}
251261

‎packages/core/src/utils/cookie.ts‎

Lines changed: 55 additions & 38 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
/**
2-
* This code was originally copied from the 'cookie` module at v0.5.0 and was simplified for our use case.
2+
* The value decoding in `cookiePairsToRecord` was originally copied from the 'cookie` module at v0.5.0.
33
* https://github.com/jshttp/cookie/blob/a0c84147aab6266bdb3996cf4062e93907c0b0fc/index.js
44
* It had the following license:
55
*
@@ -28,51 +28,68 @@
2828
* SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE.
2929
*/
3030

31-
/**
32-
* Parses a cookie string
33-
*/
34-
export function parseCookie(str: string): Record<string, string> {
35-
const obj: Record<string, string> = {};
36-
let index = 0;
31+
import { FILTERED_VALUE } from './data-collection/filtering-snippets';
3732

38-
while (index < str.length) {
39-
const eqIdx = str.indexOf('=', index);
33+
/** A cookie's name and raw value. A nameless cookie (RFC 6265bis) has the name `''`. */
34+
export type CookiePair = [name: string, value: string];
4035

41-
// no more cookie pairs
42-
if (eqIdx === -1) {
43-
break;
36+
/**
37+
* Splits a `Cookie` / `Set-Cookie` header into its ordered name-value pairs. Values are trimmed, but not
38+
* decoded or unquoted.
39+
*
40+
* A segment without an `=` is a nameless cookie, so the bare token is its value (RFC 6265bis).
41+
*/
42+
export function parseCookieHeader(value: string | string[], headerName: 'cookie' | 'set-cookie'): CookiePair[] {
43+
// Set-Cookie: one cookie per header, followed by attributes ("name=value; HttpOnly; Secure")
44+
// Cookie: multiple cookies separated by ";" (the space after ";" is not guaranteed on the wire)
45+
const segments = (Array.isArray(value) ? value : [value]).flatMap(headerValue => {
46+
if (typeof headerValue !== 'string') {
47+
return [];
4448
}
49+
return headerName === 'set-cookie' ? [headerValue.split(';')[0]!] : headerValue.split(';');
50+
});
4551

46-
let endIdx = str.indexOf(';', index);
47-
48-
if (endIdx === -1) {
49-
endIdx = str.length;
50-
} else if (endIdx < eqIdx) {
51-
// backtrack on prior semicolon
52-
index = str.lastIndexOf(';', eqIdx - 1) + 1;
53-
continue;
54-
}
52+
return (
53+
segments
54+
.map(segment => segment.trim())
55+
// ";;" and trailing ";" leave empty segments. "=" has neither name nor value, so RFC 6265bis ignores it.
56+
.filter(segment => segment !== '' && segment !== '=')
57+
.map((segment): CookiePair => {
58+
// Only first "=" separates name from value: "jwt=eyJhbGc=" has value "eyJhbGc="
59+
const equalSignIndex = segment.indexOf('=');
60+
return equalSignIndex === -1
61+
? // No "=": nameless cookie, the whole segment is the value
62+
['', segment]
63+
: // Trim both parts, so that "theme = dark" is named "theme", not "theme "
64+
[segment.slice(0, equalSignIndex).trim(), segment.slice(equalSignIndex + 1).trim()];
65+
})
66+
);
67+
}
5568

56-
const key = str.slice(index, eqIdx).trim();
69+
/**
70+
* Converts cookie pairs to a record with decoded values. The first cookie of a name wins.
71+
*
72+
* A nameless cookie's token is its value, and no name-based denylist can match it. So it is stored
73+
* under the name `''` and its value is always filtered.
74+
*/
75+
export function cookiePairsToRecord(pairs: CookiePair[]): Record<string, string> {
76+
const record: Record<string, string> = {};
5777

58-
// only assign once
59-
if (undefined === obj[key]) {
60-
let val = str.slice(eqIdx + 1, endIdx).trim();
78+
for (const [name, value] of pairs) {
79+
if (record[name] === undefined) {
80+
record[name] = name === '' ? FILTERED_VALUE : decodeCookieValue(value);
81+
}
82+
}
6183

62-
// quoted values
63-
if (val.charCodeAt(0) === 0x22) {
64-
val = val.slice(1, -1);
65-
}
84+
return record;
85+
}
6686

67-
try {
68-
obj[key] = val.indexOf('%') !== -1 ? decodeURIComponent(val) : val;
69-
} catch {
70-
obj[key] = val;
71-
}
72-
}
87+
function decodeCookieValue(value: string): string {
88+
const unquoted = value.length > 1 && value.startsWith('"') && value.endsWith('"') ? value.slice(1, -1) : value;
7389

74-
index = endIdx + 1;
90+
try {
91+
return unquoted.indexOf('%') !== -1 ? decodeURIComponent(unquoted) : unquoted;
92+
} catch {
93+
return unquoted;
7594
}
76-
77-
return obj;
7895
}
Lines changed: 14 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -1,31 +1,25 @@
11
import type { CollectBehavior } from '../../types/datacollection';
2-
import { parseCookie } from '../cookie';
3-
import { FILTERED_VALUE as FILTERED, SENSITIVE_COOKIE_NAME_SNIPPETS } from './filtering-snippets';
2+
import { cookiePairsToRecord, parseCookieHeader } from '../cookie';
3+
import { SENSITIVE_COOKIE_NAME_SNIPPETS } from './filtering-snippets';
44
import { filterKeyValueData } from './filterKeyValueData';
55

66
/**
7-
* Filters a cookie string according to a `CollectBehavior`.
7+
* Filters a `Cookie` / `Set-Cookie` header string according to a `CollectBehavior`.
88
*
9-
* When individual cookies can be parsed, each key-value pair is filtered
10-
* independently. When parsing fails, the entire string is replaced with `[Filtered]`.
11-
* A nameless segment inside an otherwise parseable string (`"opaque-blob; theme=dark"`) is
12-
* dropped, since a record key cannot carry a `[Filtered]` marker without leaking the token.
9+
* Each named cookie is filtered independently. A nameless cookie (`"opaque-blob"`, `"=opaque-blob"`)
10+
* is reported as `{ '': '[Filtered]' }`, since its token is the value.
11+
*
12+
* @param headerName - `'set-cookie'` keeps only the cookie pair and ignores the attributes (`Path`, `Max-Age`, ...)
1313
*/
14-
export function filterCookies(cookieString: string, behavior: CollectBehavior): Record<string, string> | string {
14+
export function filterCookies(
15+
cookieString: string,
16+
behavior: CollectBehavior,
17+
headerName: 'cookie' | 'set-cookie',
18+
): Record<string, string> {
1519
if (behavior === false) {
1620
return {};
1721
}
1822

19-
try {
20-
const parsed = parseCookie(cookieString);
21-
22-
// A non-empty string we cannot parse may still hold a session token, so it counts as sensitive.
23-
if (Object.keys(parsed).length === 0) {
24-
return cookieString ? FILTERED : {};
25-
}
26-
27-
return filterKeyValueData(parsed, behavior, SENSITIVE_COOKIE_NAME_SNIPPETS);
28-
} catch {
29-
return FILTERED;
30-
}
23+
const cookies = cookiePairsToRecord(parseCookieHeader(cookieString, headerName));
24+
return filterKeyValueData(cookies, behavior, SENSITIVE_COOKIE_NAME_SNIPPETS);
3125
}

‎packages/core/src/utils/request.ts‎

Lines changed: 15 additions & 35 deletions
Original file line numberDiff line numberDiff line change
@@ -1,10 +1,12 @@
11
/* eslint-disable max-lines-per-function */
22
import { DEBUG_BUILD } from '../debug-build';
33
import type { Scope } from '../scope';
4-
import type { ResolvedDataCollection } from '../types/datacollection';
4+
import type { CollectBehavior, ResolvedDataCollection } from '../types/datacollection';
55
import type { PolymorphicRequest } from '../types/polymorphics';
66
import type { RequestEventData } from '../types/request';
77
import type { WebFetchHeaders, WebFetchRequest } from '../types/webfetchapi';
8+
import type { CookiePair } from './cookie';
9+
import { parseCookieHeader } from './cookie';
810
import { debug } from './debug-logger';
911
import { FILTERED_VALUE, SENSITIVE_COOKIE_NAME_SNIPPETS } from './data-collection/filtering-snippets';
1012
import { shouldFilterDataKey } from './data-collection/filterKeyValueData';
@@ -303,18 +305,10 @@ export function httpHeadersToSpanAttributes(
303305
continue;
304306
}
305307

306-
const cookies = parseCookieHeader(value, lowerKey === 'set-cookie');
308+
const cookies = parseCookieHeader(value, lowerKey);
309+
// A cookie header without a single pair may still hold a token, so it counts as sensitive.
307310
spanAttributes[`${prefix}${lowerKey}`] = cookies.length
308-
? cookies.map(([cookieKey, cookieValue]) => {
309-
// A nameless cookie's bare token is its value; no denylist could match it, so it is
310-
// always filtered.
311-
if (cookieKey === '') {
312-
return FILTERED_VALUE;
313-
}
314-
return shouldFilterDataKey(cookieKey, cookieBehavior, SENSITIVE_COOKIE_NAME_SNIPPETS)
315-
? `${cookieKey}=${FILTERED_VALUE}`
316-
: `${cookieKey}=${cookieValue}`;
317-
})
311+
? filterCookiePairs(cookies, cookieBehavior)
318312
: [FILTERED_VALUE];
319313
} else {
320314
if (headerBehavior === false) {
@@ -343,31 +337,17 @@ export function httpHeadersToSpanAttributes(
343337
return spanAttributes;
344338
}
345339

346-
/**
347-
* Splits a `Cookie` / `Set-Cookie` header into its name-value pairs.
348-
*
349-
* A segment without an `=` is a nameless cookie, so the bare token is its value (RFC 6265bis):
350-
* it is returned as a pair with an empty name.
351-
*/
352-
function parseCookieHeader(value: string | string[], isSetCookie: boolean): [string, string][] {
353-
// Set-Cookie: one cookie per value, with attributes ("name=value; HttpOnly; Secure")
354-
// Cookie: multiple cookies separated by ";" (the space after ";" is not guaranteed on the wire)
355-
const cookies = (Array.isArray(value) ? value : [value]).flatMap(headerValue => {
356-
if (typeof headerValue !== 'string' || headerValue === '') {
357-
return [];
340+
/** Formats cookie pairs as `name=value` span attribute values, with sensitive values replaced. */
341+
export function filterCookiePairs(cookies: CookiePair[], cookieBehavior: CollectBehavior): string[] {
342+
return cookies.map(([cookieKey, cookieValue]) => {
343+
// A nameless cookie's bare token is its value; no denylist could match it, so it is always filtered.
344+
if (cookieKey === '') {
345+
return FILTERED_VALUE;
358346
}
359-
return isSetCookie ? [headerValue.split(';')[0]!] : headerValue.split(';');
347+
return shouldFilterDataKey(cookieKey, cookieBehavior, SENSITIVE_COOKIE_NAME_SNIPPETS)
348+
? `${cookieKey}=${FILTERED_VALUE}`
349+
: `${cookieKey}=${cookieValue}`;
360350
});
361-
362-
return cookies
363-
.map(cookie => cookie.trim())
364-
.filter(cookie => cookie !== '')
365-
.map(cookie => {
366-
const equalSignIndex = cookie.indexOf('=');
367-
return equalSignIndex !== -1
368-
? [cookie.substring(0, equalSignIndex), cookie.substring(equalSignIndex + 1)]
369-
: ['', cookie];
370-
});
371351
}
372352

373353
/** Extract the query params from an URL. */

0 commit comments

Comments
 (0)