userID: better config validation

modules/userId/index.js

@@ -131,7 +131,7 @@ import {config} from '../../src/config.js';
 import * as events from '../../src/events.js';
 import {getGlobal} from '../../src/prebidGlobal.js';
 import adapterManager, {gdprDataHandler} from '../../src/adapterManager.js';
-import { EVENTS } from '../../src/constants.js';
+import {EVENTS} from '../../src/constants.js';
 import {module, ready as hooksReady} from '../../src/hook.js';
 import {buildEidPermissions, createEidsArray, EID_CONFIG} from './eids.js';
 import {
@@ -147,7 +147,6 @@ import {
   getPrebidInternal,
   isArray,
   isEmpty,
-  isEmptyStr,
   isFn,
   isGptPubadsDefined,
   isNumber,
@@ -951,29 +950,40 @@ function hasValidStorageTypes(config) {
  * @param {SubmoduleConfig[]} configRegistry
  * @returns {SubmoduleConfig[]}
  */
-function getValidSubmoduleConfigs(configRegistry) {
+export function getValidSubmoduleConfigs(configRegistry) {
+  function err(msg, ...args) {
+    logWarn(`Invalid userSync.userId config: ${msg}`, ...args)
+  }
   if (!Array.isArray(configRegistry)) {
+    if (configRegistry != null) {
+      err('must be an array', configRegistry);
+    }
     return [];
   }
-  return configRegistry.reduce((carry, config) => {
-    // every submodule config obj must contain a valid 'name'
-    if (!config || isEmptyStr(config.name)) {
-      return carry;
-    }
-    // Validate storage config contains 'type' and 'name' properties with non-empty string values
-    // 'type' must be one of html5, cookies
-    if (config.storage &&
-      !isEmptyStr(config.storage.type) &&
-      !isEmptyStr(config.storage.name) &&
-      hasValidStorageTypes(config)) {
-      carry.push(config);
-    } else if (isPlainObject(config.value)) {
-      carry.push(config);
-    } else if (!config.storage && !config.value) {
-      carry.push(config);
+  return configRegistry.filter(config => {
+    if (!config?.name) {
+      return err('must specify "name"', config);
+    } else if (config.storage) {
+      if (!config.storage.name || !config.storage.type) {
+        return err('must specify "storage.name" and "storage.type"', config);
+      } else if (!hasValidStorageTypes(config)) {
+        return err('invalid "storage.type"', config)
+      }
+      ['expires', 'refreshInSeconds'].forEach(param => {
+        let value = config.storage[param];
+        if (value != null && typeof value !== 'number') {
+          value = Number(value)
+          if (isNaN(value)) {
+            err(`storage.${param} must be a number and will be ignored`, config);
+            delete config.storage[param];
+          } else {
+            config.storage[param] = value;
+          }
+        }
+      });
     }
-    return carry;
-  }, []);
+    return true;
+  })
 }
 
 const ALL_STORAGE_TYPES = new Set([LOCAL_STORAGE, COOKIE]);

test/spec/modules/userId_spec.js

@@ -4,7 +4,7 @@ import {
   coreStorage,
   dep,
   findRootDomain,
-  getConsentHash,
+  getConsentHash, getValidSubmoduleConfigs,
   init,
   PBJS_USER_ID_OPTOUT_NAME,
   requestBidsHook,
@@ -190,6 +190,65 @@ describe('User ID', function () {
     })
   })
 
+  describe('userId config validation', () => {
+    beforeEach(() => {
+      sandbox.stub(utils, 'logWarn');
+    });
+
+    function mockConfig(storageConfig = {}) {
+      return {
+        name: 'mockModule',
+        storage: {
+          name: 'mockStorage',
+          type: 'cookie',
+          ...storageConfig
+        }
+      }
+    }
+
+    Object.entries({
+      'not an object': 'garbage',
+      'missing name': {},
+      'empty name': {name: ''},
+      'empty storage config': {name: 'mockId', storage: {}},
+      'storage type, but no storage name': mockConfig({name: ''}),
+      'storage name, but no storage type': mockConfig({type: undefined}),
+    }).forEach(([t, config]) => {
+      it(`should log a warning and reject configuration with ${t}`, () => {
+        expect(getValidSubmoduleConfigs([config]).length).to.equal(0);
+        sinon.assert.called(utils.logWarn);
+      });
+    });
+
+    it('should reject non-array userId configuration', () => {
+      expect(getValidSubmoduleConfigs({})).to.eql([]);
+      sinon.assert.called(utils.logWarn);
+    });
+
+    it('should accept null configuration', () => {
+      expect(getValidSubmoduleConfigs()).to.eql([]);
+      sinon.assert.notCalled(utils.logWarn);
+    });
+
+    ['refreshInSeconds', 'expires'].forEach(param => {
+      describe(`${param} parameter`, () => {
+        it('should be made a number, when possible', () => {
+          expect(getValidSubmoduleConfigs([mockConfig({[param]: '123'})])[0].storage[param]).to.equal(123);
+        });
+
+        it('should log a warning when not a number', () => {
+          expect(getValidSubmoduleConfigs([mockConfig({[param]: 'garbage'})])[0].storage[param]).to.not.exist;
+          sinon.assert.called(utils.logWarn)
+        });
+
+        it('should be left untouched when not specified', () => {
+          expect(getValidSubmoduleConfigs([mockConfig()])[0].storage[param]).to.not.exist;
+          sinon.assert.notCalled(utils.logWarn);
+        });
+      })
+    })
+  })
+
   describe('Decorate Ad Units', function () {
     beforeEach(function () {
       // reset mockGpt so nothing else interferes
@@ -2173,60 +2232,40 @@ describe('User ID', function () {
       });
     });
 
-    describe('Set cookie behavior', function () {
-      let cookie, cookieStub;
-
-      beforeEach(function () {
-        setSubmoduleRegistry([sharedIdSystemSubmodule]);
-        init(config);
-        cookie = document.cookie;
-        cookieStub = sinon.stub(document, 'cookie');
-        cookieStub.get(() => cookie);
-        cookieStub.set((val) => cookie = val);
-      });
-
-      afterEach(function () {
-        cookieStub.restore();
-      });
-
-      it('should allow submodules to override the domain', function () {
-        const submodule = {
-          submodule: {
-            domainOverride: function () {
-              return 'foo.com'
-            }
-          },
+    describe('Submodule ID storage', () => {
+      let submodule;
+      beforeEach(() => {
+        submodule = {
+          submodule: {},
           config: {
             name: 'mockId',
-            storage: {
-              type: 'cookie'
-            }
           },
           storageMgr: {
-            setCookie: sinon.stub()
+            setCookie: sinon.stub(),
+            setDataInLocalStorage: sinon.stub()
           },
-          enabledStorageTypes: [ 'cookie' ]
+          enabledStorageTypes: ['cookie', 'html5']
         }
-        setStoredValue(submodule, 'bar');
-        expect(submodule.storageMgr.setCookie.getCall(0).args[4]).to.equal('foo.com');
       });
 
-      it('should pass no domain if submodule does not override the domain', function () {
-        const submodule = {
-          submodule: {},
-          config: {
-            name: 'mockId',
-            storage: {
-              type: 'cookie'
-            }
-          },
-          storageMgr: {
-            setCookie: sinon.stub()
-          },
-          enabledStorageTypes: [ 'cookie' ]
-        }
-        setStoredValue(submodule, 'bar');
-        expect(submodule.storageMgr.setCookie.getCall(0).args[4]).to.equal(null);
+      describe('Set cookie behavior', function () {
+        beforeEach(() => {
+          submodule.config.storage = {
+            type: 'cookie'
+          }
+        });
+        it('should allow submodules to override the domain', function () {
+          submodule.submodule.domainOverride = function() {
+            return 'foo.com'
+          }
+          setStoredValue(submodule, 'bar');
+          expect(submodule.storageMgr.setCookie.getCall(0).args[4]).to.equal('foo.com');
+        });
+
+        it('should pass no domain if submodule does not override the domain', function () {
+          setStoredValue(submodule, 'bar');
+          expect(submodule.storageMgr.setCookie.getCall(0).args[4]).to.equal(null);
+        });
       });
     });