diff --git a/src/node_contextify.cc b/src/node_contextify.cc index dc939c6cf98fe3..988de8bb506438 100644 --- a/src/node_contextify.cc +++ b/src/node_contextify.cc @@ -414,15 +414,21 @@ class ContextifyContext { return; auto attributes = PropertyAttribute::None; - bool is_declared = - ctx->global_proxy()->GetRealNamedPropertyAttributes(ctx->context(), - property) + bool is_declared_on_global_proxy = ctx->global_proxy() + ->GetRealNamedPropertyAttributes(ctx->context(), property) .To(&attributes); bool read_only = static_cast(attributes) & static_cast(PropertyAttribute::ReadOnly); - if (is_declared && read_only) + bool is_declared_on_sandbox = ctx->sandbox() + ->GetRealNamedPropertyAttributes(ctx->context(), property) + .To(&attributes); + read_only = read_only || + (static_cast(attributes) & + static_cast(PropertyAttribute::ReadOnly)); + + if (read_only) return; // true for x = 5 @@ -440,10 +446,20 @@ class ContextifyContext { // this.f = function() {}, is_contextual_store = false. bool is_function = value->IsFunction(); + bool is_declared = is_declared_on_global_proxy || is_declared_on_sandbox; if (!is_declared && args.ShouldThrowOnError() && is_contextual_store && !is_function) return; + if (!is_declared_on_global_proxy && is_declared_on_sandbox && + args.ShouldThrowOnError() && is_contextual_store && !is_function) { + // The property exists on the sandbox but not on the global + // proxy. Setting it would throw because we are in strict mode. + // Don't attempt to set it by signaling that the call was + // intercepted. Only change the value on the sandbox. + args.GetReturnValue().Set(false); + } + ctx->sandbox()->Set(property, value); } diff --git a/test/known_issues/test-vm-strict-mode.js b/test/known_issues/test-vm-strict-mode.js deleted file mode 100644 index 9528944732930c..00000000000000 --- a/test/known_issues/test-vm-strict-mode.js +++ /dev/null @@ -1,17 +0,0 @@ -'use strict'; -// https://github.com/nodejs/node/issues/12300 - -require('../common'); -const assert = require('assert'); -const vm = require('vm'); - -const ctx = vm.createContext({ x: 42 }); - -// The following line wrongly throws an -// error because GlobalPropertySetterCallback() -// does not check if the property exists -// on the sandbox. It should just set x to 1 -// instead of throwing an error. -vm.runInContext('"use strict"; x = 1', ctx); - -assert.strictEqual(ctx.x, 1); diff --git a/test/parallel/test-vm-strict-mode.js b/test/parallel/test-vm-strict-mode.js index f3820d8aead6af..b1b233664dab9b 100644 --- a/test/parallel/test-vm-strict-mode.js +++ b/test/parallel/test-vm-strict-mode.js @@ -1,27 +1,14 @@ 'use strict'; +// https://github.com/nodejs/node/issues/12300 + require('../common'); const assert = require('assert'); const vm = require('vm'); -const ctx = vm.createContext(); - -// Test strict mode inside a vm script, i.e., using an undefined variable -// throws a ReferenceError. Also check that variables -// that are not successfully set in the vm, must not be set -// on the sandboxed context. - -vm.runInContext('w = 1;', ctx); -assert.strictEqual(1, ctx.w); - -assert.throws(function() { vm.runInContext('"use strict"; x = 1;', ctx); }, - /ReferenceError: x is not defined/); -assert.strictEqual(undefined, ctx.x); -vm.runInContext('"use strict"; var y = 1;', ctx); -assert.strictEqual(1, ctx.y); +const ctx = vm.createContext({ x: 42 }); -vm.runInContext('"use strict"; this.z = 1;', ctx); -assert.strictEqual(1, ctx.z); +// This might look as if x has not been declared, but x is defined on the +// sandbox and the assignment should not throw. +vm.runInContext('"use strict"; x = 1', ctx); -// w has been defined -vm.runInContext('"use strict"; w = 2;', ctx); -assert.strictEqual(2, ctx.w); +assert.strictEqual(ctx.x, 1);