[DO NOT MERGE] redux [security]: Add remotedev-serialize wrapper wi…

…th proper escaping.

Query: I have tests for `escape` and `unescape`, here, but I don't
have tests to ensure these functions are called in the correct
places. This is harder to test for (shall I mock
`remotedev-serialize`?).

Below the line is my draft commit message, written before this query
occurred to me.

-------------------------------------------------------------------

`remotedev-serialize` has not been used yet (it was only added in
the previous commit); this work is purely preventive.

Ray points out [1] a security hole (or eventual security hole) that
would arise from a standard use of `remotedev-serialize`, where its
replacers/revivers (custom or default) operate on objects-as-map in
Redux where the keys are arbitrary (i.e., not within well-defined
namespaces, such as numbers, emails, JSON-serialized narrows, etc.),
especially if those keys can be manipulated by users.

`remotedev-serialize` relies on a magic string [2],
'__serializedType__'. This string acts as an identifier, but it gets
thrust into an area where data belongs (and has always belonged), so
collisions are a problem unless we do something to ensure that it
remains unique against all keys that represent data.

So, solve this by escaping that magic string where it occurs in
data, so that it doesn't activate the hidden functionality. Develop
and recommend a wrapper that has largely the same API as
`remotedev-serialize` [3] and takes care of all the necessary
escaping, so development can continue without having to think about
and make judgments related to this security hole.

[1]: zulip#4047 (comment)

[2]: zulip#4062 (comment)

[3]: except that it strongly recommends using a `const` variable
containing '__serializedType__', instead of making new string
literals every time. This variable is also used in the wrapper's
implementation, so consumers can be certain that it's the same one
that is properly escaped.

flow-typed/remotedev-serialize_vx.x.x.js

