fix: Fix issue processing URLs for fetch and XHR requests. (#783)

A URL filter was throwing an exception. This fixes the filter to prevent
that exception, but it also makes the fetch/xhr wrappers more resilient
to ensure we do not prevent requests from functioning when a filter
fails.

Author: kinyoklion

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]

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();
+  });
+});

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();
+  });
+});

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');
+});

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 {

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;
 }

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);
+      }
     });
   }
 

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);
+      }
     });
   }
 

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;