Skip to content

Commit 9537672

Browse files
dubzzztargos
dubzzz
authored and
targos
committed
vm: properly handle defining props on any value
While it was supposed to fix most of the remaining issues, #46458 missed some in strict mode. This PR adds some additional checks. It also clarifies what we are really checking to execute or not the `GetReturnValue`. PR-URL: #46615 Reviewed-By: Vladimir de Turckheim <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>
1 parent 7bcfd18 commit 9537672

5 files changed

+203
-12
lines changed

‎src/env_properties.h

+2
Original file line numberDiff line numberDiff line change
@@ -136,6 +136,7 @@
136136
V(frames_received_string, "framesReceived") \
137137
V(frames_sent_string, "framesSent") \
138138
V(function_string, "function") \
139+
V(get_string, "get") \
139140
V(get_data_clone_error_string, "_getDataCloneError") \
140141
V(get_shared_array_buffer_id_string, "_getSharedArrayBufferId") \
141142
V(gid_string, "gid") \
@@ -269,6 +270,7 @@
269270
V(servername_string, "servername") \
270271
V(service_string, "service") \
271272
V(session_id_string, "sessionId") \
273+
V(set_string, "set") \
272274
V(shell_string, "shell") \
273275
V(signal_string, "signal") \
274276
V(sink_string, "sink") \

‎src/node_contextify.cc

+15-3
Original file line numberDiff line numberDiff line change
@@ -527,9 +527,21 @@ void ContextifyContext::PropertySetterCallback(
527527
!is_function)
528528
return;
529529

530-
USE(ctx->sandbox()->Set(context, property, value));
531-
if (is_contextual_store || is_function) {
532-
args.GetReturnValue().Set(value);
530+
if (ctx->sandbox()->Set(context, property, value).IsNothing()) return;
531+
532+
Local<Value> desc;
533+
if (is_declared_on_sandbox &&
534+
ctx->sandbox()
535+
->GetOwnPropertyDescriptor(context, property)
536+
.ToLocal(&desc)) {
537+
Environment* env = Environment::GetCurrent(context);
538+
Local<Object> desc_obj = desc.As<Object>();
539+
540+
// We have to specify the return value for any contextual or get/set
541+
// property
542+
if (desc_obj->HasOwnProperty(context, env->get_string()).FromMaybe(false) ||
543+
desc_obj->HasOwnProperty(context, env->set_string()).FromMaybe(false))
544+
args.GetReturnValue().Set(value);
533545
}
534546
}
535547

‎test/parallel/test-vm-global-setter.js

+138-9
Original file line numberDiff line numberDiff line change
@@ -3,27 +3,156 @@ const common = require('../common');
33
const assert = require('assert');
44
const vm = require('vm');
55

6+
const getSetSymbolReceivingFunction = Symbol('sym-1');
7+
const getSetSymbolReceivingNumber = Symbol('sym-2');
8+
const symbolReceivingNumber = Symbol('sym-3');
9+
const unknownSymbolReceivingNumber = Symbol('sym-4');
10+
611
const window = createWindow();
712

