Skip to content

Commit 3e07e20

Browse files
Lms24Lms24
authored
ref(tracing): Ignore third party baggage entries from incoming requests (#5319)
On incoming requests we no longer parse and put non-`sentry-` baggage entries into the `Baggage` object but we ignore them and instead store an empty third party string. The reason for this change is that - as we discussed and agreed upon - the Sentry SDK should not be responsible for propagating other vendors' data. If users want to propagate that data as well on the service(s) where the Sentry SDK is installed, they should add the other vendors' propagation mechanisms. In fact, this is the only way we can ensure that baggage content that should _not_ be propagated further is actually stopped. It's important to note that this only concerns _incoming_ 3rd party baggage items. If other vendors (or the users themselves) add a `baggage` header to the outgoing request before the Sentry SDK adds its `baggage` header, we continue to merge the two headers such that the 3rd party baggage as well as this SDK's DSC entries are still in the final header. In (the edge) case that this 3rd party `baggage` header already contains `sentry-` entries, they will be ignored and overwritten with this SDK's `sentry-` entries. With this patch, our DSC propagation behaviour conforms with [our DSC specification](https://develop.sentry.dev/sdk/performance/dynamic-sampling-context/). In addition of these changes, this patch also contains a few small cleanups and improvements: * removed `isBaggageEmpty` helper function (+ tests) as it was not used anymore except for in tests * simplified mutability check: Because of how we parse incoming baggage headers, we can make the check slightly simpler than the pseudo code in the spec. * adjusted tests to new 3rd party handling * added node integration tests to check for correct handling of `baggage` header merging in case another vendor injects a baggage header before the Sentry SDK
1 parent b67c440 commit 3e07e20

File tree

17 files changed

+237
-97
lines changed

17 files changed

+237
-97
lines changed

‎packages/nextjs/src/utils/withSentry.ts

+1-1
Original file line numberDiff line numberDiff line change
@@ -72,7 +72,7 @@ export const withSentry = (origHandler: NextApiHandler): WrappedNextApiHandler =
7272
name: `${reqMethod}${reqPath}`,
7373
op: 'http.server',
7474
...traceparentData,
75-
metadata: { baggage: baggage },
75+
metadata: { baggage },
7676
},
7777
// extra context passed to the `tracesSampler`
7878
{ request: req },

‎packages/node-integration-tests/suites/express/sentry-trace/baggage-header-assign/test.ts

+10-9
Original file line numberDiff line numberDiff line change
@@ -7,30 +7,31 @@ test('Should not overwrite baggage if the incoming request already has Sentry ba
77
const url = await runServer(__dirname, `${path.resolve(__dirname, '..')}/server.ts`);
88

99
const response = (await getAPIResponse(new URL(`${url}/express`), {
10-
baggage: 'sentry-version=2.0.0,sentry-environment=myEnv',
10+
baggage: 'sentry-release=2.0.0,sentry-environment=myEnv',
1111
})) as TestAPIResponse;
1212

1313
expect(response).toBeDefined();
1414
expect(response).toMatchObject({
1515
test_data: {
1616
host: 'somewhere.not.sentry',
17-
baggage: 'sentry-version=2.0.0,sentry-environment=myEnv',
17+
baggage: 'sentry-release=2.0.0,sentry-environment=myEnv',
1818
},
1919
});
2020
});
2121

22-
test('Should pass along sentry and 3rd party trace baggage data from an incoming to an outgoing request.', async () => {
22+
test('Should propagate sentry trace baggage data from an incoming to an outgoing request.', async () => {
2323
const url = await runServer(__dirname, `${path.resolve(__dirname, '..')}/server.ts`);
2424

2525
const response = (await getAPIResponse(new URL(`${url}/express`), {
26-
baggage: 'sentry-version=2.0.0,sentry-environment=myEnv,dogs=great',
26+
'sentry-trace': '',
27+
baggage: 'sentry-release=2.0.0,sentry-environment=myEnv,dogs=great',
2728
})) as TestAPIResponse;
2829

2930
expect(response).toBeDefined();
3031
expect(response).toMatchObject({
3132
test_data: {
3233
host: 'somewhere.not.sentry',
33-
baggage: expect.stringContaining('dogs=great,sentry-version=2.0.0,sentry-environment=myEnv'),
34+
baggage: 'sentry-release=2.0.0,sentry-environment=myEnv',
3435
},
3536
});
3637
});
@@ -51,7 +52,7 @@ test('Should propagate empty baggage if sentry-trace header is present in incomi
5152
});
5253
});
5354

