Commands fail when currentvault names no vault (closes #156)

An empty currentvault, or one naming a vault that does not exist, made
`secret list` report an empty vault with exit 0 and `secret add` fail on a
missing directory. ResolveVaultSymlink now refuses an empty or invalid
name, or one with no directory under vaults.d, with an error that wraps
ErrVaultNotFound, names the file and says to run
`secret vault select <name>`. Every command that uses the current vault
gets it; `secret vault list` still lists the vaults.

newSizeTestVault wrote a full path into currentvault, which resolved only
because the last path element was taken as the name; it now relies on
CreateVault selecting the vault.

Model: opus-5-5

README.md

@@ -114,7 +114,10 @@ are allowed, and a name must not be empty, `.` or `..`.
 
 #### `secret vault select <name>`
 
-Switches to the specified vault for subsequent operations.
+Switches to the specified vault for subsequent operations. When `currentvault`
+is empty or names a vault that does not exist, every command that uses the
+current vault fails with an error that names the file and says to run
+`secret vault select <name>`; `secret vault list` still lists the vaults.
 
 #### `secret vault remove <name> [--force]` / `secret vault rm` ⚠️ 🛑
 

internal/cli/create_vault_test.go

@@ -144,20 +144,25 @@ 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 it fails as any subcommand of
-// `secret vault` that does not exist does.
+// 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.
 func TestNoVaultImportCommand(t *testing.T) {
 	t.Setenv(secret.EnvStateDir, t.TempDir())
-	t.Setenv(secret.EnvMnemonic, testMnemonic)
-	t.Setenv(secret.EnvUnlockPassphrase, testPassphrase)
 
-	_, err := cli.ExecuteCommandInProcess(
-		strings.Fields("vault create work"), "", nil)
-	require.NoError(t, err)
+	run := func(args ...string) string {
+		t.Setenv(secret.EnvMnemonic, testMnemonic)
+		t.Setenv(secret.EnvUnlockPassphrase, testPassphrase)
 
-	_, err = cli.ExecuteCommandInProcess(
-		strings.Fields("vault import work"), "", nil)
-	require.EqualError(t, err, `unknown command "import" for "secret vault"`)
+		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"))
 }
 
 // TestStopAtPassphrasePromptLeavesNothing is a regression test for the

internal/cli/current_vault_test.go

@@ -0,0 +1,59 @@
+package cli_test
+
+import (
+	"bytes"
+	"path/filepath"
+	"strconv"
+	"testing"
+
+	"github.com/spf13/afero"
+	"github.com/spf13/cobra"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+	"sneak.berlin/go/secret/internal/cli"
+	"sneak.berlin/go/secret/internal/secret"
+	"sneak.berlin/go/secret/internal/vault"
+)
+
+// TestListFailsWhenCurrentVaultDoesNotExist is a regression test for
+// https://git.eeqj.de/sneak/secret/issues/156, where a currentvault file
+// that was empty, or named a vault that does not exist, made `secret list`
+// report an empty vault. Listing must fail with an error that names the
+// file and says to run `secret vault select`, `secret vault list` must
+// still list the vaults, and selecting a vault must make listing work
+// again.
+func TestListFailsWhenCurrentVaultDoesNotExist(t *testing.T) {
+	t.Parallel()
+
+	for _, name := range []string{"", "gone"} {
+		t.Run(strconv.Quote(name), func(t *testing.T) {
+			t.Parallel()
+
+			fs := newTwoVaultFs(t)
+			currentVaultPath := filepath.Join(testStateDir, "currentvault")
+			require.NoError(t, afero.WriteFile(fs, currentVaultPath,
+				[]byte(name), secret.FilePerms))
+
+			c := cli.NewCLIInstanceWithStateDir(fs, testStateDir)
+
+			var out bytes.Buffer
+
+			cmd := &cobra.Command{}
+			cmd.SetOut(&out)
+
+			err := c.ListSecrets(cmd, false, true, "")
+			require.ErrorIs(t, err, vault.ErrVaultNotFound)
+			require.ErrorContains(t, err, currentVaultPath)
+			require.ErrorContains(t, err, "secret vault select")
+			assert.Empty(t, out.String())
+
+			require.NoError(t, c.ListVaults(cmd, false))
+			assert.Equal(t, "Available vaults:\n  default\n  work\n", out.String())
+
+			require.NoError(t, c.SelectVault(cmd, "default"))
+			out.Reset()
+			require.NoError(t, c.ListSecrets(cmd, false, true, ""))
+			assert.Equal(t, "x\n", out.String())
+		})
+	}
+}

internal/cli/generate.go

@@ -29,12 +29,6 @@ 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/secrets_size_test.go

@@ -36,17 +36,11 @@ func newSizeTestVault(t *testing.T) (afero.Fs, *vault.Vault) {
 
 	fs := afero.NewMemMapFs()
 
-	// Create vault
+	// Create vault, which selects it as the current vault
 	_, err := vault.CreateVault(fs, testStateDir, testVaultName,
 		testMnemonicBuffer(t), nil)
 	require.NoError(t, err)
 
-	// Set current vault
-	currentVaultPath := filepath.Join(testStateDir, "currentvault")
-	vaultPath := filepath.Join(testStateDir, "vaults.d", testVaultName)
-	err = afero.WriteFile(fs, currentVaultPath, []byte(vaultPath), 0o600)
-	require.NoError(t, err)
-
 	// Get vault and set up long-term key
 	vlt, err := vault.GetCurrentVault(fs, testStateDir)
 	require.NoError(t, err)

internal/cli/unknown_subcommand_test.go

@@ -1,58 +0,0 @@
-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,12 +126,6 @@ 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,12 +29,6 @@ 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,12 +43,6 @@ 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

internal/vault/management.go

@@ -55,7 +55,10 @@ func ValidateVaultName(name string) error {
 }
 
 // ResolveVaultSymlink reads the currentvault file to get the path to the current vault
-// The file contains just the vault name (e.g., "default")
+// The file contains just the vault name (e.g., "default"). An empty or
+// invalid name, or the name of a vault that does not exist, is an error
+// wrapping ErrVaultNotFound that names the file and says to run
+// 'secret vault select <name>'.
 func ResolveVaultSymlink(fs afero.Fs, currentVaultPath string) (string, error) {
 	secret.Debug("resolveVaultSymlink starting", "path", currentVaultPath)
 
@@ -74,6 +77,21 @@ func ResolveVaultSymlink(fs afero.Fs, currentVaultPath string) (string, error) {
 	stateDir := filepath.Dir(currentVaultPath)
 	absolutePath := filepath.Join(stateDir, "vaults.d", vaultName)
 
+	// An empty name would make vaults.d itself the vault's directory
+	exists := false
+	if isValidVaultName(vaultName) {
+		exists, err = afero.DirExists(fs, absolutePath)
+		if err != nil {
+			return "", fmt.Errorf("failed to check if vault exists: %w", err)
+		}
+	}
+
+	if !exists {
+		return "", fmt.Errorf("vault '%s' named in %s %w; run "+
+			"'secret vault select <name>' to select a vault",
+			vaultName, currentVaultPath, ErrVaultNotFound)
+	}
+
 	secret.Debug("Resolved to absolute path", "absolute_path", absolutePath)
 
 	return absolutePath, nil