From 655adeb9f3e9617049fbbe9160b9ef15218790be Mon Sep 17 00:00:00 2001 From: Romain Marcadier-Muller Date: Wed, 18 Mar 2020 16:52:22 +0100 Subject: [PATCH] fix(kernel): "any" serialization breaks private type instances (#1347) When a "private" type (aka a class that is not visible in the `.jsii` assembly) is passed from JS to Host through an `any`-typed entity, the value was serialized by-value, passing only the instance's properties, and omitting any methods. This changes the behavior of the `SerializationClass.Any` serializer, so that when an anonymous object is encountered, if it has a dynamic getter, setter or any method (including a constructor), it is passed by-reference instead of by-value. Fixes aws/aws-cdk#6746 --- docs/specifications/2-type-system.md | 5 +- packages/@jsii/kernel/lib/objects.ts | 3 +- packages/@jsii/kernel/lib/serialization.ts | 31 +++++++++- .../@jsii/kernel/test/serialization.test.ts | 57 ++++++++++++++++--- .../python-runtime/src/jsii/_reference_map.py | 8 +-- packages/@jsii/python-runtime/tests/README.md | 10 ++++ 6 files changed, 96 insertions(+), 18 deletions(-) create mode 100644 packages/@jsii/python-runtime/tests/README.md diff --git a/docs/specifications/2-type-system.md b/docs/specifications/2-type-system.md index 1b52c301b4..961e71d42f 100644 --- a/docs/specifications/2-type-system.md +++ b/docs/specifications/2-type-system.md @@ -201,8 +201,11 @@ term *primitive* encompasses `boolean`, `string`, and `number`. `interface` | `undefined` | :x: | :x: | :x: | [Reference] | [Reference] `struct` | `undefined` | :x: | :x: | :x: | :x: | [Value] `class` | `undefined` | :x: | :x: | :x: | [Reference] | [Reference] -`any` | `undefined` | [Date] | [Identity] | [Array] | [Reference] | [Mapping] +`any` | `undefined` | [Date] | [Identity] | [Array] | [Reference] | [Value] or [Reference] +In the case of `object` being passed though `any`, the value may be serialized +by [Value] only if the value being passed does not have any method or dynamic +accessor. Otherwise, it must be passed by [Reference] instead. > :warning: The serialization behavior around `undefined` values is affected by > the `optional` attribute of the declared type. As discussed earlier, the `any` diff --git a/packages/@jsii/kernel/lib/objects.ts b/packages/@jsii/kernel/lib/objects.ts index 6a0f888a3d..d1d171cab9 100644 --- a/packages/@jsii/kernel/lib/objects.ts +++ b/packages/@jsii/kernel/lib/objects.ts @@ -26,8 +26,7 @@ const JSII_SYMBOL = Symbol.for('__jsii__'); * information. */ export function jsiiTypeFqn(obj: any): string | undefined { - const jsii = obj.constructor[JSII_SYMBOL]; - return jsii?.fqn; + return obj.constructor[JSII_SYMBOL]?.fqn; } /** diff --git a/packages/@jsii/kernel/lib/serialization.ts b/packages/@jsii/kernel/lib/serialization.ts index 643b47a2db..6baa7c4ea8 100644 --- a/packages/@jsii/kernel/lib/serialization.ts +++ b/packages/@jsii/kernel/lib/serialization.ts @@ -474,7 +474,7 @@ export const SERIALIZERS: {[k: string]: Serializer} = { // If this is or should be a reference type, pass or make the reference // (Like regular reftype serialization, but without the type derivation to an interface) - const jsiiType = jsiiTypeFqn(value); + const jsiiType = jsiiTypeFqn(value) ?? (isByReferenceOnly(value) ? EMPTY_OBJECT_FQN : undefined); if (jsiiType) { return host.objects.registerObject(value, jsiiType); } // At this point we have an object that is not of an exported type. Either an object @@ -737,3 +737,32 @@ function compareSerializationClasses(l: SerializationClass, r: SerializationClas ]; return order.indexOf(l) - order.indexOf(r); } + +/** + * Determines whether `obj` must be passed by-reference or if by-value is acceptable. For example, + * objects with methods, or dynamic getters (or setters) should be passed by-reference as a matter + * of security. The behavior in non-JS runtimes could otherwise differ from that in pure JS (if + * getters are not stable, etc...). + * + * @param obj the object to be tested. + * + * @returns true if `obj` must be passed by-reference. + */ +function isByReferenceOnly(obj: any): boolean { + if (Array.isArray(obj)) { return false; } + + let curr = obj; + // Crawl up the prototype chain to look for dynamic properties or methods. + do { + for (const prop of Object.getOwnPropertyNames(curr)) { + const descr = Object.getOwnPropertyDescriptor(curr, prop); + if (descr?.get != null || descr?.set != null || typeof descr?.value === 'function') { + // Property has a dynamic getter, setter or is a constructor/method, so by-ref required! + return true; + } + } + // End when the parent proto is `Object`, which has no parent proto itself. + } while (Object.getPrototypeOf(curr = Object.getPrototypeOf(curr)) != null); + + return false; +} diff --git a/packages/@jsii/kernel/test/serialization.test.ts b/packages/@jsii/kernel/test/serialization.test.ts index 44ebdb6d4a..74ba3f99b0 100644 --- a/packages/@jsii/kernel/test/serialization.test.ts +++ b/packages/@jsii/kernel/test/serialization.test.ts @@ -1,22 +1,61 @@ -import { OptionalValue, PrimitiveType } from '@jsii/spec'; +import { CANONICAL_ANY, OptionalValue, PrimitiveType } from '@jsii/spec'; +import { TOKEN_REF } from '../lib/api'; import { ObjectTable } from '../lib/objects'; import { SerializationClass, SerializerHost, SERIALIZERS } from '../lib/serialization'; +const TYPE_ANY: OptionalValue = { type: CANONICAL_ANY }; const TYPE_BOOLEAN: OptionalValue = { type: { primitive: PrimitiveType.Boolean } }; const TYPE_NUMBER: OptionalValue = { type: { primitive: PrimitiveType.Number } }; const TYPE_STRING: OptionalValue = { type: { primitive: PrimitiveType.String } }; const TYPE_VOID = 'void'; +const lookupType: SerializerHost['lookupType'] = jest.fn().mockName('host.lookupType'); +const host: SerializerHost = { + debug: jest.fn().mockName('host.debug'), + findSymbol: jest.fn().mockName('host.findSymbol'), + lookupType, + objects: new ObjectTable(lookupType), + recurse: jest.fn().mockName('host.recurse'), +}; + +describe(SerializationClass.Any, () => { + const anySerializer = SERIALIZERS[SerializationClass.Any]; + class PrivateType { + private readonly randomValue = Math.random(); + + public random() { + return this.randomValue; + } + } + + beforeEach(done => { + (host.recurse as jest.Mock).mockImplementation((x: any, type: OptionalValue) => { + expect(type).toEqual(TYPE_ANY); + return anySerializer.serialize(x, type, host); + }); + done(); + }); + + describe(anySerializer.serialize, () => { + test('by-value object literal', () => { + expect(anySerializer.serialize({ literal: { integer: 1337 } }, TYPE_ANY, host)) + .toEqual({ literal: { integer: 1337 } }); + }); + + test('non-exported type instance', () => { + expect(anySerializer.serialize(new PrivateType, TYPE_ANY, host)) + .toEqual({ [TOKEN_REF]: 'Object@10000' }); + }); + + test('arrays', () => { + expect(anySerializer.serialize([{ literal: { integer: 1337 } }], TYPE_ANY, host)) + .toEqual([{ literal: { integer: 1337 } }]); + }); + }); +}); + describe(SerializationClass.Scalar, () => { const scalarSerializer = SERIALIZERS[SerializationClass.Scalar]; - const lookupType: SerializerHost['lookupType'] = jest.fn().mockName('host.lookupType'); - const host: SerializerHost = { - debug: jest.fn().mockName('host.debug'), - findSymbol: jest.fn().mockName('host.findSymbol'), - lookupType, - objects: new ObjectTable(lookupType), - recurse: jest.fn().mockName('host.recurse'), - }; describe(scalarSerializer.deserialize, () => { describe('void', () => { diff --git a/packages/@jsii/python-runtime/src/jsii/_reference_map.py b/packages/@jsii/python-runtime/src/jsii/_reference_map.py index 4320b897dc..1961342285 100644 --- a/packages/@jsii/python-runtime/src/jsii/_reference_map.py +++ b/packages/@jsii/python-runtime/src/jsii/_reference_map.py @@ -95,9 +95,9 @@ def resolve(self, kernel, ref): return data_type(**python_props) elif class_fqn in _enums: return _enums[class_fqn] - elif class_fqn == "Object" and ref.interfaces is not None: + elif class_fqn == "Object": # If any one interface is a struct, all of them are guaranteed to be (Kernel invariant) - if any(fqn in _data_types for fqn in ref.interfaces): + if ref.interfaces is not None and any(fqn in _data_types for fqn in ref.interfaces): # Ugly delayed import here because I can't solve the cyclic # package dependency right now :(. from ._runtime import python_jsii_mapping @@ -117,9 +117,7 @@ def resolve_id(self, id): return self._refs[id] def build_interface_proxies_for_ref(self, ref): - if ref.interfaces is None: - raise AssertionError("Attempted to create interface proxies for ObjectRef without interfaces!") - ifaces = [_interfaces[fqn] for fqn in ref.interfaces] + ifaces = [_interfaces[fqn] for fqn in ref.interfaces or []] classes = [iface.__jsii_proxy_class__() for iface in ifaces] insts = [klass.__new__(klass) for klass in classes] for inst in insts: diff --git a/packages/@jsii/python-runtime/tests/README.md b/packages/@jsii/python-runtime/tests/README.md new file mode 100644 index 0000000000..8cfb8ba88c --- /dev/null +++ b/packages/@jsii/python-runtime/tests/README.md @@ -0,0 +1,10 @@ +# Python jsii runtime tests +## Development Iteration + +When iterating on the jsii runtime for Python, the develomer must run +`yarn build` before making a subsequent attempt at running `pytest` (e.g: via +`yarn test`). This is because the tests run on the code installed in `.env` and +this is updated only by `yarn build`. + +Note also that stack traces from test failures will point to the `.env` tree, +so be careful when using IDE linkage to navigate to points of that trace.