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; its error and
README.md state the rule. Each of those commands calls it on the name
as given, before building any path; MoveSecret checks both names once,
for every form of the move, 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

README.md

@@ -113,7 +113,9 @@ automatically switch to another vault if removing the current one.
 Adds a secret to the current vault. Reads the secret value from stdin.
 - `--force, -f`: Overwrite existing secret
 
-**Secret Name Format:** `[a-z0-9\.\-\_\/]+`
+**Secret Name Format:** 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.
 - Forward slashes (`/`) are converted to percent signs (`%`) for storage
 - Examples: `database/password`, `api.key`, `ssh_private_key`
 

TODO.md

@@ -25,6 +25,13 @@ Bring the repo into policy compliance in one commit:
 
 # Completed Steps
 
+- 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
+  and `README.md` state 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
   lock, and run every case under `script/cibuild`. The image stamps the
@@ -96,8 +103,7 @@ Bring the repo into policy compliance in one commit:
       buffer.Bytes() to GPGEncryptFunc and EncryptWithPassphrase.
     - Race conditions: no file locking in vault/secrets.go:142-176;
       non-atomic writes can leave the vault inconsistent.
-    - Input validation: dots in secret names risk path traversal
-      (vault/secrets.go:75-99); no maximum secret size (DoS).
+    - Input validation: no maximum secret size (DoS).
     - Timing attacks: bytes.Equal passphrase compare (cli/init.go:
       209-216); non-constant-time public key compare (vault.go:95-100).
 - High priority:

internal/cli/crypto.go

