Merge pull request #164 from splitio/sdks-7670

[SDKS-7670] flag set input validation improvements
splitio · Oct 27, 2023 · ca13bbd · ca13bbd
2 parents 1886909 + 4703df8
commit ca13bbd
Show file tree

Hide file tree

Showing 8 changed files with 116 additions and 106 deletions.
diff --git a/client/__tests__/treatmentsByFlagSets.test.js b/client/__tests__/treatmentsByFlagSets.test.js
@@ -2,6 +2,7 @@ const request = require('supertest');
 const app = require('../../app');
 const { expectError, expectErrorContaining, expectOkMultipleResults, getLongKey } = require('../../utils/testWrapper');
 const { NULL_FLAG_SETS, EMPTY_FLAG_SETS } = require('../../utils/constants');
+const { expectedGreenResults, expectedPurpleResults, expectedPinkResults } = require('../../utils/mocks');
 
 jest.mock('node-fetch', () => {
   return jest.fn().mockImplementation((url) => {
@@ -215,7 +216,7 @@ describe('get-treatments-by-sets', () => {
 
   test('should be 400 if attributes is an invalid json (POST)', async (done) => {
     const response = await request(app)
-      .post('/client/get-treatments-by-sets?key=test&set-name=my-experiment')
+      .post('/client/get-treatments-by-sets?key=test&flag-sets=my-experiment')
       .set('Content-Type', 'application/json')
       // eslint-disable-next-line no-useless-escape
       .send('\|\\\"/regex/i') // Syntax error parsing the JSON.
@@ -225,50 +226,6 @@ describe('get-treatments-by-sets', () => {
     done();
   });
 
-  const expectedGreenResults = {
-    'test_green': {
-      treatment: 'on',
-    },
-    'test_color': {
-      treatment: 'on',
-    },
-    'test_green_config': {
-      treatment: 'on',
-      config: undefined,
-    },
-  };
-  const expectedPurpleResults = {
-    'test_purple': {
-      treatment: 'on',
-    },
-    'test_color': {
-      treatment: 'on',
-    },
-    'test_purple_config': {
-      treatment: 'on',
-      config: undefined,
-    },
-  };
-  const expectedPinkResults = {
-    'test_purple': {
-      treatment: 'on',
-    },
-    'test_green': {
-      treatment: 'on',
-    },
-    'test_color': {
-      treatment: 'on',
-    },
-    'test_purple_config': {
-      treatment: 'on',
-      config: undefined,
-    },
-    'test_green_config': {
-      treatment: 'on',
-      config: undefined,
-    },
-  };
-
   test('should be 200 if is valid attributes (GET)', async (done) => {
     const response = await request(app)
       .get('/client/get-treatments-by-sets?key=key_green&flag-sets=set_green&attributes={"test":"test"}')

diff --git a/client/__tests__/treatmentsWithConfigByFlagSets.test.js b/client/__tests__/treatmentsWithConfigByFlagSets.test.js
@@ -2,6 +2,7 @@ const request = require('supertest');
 const app = require('../../app');
 const { expectError, expectErrorContaining, expectOkMultipleResults, getLongKey } = require('../../utils/testWrapper');
 const { NULL_FLAG_SETS, EMPTY_FLAG_SETS } = require('../../utils/constants');
+const { expectedGreenResultsWithConfig, expectedPurpleResultsWithConfig, expectedPinkResultsWithConfig } = require('../../utils/mocks');
 
 jest.mock('node-fetch', () => {
   return jest.fn().mockImplementation((url) => {
@@ -204,63 +205,19 @@ describe('get-treatments-with-config-by-sets', () => {
     done();
   });
 
-  const expectedGreenResults = {
-    'test_green': {
-      treatment: 'on',
-    },
-    'test_color': {
-      treatment: 'on',
-    },
-    'test_green_config': {
-      treatment: 'on',
-      config: '{"color":"green"}',
-    },
-  };
-  const expectedPurpleResults = {
-    'test_purple': {
-      treatment: 'on',
-    },
-    'test_color': {
-      treatment: 'on',
-    },
-    'test_purple_config': {
-      treatment: 'on',
-      config: '{"color":"purple"}',
-    },
-  };
-  const expectedPinkResults = {
-    'test_purple': {
-      treatment: 'on',
-    },
-    'test_green': {
-      treatment: 'on',
-    },
-    'test_color': {
-      treatment: 'on',
-    },
-    'test_purple_config': {
-      treatment: 'on',
-      config: '{"color":"purple"}',
-    },
-    'test_green_config': {
-      treatment: 'on',
-      config: '{"color":"green"}',
-    },
-  };
-
   test('should be 200 if is valid attributes (GET)', async (done) => {
     const response = await request(app)
       .get('/client/get-treatments-with-config-by-sets?key=key_green&flag-sets=set_green&attributes={"test":"test"}')
       .set('Authorization', 'key_green');
-    expectOkMultipleResults(response, 200, expectedGreenResults, 3);
+    expectOkMultipleResults(response, 200, expectedGreenResultsWithConfig, 3);
     done();
   });
 
   test('should be 200 when attributes is null (GET)', async (done) => {
     const response = await request(app)
       .get('/client/get-treatments-with-config-by-sets?key=key_green&flag-sets=set_green')
       .set('Authorization', 'key_green');
-    expectOkMultipleResults(response, 200, expectedGreenResults, 3);
+    expectOkMultipleResults(response, 200, expectedGreenResultsWithConfig, 3);
     done();
   });
 
@@ -269,7 +226,7 @@ describe('get-treatments-with-config-by-sets', () => {
       .post('/client/get-treatments-with-config-by-sets?key=key_green&flag-sets=set_green')
       .send({attributes: { test:'test' }})
       .set('Authorization', 'key_green');
-    expectOkMultipleResults(response, 200, expectedGreenResults, 3);
+    expectOkMultipleResults(response, 200, expectedGreenResultsWithConfig, 3);
     done();
   });
 
@@ -278,7 +235,7 @@ describe('get-treatments-with-config-by-sets', () => {
       .get('/client/get-treatments-with-config-by-sets?key=key_green&flag-sets=set_green&attributes={"test":"test"}')
       .send(JSON.stringify({attributes: { test:'test' }}))
       .set('Authorization', 'key_green');
-    expectOkMultipleResults(response, 200, expectedGreenResults, 3);
+    expectOkMultipleResults(response, 200, expectedGreenResultsWithConfig, 3);
     done();
   });
 
@@ -289,31 +246,31 @@ describe('get-treatments-with-config-by-sets', () => {
         attributes: null,
       })
       .set('Authorization', 'key_green');
-    expectOkMultipleResults(response, 200, expectedGreenResults, 3);
+    expectOkMultipleResults(response, 200, expectedGreenResultsWithConfig, 3);
     done();
   });
 
   test('should be 200 with multiple evaluation but evualuate configured flag sets', async (done) => {
     const response = await request(app)
       .get('/client/get-treatments-with-config-by-sets?key=key_green&flag-sets=set_green,set_purple,nonexistant-experiment')
       .set('Authorization', 'key_green');
-    expectOkMultipleResults(response, 200, expectedGreenResults, 3);
+    expectOkMultipleResults(response, 200, expectedGreenResultsWithConfig, 3);
     done();
   });
 
   test('should be 200 with multiple evaluation but evualuate configured flag sets', async (done) => {
     const response = await request(app)
       .get('/client/get-treatments-with-config-by-sets?key=key_purple&flag-sets=set_green,set_purple,nonexistant-experiment')
       .set('Authorization', 'key_purple');
-    expectOkMultipleResults(response, 200, expectedPurpleResults, 3);
+    expectOkMultipleResults(response, 200, expectedPurpleResultsWithConfig, 3);
     done();
   });
 
   test('should be 200 with multiple evaluation but evualuate configured flag sets', async (done) => {
     const response = await request(app)
       .get('/client/get-treatments-with-config-by-sets?key=key_purple&flag-sets=set_green,set_purple,nonexistant-experiment')
       .set('Authorization', 'key_pink');
-    expectOkMultipleResults(response, 200, expectedPinkResults, 5);
+    expectOkMultipleResults(response, 200, expectedPinkResultsWithConfig, 5);
     done();
   });
 });
diff --git a/client/client.router.js b/client/client.router.js
@@ -2,7 +2,7 @@ const express = require('express');
 const router = express.Router();
 const keyValidator = require('../utils/inputValidation/key');
 const splitValidator = require('../utils/inputValidation/split');
-const flagSetValidator = require('../utils/inputValidation/flagSet');
+const flagSetsValidator = require('../utils/inputValidation/flagSets');
 const splitsValidator = require('../utils/inputValidation/splits');
 const attributesValidator = require('../utils/inputValidation/attributes');
 const trafficTypeValidator = require('../utils/inputValidation/trafficType');
@@ -86,7 +86,7 @@ const treatmentsValidation = (req, res, next) => {
 const flagSetsValidation = (req, res, next) => {
   const matchingKeyValidation = keyValidator(req.query.key, 'key');
   const bucketingKeyValidation = req.query['bucketing-key'] !== undefined ? keyValidator(req.query['bucketing-key'], 'bucketing-key') : null;
-  const flagSetNameValidation = flagSetValidator(req.query['flag-sets']);
+  const flagSetNameValidation = flagSetsValidator(req.query['flag-sets']);
   const attributesValidation = attributesValidator(req.query.attributes);
 
   const error = parseValidators([matchingKeyValidation, bucketingKeyValidation, flagSetNameValidation, attributesValidation]);

diff --git a/utils/constants.js b/utils/constants.js
@@ -1,7 +1,10 @@
+const TRIMMABLE_SPACES_REGEX = /^[\s\uFEFF\xA0]+|[\s\uFEFF\xA0]+$/;
+
 const EMPTY_FLAG_SETS = 'you passed an empty flag-sets, flag-sets must be a non-empty array.';
 const NULL_FLAG_SETS = 'you passed a null or undefined flag-sets, flag-sets must be a non-empty array.';
 
 module.exports = {
+  TRIMMABLE_SPACES_REGEX,
   EMPTY_FLAG_SETS,
   NULL_FLAG_SETS,
 };
diff --git a/utils/inputValidation/flagSet.js b/utils/inputValidation/flagSet.js
@@ -1,19 +1,18 @@
-const { NULL_FLAG_SETS, EMPTY_FLAG_SETS } = require('../constants');
 const errorWrapper = require('./wrapper/error');
 const okWrapper = require('./wrapper/ok');
-const TRIMMABLE_SPACES_REGEX = /^[\s\uFEFF\xA0]+|[\s\uFEFF\xA0]+$/;
+const { NULL_FLAG_SET, EMPTY_FLAG_SET, TRIMMABLE_SPACES_REGEX } = require('../constants');
 
 const validateFlagSet = (maybeFlagSet) => {
   // eslint-disable-next-line
-  if (maybeFlagSet == undefined) return errorWrapper(NULL_FLAG_SETS);
+  if (maybeFlagSet == undefined) return errorWrapper(NULL_FLAG_SET);
 
   if (TRIMMABLE_SPACES_REGEX.test(maybeFlagSet)) {
     console.log(`flag-sets "${maybeFlagSet}" has extra whitespace, trimming.`);
     maybeFlagSet = maybeFlagSet.trim();
   }
-  if (maybeFlagSet.length === 0) return errorWrapper(EMPTY_FLAG_SETS);
+  if (maybeFlagSet.length === 0) return errorWrapper(EMPTY_FLAG_SET);
 
-  return okWrapper(maybeFlagSet.split(','));
+  return okWrapper(maybeFlagSet);
 };
 
 module.exports = validateFlagSet;
diff --git a/utils/inputValidation/flagSets.js b/utils/inputValidation/flagSets.js
@@ -0,0 +1,28 @@
+const errorWrapper = require('./wrapper/error');
+const okWrapper = require('./wrapper/ok');
+const lang = require('../lang');
+const validateFlagSet = require('./flagSet');
+const { EMPTY_FLAG_SETS, NULL_FLAG_SETS } = require('../constants');
+
+const validateFlagSets = (maybeFlagSets) => {
+  // eslint-disable-next-line eqeqeq
+  if (maybeFlagSets == undefined) return errorWrapper(NULL_FLAG_SETS);
+
+  maybeFlagSets = maybeFlagSets.split(',');
+
+  if (maybeFlagSets.length > 0) {
+    let validatedArray = [];
+    // Remove invalid values
+    maybeFlagSets.forEach(maybeFlagSet => {
+      const flagSetValidation = validateFlagSet(maybeFlagSet);
+      if (flagSetValidation.valid) validatedArray.push(flagSetValidation.value);
+    });
+
+    // Strip off duplicated values if we have valid flag sets then return
+    if (validatedArray.length) return okWrapper(lang.uniq(validatedArray));
+  }
+
+  return errorWrapper(EMPTY_FLAG_SETS);
+};
+
+module.exports = validateFlagSets;
diff --git a/utils/inputValidation/split.js b/utils/inputValidation/split.js
@@ -1,6 +1,6 @@
 const errorWrapper = require('./wrapper/error');
 const okWrapper = require('./wrapper/ok');
-const TRIMMABLE_SPACES_REGEX = /^[\s\uFEFF\xA0]+|[\s\uFEFF\xA0]+$/;
+const { TRIMMABLE_SPACES_REGEX } = require('../constants');
 
 const validateSplit = (maybeSplit) => {
   // eslint-disable-next-line eqeqeq

diff --git a/utils/mocks/index.js b/utils/mocks/index.js
@@ -82,4 +82,70 @@ const integrations = [{
   type: 'GOOGLE_ANALYTICS_TO_SPLIT',
 }];
 
-module.exports = { core, scheduler, urls, startup, storage, sync, integrations, apiKeyMocksMap };
+const expectedGreenResults = {
+  'test_green': {
+    treatment: 'on',
+  },
+  'test_color': {
+    treatment: 'on',
+  },
+  'test_green_config': {
+    treatment: 'on',
+  },
+};
+const expectedPurpleResults = {
+  'test_purple': {
+    treatment: 'on',
+  },
+  'test_color': {
+    treatment: 'on',
+  },
+  'test_purple_config': {
+    treatment: 'on',
+  },
+};
+const expectedPinkResults = {
+  ...expectedGreenResults,
+  ...expectedPurpleResults,
+};
+
+const expectedGreenResultsWithConfig = {
+  'test_green': {
+    treatment: 'on',
+  },
+  'test_color': {
+    treatment: 'on',
+  },
+  'test_green_config': {
+    treatment: 'on',
+    config: '{"color":"green"}',
+  },
+};
+
+const expectedPurpleResultsWithConfig = {
+  'test_purple': {
+    treatment: 'on',
+  },
+  'test_color': {
+    treatment: 'on',
+  },
+  'test_purple_config': {
+    treatment: 'on',
+    config: '{"color":"purple"}',
+  },
+};
+
+const expectedPinkResultsWithConfig = {
+  ...expectedGreenResultsWithConfig,
+  ...expectedPurpleResultsWithConfig,
+};
+
+module.exports = {
+  core, scheduler, urls, startup, storage, sync, integrations, apiKeyMocksMap,
+  expectedGreenResults,
+  expectedPurpleResults,
+  expectedPinkResults,
+  expectedGreenResultsWithConfig,
+  expectedPurpleResultsWithConfig,
+  expectedPinkResultsWithConfig,
+};