54-
test('Should propagate empty sentry and original 3rd party baggage if sentry-trace header is present', async () => {
55+
test('Should propagate empty sentry and ignore original 3rd party baggage entries if sentry-trace header is present', async () => {
5556
const url = await runServer(__dirname, `${path.resolve(__dirname, '..')}/server.ts`);
5657

5758
const response = (await getAPIResponse(new URL(`${url}/express`), {
@@ -63,7 +64,7 @@ test('Should propagate empty sentry and original 3rd party baggage if sentry-tra
6364
expect(response).toMatchObject({
6465
test_data: {
6566
host: 'somewhere.not.sentry',
66-
baggage: 'foo=bar',
67+
baggage: '',
6768
},
6869
});
6970
});
@@ -85,7 +86,7 @@ test('Should populate and propagate sentry baggage if sentry-trace header does n
8586
});
8687
});
8788

88-
test('Should populate Sentry and propagate 3rd party content if sentry-trace header does not exist', async () => {
89+
test('Should populate Sentry and ignore 3rd party content if sentry-trace header does not exist', async () => {
8990
const url = await runServer(__dirname, `${path.resolve(__dirname, '..')}/server.ts`);
9091

9192
const response = (await getAPIResponse(new URL(`${url}/express`), {
@@ -98,7 +99,7 @@ test('Should populate Sentry and propagate 3rd party content if sentry-trace hea
9899
host: 'somewhere.not.sentry',
99100
// TraceId changes, hence we only expect that the string contains the traceid key
100101
baggage: expect.stringContaining(
101-
'foo=bar,bar=baz,sentry-environment=prod,sentry-release=1.0,sentry-transaction=GET%20%2Ftest%2Fexpress,' +
102+
'sentry-environment=prod,sentry-release=1.0,sentry-transaction=GET%20%2Ftest%2Fexpress,' +
102103
'sentry-public_key=public,sentry-trace_id=',
103104
),
104105
},

‎packages/node-integration-tests/suites/express/sentry-trace/baggage-other-vendors-with-sentry-entries/server.ts

+42
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,42 @@
1+
import * as Sentry from '@sentry/node';
2+
import * as Tracing from '@sentry/tracing';
3+
import cors from 'cors';
4+
import express from 'express';
5+
import http from 'http';
6+
7+
const app = express();
8+
9+
export type TestAPIResponse = { test_data: { host: string; 'sentry-trace': string; baggage: string } };
10+
11+
Sentry.init({
12+
dsn: 'https://[email protected]/1337',
13+
release: '1.0',
14+
environment: 'prod',
15+
integrations: [new Sentry.Integrations.Http({ tracing: true }), new Tracing.Integrations.Express({ app })],
16+
tracesSampleRate: 1.0,
17+
});
18+
19+
app.use(Sentry.Handlers.requestHandler());
20+
app.use(Sentry.Handlers.tracingHandler());
21+
22+
app.use(cors());
23+
24+
app.get('/test/express', (_req, res) => {
25+
// simulate setting a "third party" baggage header which the Sentry SDK should merge with Sentry DSC entries
26+
const headers = http
27+
.get({
28+
hostname: 'somewhere.not.sentry',
29+
headers: {
30+
baggage:
31+
'other=vendor,foo=bar,third=party,sentry-release=9.9.9,sentry-environment=staging,sentry-sample_rate=0.54,last=item',
32+
},
33+
})
34+
.getHeaders();
35+
36+
// Responding with the headers outgoing request headers back to the assertions.
37+
res.send({ test_data: headers });
38+
});
39+
40+
app.use(Sentry.Handlers.errorHandler());
41+
42+
export default app;

‎packages/node-integration-tests/suites/express/sentry-trace/baggage-other-vendors-with-sentry-entries/test.ts

+38
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,38 @@
1+
import * as path from 'path';
2+
3+
import { getAPIResponse, runServer } from '../../../../utils/index';
4+
import { TestAPIResponse } from '../server';
5+
6+
test('should ignore sentry-values in `baggage` header of a third party vendor and overwrite them with incoming DSC', async () => {
7+
const url = await runServer(__dirname, `${path.resolve(__dirname, '.')}/server.ts`);
8+
9+
const response = (await getAPIResponse(new URL(`${url}/express`), {
10+
'sentry-trace': '',
11+
baggage: 'sentry-release=2.1.0,sentry-environment=myEnv',
12+
})) as TestAPIResponse;
13+
14+
expect(response).toBeDefined();
15+
expect(response).toMatchObject({
16+
test_data: {
17+
host: 'somewhere.not.sentry',
18+
baggage: 'other=vendor,foo=bar,third=party,last=item,sentry-release=2.1.0,sentry-environment=myEnv',
19+
},
20+
});
21+
});
22+
23+
test('should ignore sentry-values in `baggage` header of a third party vendor and overwrite them with new DSC', async () => {
24+
const url = await runServer(__dirname, `${path.resolve(__dirname, '.')}/server.ts`);
25+
26+
const response = (await getAPIResponse(new URL(`${url}/express`), {})) as TestAPIResponse;
27+
28+
expect(response).toBeDefined();
29+
expect(response).toMatchObject({
30+
test_data: {
31+
host: 'somewhere.not.sentry',
32+
baggage: expect.stringContaining(
33+
'other=vendor,foo=bar,third=party,last=item,sentry-environment=prod,sentry-release=1.0,' +
34+
'sentry-transaction=GET%20%2Ftest%2Fexpress,sentry-public_key=public',
35+
),
36+
},
37+
});
38+
});

‎packages/node-integration-tests/suites/express/sentry-trace/baggage-other-vendors/server.ts

+36
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,36 @@
1+
import * as Sentry from '@sentry/node';
2+
import * as Tracing from '@sentry/tracing';
3+
import cors from 'cors';
4+
import express from 'express';
5+
import http from 'http';
6+
7+
const app = express();
8+
9+
export type TestAPIResponse = { test_data: { host: string; 'sentry-trace': string; baggage: string } };
10+
11+
Sentry.init({
12+
dsn: 'https://[email protected]/1337',
13+
release: '1.0',
14+
environment: 'prod',
15+
integrations: [new Sentry.Integrations.Http({ tracing: true }), new Tracing.Integrations.Express({ app })],
16+
tracesSampleRate: 1.0,
17+
});
18+
19+
app.use(Sentry.Handlers.requestHandler());
20+
app.use(Sentry.Handlers.tracingHandler());
21+
22+
app.use(cors());
23+
24+
app.get('/test/express', (_req, res) => {
25+
// simulate setting a "third party" baggage header which the Sentry SDK should merge with Sentry DSC entries
26+
const headers = http
27+
.get({ hostname: 'somewhere.not.sentry', headers: { baggage: 'other=vendor,foo=bar,third=party' } })
28+
.getHeaders();
29+
30+
// Responding with the headers outgoing request headers back to the assertions.
31+
res.send({ test_data: headers });
32+
});
33+
34+
app.use(Sentry.Handlers.errorHandler());
35+
36+
export default app;

‎packages/node-integration-tests/suites/express/sentry-trace/baggage-other-vendors/test.ts

+21
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,21 @@
1+
import * as path from 'path';
2+
3+
import { getAPIResponse, runServer } from '../../../../utils/index';
4+
import { TestAPIResponse } from '../server';
5+
6+
test('should merge `baggage` header of a third party vendor with the Sentry DSC baggage items', async () => {
7+
const url = await runServer(__dirname, `${path.resolve(__dirname, '.')}/server.ts`);
8+
9+
const response = (await getAPIResponse(new URL(`${url}/express`), {
10+
'sentry-trace': '',
11+
baggage: 'sentry-release=2.0.0,sentry-environment=myEnv',
12+
})) as TestAPIResponse;
13+
14+
expect(response).toBeDefined();
15+
expect(response).toMatchObject({
16+
test_data: {
17+
host: 'somewhere.not.sentry',
18+
baggage: 'other=vendor,foo=bar,third=party,sentry-release=2.0.0,sentry-environment=myEnv',
19+
},
20+
});
21+
});

‎packages/node/src/handlers.ts

+1-1
Original file line numberDiff line numberDiff line change
@@ -45,7 +45,7 @@ export function tracingHandler(): (
4545
name: extractPathForTransaction(req, { path: true, method: true }),
4646
op: 'http.server',
4747
...traceparentData,
48-
metadata: { baggage: baggage },
48+
metadata: { baggage },
4949
},
5050
// extra context passed to the tracesSampler
5151
{ request: extractRequestData(req) },

‎packages/node/test/handlers.test.ts

+6-4
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,7 @@ import * as sentryHub from '@sentry/hub';
33
import { Hub } from '@sentry/hub';
44
import { Transaction } from '@sentry/tracing';
55
import { Baggage, Event } from '@sentry/types';
6-
import { isBaggageEmpty, isBaggageMutable, SentryError } from '@sentry/utils';
6+
import { getThirdPartyBaggage, isBaggageMutable, isSentryBaggageEmpty, SentryError } from '@sentry/utils';
77
import * as http from 'http';
88

99
import { NodeClient } from '../src/client';
@@ -193,7 +193,8 @@ describe('tracingHandler', () => {
193193
expect(transaction.parentSpanId).toEqual('1121201211212012');
194194
expect(transaction.sampled).toEqual(false);
195195
expect(transaction.metadata?.baggage).toBeDefined();
196-
expect(isBaggageEmpty(transaction.metadata?.baggage)).toBe(true);
196+
expect(isSentryBaggageEmpty(transaction.metadata?.baggage)).toBe(true);
197+
expect(getThirdPartyBaggage(transaction.metadata?.baggage)).toEqual('');
197198
expect(isBaggageMutable(transaction.metadata?.baggage)).toBe(false);
198199
});
199200

@@ -219,8 +220,9 @@ describe('tracingHandler', () => {
219220
] as Baggage);
220221
});
221222

222-
it("pulls parent's baggage (sentry + third party entries) headers on the request", () => {
223+
it('ignores 3rd party entries in incoming baggage header', () => {
223224
req.headers = {
225+
'sentry-trace': '12312012123120121231201212312012-1121201211212012-0',
224226
baggage: 'sentry-version=1.0,sentry-environment=production,dogs=great,cats=boring',
225227
};
226228

@@ -231,7 +233,7 @@ describe('tracingHandler', () => {
231233
expect(transaction.metadata?.baggage).toBeDefined();
232234
expect(transaction.metadata?.baggage).toEqual([
233235
{ version: '1.0', environment: 'production' },
234-
'dogs=great,cats=boring',
236+
'',
235237
false,
236238
] as Baggage);
237239
});

‎packages/serverless/src/awslambda.ts

+1-1
Original file line numberDiff line numberDiff line change
@@ -289,7 +289,7 @@ export function wrapHandler<TEvent, TResult>(
289289
name: context.functionName,
290290
op: 'awslambda.handler',
291291
...traceparentData,
292-
metadata: { baggage: baggage },
292+
metadata: { baggage },
293293
});
294294

295295
const hub = getCurrentHub();

‎packages/serverless/src/gcpfunction/http.ts

+1-1
Original file line numberDiff line numberDiff line change
@@ -87,7 +87,7 @@ function _wrapHttpFunction(fn: HttpFunction, wrapOptions: Partial<HttpFunctionWr
8787
name: `${reqMethod} ${reqUrl}`,
8888
op: 'gcp.function.http',
8989
...traceparentData,
90-
metadata: { baggage: baggage },
90+
metadata: { baggage },
9191
});
9292

9393
// getCurrentHub() is expected to use current active domain as a carrier

‎packages/serverless/test/awslambda.test.ts

+1-1
Original file line numberDiff line numberDiff line change
@@ -256,7 +256,7 @@ describe('AWSLambda', () => {
256256
{
257257
release: '2.12.1',
258258
},
259-
'maisey=silly,charlie=goofy',
259+
'',
260260
false,
261261
],
262262
},

‎packages/serverless/test/gcpfunction.test.ts

+1-1
Original file line numberDiff line numberDiff line change
@@ -150,7 +150,7 @@ describe('GCPFunction', () => {
150150
{
151151
release: '2.12.1',
152152
},
153-
'maisey=silly,charlie=goofy',
153+
'',
154154
false,
155155
],
156156
},

‎packages/tracing/src/transaction.ts

+5-17
Original file line numberDiff line numberDiff line change
@@ -9,15 +9,7 @@ import {
99
TransactionContext,
1010
TransactionMetadata,
1111
} from '@sentry/types';
12-
import {
13-
createBaggage,
14-
dropUndefinedKeys,
15-
getSentryBaggageItems,
16-
getThirdPartyBaggage,
17-
isBaggageMutable,
18-
isSentryBaggageEmpty,
19-
logger,
20-
} from '@sentry/utils';
12+
import { createBaggage, dropUndefinedKeys, getSentryBaggageItems, isBaggageMutable, logger } from '@sentry/utils';
2113

2214
import { Span as SpanClass, SpanRecorder } from './span';
2315

@@ -198,17 +190,13 @@ export class Transaction extends SpanClass implements TransactionInterface {
198190

199191
// Only add Sentry baggage items to baggage, if baggage does not exist yet or it is still
200192
// empty and mutable
201-
// TODO: we might want to ditch the isSentryBaggageEmpty condition because it prevents
202-
// custom sentry-values in DSC (added by users in the future)
203193
const finalBaggage =
204-
!existingBaggage || (isBaggageMutable(existingBaggage) && isSentryBaggageEmpty(existingBaggage))
194+
!existingBaggage || isBaggageMutable(existingBaggage)
205195
? this._populateBaggageWithSentryValues(existingBaggage)
206196
: existingBaggage;
207197

208-
// In case, we poulated the DSC, we have update the stored one on the transaction.
209-
if (existingBaggage !== finalBaggage) {
210-
this.metadata.baggage = finalBaggage;
211-
}
198+
// Update the baggage stored on the transaction.
199+
this.metadata.baggage = finalBaggage;
212200

213201
return finalBaggage;
214202
}
@@ -256,7 +244,7 @@ export class Transaction extends SpanClass implements TransactionInterface {
256244
sample_rate,
257245
...getSentryBaggageItems(baggage), // keep user-added values
258246
} as BaggageObj),
259-
getThirdPartyBaggage(baggage), // TODO: remove once we ignore 3rd party baggage
247+
'',
260248
false, // set baggage immutable
261249
);
262250
}

0 commit comments

Comments
 (0)