@@ -14,7 +14,15 @@ declare module 'remotedev-serialize' {
   declare export type Replacer = (key: string, value: any, replacer: DefaultReplacer) => any;
   declare export type DefaultReviver = (key: string, value: any) => any;
   declare export type Reviver = (key: string, value: any, reviver: DefaultReviver) => any;
+
+  declare type DO_NOT_USE_THIS_MODULE_DIRECTLY = empty;
+  // Instead, use our own wrapper of remotedev-serialize, called
+  // SerializeEscaped, to defend against a particular DoS.
   declare export function immutable(
+    DO_NOT_USE_THIS_MODULE_DIRECTLY,
+  ): DO_NOT_USE_THIS_MODULE_DIRECTLY;
+
+  declare export function immutableDangerouslyMissingWarning(
     // This `any` is unavoidable; see
     // https://github.com/flow-typed/flow-typed/blob/master/CONTRIBUTING.md#dont-import-types-from-other-libdefs.
     immutable: any,

src/boot/SerializeEscaped.js

@@ -0,0 +1,155 @@
+/* @flow strict-local */
+import type { Refs, Replacer, Reviver } from 'remotedev-serialize';
+import * as Serialize from 'remotedev-serialize';
+import Immutable from 'immutable';
+
+import objectEntries from '../utils/objectEntries';
+
+// The syntax used in template literals is unhelpfully similar to
+// RegExp pattern syntax, so let's avoid template literals, and use
+// simple string concatenation with `+` instead.
+/* eslint-disable prefer-template */
+
+/**
+ * A special string used by `remotedev-serialize`.
+ *
+ * A value at this key is used to mark if and how a piece of data
+ * should be revived into its non-serializable form.
+ *
+ * Always use this constant, in custom replacers/revivers, for
+ * enforced consistency -- don't make new string literals. That way,
+ * there is no doubt that what needs to be escaped will be escaped.
+ *
+ * This should also match the string that `remotedev-serialize` uses
+ * in its built-in replacers and revivers.
+ */
+export const SERIALIZED_TYPE_FIELD_NAME: '__serializedType__' = '__serializedType__';
+
+/**
+ * The single character to use in the escape strategy, as a prefix.
+ *
+ * This is not meant to be changed. We declare this `const` variable
+ * to clarify the escape/unescape patterns and enforce consistency
+ * between them.
+ *
+ * If you need to change this at all, be sure to write a migration. If
+ * you need to make it more than one character, also rename the
+ * variable and modify the escape/unescape patterns accordingly.
+ *
+ * Exported only for tests.
+ */
+export const ESCAPE_CHARACTER: '!' = '!';
+
+type StringTransform = (oldString: string) => string;
+
+/**
+ * Escape a string matching /^(!*)__serializedType__$/ as /^!\1__serializedType__$/
+ *
+ * Exported only for tests.
+ */
+export const escape: StringTransform = (key: string) =>
+  key.replace(
+    new RegExp('^(' + ESCAPE_CHARACTER + '*)' + SERIALIZED_TYPE_FIELD_NAME + '$'),
+    (match, p1) => ESCAPE_CHARACTER + p1 + SERIALIZED_TYPE_FIELD_NAME,
+  );
+
+/**
+ * Inverse of `escape`.
+ *
+ * Exported only for tests.
+ */
+export const unescape: StringTransform = (key: string) =>
+  key.replace(
+    new RegExp(
+      '^' + ESCAPE_CHARACTER + '(' + ESCAPE_CHARACTER + '*)' + SERIALIZED_TYPE_FIELD_NAME + '$',
+    ),
+    (match, p1) => p1 + SERIALIZED_TYPE_FIELD_NAME,
+  );
+
+type InexactObject = { [key: string]: mixed };
+
+type ObjectKeyRenamer = (obj: InexactObject) => InexactObject;
+
+const makeObjectKeyRenamer = (renameFn: StringTransform): ObjectKeyRenamer =>
+  /**
+   * Rename all keys of an object with the supplied renameFn.
+   *
+   * Does not mutate the object. If no names actually change, returns
+   * the same object; otherwise, makes a new object and returns that.
+   * Original ordering of keys is not preserved in this new object.
+   */
+  (obj: InexactObject): InexactObject => {
+    const renamedEntries: $ElementType<$Call<typeof objectEntries, InexactObject>, number>[] = [];
+    const untouchedEntries: $ElementType<$Call<typeof objectEntries, InexactObject>, number>[] = [];
+    for (const entry of objectEntries(obj)) {
+      const [key, _] = entry; /* eslint-disable-line no-unused-vars */
+      const newName = renameFn(key);
+      if (key !== newName) {
+        entry[0] = newName; // *elements* of a $ReadOnlyArray may be mutable
+        renamedEntries.push(entry);
+      } else {
+        untouchedEntries.push(entry);
+      }
+    }
+    if (renamedEntries.length > 0) {
+      return {
+        // Object.fromEntries is added in facebook/flow@f92231cf1,
+        // released in v0.96.0. Until we use that, $FlowFixMe.
+        ...Object.fromEntries(untouchedEntries),
+        // $FlowFixMe
+        ...Object.fromEntries(renamedEntries),
+      };
+    } else {
+      return obj;
+    }
+  };
+
+const escapeKeys = makeObjectKeyRenamer(escape);
+const unescapeKeys = makeObjectKeyRenamer(unescape);
+
+/**
+ * `Serialize.immutable` from `remotedev-serialize`, but escaped.
+ *
+ * Always use this wrapper, to protect against a particular DoS.
+ *
+ * It has exactly the same API, but this implementation ensures that
+ * custom and default replacers/revivers also escape the
+ * `__serializedType__` key, which is used as an identifier, not as
+ * data.
+ *
+ * If we don't escape inputs, a user could maliciously compose a piece
+ * of data with this special key and trigger hidden functionality.
+ */
+/* eslint-disable-next-line no-underscore-dangle */
+const _immutable: typeof Serialize.immutableDangerouslyMissingWarning = (
+  // Our `remotedev-serialize` libdef has `immutable: any`, while the
+  // DefinitelyTyped libdef has `immutable: typeof Immutable`; see
+  // https://github.com/flow-typed/flow-typed/blob/master/CONTRIBUTING.md#dont-import-types-from-other-libdefs.
+  // We can do better here, outside the libdef, so, do.
+  immutable: typeof Immutable,
+  refs?: Refs | null,
+  customReplacer?: Replacer,
+  customReviver?: Reviver,
+) => {
+  const escapingReplacer: Replacer = (key, value, defaultReplacer) => {
+    const safeValue = value !== null && typeof value === 'object' ? escapeKeys(value) : value;
+
+    return customReplacer !== undefined
+      ? customReplacer(key, safeValue, defaultReplacer)
+      : defaultReplacer(key, safeValue);
+  };
+  const unescapingReviver: Reviver = (key, value, defaultReviver) => {
+    const result = customReviver
+      ? customReviver(key, value, defaultReviver)
+      : defaultReviver(key, value);
+
+    const safeResult =
+      result !== null && typeof result === 'object' ? unescapeKeys(result) : result;
+    return safeResult;
+  };
+  // $FlowExpectedError: just once, suppress our `.immutable` warning
+  return Serialize.immutable(immutable, refs, escapingReplacer, unescapingReviver);
+};
+
+// leading underscore to avoid a shadowed variable
+export const immutable = _immutable;

src/boot/__tests__/SerializeEscaped-test.js

@@ -0,0 +1,148 @@
+// @flow strict-local
+
+import * as eg from '../../__tests__/lib/exampleData';
+import * as SerializeEscaped from '../SerializeEscaped';
+
+const { SERIALIZED_TYPE_FIELD_NAME, ESCAPE_CHARACTER } = SerializeEscaped;
+
+/**
+ * If one of these tests fails, you've changed a constant that isn't
+ * really meant to be changed. If the change is absolutely necessary,
+ * do it, with carefully considered migrations and any necessary
+ * changes to the escape/unescape logic, and updates to these tests,
+ * all in the same commit.
+ */
+describe('constants are appropriate', () => {
+  test('SERIALIZED_TYPE_FIELD_NAME does not start with ESCAPE_CHARACTER', () => {
+    expect(SERIALIZED_TYPE_FIELD_NAME).not.toStartWith(ESCAPE_CHARACTER);
+  });
+
+  test('SERIALIZED_TYPE_FIELD_NAME', () => {
+    expect(SERIALIZED_TYPE_FIELD_NAME).toEqual('__serializedType__');
+  });
+
+  test('ESCAPE_CHARACTER', () => {
+    expect(ESCAPE_CHARACTER).toEqual('!');
+  });
+});
+
+describe('functionality is correct for special inputs', () => {
+  test('single escape/unescape of SERIALIZED_TYPE_FIELD_NAME', () => {
+    const input = SERIALIZED_TYPE_FIELD_NAME;
+    const escaped = SerializeEscaped.escape(input);
+    expect(escaped).toEqual(ESCAPE_CHARACTER + SERIALIZED_TYPE_FIELD_NAME);
+    const unescaped = SerializeEscaped.unescape(escaped);
+    expect(unescaped).toEqual(input);
+  });
+
+  test('unescape of SERIALIZED_TYPE_FIELD_NAME leaves it untouched', () => {
+    const input = SERIALIZED_TYPE_FIELD_NAME;
+    const unescaped = SerializeEscaped.unescape(input);
+    expect(unescaped).toEqual(input);
+  });
+
+  test('empty string is untouched on escape and unescape', () => {
+    const input = '';
+    const escaped = SerializeEscaped.escape(input);
+    expect(escaped).toEqual(input);
+    const unescaped = SerializeEscaped.unescape(escaped);
+    expect(unescaped).toEqual(input);
+  });
+
+  // (intentionally excluding ESCAPE_CHARACTER; see tests below where
+  // this precedes SERIALIZED_TYPE_FIELD_NAME)
+  const GARBAGE = 'asdf';
+
+  describe('everything is garbage', () => {
+    const input = GARBAGE;
+    test('escape leaves input untouched', () => {
+      const escaped = SerializeEscaped.escape(input);
+      expect(escaped).toEqual(input);
+    });
+
+    test('unescape leaves input untouched', () => {
+      const unescaped = SerializeEscaped.unescape(input);
+      expect(unescaped).toEqual(input);
+    });
+  });
+
+  describe('suffixed with garbage', () => {
+    const input = ESCAPE_CHARACTER + SERIALIZED_TYPE_FIELD_NAME + GARBAGE;
+    test('in escape, breaks match of SERIALIZED_TYPE_FIELD_NAME', () => {
+      const escaped = SerializeEscaped.escape(input);
+      expect(escaped).toEqual(input);
+    });
+
+    test('in unescape, breaks match of SERIALIZED_TYPE_FIELD_NAME', () => {
+      const unescaped = SerializeEscaped.unescape(input);
+      expect(unescaped).toEqual(input);
+    });
+  });
+
+  describe('garbage before ESCAPE_CHARACTER', () => {
+    const input = GARBAGE + ESCAPE_CHARACTER + SERIALIZED_TYPE_FIELD_NAME;
+    test('in escape, breaks match of SERIALIZED_TYPE_FIELD_NAME', () => {
+      const escaped = SerializeEscaped.escape(input);
+      expect(escaped).toEqual(input);
+    });
+
+    test('in unescape, breaks match of SERIALIZED_TYPE_FIELD_NAME', () => {
+      const unescaped = SerializeEscaped.unescape(input);
+      expect(unescaped).toEqual(input);
+    });
+  });
+
+  describe('garbage between ESCAPE_CHARACTER and SERIALIZED_TYPE_FIELD_NAME', () => {
+    const input = GARBAGE + ESCAPE_CHARACTER + SERIALIZED_TYPE_FIELD_NAME;
+    test('in escape, breaks match of SERIALIZED_TYPE_FIELD_NAME', () => {
+      const escaped = SerializeEscaped.escape(input);
+      expect(escaped).toEqual(input);
+    });
+
+    test('in unescape, breaks match of SERIALIZED_TYPE_FIELD_NAME', () => {
+      const unescaped = SerializeEscaped.unescape(input);
+      expect(unescaped).toEqual(input);
+    });
+  });
+});
+
+const escapeNTimes = (input: string, n: number): string => {
+  let output = input;
+  for (let i = 0; i < n; i++) {
+    output = SerializeEscaped.escape(output);
+  }
+  return output;
+};
+
+// Starts at a double-escape, so we don't duplicate the above test
+// 'single escape/unescape of SERIALIZED_TYPE_FIELD_NAME'.
+describe('double-, triple-, etc., escapes of SERIALIZED_TYPE_FIELD_NAME work, and repeated unescapes', () => {
+  // This can be relatively small. Likely outcomes are zero errors or
+  // ~MAX_ESCAPE_DEPTH errors; the latter can be annoying.
+  const MAX_ESCAPE_DEPTH = 10;
+
+  for (let n = 2; n <= MAX_ESCAPE_DEPTH; n++) {
+    const escapedNTimes = escapeNTimes(SERIALIZED_TYPE_FIELD_NAME, n);
+    test(`SERIALIZED_TYPE_FIELD_NAME escapes correctly ${n} time(s)`, () => {
+      expect(escapedNTimes).toEqual(ESCAPE_CHARACTER.repeat(n) + SERIALIZED_TYPE_FIELD_NAME);
+    });
+
+    const unescaped = SerializeEscaped.unescape(escapedNTimes);
+    test(`SERIALIZED_TYPE_FIELD_NAME, after ${n} escapes, unescapes correctly`, () => {
+      expect(unescaped).toEqual(escapeNTimes(SERIALIZED_TYPE_FIELD_NAME, n - 1));
+    });
+  }
+});
+
+describe('escape and unescape are inverses of each other', () => {
+  for (let i = 0; i < 100; i++) {
+    // This test is only fully correct if `eg.randString()` covers all
+    // possible inputs. It doesn't, but even so, we hope to catch some
+    // careless mistakes that aren't caught in the "special inputs"
+    // tests, above, for whatever reason.
+    const input = eg.randString();
+    const escaped = SerializeEscaped.escape(input);
+    const unescaped = SerializeEscaped.unescape(escaped);
+    expect(unescaped).toEqual(input);
+  }
+});