From baf01a255b4a27ab7a0c4179f238baee10642439 Mon Sep 17 00:00:00 2001 From: Brian Botha Date: Wed, 5 Jun 2024 16:59:25 +1000 Subject: [PATCH 1/3] fix: fixed deadlock error when pipe ends while prompting password [ci skip] --- src/utils/processors.ts | 55 +++++++++++++++++++++++++++-------------- 1 file changed, 37 insertions(+), 18 deletions(-) diff --git a/src/utils/processors.ts b/src/utils/processors.ts index da11fe36..1f1e1817 100644 --- a/src/utils/processors.ts +++ b/src/utils/processors.ts @@ -17,17 +17,28 @@ import { arrayZip } from 'polykey/dist/utils'; import config from 'polykey/dist/config'; import * as errors from '../errors'; +// A one time promise that resolves when `stdin` closes +const stdinEndP = new Promise((resolve) => { + if (!process.stdin.readable) return resolve(undefined); + process.stdin.once('close', () => resolve(undefined)); +}); + /** * Prompts for existing password * This masks SIGINT handling * When SIGINT is received this will return undefined */ async function promptPassword(): Promise { - const { password } = await prompts({ - name: 'password', - type: 'password', - message: 'Please enter the password', - }); + const { password } = await Promise.race([ + prompts({ + name: 'password', + type: 'password', + message: 'Please enter the password', + }), + stdinEndP.then(() => { + return { password: undefined }; + }), + ]); return password; } @@ -37,28 +48,36 @@ async function promptPassword(): Promise { * When SIGINT is received this will return undefined */ async function promptNewPassword(): Promise { - let password: string | undefined; + // Convert the output to a similar structure to the `prompts` output + const streamEndP = stdinEndP.then(() => { + return { password: undefined }; + }); while (true) { - ({ password } = await prompts({ - name: 'password', - type: 'password', - message: 'Enter new password', - })); + const { password } = await Promise.race([ + prompts({ + name: 'password', + type: 'password', + message: 'Enter new password', + }), + streamEndP, + ]); // If undefined, then SIGINT was sent, return undefined if (password == null) return; - const { passwordConfirm } = await prompts({ - name: 'passwordConfirm', - type: 'password', - message: 'Confirm new password', - }); + const { passwordConfirm } = await Promise.race([ + prompts({ + name: 'passwordConfirm', + type: 'password', + message: 'Confirm new password', + }), + streamEndP, + ]); // If undefined, then SIGINT was sent, return undefined if (passwordConfirm == null) return; // Compare the passwords are the same - if (password === passwordConfirm) break; + if (password === passwordConfirm) return password; // Interactive message process.stderr.write('Passwords do not match!\n'); } - return password; } /** From 4df1f83bfbac054885c2cfa7fdb89e1d55476ca1 Mon Sep 17 00:00:00 2001 From: Brian Botha Date: Fri, 7 Jun 2024 14:00:56 +1000 Subject: [PATCH 2/3] fix: simplified prompts and added check for handling piped input by failing to get password [ci skip] --- src/utils/processors.ts | 78 +++++++++++++++++++++-------------------- 1 file changed, 40 insertions(+), 38 deletions(-) diff --git a/src/utils/processors.ts b/src/utils/processors.ts index 1f1e1817..aff19484 100644 --- a/src/utils/processors.ts +++ b/src/utils/processors.ts @@ -17,28 +17,29 @@ import { arrayZip } from 'polykey/dist/utils'; import config from 'polykey/dist/config'; import * as errors from '../errors'; -// A one time promise that resolves when `stdin` closes -const stdinEndP = new Promise((resolve) => { - if (!process.stdin.readable) return resolve(undefined); - process.stdin.once('close', () => resolve(undefined)); -}); - /** * Prompts for existing password * This masks SIGINT handling * When SIGINT is received this will return undefined */ async function promptPassword(): Promise { - const { password } = await Promise.race([ - prompts({ + // If `isTTY` is `undefined` then stdin has been piped into and `prompts` will not process it properly + if (process.stdin.setRawMode == null) return; + let cancelled = false; + const { password } = await prompts( + { name: 'password', type: 'password', message: 'Please enter the password', - }), - stdinEndP.then(() => { - return { password: undefined }; - }), - ]); + }, + { + onCancel: () => { + cancelled = true; + }, + }, + ); + // If cancelled then we just return undefined + if (cancelled) return; return password; } @@ -48,32 +49,33 @@ async function promptPassword(): Promise { * When SIGINT is received this will return undefined */ async function promptNewPassword(): Promise { - // Convert the output to a similar structure to the `prompts` output - const streamEndP = stdinEndP.then(() => { - return { password: undefined }; - }); + // If `isTTY` is `undefined` then stdin has been piped into and `prompts` will not process it properly + if (process.stdin.setRawMode == null) return; while (true) { - const { password } = await Promise.race([ - prompts({ - name: 'password', - type: 'password', - message: 'Enter new password', - }), - streamEndP, - ]); - // If undefined, then SIGINT was sent, return undefined - if (password == null) return; - const { passwordConfirm } = await Promise.race([ - prompts({ - name: 'passwordConfirm', - type: 'password', - message: 'Confirm new password', - }), - streamEndP, - ]); - // If undefined, then SIGINT was sent, return undefined - if (passwordConfirm == null) return; - // Compare the passwords are the same + let cancelled = false; + const { password, passwordConfirm } = await prompts( + [ + { + name: 'password', + type: 'password', + message: 'Enter new password', + }, + { + name: 'passwordConfirm', + type: 'password', + message: 'Confirm new password', + }, + ], + { + onCancel: () => { + cancelled = true; + }, + }, + ); + console.log('passwords', password, passwordConfirm); + // If cancelled then we just return undefined + if (cancelled) return; + // Confirm that the passwords are the same if (password === passwordConfirm) return password; // Interactive message process.stderr.write('Passwords do not match!\n'); From 62c43a79d1dbb816d4043290ee547eefbd288e8c Mon Sep 17 00:00:00 2001 From: Brian Botha Date: Wed, 12 Jun 2024 14:01:30 +1000 Subject: [PATCH 3/3] tests: removed tests using nexpect to check password prompting behaviour nexpect uses a pipe to function it seems. It is incompatible with the fix. It's weird that it worked before but. [ci skip] --- src/utils/processors.ts | 1 - tests/agent/start.test.ts | 106 -------------------------------------- tests/bootstrap.test.ts | 26 ---------- 3 files changed, 133 deletions(-) diff --git a/src/utils/processors.ts b/src/utils/processors.ts index aff19484..0efc65d3 100644 --- a/src/utils/processors.ts +++ b/src/utils/processors.ts @@ -72,7 +72,6 @@ async function promptNewPassword(): Promise { }, }, ); - console.log('passwords', password, passwordConfirm); // If cancelled then we just return undefined if (cancelled) return; // Confirm that the passwords are the same diff --git a/tests/agent/start.test.ts b/tests/agent/start.test.ts index 3151c6ac..732ee776 100644 --- a/tests/agent/start.test.ts +++ b/tests/agent/start.test.ts @@ -1036,110 +1036,4 @@ describe('start', () => { globalThis.defaultTimeout * 2, ); }); - test('prompts for password twice when creating state', async () => { - const polykeyPath = path.join(dataDir, 'polykey'); - await fs.promises.mkdir(polykeyPath); - - // Starting with missing directory prompts a new password - await testUtils.pkExpect({ - args: [ - 'agent', - 'start', - '--node-path', - path.join(dataDir, 'polykey'), - '--client-host', - '127.0.0.1', - '--agent-host', - '127.0.0.1', - '--workers', - 'none', - '--seed-nodes', - '', - '--verbose', - '--format', - 'json', - '--password-ops-limit', - 'min', - '--password-mem-limit', - 'min', - ], - expect: (expectChain) => { - expectChain.expect(/Enter new password/); - expectChain.sendline('password'); - expectChain.wait(/Confirm new password/); - expectChain.sendEof(); - return expectChain; - }, - }); - }); - test( - 'prompts for password once with existing state', - async () => { - const password = 'abc123'; - const agentProcess1 = await testUtils.pkSpawn( - [ - 'agent', - 'start', - '--client-host', - '127.0.0.1', - '--agent-host', - '127.0.0.1', - '--workers', - 'none', - '--seed-nodes', - '', - '--verbose', - ], - { - env: { - PK_NODE_PATH: path.join(dataDir, 'polykey'), - PK_PASSWORD: password, - PK_PASSWORD_OPS_LIMIT: 'min', - PK_PASSWORD_MEM_LIMIT: 'min', - }, - cwd: dataDir, - }, - logger, - ); - const rlOut = readline.createInterface(agentProcess1.stdout!); - await new Promise((resolve, reject) => { - rlOut.once('line', resolve); - rlOut.once('close', () => reject(Error('closed early'))); - }); - agentProcess1.kill('SIGHUP'); - - // Starting again on existing state - await testUtils.pkExpect({ - args: [ - 'agent', - 'start', - '--node-path', - path.join(dataDir, 'polykey'), - '--client-host', - '127.0.0.1', - '--agent-host', - '127.0.0.1', - '--workers', - 'none', - '--seed-nodes', - '', - '--verbose', - '--format', - 'json', - '--password-ops-limit', - 'min', - '--password-mem-limit', - 'min', - ], - expect: (expectChain) => { - expectChain.expect(/Please enter the password/); - expectChain.sendline('password'); - expectChain.wait(/Creating PolykeyAgent/); - expectChain.sendEof(); - return expectChain; - }, - }); - }, - globalThis.defaultTimeout * 2, - ); }); diff --git a/tests/bootstrap.test.ts b/tests/bootstrap.test.ts index 6a2e53b6..fd550019 100644 --- a/tests/bootstrap.test.ts +++ b/tests/bootstrap.test.ts @@ -1,4 +1,3 @@ -import type { IChain } from 'nexpect'; import path from 'path'; import fs from 'fs'; import readline from 'readline'; @@ -303,29 +302,4 @@ describe('bootstrap', () => { }, globalThis.defaultTimeout * 2, ); - test( - 'bootstraps node state prompts for password twice ', - async () => { - const password = 'password'; - const passwordPath = path.join(dataDir, 'password'); - await fs.promises.writeFile(passwordPath, password); - await testUtils.pkExpect({ - args: ['bootstrap', '--verbose'], - env: { - PK_NODE_PATH: path.join(dataDir, 'polykey'), - PK_PASSWORD_OPS_LIMIT: 'min', - PK_PASSWORD_MEM_LIMIT: 'min', - }, - cwd: dataDir, - expect: (expectChain: IChain) => { - expectChain.expect(/Enter new password/); - expectChain.sendline('password'); - expectChain.wait(/Confirm new password/); - expectChain.sendEof(); - return expectChain; - }, - }); - }, - globalThis.defaultTimeout * 2, - ); });