Conversation
size-limit report 📦
|
|
bugbot run |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 6db8941. Configure here.
| setCookie: 'session=abc123; theme=dark', | ||
| setCookie: 'session=abc123; Path=/', |
There was a problem hiding this comment.
q: were these tests just wrong before? As in, multiple cookies being set in one set-cookie header?
There was a problem hiding this comment.
Yes, that's the Cookie syntax. For Set-Cookie, those other values are just other attributes like Max-Age or Path (which we don't anymore now - just key/value).
But outcome of our offline discussion was that we might send the set-cookie attributes as well and see set-cookie as one joined string.
| .map(segment => segment.trim()) | ||
| // ";;" and trailing ";" leave empty segments | ||
| .filter(segment => segment !== '') | ||
| .map(segment => { |
There was a problem hiding this comment.
l: should we use a good old for loop over the three loops here? This might be slightly more performant but given we're deailing with a list of cookies, it's not a lot of entries most likely. Feel free to keep as-is.
There was a problem hiding this comment.
I would keep it as a cookie header only has a handful of entries (so performance does not really matter) and it gives better readability.
There was a problem hiding this comment.
This might be slightly more performant
(nerd-sniped) Technically this approach is just a hair less performant, because we could do the map/filter/map in one pass over the items instead of 3. But even a huge cookie header is capped at a hard limit of 4KiB, so even if they're all single-value keys and values, that's an absolute hard max of less than 1024 items, which is several orders of magnitude less than what would matter, and so we should just optimize for readability.
isaacs
left a comment
isaacs
left a comment
There was a problem hiding this comment.
Some comments that could be good to fix while we're in there, but overall, this is great, and pretty much exactly what stood out as the main improvement suggested by #24090.
Splitting "parse the header into ordered pairs" from "collapse pairs into a record" is the right move. The span path wants order and duplicates, the event path wants a first-wins record with decoded values, and neither had a reason to own a parser.
Tests are much improved as well, love too see it 🔥
| const cookies = normalizedRequest.cookies || (headers?.cookie ? parseCookie(headers.cookie) : undefined); | ||
| const cookies = | ||
| normalizedRequest.cookies || | ||
| (headers?.cookie ? cookiePairsToRecord(parseCookieHeader(headers.cookie, 'cookie')) : undefined); |
There was a problem hiding this comment.
There's a decoding-escape hole here.
This bit produces a record of decoded values. Then line 190 turns that record back into a header string and line 192 re-parses the result. A percent-encoded ; in a cookie value splits into a second, differently named cookie, and that second name escapes the denylist.
Reproduced end to end through processSegmentSpan:
headers: { cookie: 'session=%3Btheme%3Ds3cr3t' }
'http.request.header.cookie': ['session=[Filtered]', 'theme=s3cr3t']
Two more lines keep this alive:
packages/core/src/utils/cookie.tsline 79: decodes the value.packages/core/src/tracing/spans/captureSpan.tsline 107:safeSetSpanJSONAttributesskips keys that already exist. The later pass overrequestData.headersatpackages/core/src/integrations/requestdata.tsline 197 would parse the raw header correctly, but it is a no-op because the cookie pass at line 193 already sethttp.request.header.cookie.
This is pre-existing. But since we're cleaning up cookie handling, and this is the last place that parses a cookie string it built itself, and the fix is small, probably a good idea to clean it up.
Suggestion: only synthesize a cookie string when normalizedRequest.cookies was supplied by the framework; when the data came from headers.cookie, let the header pass at line 197 handle it against the raw value.
Or, maybe better: have extractNormalizedRequestData hand back the CookiePair[] so nothing has to round-trip through a string at all.
| cookieString: string, | ||
| behavior: CollectBehavior, | ||
| headerName: 'cookie' | 'set-cookie' = 'cookie', | ||
| ): Record<string, string> | string { |
There was a problem hiding this comment.
We can drop the | string from this type, I think, if we make line 29 return {}.
| ): Record<string, string> | string { | |
| ): Record<string, string> { |
There was a problem hiding this comment.
Then we can also drop a bunch of ternaries in httpclient.ts, because it'll always be a Record<string,string>.
| // A non-empty string we cannot parse may still hold a session token, so it counts as sensitive. | ||
| if (Object.keys(parsed).length === 0) { | ||
| if (Object.keys(cookies).length === 0) { | ||
| return cookieString ? FILTERED : {}; |
There was a problem hiding this comment.
related to previous comment, we can pare down the return type a bit.
| return cookieString ? FILTERED : {}; | |
| return {}; |
| const parsed = parseCookie(cookieString); | ||
| const cookies = cookiePairsToRecord(parseCookieHeader(cookieString, headerName)); | ||
|
|
||
| // A non-empty string we cannot parse may still hold a session token, so it counts as sensitive. |
There was a problem hiding this comment.
I think this is no longer true, because we throw out anything that isn't a valid key=value pair.
| if (typeof headerValue !== 'string') { | ||
| return []; | ||
| } | ||
| return headerName === 'set-cookie' ? [headerValue.split(';')[0]!] : headerValue.split(';'); |
There was a problem hiding this comment.
Oh! this is going to be a problem if you have multiple cookies in a single set-cookie header, because http headers can be joined by ,.
filterCookies('sid=1; Path=/, theme=dark; Path=/', true, 'set-cookie')
=> { sid: '[Filtered]' }
Where I'd expect that to be { sid: '[Filtered]', theme: 'dark' }
I think the fix here is to first split by ,, and then collect all the set-cookie-style parsed sections, to throw away everything after the first ;.
| if (eqIdx === -1) { | ||
| break; | ||
| /** | ||
| * Splits a `Cookie` / `Set-Cookie` header into its ordered name-value pairs. Values stay as they are on the wire. |
There was a problem hiding this comment.
This isn't quite true? The values get trimmed, at least, right?
| * Splits a `Cookie` / `Set-Cookie` header into its ordered name-value pairs. Values stay as they are on the wire. | |
| * Splits a `Cookie` / `Set-Cookie` header into its ordered name-value pairs. Values are not decoded or unquoted, but may be truncated. |
| return {}; | ||
| } | ||
|
|
||
| try { |
There was a problem hiding this comment.
I think this is a dead try/catch now, right? Can cookiePairsToRecord(parseCookieHeader(cookieString, headerName)) throw?
| .map(segment => segment.trim()) | ||
| // ";;" and trailing ";" leave empty segments | ||
| .filter(segment => segment !== '') | ||
| .map(segment => { |
There was a problem hiding this comment.
If we annotate the type here, it keeps it from slipping open to string[][].
| .map(segment => { | |
| .map((segment): CookiePair => { |
| } | ||
| } | ||
| function decodeCookieValue(value: string): string { | ||
| const unquoted = value.charCodeAt(0) === 0x22 ? value.slice(1, -1) : value; |
There was a problem hiding this comment.
decodeCookieValue strips the last character whenever the first is ", without checking that the last one is also ". Verified: cookiePairsToRecord([['a', '"bar']]) gives { a: 'ba' }. Also it returns '' if the value is '"', which... idk if that's wrong, but it's weird?
| const unquoted = value.charCodeAt(0) === 0x22 ? value.slice(1, -1) : value; | |
| const unquoted = value.length > 1 && value.startsWith('"') && value.endsWith('"') ? value.slice(1, -1) : value; |
This branch has not been deployed
3 participants
parseCookie(used for event cookie records) andparseCookieHeader(used for span attributes) had a different implementation for nameless segments,Set-Cookieattributes, and decoding.parseCookieHeaderis the new parser for both and returns ordered[name, value]pairs, with aset-cookiemode that ignores cookie attributes (like Max-Age).Changes for event attributes
Set-Cookieattributes (e.g. Max-Age)filterCookies('sid=1; Max-Age=3600; Path=/', true, 'set-cookie'){ sid: '[Filtered]', 'Max-Age': '3600', Path: '/' }{ sid: '[Filtered]' }=tokenformfilterCookies('=s3cr3t; theme=dark'){ '': 's3cr3t', theme: 'dark' }, so the token leaks{ '': '[Filtered]', theme: 'dark' }filterCookies('s3cr3t; theme=dark'){ theme: 'dark' }, the token is dropped{ '': '[Filtered]', theme: 'dark' }What stays the same
email=jane%40example.com{ email: 'jane@example.com' }(decoded)['email=jane%40example.com'](raw, as sent)lang=en; lang=de{ lang: 'en' }(first wins)['lang=en', 'lang=de'];;;'[Filtered]'['[Filtered]']Fixes #24501
Added a changelog contribution entry because of this PR: #24525