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,
the version commands, encrypt and decrypt built paths from the name
without checking it; import checked it only after reading the source
file.

vault.ValidateSecretName wraps the existing name rule and its error
states the rule. Each of those commands calls it on the name as given,
before building any path; a move checks both names before switching
the current vault. AddSecret, GetSecretVersion and GetSecretObject use
it too.

The regression test copies two in-memory vaults for each rejected
command and requires the exact error and an unchanged state directory.

Model: opus-5-5

TODO.md

@@ -28,8 +28,9 @@ 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), `import`, `version list`/`promote`/`rm`,
+  and between vaults, before switching the current vault), `import`,
-  `encrypt` and `decrypt`. Before, `secret rm ..` deleted the whole
+  `version list`/`promote`/`rm`, `encrypt` and `decrypt`. The error
+  states the naming rule. 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,7 +1,10 @@
 package cli_test
 
 import (
+	"maps"
 	"os"
+	"slices"
+	"strings"
 	"testing"
 
 	"git.eeqj.de/sneak/secret/internal/cli"
@@ -90,93 +93,128 @@ func snapshotStateDir(t *testing.T, fs afero.Fs) map[string]string {
 	return tree
 }
 
-// requireRejectedAndUnchanged runs a command against the two test vaults
+// newFsFromSnapshot returns a new in-memory filesystem holding exactly the
-// and requires that it fails with vault.ErrInvalidSecretName and leaves
+// directories and files recorded by snapshotStateDir.
-// everything under the state directory as it was. The error alone proves
+//
-// nothing: it could be returned after the vault had already been deleted.
+//nolint:ireturn // afero.Fs is the filesystem abstraction used throughout
-func requireRejectedAndUnchanged(t *testing.T, run func(c *cli.Instance) error) {
+func newFsFromSnapshot(t *testing.T, tree map[string]string) afero.Fs {
 	t.Helper()
 
-	fs := newTwoVaultFs(t)
+	fs := afero.NewMemMapFs()
-	before := snapshotStateDir(t, fs)
 
-	vaultDir := testStateDir + "/vaults.d/default"
+	// In sorted order every directory comes before its contents.
-	require.Contains(t, before, vaultDir+"/secrets.d/x/")
+	for _, path := range slices.Sorted(maps.Keys(tree)) {
-	require.Contains(t, before, vaultDir+"/unlockers.d/passphrase/")
+		dir, isDir := strings.CutSuffix(path, "/")
+		if isDir {
+			require.NoError(t, fs.MkdirAll(dir, secret.DirPerms))
 
-	err := run(cli.NewCLIInstanceWithStateDir(fs, testStateDir))
+			continue
+		}
 
-	require.Equal(t, before, snapshotStateDir(t, fs))
+		err := afero.WriteFile(fs, path, []byte(tree[path]), secret.FilePerms)
-	require.ErrorIs(t, err, vault.ErrInvalidSecretName)
+		require.NoError(t, err)
+	}
+
+	return fs
 }
 
 // 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.
-// Moves and imports use --force, so that only the name check stands in
+// Each command must fail with exactly the error vault.ValidateSecretName
-// the way.
+// 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.
 //
-//nolint:paralleltest // subtests use t.Setenv via newTwoVaultFs
+//nolint:paralleltest // newTwoVaultFs uses t.Setenv
 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
 	}{
-		{"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", func(c *cli.Instance) error {
+		{"rm ../../etc", "../../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)
 		}},
-		{"mv --force default:.. work", func(c *cli.Instance) error {
+		// "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 {
 			return c.MoveSecret(cmd, "default:..", "work", true)
 		}},
-		{"mv --force default:x work:..", func(c *cli.Instance) error {
+		{"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 {
 			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", func(c *cli.Instance) error {
+		{"import --force ../../etc", "../../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) {
-			requireRejectedAndUnchanged(t, tt.run)
+			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())
 		})
 	}
 }

internal/cli/secrets.go

@@ -747,10 +747,22 @@ 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)
 		}
@@ -828,23 +840,14 @@ func (cli *Instance) moveSecretWithinVault(
 	return nil
 }
 
-// moveSecretCrossVault handles moving between different vaults
+// moveSecretCrossVault handles moving between different vaults. Its caller,
+// MoveSecret, has already checked both secret names.
 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,10 +29,11 @@ var (
 	// ErrNilValueBuffer indicates a nil value buffer was supplied.
 	ErrNilValueBuffer = errors.New("value buffer is nil")
 
-	// ErrInvalidSecretName indicates a secret name that does not match
+	// ErrInvalidSecretName indicates a secret name that breaks the naming
-	// the allowed pattern [a-z0-9.\-_/]+. Composed as
+	// rule: only letters, digits, '.', '-', '_' and '/'; not empty; no
-	// "invalid secret name '<name>': must match pattern [a-z0-9.\-_/]+",
+	// leading '.' or '/', no trailing '/', no '//', no '..' path segment.
-	// or as "invalid secret name: <name>" by GetSecretObject.
+	// Composed by ValidateSecretName as
+	// "invalid secret name '<name>': <the rule>".
 	ErrInvalidSecretName = errors.New("invalid secret name")
 
 	// ErrSecretExists indicates the secret already exists and --force

internal/vault/secrets.go

@@ -79,6 +79,7 @@ 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
@@ -116,7 +117,9 @@ func isValidSecretName(name string) bool {
 func ValidateSecretName(name string) error {
 	if !isValidSecretName(name) {
 		return fmt.Errorf(
-			"%w '%s': must match pattern [a-z0-9.\\-_/]+",
+			"%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",
 			ErrInvalidSecretName, name,
 		)
 	}
@@ -370,8 +373,9 @@ 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) {
-	if !isValidSecretName(name) {
+	err := ValidateSecretName(name)
-		return nil, fmt.Errorf("%w: %s", ErrInvalidSecretName, name)
+	if err != nil {
+		return nil, err
 	}
 
 	// First check if the secret exists by checking for the metadata file