Skip to content

Commit

Permalink
ref(core): Do not check baggage validity (#14479)
Browse files Browse the repository at this point in the history
This PR drops the validation for baggage content. We didn't do this for
browser previously, only for node, but it adds bundle size and does not
appear too important.

I left the trace header validation in for now, we may also drop this but
it is smaller and I guess also more important to us...?
  • Loading branch information
mydea authored Nov 27, 2024
1 parent de65590 commit 1e0cb04
Show file tree
Hide file tree
Showing 2 changed files with 1 addition and 101 deletions.
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);
});
});

0 comments on commit 1e0cb04

Please sign in to comment.