Skip to content

Commit 0dfe288

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 b981253 commit 0dfe288

4 files changed

Lines changed: 814 additions & 618 deletions

File tree

lib/repl.js

Lines changed: 42 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1328,9 +1328,11 @@ function completeFSFunctions(match) {
13281328
// -> [['util.print', 'util.debug', 'util.log', 'util.inspect'],
13291329
// 'util.' ]
13301330
//
1331-
// Warning: This eval's code like "foo.bar.baz", so it will run property
1332-
// getter code.
1333-
function complete(line, callback) {
1331+
// Warning: This eval's code like "foo.bar.baz", so it could run property
1332+
// getter code. To avoid potential side-effecful getters the completion
1333+
// logic is skipped when getters or proxies are involved in the expression.
1334+
// (see: https://github.com/nodejs/node/issues/57829).
1335+
async function complete(line, callback) {
13341336
// List of completion lists, one for each inheritance "level"
13351337
let completionGroups = [];
13361338
let completeOn, group;
@@ -1525,6 +1527,12 @@ function complete(line, callback) {
15251527
return;
15261528
}
15271529

1530+
if (await includesProxiesOrGetters(match, this.eval, this.context)) {
1531+
// The expression involves proxies or getters, meaning that it
1532+
// can trigger side-effectful behaviors, so bail out
1533+
return completionGroupsLoaded();
1534+
}
1535+
15281536
let chaining = '.';
15291537
if (StringPrototypeEndsWith(expr, '?')) {
15301538
expr = StringPrototypeSlice(expr, 0, -1);
@@ -1626,6 +1634,37 @@ function complete(line, callback) {
16261634
}
16271635
}
16281636

1637+
async function includesProxiesOrGetters(fullExpr, evalFn, context) {
1638+
const bits = StringPrototypeSplit(fullExpr, '.');
1639+
1640+
let currentExpr = '';
1641+
for (let i = 0; i < bits.length - 1; i++) {
1642+
currentExpr += `${i === 0 ? '' : '.'}${bits[i]}`;
1643+
const currentExprIsObject = await evalPromisified(`try { ${currentExpr} !== null && typeof ${currentExpr} === 'object' } catch { false }`);
1644+
if (!currentExprIsObject) {
1645+
return false;
1646+
}
1647+
1648+
const currentExprIsProxy = await evalPromisified(`require("node:util/types").isProxy(${currentExpr})`);
1649+
if (currentExprIsProxy) {
1650+
return true;
1651+
}
1652+
1653+
const typeOfNextBitGet = await evalPromisified(`typeof Object.getOwnPropertyDescriptor(${currentExpr}, '${bits[i + 1]}')?.get`);
1654+
const nextBitHasGetter = typeOfNextBitGet === 'function';
1655+
if (nextBitHasGetter) {
1656+
return true;
1657+
}
1658+
}
1659+
1660+
function evalPromisified(evalExpr) {
1661+
return new Promise((resolve, reject) =>
1662+
evalFn(evalExpr, context, getREPLResourceName(), (_, res) => {
1663+
resolve(res);
1664+
}));
1665+
}
1666+
}
1667+
16291668
REPLServer.prototype.completeOnEditorMode = (callback) => (err, results) => {
16301669
if (err) return callback(err);
16311670

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)