Skip to content
Open
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
55 changes: 38 additions & 17 deletions packages/core/src/fetch.ts
Original file line number Diff line number Diff line change
Expand Up @@ -19,8 +19,9 @@
} from './utils/url';

type PolymorphicRequestHeaders =
| Record<string, string | undefined>
| Array<[string, string]>
| Record<string, unknown>
| Array<[string, unknown]>
| Iterable<Iterable<unknown>>
Comment on lines +22 to +24
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: The function _INTERNAL_getTracingHeadersForFetchRequest doesn't handle Iterable header types like Map, causing user-provided headers to be silently discarded when passed.
Severity: HIGH

Suggested Fix

Add a new branch to _INTERNAL_getTracingHeadersForFetchRequest to handle Iterable<Iterable<unknown>> types. This branch should convert the iterable into a Headers object or a plain object before merging it with the Sentry tracing headers. For example, new Headers(originalHeaders) can be used to correctly process the iterable.

Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.

Location: packages/core/src/fetch.ts#L22-L24

Potential issue: The `PolymorphicRequestHeaders` type was expanded to include
`Iterable<Iterable<unknown>>` to support environments like Cloudflare Workers. However,
the implementation of `_INTERNAL_getTracingHeadersForFetchRequest` was not updated to
handle this case. When an iterable that is not an array (like a `Map`) is passed as
headers, it falls into an `else` block that incorrectly assumes it's a plain object.
Using the spread operator (`...`) on a `Map` to create a new object results in an empty
object, causing all original headers to be silently dropped from the request.

