feat(core): Parse individual cookies from cookie header #18325
Conversation
| } | ||
|
|
||
| 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.
|
| 'bearer', | ||
| 'sso', | ||
| 'saml', | ||
| 'crsf', |
There was a problem hiding this comment.
Bug: Typo in sensitive header snippet breaks CSRF filtering
The sensitive header snippet 'crsf' is a typo that should be 'csrf' (Cross-Site Request Forgery). This prevents proper filtering of CSRF-related headers and cookies that don't contain other sensitive keywords. For example, a header like X-CSRF or a cookie named csrf-token would not be filtered because 'x-csrf'.includes('crsf') returns false. The existing test passes only because X-CSRF-Token also contains token, which masks this bug.
| } else if (typeof value === 'string') { | ||
| spanAttributes[normalizedKey] = value; | ||
| spanAttributes[normalizedKey] = handleHttpHeader(lowerCasedCookieKey, cookieValue, sendDefaultPii); | ||
| } |
There was a problem hiding this comment.
Bug: Set-Cookie attributes incorrectly parsed as separate cookies
The cookie parsing logic treats Cookie and Set-Cookie headers identically by splitting on '; ', but these headers have fundamentally different formats. Cookie headers contain multiple cookies (name1=value1; name2=value2), while Set-Cookie headers contain a single cookie with attributes (name=value; Path=/; HttpOnly). When processing a Set-Cookie header like session=abc; Path=/; HttpOnly, the code incorrectly creates span attributes for path and httponly as if they were cookie names rather than attributes of the session cookie.
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?)
| }); | ||
|
|
||
| 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)
Parse each individual cookie header and filter sensitive cookies to at least know which keys the cookie string included.
Follow-up on #18311