From 84d762d2ff3ff70b23c9f9ebf206126f2a8d5f28 Mon Sep 17 00:00:00 2001 From: Rob Eisenberg Date: Fri, 25 Feb 2022 12:23:42 -0500 Subject: [PATCH 1/3] fix: defend against for/in use on arrays --- .../fast-element/src/observation/array-observer.ts | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/packages/web-components/fast-element/src/observation/array-observer.ts b/packages/web-components/fast-element/src/observation/array-observer.ts index b71d1776994..b730e82ebff 100644 --- a/packages/web-components/fast-element/src/observation/array-observer.ts +++ b/packages/web-components/fast-element/src/observation/array-observer.ts @@ -4,6 +4,13 @@ import { SubscriberSet } from "./notifier.js"; import type { Notifier } from "./notifier.js"; import { Observable } from "./observable.js"; +function setNonEnumerable(target: any, property: string, value: any) { + Reflect.defineProperty(target, property, { + value, + enumerable: false, + }); +} + class ArrayObserver extends SubscriberSet { private oldCollection: any[] | undefined = void 0; private splices: Splice[] | undefined = void 0; @@ -12,7 +19,7 @@ class ArrayObserver extends SubscriberSet { constructor(subject: any[]) { super(subject); - (subject as any).$fastController = this; + setNonEnumerable(subject, "$fastController", this); } public addSplice(splice: Splice): void { @@ -81,7 +88,7 @@ export function enableArrayObservation(): void { const proto = Array.prototype; if (!(proto as any).$fastPatch) { - (proto as any).$fastPatch = true; + setNonEnumerable(proto, "$fastPatch", 1); const pop = proto.pop; const push = proto.push; From 9cf34a97f36ece87d34b4c7a99990805bacd87c2 Mon Sep 17 00:00:00 2001 From: EisenbergEffect Date: Fri, 25 Feb 2022 12:25:03 -0500 Subject: [PATCH 2/3] Change files --- ...-fast-element-8e5ad334-ac12-466b-8440-415abd011eb0.json | 7 +++++++ 1 file changed, 7 insertions(+) create mode 100644 change/@microsoft-fast-element-8e5ad334-ac12-466b-8440-415abd011eb0.json diff --git a/change/@microsoft-fast-element-8e5ad334-ac12-466b-8440-415abd011eb0.json b/change/@microsoft-fast-element-8e5ad334-ac12-466b-8440-415abd011eb0.json new file mode 100644 index 00000000000..92598d2f97c --- /dev/null +++ b/change/@microsoft-fast-element-8e5ad334-ac12-466b-8440-415abd011eb0.json @@ -0,0 +1,7 @@ +{ + "type": "patch", + "comment": "fix: defend against for/in use on arrays", + "packageName": "@microsoft/fast-element", + "email": "roeisenb@microsoft.com", + "dependentChangeType": "patch" +} From 94238296b815332578c05f5c16dc0d7b01570eaa Mon Sep 17 00:00:00 2001 From: Rob Eisenberg Date: Fri, 25 Feb 2022 15:24:05 -0500 Subject: [PATCH 3/3] test: added some basic tests for array observers --- .../src/observation/array-observer.spec.ts | 56 +++++++++++++++++++ .../src/observation/array-observer.ts | 4 +- .../src/observation/observable.spec.ts | 24 ++------ 3 files changed, 61 insertions(+), 23 deletions(-) create mode 100644 packages/web-components/fast-element/src/observation/array-observer.spec.ts diff --git a/packages/web-components/fast-element/src/observation/array-observer.spec.ts b/packages/web-components/fast-element/src/observation/array-observer.spec.ts new file mode 100644 index 00000000000..61411bea639 --- /dev/null +++ b/packages/web-components/fast-element/src/observation/array-observer.spec.ts @@ -0,0 +1,56 @@ +import { expect } from "chai"; +import { Observable } from "./observable"; +import { enableArrayObservation } from "./array-observer"; +import { SubscriberSet } from "./notifier"; + +describe("The ArrayObserver", () => { + it("can be retrieved through Observable.getNotifier()", () => { + enableArrayObservation(); + const array = []; + const notifier = Observable.getNotifier(array); + expect(notifier).to.be.instanceOf(SubscriberSet); + }); + + it("is the same instance for multiple calls to Observable.getNotifier() on the same array", () => { + enableArrayObservation(); + const array = []; + const notifier = Observable.getNotifier(array); + const notifier2 = Observable.getNotifier(array); + expect(notifier).to.equal(notifier2); + }); + + it("is different for different arrays", () => { + enableArrayObservation(); + const notifier = Observable.getNotifier([]); + const notifier2 = Observable.getNotifier([]); + expect(notifier).to.not.equal(notifier2); + }); + + it("doesn't affect for/in loops on arrays when enabled", () => { + enableArrayObservation(); + + const array = [1, 2, 3]; + const keys: string[] = []; + + for (const key in array) { + keys.push(key); + } + + expect(keys).eql(["0", "1", "2"]); + }); + + it("doesn't affect for/in loops on arrays when the array is observed", () => { + enableArrayObservation(); + + const array = [1, 2, 3]; + const keys: string[] = []; + const notifier = Observable.getNotifier(array); + + for (const key in array) { + keys.push(key); + } + + expect(notifier).to.be.instanceOf(SubscriberSet); + expect(keys).eql(["0", "1", "2"]) + }); +}); diff --git a/packages/web-components/fast-element/src/observation/array-observer.ts b/packages/web-components/fast-element/src/observation/array-observer.ts index b730e82ebff..ad52f93d02d 100644 --- a/packages/web-components/fast-element/src/observation/array-observer.ts +++ b/packages/web-components/fast-element/src/observation/array-observer.ts @@ -80,9 +80,7 @@ export function enableArrayObservation(): void { enabled = true; Observable.setArrayObserverFactory( - (collection: any[]): Notifier => { - return new ArrayObserver(collection); - } + (collection: any[]): Notifier => new ArrayObserver(collection) ); const proto = Array.prototype; diff --git a/packages/web-components/fast-element/src/observation/observable.spec.ts b/packages/web-components/fast-element/src/observation/observable.spec.ts index bbc8218aba5..c174b52ab0d 100644 --- a/packages/web-components/fast-element/src/observation/observable.spec.ts +++ b/packages/web-components/fast-element/src/observation/observable.spec.ts @@ -61,14 +61,6 @@ describe("The Observable", () => { } context("facade", () => { - it("can set an array observer factory", () => { - const fakeObserver = new SubscriberSet([]); - Observable.setArrayObserverFactory((array: any[]) => fakeObserver); - const array = []; - const observer = Observable.getNotifier(array); - expect(observer).to.equal(fakeObserver); - }); - it("can get a notifier for an object", () => { const instance = new Model(); const notifier = Observable.getNotifier(instance); @@ -84,19 +76,11 @@ describe("The Observable", () => { expect(notifier).to.equal(notifier2); }); - it("can get a notifier for an array", () => { - enableArrayObservation(); - const array = []; - const notifier = Observable.getNotifier(array); - expect(notifier).to.be.instanceOf(SubscriberSet); - }); + it("gets different notifiers for different objects", () => { + const notifier = Observable.getNotifier(new Model()); + const notifier2 = Observable.getNotifier(new Model()); - it("gets the same notifier for the same array", () => { - enableArrayObservation(); - const array = []; - const notifier = Observable.getNotifier(array); - const notifier2 = Observable.getNotifier(array); - expect(notifier).to.equal(notifier2); + expect(notifier).to.not.equal(notifier2); }); it("can notify a change on an object", () => {