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` and both
vault names of `mv` 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/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