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`,
+  and between vaults), `import`, `version list`/`promote`/`rm`,
-  `version list`/`promote`/`rm`, `encrypt` and `decrypt`. The error
+  `encrypt` and `decrypt`. Before, `secret rm ..` deleted the whole
-  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,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
+// requireRejectedAndUnchanged runs a command against the two test vaults
-// directories and files recorded by snapshotStateDir.
+// and requires that it fails with vault.ErrInvalidSecretName and leaves
-//
+// everything under the state directory as it was. The error alone proves
-//nolint:ireturn // afero.Fs is the filesystem abstraction used throughout
+// nothing: it could be returned after the vault had already been deleted.
-func newFsFromSnapshot(t *testing.T, tree map[string]string) afero.Fs {
+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.
+	vaultDir := testStateDir + "/vaults.d/default"
-	for _, path := range slices.Sorted(maps.Keys(tree)) {
+	require.Contains(t, before, vaultDir+"/secrets.d/x/")
-		dir, isDir := strings.CutSuffix(path, "/")
+	require.Contains(t, before, vaultDir+"/unlockers.d/passphrase/")
-		if isDir {
-			require.NoError(t, fs.MkdirAll(dir, secret.DirPerms))
 
-			continue
+	err := run(cli.NewCLIInstanceWithStateDir(fs, testStateDir))
-		}
 
-		err := afero.WriteFile(fs, path, []byte(tree[path]), secret.FilePerms)
+	require.Equal(t, before, snapshotStateDir(t, fs))
-		require.NoError(t, err)
+	require.ErrorIs(t, err, vault.ErrInvalidSecretName)
-	}
-
-	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.
-// Each command must fail with exactly the error vault.ValidateSecretName
+// Moves and imports use --force, so that only the name check stands in
-// gives for the name, and leave everything under the state directory as it
+// the way.
-// 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 // 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
+		command string
-		rejected string // the secret name the command must reject
+		run     func(c *cli.Instance) error
-		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
+		{"mv --force default:.. work", func(c *cli.Instance) error {
-		// 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:.. work:y", "..", func(c *cli.Instance) error {
+		{"mv --force default:x work:..", 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", "../../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)
+			requireRejectedAndUnchanged(t, tt.run)
-
-			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,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,
+// moveSecretCrossVault handles moving between different vaults
-// 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,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
+	// ErrInvalidSecretName indicates a secret name that does not match
-	// rule: only letters, digits, '.', '-', '_' and '/'; not empty; no
+	// the allowed pattern [a-z0-9.\-_/]+. Composed as
-	// leading '.' or '/', no trailing '/', no '//', no '..' path segment.
+	// "invalid secret name '<name>': must match pattern [a-z0-9.\-_/]+",
-	// Composed by ValidateSecretName as
+	// or as "invalid secret name: <name>" by GetSecretObject.
-	// "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,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, "+
+			"%w '%s': must match pattern [a-z0-9.\\-_/]+",
-				"and a name must not be empty, start with '.' or '/', end with '/', "+
-				"contain '//', or have '..' as a path segment",
 			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 !isValidSecretName(name) {
-	if err != nil {
+		return nil, fmt.Errorf("%w: %s", ErrInvalidSecretName, name)
-		return nil, err
 	}
 
 	// First check if the secret exists by checking for the metadata file