Merge pull request #451 from splitio/fix-fallbacks-sanitizer

Fix and apply fallback sanitization during settings validation

Author: EmilianoSanchez

.github/workflows/ci.yml

@@ -24,7 +24,8 @@ jobs:
       - name: Set up nodejs
         uses: actions/setup-node@v4
         with:
-          node-version: 'lts/*'
+          # @TODO: rollback to 'lts/*'
+          node-version: '22'
           cache: 'npm'
 
       - name: npm CI

src/evaluator/fallbackTreatmentsCalculator/__tests__/fallback-calculator.spec.ts

@@ -1,99 +1,15 @@
 import { FallbackTreatmentsCalculator } from '../';
 import type { FallbackTreatmentConfiguration } from '../../../../types/splitio';
-import { loggerMock } from '../../../logger/__tests__/sdkLogger.mock';
 import { CONTROL } from '../../../utils/constants';
 
 describe('FallbackTreatmentsCalculator' , () => {
-  const longName = 'a'.repeat(101);
-
-  test('logs an error if flag name is invalid - by Flag', () => {
-    let config: FallbackTreatmentConfiguration = {
-      byFlag: {
-        'feature A': { treatment: 'TREATMENT_A', config: '{ value: 1 }' },
-      },
-    };
-    new FallbackTreatmentsCalculator(loggerMock, config);
-    expect(loggerMock.error.mock.calls[0][0]).toBe(
-      'Fallback treatments - Discarded flag \'feature A\': Invalid flag name (max 100 chars, no spaces)'
-    );
-    config = {
-      byFlag: {
-        [longName]: { treatment: 'TREATMENT_A', config: '{ value: 1 }' },
-      },
-    };
-    new FallbackTreatmentsCalculator(loggerMock, config);
-    expect(loggerMock.error.mock.calls[1][0]).toBe(
-      `Fallback treatments - Discarded flag '${longName}': Invalid flag name (max 100 chars, no spaces)`
-    );
-
-    config = {
-      byFlag: {
-        'featureB': { treatment: longName, config: '{ value: 1 }' },
-      },
-    };
-    new FallbackTreatmentsCalculator(loggerMock, config);
-    expect(loggerMock.error.mock.calls[2][0]).toBe(
-      'Fallback treatments - Discarded treatment for flag \'featureB\': Invalid treatment (max 100 chars and must match pattern)'
-    );
-
-    config = {
-      byFlag: {
-        // @ts-ignore
-        'featureC': { config: '{ global: true }' },
-      },
-    };
-    new FallbackTreatmentsCalculator(loggerMock, config);
-    expect(loggerMock.error.mock.calls[3][0]).toBe(
-      'Fallback treatments - Discarded treatment for flag \'featureC\': Invalid treatment (max 100 chars and must match pattern)'
-    );
-
-    config = {
-      byFlag: {
-        // @ts-ignore
-        'featureC': { treatment: 'invalid treatment!', config: '{ global: true }' },
-      },
-    };
-    new FallbackTreatmentsCalculator(loggerMock, config);
-    expect(loggerMock.error.mock.calls[4][0]).toBe(
-      'Fallback treatments - Discarded treatment for flag \'featureC\': Invalid treatment (max 100 chars and must match pattern)'
-    );
-  });
-
-  test('logs an error if flag name is invalid - global', () => {
-    let config: FallbackTreatmentConfiguration = {
-      global: { treatment: longName, config: '{ value: 1 }' },
-    };
-    new FallbackTreatmentsCalculator(loggerMock, config);
-    expect(loggerMock.error.mock.calls[2][0]).toBe(
-      'Fallback treatments - Discarded treatment for flag \'featureB\': Invalid treatment (max 100 chars and must match pattern)'
-    );
-
-    config = {
-      // @ts-ignore
-      global: { config: '{ global: true }' },
-    };
-    new FallbackTreatmentsCalculator(loggerMock, config);
-    expect(loggerMock.error.mock.calls[3][0]).toBe(
-      'Fallback treatments - Discarded treatment for flag \'featureC\': Invalid treatment (max 100 chars and must match pattern)'
-    );
-
-    config = {
-      // @ts-ignore
-      global: { treatment: 'invalid treatment!', config: '{ global: true }' },
-    };
-    new FallbackTreatmentsCalculator(loggerMock, config);
-    expect(loggerMock.error.mock.calls[4][0]).toBe(
-      'Fallback treatments - Discarded treatment for flag \'featureC\': Invalid treatment (max 100 chars and must match pattern)'
-    );
-  });
-
   test('returns specific fallback if flag exists', () => {
     const config: FallbackTreatmentConfiguration = {
       byFlag: {
         'featureA': { treatment: 'TREATMENT_A', config: '{ value: 1 }' },
       },
     };
-    const calculator = new FallbackTreatmentsCalculator(loggerMock, config);
+    const calculator = new FallbackTreatmentsCalculator(config);
     const result = calculator.resolve('featureA', 'label by flag');
 
     expect(result).toEqual({
@@ -108,7 +24,7 @@ describe('FallbackTreatmentsCalculator' , () => {
       byFlag: {},
       global: { treatment: 'GLOBAL_TREATMENT', config: '{ global: true }' },
     };
-    const calculator = new FallbackTreatmentsCalculator(loggerMock, config);
+    const calculator = new FallbackTreatmentsCalculator(config);
     const result = calculator.resolve('missingFlag', 'label by global');
 
     expect(result).toEqual({
@@ -122,7 +38,7 @@ describe('FallbackTreatmentsCalculator' , () => {
     const config: FallbackTreatmentConfiguration = {
       byFlag: {},
     };
-    const calculator = new FallbackTreatmentsCalculator(loggerMock, config);
+    const calculator = new FallbackTreatmentsCalculator(config);
     const result = calculator.resolve('missingFlag', 'label by noFallback');
 
     expect(result).toEqual({

src/evaluator/fallbackTreatmentsCalculator/__tests__/fallback-sanitizer.spec.ts

@@ -1,65 +1,70 @@
-import { FallbacksSanitizer } from '../fallbackSanitizer';
+import { isValidFlagName, isValidTreatment, sanitizeFallbacks } from '../fallbackSanitizer';
 import { TreatmentWithConfig } from '../../../../types/splitio';
 import { loggerMock } from '../../../logger/__tests__/sdkLogger.mock';
 
 describe('FallbacksSanitizer', () => {
   const validTreatment: TreatmentWithConfig = { treatment: 'on', config: '{"color":"blue"}' };
   const invalidTreatment: TreatmentWithConfig = { treatment: ' ', config: null };
+  const fallbackMock = {
+    global: undefined,
+    byFlag: {}
+  };
 
   beforeEach(() => {
-    jest.spyOn(console, 'error').mockImplementation(() => {});
-  });
-
-  afterEach(() => {
-    (loggerMock.error as jest.Mock).mockRestore();
+    loggerMock.mockClear();
   });
 
   describe('isValidFlagName', () => {
     test('returns true for a valid flag name', () => {
       // @ts-expect-private-access
-      expect((FallbacksSanitizer as any).isValidFlagName('my_flag')).toBe(true);
+      expect(isValidFlagName('my_flag')).toBe(true);
     });
 
     test('returns false for a name longer than 100 chars', () => {
       const longName = 'a'.repeat(101);
-      expect((FallbacksSanitizer as any).isValidFlagName(longName)).toBe(false);
+      expect(isValidFlagName(longName)).toBe(false);
+    });
+
+    test('returns false if the name contains spaces', () => {
+      expect(isValidFlagName('invalid flag')).toBe(false);
     });
 
     test('returns false if the name contains spaces', () => {
-      expect((FallbacksSanitizer as any).isValidFlagName('invalid flag')).toBe(false);
+      // @ts-ignore
+      expect(isValidFlagName(true)).toBe(false);
     });
   });
 
   describe('isValidTreatment', () => {
     test('returns true for a valid treatment string', () => {
-      expect((FallbacksSanitizer as any).isValidTreatment(validTreatment)).toBe(true);
+      expect(isValidTreatment(validTreatment)).toBe(true);
     });
 
     test('returns false for null or undefined', () => {
-      expect((FallbacksSanitizer as any).isValidTreatment(null)).toBe(false);
-      expect((FallbacksSanitizer as any).isValidTreatment(undefined)).toBe(false);
+      expect(isValidTreatment()).toBe(false);
+      expect(isValidTreatment(undefined)).toBe(false);
     });
 
     test('returns false for a treatment longer than 100 chars', () => {
-      const long = { treatment: 'a'.repeat(101) };
-      expect((FallbacksSanitizer as any).isValidTreatment(long)).toBe(false);
+      const long = { treatment: 'a'.repeat(101), config: null };
+      expect(isValidTreatment(long)).toBe(false);
     });
 
     test('returns false if treatment does not match regex pattern', () => {
-      const invalid = { treatment: 'invalid treatment!' };
-      expect((FallbacksSanitizer as any).isValidTreatment(invalid)).toBe(false);
+      const invalid = { treatment: 'invalid treatment!', config: null };
+      expect(isValidTreatment(invalid)).toBe(false);
     });
   });
 
   describe('sanitizeGlobal', () => {
     test('returns the treatment if valid', () => {
-      expect(FallbacksSanitizer.sanitizeGlobal(loggerMock, validTreatment)).toEqual(validTreatment);
+      expect(sanitizeFallbacks(loggerMock, { ...fallbackMock, global: validTreatment })).toEqual({ ...fallbackMock, global: validTreatment });
       expect(loggerMock.error).not.toHaveBeenCalled();
     });
 
     test('returns undefined and logs error if invalid', () => {
-      const result = FallbacksSanitizer.sanitizeGlobal(loggerMock, invalidTreatment);
-      expect(result).toBeUndefined();
+      const result = sanitizeFallbacks(loggerMock, { ...fallbackMock, global: invalidTreatment });
+      expect(result).toEqual(fallbackMock);
       expect(loggerMock.error).toHaveBeenCalledWith(
         expect.stringContaining('Fallback treatments - Discarded fallback')
       );
@@ -74,9 +79,9 @@ describe('FallbacksSanitizer', () => {
         bad_treatment: invalidTreatment,
       };
 
-      const result = FallbacksSanitizer.sanitizeByFlag(loggerMock, input);
+      const result = sanitizeFallbacks(loggerMock, {...fallbackMock, byFlag: input});
 
-      expect(result).toEqual({ valid_flag: validTreatment });
+      expect(result).toEqual({ ...fallbackMock, byFlag: { valid_flag: validTreatment } });
       expect(loggerMock.error).toHaveBeenCalledTimes(2); // invalid flag + bad_treatment
     });
 
@@ -85,20 +90,46 @@ describe('FallbacksSanitizer', () => {
         'invalid flag': invalidTreatment,
       };
 
-      const result = FallbacksSanitizer.sanitizeByFlag(loggerMock, input);
-      expect(result).toEqual({});
+      const result = sanitizeFallbacks(loggerMock, {...fallbackMock, byFlag: input});
+      expect(result).toEqual(fallbackMock);
       expect(loggerMock.error).toHaveBeenCalled();
     });
 
     test('returns same object if all valid', () => {
       const input = {
-        flag_one: validTreatment,
-        flag_two: { treatment: 'valid_2', config: null },
+        ...fallbackMock,
+        byFlag:{
+          flag_one: validTreatment,
+          flag_two: { treatment: 'valid_2', config: null },
+        }
       };
 
-      const result = FallbacksSanitizer.sanitizeByFlag(loggerMock, input);
+      const result = sanitizeFallbacks(loggerMock, input);
       expect(result).toEqual(input);
       expect(loggerMock.error).not.toHaveBeenCalled();
     });
   });
+
+  describe('sanitizeFallbacks', () => {
+    test('returns undefined and logs error if fallbacks is not an object', () => { // @ts-expect-error
+      const result = sanitizeFallbacks(loggerMock, 'invalid_fallbacks');
+      expect(result).toBeUndefined();
+      expect(loggerMock.error).toHaveBeenCalledWith(
+        'Fallback treatments - Discarded configuration: it must be an object with optional `global` and `byFlag` properties'
+      );
+    });
+
+    test('returns undefined and logs error if fallbacks is not an object', () => { // @ts-expect-error
+      const result = sanitizeFallbacks(loggerMock, true);
+      expect(result).toBeUndefined();
+      expect(loggerMock.error).toHaveBeenCalledWith(
+        'Fallback treatments - Discarded configuration: it must be an object with optional `global` and `byFlag` properties'
+      );
+    });
+
+    test('sanitizes both global and byFlag fallbacks for empty object', () => { // @ts-expect-error
+      const result = sanitizeFallbacks(loggerMock, { global: {} });
+      expect(result).toEqual({ global: undefined, byFlag: {} });
+    });
+  });
 });

src/evaluator/fallbackTreatmentsCalculator/fallbackSanitizer/index.ts

@@ -1,4 +1,4 @@
-import { Treatment, TreatmentWithConfig } from '../../../../types/splitio';
+import { FallbackTreatmentConfiguration, Treatment, TreatmentWithConfig } from '../../../../types/splitio';
 import { ILogger } from '../../../logger/types';
 import { isObject, isString } from '../../../utils/lang';
 
@@ -7,59 +7,65 @@ enum FallbackDiscardReason {
   Treatment = 'Invalid treatment (max 100 chars and must match pattern)',
 }
 
-export class FallbacksSanitizer {
+const TREATMENT_PATTERN = /^[0-9]+[.a-zA-Z0-9_-]*$|^[a-zA-Z]+[a-zA-Z0-9_-]*$/;
 
-  private static readonly pattern = /^[0-9]+[.a-zA-Z0-9_-]*$|^[a-zA-Z]+[a-zA-Z0-9_-]*$/;
-
-  private static isValidFlagName(name: string): boolean {
-    return name.length <= 100 && !name.includes(' ');
-  }
+export function isValidFlagName(name: string): boolean {
+  return name.length <= 100 && !name.includes(' ');
+}
 
-  private static isValidTreatment(t?: Treatment | TreatmentWithConfig): boolean {
-    const treatment = isObject(t) ? (t as TreatmentWithConfig).treatment : t;
+export function isValidTreatment(t?: Treatment | TreatmentWithConfig): boolean {
+  const treatment = isObject(t) ? (t as TreatmentWithConfig).treatment : t;
 
-    if (!isString(treatment) || treatment.length > 100) {
-      return false;
-    }
-    return FallbacksSanitizer.pattern.test(treatment);
+  if (!isString(treatment) || treatment.length > 100) {
+    return false;
   }
+  return TREATMENT_PATTERN.test(treatment);
+}
 
-  static sanitizeGlobal(logger: ILogger, treatment?: Treatment | TreatmentWithConfig): Treatment | TreatmentWithConfig | undefined {
-    if (!this.isValidTreatment(treatment)) {
-      logger.error(
-        `Fallback treatments - Discarded fallback: ${FallbackDiscardReason.Treatment}`
-      );
-      return undefined;
-    }
-    return treatment;
+function sanitizeGlobal(logger: ILogger, treatment?: Treatment | TreatmentWithConfig): Treatment | TreatmentWithConfig | undefined {
+  if (treatment === undefined) return undefined;
+  if (!isValidTreatment(treatment)) {
+    logger.error(`Fallback treatments - Discarded fallback: ${FallbackDiscardReason.Treatment}`);
+    return undefined;
   }
+  return treatment;
+}
 
-  static sanitizeByFlag(
-    logger: ILogger,
-    byFlagFallbacks: Record<string, Treatment | TreatmentWithConfig>
-  ): Record<string, Treatment | TreatmentWithConfig> {
-    const sanitizedByFlag: Record<string, Treatment | TreatmentWithConfig> = {};
+function sanitizeByFlag(
+  logger: ILogger,
+  byFlagFallbacks?: Record<string, Treatment | TreatmentWithConfig>
+): Record<string, Treatment | TreatmentWithConfig> {
+  const sanitizedByFlag: Record<string, Treatment | TreatmentWithConfig> = {};
 
-    const entries = Object.keys(byFlagFallbacks);
-    entries.forEach((flag) => {
-      const t = byFlagFallbacks[flag];
-      if (!this.isValidFlagName(flag)) {
-        logger.error(
-          `Fallback treatments - Discarded flag '${flag}': ${FallbackDiscardReason.FlagName}`
-        );
-        return;
-      }
+  if (!isObject(byFlagFallbacks)) return sanitizedByFlag;
 
-      if (!this.isValidTreatment(t)) {
-        logger.error(
-          `Fallback treatments - Discarded treatment for flag '${flag}': ${FallbackDiscardReason.Treatment}`
-        );
-        return;
-      }
+  Object.keys(byFlagFallbacks!).forEach((flag) => {
+    const t = byFlagFallbacks![flag];
 
-      sanitizedByFlag[flag] = t;
-    });
+    if (!isValidFlagName(flag)) {
+      logger.error(`Fallback treatments - Discarded flag '${flag}': ${FallbackDiscardReason.FlagName}`);
+      return;
+    }
+
+    if (!isValidTreatment(t)) {
+      logger.error(`Fallback treatments - Discarded treatment for flag '${flag}': ${FallbackDiscardReason.Treatment}`);
+      return;
+    }
+
+    sanitizedByFlag[flag] = t;
+  });
 
-    return sanitizedByFlag;
+  return sanitizedByFlag;
+}
+
+export function sanitizeFallbacks(logger: ILogger, fallbacks: FallbackTreatmentConfiguration): FallbackTreatmentConfiguration | undefined {
+  if (!isObject(fallbacks)) {
+    logger.error('Fallback treatments - Discarded configuration: it must be an object with optional `global` and `byFlag` properties');
+    return;
   }
+
+  return {
+    global: sanitizeGlobal(logger, fallbacks.global),
+    byFlag: sanitizeByFlag(logger, fallbacks.byFlag)
+  };
 }