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

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);
+  }
+}