Skip to content

Commit fb90b6b

Browse files
dubzzzMylesBorins
dubzzz
authored and
MylesBorins
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 <vlad2t@hotmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>
1 parent e5e385d commit fb90b6b

5 files changed

Lines changed: 203 additions & 12 deletions

File tree

‎src/env_properties.h‎

Lines changed: 2 additions & 0 deletions
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‎

Lines changed: 15 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -529,9 +529,21 @@ void ContextifyContext::PropertySetterCallback(
529529
!is_function)
530530
return;
531531

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

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

Lines changed: 138 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -3,27 +3,156 @@ const common = require('../common');
33
constassert=require('assert');
44
constvm=require('vm');
55

6+
constgetSetSymbolReceivingFunction=Symbol('sym-1');
7+
constgetSetSymbolReceivingNumber=Symbol('sym-2');
8+
constsymbolReceivingNumber=Symbol('sym-3');
9+
constunknownSymbolReceivingNumber=Symbol('sym-4');
10+
611
constwindow=createWindow();
712

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

11-
assert.strictEqual(typeofdescriptor.get,'function');
12-
assert.strictEqual(typeofdescriptor.set,'function');
13-
assert.strictEqual(descriptor.configurable,true);
63+
constdescriptor8=Object.getOwnPropertyDescriptor(
64+
window.globalProxy,
65+
unknownSymbolReceivingNumber
66+
);
67+
assert.strictEqual(descriptor8,undefined);
68+
69+
constdescriptor9=Object.getOwnPropertyDescriptor(
70+
window.globalProxy,
71+
'getSetPropThrowing'
72+
);
73+
assert.strictEqual(typeofdescriptor9.get,'function');
74+
assert.strictEqual(typeofdescriptor9.set,'function');
75+
assert.strictEqual(descriptor9.configurable,true);
76+
77+
constdescriptor10=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+
newError('setter called')
112+
);
113+
assert.strictEqual(window.globalProxy.getSetPropThrowing,50);
114+
115+
assert.throws(
116+
()=>(window.globalProxy.nonWritableProp=151),
117+
newTypeError('Cannot redefine property: nonWritableProp')
118+
);
119+
assert.strictEqual(window.globalProxy.nonWritableProp,51);
19120

20121
functioncreateWindow(){
21122
constobj={};
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+
thrownewError('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‎

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@ const assert = require('assert');
44
constvm=require('vm');
55

66
constglobal=vm.runInContext('this',vm.createContext());
7+
78
consttotoSymbol=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+
consttotoKey='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‎

Lines changed: 37 additions & 0 deletions
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+
varassert=require('assert');
7+
varvm=require('vm');
8+
9+
vardata=[];
10+
vara='direct';
11+
deletea;
12+
data.push(a);
13+
14+
varitem2=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(typeofunusedB,'number');// Declared within runInThisContext
37+
assert.strictEqual(typeofunusedC,'undefined');// Declared within runInContext

0 commit comments

Comments
 (0)