From fe9624ec26c64db8da701ea5dc1ea089bd5a9f29 Mon Sep 17 00:00:00 2001 From: Ryan Lamb <4955475+kinyoklion@users.noreply.github.com> Date: Fri, 27 Sep 2024 13:35:30 -0700 Subject: [PATCH 1/2] fix: Ensure client logger is always wrapped in a safe logger. --- .../configuration/Configuration.test.ts | 47 +++++++++++++++++++ .../__tests__/context/addAutoEnv.test.ts | 10 ++-- .../src/configuration/Configuration.ts | 21 ++++++--- .../createDiagnosticsInitConfig.ts | 6 +-- 4 files changed, 69 insertions(+), 15 deletions(-) diff --git a/packages/shared/sdk-client/__tests__/configuration/Configuration.test.ts b/packages/shared/sdk-client/__tests__/configuration/Configuration.test.ts index f8af0b79cc..81a797a7d3 100644 --- a/packages/shared/sdk-client/__tests__/configuration/Configuration.test.ts +++ b/packages/shared/sdk-client/__tests__/configuration/Configuration.test.ts @@ -1,4 +1,6 @@ /* eslint-disable no-console */ +import { createSafeLogger } from '@launchdarkly/js-sdk-common'; + import ConfigurationImpl from '../../src/configuration/Configuration'; describe('Configuration', () => { @@ -120,3 +122,48 @@ describe('Configuration', () => { }, ); }); + +it('makes a safe logger', () => { + const badLogger = { + debug: () => { + throw new Error('bad'); + }, + info: () => { + throw new Error('bad'); + }, + warn: () => { + throw new Error('bad'); + }, + error: () => { + throw new Error('bad'); + }, + }; + const config = new ConfigurationImpl({ + logger: badLogger, + }); + + expect(() => config.logger.debug('debug')).not.toThrow(); + expect(() => config.logger.info('info')).not.toThrow(); + expect(() => config.logger.warn('warn')).not.toThrow(); + expect(() => config.logger.error('error')).not.toThrow(); + expect(config.logger).not.toBe(badLogger); +}); + +it('does not wrap already safe loggers', () => { + const logger = createSafeLogger({ + debug: () => { + throw new Error('bad'); + }, + info: () => { + throw new Error('bad'); + }, + warn: () => { + throw new Error('bad'); + }, + error: () => { + throw new Error('bad'); + }, + }); + const config = new ConfigurationImpl({ logger }); + expect(config.logger).toBe(logger); +}); diff --git a/packages/shared/sdk-client/__tests__/context/addAutoEnv.test.ts b/packages/shared/sdk-client/__tests__/context/addAutoEnv.test.ts index e6980d1379..b686a3c1b9 100644 --- a/packages/shared/sdk-client/__tests__/context/addAutoEnv.test.ts +++ b/packages/shared/sdk-client/__tests__/context/addAutoEnv.test.ts @@ -233,8 +233,8 @@ describe('automatic environment attributes', () => { await addAutoEnv(context, mockPlatform, config); - expect(config.logger.warn).toHaveBeenCalledTimes(1); - expect(config.logger.warn).toHaveBeenCalledWith( + expect(logger.warn).toHaveBeenCalledTimes(1); + expect(logger.warn).toHaveBeenCalledWith( expect.stringMatching(/ld_application.*already exists/), ); }); @@ -251,10 +251,8 @@ describe('automatic environment attributes', () => { await addAutoEnv(context, mockPlatform, config); - expect(config.logger.warn).toHaveBeenCalledTimes(1); - expect(config.logger.warn).toHaveBeenCalledWith( - expect.stringMatching(/ld_device.*already exists/), - ); + expect(logger.warn).toHaveBeenCalledTimes(1); + expect(logger.warn).toHaveBeenCalledWith(expect.stringMatching(/ld_device.*already exists/)); }); test('single context with an attribute called ld_application should have auto env attributes', async () => { diff --git a/packages/shared/sdk-client/src/configuration/Configuration.ts b/packages/shared/sdk-client/src/configuration/Configuration.ts index 1066fdd9ef..50ce86f3ff 100644 --- a/packages/shared/sdk-client/src/configuration/Configuration.ts +++ b/packages/shared/sdk-client/src/configuration/Configuration.ts @@ -6,6 +6,7 @@ import { LDLogger, NumberWithMinimum, OptionMessages, + SafeLogger, ServiceEndpoints, TypeValidators, } from '@launchdarkly/js-sdk-common'; @@ -21,9 +22,6 @@ export interface LDClientInternalOptions extends internal.LDInternalOptions { export interface Configuration { readonly logger: LDLogger; - readonly baseUri: string; - readonly eventsUri: string; - readonly streamUri: string; readonly maxCachedContexts: number; readonly capacity: number; readonly diagnosticRecordingInterval: number; @@ -61,12 +59,20 @@ const DEFAULT_STREAM: string = 'https://clientstream.launchdarkly.com'; export { DEFAULT_POLLING, DEFAULT_STREAM }; +function ensureSafeLogger(logger?: LDLogger): LDLogger { + if (logger instanceof SafeLogger) { + return logger; + } + // Even if logger is not defined this will produce a valid logger. + return createSafeLogger(logger); +} + export default class ConfigurationImpl implements Configuration { public readonly logger: LDLogger = createSafeLogger(); - public readonly baseUri = DEFAULT_POLLING; - public readonly eventsUri = ServiceEndpoints.DEFAULT_EVENTS; - public readonly streamUri = DEFAULT_STREAM; + private readonly baseUri = DEFAULT_POLLING; + private readonly eventsUri = ServiceEndpoints.DEFAULT_EVENTS; + private readonly streamUri = DEFAULT_STREAM; public readonly maxCachedContexts = 5; @@ -116,6 +122,7 @@ export default class ConfigurationImpl implements Configuration { [index: string]: any; constructor(pristineOptions: LDOptions = {}, internalOptions: LDClientInternalOptions = {}) { + this.logger = ensureSafeLogger(pristineOptions.logger); const errors = this.validateTypesAndNames(pristineOptions); errors.forEach((e: string) => this.logger.warn(e)); @@ -161,6 +168,8 @@ export default class ConfigurationImpl implements Configuration { } else { errors.push(OptionMessages.wrongOptionType(k, validator.getType(), typeof v)); } + } else if (k === 'logger') { + // Logger already assigned. } else { // if an option is explicitly null, coerce to undefined this[k] = v ?? undefined; diff --git a/packages/shared/sdk-client/src/diagnostics/createDiagnosticsInitConfig.ts b/packages/shared/sdk-client/src/diagnostics/createDiagnosticsInitConfig.ts index e3230b05cb..da6be4db10 100644 --- a/packages/shared/sdk-client/src/diagnostics/createDiagnosticsInitConfig.ts +++ b/packages/shared/sdk-client/src/diagnostics/createDiagnosticsInitConfig.ts @@ -18,9 +18,9 @@ export type DiagnosticsInitConfig = { bootstrapMode: boolean; }; const createDiagnosticsInitConfig = (config: Configuration): DiagnosticsInitConfig => ({ - customBaseURI: config.baseUri !== DEFAULT_POLLING, - customStreamURI: config.streamUri !== DEFAULT_STREAM, - customEventsURI: config.eventsUri !== ServiceEndpoints.DEFAULT_EVENTS, + customBaseURI: config.serviceEndpoints.polling !== DEFAULT_POLLING, + customStreamURI: config.serviceEndpoints.streaming !== DEFAULT_STREAM, + customEventsURI: config.serviceEndpoints.events !== ServiceEndpoints.DEFAULT_EVENTS, eventsCapacity: config.capacity, eventsFlushIntervalMillis: secondsToMillis(config.flushInterval), reconnectTimeMillis: secondsToMillis(config.streamInitialReconnectDelay), From 9ab4bdd508f03c1519d275bddba07cc6a49b6569 Mon Sep 17 00:00:00 2001 From: Ryan Lamb <4955475+kinyoklion@users.noreply.github.com> Date: Fri, 27 Sep 2024 13:52:12 -0700 Subject: [PATCH 2/2] Fix extra fields specified in config. --- packages/sdk/browser/__tests__/BrowserDataManager.test.ts | 3 --- packages/sdk/react-native/__tests__/MobileDataManager.test.ts | 3 --- 2 files changed, 6 deletions(-) diff --git a/packages/sdk/browser/__tests__/BrowserDataManager.test.ts b/packages/sdk/browser/__tests__/BrowserDataManager.test.ts index fee37b29b5..d5e05ba7cc 100644 --- a/packages/sdk/browser/__tests__/BrowserDataManager.test.ts +++ b/packages/sdk/browser/__tests__/BrowserDataManager.test.ts @@ -74,9 +74,6 @@ describe('given a BrowserDataManager with mocked dependencies', () => { }; config = { logger, - baseUri: 'string', - eventsUri: 'string', - streamUri: 'string', maxCachedContexts: 5, capacity: 100, diagnosticRecordingInterval: 1000, diff --git a/packages/sdk/react-native/__tests__/MobileDataManager.test.ts b/packages/sdk/react-native/__tests__/MobileDataManager.test.ts index 1e54ed5b54..a7e0447093 100644 --- a/packages/sdk/react-native/__tests__/MobileDataManager.test.ts +++ b/packages/sdk/react-native/__tests__/MobileDataManager.test.ts @@ -63,9 +63,6 @@ describe('given a MobileDataManager with mocked dependencies', () => { }; config = { logger, - baseUri: 'string', - eventsUri: 'string', - streamUri: 'string', maxCachedContexts: 5, capacity: 100, diagnosticRecordingInterval: 1000,