Reject invalid secret names before any command builds a path (closes #33)

`secret rm ..` resolved to the vault directory and deleted the whole
vault; `secret rm .` and `secret rm ""` deleted every secret. rm, mv and
import built paths from the name without checking it, and so did the
version commands, encrypt and decrypt.

vault.ValidateSecretName wraps the existing name rule and returns
ErrInvalidSecretName. Each of those commands calls it on the name as
given, both names for a move, before building any path. AddSecret and
GetSecretVersion use it too, so the rule has one implementation.

The regression test snapshots every file under the state directory of
two in-memory vaults and requires it unchanged after each rejected
command.

Model: opus-5-5

TODO.md

@@ -28,9 +28,8 @@ Bring the repo into policy compliance in one commit:
 - 2026-10-03: Every command that builds a path from a secret name
   checks the name first with `vault.ValidateSecretName` and touches
   nothing when it is invalid: `rm`, `mv` (both names, within a vault
-  and between vaults, before switching the current vault), `import`,
-  `version list`/`promote`/`rm`, `encrypt` and `decrypt`. The error
-  states the naming rule. Before, `secret rm ..` deleted the whole
+  and between vaults), `import`, `version list`/`promote`/`rm`,
+  `encrypt` and `decrypt`. Before, `secret rm ..` deleted the whole
   vault and `secret rm .` every secret in it.
 - 2026-10-02: A plain `docker build .` builds again: the size tests
   skip a case that needs more locked memory than the process can

internal/cli/path_traversal_test.go

