feat: removed newlines by default in secrets env command

feat: added option to preserve newlines

chore: removed listeners after closing edit process

chore: added fastcheck tests for added features

chore: automatically removing -- from parsed command list

chore: added comments for empty cases

chore: optimised handling of newline preservation

chore: updated tests

src/errors.ts

@@ -84,6 +84,11 @@ class ErrorPolykeyCLIUnexpectedError<T> extends ErrorPolykeyCLI<T> {
   exitCode = sysexits.SOFTWARE;
 }
 
+class ErrorPolykeyCLISubprocessFailure<T> extends ErrorPolykeyCLI<T> {
+  static description = 'A subprocess failed to exit gracefully';
+  exitCode = sysexits.UNKNOWN;
+}
+
 class ErrorPolykeyCLINodePath<T> extends ErrorPolykeyCLI<T> {
   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,

src/identities/CommandDiscover.ts

@@ -103,8 +103,8 @@ class CommandDiscover extends CommandPolykey {
                   case 'queued':
                     queuedSet.add(vertex);
                     break;
-                  case 'processed':
-                  case 'cancelled':
+                  case 'processed': // Fallthrough
+                  case 'cancelled': // Fallthrough
                   case 'failed':
                     queuedSet.delete(vertex);
                     break;

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,

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,26 +15,22 @@ 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);
     this.addOption(binOptions.clientPort);
     this.addOption(binOptions.envFormat);
     this.addOption(binOptions.envInvalid);
     this.addOption(binOptions.envDuplicate);
+    this.addOption(binOptions.preserveNewline);
     this.argument(
       '<args...>',
-      'command and arguments formatted as [envPaths...][-- cmd [cmdArgs...]]',
+      '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<string>],
-        options,
-      ) => {
-        args[1].shift();
+      async (args: [Array<ParsedSecretPathValue>, Array<string>], options) => {
         const { default: PolykeyClient } = await import(
           'polykey/dist/PolykeyClient'
         );
@@ -46,13 +43,22 @@ class CommandEnv extends CommandPolykey {
           envDuplicate: 'keep' | 'overwrite' | 'warn' | 'error';
           envFormat: 'auto' | 'unix' | 'cmd' | 'powershell' | 'json';
         } = options;
+        // Populate a set with all the paths we want to preserve newlines for
+        const preservedSecrets = new Set<string>();
+        for (const [vaultName, secretPath] of options.preserveNewline) {
+          // The vault name is guaranteed to have a value.
+          // If a secret path is undefined, then the newline preservation was
+          // targeting the secrets of the entire vault. Otherwise, the target
+          // was a single secret.
+          if (secretPath == null) preservedSecrets.add(vaultName);
+          else preservedSecrets.add(`${vaultName}:${secretPath}`);
+        }
         // There are a few stages here
         // 1. parse the desired secrets
         // 2. obtain the desired secrets
         // 3. switching behaviour here based on parameters
         //   a. exec the command with the provided env variables from the secrets
         //   b. output the env variables in the desired format
-
         const [envVariables, [cmd, ...argv]] = args;
         const clientOptions = await binProcessors.processClientOptions(
           options.nodePath,
@@ -160,7 +166,26 @@ class CommandEnv extends CommandPolykey {
                     utils.never();
                 }
               }
-              envp[newName] = secretContent;
+
+              // Find if we need to preserve the newline for this secret
+              let preserveNewline = false;
+              // If only the vault name is specified to be preserved, then
+              // preserve the newlines of all secrets inside the vault.
+              // Otherwise, if a full secret path has been specified, then
+              // preserve that secret path.
+              if (
+                preservedSecrets.has(nameOrId) ||
+                preservedSecrets.has(`${nameOrId}:${newName}`)
+              ) {
+                preserveNewline = true;
+              }
+
+              // 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 +200,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 '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: {

src/types.ts

@@ -61,6 +61,8 @@ type PromiseDeconstructed<T> = {
   rejectP: (reason?: any) => void;
 };
 
+type ParsedSecretPathValue = [string, string?, string?];
+
 export type {
   TableRow,
   TableOptions,
@@ -69,4 +71,5 @@ export type {
   AgentChildProcessInput,
   AgentChildProcessOutput,
   PromiseDeconstructed,
+  ParsedSecretPathValue,
 };

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 <path>',
+  '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,
 };

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';
@@ -56,9 +57,9 @@ function parseCoreCount(v: string): number | undefined {
   switch (v) {
     case 'all':
       return 0;
-    case 'none':
-    case 'no':
-    case 'false':
+    case 'none': // Fallthrough
+    case 'no': // Fallthrough
+    case 'false': // Fallthrough
     case 'null':
       return undefined;
     default:
@@ -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<string>] | undefined,
-): [Array<[string, string?, string?]>, Array<string>] {
-  const current: [Array<[string, string?, string?]>, Array<string>] = prev ?? [
-    [],
-    [],
-  ];
-  if (current[1].length === 0) {
+  prev: [Array<ParsedSecretPathValue>, Array<string>, boolean] | undefined,
+): [Array<ParsedSecretPathValue>, Array<string>, boolean] {
+  const current: [Array<ParsedSecretPathValue>, Array<string>, 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 {