From 7a1bc934bea326f0bc8cfc7fdfb74fff2a21d2ef Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Mon, 27 Jun 2022 11:09:52 +0200 Subject: [PATCH 1/7] ignore 3rd party entries baggage in incoming requests --- packages/nextjs/src/utils/withSentry.ts | 2 +- packages/node/src/handlers.ts | 2 +- packages/serverless/src/awslambda.ts | 2 +- packages/serverless/src/gcpfunction/http.ts | 2 +- packages/tracing/src/transaction.ts | 22 ++------ packages/utils/src/baggage.ts | 33 ++++++++---- packages/utils/test/baggage.test.ts | 56 +++++++++++++-------- 7 files changed, 65 insertions(+), 54 deletions(-) diff --git a/packages/nextjs/src/utils/withSentry.ts b/packages/nextjs/src/utils/withSentry.ts index 1684b8323215..714a2271ceae 100644 --- a/packages/nextjs/src/utils/withSentry.ts +++ b/packages/nextjs/src/utils/withSentry.ts @@ -72,7 +72,7 @@ export const withSentry = (origHandler: NextApiHandler): WrappedNextApiHandler = name: `${reqMethod}${reqPath}`, op: 'http.server', ...traceparentData, - metadata: { baggage: baggage }, + metadata: { baggage }, }, // extra context passed to the `tracesSampler` { request: req }, diff --git a/packages/node/src/handlers.ts b/packages/node/src/handlers.ts index 95158d28390e..417b60c4795f 100644 --- a/packages/node/src/handlers.ts +++ b/packages/node/src/handlers.ts @@ -45,7 +45,7 @@ export function tracingHandler(): ( name: extractPathForTransaction(req, { path: true, method: true }), op: 'http.server', ...traceparentData, - metadata: { baggage: baggage }, + metadata: { baggage }, }, // extra context passed to the tracesSampler { request: extractRequestData(req) }, diff --git a/packages/serverless/src/awslambda.ts b/packages/serverless/src/awslambda.ts index c7c29f7efb2a..fe785e061b9e 100644 --- a/packages/serverless/src/awslambda.ts +++ b/packages/serverless/src/awslambda.ts @@ -289,7 +289,7 @@ export function wrapHandler( name: context.functionName, op: 'awslambda.handler', ...traceparentData, - metadata: { baggage: baggage }, + metadata: { baggage }, }); const hub = getCurrentHub(); diff --git a/packages/serverless/src/gcpfunction/http.ts b/packages/serverless/src/gcpfunction/http.ts index 84551170c4de..07728f54f0f6 100644 --- a/packages/serverless/src/gcpfunction/http.ts +++ b/packages/serverless/src/gcpfunction/http.ts @@ -87,7 +87,7 @@ function _wrapHttpFunction(fn: HttpFunction, wrapOptions: Partial entry.trim()) - .filter(entry => entry !== ''); + .filter(entry => entry !== '' && (includeThirdPartyEntries || SENTRY_BAGGAGE_KEY_PREFIX_REGEX.test(entry))); return baggageEntries.reduce( ([baggageObj, baggageString], curr) => { @@ -152,13 +161,10 @@ export function mergeAndSerializeBaggage(incomingBaggage?: Baggage, thirdPartyBa return ''; } - const headerBaggage = (thirdPartyBaggageHeader && parseBaggageHeader(thirdPartyBaggageHeader)) || undefined; + const headerBaggage = (thirdPartyBaggageHeader && parseBaggageHeader(thirdPartyBaggageHeader, true)) || undefined; const thirdPartyHeaderBaggage = headerBaggage && getThirdPartyBaggage(headerBaggage); - const finalBaggage = createBaggage( - (incomingBaggage && incomingBaggage[0]) || {}, - thirdPartyHeaderBaggage || (incomingBaggage && incomingBaggage[1]) || '', - ); + const finalBaggage = createBaggage((incomingBaggage && incomingBaggage[0]) || {}, thirdPartyHeaderBaggage || ''); return serializeBaggage(finalBaggage); } @@ -179,8 +185,13 @@ export function parseBaggageSetMutability( sentryTraceHeader: TraceparentData | string | false | undefined | null, ): Baggage { const baggage = parseBaggageHeader(rawBaggageValue || ''); - if (!isSentryBaggageEmpty(baggage) || (sentryTraceHeader && isSentryBaggageEmpty(baggage))) { - setBaggageImmutable(baggage); - } + + // Because we are always creating a Baggage object by calling `parseBaggageHeader` above + // (either a filled one or an empty one, even if we didn't get a `baggage` header), + // we only need to check if we have a sentry-trace header or not. As soon as we have it, + // we set baggage immutable. In case we don't get a sentry-trace header, we can assume that + // this SDK is the head of the trace and thus we still permit mutation at this time. + sentryTraceHeader && setBaggageImmutable(baggage); + return baggage; } diff --git a/packages/utils/test/baggage.test.ts b/packages/utils/test/baggage.test.ts index 3160cc21487c..5faee8af2e1f 100644 --- a/packages/utils/test/baggage.test.ts +++ b/packages/utils/test/baggage.test.ts @@ -99,24 +99,33 @@ describe('Baggage', () => { describe('parseBaggageHeader', () => { it.each([ - ['parses an empty string', '', createBaggage({})], - ['parses a blank string', ' ', createBaggage({})], + ['parses an empty string', '', undefined, createBaggage({})], + ['parses a blank string', ' ', undefined, createBaggage({})], [ 'parses sentry values into baggage', 'sentry-environment=production,sentry-release=10.0.2', + undefined, createBaggage({ environment: 'production', release: '10.0.2' }), ], [ - 'parses arbitrary baggage headers', + 'ignores 3rd party entries by default', 'userId=alice,serverNode=DF%2028,isProduction=false,sentry-environment=production,sentry-release=10.0.2', + undefined, + createBaggage({ environment: 'production', release: '10.0.2' }, ''), + ], + [ + 'parses sentry- and arbitrary 3rd party values if the 3rd party entries flag is set to true', + 'userId=alice,serverNode=DF%2028,isProduction=false,sentry-environment=production,sentry-release=10.0.2', + true, createBaggage( { environment: 'production', release: '10.0.2' }, 'userId=alice,serverNode=DF%2028,isProduction=false', ), ], [ - 'parses arbitrary baggage headers from string with empty and blank entries', + 'parses arbitrary baggage entries from string with empty and blank entries', 'userId=alice, serverNode=DF%2028 , isProduction=false, ,,sentry-environment=production,,sentry-release=10.0.2', + true, createBaggage( { environment: 'production', release: '10.0.2' }, 'userId=alice,serverNode=DF%2028,isProduction=false', @@ -125,21 +134,24 @@ describe('Baggage', () => { [ 'parses a string array', ['userId=alice', 'sentry-environment=production', 'foo=bar'], + true, createBaggage({ environment: 'production' }, 'userId=alice,foo=bar'), ], [ 'parses a string array with items containing multiple entries', ['userId=alice, userName=bob', 'sentry-environment=production,sentry-release=1.0.1', 'foo=bar'], + true, createBaggage({ environment: 'production', release: '1.0.1' }, 'userId=alice,userName=bob,foo=bar'), ], [ 'parses a string array with empty/blank entries', ['', 'sentry-environment=production,sentry-release=1.0.1', ' ', 'foo=bar'], + true, createBaggage({ environment: 'production', release: '1.0.1' }, 'foo=bar'), ], - ['ignorese other input types than string and string[]', 42, createBaggage({}, '')], - ])('%s', (_: string, baggageValue, baggage) => { - expect(parseBaggageHeader(baggageValue)).toEqual(baggage); + ['ignorese other input types than string and string[]', 42, undefined, createBaggage({}, '')], + ])('%s', (_: string, baggageValue, includeThirPartyEntries, expectedBaggage) => { + expect(parseBaggageHeader(baggageValue, includeThirPartyEntries)).toEqual(expectedBaggage); }); }); @@ -166,18 +178,24 @@ describe('Baggage', () => { describe('mergeAndSerializeBaggage', () => { it.each([ [ - 'returns original baggage when there is no additional baggage', - createBaggage({ release: '1.1.1', userid: '1234' }, 'foo=bar'), + 'returns original baggage when there is no additional baggage header', + createBaggage({ release: '1.1.1', user_id: '1234' }), undefined, - 'foo=bar,sentry-release=1.1.1,sentry-userid=1234', + 'sentry-release=1.1.1,sentry-user_id=1234', ], [ 'returns merged baggage when there is a 3rd party header added', - createBaggage({ release: '1.1.1', userid: '1234' }, 'foo=bar'), + createBaggage({ release: '1.1.1', user_id: '1234' }, 'foo=bar'), 'bar=baz,key=value', - 'bar=baz,key=value,sentry-release=1.1.1,sentry-userid=1234', + 'bar=baz,key=value,sentry-release=1.1.1,sentry-user_id=1234', ], ['returns merged baggage original baggage is empty', createBaggage({}), 'bar=baz,key=value', 'bar=baz,key=value'], + [ + 'ignores sentry- items in 3rd party baggage header', + createBaggage({}), + 'bar=baz,sentry-user_id=abc,key=value,sentry-sample_rate=0.76', + 'bar=baz,key=value', + ], ['returns empty string when original and 3rd party baggage are empty', createBaggage({}), '', ''], ['returns merged baggage original baggage is undefined', undefined, 'bar=baz,key=value', 'bar=baz,key=value'], ['returns empty string when both params are undefined', undefined, undefined, ''], @@ -207,22 +225,16 @@ describe('Baggage', () => { [{}, '', false] as Baggage, ], [ - 'returns a non-empty, mutable baggage object if sentry-trace is not defined and only 3rd party baggage items are passed', + 'returns a non-empty, mutable baggage object if sentry-trace is not defined and ignores 3rd party baggage items', 'foo=bar', undefined, - [{}, 'foo=bar', true] as Baggage, - ], - [ - 'returns a non-empty, immutable baggage object if sentry-trace is not defined and Sentry baggage items are passed', - 'sentry-environment=production,foo=bar', - undefined, - [{ environment: 'production' }, 'foo=bar', false] as Baggage, + [{}, '', true] as Baggage, ], [ 'returns a non-empty, immutable baggage object if sentry-trace is defined', - 'foo=bar', + 'foo=bar,sentry-environment=production,sentry-sample_rate=0.96', {}, - [{}, 'foo=bar', false] as Baggage, + [{ environment: 'production', sample_rate: '0.96' }, '', false] as Baggage, ], ])( '%s', From 9359d3fa62b53161db03c000734f775165c7c4ba Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Mon, 27 Jun 2022 13:02:50 +0200 Subject: [PATCH 2/7] fix (integration) tests --- packages/node-integration-tests/package.json | 1 - .../baggage-header-assign/test.ts | 20 ++++++++++--------- packages/node/test/handlers.test.ts | 5 +++-- .../test/browser/browsertracing.test.ts | 10 +++++----- packages/tracing/test/span.test.ts | 4 ++-- 5 files changed, 21 insertions(+), 19 deletions(-) diff --git a/packages/node-integration-tests/package.json b/packages/node-integration-tests/package.json index 4b85157ff50e..b61a72949991 100644 --- a/packages/node-integration-tests/package.json +++ b/packages/node-integration-tests/package.json @@ -13,7 +13,6 @@ "lint:eslint": "eslint . --cache --cache-location '../../eslintcache/' --format stylish", "lint:prettier": "prettier --check \"{suites,utils}/**/*.ts\"", "type-check": "tsc", - "pretest": "run-s --silent prisma:init", "test": "jest --runInBand --forceExit", "test:watch": "yarn test --watch" }, diff --git a/packages/node-integration-tests/suites/express/sentry-trace/baggage-header-assign/test.ts b/packages/node-integration-tests/suites/express/sentry-trace/baggage-header-assign/test.ts index aa015e05529e..00c64ad8c9ba 100644 --- a/packages/node-integration-tests/suites/express/sentry-trace/baggage-header-assign/test.ts +++ b/packages/node-integration-tests/suites/express/sentry-trace/baggage-header-assign/test.ts @@ -7,30 +7,32 @@ test('Should not overwrite baggage if the incoming request already has Sentry ba const url = await runServer(__dirname, `${path.resolve(__dirname, '..')}/server.ts`); const response = (await getAPIResponse(new URL(`${url}/express`), { - baggage: 'sentry-version=2.0.0,sentry-environment=myEnv', + 'sentry-trace': '', + baggage: 'sentry-release=2.0.0,sentry-environment=myEnv', })) as TestAPIResponse; expect(response).toBeDefined(); expect(response).toMatchObject({ test_data: { host: 'somewhere.not.sentry', - baggage: 'sentry-version=2.0.0,sentry-environment=myEnv', + baggage: 'sentry-release=2.0.0,sentry-environment=myEnv', }, }); }); -test('Should pass along sentry and 3rd party trace baggage data from an incoming to an outgoing request.', async () => { +test('Should propagate sentry trace baggage data from an incoming to an outgoing request.', async () => { const url = await runServer(__dirname, `${path.resolve(__dirname, '..')}/server.ts`); const response = (await getAPIResponse(new URL(`${url}/express`), { - baggage: 'sentry-version=2.0.0,sentry-environment=myEnv,dogs=great', + 'sentry-trace': '', + baggage: 'sentry-release=2.0.0,sentry-environment=myEnv,dogs=great', })) as TestAPIResponse; expect(response).toBeDefined(); expect(response).toMatchObject({ test_data: { host: 'somewhere.not.sentry', - baggage: expect.stringContaining('dogs=great,sentry-version=2.0.0,sentry-environment=myEnv'), + baggage: 'sentry-release=2.0.0,sentry-environment=myEnv', }, }); }); @@ -51,7 +53,7 @@ test('Should propagate empty baggage if sentry-trace header is present in incomi }); }); -test('Should propagate empty sentry and original 3rd party baggage if sentry-trace header is present', async () => { +test('Should propagate empty sentry and ignore original 3rd party baggage entries if sentry-trace header is present', async () => { const url = await runServer(__dirname, `${path.resolve(__dirname, '..')}/server.ts`); const response = (await getAPIResponse(new URL(`${url}/express`), { @@ -63,7 +65,7 @@ test('Should propagate empty sentry and original 3rd party baggage if sentry-tra expect(response).toMatchObject({ test_data: { host: 'somewhere.not.sentry', - baggage: 'foo=bar', + baggage: '', }, }); }); @@ -85,7 +87,7 @@ test('Should populate and propagate sentry baggage if sentry-trace header does n }); }); -test('Should populate Sentry and propagate 3rd party content if sentry-trace header does not exist', async () => { +test('Should populate Sentry and ignore 3rd party content if sentry-trace header does not exist', async () => { const url = await runServer(__dirname, `${path.resolve(__dirname, '..')}/server.ts`); const response = (await getAPIResponse(new URL(`${url}/express`), { @@ -98,7 +100,7 @@ test('Should populate Sentry and propagate 3rd party content if sentry-trace hea host: 'somewhere.not.sentry', // TraceId changes, hence we only expect that the string contains the traceid key baggage: expect.stringContaining( - 'foo=bar,bar=baz,sentry-environment=prod,sentry-release=1.0,sentry-transaction=GET%20%2Ftest%2Fexpress,' + + 'sentry-environment=prod,sentry-release=1.0,sentry-transaction=GET%20%2Ftest%2Fexpress,' + 'sentry-public_key=public,sentry-trace_id=', ), }, diff --git a/packages/node/test/handlers.test.ts b/packages/node/test/handlers.test.ts index 9263e1082871..7047211d25ff 100644 --- a/packages/node/test/handlers.test.ts +++ b/packages/node/test/handlers.test.ts @@ -219,8 +219,9 @@ describe('tracingHandler', () => { ] as Baggage); }); - it("pulls parent's baggage (sentry + third party entries) headers on the request", () => { + it('ignores 3rd party entries in incoming baggage header', () => { req.headers = { + 'sentry-trace': '12312012123120121231201212312012-1121201211212012-0', baggage: 'sentry-version=1.0,sentry-environment=production,dogs=great,cats=boring', }; @@ -231,7 +232,7 @@ describe('tracingHandler', () => { expect(transaction.metadata?.baggage).toBeDefined(); expect(transaction.metadata?.baggage).toEqual([ { version: '1.0', environment: 'production' }, - 'dogs=great,cats=boring', + '', false, ] as Baggage); }); diff --git a/packages/tracing/test/browser/browsertracing.test.ts b/packages/tracing/test/browser/browsertracing.test.ts index 4655e14297b6..e881105450ef 100644 --- a/packages/tracing/test/browser/browsertracing.test.ts +++ b/packages/tracing/test/browser/browsertracing.test.ts @@ -234,7 +234,7 @@ describe('BrowserTracing', () => { parentSpanId: 'b6e54397b12a2a0f', parentSampled: true, metadata: { - baggage: [{ release: '2.1.14' }, 'foo=bar', false], + baggage: [{ release: '2.1.14' }, '', false], }, }), expect.any(Number), @@ -375,7 +375,7 @@ describe('BrowserTracing', () => { expect(headerContext!.parentSampled).toEqual(false); }); - it('correctly parses a valid baggage meta header', () => { + it('correctly parses a valid baggage meta header and ignored 3rd party entries', () => { document.head.innerHTML = ''; const headerContext = extractTraceDataFromMetaTags(); @@ -388,7 +388,7 @@ describe('BrowserTracing', () => { release: '2.1.12', } as BaggageObj); expect(baggage && baggage[1]).toBeDefined(); - expect(baggage && baggage[1]).toEqual('foo=bar'); + expect(baggage && baggage[1]).toEqual(''); }); it('returns undefined if the sentry-trace header is malformed', () => { @@ -461,7 +461,7 @@ describe('BrowserTracing', () => { expect(baggage[0]).toBeDefined(); expect(baggage[0]).toEqual({ release: '2.1.14' }); expect(baggage[1]).toBeDefined(); - expect(baggage[1]).toEqual('foo=bar'); + expect(baggage[1]).toEqual(''); }); it('does not add Sentry baggage data to pageload transactions if sentry-trace data is present but passes on 3rd party baggage', () => { @@ -483,7 +483,7 @@ describe('BrowserTracing', () => { expect(baggage).toBeDefined(); expect(isSentryBaggageEmpty(baggage)).toBe(true); expect(baggage[1]).toBeDefined(); - expect(baggage[1]).toEqual('foo=bar'); + expect(baggage[1]).toEqual(''); }); it('ignores the data for navigation transactions', () => { diff --git a/packages/tracing/test/span.test.ts b/packages/tracing/test/span.test.ts index a0b08d328997..b1615b2ea62e 100644 --- a/packages/tracing/test/span.test.ts +++ b/packages/tracing/test/span.test.ts @@ -403,11 +403,11 @@ describe('Span', () => { }; }); - test('leave baggage content untouched and just return baggage if there already is Sentry content in it', () => { + test('leave baggage content untouched and just return baggage if it is immutable', () => { const transaction = new Transaction( { name: 'tx', - metadata: { baggage: createBaggage({ environment: 'myEnv' }, '') }, + metadata: { baggage: createBaggage({ environment: 'myEnv' }, '', false) }, }, hub, ); From e9d1c2ade2e3cc23008a62c91e80fbd71aa887b3 Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Mon, 27 Jun 2022 13:09:08 +0200 Subject: [PATCH 3/7] remove isBaggageEmpty helper function was not used anymore (except in tests) --- packages/node/test/handlers.test.ts | 5 +++-- packages/tracing/test/browser/browsertracing.test.ts | 8 +++++--- packages/utils/src/baggage.ts | 6 ------ packages/utils/test/baggage.test.ts | 12 ------------ 4 files changed, 8 insertions(+), 23 deletions(-) diff --git a/packages/node/test/handlers.test.ts b/packages/node/test/handlers.test.ts index 7047211d25ff..e06c1846d0c2 100644 --- a/packages/node/test/handlers.test.ts +++ b/packages/node/test/handlers.test.ts @@ -3,7 +3,7 @@ import * as sentryHub from '@sentry/hub'; import { Hub } from '@sentry/hub'; import { Transaction } from '@sentry/tracing'; import { Baggage, Event } from '@sentry/types'; -import { isBaggageEmpty, isBaggageMutable, SentryError } from '@sentry/utils'; +import { getThirdPartyBaggage, isBaggageMutable, isSentryBaggageEmpty, SentryError } from '@sentry/utils'; import * as http from 'http'; import { NodeClient } from '../src/client'; @@ -193,7 +193,8 @@ describe('tracingHandler', () => { expect(transaction.parentSpanId).toEqual('1121201211212012'); expect(transaction.sampled).toEqual(false); expect(transaction.metadata?.baggage).toBeDefined(); - expect(isBaggageEmpty(transaction.metadata?.baggage)).toBe(true); + expect(isSentryBaggageEmpty(transaction.metadata?.baggage)).toBe(true); + expect(getThirdPartyBaggage(transaction.metadata?.baggage)).toEqual(''); expect(isBaggageMutable(transaction.metadata?.baggage)).toBe(false); }); diff --git a/packages/tracing/test/browser/browsertracing.test.ts b/packages/tracing/test/browser/browsertracing.test.ts index e881105450ef..aab9d59c38b8 100644 --- a/packages/tracing/test/browser/browsertracing.test.ts +++ b/packages/tracing/test/browser/browsertracing.test.ts @@ -3,9 +3,9 @@ import { Hub, makeMain } from '@sentry/hub'; import type { Baggage, BaggageObj, BaseTransportOptions, ClientOptions, DsnComponents } from '@sentry/types'; import { getGlobalObject, + getThirdPartyBaggage, InstrumentHandlerCallback, InstrumentHandlerType, - isBaggageEmpty, isSentryBaggageEmpty, } from '@sentry/utils'; import { JSDOM } from 'jsdom'; @@ -398,7 +398,8 @@ describe('BrowserTracing', () => { expect(headerContext).toBeDefined(); expect(headerContext?.metadata?.baggage).toBeDefined(); - expect(isBaggageEmpty(headerContext?.metadata?.baggage as Baggage)).toBe(true); + expect(isSentryBaggageEmpty(headerContext?.metadata?.baggage as Baggage)).toBe(true); + expect(getThirdPartyBaggage(headerContext?.metadata?.baggage as Baggage)).toEqual(''); }); it('does not crash if the baggage header is malformed', () => { @@ -421,7 +422,8 @@ describe('BrowserTracing', () => { expect(headerContext).toBeDefined(); expect(headerContext?.metadata?.baggage).toBeDefined(); - expect(isBaggageEmpty(headerContext?.metadata?.baggage as Baggage)).toBe(true); + expect(isSentryBaggageEmpty(headerContext?.metadata?.baggage as Baggage)).toBe(true); + expect(getThirdPartyBaggage(headerContext?.metadata?.baggage as Baggage)).toEqual(''); }); }); diff --git a/packages/utils/src/baggage.ts b/packages/utils/src/baggage.ts index 05c1028f7bc5..11436c9d9675 100644 --- a/packages/utils/src/baggage.ts +++ b/packages/utils/src/baggage.ts @@ -38,12 +38,6 @@ export function isSentryBaggageEmpty(baggage: Baggage): boolean { return Object.keys(baggage[0]).length === 0; } -/** Check if the Sentry part of the passed baggage (i.e. the first element in the tuple) is empty */ -export function isBaggageEmpty(baggage: Baggage): boolean { - const thirdPartyBaggage = getThirdPartyBaggage(baggage); - return isSentryBaggageEmpty(baggage) && (thirdPartyBaggage == undefined || thirdPartyBaggage.length === 0); -} - /** Returns Sentry specific baggage values */ export function getSentryBaggageItems(baggage: Baggage): BaggageObj { return baggage[0]; diff --git a/packages/utils/test/baggage.test.ts b/packages/utils/test/baggage.test.ts index 5faee8af2e1f..cfd8ebd5f239 100644 --- a/packages/utils/test/baggage.test.ts +++ b/packages/utils/test/baggage.test.ts @@ -3,7 +3,6 @@ import { Baggage } from '@sentry/types'; import { createBaggage, getBaggageValue, - isBaggageEmpty, isBaggageMutable, isSentryBaggageEmpty, mergeAndSerializeBaggage, @@ -155,17 +154,6 @@ describe('Baggage', () => { }); }); - describe('isBaggageEmpty', () => { - it.each([ - ['returns true if the entire baggage tuple is empty', createBaggage({}), true], - ['returns false if the Sentry part of baggage is not empty', createBaggage({ release: '10.0.2' }), false], - ['returns false if the 3rd party part of baggage is not empty', createBaggage({}, 'foo=bar'), false], - ['returns false if both parts of baggage are not empty', createBaggage({ release: '10.0.2' }, 'foo=bar'), false], - ])('%s', (_: string, baggage, outcome) => { - expect(isBaggageEmpty(baggage)).toEqual(outcome); - }); - }); - describe('isSentryBaggageEmpty', () => { it.each([ ['returns true if the Sentry part of baggage is empty', createBaggage({}), true], From 55e024344113af37585591cb99cdac7954a8fcf9 Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Mon, 27 Jun 2022 13:30:27 +0200 Subject: [PATCH 4/7] add node integration tests to test correct merging of 3rd party baggage headers --- .../server.ts | 42 +++++++++++++++++++ .../test.ts | 38 +++++++++++++++++ .../baggage-other-vendors/server.ts | 36 ++++++++++++++++ .../baggage-other-vendors/test.ts | 21 ++++++++++ 4 files changed, 137 insertions(+) create mode 100644 packages/node-integration-tests/suites/express/sentry-trace/baggage-other-vendors-with-sentry-entries/server.ts create mode 100644 packages/node-integration-tests/suites/express/sentry-trace/baggage-other-vendors-with-sentry-entries/test.ts create mode 100644 packages/node-integration-tests/suites/express/sentry-trace/baggage-other-vendors/server.ts create mode 100644 packages/node-integration-tests/suites/express/sentry-trace/baggage-other-vendors/test.ts diff --git a/packages/node-integration-tests/suites/express/sentry-trace/baggage-other-vendors-with-sentry-entries/server.ts b/packages/node-integration-tests/suites/express/sentry-trace/baggage-other-vendors-with-sentry-entries/server.ts new file mode 100644 index 000000000000..1feae49936e5 --- /dev/null +++ b/packages/node-integration-tests/suites/express/sentry-trace/baggage-other-vendors-with-sentry-entries/server.ts @@ -0,0 +1,42 @@ +import * as Sentry from '@sentry/node'; +import * as Tracing from '@sentry/tracing'; +import cors from 'cors'; +import express from 'express'; +import http from 'http'; + +const app = express(); + +export type TestAPIResponse = { test_data: { host: string; 'sentry-trace': string; baggage: string } }; + +Sentry.init({ + dsn: 'https://public@dsn.ingest.sentry.io/1337', + release: '1.0', + environment: 'prod', + integrations: [new Sentry.Integrations.Http({ tracing: true }), new Tracing.Integrations.Express({ app })], + tracesSampleRate: 1.0, +}); + +app.use(Sentry.Handlers.requestHandler()); +app.use(Sentry.Handlers.tracingHandler()); + +app.use(cors()); + +app.get('/test/express', (_req, res) => { + // simulate setting a "third party" baggage header which the Sentry SDK should merge with Sentry DSC entries + const headers = http + .get({ + hostname: 'somewhere.not.sentry', + headers: { + baggage: + 'other=vendor,foo=bar,third=party,sentry-release=9.9.9,sentry-environment=staging,sentry-sample_rate=0.54,last=item', + }, + }) + .getHeaders(); + + // Responding with the headers outgoing request headers back to the assertions. + res.send({ test_data: headers }); +}); + +app.use(Sentry.Handlers.errorHandler()); + +export default app; diff --git a/packages/node-integration-tests/suites/express/sentry-trace/baggage-other-vendors-with-sentry-entries/test.ts b/packages/node-integration-tests/suites/express/sentry-trace/baggage-other-vendors-with-sentry-entries/test.ts new file mode 100644 index 000000000000..4ee6bef7a474 --- /dev/null +++ b/packages/node-integration-tests/suites/express/sentry-trace/baggage-other-vendors-with-sentry-entries/test.ts @@ -0,0 +1,38 @@ +import * as path from 'path'; + +import { getAPIResponse, runServer } from '../../../../utils/index'; +import { TestAPIResponse } from '../server'; + +test('should ignore sentry-values in `baggage` header of a third party vendor and overwrite them with incoming DSC', async () => { + const url = await runServer(__dirname, `${path.resolve(__dirname, '.')}/server.ts`); + + const response = (await getAPIResponse(new URL(`${url}/express`), { + 'sentry-trace': '', + baggage: 'sentry-release=2.1.0,sentry-environment=myEnv', + })) as TestAPIResponse; + + expect(response).toBeDefined(); + expect(response).toMatchObject({ + test_data: { + host: 'somewhere.not.sentry', + baggage: 'other=vendor,foo=bar,third=party,last=item,sentry-release=2.1.0,sentry-environment=myEnv', + }, + }); +}); + +test('should ignore sentry-values in `baggage` header of a third party vendor and overwrite them with new DSC', async () => { + const url = await runServer(__dirname, `${path.resolve(__dirname, '.')}/server.ts`); + + const response = (await getAPIResponse(new URL(`${url}/express`), {})) as TestAPIResponse; + + expect(response).toBeDefined(); + expect(response).toMatchObject({ + test_data: { + host: 'somewhere.not.sentry', + baggage: expect.stringContaining( + 'other=vendor,foo=bar,third=party,last=item,sentry-environment=prod,sentry-release=1.0,' + + 'sentry-transaction=GET%20%2Ftest%2Fexpress,sentry-public_key=public', + ), + }, + }); +}); diff --git a/packages/node-integration-tests/suites/express/sentry-trace/baggage-other-vendors/server.ts b/packages/node-integration-tests/suites/express/sentry-trace/baggage-other-vendors/server.ts new file mode 100644 index 000000000000..475b6928cd8a --- /dev/null +++ b/packages/node-integration-tests/suites/express/sentry-trace/baggage-other-vendors/server.ts @@ -0,0 +1,36 @@ +import * as Sentry from '@sentry/node'; +import * as Tracing from '@sentry/tracing'; +import cors from 'cors'; +import express from 'express'; +import http from 'http'; + +const app = express(); + +export type TestAPIResponse = { test_data: { host: string; 'sentry-trace': string; baggage: string } }; + +Sentry.init({ + dsn: 'https://public@dsn.ingest.sentry.io/1337', + release: '1.0', + environment: 'prod', + integrations: [new Sentry.Integrations.Http({ tracing: true }), new Tracing.Integrations.Express({ app })], + tracesSampleRate: 1.0, +}); + +app.use(Sentry.Handlers.requestHandler()); +app.use(Sentry.Handlers.tracingHandler()); + +app.use(cors()); + +app.get('/test/express', (_req, res) => { + // simulate setting a "third party" baggage header which the Sentry SDK should merge with Sentry DSC entries + const headers = http + .get({ hostname: 'somewhere.not.sentry', headers: { baggage: 'other=vendor,foo=bar,third=party' } }) + .getHeaders(); + + // Responding with the headers outgoing request headers back to the assertions. + res.send({ test_data: headers }); +}); + +app.use(Sentry.Handlers.errorHandler()); + +export default app; diff --git a/packages/node-integration-tests/suites/express/sentry-trace/baggage-other-vendors/test.ts b/packages/node-integration-tests/suites/express/sentry-trace/baggage-other-vendors/test.ts new file mode 100644 index 000000000000..34cadd277a9e --- /dev/null +++ b/packages/node-integration-tests/suites/express/sentry-trace/baggage-other-vendors/test.ts @@ -0,0 +1,21 @@ +import * as path from 'path'; + +import { getAPIResponse, runServer } from '../../../../utils/index'; +import { TestAPIResponse } from '../server'; + +test('should merge `baggage` header of a third party vendor with the Sentry DSC baggage items', async () => { + const url = await runServer(__dirname, `${path.resolve(__dirname, '.')}/server.ts`); + + const response = (await getAPIResponse(new URL(`${url}/express`), { + 'sentry-trace': '', + baggage: 'sentry-release=2.0.0,sentry-environment=myEnv', + })) as TestAPIResponse; + + expect(response).toBeDefined(); + expect(response).toMatchObject({ + test_data: { + host: 'somewhere.not.sentry', + baggage: 'other=vendor,foo=bar,third=party,sentry-release=2.0.0,sentry-environment=myEnv', + }, + }); +}); From 24090632ef8ca99ad8321fd5ce2cab7fc41d1b6d Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Mon, 27 Jun 2022 13:52:41 +0200 Subject: [PATCH 5/7] re-add accidentally deleted yarn script --- packages/node-integration-tests/package.json | 1 + 1 file changed, 1 insertion(+) diff --git a/packages/node-integration-tests/package.json b/packages/node-integration-tests/package.json index b61a72949991..4b85157ff50e 100644 --- a/packages/node-integration-tests/package.json +++ b/packages/node-integration-tests/package.json @@ -13,6 +13,7 @@ "lint:eslint": "eslint . --cache --cache-location '../../eslintcache/' --format stylish", "lint:prettier": "prettier --check \"{suites,utils}/**/*.ts\"", "type-check": "tsc", + "pretest": "run-s --silent prisma:init", "test": "jest --runInBand --forceExit", "test:watch": "yarn test --watch" }, From 1dc038f179f1b522c09f841a08a011049175241d Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Mon, 27 Jun 2022 14:07:44 +0200 Subject: [PATCH 6/7] fix serverless unit tests --- packages/serverless/test/awslambda.test.ts | 2 +- packages/serverless/test/gcpfunction.test.ts | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/serverless/test/awslambda.test.ts b/packages/serverless/test/awslambda.test.ts index 20eb3cb410c0..69c8481f89d1 100644 --- a/packages/serverless/test/awslambda.test.ts +++ b/packages/serverless/test/awslambda.test.ts @@ -256,7 +256,7 @@ describe('AWSLambda', () => { { release: '2.12.1', }, - 'maisey=silly,charlie=goofy', + '', false, ], }, diff --git a/packages/serverless/test/gcpfunction.test.ts b/packages/serverless/test/gcpfunction.test.ts index e9667adfd81d..d6229394990b 100644 --- a/packages/serverless/test/gcpfunction.test.ts +++ b/packages/serverless/test/gcpfunction.test.ts @@ -150,7 +150,7 @@ describe('GCPFunction', () => { { release: '2.12.1', }, - 'maisey=silly,charlie=goofy', + '', false, ], }, From 795dd25bb5ec16d81c10c4d3afeb642aba1a688c Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Mon, 27 Jun 2022 17:56:20 +0200 Subject: [PATCH 7/7] change mutability condition in parseBaggageSetMutability --- .../express/sentry-trace/baggage-header-assign/test.ts | 1 - packages/utils/src/baggage.ts | 7 ++++++- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/packages/node-integration-tests/suites/express/sentry-trace/baggage-header-assign/test.ts b/packages/node-integration-tests/suites/express/sentry-trace/baggage-header-assign/test.ts index 00c64ad8c9ba..205451292baf 100644 --- a/packages/node-integration-tests/suites/express/sentry-trace/baggage-header-assign/test.ts +++ b/packages/node-integration-tests/suites/express/sentry-trace/baggage-header-assign/test.ts @@ -7,7 +7,6 @@ test('Should not overwrite baggage if the incoming request already has Sentry ba const url = await runServer(__dirname, `${path.resolve(__dirname, '..')}/server.ts`); const response = (await getAPIResponse(new URL(`${url}/express`), { - 'sentry-trace': '', baggage: 'sentry-release=2.0.0,sentry-environment=myEnv', })) as TestAPIResponse; diff --git a/packages/utils/src/baggage.ts b/packages/utils/src/baggage.ts index 11436c9d9675..9c9b7405aea2 100644 --- a/packages/utils/src/baggage.ts +++ b/packages/utils/src/baggage.ts @@ -185,7 +185,12 @@ export function parseBaggageSetMutability( // we only need to check if we have a sentry-trace header or not. As soon as we have it, // we set baggage immutable. In case we don't get a sentry-trace header, we can assume that // this SDK is the head of the trace and thus we still permit mutation at this time. - sentryTraceHeader && setBaggageImmutable(baggage); + // There is one exception though, which is that we get a baggage-header with `sentry-` + // items but NO sentry-trace header. In this case we also set the baggage immutable for now + // but if smoething like this would ever happen, we should revisit this and determine + // what this would actually mean for the trace (i.e. is this SDK the head?, what happened + // before that we don't have a sentry-trace header?, etc) + (sentryTraceHeader || !isSentryBaggageEmpty(baggage)) && setBaggageImmutable(baggage); return baggage; }