-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
feat(core): Parse individual cookies from cookie header #18325
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 2 commits
22cb73e
27cc608
5a8f350
e5407f0
1b4d3c5
f3231a3
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 | ||||
|---|---|---|---|---|---|---|
| @@ -1,3 +1,4 @@ | ||||||
| import { getClient } from '../currentScopes'; | ||||||
| import type { PolymorphicRequest } from '../types-hoist/polymorphics'; | ||||||
| import type { RequestEventData } from '../types-hoist/request'; | ||||||
| import type { WebFetchHeaders, WebFetchRequest } from '../types-hoist/webfetchapi'; | ||||||
|
|
@@ -128,21 +129,29 @@ function getAbsoluteUrl({ | |||||
| return undefined; | ||||||
| } | ||||||
|
|
||||||
| // "-user" because otherwise it would match "user-agent" | ||||||
| const SENSITIVE_HEADER_SNIPPETS = [ | ||||||
| 'auth', | ||||||
| 'token', | ||||||
| 'secret', | ||||||
| 'cookie', | ||||||
| '-user', | ||||||
| 'session', // for the user_session cookie | ||||||
| 'password', | ||||||
| 'passwd', | ||||||
| 'pwd', | ||||||
| 'key', | ||||||
| 'jwt', | ||||||
| 'bearer', | ||||||
| 'sso', | ||||||
| 'saml', | ||||||
| 'crsf', | ||||||
| 'xsrf', | ||||||
| 'credentials', | ||||||
| // Always treat cookie headers as sensitive in case individual key-value cookie pairs cannot properly be extracted | ||||||
| 'set-cookie', | ||||||
| 'cookie', | ||||||
| ]; | ||||||
|
|
||||||
| const PII_HEADER_SNIPPETS = ['x-forwarded-', '-user']; | ||||||
|
|
||||||
| /** | ||||||
| * Converts incoming HTTP request headers to OpenTelemetry span attributes following semantic conventions. | ||||||
| * Header names are converted to the format: http.request.header.<key> | ||||||
|
|
@@ -152,6 +161,7 @@ const SENSITIVE_HEADER_SNIPPETS = [ | |||||
| */ | ||||||
| export function httpHeadersToSpanAttributes( | ||||||
| headers: Record<string, string | string[] | undefined>, | ||||||
| sendDefaultPii: boolean = false, | ||||||
| ): Record<string, string> { | ||||||
| const spanAttributes: Record<string, string> = {}; | ||||||
|
|
||||||
|
|
@@ -161,16 +171,26 @@ export function httpHeadersToSpanAttributes( | |||||
| return; | ||||||
| } | ||||||
|
|
||||||
| const lowerCasedKey = key.toLowerCase(); | ||||||
| const isSensitive = SENSITIVE_HEADER_SNIPPETS.some(snippet => lowerCasedKey.includes(snippet)); | ||||||
| const normalizedKey = `http.request.header.${lowerCasedKey.replace(/-/g, '_')}`; | ||||||
| const lowerCasedHeaderKey = key.toLowerCase(); | ||||||
| const isCookieHeader = lowerCasedHeaderKey === 'cookie' || lowerCasedHeaderKey === 'set-cookie'; | ||||||
|
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. l: probably saves us a few bytes:
Suggested change
|
||||||
|
|
||||||
| if (isCookieHeader && typeof value === 'string' && value !== '') { | ||||||
|
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. q: do we handle arrays of cookie headers? (or is this not relevant for cookie/set-cookie?) |
||||||
| const cookies = value.split('; '); | ||||||
|
|
||||||
| for (const cookie of cookies) { | ||||||
| // Split only at the first '=' to preserve '=' characters in cookie values | ||||||
| const equalSignIndex = cookie.indexOf('='); | ||||||
| const cookieKey = equalSignIndex !== -1 ? cookie.substring(0, equalSignIndex) : cookie; | ||||||
| const cookieValue = equalSignIndex !== -1 ? cookie.substring(equalSignIndex + 1) : ''; | ||||||
|
|
||||||
| const lowerCasedCookieKey = String(cookieKey).toLowerCase(); | ||||||
| const normalizedKey = `http.request.header.${normalizeAttributeKey(lowerCasedHeaderKey)}.${normalizeAttributeKey(lowerCasedCookieKey)}`; | ||||||
|
|
||||||
| if (isSensitive) { | ||||||
| spanAttributes[normalizedKey] = '[Filtered]'; | ||||||
| } else if (Array.isArray(value)) { | ||||||
| spanAttributes[normalizedKey] = value.map(v => (v != null ? String(v) : v)).join(';'); | ||||||
| } else if (typeof value === 'string') { | ||||||
| spanAttributes[normalizedKey] = value; | ||||||
| spanAttributes[normalizedKey] = handleHttpHeader(lowerCasedCookieKey, cookieValue, sendDefaultPii); | ||||||
| } | ||||||
|
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. Bug: Set-Cookie attributes incorrectly parsed as separate cookiesThe cookie parsing logic treats |
||||||
| } else { | ||||||
| const normalizedKey = `http.request.header.${normalizeAttributeKey(lowerCasedHeaderKey)}`; | ||||||
| spanAttributes[normalizedKey] = handleHttpHeader(lowerCasedHeaderKey, value, sendDefaultPii); | ||||||
| } | ||||||
| }); | ||||||
| } catch { | ||||||
|
|
@@ -180,6 +200,26 @@ export function httpHeadersToSpanAttributes( | |||||
| return spanAttributes; | ||||||
| } | ||||||
This comment was marked as outdated.
Sorry, something went wrong. |
||||||
|
|
||||||
| function normalizeAttributeKey(key: string): string { | ||||||
| return key.replace(/-/g, '_'); | ||||||
| } | ||||||
|
|
||||||
| function handleHttpHeader(lowerCasedKey: string, value: string | string[] | undefined, sendPii: boolean): string { | ||||||
| const isSensitive = sendPii | ||||||
| ? SENSITIVE_HEADER_SNIPPETS.some(snippet => lowerCasedKey.includes(snippet)) | ||||||
| : [...PII_HEADER_SNIPPETS, ...SENSITIVE_HEADER_SNIPPETS].some(snippet => lowerCasedKey.includes(snippet)); | ||||||
|
|
||||||
| if (isSensitive) { | ||||||
| return '[Filtered]'; | ||||||
| } else if (Array.isArray(value)) { | ||||||
| return value.map(v => (v != null ? String(v) : v)).join(';'); | ||||||
| } else if (typeof value === 'string') { | ||||||
| return value; | ||||||
| } | ||||||
|
|
||||||
| return ''; // Fallback for unexpected types | ||||||
| } | ||||||
|
|
||||||
| /** Extract the query params from an URL. */ | ||||||
| export function extractQueryParamsFromUrl(url: string): string | undefined { | ||||||
| // url is path and query string | ||||||
|
|
||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -612,7 +612,7 @@ describe('request utils', () => { | |
| }); | ||
| }); | ||
|
|
||
| describe('PII filtering', () => { | ||
| describe('PII/Sensitive data filtering', () => { | ||
| it('filters sensitive headers case-insensitively', () => { | ||
| const headers = { | ||
| AUTHORIZATION: 'Bearer secret-token', | ||
|
|
@@ -625,12 +625,101 @@ describe('request utils', () => { | |
|
|
||
| expect(result).toEqual({ | ||
| 'http.request.header.content_type': 'application/json', | ||
| 'http.request.header.cookie': '[Filtered]', | ||
| 'http.request.header.cookie.session': '[Filtered]', | ||
| 'http.request.header.x_api_key': '[Filtered]', | ||
| 'http.request.header.authorization': '[Filtered]', | ||
| }); | ||
| }); | ||
|
|
||
| it('attaches and filters sensitive cookie headers', () => { | ||
| const headers = { | ||
| Cookie: | ||
| 'session=abc123; tracking=enabled; cookie-authentication-key-without-value; theme=dark; lang=en; user_session=xyz789; pref=1', | ||
| }; | ||
|
|
||
| const result = httpHeadersToSpanAttributes(headers); | ||
|
|
||
| expect(result).toEqual({ | ||
| 'http.request.header.cookie.session': '[Filtered]', | ||
| 'http.request.header.cookie.tracking': 'enabled', | ||
| 'http.request.header.cookie.theme': 'dark', | ||
| 'http.request.header.cookie.lang': 'en', | ||
| 'http.request.header.cookie.user_session': '[Filtered]', | ||
| 'http.request.header.cookie.cookie_authentication_key_without_value': '[Filtered]', | ||
| 'http.request.header.cookie.pref': '1', | ||
| }); | ||
| }); | ||
|
|
||
| it('adds a filtered cookie header when cookie header is present, but has no valid key=value pairs', () => { | ||
| const headers1 = { Cookie: ['key', 'val'] }; | ||
| const result1 = httpHeadersToSpanAttributes(headers1); | ||
| expect(result1).toEqual({ 'http.request.header.cookie': '[Filtered]' }); | ||
|
|
||
| const headers3 = { Cookie: '' }; | ||
| const result3 = httpHeadersToSpanAttributes(headers3); | ||
| expect(result3).toEqual({ 'http.request.header.cookie': '[Filtered]' }); | ||
| }); | ||
|
|
||
| it('attaches and filters sensitive a set-cookie header', () => { | ||
| const headers1 = { 'Set-Cookie': 'user_session=def456' }; | ||
|
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. l: let's add or adjust a test here for a set-cookie header with additional properties (e.g. like max-age) |
||
| const result1 = httpHeadersToSpanAttributes(headers1); | ||
| expect(result1).toEqual({ 'http.request.header.set_cookie.user_session': '[Filtered]' }); | ||
|
|
||
| const headers2 = { 'Set-Cookie': 'preferred-color-mode=light' }; | ||
| const result2 = httpHeadersToSpanAttributes(headers2); | ||
| expect(result2).toEqual({ 'http.request.header.set_cookie.preferred_color_mode': 'light' }); | ||
|
|
||
| const headers3 = { 'Set-Cookie': 'lang=en' }; | ||
| const result3 = httpHeadersToSpanAttributes(headers3); | ||
| expect(result3).toEqual({ 'http.request.header.set_cookie.lang': 'en' }); | ||
|
|
||
| const headers4 = { 'Set-Cookie': 'timezone=UTC' }; | ||
| const result4 = httpHeadersToSpanAttributes(headers4); | ||
| expect(result4).toEqual({ 'http.request.header.set_cookie.timezone': 'UTC' }); | ||
| }); | ||
|
|
||
| it('only splits cookies once between key and value, even when more equals signs are present', () => { | ||
| const headers = { Cookie: 'random-string=eyJhbGc=.eyJzdWI=.SflKxw' }; | ||
| const result = httpHeadersToSpanAttributes(headers); | ||
| expect(result).toEqual({ 'http.request.header.cookie.random_string': 'eyJhbGc=.eyJzdWI=.SflKxw' }); | ||
| }); | ||
|
|
||
| it.each([ | ||
| { sendDefaultPii: false, description: 'sendDefaultPii is false (default)' }, | ||
| { sendDefaultPii: true, description: 'sendDefaultPii is true' }, | ||
| ])('does not include PII headers when $description', ({ sendDefaultPii }) => { | ||
| const headers = { | ||
| 'Content-Type': 'application/json', | ||
| 'User-Agent': 'Mozilla/5.0', | ||
| 'x-user': 'my-personal-username', | ||
| 'X-Forwarded-For': '192.168.1.1', | ||
| 'X-Forwarded-Host': 'example.com', | ||
| 'X-Forwarded-Proto': 'https', | ||
| }; | ||
|
|
||
| const result = httpHeadersToSpanAttributes(headers, sendDefaultPii); | ||
|
|
||
| if (sendDefaultPii) { | ||
| expect(result).toEqual({ | ||
| 'http.request.header.content_type': 'application/json', | ||
| 'http.request.header.user_agent': 'Mozilla/5.0', | ||
| 'http.request.header.x_user': 'my-personal-username', | ||
| 'http.request.header.x_forwarded_for': '192.168.1.1', | ||
| 'http.request.header.x_forwarded_host': 'example.com', | ||
| 'http.request.header.x_forwarded_proto': 'https', | ||
| }); | ||
| } else { | ||
| expect(result).toEqual({ | ||
| 'http.request.header.content_type': 'application/json', | ||
| 'http.request.header.user_agent': 'Mozilla/5.0', | ||
| 'http.request.header.x_user': '[Filtered]', | ||
| 'http.request.header.x_forwarded_for': '[Filtered]', | ||
| 'http.request.header.x_forwarded_host': '[Filtered]', | ||
| 'http.request.header.x_forwarded_proto': '[Filtered]', | ||
| }); | ||
| } | ||
| }); | ||
|
|
||
| it('always filters comprehensive list of sensitive headers', () => { | ||
| const headers = { | ||
| 'Content-Type': 'application/json', | ||
|
|
@@ -671,8 +760,8 @@ describe('request utils', () => { | |
| 'http.request.header.accept': 'application/json', | ||
| 'http.request.header.host': 'example.com', | ||
| 'http.request.header.authorization': '[Filtered]', | ||
| 'http.request.header.cookie': '[Filtered]', | ||
| 'http.request.header.set_cookie': '[Filtered]', | ||
| 'http.request.header.cookie.session': '[Filtered]', | ||
| 'http.request.header.set_cookie.session': '[Filtered]', | ||
| 'http.request.header.x_api_key': '[Filtered]', | ||
| 'http.request.header.x_auth_token': '[Filtered]', | ||
| 'http.request.header.x_secret': '[Filtered]', | ||
|
|
||
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.
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 likeX-CSRFor a cookie namedcsrf-tokenwould not be filtered because'x-csrf'.includes('crsf')returns false. The existing test passes only becauseX-CSRF-Tokenalso containstoken, which masks this bug.