Unknown subcommands of vault, unlocker, version and generate fail (closes #183)

Given a subcommand they do not have, `secret vault`, `secret unlocker`,
`secret version` and `secret generate` printed their help and exited 0,
so a mistyped command looked like a successful one. Each now exits 1
with `unknown command "<name>" for "secret <parent>"`, and with no
arguments still prints its help.

Cobra checks a command's Args only when it has a RunE, so each parent
gets `Args: cobra.NoArgs` and a RunE that prints its help. Args alone
changes nothing. TestNoVaultImportCommand required the old answer to
`secret vault import` and now requires the error.

Judgement call: their help now shows a `secret <parent> [flags]` usage
line, which cobra adds for any command that runs.
Judgement call: `secret vault help` now fails as an unknown subcommand;
`secret help vault` and `secret vault --help` still work.

Model: opus-5-5

README.md

@@ -313,11 +313,6 @@ generates a new Age key.
 
 Decrypts data using an Age key stored as a secret.
 
-With `--output`, both hold the result in memory and write it to `--output` only
-once encryption or decryption has succeeded, so a failure to encrypt or decrypt
-leaves `--output` untouched. A failure while writing `--output`, such as a full
-disk, can leave it empty or partly written.
-
 ## Storage Architecture
 
 ### Directory Structure

internal/cli/create_vault_test.go

@@ -144,25 +144,20 @@ func TestVaultCreationLeavesNoSecretInEnvironment(t *testing.T) {
 // TestNoVaultImportCommand is a regression test for
 // https://git.eeqj.de/sneak/secret/issues/139: `secret vault import`
 // refused every vault that `secret vault create` makes, because each has a
-// long-term key. The command is gone, so cobra answers it with the help of
-// `secret vault`, as it does any subcommand of `secret vault` it does not
-// know.
+// long-term key. The command is gone, so it fails as any subcommand of
+// `secret vault` that does not exist does.
 func TestNoVaultImportCommand(t *testing.T) {
 	t.Setenv(secret.EnvStateDir, t.TempDir())
+	t.Setenv(secret.EnvMnemonic, testMnemonic)
+	t.Setenv(secret.EnvUnlockPassphrase, testPassphrase)
 
-	run := func(args ...string) string {
-		t.Setenv(secret.EnvMnemonic, testMnemonic)
-		t.Setenv(secret.EnvUnlockPassphrase, testPassphrase)
+	_, err := cli.ExecuteCommandInProcess(
+		strings.Fields("vault create work"), "", nil)
+	require.NoError(t, err)
 
-		output, err := cli.ExecuteCommandInProcess(args, "", nil)
-		require.NoError(t, err, "%v", args)
-
-		return output
-	}
-
-	run("vault", "create", "work")
-
-	require.Equal(t, run("vault"), run("vault", "import", "work"))
+	_, err = cli.ExecuteCommandInProcess(
+		strings.Fields("vault import work"), "", nil)
+	require.EqualError(t, err, `unknown command "import" for "secret vault"`)
 }
 
 // TestStopAtPassphrasePromptLeavesNothing is a regression test for the

internal/cli/crypto.go

@@ -1,7 +1,6 @@
 package cli
 
 import (
-	"bytes"
 	"errors"
 	"fmt"
 	"io"
@@ -108,8 +107,8 @@ func (cli *Instance) storeNewEncryptionKey(
 // does not exist. The caller must destroy the returned buffer. Only storing
 // a new key takes the state directory lock, so that reading an existing key
 // works on a read-only state directory and keeps no other command waiting
-// at the passphrase prompt, and Encrypt reads its input and writes its
-// output without the lock.
+// at the passphrase prompt, and Encrypt streams its input and output
+// unlocked.
 func (cli *Instance) resolveEncryptionKey(
 	vlt *vault.Vault, secretName string,
 ) (*memguard.LockedBuffer, error) {
@@ -189,17 +188,17 @@ func (cli *Instance) Encrypt(secretName, inputFile, outputFile string) error {
 		input = file
 	}
 
-	// Set up output writer. With --output the result is held in memory
-	// and written only once encryption has succeeded, so a failure to
-	// encrypt leaves --output untouched and --input can be the same file.
-	// A failure while writing --output, such as a full disk, can leave it
-	// empty or partly written.
+	// Set up output writer
 	output := cli.cmd.OutOrStdout()
 
-	var result bytes.Buffer
-
 	if outputFile != "" {
-		output = &result
+		file, err := cli.fs.Create(outputFile)
+		if err != nil {
+			return fmt.Errorf("failed to create output file: %w", err)
+		}
+		defer func() { _ = file.Close() }()
+
+		output = file
 	}
 
 	// Encrypt the data
@@ -218,11 +217,7 @@ func (cli *Instance) Encrypt(secretName, inputFile, outputFile string) error {
 		return fmt.Errorf("failed to finalize encryption: %w", err)
 	}
 
-	if outputFile == "" {
-		return nil
-	}
-
-	return cli.writeOutputFile(outputFile, result.Bytes())
+	return nil
 }
 
 // Decrypt decrypts data using an age secret key stored in a secret
@@ -283,17 +278,17 @@ func (cli *Instance) Decrypt(secretName, inputFile, outputFile string) error {
 		input = file
 	}
 
-	// Set up output writer. With --output the result is held in memory
-	// and written only once decryption has succeeded, so a failure to
-	// decrypt leaves --output untouched and --input can be the same file.
-	// A failure while writing --output, such as a full disk, can leave it
-	// empty or partly written.
+	// Set up output writer
 	output := cli.cmd.OutOrStdout()
 
-	var result bytes.Buffer
-
 	if outputFile != "" {
-		output = &result
+		file, err := cli.fs.Create(outputFile)
+		if err != nil {
+			return fmt.Errorf("failed to create output file: %w", err)
+		}
+		defer func() { _ = file.Close() }()
+
+		output = file
 	}
 
 	// Decrypt the data
@@ -307,32 +302,6 @@ func (cli *Instance) Decrypt(secretName, inputFile, outputFile string) error {
 		return fmt.Errorf("failed to decrypt data: %w", err)
 	}
 
-	if outputFile == "" {
-		return nil
-	}
-
-	return cli.writeOutputFile(outputFile, result.Bytes())
-}
-
-// writeOutputFile creates outputFile, or empties it if it exists, and
-// writes result into it.
-func (cli *Instance) writeOutputFile(outputFile string, result []byte) error {
-	file, err := cli.fs.Create(outputFile)
-	if err != nil {
-		return fmt.Errorf("failed to create output file: %w", err)
-	}
-
-	_, err = file.Write(result)
-
-	closeErr := file.Close()
-	if err == nil {
-		err = closeErr
-	}
-
-	if err != nil {
-		return fmt.Errorf("failed to write output file: %w", err)
-	}
-
 	return nil
 }
 

internal/cli/crypto_test.go

@@ -1,185 +0,0 @@
-//nolint:testpackage // sets the unexported fields of Instance
-package cli
-
-import (
-	"errors"
-	"fmt"
-	"io"
-	"os"
-	"path/filepath"
-	"testing"
-
-	"github.com/spf13/afero"
-	"github.com/spf13/cobra"
-	"github.com/stretchr/testify/require"
-	"sneak.berlin/go/secret/internal/vault"
-)
-
-// plainText is the content of plain.txt in the directory newCryptoTest makes.
-const plainText = "document"
-
-// newCryptoTest returns an Instance whose current vault, in a new state
-// directory on the real filesystem, holds the age key secret "key", and a
-// new directory holding only plain.txt and plain.txt.age, its encryption
-// with that key.
-func newCryptoTest(t *testing.T) (*Instance, string) {
-	t.Helper()
-
-	fs := afero.NewOsFs()
-	stateDir := t.TempDir()
-
-	_, err := vault.CreateVault(fs, stateDir, "default", testMnemonicBuffer(t), nil)
-	require.NoError(t, err)
-
-	cli := NewCLIInstanceWithStateDir(fs, stateDir)
-	cli.Mnemonic = testMnemonicBuffer(t)
-	cli.cmd = &cobra.Command{}
-
-	dir := t.TempDir()
-	plain := filepath.Join(dir, "plain.txt")
-	require.NoError(t, afero.WriteFile(fs, plain, []byte(plainText), 0o600))
-	require.NoError(t, cli.Encrypt("key", plain, plain+".age"))
-
-	return cli, dir
-}
-
-// TestFailedCryptoLeavesOutputUnchanged is a regression test for
-// https://git.eeqj.de/sneak/secret/issues/143, where secret encrypt and
-// secret decrypt emptied the file named by --output before they failed.
-// A failed command must leave an existing file as it was and create none,
-// and give the same error as without --output.
-func TestFailedCryptoLeavesOutputUnchanged(t *testing.T) {
-	t.Parallel()
-
-	tests := []struct {
-		name string
-		run  func(cli *Instance, dir, output string) error
-	}{
-		// Reading a directory fails once encryption has begun
-		{"encrypt", func(cli *Instance, dir, output string) error {
-			return cli.Encrypt("key", dir, output)
-		}},
-		// plain.txt is not an age file
-		{"decrypt", func(cli *Instance, dir, output string) error {
-			return cli.Decrypt("key", filepath.Join(dir, "plain.txt"), output)
-		}},
-	}
-
-	for _, tt := range tests {
-		t.Run(tt.name, func(t *testing.T) {
-			t.Parallel()
-
-			cli, dir := newCryptoTest(t)
-			existing := filepath.Join(dir, "existing.txt")
-			require.NoError(t, afero.WriteFile(cli.fs, existing, []byte("precious"), 0o600))
-
-			cli.cmd.SetOut(io.Discard)
-			toStdout := tt.run(cli, dir, "")
-			require.Error(t, toStdout)
-
-			require.EqualError(t, tt.run(cli, dir, existing), toStdout.Error())
-			require.EqualError(t, tt.run(cli, dir, filepath.Join(dir, "new.txt")),
-				toStdout.Error())
-
-			content, err := afero.ReadFile(cli.fs, existing)
-			require.NoError(t, err)
-			require.Equal(t, "precious", string(content))
-
-			exists, err := afero.Exists(cli.fs, filepath.Join(dir, "new.txt"))
-			require.NoError(t, err)
-			require.False(t, exists, "a failed %s created --output", tt.name)
-		})
-	}
-}
-
-// TestCryptoInPlace checks that secret encrypt and secret decrypt given
-// one file as both --input and --output replace it with the right result.
-// Before https://git.eeqj.de/sneak/secret/issues/143 they emptied the file
-// before reading it.
-func TestCryptoInPlace(t *testing.T) {
-	t.Parallel()
-
-	cli, dir := newCryptoTest(t)
-	file := filepath.Join(dir, "plain.txt")
-
-	require.NoError(t, cli.Encrypt("key", file, file))
-	require.NoError(t, cli.Decrypt("key", file, file))
-
-	content, err := afero.ReadFile(cli.fs, file)
-	require.NoError(t, err)
-	require.Equal(t, plainText, string(content))
-}
-
-// errCloseFailed is returned by the files closeFailFs creates in place of
-// a successful Close, as NFS reports a full disk only when the file is
-// closed.
-var errCloseFailed = errors.New("no space left on device")
-
-// closeFailFs creates files whose Close fails with errCloseFailed.
-type closeFailFs struct {
-	afero.Fs
-}
-
-// closeFailFile is a file created through closeFailFs.
-type closeFailFile struct {
-	afero.File
-}
-
-func (f closeFailFile) Close() error {
-	_ = f.File.Close()
-
-	return errCloseFailed
-}
-
-//nolint:ireturn // afero.File is the interface required by afero.Fs
-func (f closeFailFs) Create(name string) (afero.File, error) {
-	file, err := f.Fs.Create(name)
-	if err != nil {
-		return nil, err
-	}
-
-	return closeFailFile{File: file}, nil
-}
-
-// TestDecryptFailsWhenClosingOutputFails checks that secret decrypt fails
-// when closing --output fails, since some filesystems report a failed
-// write only then.
-func TestDecryptFailsWhenClosingOutputFails(t *testing.T) {
-	t.Parallel()
-
-	cli, dir := newCryptoTest(t)
-	cli.fs = closeFailFs{Fs: cli.fs}
-
-	err := cli.Decrypt("key", filepath.Join(dir, "plain.txt.age"),
-		filepath.Join(dir, "out.txt"))
-	require.ErrorIs(t, err, errCloseFailed)
-}
-
-// TestDecryptToDescriptorOfDeletedFile checks that secret decrypt given
-// /dev/fd/N as --output writes through that descriptor. A program may give
-// the command a temporary file it has already deleted as stdout, pass
-// /dev/stdout as --output and read the result back from that file.
-func TestDecryptToDescriptorOfDeletedFile(t *testing.T) {
-	t.Parallel()
-
-	cli, dir := newCryptoTest(t)
-
-	file, err := os.CreateTemp(t.TempDir(), "out")
-	require.NoError(t, err)
-
-	defer func() { _ = file.Close() }()
-
-	require.NoError(t, os.Remove(file.Name()))
-
-	output := fmt.Sprintf("/dev/fd/%d", file.Fd())
-	require.NoError(t, cli.Decrypt("key", filepath.Join(dir, "plain.txt.age"), output))
-
-	// On macOS the command's open of /dev/fd/N shares this descriptor's
-	// position, which its write leaves at the end.
-	_, err = file.Seek(0, io.SeekStart)
-	require.NoError(t, err)
-
-	content, err := io.ReadAll(file)
-	require.NoError(t, err)
-	require.Equal(t, plainText, string(content))
-}

internal/cli/generate.go

@@ -29,6 +29,12 @@ func newGenerateCmd() *cobra.Command {
 		Use:   "generate",
 		Short: "Generate random data",
 		Long:  `Generate various types of random data including mnemonics and secrets.`,
+		// Cobra checks Args only on a command that has a RunE; without
+		// one, an unknown subcommand prints this help and exits 0.
+		Args: cobra.NoArgs,
+		RunE: func(cmd *cobra.Command, _ []string) error {
+			return cmd.Help()
+		},
 	}
 
 	cmd.AddCommand(newGenerateMnemonicCmd())

internal/cli/unknown_subcommand_test.go

@@ -0,0 +1,58 @@
+package cli_test
+
+import (
+	"fmt"
+	"os/exec"
+	"strings"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+	"sneak.berlin/go/secret/internal/secret"
+)
+
+// TestUnknownSubcommandFails is a regression test for
+// https://git.eeqj.de/sneak/secret/issues/183: given a subcommand it does not
+// have, `secret vault`, `secret unlocker`, `secret version` and `secret
+// generate` printed their help and exited 0, so a script could not tell a
+// mistyped command from a successful one. Each runs here as the built binary
+// and must exit 1 naming the subcommand, as an unknown command does at the
+// top level, and must still print its help and exit 0 when given nothing.
+func TestUnknownSubcommandFails(t *testing.T) {
+	t.Parallel()
+
+	stateDir := t.TempDir()
+
+	// run runs the built binary with args and returns what it printed and
+	// its exit code
+	run := func(args ...string) (string, int) {
+		//nolint:gosec // G204: test executes the freshly built secret binary
+		cmd := exec.CommandContext(t.Context(), secretBinaryPath(t), args...)
+		cmd.Env = []string{secret.EnvStateDir + "=" + stateDir}
+
+		output, err := cmd.CombinedOutput()
+		if err != nil {
+			var exitErr *exec.ExitError
+			require.ErrorAs(t, err, &exitErr, "secret %v did not run", args)
+		}
+
+		return string(output), cmd.ProcessState.ExitCode()
+	}
+
+	for _, call := range []string{
+		"vault improt", "unlocker foo", "version foo", "generate foo",
+	} {
+		parent, unknown, _ := strings.Cut(call, " ")
+
+		output, exitCode := run(parent, unknown)
+		assert.Equal(t, 1, exitCode, "secret %s should fail:\n%s", call, output)
+		assert.Contains(t, output, fmt.Sprintf(
+			"unknown command %q for \"secret %s\"", unknown, parent))
+
+		output, exitCode = run(parent)
+		assert.Equal(t, 0, exitCode,
+			"secret %s should succeed:\n%s", parent, output)
+		assert.Contains(t, output, usageHeading,
+			"secret %s should print its help", parent)
+	}
+}

internal/cli/unlockers.go

@@ -126,6 +126,12 @@ func newUnlockerCmd() *cobra.Command {
 		Use:   "unlocker",
 		Short: "Manage unlockers",
 		Long:  `Create, list, and remove unlockers for the current vault.`,
+		// Cobra checks Args only on a command that has a RunE; without
+		// one, an unknown subcommand prints this help and exits 0.
+		Args: cobra.NoArgs,
+		RunE: func(cmd *cobra.Command, _ []string) error {
+			return cmd.Help()
+		},
 	}
 
 	cmd.AddCommand(newUnlockerListCmd())

internal/cli/vault.go

@@ -29,6 +29,12 @@ func newVaultCmd() *cobra.Command {
 		Use:   "vault",
 		Short: "Manage vaults",
 		Long:  `Create, list, and select vaults for organizing secrets.`,
+		// Cobra checks Args only on a command that has a RunE; without
+		// one, an unknown subcommand prints this help and exits 0.
+		Args: cobra.NoArgs,
+		RunE: func(cmd *cobra.Command, _ []string) error {
+			return cmd.Help()
+		},
 	}
 
 	cmd.AddCommand(newVaultListCmd())

internal/cli/version.go

@@ -43,6 +43,12 @@ func VersionCommands(cli *Instance) *cobra.Command {
 		Short: "Manage secret versions",
 		Long: "Commands for managing secret versions including listing, " +
 			"promoting, and retrieving specific versions",
+		// Cobra checks Args only on a command that has a RunE; without
+		// one, an unknown subcommand prints this help and exits 0.
+		Args: cobra.NoArgs,
+		RunE: func(cmd *cobra.Command, _ []string) error {
+			return cmd.Help()
+		},
 	}
 
 	// List versions command