diff --git a/src/errors.ts b/src/errors.ts index f147e8fd..40314093 100644 --- a/src/errors.ts +++ b/src/errors.ts @@ -84,6 +84,11 @@ class ErrorPolykeyCLIUnexpectedError extends ErrorPolykeyCLI { exitCode = sysexits.SOFTWARE; } +class ErrorPolykeyCLISubprocessFailure extends ErrorPolykeyCLI { + static description = 'A subprocess failed to exit gracefully'; + exitCode = sysexits.UNKNOWN; +} + class ErrorPolykeyCLINodePath extends ErrorPolykeyCLI { static description = 'Cannot derive default node path from unknown platform'; exitCode = sysexits.USAGE; @@ -191,6 +196,7 @@ export { ErrorPolykeyCLIUncaughtException, ErrorPolykeyCLIUnhandledRejection, ErrorPolykeyCLIUnexpectedError, + ErrorPolykeyCLISubprocessFailure, ErrorPolykeyCLIAsynchronousDeadlock, ErrorPolykeyCLINodePath, ErrorPolykeyCLIClientOptions, diff --git a/src/secrets/CommandEdit.ts b/src/secrets/CommandEdit.ts index afdfe442..672b9ced 100644 --- a/src/secrets/CommandEdit.ts +++ b/src/secrets/CommandEdit.ts @@ -22,7 +22,9 @@ class CommandEdit extends CommandPolykey { this.addOption(binOptions.nodeId); this.addOption(binOptions.clientHost); this.addOption(binOptions.clientPort); - this.action(async (secretPath, options) => { + this.action(async (fullSecretPath, options) => { + const vaultName = fullSecretPath[0]; + const secretPath = fullSecretPath[1] ?? '/'; const os = await import('os'); const { spawn } = await import('child_process'); const vaultsErrors = await import('polykey/dist/vaults/errors'); @@ -60,13 +62,13 @@ class CommandEdit extends CommandPolykey { }, logger: this.logger.getChild(PolykeyClient.name), }); - const tmpFile = path.join(tmpDir, path.basename(secretPath[1])); + const tmpFile = path.join(tmpDir, path.basename(secretPath)); const secretExists = await binUtils.retryAuthentication( async (auth) => { let exists = true; const response = await pkClient.rpcClient.methods.vaultsSecretsGet({ - nameOrId: secretPath[0], - secretName: secretPath[1] ?? '/', + nameOrId: vaultName, + secretName: secretPath, metadata: auth, }); try { @@ -86,7 +88,7 @@ class CommandEdit extends CommandPolykey { // First, write the inline error to standard error like other // secrets commands do. process.stderr.write( - `edit: ${secretPath[1] ?? '/'}: No such file or directory\n`, + `edit: ${secretPath}: No such file or directory\n`, ); // Then, throw an error to get the non-zero exit code. As this // command is Polykey-specific, the code doesn't really matter @@ -111,22 +113,33 @@ class CommandEdit extends CommandPolykey { const editorProc = spawn(process.env.EDITOR ?? 'nano', [tmpFile], { stdio: 'inherit', }); - editorProc.on('error', (e) => { - const error = new errors.ErrorPolykeyCLIEditSecret( - `Failed to run command ${process.env.EDITOR}`, + // Define event handlers + const cleanup = () => { + editorProc.removeListener('error', onError); + editorProc.removeListener('close', onClose); + }; + const onError = (e: Error) => { + cleanup(); + const error = new errors.ErrorPolykeyCLISubprocessFailure( + `Failed to run command '${process.env.EDITOR}'`, { cause: e }, ); reject(error); - }); - editorProc.on('close', (code) => { + }; + const onClose = (code: number | null) => { + cleanup(); if (code !== 0) { - const error = new errors.ErrorPolykeyCLIEditSecret( + const error = new errors.ErrorPolykeyCLISubprocessFailure( `Editor exited with code ${code}`, ); reject(error); + } else { + resolve(); } - resolve(); - }); + }; + // Connect event handlers to events + editorProc.on('error', onError); + editorProc.on('close', onClose); }); let content: string; try { @@ -160,8 +173,8 @@ class CommandEdit extends CommandPolykey { async (auth) => await pkClient.rpcClient.methods.vaultsSecretsWriteFile({ metadata: auth, - nameOrId: secretPath[0], - secretName: secretPath[1], + nameOrId: vaultName, + secretName: secretPath, secretContent: content, }), meta, diff --git a/src/secrets/CommandEnv.ts b/src/secrets/CommandEnv.ts index 3550aa23..47abc497 100644 --- a/src/secrets/CommandEnv.ts +++ b/src/secrets/CommandEnv.ts @@ -1,11 +1,12 @@ import type PolykeyClient from 'polykey/dist/PolykeyClient'; +import type { ParsedSecretPathValue } from '../types'; import path from 'path'; import os from 'os'; import * as utils from 'polykey/dist/utils'; +import CommandPolykey from '../CommandPolykey'; import * as binProcessors from '../utils/processors'; import * as binUtils from '../utils'; import * as binErrors from '../errors'; -import CommandPolykey from '../CommandPolykey'; import * as binOptions from '../utils/options'; import * as binParsers from '../utils/parsers'; @@ -14,7 +15,7 @@ class CommandEnv extends CommandPolykey { super(...args); this.name('env'); this.description( - `Run a command with the given secrets and env variables using process replacement. If no command is specified then the variables are printed to stdout in the format specified by env-format.`, + `Run a command with the given secrets and env variables. If no command is specified then the variables are printed to stdout in the format specified by env-format.`, ); this.addOption(binOptions.nodeId); this.addOption(binOptions.clientHost); @@ -22,18 +23,14 @@ class CommandEnv extends CommandPolykey { this.addOption(binOptions.envFormat); this.addOption(binOptions.envInvalid); this.addOption(binOptions.envDuplicate); + this.addOption(binOptions.preserveNewline); this.argument( '', 'command and arguments formatted as [envPaths...][-- cmd [cmdArgs...]]', binParsers.parseEnvArgs, ); - this.passThroughOptions(); // Let -- pass through as-is to parse as delimiter for cmd this.action( - async ( - args: [Array<[string, string?, string?]>, Array], - options, - ) => { - args[1].shift(); + async (args: [Array, Array], options) => { const { default: PolykeyClient } = await import( 'polykey/dist/PolykeyClient' ); @@ -160,7 +157,27 @@ class CommandEnv extends CommandPolykey { utils.never(); } } - envp[newName] = secretContent; + + // Find if we need to preserve the newline for this secret + let preserveNewline = false; + if (options.preserveNewline) { + for (const pair of options.preserveNewline) { + if ( + pair[0] === nameOrId && + (pair[1] === newName || pair[1] == null) + ) { + preserveNewline = true; + break; + } + } + } + + // Trim the single trailing newline if it exists + if (!preserveNewline && secretContent.endsWith('\n')) { + envp[newName] = secretContent.slice(0, -1); + } else { + envp[newName] = secretContent; + } envpPath[newName] = { nameOrId, secretName, @@ -175,14 +192,26 @@ class CommandEnv extends CommandPolykey { // Here we want to switch between the different usages const platform = os.platform(); if (cmd != null) { - // If a cmd is| provided then we default to exec it + // If a cmd is provided then we default to exec it switch (platform) { case 'linux': - // Fallthrough case 'darwin': { const { exec } = await import('@matrixai/exec'); - exec.execvp(cmd, argv, envp); + try { + exec.execvp(cmd, argv, envp); + } catch (e) { + if ('code' in e && e.code === 'GenericFailure') { + throw new binErrors.ErrorPolykeyCLISubprocessFailure( + `Command failed with error ${e}`, + { + cause: e, + data: { command: [cmd, ...argv] }, + }, + ); + } + throw e; + } } break; default: { diff --git a/src/types.ts b/src/types.ts index b8bccd1f..ab9c8ee7 100644 --- a/src/types.ts +++ b/src/types.ts @@ -61,6 +61,8 @@ type PromiseDeconstructed = { rejectP: (reason?: any) => void; }; +type ParsedSecretPathValue = [string, string?, string?]; + export type { TableRow, TableOptions, @@ -69,4 +71,5 @@ export type { AgentChildProcessInput, AgentChildProcessOutput, PromiseDeconstructed, + ParsedSecretPathValue, }; diff --git a/src/utils/options.ts b/src/utils/options.ts index ba831e7c..d50e7330 100644 --- a/src/utils/options.ts +++ b/src/utils/options.ts @@ -318,6 +318,17 @@ const parents = new commander.Option( 'If enabled, create all parent directories as well. If the directories exist, do nothing.', ).default(false); +const preserveNewline = new commander.Option( + '-pn --preserve-newline ', + 'Preserve the last trailing newline for the secret content', +) + .argParser((value: string, previous: Array<[string, string?, string?]>) => { + const out = previous ?? []; + out.push(binParsers.parseSecretPathEnv(value)); + return out; + }) + .default([]); + export { nodePath, format, @@ -361,4 +372,5 @@ export { order, recursive, parents, + preserveNewline, }; diff --git a/src/utils/parsers.ts b/src/utils/parsers.ts index eef2dd50..16de93ce 100644 --- a/src/utils/parsers.ts +++ b/src/utils/parsers.ts @@ -1,5 +1,6 @@ import type { Host, Hostname, Port } from 'polykey/dist/network/types'; import type { SeedNodes } from 'polykey/dist/nodes/types'; +import type { ParsedSecretPathValue } from '../types'; import commander from 'commander'; import * as validationUtils from 'polykey/dist/validation/utils'; import * as validationErrors from 'polykey/dist/validation/errors'; @@ -81,7 +82,7 @@ function parseVaultName(vaultName: string): string { // If 'vault1:', an error is thrown // If 'a/b/c', an error is thrown // Splits out everything after an `=` separator -function parseSecretPath(inputPath: string): [string, string?, string?] { +function parseSecretPath(inputPath: string): ParsedSecretPathValue { // The colon character `:` is prohibited in vaultName, so it's first occurence // means that this is the delimiter between vaultName and secretPath. const colonIndex = inputPath.indexOf(':'); @@ -116,7 +117,7 @@ function parseSecretPath(inputPath: string): [string, string?, string?] { return [vaultName, secretPath, value]; } -function parseSecretPathValue(secretPath: string): [string, string?, string?] { +function parseSecretPathValue(secretPath: string): ParsedSecretPathValue { const [vaultName, directoryPath, value] = parseSecretPath(secretPath); if (value != null && !secretPathValueRegex.test(value)) { throw new commander.InvalidArgumentError( @@ -126,7 +127,7 @@ function parseSecretPathValue(secretPath: string): [string, string?, string?] { return [vaultName, directoryPath, value]; } -function parseSecretPathEnv(secretPath: string): [string, string?, string?] { +function parseSecretPathEnv(secretPath: string): ParsedSecretPathValue { const [vaultName, directoryPath, value] = parseSecretPath(secretPath); if (value != null && !environmentVariableRegex.test(value)) { throw new commander.InvalidArgumentError( @@ -202,30 +203,29 @@ const parseSeedNodes: (data: string) => [SeedNodes, boolean] = */ function parseEnvArgs( value: string, - prev: [Array<[string, string?, string?]>, Array] | undefined, -): [Array<[string, string?, string?]>, Array] { - const current: [Array<[string, string?, string?]>, Array] = prev ?? [ - [], - [], - ]; - if (current[1].length === 0) { + prev: [Array, Array, boolean] | undefined, +): [Array, Array, boolean] { + const current: [Array, Array, boolean] = + prev ?? [[], [], false]; + const [secretsList, commandList, parsingCommandCurrent] = current; + let parsingCommand = parsingCommandCurrent; + if (!parsingCommand) { // Parse a secret path if (value !== '--') { - current[0].push(parseSecretPathEnv(value)); + secretsList.push(parseSecretPathEnv(value)); } else { - current[1].push(value); - return current; + parsingCommand = true; } } else { // Otherwise we just have the cmd args - current[1].push(value); + commandList.push(value); } - if (current[0].length === 0 && current[1].length > 0) { + if (secretsList.length === 0 && commandList.length > 0) { throw new commander.InvalidArgumentError( 'You must provide at least 1 secret path', ); } - return current; + return [secretsList, commandList, parsingCommand]; } export { diff --git a/src/utils/utils.ts b/src/utils/utils.ts index 358b0bc4..c97de0f4 100644 --- a/src/utils/utils.ts +++ b/src/utils/utils.ts @@ -448,7 +448,11 @@ function outputFormatterError(err: any): string { } output += `${indent}timestamp\t${err.timestamp}\n`; } else { - output += '\n'; + if (err.data && !utils.isEmptyObject(err.data)) { + output += `\n${indent}data: ${JSON.stringify(err.data)}\n`; + } else { + output += '\n'; + } } output += `${indent}cause: `; err = err.cause; diff --git a/tests/secrets/env.test.ts b/tests/secrets/env.test.ts index f84b958f..ee848d78 100644 --- a/tests/secrets/env.test.ts +++ b/tests/secrets/env.test.ts @@ -1,4 +1,5 @@ import type { VaultName } from 'polykey/dist/vaults/types'; +import type { ParsedSecretPathValue } from '@/types'; import path from 'path'; import fs from 'fs'; import fc from 'fast-check'; @@ -18,6 +19,8 @@ describe('commandEnv', () => { let dataDir: string; let polykeyAgent: PolykeyAgent; + const secretContentNewlineArb = fc.stringMatching(/^[ -~]+\n$/).noShrink(); + beforeEach(async () => { dataDir = await fs.promises.mkdtemp( path.join(globalThis.tmpDir, 'polykey-test-'), @@ -59,6 +62,7 @@ describe('commandEnv', () => { 'unix', `${vaultName}:SECRET`, '--', + '--', 'node', '-e', 'console.log(JSON.stringify(process.env))', @@ -86,6 +90,7 @@ describe('commandEnv', () => { `${vaultName}:SECRET1`, `${vaultName}:SECRET2`, '--', + '--', 'node', '-e', 'console.log(JSON.stringify(process.env))', @@ -115,6 +120,7 @@ describe('commandEnv', () => { 'unix', `${vaultName}:dir1`, '--', + '--', 'node', '-e', 'console.log(JSON.stringify(process.env))', @@ -142,6 +148,7 @@ describe('commandEnv', () => { 'unix', `${vaultName}:SECRET=SECRET_NEW`, '--', + '--', 'node', '-e', 'console.log(JSON.stringify(process.env))', @@ -170,6 +177,7 @@ describe('commandEnv', () => { 'unix', `${vaultName}:dir1=SECRET_NEW`, '--', + '--', 'node', '-e', 'console.log(JSON.stringify(process.env))', @@ -203,6 +211,7 @@ describe('commandEnv', () => { `${vaultName}:SECRET2`, `${vaultName}:dir1`, '--', + '--', 'node', '-e', 'console.log(JSON.stringify(process.env))', @@ -231,6 +240,7 @@ describe('commandEnv', () => { 'unix', `${vaultName}:SECRET1`, '--', + '--', 'node', '-e', 'console.log(JSON.stringify(process.env))', @@ -267,6 +277,7 @@ describe('commandEnv', () => { `${vaultName}:SECRET3=SECRET4`, `${vaultName}:dir1`, '--', + '--', 'node', '-e', 'console.log(JSON.stringify(process.env))', @@ -669,12 +680,11 @@ describe('commandEnv', () => { }); test('newlines in secrets are untouched', async () => { const vaultId = await polykeyAgent.vaultManager.createVault(vaultName); + const secretName = 'SECRET'; + const secretContent = + 'this is a secret\nit has multiple lines\n\nand some more lines'; await polykeyAgent.vaultManager.withVaults([vaultId], async (vault) => { - await vaultOps.addSecret( - vault, - 'SECRET', - 'this is a secret\nit has multiple lines\n', - ); + await vaultOps.addSecret(vault, secretName, secretContent); }); const command = [ 'secrets', @@ -683,7 +693,8 @@ describe('commandEnv', () => { dataDir, '--env-format', 'unix', - `${vaultName}:SECRET`, + `${vaultName}:${secretName}`, + '--', '--', 'node', '-e', @@ -694,9 +705,77 @@ describe('commandEnv', () => { }); expect(result.exitCode).toBe(0); const jsonOut = JSON.parse(result.stdout); - expect(jsonOut['SECRET']).toBe('this is a secret\nit has multiple lines\n'); + expect(jsonOut[secretName]).toBe(secretContent); }); - test.prop([ + test.prop([testUtils.vaultNameArb, secretContentNewlineArb], { + numRuns: 2, + })( + 'single trailing newline is automatically removed', + async (vaultName, secretContent) => { + const vaultId = await polykeyAgent.vaultManager.createVault(vaultName); + const secretName = 'SECRET'; + await polykeyAgent.vaultManager.withVaults([vaultId], async (vault) => { + await vaultOps.addSecret(vault, secretName, secretContent); + }); + const command = [ + 'secrets', + 'env', + '-np', + dataDir, + '--env-format', + 'unix', + `${vaultName}:${secretName}`, + '--', + '--', + 'node', + '-e', + 'console.log(JSON.stringify(process.env))', + ]; + const result = await testUtils.pkExec(command, { + env: { PK_PASSWORD: password }, + }); + expect(result.exitCode).toBe(0); + const jsonOut = JSON.parse(result.stdout); + // Remove last character + expect(jsonOut[secretName]).toBe(secretContent.slice(0, -1)); + }, + ); + test.prop([testUtils.vaultNameArb, secretContentNewlineArb], { + numRuns: 2, + })( + 'trailing newlines are preserved with option', + async (vaultName, secretContent) => { + const vaultId = await polykeyAgent.vaultManager.createVault(vaultName); + const secretName = 'SECRET'; + await polykeyAgent.vaultManager.withVaults([vaultId], async (vault) => { + await vaultOps.addSecret(vault, secretName, secretContent); + }); + const command = [ + 'secrets', + 'env', + '-np', + dataDir, + '--env-format', + 'unix', + '--preserve-newline', + `${vaultName}:${secretName}`, + `${vaultName}:${secretName}`, + '--', + '--', + 'node', + '-e', + 'console.log(JSON.stringify(process.env))', + ]; + const result = await testUtils.pkExec(command, { + env: { PK_PASSWORD: password }, + }); + expect(result.exitCode).toBe(0); + const jsonOut = JSON.parse(result.stdout); + expect(jsonOut[secretName]).toBe(secretContent); + await polykeyAgent.vaultManager.destroyVault(vaultId); + }, + ); + test.only.prop([ testUtils.secretPathEnvArrayArb, fc.string().noShrink(), testUtils.cmdArgsArrayArb, @@ -704,8 +783,10 @@ describe('commandEnv', () => { 'parse secrets env arguments', async (secretPathEnvArray, cmd, cmdArgsArray) => { let output: - | [Array<[string, string?, string?]>, Array] + | [Array, Array, boolean] | undefined = undefined; + // By running the parser directly, we are bypassing commander, so it works + // with a single -- const args: Array = [ ...secretPathEnvArray, '--', @@ -720,15 +801,15 @@ describe('commandEnv', () => { return binParsers.parseSecretPath(v); }); expect(parsedEnvs).toMatchObject(expectedSecretPathArray); - expect(parsedArgs).toMatchObject(['--', cmd, ...cmdArgsArray]); + expect(parsedArgs).toMatchObject([cmd, ...cmdArgsArray]); }, ); test('handles no arguments', async () => { const command = ['secrets', 'env', '-np', dataDir, '--env-format', 'unix']; - const result1 = await testUtils.pkExec(command, { + const result = await testUtils.pkExec(command, { env: { PK_PASSWORD: password }, }); - expect(result1.exitCode).toBe(64); + expect(result.exitCode).toBe(64); }); test('handles providing no secret paths', async () => { const command = [ @@ -739,12 +820,13 @@ describe('commandEnv', () => { '--env-format', 'unix', '--', + '--', 'someCommand', ]; - const result1 = await testUtils.pkExec(command, { + const result = await testUtils.pkExec(command, { env: { PK_PASSWORD: password }, }); - expect(result1.exitCode).toBe(64); + expect(result.exitCode).toBe(64); }); test('should output all secrets without explicit secret path', async () => { const vaultId1 = await polykeyAgent.vaultManager.createVault( diff --git a/tests/utils.test.ts b/tests/utils.test.ts index 63dee511..6f2a38f6 100644 --- a/tests/utils.test.ts +++ b/tests/utils.test.ts @@ -8,6 +8,7 @@ import * as polykeyErrors from 'polykey/dist/errors'; import * as fc from 'fast-check'; import * as binUtils from '@/utils/utils'; import * as binParsers from '@/utils/parsers'; +import * as testUtils from './utils'; describe('outputFormatters', () => { const nonPrintableCharArb = fc @@ -329,10 +330,6 @@ describe('outputFormatters', () => { }); describe('parsers', () => { - const vaultNameArb = fc.stringOf( - fc.char().filter((c) => binParsers.vaultNameRegex.test(c)), - { minLength: 1, maxLength: 100 }, - ); const singleSecretPathArb = fc.stringOf( fc.char().filter((c) => binParsers.secretPathRegex.test(c)), { minLength: 1, maxLength: 25 }, @@ -349,20 +346,20 @@ describe('parsers', () => { .tuple(valueFirstCharArb, valueRestCharArb) .map((components) => components.join('')); - test.prop([vaultNameArb], { numRuns: 100 })( + test.prop([testUtils.vaultNameArb], { numRuns: 100 })( 'should parse vault name', async (vaultName) => { expect(binParsers.parseVaultName(vaultName)).toEqual(vaultName); }, ); - test.prop([vaultNameArb], { numRuns: 10 })( + test.prop([testUtils.vaultNameArb], { numRuns: 10 })( 'should parse secret path with only vault name', async (vaultName) => { const result = [vaultName, undefined, undefined]; expect(binParsers.parseSecretPath(vaultName)).toEqual(result); }, ); - test.prop([vaultNameArb, secretPathArb], { numRuns: 100 })( + test.prop([testUtils.vaultNameArb, secretPathArb], { numRuns: 100 })( 'should parse full secret path with vault name', async (vaultName, secretPath) => { const query = `${vaultName}:${secretPath}`; @@ -370,7 +367,9 @@ describe('parsers', () => { expect(binParsers.parseSecretPath(query)).toEqual(result); }, ); - test.prop([vaultNameArb, secretPathArb, valueDataArb], { numRuns: 100 })( + test.prop([testUtils.vaultNameArb, secretPathArb, valueDataArb], { + numRuns: 100, + })( 'should parse full secret path with vault name and value', async (vaultName, secretPath, valueData) => { const query = `${vaultName}:${secretPath}=${valueData}`; diff --git a/tests/utils/utils.ts b/tests/utils/utils.ts index a9fa9eb5..6d185311 100644 --- a/tests/utils/utils.ts +++ b/tests/utils/utils.ts @@ -112,6 +112,13 @@ const cmdArgsArrayArb = fc }) .noShrink(); +const vaultNameArb = fc + .stringOf( + fc.char().filter((c) => binParsers.vaultNameRegex.test(c)), + { minLength: 1, maxLength: 100 }, + ) + .noShrink(); + export { testIf, describeIf, @@ -120,4 +127,5 @@ export { secretPathEnvArb, secretPathEnvArrayArb, cmdArgsArrayArb, + vaultNameArb, };