feat(core): Parse individual cookies from cookie header#18325
feat(core): Parse individual cookies from cookie header#18325
Conversation
![sentry[bot]](https://avatars-githubusercontent-com-gh.computerqwq.top/in/12637?s=60&v=4){kind=small}
| @@ -180,6 +196,26 @@ export function httpHeadersToSpanAttributes( | |||
| return spanAttributes; | |||
| } | |||
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
size-limit report 📦
|
node-overhead report 🧳Note: This is a synthetic benchmark with a minimal express app and does not necessarily reflect the real-world performance impact in an application.
|
![cursor[bot]](https://avatars-githubusercontent-com-gh.computerqwq.top/in/1210556?s=60&v=4){kind=small}
Lms24
left a comment
Lms24
left a comment
There was a problem hiding this comment.
I think bugbot is right with the cookie/set-cookie difference, so let's double check this.
one more Q: Do we need to adjust the semantic conventions for "sub" http headers? From what I can tell the spec covers arbitrary keys afoter http.request.headers.<key> but I just want to make sure <key> could contain another . as in the cookie header attribute case.
| } else if (typeof value === 'string') { | ||
| spanAttributes[normalizedKey] = value; | ||
| const lowerCasedHeaderKey = key.toLowerCase(); | ||
| const isCookieHeader = lowerCasedHeaderKey === 'cookie' || lowerCasedHeaderKey === 'set-cookie'; |
There was a problem hiding this comment.
l: probably saves us a few bytes:
| const isCookieHeader = lowerCasedHeaderKey === 'cookie' || lowerCasedHeaderKey === 'set-cookie'; | |
| const isCookieHeader = /^(set-)cookie$?/.test(lowerCasedHeaderKey) |
| const lowerCasedHeaderKey = key.toLowerCase(); | ||
| const isCookieHeader = lowerCasedHeaderKey === 'cookie' || lowerCasedHeaderKey === 'set-cookie'; | ||
|
|
||
| if (isCookieHeader && typeof value === 'string' && value !== '') { |
There was a problem hiding this comment.
q: do we handle arrays of cookie headers? (or is this not relevant for cookie/set-cookie?)
There was a problem hiding this comment.
For the sake of simplicity (as cookies are one string), an array would just be parsed as ...header.cookie: [Filtered]. There is also a test that checks that a cookie attribute is always filtered.
| }); | ||
|
|
||
| it('attaches and filters sensitive a set-cookie header', () => { | ||
| const headers1 = { 'Set-Cookie': 'user_session=def456' }; |
There was a problem hiding this comment.
l: let's add or adjust a test here for a set-cookie header with additional properties (e.g. like max-age)
| const isSetCookie = lowerCasedHeaderKey === 'set-cookie'; | ||
| const semicolonIndex = value.indexOf(';'); |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
2 participants
Parse each individual cookie header and filter sensitive cookies to at least know which keys the cookie string included.
Follow-up on #18311
Closes #18441