Merge pull request #1962 from hypothesis/hypothesis-confg-casting

Add translation map for specific host-config values

src/shared/test/type-coercions-test.js

+import { toBoolean, toInteger, toObject, toString } from '../type-coercions';
+
+describe('shared/type-coercions', () => {
+  describe('toBoolean', () => {
+    [
+      {
+        value: true,
+        result: true,
+      },
+      {
+        value: 'true',
+        result: true,
+      },
+      {
+        value: 'any',
+        result: true,
+      },
+      {
+        value: 'false',
+        result: false,
+      },
+      {
+        value: ' false',
+        result: false,
+      },
+      {
+        value: 'False',
+        result: false,
+      },
+      {
+        value: 'FALSE',
+        result: false,
+      },
+      {
+        value: '',
+        result: false,
+      },
+      {
+        value: '1',
+        result: true,
+      },
+      {
+        value: '0',
+        result: false,
+      },
+      {
+        value: 1,
+        result: true,
+      },
+      {
+        value: null,
+        result: false,
+      },
+      {
+        value: undefined,
+        result: false,
+      },
+    ].forEach(test => {
+      it('coerces the values appropriately', () => {
+        assert.equal(toBoolean(test.value), test.result);
+      });
+    });
+  });
+
+  describe('toInteger', () => {
+    [
+      {
+        value: '1',
+        result: 1,
+      },
+      {
+        value: '0',
+        result: 0,
+      },
+      {
+        value: 1,
+        result: 1,
+      },
+      {
+        value: 1.1,
+        result: 1,
+      },
+      {
+        value: 'a',
+        result: NaN,
+      },
+    ].forEach(test => {
+      it('coerces the values appropriately', () => {
+        assert.deepEqual(toInteger(test.value), test.result);
+      });
+    });
+  });
+
+  describe('toObject', () => {
+    [
+      {
+        value: { a: 'a', b: { c: ['c'] } },
+        result: { a: 'a', b: { c: ['c'] } },
+      },
+      {
+        value: 1,
+        result: {},
+      },
+      {
+        value: null,
+        result: {},
+      },
+      {
+        value: undefined,
+        result: {},
+      },
+    ].forEach(test => {
+      it('coerces the values appropriately', () => {
+        assert.deepEqual(toObject(test.value), test.result);
+      });
+    });
+  });
+
+  describe('toString', () => {
+    [
+      {
+        value: 'a',
+        result: 'a',
+      },
+      {
+        value: 1,
+        result: '1',
+      },
+      {
+        value: null,
+        result: '',
+      },
+      {
+        value: undefined,
+        result: '',
+      },
+      {
+        value: {
+          // In a rare case where its an object with custom
+          // toString value that is not a function.
+          toString: false,
+        },
+        result: '',
+      },
+    ].forEach(test => {
+      it('coerces the values appropriately', () => {
+        assert.equal(toString(test.value), test.result);
+      });
+    });
+  });
+});

src/shared/type-coercions.js

+/**
+ * Type conversion methods that coerce incoming configuration values to an
+ * expected type or format that other parts of the UI may make assumptions
+ * on. This is needed for incoming configuration values that are otherwise
+ * not sanitized.
+ *
+ * Note that if the values passed are plain javascript values (such as ones
+ * produced from JSON.parse), then these methods do not throw errors.
+ */
+
+/**
+ * Returns a boolean
+ *
+ * @param {any} value - initial value
+ * @return {boolean}
+ */
+export function toBoolean(value) {
+  if (typeof value === 'string') {
+    if (value.trim().toLocaleLowerCase() === 'false') {
+      // "false", "False", " false", "FALSE" all return false
+      return false;
+    }
+  }
+  const numericalVal = Number(value);
+  if (!isNaN(numericalVal)) {
+    return Boolean(numericalVal);
+  }
+  // Any non numerical or falsely string should return true, otherwise return false
+  return typeof value === 'string';
+}
+
+/**
+ * Returns either an integer or NaN
+ *
+ * @param {any} value - initial value
+ * @return {number}
+ */
+export function toInteger(value) {
+  // Acts as a simple wrapper
+  return parseInt(value);
+}
+
+/**
+ * Returns either the value if its an object or an empty object
+ *
+ * @param {any} value - initial value
+ * @return {object}
+ */
+export function toObject(value) {
+  if (typeof value === 'object' && value !== null) {
+    return value;
+  }
+  // Don't attempt to fix the values, just ensure type correctness
+  return {};
+}
+
+/**
+ * Returns the value as a string or an empty string if the
+ * value undefined, null or otherwise falsely.
+ *
+ * @param {any} value - initial value
+ * @return {string}
+ */
+export function toString(value) {
+  if (value && typeof value.toString === 'function') {
+    return value.toString();
+  }
+  return '';
+}

src/sidebar/host-config.js

 import * as queryString from 'query-string';
+import {
+  toBoolean,
+  toInteger,
+  toObject,
+  toString,
+} from '../shared/type-coercions';
 
 /**
  * Return the app configuration specified by the frame embedding the Hypothesis
@@ -50,13 +56,52 @@ export default function hostPageConfig(window) {
     'usernameUrl',
   ];
 
+  // We need to coerce incoming values from the host config for 2 reasons:
+  //
+  // 1. New versions of via may no longer support passing any type other than
+  // string and our client is set up to expect values that are in fact not a
+  // string in some cases. This will help cast these values to the expected
+  // type if they can be.
+  //
+  // 2. A publisher of our sidebar could accidentally pass un-sanitized values
+  // into the config and this ensures they safely work downstream even if they
+  // are incorrect.
+  //
+  // Currently we are only handling the following config values do to the fact
+  // that via3 will soon discontinue passing boolean types or integer types.
+  //  - requestConfigFromFrame
+  //  - openSidebar
+  //
+  // It is assumed we should expand this list and coerce and eventually
+  // even validate all such config values.
+  // See https://github.com/hypothesis/client/issues/1968
+  const coercions = {
+    openSidebar: toBoolean,
+    requestConfigFromFrame: value => {
+      if (typeof value === 'string') {
+        // Legacy `requestConfigFromFrame` value which holds only the origin.
+        return value;
+      }
+      const objectVal = toObject(value);
+      return {
+        origin: toString(objectVal.origin),
+        ancestorLevel: toInteger(objectVal.ancestorLevel),
+      };
+    },
+  };
+
   return Object.keys(config).reduce(function(result, key) {
     if (paramWhiteList.indexOf(key) !== -1) {
       // Ignore `null` values as these indicate a default value.
       // In this case the config value set in the sidebar app HTML config is
       // used.
       if (config[key] !== null) {
-        result[key] = config[key];
+        if (coercions[key]) {
+          // If a coercion method exists, pass it through
+          result[key] = coercions[key](config[key]);
+        } else {
+          result[key] = config[key];
+        }
       }
     }
     return result;

src/sidebar/test/host-config-test.js

@@ -39,6 +39,24 @@ describe('hostPageConfig', function() {
     });
   });
 
+  it('coerces `requestConfigFromFrame` and `openSidebar` values', function() {
+    const window_ = fakeWindow({
+      openSidebar: 'false',
+      requestConfigFromFrame: {
+        origin: 'origin',
+        ancestorLevel: '2',
+      },
+    });
+
+    assert.deepEqual(hostPageConfig(window_), {
+      openSidebar: false,
+      requestConfigFromFrame: {
+        origin: 'origin',
+        ancestorLevel: 2,
+      },
+    });
+  });
+
   it('ignores non-whitelisted config params', function() {
     const window_ = fakeWindow({
       apiUrl: 'https://not-the-hypothesis/api/',