8-
const descriptor =
9-
Object.getOwnPropertyDescriptor(window.globalProxy, 'onhashchange');
13+
const descriptor1 = Object.getOwnPropertyDescriptor(
14+
window.globalProxy,
15+
'getSetPropReceivingFunction'
16+
);
17+
assert.strictEqual(typeof descriptor1.get, 'function');
18+
assert.strictEqual(typeof descriptor1.set, 'function');
19+
assert.strictEqual(descriptor1.configurable, true);
20+
21+
const descriptor2 = Object.getOwnPropertyDescriptor(
22+
window.globalProxy,
23+
'getSetPropReceivingNumber'
24+
);
25+
assert.strictEqual(typeof descriptor2.get, 'function');
26+
assert.strictEqual(typeof descriptor2.set, 'function');
27+
assert.strictEqual(descriptor2.configurable, true);
28+
29+
const descriptor3 = Object.getOwnPropertyDescriptor(
30+
window.globalProxy,
31+
'propReceivingNumber'
32+
);
33+
assert.strictEqual(descriptor3.value, 44);
34+
35+
const descriptor4 = Object.getOwnPropertyDescriptor(
36+
window.globalProxy,
37+
'unknownPropReceivingNumber'
38+
);
39+
assert.strictEqual(descriptor4, undefined);
40+
41+
const descriptor5 = Object.getOwnPropertyDescriptor(
42+
window.globalProxy,
43+
getSetSymbolReceivingFunction
44+
);
45+
assert.strictEqual(typeof descriptor5.get, 'function');
46+
assert.strictEqual(typeof descriptor5.set, 'function');
47+
assert.strictEqual(descriptor5.configurable, true);
48+
49+
const descriptor6 = Object.getOwnPropertyDescriptor(
50+
window.globalProxy,
51+
getSetSymbolReceivingNumber
52+
);
53+
assert.strictEqual(typeof descriptor6.get, 'function');
54+
assert.strictEqual(typeof descriptor6.set, 'function');
55+
assert.strictEqual(descriptor6.configurable, true);
56+
57+
const descriptor7 = Object.getOwnPropertyDescriptor(
58+
window.globalProxy,
59+
symbolReceivingNumber
60+
);
61+
assert.strictEqual(descriptor7.value, 48);
1062

11-
assert.strictEqual(typeof descriptor.get, 'function');
12-
assert.strictEqual(typeof descriptor.set, 'function');
13-
assert.strictEqual(descriptor.configurable, true);
63+
const descriptor8 = Object.getOwnPropertyDescriptor(
64+
window.globalProxy,
65+
unknownSymbolReceivingNumber
66+
);
67+
assert.strictEqual(descriptor8, undefined);
68+
69+
const descriptor9 = Object.getOwnPropertyDescriptor(
70+
window.globalProxy,
71+
'getSetPropThrowing'
72+
);
73+
assert.strictEqual(typeof descriptor9.get, 'function');
74+
assert.strictEqual(typeof descriptor9.set, 'function');
75+
assert.strictEqual(descriptor9.configurable, true);
76+
77+
const descriptor10 = Object.getOwnPropertyDescriptor(
78+
window.globalProxy,
79+
'nonWritableProp'
80+
);
81+
assert.strictEqual(descriptor10.value, 51);
82+
assert.strictEqual(descriptor10.writable, false);
1483

1584
// Regression test for GH-42962. This assignment should not throw.
16-
window.globalProxy.onhashchange = () => {};
85+
window.globalProxy.getSetPropReceivingFunction = () => {};
86+
assert.strictEqual(window.globalProxy.getSetPropReceivingFunction, 42);
87+
88+
window.globalProxy.getSetPropReceivingNumber = 143;
89+
assert.strictEqual(window.globalProxy.getSetPropReceivingNumber, 43);
90+
91+
window.globalProxy.propReceivingNumber = 144;
92+
assert.strictEqual(window.globalProxy.propReceivingNumber, 144);
93+
94+
window.globalProxy.unknownPropReceivingNumber = 145;
95+
assert.strictEqual(window.globalProxy.unknownPropReceivingNumber, 145);
96+
97+
window.globalProxy[getSetSymbolReceivingFunction] = () => {};
98+
assert.strictEqual(window.globalProxy[getSetSymbolReceivingFunction], 46);
1799

18-
assert.strictEqual(window.globalProxy.onhashchange, 42);
100+
window.globalProxy[getSetSymbolReceivingNumber] = 147;
101+
assert.strictEqual(window.globalProxy[getSetSymbolReceivingNumber], 47);
102+
103+
window.globalProxy[symbolReceivingNumber] = 148;
104+
assert.strictEqual(window.globalProxy[symbolReceivingNumber], 148);
105+
106+
window.globalProxy[unknownSymbolReceivingNumber] = 149;
107+
assert.strictEqual(window.globalProxy[unknownSymbolReceivingNumber], 149);
108+
109+
assert.throws(
110+
() => (window.globalProxy.getSetPropThrowing = 150),
111+
new Error('setter called')
112+
);
113+
assert.strictEqual(window.globalProxy.getSetPropThrowing, 50);
114+
115+
assert.throws(
116+
() => (window.globalProxy.nonWritableProp = 151),
117+
new TypeError('Cannot redefine property: nonWritableProp')
118+
);
119+
assert.strictEqual(window.globalProxy.nonWritableProp, 51);
19120

