Conversation
…red fetch headers Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
size-limit report 📦
|
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
bugbot run |
| */ | ||
| function parseCookieHeader(value: string, isSetCookie: boolean): Record<string, string> | undefined { | ||
| // Set-Cookie: single cookie with attributes ("name=value; HttpOnly; Secure") | ||
| // Cookie: multiple cookies separated by "; " ("cookie1=value1; cookie2=value2") |
There was a problem hiding this comment.
m: The split is on '; ' (semicolon plus space), so a header without the space could still leak.
Cookie: theme=dark;__Secure-session=abc123
-> { 'http.request.header.cookie.theme': 'dark;__Secure-session=abc123' }
Could we use parseCookie and drop this function entirely? It seems to cover all the cases we need to support.
There was a problem hiding this comment.
The parseCookie function does some things a bit differently. Like it does not accept nameless segments and it dedupes cookie names.
The rest is addressed and there's a test for it.
| // A non-empty string we cannot parse may still hold a session token, so it counts as sensitive. | ||
| if (Object.keys(parsed).length === 0) { | ||
| return {}; | ||
| return cookieString ? FILTERED : {}; |
There was a problem hiding this comment.
l: This might be a no-op, both httpclient.ts call sites check for an object
const filtered = _INTERNAL_filterCookies(reqCookieStr, dc.cookies);
if (typeof filtered === 'object') {
requestCookies = filtered;
}So it would be dropped rather than show up as [FILTERED], maybe we need to adjust those checks as well?
There was a problem hiding this comment.
Yeah, came to say a similar thing:
filterCookies('opaque-blob; theme=dark', true) => {"theme":"dark"}
httpHeadersToSpanAttributes({Cookie: 'opaque-blob; theme=dark'}) => ["[Filtered]","theme=dark"]
It seems like we should maybe call out that non-key=value-parseable cookie segments are just dropped?
|
Will wait on the cookie changes Lukas is working on until I rebase this. |
# Conflicts: # packages/core/src/utils/request.ts # packages/core/test/lib/utils/request.test.ts
isaacs
left a comment
isaacs
left a comment
There was a problem hiding this comment.
Overall, the design and intention here is correct, and it does fix a lot of edge cases that were unhandled (or handled improperly) before.
I think we can consolidate a few things to make it even cleaner, but that doesn't need to block landing this, imo.
| const attrValue = Array.isArray(value) ? value : [value]; | ||
| spanAttributes[`http.request.header.${name}`] = attrValue; | ||
| // An allowlist entry does not exempt a header from the denylist. | ||
| spanAttributes[`http.request.header.${name}`] = _INTERNAL_shouldFilterDataKey(name, true) |
There was a problem hiding this comment.
I could be misunderstanding this, but it seems like _INTERNAL_shouldFilterDataKey(name, true) hardcodes true as the behavior, so only the built-in denylist applies. The user's own configuration is still bypassed:
dataCollection: { httpHeaders: { request: false } }means "collect no request headers". Every header listed inheadersToSpanAttributesis still attached to the span.dataCollection: { httpHeaders: { request: { deny: ['x-tenant-id'] } } }is not consulted, so an allowlistedx-tenant-idstill goes out.
Can we pass the resolved behavior instead of true? getClient() is already imported and used in this file (on line 262), so client.getDataCollectionOptions().httpHeaders.request seems like an option.
One side effect of that: with a user behavior of { allow: [...] }, shouldFilterDataKey filters everything not in allow, which would nerf headersToSpanAttributes unless the same names appear in both lists. If that is too aggressive, we could honor false and deny only, and say so in the JSDoc.
There was a problem hiding this comment.
Oh, also: couldn't this loop, and the one on lines 363-387, reuse httpHeadersToSpanAttributes?
Something like this:
const allowlisted: Record<string, string | string[]> = {};
for (const [name, value] of headersMap.entries()) {
if (headersToAttribs.has(name)) {
allowlisted[name] = value;
}
}
Object.assign(
spanAttributes,
httpHeadersToSpanAttributes(allowlisted, client.getDataCollectionOptions(), 'request'),
);But like I said, I could be misunderstanding the reason for the divergence, so if there's a reason to handle it separately, please ignore :)
There was a problem hiding this comment.
Good catch. I implemented your second suggestion!
| // A non-empty string we cannot parse may still hold a session token, so it counts as sensitive. | ||
| if (Object.keys(parsed).length === 0) { | ||
| return {}; | ||
| return cookieString ? FILTERED : {}; |
There was a problem hiding this comment.
Yeah, came to say a similar thing:
filterCookies('opaque-blob; theme=dark', true) => {"theme":"dark"}
httpHeadersToSpanAttributes({Cookie: 'opaque-blob; theme=dark'}) => ["[Filtered]","theme=dark"]
It seems like we should maybe call out that non-key=value-parseable cookie segments are just dropped?
| return spanAttributes; | ||
| } | ||
|
|
||
| /** |
There was a problem hiding this comment.
I think it's probably out of scope for this PR, and can definitely be filed as a followup. But, as of this, we have 2 cookie parsers that disagree subtly.
parseCookieHeader (this file) and parseCookie (packages/core/src/utils/cookie.ts line 34, used by filterCookies) differ on nameless segments, URL-decoding, quote-stripping, and Set-Cookie attributes.
One parser returning [name, value][] would let filterCookies and httpHeadersToSpanAttributes share the same rules. Recommend filing it as follow-up rather than growing this patch.
There was a problem hiding this comment.
Also, this is kind of weird:
filterCookies('sid=1; Max-Age=3600; Path=/', true)
=> {"sid":"[Filtered]","Max-Age":"3600","Path":"/"}
(Max-Age and Path are reported as if they're cookie key/value pairs.)
Not introduced here, but it probably should get fixed either now or in a cookie parsing consolidation follow-up issue.
|
bugbot run |
![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 1 potential issue.
❌ 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 bf324ae. Configure here.
|
|
||
| // An entry in `headersToSpanAttributes` does not exempt a header from the `dataCollection` | ||
| // filtering, so the allowlisted subset goes through the same pipeline as any other header. | ||
| Object.assign(spanAttributes, httpHeadersToSpanAttributes(allowlisted, client.getDataCollectionOptions())); |
There was a problem hiding this comment.
Avoidable getClient in later callbacks
Low Severity
This is more an "is this necessary" check than a hard violation. The new getClient() calls run in later undici channel callbacks, where this file already notes the active context is no longer correct. That can yield no client or the wrong one, so headersToSpanAttributes is skipped or filtered with another client's dataCollection options. Flagged because the review rules ask to call out avoidable getClient() usage in production code.
Additional Locations (1)
Triggered by project rule: PR Review Guidelines for Cursor Bot
Reviewed by Cursor Bugbot for commit bf324ae. Configure here.
3 participants
Three ways a sensitive value slipped past the denylist, now that cookies ship as one array attribute (#24231).
A cookie segment without an
=is a nameless cookie, so the bare token is its value (RFC 6265bis). The SDK treated it as a name, and no name-based denylist can match a value, soCookie: <session-token>shipped the token in the clear. Such segments now become a[Filtered]array element. TheCookieheader was also split on"; ", but the space is not guaranteed on the wire, so a cookie glued on with a bare;leaked inside the previous cookie's value. The split is now on";".Headers listed in
headersToSpanAttributesskipped the denylist entirely, soauthorizationwent out in the clear. The spec says an allowlist never exempts a sensitive name, so those now emit['[Filtered]'].Fixes #24085