@@ -1,10 +1,7 @@
 package cli_test
 
 import (
-	"maps"
 	"os"
-	"slices"
-	"strings"
 	"testing"
 
 	"git.eeqj.de/sneak/secret/internal/cli"
@@ -93,128 +90,93 @@ func snapshotStateDir(t *testing.T, fs afero.Fs) map[string]string {
 	return tree
 }
 
-// newFsFromSnapshot returns a new in-memory filesystem holding exactly the
-// directories and files recorded by snapshotStateDir.
-//
-//nolint:ireturn // afero.Fs is the filesystem abstraction used throughout
-func newFsFromSnapshot(t *testing.T, tree map[string]string) afero.Fs {
+// requireRejectedAndUnchanged runs a command against the two test vaults
+// and requires that it fails with vault.ErrInvalidSecretName and leaves
+// everything under the state directory as it was. The error alone proves
+// nothing: it could be returned after the vault had already been deleted.
+func requireRejectedAndUnchanged(t *testing.T, run func(c *cli.Instance) error) {
 	t.Helper()
 
-	fs := afero.NewMemMapFs()
+	fs := newTwoVaultFs(t)
+	before := snapshotStateDir(t, fs)
 
-	// In sorted order every directory comes before its contents.
-	for _, path := range slices.Sorted(maps.Keys(tree)) {
-		dir, isDir := strings.CutSuffix(path, "/")
-		if isDir {
-			require.NoError(t, fs.MkdirAll(dir, secret.DirPerms))
+	vaultDir := testStateDir + "/vaults.d/default"
+	require.Contains(t, before, vaultDir+"/secrets.d/x/")
+	require.Contains(t, before, vaultDir+"/unlockers.d/passphrase/")
 
-			continue
-		}
+	err := run(cli.NewCLIInstanceWithStateDir(fs, testStateDir))
 
-		err := afero.WriteFile(fs, path, []byte(tree[path]), secret.FilePerms)
-		require.NoError(t, err)
-	}
-
-	return fs
+	require.Equal(t, before, snapshotStateDir(t, fs))
+	require.ErrorIs(t, err, vault.ErrInvalidSecretName)
 }
 
 // TestInvalidSecretNameLeavesVaultsUnchanged is a regression test for
 // https://git.eeqj.de/sneak/secret/issues/33, where `secret rm ..` deleted
 // the whole vault, and `secret rm .` or `secret rm ""` every secret in it.
-// Each command must fail with exactly the error vault.ValidateSecretName
-// gives for the name, and leave everything under the state directory as it
-// was: the error alone proves nothing, since it could come after the vault
-// had already been deleted. Moves and imports use --force, so that only
-// the name check stands in the way.
+// Moves and imports use --force, so that only the name check stands in
+// the way.
 //
-//nolint:paralleltest // newTwoVaultFs uses t.Setenv
+//nolint:paralleltest // subtests use t.Setenv via newTwoVaultFs
 func TestInvalidSecretNameLeavesVaultsUnchanged(t *testing.T) {
-	// Creating a passphrase unlocker is slow by design, so the vaults are
-	// created once and each case runs on its own copy of them.
-	before := snapshotStateDir(t, newTwoVaultFs(t))
-
-	vaultDir := testStateDir + "/vaults.d/default"
-	require.Contains(t, before, vaultDir+"/secrets.d/x/")
-	require.Contains(t, before, vaultDir+"/unlockers.d/passphrase/")
-	require.Equal(t, "default", before[testStateDir+"/currentvault"])
-
 	cmd := &cobra.Command{}
 
 	tests := []struct {
-		command  string
-		rejected string // the secret name the command must reject
-		run      func(c *cli.Instance) error
+		command string
+		run     func(c *cli.Instance) error
 	}{
-		{"rm ..", "..", func(c *cli.Instance) error {
+		{"rm ..", func(c *cli.Instance) error {
 			return c.RemoveSecret(cmd, "..", false)
 		}},
-		{"rm .", ".", func(c *cli.Instance) error {
+		{"rm .", func(c *cli.Instance) error {
 			return c.RemoveSecret(cmd, ".", false)
 		}},
-		{`rm ""`, "", func(c *cli.Instance) error {
+		{`rm ""`, func(c *cli.Instance) error {
 			return c.RemoveSecret(cmd, "", false)
 		}},
-		{"rm ../../etc", "../../etc", func(c *cli.Instance) error {
+		{"rm ../../etc", func(c *cli.Instance) error {
 			return c.RemoveSecret(cmd, "../../etc", false)
 		}},
-		{"mv --force .. x", "..", func(c *cli.Instance) error {
+		{"mv --force .. x", func(c *cli.Instance) error {
 			return c.MoveSecret(cmd, "..", "x", true)
 		}},
-		{"mv --force x ..", "..", func(c *cli.Instance) error {
+		{"mv --force x ..", func(c *cli.Instance) error {
 			return c.MoveSecret(cmd, "x", "..", true)
 		}},
-		// "work" is not the current vault: a move within it must not
-		// select it when a name is rejected.
-		{"mv --force work:.. work:x", "..", func(c *cli.Instance) error {
-			return c.MoveSecret(cmd, "work:..", "work:x", true)
-		}},
-		{"mv --force work:x work:..", "..", func(c *cli.Instance) error {
-			return c.MoveSecret(cmd, "work:x", "work:..", true)
-		}},
-		{"mv --force default:.. work", "..", func(c *cli.Instance) error {
+		{"mv --force default:.. work", func(c *cli.Instance) error {
 			return c.MoveSecret(cmd, "default:..", "work", true)
 		}},
-		{"mv --force default:.. work:y", "..", func(c *cli.Instance) error {
-			return c.MoveSecret(cmd, "default:..", "work:y", true)
-		}},
-		{"mv --force default:x work:..", "..", func(c *cli.Instance) error {
+		{"mv --force default:x work:..", func(c *cli.Instance) error {
 			return c.MoveSecret(cmd, "default:x", "work:..", true)
 		}},
-		{"import --force ..", "..", func(c *cli.Instance) error {
+		{"import --force ..", func(c *cli.Instance) error {
 			return c.ImportSecret(cmd, "..", missingFile, true)
 		}},
-		{"import --force .", ".", func(c *cli.Instance) error {
+		{"import --force .", func(c *cli.Instance) error {
 			return c.ImportSecret(cmd, ".", missingFile, true)
 		}},
-		{"import --force ../../etc", "../../etc", func(c *cli.Instance) error {
+		{"import --force ../../etc", func(c *cli.Instance) error {
 			return c.ImportSecret(cmd, "../../etc", missingFile, true)
 		}},
-		{"version list ..", "..", func(c *cli.Instance) error {
+		{"version list ..", func(c *cli.Instance) error {
 			return c.ListVersions(cmd, "..")
 		}},
-		{"version promote ..", "..", func(c *cli.Instance) error {
+		{"version promote ..", func(c *cli.Instance) error {
 			return c.PromoteVersion(cmd, "..", testVersion)
 		}},
-		{"version rm ..", "..", func(c *cli.Instance) error {
+		{"version rm ..", func(c *cli.Instance) error {
 			return c.RemoveVersion(cmd, "..", testVersion)
 		}},
-		{"encrypt ..", "..", func(c *cli.Instance) error {
+		{"encrypt ..", func(c *cli.Instance) error {
 			return c.Encrypt("..", "", "")
 		}},
-		{"decrypt ..", "..", func(c *cli.Instance) error {
+		{"decrypt ..", func(c *cli.Instance) error {
 			return c.Decrypt("..", "", "")
 		}},
 	}
 
 	for _, tt := range tests {
 		t.Run(tt.command, func(t *testing.T) {
-			fs := newFsFromSnapshot(t, before)
-
-			err := tt.run(cli.NewCLIInstanceWithStateDir(fs, testStateDir))
-
-			require.Equal(t, before, snapshotStateDir(t, fs))
-			require.ErrorIs(t, err, vault.ErrInvalidSecretName)
-			require.EqualError(t, err, vault.ValidateSecretName(tt.rejected).Error())
+			requireRejectedAndUnchanged(t, tt.run)
 		})
 	}
 }

internal/cli/secrets.go

@@ -747,22 +747,10 @@ func (cli *Instance) MoveSecret(
 		destSecretName = srcSecretName
 	}
 
-	// Check both names before selecting a vault below, so that a rejected
-	// move leaves the current vault as it was.
-	err := vault.ValidateSecretName(srcSecretName)
-	if err != nil {
-		return err
-	}
-
-	err = vault.ValidateSecretName(destSecretName)
-	if err != nil {
-		return err
-	}
-
 	// Same vault? Use simple rename if possible (optimization)
 	if srcVaultName == destVaultName {
 		// Select the vault and do a simple move
-		err = vault.SelectVault(cli.fs, cli.stateDir, srcVaultName)
+		err := vault.SelectVault(cli.fs, cli.stateDir, srcVaultName)
 		if err != nil {
 			return fmt.Errorf("failed to select vault '%s': %w", srcVaultName, err)
 		}
@@ -840,14 +828,23 @@ func (cli *Instance) moveSecretWithinVault(
 	return nil
 }
 
-// moveSecretCrossVault handles moving between different vaults. Its caller,
-// MoveSecret, has already checked both secret names.
+// moveSecretCrossVault handles moving between different vaults
 func (cli *Instance) moveSecretCrossVault(
 	cmd *cobra.Command,
 	srcVaultName, srcSecretName,
 	destVaultName, destSecretName string,
 	force bool,
 ) error {
+	err := vault.ValidateSecretName(srcSecretName)
+	if err != nil {
+		return err
+	}
+
+	err = vault.ValidateSecretName(destSecretName)
+	if err != nil {
+		return err
+	}
+
 	// Get source vault
 	srcVault := vault.NewVault(cli.fs, cli.stateDir, srcVaultName)
 

internal/vault/errors.go

@@ -29,11 +29,10 @@ var (
 	// ErrNilValueBuffer indicates a nil value buffer was supplied.
 	ErrNilValueBuffer = errors.New("value buffer is nil")
 
-	// ErrInvalidSecretName indicates a secret name that breaks the naming
-	// rule: only letters, digits, '.', '-', '_' and '/'; not empty; no
-	// leading '.' or '/', no trailing '/', no '//', no '..' path segment.
-	// Composed by ValidateSecretName as
-	// "invalid secret name '<name>': <the rule>".
+	// ErrInvalidSecretName indicates a secret name that does not match
+	// the allowed pattern [a-z0-9.\-_/]+. Composed as
+	// "invalid secret name '<name>': must match pattern [a-z0-9.\-_/]+",
+	// or as "invalid secret name: <name>" by GetSecretObject.
 	ErrInvalidSecretName = errors.New("invalid secret name")
 
 	// ErrSecretExists indicates the secret already exists and --force

internal/vault/secrets.go

@@ -79,7 +79,6 @@ func (v *Vault) ListSecrets() ([]string, error) {
 // - No leading or trailing slashes
 // - No double slashes
 // - No names starting with dots
-// - No ".." path segments
 func isValidSecretName(name string) bool {
 	if name == "" {
 		return false
@@ -117,9 +116,7 @@ func isValidSecretName(name string) bool {
 func ValidateSecretName(name string) error {
 	if !isValidSecretName(name) {
 		return fmt.Errorf(
-			"%w '%s': only letters, digits, '.', '-', '_' and '/' are allowed, "+
-				"and a name must not be empty, start with '.' or '/', end with '/', "+
-				"contain '//', or have '..' as a path segment",
+			"%w '%s': must match pattern [a-z0-9.\\-_/]+",
 			ErrInvalidSecretName, name,
 		)
 	}
@@ -373,9 +370,8 @@ func (v *Vault) UnlockVault() (*age.X25519Identity, error) {
 
 // GetSecretObject retrieves a Secret object with metadata loaded from this vault
 func (v *Vault) GetSecretObject(name string) (*secret.Secret, error) {
-	err := ValidateSecretName(name)
-	if err != nil {
-		return nil, err
+	if !isValidSecretName(name) {
+		return nil, fmt.Errorf("%w: %s", ErrInvalidSecretName, name)
 	}
 
 	// First check if the secret exists by checking for the metadata file