feat: Update default trace propagation targets logic in the browser (#10621)

Author: Luca Forstner

MIGRATION.md

@@ -1,5 +1,33 @@
 # Upgrading from 7.x to 8.x
 
+## Updated behaviour of `tracePropagationTargets` in the browser (HTTP tracing headers & CORS)
+
+We updated the behaviour of the SDKs when no `tracePropagationTargets` option was defined. As a reminder, you can
+provide a list of strings or RegExes that will be matched against URLs to tell the SDK, to which outgoing requests
+tracing HTTP headers should be attached to. These tracing headers are used for distributed tracing.
+
+Previously, on the browser, when `tracePropagationTargets` were not defined, they defaulted to the following:
+`['localhost', /^\/(?!\/)/]`. This meant that all request targets to that had "localhost" in the URL, or started with a
+`/` were equipped with tracing headers. This default was chosen to prevent CORS errors in your browser applications.
+However, this default had a few flaws.
+
+Going forward, when the `tracePropagationTargets` option is not set, tracing headers will be attached to all outgoing
+requests on the same origin. For example, if you're on `https://example.com/` and you send a request to
+`https://example.com/api`, the request will be traced (ie. will have trace headers attached). Requests to
+`https://api.example.com/` will not, because it is on a different origin. The same goes for all applications running on
+`localhost`.
+
+When you provide a `tracePropagationTargets` option, all of the entries you defined will now be matched be matched
+against the full URL of the outgoing request. Previously, it was only matched against what you called request APIs with.
+For example, if you made a request like `fetch("/api/posts")`, the provided `tracePropagationTargets` were only compared
+against `"/api/posts"`. Going forward they will be matched against the entire URL, for example, if you were on the page
+`https://example.com/` and you made the same request, it would be matched against `"https://example.com/api/posts"`.
+
+But that is not all. Because it would be annoying having to create matchers for the entire URL, if the request is a
+same-origin request, we also match the `tracePropagationTargets` against the resolved `pathname` of the request.
+Meaning, a matcher like `/^\/api/` would match a request call like `fetch('/api/posts')`, or
+`fetch('https://same-origin.com/api/posts')` but not `fetch('https://different-origin.com/api/posts')`.
+
 ## Removal of the `tracingOrigins` option
 
 After its deprecation in v7 the `tracingOrigins` option is now removed in favor of the `tracePropagationTargets` option.

dev-packages/browser-integration-tests/suites/tracing/browserTracingIntegration/tracePropagationTargets/defaultTargetsMatch/subject.js

@@ -1 +1 @@
-fetch('http://localhost:4200/0').then(fetch('http://localhost:4200/1').then(fetch('http://localhost:4200/2')));
+fetch(`/0`).then(fetch('/1').then(fetch('/2')));

dev-packages/browser-integration-tests/suites/tracing/browserTracingIntegration/tracePropagationTargets/defaultTargetsMatch/test.ts

@@ -4,19 +4,16 @@ import { sentryTest } from '../../../../../utils/fixtures';
 import { shouldSkipTracingTest } from '../../../../../utils/helpers';
 
 sentryTest(
-  'should attach `sentry-trace` and `baggage` header to request matching default tracePropagationTargets',
-  async ({ getLocalTestPath, page }) => {
+  'should attach `sentry-trace` and `baggage` header to same-origin requests when no tracePropagationTargets are defined',
+  async ({ getLocalTestUrl, page }) => {
     if (shouldSkipTracingTest()) {
       sentryTest.skip();
     }
 
-    const url = await getLocalTestPath({ testDir: __dirname });
+    const url = await getLocalTestUrl({ testDir: __dirname });
 
     const requests = (
-      await Promise.all([
-        page.goto(url),
-        Promise.all([0, 1, 2].map(idx => page.waitForRequest(`http://localhost:4200/${idx}`))),
-      ])
+      await Promise.all([page.goto(url), Promise.all([0, 1, 2].map(idx => page.waitForRequest(`**/${idx}`)))])
     )[1];
 
     expect(requests).toHaveLength(3);

dev-packages/browser-integration-tests/suites/tracing/browserTracingIntegration/tracePropagationTargets/defaultTargetsNoMatch/test.ts

@@ -4,7 +4,7 @@ import { sentryTest } from '../../../../../utils/fixtures';
 import { shouldSkipTracingTest } from '../../../../../utils/helpers';
 
 sentryTest(
-  'should not attach `sentry-trace` and `baggage` header to request not matching default tracePropagationTargets',
+  'should not attach `sentry-trace` and `baggage` header to cross-origin requests when no tracePropagationTargets are defined',
   async ({ getLocalTestPath, page }) => {
     if (shouldSkipTracingTest()) {
       sentryTest.skip();

dev-packages/browser-integration-tests/suites/tracing/browsertracing/tracePropagationTargets/defaultTargetsMatch/subject.js

@@ -1 +1 @@
-fetch('http://localhost:4200/0').then(fetch('http://localhost:4200/1').then(fetch('http://localhost:4200/2')));
+fetch('/0').then(fetch('/1').then(fetch('/2')));

dev-packages/browser-integration-tests/suites/tracing/browsertracing/tracePropagationTargets/defaultTargetsMatch/test.ts

@@ -4,19 +4,16 @@ import { sentryTest } from '../../../../../utils/fixtures';
 import { shouldSkipTracingTest } from '../../../../../utils/helpers';
 
 sentryTest(
-  'should attach `sentry-trace` and `baggage` header to request matching default tracePropagationTargets',
-  async ({ getLocalTestPath, page }) => {
+  'should attach `sentry-trace` and `baggage` header to same-origin requests when no tracePropagationTargets are defined',
+  async ({ getLocalTestUrl, page }) => {
     if (shouldSkipTracingTest()) {
       sentryTest.skip();
     }
 
-    const url = await getLocalTestPath({ testDir: __dirname });
+    const url = await getLocalTestUrl({ testDir: __dirname });
 
     const requests = (
-      await Promise.all([
-        page.goto(url),
-        Promise.all([0, 1, 2].map(idx => page.waitForRequest(`http://localhost:4200/${idx}`))),
-      ])
+      await Promise.all([page.goto(url), Promise.all([0, 1, 2].map(idx => page.waitForRequest(`**/${idx}`)))])
     )[1];
 
     expect(requests).toHaveLength(3);

dev-packages/browser-integration-tests/suites/tracing/browsertracing/tracePropagationTargets/defaultTargetsNoMatch/init.js

@@ -1,10 +1,9 @@
 import * as Sentry from '@sentry/browser';
-import { Integrations } from '@sentry/tracing';
 
 window.Sentry = Sentry;
 
 Sentry.init({
   dsn: 'https://[email protected]/1337',
-  integrations: [new Integrations.BrowserTracing()],
+  integrations: [Sentry.browserTracingIntegration()],
   tracesSampleRate: 1,
 });

dev-packages/browser-integration-tests/suites/tracing/browsertracing/tracePropagationTargets/defaultTargetsNoMatch/test.ts

@@ -4,19 +4,16 @@ import { sentryTest } from '../../../../../utils/fixtures';
 import { shouldSkipTracingTest } from '../../../../../utils/helpers';
 
 sentryTest(
-  'should not attach `sentry-trace` and `baggage` header to request not matching default tracePropagationTargets',
-  async ({ getLocalTestPath, page }) => {
+  'should not attach `sentry-trace` and `baggage` header to cross-origin requests when no tracePropagationTargets are defined',
+  async ({ getLocalTestUrl, page }) => {
     if (shouldSkipTracingTest()) {
       sentryTest.skip();
     }
 
-    const url = await getLocalTestPath({ testDir: __dirname });
+    const url = await getLocalTestUrl({ testDir: __dirname });
 
     const requests = (
-      await Promise.all([
-        page.goto(url),
-        Promise.all([0, 1, 2].map(idx => page.waitForRequest(`http://example.com/${idx}`))),
-      ])
+      await Promise.all([page.goto(url), Promise.all([0, 1, 2].map(idx => page.waitForRequest(`**/${idx}`)))])
     )[1];
 
     expect(requests).toHaveLength(3);

packages/browser/src/index.ts

@@ -62,7 +62,6 @@ export {
   browserTracingIntegration,
   startBrowserTracingNavigationSpan,
   startBrowserTracingPageLoadSpan,
-  DEFAULT_TRACE_PROPAGATION_TARGETS,
 } from '@sentry-internal/tracing';
 export type { RequestInstrumentationOptions } from '@sentry-internal/tracing';
 export {

packages/nextjs/src/client/index.ts

@@ -1,10 +1,6 @@
 import { addEventProcessor, applySdkMetadata, hasTracingEnabled, setTag } from '@sentry/core';
 import type { BrowserOptions } from '@sentry/react';
-import {
-  DEFAULT_TRACE_PROPAGATION_TARGETS,
-  getDefaultIntegrations as getReactDefaultIntegrations,
-  init as reactInit,
-} from '@sentry/react';
+import { getDefaultIntegrations as getReactDefaultIntegrations, init as reactInit } from '@sentry/react';
 import type { EventProcessor, Integration } from '@sentry/types';
 
 import { devErrorSymbolicationEventProcessor } from '../common/devErrorSymbolicationEventProcessor';
@@ -28,17 +24,6 @@ declare const __SENTRY_TRACING__: boolean;
 export function init(options: BrowserOptions): void {
   const opts = {
     environment: getVercelEnv(true) || process.env.NODE_ENV,
-
-    tracePropagationTargets:
-      process.env.NODE_ENV === 'development'
-        ? [
-            // Will match any URL that contains "localhost" but not "webpack.hot-update.json" - The webpack dev-server
-            // has cors and it doesn't like extra headers when it's accessed from a different URL.
-            // TODO(v8): Ideally we rework our tracePropagationTargets logic so this hack won't be necessary anymore (see issue #9764)
-            /^(?=.*localhost)(?!.*webpack\.hot-update\.json).*/,
-            /^\/(?!\/)/,
-          ]
-        : [...DEFAULT_TRACE_PROPAGATION_TARGETS, /^(api\/)/],
     defaultIntegrations: getDefaultIntegrations(options),
     ...options,
   } satisfies BrowserOptions;

packages/tracing-internal/src/browser/request.ts

@@ -1,4 +1,3 @@
-/* eslint-disable max-lines */
 import {
   SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN,
   getClient,
@@ -26,18 +25,38 @@ import {
 
 import { instrumentFetchRequest } from '../common/fetch';
 import { addPerformanceInstrumentationHandler } from './instrument';
-
-export const DEFAULT_TRACE_PROPAGATION_TARGETS = ['localhost', /^\/(?!\/)/];
+import { WINDOW } from './types';
 
 /** Options for Request Instrumentation */
 export interface RequestInstrumentationOptions {
   /**
    * List of strings and/or Regular Expressions used to determine which outgoing requests will have `sentry-trace` and `baggage`
    * headers attached.
    *
-   * Default: ['localhost', /^\//]
+   * **Default:** If this option is not provided, tracing headers will be attached to all outgoing requests.
+   * If you are using a browser SDK, by default, tracing headers will only be attached to outgoing requests to the same origin.
+   *
+   * **Disclaimer:** Carelessly setting this option in browser environments may result into CORS errors!
+   * Only attach tracing headers to requests to the same origin, or to requests to services you can control CORS headers of.
+   * Cross-origin requests, meaning requests to a different domain, for example a request to `https://api.example.com/` while you're on `https://example.com/`, take special care.
+   * If you are attaching headers to cross-origin requests, make sure the backend handling the request returns a `"Access-Control-Allow-Headers: sentry-trace, baggage"` header to ensure your requests aren't blocked.
+   *
+   * If you provide a `tracePropagationTargets` array, the entries you provide will be matched against the entire URL of the outgoing request.
+   * If you are using a browser SDK, the entries will also be matched against the pathname of the outgoing requests.
+   * This is so you can have matchers for relative requests, for example, `/^\/api/` if you want to trace requests to your `/api` routes on the same domain.
+   *
+   * If any of the two match any of the provided values, tracing headers will be attached to the outgoing request.
+   * Both, the string values, and the RegExes you provide in the array will match if they partially match the URL or pathname.
+   *
+   * Examples:
+   * - `tracePropagationTargets: [/^\/api/]` and request to `https://same-origin.com/api/posts`:
+   *   - Tracing headers will be attached because the request is sent to the same origin and the regex matches the pathname "/api/posts".
+   * - `tracePropagationTargets: [/^\/api/]` and request to `https://different-origin.com/api/posts`:
+   *   - Tracing headers will not be attached because the pathname will only be compared when the request target lives on the same origin.
+   * - `tracePropagationTargets: [/^\/api/, 'https://external-api.com']` and request to `https://external-api.com/v1/data`:
+   *   - Tracing headers will be attached because the request URL matches the string `'https://external-api.com'`.
    */
-  tracePropagationTargets: Array<string | RegExp>;
+  tracePropagationTargets?: Array<string | RegExp>;
 
   /**
    * Flag to disable patching all together for fetch requests.
@@ -73,7 +92,6 @@ export const defaultRequestInstrumentationOptions: RequestInstrumentationOptions
   traceFetch: true,
   traceXHR: true,
   enableHTTPTimings: true,
-  tracePropagationTargets: DEFAULT_TRACE_PROPAGATION_TARGETS,
 };
 
 /** Registers span creators for xhr and fetch requests  */
@@ -207,19 +225,55 @@ function resourceTimingEntryToSpanData(resourceTiming: PerformanceResourceTiming
 
 /**
  * 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.
+ * We only export this function for testing purposes.
  */
-export function shouldAttachHeaders(url: string, tracePropagationTargets: (string | RegExp)[] | undefined): boolean {
-  return stringMatchesSomePattern(url, tracePropagationTargets || DEFAULT_TRACE_PROPAGATION_TARGETS);
+export function shouldAttachHeaders(
+  targetUrl: string,
+  tracePropagationTargets: (string | RegExp)[] | undefined,
+): boolean {
+  // window.location.href not being defined is an edge case in the browser but we need to handle it.
+  // Potentially dangerous situations where it may not be defined: Browser Extensions, Web Workers, patching of the location obj
+  const href: string | undefined = WINDOW.location && WINDOW.location.href;
+
+  if (!href) {
+    // If there is no window.location.origin, we default to only attaching tracing headers to relative requests, i.e. ones that start with `/`
+    // BIG DISCLAIMER: Users can call URLs with a double slash (fetch("//example.com/api")), this is a shorthand for "send to the same protocol",
+    // so we need a to exclude those requests, because they might be cross origin.
+    const isRelativeSameOriginRequest = !!targetUrl.match(/^\/(?!\/)/);
+    if (!tracePropagationTargets) {
+      return isRelativeSameOriginRequest;
+    } else {
+      return stringMatchesSomePattern(targetUrl, tracePropagationTargets);
+    }
+  } else {
+    let resolvedUrl;
+    let currentOrigin;
+
+    // URL parsing may fail, we default to not attaching trace headers in that case.
+    try {
+      resolvedUrl = new URL(targetUrl, href);
+      currentOrigin = new URL(href).origin;
+    } catch (e) {
+      return false;
+    }
+
+    const isSameOriginRequest = resolvedUrl.origin === currentOrigin;
+    if (!tracePropagationTargets) {
+      return isSameOriginRequest;
+    } else {
+      return (
+        stringMatchesSomePattern(resolvedUrl.toString(), tracePropagationTargets) ||
+        (isSameOriginRequest && stringMatchesSomePattern(resolvedUrl.pathname, tracePropagationTargets))
+      );
+    }
+  }
 }
 
 /**
  * Create and track xhr request spans
  *
  * @returns Span if a span was created, otherwise void.
  */
-// eslint-disable-next-line complexity
 export function xhrCallback(
   handlerData: HandlerDataXhr,
   shouldCreateSpan: (url: string) => boolean,

packages/tracing-internal/src/index.ts

@@ -32,5 +32,3 @@ export { addTracingHeadersToFetchRequest, instrumentFetchRequest } from './commo
 export type { RequestInstrumentationOptions } from './browser';
 
 export { addExtensionMethods } from './extensions';
-
-export { DEFAULT_TRACE_PROPAGATION_TARGETS } from './browser/request';