From deedfcbc93f540130234e1c777cc44fdbef34bb2 Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Wed, 21 May 2025 14:17:17 +0200 Subject: [PATCH 1/3] feat(browser): Improve BrowserClient default data handling --- .../skip-init-browser-extension/test.ts | 7 +- .../skip-init-chrome-extension/test.ts | 7 +- packages/browser/src/client.ts | 40 ++++-- packages/browser/src/sdk.ts | 116 ++--------------- .../src/utils/detectBrowserExtension.ts | 65 ++++++++++ packages/browser/test/client.test.ts | 69 +++++++++- packages/browser/test/sdk.test.ts | 120 ++---------------- 7 files changed, 192 insertions(+), 232 deletions(-) create mode 100644 packages/browser/src/utils/detectBrowserExtension.ts diff --git a/dev-packages/browser-integration-tests/suites/manual-client/skip-init-browser-extension/test.ts b/dev-packages/browser-integration-tests/suites/manual-client/skip-init-browser-extension/test.ts index 5098b4aa6552..791052f40ba8 100644 --- a/dev-packages/browser-integration-tests/suites/manual-client/skip-init-browser-extension/test.ts +++ b/dev-packages/browser-integration-tests/suites/manual-client/skip-init-browser-extension/test.ts @@ -18,7 +18,12 @@ sentryTest( return !!(window as any).Sentry.isInitialized(); }); - expect(isInitialized).toEqual(false); + const isEnabled = await page.evaluate(() => { + return !!(window as any).Sentry.isEnabled(); + }); + + expect(isInitialized).toEqual(true); + expect(isEnabled).toEqual(false); if (hasDebugLogs()) { expect(errorLogs.length).toEqual(1); diff --git a/dev-packages/browser-integration-tests/suites/manual-client/skip-init-chrome-extension/test.ts b/dev-packages/browser-integration-tests/suites/manual-client/skip-init-chrome-extension/test.ts index 411fbd2f9db8..3b138516e2a7 100644 --- a/dev-packages/browser-integration-tests/suites/manual-client/skip-init-chrome-extension/test.ts +++ b/dev-packages/browser-integration-tests/suites/manual-client/skip-init-chrome-extension/test.ts @@ -16,7 +16,12 @@ sentryTest('should not initialize when inside a Chrome browser extension', async return !!(window as any).Sentry.isInitialized(); }); - expect(isInitialized).toEqual(false); + const isEnabled = await page.evaluate(() => { + return !!(window as any).Sentry.isEnabled(); + }); + + expect(isInitialized).toEqual(true); + expect(isEnabled).toEqual(false); if (hasDebugLogs()) { expect(errorLogs.length).toEqual(1); diff --git a/packages/browser/src/client.ts b/packages/browser/src/client.ts index c0841d161ed6..5c50e53e708b 100644 --- a/packages/browser/src/client.ts +++ b/packages/browser/src/client.ts @@ -21,15 +21,24 @@ import { eventFromException, eventFromMessage } from './eventbuilder'; import { WINDOW } from './helpers'; import type { BrowserTransportOptions } from './transports/types'; +/** + * A magic string that build tooling can leverage in order to inject a release value into the SDK. + */ +declare const __SENTRY_RELEASE__: string | undefined; + const DEFAULT_FLUSH_INTERVAL = 5000; +type BrowserSpecificOptions = BrowserClientReplayOptions & + BrowserClientProfilingOptions & { + /** If configured, this URL will be used as base URL for lazy loading integration. */ + cdnBaseUrl?: string; + }; /** * Configuration options for the Sentry Browser SDK. * @see @sentry/core Options for more information. */ export type BrowserOptions = Options & - BrowserClientReplayOptions & - BrowserClientProfilingOptions & { + BrowserSpecificOptions & { /** * Important: Only set this option if you know what you are doing! * @@ -54,12 +63,7 @@ export type BrowserOptions = Options & * Configuration options for the Sentry Browser SDK Client class * @see BrowserClient for more information. */ -export type BrowserClientOptions = ClientOptions & - BrowserClientReplayOptions & - BrowserClientProfilingOptions & { - /** If configured, this URL will be used as base URL for lazy loading integration. */ - cdnBaseUrl?: string; - }; +export type BrowserClientOptions = ClientOptions & BrowserSpecificOptions; /** * The Sentry Browser SDK Client. @@ -75,11 +79,7 @@ export class BrowserClient extends Client { * @param options Configuration options for this SDK. */ public constructor(options: BrowserClientOptions) { - const opts = { - // We default this to true, as it is the safer scenario - parentSpanIsAlwaysRootSpan: true, - ...options, - }; + const opts = applyDefaultOptions(options); const sdkSource = WINDOW.SENTRY_SDK_SOURCE || getSDKSource(); applySdkMetadata(opts, 'browser', ['browser'], sdkSource); @@ -155,3 +155,17 @@ export class BrowserClient extends Client { return super._prepareEvent(event, hint, currentScope, isolationScope); } } + +/** Exported only for tests. */ +export function applyDefaultOptions>(optionsArg: T): T { + return { + release: + typeof __SENTRY_RELEASE__ === 'string' // This allows build tooling to find-and-replace __SENTRY_RELEASE__ to inject a release value + ? __SENTRY_RELEASE__ + : WINDOW.SENTRY_RELEASE?.id, // This supports the variable that sentry-webpack-plugin injects + sendClientReports: true, + // We default this to true, as it is the safer scenario + parentSpanIsAlwaysRootSpan: true, + ...optionsArg, + }; +} diff --git a/packages/browser/src/sdk.ts b/packages/browser/src/sdk.ts index 56f3ace8f193..32338f3ebbe8 100644 --- a/packages/browser/src/sdk.ts +++ b/packages/browser/src/sdk.ts @@ -1,18 +1,14 @@ import type { Client, Integration, Options } from '@sentry/core'; import { - consoleSandbox, dedupeIntegration, functionToStringIntegration, getIntegrationsToSetup, - getLocationHref, inboundFiltersIntegration, initAndBind, stackParserFromStackParserOptions, } from '@sentry/core'; import type { BrowserClientOptions, BrowserOptions } from './client'; import { BrowserClient } from './client'; -import { DEBUG_BUILD } from './debug-build'; -import { WINDOW } from './helpers'; import { breadcrumbsIntegration } from './integrations/breadcrumbs'; import { browserApiErrorsIntegration } from './integrations/browserapierrors'; import { browserSessionIntegration } from './integrations/browsersession'; @@ -21,22 +17,7 @@ import { httpContextIntegration } from './integrations/httpcontext'; import { linkedErrorsIntegration } from './integrations/linkederrors'; import { defaultStackParser } from './stack-parsers'; import { makeFetchTransport } from './transports/fetch'; - -type ExtensionProperties = { - chrome?: Runtime; - browser?: Runtime; - nw?: unknown; -}; -type Runtime = { - runtime?: { - id?: string; - }; -}; - -/** - * A magic string that build tooling can leverage in order to inject a release value into the SDK. - */ -declare const __SENTRY_RELEASE__: string | undefined; +import { checkAndWarnIfIsEmbeddedBrowserExtension } from './utils/detectBrowserExtension'; /** Get the default integrations for the browser SDK. */ export function getDefaultIntegrations(_options: Options): Integration[] { @@ -59,40 +40,6 @@ export function getDefaultIntegrations(_options: Options): Integration[] { ]; } -/** Exported only for tests. */ -export function applyDefaultOptions(optionsArg: BrowserOptions = {}): BrowserOptions { - const defaultOptions: BrowserOptions = { - defaultIntegrations: getDefaultIntegrations(optionsArg), - release: - typeof __SENTRY_RELEASE__ === 'string' // This allows build tooling to find-and-replace __SENTRY_RELEASE__ to inject a release value - ? __SENTRY_RELEASE__ - : WINDOW.SENTRY_RELEASE?.id, // This supports the variable that sentry-webpack-plugin injects - sendClientReports: true, - }; - - return { - ...defaultOptions, - ...dropTopLevelUndefinedKeys(optionsArg), - }; -} - -/** - * In contrast to the regular `dropUndefinedKeys` method, - * this one does not deep-drop keys, but only on the top level. - */ -function dropTopLevelUndefinedKeys(obj: T): Partial { - const mutatetedObj: Partial = {}; - - for (const k of Object.getOwnPropertyNames(obj)) { - const key = k as keyof T; - if (obj[key] !== undefined) { - mutatetedObj[key] = obj[key]; - } - } - - return mutatetedObj; -} - /** * The Sentry Browser SDK Client. * @@ -139,19 +86,21 @@ function dropTopLevelUndefinedKeys(obj: T): Partial { * * @see {@link BrowserOptions} for documentation on configuration options. */ -export function init(browserOptions: BrowserOptions = {}): Client | undefined { - if (!browserOptions.skipBrowserExtensionCheck && _checkForBrowserExtension()) { - return; - } +export function init(options: BrowserOptions = {}): Client | undefined { + const shouldDisableBecauseIsBrowserExtenstion = + !options.skipBrowserExtensionCheck && checkAndWarnIfIsEmbeddedBrowserExtension(); - const options = applyDefaultOptions(browserOptions); const clientOptions: BrowserClientOptions = { + enabled: !shouldDisableBecauseIsBrowserExtenstion, ...options, stackParser: stackParserFromStackParserOptions(options.stackParser || defaultStackParser), - integrations: getIntegrationsToSetup(options), + integrations: getIntegrationsToSetup({ + integrations: options.integrations, + defaultIntegrations: + options.defaultIntegrations == null ? getDefaultIntegrations(options) : options.defaultIntegrations, + }), transport: options.transport || makeFetchTransport, }; - return initAndBind(BrowserClient, clientOptions); } @@ -170,48 +119,3 @@ export function forceLoad(): void { export function onLoad(callback: () => void): void { callback(); } - -function _isEmbeddedBrowserExtension(): boolean { - if (typeof WINDOW.window === 'undefined') { - // No need to show the error if we're not in a browser window environment (e.g. service workers) - return false; - } - - const _window = WINDOW as typeof WINDOW & ExtensionProperties; - - // Running the SDK in NW.js, which appears like a browser extension but isn't, is also fine - // see: https://github.com/getsentry/sentry-javascript/issues/12668 - if (_window.nw) { - return false; - } - - const extensionObject = _window['chrome'] || _window['browser']; - - if (!extensionObject?.runtime?.id) { - return false; - } - - const href = getLocationHref(); - const extensionProtocols = ['chrome-extension', 'moz-extension', 'ms-browser-extension', 'safari-web-extension']; - - // Running the SDK in a dedicated extension page and calling Sentry.init is fine; no risk of data leakage - const isDedicatedExtensionPage = - WINDOW === WINDOW.top && extensionProtocols.some(protocol => href.startsWith(`${protocol}://`)); - - return !isDedicatedExtensionPage; -} - -function _checkForBrowserExtension(): true | void { - if (_isEmbeddedBrowserExtension()) { - if (DEBUG_BUILD) { - consoleSandbox(() => { - // eslint-disable-next-line no-console - console.error( - '[Sentry] You cannot use Sentry.init() in a browser extension, see: https://docs.sentry.io/platforms/javascript/best-practices/browser-extensions/', - ); - }); - } - - return true; - } -} diff --git a/packages/browser/src/utils/detectBrowserExtension.ts b/packages/browser/src/utils/detectBrowserExtension.ts new file mode 100644 index 000000000000..52e667ccecf2 --- /dev/null +++ b/packages/browser/src/utils/detectBrowserExtension.ts @@ -0,0 +1,65 @@ +import { consoleSandbox, getLocationHref } from '@sentry/core'; +import { DEBUG_BUILD } from '../debug-build'; +import { WINDOW } from '../helpers'; + +type ExtensionRuntime = { + runtime?: { + id?: string; + }; +}; +type ExtensionProperties = { + chrome?: ExtensionRuntime; + browser?: ExtensionRuntime; + nw?: unknown; +}; + +/** + * Returns true if the SDK is running in an embedded browser extension. + * Stand-alone browser extensions (which do not share the same data as the main browser page) are fine. + */ +export function checkAndWarnIfIsEmbeddedBrowserExtension(): boolean { + if (_isEmbeddedBrowserExtension()) { + if (DEBUG_BUILD) { + consoleSandbox(() => { + // eslint-disable-next-line no-console + console.error( + '[Sentry] You cannot use Sentry.init() in a browser extension, see: https://docs.sentry.io/platforms/javascript/best-practices/browser-extensions/', + ); + }); + } + + return true; + } + + return false; +} + +function _isEmbeddedBrowserExtension(): boolean { + if (typeof WINDOW.window === 'undefined') { + // No need to show the error if we're not in a browser window environment (e.g. service workers) + return false; + } + + const _window = WINDOW as typeof WINDOW & ExtensionProperties; + + // Running the SDK in NW.js, which appears like a browser extension but isn't, is also fine + // see: https://github.com/getsentry/sentry-javascript/issues/12668 + if (_window.nw) { + return false; + } + + const extensionObject = _window['chrome'] || _window['browser']; + + if (!extensionObject?.runtime?.id) { + return false; + } + + const href = getLocationHref(); + const extensionProtocols = ['chrome-extension', 'moz-extension', 'ms-browser-extension', 'safari-web-extension']; + + // Running the SDK in a dedicated extension page and calling Sentry.init is fine; no risk of data leakage + const isDedicatedExtensionPage = + WINDOW === WINDOW.top && extensionProtocols.some(protocol => href.startsWith(`${protocol}://`)); + + return !isDedicatedExtensionPage; +} diff --git a/packages/browser/test/client.test.ts b/packages/browser/test/client.test.ts index a90f8cdbc388..c0f4e649501a 100644 --- a/packages/browser/test/client.test.ts +++ b/packages/browser/test/client.test.ts @@ -4,7 +4,7 @@ import * as sentryCore from '@sentry/core'; import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; -import { BrowserClient } from '../src/client'; +import { applyDefaultOptions, BrowserClient } from '../src/client'; import { WINDOW } from '../src/helpers'; import { getDefaultBrowserClientOptions } from './helper/browser-client-options'; @@ -118,3 +118,70 @@ describe('BrowserClient', () => { }); }); }); + +describe('applyDefaultOptions', () => { + it('works with empty options', () => { + const options = {}; + const actual = applyDefaultOptions(options); + + expect(actual).toEqual({ + release: undefined, + sendClientReports: true, + parentSpanIsAlwaysRootSpan: true, + }); + }); + + it('works with options', () => { + const options = { + tracesSampleRate: 0.5, + release: '1.0.0', + }; + const actual = applyDefaultOptions(options); + + expect(actual).toEqual({ + release: '1.0.0', + sendClientReports: true, + tracesSampleRate: 0.5, + parentSpanIsAlwaysRootSpan: true, + }); + }); + + it('picks up release from WINDOW.SENTRY_RELEASE.id', () => { + const releaseBefore = WINDOW.SENTRY_RELEASE; + + WINDOW.SENTRY_RELEASE = { id: '1.0.0' }; + const options = { + tracesSampleRate: 0.5, + }; + const actual = applyDefaultOptions(options); + + expect(actual).toEqual({ + release: '1.0.0', + sendClientReports: true, + tracesSampleRate: 0.5, + parentSpanIsAlwaysRootSpan: true, + }); + + WINDOW.SENTRY_RELEASE = releaseBefore; + }); + + it('passed in release takes precedence over WINDOW.SENTRY_RELEASE.id', () => { + const releaseBefore = WINDOW.SENTRY_RELEASE; + + WINDOW.SENTRY_RELEASE = { id: '1.0.0' }; + const options = { + release: '2.0.0', + tracesSampleRate: 0.5, + }; + const actual = applyDefaultOptions(options); + + expect(actual).toEqual({ + release: '2.0.0', + sendClientReports: true, + tracesSampleRate: 0.5, + parentSpanIsAlwaysRootSpan: true, + }); + + WINDOW.SENTRY_RELEASE = releaseBefore; + }); +}); diff --git a/packages/browser/test/sdk.test.ts b/packages/browser/test/sdk.test.ts index 342b008bfc18..b7972797182f 100644 --- a/packages/browser/test/sdk.test.ts +++ b/packages/browser/test/sdk.test.ts @@ -7,10 +7,10 @@ import type { Integration } from '@sentry/core'; import * as SentryCore from '@sentry/core'; import { createTransport, resolvedSyncPromise } from '@sentry/core'; import type { Mock } from 'vitest'; -import { afterAll, afterEach, beforeEach, describe, expect, it, test, vi } from 'vitest'; +import { afterEach, describe, expect, it, test, vi } from 'vitest'; import type { BrowserOptions } from '../src'; import { WINDOW } from '../src'; -import { applyDefaultOptions, getDefaultIntegrations, init } from '../src/sdk'; +import { init } from '../src/sdk'; const PUBLIC_DSN = 'https://username@domain/123'; @@ -32,15 +32,11 @@ export class MockIntegration implements Integration { } describe('init', () => { - beforeEach(() => { - vi.clearAllMocks(); + afterEach(() => { + vi.restoreAllMocks(); }); - afterAll(() => { - vi.resetAllMocks(); - }); - - test('installs default integrations', () => { + test('installs passed default integrations', () => { const DEFAULT_INTEGRATIONS: Integration[] = [ new MockIntegration('MockIntegration 0.1'), new MockIntegration('MockIntegration 0.2'), @@ -134,7 +130,7 @@ describe('init', () => { Object.defineProperty(WINDOW, 'browser', { value: undefined, writable: true }); Object.defineProperty(WINDOW, 'nw', { value: undefined, writable: true }); Object.defineProperty(WINDOW, 'window', { value: WINDOW, writable: true }); - vi.clearAllMocks(); + vi.restoreAllMocks(); }); it('logs a browser extension error if executed inside a Chrome extension', () => { @@ -151,8 +147,6 @@ describe('init', () => { expect(consoleErrorSpy).toHaveBeenCalledWith( '[Sentry] You cannot use Sentry.init() in a browser extension, see: https://docs.sentry.io/platforms/javascript/best-practices/browser-extensions/', ); - - consoleErrorSpy.mockRestore(); }); it('logs a browser extension error if executed inside a Firefox/Safari extension', () => { @@ -166,8 +160,6 @@ describe('init', () => { expect(consoleErrorSpy).toHaveBeenCalledWith( '[Sentry] You cannot use Sentry.init() in a browser extension, see: https://docs.sentry.io/platforms/javascript/best-practices/browser-extensions/', ); - - consoleErrorSpy.mockRestore(); }); it.each(['chrome-extension', 'moz-extension', 'ms-browser-extension', 'safari-web-extension'])( @@ -224,7 +216,7 @@ describe('init', () => { consoleErrorSpy.mockRestore(); }); - it("doesn't return a client on initialization error", () => { + it('returns a disabled client on initialization error', () => { const consoleErrorSpy = vi.spyOn(console, 'error').mockImplementation(() => {}); Object.defineProperty(WINDOW, 'chrome', { @@ -234,7 +226,9 @@ describe('init', () => { const client = init(options); - expect(client).toBeUndefined(); + expect(client).toBeDefined(); + expect(SentryCore.isEnabled()).toBe(false); + expect(client!['_isEnabled']()).toBe(false); consoleErrorSpy.mockRestore(); }); @@ -245,97 +239,3 @@ describe('init', () => { expect(client).not.toBeUndefined(); }); }); - -describe('applyDefaultOptions', () => { - test('it works with empty options', () => { - const options = {}; - const actual = applyDefaultOptions(options); - - expect(actual).toEqual({ - defaultIntegrations: expect.any(Array), - release: undefined, - sendClientReports: true, - }); - - expect((actual.defaultIntegrations as { name: string }[]).map(i => i.name)).toEqual( - getDefaultIntegrations(options).map(i => i.name), - ); - }); - - test('it works with options', () => { - const options = { - tracesSampleRate: 0.5, - release: '1.0.0', - }; - const actual = applyDefaultOptions(options); - - expect(actual).toEqual({ - defaultIntegrations: expect.any(Array), - release: '1.0.0', - sendClientReports: true, - tracesSampleRate: 0.5, - }); - - expect((actual.defaultIntegrations as { name: string }[]).map(i => i.name)).toEqual( - getDefaultIntegrations(options).map(i => i.name), - ); - }); - - test('it works with defaultIntegrations=false', () => { - const options = { - defaultIntegrations: false, - } as const; - const actual = applyDefaultOptions(options); - - expect(actual.defaultIntegrations).toStrictEqual(false); - }); - - test('it works with defaultIntegrations=[]', () => { - const options = { - defaultIntegrations: [], - }; - const actual = applyDefaultOptions(options); - - expect(actual.defaultIntegrations).toEqual([]); - }); - - test('it works with tracesSampleRate=undefined', () => { - const options = { - tracesSampleRate: undefined, - } as const; - const actual = applyDefaultOptions(options); - - // Not defined, not even undefined - expect('tracesSampleRate' in actual).toBe(false); - }); - - test('it works with tracesSampleRate=null', () => { - const options = { - tracesSampleRate: null, - } as any; - const actual = applyDefaultOptions(options); - - expect(actual.tracesSampleRate).toStrictEqual(null); - }); - - test('it works with tracesSampleRate=0', () => { - const options = { - tracesSampleRate: 0, - } as const; - const actual = applyDefaultOptions(options); - - expect(actual.tracesSampleRate).toStrictEqual(0); - }); - - test('it does not deep-drop undefined keys', () => { - const options = { - obj: { - prop: undefined, - }, - } as any; - const actual = applyDefaultOptions(options) as any; - - expect('prop' in actual.obj).toBe(true); - expect(actual.obj.prop).toStrictEqual(undefined); - }); -}); From 3cc641db1e723cd1f1391325c36df30fd3ab841d Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Wed, 21 May 2025 16:51:44 +0200 Subject: [PATCH 2/3] fix tests --- .../suites/manual-client/skip-init-browser-extension/test.ts | 2 +- .../suites/manual-client/skip-init-chrome-extension/test.ts | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/dev-packages/browser-integration-tests/suites/manual-client/skip-init-browser-extension/test.ts b/dev-packages/browser-integration-tests/suites/manual-client/skip-init-browser-extension/test.ts index 791052f40ba8..d949e5d2b19e 100644 --- a/dev-packages/browser-integration-tests/suites/manual-client/skip-init-browser-extension/test.ts +++ b/dev-packages/browser-integration-tests/suites/manual-client/skip-init-browser-extension/test.ts @@ -19,7 +19,7 @@ sentryTest( }); const isEnabled = await page.evaluate(() => { - return !!(window as any).Sentry.isEnabled(); + return !!(window as any).Sentry.getClient()?.getOptions().enabled; }); expect(isInitialized).toEqual(true); diff --git a/dev-packages/browser-integration-tests/suites/manual-client/skip-init-chrome-extension/test.ts b/dev-packages/browser-integration-tests/suites/manual-client/skip-init-chrome-extension/test.ts index 3b138516e2a7..83996a83aad7 100644 --- a/dev-packages/browser-integration-tests/suites/manual-client/skip-init-chrome-extension/test.ts +++ b/dev-packages/browser-integration-tests/suites/manual-client/skip-init-chrome-extension/test.ts @@ -17,7 +17,7 @@ sentryTest('should not initialize when inside a Chrome browser extension', async }); const isEnabled = await page.evaluate(() => { - return !!(window as any).Sentry.isEnabled(); + return !!(window as any).Sentry.getClient()?.getOptions().enabled; }); expect(isInitialized).toEqual(true); From 0eb6c5fe27e7d7766f680ae2c0be9bf920d0eab8 Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Fri, 23 May 2025 09:32:36 +0200 Subject: [PATCH 3/3] fix edge case --- packages/browser/src/sdk.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/browser/src/sdk.ts b/packages/browser/src/sdk.ts index 32338f3ebbe8..2101e4e36074 100644 --- a/packages/browser/src/sdk.ts +++ b/packages/browser/src/sdk.ts @@ -91,8 +91,8 @@ export function init(options: BrowserOptions = {}): Client | undefined { !options.skipBrowserExtensionCheck && checkAndWarnIfIsEmbeddedBrowserExtension(); const clientOptions: BrowserClientOptions = { - enabled: !shouldDisableBecauseIsBrowserExtenstion, ...options, + enabled: shouldDisableBecauseIsBrowserExtenstion ? false : options.enabled, stackParser: stackParserFromStackParserOptions(options.stackParser || defaultStackParser), integrations: getIntegrationsToSetup({ integrations: options.integrations,