@@ -122,6 +122,11 @@ func (cli *Instance) resolveEncryptionKey(
 
 // Encrypt encrypts data using an age secret key stored in a secret
 func (cli *Instance) Encrypt(secretName, inputFile, outputFile string) error {
+	err := vault.ValidateSecretName(secretName)
+	if err != nil {
+		return err
+	}
+
 	// Get current vault
 	vlt, err := vault.GetCurrentVault(cli.fs, cli.stateDir)
 	if err != nil {
@@ -191,6 +196,11 @@ func (cli *Instance) Encrypt(secretName, inputFile, outputFile string) error {
 
 // Decrypt decrypts data using an age secret key stored in a secret
 func (cli *Instance) Decrypt(secretName, inputFile, outputFile string) error {
+	err := vault.ValidateSecretName(secretName)
+	if err != nil {
+		return err
+	}
+
 	// Get current vault
 	vlt, err := vault.GetCurrentVault(cli.fs, cli.stateDir)
 	if err != nil {

internal/cli/path_traversal_test.go

@@ -0,0 +1,232 @@
+package cli_test
+
+import (
+	"maps"
+	"os"
+	"slices"
+	"strings"
+	"testing"
+
+	"git.eeqj.de/sneak/secret/internal/cli"
+	"git.eeqj.de/sneak/secret/internal/secret"
+	"git.eeqj.de/sneak/secret/internal/vault"
+	"github.com/awnumar/memguard"
+	"github.com/spf13/afero"
+	"github.com/spf13/cobra"
+	"github.com/stretchr/testify/require"
+)
+
+const (
+	// testStateDir is the in-memory state directory of the test vaults.
+	testStateDir = "/test/state"
+
+	// testPassphrase protects the passphrase unlocker of each test vault.
+	testPassphrase = "test-passphrase"
+
+	// testVersion is a version name in the format the vault uses.
+	testVersion = "20260101.001"
+
+	// missingFile is an import source that does not exist, so an import
+	// that opened it before checking the name would fail with another error.
+	missingFile = "/no/such/file"
+)
+
+// newTwoVaultFs returns an in-memory filesystem holding the vaults "work"
+// and "default", the current one. Each holds the secret "x" and a
+// passphrase unlocker, so both secrets.d and unlockers.d have contents.
+//
+//nolint:ireturn // afero.Fs is the filesystem abstraction used throughout
+func newTwoVaultFs(t *testing.T) afero.Fs {
+	t.Helper()
+
+	t.Setenv(secret.EnvMnemonic, testMnemonic)
+
+	fs := afero.NewMemMapFs()
+
+	for _, name := range []string{"work", "default"} {
+		vlt, err := vault.CreateVault(fs, testStateDir, name)
+		require.NoError(t, err)
+
+		err = vlt.AddSecret("x", memguard.NewBufferFromBytes([]byte("value")), false)
+		require.NoError(t, err)
+
+		_, err = vlt.CreatePassphraseUnlocker(
+			memguard.NewBufferFromBytes([]byte(testPassphrase)))
+		require.NoError(t, err)
+	}
+
+	return fs
+}
+
+// snapshotStateDir maps every file under the state directory to its
+// contents, and every directory, written with a trailing "/", to "". Two
+// snapshots are equal only if nothing in it was added, removed or changed.
+func snapshotStateDir(t *testing.T, fs afero.Fs) map[string]string {
+	t.Helper()
+
+	tree := map[string]string{}
+
+	err := afero.Walk(fs, testStateDir, func(
+		path string, info os.FileInfo, err error,
+	) error {
+		if err != nil {
+			return err
+		}
+
+		if info.IsDir() {
+			tree[path+"/"] = ""
+
+			return nil
+		}
+
+		content, err := afero.ReadFile(fs, path)
+		if err != nil {
+			return err
+		}
+
+		tree[path] = string(content)
+
+		return nil
+	})
+	require.NoError(t, err)
+
+	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 {
+	t.Helper()
+
+	fs := afero.NewMemMapFs()
+
+	// 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))
+
+			continue
+		}
+
+		err := afero.WriteFile(fs, path, []byte(tree[path]), secret.FilePerms)
+		require.NoError(t, err)
+	}
+
+	return fs
+}
+
+// requireRejectedAndUnchanged runs a command on a copy of the state
+// directory recorded in before. It requires exactly the error
+// vault.ValidateSecretName gives for the rejected name, so that a later
+// check rejecting the name does not count, and everything under the state
+// directory as it was: the error alone proves nothing, since it could come
+// after the vault had already been deleted.
+func requireRejectedAndUnchanged(
+	t *testing.T, before map[string]string, rejected string,
+	run func(c *cli.Instance) error,
+) {
+	t.Helper()
+
+	fs := newFsFromSnapshot(t, before)
+
+	err := run(cli.NewCLIInstanceWithStateDir(fs, testStateDir))
+
+	require.Equal(t, before, snapshotStateDir(t, fs))
+	require.ErrorIs(t, err, vault.ErrInvalidSecretName)
+	require.EqualError(t, err, vault.ValidateSecretName(rejected).Error())
+}
+
+// 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
+// the way.
+//
+//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 {
+			return c.RemoveSecret(cmd, "..", false)
+		}},
+		{"rm .", ".", func(c *cli.Instance) error {
+			return c.RemoveSecret(cmd, ".", false)
+		}},
+		{`rm ""`, "", func(c *cli.Instance) error {
+			return c.RemoveSecret(cmd, "", false)
+		}},
+		{"rm ../../etc", "../../etc", func(c *cli.Instance) error {
+			return c.RemoveSecret(cmd, "../../etc", false)
+		}},
+		{"mv --force .. x", "..", func(c *cli.Instance) error {
+			return c.MoveSecret(cmd, "..", "x", true)
+		}},
+		{"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 {
+			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 {
+			return c.MoveSecret(cmd, "default:x", "work:..", true)
+		}},
+		{"import --force ..", "..", func(c *cli.Instance) error {
+			return c.ImportSecret(cmd, "..", missingFile, true)
+		}},
+		{"import --force .", ".", func(c *cli.Instance) error {
+			return c.ImportSecret(cmd, ".", missingFile, true)
+		}},
+		{"import --force ../../etc", "../../etc", func(c *cli.Instance) error {
+			return c.ImportSecret(cmd, "../../etc", missingFile, true)
+		}},
+		{"version list ..", "..", func(c *cli.Instance) error {
+			return c.ListVersions(cmd, "..")
+		}},
+		{"version promote ..", "..", func(c *cli.Instance) error {
+			return c.PromoteVersion(cmd, "..", testVersion)
+		}},
+		{"version rm ..", "..", func(c *cli.Instance) error {
+			return c.RemoveVersion(cmd, "..", testVersion)
+		}},
+		{"encrypt ..", "..", func(c *cli.Instance) error {
+			return c.Encrypt("..", "", "")
+		}},
+		{"decrypt ..", "..", func(c *cli.Instance) error {
+			return c.Decrypt("..", "", "")
+		}},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.command, func(t *testing.T) {
+			requireRejectedAndUnchanged(t, before, tt.rejected, tt.run)
+		})
+	}
+}

