fix: Disallow unregistered classes in JSON RPC interface and match by…

… name (#1820)

Prevents from accidentally passing an unregistered class in the
autogenerated JSON RPC client and server. Picks the converter to use for
serialisation based on constructor name if function equality match fails
(since constructor name match may fail in minimised browser bundles).

We were bit by this when the `PrivateKey` class was registered in the
client, but due to a duplicated module, the `PrivateKey` class
registered was not the same as the one passed as an argument. This
caused the object not to be properly serialised, which due to #1819 was
not picked up on the server side, and caused all sort of issues.

Fixes #1826

---------

Co-authored-by: spypsy <spypsy@outlook.com>

yarn-project/foundation/src/json-rpc/class_converter.ts

@@ -158,7 +158,8 @@ export class ClassConverter {
    * @returns If it is a registered class.
    */
   isRegisteredClass(obj: any) {
-    return this.toName.has(obj);
+    const name = obj.prototype.constructor.name;
+    return this.toName.has(obj) || this.isRegisteredClassName(name);
   }
   /**
    * Convert a JSON-like object to a class object.
@@ -182,10 +183,23 @@ export class ClassConverter {
    * @returns The class object.
    */
   toJsonObj(classObj: any): JsonEncodedClass | StringEncodedClass {
-    const result = this.toName.get(classObj.constructor);
-    assert(result, `Could not find class in lookup.`);
-    const [type, encoding] = result;
+    const { type, encoding } = this.lookupObject(classObj);
     const data = encoding === 'string' ? classObj.toString() : classObj.toJSON();
     return { type: type!, data };
   }
+
+  /**
+   * Loads the corresponding type for this class based on constructor first and constructor name if not found.
+   * Constructor match works in the event of a minifier changing function names, and constructor name match
+   * works in the event of duplicated instances of node modules being loaded (see #1826).
+   * @param classObj - Object to lookup in the registered types.
+   * @returns Registered type name and encoding.
+   */
+  private lookupObject(classObj: any) {
+    const nameResult = this.toName.get(classObj.constructor);
+    if (nameResult) return { type: nameResult[0], encoding: nameResult[1] };
+    const classResult = this.toClass.get(classObj.constructor.name);
+    if (classResult) return { type: classObj.constructor.name, encoding: classResult[1] };
+    throw new Error(`Could not find class ${classObj.constructor.name} in lookup.`);
+  }
 }

yarn-project/foundation/src/json-rpc/convert.test.ts

@@ -2,6 +2,8 @@ import { Buffer } from 'buffer';
 
 import { ClassConverter } from './class_converter.js';
 import { convertBigintsInObj, convertFromJsonObj, convertToJsonObj } from './convert.js';
+import { ToStringClass as ToStringClassA } from './fixtures/class_a.js';
+import { ToStringClass as ToStringClassB } from './fixtures/class_b.js';
 import { TestNote } from './fixtures/test_state.js';
 
 const TEST_BASE64 = 'YmFzZTY0IGRlY29kZXI=';
@@ -24,3 +26,52 @@ test('does not convert a string', () => {
   expect(convertBigintsInObj('hello')).toEqual('hello');
   expect(convertBigintsInObj({ msg: 'hello' })).toEqual({ msg: 'hello' });
 });
+
+test('converts a registered class', () => {
+  const cc = new ClassConverter({ ToStringClass: ToStringClassA });
+  const obj = { content: new ToStringClassA('a', 'b') };
+  const serialised = convertToJsonObj(cc, obj);
+  const deserialised = convertFromJsonObj(cc, serialised) as { content: ToStringClassA };
+  expect(deserialised.content).toBeInstanceOf(ToStringClassA);
+  expect(deserialised.content.x).toEqual('a');
+  expect(deserialised.content.y).toEqual('b');
+});
+
+test('converts a class by name in the event of duplicate modules being loaded', () => {
+  expect(ToStringClassA.prototype.constructor.name).toEqual('ToStringClass');
+  expect(ToStringClassB.prototype.constructor.name).toEqual('ToStringClass');
+  const cc = new ClassConverter({ ToStringClass: ToStringClassA });
+  const obj = { content: new ToStringClassB('a', 'b') };
+  const serialised = convertToJsonObj(cc, obj);
+  const deserialised = convertFromJsonObj(cc, serialised) as { content: ToStringClassA };
+  expect(deserialised.content).toBeInstanceOf(ToStringClassA);
+  expect(deserialised.content.x).toEqual('a');
+  expect(deserialised.content.y).toEqual('b');
+});
+
+test('converts a class by constructor instead of name in the event of minified bundle', () => {
+  const cc = new ClassConverter({ NotMinifiedToStringClassName: ToStringClassA });
+  const obj = { content: new ToStringClassA('a', 'b') };
+  const serialised = convertToJsonObj(cc, obj);
+  const deserialised = convertFromJsonObj(cc, serialised) as { content: ToStringClassA };
+  expect(deserialised.content).toBeInstanceOf(ToStringClassA);
+  expect(deserialised.content.x).toEqual('a');
+  expect(deserialised.content.y).toEqual('b');
+});
+
+test('converts a plain object', () => {
+  const obj = { a: 10, b: [20, 30], c: 'foo' };
+  const cc = new ClassConverter();
+  expect(convertFromJsonObj(cc, convertToJsonObj(cc, obj))).toEqual(obj);
+});
+
+test('refuses to convert to json an unknown class', () => {
+  const cc = new ClassConverter();
+  expect(() => convertToJsonObj(cc, { content: new ToStringClassA('a', 'b') })).toThrowError(/not registered/);
+});
+
+test('refuses to convert from json an unknown class', () => {
+  const cc = new ClassConverter({ ToStringClass: ToStringClassA });
+  const serialised = convertToJsonObj(cc, { content: new ToStringClassA('a', 'b') });
+  expect(() => convertFromJsonObj(new ClassConverter(), serialised)).toThrowError(/not registered/);
+});

yarn-project/foundation/src/json-rpc/convert.ts

@@ -3,6 +3,31 @@ import cloneDeepWith from 'lodash.clonedeepwith';
 
 import { ClassConverter } from './class_converter.js';
 
+/**
+ * Check prototype chain to determine if an object is 'plain' (not a class instance).
+ * @param obj - The object to check.
+ * @returns True if the object is 'plain'.
+ */
+function isPlainObject(obj: any) {
+  if (obj === null) {
+    return false;
+  }
+
+  let proto = obj;
+  let counter = 0;
+  const MAX_PROTOTYPE_CHAIN_LENGTH = 1000; // Adjust as needed
+  while (Object.getPrototypeOf(proto) !== null) {
+    if (counter >= MAX_PROTOTYPE_CHAIN_LENGTH) {
+      // This is a failsafe in case circular prototype chain has been created. It should not be hit
+      return false;
+    }
+    proto = Object.getPrototypeOf(proto);
+    counter++;
+  }
+
+  return Object.getPrototypeOf(obj) === proto;
+}
+
 /**
  * Recursively looks through an object for bigints and converts them to object format.
  * @param obj - The object to convert.
@@ -59,8 +84,12 @@ export function convertFromJsonObj(cc: ClassConverter, obj: any): any {
   }
 
   // Is this a convertible type?
-  if (typeof obj.type === 'string' && cc.isRegisteredClassName(obj.type)) {
-    return cc.toClassObj(obj);
+  if (typeof obj.type === 'string') {
+    if (cc.isRegisteredClassName(obj.type)) {
+      return cc.toClassObj(obj);
+    } else {
+      throw new Error(`Object ${obj.type} not registered for serialisation`);
+    }
   }
 
   // Is this an array?
@@ -118,7 +147,10 @@ export function convertToJsonObj(cc: ClassConverter, obj: any): any {
     }
     return newObj;
   }
-
+  // Throw if this is a non-primitive class that was not registered
+  if (typeof obj === 'object' && !isPlainObject(obj)) {
+    throw new Error(`Object ${obj.constructor.name} not registered for serialisation`);
+  }
   // Leave alone, assume JSON primitive
   return obj;
 }

yarn-project/foundation/src/json-rpc/fixtures/class_a.ts

@@ -0,0 +1,15 @@
+/**
+ * Test class for testing string converter.
+ */
+export class ToStringClass {
+  constructor(/** A value */ public readonly x: string, /** Another value */ public readonly y: string) {}
+
+  toString(): string {
+    return [this.x, this.y].join('-');
+  }
+
+  static fromString(value: string) {
+    const [x, y] = value.split('-');
+    return new ToStringClass(x, y);
+  }
+}

yarn-project/foundation/src/json-rpc/fixtures/class_b.ts

@@ -0,0 +1,15 @@
+/**
+ * Test class for testing string converter.
+ */
+export class ToStringClass {
+  constructor(/** A value */ public readonly x: string, /** Another value */ public readonly y: string) {}
+
+  toString(): string {
+    return [this.x, this.y].join('-');
+  }
+
+  static fromString(value: string) {
+    const [x, y] = value.split('-');
+    return new ToStringClass(x, y);
+  }
+}