Skip to content

Commit 2360186

Browse files
lib: disable REPL completion on proxies and getters
the REPL completion logic evaluates code, this is generally ok but it can be problematic when there are objects with nested properties since the code evaluation would trigger their potential getters (e.g. the `obj.foo.b` line would trigger the getter of `foo`), the changes here disable the completion logic when proxies and getters are involved thus making sure that code evaluation does not take place when it can potentially (and surprisingly to the user) trigger side effectful behaviors
1 parent 95b0f9d commit 2360186

File tree

4 files changed

+814
-618
lines changed

4 files changed

+814
-618
lines changed

lib/repl.js

Lines changed: 42 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1311,9 +1311,11 @@ function completeFSFunctions(match) {
13111311
// -> [['util.print', 'util.debug', 'util.log', 'util.inspect'],
13121312
// 'util.' ]
13131313
//
1314-
// Warning: This eval's code like "foo.bar.baz", so it will run property
1315-
// getter code.
1316-
function complete(line, callback) {
1314+
// Warning: This eval's code like "foo.bar.baz", so it could run property
1315+
// getter code. To avoid potential side-effecful getters the completion
1316+
// logic is skipped when getters or proxies are involved in the expression.
1317+
// (see: https://github.com/nodejs/node/issues/57829).
1318+
async function complete(line, callback) {
13171319
// List of completion lists, one for each inheritance "level"
13181320
let completionGroups = [];
13191321
let completeOn, group;
@@ -1508,6 +1510,12 @@ function complete(line, callback) {
15081510
return;
15091511
}
15101512

1513+
if (await includesProxiesOrGetters(match, this.eval, this.context)) {
1514+
// The expression involves proxies or getters, meaning that it
1515+
// can trigger side-effectful behaviors, so bail out
1516+
return completionGroupsLoaded();
1517+
}
1518+
15111519
let chaining = '.';
15121520
if (StringPrototypeEndsWith(expr, '?')) {
15131521
expr = StringPrototypeSlice(expr, 0, -1);
@@ -1609,6 +1617,37 @@ function complete(line, callback) {
16091617
}
16101618
}
16111619

1620+
async function includesProxiesOrGetters(fullExpr, evalFn, context) {
1621+
const bits = StringPrototypeSplit(fullExpr, '.');
1622+
1623+
let currentExpr = '';
1624+
for (let i = 0; i < bits.length - 1; i++) {
1625+
currentExpr += `${i===0? '' : '.'}${bits[i]}`;
1626+
const currentExprIsObject = await evalPromisified(`try { ${currentExpr} !== null && typeof ${currentExpr} === 'object' } catch { false }`);
1627+
if (!currentExprIsObject) {
1628+
return false;
1629+
}
1630+
1631+
const currentExprIsProxy = await evalPromisified(`require("node:util/types").isProxy(${currentExpr})`);
1632+
if (currentExprIsProxy) {
1633+
return true;
1634+
}
1635+
1636+
const typeOfNextBitGet = await evalPromisified(`typeof Object.getOwnPropertyDescriptor(${currentExpr}, '${bits[i+1]}')?.get`);
1637+
const nextBitHasGetter = typeOfNextBitGet === 'function';
1638+
if (nextBitHasGetter) {
1639+
return true;
1640+
}
1641+
}
1642+
1643+
function evalPromisified (evalExpr) {
1644+
return new Promise((resolve, reject) =>
1645+
evalFn(evalExpr, context, getREPLResourceName(), (_, res) => {
1646+
resolve(res);
1647+
}));
1648+
}
1649+
}
1650+
16121651
REPLServer.prototype.completeOnEditorMode = (callback) => (err, results) => {
16131652
if (err) return callback(err);
16141653

Lines changed: 119 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,119 @@
1+
'use strict';
2+
3+
const common = require('../common');
4+
const assert = require('node:assert');
5+
const { describe, test } = require('node:test');
6+
7+
const ArrayStream = require('../common/arraystream');
8+
9+
const repl = require('node:repl');
10+
11+
function runCompletionTests(replInit, tests) {
12+
const stream = new ArrayStream();
13+
const testRepl = repl.start({ stream });
14+
15+
// Some errors are passed to the domain
16+
testRepl._domain.on('error', assert.ifError);
17+
18+
testRepl.write(replInit);
19+
testRepl.write('\n');
20+
21+
tests.forEach(([query, expectedCompletions]) => {
22+
testRepl.complete(query, common.mustCall((error, data) => {
23+
const actualCompletions = data[0];
24+
if (expectedCompletions.length === 0) {
25+
assert.deepStrictEqual(actualCompletions, []);
26+
} else {
27+
expectedCompletions.forEach((expectedCompletion) =>
28+
assert(actualCompletions.includes(expectedCompletion), `completion '${expectedCompletion}' not found`)
29+
);
30+
}
31+
}));
32+
});
33+
}
34+
35+
describe('REPL completion in relation of getters', () => {
36+
describe('standard behavior without proxies/getters', () => {
37+
test('completion of nested properties of an undeclared objects', () => {
38+
runCompletionTests('', [
39+
['nonExisting.', []],
40+
['nonExisting.f', []],
41+
['nonExisting.foo', []],
42+
['nonExisting.foo.', []],
43+
['nonExisting.foo.bar.b', []],
44+
]);
45+
});
46+
47+
test('completion of nested properties on plain objects', () => {
48+
runCompletionTests('const plainObj = { foo: { bar: { baz: {} } } };', [
49+
['plainObj.', ['plainObj.foo']],
50+
['plainObj.f', ['plainObj.foo']],
51+
['plainObj.foo', ['plainObj.foo']],
52+
['plainObj.foo.', ['plainObj.foo.bar']],
53+
['plainObj.foo.bar.b', ['plainObj.foo.bar.baz']],
54+
['plainObj.fooBar.', []],
55+
['plainObj.fooBar.baz', []],
56+
]);
57+
});
58+
});
59+
60+
describe('completions on an object with getters', () => {
61+
test(`completions are generated for properties that don't trigger getters`, () => {
62+
runCompletionTests(
63+
`
64+
const objWithGetters = {
65+
foo: { bar: { baz: {} }, get gBar() { return { baz: {} } } },
66+
get gFoo() { return { bar: { baz: {} } }; }
67+
};
68+
`, [
69+
['objWithGetters.', ['objWithGetters.foo']],
70+
['objWithGetters.f', ['objWithGetters.foo']],
71+
['objWithGetters.foo', ['objWithGetters.foo']],
72+
['objWithGetters.foo.', ['objWithGetters.foo.bar']],
73+
['objWithGetters.foo.bar.b', ['objWithGetters.foo.bar.baz']],
74+
['objWithGetters.gFo', ['objWithGetters.gFoo']],
75+
['objWithGetters.foo.gB', ['objWithGetters.foo.gBar']],
76+
]);
77+
});
78+
79+
test('no completions are generated for properties that trigger getters', () => {
80+
runCompletionTests(
81+
`
82+
const objWithGetters = {
83+
foo: { bar: { baz: {} }, get gBar() { return { baz: {} } } },
84+
get gFoo() { return { bar: { baz: {} } }; }
85+
};
86+
`,
87+
[
88+
['objWithGetters.gFoo.', []],
89+
['objWithGetters.gFoo.b', []],
90+
['objWithGetters.gFoo.bar.b', []],
91+
['objWithGetters.foo.gBar.', []],
92+
['objWithGetters.foo.gBar.b', []],
93+
]);
94+
});
95+
});
96+
97+
describe('completions on proxies', () => {
98+
test('no completions are generated for a proxy object', () => {
99+
runCompletionTests('const proxyObj = new Proxy({ foo: { bar: { baz: {} } } }, {});', [
100+
['proxyObj.', []],
101+
['proxyObj.f', []],
102+
['proxyObj.foo', []],
103+
['proxyObj.foo.', []],
104+
['proxyObj.foo.bar.b', []],
105+
]);
106+
});
107+
108+
test('no completions are generated for a proxy present in a standard object', () => {
109+
runCompletionTests(
110+
'const objWithProxy = { foo: { bar: new Proxy({ baz: {} }, {}) } };', [
111+
['objWithProxy.', ['objWithProxy.foo']],
112+
['objWithProxy.foo', ['objWithProxy.foo']],
113+
['objWithProxy.foo.', ['objWithProxy.foo.bar']],
114+
['objWithProxy.foo.b', ['objWithProxy.foo.bar']],
115+
['objWithProxy.foo.bar.', []],
116+
]);
117+
});
118+
});
119+
});

0 commit comments

Comments
 (0)