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: #47799
PR-URL: #51602
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
  • Loading branch information
fhinkel authored and richardlau committed Mar 25, 2024
1 parent bca6dcc commit 62707a9
Show file tree
Hide file tree
Showing 2 changed files with 21 additions and 3 deletions.
9 changes: 6 additions & 3 deletions src/node_contextify.cc
Original file line number Diff line number Diff line change
Expand Up @@ -609,11 +609,14 @@ 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();

const 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 62707a9

Please sign in to comment.