Consolidate the two cookie parsers parseCookieString and getCookieAttachments - #24525
Dextheking1 wants to merge 1 commit into
Conversation
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Want reviews to match your repository better? Bugbot Learning can learn team-specific rules from PR activity. A team admin can enable Learning in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit d673c09. Configure here.
Replaces the diverging `parseCookie` (utils/cookie.ts) and the private `parseCookieHeader` (utils/request.ts) with a single parser returning ordered `[name, value]` pairs, as requested in the linked issue. The consolidated parser keeps `parseCookie`'s splitting, quote-stripping and URL-decoding, gains `parseCookieHeader`'s `Set-Cookie` mode and array input, and additionally: - returns nameless segments as `['', segment]` (dropped by `filterCookies`, always `[Filtered]` in span attributes), - splits on `;` with or without a trailing space, - drops known `Set-Cookie` attribute names (`Max-Age`, `Path`, `Expires`, `Domain`, ...) in cookie mode so they are no longer reported as cookies. Fixes getsentry#24501 Co-Authored-By: Muse Spark <noreply@meta.com>
d673c09 to
c0801ce
Compare
|
Both findings addressed: Reparse corruption — real, and it was in I extracted the filtering into Non-null assertion — replaced
|
Dextheking1
changed the title
parseCookie and parseCookieHeaderparseCookieString and getCookieAttachments
`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
2 participants
Summary
Fixes #24501.
parseCookie(event cookie records viafilterCookies) andparseCookieHeader(span attributes viahttpHeadersToSpanAttributes) disagreed on three things: nameless segments,Set-Cookieattributes, and URL-decoding/quote-stripping. This consolidates them into a singleparseCookiePairs(value, setCookie)inpackages/core/src/utils/cookie.tsthat returns ordered[name, value][]pairs:Set-Cookiemode parses only the segment before the first;; otherwise every;-separated segment is a pair.Set-Cookieattributes (expires,max-age,domain,path,secure,httponly,samesite,partitioned, case-insensitive) are dropped by name in cookie mode — they are metadata, not cookies.=surface as[, segment]; values are unquoted and URL-decoded (with a safe fallback for malformed escapes).requestdata.tsgets a localparseCookieRecordhelper preserving the exact oldparseCookiesemantics for event request data (first-wins record, nameless dropped).Tests
filterCookies.test.ts: flipped the two intended-behavior cases with their exact expectations from the fix(core): Apply the sensitive denylist to cookie headers and configured fetch headers #24090 branch (sid=1; Max-Age=3600; Path=/→{sid: '[Filtered]'}; Expires/Domain case →{theme: 'dark'}), plus a nameless-drop case.request.test.ts: two span-attribute expectations updated for the intended change (auth_required; HttpOnly→['[Filtered]']), plus a new URL-decode/unquote span test.cookie.test.ts: 17 tests (ported cases + new Set-Cookie/attribute/nameless/array coverage).cookie.test.ts(17),filterCookies.test.ts(14),request.test.ts(89),requestdata.test.ts(75).Note: the
it.failstests from #24090 do not exist ondevelopyet (they live only in that still-open PR), so they were re-implemented here with the same expectations — small rebase conflict infilterCookies.test.tsis possible if #24090 merges first.