Skip to content

Commit

Permalink
src: fix vm bug for configurable globalThis
Browse files Browse the repository at this point in the history
Object.defineProperty allows to change the value for non-writable
properties if they are configurable. We missed that case when checking if a
property is read-only.

Fixes: nodejs#47799
  • Loading branch information
fhinkel committed Jan 30, 2024
1 parent d4eb03f commit 12f69ff
Show file tree
Hide file tree
Showing 2 changed files with 22 additions and 3 deletions.
10 changes: 7 additions & 3 deletions src/node_contextify.cc
Original file line number Diff line number Diff line change
Expand Up @@ -609,11 +609,15 @@ void ContextifyContext::PropertyDefinerCallback(
bool read_only =
static_cast<int>(attributes) &
static_cast<int>(PropertyAttribute::ReadOnly);
bool dont_delete =
static_cast<int>(attributes) &
static_cast<int>(PropertyAttribute::DontDelete);

// If the property is set on the global as read_only, don't change it on
// the global or sandbox.
if (is_declared && read_only)
// If the property is set on the global as neither writable nor
// configurable, don't change it on the global or sandbox.
if (is_declared && read_only && dont_delete) {
return;
}

Local<Object> sandbox = ctx->sandbox();

Expand Down
15 changes: 15 additions & 0 deletions test/parallel/test-vm-global-configurable-properties.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
'use strict';
// https://github.com/nodejs/node/issues/47799

require('../common');
const assert = require('assert');
const vm = require('vm');

const ctx = vm.createContext();

let window = vm.runInContext("this", ctx);

Object.defineProperty(window, "x", { value: "1", configurable: true});
assert.strictEqual(window.x, "1");
Object.defineProperty(window, "x", { value: "2", configurable: true });
assert.strictEqual(window.x, "2");

0 comments on commit 12f69ff

Please sign in to comment.