diff --git a/packages/telemetry/browser-telemetry/README.md b/packages/telemetry/browser-telemetry/README.md index bc0633f6b5..159bdb00b7 100644 --- a/packages/telemetry/browser-telemetry/README.md +++ b/packages/telemetry/browser-telemetry/README.md @@ -1,6 +1,5 @@ # Telemetry integration for LaunchDarkly browser SDKs. - [![NPM][browser-telemetry-npm-badge]][browser-telemetry-npm-link] [![Actions Status][browser-telemetry-ci-badge]][browser-telemetry-ci] [![Documentation][browser-telemetry-ghp-badge]][browser-telemetry-ghp-link] diff --git a/packages/telemetry/browser-telemetry/__tests__/collectors/http/fetch.test.ts b/packages/telemetry/browser-telemetry/__tests__/collectors/http/fetch.test.ts index b11c9a506c..766fc9a8af 100644 --- a/packages/telemetry/browser-telemetry/__tests__/collectors/http/fetch.test.ts +++ b/packages/telemetry/browser-telemetry/__tests__/collectors/http/fetch.test.ts @@ -1,4 +1,5 @@ import { HttpBreadcrumb } from '../../../src/api/Breadcrumb'; +import { MinLogger } from '../../../src/api/MinLogger'; import { Recorder } from '../../../src/api/Recorder'; import FetchCollector from '../../../src/collectors/http/fetch'; @@ -140,3 +141,64 @@ describe('given a FetchCollector with a mock recorder', () => { ); }); }); + +describe('given a FetchCollector with a URL filter that throws an error', () => { + let mockRecorder: Recorder; + let collector: FetchCollector; + let mockLogger: MinLogger; + beforeEach(() => { + mockLogger = { + warn: jest.fn(), + }; + // Create mock recorder + mockRecorder = { + addBreadcrumb: jest.fn(), + captureError: jest.fn(), + captureErrorEvent: jest.fn(), + }; + collector = new FetchCollector({ + urlFilters: [ + () => { + throw new Error('test error'); + }, + ], + getLogger: () => mockLogger, + }); + }); + + it('logs an error if it fails to filter a breadcrumb', async () => { + collector.register(mockRecorder, 'test-session'); + + const mockResponse = new Response('test response', { status: 200, statusText: 'OK' }); + (initialFetch as jest.Mock).mockResolvedValue(mockResponse); + + await fetch('https://api.example.com/data'); + + expect(mockLogger.warn).toHaveBeenCalledWith( + 'Error filtering http breadcrumb', + new Error('test error'), + ); + expect(initialFetch).toHaveBeenCalledWith('https://api.example.com/data'); + + expect(mockRecorder.addBreadcrumb).not.toHaveBeenCalled(); + }); + + it('only logs the filter error once for multiple requests', async () => { + collector.register(mockRecorder, 'test-session'); + + const mockResponse = new Response('test response', { status: 200, statusText: 'OK' }); + (initialFetch as jest.Mock).mockResolvedValue(mockResponse); + + // Make multiple fetch calls that will trigger the filter error + await fetch('https://api.example.com/data'); + await fetch('https://api.example.com/data2'); + await fetch('https://api.example.com/data3'); + + expect(mockLogger.warn).toHaveBeenCalledTimes(1); + expect(mockLogger.warn).toHaveBeenCalledWith( + 'Error filtering http breadcrumb', + new Error('test error'), + ); + expect(mockRecorder.addBreadcrumb).not.toHaveBeenCalled(); + }); +}); diff --git a/packages/telemetry/browser-telemetry/__tests__/collectors/http/xhr.test.ts b/packages/telemetry/browser-telemetry/__tests__/collectors/http/xhr.test.ts index 125b639ab3..f301c2bff9 100644 --- a/packages/telemetry/browser-telemetry/__tests__/collectors/http/xhr.test.ts +++ b/packages/telemetry/browser-telemetry/__tests__/collectors/http/xhr.test.ts @@ -1,4 +1,5 @@ import { HttpBreadcrumb } from '../../../src/api/Breadcrumb'; +import { MinLogger } from '../../../src/api/MinLogger'; import { Recorder } from '../../../src/api/Recorder'; import XhrCollector from '../../../src/collectors/http/xhr'; @@ -137,3 +138,70 @@ it('applies URL filters to requests', () => { afterEach(() => { window.XMLHttpRequest = initialXhr; }); + +describe('given a XhrCollector with a URL filter that throws an error', () => { + let mockRecorder: Recorder; + let collector: XhrCollector; + let mockLogger: MinLogger; + beforeEach(() => { + mockLogger = { + warn: jest.fn(), + }; + mockRecorder = { + addBreadcrumb: jest.fn(), + captureError: jest.fn(), + captureErrorEvent: jest.fn(), + }; + collector = new XhrCollector({ + urlFilters: [ + () => { + throw new Error('test error'); + }, + ], + getLogger: () => mockLogger, + }); + }); + + it('logs an error if it fails to filter a breadcrumb', async () => { + collector.register(mockRecorder, 'test-session'); + + const xhr = new XMLHttpRequest(); + xhr.open('GET', 'https://api.example.com/data?token=secret123'); + xhr.send(); + + Object.defineProperty(xhr, 'status', { value: 200 }); + xhr.dispatchEvent(new Event('loadend')); + + expect(mockLogger.warn).toHaveBeenCalledWith( + 'Error filtering http breadcrumb', + new Error('test error'), + ); + + expect(mockRecorder.addBreadcrumb).not.toHaveBeenCalled(); + }); + + it('only logs the filter error once for multiple requests', async () => { + collector.register(mockRecorder, 'test-session'); + + const xhr = new XMLHttpRequest(); + xhr.open('GET', 'https://api.example.com/data?token=secret123'); + xhr.send(); + + Object.defineProperty(xhr, 'status', { value: 200 }); + xhr.dispatchEvent(new Event('loadend')); + + const xhr2 = new XMLHttpRequest(); + xhr2.open('GET', 'https://api.example.com/data?token=secret123'); + xhr2.send(); + + Object.defineProperty(xhr2, 'status', { value: 200 }); + xhr2.dispatchEvent(new Event('loadend')); + + expect(mockLogger.warn).toHaveBeenCalledTimes(1); + expect(mockLogger.warn).toHaveBeenCalledWith( + 'Error filtering http breadcrumb', + new Error('test error'), + ); + expect(mockRecorder.addBreadcrumb).not.toHaveBeenCalled(); + }); +}); diff --git a/packages/telemetry/browser-telemetry/__tests__/filters/defaultUrlFilter.test.ts b/packages/telemetry/browser-telemetry/__tests__/filters/defaultUrlFilter.test.ts index 75a64e0a95..e3ae2f2659 100644 --- a/packages/telemetry/browser-telemetry/__tests__/filters/defaultUrlFilter.test.ts +++ b/packages/telemetry/browser-telemetry/__tests__/filters/defaultUrlFilter.test.ts @@ -63,3 +63,11 @@ it('filters out username and password from URLs', () => { expect(defaultUrlFilter(input)).toBe(expected); }); }); + +it('can handle partial URLs', () => { + expect(defaultUrlFilter('/partial/url')).toBe('/partial/url'); +}); + +it('can handle invalid URLs', () => { + expect(defaultUrlFilter('invalid url')).toBe('invalid url'); +}); diff --git a/packages/telemetry/browser-telemetry/src/BrowserTelemetryImpl.ts b/packages/telemetry/browser-telemetry/src/BrowserTelemetryImpl.ts index 05a90c9ccf..4b372e6489 100644 --- a/packages/telemetry/browser-telemetry/src/BrowserTelemetryImpl.ts +++ b/packages/telemetry/browser-telemetry/src/BrowserTelemetryImpl.ts @@ -114,8 +114,6 @@ export default class BrowserTelemetryImpl implements BrowserTelemetry { private _registrationComplete: boolean = false; - // Used to ensure we only log the event dropped message once. - private _clientRegistered: boolean = false; // Used to ensure we only log the event dropped message once. private _eventsDropped: boolean = false; // Used to ensure we only log the breadcrumb filter error once. @@ -133,6 +131,10 @@ export default class BrowserTelemetryImpl implements BrowserTelemetry { this._maxPendingEvents = _options.maxPendingEvents; this._maxBreadcrumbs = _options.breadcrumbs.maxBreadcrumbs; + // Set the initial logger, it may be replaced when the client is registered. + // For typescript purposes, we need the logger to be directly set in the constructor. + this._logger = this._options.logger ?? fallbackLogger; + const urlFilters = [defaultUrlFilter]; if (_options.breadcrumbs.http.customUrlFilter) { urlFilters.push(_options.breadcrumbs.http.customUrlFilter); @@ -142,6 +144,7 @@ export default class BrowserTelemetryImpl implements BrowserTelemetry { this._collectors.push( new FetchCollector({ urlFilters, + getLogger: () => this._logger, }), ); } @@ -150,6 +153,7 @@ export default class BrowserTelemetryImpl implements BrowserTelemetry { this._collectors.push( new XhrCollector({ urlFilters, + getLogger: () => this._logger, }), ); } @@ -170,10 +174,6 @@ export default class BrowserTelemetryImpl implements BrowserTelemetry { const inspectors: BrowserTelemetryInspector[] = []; makeInspectors(_options, inspectors, impl); this._inspectorInstances.push(...inspectors); - - // Set the initial logger, it may be replaced when the client is registered. - // For typescript purposes, we need the logger to be directly set in the constructor. - this._logger = this._options.logger ?? fallbackLogger; } register(client: LDClientTracking): void { diff --git a/packages/telemetry/browser-telemetry/src/collectors/http/HttpCollectorOptions.ts b/packages/telemetry/browser-telemetry/src/collectors/http/HttpCollectorOptions.ts index 2f6c4bab47..13313a5a1b 100644 --- a/packages/telemetry/browser-telemetry/src/collectors/http/HttpCollectorOptions.ts +++ b/packages/telemetry/browser-telemetry/src/collectors/http/HttpCollectorOptions.ts @@ -1,3 +1,4 @@ +import { MinLogger } from '../../api/MinLogger'; import { UrlFilter } from '../../api/Options'; /** @@ -10,4 +11,12 @@ export default interface HttpCollectorOptions { * This allows for redaction of potentially sensitive information in URLs. */ urlFilters: UrlFilter[]; + + /** + * Method to get a logger for warnings. + * + * This is a function to allow for accessing the current logger, as the logger + * instance may change during runtime. + */ + getLogger?: () => MinLogger; } diff --git a/packages/telemetry/browser-telemetry/src/collectors/http/fetch.ts b/packages/telemetry/browser-telemetry/src/collectors/http/fetch.ts index 0baa0739b4..c53716cc21 100644 --- a/packages/telemetry/browser-telemetry/src/collectors/http/fetch.ts +++ b/packages/telemetry/browser-telemetry/src/collectors/http/fetch.ts @@ -9,11 +9,25 @@ import HttpCollectorOptions from './HttpCollectorOptions'; */ export default class FetchCollector implements Collector { private _destination?: Recorder; + private _loggedIssue: boolean = false; constructor(options: HttpCollectorOptions) { decorateFetch((breadcrumb) => { - filterHttpBreadcrumb(breadcrumb, options); - this._destination?.addBreadcrumb(breadcrumb); + let filtersExecuted = false; + try { + filterHttpBreadcrumb(breadcrumb, options); + filtersExecuted = true; + } catch (err) { + if (!this._loggedIssue) { + options.getLogger?.()?.warn('Error filtering http breadcrumb', err); + this._loggedIssue = true; + } + } + // Only add the breadcrumb if the filter didn't throw. We don't want to + // report a breadcrumb that may have not have had the correct information redacted. + if (filtersExecuted) { + this._destination?.addBreadcrumb(breadcrumb); + } }); } diff --git a/packages/telemetry/browser-telemetry/src/collectors/http/xhr.ts b/packages/telemetry/browser-telemetry/src/collectors/http/xhr.ts index bf9f3b9b12..2708cf937e 100644 --- a/packages/telemetry/browser-telemetry/src/collectors/http/xhr.ts +++ b/packages/telemetry/browser-telemetry/src/collectors/http/xhr.ts @@ -10,11 +10,25 @@ import decorateXhr from './xhrDecorator'; */ export default class XhrCollector implements Collector { private _destination?: Recorder; + private _loggedIssue: boolean = false; constructor(options: HttpCollectorOptions) { decorateXhr((breadcrumb) => { - filterHttpBreadcrumb(breadcrumb, options); - this._destination?.addBreadcrumb(breadcrumb); + let filtersExecuted = false; + try { + filterHttpBreadcrumb(breadcrumb, options); + filtersExecuted = true; + } catch (err) { + if (!this._loggedIssue) { + options.getLogger?.()?.warn('Error filtering http breadcrumb', err); + this._loggedIssue = true; + } + } + // Only add the breadcrumb if the filter didn't throw. We don't want to + // report a breadcrumb that may have not have had the correct information redacted. + if (filtersExecuted) { + this._destination?.addBreadcrumb(breadcrumb); + } }); } diff --git a/packages/telemetry/browser-telemetry/src/filters/defaultUrlFilter.ts b/packages/telemetry/browser-telemetry/src/filters/defaultUrlFilter.ts index 0cd85f5a44..9ae2062545 100644 --- a/packages/telemetry/browser-telemetry/src/filters/defaultUrlFilter.ts +++ b/packages/telemetry/browser-telemetry/src/filters/defaultUrlFilter.ts @@ -14,18 +14,27 @@ function authorityUrlFilter(url: string): string { // This will work in browser environments, but in the future we may want to consider an approach // which doesn't rely on the browser's URL parsing. This is because other environments we may // want to target, such as ReactNative, may not have as robust URL parsing. - const urlObj = new URL(url); - let hadAuth = false; - if (urlObj.username) { - urlObj.username = 'redacted'; - hadAuth = true; - } - if (urlObj.password) { - urlObj.password = 'redacted'; - hadAuth = true; - } - if (hadAuth) { - return urlObj.toString(); + // We first check if the URL can be parsed, because it may not include the base URL. + try { + // If the URL includes a protocol, if so, then it can probably be parsed. + // Credentials require a full URL. + if (url.includes('://')) { + const urlObj = new URL(url); + let hadAuth = false; + if (urlObj.username) { + urlObj.username = 'redacted'; + hadAuth = true; + } + if (urlObj.password) { + urlObj.password = 'redacted'; + hadAuth = true; + } + if (hadAuth) { + return urlObj.toString(); + } + } + } catch { + // Could not parse the URL. } // If there was no auth information, then we don't need to modify the URL. return url;