Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

Commit fb90b6b

Browse filesBrowse 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
Copy full SHA for fb90b6b

File tree

Expand file treeCollapse file tree

5 files changed

+203
-12
lines changed
Open diff view settings
Filter options
Expand file treeCollapse file tree

5 files changed

+203
-12
lines changed
Open diff view settings
Collapse file

‎src/env_properties.h‎

Copy file name to clipboardExpand all lines: src/env_properties.h
+2Lines 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") \
Collapse file

‎src/node_contextify.cc‎

Copy file name to clipboardExpand all lines: src/node_contextify.cc
+15-3Lines 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

Collapse file

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

Copy file name to clipboardExpand all lines: test/parallel/test-vm-global-setter.js
+138-9Lines changed: 138 additions & 9 deletions
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);
Collapse file

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

Copy file name to clipboardExpand all lines: test/parallel/test-vm-global-symbol.js
+11Lines changed: 11 additions & 0 deletions
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));
Collapse file

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

Copy file name to clipboard
+37Lines 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+
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 (0)
Morty Proxy This is a proxified and sanitized view of the page, visit original site.