src: use new V8 API in vm module

Remove the CopyProperties() hack in the vm module, i.e., Contextify. Use different V8 API methods, that allow interception of DefineProperty() and do not flatten accessor descriptors to property descriptors. Move many known issues to test cases. Factor out the last test in test-vm-context.js for #10223 into its own file, test-vm-strict-assign.js. Part of this CL is taken from a stalled PR by https://github.com/AnnaMag #13265 This PR requires a backport of https://chromium.googlesource.com/v8/v8/+/37a3a15c3e52e2146e45f41c427f24414e4d7f6f Refs: #6283 Refs: #15114 Refs: #13265 Fixes: #2734 Fixes: #10223 Fixes: #11803 Fixes: #11902
nodejs · fhinkel · Oct 17, 2017 · Oct 23, 2017 · f7d22e5d0089f96301bc80605214ae6ee9e93f4d
commit f7d22e5d0089f96301bc80605214ae6ee9e93f4d
diff --git a/src/node_contextify.cc b/src/node_contextify.cc
diff --git a/test/known_issues/test-vm-attributes-property-not-on-sandbox.js b/test/known_issues/test-vm-attributes-property-not-on-sandbox.js
diff --git a/test/parallel/test-vm-attributes-property-not-on-sandbox.js b/test/parallel/test-vm-attributes-property-not-on-sandbox.js
@@ -0,0 +1,18 @@
+'use strict';
+require('../common');
+const assert = require('assert');
+const vm = require('vm');
+
+// Assert that accessor descriptors are not flattened on the sandbox.
+// Issue: https://github.com/nodejs/node/issues/2734
+const sandbox = {};
+vm.createContext(sandbox);
+const code = `Object.defineProperty(
+               this,
+               'foo',
+               { get: function() {return 17} }
+             );
+             var desc = Object.getOwnPropertyDescriptor(this, 'foo');`;
+
+vm.runInContext(code, sandbox);
+assert.strictEqual(typeof sandbox.desc.get, 'function');
diff --git a/test/parallel/test-vm-context.js b/test/parallel/test-vm-context.js
@@ -106,16 +106,3 @@ assert.throws(() => {
 // https://github.com/nodejs/node/issues/6158
 ctx = new Proxy({}, {});
 assert.strictEqual(typeof vm.runInNewContext('String', ctx), 'function');
-
-// https://github.com/nodejs/node/issues/10223
-ctx = vm.createContext();
-vm.runInContext('Object.defineProperty(this, "x", { value: 42 })', ctx);
-assert.strictEqual(ctx.x, 42);
-assert.strictEqual(vm.runInContext('x', ctx), 42);
-
-vm.runInContext('x = 0', ctx);                      // Does not throw but x...
-assert.strictEqual(vm.runInContext('x', ctx), 42);  // ...should be unaltered.
-
-assert.throws(() => vm.runInContext('"use strict"; x = 0', ctx),
-              /Cannot assign to read only property 'x'/);
-assert.strictEqual(vm.runInContext('x', ctx), 42);
diff --git a/..._issues/test-vm-data-property-writable.js → ...arallel/test-vm-data-property-writable.js b/..._issues/test-vm-data-property-writable.js → ...arallel/test-vm-data-property-writable.js
@@ -7,11 +7,22 @@ const assert = require('assert');
 
 const context = vm.createContext({});
 
-const code = `
+let code = `
    Object.defineProperty(this, 'foo', {value: 5});
    Object.getOwnPropertyDescriptor(this, 'foo');
 `;
 
-const desc = vm.runInContext(code, context);
+let desc = vm.runInContext(code, context);
 
 assert.strictEqual(desc.writable, false);
+
+// Check that interceptors work for symbols.
+code = `
+   const bar = Symbol('bar');
+   Object.defineProperty(this, bar, {value: 6});
+   Object.getOwnPropertyDescriptor(this, bar);
+`;
+
+desc = vm.runInContext(code, context);
+
+assert.strictEqual(desc.value, 6);
diff --git a/test/known_issues/test-vm-getters.js → test/parallel/test-vm-getters.js b/test/known_issues/test-vm-getters.js → test/parallel/test-vm-getters.js
diff --git a/test/parallel/test-vm-global-define-property.js b/test/parallel/test-vm-global-define-property.js
@@ -44,4 +44,4 @@ assert(res);
 assert.strictEqual(typeof res, 'object');
 assert.strictEqual(res, x);
 assert.strictEqual(o.f, res);
-assert.deepStrictEqual(Object.keys(o), ['console', 'x', 'g', 'f']);
+assert.deepStrictEqual(Object.keys(o), ['console', 'x', 'f', 'g']);
diff --git a/...test-vm-global-non-writable-properties.js → ...test-vm-global-non-writable-properties.js b/...test-vm-global-non-writable-properties.js → ...test-vm-global-non-writable-properties.js
@@ -7,7 +7,6 @@ const vm = require('vm');
 
 const ctx = vm.createContext();
 vm.runInContext('Object.defineProperty(this, "x", { value: 42 })', ctx);
-assert.strictEqual(ctx.x, undefined);  // Not copied out by cloneProperty().
 assert.strictEqual(vm.runInContext('x', ctx), 42);
 vm.runInContext('x = 0', ctx);                      // Does not throw but x...
 assert.strictEqual(vm.runInContext('x', ctx), 42);  // ...should be unaltered.

diff --git a/.../known_issues/test-vm-proxy-failure-CP.js → test/parallel/test-vm-proxy-failure-CP.js b/.../known_issues/test-vm-proxy-failure-CP.js → test/parallel/test-vm-proxy-failure-CP.js
@@ -1,13 +1,10 @@
 'use strict';
-
-// Sandbox throws in CopyProperties() despite no code being run
-// Issue: https://github.com/nodejs/node/issues/11902
-
-
 require('../common');
 const assert = require('assert');
 const vm = require('vm');
 
+// Check that we do not accidentally query attributes.
+// Issue: https://github.com/nodejs/node/issues/11902
 const handler = {
   getOwnPropertyDescriptor: (target, prop) => {
     throw new Error('whoops');
@@ -16,5 +13,4 @@ const handler = {
 const sandbox = new Proxy({ foo: 'bar' }, handler);
 const context = vm.createContext(sandbox);
 
-
 assert.doesNotThrow(() => vm.runInContext('', context));
diff --git a/test/parallel/test-vm-strict-assign.js b/test/parallel/test-vm-strict-assign.js
@@ -0,0 +1,20 @@
+'use strict';
+require('../common');
+const assert = require('assert');
+
+const vm = require('vm');
+
+// https://github.com/nodejs/node/issues/10223
+const ctx = vm.createContext();
+
+// Define x with writable = false.
+vm.runInContext('Object.defineProperty(this, "x", { value: 42 })', ctx);
+assert.strictEqual(ctx.x, 42);
+assert.strictEqual(vm.runInContext('x', ctx), 42);
+
+vm.runInContext('x = 0', ctx);                      // Does not throw but x...
+assert.strictEqual(vm.runInContext('x', ctx), 42);  // ...should be unaltered.
+
+assert.throws(() => vm.runInContext('"use strict"; x = 0', ctx),
+              /Cannot assign to read only property 'x'/);
+assert.strictEqual(vm.runInContext('x', ctx), 42);
diff --git a/test/parallel/test-vm-strict-mode.js b/test/parallel/test-vm-strict-mode.js