diff --git a/src/node_contextify.cc b/src/node_contextify.cc index fc8aa451347b94..d74b01ea0da371 100644 --- a/src/node_contextify.cc +++ b/src/node_contextify.cc @@ -383,22 +383,19 @@ class ContextifyContext { if (ctx->context_.IsEmpty()) return; - auto attributes = PropertyAttribute::None; bool is_declared = - ctx->global_proxy()->GetRealNamedPropertyAttributes(ctx->context(), - property) - .To(&attributes); - bool read_only = - static_cast(attributes) & - static_cast(PropertyAttribute::ReadOnly); - - if (is_declared && read_only) - return; + ctx->global_proxy()->HasRealNamedProperty(ctx->context(), + property).FromJust(); + bool is_contextual_store = ctx->global_proxy() != args.This(); - if (!is_declared && args.ShouldThrowOnError()) - return; + bool set_property_will_throw = + args.ShouldThrowOnError() && + !is_declared && + is_contextual_store; - ctx->sandbox()->Set(property, value); + if (!set_property_will_throw) { + ctx->sandbox()->Set(property, value); + } } diff --git a/test/known_issues/test-vm-global-non-writable-properties.js b/test/known_issues/test-vm-global-non-writable-properties.js new file mode 100644 index 00000000000000..8d7dfce55e3d81 --- /dev/null +++ b/test/known_issues/test-vm-global-non-writable-properties.js @@ -0,0 +1,16 @@ +'use strict'; +// https://github.com/nodejs/node/issues/10223 + +require('../common'); +const assert = require('assert'); +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. +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-context.js b/test/parallel/test-vm-context.js index 7056ce7be35d05..75d91ce8129dcb 100644 --- a/test/parallel/test-vm-context.js +++ b/test/parallel/test-vm-context.js @@ -75,14 +75,3 @@ assert.throws(function() { // 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, 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. -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-global-assignment.js b/test/parallel/test-vm-global-assignment.js new file mode 100644 index 00000000000000..3fb3470b4c2a43 --- /dev/null +++ b/test/parallel/test-vm-global-assignment.js @@ -0,0 +1,15 @@ +'use strict'; +// Regression test for https://github.com/nodejs/node/issues/10806 + +require('../common'); +const assert = require('assert'); +const vm = require('vm'); +const ctx = vm.createContext({ open() { } }); +const window = vm.runInContext('this', ctx); +const other = 123; + +assert.notStrictEqual(window.open, other); +window.open = other; +assert.strictEqual(window.open, other); +window.open = other; +assert.strictEqual(window.open, other);