// the below is not precisely the Header type used in Request, but it'll pass duck-typing
| {
append: (key: string, value: string) => void;
Expand Down Expand Up @@ -124,7 +125,7 @@
// Examples: users re-using same options object for multiple fetch calls, frozen objects
const options: { [key: string]: unknown } = { ...(handlerData.args[1] || {}) };

const headers = _addTracingHeadersToFetchRequest(
const headers = _INTERNAL_getTracingHeadersForFetchRequest(
request,
options,
// If performance is disabled (TWP) or there's no active root span (pageload/navigation/interaction),
Expand Down Expand Up @@ -176,17 +177,21 @@
}

/**
* Adds sentry-trace and baggage headers to the various forms of fetch headers.
* exported only for testing purposes
* Builds merged fetch headers that include `sentry-trace` and `baggage` (and optionally `traceparent`)
* for the given request and init, without mutating the original request or options.
* Returns `undefined` when there is no `sentry-trace` value to attach.
*
* When we determine if we should add a baggage header, there are 3 cases:
* 1. No previous baggage header -> add baggage
* 2. Previous baggage header has no sentry baggage values -> add our baggage
* 3. Previous baggage header has sentry baggage values -> do nothing (might have been added manually by users)
* @internal Exported for cross-package instrumentation (for example Cloudflare Workers fetcher bindings)
* and unit tests
*
* Baggage handling:
* 1. No previous baggage header → include Sentry baggage
* 2. Previous baggage has no Sentry entries → merge Sentry baggage in
* 3. Previous baggage already has Sentry entries → leave as-is (may be user-defined)
*/
// eslint-disable-next-line complexity -- yup it's this complicated :(
export function _addTracingHeadersToFetchRequest(
request: string | Request,
export function _INTERNAL_getTracingHeadersForFetchRequest(
request: string | URL | Request,
fetchOptionsObj: {
headers?:
| {
Expand Down Expand Up @@ -234,19 +239,20 @@
}

return newHeaders;
} else if (Array.isArray(originalHeaders)) {
} else if (isHeadersInitTupleArray(originalHeaders)) {
const newHeaders = [...originalHeaders];

if (!originalHeaders.find(header => header[0] === 'sentry-trace')) {
if (!newHeaders.find(header => header[0] === 'sentry-trace')) {
newHeaders.push(['sentry-trace', sentryTrace]);
}

if (propagateTraceparent && traceparent && !originalHeaders.find(header => header[0] === 'traceparent')) {
if (propagateTraceparent && traceparent && !newHeaders.find(header => header[0] === 'traceparent')) {
newHeaders.push(['traceparent', traceparent]);
}

const prevBaggageHeaderWithSentryValues = originalHeaders.find(
header => header[0] === 'baggage' && baggageHeaderHasSentryBaggageValues(header[1]),
header =>
header[0] === 'baggage' && typeof header[1] === 'string' && baggageHeaderHasSentryBaggageValues(header[1]),
);

if (baggage && !prevBaggageHeaderWithSentryValues) {
Expand All @@ -255,7 +261,7 @@
newHeaders.push(['baggage', baggage]);
}

return newHeaders as PolymorphicRequestHeaders;
return newHeaders;
} else {
const existingSentryTraceHeader = 'sentry-trace' in originalHeaders ? originalHeaders['sentry-trace'] : undefined;
const existingTraceparentHeader = 'traceparent' in originalHeaders ? originalHeaders.traceparent : undefined;
Expand All @@ -282,7 +288,7 @@
baggage: string | undefined;
traceparent?: string;
} = {
...originalHeaders,

Check warning on line 291 in packages/core/src/fetch.ts

View workflow job for this annotation

GitHub Actions / Lint

typescript-eslint(no-misused-spread)

Using the spread operator on an Iterable in an object can cause unexpected behavior.
'sentry-trace': (existingSentryTraceHeader as string | undefined) ?? sentryTrace,
baggage: newBaggageHeaders.length > 0 ? newBaggageHeaders.join(',') : undefined,
};
Expand Down Expand Up @@ -313,14 +319,29 @@
span.end();
}

function baggageHeaderHasSentryBaggageValues(baggageHeader: string): boolean {
function baggageHeaderHasSentryBaggageValues(baggageHeader: unknown): boolean {
if (typeof baggageHeader !== 'string') {
return false;
}

return baggageHeader.split(',').some(baggageEntry => baggageEntry.trim().startsWith(SENTRY_BAGGAGE_KEY_PREFIX));
}

function isHeaders(headers: unknown): headers is Headers {
return typeof Headers !== 'undefined' && isInstanceOf(headers, Headers);
}

/** `HeadersInit` array form: each entry is a [name, value] pair of strings. */
function isHeadersInitTupleArray(headers: unknown): headers is [string, unknown][] {
if (!Array.isArray(headers)) {
return false;
}

return headers.every(
(item): item is [string, unknown] => Array.isArray(item) && item.length === 2 && typeof item[0] === 'string',
);
}

function getSpanStartOptions(
url: string,
method: string,
Expand Down
2 changes: 1 addition & 1 deletion packages/core/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -142,7 +142,7 @@ export { profiler } from './profiling';
// eslint thinks the entire function is deprecated (while only one overload is actually deprecated)
// Therefore:
// eslint-disable-next-line deprecation/deprecation
export { instrumentFetchRequest } from './fetch';
export { instrumentFetchRequest, _INTERNAL_getTracingHeadersForFetchRequest } from './fetch';
export { trpcMiddleware } from './trpc';
export { wrapMcpServerWithSentry } from './integrations/mcp-server';
export { captureFeedback } from './feedback';
Expand Down
75 changes: 45 additions & 30 deletions packages/core/test/lib/fetch.test.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import { beforeEach, describe, expect, it, vi } from 'vitest';
import type { HandlerDataFetch } from '../../src';
import { _addTracingHeadersToFetchRequest, instrumentFetchRequest } from '../../src/fetch';
import { _INTERNAL_getTracingHeadersForFetchRequest, instrumentFetchRequest } from '../../src/fetch';
import type { Span } from '../../src/types-hoist/span';

const { DEFAULT_SENTRY_TRACE, DEFAULT_BAGGAGE, hasSpansEnabled } = vi.hoisted(() => ({
Expand Down Expand Up @@ -31,7 +31,7 @@ vi.mock('../../src/utils/hasSpansEnabled', () => {
};
});

describe('_addTracingHeadersToFetchRequest', () => {
describe('_INTERNAL_getTracingHeadersForFetchRequest', () => {
beforeEach(() => {
vi.clearAllMocks();
hasSpansEnabled.mockReturnValue(false);
Expand All @@ -47,7 +47,7 @@ describe('_addTracingHeadersToFetchRequest', () => {
options: { headers: {} },
},
])('attaches sentry headers (options: $options)', ({ options }) => {
expect(_addTracingHeadersToFetchRequest('/api/test', options)).toEqual({
expect(_INTERNAL_getTracingHeadersForFetchRequest('/api/test', options)).toEqual({
'sentry-trace': DEFAULT_SENTRY_TRACE,
baggage: DEFAULT_BAGGAGE,
});
Expand All @@ -56,17 +56,17 @@ describe('_addTracingHeadersToFetchRequest', () => {

describe('and request headers are set in options', () => {
it('attaches sentry headers to headers object', () => {
expect(_addTracingHeadersToFetchRequest('/api/test', { headers: { 'custom-header': 'custom-value' } })).toEqual(
{
'sentry-trace': DEFAULT_SENTRY_TRACE,
baggage: DEFAULT_BAGGAGE,
'custom-header': 'custom-value',
},
);
expect(
_INTERNAL_getTracingHeadersForFetchRequest('/api/test', { headers: { 'custom-header': 'custom-value' } }),
).toEqual({
'sentry-trace': DEFAULT_SENTRY_TRACE,
baggage: DEFAULT_BAGGAGE,
'custom-header': 'custom-value',
});
});

it('attaches sentry headers to a Headers instance', () => {
const returnedHeaders = _addTracingHeadersToFetchRequest('/api/test', {
const returnedHeaders = _INTERNAL_getTracingHeadersForFetchRequest('/api/test', {
headers: new Headers({ 'custom-header': 'custom-value' }),
});

Expand All @@ -81,7 +81,7 @@ describe('_addTracingHeadersToFetchRequest', () => {
});

it('attaches sentry headers to headers array', () => {
const returnedHeaders = _addTracingHeadersToFetchRequest('/api/test', {
const returnedHeaders = _INTERNAL_getTracingHeadersForFetchRequest('/api/test', {
headers: [['custom-header', 'custom-value']],
});

Expand All @@ -92,11 +92,26 @@ describe('_addTracingHeadersToFetchRequest', () => {
['baggage', DEFAULT_BAGGAGE],
]);
});

it('treats array with non-tuple items as headers object', () => {
const returnedHeaders = _INTERNAL_getTracingHeadersForFetchRequest('/api/test', {
headers: ['not-a-tuple', 'also-not-a-tuple'],
});

// Falls through to the else branch (headers object handling)
// since the array items are not [string, string] tuples
expect(returnedHeaders).toEqual({
'0': 'not-a-tuple',
'1': 'also-not-a-tuple',
'sentry-trace': DEFAULT_SENTRY_TRACE,
baggage: DEFAULT_BAGGAGE,
});
});
});

describe('and 3rd party baggage header is set', () => {
it('adds additional sentry baggage values to Headers instance', () => {
const returnedHeaders = _addTracingHeadersToFetchRequest('/api/test', {
const returnedHeaders = _INTERNAL_getTracingHeadersForFetchRequest('/api/test', {
headers: new Headers({
baggage: 'custom-baggage=1,someVal=bar',
}),
Expand All @@ -112,7 +127,7 @@ describe('_addTracingHeadersToFetchRequest', () => {
});

it('adds additional sentry baggage values to headers array', () => {
const returnedHeaders = _addTracingHeadersToFetchRequest('/api/test', {
const returnedHeaders = _INTERNAL_getTracingHeadersForFetchRequest('/api/test', {
headers: [['baggage', 'custom-baggage=1,someVal=bar']],
});

Expand All @@ -126,7 +141,7 @@ describe('_addTracingHeadersToFetchRequest', () => {
});

it('adds additional sentry baggage values to headers object', () => {
const returnedHeaders = _addTracingHeadersToFetchRequest('/api/test', {
const returnedHeaders = _INTERNAL_getTracingHeadersForFetchRequest('/api/test', {
headers: {
baggage: 'custom-baggage=1,someVal=bar',
},
Expand All @@ -141,7 +156,7 @@ describe('_addTracingHeadersToFetchRequest', () => {
});

it('adds additional sentry baggage values to headers object with arrays', () => {
const returnedHeaders = _addTracingHeadersToFetchRequest('/api/test', {
const returnedHeaders = _INTERNAL_getTracingHeadersForFetchRequest('/api/test', {
headers: {
baggage: ['custom-baggage=1,someVal=bar', 'other-vendor-key=value'],
},
Expand All @@ -158,7 +173,7 @@ describe('_addTracingHeadersToFetchRequest', () => {

describe('and Sentry values are already set', () => {
it('does not override them (Headers instance)', () => {
const returnedHeaders = _addTracingHeadersToFetchRequest('/api/test', {
const returnedHeaders = _INTERNAL_getTracingHeadersForFetchRequest('/api/test', {
headers: new Headers({
'sentry-trace': CUSTOM_SENTRY_TRACE,
baggage: CUSTOM_BAGGAGE,
Expand All @@ -177,7 +192,7 @@ describe('_addTracingHeadersToFetchRequest', () => {
});

it('does not override them (headers array)', () => {
const returnedHeaders = _addTracingHeadersToFetchRequest('/api/test', {
const returnedHeaders = _INTERNAL_getTracingHeadersForFetchRequest('/api/test', {
headers: [
['sentry-trace', CUSTOM_SENTRY_TRACE],
['baggage', CUSTOM_BAGGAGE],
Expand All @@ -195,7 +210,7 @@ describe('_addTracingHeadersToFetchRequest', () => {
});

it('does not override them (headers object)', () => {
const returnedHeaders = _addTracingHeadersToFetchRequest('/api/test', {
const returnedHeaders = _INTERNAL_getTracingHeadersForFetchRequest('/api/test', {
headers: {
'sentry-trace': CUSTOM_SENTRY_TRACE,
baggage: CUSTOM_BAGGAGE,
Expand All @@ -218,7 +233,7 @@ describe('_addTracingHeadersToFetchRequest', () => {
describe('and no request headers are set', () => {
it('attaches sentry headers', () => {
const request = new Request('http://locahlost:3000/api/test');
const returnedHeaders = _addTracingHeadersToFetchRequest(request, {});
const returnedHeaders = _INTERNAL_getTracingHeadersForFetchRequest(request, {});

expect(returnedHeaders).toBeInstanceOf(Headers);

Expand All @@ -236,7 +251,7 @@ describe('_addTracingHeadersToFetchRequest', () => {
headers: new Headers({ 'custom-header': 'custom-value' }),
});

const returnedHeaders = _addTracingHeadersToFetchRequest(request, {});
const returnedHeaders = _INTERNAL_getTracingHeadersForFetchRequest(request, {});

expect(returnedHeaders).toBeInstanceOf(Headers);

Expand All @@ -253,7 +268,7 @@ describe('_addTracingHeadersToFetchRequest', () => {
headers: { 'custom-header': 'custom-value' },
});

const returnedHeaders = _addTracingHeadersToFetchRequest(request, {});
const returnedHeaders = _INTERNAL_getTracingHeadersForFetchRequest(request, {});

expect(returnedHeaders).toBeInstanceOf(Headers);

Expand All @@ -270,7 +285,7 @@ describe('_addTracingHeadersToFetchRequest', () => {
headers: [['custom-header', 'custom-value']],
});

const returnedHeaders = _addTracingHeadersToFetchRequest(request, {});
const returnedHeaders = _INTERNAL_getTracingHeadersForFetchRequest(request, {});

expect(returnedHeaders).toBeInstanceOf(Headers);

Expand All @@ -292,7 +307,7 @@ describe('_addTracingHeadersToFetchRequest', () => {
}),
});

const returnedHeaders = _addTracingHeadersToFetchRequest(request, {});
const returnedHeaders = _INTERNAL_getTracingHeadersForFetchRequest(request, {});

expect(returnedHeaders).toBeInstanceOf(Headers);

Expand All @@ -309,7 +324,7 @@ describe('_addTracingHeadersToFetchRequest', () => {
headers: [['baggage', 'custom-baggage=1,someVal=bar']],
});

const returnedHeaders = _addTracingHeadersToFetchRequest(request, {});
const returnedHeaders = _INTERNAL_getTracingHeadersForFetchRequest(request, {});

expect(returnedHeaders).toBeInstanceOf(Headers);

Expand All @@ -327,7 +342,7 @@ describe('_addTracingHeadersToFetchRequest', () => {
},
});

const returnedHeaders = _addTracingHeadersToFetchRequest(request, {});
const returnedHeaders = _INTERNAL_getTracingHeadersForFetchRequest(request, {});

expect(returnedHeaders).toBeInstanceOf(Headers);

Expand All @@ -345,7 +360,7 @@ describe('_addTracingHeadersToFetchRequest', () => {
},
});

const returnedHeaders = _addTracingHeadersToFetchRequest(request, {});
const returnedHeaders = _INTERNAL_getTracingHeadersForFetchRequest(request, {});

expect(returnedHeaders).toBeInstanceOf(Headers);

Expand All @@ -367,7 +382,7 @@ describe('_addTracingHeadersToFetchRequest', () => {
}),
});

const returnedHeaders = _addTracingHeadersToFetchRequest(request, {});
const returnedHeaders = _INTERNAL_getTracingHeadersForFetchRequest(request, {});

expect(returnedHeaders).toBeInstanceOf(Headers);

Expand All @@ -388,7 +403,7 @@ describe('_addTracingHeadersToFetchRequest', () => {
],
});

const returnedHeaders = _addTracingHeadersToFetchRequest(request, {});
const returnedHeaders = _INTERNAL_getTracingHeadersForFetchRequest(request, {});

expect(returnedHeaders).toBeInstanceOf(Headers);

Expand All @@ -409,7 +424,7 @@ describe('_addTracingHeadersToFetchRequest', () => {
},
});

const returnedHeaders = _addTracingHeadersToFetchRequest(request, {});
const returnedHeaders = _INTERNAL_getTracingHeadersForFetchRequest(request, {});

expect(returnedHeaders).toBeInstanceOf(Headers);

Expand Down
Loading