From 3e851cf18b06c50df79f2ecc4dce0ec7379c2f41 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Fri, 20 Jan 2017 19:36:57 +0100 Subject: [PATCH] Revert "src: don't overwrite non-writable vm globals" This reverts commit 524f693872cf453af2655ec47356d25d52394e3d. Fixes: https://github.com/nodejs/node/issues/10806 Fixes: https://github.com/nodejs/node/issues/10492 Ref: https://github.com/nodejs/node/pull/10227 PR-URL: https://github.com/nodejs/node/pull/10920 Reviewed-By: Franziska Hinkelmann Reviewed-By: James M Snell Reviewed-By: Ben Noordhuis --- src/node_contextify.cc | 23 ++++++++++------------- test/parallel/test-vm-context.js | 11 ----------- 2 files changed, 10 insertions(+), 24 deletions(-) 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/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);