From 3ce137426baa9a739c7a1fdb0d3f5de09d74c690 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=F0=9F=A7=91=F0=9F=8F=BB=E2=80=8D=F0=9F=92=BB=20Romain=20M?= =?UTF-8?q?arcadier?= Date: Fri, 14 Jan 2022 17:45:03 +0100 Subject: [PATCH 1/2] fix(kernel): kernel's private object annotations are enumerable The jsii kernel needs to attach metadata to instances that are sent across the process boundary. While these properties use symbol names to eliminate the risk for collisions with user-defined properties, they were inadvertently set as `enumerable`, causing those to show in outputs of `JSON.stringify` and to be copied over as part of spat expressions (e.g: `{...obj}`), which resulted in the new value sharing the same identity as the previous one when sent over the wire. Caused: https://github.com/aws/aws-cdk/issues/17876 --- packages/@jsii/kernel/lib/objects.ts | 42 ++++++++++++++++++++-- packages/@jsii/kernel/test/objects.test.ts | 26 ++++++++++++++ 2 files changed, 65 insertions(+), 3 deletions(-) create mode 100644 packages/@jsii/kernel/test/objects.test.ts diff --git a/packages/@jsii/kernel/lib/objects.ts b/packages/@jsii/kernel/lib/objects.ts index 79eec58cae..8e99dae453 100644 --- a/packages/@jsii/kernel/lib/objects.ts +++ b/packages/@jsii/kernel/lib/objects.ts @@ -52,9 +52,35 @@ type ManagedObject = { }; function tagObject(obj: unknown, objid: string, interfaces?: string[]) { - const managed = obj as ManagedObject; - managed[OBJID_SYMBOL] = objid; - managed[IFACES_SYMBOL] = interfaces; + const privateField: Omit = { + // Make sure the filed does not show in `JSON.stringify` outputs, and is not + // copied by splat expressions (`{...obj}`), as this would be problematic. + // See https://github.com/aws/aws-cdk/issues/17876 for an example of the + // consequences this could have. + enumerable: false, + // Probably not necessary, but allow the property to be re-configured (it + // would be good to make this `false` in the future, but might cause weird + // bugs, so not doing it now...) + configurable: true, + writable: true, + }; + + // Log a warning in case we are re-tagging this value, so we can hopefully + // discover about the bugs we'd have if we did not make it configurable nor + // writable. + if (Object.prototype.hasOwnProperty.call(obj, OBJID_SYMBOL)) { + console.error( + `[jsii/kernel] WARNING: object ${JSON.stringify( + obj as any, + )} was already tagged as ${(obj as any)[OBJID_SYMBOL]}!`, + ); + } + + Object.defineProperty(obj, OBJID_SYMBOL, { ...privateField, value: objid }); + Object.defineProperty(obj, IFACES_SYMBOL, { + ...privateField, + value: interfaces, + }); } /** @@ -104,6 +130,16 @@ export class ObjectTable { for (const iface of existingRef[api.TOKEN_INTERFACES] ?? []) { allIfaces.add(iface); } + // Note - obj[INTERFACES_SYMBOL] should already have been delcared as a + // private property by a previous call to tagObject at this stage. + if (!Object.prototype.hasOwnProperty.call(obj, IFACES_SYMBOL)) { + console.error( + `[jsii/kernel] WARNING: referenced object ${ + existingRef[api.TOKEN_REF] + } does not have the ${String(IFACES_SYMBOL)} property!`, + ); + } + this.objects[existingRef[api.TOKEN_REF]].interfaces = (obj as any)[IFACES_SYMBOL] = existingRef[api.TOKEN_INTERFACES] = diff --git a/packages/@jsii/kernel/test/objects.test.ts b/packages/@jsii/kernel/test/objects.test.ts new file mode 100644 index 0000000000..2d11966e1c --- /dev/null +++ b/packages/@jsii/kernel/test/objects.test.ts @@ -0,0 +1,26 @@ +import { ObjectTable } from '../lib/objects'; + +const mockResolve = jest.fn(); + +test('can spread registered objects without consequences', () => { + const subject = new ObjectTable(mockResolve); + + const obj = { foo: 'bar', baz: 1337 }; + const objRef = subject.registerObject(obj, 'Object'); + + const copy = { ...obj, foo: undefined, baz: undefined }; + const copyRef = subject.registerObject(copy, 'Object'); + + expect(objRef).not.toEqual(copyRef); +}); + +test('registered objects have clean JSON.Stringify', () => { + const subject = new ObjectTable(mockResolve); + + const obj = { foo: 'bar', baz: 1337 }; + const expected = JSON.stringify(obj); + + subject.registerObject(obj, 'Object'); + + expect(JSON.stringify(obj)).toEqual(expected); +}); From cd17e6e64d6ee955e0fad27d8fc46d3a38a0917a Mon Sep 17 00:00:00 2001 From: Romain Marcadier Date: Mon, 17 Jan 2022 10:16:48 +0100 Subject: [PATCH 2/2] Apply suggestions from code review Co-authored-by: Nick Lynch --- packages/@jsii/kernel/lib/objects.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/@jsii/kernel/lib/objects.ts b/packages/@jsii/kernel/lib/objects.ts index 8e99dae453..a72ee20d4b 100644 --- a/packages/@jsii/kernel/lib/objects.ts +++ b/packages/@jsii/kernel/lib/objects.ts @@ -53,7 +53,7 @@ type ManagedObject = { function tagObject(obj: unknown, objid: string, interfaces?: string[]) { const privateField: Omit = { - // Make sure the filed does not show in `JSON.stringify` outputs, and is not + // Make sure the field does not show in `JSON.stringify` outputs, and is not // copied by splat expressions (`{...obj}`), as this would be problematic. // See https://github.com/aws/aws-cdk/issues/17876 for an example of the // consequences this could have. @@ -130,7 +130,7 @@ export class ObjectTable { for (const iface of existingRef[api.TOKEN_INTERFACES] ?? []) { allIfaces.add(iface); } - // Note - obj[INTERFACES_SYMBOL] should already have been delcared as a + // Note - obj[INTERFACES_SYMBOL] should already have been declared as a // private property by a previous call to tagObject at this stage. if (!Object.prototype.hasOwnProperty.call(obj, IFACES_SYMBOL)) { console.error(