20121
function createWindow() {
21122
const obj = {};
22123
vm.createContext(obj);
23-
Object.defineProperty(obj, 'onhashchange', {
124+
Object.defineProperty(obj, 'getSetPropReceivingFunction', {
24125
get: common.mustCall(() => 42),
25126
set: common.mustCall(),
26-
configurable: true
127+
configurable: true,
128+
});
129+
Object.defineProperty(obj, 'getSetPropReceivingNumber', {
130+
get: common.mustCall(() => 43),
131+
set: common.mustCall(),
132+
configurable: true,
133+
});
134+
obj.propReceivingNumber = 44;
135+
Object.defineProperty(obj, getSetSymbolReceivingFunction, {
136+
get: common.mustCall(() => 46),
137+
set: common.mustCall(),
138+
configurable: true,
139+
});
140+
Object.defineProperty(obj, getSetSymbolReceivingNumber, {
141+
get: common.mustCall(() => 47),
142+
set: common.mustCall(),
143+
configurable: true,
144+
});
145+
obj[symbolReceivingNumber] = 48;
146+
Object.defineProperty(obj, 'getSetPropThrowing', {
147+
get: common.mustCall(() => 50),
148+
set: common.mustCall(() => {
149+
throw new Error('setter called');
150+
}),
151+
configurable: true,
152+
});
153+
Object.defineProperty(obj, 'nonWritableProp', {
154+
value: 51,
155+
writable: false,
27156
});
28157

29158
obj.globalProxy = vm.runInContext('this', obj);

‎test/parallel/test-vm-global-symbol.js

+11
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@ const assert = require('assert');
44
const vm = require('vm');
55

66
const global = vm.runInContext('this', vm.createContext());
7+
78
const totoSymbol = Symbol.for('toto');
89
Object.defineProperty(global, totoSymbol, {
910
enumerable: true,
@@ -13,3 +14,13 @@ Object.defineProperty(global, totoSymbol, {
1314
});
1415
assert.strictEqual(global[totoSymbol], 4);
1516
assert.ok(Object.getOwnPropertySymbols(global).includes(totoSymbol));
17+
18+
const totoKey = 'toto';
19+
Object.defineProperty(global, totoKey, {
20+
enumerable: true,
21+
writable: true,
22+
value: 5,
23+
configurable: true,
24+
});
25+
assert.strictEqual(global[totoKey], 5);
26+
assert.ok(Object.getOwnPropertyNames(global).includes(totoKey));

‎test/parallel/test-vm-not-strict.js

+37
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,37 @@
1+
/* eslint-disable strict, no-var, no-delete-var, no-undef, node-core/required-modules, node-core/require-common-first */
2+
// Importing common would break the execution. Indeed running `vm.runInThisContext` alters the global context
3+
// when declaring new variables with `var`. The other rules (strict, no-var, no-delete-var) have been disabled
4+
// in order to be able to test this specific not-strict case playing with `var` and `delete`.
5+
// Related to bug report: https://github.com/nodejs/node/issues/43129
6+
var assert = require('assert');
7+
var vm = require('vm');
8+
9+
var data = [];
10+
var a = 'direct';
11+
delete a;
12+
data.push(a);
13+
14+
var item2 = vm.runInThisContext(`
15+
var unusedB = 1;
16+
var data = [];
17+
var b = "this";
18+
delete b;
19+
data.push(b);
20+
data[0]
21+
`);
22+
data.push(item2);
23+
24+
vm.runInContext(
25+
`
26+
var unusedC = 1;
27+
var c = "new";
28+
delete c;
29+
data.push(c);
30+
`,
31+
vm.createContext({ data: data })
32+
);
33+
34+
assert.deepStrictEqual(data, ['direct', 'this', 'new']);
35+
36+
assert.strictEqual(typeof unusedB, 'number'); // Declared within runInThisContext
37+
assert.strictEqual(typeof unusedC, 'undefined'); // Declared within runInContext

0 commit comments

Comments
 (0)