Skip to content

Commit

Permalink
fix(kernel): kernel's private object annotations are enumerable
Browse files Browse the repository at this point in the history
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: aws/aws-cdk#17876
  • Loading branch information
RomainMuller committed Jan 14, 2022
1 parent 309da85 commit 3ce1374
Show file tree
Hide file tree
Showing 2 changed files with 65 additions and 3 deletions.
42 changes: 39 additions & 3 deletions packages/@jsii/kernel/lib/objects.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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<PropertyDescriptor, 'value' | 'get' | 'set'> = {
// 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,
});
}

/**
Expand Down Expand Up @@ -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] =
Expand Down
26 changes: 26 additions & 0 deletions packages/@jsii/kernel/test/objects.test.ts
Original file line number Diff line number Diff line change
@@ -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);
});

0 comments on commit 3ce1374

Please sign in to comment.