more cleanup, fix edge case with odd headers, dedupe traceparent

Lms24 · Lms24 · commit 4caafd88d693 · 2026-03-30T12:29:36.000+02:00
diff --git a/packages/node-core/src/utils/outgoingFetchRequest.ts b/packages/node-core/src/utils/outgoingFetchRequest.ts
@@ -13,7 +13,7 @@ import { mergeBaggageHeaders } from './baggage';
 
 const SENTRY_TRACE_HEADER = 'sentry-trace';
 const SENTRY_BAGGAGE_HEADER = 'baggage';
-
+const W3C_TRACEPARENT_HEADER = 'traceparent';
 /**
  * Add trace propagation headers to an outgoing fetch/undici request.
  *
@@ -46,14 +46,18 @@ export function addTracePropagationHeadersToFetchRequest(
 
   // OTel's UndiciInstrumentation calls propagation.inject() which unconditionally
   // appends headers to the request. When the user also sets headers via getTraceData(),
-  // this results in duplicate sentry-trace and baggage entries.
+  // this results in duplicate sentry-trace and baggage (and optionally traceparent) entries.
   // We clean these up before applying our own logic.
-  _deduplicateArrayHeaders(requestHeaders);
+  _deduplicateArrayHeader(requestHeaders, SENTRY_TRACE_HEADER);
+  _deduplicateArrayHeader(requestHeaders, SENTRY_BAGGAGE_HEADER);
+  if (propagateTraceparent) {
+    _deduplicateArrayHeader(requestHeaders, W3C_TRACEPARENT_HEADER);
+  }
 
   // We do not want to overwrite existing headers here
   // If the core UndiciInstrumentation is registered, it will already have set the headers
   // We do not want to add any then
-  const hasExistingSentryTraceHeader = requestHeaders.includes(SENTRY_TRACE_HEADER);
+  const hasExistingSentryTraceHeader = _findExistingHeaderIndex(requestHeaders, SENTRY_TRACE_HEADER) !== -1;
 
   // We do not want to set any headers if we already have an existing sentry-trace header.
   // sentry-trace is still the source of truth, otherwise we risk mixing up baggage and sentry-trace values.
@@ -62,20 +66,20 @@ export function addTracePropagationHeadersToFetchRequest(
       requestHeaders.push(SENTRY_TRACE_HEADER, sentryTrace);
     }
 
-    if (traceparent && !requestHeaders.includes('traceparent')) {
+    if (traceparent && _findExistingHeaderIndex(requestHeaders, 'traceparent') === -1) {
       requestHeaders.push('traceparent', traceparent);
     }
 
     // For baggage, we make sure to merge this into a possibly existing header
-    const existingBaggagePos = requestHeaders.findIndex(header => header === SENTRY_BAGGAGE_HEADER);
-    if (baggage && existingBaggagePos === -1) {
+    const existingBaggageIndex = _findExistingHeaderIndex(requestHeaders, SENTRY_BAGGAGE_HEADER);
+    if (baggage && existingBaggageIndex === -1) {
       requestHeaders.push(SENTRY_BAGGAGE_HEADER, baggage);
     } else if (baggage) {
       // headers in format [key_0, value_0, key_1, value_1, ...], hence the +1 here
-      const existingBaggage = requestHeaders[existingBaggagePos + 1];
-      const merged = mergeBaggageHeaders(existingBaggage, baggage);
+      const existingBaggageValue = requestHeaders[existingBaggageIndex + 1];
+      const merged = mergeBaggageHeaders(existingBaggageValue, baggage);
       if (merged) {
-        requestHeaders[existingBaggagePos + 1] = merged;
+        requestHeaders[existingBaggageIndex + 1] = merged;
       }
     }
   }
@@ -108,11 +112,15 @@ function stringToArrayHeaders(requestHeaders: string): string[] {
 function arrayToStringHeaders(headers: string[]): string {
   const headerPairs: string[] = [];
 
-  try {
-    for (let i = 0; i < headers.length; i += 2) {
-      headerPairs.push(`${headers[i]}: ${headers[i + 1]}`);
+  for (let i = 0; i < headers.length; i += 2) {
+    const key = headers[i];
+    const value = headers[i + 1];
+    if (!key || value == null) {
+      // skip falsy keys but only null/undefined values
+      continue;
     }
-  } catch {}
+    headerPairs.push(`${key}: ${value}`);
+  }
 
   if (!headerPairs.length) {
     return '';
@@ -121,21 +129,6 @@ function arrayToStringHeaders(headers: string[]): string {
   return headerPairs.join('\r\n').concat('\r\n');
 }
 
-/**
- * Remove duplicate sentry-trace and baggage headers from the request.
- *
- * OTel's UndiciInstrumentation unconditionally appends headers via propagation.inject(),
- * which can create duplicates when the user has already set these headers (e.g. via getTraceData()).
- * For sentry-trace, we keep the first occurrence (user-set).
- * For baggage, we merge all occurrences into one header to preserve non-sentry entries. For Sentry
- * entries, we keep the first occurrence.
- */
-
-function _deduplicateArrayHeaders(headers: string[]): void {
-  _deduplicateArrayHeader(headers, SENTRY_TRACE_HEADER);
-  _deduplicateArrayHeader(headers, SENTRY_BAGGAGE_HEADER);
-}
-
 /**
  * For a given header name, if there are multiple entries in the [key, value, key, value, ...] array,
  * keep the first entry and remove the rest.
@@ -167,6 +160,15 @@ function _deduplicateArrayHeader(headers: string[], headerName: string): void {
   }
 }
 
+/**
+ * Find the index of an existing header in an array of headers.
+ * Only take even indices, because headers are in format [key_0, value_0, key_1, value_1, ...]
+ * otherwise we could match a header _value_ with @param name
+ */
+function _findExistingHeaderIndex(headers: string[], name: string): number {
+  return headers.findIndex((header, i) => i % 2 === 0 && header === name);
+}
+
 /** Add a breadcrumb for an outgoing fetch/undici request. */
 export function addFetchRequestBreadcrumb(request: UndiciRequest, response: UndiciResponse): void {
   const data = getBreadcrumbData(request);
diff --git a/packages/node-core/test/utils/outgoingFetchRequest.test.ts b/packages/node-core/test/utils/outgoingFetchRequest.test.ts
@@ -1,24 +1,30 @@
+import type { MockedFunction } from 'vitest';
 import { describe, beforeEach, vi, expect, it } from 'vitest';
 import type { UndiciRequest } from '../../src/integrations/node-fetch/types';
 import { addTracePropagationHeadersToFetchRequest } from '../../src/utils/outgoingFetchRequest';
 import { LRUMap } from '@sentry/core';
 import * as SentryCore from '@sentry/core';
 
-const mockedGetTraceData = vi.hoisted(() =>
+const mockedGetTraceData: MockedFunction<() => ReturnType<typeof SentryCore.getTraceData>> = vi.hoisted(() =>
   vi.fn(() => ({
     'sentry-trace': 'trace_id_1-span_id_1-1',
     baggage: 'sentry-trace_id=trace_id_1,sentry-sampled=true,sentry-environment=staging',
   })),
 );
 
+const mockedClientGetOptions: MockedFunction<() => Partial<SentryCore.ClientOptions>> = vi.hoisted(() =>
+  vi.fn(() => ({
+    tracePropagationTargets: ['https://example.com'],
+    propagateTraceparent: true,
+  })),
+);
+
 vi.mock('@sentry/core', async () => {
   const actual = await vi.importActual('@sentry/core');
   return {
     ...actual,
     getClient: vi.fn(() => ({
-      getOptions: vi.fn(() => ({
-        tracePropagationTargets: ['https://example.com'],
-      })),
+      getOptions: mockedClientGetOptions,
     })),
     shouldPropagateTraceForUrl: () => true,
     getTraceData: mockedGetTraceData,
@@ -62,6 +68,31 @@ describe('addTracePropagationHeadersToFetchRequest', () => {
       ]);
     });
 
+    it('adds sentry-trace, baggage and traceparent headers to request', () => {
+      mockedGetTraceData.mockReturnValueOnce({
+        'sentry-trace': 'trace_id_1-span_id_1-1',
+        baggage: 'sentry-trace_id=trace_id_1,sentry-sampled=true,sentry-environment=staging',
+        traceparent: '00-trace_id_1-span_id_1-01',
+      });
+
+      const request = {
+        headers: [] as string[],
+        origin: 'https://some-service.com',
+        path: '/api/test',
+      } as UndiciRequest;
+
+      addTracePropagationHeadersToFetchRequest(request, new LRUMap<string, boolean>(100));
+
+      expect(request.headers).toEqual([
+        'sentry-trace',
+        'trace_id_1-span_id_1-1',
+        'traceparent',
+        '00-trace_id_1-span_id_1-01',
+        'baggage',
+        'sentry-trace_id=trace_id_1,sentry-sampled=true,sentry-environment=staging',
+      ]);
+    });
+
     it('preserves non-sentry entries in existing baggage header', () => {
       const request = {
         headers: ['baggage', 'other=entry,not=sentry'],
@@ -79,6 +110,31 @@ describe('addTracePropagationHeadersToFetchRequest', () => {
       ]);
     });
 
+    it('preserves pre-existing traceparent header', () => {
+      mockedGetTraceData.mockReturnValueOnce({
+        'sentry-trace': 'trace_id_1-span_id_1-1',
+        baggage: 'sentry-trace_id=trace_id_1,sentry-sampled=true,sentry-environment=staging',
+        traceparent: '00-trace_id_1-span_id_1-01',
+      });
+
+      const request = {
+        headers: ['traceparent', '00-some-other-trace_id-span_id_x-01'],
+        origin: 'https://some-service.com',
+        path: '/api/test',
+      } as UndiciRequest;
+
+      addTracePropagationHeadersToFetchRequest(request, new LRUMap<string, boolean>(100));
+
+      expect(request.headers).toEqual([
+        'traceparent',
+        '00-some-other-trace_id-span_id_x-01',
+        'sentry-trace',
+        'trace_id_1-span_id_1-1',
+        'baggage',
+        'sentry-trace_id=trace_id_1,sentry-sampled=true,sentry-environment=staging',
+      ]);
+    });
+
     describe('when sentry-trace is already set', () => {
       it("preserves original sentry-trace header doesn't add baggage", () => {
         const request = {
@@ -113,6 +169,24 @@ describe('addTracePropagationHeadersToFetchRequest', () => {
           'sentry-trace_id=trace_id_2,sentry-sampled=true,sentry-environment=staging',
         ]);
       });
+
+      it("doesn't add traceparent header even if propagateTraceparent is true", () => {
+        mockedGetTraceData.mockReturnValueOnce({
+          'sentry-trace': 'trace_id_2-span_id_2-1',
+          baggage: 'sentry-trace_id=trace_id_2,sentry-sampled=true,sentry-environment=staging',
+          traceparent: '00-trace_id_2-span_id_2-01',
+        });
+
+        const request = {
+          headers: ['sentry-trace', 'trace_id_2-span_id_2-1'],
+          origin: 'https://some-service.com',
+          path: '/api/test',
+        } as UndiciRequest;
+
+        addTracePropagationHeadersToFetchRequest(request, new LRUMap<string, boolean>(100));
+
+        expect(request.headers).toEqual(['sentry-trace', 'trace_id_2-span_id_2-1']);
+      });
     });
 
     describe('pre-existing header deduplication', () => {
@@ -142,6 +216,43 @@ describe('addTracePropagationHeadersToFetchRequest', () => {
         ]);
       });
 
+      it('deduplicates traceparent headers if propagateTraceparent is true', () => {
+        mockedClientGetOptions.mockReturnValueOnce({
+          tracePropagationTargets: ['https://example.com'],
+          propagateTraceparent: true,
+        });
+
+        const request = {
+          headers: [
+            'sentry-trace',
+            'user-trace_id-xyz-1',
+            'baggage',
+            'sentry-trace_id=user-trace_id-xyz-1,sentry-sampled=true,sentry-environment=user',
+            'traceparent',
+            '00-user-trace_id-xyz-1-01',
+            'sentry-trace',
+            'undici-trace_id-abc-1',
+            'baggage',
+            'sentry-trace_id=undici-trace_id-abc-1,sentry-sampled=true,sentry-environment=undici',
+            'traceparent',
+            '00-undici-trace_id-abc-1-01',
+          ],
+          origin: 'https://some-service.com',
+          path: '/api/test',
+        } as UndiciRequest;
+
+        addTracePropagationHeadersToFetchRequest(request, new LRUMap<string, boolean>(100));
+
+        expect(request.headers).toEqual([
+          'sentry-trace',
+          'user-trace_id-xyz-1',
+          'baggage',
+          'sentry-trace_id=user-trace_id-xyz-1,sentry-sampled=true,sentry-environment=user',
+          'traceparent',
+          '00-user-trace_id-xyz-1-01',
+        ]);
+      });
+
       // admittedly an unrealistic edge case but doesn't hurt to test it
       it("doesn't crash with incomplete headers array", () => {
         const request = {
@@ -215,6 +326,44 @@ describe('addTracePropagationHeadersToFetchRequest', () => {
         ]);
       });
     });
+
+    it('doesn\'t mistake a header value with "sentry-trace" for a sentry-trace header', () => {
+      const request = {
+        headers: ['x-allow-header', 'sentry-trace'],
+        origin: 'https://some-service.com',
+        path: '/api/test',
+      } as UndiciRequest;
+
+      addTracePropagationHeadersToFetchRequest(request, new LRUMap<string, boolean>(100));
+
+      expect(request.headers).toEqual([
+        'x-allow-header',
+        'sentry-trace',
+        'sentry-trace',
+        'trace_id_1-span_id_1-1',
+        'baggage',
+        'sentry-trace_id=trace_id_1,sentry-sampled=true,sentry-environment=staging',
+      ]);
+    });
+
+    it('doesn\'t mistake a header value with "baggage" for a sentry-trace header', () => {
+      const request = {
+        headers: ['x-allow-header', 'baggage'],
+        origin: 'https://some-service.com',
+        path: '/api/test',
+      } as UndiciRequest;
+
+      addTracePropagationHeadersToFetchRequest(request, new LRUMap<string, boolean>(100));
+
+      expect(request.headers).toEqual([
+        'x-allow-header',
+        'baggage',
+        'sentry-trace',
+        'trace_id_1-span_id_1-1',
+        'baggage',
+        'sentry-trace_id=trace_id_1,sentry-sampled=true,sentry-environment=staging',
+      ]);
+    });
   });
 
   describe('when headers are a string', () => {
