-
-
Notifications
You must be signed in to change notification settings - Fork 1.9k
fix(core): Apply the sensitive denylist to cookie headers and configured fetch headers #24090
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: develop
Are you sure you want to change the base?
Changes from all commits
0f51828
cde83e3
7ec524d
15156e1
fba09aa
8babbb5
bf324ae
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -305,11 +305,16 @@ export function httpHeadersToSpanAttributes( | |
|
|
||
| const cookies = parseCookieHeader(value, lowerKey === 'set-cookie'); | ||
| spanAttributes[`${prefix}${lowerKey}`] = cookies.length | ||
| ? cookies.map(([cookieKey, cookieValue]) => | ||
| shouldFilterDataKey(cookieKey, cookieBehavior, SENSITIVE_COOKIE_NAME_SNIPPETS) | ||
| ? cookies.map(([cookieKey, cookieValue]) => { | ||
| // A nameless cookie's bare token is its value; no denylist could match it, so it is | ||
| // always filtered. | ||
| if (cookieKey === '') { | ||
| return FILTERED_VALUE; | ||
| } | ||
| return shouldFilterDataKey(cookieKey, cookieBehavior, SENSITIVE_COOKIE_NAME_SNIPPETS) | ||
| ? `${cookieKey}=${FILTERED_VALUE}` | ||
| : `${cookieKey}=${cookieValue}`, | ||
| ) | ||
| : `${cookieKey}=${cookieValue}`; | ||
| }) | ||
| : [FILTERED_VALUE]; | ||
| } else { | ||
| if (headerBehavior === false) { | ||
|
|
@@ -338,22 +343,31 @@ export function httpHeadersToSpanAttributes( | |
| return spanAttributes; | ||
| } | ||
|
|
||
| /** | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.
One parser returning
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also, this is kind of weird: ( Not introduced here, but it probably should get fixed either now or in a cookie parsing consolidation follow-up issue.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I created an issue for that: #24501 |
||
| * Splits a `Cookie` / `Set-Cookie` header into its name-value pairs. | ||
| * | ||
| * A segment without an `=` is a nameless cookie, so the bare token is its value (RFC 6265bis): | ||
| * it is returned as a pair with an empty name. | ||
| */ | ||
| function parseCookieHeader(value: string | string[], isSetCookie: boolean): [string, string][] { | ||
| // Set-Cookie: one cookie per value, with attributes ("name=value; HttpOnly; Secure") | ||
| // Cookie: multiple cookies separated by "; " ("cookie1=value1; cookie2=value2") | ||
| // Cookie: multiple cookies separated by ";" (the space after ";" is not guaranteed on the wire) | ||
| const cookies = (Array.isArray(value) ? value : [value]).flatMap(headerValue => { | ||
| if (typeof headerValue !== 'string' || headerValue === '') { | ||
| return []; | ||
| } | ||
| return isSetCookie ? [headerValue.split(';')[0]!] : headerValue.split('; '); | ||
| return isSetCookie ? [headerValue.split(';')[0]!] : headerValue.split(';'); | ||
| }); | ||
|
|
||
| return cookies.map(cookie => { | ||
| const equalSignIndex = cookie.indexOf('='); | ||
| return equalSignIndex !== -1 | ||
| ? [cookie.substring(0, equalSignIndex), cookie.substring(equalSignIndex + 1)] | ||
| : [cookie, '']; | ||
| }); | ||
| return cookies | ||
| .map(cookie => cookie.trim()) | ||
| .filter(cookie => cookie !== '') | ||
| .map(cookie => { | ||
| const equalSignIndex = cookie.indexOf('='); | ||
| return equalSignIndex !== -1 | ||
| ? [cookie.substring(0, equalSignIndex), cookie.substring(equalSignIndex + 1)] | ||
| : ['', cookie]; | ||
| }); | ||
| } | ||
|
|
||
| /** Extract the query params from an URL. */ | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -40,6 +40,7 @@ import { | |
| getUrlQuery, | ||
| filterCollectedUrl, | ||
| filterCollectedUrlQuery, | ||
| httpHeadersToSpanAttributes, | ||
| } from '@sentry/core'; | ||
| import { addFetchRequestBreadcrumb, addTracePropagationHeadersToFetchRequest } from '../../utils/outgoingFetchRequest'; | ||
| import { | ||
|
|
@@ -312,16 +313,21 @@ function onRequestHeaders(config: NodeFetchOptions, { request, socket }: Request | |
|
|
||
| // After hooks have been processed (which may modify request headers) | ||
| // we can collect the headers based on the configuration | ||
| if (config.headersToSpanAttributes?.requestHeaders) { | ||
| const client = getClient(); | ||
| if (config.headersToSpanAttributes?.requestHeaders && client) { | ||
| const headersToAttribs = new Set(config.headersToSpanAttributes.requestHeaders.map(n => n.toLowerCase())); | ||
| const headersMap = parseRequestHeaders(request); | ||
|
|
||
| const allowlisted: Record<string, string | string[]> = {}; | ||
| for (const [name, value] of headersMap.entries()) { | ||
| if (headersToAttribs.has(name)) { | ||
| const attrValue = Array.isArray(value) ? value : [value]; | ||
| spanAttributes[`http.request.header.${name}`] = attrValue; | ||
| allowlisted[name] = value; | ||
| } | ||
| } | ||
|
|
||
| // 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Avoidable getClient in later callbacksLow Severity This is more an "is this necessary" check than a hard violation. The new Additional Locations (1)Triggered by project rule: PR Review Guidelines for Cursor Bot Reviewed by Cursor Bugbot for commit bf324ae. Configure here. |
||
| } | ||
|
|
||
| span.setAttributes(spanAttributes); | ||
|
|
@@ -354,28 +360,31 @@ function onResponseHeaders(config: NodeFetchOptions, { request, response }: Resp | |
| () => undefined, | ||
| ); | ||
|
|
||
| if (config.headersToSpanAttributes?.responseHeaders) { | ||
| const client = getClient(); | ||
| if (config.headersToSpanAttributes?.responseHeaders && client) { | ||
| const headersToAttribs = new Set<string>(); | ||
| config.headersToSpanAttributes?.responseHeaders.forEach(name => headersToAttribs.add(name.toLowerCase())); | ||
|
|
||
| const allowlisted: Record<string, string[]> = {}; | ||
| for (let idx = 0; idx < response.headers.length; idx = idx + 2) { | ||
| const nameBuf = response.headers[idx]; | ||
| const valueBuf = response.headers[idx + 1]; | ||
| if (nameBuf === undefined || valueBuf === undefined) { | ||
| continue; | ||
| } | ||
| const name = nameBuf.toString().toLowerCase(); | ||
| const value = valueBuf; | ||
|
|
||
| if (headersToAttribs.has(name)) { | ||
| const attrName = `http.response.header.${name}`; | ||
| if (!Object.prototype.hasOwnProperty.call(spanAttributes, attrName)) { | ||
| spanAttributes[attrName] = [value.toString()]; | ||
| } else { | ||
| (spanAttributes[attrName] as string[]).push(value.toString()); | ||
| } | ||
| (allowlisted[name] ??= []).push(valueBuf.toString()); | ||
| } | ||
| } | ||
|
|
||
| // 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(), 'response'), | ||
| ); | ||
| } | ||
|
|
||
| span.setAttributes(spanAttributes); | ||
|
|
||


There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
l: This might be a no-op, both
httpclient.tscall sites check for an objectSo 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, came to say a similar thing:
It seems like we should maybe call out that non-
key=value-parseable cookie segments are just dropped?