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

PR-URL: #16293
Fixes: #2734
Fixes: #10223
Fixes: #11803
Fixes: #11902
Ref: #6283
Ref: #15114
Ref: #13265
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>

Author: fhinkel

src/node_contextify.cc

@@ -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');

test/known_issues/test-vm-attributes-property-not-on-sandbox.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);

test/parallel/test-vm-attributes-property-not-on-sandbox.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);

test/parallel/test-vm-context.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']);

test/known_issues/test-vm-data-property-writable.js renamed to test/parallel/test-vm-data-property-writable.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.

test/known_issues/test-vm-getters.js renamed to test/parallel/test-vm-getters.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));

test/parallel/test-vm-global-define-property.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);