UserID module: graceful handling of exceptions from ID submodules (#8401

) * Do not error out when malformed JSON is set in cookies * UserID module: graceful handling of exceptions from ID submodules
prebid · May 10, 2022 · 04fefef · 04fefef
1 parent 8a4fd44
commit 04fefef
Show file tree

Hide file tree

Showing 5 changed files with 104 additions and 9 deletions.
diff --git a/modules/id5IdSystem.js b/modules/id5IdSystem.js
@@ -5,7 +5,16 @@
  * @requires module:modules/userId
  */
 
-import { deepAccess, logInfo, deepSetValue, logError, isEmpty, isEmptyStr, logWarn } from '../src/utils.js';
+import {
+  deepAccess,
+  logInfo,
+  deepSetValue,
+  logError,
+  isEmpty,
+  isEmptyStr,
+  logWarn,
+  safeJSONParse
+} from '../src/utils.js';
 import { ajax } from '../src/ajax.js';
 import { submodule } from '../src/hook.js';
 import { getRefererInfo } from '../src/refererDetection.js';
@@ -24,7 +33,7 @@ const LOG_PREFIX = 'User ID - ID5 submodule: ';
 // cookie in the array is the most preferred to use
 const LEGACY_COOKIE_NAMES = [ 'pbjs-id5id', 'id5id.1st', 'id5id' ];
 
-const storage = getStorageManager({gvlid: GVLID, moduleName: MODULE_NAME});
+export const storage = getStorageManager({gvlid: GVLID, moduleName: MODULE_NAME});
 
 /** @type {Submodule} */
 export const id5IdSubmodule = {
@@ -253,7 +262,7 @@ function getLegacyCookieSignature() {
   let legacyStoredValue;
   LEGACY_COOKIE_NAMES.forEach(function(cookie) {
     if (storage.getCookie(cookie)) {
-      legacyStoredValue = JSON.parse(storage.getCookie(cookie)) || legacyStoredValue;
+      legacyStoredValue = safeJSONParse(storage.getCookie(cookie)) || legacyStoredValue;
     }
   });
   return (legacyStoredValue && legacyStoredValue.signature) || '';

diff --git a/modules/userId/index.js b/modules/userId/index.js
@@ -423,7 +423,7 @@ function processSubmoduleCallbacks(submodules, cb) {
     }, submodules.length);
   }
   submodules.forEach(function (submodule) {
-    submodule.callback(function callbackCompleted(idObj) {
+    function callbackCompleted(idObj) {
       // if valid, id data should be saved to cookie/html storage
       if (idObj) {
         if (submodule.config.storage) {
@@ -435,8 +435,13 @@ function processSubmoduleCallbacks(submodules, cb) {
         logInfo(`${MODULE_NAME}: ${submodule.submodule.name} - request id responded with an empty value`);
       }
       done();
-    });
-
+    }
+    try {
+      submodule.callback(callbackCompleted);
+    } catch (e) {
+      logError(`Error in userID module '${submodule.submodule.name}':`, e);
+      done();
+    }
     // clear callback, this prop is used to test if all submodule callbacks are complete below
     submodule.callback = undefined;
   });
@@ -881,8 +886,12 @@ function initSubmodules(dest, submodules, consentData, forceRefresh = false) {
   setStoredConsentData(consentData);
 
   const initialized = userIdModules.reduce((carry, submodule) => {
-    populateSubmoduleId(submodule, consentData, storedConsentData, forceRefresh);
-    carry.push(submodule);
+    try {
+      populateSubmoduleId(submodule, consentData, storedConsentData, forceRefresh);
+      carry.push(submodule);
+    } catch (e) {
+      logError(`Error in userID module '${submodule.submodule.name}':`, e);
+    }
     return carry;
   }, []);
   if (initialized.length) {

diff --git a/src/utils.js b/src/utils.js
@@ -1364,3 +1364,14 @@ export function cyrb53Hash(str, seed = 0) {
 export function getWindowFromDocument(doc) {
   return (doc) ? doc.defaultView : null;
 }
+
+/**
+ * returns the result of `JSON.parse(data)`, or undefined if that throws an error.
+ * @param data
+ * @returns {any}
+ */
+export function safeJSONParse(data) {
+  try {
+    return JSON.parse(data);
+  } catch (e) {}
+}
diff --git a/test/spec/modules/id5IdSystem_spec.js b/test/spec/modules/id5IdSystem_spec.js
@@ -5,7 +5,7 @@ import {
   ID5_PRIVACY_STORAGE_NAME,
   ID5_STORAGE_NAME,
   id5IdSubmodule,
-  nbCacheName,
+  nbCacheName, storage,
   storeInLocalStorage,
   storeNbInCache,
 } from 'modules/id5IdSystem.js';
@@ -310,6 +310,21 @@ describe('ID5 ID System', function() {
       request.respond(200, responseHeader, JSON.stringify(ID5_JSON_RESPONSE));
       expect(getFromLocalStorage(ID5_PRIVACY_STORAGE_NAME)).to.be.null;
     });
+
+    describe('when legacy cookies are set', () => {
+      let sandbox;
+      beforeEach(() => {
+        sandbox = sinon.sandbox.create();
+        sandbox.stub(storage, 'getCookie');
+      });
+      afterEach(() => {
+        sandbox.restore();
+      });
+      it('should not throw if malformed JSON is forced into cookies', () => {
+        storage.getCookie.callsFake(() => ' Not JSON ');
+        id5IdSubmodule.getId(getId5FetchConfig());
+      });
+    })
   });
 
   describe('Request Bids Hook', function() {

diff --git a/test/spec/modules/userId_spec.js b/test/spec/modules/userId_spec.js
@@ -507,6 +507,57 @@ describe('User ID', function () {
       });
     });
 
+    describe('when ID systems throw errors', () => {
+      function mockIdSystem(name) {
+        return {
+          name,
+          decode: function(value) {
+            return {
+              [name]: value
+            };
+          },
+          getId: sinon.stub().callsFake(() => ({id: name}))
+        };
+      }
+      let id1, id2;
+      beforeEach(() => {
+        id1 = mockIdSystem('mock1');
+        id2 = mockIdSystem('mock2');
+        init(config);
+        setSubmoduleRegistry([id1, id2]);
+        config.setConfig({
+          userSync: {
+            auctionDelay: 10,
+            userIds: [{
+              name: 'mock1',
+              storage: {name: 'mock1', type: 'cookie'}
+            }, {
+              name: 'mock2',
+              storage: {name: 'mock2', type: 'cookie'}
+            }]
+          }
+        })
+      });
+      afterEach(() => {
+        config.resetConfig();
+      })
+      Object.entries({
+        'in init': () => id1.getId.callsFake(() => { throw new Error() }),
+        'in callback': () => {
+          const mockCallback = sinon.stub().callsFake(() => { throw new Error() });
+          id1.getId.callsFake(() => ({callback: mockCallback}))
+        }
+      }).forEach(([t, setup]) => {
+        describe(`${t}`, () => {
+          beforeEach(setup);
+          it('should still retrieve IDs that do not throw', () => {
+            return getGlobal().getUserIdsAsync().then((uid) => {
+              expect(uid.mock2).to.not.be.undefined;
+            })
+          });
+        })
+      })
+    });
     it('pbjs.refreshUserIds updates submodules', function(done) {
       let sandbox = sinon.createSandbox();
       let mockIdCallback = sandbox.stub().returns({id: {'MOCKID': '1111'}});