internal/cli/secrets.go

@@ -603,6 +603,11 @@ func printSecretsTable(
 func (cli *Instance) ImportSecret(
 	cmd *cobra.Command, secretName, sourceFile string, force bool,
 ) error {
+	err := vault.ValidateSecretName(secretName)
+	if err != nil {
+		return err
+	}
+
 	// Get current vault
 	vlt, err := vault.GetCurrentVault(cli.fs, cli.stateDir)
 	if err != nil {
@@ -649,6 +654,11 @@ func (cli *Instance) ImportSecret(
 
 // RemoveSecret removes a secret from the vault
 func (cli *Instance) RemoveSecret(cmd *cobra.Command, secretName string, _ bool) error {
+	err := vault.ValidateSecretName(secretName)
+	if err != nil {
+		return err
+	}
+
 	// Get current vault
 	currentVlt, err := vault.GetCurrentVault(cli.fs, cli.stateDir)
 	if err != nil {
@@ -702,13 +712,8 @@ func (cli *Instance) MoveSecret(
 	srcVaultName, srcSecretName, srcQualified := ParseVaultSecretRef(source)
 	destVaultName, destSecretName, destQualified := ParseVaultSecretRef(dest)
 
-	// If neither is qualified, this is a simple within-vault rename
-	if !srcQualified && !destQualified {
-		return cli.moveSecretWithinVault(cmd, srcSecretName, destSecretName, force)
-	}
-
 	// Cross-vault move requires source to be qualified
-	if !srcQualified {
+	if !srcQualified && destQualified {
 		return errCrossVaultSourceUnqualified
 	}
 
@@ -716,31 +721,46 @@ func (cli *Instance) MoveSecret(
 	// Format: "work:secret default" means move to vault "default"
 	// Format: "work:secret default:newname" means move to vault "default"
 	// with a new name
-	if !destQualified {
+	if srcQualified && !destQualified {
 		// Check if dest is actually a vault name
 		vaults, err := vault.ListVaults(cli.fs, cli.stateDir)
 		if err == nil && slices.Contains(vaults, dest) {
 			// dest is a vault name, use source secret name
 			destVaultName = dest
 			destSecretName = srcSecretName
-		}
-
-		// If destVaultName is still empty, dest is a secret name in source vault
-		if destVaultName == "" {
+		} else {
+			// dest is a secret name in source vault
 			destVaultName = srcVaultName
-			destSecretName = dest
 		}
 	}
 
-	// If destination secret name is empty, use source secret name
-	if destSecretName == "" {
+	// If destination secret name is empty, use source secret name. A plain
+	// rename keeps it empty, so that the check below rejects it.
+	if srcQualified && destSecretName == "" {
 		destSecretName = srcSecretName
 	}
 
+	// Check both names, for every form of the move, 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
+	}
+
+	// If neither is qualified, this is a simple within-vault rename
+	if !srcQualified && !destQualified {
+		return cli.moveSecretWithinVault(cmd, srcSecretName, destSecretName, force)
+	}
+
 	// 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)
 		}
@@ -753,7 +773,8 @@ func (cli *Instance) MoveSecret(
 		cmd, srcVaultName, srcSecretName, destVaultName, destSecretName, force)
 }
 
-// moveSecretWithinVault handles rename within the current vault
+// moveSecretWithinVault handles rename within the current vault. Its caller,
+// MoveSecret, has already checked both secret names.
 func (cli *Instance) moveSecretWithinVault(
 	cmd *cobra.Command, source, dest string, force bool,
 ) error {
@@ -808,7 +829,8 @@ 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,

internal/cli/version.go

@@ -112,6 +112,11 @@ func VersionCommands(cli *Instance) *cobra.Command {
 func (cli *Instance) ListVersions(cmd *cobra.Command, secretName string) error {
 	secret.Debug("ListVersions called", "secret_name", secretName)
 
+	err := vault.ValidateSecretName(secretName)
+	if err != nil {
+		return err
+	}
+
 	// Get current vault
 	vlt, err := vault.GetCurrentVault(cli.fs, cli.stateDir)
 	if err != nil {
@@ -239,6 +244,11 @@ func formatVersionTime(t *time.Time) string {
 func (cli *Instance) PromoteVersion(
 	cmd *cobra.Command, secretName string, version string,
 ) error {
+	err := vault.ValidateSecretName(secretName)
+	if err != nil {
+		return err
+	}
+
 	// Get current vault
 	vlt, err := vault.GetCurrentVault(cli.fs, cli.stateDir)
 	if err != nil {
@@ -282,6 +292,11 @@ func (cli *Instance) PromoteVersion(
 func (cli *Instance) RemoveVersion(
 	cmd *cobra.Command, secretName string, version string,
 ) error {
+	err := vault.ValidateSecretName(secretName)
+	if err != nil {
+		return err
+	}
+
 	// Get current vault
 	vlt, err := vault.GetCurrentVault(cli.fs, cli.stateDir)
 	if err != nil {

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
-	// 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 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 = 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
@@ -110,6 +111,22 @@ func isValidSecretName(name string) bool {
 	return matched
 }
 
+// ValidateSecretName returns an error wrapping ErrInvalidSecretName when
+// name is not a valid secret name. Call it on the name exactly as the user
+// gave it, before building any path from it.
+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",
+			ErrInvalidSecretName, name,
+		)
+	}
+
+	return nil
+}
+
 // AddSecret adds a secret to this vault
 func (v *Vault) AddSecret(name string, value *memguard.LockedBuffer, force bool) error {
 	if value == nil {
@@ -124,13 +141,11 @@ func (v *Vault) AddSecret(name string, value *memguard.LockedBuffer, force bool)
 	)
 
 	// Validate secret name
-	if !isValidSecretName(name) {
+	err := ValidateSecretName(name)
+	if err != nil {
 		secret.Debug("Invalid secret name provided", "secret_name", name)
 
-		return fmt.Errorf(
-			"%w '%s': must match pattern [a-z0-9.\\-_/]+",
-			ErrInvalidSecretName, name,
-		)
+		return err
 	}
 
 	secret.Debug("Secret name validation passed", "secret_name", name)
@@ -358,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) {
-		return nil, fmt.Errorf("%w: %s", ErrInvalidSecretName, name)
+	err := ValidateSecretName(name)
+	if err != nil {
+		return nil, err
 	}
 
 	// First check if the secret exists by checking for the metadata file
@@ -628,13 +644,11 @@ func (v *Vault) updatePreviousVersion(
 // version exist, and resolves an empty version to the current one.
 func (v *Vault) resolveSecretVersion(name, version string) (string, error) {
 	// Validate secret name to prevent path traversal
-	if !isValidSecretName(name) {
+	err := ValidateSecretName(name)
+	if err != nil {
 		secret.Debug("Invalid secret name provided", "secret_name", name)
 
-		return "", fmt.Errorf(
-			"%w '%s': must match pattern [a-z0-9.\\-_/]+",
-			ErrInvalidSecretName, name,
-		)
+		return "", err
 	}
 
 	// Get vault directory