From 3029e9df6ef110cdf4f68a379600e4b5308db86f Mon Sep 17 00:00:00 2001 From: Jacob Quant Date: Fri, 14 Dec 2018 16:10:32 -0600 Subject: [PATCH 1/3] test(NativeArray): Check for cyclic Array.prototype (ref #17190) --- .../runtime/tests/array/apply-test.js | 34 +++++++++++++++++++ 1 file changed, 34 insertions(+) create mode 100644 packages/@ember/-internals/runtime/tests/array/apply-test.js diff --git a/packages/@ember/-internals/runtime/tests/array/apply-test.js b/packages/@ember/-internals/runtime/tests/array/apply-test.js new file mode 100644 index 00000000000..ac277599f4a --- /dev/null +++ b/packages/@ember/-internals/runtime/tests/array/apply-test.js @@ -0,0 +1,34 @@ +import { NativeArray } from '../../lib/mixins/array'; +import { AbstractTestCase, moduleFor } from 'internal-test-helpers'; + +class ArrayPrototypeExtensionSelfReferenceTests extends AbstractTestCase { + '@test should not create non-Symbol, enumerable properties that refer to itself'() { + // Don't want to pollute Array.prototype so we make our own to extend + class ThrowAwayArray extends Array {} + + // Extend our throw-away prototype (like EXTEND_PROTOTYPES.Array would) + NativeArray.apply(ThrowAwayArray.prototype); + + // Create an instance to test + let obj = new ThrowAwayArray(); + + // Make sure we have an array-like thing & avoid the zero assertion problem is there are no enumerable properties + this.assert.strictEqual(obj.length, 0); + + // Make sure that no enumerable properties refer back to the object (creating a cyclic structure) + for (let p in obj) { + this.assert.notStrictEqual( + obj[p], + obj, + `Property "${p}" is an enumerable part of the prototype + so must not refer back to the original array. + Otherwise code that explores all properties, + such as jQuery.extend and other "deep cloning" functions, + will get stuck in an infinite loop. + `.replace(/\s+/g, ' ') + ); + } + } +} + +moduleFor(`NativeArray: apply`, ArrayPrototypeExtensionSelfReferenceTests); From 98afaff5f82bc5afa743bf8889ea10977d7f76f9 Mon Sep 17 00:00:00 2001 From: Robert Jackson Date: Fri, 14 Dec 2018 19:48:23 -0500 Subject: [PATCH 2/3] [BUGFIX release] Ensure properties on Array.prototype are non-enumerable. This fixes the following example case: ```js $.extend(true, {}, {a:['a']}) ``` Prior to this change, the above would trigger maximum call stack error. This is because the `[]` computed property added to the array prototype references itself, which ultimately makes `$.extend` (and other deep equality style comparisons) fail. --- .../-internals/runtime/lib/mixins/array.js | 22 ++++++++++++++----- 1 file changed, 17 insertions(+), 5 deletions(-) diff --git a/packages/@ember/-internals/runtime/lib/mixins/array.js b/packages/@ember/-internals/runtime/lib/mixins/array.js index 054636b24fe..909369fcb30 100644 --- a/packages/@ember/-internals/runtime/lib/mixins/array.js +++ b/packages/@ember/-internals/runtime/lib/mixins/array.js @@ -171,6 +171,18 @@ export function isArray(_obj) { return false; } +/* + This allows us to define computed properties that are not enumerable. + The primary reason this is important is that when `NativeArray` is + applied to `Array.prototype` we need to ensure that we do not add _any_ + new enumerable properties. +*/ +function nonEnumerableComputed() { + let property = computed(...arguments); + property.enumerable = false; + return property; +} + // .......................................................... // ARRAY // @@ -273,7 +285,7 @@ const ArrayMixin = Mixin.create(Enumerable, { @return this @public */ - '[]': computed({ + '[]': nonEnumerableComputed({ get() { return this; }, @@ -290,7 +302,7 @@ const ArrayMixin = Mixin.create(Enumerable, { @return {Object | undefined} The first object in the array @public */ - firstObject: computed(function() { + firstObject: nonEnumerableComputed(function() { return objectAt(this, 0); }).readOnly(), @@ -301,7 +313,7 @@ const ArrayMixin = Mixin.create(Enumerable, { @return {Object | undefined} The last object in the array @public */ - lastObject: computed(function() { + lastObject: nonEnumerableComputed(function() { return objectAt(this, this.length - 1); }).readOnly(), @@ -474,7 +486,7 @@ const ArrayMixin = Mixin.create(Enumerable, { @property {Boolean} hasArrayObservers @public */ - hasArrayObservers: computed(function() { + hasArrayObservers: nonEnumerableComputed(function() { return hasListeners(this, '@array:change') || hasListeners(this, '@array:before'); }), @@ -1154,7 +1166,7 @@ const ArrayMixin = Mixin.create(Enumerable, { @public */ '@each': ARRAY_AT_EACH - ? computed(function() { + ? nonEnumerableComputed(function() { deprecate(`Getting the '@each' property on object ${toString(this)} is deprecated`, false, { id: 'ember-metal.getting-each', until: '3.5.0', From 6e74ef842b39acfbad39feacf49e7e53a451b583 Mon Sep 17 00:00:00 2001 From: Robert Jackson Date: Fri, 14 Dec 2018 20:10:21 -0500 Subject: [PATCH 3/3] Avoid extended array directly in tests. Unfortunately, `class Foo extends Array{}` doesn't work well with older browsers (and would even require changes in our transpilation for production builds for modern ones). The actual test doesn't care if its running on an Array (without the fixes in this PR this modified test still fails the same way). --- .../@ember/-internals/runtime/tests/array/apply-test.js | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/packages/@ember/-internals/runtime/tests/array/apply-test.js b/packages/@ember/-internals/runtime/tests/array/apply-test.js index ac277599f4a..e40068c55b1 100644 --- a/packages/@ember/-internals/runtime/tests/array/apply-test.js +++ b/packages/@ember/-internals/runtime/tests/array/apply-test.js @@ -3,8 +3,8 @@ import { AbstractTestCase, moduleFor } from 'internal-test-helpers'; class ArrayPrototypeExtensionSelfReferenceTests extends AbstractTestCase { '@test should not create non-Symbol, enumerable properties that refer to itself'() { - // Don't want to pollute Array.prototype so we make our own to extend - class ThrowAwayArray extends Array {} + // Don't want to pollute Array.prototype so we make a fake / simple prototype + function ThrowAwayArray() {} // Extend our throw-away prototype (like EXTEND_PROTOTYPES.Array would) NativeArray.apply(ThrowAwayArray.prototype); @@ -12,9 +12,6 @@ class ArrayPrototypeExtensionSelfReferenceTests extends AbstractTestCase { // Create an instance to test let obj = new ThrowAwayArray(); - // Make sure we have an array-like thing & avoid the zero assertion problem is there are no enumerable properties - this.assert.strictEqual(obj.length, 0); - // Make sure that no enumerable properties refer back to the object (creating a cyclic structure) for (let p in obj) { this.assert.notStrictEqual(