Skip to content

ref(core): Do not check baggage validity #14479

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

Merged
merged 2 commits into from
Nov 27, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 1 addition & 28 deletions packages/core/src/utils/traceData.ts
Original file line number Diff line number Diff line change
Expand Up @@ -44,39 +44,12 @@ export function getTraceData(options: { span?: Span } = {}): SerializedTraceData
return {};
}

const validBaggage = isValidBaggageString(baggage);
if (!validBaggage) {
logger.warn('Invalid baggage data. Not returning "baggage" value');
}

return {
'sentry-trace': sentryTrace,
...(validBaggage && { baggage }),
baggage,
};
}

/**
* Tests string against baggage spec as defined in:
*
* - W3C Baggage grammar: https://www.w3.org/TR/baggage/#definition
* - RFC7230 token definition: https://datatracker.ietf.org/doc/html/rfc7230#section-3.2.6
*
* exported for testing
*/
export function isValidBaggageString(baggage?: string): boolean {
if (!baggage || !baggage.length) {
return false;
}
const keyRegex = "[-!#$%&'*+.^_`|~A-Za-z0-9]+";
const valueRegex = '[!#-+-./0-9:<=>?@A-Z\\[\\]a-z{-}]+';
const spaces = '\\s*';
// eslint-disable-next-line @sentry-internal/sdk/no-regexp-constructor -- RegExp for readability, no user input
const baggageRegex = new RegExp(
`^${keyRegex}${spaces}=${spaces}${valueRegex}(${spaces},${spaces}${keyRegex}${spaces}=${spaces}${valueRegex})*$`,
);
return baggageRegex.test(baggage);
}

/**
* Get a sentry-trace header value for the given scope.
*/
Expand Down
73 changes: 0 additions & 73 deletions packages/core/test/lib/utils/traceData.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,6 @@ import {
import { getAsyncContextStrategy } from '../../../src/asyncContext';
import { freezeDscOnSpan } from '../../../src/tracing/dynamicSamplingContext';

import { isValidBaggageString } from '../../../src/utils/traceData';
import type { TestClientOptions } from '../../mocks/client';
import { TestClient, getDefaultTestClientOptions } from '../../mocks/client';

Expand Down Expand Up @@ -281,75 +280,3 @@ describe('getTraceData', () => {
expect(traceData).toEqual({});
});
});

describe('isValidBaggageString', () => {
it.each([
'sentry-environment=production',
'sentry-environment=staging,sentry-public_key=key,sentry-trace_id=abc',
// @ is allowed in values
'[email protected]',
// spaces are allowed around the delimiters
'sentry-environment=staging , sentry-public_key=key ,[email protected]',
'sentry-environment=staging , thirdparty=value ,[email protected]',
// these characters are explicitly allowed for keys in the baggage spec:
"!#$%&'*+-.^_`|~1234567890abcxyzABCXYZ=true",
// special characters in values are fine (except for ",;\ - see other test)
'key=(value)',
'key=[{(value)}]',
'key=some$value',
'key=more#value',
'key=max&value',
'key=max:value',
'key=x=value',
])('returns true if the baggage string is valid (%s)', baggageString => {
expect(isValidBaggageString(baggageString)).toBe(true);
});

it.each([
// baggage spec doesn't permit leading spaces
' sentry-environment=production,sentry-publickey=key,sentry-trace_id=abc',
// no spaces in keys or values
'sentry-public key=key',
'sentry-publickey=my key',
// no delimiters ("(),/:;<=>?@[\]{}") in keys
'asdf(x=value',
'asdf)x=value',
'asdf,x=value',
'asdf/x=value',
'asdf:x=value',
'asdf;x=value',
'asdf<x=value',
'asdf>x=value',
'asdf?x=value',
'asdf@x=value',
'asdf[x=value',
'asdf]x=value',
'asdf\\x=value',
'asdf{x=value',
'asdf}x=value',
// no ,;\" in values
'key=va,lue',
'key=va;lue',
'key=va\\lue',
'key=va"lue"',
// baggage headers can have properties but we currently don't support them
'sentry-environment=production;prop1=foo;prop2=bar,nextkey=value',
// no fishy stuff
'absolutely not a valid baggage string',
'val"/><script>alert("xss")</script>',
'something"/>',
'<script>alert("xss")</script>',
'/>',
'" onblur="alert("xss")',
])('returns false if the baggage string is invalid (%s)', baggageString => {
expect(isValidBaggageString(baggageString)).toBe(false);
});

it('returns false if the baggage string is empty', () => {
expect(isValidBaggageString('')).toBe(false);
});

it('returns false if the baggage string is empty', () => {
expect(isValidBaggageString(undefined)).toBe(false);
});
});
Loading