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

PR-URL: https://github.com/nodejs/node/pull/57909
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

lib/repl.js

@@ -96,6 +96,10 @@ const {
   globalThis,
 } = primordials;
 
+const {
+  isProxy,
+} = require('internal/util/types');
+
 const { BuiltinModule } = require('internal/bootstrap/realm');
 const {
   makeRequireFunction,
@@ -1328,8 +1332,10 @@ function completeFSFunctions(match) {
 //    -> [['util.print', 'util.debug', 'util.log', 'util.inspect'],
 //        'util.' ]
 //
-// Warning: This eval's code like "foo.bar.baz", so it will run property
-// getter code.
+// Warning: This evals code like "foo.bar.baz", so it could run property
+// getter code. To avoid potential triggering side-effects with getters the completion
+// logic is skipped when getters or proxies are involved in the expression.
+// (see: https://github.com/nodejs/node/issues/57829).
 function complete(line, callback) {
   // List of completion lists, one for each inheritance "level"
   let completionGroups = [];
@@ -1525,50 +1531,61 @@ function complete(line, callback) {
       return;
     }
 
-    let chaining = '.';
-    if (StringPrototypeEndsWith(expr, '?')) {
-      expr = StringPrototypeSlice(expr, 0, -1);
-      chaining = '?.';
-    }
-
-    const memberGroups = [];
-    const evalExpr = `try { ${expr} } catch {}`;
-    this.eval(evalExpr, this.context, getREPLResourceName(), (e, obj) => {
-      try {
-        let p;
-        if ((typeof obj === 'object' && obj !== null) ||
-            typeof obj === 'function') {
-          ArrayPrototypePush(memberGroups, filteredOwnPropertyNames(obj));
-          p = ObjectGetPrototypeOf(obj);
-        } else {
-          p = obj.constructor ? obj.constructor.prototype : null;
+    return includesProxiesOrGetters(
+      StringPrototypeSplit(match, '.'),
+      this.eval,
+      this.context,
+      (includes) => {
+        if (includes) {
+        // The expression involves proxies or getters, meaning that it
+        // can trigger side-effectful behaviors, so bail out
+          return completionGroupsLoaded();
         }
-        // Circular refs possible? Let's guard against that.
-        let sentinel = 5;
-        while (p !== null && sentinel-- !== 0) {
-          ArrayPrototypePush(memberGroups, filteredOwnPropertyNames(p));
-          p = ObjectGetPrototypeOf(p);
-        }
-      } catch {
-        // Maybe a Proxy object without `getOwnPropertyNames` trap.
-        // We simply ignore it here, as we don't want to break the
-        // autocompletion. Fixes the bug
-        // https://github.com/nodejs/node/issues/2119
-      }
 
-      if (memberGroups.length) {
-        expr += chaining;
-        ArrayPrototypeForEach(memberGroups, (group) => {
-          ArrayPrototypePush(completionGroups,
-                             ArrayPrototypeMap(group,
-                                               (member) => `${expr}${member}`));
+        let chaining = '.';
+        if (StringPrototypeEndsWith(expr, '?')) {
+          expr = StringPrototypeSlice(expr, 0, -1);
+          chaining = '?.';
+        }
+
+        const memberGroups = [];
+        const evalExpr = `try { ${expr} } catch {}`;
+        this.eval(evalExpr, this.context, getREPLResourceName(), (e, obj) => {
+          try {
+            let p;
+            if ((typeof obj === 'object' && obj !== null) ||
+              typeof obj === 'function') {
+              ArrayPrototypePush(memberGroups, filteredOwnPropertyNames(obj));
+              p = ObjectGetPrototypeOf(obj);
+            } else {
+              p = obj.constructor ? obj.constructor.prototype : null;
+            }
+            // Circular refs possible? Let's guard against that.
+            let sentinel = 5;
+            while (p !== null && sentinel-- !== 0) {
+              ArrayPrototypePush(memberGroups, filteredOwnPropertyNames(p));
+              p = ObjectGetPrototypeOf(p);
+            }
+          } catch {
+          // Maybe a Proxy object without `getOwnPropertyNames` trap.
+          // We simply ignore it here, as we don't want to break the
+          // autocompletion. Fixes the bug
+          // https://github.com/nodejs/node/issues/2119
+          }
+
+          if (memberGroups.length) {
+            expr += chaining;
+            ArrayPrototypeForEach(memberGroups, (group) => {
+              ArrayPrototypePush(completionGroups,
+                                 ArrayPrototypeMap(group,
+                                                   (member) => `${expr}${member}`));
+            });
+            filter &&= `${expr}${filter}`;
+          }
+
+          completionGroupsLoaded();
         });
-        filter &&= `${expr}${filter}`;
-      }
-
-      completionGroupsLoaded();
-    });
-    return;
+      });
   }
 
   return completionGroupsLoaded();
@@ -1626,6 +1643,34 @@ function complete(line, callback) {
   }
 }
 
+function includesProxiesOrGetters(exprSegments, evalFn, context, callback, currentExpr = '', idx = 0) {
+  const currentSegment = exprSegments[idx];
+  currentExpr += `${currentExpr.length === 0 ? '' : '.'}${currentSegment}`;
+  evalFn(`try { ${currentExpr} } catch { }`, context, getREPLResourceName(), (_, currentObj) => {
+    if (typeof currentObj !== 'object' || currentObj === null) {
+      return callback(false);
+    }
+
+    if (isProxy(currentObj)) {
+      return callback(true);
+    }
+
+    const nextIdx = idx + 1;
+
+    if (nextIdx >= exprSegments.length) {
+      return callback(false);
+    }
+
+    const nextSegmentProp = ObjectGetOwnPropertyDescriptor(currentObj, exprSegments[nextIdx]);
+    const nextSegmentPropHasGetter = typeof nextSegmentProp?.get === 'function';
+    if (nextSegmentPropHasGetter) {
+      return callback(true);
+    }
+
+    return includesProxiesOrGetters(exprSegments, evalFn, context, callback, currentExpr, nextIdx);
+  });
+}
+
 REPLServer.prototype.completeOnEditorMode = (callback) => (err, results) => {
   if (err) return callback(err);
 

test/parallel/test-repl-completion-on-getters-disabled.js

@@ -0,0 +1,119 @@
+'use strict';
+
+const common = require('../common');
+const assert = require('node:assert');
+const { describe, test } = require('node:test');
+
+const ArrayStream = require('../common/arraystream');
+
+const repl = require('node:repl');
+
+function runCompletionTests(replInit, tests) {
+  const stream = new ArrayStream();
+  const testRepl = repl.start({ stream });
+
+  // Some errors are passed to the domain
+  testRepl._domain.on('error', assert.ifError);
+
+  testRepl.write(replInit);
+  testRepl.write('\n');
+
+  tests.forEach(([query, expectedCompletions]) => {
+    testRepl.complete(query, common.mustCall((error, data) => {
+      const actualCompletions = data[0];
+      if (expectedCompletions.length === 0) {
+        assert.deepStrictEqual(actualCompletions, []);
+      } else {
+        expectedCompletions.forEach((expectedCompletion) =>
+          assert(actualCompletions.includes(expectedCompletion), `completion '${expectedCompletion}' not found`)
+        );
+      }
+    }));
+  });
+}
+
+describe('REPL completion in relation of getters', () => {
+  describe('standard behavior without proxies/getters', () => {
+    test('completion of nested properties of an undeclared objects', () => {
+      runCompletionTests('', [
+        ['nonExisting.', []],
+        ['nonExisting.f', []],
+        ['nonExisting.foo', []],
+        ['nonExisting.foo.', []],
+        ['nonExisting.foo.bar.b', []],
+      ]);
+    });
+
+    test('completion of nested properties on plain objects', () => {
+      runCompletionTests('const plainObj = { foo: { bar: { baz: {} } } };', [
+        ['plainObj.', ['plainObj.foo']],
+        ['plainObj.f', ['plainObj.foo']],
+        ['plainObj.foo', ['plainObj.foo']],
+        ['plainObj.foo.', ['plainObj.foo.bar']],
+        ['plainObj.foo.bar.b', ['plainObj.foo.bar.baz']],
+        ['plainObj.fooBar.', []],
+        ['plainObj.fooBar.baz', []],
+      ]);
+    });
+  });
+
+  describe('completions on an object with getters', () => {
+    test(`completions are generated for properties that don't trigger getters`, () => {
+      runCompletionTests(
+        `
+        const objWithGetters = {
+          foo: { bar: { baz: {} }, get gBar() { return { baz: {} } } },
+          get gFoo() { return { bar: { baz: {} } }; }
+        };
+        `, [
+          ['objWithGetters.', ['objWithGetters.foo']],
+          ['objWithGetters.f', ['objWithGetters.foo']],
+          ['objWithGetters.foo', ['objWithGetters.foo']],
+          ['objWithGetters.foo.', ['objWithGetters.foo.bar']],
+          ['objWithGetters.foo.bar.b', ['objWithGetters.foo.bar.baz']],
+          ['objWithGetters.gFo', ['objWithGetters.gFoo']],
+          ['objWithGetters.foo.gB', ['objWithGetters.foo.gBar']],
+        ]);
+    });
+
+    test('no completions are generated for properties that trigger getters', () => {
+      runCompletionTests(
+        `
+        const objWithGetters = {
+          foo: { bar: { baz: {} }, get gBar() { return { baz: {} } } },
+          get gFoo() { return { bar: { baz: {} } }; }
+        };
+        `,
+        [
+          ['objWithGetters.gFoo.', []],
+          ['objWithGetters.gFoo.b', []],
+          ['objWithGetters.gFoo.bar.b', []],
+          ['objWithGetters.foo.gBar.', []],
+          ['objWithGetters.foo.gBar.b', []],
+        ]);
+    });
+  });
+
+  describe('completions on proxies', () => {
+    test('no completions are generated for a proxy object', () => {
+      runCompletionTests('const proxyObj = new Proxy({ foo: { bar: { baz: {} } } }, {});', [
+        ['proxyObj.', []],
+        ['proxyObj.f', []],
+        ['proxyObj.foo', []],
+        ['proxyObj.foo.', []],
+        ['proxyObj.foo.bar.b', []],
+      ]);
+    });
+
+    test('no completions are generated for a proxy present in a standard object', () => {
+      runCompletionTests(
+        'const objWithProxy = { foo: { bar: new Proxy({ baz: {} }, {}) } };', [
+          ['objWithProxy.', ['objWithProxy.foo']],
+          ['objWithProxy.foo', ['objWithProxy.foo']],
+          ['objWithProxy.foo.', ['objWithProxy.foo.bar']],
+          ['objWithProxy.foo.b', ['objWithProxy.foo.bar']],
+          ['objWithProxy.foo.bar.', []],
+        ]);
+    });
+  });
+});

test/parallel/test-repl-save-load.js

@@ -27,138 +27,155 @@ const fs = require('fs');
 const tmpdir = require('../common/tmpdir');
 tmpdir.refresh();
 
-const repl = require('repl');
+// TODO: the following async IIFE and the completePromise function are necessary because
+//       the reply tests are all run against the same repl instance (testMe) and thus coordination
+//       needs to be in place for the tests not to interfere with each other, this is really
+//       not ideal, the tests in this file should be refactored so that each use its own isolated
+//       repl instance, making sure that no special coordination needs to be in place for them
+//       and also allowing the tests to all be run in parallel
+(async () => {
+  const repl = require('repl');
 
-const works = [['inner.one'], 'inner.o'];
+  const works = [['inner.one'], 'inner.o'];
 
-const putIn = new ArrayStream();
-const testMe = repl.start('', putIn);
-
-// Some errors might be passed to the domain.
-testMe._domain.on('error', function(reason) {
-  const err = new Error('Test failed');
-  err.reason = reason;
-  throw err;
-});
-
-const testFile = [
-  'let inner = (function() {',
-  '  return {one:1};',
-  '})()',
-];
-const saveFileName = tmpdir.resolve('test.save.js');
-
-// Add some data.
-putIn.run(testFile);
-
-// Save it to a file.
-putIn.run([`.save ${saveFileName}`]);
-
-// The file should have what I wrote.
-assert.strictEqual(fs.readFileSync(saveFileName, 'utf8'),
-                   testFile.join('\n'));
-
-// Make sure that the REPL data is "correct".
-testMe.complete('inner.o', common.mustSucceed((data) => {
-  assert.deepStrictEqual(data, works);
-}));
-
-// Clear the REPL.
-putIn.run(['.clear']);
-
-testMe._sawKeyPress = true;
-// Load the file back in.
-putIn.run([`.load ${saveFileName}`]);
-
-// Make sure loading doesn't insert extra indentation
-// https://github.com/nodejs/node/issues/47673
-assert.strictEqual(testMe.line, '');
-
-// Make sure that the REPL data is "correct".
-testMe.complete('inner.o', common.mustSucceed((data) => {
-  assert.deepStrictEqual(data, works);
-}));
-
-// Clear the REPL.
-putIn.run(['.clear']);
-
-let loadFile = tmpdir.resolve('file.does.not.exist');
-
-// Should not break.
-putIn.write = common.mustCall(function(data) {
-  // Make sure I get a failed to load message and not some crazy error.
-  assert.strictEqual(data, `Failed to load: ${loadFile}\n`);
-  // Eat me to avoid work.
-  putIn.write = () => {};
-});
-putIn.run([`.load ${loadFile}`]);
-
-// Throw error on loading directory.
-loadFile = tmpdir.path;
-putIn.write = common.mustCall(function(data) {
-  assert.strictEqual(data, `Failed to load: ${loadFile} is not a valid file\n`);
-  putIn.write = () => {};
-});
-putIn.run([`.load ${loadFile}`]);
-
-// Clear the REPL.
-putIn.run(['.clear']);
-
-// NUL (\0) is disallowed in filenames in UNIX-like operating systems and
-// Windows so we can use that to test failed saves.
-const invalidFileName = tmpdir.resolve('\0\0\0\0\0');
-
-// Should not break.
-putIn.write = common.mustCall(function(data) {
-  // Make sure I get a failed to save message and not some other error.
-  assert.strictEqual(data, `Failed to save: ${invalidFileName}\n`);
-  // Reset to no-op.
-  putIn.write = () => {};
-});
-
-// Save it to a file.
-putIn.run([`.save ${invalidFileName}`]);
-
-{
-  // Save .editor mode code.
-  const cmds = [
-    'function testSave() {',
-    'return "saved";',
-    '}',
-  ];
   const putIn = new ArrayStream();
-  const replServer = repl.start({ terminal: true, stream: putIn });
+  const testMe = repl.start('', putIn);
 
-  putIn.run(['.editor']);
-  putIn.run(cmds);
-  replServer.write('', { ctrl: true, name: 'd' });
+  // Some errors might be passed to the domain.
+  testMe._domain.on('error', function(reason) {
+    const err = new Error('Test failed');
+    err.reason = reason;
+    throw err;
+  });
 
+  async function completePromise(query, callback) {
+    return new Promise((resolve) => {
+      testMe.complete(query, (...args) => {
+        callback(...args);
+        resolve();
+      });
+    });
+  }
+
+  const testFile = [
+    'let inner = (function() {',
+    '  return {one:1};',
+    '})()',
+  ];
+  const saveFileName = tmpdir.resolve('test.save.js');
+
+  // Add some data.
+  putIn.run(testFile);
+
+  // Save it to a file.
   putIn.run([`.save ${saveFileName}`]);
-  replServer.close();
+
+  // The file should have what I wrote.
   assert.strictEqual(fs.readFileSync(saveFileName, 'utf8'),
-                     `${cmds.join('\n')}\n`);
-}
+                     testFile.join('\n'));
 
-// Check if the file is present when using save
+  // Make sure that the REPL data is "correct".
+  await completePromise('inner.o', common.mustSucceed((data) => {
+    assert.deepStrictEqual(data, works);
+  }));
 
-// Clear the REPL.
-putIn.run(['.clear']);
+  // Clear the REPL.
+  putIn.run(['.clear']);
 
-// Error message when using save without a file
-putIn.write = common.mustCall(function(data) {
-  assert.strictEqual(data, 'The "file" argument must be specified\n');
-  putIn.write = () => {};
-});
-putIn.run(['.save']);
+  testMe._sawKeyPress = true;
+  // Load the file back in.
+  putIn.run([`.load ${saveFileName}`]);
 
-// Check if the file is present when using load
+  // Make sure loading doesn't insert extra indentation
+  // https://github.com/nodejs/node/issues/47673
+  assert.strictEqual(testMe.line, '');
 
-// Clear the REPL.
-putIn.run(['.clear']);
+  // Make sure that the REPL data is "correct".
+  await completePromise('inner.o', common.mustSucceed((data) => {
+    assert.deepStrictEqual(data, works);
+  }));
 
-// Error message when using load without a file
-putIn.write = common.mustCall(function(data) {
-  assert.strictEqual(data, 'The "file" argument must be specified\n');
-  putIn.write = () => {};
-});
-putIn.run(['.load']);
+  // Clear the REPL.
+  putIn.run(['.clear']);
+
+  let loadFile = tmpdir.resolve('file.does.not.exist');
+
+  // Should not break.
+  putIn.write = common.mustCall(function(data) {
+    // Make sure I get a failed to load message and not some crazy error.
+    assert.strictEqual(data, `Failed to load: ${loadFile}\n`);
+    // Eat me to avoid work.
+    putIn.write = () => {};
+  });
+  putIn.run([`.load ${loadFile}`]);
+
+  // Throw error on loading directory.
+  loadFile = tmpdir.path;
+  putIn.write = common.mustCall(function(data) {
+    assert.strictEqual(data, `Failed to load: ${loadFile} is not a valid file\n`);
+    putIn.write = () => {};
+  });
+  putIn.run([`.load ${loadFile}`]);
+
+  // Clear the REPL.
+  putIn.run(['.clear']);
+
+  // NUL (\0) is disallowed in filenames in UNIX-like operating systems and
+  // Windows so we can use that to test failed saves.
+  const invalidFileName = tmpdir.resolve('\0\0\0\0\0');
+
+  // Should not break.
+  putIn.write = common.mustCall(function(data) {
+    // Make sure I get a failed to save message and not some other error.
+    assert.strictEqual(data, `Failed to save: ${invalidFileName}\n`);
+    // Reset to no-op.
+    putIn.write = () => {};
+  });
+
+  // Save it to a file.
+  putIn.run([`.save ${invalidFileName}`]);
+
+  {
+    // Save .editor mode code.
+    const cmds = [
+      'function testSave() {',
+      'return "saved";',
+      '}',
+    ];
+    const putIn = new ArrayStream();
+    const replServer = repl.start({ terminal: true, stream: putIn });
+
+    putIn.run(['.editor']);
+    putIn.run(cmds);
+    replServer.write('', { ctrl: true, name: 'd' });
+
+    putIn.run([`.save ${saveFileName}`]);
+    replServer.close();
+    assert.strictEqual(fs.readFileSync(saveFileName, 'utf8'),
+                       `${cmds.join('\n')}\n`);
+  }
+
+  // Check if the file is present when using save
+
+  // Clear the REPL.
+  putIn.run(['.clear']);
+
+  // Error message when using save without a file
+  putIn.write = common.mustCall(function(data) {
+    assert.strictEqual(data, 'The "file" argument must be specified\n');
+    putIn.write = () => {};
+  });
+  putIn.run(['.save']);
+
+  // Check if the file is present when using load
+
+  // Clear the REPL.
+  putIn.run(['.clear']);
+
+  // Error message when using load without a file
+  putIn.write = common.mustCall(function(data) {
+    assert.strictEqual(data, 'The "file" argument must be specified\n');
+    putIn.write = () => {};
+  });
+  putIn.run(['.load']);
+})().then(common.mustCall());