Lock the state directory and write vault files atomically (closes #34)

Each command that changes the state directory holds one lock: flock(2)
on `lock` in the state directory, dropped by the kernel if the process
dies, or a process-wide mutex on the in-memory test filesystem. It
covers the state directory, not each vault, because `currentvault`,
`vault create` and cross-vault moves span vaults, and a lock file in a
vault would be deleted by `vault remove` under a waiting command.

Files go through `secret.WriteFileAtomic`; versions, new secrets and
cross-vault copies are built in a temporary directory and renamed into
place; removals rename out of the way first. An unlocker added under
an existing unlocker's directory name is still rewritten file by file:
#71.

Model: opus-5-5

README.md

@@ -113,9 +113,7 @@ 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:** only ASCII letters, digits, `.`, `-`, `_` and `/`
-are allowed, and a name must not be empty, start with `.` or `/`, end with
-`/`, contain `//`, or have `..` as a path segment.
+**Secret Name Format:** `[a-z0-9\.\-\_\/]+`
 - Forward slashes (`/`) are converted to percent signs (`%`) for storage
 - Examples: `database/password`, `api.key`, `ssh_private_key`
 

TODO.md

@@ -25,69 +25,21 @@ Bring the repo into policy compliance in one commit:
 
 # Completed Steps
 
-- 2026-10-03: `secret mv` rejects a move whose destination is the
-  source (`mv --force x x`, `mv --force work:x work:`, or an empty
-  destination, which defaults to the source name) before changing
-  anything; before, `--force` removed the destination first and so
-  deleted the secret. Every vault name given with `vault:` must be one
-  of the existing vaults by exact name, so `work:x work/:x` is rejected
-  instead of being taken for a move between two vaults. A move within a
-  named vault no longer makes that vault the current one, whether it
-  succeeds or fails.
 - 2026-10-03: Commands that change the state directory hold one lock
   (`flock` on `lock` in the state directory; a mutex on the in-memory
   test filesystem), so concurrent commands no longer lose versions or
   race on the current pointers. Every file is written through
-  `secret.WriteFileAtomic` (temporary file, sync, rename), so no file
-  is ever half-written and `current`, `currentvault` and
-  `current-unlocker` never go missing. New versions, new secrets and
-  cross-vault copies are built in a temporary directory and renamed
-  into place, and removals rename out of the way first, so a version
-  or secret is never half-added and never half-removed. An
-  interrupted command can still leave:
-    - a broken unlocker, when it was replacing one: an unlocker added
-      under the directory name of an existing one is rewritten file by
-      file. That happens to a passphrase unlocker added to a vault that
-      has one, and to a PGP, keychain or Secure Enclave unlocker added
-      on the same host and day as another of its type
-      (https://git.eeqj.de/sneak/secret/issues/71);
-    - from `vault create` stopped at the passphrase prompt, a new vault
-      with no unlocker that is already the current vault; from `init`
-      stopped there, the default vault with no unlocker;
-    - from an unlocker add stopped before its metadata is written, a
-      directory that `unlocker list` warns about and `unlocker rm`
-      cannot remove;
-    - data under a `.tmp-` name in the state directory: a secret or
-      version being added, or the secret, version, unlocker or vault
-      being removed, encrypted keys included. Nothing deletes it; it
-      must be deleted by hand
-      (https://git.eeqj.de/sneak/secret/issues/75).
-- 2026-10-03: `version rm`, `version promote` and `get --version`
-  accept a version only if it is one of the versions `version list`
-  lists for that secret, compared as typed before any path is built
-  (`secret.VersionExists`), and touch nothing otherwise. An empty
-  `--version` is rejected instead of meaning the current version.
-  Before, `secret version rm x ../../..` deleted the whole vault,
-  `secret version rm x ..` the secret, and `.` or `""` every version.
-- 2026-10-03: Key material is wiped on every exit: `Entry()` returns
-  the exit code after its deferred `memguard.Purge()` has run, and only
-  `main` calls `os.Exit`. SIGINT and SIGTERM go through memguard's
-  handler, which wipes every buffer before exiting; when the process is
-  in the terminal's foreground process group it first restores the
-  terminal settings from startup, so an interrupted passphrase prompt no
-  longer leaves echo off.
-- 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-03: The keychain unlocker's age key passphrase stays in
-  locked memory: it is generated into a locked buffer, and the
-  keychain JSON is written and read by `KeychainData` code in
-  `internal/secret/keychaindata.go` (tested on Linux) without
-  `encoding/json` holding it; the JSON field names are unchanged.
+  `secret.WriteFileAtomic` (temporary file, sync, rename); new
+  versions, new secrets and cross-vault copies are built in a
+  temporary directory and renamed into place, and removals rename out
+  of the way first, so an interrupted command leaves nothing
+  half-written, with one exception: an unlocker added under the
+  directory name of an existing one is rewritten in place, file by
+  file, and a crash part-way leaves it unable to open the vault. That
+  happens to a passphrase unlocker added to a vault that has one, and
+  to a PGP, keychain or Secure Enclave unlocker added on the same host
+  and day as another of its type
+  (https://git.eeqj.de/sneak/secret/issues/71).
 - 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
@@ -151,12 +103,14 @@ Bring the repo into policy compliance in one commit:
     - Command injection: GPG key IDs passed unescaped to exec.Command
       (pgpunlocker.go:323-327); data.String() passed unescaped to the
       security command (keychainunlocker.go:472-476).
-    - Memory security: age identity .String() creates unprotected
-      copies (keychainunlocker.go:356, pgpunlocker.go:256,
-      version.go:155); age secret key held in a plain string in
-      cli/crypto.go:86,91,113; private keys exposed via buffer.Bytes()
-      to GPGEncryptFunc and EncryptWithPassphrase.
-    - Input validation: no maximum secret size (DoS).
+    - Memory security: KeychainData stores AgePrivKeyPassphrase as a
+      plain string (keychainunlocker.go:342,393-396); age identity
+      .String() creates unprotected copies (keychainunlocker.go:356,
+      pgpunlocker.go:256, version.go:155); age secret key held in a
+      plain string in cli/crypto.go:86,91,113; private keys exposed via
+      buffer.Bytes() to GPGEncryptFunc and EncryptWithPassphrase.
+    - Input validation: dots in secret names risk path traversal
+      (vault/secrets.go:75-99); 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:

cmd/secret/main.go

@@ -1,12 +1,8 @@
 // Package main is the entry point for the secret CLI application.
 package main
 
-import (
-	"os"
-
-	"git.eeqj.de/sneak/secret/internal/cli"
-)
+import "git.eeqj.de/sneak/secret/internal/cli"
 
 func main() {
-	os.Exit(cli.Entry())
+	cli.Entry()
 }

internal/cli/crypto.go

@@ -70,11 +70,12 @@ func newDecryptCmd() *cobra.Command {
 	)
 }
 
-// storeNewEncryptionKey generates an age secret key and stores it as the
-// named secret, holding the state directory lock while it does. It fails
-// with vault.ErrSecretExists if another command stored the secret first.
-// The caller must destroy the returned buffer.
-func (cli *Instance) storeNewEncryptionKey(
+// resolveEncryptionKey returns a secure buffer holding the age secret key
+// for the named secret, generating and storing a new key if the secret
+// does not exist. The caller must destroy the returned buffer. It holds the
+// state directory lock itself, so that Encrypt streams its input and output
+// unlocked and cannot block a secret command at the other end of a pipe.
+func (cli *Instance) resolveEncryptionKey(
 	vlt *vault.Vault, secretName string,
 ) (*memguard.LockedBuffer, error) {
 	release, err := vault.LockStateDir(cli.fs, cli.stateDir)
@@ -83,34 +84,6 @@ func (cli *Instance) storeNewEncryptionKey(
 	}
 	defer release()
 
-	identity, err := age.GenerateX25519Identity()
-	if err != nil {
-		return nil, fmt.Errorf("failed to generate age key: %w", err)
-	}
-
-	// Store the generated key directly in a secure buffer
-	secureBuffer := memguard.NewBufferFromBytes([]byte(identity.String()))
-
-	err = vlt.AddSecret(secretName, secureBuffer, false)
-	if err != nil {
-		secureBuffer.Destroy()
-
-		return nil, fmt.Errorf("failed to store age key: %w", err)
-	}
-
-	return secureBuffer, nil
-}
-
-// resolveEncryptionKey returns a secure buffer holding the age secret key
-// for the named secret, generating and storing a new key if the secret
-// does not exist. The caller must destroy the returned buffer. Only storing
-// a new key takes the state directory lock, so that reading an existing key
-// works on a read-only state directory and keeps no other command waiting
-// at the passphrase prompt, and Encrypt streams its input and output
-// unlocked.
-func (cli *Instance) resolveEncryptionKey(
-	vlt *vault.Vault, secretName string,
-) (*memguard.LockedBuffer, error) {
 	// Check if secret exists
 	secretObj := secret.NewSecret(vlt, secretName)
 
@@ -120,11 +93,23 @@ func (cli *Instance) resolveEncryptionKey(
 	}
 
 	if !exists {
-		key, err := cli.storeNewEncryptionKey(vlt, secretName)
-		if !errors.Is(err, vault.ErrSecretExists) {
-			return key, err
+		// Secret doesn't exist, generate new age key and store it
+		identity, err := age.GenerateX25519Identity()
+		if err != nil {
+			return nil, fmt.Errorf("failed to generate age key: %w", err)
 		}
-		// Another command stored the key since the check above: read it
+
+		// Store the generated key directly in a secure buffer
+		secureBuffer := memguard.NewBufferFromBytes([]byte(identity.String()))
+
+		err = vlt.AddSecret(secretName, secureBuffer, false)
+		if err != nil {
+			secureBuffer.Destroy()
+
+			return nil, fmt.Errorf("failed to store age key: %w", err)
+		}
+
+		return secureBuffer, nil
 	}
 
 	// Secret exists, get the age secret key from it
@@ -145,11 +130,6 @@ 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 {
@@ -219,11 +199,6 @@ 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/entry_test.go

@@ -1,108 +0,0 @@
-package cli_test
-
-import (
-	"bufio"
-	"context"
-	"os"
-	"os/exec"
-	"path/filepath"
-	"strings"
-	"testing"
-	"time"
-
-	"git.eeqj.de/sneak/secret/internal/cli"
-	"git.eeqj.de/sneak/secret/internal/secret"
-	"github.com/awnumar/memguard"
-	"github.com/stretchr/testify/assert"
-	"github.com/stretchr/testify/require"
-)
-
-// Entry must return its exit code rather than exit, so that its deferred
-// memguard purge runs on the success and the error path alike.
-//
-//nolint:paralleltest // sets os.Args, and Entry wipes every buffer in the process
-func TestEntryWipesBuffersAndReturnsExitCode(t *testing.T) {
-	savedArgs := os.Args
-
-	t.Cleanup(func() { os.Args = savedArgs })
-
-	tests := []struct {
-		args     []string
-		exitCode int
-	}{
-		{args: []string{"secret", "--help"}, exitCode: 0},
-		{args: []string{"secret", "no-such-command"}, exitCode: 1},
-	}
-
-	for _, tt := range tests {
-		buf := memguard.NewBufferFromBytes([]byte("key material"))
-		os.Args = tt.args
-
-		assert.Equal(t, tt.exitCode, cli.Entry(), "exit code for %v", tt.args)
-		assert.False(t, buf.IsAlive(), "Entry left a buffer unwiped for %v", tt.args)
-	}
-}
-
-// Ctrl-C while `secret add` waits for the value on stdin must end the
-// process through memguard's signal handler, which wipes every buffer and
-// exits with status 1, not through Go's default handling, which kills the
-// process with the buffers intact.
-func TestInterruptExitsThroughMemguard(t *testing.T) {
-	t.Parallel()
-
-	const waitingForValue = "Reading secret value from stdin"
-
-	ctx, cancel := context.WithTimeout(t.Context(), time.Minute)
-	defer cancel()
-
-	wd, err := filepath.Abs("../..")
-	require.NoError(t, err)
-
-	secretPath := filepath.Join(wd, "secret")
-	env := []string{
-		secret.EnvStateDir + "=" + t.TempDir(),
-		secret.EnvMnemonic + "=" + testMnemonic,
-		secret.EnvUnlockPassphrase + "=test-passphrase",
-		"PATH=/usr/bin:/bin",
-		// The debug log on stderr shows when add starts waiting for the value.
-		"GODEBUG=berlin.sneak.pkg.secret",
-	}
-
-	//nolint:gosec // G204: test executes the freshly built secret binary
-	initCmd := exec.CommandContext(ctx, secretPath, "init")
-	initCmd.Env = env
-
-	output, err := initCmd.CombinedOutput()
-	require.NoError(t, err, "init should succeed: %s", output)
-
-	//nolint:gosec // G204: test executes the freshly built secret binary
-	addCmd := exec.CommandContext(ctx, secretPath, "add", "test/secret")
-	addCmd.Env = env
-
-	// Held open and never written, so add keeps waiting for the value.
-	stdin, err := addCmd.StdinPipe()
-	require.NoError(t, err)
-
-	defer func() { _ = stdin.Close() }()
-
-	stderr, err := addCmd.StderrPipe()
-	require.NoError(t, err)
-	require.NoError(t, addCmd.Start())
-
-	waiting := false
-
-	scanner := bufio.NewScanner(stderr)
-	for !waiting && scanner.Scan() {
-		waiting = strings.Contains(scanner.Text(), waitingForValue)
-	}
-
-	require.True(t, waiting, "add never logged %q", waitingForValue)
-	require.NoError(t, addCmd.Process.Signal(os.Interrupt))
-
-	err = addCmd.Wait()
-
-	var exitErr *exec.ExitError
-
-	require.ErrorAs(t, err, &exitErr)
-	assert.Equal(t, 1, exitErr.ExitCode(), "add ended with %v", err)
-}

internal/cli/integration_test.go

@@ -829,14 +829,6 @@ func test09GetSpecificVersion(t *testing.T, tempDir, testMnemonic string, runSec
 
 	require.NoError(t, err, "get current version should succeed")
 	assert.Equal(t, "newpassword456", strings.TrimSpace(output), "should return new secret value without --version")
-
-	// An empty --version is not a version; it does not mean the current one
-	output, err = runSecretWithEnv(map[string]string{
-		secret.EnvMnemonic: testMnemonic,
-	}, "get", "--version", "", "database/password")
-
-	require.Error(t, err, "get with an empty version should fail")
-	assert.Contains(t, output, "version '' not found", "should reject the empty version")
 }
 
 func test10PromoteVersion(t *testing.T, tempDir, testMnemonic string, runSecret func(...string) (string, error), runSecretWithEnv func(map[string]string, ...string) (string, error)) {

internal/cli/lock_test.go

@@ -3,7 +3,6 @@ package cli
 
 import (
 	"io"
-	"os"
 	"path/filepath"
 	"strconv"
 	"strings"
@@ -13,47 +12,12 @@ import (
 
 	"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/assert"
 	"github.com/stretchr/testify/require"
 )
 
-const (
-	// lockWait is how long a test waits for something that must happen
-	// once the lock is free.
-	lockWait = 10 * time.Second
-
-	// heldWait is how long a test watches a command that must wait for the
-	// lock. A command that takes no lock changes the state directory well
-	// within it.
-	heldWait = 100 * time.Millisecond
-
-	// testPassphrase protects the passphrase unlockers the tests create.
-	testPassphrase = "test-passphrase"
-
-	// testInput is a file outside the state directory that commands read.
-	testInput = "/input"
-)
-
-// lockInBackground starts taking the state directory lock and returns a
-// channel that delivers the function releasing it once it has been taken.
-func lockInBackground(t *testing.T, fs afero.Fs) <-chan func() {
-	t.Helper()
-
-	taken := make(chan func(), 1)
-
-	go func() {
-		release, err := vault.LockStateDir(fs, testStateDir)
-		if assert.NoError(t, err) {
-			taken <- release
-		}
-	}()
-
-	return taken
-}
-
 // addAtOnce runs one add of the secret name per value, all at once, and
 // returns their errors.
 func addAtOnce(
@@ -184,7 +148,7 @@ func TestEncryptPipedIntoAdd(t *testing.T) {
 	fs := afero.NewMemMapFs()
 	_, err := vault.CreateVault(fs, testStateDir, "default")
 	require.NoError(t, err)
-	require.NoError(t, afero.WriteFile(fs, testInput, []byte("piped"), 0o600))
+	require.NoError(t, afero.WriteFile(fs, "/plaintext", []byte("piped"), 0o600))
 
 	pipeReader, pipeWriter := io.Pipe()
 	// If the test gives up, this makes add's read fail, so that both
@@ -211,14 +175,14 @@ func TestEncryptPipedIntoAdd(t *testing.T) {
 		encrypt.cmd = &cobra.Command{}
 		encrypt.cmd.SetOut(pipeWriter)
 
-		err := encrypt.Encrypt("key", testInput, "")
+		err := encrypt.Encrypt("key", "/plaintext", "")
 		// Ends add's input, as the end of the pipe does
 		_ = pipeWriter.CloseWithError(err)
 
 		results <- err
 	}()
 
-	timeout := time.After(lockWait)
+	timeout := time.After(10 * time.Second)
 
 	for range commands {
 		select {
@@ -242,288 +206,19 @@ func TestFailedCommandReleasesLock(t *testing.T) {
 	err := cli.RemoveSecret(&cobra.Command{}, "missing", false)
 	require.Error(t, err)
 
-	select {
-	case release := <-lockInBackground(t, fs):
-		release()
-	case <-time.After(lockWait):
-		t.Fatal("the failed command left the state directory locked")
-	}
-}
-
-// stateDirModTimes returns the modification time of every file and
-// directory under the test state directory. Any change a command makes, even
-// rewriting a file with the same content, changes it.
-func stateDirModTimes(t *testing.T, fs afero.Fs) map[string]int64 {
-	t.Helper()
-
-	modTimes := make(map[string]int64)
-
-	err := afero.Walk(fs, testStateDir,
-		func(path string, info os.FileInfo, err error) error {
-			if err != nil {
-				return err
-			}
-
-			modTimes[path] = info.ModTime().UnixNano()
-
-			return nil
-		})
-	require.NoError(t, err)
-
-	return modTimes
-}
-
-// setupEveryCommand makes what each command in
-// TestChangingCommandsWaitForLock needs: the current vault "default" with
-// two versions of "test/secret", the vault "other" without a long-term key,
-// for vault import, and the file testInput. If withUnlocker is set, it also
-// gives "default" a passphrase unlocker, which is slow. It returns the older
-// version and the unlocker's ID.
-func setupEveryCommand(
-	t *testing.T, fs afero.Fs, withUnlocker bool,
-) (string, string) {
-	t.Helper()
-
-	other, err := vault.CreateVault(fs, testStateDir, "other")
-	require.NoError(t, err)
-
-	otherDir, err := other.GetDirectory()
-	require.NoError(t, err)
-	require.NoError(t, fs.Remove(filepath.Join(otherDir, "pub.age")))
-
-	vlt, err := vault.CreateVault(fs, testStateDir, "default")
-	require.NoError(t, err)
-
-	addTestSecret(t, vlt, []byte("older"), false)
-	addTestSecret(t, vlt, []byte("newer"), true)
-
-	vaultDir, err := vlt.GetDirectory()
-	require.NoError(t, err)
-
-	versions, err := secret.ListVersions(fs,
-		filepath.Join(vaultDir, "secrets.d", "test%secret"))
-	require.NoError(t, err)
-	require.Len(t, versions, 2)
-
-	unlockerID := ""
-
-	if withUnlocker {
-		passphrase := memguard.NewBufferFromBytes([]byte(testPassphrase))
-		defer passphrase.Destroy()
-
-		unlocker, err := vlt.CreatePassphraseUnlocker(passphrase)
-		require.NoError(t, err)
-
-		unlockerID = unlocker.GetID()
-	}
-
-	require.NoError(t, afero.WriteFile(fs, testInput, []byte("input"), 0o600))
-
-	// Newest first
-	return versions[1], unlockerID
-}
-
-// requireWaitsForLock runs a command, given what setupEveryCommand made,
-// while holding the state directory lock. The command must neither finish
-// nor change anything while the lock is held, and must succeed once it is
-// released.
-func requireWaitsForLock(
-	t *testing.T,
-	withUnlocker bool,
-	run func(cli *Instance, olderVersion, unlockerID string) error,
-) {
-	t.Helper()
-
-	fs := afero.NewMemMapFs()
-	olderVersion, unlockerID := setupEveryCommand(t, fs, withUnlocker)
-	before := stateDirModTimes(t, fs)
-
-	release, err := vault.LockStateDir(fs, testStateDir)
-	require.NoError(t, err)
-
-	// Released at most once, and also if the test fails while holding it,
-	// so that later tests can take it
-	release = sync.OnceFunc(release)
-	defer release()
-
-	cli := NewCLIInstanceWithStateDir(fs, testStateDir)
-	cli.cmd = &cobra.Command{}
-	cli.cmd.SetIn(strings.NewReader("value"))
-	cli.cmd.SetOut(io.Discard)
-
-	done := make(chan error, 1)
-
-	go func() { done <- run(cli, olderVersion, unlockerID) }()
-
-	select {
-	case err := <-done:
-		t.Fatalf("finished while the lock was held, with error %v", err)
-	case <-time.After(heldWait):
-	}
-
-	assert.Equal(t, before, stateDirModTimes(t, fs),
-		"changed the state directory while the lock was held")
-
-	release()
-
-	select {
-	case err := <-done:
-		require.NoError(t, err)
-	case <-time.After(lockWait):
-		t.Fatal("did not finish once the lock was released")
-	}
-}
-
-// TestChangingCommandsWaitForLock checks that each command that changes the
-// state directory waits for its lock.
-//
-//nolint:paralleltest // t.Setenv forbids parallel subtests
-func TestChangingCommandsWaitForLock(t *testing.T) {
-	t.Setenv(secret.EnvMnemonic, testMnemonic)
-	t.Setenv(secret.EnvUnlockPassphrase, testPassphrase)
-
-	for _, tc := range []struct {
-		name         string
-		withUnlocker bool
-		run          func(cli *Instance, olderVersion, unlockerID string) error
-	}{
-		{"add", false, func(cli *Instance, _, _ string) error {
-			return cli.AddSecret("added", false)
-		}},
-		{"import", false, func(cli *Instance, _, _ string) error {
-			return cli.ImportSecret(cli.cmd, "imported", testInput, false)
-		}},
-		{"generate secret", false, func(cli *Instance, _, _ string) error {
-			return cli.GenerateSecret(cli.cmd, "generated", 16, "base58", false)
-		}},
-		{"encrypt", false, func(cli *Instance, _, _ string) error {
-			return cli.Encrypt("key", testInput, "")
-		}},
-		{"rm", false, func(cli *Instance, _, _ string) error {
-			return cli.RemoveSecret(cli.cmd, "test/secret", false)
-		}},
-		{"move", false, func(cli *Instance, _, _ string) error {
-			return cli.MoveSecret(cli.cmd, "test/secret", "moved", false)
-		}},
-		{"version promote", false, func(cli *Instance, olderVersion, _ string) error {
-			return cli.PromoteVersion(cli.cmd, "test/secret", olderVersion)
-		}},
-		{"version rm", false, func(cli *Instance, olderVersion, _ string) error {
-			return cli.RemoveVersion(cli.cmd, "test/secret", olderVersion)
-		}},
-		{"vault create", false, func(cli *Instance, _, _ string) error {
-			return cli.CreateVault(cli.cmd, "created")
-		}},
-		{"vault select", false, func(cli *Instance, _, _ string) error {
-			return cli.SelectVault(cli.cmd, "other")
-		}},
-		{"vault import", false, func(cli *Instance, _, _ string) error {
-			return cli.VaultImport(cli.cmd, "other")
-		}},
-		{"vault rm", false, func(cli *Instance, _, _ string) error {
-			return cli.RemoveVault(cli.cmd, "other", false)
-		}},
-		{"unlocker add", false, func(cli *Instance, _, _ string) error {
-			return cli.UnlockersAdd("passphrase", cli.cmd)
-		}},
-		{"unlocker rm", true, func(cli *Instance, _, unlockerID string) error {
-			return cli.UnlockersRemove(unlockerID, true, cli.cmd)
-		}},
-		{"unlocker select", true, func(cli *Instance, _, unlockerID string) error {
-			return cli.UnlockerSelect(unlockerID)
-		}},
-		{"init", false, func(cli *Instance, _, _ string) error {
-			return cli.Init(cli.cmd)
-		}},
-	} {
-		t.Run(tc.name, func(t *testing.T) {
-			requireWaitsForLock(t, tc.withUnlocker, tc.run)
-		})
-	}
-}
-
-// TestEncryptWithExistingKeyTakesNoLock checks that secret encrypt with a
-// key that already exists, which only reads the state directory, finishes
-// while another command holds the state directory lock.
-func TestEncryptWithExistingKeyTakesNoLock(t *testing.T) {
-	t.Setenv(secret.EnvMnemonic, testMnemonic)
-
-	fs := afero.NewMemMapFs()
-	_, err := vault.CreateVault(fs, testStateDir, "default")
-	require.NoError(t, err)
-	require.NoError(t, afero.WriteFile(fs, testInput, []byte("input"), 0o600))
-
-	encrypt := NewCLIInstanceWithStateDir(fs, testStateDir)
-	encrypt.cmd = &cobra.Command{}
-	encrypt.cmd.SetOut(io.Discard)
-
-	// Stores the key
-	require.NoError(t, encrypt.Encrypt("key", testInput, ""))
-
-	release, err := vault.LockStateDir(fs, testStateDir)
-	require.NoError(t, err)
-	// Also frees a waiting encrypt if the test fails, so that it releases
-	// the lock the other tests use
-	defer release()
-
-	done := make(chan error, 1)
-
-	go func() { done <- encrypt.Encrypt("key", testInput, "") }()
-
-	select {
-	case err := <-done:
-		require.NoError(t, err)
-	case <-time.After(lockWait):
-		t.Fatal("secret encrypt with an existing key waited for the lock")
-	}
-}
-
-// TestEncryptStreamsUnlocked checks that secret encrypt has released the
-// state directory lock by the time it writes its output. Holding it while
-// streaming would stall every other changing command for as long as the
-// stream lasts, and forever when the other end of the pipe is one of them.
-func TestEncryptStreamsUnlocked(t *testing.T) {
-	t.Setenv(secret.EnvMnemonic, testMnemonic)
-
-	fs := afero.NewMemMapFs()
-	_, err := vault.CreateVault(fs, testStateDir, "default")
-	require.NoError(t, err)
-	require.NoError(t, afero.WriteFile(fs, testInput, []byte("streamed"), 0o600))
-
-	outputReader, outputWriter := io.Pipe()
-	done := make(chan error, 1)
+	taken := make(chan func(), 1)
 
 	go func() {
-		encrypt := NewCLIInstanceWithStateDir(fs, testStateDir)
-		encrypt.cmd = &cobra.Command{}
-		encrypt.cmd.SetOut(outputWriter)
-
-		err := encrypt.Encrypt("key", testInput, "")
-		_ = outputWriter.CloseWithError(err)
-
-		done <- err
+		release, err := vault.LockStateDir(fs, testStateDir)
+		if assert.NoError(t, err) {
+			taken <- release
+		}
 	}()
 
-	// The first byte of output: encrypt is streaming now, and blocked
-	// writing until it is read
-	_, err = io.ReadFull(outputReader, make([]byte, 1))
-	require.NoError(t, err)
-
-	taken := lockInBackground(t, fs)
-
 	select {
 	case release := <-taken:
 		release()
-	case <-time.After(lockWait):
-		// Let encrypt finish, so that it releases the lock, then free it
-		// again for the tests that follow
-		_, _ = io.Copy(io.Discard, outputReader)
-
-		(<-taken)()
-		t.Fatal("secret encrypt held the lock while streaming")
+	case <-time.After(10 * time.Second):
+		t.Fatal("the failed command left the state directory locked")
 	}
-
-	_, err = io.Copy(io.Discard, outputReader)
-	require.NoError(t, err)
-	require.NoError(t, <-done)
 }

internal/cli/move_test.go

@@ -1,99 +0,0 @@
-package cli_test
-
-import (
-	"testing"
-
-	"git.eeqj.de/sneak/secret/internal/cli"
-	"github.com/spf13/cobra"
-	"github.com/stretchr/testify/require"
-)
-
-// TestRejectedMoveWithinVaultLeavesStateUnchanged is a regression test for
-// https://git.eeqj.de/sneak/secret/issues/73, where a forced move of a secret
-// onto itself deleted it, also when "work" was spelled two ways, and a failed
-// move within "work" left "work" the current vault. "default" is the current
-// vault in every case, and each case runs on its own copy of the state
-// directory.
-//
-//nolint:paralleltest // newTwoVaultFs uses t.Setenv
-func TestRejectedMoveWithinVaultLeavesStateUnchanged(t *testing.T) {
-	before := snapshotStateDir(t, newTwoVaultFs(t))
-	require.Equal(t, "default", before[testStateDir+"/currentvault"])
-
-	const (
-		ontoItself = "secret 'x' cannot be moved onto itself"
-		workX      = "work:x"
-	)
-
-	tests := []struct {
-		command      string
-		source, dest string
-		force        bool
-		wantErr      string
-	}{
-		{"mv x x", "x", "x", false, ontoItself},
-		{"mv --force x x", "x", "x", true, ontoItself},
-		{"mv --force work:x work:", workX, "work:", true, ontoItself},
-		// An empty destination name defaults to the source name.
-		{`mv --force work:x ""`, workX, "", true, ontoItself},
-		// "work" is a vault name, so the destination is work:x.
-		{"mv --force work:x work", workX, "work", true, ontoItself},
-		{
-			"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.
-		{
-			"mv --force ..:x ..:y", "..:x", "..:y", true,
-			"vault '..' 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.
-		{
-			"mv --force work:x work/:x", workX, "work/:x", true,
-			"vault 'work/' does not exist",
-		},
-		{
-			"mv --force work/:x work:", "work/:x", "work:", true,
-			"vault 'work/' does not exist",
-		},
-		{
-			"mv --force work:x ./work:x", workX, "./work:x", true,
-			"vault './work' does not exist",
-		},
-	}
-
-	for _, tt := range tests {
-		t.Run(tt.command, func(t *testing.T) {
-			fs := newFsFromSnapshot(t, before)
-			c := cli.NewCLIInstanceWithStateDir(fs, testStateDir)
-
-			err := c.MoveSecret(&cobra.Command{}, tt.source, tt.dest, tt.force)
-
-			require.Equal(t, before, snapshotStateDir(t, fs))
-			require.EqualError(t, err, tt.wantErr)
-		})
-	}
-}
-
-// TestMoveWithinOtherVaultKeepsCurrentVault checks that `secret mv work:x
-// work:y`, with "default" the current vault, renames "x" to "y" in "work" and
-// leaves "default" the current vault.
-//
-//nolint:paralleltest // newTwoVaultFs uses t.Setenv
-func TestMoveWithinOtherVaultKeepsCurrentVault(t *testing.T) {
-	fs := newTwoVaultFs(t)
-	c := cli.NewCLIInstanceWithStateDir(fs, testStateDir)
-
-	err := c.MoveSecret(&cobra.Command{}, "work:x", "work:y", false)
-	require.NoError(t, err)
-
-	after := snapshotStateDir(t, fs)
-	workSecrets := testStateDir + "/vaults.d/work/secrets.d/"
-
-	require.Equal(t, "default", after[testStateDir+"/currentvault"])
-	require.Contains(t, after, workSecrets+"y/")
-	require.NotContains(t, after, workSecrets+"x/")
-}

internal/cli/path_traversal_test.go

@@ -1,348 +0,0 @@
-package cli_test
-
-import (
-	"fmt"
-	"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 an error with exactly the
-// message of want, so that a later check rejecting the argument 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, want error,
-	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.EqualError(t, err, want.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)
-		}},
-		{`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, vault.ValidateSecretName(tt.rejected), tt.run)
-		})
-	}
-}
-
-// TestInvalidVersionLeavesVaultsUnchanged is a regression test for
-// https://git.eeqj.de/sneak/secret/issues/67, where
-// `secret version rm x ../../..` deleted the whole vault,
-// `secret version rm x ..` the secret x, and `secret version rm x .` or
-// `secret version rm x ""` every version of x. A version argument is
-// accepted only if it is one of the versions `secret version list` lists.
-//
-//nolint:paralleltest // newTwoVaultFs uses t.Setenv
-func TestInvalidVersionLeavesVaultsUnchanged(t *testing.T) {
-	before := snapshotStateDir(t, newTwoVaultFs(t))
-
-	cmd := &cobra.Command{}
-
-	commands := []struct {
-		command string
-		run     func(c *cli.Instance, version string) error
-	}{
-		{"version rm x", func(c *cli.Instance, version string) error {
-			return c.RemoveVersion(cmd, "x", version)
-		}},
-		{"version promote x", func(c *cli.Instance, version string) error {
-			return c.PromoteVersion(cmd, "x", version)
-		}},
-		{"get x --version", func(c *cli.Instance, version string) error {
-			return c.GetSecretWithVersion(cmd, "x", version)
-		}},
-	}
-
-	for _, tt := range commands {
-		for _, version := range []string{"", ".", "..", "../../..", "a/b"} {
-			t.Run(fmt.Sprintf("%s %q", tt.command, version), func(t *testing.T) {
-				want := fmt.Errorf("version '%s' %w '%s'",
-					version, vault.ErrVersionNotFound, "x")
-				requireRejectedAndUnchanged(t, before, want,
-					func(c *cli.Instance) error { return tt.run(c, version) })
-			})
-		}
-	}
-}
-
-// TestRemoveVersionRemovesOnlyThatVersion checks that `secret version rm`
-// with a version that is not the current one removes that version and
-// changes nothing else.
-//
-//nolint:paralleltest // newTwoVaultFs uses t.Setenv
-func TestRemoveVersionRemovesOnlyThatVersion(t *testing.T) {
-	fs := newTwoVaultFs(t)
-
-	vlt, err := vault.GetCurrentVault(fs, testStateDir)
-	require.NoError(t, err)
-
-	// A second version of "x" becomes the current one.
-	err = vlt.AddSecret("x", memguard.NewBufferFromBytes([]byte("new")), true)
-	require.NoError(t, err)
-
-	secretDir := testStateDir + "/vaults.d/default/secrets.d/x"
-	versions, err := secret.ListVersions(fs, secretDir)
-	require.NoError(t, err)
-	require.Len(t, versions, 2)
-
-	// ListVersions lists the newest version first.
-	oldDir := secretDir + "/versions/" + versions[1] + "/"
-	before := snapshotStateDir(t, fs)
-	require.Contains(t, before, oldDir)
-
-	c := cli.NewCLIInstanceWithStateDir(fs, testStateDir)
-	err = c.RemoveVersion(&cobra.Command{}, "x", versions[1])
-	require.NoError(t, err)
-
-	// Expected: the state as before without everything under oldDir.
-	want := map[string]string{}
-
-	for path, content := range before {
-		if !strings.HasPrefix(path, oldDir) {
-			want[path] = content
-		}
-	}
-
-	require.Equal(t, want, snapshotStateDir(t, fs))
-}
-
-// TestMoveToVaultNameRenamesInCurrentVault checks that `secret mv x work`,
-// where "work" is also the name of a vault, renames the secret "x" to "work"
-// in the current vault and changes nothing else.
-//
-//nolint:paralleltest // newTwoVaultFs uses t.Setenv
-func TestMoveToVaultNameRenamesInCurrentVault(t *testing.T) {
-	before := snapshotStateDir(t, newTwoVaultFs(t))
-	fs := newFsFromSnapshot(t, before)
-
-	c := cli.NewCLIInstanceWithStateDir(fs, testStateDir)
-	err := c.MoveSecret(&cobra.Command{}, "x", "work", false)
-	require.NoError(t, err)
-
-	// Expected: the state as before, with everything under the current
-	// vault's secrets.d/x/ now under secrets.d/work/.
-	oldDir := testStateDir + "/vaults.d/default/secrets.d/x/"
-	newDir := testStateDir + "/vaults.d/default/secrets.d/work/"
-	want := map[string]string{}
-
-	for path, content := range before {
-		rest, found := strings.CutPrefix(path, oldDir)
-		if found {
-			path = newDir + rest
-		}
-
-		want[path] = content
-	}
-
-	require.Contains(t, want, newDir)
-	require.Equal(t, want, snapshotStateDir(t, fs))
-}

internal/cli/root.go

@@ -4,38 +4,17 @@ import (
 	"os"
 
 	"git.eeqj.de/sneak/secret/internal/secret"
-	"github.com/awnumar/memguard"
 	"github.com/spf13/cobra"
-	"golang.org/x/sys/unix"
-	"golang.org/x/term"
 )
 
-// Entry runs the secret CLI and returns the process exit code. It wipes
-// every memguard buffer before it returns, so the caller must do nothing
-// but exit with the code.
-func Entry() int {
-	// On SIGINT or SIGTERM memguard runs this function, wipes every buffer
-	// and exits with status 1. The passphrase prompt turns terminal echo
-	// off until the read finishes, so a signal there would leave echo off.
-	// Only a process in the terminal's foreground process group may reset
-	// it: one in the background that tries is stopped instead of exiting.
-	terminalState, terminalErr := term.GetState(unix.Stdin)
+// Entry is the entry point for the secret CLI application
+func Entry() {
+	cmd := newRootCmd()
 
-	memguard.CatchSignal(func(os.Signal) {
-		foreground, err := unix.IoctlGetInt(unix.Stdin, unix.TIOCGPGRP)
-		if terminalErr == nil && err == nil && foreground == unix.Getpgrp() {
-			_ = term.Restore(unix.Stdin, terminalState)
-		}
-	}, os.Interrupt, unix.SIGTERM)
-
-	defer memguard.Purge()
-
-	err := newRootCmd().Execute()
+	err := cmd.Execute()
 	if err != nil {
-		return 1
+		os.Exit(1)
 	}
-
-	return 0
 }
 
 func newRootCmd() *cobra.Command {

internal/cli/secrets.go

@@ -40,7 +40,6 @@ var (
 	errVaultDoesNotExist           = errors.New("does not exist")
 	errCrossVaultSourceUnqualified = errors.New(
 		"source must specify vault (e.g., vault:secret) for cross-vault move")
-	errMoveOntoItself = errors.New("cannot be moved onto itself")
 )
 
 // bufferInfo tracks a protected buffer and the number of bytes used in it
@@ -110,12 +109,6 @@ func newGetCmd() *cobra.Command {
 				return fmt.Errorf("failed to initialize CLI: %w", err)
 			}
 
-			// Without --version, get the current version. A given
-			// --version is checked as typed, so an empty one is rejected.
-			if !cmd.Flags().Changed("version") {
-				return cli.GetSecret(cmd, args[0])
-			}
-
 			return cli.GetSecretWithVersion(cmd, args[0], version)
 		},
 	}
@@ -409,32 +402,12 @@ func (cli *Instance) AddSecret(secretName string, force bool) error {
 	return nil
 }
 
-// GetSecret retrieves and prints the current version of a secret
+// GetSecret retrieves and prints a secret from the current vault
 func (cli *Instance) GetSecret(cmd *cobra.Command, secretName string) error {
-	secret.Debug("GetSecret called", "secretName", secretName)
-
-	// Store the command for output
-	cli.cmd = cmd
-
-	// Get current vault
-	vlt, err := vault.GetCurrentVault(cli.fs, cli.stateDir)
-	if err != nil {
-		return err
-	}
-
-	value, err := vlt.GetSecret(secretName)
-	if err != nil {
-		return err
-	}
-
-	// Print the secret value to stdout
-	_, _ = cli.Print(string(value))
-
-	return nil
+	return cli.GetSecretWithVersion(cmd, secretName, "")
 }
 
-// GetSecretWithVersion retrieves and prints a specific version of a secret.
-// The version must be one of the secret's versions.
+// GetSecretWithVersion retrieves and prints a specific version of a secret
 func (cli *Instance) GetSecretWithVersion(
 	cmd *cobra.Command, secretName string, version string,
 ) error {
@@ -453,7 +426,13 @@ func (cli *Instance) GetSecretWithVersion(
 	}
 
 	// Get the secret value
-	value, err := vlt.GetSecretVersion(secretName, version)
+	var value []byte
+	if version == "" {
+		value, err = vlt.GetSecret(secretName)
+	} else {
+		value, err = vlt.GetSecretVersion(secretName, version)
+	}
+
 	if err != nil {
 		secret.Debug("Failed to get secret", "error", err)
 
@@ -633,11 +612,6 @@ 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 {
@@ -692,11 +666,6 @@ 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
-	}
-
 	release, err := vault.LockStateDir(cli.fs, cli.stateDir)
 	if err != nil {
 		return err
@@ -748,8 +717,7 @@ func (cli *Instance) RemoveSecret(cmd *cobra.Command, secretName string, _ bool)
 	return nil
 }
 
-// MoveSecret moves or renames a secret (within or across vaults), holding
-// the state directory lock.
+// MoveSecret moves or renames a secret (within or across vaults)
 func (cli *Instance) MoveSecret(
 	cmd *cobra.Command, source, dest string, force bool,
 ) error {
@@ -759,20 +727,17 @@ func (cli *Instance) MoveSecret(
 	}
 	defer release()
 
-	return cli.moveSecret(cmd, source, dest, force)
-}
-
-// moveSecret does the work of MoveSecret. The caller holds the state
-// directory lock.
-func (cli *Instance) moveSecret(
-	cmd *cobra.Command, source, dest string, force bool,
-) error {
 	// Parse source and destination
 	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 && destQualified {
+	if !srcQualified {
 		return errCrossVaultSourceUnqualified
 	}
 
@@ -780,97 +745,53 @@ 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 srcQualified && !destQualified {
+	if !destQualified {
 		// Check if dest is actually a vault name
-		_, err := cli.existingVault(dest)
-		if err == nil {
+		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
-		} else {
-			// dest is a secret name in source vault
+		}
+
+		// If destVaultName is still empty, dest is a secret name in source vault
+		if destVaultName == "" {
 			destVaultName = srcVaultName
+			destSecretName = dest
 		}
 	}
 
-	// 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 == "" {
+	// If destination secret name is empty, use source secret name
+	if destSecretName == "" {
 		destSecretName = srcSecretName
 	}
 
-	// Check both names, for every form of the move, before building any path
-	// from them.
-	err := vault.ValidateSecretName(srcSecretName)
-	if err != nil {
-		return err
-	}
-
-	err = vault.ValidateSecretName(destSecretName)
-	if err != nil {
-		return err
-	}
-
-	// Neither name is qualified: a rename within the current vault.
-	if !srcQualified {
-		vlt, err := vault.GetCurrentVault(cli.fs, cli.stateDir)
+	// 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)
 		if err != nil {
-			return err
+			return fmt.Errorf("failed to select vault '%s': %w", srcVaultName, err)
 		}
 
-		return cli.moveSecretWithinVault(
-			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.
-	srcVault, err := cli.existingVault(srcVaultName)
-	if err != nil {
-		return err
-	}
-
-	destVault, err := cli.existingVault(destVaultName)
-	if err != nil {
-		return err
-	}
-
-	if srcVaultName == destVaultName {
-		return cli.moveSecretWithinVault(
-			cmd, srcVault, srcSecretName, destSecretName, force)
+		return cli.moveSecretWithinVault(cmd, srcSecretName, destSecretName, force)
 	}
 
+	// Cross-vault move
 	return cli.moveSecretCrossVault(
-		cmd, srcVault, srcSecretName, destVault, destSecretName, force)
+		cmd, srcVaultName, srcSecretName, destVaultName, 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.
-func (cli *Instance) existingVault(name string) (*vault.Vault, error) {
-	vaults, err := vault.ListVaults(cli.fs, cli.stateDir)
-	if err != nil {
-		return nil, fmt.Errorf("failed to list vaults: %w", err)
-	}
-
-	if !slices.Contains(vaults, name) {
-		return nil, fmt.Errorf("vault '%s' %w", name, errVaultDoesNotExist)
-	}
-
-	return vault.NewVault(cli.fs, cli.stateDir, name), nil
-}
-
-// moveSecretWithinVault renames a secret within the vault vlt. Its caller,
-// MoveSecret, has already checked both secret names.
+// moveSecretWithinVault handles rename within the current vault
 func (cli *Instance) moveSecretWithinVault(
-	cmd *cobra.Command, vlt *vault.Vault, source, dest string, force bool,
+	cmd *cobra.Command, source, dest string, force bool,
 ) error {
-	// With --force the destination is removed before the source is renamed
-	// onto it, which would delete the secret.
-	if source == dest {
-		return fmt.Errorf("secret '%s' %w", source, errMoveOntoItself)
+	currentVlt, err := vault.GetCurrentVault(cli.fs, cli.stateDir)
+	if err != nil {
+		return err
 	}
 
-	vaultDir, err := vlt.GetDirectory()
+	vaultDir, err := currentVlt.GetDirectory()
 	if err != nil {
 		return err
 	}
@@ -916,34 +837,56 @@ func (cli *Instance) moveSecretWithinVault(
 	return nil
 }
 
-// moveSecretCrossVault handles moving between two different vaults. Its
-// caller, MoveSecret, has already checked both secret names and that both
-// vaults exist.
+// moveSecretCrossVault handles moving between different vaults
 func (cli *Instance) moveSecretCrossVault(
 	cmd *cobra.Command,
-	srcVault *vault.Vault, srcSecretName string,
-	destVault *vault.Vault, destSecretName string,
+	srcVaultName, srcSecretName,
+	destVaultName, destSecretName string,
 	force bool,
 ) error {
+	// Get source vault
+	srcVault := vault.NewVault(cli.fs, cli.stateDir, srcVaultName)
+
 	srcVaultDir, err := srcVault.GetDirectory()
 	if err != nil {
 		return fmt.Errorf("failed to get source vault directory: %w", err)
 	}
 
+	// Verify source vault exists
+	exists, err := afero.DirExists(cli.fs, srcVaultDir)
+	if err != nil || !exists {
+		return fmt.Errorf("source vault '%s' %w", srcVaultName, errVaultDoesNotExist)
+	}
+
 	// Verify source secret exists
 	srcStorageName := strings.ReplaceAll(srcSecretName, "/", "%")
 	srcSecretDir := filepath.Join(srcVaultDir, "secrets.d", srcStorageName)
 
-	exists, err := afero.DirExists(cli.fs, srcSecretDir)
+	exists, err = afero.DirExists(cli.fs, srcSecretDir)
 	if err != nil || !exists {
 		return fmt.Errorf("secret '%s' %w in vault '%s'",
-			srcSecretName, errSecretNotFound, srcVault.Name)
+			srcSecretName, errSecretNotFound, srcVaultName)
+	}
+
+	// Get destination vault
+	destVault := vault.NewVault(cli.fs, cli.stateDir, destVaultName)
+
+	destVaultDir, err := destVault.GetDirectory()
+	if err != nil {
+		return fmt.Errorf("failed to get destination vault directory: %w", err)
+	}
+
+	// Verify destination vault exists
+	exists, err = afero.DirExists(cli.fs, destVaultDir)
+	if err != nil || !exists {
+		return fmt.Errorf("destination vault '%s' %w",
+			destVaultName, errVaultDoesNotExist)
 	}
 
 	// Unlock destination vault (will fail if neither mnemonic nor unlocker available)
 	_, err = destVault.GetOrDeriveLongTermKey()
 	if err != nil {
-		return fmt.Errorf("failed to unlock destination vault '%s': %w", destVault.Name, err)
+		return fmt.Errorf("failed to unlock destination vault '%s': %w", destVaultName, err)
 	}
 
 	// Count versions for user feedback
@@ -963,13 +906,13 @@ func (cli *Instance) moveSecretCrossVault(
 		// Copy succeeded but delete failed - warn but don't fail
 		cmd.Printf("Warning: copied secret but failed to remove source: %v\n", err)
 		cmd.Printf("Moved secret '%s:%s' to '%s:%s' (%d version(s))\n",
-			srcVault.Name, srcSecretName, destVault.Name, destSecretName, versionCount)
+			srcVaultName, srcSecretName, destVaultName, destSecretName, versionCount)
 
 		return nil
 	}
 
 	cmd.Printf("Moved secret '%s:%s' to '%s:%s' (%d version(s))\n",
-		srcVault.Name, srcSecretName, destVault.Name, destSecretName, versionCount)
+		srcVaultName, srcSecretName, destVaultName, destSecretName, versionCount)
 
 	return nil
 }

internal/cli/version.go

@@ -112,11 +112,6 @@ 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 {
@@ -244,11 +239,6 @@ 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
-	}
-
 	release, err := vault.LockStateDir(cli.fs, cli.stateDir)
 	if err != nil {
 		return err
@@ -271,7 +261,9 @@ func (cli *Instance) PromoteVersion(
 	secretDir := filepath.Join(vaultDir, "secrets.d", encodedName)
 
 	// Check if version exists
-	exists, err := secret.VersionExists(cli.fs, secretDir, version)
+	versionDir := filepath.Join(secretDir, "versions", version)
+
+	exists, err := afero.DirExists(cli.fs, versionDir)
 	if err != nil {
 		return fmt.Errorf("failed to check if version exists: %w", err)
 	}
@@ -296,11 +288,6 @@ 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
-	}
-
 	release, err := vault.LockStateDir(cli.fs, cli.stateDir)
 	if err != nil {
 		return err
@@ -333,7 +320,9 @@ func (cli *Instance) RemoveVersion(
 	}
 
 	// Check if version exists
-	exists, err = secret.VersionExists(cli.fs, secretDir, version)
+	versionDir := filepath.Join(secretDir, "versions", version)
+
+	exists, err = afero.DirExists(cli.fs, versionDir)
 	if err != nil {
 		return fmt.Errorf("failed to check if version exists: %w", err)
 	}
@@ -356,8 +345,6 @@ func (cli *Instance) RemoveVersion(
 	}
 
 	// Remove the version directory
-	versionDir := filepath.Join(secretDir, "versions", version)
-
 	err = secret.RemoveDirAtomic(cli.fs, versionDir)
 	if err != nil {
 		return fmt.Errorf("failed to remove version: %w", err)

internal/cli/version_test.go

@@ -276,8 +276,8 @@ func TestGetSecretWithVersion(t *testing.T) {
 	var buf bytes.Buffer
 	cmd.SetOut(&buf)
 
-	// Test getting the current version
-	err = cli.GetSecret(cmd, "test/secret")
+	// Test getting current version (empty version string)
+	err = cli.GetSecretWithVersion(cmd, "test/secret", "")
 	require.NoError(t, err)
 	assert.Equal(t, "version-2", buf.String())
 

internal/secret/atomic_test.go

@@ -22,7 +22,6 @@ var errInjected = errors.New("injected failure")
 const (
 	opCreate = "create"
 	opOpen   = "open"
-	opSync   = "sync"
 	opMkdir  = "mkdir"
 	opRemove = "remove"
 	opRename = "rename"
@@ -32,41 +31,15 @@ const (
 // version.
 const currentFile = "current"
 
-// unlockerMetadataFile is the file a new unlocker writes last.
-const unlockerMetadataFile = "unlocker-metadata.json"
-
-// unlockerPassphrase protects the passphrase unlockers the tests create.
-//
-//nolint:gosec // G101: test data, not a real credential
-const unlockerPassphrase = "unlocker passphrase"
-
 // hookFs passes every call through to Fs, but first calls before for each
-// call that changes the filesystem, and for each Sync of a file opened
-// through it, with the path it changes (the new path, for Rename). A test
-// uses before to inspect the tree at every point where a crash could stop
-// the code under test, or returns an error from it to make that call fail.
-// If opened is set, OpenFile also tells it the mode it opens each file with.
+// call that changes the filesystem, with the path it changes (the new path,
+// for Rename). A test uses before to inspect the tree at every point where
+// a crash could stop the code under test, or returns an error from it to
+// make that call fail.
 type hookFs struct {
 	afero.Fs
 
 	before func(op, path string) error
-	opened func(path string, perm os.FileMode)
-}
-
-// hookFile is a file opened through hookFs.
-type hookFile struct {
-	afero.File
-
-	before func(op, path string) error
-}
-
-func (f hookFile) Sync() error {
-	err := f.before(opSync, f.Name())
-	if err != nil {
-		return err
-	}
-
-	return f.File.Sync()
 }
 
 //nolint:ireturn // implements afero.Fs
@@ -76,12 +49,7 @@ func (h hookFs) Create(name string) (afero.File, error) {
 		return nil, err
 	}
 
-	file, err := h.Fs.Create(name)
-	if err != nil {
-		return nil, err
-	}
-
-	return hookFile{File: file, before: h.before}, nil
+	return h.Fs.Create(name)
 }
 
 //nolint:ireturn // implements afero.Fs
@@ -93,16 +61,7 @@ func (h hookFs) OpenFile(
 		return nil, err
 	}
 
-	if h.opened != nil {
-		h.opened(name, perm)
-	}
-
-	file, err := h.Fs.OpenFile(name, flag, perm)
-	if err != nil {
-		return nil, err
-	}
-
-	return hookFile{File: file, before: h.before}, nil
+	return h.Fs.OpenFile(name, flag, perm)
 }
 
 func (h hookFs) Mkdir(name string, perm os.FileMode) error {
@@ -513,68 +472,10 @@ func TestVersionSaveFailureLeavesNothing(t *testing.T) {
 	}
 }
 
-// TestCurrentFilesNeverMissing selects the current version, vault and
-// unlocker again and checks, before each change this makes, that the file
-// naming the current one exists: a reader or a crash never finds it
-// missing.
-//
-//nolint:paralleltest // t.Setenv forbids t.Parallel
-func TestCurrentFilesNeverMissing(t *testing.T) {
-	t.Setenv(secret.EnvMnemonic, testMnemonic)
-
-	for _, tfs := range testFilesystems {
-		t.Run(tfs.name, func(t *testing.T) {
-			base, stateDir := tfs.open(t)
-			vlt := newVaultWithSecret(t, base, stateDir, testVaultName, "value")
-
-			passphrase := memguard.NewBufferFromBytes([]byte(unlockerPassphrase))
-			defer passphrase.Destroy()
-
-			// Created as the current unlocker
-			unlocker, err := vlt.CreatePassphraseUnlocker(passphrase)
-			require.NoError(t, err)
-
-			vaultDir, err := vlt.GetDirectory()
-			require.NoError(t, err)
-
-			secretDir := filepath.Join(vaultDir, "secrets.d", "shared")
-			version, err := secret.GetCurrentVersion(base, secretDir)
-			require.NoError(t, err)
-
-			for _, tc := range []struct {
-				path     string
-				reselect func(fs afero.Fs) error
-			}{
-				{filepath.Join(secretDir, currentFile), func(fs afero.Fs) error {
-					return secret.SetCurrentVersion(fs, secretDir, version)
-				}},
-				{filepath.Join(stateDir, "currentvault"), func(fs afero.Fs) error {
-					return vault.SelectVault(fs, stateDir, testVaultName)
-				}},
-				{filepath.Join(vaultDir, "current-unlocker"), func(fs afero.Fs) error {
-					return vault.NewVault(fs, stateDir, testVaultName).
-						SelectUnlocker(unlocker.GetID())
-				}},
-			} {
-				fs := hookFs{Fs: base, before: func(string, string) error {
-					exists, err := afero.Exists(base, tc.path)
-					require.NoError(t, err)
-					assert.True(t, exists, "%s is missing", filepath.Base(tc.path))
-
-					return nil
-				}}
-
-				require.NoError(t, tc.reselect(fs))
-			}
-		})
-	}
-}
-
-// TestWriteFileAtomicTempFile checks that WriteFileAtomic creates its
-// temporary file with mode 0600, rather than wider and narrowed later, so
-// that no other user can ever read it, and syncs it before renaming it into
-// place, so that a crash cannot leave the file named but its data lost.
-func TestWriteFileAtomicTempFile(t *testing.T) {
+// TestSetCurrentVersionNeverMissing checks, before every change
+// SetCurrentVersion makes, that the current file exists: a reader or a crash
+// never finds the secret without a current version.
+func TestSetCurrentVersionNeverMissing(t *testing.T) {
 	t.Parallel()
 
 	for _, tfs := range testFilesystems {
@@ -582,91 +483,24 @@ func TestWriteFileAtomicTempFile(t *testing.T) {
 			t.Parallel()
 
 			base, dir := tfs.open(t)
+			secretDir := filepath.Join(dir, "secret")
+			require.NoError(t, base.MkdirAll(secretDir, 0o700))
+			require.NoError(t, secret.SetCurrentVersion(base, secretDir, "20231216.001"))
 
-			var modes []os.FileMode
+			currentPath := filepath.Join(secretDir, currentFile)
+			fs := hookFs{Fs: base, before: func(string, string) error {
+				exists, err := afero.Exists(base, currentPath)
+				require.NoError(t, err)
+				assert.True(t, exists, "current is missing")
 
-			synced := false
-			fs := hookFs{
-				Fs: base,
-				before: func(op, _ string) error {
-					switch op {
-					case opSync:
-						synced = true
-					case opRename:
-						assert.True(t, synced, "renamed before syncing")
-					}
+				return nil
+			}}
 
-					return nil
-				},
-				opened: func(_ string, perm os.FileMode) {
-					modes = append(modes, perm)
-				},
-			}
+			require.NoError(t, secret.SetCurrentVersion(fs, secretDir, "20231216.002"))
 
-			require.NoError(t, secret.WriteFileAtomic(fs,
-				filepath.Join(dir, currentFile), []byte("new")))
-			assert.Equal(t, []os.FileMode{secret.FilePerms}, modes)
+			version, err := secret.GetCurrentVersion(base, secretDir)
+			require.NoError(t, err)
+			assert.Equal(t, "20231216.002", version)
 		})
 	}
 }
-
-// TestPassphraseUnlockerGetsKeyFirst creates a passphrase unlocker in a
-// vault whose long-term key cannot be had: it must fail without writing
-// anything, so that it never leaves a partial unlocker, nor breaks the one
-// it would replace.
-func TestPassphraseUnlockerGetsKeyFirst(t *testing.T) {
-	// No mnemonic, and no current unlocker to get the key from
-	t.Setenv(secret.EnvMnemonic, "")
-
-	base := afero.NewMemMapFs()
-	_, err := vault.CreateVault(base, testVaultStateDir, testVaultName)
-	require.NoError(t, err)
-
-	fs := hookFs{Fs: base, before: func(_, path string) error {
-		t.Errorf("changed %s before getting the long-term key", path)
-
-		return nil
-	}}
-
-	passphrase := memguard.NewBufferFromBytes([]byte(unlockerPassphrase))
-	defer passphrase.Destroy()
-
-	_, err = vault.NewVault(fs, testVaultStateDir, testVaultName).
-		CreatePassphraseUnlocker(passphrase)
-	require.Error(t, err)
-}
-
-// TestPassphraseUnlockerWritesMetadataLast checks that the last file a new
-// passphrase unlocker writes in its directory is its metadata: an unlocker
-// directory without metadata is never used, so one interrupted earlier
-// cannot be.
-func TestPassphraseUnlockerWritesMetadataLast(t *testing.T) {
-	t.Setenv(secret.EnvMnemonic, testMnemonic)
-
-	base := afero.NewMemMapFs()
-	vlt, err := vault.CreateVault(base, testVaultStateDir, testVaultName)
-	require.NoError(t, err)
-
-	vaultDir, err := vlt.GetDirectory()
-	require.NoError(t, err)
-
-	unlockerDir := filepath.Join(vaultDir, "unlockers.d", "passphrase")
-
-	var last string
-
-	fs := hookFs{Fs: base, before: func(_, path string) error {
-		if filepath.Dir(path) == unlockerDir {
-			last = filepath.Base(path)
-		}
-
-		return nil
-	}}
-
-	passphrase := memguard.NewBufferFromBytes([]byte(unlockerPassphrase))
-	defer passphrase.Destroy()
-
-	_, err = vault.NewVault(fs, testVaultStateDir, testVaultName).
-		CreatePassphraseUnlocker(passphrase)
-	require.NoError(t, err)
-	assert.Equal(t, unlockerMetadataFile, last)
-}

internal/secret/helpers_darwin.go

@@ -0,0 +1,29 @@
+//go:build darwin
+
+package secret
+
+import (
+	"crypto/rand"
+	"fmt"
+	"math/big"
+)
+
+// generateRandomString generates a random string of the specified length using the given character set
+func generateRandomString(length int, charset string) (string, error) {
+	if length <= 0 {
+		return "", fmt.Errorf("length must be positive")
+	}
+
+	result := make([]byte, length)
+	charsetLen := big.NewInt(int64(len(charset)))
+
+	for i := range length {
+		randomIndex, err := rand.Int(rand.Reader, charsetLen)
+		if err != nil {
+			return "", fmt.Errorf("failed to generate random number: %w", err)
+		}
+		result[i] = charset[randomIndex.Int64()]
+	}
+
+	return string(result), nil
+}

internal/secret/keychaindata.go

@@ -1,142 +0,0 @@
-package secret
-
-import (
-	"bytes"
-	"encoding/hex"
-	"encoding/json"
-	"errors"
-	"fmt"
-	"strings"
-
-	"github.com/awnumar/memguard"
-)
-
-var (
-	errPassphraseLength = errors.New(
-		"passphrase length must be a positive even number")
-	errPassphraseNotHex = errors.New(
-		"keychain passphrase must be lowercase hex")
-	errNoKeychainPassphrase = errors.New(
-		"keychain data has no agePrivKeyPassphrase string")
-)
-
-// KeychainData is what a keychain unlocker stores in the macOS keychain.
-// It is stored as JSON, but encode and decodeKeychainData keep the
-// passphrase out of encoding/json, which would leave copies of it in
-// ordinary memory.
-type KeychainData struct {
-	AgePublicKey         string
-	AgePrivKeyPassphrase *memguard.LockedBuffer
-	EncryptedLongtermKey string
-}
-
-// generateRandomPassphrase returns length random lowercase hex characters
-// in a locked buffer. The caller must destroy it.
-func generateRandomPassphrase(length int) (*memguard.LockedBuffer, error) {
-	// Each random byte becomes two hex characters.
-	randomBytes := hex.DecodedLen(length)
-	if length <= 0 || hex.EncodedLen(randomBytes) != length {
-		return nil, errPassphraseLength
-	}
-
-	random := memguard.NewBufferRandom(randomBytes)
-	defer random.Destroy()
-
-	passphrase := memguard.NewBuffer(length)
-	hex.Encode(passphrase.Bytes(), random.Bytes())
-	passphrase.Freeze()
-
-	return passphrase, nil
-}
-
-// encode returns d as JSON in a locked buffer:
-// {"agePublicKey":"...","agePrivKeyPassphrase":"...","encryptedLongtermKey":"..."}.
-// The passphrase is copied straight into the buffer, so it must be hex,
-// which JSON does not escape. The caller must destroy the returned buffer.
-func (d *KeychainData) encode() (*memguard.LockedBuffer, error) {
-	if d.AgePrivKeyPassphrase == nil {
-		return nil, errNilPassphraseBuffer
-	}
-
-	if d.AgePrivKeyPassphrase.Size() == 0 {
-		return nil, errEmptyPassphrase
-	}
-
-	for _, c := range d.AgePrivKeyPassphrase.Bytes() {
-		if strings.IndexByte("0123456789abcdef", c) < 0 {
-			return nil, errPassphraseNotHex
-		}
-	}
-
-	publicKey, err := json.Marshal(d.AgePublicKey)
-	if err != nil {
-		return nil, fmt.Errorf("failed to encode age public key: %w", err)
-	}
-
-	longtermKey, err := json.Marshal(d.EncryptedLongtermKey)
-	if err != nil {
-		return nil, fmt.Errorf("failed to encode long-term key: %w", err)
-	}
-
-	parts := [][]byte{
-		[]byte(`{"agePublicKey":`), publicKey,
-		[]byte(`,"agePrivKeyPassphrase":"`), d.AgePrivKeyPassphrase.Bytes(),
-		[]byte(`","encryptedLongtermKey":`), longtermKey,
-		[]byte(`}`),
-	}
-
-	size := 0
-	for _, part := range parts {
-		size += len(part)
-	}
-
-	encoded := memguard.NewBuffer(size)
-
-	written := 0
-	for _, part := range parts {
-		written += copy(encoded.Bytes()[written:], part)
-	}
-
-	encoded.Freeze()
-
-	return encoded, nil
-}
-
-// decodeKeychainData parses keychain data written by encode. The caller
-// must destroy the returned AgePrivKeyPassphrase.
-func decodeKeychainData(data *memguard.LockedBuffer) (*KeychainData, error) {
-	if data == nil {
-		return nil, errNilDataBuffer
-	}
-
-	// json.Unmarshal gives a json.RawMessage field the field's JSON text
-	// unchanged, in the one copy RawMessage makes; it is wiped on return.
-	var fields struct {
-		AgePublicKey         string          `json:"agePublicKey"`
-		AgePrivKeyPassphrase json.RawMessage `json:"agePrivKeyPassphrase"`
-		EncryptedLongtermKey string          `json:"encryptedLongtermKey"`
-	}
-
-	defer func() { memguard.WipeBytes(fields.AgePrivKeyPassphrase) }()
-
-	err := json.Unmarshal(data.Bytes(), &fields)
-	if err != nil {
-		return nil, fmt.Errorf("failed to parse keychain data: %w", err)
-	}
-
-	// json.Unmarshal accepted the JSON, so text that starts with a quote is
-	// a whole string. The passphrase is hex, so it is the text between the
-	// quotes.
-	quoted := fields.AgePrivKeyPassphrase
-	if !bytes.HasPrefix(quoted, []byte(`"`)) {
-		return nil, errNoKeychainPassphrase
-	}
-
-	return &KeychainData{
-		AgePublicKey: fields.AgePublicKey,
-		// NewBufferFromBytes wipes the bytes it copies.
-		AgePrivKeyPassphrase: memguard.NewBufferFromBytes(
-			quoted[1 : len(quoted)-1]),
-		EncryptedLongtermKey: fields.EncryptedLongtermKey,
-	}, nil
-}

internal/secret/keychaindata_test.go

@@ -1,118 +0,0 @@
-//nolint:testpackage // white-box test of unexported internals
-package secret
-
-import (
-	"encoding/json"
-	"testing"
-
-	"github.com/awnumar/memguard"
-	"github.com/stretchr/testify/assert"
-	"github.com/stretchr/testify/require"
-)
-
-func TestGenerateRandomPassphrase(t *testing.T) {
-	t.Parallel()
-
-	first, err := generateRandomPassphrase(64)
-	require.NoError(t, err)
-
-	defer first.Destroy()
-
-	second, err := generateRandomPassphrase(64)
-	require.NoError(t, err)
-
-	defer second.Destroy()
-
-	assert.Regexp(t, `^[0-9a-f]{64}$`, first.String())
-	assert.NotEqual(t, first.String(), second.String())
-	assert.False(t, first.IsMutable())
-
-	for _, length := range []int{0, -2, 63} {
-		_, err := generateRandomPassphrase(length)
-		require.ErrorIs(t, err, errPassphraseLength, "length %d", length)
-	}
-}
-
-func TestKeychainDataEncodeDecode(t *testing.T) {
-	t.Parallel()
-
-	passphrase := memguard.NewBufferFromBytes([]byte("0a1b2c3d"))
-	defer passphrase.Destroy()
-
-	data := KeychainData{
-		AgePublicKey:         "age1example",
-		AgePrivKeyPassphrase: passphrase,
-		EncryptedLongtermKey: "beef",
-	}
-
-	encoded, err := data.encode()
-	require.NoError(t, err)
-
-	defer encoded.Destroy()
-
-	assert.JSONEq(t,
-		`{"agePublicKey":"age1example",`+
-			`"agePrivKeyPassphrase":"0a1b2c3d",`+
-			`"encryptedLongtermKey":"beef"}`,
-		encoded.String())
-	assert.False(t, encoded.IsMutable())
-
-	decoded, err := decodeKeychainData(encoded)
-	require.NoError(t, err)
-
-	defer decoded.AgePrivKeyPassphrase.Destroy()
-
-	assert.Equal(t, "age1example", decoded.AgePublicKey)
-	assert.Equal(t, "0a1b2c3d", decoded.AgePrivKeyPassphrase.String())
-	assert.Equal(t, "beef", decoded.EncryptedLongtermKey)
-}
-
-func TestKeychainDataEncodeRejectsBadPassphrase(t *testing.T) {
-	t.Parallel()
-
-	tests := []struct {
-		name       string
-		passphrase *memguard.LockedBuffer
-		wantErr    error
-	}{
-		{"nil", nil, errNilPassphraseBuffer},
-		{"empty", memguard.NewBuffer(0), errEmptyPassphrase},
-		{
-			"not hex",
-			memguard.NewBufferFromBytes([]byte(`abc"def`)),
-			errPassphraseNotHex,
-		},
-	}
-
-	for _, tt := range tests {
-		t.Run(tt.name, func(t *testing.T) {
-			t.Parallel()
-
-			data := KeychainData{AgePrivKeyPassphrase: tt.passphrase}
-			_, err := data.encode()
-			require.ErrorIs(t, err, tt.wantErr)
-		})
-	}
-}
-
-func TestDecodeKeychainDataRejectsBadData(t *testing.T) {
-	t.Parallel()
-
-	for _, text := range []string{
-		`{"agePublicKey":"age1example"}`,
-		`{"agePrivKeyPassphrase":42}`,
-	} {
-		data := memguard.NewBufferFromBytes([]byte(text))
-		_, err := decodeKeychainData(data)
-		data.Destroy()
-		require.ErrorIs(t, err, errNoKeychainPassphrase, text)
-	}
-
-	notJSON := memguard.NewBufferFromBytes([]byte(`{"agePrivKeyPassphrase":`))
-	defer notJSON.Destroy()
-
-	_, err := decodeKeychainData(notJSON)
-
-	var syntaxError *json.SyntaxError
-	require.ErrorAs(t, err, &syntaxError)
-}

internal/secret/keychainunlocker.go

@@ -45,6 +45,13 @@ type KeychainUnlocker struct {
 	fs        afero.Fs
 }
 
+// KeychainData represents the data stored in the macOS keychain
+type KeychainData struct {
+	AgePublicKey         string `json:"agePublicKey"`
+	AgePrivKeyPassphrase string `json:"agePrivKeyPassphrase"`
+	EncryptedLongtermKey string `json:"encryptedLongtermKey"`
+}
+
 // GetIdentity implements Unlocker interface for Keychain-based unlockers
 func (k *KeychainUnlocker) GetIdentity() (*age.X25519Identity, error) {
 	DebugWith("Getting keychain unlocker identity",
@@ -74,18 +81,13 @@ func (k *KeychainUnlocker) GetIdentity() (*age.X25519Identity, error) {
 		slog.Int("data_length", len(keychainDataBytes)),
 	)
 
-	// Move the keychain data into locked memory; this wipes keychainDataBytes
-	keychainDataBuffer := memguard.NewBufferFromBytes(keychainDataBytes)
-	defer keychainDataBuffer.Destroy()
-
 	// Step 3: Parse keychain data
-	keychainData, err := decodeKeychainData(keychainDataBuffer)
-	if err != nil {
+	var keychainData KeychainData
+	if err := json.Unmarshal(keychainDataBytes, &keychainData); err != nil {
 		Debug("Failed to parse keychain data", "error", err, "unlocker_id", k.GetID())
 
 		return nil, fmt.Errorf("failed to parse keychain data: %w", err)
 	}
-	defer keychainData.AgePrivKeyPassphrase.Destroy()
 
 	Debug("Parsed keychain data successfully", "unlocker_id", k.GetID())
 
@@ -107,7 +109,11 @@ func (k *KeychainUnlocker) GetIdentity() (*age.X25519Identity, error) {
 
 	// Step 5: Decrypt the age private key using the passphrase from keychain
 	Debug("Decrypting age private key with keychain passphrase", "unlocker_id", k.GetID())
-	agePrivKeyBuffer, err := DecryptWithPassphrase(encryptedAgePrivKeyData, keychainData.AgePrivKeyPassphrase)
+	// Create secure buffer for the keychain passphrase
+	passphraseBuffer := memguard.NewBufferFromBytes([]byte(keychainData.AgePrivKeyPassphrase))
+	defer passphraseBuffer.Destroy()
+
+	agePrivKeyBuffer, err := DecryptWithPassphrase(encryptedAgePrivKeyData, passphraseBuffer)
 	if err != nil {
 		Debug("Failed to decrypt age private key with keychain passphrase", "error", err, "unlocker_id", k.GetID())
 
@@ -363,7 +369,6 @@ func CreateKeychainUnlocker(fs afero.Fs, stateDir string) (*KeychainUnlocker, er
 	if err != nil {
 		return nil, fmt.Errorf("failed to generate age private key passphrase: %w", err)
 	}
-	defer agePrivKeyPassphrase.Destroy()
 
 	// Step 3: Store age recipient as plaintext
 	ageRecipient := ageIdentity.Recipient().String()
@@ -373,12 +378,15 @@ func CreateKeychainUnlocker(fs afero.Fs, stateDir string) (*KeychainUnlocker, er
 	}
 
 	// Step 4: Encrypt age private key with the generated passphrase and store on disk
-	// Create a secure buffer for the private key
+	// Create secure buffers for both the private key and passphrase
 	agePrivKeyStr := ageIdentity.String()
 	agePrivKeyBuffer := memguard.NewBufferFromBytes([]byte(agePrivKeyStr))
 	defer agePrivKeyBuffer.Destroy()
 
-	encryptedAgePrivKey, err := EncryptWithPassphrase(agePrivKeyBuffer, agePrivKeyPassphrase)
+	passphraseBuffer := memguard.NewBufferFromBytes([]byte(agePrivKeyPassphrase))
+	defer passphraseBuffer.Destroy()
+
+	encryptedAgePrivKey, err := EncryptWithPassphrase(agePrivKeyBuffer, passphraseBuffer)
 	if err != nil {
 		return nil, fmt.Errorf("failed to encrypt age private key with passphrase: %w", err)
 	}
@@ -414,10 +422,13 @@ func CreateKeychainUnlocker(fs afero.Fs, stateDir string) (*KeychainUnlocker, er
 		EncryptedLongtermKey: hex.EncodeToString(encryptedLtPrivKeyToAge),
 	}
 
-	keychainDataBuffer, err := keychainData.encode()
+	keychainDataBytes, err := json.Marshal(keychainData)
 	if err != nil {
-		return nil, fmt.Errorf("failed to encode keychain data: %w", err)
+		return nil, fmt.Errorf("failed to marshal keychain data: %w", err)
 	}
+
+	// Create a secure buffer for keychain data
+	keychainDataBuffer := memguard.NewBufferFromBytes(keychainDataBytes)
 	defer keychainDataBuffer.Destroy()
 
 	// Step 8: Store data in keychain
@@ -490,7 +501,7 @@ func storeInKeychain(itemName string, data *memguard.LockedBuffer) error {
 	item.SetAccount(itemName)
 	item.SetLabel(fmt.Sprintf("%s - %s", KEYCHAIN_APP_IDENTIFIER, itemName))
 	item.SetDescription("Secret vault keychain data")
-	item.SetData(data.Bytes())
+	item.SetData([]byte(data.String()))
 	item.SetSynchronizable(keychain.SynchronizableNo)
 	// Use AccessibleWhenUnlockedThisDeviceOnly for better security and to trigger auth
 	item.SetAccessible(keychain.AccessibleWhenUnlockedThisDeviceOnly)
@@ -565,3 +576,8 @@ func deleteFromKeychain(itemName string) error {
 
 	return nil
 }
+
+// generateRandomPassphrase generates a random passphrase for encrypting the age private key
+func generateRandomPassphrase(length int) (string, error) {
+	return generateRandomString(length, "0123456789abcdef")
+}

internal/secret/secret_test.go

@@ -310,6 +310,64 @@ func TestPerSecretKeyFunctionality(t *testing.T) {
 	})
 }
 
+// For testing purposes only
+func isValidSecretName(name string) bool {
+	if name == "" {
+		return false
+	}
+	// Valid characters for secret names: letters, numbers, dash, dot, underscore, slash
+	for _, char := range name {
+		if (char < 'a' || char > 'z') && // lowercase letters
+			(char < 'A' || char > 'Z') && // uppercase letters
+			(char < '0' || char > '9') && // numbers
+			char != '-' && // dash
+			char != '.' && // dot
+			char != '_' && // underscore
+			char != '/' { // slash
+			return false
+		}
+	}
+
+	return true
+}
+
+func TestSecretNameValidation(t *testing.T) {
+	t.Parallel()
+
+	tests := []struct {
+		name  string
+		valid bool
+	}{
+		{"valid-name", true},
+		{"valid.name", true},
+		{"valid_name", true},
+		{"valid/path/name", true},
+		{"123valid", true},
+		{"", false},
+		{"Valid-Upper-Name", true}, // uppercase allowed
+		{"2025-11-21-ber1app1-vaultik-test-bucket-AKI", true}, // real-world uppercase key ID
+		{"MixedCase/Path/Name", true},                         // mixed case with path
+		{"invalid name", false},                               // space not allowed
+		{"invalid@name", false},                               // @ not allowed
+	}
+
+	for _, test := range tests {
+		t.Run(test.name, func(t *testing.T) {
+			t.Parallel()
+
+			result := isValidSecretName(test.name)
+			if result != test.valid {
+				t.Errorf(
+					"isValidSecretName(%q) = %v, want %v",
+					test.name,
+					result,
+					test.valid,
+				)
+			}
+		})
+	}
+}
+
 func TestSecretGetValueWithEnvMnemonicUsesVaultDerivationIndex(t *testing.T) {
 	// This test demonstrates the bug where GetValue uses hardcoded index 0
 	// instead of the vault's actual derivation index when using environment mnemonic

internal/secret/version.go

@@ -6,7 +6,6 @@ import (
 	"fmt"
 	"log/slog"
 	"path/filepath"
-	"slices"
 	"sort"
 	"strings"
 	"time"
@@ -546,18 +545,6 @@ func ListVersions(fs afero.Fs, secretDir string) ([]string, error) {
 	return versions, nil
 }
 
-// VersionExists reports whether version is one of the versions ListVersions
-// lists for the secret in secretDir. It only compares names, so a version
-// the user typed can be checked with it before any path is built from it.
-func VersionExists(fs afero.Fs, secretDir string, version string) (bool, error) {
-	versions, err := ListVersions(fs, secretDir)
-	if err != nil {
-		return false, err
-	}
-
-	return slices.Contains(versions, version), nil
-}
-
 // GetCurrentVersion returns the version that the "current" file points to
 // The file contains just the version name (e.g., "20231215.001")
 func GetCurrentVersion(fs afero.Fs, secretDir string) (string, error) {

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 ASCII 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
@@ -49,7 +48,7 @@ var (
 
 	// ErrVersionNotFound indicates the requested secret version does not
 	// exist. Composed as
-	// "version '<version>' not found for secret '<name>'".
+	// "version <version> not found for secret <name>".
 	ErrVersionNotFound = errors.New("not found for secret")
 
 	// ErrNoVersions indicates the source secret has no versions. Composed

internal/vault/integration_version_test.go

@@ -235,10 +235,10 @@ func testRetrieveSpecificVersions(
 	require.NoError(t, err)
 	assert.Equal(t, []byte("version-3-data"), value3)
 
-	// An empty version is not one of the versions; GetSecret gets the
-	// current one
-	_, err = vault.GetSecretVersion(secretName, "")
-	require.ErrorIs(t, err, ErrVersionNotFound)
+	// Empty version should return current
+	valueCurrent, err := vault.GetSecretVersion(secretName, "")
+	require.NoError(t, err)
+	assert.Equal(t, []byte("version-3-data"), valueCurrent)
 }
 
 func testPromoteOldVersion(

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
@@ -111,22 +110,6 @@ 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 ASCII 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 {
@@ -141,11 +124,13 @@ func (v *Vault) AddSecret(name string, value *memguard.LockedBuffer, force bool)
 	)
 
 	// Validate secret name
-	err := ValidateSecretName(name)
-	if err != nil {
+	if !isValidSecretName(name) {
 		secret.Debug("Invalid secret name provided", "secret_name", name)
 
-		return err
+		return fmt.Errorf(
+			"%w '%s': must match pattern [a-z0-9.\\-_/]+",
+			ErrInvalidSecretName, name,
+		)
 	}
 
 	secret.Debug("Secret name validation passed", "secret_name", name)
@@ -301,31 +286,18 @@ func updateVersionMetadata(
 	return nil
 }
 
-// GetSecret retrieves the current version of a secret from this vault
+// GetSecret retrieves a secret from this vault
 func (v *Vault) GetSecret(name string) ([]byte, error) {
 	secret.DebugWith("Getting secret from vault",
 		slog.String("vault_name", v.Name),
 		slog.String("secret_name", name),
 	)
 
-	// GetSecretObject validates the name and checks that the secret exists
-	secretObj, err := v.GetSecretObject(name)
-	if err != nil {
-		return nil, err
-	}
-
-	currentVersion, err := secret.GetCurrentVersion(v.fs, secretObj.Directory)
-	if err != nil {
-		secret.Debug("Failed to get current version", "error", err, "secret_name", name)
-
-		return nil, fmt.Errorf("failed to get current version: %w", err)
-	}
-
-	return v.GetSecretVersion(name, currentVersion)
+	return v.GetSecretVersion(name, "")
 }
 
-// GetSecretVersion retrieves a specific version of a secret. The version
-// must be one of the secret's versions; GetSecret gets the current one.
+// GetSecretVersion retrieves a specific version of a secret (empty version
+// means current)
 func (v *Vault) GetSecretVersion(name string, version string) ([]byte, error) {
 	secret.DebugWith("Getting secret version from vault",
 		slog.String("vault_name", v.Name),
@@ -333,8 +305,8 @@ func (v *Vault) GetSecretVersion(name string, version string) ([]byte, error) {
 		slog.String("version", version),
 	)
 
-	// Validate the name and check that the version exists
-	err := v.checkSecretVersion(name, version)
+	// Validate the name and resolve the version to fetch
+	version, err := v.resolveSecretVersion(name, version)
 	if err != nil {
 		return nil, err
 	}
@@ -428,9 +400,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
@@ -679,15 +650,17 @@ func (v *Vault) updatePreviousVersion(
 	return nil
 }
 
-// checkSecretVersion validates the secret name and verifies that the secret
-// exists and that version is one of its versions.
-func (v *Vault) checkSecretVersion(name, version string) error {
+// resolveSecretVersion validates the secret name, verifies the secret and
+// 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
-	err := ValidateSecretName(name)
-	if err != nil {
+	if !isValidSecretName(name) {
 		secret.Debug("Invalid secret name provided", "secret_name", name)
 
-		return err
+		return "", fmt.Errorf(
+			"%w '%s': must match pattern [a-z0-9.\\-_/]+",
+			ErrInvalidSecretName, name,
+		)
 	}
 
 	// Get vault directory
@@ -695,7 +668,7 @@ func (v *Vault) checkSecretVersion(name, version string) error {
 	if err != nil {
 		secret.Debug("Failed to get vault directory", "error", err, "vault_name", v.Name)
 
-		return err
+		return "", err
 	}
 
 	// Convert slashes to percent signs for storage
@@ -707,30 +680,50 @@ func (v *Vault) checkSecretVersion(name, version string) error {
 	if err != nil {
 		secret.Debug("Failed to check if secret exists", "error", err, "secret_name", name)
 
-		return fmt.Errorf("failed to check if secret exists: %w", err)
+		return "", fmt.Errorf("failed to check if secret exists: %w", err)
 	}
 
 	if !exists {
 		secret.Debug("Secret not found in vault", "secret_name", name, "vault_name", v.Name)
 
-		return fmt.Errorf("secret %s %w", name, ErrSecretNotFound)
+		return "", fmt.Errorf("secret %s %w", name, ErrSecretNotFound)
+	}
+
+	// Determine which version to get
+	if version == "" {
+		// Get current version
+		currentVersion, err := secret.GetCurrentVersion(v.fs, secretDir)
+		if err != nil {
+			secret.Debug("Failed to get current version", "error", err, "secret_name", name)
+
+			return "", fmt.Errorf("failed to get current version: %w", err)
+		}
+
+		version = currentVersion
+
+		secret.Debug("Using current version", "version", version, "secret_name", name)
 	}
 
 	// Check if version exists
-	exists, err = secret.VersionExists(v.fs, secretDir, version)
+	versionPath := filepath.Join(secretDir, "versions", version)
+
+	exists, err = afero.DirExists(v.fs, versionPath)
 	if err != nil {
 		secret.Debug("Failed to check if version exists", "error", err, "version", version)
 
-		return fmt.Errorf("failed to check if version exists: %w", err)
+		return "", fmt.Errorf("failed to check if version exists: %w", err)
 	}
 
 	if !exists {
 		secret.Debug("Version not found", "version", version, "secret_name", name)
 
-		return fmt.Errorf("version '%s' %w '%s'", version, ErrVersionNotFound, name)
+		return "", fmt.Errorf(
+			"version %s %w %s",
+			version, ErrVersionNotFound, name,
+		)
 	}
 
-	return nil
+	return version, nil
 }
 
 // createAndSaveVersion generates a new version name, sets the version

internal/vault/secrets_version_test.go

@@ -202,10 +202,10 @@ func TestVaultGetSecretVersion(t *testing.T) {
 	require.NoError(t, err)
 	assert.Equal(t, []byte("version-2"), value)
 
-	// An empty version is not one of the versions; GetSecret gets the
-	// current one
-	_, err = vault.GetSecretVersion(testSecretPath, "")
-	require.ErrorIs(t, err, ErrVersionNotFound)
+	// Get current (empty version)
+	value, err = vault.GetSecretVersion(testSecretPath, "")
+	require.NoError(t, err)
+	assert.Equal(t, []byte("version-2"), value)
 }
 
 //nolint:paralleltest // createTestVaultWithKey uses t.Setenv