Check vault names in every command that takes one (closes #68)

A vault name may use only lowercase ASCII letters, digits, `.`, `-` and
`_`, and must not be empty, `.` or `..`; the error now states that rule.
`vault create`, `vault import`, `vault select`, `vault remove`, both
vault names of `mv` and shell completion of a `vault:secret` argument
check the name as typed before building any path from it. Before,
`vault import ..` wrote a long-term key and an unlocker into the state
directory itself, and `vault select ..` made that the current vault.

Model: opus-5-5

internal/cli/completions.go

@@ -123,7 +123,9 @@ func getVaultNamesCompletionFunc(fs afero.Fs, stateDir string) func(
 }
 
 // completeVaultQualifiedSecrets completes "vault:secret" references once a
-// colon is present in the input
+// colon is present in the input. It completes nothing when the vault part
+// is not a valid vault name, so that a name such as ".." cannot list a
+// directory outside vaults.d.
 func completeVaultQualifiedSecrets(
 	fs afero.Fs, stateDir, toComplete string,
 ) []string {
@@ -134,6 +136,10 @@ func completeVaultQualifiedSecrets(
 	vaultName := parts[0]
 	secretPrefix := parts[1]
 
+	if vault.ValidateVaultName(vaultName) != nil {
+		return nil
+	}
+
 	vlt := vault.NewVault(fs, stateDir, vaultName)
 
 	secrets, err := vlt.ListSecrets()

internal/cli/completions_test.go

@@ -0,0 +1,41 @@
+//nolint:testpackage // white-box test of unexported internals
+package cli
+
+import (
+	"path/filepath"
+	"testing"
+
+	"github.com/spf13/afero"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+// TestVaultSecretCompletionRejectsInvalidVaultName is a regression test for
+// https://git.eeqj.de/sneak/secret/issues/68: completing a `vault:secret`
+// argument lists nothing when the vault part is not a valid vault name, even
+// where that name, joined onto vaults.d, leads to a secrets.d directory.
+func TestVaultSecretCompletionRejectsInvalidVaultName(t *testing.T) {
+	t.Parallel()
+
+	const (
+		stateDir = "/state"
+		dirPerm  = 0o700
+	)
+
+	fs := afero.NewMemMapFs()
+
+	// The vault "work" holds the secret "x". So does every directory an
+	// invalid name below would lead to from vaults.d.
+	for _, vaultName := range []string{"work", ".", "..", "a/b"} {
+		secretDir := filepath.Join(stateDir, "vaults.d", vaultName, "secrets.d", "x")
+		require.NoError(t, fs.MkdirAll(secretDir, dirPerm))
+	}
+
+	assert.Equal(t, []string{"work:x"},
+		completeVaultQualifiedSecrets(fs, stateDir, "work:"))
+
+	for _, toComplete := range []string{".:", "..:", "a/b:"} {
+		assert.Empty(t, completeVaultQualifiedSecrets(fs, stateDir, toComplete),
+			"completing %q", toComplete)
+	}
+}

internal/cli/move_test.go

@@ -48,26 +48,25 @@ func TestRejectedMoveWithinVaultLeavesStateUnchanged(t *testing.T) {
 			"mv work:nosuch work:y", "work:nosuch", "work:y", false,
 			"secret 'nosuch' not found",
 		},
-		// Only an existing vault is used, so ".." cannot reach the state
-		// directory itself.
+		// Only an existing vault is used.
 		{
-			"mv --force ..:x ..:y", "..:x", "..:y", true,
-			"vault '..' does not exist",
+			"mv --force nosuch:x nosuch:y", "nosuch:x", "nosuch:y", true,
+			"vault 'nosuch' does not exist",
 		},
-		// Each of these spells "work" a second way. The spelling is not an
-		// existing vault name, so the move is not taken for a move between
-		// two vaults, which would delete the destination, here the source.
+		// Each of these spells "work" a second way. The spelling is not a
+		// valid vault name, so the move is not taken for a move between two
+		// vaults, which would delete the destination, here the source.
 		{
 			"mv --force work:x work/:x", workX, "work/:x", true,
-			"vault 'work/' does not exist",
+			vault.ValidateVaultName("work/").Error(),
 		},
 		{
 			"mv --force work/:x work:", "work/:x", "work:", true,
-			"vault 'work/' does not exist",
+			vault.ValidateVaultName("work/").Error(),
 		},
 		{
 			"mv --force work:x ./work:x", workX, "./work:x", true,
-			"vault './work' does not exist",
+			vault.ValidateVaultName("./work").Error(),
 		},
 	}
 

internal/cli/path_traversal_test.go

@@ -292,6 +292,58 @@ func TestInvalidVersionLeavesVaultsUnchanged(t *testing.T) {
 	}
 }
 
+// TestInvalidVaultNameLeavesStateUnchanged is a regression test for
+// https://git.eeqj.de/sneak/secret/issues/68, where
+// `secret vault import ..` wrote a long-term key and an unlocker into the
+// state directory itself, and `secret vault select ..` made it the current
+// vault. Each command that takes a vault name must reject an invalid one
+// before building a path from it. The mnemonic and the passphrase are set,
+// and moves and removals use --force, so that only the name check stands
+// in the way.
+//
+//nolint:paralleltest // newTwoVaultFs uses t.Setenv
+func TestInvalidVaultNameLeavesStateUnchanged(t *testing.T) {
+	before := snapshotStateDir(t, newTwoVaultFs(t))
+
+	t.Setenv(secret.EnvUnlockPassphrase, testPassphrase)
+
+	cmd := &cobra.Command{}
+
+	// Each command is a format with %q where the vault name goes.
+	commands := []struct {
+		command string
+		run     func(c *cli.Instance, name string) error
+	}{
+		{"vault create %q", func(c *cli.Instance, name string) error {
+			return c.CreateVault(cmd, name)
+		}},
+		{"vault import %q", func(c *cli.Instance, name string) error {
+			return c.VaultImport(cmd, name)
+		}},
+		{"vault select %q", func(c *cli.Instance, name string) error {
+			return c.SelectVault(cmd, name)
+		}},
+		{"vault remove --force %q", func(c *cli.Instance, name string) error {
+			return c.RemoveVault(cmd, name, true)
+		}},
+		{"mv --force %q:x work:x", func(c *cli.Instance, name string) error {
+			return c.MoveSecret(cmd, name+":x", "work:x", true)
+		}},
+		{"mv --force default:x %q:x", func(c *cli.Instance, name string) error {
+			return c.MoveSecret(cmd, "default:x", name+":x", true)
+		}},
+	}
+
+	for _, tt := range commands {
+		for _, name := range []string{"", ".", "..", "a/b"} {
+			t.Run(fmt.Sprintf(tt.command, name), func(t *testing.T) {
+				requireRejectedAndUnchanged(t, before, vault.ValidateVaultName(name),
+					func(c *cli.Instance) error { return tt.run(c, name) })
+			})
+		}
+	}
+}
+
 // TestRemoveVersionRemovesOnlyThatVersion checks that `secret version rm`
 // with a version that is not the current one removes that version and
 // changes nothing else.

internal/cli/secrets.go

@@ -811,9 +811,9 @@ func (cli *Instance) moveSecret(
 			cmd, vlt, srcSecretName, destSecretName, force)
 	}
 
-	// Both vaults must be existing vaults by exact name, so that two
-	// spellings of one vault, such as "work" and "work/", are never taken for
-	// two vaults. A named vault does not become the current vault.
+	// Both vault names must be valid and name existing vaults exactly, so
+	// that two spellings of one vault, such as "work" and "work/", are never
+	// taken for two vaults. A named vault does not become the current vault.
 	srcVault, err := cli.existingVault(srcVaultName)
 	if err != nil {
 		return err
@@ -833,9 +833,15 @@ func (cli *Instance) moveSecret(
 		cmd, srcVault, srcSecretName, destVault, destSecretName, force)
 }
 
-// existingVault returns the vault with the given name, or an error if there
-// is none. Unlike vault.SelectVault, it leaves the current vault as it is.
+// existingVault returns the vault with the given name, or an error if the
+// name is not a valid vault name or there is no such vault. Unlike
+// vault.SelectVault, it leaves the current vault as it is.
 func (cli *Instance) existingVault(name string) (*vault.Vault, error) {
+	err := vault.ValidateVaultName(name)
+	if err != nil {
+		return nil, err
+	}
+
 	vaults, err := vault.ListVaults(cli.fs, cli.stateDir)
 	if err != nil {
 		return nil, fmt.Errorf("failed to list vaults: %w", err)

internal/cli/vault.go

@@ -462,6 +462,11 @@ func updateVaultImportMetadata(
 // VaultImport imports a mnemonic into a specific vault, holding the state
 // directory lock while importMnemonic runs
 func (cli *Instance) VaultImport(cmd *cobra.Command, vaultName string) error {
+	err := vault.ValidateVaultName(vaultName)
+	if err != nil {
+		return err
+	}
+
 	release, err := vault.LockStateDir(cli.fs, cli.stateDir)
 	if err != nil {
 		return err
@@ -617,6 +622,11 @@ func (cli *Instance) switchAwayFromVault(
 // RemoveVault removes a vault, holding the state directory lock while
 // removeVault runs
 func (cli *Instance) RemoveVault(cmd *cobra.Command, name string, force bool) error {
+	err := vault.ValidateVaultName(name)
+	if err != nil {
+		return err
+	}
+
 	release, err := vault.LockStateDir(cli.fs, cli.stateDir)
 	if err != nil {
 		return err

internal/vault/errors.go

@@ -17,9 +17,10 @@ var (
 		"derived public key does not match vault: mnemonic may be incorrect",
 	)
 
-	// ErrInvalidVaultName indicates a vault name that does not match the
-	// allowed pattern [a-z0-9.\-_]+. Composed as
-	// "invalid vault name '<name>': must match pattern [a-z0-9.\-_]+".
+	// ErrInvalidVaultName indicates a vault name that breaks the naming
+	// rule: only lowercase ASCII letters, digits, '.', '-' and '_'; not
+	// empty, "." or "..". Composed by ValidateVaultName as
+	// "invalid vault name '<name>': <the rule>".
 	ErrInvalidVaultName = errors.New("invalid vault name")
 
 	// ErrVaultNotFound indicates the named vault does not exist. Composed

internal/vault/management.go

@@ -24,10 +24,12 @@ func init() {
 		})
 }
 
-// isValidVaultName validates vault names according to the format [a-z0-9\.\-\_]+
-// Note: We don't allow slashes in vault names unlike secret names
+// isValidVaultName reports whether name is a valid vault name: only
+// lowercase ASCII letters, digits, '.', '-' and '_', and not empty, "." or
+// "..". With no path separator allowed, a vault is always one directory
+// directly under vaults.d.
 func isValidVaultName(name string) bool {
-	if name == "" {
+	if name == "" || name == "." || name == ".." {
 		return false
 	}
 
@@ -36,6 +38,21 @@ func isValidVaultName(name string) bool {
 	return matched
 }
 
+// ValidateVaultName returns an error wrapping ErrInvalidVaultName when name
+// is not a valid vault name. Call it on the name exactly as the user gave it,
+// before building any path from it.
+func ValidateVaultName(name string) error {
+	if !isValidVaultName(name) {
+		return fmt.Errorf(
+			"%w '%s': only lowercase ASCII letters, digits, '.', '-' and '_' "+
+				"are allowed, and a name must not be empty, '.' or '..'",
+			ErrInvalidVaultName, name,
+		)
+	}
+
+	return nil
+}
+
 // ResolveVaultSymlink reads the currentvault file to get the path to the current vault
 // The file contains just the vault name (e.g., "default")
 func ResolveVaultSymlink(fs afero.Fs, currentVaultPath string) (string, error) {
@@ -199,14 +216,11 @@ func processMnemonicForVault(
 func CreateVault(fs afero.Fs, stateDir string, name string) (*Vault, error) {
 	secret.Debug("Creating new vault", "name", name, "state_dir", stateDir)
 
-	// Validate vault name
-	if !isValidVaultName(name) {
+	err := ValidateVaultName(name)
+	if err != nil {
 		secret.Debug("Invalid vault name provided", "vault_name", name)
 
-		return nil, fmt.Errorf(
-			"%w '%s': must match pattern [a-z0-9.\\-_]+",
-			ErrInvalidVaultName, name,
-		)
+		return nil, err
 	}
 
 	secret.Debug("Vault name validation passed", "vault_name", name)
@@ -285,14 +299,11 @@ func CreateVault(fs afero.Fs, stateDir string, name string) (*Vault, error) {
 func SelectVault(fs afero.Fs, stateDir string, name string) error {
 	secret.Debug("Selecting vault", "vault_name", name, "state_dir", stateDir)
 
-	// Validate vault name
-	if !isValidVaultName(name) {
+	err := ValidateVaultName(name)
+	if err != nil {
 		secret.Debug("Invalid vault name provided", "vault_name", name)
 
-		return fmt.Errorf(
-			"%w '%s': must match pattern [a-z0-9.\\-_]+",
-			ErrInvalidVaultName, name,
-		)
+		return err
 	}
 
 	secret.Debug("Vault name validation passed", "vault_name", name)