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.
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