From c546267dd2c9e3a49345ac56232ec20c3fc07db7 Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Mon, 12 Feb 2024 15:54:29 +0000 Subject: [PATCH 1/7] feat: Update default trace propagation targets logic --- packages/browser/src/index.ts | 1 - packages/nextjs/src/client/index.ts | 12 ------ .../tracing-internal/src/browser/request.ts | 43 +++++++++++++++---- packages/tracing-internal/src/index.ts | 2 - packages/types/src/options.ts | 30 +++++++------ 5 files changed, 52 insertions(+), 36 deletions(-) diff --git a/packages/browser/src/index.ts b/packages/browser/src/index.ts index 4e9369707eca..15c5adee2cc4 100644 --- a/packages/browser/src/index.ts +++ b/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 { diff --git a/packages/nextjs/src/client/index.ts b/packages/nextjs/src/client/index.ts index 5737816ba873..da1c3631ecd8 100644 --- a/packages/nextjs/src/client/index.ts +++ b/packages/nextjs/src/client/index.ts @@ -1,7 +1,6 @@ import { applySdkMetadata, hasTracingEnabled } from '@sentry/core'; import type { BrowserOptions } from '@sentry/react'; import { - DEFAULT_TRACE_PROPAGATION_TARGETS, getCurrentScope, getDefaultIntegrations as getReactDefaultIntegrations, init as reactInit, @@ -29,17 +28,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; diff --git a/packages/tracing-internal/src/browser/request.ts b/packages/tracing-internal/src/browser/request.ts index e722f4be6a85..7a01cc7c4bb5 100644 --- a/packages/tracing-internal/src/browser/request.ts +++ b/packages/tracing-internal/src/browser/request.ts @@ -26,8 +26,7 @@ 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 { @@ -35,9 +34,25 @@ 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', /^\//] + * By default, if this option is not provided, tracing headers will be attached to all outgoing requests to the same origin as the current page. + * + * NOTE: Carelessly setting this option may result into CORS errors. + * + * If you provide a `tracePropagationTargets` array, the entries you provide will be matched against two values: + * - The entire URL of the outgoing request. + * - The pathname of the outgoing request (only if it is a same-origin request) + * + * If any of the two match any of the provided values, tracing headers will be attached to the outgoing request. + * + * 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; + tracePropagationTargets?: Array; /** * Flag to disable patching all together for fetch requests. @@ -73,7 +88,6 @@ export const defaultRequestInstrumentationOptions: RequestInstrumentationOptions traceFetch: true, traceXHR: true, enableHTTPTimings: true, - tracePropagationTargets: DEFAULT_TRACE_PROPAGATION_TARGETS, }; /** Registers span creators for xhr and fetch requests */ @@ -208,10 +222,23 @@ 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 { + const resolvedUrl = new URL(targetUrl, WINDOW.location.origin); + const isSameOriginRequest = resolvedUrl.origin === WINDOW.location.origin; + + if (!tracePropagationTargets) { + return isSameOriginRequest; + } + + return ( + stringMatchesSomePattern(resolvedUrl.toString(), tracePropagationTargets) || + (isSameOriginRequest && stringMatchesSomePattern(resolvedUrl.pathname, tracePropagationTargets)) + ); } /** diff --git a/packages/tracing-internal/src/index.ts b/packages/tracing-internal/src/index.ts index 47a26c8a4d92..4f09ca6a2e96 100644 --- a/packages/tracing-internal/src/index.ts +++ b/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'; diff --git a/packages/types/src/options.ts b/packages/types/src/options.ts index 74bcfad771f4..4a08679fde4d 100644 --- a/packages/types/src/options.ts +++ b/packages/types/src/options.ts @@ -223,22 +223,26 @@ export interface ClientOptions; /** - * List of strings/regex controlling to which outgoing requests - * the SDK will attach tracing headers. + * List of strings and/or Regular Expressions used to determine which outgoing requests will have `sentry-trace` and `baggage` + * headers attached. * - * By default the SDK will attach those headers to all requests to localhost - * and same origin. If this option is provided, the SDK will match the - * request URL of outgoing requests against the items in this - * array, and only attach tracing headers if a match was found. + * By default, if this option is not provided, tracing headers will be attached to all outgoing requests to the same origin as the current page. * - * @example - * ```js - * Sentry.init({ - * tracePropagationTargets: ['api.site.com'], - * }); - * ``` + * NOTE: Carelessly setting this option may result into CORS errors. * - * Default: ['localhost', /^\//] {@see DEFAULT_TRACE_PROPAGATION_TARGETS} + * If you provide a `tracePropagationTargets` array, the entries you provide will be matched against two values: + * - The entire URL of the outgoing request. + * - The pathname of the outgoing request (only if it is a same-origin request) + * + * If any of the two match any of the provided values, tracing headers will be attached to the outgoing request. + * + * 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?: TracePropagationTargets; From 0d3cefd50d7bc3a9625fdd241d4781dad8762cb5 Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Tue, 13 Feb 2024 10:13:59 +0000 Subject: [PATCH 2/7] Better explanation --- .../tracing-internal/src/browser/request.ts | 17 ++++++++++------- packages/types/src/options.ts | 15 ++++++++++----- 2 files changed, 20 insertions(+), 12 deletions(-) diff --git a/packages/tracing-internal/src/browser/request.ts b/packages/tracing-internal/src/browser/request.ts index 7a01cc7c4bb5..18dcaa7d0a16 100644 --- a/packages/tracing-internal/src/browser/request.ts +++ b/packages/tracing-internal/src/browser/request.ts @@ -1,4 +1,3 @@ -/* eslint-disable max-lines */ import { SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN, getClient, @@ -34,15 +33,20 @@ export interface RequestInstrumentationOptions { * List of strings and/or Regular Expressions used to determine which outgoing requests will have `sentry-trace` and `baggage` * headers attached. * - * By default, if this option is not provided, tracing headers will be attached to all outgoing requests to the same origin as the current page. + * **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. * - * NOTE: Carelessly setting this option may result into CORS errors. + * **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 two values: - * - The entire URL of the outgoing request. - * - The pathname of the outgoing request (only if it is a same-origin request) + * 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`: @@ -246,7 +250,6 @@ export function shouldAttachHeaders( * * @returns Span if a span was created, otherwise void. */ -// eslint-disable-next-line complexity export function xhrCallback( handlerData: HandlerDataXhr, shouldCreateSpan: (url: string) => boolean, diff --git a/packages/types/src/options.ts b/packages/types/src/options.ts index 4a08679fde4d..bcc3498a1e59 100644 --- a/packages/types/src/options.ts +++ b/packages/types/src/options.ts @@ -226,15 +226,20 @@ export interface ClientOptions Date: Tue, 13 Feb 2024 11:09:43 +0000 Subject: [PATCH 3/7] Handle window.location.origin being undefined --- .../tracing-internal/src/browser/request.ts | 31 +++++++++++++------ 1 file changed, 21 insertions(+), 10 deletions(-) diff --git a/packages/tracing-internal/src/browser/request.ts b/packages/tracing-internal/src/browser/request.ts index 18dcaa7d0a16..15d54da6b2a4 100644 --- a/packages/tracing-internal/src/browser/request.ts +++ b/packages/tracing-internal/src/browser/request.ts @@ -232,17 +232,28 @@ export function shouldAttachHeaders( targetUrl: string, tracePropagationTargets: (string | RegExp)[] | undefined, ): boolean { - const resolvedUrl = new URL(targetUrl, WINDOW.location.origin); - const isSameOriginRequest = resolvedUrl.origin === WINDOW.location.origin; - - if (!tracePropagationTargets) { - return isSameOriginRequest; + // window.location.origin 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 origin: string | undefined = WINDOW.location && WINDOW.location.origin; + + if (!origin) { + // 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 { + const resolvedUrl = new URL(targetUrl, origin); + const isSameOriginRequest = resolvedUrl.origin === WINDOW.location.origin; + return ( + stringMatchesSomePattern(resolvedUrl.toString(), tracePropagationTargets) || + (isSameOriginRequest && stringMatchesSomePattern(resolvedUrl.pathname, tracePropagationTargets)) + ); } - - return ( - stringMatchesSomePattern(resolvedUrl.toString(), tracePropagationTargets) || - (isSameOriginRequest && stringMatchesSomePattern(resolvedUrl.pathname, tracePropagationTargets)) - ); } /** From ca15c4631f7cbabafb91a17d562284a80420b696 Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Tue, 13 Feb 2024 14:06:58 +0000 Subject: [PATCH 4/7] Update tests --- .../tracing-internal/src/browser/request.ts | 32 ++- .../test/browser/request.test.ts | 262 +++++++++++++++++- 2 files changed, 274 insertions(+), 20 deletions(-) diff --git a/packages/tracing-internal/src/browser/request.ts b/packages/tracing-internal/src/browser/request.ts index 15d54da6b2a4..89ca9cebbf26 100644 --- a/packages/tracing-internal/src/browser/request.ts +++ b/packages/tracing-internal/src/browser/request.ts @@ -232,11 +232,11 @@ export function shouldAttachHeaders( targetUrl: string, tracePropagationTargets: (string | RegExp)[] | undefined, ): boolean { - // window.location.origin not being defined is an edge case in the browser but we need to handle it. + // 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 origin: string | undefined = WINDOW.location && WINDOW.location.origin; + const href: string | undefined = WINDOW.location && WINDOW.location.href; - if (!origin) { + 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. @@ -247,12 +247,26 @@ export function shouldAttachHeaders( return stringMatchesSomePattern(targetUrl, tracePropagationTargets); } } else { - const resolvedUrl = new URL(targetUrl, origin); - const isSameOriginRequest = resolvedUrl.origin === WINDOW.location.origin; - return ( - stringMatchesSomePattern(resolvedUrl.toString(), tracePropagationTargets) || - (isSameOriginRequest && stringMatchesSomePattern(resolvedUrl.pathname, tracePropagationTargets)) - ); + 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)) + ); + } } } diff --git a/packages/tracing-internal/test/browser/request.test.ts b/packages/tracing-internal/test/browser/request.test.ts index 426325072984..8855203ce136 100644 --- a/packages/tracing-internal/test/browser/request.test.ts +++ b/packages/tracing-internal/test/browser/request.test.ts @@ -2,6 +2,7 @@ import * as utils from '@sentry/utils'; import { extractNetworkProtocol, instrumentOutgoingRequests, shouldAttachHeaders } from '../../src/browser/request'; +import { WINDOW } from '../../src/browser/types'; beforeAll(() => { // @ts-expect-error need to override global Request because it's not in the jest environment (even with an @@ -107,23 +108,262 @@ describe('shouldAttachHeaders', () => { }); }); - describe('should fall back to defaults if no options are specified', () => { + describe('with no defined `tracePropagationTargets`', () => { + let originalWindowLocation: Location; + + beforeAll(() => { + originalWindowLocation = WINDOW.location; + // @ts-expect-error We are missing some fields of the Origin interface but it doesn't matter for these tests. + WINDOW.location = new URL('https://my-origin.com'); + }); + + afterAll(() => { + WINDOW.location = originalWindowLocation; + }); + it.each([ - '/api/test', - 'http://localhost:3000/test', - 'http://somewhere.com/test/localhost/123', - 'http://somewhere.com/test?url=localhost:3000&test=123', - '//localhost:3000/test', + 'https://my-origin.com', + 'https://my-origin.com/test', '/', - ])('return `true` for urls matching defaults (%s)', url => { + '/api/test', + '//my-origin.com/', + '//my-origin.com/test', + 'foobar', // this is a relative request + 'not-my-origin.com', // this is a relative request + 'not-my-origin.com/api/test', // this is a relative request + ])('should return `true` for same-origin URLs (%s)', url => { expect(shouldAttachHeaders(url, undefined)).toBe(true); }); - it.each(['notmydoman/api/test', 'example.com', '//example.com'])( - 'return `false` for urls not matching defaults (%s)', - url => { - expect(shouldAttachHeaders(url, undefined)).toBe(false); + it.each([ + 'http://my-origin.com', // wrong protocol + 'http://my-origin.com/api', // wrong protocol + 'http://localhost:3000', + '//not-my-origin.com/test', + 'https://somewhere.com/test/localhost/123', + 'https://somewhere.com/test?url=https://my-origin.com', + '//example.com', + ])('should return `false` for cross-origin URLs (%s)', url => { + expect(shouldAttachHeaders(url, undefined)).toBe(false); + }); + }); + + describe('with `tracePropagationTargets`', () => { + let originalWindowLocation: Location; + + beforeAll(() => { + originalWindowLocation = WINDOW.location; + // @ts-expect-error We are missing some fields of the Origin interface but it doesn't matter for these tests. + WINDOW.location = new URL('https://my-origin.com/api/my-route'); + }); + + afterAll(() => { + WINDOW.location = originalWindowLocation; + }); + + it.each([ + ['https://my-origin.com', /^\//, true], // pathname defaults to "/" + ['https://my-origin.com/', /^\//, true], + ['https://not-my-origin.com', /^\//, false], // pathname does not match in isolation for cross origin + ['https://not-my-origin.com/', /^\//, false], // pathname does not match in isolation for cross origin + + ['http://my-origin.com/', /^\//, false], // different protocol than origin + + ['//my-origin.com', /^\//, true], // pathname defaults to "/" + ['//my-origin.com/', /^\//, true], // matches pathname + ['//not-my-origin.com', /^\//, false], + ['//not-my-origin.com/', /^\//, false], // different origin should not match pathname + + ['//my-origin.com', /^https:/, true], + ['//not-my-origin.com', /^https:/, true], + ['//my-origin.com', /^http:/, false], + ['//not-my-origin.com', /^http:/, false], + + ['https://my-origin.com/api', /^\/api/, true], + ['https://not-my-origin.com/api', /^\/api/, false], // different origin should not match pathname in isolation + + ['https://my-origin.com/api', /api/, true], + ['https://not-my-origin.com/api', /api/, true], + + ['/api', /^\/api/, true], // matches pathname + ['/api', /\/\/my-origin\.com\/api/, true], // matches full url + ['foobar', /\/foobar/, true], // matches full url + ['foobar', /^\/api\/foobar/, true], // full url match + ['some-url.com', /\/some-url\.com/, true], + ['some-url.com', /^\/some-url\.com/, false], // does not match pathname or full url + ['some-url.com', /^\/api\/some-url\.com/, true], // matches pathname + + ['/api', /^http:/, false], + ['foobar', /^http:/, false], + ['some-url.com', /^http:/, false], + ['/api', /^https:/, true], + ['foobar', /^https:/, true], + ['some-url.com', /^https:/, true], + + ['https://my-origin.com', 'my-origin', true], + ['https://not-my-origin.com', 'my-origin', true], + ['https://my-origin.com', 'not-my-origin', false], + ['https://not-my-origin.com', 'not-my-origin', true], + + ['https://my-origin.com', 'https', true], + ['https://my-origin.com', 'http', true], // partially matches https + ['//my-origin.com', 'https', true], + ['//my-origin.com', 'http', true], // partially matches https + + ['/api', '/api', true], + ['api', '/api', true], // full url match + ['https://not-my-origin.com/api', 'api', true], + ['https://my-origin.com?my-query', 'my-query', true], + ['https://not-my-origin.com?my-query', 'my-query', true], + ])( + 'for url %p and tracePropagationTarget %p on page "https://my-origin.com/api/my-route" should return %p', + (url, matcher, result) => { + expect(shouldAttachHeaders(url, [matcher])).toBe(result); }, ); }); + + it.each([ + 'https://my-origin.com', + 'https://my-origin.com/', + 'https://not-my-origin.com', + 'https://not-my-origin.com/', + 'http://my-origin.com/', + '//my-origin.com', + '//my-origin.com/', + '//not-my-origin.com', + '//not-my-origin.com/', + '//my-origin.com', + '//not-my-origin.com', + '//my-origin.com', + '//not-my-origin.com', + 'https://my-origin.com/api', + 'https://not-my-origin.com/api', + 'https://my-origin.com/api', + 'https://not-my-origin.com/api', + '/api', + '/api', + 'foobar', + 'foobar', + 'some-url.com', + 'some-url.com', + 'some-url.com', + '/api', + 'foobar', + 'some-url.com', + '/api', + 'foobar', + 'some-url.com', + 'https://my-origin.com', + 'https://not-my-origin.com', + 'https://my-origin.com', + 'https://not-my-origin.com', + 'https://my-origin.com', + 'https://my-origin.com', + '//my-origin.com', + '//my-origin.com', + '/api', + 'api', + 'https://not-my-origin.com/api', + 'https://my-origin.com?my-query', + 'https://not-my-origin.com?my-query', + ])('should return false for everything if tracePropagationTargets are empty (%p)', url => { + expect(shouldAttachHeaders(url, [])).toBe(false); + }); + + describe('when window.location.href is not available', () => { + let originalWindowLocation: Location; + + beforeAll(() => { + originalWindowLocation = WINDOW.location; + // @ts-expect-error We need to simulate an edge-case + WINDOW.location = undefined; + }); + + afterAll(() => { + WINDOW.location = originalWindowLocation; + }); + + describe('with no defined `tracePropagationTargets`', () => { + it.each([ + ['https://my-origin.com', false], + ['https://my-origin.com/test', false], + ['/', true], + ['/api/test', true], + ['//my-origin.com/', false], + ['//my-origin.com/test', false], + ['//not-my-origin.com/test', false], + ['foobar', false], + ['not-my-origin.com', false], + ['not-my-origin.com/api/test', false], + ['http://my-origin.com', false], + ['http://my-origin.com/api', false], + ['http://localhost:3000', false], + ['https://somewhere.com/test/localhost/123', false], + ['https://somewhere.com/test?url=https://my-origin.com', false], + ])('for URL %p should return %p', (url, expectedResult) => { + expect(shouldAttachHeaders(url, undefined)).toBe(expectedResult); + }); + }); + + // Here we should only quite literally match the provided urls + it.each([ + ['https://my-origin.com', /^\//, false], + ['https://my-origin.com/', /^\//, false], + ['https://not-my-origin.com', /^\//, false], + ['https://not-my-origin.com/', /^\//, false], + + ['http://my-origin.com/', /^\//, false], + + // It is arguably bad that these match, at the same time, these targets are very unusual in environments without location. + ['//my-origin.com', /^\//, true], + ['//my-origin.com/', /^\//, true], + ['//not-my-origin.com', /^\//, true], + ['//not-my-origin.com/', /^\//, true], + + ['//my-origin.com', /^https:/, false], + ['//not-my-origin.com', /^https:/, false], + ['//my-origin.com', /^http:/, false], + ['//not-my-origin.com', /^http:/, false], + + ['https://my-origin.com/api', /^\/api/, false], + ['https://not-my-origin.com/api', /^\/api/, false], + + ['https://my-origin.com/api', /api/, true], + ['https://not-my-origin.com/api', /api/, true], + + ['/api', /^\/api/, true], + ['/api', /\/\/my-origin\.com\/api/, false], + ['foobar', /\/foobar/, false], + ['foobar', /^\/api\/foobar/, false], + ['some-url.com', /\/some-url\.com/, false], + ['some-url.com', /^\/some-url\.com/, false], + ['some-url.com', /^\/api\/some-url\.com/, false], + + ['/api', /^http:/, false], + ['foobar', /^http:/, false], + ['some-url.com', /^http:/, false], + ['/api', /^https:/, false], + ['foobar', /^https:/, false], + ['some-url.com', /^https:/, false], + + ['https://my-origin.com', 'my-origin', true], + ['https://not-my-origin.com', 'my-origin', true], + ['https://my-origin.com', 'not-my-origin', false], + ['https://not-my-origin.com', 'not-my-origin', true], + + ['https://my-origin.com', 'https', true], + ['https://my-origin.com', 'http', true], + ['//my-origin.com', 'https', false], + ['//my-origin.com', 'http', false], + + ['/api', '/api', true], + ['api', '/api', false], + ['https://not-my-origin.com/api', 'api', true], + ['https://my-origin.com?my-query', 'my-query', true], + ['https://not-my-origin.com?my-query', 'my-query', true], + ])('for url %p and tracePropagationTarget %p should return %p', (url, matcher, result) => { + expect(shouldAttachHeaders(url, [matcher])).toBe(result); + }); + }); }); From c7b64afefd973daa7c77499f08fafd0bb154d039 Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Tue, 13 Feb 2024 14:23:44 +0000 Subject: [PATCH 5/7] Remove comment --- packages/tracing-internal/src/browser/request.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/packages/tracing-internal/src/browser/request.ts b/packages/tracing-internal/src/browser/request.ts index 89ca9cebbf26..5bcea7eac108 100644 --- a/packages/tracing-internal/src/browser/request.ts +++ b/packages/tracing-internal/src/browser/request.ts @@ -225,7 +225,6 @@ 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 function for testing purposes. */ export function shouldAttachHeaders( From c50e5cea766a4c7107747648c820a8c35e4c3ef7 Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Tue, 13 Feb 2024 15:35:32 +0000 Subject: [PATCH 6/7] =?UTF-8?q?Fix=20tests=20=F0=9F=8E=89?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .../defaultTargetsMatch/subject.js | 2 +- .../defaultTargetsMatch/test.ts | 11 ++++------- .../defaultTargetsNoMatch/test.ts | 2 +- .../defaultTargetsMatch/subject.js | 2 +- .../defaultTargetsMatch/test.ts | 11 ++++------- .../defaultTargetsNoMatch/init.js | 3 +-- .../defaultTargetsNoMatch/test.ts | 11 ++++------- 7 files changed, 16 insertions(+), 26 deletions(-) diff --git a/dev-packages/browser-integration-tests/suites/tracing/browserTracingIntegration/tracePropagationTargets/defaultTargetsMatch/subject.js b/dev-packages/browser-integration-tests/suites/tracing/browserTracingIntegration/tracePropagationTargets/defaultTargetsMatch/subject.js index 4e9cf0d01004..c6cebfd1d083 100644 --- a/dev-packages/browser-integration-tests/suites/tracing/browserTracingIntegration/tracePropagationTargets/defaultTargetsMatch/subject.js +++ b/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'))); diff --git a/dev-packages/browser-integration-tests/suites/tracing/browserTracingIntegration/tracePropagationTargets/defaultTargetsMatch/test.ts b/dev-packages/browser-integration-tests/suites/tracing/browserTracingIntegration/tracePropagationTargets/defaultTargetsMatch/test.ts index 120b36ec88db..f9f9af3ddb47 100644 --- a/dev-packages/browser-integration-tests/suites/tracing/browserTracingIntegration/tracePropagationTargets/defaultTargetsMatch/test.ts +++ b/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); diff --git a/dev-packages/browser-integration-tests/suites/tracing/browserTracingIntegration/tracePropagationTargets/defaultTargetsNoMatch/test.ts b/dev-packages/browser-integration-tests/suites/tracing/browserTracingIntegration/tracePropagationTargets/defaultTargetsNoMatch/test.ts index 116319259101..0f7323d484e7 100644 --- a/dev-packages/browser-integration-tests/suites/tracing/browserTracingIntegration/tracePropagationTargets/defaultTargetsNoMatch/test.ts +++ b/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(); diff --git a/dev-packages/browser-integration-tests/suites/tracing/browsertracing/tracePropagationTargets/defaultTargetsMatch/subject.js b/dev-packages/browser-integration-tests/suites/tracing/browsertracing/tracePropagationTargets/defaultTargetsMatch/subject.js index 4e9cf0d01004..7e662b55c333 100644 --- a/dev-packages/browser-integration-tests/suites/tracing/browsertracing/tracePropagationTargets/defaultTargetsMatch/subject.js +++ b/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'))); diff --git a/dev-packages/browser-integration-tests/suites/tracing/browsertracing/tracePropagationTargets/defaultTargetsMatch/test.ts b/dev-packages/browser-integration-tests/suites/tracing/browsertracing/tracePropagationTargets/defaultTargetsMatch/test.ts index 120b36ec88db..f9f9af3ddb47 100644 --- a/dev-packages/browser-integration-tests/suites/tracing/browsertracing/tracePropagationTargets/defaultTargetsMatch/test.ts +++ b/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); diff --git a/dev-packages/browser-integration-tests/suites/tracing/browsertracing/tracePropagationTargets/defaultTargetsNoMatch/init.js b/dev-packages/browser-integration-tests/suites/tracing/browsertracing/tracePropagationTargets/defaultTargetsNoMatch/init.js index ce4e0c4ad7f7..83076460599f 100644 --- a/dev-packages/browser-integration-tests/suites/tracing/browsertracing/tracePropagationTargets/defaultTargetsNoMatch/init.js +++ b/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://public@dsn.ingest.sentry.io/1337', - integrations: [new Integrations.BrowserTracing()], + integrations: [Sentry.browserTracingIntegration()], tracesSampleRate: 1, }); diff --git a/dev-packages/browser-integration-tests/suites/tracing/browsertracing/tracePropagationTargets/defaultTargetsNoMatch/test.ts b/dev-packages/browser-integration-tests/suites/tracing/browsertracing/tracePropagationTargets/defaultTargetsNoMatch/test.ts index 116319259101..6739b7ce3621 100644 --- a/dev-packages/browser-integration-tests/suites/tracing/browsertracing/tracePropagationTargets/defaultTargetsNoMatch/test.ts +++ b/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); From 5734e9f03a9d98d944cef93976ec969ac3c77efb Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Wed, 14 Feb 2024 10:01:45 +0000 Subject: [PATCH 7/7] Add migration docs --- MIGRATION.md | 28 ++++++++++++++++++++++++++++ 1 file changed, 28 insertions(+) diff --git a/MIGRATION.md b/MIGRATION.md index c38572523fab..261ee10d0bf6 100644 --- a/MIGRATION.md +++ b/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.