From 949f08a901b14b51d02a221ed49074972d8d06c2 Mon Sep 17 00:00:00 2001 From: islandryu Date: Thu, 3 Jul 2025 23:05:28 +0900 Subject: [PATCH 1/5] repl: catch promise errors during eval in completion Fixes: https://github.com/nodejs/node/issues/58903 --- lib/repl.js | 44 ++++++++- .../repl-tab-completion-nested-repls.js | 2 +- .../test-repl-tab-complete-getter-error.js | 2 +- .../test-repl-tab-complete-promises.js | 97 +++++++++++++++++++ test/parallel/test-repl-tab-complete.js | 2 +- 5 files changed, 141 insertions(+), 6 deletions(-) create mode 100644 test/parallel/test-repl-tab-complete-promises.js diff --git a/lib/repl.js b/lib/repl.js index 443971df63b0e8..49848069063112 100644 --- a/lib/repl.js +++ b/lib/repl.js @@ -43,6 +43,7 @@ 'use strict'; const { + ArrayIsArray, ArrayPrototypeAt, ArrayPrototypeFilter, ArrayPrototypeFindLastIndex, @@ -98,6 +99,7 @@ const { const { isProxy, + isPromise, } = require('internal/util/types'); const { BuiltinModule } = require('internal/bootstrap/realm'); @@ -351,6 +353,7 @@ function REPLServer(prompt, this.allowBlockingCompletions = !!options.allowBlockingCompletions; this.useColors = !!options.useColors; this._domain = options.domain || domain.create(); + this._completeDomain = domain.create(); this.useGlobal = !!useGlobal; this.ignoreUndefined = !!ignoreUndefined; this.replMode = replMode || module.exports.REPL_MODE_SLOPPY; @@ -668,6 +671,8 @@ function REPLServer(prompt, } self.eval = self._domain.bind(eval_); + self.completeEval = self._completeDomain.bind(eval_); + self._completeDomain.on('error', (err) => { }); self._domain.on('error', function debugDomainError(e) { debug('domain error'); @@ -1541,7 +1546,7 @@ function complete(line, callback) { return includesProxiesOrGetters( completeTargetAst.body[0].expression, parsableCompleteTarget, - this.eval, + this.completeEval, this.context, (includes) => { if (includes) { @@ -1558,8 +1563,9 @@ function complete(line, callback) { const memberGroups = []; const evalExpr = `try { ${expr} } catch {}`; - this.eval(evalExpr, this.context, getREPLResourceName(), (e, obj) => { + this.completeEval(evalExpr, this.context, getREPLResourceName(), (e, obj) => { try { + reclusiveCatchPromise(obj); let p; if ((typeof obj === 'object' && obj !== null) || typeof obj === 'function') { @@ -1800,6 +1806,7 @@ function includesProxiesOrGetters(expr, exprStr, evalFn, ctx, callback) { // is the property identifier/literal) if (expr.object.type === 'Identifier') { return evalFn(`try { ${expr.object.name} } catch {}`, ctx, getREPLResourceName(), (err, obj) => { + reclusiveCatchPromise(obj); if (err) { return callback(false); } @@ -1815,6 +1822,7 @@ function includesProxiesOrGetters(expr, exprStr, evalFn, ctx, callback) { return evalFn( `try { ${exprStr} } catch {} `, ctx, getREPLResourceName(), (err, obj) => { + reclusiveCatchPromise(obj); if (err) { return callback(false); } @@ -1877,6 +1885,7 @@ function includesProxiesOrGetters(expr, exprStr, evalFn, ctx, callback) { ctx, getREPLResourceName(), (err, evaledProp) => { + reclusiveCatchPromise(evaledProp); if (err) { return callback(false); } @@ -1902,7 +1911,9 @@ function includesProxiesOrGetters(expr, exprStr, evalFn, ctx, callback) { function safeIsProxyAccess(obj, prop) { // Accessing `prop` may trigger a getter that throws, so we use try-catch to guard against it try { - return isProxy(obj[prop]); + const value = obj[prop]; + reclusiveCatchPromise(value); + return isProxy(value); } catch { return false; } @@ -1911,6 +1922,33 @@ function includesProxiesOrGetters(expr, exprStr, evalFn, ctx, callback) { return callback(false); } +function reclusiveCatchPromise(obj, seen = new SafeWeakSet()) { + if (isPromise(obj)) { + return obj.catch(() => {}); + } else if (ArrayIsArray(obj)) { + obj.forEach((item) => { + reclusiveCatchPromise(item, seen); + }); + } else if (obj && typeof obj === 'object') { + if (seen.has(obj)) return; + seen.add(obj); + + let props; + try { + props = ObjectGetOwnPropertyNames(obj); + } catch { + return; + } + for (const key of props) { + try { + reclusiveCatchPromise(obj[key], seen); + } catch { + continue; + } + } + } +} + REPLServer.prototype.completeOnEditorMode = (callback) => (err, results) => { if (err) return callback(err); diff --git a/test/fixtures/repl-tab-completion-nested-repls.js b/test/fixtures/repl-tab-completion-nested-repls.js index 1d2b154f2b3341..5a0c64d60a8b46 100644 --- a/test/fixtures/repl-tab-completion-nested-repls.js +++ b/test/fixtures/repl-tab-completion-nested-repls.js @@ -32,7 +32,7 @@ const putIn = new ArrayStream(); const testMe = repl.start('', putIn); // Some errors are passed to the domain, but do not callback. -testMe._domain.on('error', function(err) { +testMe._completeDomain.on('error', function(err) { throw err; }); diff --git a/test/parallel/test-repl-tab-complete-getter-error.js b/test/parallel/test-repl-tab-complete-getter-error.js index e2e36b85c58baa..5ea9e4266f1d40 100644 --- a/test/parallel/test-repl-tab-complete-getter-error.js +++ b/test/parallel/test-repl-tab-complete-getter-error.js @@ -21,7 +21,7 @@ async function runTest() { terminal: true }); - replServer._domain.on('error', (e) => { + replServer._completeDomain.on('error', (e) => { assert.fail(`Error in REPL domain: ${e}`); }); diff --git a/test/parallel/test-repl-tab-complete-promises.js b/test/parallel/test-repl-tab-complete-promises.js new file mode 100644 index 00000000000000..b2225f8b03ca90 --- /dev/null +++ b/test/parallel/test-repl-tab-complete-promises.js @@ -0,0 +1,97 @@ +'use strict'; + +const common = require('../common'); +const repl = require('repl'); +const ArrayStream = require('../common/arraystream'); +const assert = require('assert'); + +const completionTests = [ + { send: 'Promise.reject().' }, + { send: 'let p = Promise.reject().' }, + { send: 'Promise.resolve().' }, + { send: 'Promise.resolve().then(() => {}).' }, + { send: `async function f() {throw new Error('test');}; f().` }, + { send: `async function f() {}; f().` }, + { send: 'const foo = { bar: Promise.reject() }; foo.bar.' }, + // Test for that reclusiveCatchPromise does not infinitely recurse + // see lib/repl.js:reclusiveCatchPromise + { send: 'const a = {}; a.self = a; a.self.' }, + { run: `const foo = { get name() { return Promise.reject(); } };`, + send: `foo.name` }, + { run: 'const baz = { get bar() { return ""; } }; const getPropText = () => Promise.reject();', + send: 'baz[getPropText()].', + completeError: true }, + { + send: 'const quux = { bar: { return Promise.reject(); } }; const getPropText = () => "bar"; quux[getPropText()].', + }, +]; + +(async function() { + await runReplCompleteTests(completionTests); +})().then(common.mustCall()); + +async function runReplCompleteTests(tests) { + const input = new ArrayStream(); + const output = new ArrayStream(); + + const replServer = repl.start({ + prompt: '', + input, + output: output, + allowBlockingCompletions: true, + terminal: true + }); + + for (const { send, run, completeError = false } of tests) { + if (run) { + await new Promise((resolve, reject) => { + replServer.eval(run, replServer.context, '', (err) => { + if (err) { + reject(err); + } else { + resolve(); + } + }); + }); + } + + const onError = (e) => { + assert.fail(`Unexpected error: ${e.message}`); + }; + + let completeErrorPromise = Promise.resolve(); + + if (completeError) { + completeErrorPromise = new Promise((resolve) => { + const handleError = () => { + replServer._completeDomain.removeListener('error', handleError); + resolve(); + }; + replServer._completeDomain.on('error', handleError); + }); + } else { + replServer._completeDomain.on('error', onError); + } + + await replServer.complete( + send, + common.mustCall((error, data) => { + assert.strictEqual(error, null); + assert.strictEqual(data.length, 2); + assert.strictEqual(typeof data[1], 'string'); + assert.ok(send.includes(data[1])); + }) + ); + + await completeErrorPromise; + + if (!completeError) { + await new Promise((resolve) => { + setImmediate(() => { + replServer._completeDomain.removeListener('error', onError); + resolve(); + }); + }); + } + } +} diff --git a/test/parallel/test-repl-tab-complete.js b/test/parallel/test-repl-tab-complete.js index f37916e30d8411..d77067cb8f523f 100644 --- a/test/parallel/test-repl-tab-complete.js +++ b/test/parallel/test-repl-tab-complete.js @@ -44,7 +44,7 @@ function prepareREPL() { }); // Some errors are passed to the domain, but do not callback - replServer._domain.on('error', assert.ifError); + replServer._completeDomain.on('error', assert.ifError); return { replServer, input }; } From 0e44668d51d37203d04a20d38ed94112d10d9abe Mon Sep 17 00:00:00 2001 From: islandryu Date: Tue, 8 Jul 2025 22:57:15 +0900 Subject: [PATCH 2/5] test: add repl test for promise eval --- test/parallel/test-repl-eval-promises.js | 62 ++++++++++++++++++++++++ 1 file changed, 62 insertions(+) create mode 100644 test/parallel/test-repl-eval-promises.js diff --git a/test/parallel/test-repl-eval-promises.js b/test/parallel/test-repl-eval-promises.js new file mode 100644 index 00000000000000..9e5fffda128a47 --- /dev/null +++ b/test/parallel/test-repl-eval-promises.js @@ -0,0 +1,62 @@ +'use strict'; + +const common = require('../common'); +const repl = require('repl'); +const ArrayStream = require('../common/arraystream'); +const assert = require('assert'); + +const tests = [ + { + send: 'Promise.reject()', + expect: /Promise \{[\s\S]*?Uncaught undefined\n?$/ + }, + { + send: 'let p = Promise.reject()', + expect: /undefined\nUncaught undefined\n?$/ + }, + { + send: `Promise.resolve()`, + expect: /Promise \{[\s\S]*?}\n?$/ + }, + { + send: `Promise.resolve().then(() => {})`, + expect: /Promise \{[\s\S]*?}\n?$/ + }, + { + send: `async function f() { throw new Error('test'); };f();`, + expect: /Promise \{[\s\S]*? Error: test[\s\S]*?Uncaught Error: test[\s\S]*?\n?$/ + }, + { + send: `async function f() {};f();`, + expect: /Promise \{[\s\S]*?}\n?$/ + }, +]; + +(async function() { + await runReplTests(tests); +})().then(common.mustCall()); + +async function runReplTests(tests) { + for (const { send, expect } of tests) { + const input = new ArrayStream(); + const output = new ArrayStream(); + let outputText = ''; + function write(data) { + outputText += data; + } + output.write = write; + const replServer = repl.start({ + prompt: '', + input, + output: output, + }); + input.emit('data', `${send}\n`); + await new Promise((resolve) => { + setTimeout(() => { + assert.match(outputText, expect); + replServer.close(); + resolve(); + }, 10); + }); + } +} From 5f15beff05c863e51282462a31316da4fb702c5d Mon Sep 17 00:00:00 2001 From: islandryu Date: Mon, 21 Jul 2025 09:33:04 +0900 Subject: [PATCH 3/5] use mustCall for domain error --- test/parallel/test-repl-tab-complete-promises.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/parallel/test-repl-tab-complete-promises.js b/test/parallel/test-repl-tab-complete-promises.js index b2225f8b03ca90..ff5cbcdec028a4 100644 --- a/test/parallel/test-repl-tab-complete-promises.js +++ b/test/parallel/test-repl-tab-complete-promises.js @@ -67,7 +67,7 @@ async function runReplCompleteTests(tests) { replServer._completeDomain.removeListener('error', handleError); resolve(); }; - replServer._completeDomain.on('error', handleError); + replServer._completeDomain.on('error', common.mustCall(handleError)); }); } else { replServer._completeDomain.on('error', onError); From 86b5a9bde2d053f0830af8f98de376f14d72f66b Mon Sep 17 00:00:00 2001 From: islandryu Date: Mon, 21 Jul 2025 09:37:22 +0900 Subject: [PATCH 4/5] assert no error occurs in _domain --- test/parallel/test-repl-tab-complete-promises.js | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/test/parallel/test-repl-tab-complete-promises.js b/test/parallel/test-repl-tab-complete-promises.js index ff5cbcdec028a4..b68aa338dffa27 100644 --- a/test/parallel/test-repl-tab-complete-promises.js +++ b/test/parallel/test-repl-tab-complete-promises.js @@ -42,6 +42,10 @@ async function runReplCompleteTests(tests) { terminal: true }); + replServer._domain.on('error', (err) => { + assert.fail(`Unexpected domain error: ${err.message}`); + }); + for (const { send, run, completeError = false } of tests) { if (run) { await new Promise((resolve, reject) => { From 422d0e3dd360cb437e4b049bb8d809777ae3a2ab Mon Sep 17 00:00:00 2001 From: islandryu Date: Thu, 31 Jul 2025 23:47:43 +0900 Subject: [PATCH 5/5] unuse completeDomain and disable completion on call expression --- lib/repl.js | 24 +++++---- .../repl-tab-completion-nested-repls.js | 2 +- .../test-completion-on-function-disabled.js | 50 +++++++++++++++++++ ...est-repl-completion-on-getters-disabled.js | 1 - .../test-repl-tab-complete-getter-error.js | 2 +- .../test-repl-tab-complete-promises.js | 31 ++---------- test/parallel/test-repl-tab-complete.js | 2 +- 7 files changed, 69 insertions(+), 43 deletions(-) create mode 100644 test/parallel/test-completion-on-function-disabled.js diff --git a/lib/repl.js b/lib/repl.js index 49848069063112..8abb2a2d550151 100644 --- a/lib/repl.js +++ b/lib/repl.js @@ -353,7 +353,6 @@ function REPLServer(prompt, this.allowBlockingCompletions = !!options.allowBlockingCompletions; this.useColors = !!options.useColors; this._domain = options.domain || domain.create(); - this._completeDomain = domain.create(); this.useGlobal = !!useGlobal; this.ignoreUndefined = !!ignoreUndefined; this.replMode = replMode || module.exports.REPL_MODE_SLOPPY; @@ -671,8 +670,6 @@ function REPLServer(prompt, } self.eval = self._domain.bind(eval_); - self.completeEval = self._completeDomain.bind(eval_); - self._completeDomain.on('error', (err) => { }); self._domain.on('error', function debugDomainError(e) { debug('domain error'); @@ -1543,10 +1540,10 @@ function complete(line, callback) { return completionGroupsLoaded(); } - return includesProxiesOrGetters( + return potentiallySideEffectfulAccess( completeTargetAst.body[0].expression, parsableCompleteTarget, - this.completeEval, + this.eval, this.context, (includes) => { if (includes) { @@ -1563,7 +1560,7 @@ function complete(line, callback) { const memberGroups = []; const evalExpr = `try { ${expr} } catch {}`; - this.completeEval(evalExpr, this.context, getREPLResourceName(), (e, obj) => { + this.eval(evalExpr, this.context, getREPLResourceName(), (e, obj) => { try { reclusiveCatchPromise(obj); let p; @@ -1756,10 +1753,11 @@ function findExpressionCompleteTarget(code) { } /** - * Utility used to determine if an expression includes object getters or proxies. + * Utility to determine if accessing a given expression could have side effects. * - * Example: given `obj.foo`, the function lets you know if `foo` has a getter function - * associated to it, or if `obj` is a proxy + * Example: given `obj.foo`, this function checks if accessing `foo` may trigger a getter, + * or if any part of the chain is a Proxy, or if evaluating the property could cause side effects. + * This is used to avoid triggering user code or side effects during tab completion. * @param {any} expr The expression, in AST format to analyze * @param {string} exprStr The string representation of the expression * @param {(str: string, ctx: any, resourceName: string, cb: (error, evaled) => void) => void} evalFn @@ -1768,15 +1766,19 @@ function findExpressionCompleteTarget(code) { * @param {(includes: boolean) => void} callback Callback that will be called with the result of the operation * @returns {void} */ -function includesProxiesOrGetters(expr, exprStr, evalFn, ctx, callback) { +function potentiallySideEffectfulAccess(expr, exprStr, evalFn, ctx, callback) { if (expr?.type !== 'MemberExpression') { // If the expression is not a member one for obvious reasons no getters are involved return callback(false); } + if (expr.object.type === 'CallExpression' || expr.property.type === 'CallExpression') { + return callback(true); + } + if (expr.object.type === 'MemberExpression') { // The object itself is a member expression, so we need to recurse (e.g. the expression is `obj.foo.bar`) - return includesProxiesOrGetters( + return potentiallySideEffectfulAccess( expr.object, exprStr.slice(0, expr.object.end), evalFn, diff --git a/test/fixtures/repl-tab-completion-nested-repls.js b/test/fixtures/repl-tab-completion-nested-repls.js index 5a0c64d60a8b46..1d2b154f2b3341 100644 --- a/test/fixtures/repl-tab-completion-nested-repls.js +++ b/test/fixtures/repl-tab-completion-nested-repls.js @@ -32,7 +32,7 @@ const putIn = new ArrayStream(); const testMe = repl.start('', putIn); // Some errors are passed to the domain, but do not callback. -testMe._completeDomain.on('error', function(err) { +testMe._domain.on('error', function(err) { throw err; }); diff --git a/test/parallel/test-completion-on-function-disabled.js b/test/parallel/test-completion-on-function-disabled.js new file mode 100644 index 00000000000000..e1324be6063513 --- /dev/null +++ b/test/parallel/test-completion-on-function-disabled.js @@ -0,0 +1,50 @@ +'use strict'; + +const common = require('../common'); +const assert = require('node:assert'); +const { 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`) + ); + } + })); + }); +} + +test('REPL completion on function disabled', () => { + runCompletionTests(` + function foo() { return { a: { b: 5 } } } + const obj = { a: 5 } + const getKey = () => 'a'; + `, [ + ['foo().', []], + ['foo().a', []], + ['foo().a.', []], + ['foo()["a"]', []], + ['foo()["a"].', []], + ['foo()["a"].b', []], + ['obj[getKey()].', []], + ]); + +}); diff --git a/test/parallel/test-repl-completion-on-getters-disabled.js b/test/parallel/test-repl-completion-on-getters-disabled.js index 05cdd630d5a298..3a42de4307f2f4 100644 --- a/test/parallel/test-repl-completion-on-getters-disabled.js +++ b/test/parallel/test-repl-completion-on-getters-disabled.js @@ -90,7 +90,6 @@ describe('REPL completion in relation of getters', () => { ["objWithGetters[keys['foo key']].b", ["objWithGetters[keys['foo key']].bar"]], ['objWithGetters[fooKey].b', ['objWithGetters[fooKey].bar']], ["objWithGetters['f' + 'oo'].b", ["objWithGetters['f' + 'oo'].bar"]], - ['objWithGetters[getFooKey()].b', ['objWithGetters[getFooKey()].bar']], ]); }); diff --git a/test/parallel/test-repl-tab-complete-getter-error.js b/test/parallel/test-repl-tab-complete-getter-error.js index 5ea9e4266f1d40..e2e36b85c58baa 100644 --- a/test/parallel/test-repl-tab-complete-getter-error.js +++ b/test/parallel/test-repl-tab-complete-getter-error.js @@ -21,7 +21,7 @@ async function runTest() { terminal: true }); - replServer._completeDomain.on('error', (e) => { + replServer._domain.on('error', (e) => { assert.fail(`Error in REPL domain: ${e}`); }); diff --git a/test/parallel/test-repl-tab-complete-promises.js b/test/parallel/test-repl-tab-complete-promises.js index b68aa338dffa27..5051541d18daf5 100644 --- a/test/parallel/test-repl-tab-complete-promises.js +++ b/test/parallel/test-repl-tab-complete-promises.js @@ -19,8 +19,7 @@ const completionTests = [ { run: `const foo = { get name() { return Promise.reject(); } };`, send: `foo.name` }, { run: 'const baz = { get bar() { return ""; } }; const getPropText = () => Promise.reject();', - send: 'baz[getPropText()].', - completeError: true }, + send: 'baz[getPropText()].' }, { send: 'const quux = { bar: { return Promise.reject(); } }; const getPropText = () => "bar"; quux[getPropText()].', }, @@ -46,7 +45,7 @@ async function runReplCompleteTests(tests) { assert.fail(`Unexpected domain error: ${err.message}`); }); - for (const { send, run, completeError = false } of tests) { + for (const { send, run } of tests) { if (run) { await new Promise((resolve, reject) => { replServer.eval(run, replServer.context, '', (err) => { @@ -59,23 +58,7 @@ async function runReplCompleteTests(tests) { }); } - const onError = (e) => { - assert.fail(`Unexpected error: ${e.message}`); - }; - - let completeErrorPromise = Promise.resolve(); - - if (completeError) { - completeErrorPromise = new Promise((resolve) => { - const handleError = () => { - replServer._completeDomain.removeListener('error', handleError); - resolve(); - }; - replServer._completeDomain.on('error', common.mustCall(handleError)); - }); - } else { - replServer._completeDomain.on('error', onError); - } + const completeErrorPromise = Promise.resolve(); await replServer.complete( send, @@ -89,13 +72,5 @@ async function runReplCompleteTests(tests) { await completeErrorPromise; - if (!completeError) { - await new Promise((resolve) => { - setImmediate(() => { - replServer._completeDomain.removeListener('error', onError); - resolve(); - }); - }); - } } } diff --git a/test/parallel/test-repl-tab-complete.js b/test/parallel/test-repl-tab-complete.js index d77067cb8f523f..f37916e30d8411 100644 --- a/test/parallel/test-repl-tab-complete.js +++ b/test/parallel/test-repl-tab-complete.js @@ -44,7 +44,7 @@ function prepareREPL() { }); // Some errors are passed to the domain, but do not callback - replServer._completeDomain.on('error', assert.ifError); + replServer._domain.on('error', assert.ifError); return { replServer, input }; }