Skip to content

Commit 51a9def

Browse files
authored
fix(core): Keep all property values in baggage header (#17847)
https://www.w3.org/TR/baggage/#example contains an example where properties can include an `=`, e.g. `key3=value3; propertyKey=propertyValue` This PR updates our baggage parsing to not swallow these property values closes #17845
1 parent 743c4eb commit 51a9def

File tree

9 files changed

+193
-4
lines changed

9 files changed

+193
-4
lines changed
Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,9 @@
1+
fetchButton.addEventListener('click', () => {
2+
// W3C spec example: property values can contain = signs
3+
// See: https://www.w3.org/TR/baggage/#example
4+
fetch('http://sentry-test-site.example/fetch-test', {
5+
headers: {
6+
baggage: 'key1=value1;property1;property2,key2=value2,key3=value3; propertyKey=propertyValue',
7+
},
8+
});
9+
});
Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,11 @@
1+
<!doctype html>
2+
<html lang="en">
3+
<head>
4+
<meta charset="UTF-8" />
5+
<meta name="viewport" content="width=device-width, initial-scale=1.0" />
6+
<title>Fetch Baggage Property Values Test</title>
7+
</head>
8+
<body>
9+
<button id="fetchButton">Make Fetch Request</button>
10+
</body>
11+
</html>
Lines changed: 41 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,41 @@
1+
import { expect } from '@playwright/test';
2+
import { TRACEPARENT_REGEXP } from '@sentry/core';
3+
import { sentryTest } from '../../../../utils/fixtures';
4+
import { shouldSkipTracingTest } from '../../../../utils/helpers';
5+
6+
sentryTest(
7+
'preserves baggage property values with equal signs in fetch requests',
8+
async ({ getLocalTestUrl, page }) => {
9+
if (shouldSkipTracingTest()) {
10+
sentryTest.skip();
11+
}
12+
13+
const url = await getLocalTestUrl({ testDir: __dirname });
14+
15+
const requestPromise = page.waitForRequest('http://sentry-test-site.example/fetch-test');
16+
17+
await page.goto(url);
18+
await page.click('#fetchButton');
19+
20+
const request = await requestPromise;
21+
22+
const requestHeaders = request.headers();
23+
24+
expect(requestHeaders).toMatchObject({
25+
'sentry-trace': expect.stringMatching(TRACEPARENT_REGEXP),
26+
});
27+
28+
const baggageHeader = requestHeaders.baggage;
29+
expect(baggageHeader).toBeDefined();
30+
31+
const baggageItems = baggageHeader.split(',').map(item => decodeURIComponent(item.trim()));
32+
33+
// Verify property values with = signs are preserved
34+
expect(baggageItems).toContainEqual(expect.stringContaining('key1=value1;property1;property2'));
35+
expect(baggageItems).toContainEqual(expect.stringContaining('key2=value2'));
36+
expect(baggageItems).toContainEqual(expect.stringContaining('key3=value3; propertyKey=propertyValue'));
37+
38+
// Verify Sentry baggage is also present
39+
expect(baggageHeader).toMatch(/sentry-/);
40+
},
41+
);
Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,8 @@
1+
const xhr = new XMLHttpRequest();
2+
3+
xhr.open('GET', 'http://sentry-test-site.example/1');
4+
// W3C spec example: property values can contain = signs
5+
// See: https://www.w3.org/TR/baggage/#example
6+
xhr.setRequestHeader('baggage', 'key1=value1;property1;property2,key2=value2,key3=value3; propertyKey=propertyValue');
7+
8+
xhr.send();
Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,36 @@
1+
import { expect } from '@playwright/test';
2+
import { TRACEPARENT_REGEXP } from '@sentry/core';
3+
import { sentryTest } from '../../../../utils/fixtures';
4+
import { shouldSkipTracingTest } from '../../../../utils/helpers';
5+
6+
sentryTest('preserves baggage property values with equal signs in XHR requests', async ({ getLocalTestUrl, page }) => {
7+
if (shouldSkipTracingTest()) {
8+
sentryTest.skip();
9+
}
10+
11+
const url = await getLocalTestUrl({ testDir: __dirname });
12+
13+
const requestPromise = page.waitForRequest('http://sentry-test-site.example/1');
14+
15+
await page.goto(url);
16+
17+
const request = await requestPromise;
18+
19+
const requestHeaders = request.headers();
20+
21+
expect(requestHeaders).toMatchObject({
22+
'sentry-trace': expect.stringMatching(TRACEPARENT_REGEXP),
23+
});
24+
25+
const baggageHeader = requestHeaders.baggage;
26+
expect(baggageHeader).toBeDefined();
27+
const baggageItems = baggageHeader.split(',').map(item => decodeURIComponent(item.trim()));
28+
29+
// Verify property values with = signs are preserved
30+
expect(baggageItems).toContainEqual(expect.stringContaining('key1=value1;property1;property2'));
31+
expect(baggageItems).toContainEqual(expect.stringContaining('key2=value2'));
32+
expect(baggageItems).toContainEqual(expect.stringContaining('key3=value3; propertyKey=propertyValue'));
33+
34+
// Verify Sentry baggage is also present
35+
expect(baggageHeader).toMatch(/sentry-/);
36+
});
Lines changed: 37 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,37 @@
1+
import * as Sentry from '@sentry/node';
2+
import { loggingTransport, startExpressServerAndSendPortToRunner } from '@sentry-internal/node-integration-tests';
3+
4+
export type TestAPIResponse = { test_data: { host: string; 'sentry-trace': string; baggage: string } };
5+
6+
Sentry.init({
7+
dsn: 'https://[email protected]/1337',
8+
release: '1.0',
9+
environment: 'prod',
10+
// disable requests to /express
11+
tracePropagationTargets: [/^(?!.*express).*$/],
12+
tracesSampleRate: 1.0,
13+
transport: loggingTransport,
14+
});
15+
16+
import cors from 'cors';
17+
import express from 'express';
18+
import http from 'http';
19+
20+
const app = express();
21+
22+
app.use(cors());
23+
24+
app.get('/test/express-property-values', (req, res) => {
25+
const incomingBaggage = req.headers.baggage;
26+
27+
// Forward the incoming baggage (which contains property values) to the outgoing request
28+
// This tests that property values with = signs are preserved during parsing and re-serialization
29+
const headers = http.get({ hostname: 'somewhere.not.sentry', headers: { baggage: incomingBaggage } }).getHeaders();
30+
31+
// Responding with the headers outgoing request headers back to the assertions.
32+
res.send({ test_data: headers });
33+
});
34+
35+
Sentry.setupExpressErrorHandler(app);
36+
37+
startExpressServerAndSendPortToRunner(app);
Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,28 @@
1+
import { afterAll, expect, test } from 'vitest';
2+
import { cleanupChildProcesses, createRunner } from '../../../../utils/runner';
3+
import type { TestAPIResponse } from './server';
4+
5+
afterAll(() => {
6+
cleanupChildProcesses();
7+
});
8+
9+
test('should preserve baggage property values with equal signs (W3C spec compliance)', async () => {
10+
const runner = createRunner(__dirname, 'server.ts').start();
11+
12+
// W3C spec example: https://www.w3.org/TR/baggage/#example
13+
const response = await runner.makeRequest<TestAPIResponse>('get', '/test/express-property-values', {
14+
headers: {
15+
'sentry-trace': '12312012123120121231201212312012-1121201211212012-1',
16+
baggage: 'key1=value1;property1;property2,key2=value2,key3=value3; propertyKey=propertyValue',
17+
},
18+
});
19+
20+
expect(response).toBeDefined();
21+
22+
// The baggage should be parsed and re-serialized, preserving property values with = signs
23+
const baggageItems = response?.test_data.baggage?.split(',').map(item => decodeURIComponent(item.trim()));
24+
25+
expect(baggageItems).toContain('key1=value1;property1;property2');
26+
expect(baggageItems).toContain('key2=value2');
27+
expect(baggageItems).toContain('key3=value3; propertyKey=propertyValue');
28+
});

packages/core/src/utils/baggage.ts

Lines changed: 11 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -113,17 +113,24 @@ export function parseBaggageHeader(
113113
function baggageHeaderToObject(baggageHeader: string): Record<string, string> {
114114
return baggageHeader
115115
.split(',')
116-
.map(baggageEntry =>
117-
baggageEntry.split('=').map(keyOrValue => {
116+
.map(baggageEntry => {
117+
const eqIdx = baggageEntry.indexOf('=');
118+
if (eqIdx === -1) {
119+
// Likely an invalid entry
120+
return [];
121+
}
122+
const key = baggageEntry.slice(0, eqIdx);
123+
const value = baggageEntry.slice(eqIdx + 1);
124+
return [key, value].map(keyOrValue => {
118125
try {
119126
return decodeURIComponent(keyOrValue.trim());
120127
} catch {
121128
// We ignore errors here, e.g. if the value cannot be URL decoded.
122129
// This will then be skipped in the next step
123130
return;
124131
}
125-
}),
126-
)
132+
});
133+
})
127134
.reduce<Record<string, string>>((acc, [key, value]) => {
128135
if (key && value) {
129136
acc[key] = value;

packages/core/test/lib/utils/baggage.test.ts

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -71,4 +71,16 @@ describe('parseBaggageHeader', () => {
7171
const actual = parseBaggageHeader(input);
7272
expect(actual).toStrictEqual(expectedOutput);
7373
});
74+
75+
test('should preserve property values with equal signs', () => {
76+
// see https://www.w3.org/TR/baggage/#example
77+
const baggageHeader = 'key1=value1;property1;property2, key2 = value2, key3=value3; propertyKey=propertyValue';
78+
const result = parseBaggageHeader(baggageHeader);
79+
80+
expect(result).toStrictEqual({
81+
key1: 'value1;property1;property2',
82+
key2: 'value2',
83+
key3: 'value3; propertyKey=propertyValue',
84+
});
85+
});
7486
});

0 commit comments

Comments
 (0)