From 898fbe05b6b1650fe24b3cec82002bdf8bab717d Mon Sep 17 00:00:00 2001 From: Santiago Palladino Date: Fri, 25 Aug 2023 15:31:36 -0300 Subject: [PATCH 1/4] fix: Do not allow unregistered classes in JSON RPC interface --- yarn-project/foundation/src/json-rpc/convert.ts | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/yarn-project/foundation/src/json-rpc/convert.ts b/yarn-project/foundation/src/json-rpc/convert.ts index ac6cef5f381a..acbfd2e544d2 100644 --- a/yarn-project/foundation/src/json-rpc/convert.ts +++ b/yarn-project/foundation/src/json-rpc/convert.ts @@ -75,7 +75,10 @@ export function convertFromJsonObj(cc: ClassConverter, obj: any): any { } return newObj; } - + // Throw if this is a non-primitive class that was not registered + if (typeof obj === 'object' && obj.constructor !== Object) { + throw new Error(`Object ${obj.constructor.name} not registered for deserialisation`); + } // Leave alone, assume JSON primitive return obj; } @@ -118,7 +121,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' && obj.constructor !== Object) { + throw new Error(`Object ${obj.constructor.name} not registered for serialisation`); + } // Leave alone, assume JSON primitive return obj; } From ff1cd9fa3b0a26d61bd42304daa924fa53498d97 Mon Sep 17 00:00:00 2001 From: Santiago Palladino Date: Fri, 25 Aug 2023 20:14:08 -0300 Subject: [PATCH 2/4] Fall back to match registered type by name --- .../src/json-rpc/class_converter.ts | 22 ++++++++++--- .../foundation/src/json-rpc/convert.test.ts | 32 +++++++++++++++++++ .../foundation/src/json-rpc/convert.ts | 6 ++-- .../src/json-rpc/fixtures/class_a.ts | 15 +++++++++ .../src/json-rpc/fixtures/class_b.ts | 15 +++++++++ 5 files changed, 83 insertions(+), 7 deletions(-) create mode 100644 yarn-project/foundation/src/json-rpc/fixtures/class_a.ts create mode 100644 yarn-project/foundation/src/json-rpc/fixtures/class_b.ts diff --git a/yarn-project/foundation/src/json-rpc/class_converter.ts b/yarn-project/foundation/src/json-rpc/class_converter.ts index 2be9dab1d5b5..c1e9478380ef 100644 --- a/yarn-project/foundation/src/json-rpc/class_converter.ts +++ b/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.`); + } } diff --git a/yarn-project/foundation/src/json-rpc/convert.test.ts b/yarn-project/foundation/src/json-rpc/convert.test.ts index 4927b9c68a08..2951fcea86f4 100644 --- a/yarn-project/foundation/src/json-rpc/convert.test.ts +++ b/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,33 @@ 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', () => { + 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'); +}); diff --git a/yarn-project/foundation/src/json-rpc/convert.ts b/yarn-project/foundation/src/json-rpc/convert.ts index acbfd2e544d2..5ebbaf8bb89f 100644 --- a/yarn-project/foundation/src/json-rpc/convert.ts +++ b/yarn-project/foundation/src/json-rpc/convert.ts @@ -76,8 +76,8 @@ export function convertFromJsonObj(cc: ClassConverter, obj: any): any { return newObj; } // Throw if this is a non-primitive class that was not registered - if (typeof obj === 'object' && obj.constructor !== Object) { - throw new Error(`Object ${obj.constructor.name} not registered for deserialisation`); + if (typeof obj === 'object' && Object.getPrototypeOf(obj) === Object.getPrototypeOf({})) { + throw new Error(`Class ${obj.constructor.name} not registered for deserialisation`); } // Leave alone, assume JSON primitive return obj; @@ -122,7 +122,7 @@ 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' && obj.constructor !== Object) { + if (typeof obj === 'object' && Object.getPrototypeOf(obj) === Object.getPrototypeOf({})) { throw new Error(`Object ${obj.constructor.name} not registered for serialisation`); } // Leave alone, assume JSON primitive diff --git a/yarn-project/foundation/src/json-rpc/fixtures/class_a.ts b/yarn-project/foundation/src/json-rpc/fixtures/class_a.ts new file mode 100644 index 000000000000..58bb9a0cc193 --- /dev/null +++ b/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); + } +} diff --git a/yarn-project/foundation/src/json-rpc/fixtures/class_b.ts b/yarn-project/foundation/src/json-rpc/fixtures/class_b.ts new file mode 100644 index 000000000000..58bb9a0cc193 --- /dev/null +++ b/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); + } +} From fdb92f021277fa050fd29ba26e9a1826c0821a4e Mon Sep 17 00:00:00 2001 From: Santiago Palladino Date: Sat, 26 Aug 2023 22:12:15 -0300 Subject: [PATCH 3/4] Fix wrong prototype check --- .../foundation/src/json-rpc/convert.test.ts | 19 +++++++++++++++++++ .../foundation/src/json-rpc/convert.ts | 15 ++++++++------- 2 files changed, 27 insertions(+), 7 deletions(-) diff --git a/yarn-project/foundation/src/json-rpc/convert.test.ts b/yarn-project/foundation/src/json-rpc/convert.test.ts index 2951fcea86f4..53b567769046 100644 --- a/yarn-project/foundation/src/json-rpc/convert.test.ts +++ b/yarn-project/foundation/src/json-rpc/convert.test.ts @@ -38,6 +38,8 @@ test('converts a registered class', () => { }); 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); @@ -56,3 +58,20 @@ test('converts a class by constructor instead of name in the event of minified b 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/); +}); diff --git a/yarn-project/foundation/src/json-rpc/convert.ts b/yarn-project/foundation/src/json-rpc/convert.ts index 5ebbaf8bb89f..fe6476aa8631 100644 --- a/yarn-project/foundation/src/json-rpc/convert.ts +++ b/yarn-project/foundation/src/json-rpc/convert.ts @@ -59,8 +59,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? @@ -75,10 +79,7 @@ export function convertFromJsonObj(cc: ClassConverter, obj: any): any { } return newObj; } - // Throw if this is a non-primitive class that was not registered - if (typeof obj === 'object' && Object.getPrototypeOf(obj) === Object.getPrototypeOf({})) { - throw new Error(`Class ${obj.constructor.name} not registered for deserialisation`); - } + // Leave alone, assume JSON primitive return obj; } @@ -122,7 +123,7 @@ 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' && Object.getPrototypeOf(obj) === Object.getPrototypeOf({})) { + if (typeof obj === 'object' && Object.getPrototypeOf(obj) !== Object.getPrototypeOf({})) { throw new Error(`Object ${obj.constructor.name} not registered for serialisation`); } // Leave alone, assume JSON primitive From 087da139ae416a270c595ef770c9729a96d34815 Mon Sep 17 00:00:00 2001 From: spypsy Date: Mon, 28 Aug 2023 11:39:09 +0000 Subject: [PATCH 4/4] Update plain object check --- .../foundation/src/json-rpc/convert.ts | 27 ++++++++++++++++++- 1 file changed, 26 insertions(+), 1 deletion(-) diff --git a/yarn-project/foundation/src/json-rpc/convert.ts b/yarn-project/foundation/src/json-rpc/convert.ts index fe6476aa8631..7ca545ddbd4d 100644 --- a/yarn-project/foundation/src/json-rpc/convert.ts +++ b/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. @@ -123,7 +148,7 @@ 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' && Object.getPrototypeOf(obj) !== Object.getPrototypeOf({})) { + if (typeof obj === 'object' && !isPlainObject(obj)) { throw new Error(`Object ${obj.constructor.name} not registered for serialisation`); } // Leave alone, assume JSON primitive