fix(tracing): Make shouldAttachHeaders not fall back to default values (#6238)

This patch fixes a bug in our `shouldAttachHeaders` function that determines internally if outgoing requests should have headers attached or not. 

Previously, we assigned the default values to `tracingOrigins` as well as `tracePropagationTargets` and overwrote these defaults, if users provided one (or (unlikely) both) options. In `shouldAttachHeaders`, we then first tested `tracingOrigins` for a match and in case there was no match, we'd test `tracePropagationTargets`.  This, however, also meant that if users specified `tracingOrigins` and an URL didn't match them, there was still a chance that headers would be attached - in case the url matched the `tracePropagationTargets`' default values.

With this fix, we don't assign default values to either option but check for definedness of them and only if they're both undefined, well fall back to defaults. 

### Example:
url: `/api/test/123`
tracingOrigins: `[]`
tracePropTargets: `['localhost', /^\//]` // default values

`shouldAttachHeaders` returned `true` but should have returned `false`. This PR fixes this behaviour.

In case, both options are defined, we prefer `tracePropagationTargets` over `tracingOrigins`, as defined in the [dev specification](https://develop.sentry.dev/sdk/performance/#example).

Furthermore, this patch extracts the `shouldAttachHeaders` function by wrapping it in a factory function so that it's easier to test this behaviour. And it adds some tests to verify correct behaviour.
We can get rid of some code around this in v8 when we finally remove `tracingOrigins`.

Side-note: Because we're exporting `defaultRequestInstrumentationOptions` as public API, I decided to leave the optional value assignment in place but to simply not take the default values from it anymore in `instrumentOutgoingRequests` (see [aed40a6](aed40a6)).

Author: Lms24

packages/tracing/src/browser/request.ts

@@ -10,8 +10,6 @@ import {
 
 import { getActiveTransaction, hasTracingEnabled } from '../utils';
 
-// TODO (v8): Remove `tracingOrigins`
-export const DEFAULT_TRACING_ORIGINS = ['localhost', /^\//];
 export const DEFAULT_TRACE_PROPAGATION_TARGETS = ['localhost', /^\//];
 
 /** Options for Request Instrumentation */
@@ -107,39 +105,61 @@ type PolymorphicRequestHeaders =
 export const defaultRequestInstrumentationOptions: RequestInstrumentationOptions = {
   traceFetch: true,
   traceXHR: true,
-  tracingOrigins: DEFAULT_TRACING_ORIGINS,
+  // TODO (v8): Remove this property
+  tracingOrigins: DEFAULT_TRACE_PROPAGATION_TARGETS,
   tracePropagationTargets: DEFAULT_TRACE_PROPAGATION_TARGETS,
 };
 
 /** Registers span creators for xhr and fetch requests  */
 export function instrumentOutgoingRequests(_options?: Partial<RequestInstrumentationOptions>): void {
   // eslint-disable-next-line deprecation/deprecation
   const { traceFetch, traceXHR, tracingOrigins, tracePropagationTargets, shouldCreateSpanForRequest } = {
-    ...defaultRequestInstrumentationOptions,
+    traceFetch: defaultRequestInstrumentationOptions.traceFetch,
+    traceXHR: defaultRequestInstrumentationOptions.traceXHR,
     ..._options,
   };
 
   const shouldCreateSpan =
     typeof shouldCreateSpanForRequest === 'function' ? shouldCreateSpanForRequest : (_: string) => true;
 
-  const shouldAttachHeaders = (url: string): boolean =>
-    stringMatchesSomePattern(url, tracingOrigins) || stringMatchesSomePattern(url, tracePropagationTargets);
+  const shouldAttachHeadersWithTargets = (url: string): boolean =>
+    shouldAttachHeaders(url, tracingOrigins, tracePropagationTargets);
 
   const spans: Record<string, Span> = {};
 
   if (traceFetch) {
     addInstrumentationHandler('fetch', (handlerData: FetchData) => {
-      fetchCallback(handlerData, shouldCreateSpan, shouldAttachHeaders, spans);
+      fetchCallback(handlerData, shouldCreateSpan, shouldAttachHeadersWithTargets, spans);
     });
   }
 
   if (traceXHR) {
     addInstrumentationHandler('xhr', (handlerData: XHRData) => {
-      xhrCallback(handlerData, shouldCreateSpan, shouldAttachHeaders, spans);
+      xhrCallback(handlerData, shouldCreateSpan, shouldAttachHeadersWithTargets, spans);
     });
   }
 }
 
+/**
+ * A function that determines whether to attach tracing headers to a request.
+ * This was extracted from `instrumentOutgoingRequests` to make it easier to test shouldAttachHeaders.
+ * We only export this fuction for testing purposes.
+ *
+ * TODO (v8): Remove `tracingOrigins` which should drastically simplify this function.
+ */
+export function shouldAttachHeaders(
+  url: string,
+  tracePropagationTargets: (string | RegExp)[] | undefined,
+  tracingOrigins: (string | RegExp)[] | undefined,
+): boolean {
+  // TODO (v8): Replace the entire code below with this one-liner:
+  // return stringMatchesSomePattern(url, tracePropagationTargets || DEFAULT_TRACE_PROPAGATION_TARGETS);
+  if (tracePropagationTargets || tracingOrigins) {
+    return stringMatchesSomePattern(url, tracePropagationTargets || tracingOrigins);
+  }
+  return stringMatchesSomePattern(url, DEFAULT_TRACE_PROPAGATION_TARGETS);
+}
+
 /**
  * Create and track fetch request spans
  */

packages/tracing/test/browser/request.test.ts

@@ -3,7 +3,14 @@ import { Hub, makeMain } from '@sentry/core';
 import * as utils from '@sentry/utils';
 
 import { Span, spanStatusfromHttpCode, Transaction } from '../../src';
-import { fetchCallback, FetchData, instrumentOutgoingRequests, xhrCallback, XHRData } from '../../src/browser/request';
+import {
+  fetchCallback,
+  FetchData,
+  instrumentOutgoingRequests,
+  shouldAttachHeaders,
+  xhrCallback,
+  XHRData,
+} from '../../src/browser/request';
 import { addExtensionMethods } from '../../src/hubextensions';
 import * as tracingUtils from '../../src/utils';
 import { getDefaultBrowserClientOptions } from '../testutils';
@@ -383,3 +390,49 @@ describe('callbacks', () => {
     });
   });
 });
+
+// TODO (v8): Adapt these tests once we remove `tracingOrigins`
+describe('[pre-v8]: shouldAttachHeaders', () => {
+  describe('prefer `tracePropagationTargets` over `tracingOrigins`', () => {
+    it('should return `true` if the url matches the new tracePropagationTargets', () => {
+      expect(shouldAttachHeaders('http://example.com', ['example.com'], undefined)).toBe(true);
+    });
+
+    it('should return `false` if tracePropagationTargets array is empty', () => {
+      expect(shouldAttachHeaders('http://localhost:3000/test', [], ['localhost'])).toBe(false);
+    });
+
+    it("should return `false` if tracePropagationTargets array doesn't match", () => {
+      expect(shouldAttachHeaders('http://localhost:3000/test', ['example.com'], ['localhost'])).toBe(false);
+    });
+  });
+
+  describe('tracingOrigins backwards compatibility (tracePropagationTargets not defined)', () => {
+    it('should return `true` if the url matches tracingOrigns', () => {
+      expect(shouldAttachHeaders('http://example.com', undefined, ['example.com'])).toBe(true);
+    });
+
+    it('should return `false` if tracePropagationTargets array is empty', () => {
+      expect(shouldAttachHeaders('http://localhost:3000/test', undefined, [])).toBe(false);
+    });
+
+    it("should return `false` if tracePropagationTargets array doesn't match", () => {
+      expect(shouldAttachHeaders('http://localhost:3000/test', undefined, ['example.com'])).toBe(false);
+    });
+  });
+
+  describe('should fall back to defaults if no options are specified', () => {
+    it.each([
+      '/api/test',
+      'http://localhost:3000/test',
+      'http://somewhere.com/test/localhost/123',
+      'http://somewhere.com/test?url=localhost:3000&test=123',
+    ])('return `true` for urls matching defaults (%s)', url => {
+      expect(shouldAttachHeaders(url, undefined, undefined)).toBe(true);
+    });
+
+    it.each(['notmydoman/api/test', 'example.com'])('return `false` for urls not matching defaults (%s)', url => {
+      expect(shouldAttachHeaders(url, undefined, undefined)).toBe(false);
+    });
+  });
+});