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

`secret rm ..` 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 unchecked; import checked it only after
reading the source file.

Each now calls vault.ValidateSecretName, which wraps the existing rule,
on the name as given, before building any path; MoveSecret checks both
names before switching the current vault. Its error and README.md state
the rule. The test-only copy of the rule in internal/secret is removed.

The regression test runs each rejected command on a copy of two
in-memory vaults 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 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.
 - Forward slashes (`/`) are converted to percent signs (`%`) for storage
 - Examples: `database/password`, `api.key`, `ssh_private_key`
 

TODO.md

@@ -25,34 +25,13 @@ Bring the repo into policy compliance in one commit:
 
 # Completed Steps
 
-- 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: 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
@@ -126,8 +105,9 @@ Bring the repo into policy compliance in one commit:
       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).
+    - Race conditions: no file locking in vault/secrets.go:142-176;
+      non-atomic writes can leave the vault inconsistent.
+    - 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

@@ -72,18 +72,10 @@ func newDecryptCmd() *cobra.Command {
 
 // 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.
+// does not exist. The caller must destroy the returned buffer.
 func (cli *Instance) resolveEncryptionKey(
 	vlt *vault.Vault, secretName string,
 ) (*memguard.LockedBuffer, error) {
-	release, err := vault.LockStateDir(cli.fs, cli.stateDir)
-	if err != nil {
-		return nil, err
-	}
-	defer release()
-
 	// Check if secret exists
 	secretObj := secret.NewSecret(vlt, secretName)
 
@@ -130,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 {
@@ -199,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/generate.go

@@ -155,12 +155,6 @@ func (cli *Instance) GenerateSecret(
 		return fmt.Errorf("failed to generate random secret: %w", err)
 	}
 
-	release, err := vault.LockStateDir(cli.fs, cli.stateDir)
-	if err != nil {
-		return err
-	}
-	defer release()
-
 	// Store the secret in the vault
 	vlt, err := vault.GetCurrentVault(cli.fs, cli.stateDir)
 	if err != nil {

internal/cli/init.go

@@ -103,21 +103,8 @@ func (cli *Instance) setupDefaultVault(
 	return vlt, ltIdentity, nil
 }
 
-// Init initializes the secret manager, holding the state directory lock
-// while initialize runs
+// Init initializes the secret manager
 func (cli *Instance) Init(cmd *cobra.Command) error {
-	release, err := vault.LockStateDir(cli.fs, cli.stateDir)
-	if err != nil {
-		return err
-	}
-	defer release()
-
-	return cli.initialize(cmd)
-}
-
-// initialize creates the state directory, the default vault and its first
-// unlocker
-func (cli *Instance) initialize(cmd *cobra.Command) error {
 	secret.Debug("Starting secret manager initialization")
 
 	// Create state directory

internal/cli/lock_test.go

@@ -1,493 +0,0 @@
-//nolint:testpackage // sets the unexported fields of Instance
-package cli
-
-import (
-	"io"
-	"os"
-	"path/filepath"
-	"strconv"
-	"strings"
-	"sync"
-	"testing"
-	"time"
-
-	"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(
-	fs afero.Fs, stateDir, name string, force bool, values []string,
-) []error {
-	errs := make(chan error, len(values))
-
-	for _, value := range values {
-		go func() {
-			cli := NewCLIInstanceWithStateDir(fs, stateDir)
-			cli.cmd = &cobra.Command{}
-			cli.cmd.SetIn(strings.NewReader(value))
-
-			errs <- cli.AddSecret(name, force)
-		}()
-	}
-
-	results := make([]error, 0, len(values))
-	for range values {
-		results = append(results, <-errs)
-	}
-
-	return results
-}
-
-// numbered returns count distinct values starting with prefix.
-func numbered(prefix string, count int) []string {
-	values := make([]string, 0, count)
-	for i := range count {
-		values = append(values, prefix+"-"+strconv.Itoa(i))
-	}
-
-	return values
-}
-
-// TestConcurrentAddsKeepEveryVersion runs adds of one secret at once, on
-// the in-memory and on the real filesystem. Without the state directory
-// lock, adds of a new secret all find it absent and replace each other, and
-// forced adds read the same highest version number and overwrite each
-// other's version. With it they behave as if run one after another.
-//
-//nolint:paralleltest // t.Setenv forbids parallel subtests
-func TestConcurrentAddsKeepEveryVersion(t *testing.T) {
-	t.Setenv(secret.EnvMnemonic, testMnemonic)
-
-	const adds = 8
-
-	for _, tc := range []struct {
-		name     string
-		fs       afero.Fs
-		stateDir string
-	}{
-		{"memory", afero.NewMemMapFs(), testStateDir},
-		{"real", afero.NewOsFs(), t.TempDir()},
-	} {
-		t.Run(tc.name, func(t *testing.T) {
-			_, err := vault.CreateVault(tc.fs, tc.stateDir, "default")
-			require.NoError(t, err)
-
-			// One add creates the secret; the others find that it exists
-			created := 0
-
-			for _, err := range addAtOnce(tc.fs, tc.stateDir, "shared", false,
-				numbered("create", adds)) {
-				if err == nil {
-					created++
-				} else {
-					require.ErrorIs(t, err, vault.ErrSecretExists)
-				}
-			}
-
-			require.Equal(t, 1, created, "exactly one add creates the secret")
-
-			// Every forced add stores a version of its own
-			for _, err := range addAtOnce(tc.fs, tc.stateDir, "shared", true,
-				numbered("force", adds)) {
-				require.NoError(t, err)
-			}
-
-			vlt, err := vault.GetCurrentVault(tc.fs, tc.stateDir)
-			require.NoError(t, err)
-
-			vaultDir, err := vlt.GetDirectory()
-			require.NoError(t, err)
-
-			versions, err := secret.ListVersions(tc.fs,
-				filepath.Join(vaultDir, "secrets.d", "shared"))
-			require.NoError(t, err)
-			require.Len(t, versions, adds+1, "one version per successful add")
-
-			values := make(map[string]bool, len(versions))
-
-			for _, version := range versions {
-				value, err := vlt.GetSecretVersion("shared", version)
-				require.NoError(t, err)
-
-				values[string(value)] = true
-			}
-
-			assert.Len(t, values, adds+1, "every add stored its own value")
-		})
-	}
-}
-
-// readNotifier passes reads through to Reader and closes reading at the
-// first one.
-type readNotifier struct {
-	io.Reader
-
-	reading chan struct{}
-	once    sync.Once
-}
-
-func (r *readNotifier) Read(p []byte) (int, error) {
-	r.once.Do(func() { close(r.reading) })
-
-	return r.Reader.Read(p)
-}
-
-// TestEncryptPipedIntoAdd runs `secret encrypt key | secret add name` in
-// one process, starting encrypt once add is reading its input. Had add
-// taken the state directory lock before reading, it would hold the lock
-// while waiting for encrypt's output, and encrypt would wait for the lock
-// to store its key: neither would finish.
-func TestEncryptPipedIntoAdd(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("piped"), 0o600))
-
-	pipeReader, pipeWriter := io.Pipe()
-	// If the test gives up, this makes add's read fail, so that both
-	// commands return and release the lock the other tests use
-	t.Cleanup(func() { _ = pipeReader.Close() })
-
-	const commands = 2
-
-	input := &readNotifier{Reader: pipeReader, reading: make(chan struct{})}
-	results := make(chan error, commands)
-
-	go func() {
-		add := NewCLIInstanceWithStateDir(fs, testStateDir)
-		add.cmd = &cobra.Command{}
-		add.cmd.SetIn(input)
-
-		results <- add.AddSecret("encrypted", false)
-	}()
-
-	go func() {
-		<-input.reading
-
-		encrypt := NewCLIInstanceWithStateDir(fs, testStateDir)
-		encrypt.cmd = &cobra.Command{}
-		encrypt.cmd.SetOut(pipeWriter)
-
-		err := encrypt.Encrypt("key", testInput, "")
-		// Ends add's input, as the end of the pipe does
-		_ = pipeWriter.CloseWithError(err)
-
-		results <- err
-	}()
-
-	timeout := time.After(lockWait)
-
-	for range commands {
-		select {
-		case err := <-results:
-			require.NoError(t, err)
-		case <-timeout:
-			t.Fatal("secret encrypt piped into secret add never finished")
-		}
-	}
-}
-
-// TestFailedCommandReleasesLock checks that a command failing after it
-// took the state directory lock leaves the lock free for the next command.
-func TestFailedCommandReleasesLock(t *testing.T) {
-	t.Parallel()
-
-	fs := afero.NewMemMapFs()
-	cli := NewCLIInstanceWithStateDir(fs, testStateDir)
-
-	// Fails once it holds the lock: there is no current vault
-	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)
-		})
-	}
-}
-
-// 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)
-
-	go func() {
-		encrypt := NewCLIInstanceWithStateDir(fs, testStateDir)
-		encrypt.cmd = &cobra.Command{}
-		encrypt.cmd.SetOut(outputWriter)
-
-		err := encrypt.Encrypt("key", testInput, "")
-		_ = outputWriter.CloseWithError(err)
-
-		done <- err
-	}()
-
-	// 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")
-	}
-
-	_, err = io.Copy(io.Discard, outputReader)
-	require.NoError(t, err)
-	require.NoError(t, <-done)
-}

internal/cli/path_traversal_test.go

@@ -0,0 +1,267 @@
+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)
+		}},
+		{`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)
+		})
+	}
+}
+
+// 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/secrets.go

@@ -377,15 +377,6 @@ func (cli *Instance) AddSecret(secretName string, force bool) error {
 	valueBuffer := combineBuffers(buffers, totalSize)
 	defer valueBuffer.Destroy()
 
-	// Locked only now that stdin has been read: in `secret encrypt key |
-	// secret add name`, holding the lock while reading would leave each
-	// command waiting for the other.
-	release, err := vault.LockStateDir(cli.fs, cli.stateDir)
-	if err != nil {
-		return err
-	}
-	defer release()
-
 	// Add the secret to the vault
 	secret.Debug("Calling vault.AddSecret", "secret_name", secretName,
 		"value_length", valueBuffer.Size(), "force", force)
@@ -612,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 {
@@ -644,14 +640,6 @@ func (cli *Instance) ImportSecret(
 	valueBuffer := combineBuffers(buffers, totalSize)
 	defer valueBuffer.Destroy()
 
-	// Locked only now that the file has been read, as in AddSecret: the
-	// file may be a pipe written by another secret command.
-	release, err := vault.LockStateDir(cli.fs, cli.stateDir)
-	if err != nil {
-		return err
-	}
-	defer release()
-
 	// Store the secret in the vault
 	err = vlt.AddSecret(secretName, valueBuffer, force)
 	if err != nil {
@@ -666,11 +654,10 @@ func (cli *Instance) ImportSecret(
 
 // RemoveSecret removes a secret from the vault
 func (cli *Instance) RemoveSecret(cmd *cobra.Command, secretName string, _ bool) error {
-	release, err := vault.LockStateDir(cli.fs, cli.stateDir)
+	err := vault.ValidateSecretName(secretName)
 	if err != nil {
 		return err
 	}
-	defer release()
 
 	// Get current vault
 	currentVlt, err := vault.GetCurrentVault(cli.fs, cli.stateDir)
@@ -706,7 +693,7 @@ func (cli *Instance) RemoveSecret(cmd *cobra.Command, secretName string, _ bool)
 	}
 
 	// Remove the secret directory
-	err = secret.RemoveDirAtomic(cli.fs, secretDir)
+	err = cli.fs.RemoveAll(secretDir)
 	if err != nil {
 		return fmt.Errorf("failed to remove secret: %w", err)
 	}
@@ -721,23 +708,12 @@ func (cli *Instance) RemoveSecret(cmd *cobra.Command, secretName string, _ bool)
 func (cli *Instance) MoveSecret(
 	cmd *cobra.Command, source, dest string, force bool,
 ) error {
-	release, err := vault.LockStateDir(cli.fs, cli.stateDir)
-	if err != nil {
-		return err
-	}
-	defer release()
-
 	// 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 {
+	if !srcQualified && destQualified {
 		return errCrossVaultSourceUnqualified
 	}
 
@@ -745,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)
 		}
@@ -782,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 {
@@ -821,7 +813,7 @@ func (cli *Instance) moveSecretWithinVault(
 			return fmt.Errorf("secret '%s' %w", dest, errSecretExistsNoForce)
 		}
 
-		err = secret.RemoveDirAtomic(cli.fs, destDir)
+		err = cli.fs.RemoveAll(destDir)
 		if err != nil {
 			return fmt.Errorf("failed to remove existing destination: %w", err)
 		}
@@ -837,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,
@@ -901,7 +894,7 @@ func (cli *Instance) moveSecretCrossVault(
 	}
 
 	// Delete source secret
-	err = secret.RemoveDirAtomic(cli.fs, srcSecretDir)
+	err = cli.fs.RemoveAll(srcSecretDir)
 	if err != nil {
 		// Copy succeeded but delete failed - warn but don't fail
 		cmd.Printf("Warning: copied secret but failed to remove source: %v\n", err)

internal/cli/unlockers.go

@@ -534,12 +534,6 @@ func (cli *Instance) printUnlockersTable(unlockers []UnlockerInfo) error {
 
 // UnlockersAdd adds a new unlocker
 func (cli *Instance) UnlockersAdd(unlockerType string, cmd *cobra.Command) error {
-	release, err := vault.LockStateDir(cli.fs, cli.stateDir)
-	if err != nil {
-		return err
-	}
-	defer release()
-
 	switch unlockerType {
 	case unlockerTypePassphrase:
 		return cli.addPassphraseUnlocker(cmd)
@@ -720,12 +714,6 @@ func (cli *Instance) addPGPUnlocker(cmd *cobra.Command) error {
 func (cli *Instance) UnlockersRemove(
 	unlockerID string, force bool, cmd *cobra.Command,
 ) error {
-	release, err := vault.LockStateDir(cli.fs, cli.stateDir)
-	if err != nil {
-		return err
-	}
-	defer release()
-
 	// Get current vault
 	vlt, err := vault.GetCurrentVault(cli.fs, cli.stateDir)
 	if err != nil {
@@ -775,12 +763,6 @@ func (cli *Instance) UnlockersRemove(
 
 // UnlockerSelect selects an unlocker as current
 func (cli *Instance) UnlockerSelect(unlockerID string) error {
-	release, err := vault.LockStateDir(cli.fs, cli.stateDir)
-	if err != nil {
-		return err
-	}
-	defer release()
-
 	// Get current vault
 	vlt, err := vault.GetCurrentVault(cli.fs, cli.stateDir)
 	if err != nil {

internal/cli/vault.go

@@ -267,12 +267,6 @@ func resolvePassphrase() (*memguard.LockedBuffer, error) {
 func (cli *Instance) CreateVault(cmd *cobra.Command, name string) error {
 	secret.Debug("Creating new vault", "name", name, "state_dir", cli.stateDir)
 
-	release, err := vault.LockStateDir(cli.fs, cli.stateDir)
-	if err != nil {
-		return err
-	}
-	defer release()
-
 	// Get or prompt for mnemonic
 	var mnemonicStr string
 
@@ -360,13 +354,7 @@ func (cli *Instance) CreateVault(cmd *cobra.Command, name string) error {
 
 // SelectVault selects a vault as the current one
 func (cli *Instance) SelectVault(cmd *cobra.Command, name string) error {
-	release, err := vault.LockStateDir(cli.fs, cli.stateDir)
-	if err != nil {
-		return err
-	}
-	defer release()
-
-	err = vault.SelectVault(cli.fs, cli.stateDir, name)
+	err := vault.SelectVault(cli.fs, cli.stateDir, name)
 	if err != nil {
 		return err
 	}
@@ -454,21 +442,8 @@ func updateVaultImportMetadata(
 	return nil
 }
 
-// VaultImport imports a mnemonic into a specific vault, holding the state
-// directory lock while importMnemonic runs
+// VaultImport imports a mnemonic into a specific vault
 func (cli *Instance) VaultImport(cmd *cobra.Command, vaultName string) error {
-	release, err := vault.LockStateDir(cli.fs, cli.stateDir)
-	if err != nil {
-		return err
-	}
-	defer release()
-
-	return cli.importMnemonic(cmd, vaultName)
-}
-
-// importMnemonic gives the vault a long-term key derived from the mnemonic
-// and a passphrase unlocker
-func (cli *Instance) importMnemonic(cmd *cobra.Command, vaultName string) error {
 	secret.Debug("Importing mnemonic into vault",
 		"vault_name", vaultName, "state_dir", cli.stateDir)
 
@@ -503,7 +478,7 @@ func (cli *Instance) importMnemonic(cmd *cobra.Command, vaultName string) error
 	secret.Debug("Storing long-term public key",
 		"pubkey", ltPublicKey, "vault_dir", vaultDir)
 
-	err = secret.WriteFileAtomic(cli.fs, pubKeyPath, []byte(ltPublicKey))
+	err = afero.WriteFile(cli.fs, pubKeyPath, []byte(ltPublicKey), secret.FilePerms)
 	if err != nil {
 		return fmt.Errorf("failed to store long-term public key: %w", err)
 	}
@@ -602,12 +577,6 @@ func (cli *Instance) switchAwayFromVault(
 
 // RemoveVault removes a vault with safety checks
 func (cli *Instance) RemoveVault(cmd *cobra.Command, name string, force bool) error {
-	release, err := vault.LockStateDir(cli.fs, cli.stateDir)
-	if err != nil {
-		return err
-	}
-	defer release()
-
 	// Get list of all vaults
 	vaults, err := vault.ListVaults(cli.fs, cli.stateDir)
 	if err != nil {
@@ -657,7 +626,7 @@ func (cli *Instance) RemoveVault(cmd *cobra.Command, name string, force bool) er
 	}
 
 	// Remove the vault directory
-	err = secret.RemoveDirAtomic(cli.fs, vaultDir)
+	err = cli.fs.RemoveAll(vaultDir)
 	if err != nil {
 		return fmt.Errorf("failed to remove vault directory: %w", err)
 	}

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,11 +244,10 @@ func formatVersionTime(t *time.Time) string {
 func (cli *Instance) PromoteVersion(
 	cmd *cobra.Command, secretName string, version string,
 ) error {
-	release, err := vault.LockStateDir(cli.fs, cli.stateDir)
+	err := vault.ValidateSecretName(secretName)
 	if err != nil {
 		return err
 	}
-	defer release()
 
 	// Get current vault
 	vlt, err := vault.GetCurrentVault(cli.fs, cli.stateDir)
@@ -288,11 +292,10 @@ func (cli *Instance) PromoteVersion(
 func (cli *Instance) RemoveVersion(
 	cmd *cobra.Command, secretName string, version string,
 ) error {
-	release, err := vault.LockStateDir(cli.fs, cli.stateDir)
+	err := vault.ValidateSecretName(secretName)
 	if err != nil {
 		return err
 	}
-	defer release()
 
 	// Get current vault
 	vlt, err := vault.GetCurrentVault(cli.fs, cli.stateDir)
@@ -345,7 +348,7 @@ func (cli *Instance) RemoveVersion(
 	}
 
 	// Remove the version directory
-	err = secret.RemoveDirAtomic(cli.fs, versionDir)
+	err = cli.fs.RemoveAll(versionDir)
 	if err != nil {
 		return fmt.Errorf("failed to remove version: %w", err)
 	}

internal/secret/atomic.go

@@ -1,86 +0,0 @@
-package secret
-
-import (
-	"fmt"
-	"path/filepath"
-
-	"github.com/spf13/afero"
-)
-
-// WriteFileAtomic replaces the file at path with data so that a reader, or
-// a crash at any moment, finds either the old content or the new, never a
-// partial file. The data goes into a temporary file that afero.TempFile
-// creates with mode 0600 in the same directory (a rename is only atomic
-// within one filesystem), is synced to disk, and is renamed over path. The
-// temporary file is removed if any step fails.
-func WriteFileAtomic(fs afero.Fs, path string, data []byte) error {
-	tmp, err := afero.TempFile(fs, filepath.Dir(path),
-		"."+filepath.Base(path)+".tmp-*")
-	if err != nil {
-		return fmt.Errorf("failed to create temporary file for %s: %w", path, err)
-	}
-
-	_, err = tmp.Write(data)
-	if err == nil {
-		err = tmp.Sync()
-	}
-
-	closeErr := tmp.Close()
-	if err == nil {
-		err = closeErr
-	}
-
-	if err == nil {
-		err = fs.Rename(tmp.Name(), path)
-	}
-
-	if err != nil {
-		_ = fs.Remove(tmp.Name())
-
-		return fmt.Errorf("failed to write %s: %w", path, err)
-	}
-
-	return nil
-}
-
-// TempDirFor creates an empty temporary directory in which to build the
-// directory target before renaming it into place, or into which to move
-// target before deleting it. It is made in target's grandparent: on the
-// same filesystem, so the rename is atomic, and outside target's parent,
-// the directory that is listed to find vaults, secrets, versions and
-// unlockers, so one left behind by a crash is never taken for one of them.
-// Its name leaves out target's, which may already be as long as a file name
-// can be.
-func TempDirFor(fs afero.Fs, target string) (string, error) {
-	dir, err := afero.TempDir(fs, filepath.Dir(filepath.Dir(target)), ".tmp-")
-	if err != nil {
-		return "", fmt.Errorf(
-			"failed to create temporary directory for %s: %w", target, err)
-	}
-
-	return dir, nil
-}
-
-// RemoveDirAtomic deletes the directory dir so that it disappears in one
-// rename: dir is moved into a new directory from TempDirFor, which is then
-// deleted. A crash part-way leaves only that temporary directory behind.
-func RemoveDirAtomic(fs afero.Fs, dir string) error {
-	tmp, err := TempDirFor(fs, dir)
-	if err != nil {
-		return err
-	}
-
-	err = fs.Rename(dir, filepath.Join(tmp, filepath.Base(dir)))
-	if err != nil {
-		_ = fs.Remove(tmp)
-
-		return fmt.Errorf("failed to remove %s: %w", dir, err)
-	}
-
-	err = fs.RemoveAll(tmp)
-	if err != nil {
-		return fmt.Errorf("failed to remove %s: %w", dir, err)
-	}
-
-	return nil
-}

internal/secret/atomic_test.go

@@ -1,672 +0,0 @@
-package secret_test
-
-import (
-	"errors"
-	"os"
-	"path/filepath"
-	"strings"
-	"testing"
-
-	"filippo.io/age"
-	"git.eeqj.de/sneak/secret/internal/secret"
-	"git.eeqj.de/sneak/secret/internal/vault"
-	"github.com/awnumar/memguard"
-	"github.com/spf13/afero"
-	"github.com/stretchr/testify/assert"
-	"github.com/stretchr/testify/require"
-)
-
-var errInjected = errors.New("injected failure")
-
-// The kinds of change hookFs passes to before.
-const (
-	opCreate = "create"
-	opOpen   = "open"
-	opSync   = "sync"
-	opMkdir  = "mkdir"
-	opRemove = "remove"
-	opRename = "rename"
-)
-
-// currentFile is the file in a secret's directory that names its current
-// 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.
-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
-func (h hookFs) Create(name string) (afero.File, error) {
-	err := h.before(opCreate, name)
-	if err != nil {
-		return nil, err
-	}
-
-	file, err := h.Fs.Create(name)
-	if err != nil {
-		return nil, err
-	}
-
-	return hookFile{File: file, before: h.before}, nil
-}
-
-//nolint:ireturn // implements afero.Fs
-func (h hookFs) OpenFile(
-	name string, flag int, perm os.FileMode,
-) (afero.File, error) {
-	err := h.before(opOpen, name)
-	if err != nil {
-		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
-}
-
-func (h hookFs) Mkdir(name string, perm os.FileMode) error {
-	err := h.before(opMkdir, name)
-	if err != nil {
-		return err
-	}
-
-	return h.Fs.Mkdir(name, perm)
-}
-
-func (h hookFs) MkdirAll(path string, perm os.FileMode) error {
-	err := h.before(opMkdir, path)
-	if err != nil {
-		return err
-	}
-
-	return h.Fs.MkdirAll(path, perm)
-}
-
-func (h hookFs) Remove(name string) error {
-	err := h.before(opRemove, name)
-	if err != nil {
-		return err
-	}
-
-	return h.Fs.Remove(name)
-}
-
-func (h hookFs) RemoveAll(path string) error {
-	err := h.before(opRemove, path)
-	if err != nil {
-		return err
-	}
-
-	return h.Fs.RemoveAll(path)
-}
-
-func (h hookFs) Rename(oldname, newname string) error {
-	err := h.before(opRename, newname)
-	if err != nil {
-		return err
-	}
-
-	return h.Fs.Rename(oldname, newname)
-}
-
-// testFilesystem is a filesystem to run a test on, with a directory in it
-// to work in.
-type testFilesystem struct {
-	name string
-	open func(t *testing.T) (afero.Fs, string)
-}
-
-// testFilesystems are the in-memory filesystem that most tests use and the
-// real one: every rename-based guarantee is checked on both.
-//
-//nolint:gochecknoglobals // read-only table shared by the tests below
-var testFilesystems = []testFilesystem{
-	{"memory", func(*testing.T) (afero.Fs, string) {
-		return afero.NewMemMapFs(), "/test"
-	}},
-	{"real", func(t *testing.T) (afero.Fs, string) {
-		t.Helper()
-
-		return afero.NewOsFs(), t.TempDir()
-	}},
-}
-
-// dirNames lists the names in dir.
-func dirNames(t *testing.T, fs afero.Fs, dir string) []string {
-	t.Helper()
-
-	entries, err := afero.ReadDir(fs, dir)
-	require.NoError(t, err)
-
-	names := make([]string, 0, len(entries))
-	for _, entry := range entries {
-		names = append(names, entry.Name())
-	}
-
-	return names
-}
-
-// writeLongTermKey gives the test vault under stateDir a new long-term key
-// and returns it.
-func writeLongTermKey(
-	t *testing.T, fs afero.Fs, stateDir string,
-) *age.X25519Identity {
-	t.Helper()
-
-	vault := &MockVersionVault{Name: testVaultName, fs: fs, stateDir: stateDir}
-
-	vaultDir, err := vault.GetDirectory()
-	require.NoError(t, err)
-	require.NoError(t, fs.MkdirAll(vaultDir, 0o700))
-
-	ltIdentity, err := age.GenerateX25519Identity()
-	require.NoError(t, err)
-	require.NoError(t, afero.WriteFile(fs, filepath.Join(vaultDir, "pub.age"),
-		[]byte(ltIdentity.Recipient().String()), 0o600))
-
-	return ltIdentity
-}
-
-// newVaultWithSecret creates the vault name under stateDir from the test
-// mnemonic, with a secret "shared" in it that holds value.
-func newVaultWithSecret(
-	t *testing.T, fs afero.Fs, stateDir, name, value string,
-) *vault.Vault {
-	t.Helper()
-
-	vlt, err := vault.CreateVault(fs, stateDir, name)
-	require.NoError(t, err)
-
-	buffer := memguard.NewBufferFromBytes([]byte(value))
-	defer buffer.Destroy()
-
-	require.NoError(t, vlt.AddSecret("shared", buffer, false))
-
-	return vlt
-}
-
-func TestWriteFileAtomicReplacesFile(t *testing.T) {
-	t.Parallel()
-
-	for _, tfs := range testFilesystems {
-		t.Run(tfs.name, func(t *testing.T) {
-			t.Parallel()
-
-			fs, dir := tfs.open(t)
-			path := filepath.Join(dir, currentFile)
-
-			require.NoError(t, secret.WriteFileAtomic(fs, path, []byte("old")))
-			require.NoError(t, secret.WriteFileAtomic(fs, path, []byte("new")))
-
-			data, err := afero.ReadFile(fs, path)
-			require.NoError(t, err)
-			assert.Equal(t, "new", string(data))
-
-			info, err := fs.Stat(path)
-			require.NoError(t, err)
-			assert.Equal(t, secret.FilePerms, info.Mode().Perm())
-
-			// No temporary file is left next to it
-			assert.Equal(t, []string{currentFile}, dirNames(t, fs, dir))
-		})
-	}
-}
-
-func TestWriteFileAtomicFailureKeepsOldFile(t *testing.T) {
-	t.Parallel()
-
-	for _, tfs := range testFilesystems {
-		t.Run(tfs.name, func(t *testing.T) {
-			t.Parallel()
-
-			base, dir := tfs.open(t)
-			path := filepath.Join(dir, currentFile)
-			require.NoError(t, secret.WriteFileAtomic(base, path, []byte("old")))
-
-			fs := hookFs{Fs: base, before: func(op, _ string) error {
-				if op == opRename {
-					return errInjected
-				}
-
-				return nil
-			}}
-
-			err := secret.WriteFileAtomic(fs, path, []byte("new"))
-			require.ErrorIs(t, err, errInjected)
-
-			data, err := afero.ReadFile(base, path)
-			require.NoError(t, err)
-			assert.Equal(t, "old", string(data))
-
-			// The temporary file is removed again
-			assert.Equal(t, []string{currentFile}, dirNames(t, base, dir))
-		})
-	}
-}
-
-// TestRemoveDirAtomic checks that RemoveDirAtomic deletes nothing where the
-// directory stands, which a crash could stop half-way, and that it leaves
-// nothing behind.
-func TestRemoveDirAtomic(t *testing.T) {
-	t.Parallel()
-
-	for _, tfs := range testFilesystems {
-		t.Run(tfs.name, func(t *testing.T) {
-			t.Parallel()
-
-			base, dir := tfs.open(t)
-			listed := filepath.Join(dir, "secrets.d")
-			target := filepath.Join(listed, "doomed")
-
-			require.NoError(t, base.MkdirAll(filepath.Join(target, "versions"), 0o700))
-			require.NoError(t, secret.WriteFileAtomic(base,
-				filepath.Join(target, currentFile), []byte("20231216.001")))
-
-			fs := hookFs{Fs: base, before: func(op, path string) error {
-				if op == opRemove && strings.HasPrefix(path, target) {
-					t.Errorf("deleted %s where it stands", path)
-				}
-
-				return nil
-			}}
-
-			require.NoError(t, secret.RemoveDirAtomic(fs, target))
-
-			// Gone, and no temporary directory is left in the directory
-			// that is listed or in the one above it
-			assert.Empty(t, dirNames(t, base, listed))
-			assert.Equal(t, []string{"secrets.d"}, dirNames(t, base, dir))
-		})
-	}
-}
-
-// TestLongestNames adds a secret to a vault and removes the vault, both
-// named with 255 bytes, the most a file name may have, on the real
-// filesystem: the temporary directories they use must fit that limit too.
-func TestLongestNames(t *testing.T) {
-	t.Setenv(secret.EnvMnemonic, testMnemonic)
-
-	const longestName = 255
-
-	fs := afero.NewOsFs()
-	name := strings.Repeat("a", longestName)
-
-	vlt, err := vault.CreateVault(fs, t.TempDir(), name)
-	require.NoError(t, err)
-
-	value := memguard.NewBufferFromBytes([]byte("long"))
-	defer value.Destroy()
-
-	require.NoError(t, vlt.AddSecret(name, value, false))
-
-	got, err := vlt.GetSecret(name)
-	require.NoError(t, err)
-	assert.Equal(t, "long", string(got))
-
-	vaultDir, err := vlt.GetDirectory()
-	require.NoError(t, err)
-	require.NoError(t, secret.RemoveDirAtomic(fs, vaultDir))
-	assert.NoDirExists(t, vaultDir)
-}
-
-// TestForcedCopyKeepsDestinationUntilReplaced copies a secret over one in
-// another vault, as a forced move between vaults does, and makes the last
-// step that completes the copy fail. The secret it was to replace must
-// still be there unchanged: it may go only once its replacement is whole.
-//
-//nolint:paralleltest // t.Setenv forbids t.Parallel
-func TestForcedCopyKeepsDestinationUntilReplaced(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)
-			src := newVaultWithSecret(t, base, stateDir, "source", "new")
-			dest := newVaultWithSecret(t, base, stateDir, "dest", "old")
-
-			// The copy is complete once its current file is written
-			fs := hookFs{Fs: base, before: func(op, path string) error {
-				if op == opRename && filepath.Base(path) == currentFile {
-					return errInjected
-				}
-
-				return nil
-			}}
-
-			err := vault.NewVault(fs, stateDir, "dest").
-				CopySecretAllVersions(src, "shared", "shared", true)
-			require.ErrorIs(t, err, errInjected)
-
-			value, err := dest.GetSecret("shared")
-			require.NoError(t, err)
-			assert.Equal(t, "old", string(value))
-		})
-	}
-}
-
-// TestTempDirsStayOutOfListings adds a version, adds a secret, copies a
-// secret over another and removes one, and checks that none of them makes a
-// directory directly in secrets.d or in a versions directory. Those are
-// listed to find secrets and versions, so a temporary directory made there
-// would be listed while half-built, and one left by a crash would stay.
-//
-//nolint:paralleltest // t.Setenv forbids t.Parallel
-func TestTempDirsStayOutOfListings(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)
-			newVaultWithSecret(t, base, stateDir, "default", "first")
-
-			fs := hookFs{Fs: base, before: func(op, path string) error {
-				parent := filepath.Base(filepath.Dir(path))
-				if op == opMkdir && (parent == "secrets.d" || parent == "versions") {
-					t.Errorf("made %s where it is listed", path)
-				}
-
-				return nil
-			}}
-			vlt := vault.NewVault(fs, stateDir, "default")
-
-			value := memguard.NewBufferFromBytes([]byte("second"))
-			defer value.Destroy()
-
-			require.NoError(t, vlt.AddSecret("shared", value, true))
-			require.NoError(t, vlt.AddSecret("other", value, false))
-			require.NoError(t, vlt.CopySecretAllVersions(vlt, "shared", "other", true))
-
-			vaultDir, err := vlt.GetDirectory()
-			require.NoError(t, err)
-			require.NoError(t, secret.RemoveDirAtomic(fs,
-				filepath.Join(vaultDir, "secrets.d", "shared")))
-		})
-	}
-}
-
-// TestVersionSaveIsWholeOrAbsent checks, before every change Save makes and
-// once after it returns, that the version directory either does not exist
-// or holds all of its files: a crash at any point leaves no version that
-// cannot be decrypted.
-func TestVersionSaveIsWholeOrAbsent(t *testing.T) {
-	t.Parallel()
-
-	for _, tfs := range testFilesystems {
-		t.Run(tfs.name, func(t *testing.T) {
-			t.Parallel()
-
-			base, stateDir := tfs.open(t)
-			ltIdentity := writeLongTermKey(t, base, stateDir)
-
-			var versionDir string
-
-			checkVersionDir := func(string, string) error {
-				exists, err := afero.DirExists(base, versionDir)
-				require.NoError(t, err)
-
-				if exists {
-					assert.ElementsMatch(t,
-						[]string{"pub.age", "value.age", "priv.age", "metadata.age"},
-						dirNames(t, base, versionDir),
-						"version directory visible before it was complete")
-				}
-
-				return nil
-			}
-
-			fs := hookFs{Fs: base, before: checkVersionDir}
-			vault := &MockVersionVault{Name: testVaultName, fs: fs, stateDir: stateDir}
-			sv := secret.NewVersion(vault, "test/secret", "20231215.001")
-			versionDir = sv.Directory
-
-			value := memguard.NewBufferFromBytes([]byte("whole or nothing"))
-			defer value.Destroy()
-
-			require.NoError(t, sv.Save(value))
-			require.NoError(t, checkVersionDir("", ""))
-
-			got, err := sv.GetValue(ltIdentity)
-			require.NoError(t, err)
-
-			defer got.Destroy()
-
-			assert.Equal(t, "whole or nothing", got.String())
-		})
-	}
-}
-
-// TestVersionSaveFailureLeavesNothing makes the write of the encrypted
-// private key fail, after the value has been written, and checks that
-// neither the version nor its temporary directory is left behind.
-func TestVersionSaveFailureLeavesNothing(t *testing.T) {
-	t.Parallel()
-
-	for _, tfs := range testFilesystems {
-		t.Run(tfs.name, func(t *testing.T) {
-			t.Parallel()
-
-			base, stateDir := tfs.open(t)
-			writeLongTermKey(t, base, stateDir)
-
-			fs := hookFs{Fs: base, before: func(op, path string) error {
-				if op == opRename && filepath.Base(path) == "priv.age" {
-					return errInjected
-				}
-
-				return nil
-			}}
-			vault := &MockVersionVault{Name: testVaultName, fs: fs, stateDir: stateDir}
-			sv := secret.NewVersion(vault, "test/secret", "20231215.001")
-
-			value := memguard.NewBufferFromBytes([]byte("never stored"))
-			defer value.Destroy()
-
-			require.ErrorIs(t, sv.Save(value), errInjected)
-
-			// The secret directory holds only the empty versions directory
-			versionsDir := filepath.Dir(sv.Directory)
-			assert.Equal(t, []string{"versions"},
-				dirNames(t, base, filepath.Dir(versionsDir)))
-			assert.Empty(t, dirNames(t, base, versionsDir))
-		})
-	}
-}
-
-// 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) {
-	t.Parallel()
-
-	for _, tfs := range testFilesystems {
-		t.Run(tfs.name, func(t *testing.T) {
-			t.Parallel()
-
-			base, dir := tfs.open(t)
-
-			var modes []os.FileMode
-
-			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
-				},
-				opened: func(_ string, perm os.FileMode) {
-					modes = append(modes, perm)
-				},
-			}
-
-			require.NoError(t, secret.WriteFileAtomic(fs,
-				filepath.Join(dir, currentFile), []byte("new")))
-			assert.Equal(t, []os.FileMode{secret.FilePerms}, modes)
-		})
-	}
-}
-
-// 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/keychainunlocker.go

@@ -189,7 +189,7 @@ func (k *KeychainUnlocker) Remove() error {
 
 	// Step 3: Remove directory
 	Debug("Removing keychain unlocker directory", "directory", k.Directory)
-	if err := RemoveDirAtomic(k.fs, k.Directory); err != nil {
+	if err := k.fs.RemoveAll(k.Directory); err != nil {
 		Debug("Failed to remove keychain unlocker directory", "error", err, "directory", k.Directory)
 
 		return fmt.Errorf("failed to remove keychain unlocker directory: %w", err)
@@ -368,7 +368,7 @@ func CreateKeychainUnlocker(fs afero.Fs, stateDir string) (*KeychainUnlocker, er
 	// Step 3: Store age recipient as plaintext
 	ageRecipient := ageIdentity.Recipient().String()
 	recipientPath := filepath.Join(unlockerDir, "pub.txt")
-	if err := WriteFileAtomic(fs, recipientPath, []byte(ageRecipient)); err != nil {
+	if err := afero.WriteFile(fs, recipientPath, []byte(ageRecipient), FilePerms); err != nil {
 		return nil, fmt.Errorf("failed to write age recipient: %w", err)
 	}
 
@@ -384,7 +384,7 @@ func CreateKeychainUnlocker(fs afero.Fs, stateDir string) (*KeychainUnlocker, er
 	}
 
 	agePrivKeyPath := filepath.Join(unlockerDir, "priv.age")
-	if err := WriteFileAtomic(fs, agePrivKeyPath, encryptedAgePrivKey); err != nil {
+	if err := afero.WriteFile(fs, agePrivKeyPath, encryptedAgePrivKey, FilePerms); err != nil {
 		return nil, fmt.Errorf("failed to write encrypted age private key: %w", err)
 	}
 
@@ -403,7 +403,7 @@ func CreateKeychainUnlocker(fs afero.Fs, stateDir string) (*KeychainUnlocker, er
 
 	// Write encrypted long-term private key
 	ltPrivKeyPath := filepath.Join(unlockerDir, "longterm.age")
-	if err := WriteFileAtomic(fs, ltPrivKeyPath, encryptedLtPrivKeyToAge); err != nil {
+	if err := afero.WriteFile(fs, ltPrivKeyPath, encryptedLtPrivKeyToAge, FilePerms); err != nil {
 		return nil, fmt.Errorf("failed to write encrypted long-term private key: %w", err)
 	}
 
@@ -440,9 +440,9 @@ func CreateKeychainUnlocker(fs afero.Fs, stateDir string) (*KeychainUnlocker, er
 		return nil, fmt.Errorf("failed to marshal unlocker metadata: %w", err)
 	}
 
-	if err := WriteFileAtomic(fs,
+	if err := afero.WriteFile(fs,
 		filepath.Join(unlockerDir, "unlocker-metadata.json"),
-		metadataBytes); err != nil {
+		metadataBytes, FilePerms); err != nil {
 		return nil, fmt.Errorf("failed to write unlocker metadata: %w", err)
 	}
 

internal/secret/passphraseunlocker.go

@@ -127,7 +127,7 @@ func (p *PassphraseUnlocker) Remove() error {
 
 	// For passphrase unlockers, we just need to remove the directory
 	// No external resources (like keychain items) to clean up
-	err := RemoveDirAtomic(p.fs, p.Directory)
+	err := p.fs.RemoveAll(p.Directory)
 	if err != nil {
 		return fmt.Errorf("failed to remove passphrase unlocker directory: %w", err)
 	}

internal/secret/pgpunlocker.go

@@ -172,7 +172,7 @@ func (p *PGPUnlocker) GetID() string {
 func (p *PGPUnlocker) Remove() error {
 	// For PGP unlockers, we just need to remove the directory
 	// No external resources (like keychain items) to clean up
-	err := RemoveDirAtomic(p.fs, p.Directory)
+	err := p.fs.RemoveAll(p.Directory)
 	if err != nil {
 		return fmt.Errorf("failed to remove PGP unlocker directory: %w", err)
 	}
@@ -275,7 +275,7 @@ func CreatePGPUnlocker(
 	ageRecipient := ageIdentity.Recipient().String()
 	recipientPath := filepath.Join(unlockerDir, "pub.txt")
 
-	err = WriteFileAtomic(fs, recipientPath, []byte(ageRecipient))
+	err = afero.WriteFile(fs, recipientPath, []byte(ageRecipient), FilePerms)
 	if err != nil {
 		return nil, fmt.Errorf("failed to write age recipient: %w", err)
 	}
@@ -298,7 +298,7 @@ func CreatePGPUnlocker(
 	// Write encrypted long-term private key
 	ltPrivKeyPath := filepath.Join(unlockerDir, "longterm.age")
 
-	err = WriteFileAtomic(fs, ltPrivKeyPath, encryptedLtPrivKeyToAge)
+	err = afero.WriteFile(fs, ltPrivKeyPath, encryptedLtPrivKeyToAge, FilePerms)
 	if err != nil {
 		return nil, fmt.Errorf("failed to write encrypted long-term private key: %w", err)
 	}
@@ -315,7 +315,7 @@ func CreatePGPUnlocker(
 
 	agePrivKeyPath := filepath.Join(unlockerDir, "priv.age.gpg")
 
-	err = WriteFileAtomic(fs, agePrivKeyPath, encryptedAgePrivKey)
+	err = afero.WriteFile(fs, agePrivKeyPath, encryptedAgePrivKey, FilePerms)
 	if err != nil {
 		return nil, fmt.Errorf("failed to write encrypted age private key: %w", err)
 	}
@@ -357,8 +357,9 @@ func writePGPUnlockerMetadata(
 		return nil, fmt.Errorf("failed to marshal unlocker metadata: %w", err)
 	}
 
-	err = WriteFileAtomic(fs,
-		filepath.Join(unlockerDir, "unlocker-metadata.json"), metadataBytes)
+	err = afero.WriteFile(fs,
+		filepath.Join(unlockerDir, "unlocker-metadata.json"),
+		metadataBytes, FilePerms)
 	if err != nil {
 		return nil, fmt.Errorf("failed to write unlocker metadata: %w", err)
 	}

internal/secret/secret_test.go

@@ -310,64 +310,6 @@ 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/seunlocker_darwin.go

@@ -148,7 +148,7 @@ func (s *SecureEnclaveUnlocker) Remove() error {
 	}
 
 	Debug("Removing SE unlocker directory", "directory", s.Directory)
-	if err := RemoveDirAtomic(s.fs, s.Directory); err != nil {
+	if err := s.fs.RemoveAll(s.Directory); err != nil {
 		return fmt.Errorf("failed to remove SE unlocker directory: %w", err)
 	}
 
@@ -271,7 +271,7 @@ func CreateSecureEnclaveUnlocker(
 
 	// Write SE-encrypted long-term key
 	ltKeyPath := filepath.Join(unlockerDir, seLongtermFilename)
-	if err := WriteFileAtomic(fs, ltKeyPath, encryptedLtKey); err != nil {
+	if err := afero.WriteFile(fs, ltKeyPath, encryptedLtKey, FilePerms); err != nil {
 		return nil, fmt.Errorf(
 			"failed to write SE-encrypted long-term key: %w",
 			err,
@@ -295,7 +295,7 @@ func CreateSecureEnclaveUnlocker(
 	}
 
 	metadataPath := filepath.Join(unlockerDir, "unlocker-metadata.json")
-	if err := WriteFileAtomic(fs, metadataPath, metadataBytes); err != nil {
+	if err := afero.WriteFile(fs, metadataPath, metadataBytes, FilePerms); err != nil {
 		return nil, fmt.Errorf("failed to write metadata: %w", err)
 	}
 

internal/secret/version.go

@@ -131,10 +131,7 @@ func GenerateVersionName(fs afero.Fs, secretDir string) (string, error) {
 	return fmt.Sprintf("%s.%03d", today, newSerial), nil
 }
 
-// Save saves the version metadata and value. The files are written into a
-// temporary directory that is renamed to sv.Directory once all of them are
-// complete, so the version directory is either whole or absent, even if the
-// process dies part-way.
+// Save saves the version metadata and value
 func (sv *Version) Save(value *memguard.LockedBuffer) error {
 	if value == nil {
 		return errNilValueBuffer
@@ -148,22 +145,14 @@ func (sv *Version) Save(value *memguard.LockedBuffer) error {
 
 	fs := sv.vault.GetFilesystem()
 
-	// Create the versions directory the finished version is renamed into
-	err := fs.MkdirAll(filepath.Dir(sv.Directory), DirPerms)
+	// Create version directory
+	err := fs.MkdirAll(sv.Directory, DirPerms)
 	if err != nil {
-		Debug("Failed to create versions directory", "error", err, "dir", sv.Directory)
+		Debug("Failed to create version directory", "error", err, "dir", sv.Directory)
 
-		return fmt.Errorf("failed to create versions directory: %w", err)
+		return fmt.Errorf("failed to create version directory: %w", err)
 	}
 
-	tmpDir, err := TempDirFor(fs, sv.Directory)
-	if err != nil {
-		return err
-	}
-
-	// Once the rename below has moved it into place, this finds nothing.
-	defer func() { _ = fs.RemoveAll(tmpDir) }()
-
 	// Generate a new keypair for this version
 	Debug("Generating version-specific keypair", "version", sv.Version)
 
@@ -184,28 +173,21 @@ func (sv *Version) Save(value *memguard.LockedBuffer) error {
 		slog.String("public_key", versionIdentity.Recipient().String()),
 	)
 
-	err = sv.writePublicKeyAndValue(fs, tmpDir, versionIdentity, value)
+	err = sv.writePublicKeyAndValue(fs, versionIdentity, value)
 	if err != nil {
 		return err
 	}
 
-	err = sv.writeEncryptedPrivateKey(fs, tmpDir, versionPrivateKeyBuffer)
+	err = sv.writeEncryptedPrivateKey(fs, versionPrivateKeyBuffer)
 	if err != nil {
 		return err
 	}
 
-	err = sv.writeEncryptedMetadata(fs, tmpDir, versionIdentity)
+	err = sv.writeEncryptedMetadata(fs, versionIdentity)
 	if err != nil {
 		return err
 	}
 
-	err = fs.Rename(tmpDir, sv.Directory)
-	if err != nil {
-		Debug("Failed to move version into place", "error", err, "dir", sv.Directory)
-
-		return fmt.Errorf("failed to move version into place: %w", err)
-	}
-
 	Debug("Successfully saved secret version",
 		"version", sv.Version, "secret_name", sv.SecretName)
 
@@ -376,18 +358,17 @@ func (sv *Version) GetValue(
 }
 
 // writePublicKeyAndValue stores the version's public key and the value
-// encrypted to it in dir.
+// encrypted to it.
 func (sv *Version) writePublicKeyAndValue(
 	fs afero.Fs,
-	dir string,
 	versionIdentity *age.X25519Identity,
 	value *memguard.LockedBuffer,
 ) error {
 	versionPublicKey := versionIdentity.Recipient().String()
-	pubKeyPath := filepath.Join(dir, "pub.age")
+	pubKeyPath := filepath.Join(sv.Directory, "pub.age")
 	Debug("Writing version public key", "path", pubKeyPath)
 
-	err := WriteFileAtomic(fs, pubKeyPath, []byte(versionPublicKey))
+	err := afero.WriteFile(fs, pubKeyPath, []byte(versionPublicKey), FilePerms)
 	if err != nil {
 		Debug("Failed to write version public key", "error", err, "path", pubKeyPath)
 
@@ -404,10 +385,10 @@ func (sv *Version) writePublicKeyAndValue(
 		return fmt.Errorf("failed to encrypt version value: %w", err)
 	}
 
-	valuePath := filepath.Join(dir, "value.age")
+	valuePath := filepath.Join(sv.Directory, "value.age")
 	Debug("Writing encrypted version value", "path", valuePath)
 
-	err = WriteFileAtomic(fs, valuePath, encryptedValue)
+	err = afero.WriteFile(fs, valuePath, encryptedValue, FilePerms)
 	if err != nil {
 		Debug("Failed to write encrypted version value", "error", err, "path", valuePath)
 
@@ -418,10 +399,9 @@ func (sv *Version) writePublicKeyAndValue(
 }
 
 // writeEncryptedPrivateKey encrypts the version's private key to the
-// vault's long-term public key and stores it in dir.
+// vault's long-term public key and stores it.
 func (sv *Version) writeEncryptedPrivateKey(
 	fs afero.Fs,
-	dir string,
 	versionPrivateKeyBuffer *memguard.LockedBuffer,
 ) error {
 	vaultDir, _ := sv.vault.GetDirectory()
@@ -455,10 +435,10 @@ func (sv *Version) writeEncryptedPrivateKey(
 		return fmt.Errorf("failed to encrypt version private key: %w", err)
 	}
 
-	privKeyPath := filepath.Join(dir, "priv.age")
+	privKeyPath := filepath.Join(sv.Directory, "priv.age")
 	Debug("Writing encrypted version private key", "path", privKeyPath)
 
-	err = WriteFileAtomic(fs, privKeyPath, encryptedPrivKey)
+	err = afero.WriteFile(fs, privKeyPath, encryptedPrivKey, FilePerms)
 	if err != nil {
 		Debug("Failed to write encrypted version private key",
 			"error", err, "path", privKeyPath)
@@ -470,10 +450,9 @@ func (sv *Version) writeEncryptedPrivateKey(
 }
 
 // writeEncryptedMetadata encrypts the version metadata to the version's
-// public key and stores it in dir.
+// public key and stores it.
 func (sv *Version) writeEncryptedMetadata(
 	fs afero.Fs,
-	dir string,
 	versionIdentity *age.X25519Identity,
 ) error {
 	Debug("Encrypting version metadata", "version", sv.Version)
@@ -497,10 +476,10 @@ func (sv *Version) writeEncryptedMetadata(
 		return fmt.Errorf("failed to encrypt version metadata: %w", err)
 	}
 
-	metadataPath := filepath.Join(dir, "metadata.age")
+	metadataPath := filepath.Join(sv.Directory, "metadata.age")
 	Debug("Writing encrypted version metadata", "path", metadataPath)
 
-	err = WriteFileAtomic(fs, metadataPath, encryptedMetadata)
+	err = afero.WriteFile(fs, metadataPath, encryptedMetadata, FilePerms)
 	if err != nil {
 		Debug("Failed to write encrypted version metadata",
 			"error", err, "path", metadataPath)
@@ -561,12 +540,15 @@ func GetCurrentVersion(fs afero.Fs, secretDir string) (string, error) {
 }
 
 // SetCurrentVersion updates the "current" file to point to a specific version
-// The file contains just the version name (e.g., "20231215.001"). It is
-// replaced in one rename, so once written it always exists.
+// The file contains just the version name (e.g., "20231215.001")
 func SetCurrentVersion(fs afero.Fs, secretDir string, version string) error {
 	currentPath := filepath.Join(secretDir, "current")
 
-	err := WriteFileAtomic(fs, currentPath, []byte(version))
+	// Remove existing file if it exists
+	_ = fs.Remove(currentPath)
+
+	// Write just the version name to the file
+	err := afero.WriteFile(fs, currentPath, []byte(version), FilePerms)
 	if err != nil {
 		return fmt.Errorf("failed to create current version file: %w", err)
 	}

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 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 = errors.New("invalid secret name")
 
 	// ErrSecretExists indicates the secret already exists and --force
@@ -62,10 +63,4 @@ var (
 	// ErrUnlockerNotFound indicates no unlocker with the given ID exists.
 	// Composed as "unlocker with ID <id> not found".
 	ErrUnlockerNotFound = errors.New("not found")
-
-	// ErrNoLockForFilesystem indicates LockStateDir was given a filesystem
-	// it cannot lock. Composed as "cannot lock the state directory on
-	// filesystem <type>".
-	ErrNoLockForFilesystem = errors.New(
-		"cannot lock the state directory on filesystem")
 )

internal/vault/lock.go

@@ -1,73 +0,0 @@
-package vault
-
-import (
-	"fmt"
-	"os"
-	"path/filepath"
-	"sync"
-	"syscall"
-
-	"git.eeqj.de/sneak/secret/internal/secret"
-	"github.com/spf13/afero"
-)
-
-// lockFileName is the file in the state directory that LockStateDir locks.
-const lockFileName = "lock"
-
-// memFsLock stands in for the lock file on the in-memory filesystem, which
-// has no file locks. Every in-memory filesystem in the process shares it.
-//
-//nolint:gochecknoglobals // must outlive the call that takes it
-var memFsLock sync.Mutex
-
-// LockStateDir takes the lock that a command changing anything under
-// stateDir holds until it returns, and returns the function that releases
-// it. While one command holds it, the next one waits here. Reads take no
-// lock: each file or directory a command changes is replaced in a single
-// rename, so a reader finds it as it was before or after, never half-made.
-//
-// On the real filesystem the lock is flock(2) on the file "lock" in
-// stateDir, which the kernel releases when the process dies, so a killed
-// command never leaves the tool locked. The in-memory filesystem the tests
-// use has no file locks, so a process-wide mutex stands in for flock there.
-// Any other filesystem is refused rather than left unlocked.
-func LockStateDir(fs afero.Fs, stateDir string) (func(), error) {
-	switch fs.(type) {
-	case *afero.OsFs:
-		return flockStateDir(stateDir)
-	case *afero.MemMapFs:
-		memFsLock.Lock()
-
-		return memFsLock.Unlock, nil
-	default:
-		return nil, fmt.Errorf("%w %T", ErrNoLockForFilesystem, fs)
-	}
-}
-
-// flockStateDir takes flock(2) on the lock file in stateDir, creating the
-// directory and the file if needed. Go opens files close-on-exec, so
-// programs the command runs, such as gpg, do not inherit the lock.
-func flockStateDir(stateDir string) (func(), error) {
-	err := os.MkdirAll(stateDir, secret.DirPerms)
-	if err != nil {
-		return nil, fmt.Errorf("failed to create state directory: %w", err)
-	}
-
-	lockPath := filepath.Join(stateDir, lockFileName)
-
-	//nolint:gosec // G304: the path is the lock file in the state directory
-	file, err := os.OpenFile(lockPath, os.O_RDWR|os.O_CREATE, secret.FilePerms)
-	if err != nil {
-		return nil, fmt.Errorf("failed to open lock file: %w", err)
-	}
-
-	err = syscall.Flock(int(file.Fd()), syscall.LOCK_EX)
-	if err != nil {
-		_ = file.Close()
-
-		return nil, fmt.Errorf("failed to lock %s: %w", lockPath, err)
-	}
-
-	// Closing the file releases the lock.
-	return func() { _ = file.Close() }, nil
-}

internal/vault/lock_test.go

@@ -1,134 +0,0 @@
-package vault_test
-
-import (
-	"testing"
-	"time"
-
-	"git.eeqj.de/sneak/secret/internal/vault"
-	"github.com/spf13/afero"
-	"github.com/stretchr/testify/assert"
-	"github.com/stretchr/testify/require"
-)
-
-const (
-	// lockWait is how long a test waits for the lock before deciding it
-	// will never come free.
-	lockWait = 10 * time.Second
-
-	// heldWait is how long a test watches a second holder fail to take a
-	// lock that is held. Broken exclusion lets it in at once.
-	heldWait = 100 * time.Millisecond
-)
-
-// lockFilesystem is a filesystem LockStateDir can lock, with a state
-// directory on it.
-type lockFilesystem struct {
-	name     string
-	fs       afero.Fs
-	stateDir string
-}
-
-// lockFilesystems returns the real filesystem, locked with flock, and the
-// in-memory one, locked with a mutex.
-func lockFilesystems(t *testing.T) []lockFilesystem {
-	t.Helper()
-
-	return []lockFilesystem{
-		{"memory", afero.NewMemMapFs(), testStateDir},
-		{"real", afero.NewOsFs(), t.TempDir()},
-	}
-}
-
-// lockInBackground starts taking the lock and returns a channel that
-// delivers the function releasing it once it has been taken.
-func lockInBackground(
-	t *testing.T, fs afero.Fs, stateDir string,
-) <-chan func() {
-	t.Helper()
-
-	taken := make(chan func(), 1)
-
-	go func() {
-		release, err := vault.LockStateDir(fs, stateDir)
-		if assert.NoError(t, err) {
-			taken <- release
-		}
-	}()
-
-	return taken
-}
-
-// TestLockStateDirExcludes checks that while the lock is held a second
-// holder, with its own open lock file on the real filesystem, waits, and
-// that it gets the lock once the first releases it.
-func TestLockStateDirExcludes(t *testing.T) {
-	t.Parallel()
-
-	for _, lfs := range lockFilesystems(t) {
-		t.Run(lfs.name, func(t *testing.T) {
-			t.Parallel()
-
-			release, err := vault.LockStateDir(lfs.fs, lfs.stateDir)
-			require.NoError(t, err)
-
-			taken := lockInBackground(t, lfs.fs, lfs.stateDir)
-
-			select {
-			case second := <-taken:
-				second()
-				release()
-				t.Fatal("a second holder took the lock while it was held")
-			case <-time.After(heldWait):
-			}
-
-			release()
-
-			select {
-			case second := <-taken:
-				second()
-			case <-time.After(lockWait):
-				t.Fatal("the second holder never got the lock")
-			}
-		})
-	}
-}
-
-// TestLockStateDirFreeAfterPanic checks that a holder that panics, and
-// releases the lock with defer as every command does, leaves it free.
-func TestLockStateDirFreeAfterPanic(t *testing.T) {
-	t.Parallel()
-
-	for _, lfs := range lockFilesystems(t) {
-		t.Run(lfs.name, func(t *testing.T) {
-			t.Parallel()
-
-			assert.Panics(t, func() {
-				release, err := vault.LockStateDir(lfs.fs, lfs.stateDir)
-				require.NoError(t, err)
-
-				defer release()
-
-				panic("the command failed")
-			})
-
-			select {
-			case release := <-lockInBackground(t, lfs.fs, lfs.stateDir):
-				release()
-			case <-time.After(lockWait):
-				t.Fatal("the lock was still held after its holder panicked")
-			}
-		})
-	}
-}
-
-// TestLockStateDirRefusesOtherFilesystems checks that a filesystem with no
-// lock implementation is refused instead of being used unlocked.
-func TestLockStateDirRefusesOtherFilesystems(t *testing.T) {
-	t.Parallel()
-
-	fs := afero.NewReadOnlyFs(afero.NewMemMapFs())
-
-	release, err := vault.LockStateDir(fs, testStateDir)
-	require.ErrorIs(t, err, vault.ErrNoLockForFilesystem)
-	assert.Nil(t, release)
-}

internal/vault/management.go

@@ -169,7 +169,7 @@ func processMnemonicForVault(
 
 	ltPubKeyPath := filepath.Join(vaultDir, "pub.age")
 
-	err = secret.WriteFileAtomic(fs, ltPubKeyPath, []byte(ltPubKey))
+	err = afero.WriteFile(fs, ltPubKeyPath, []byte(ltPubKey), secret.FilePerms)
 	if err != nil {
 		return 0, "", "", fmt.Errorf("failed to write long-term public key: %w", err)
 	}
@@ -295,13 +295,21 @@ func SelectVault(fs afero.Fs, stateDir string, name string) error {
 		return fmt.Errorf("vault %s %w", name, ErrVaultNotFound)
 	}
 
-	// Create or replace the currentvault file with just the vault name. It
-	// is replaced in one rename, so it never goes missing.
+	// Create or update the currentvault file with just the vault name
 	currentVaultPath := filepath.Join(stateDir, "currentvault")
 
+	// Remove existing file if it exists
+	_, err = fs.Stat(currentVaultPath)
+	if err == nil {
+		secret.Debug("Removing existing currentvault file", "path", currentVaultPath)
+
+		_ = fs.Remove(currentVaultPath)
+	}
+
+	// Write just the vault name to the file
 	secret.Debug("Writing currentvault file", "vault_name", name)
 
-	err = secret.WriteFileAtomic(fs, currentVaultPath, []byte(name))
+	err = afero.WriteFile(fs, currentVaultPath, []byte(name), secret.FilePerms)
 	if err != nil {
 		return fmt.Errorf("failed to select vault: %w", err)
 	}

internal/vault/metadata.go

@@ -113,7 +113,7 @@ func SaveVaultMetadata(fs afero.Fs, vaultDir string, metadata *Metadata) error {
 		return fmt.Errorf("failed to marshal vault metadata: %w", err)
 	}
 
-	err = secret.WriteFileAtomic(fs, metadataPath, metadataBytes)
+	err = afero.WriteFile(fs, metadataPath, metadataBytes, secret.FilePerms)
 	if err != nil {
 		return fmt.Errorf("failed to write vault metadata: %w", err)
 	}

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 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 {
@@ -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)
@@ -156,59 +171,17 @@ func (v *Vault) AddSecret(name string, value *memguard.LockedBuffer, force bool)
 		slog.String("secret_dir", secretDir),
 	)
 
-	// Check for an existing secret and the version the new one supersedes
-	exists, previousVersion, err := v.checkExistingSecret(name, secretDir, force)
+	// Check for an existing secret and prepare its directory
+	exists, previousVersion, err := v.prepareSecretDir(name, secretDir, force)
 	if err != nil {
 		return err
 	}
 
-	if exists {
-		return v.addVersion(name, secretDir, value, previousVersion)
-	}
-
-	return v.addNewSecret(name, secretDir, value)
-}
-
-// addNewSecret creates a secret by assembling its first version and current
-// pointer in a temporary directory, then renaming that directory to
-// secretDir, so an interrupted add leaves no half-made secret behind.
-func (v *Vault) addNewSecret(
-	name, secretDir string, value *memguard.LockedBuffer,
-) error {
-	buildDir, err := secret.TempDirFor(v.fs, secretDir)
-	if err != nil {
-		return err
-	}
-
-	// Once the rename below has moved it into place, this finds nothing.
-	defer func() { _ = v.fs.RemoveAll(buildDir) }()
-
-	err = v.addVersion(name, buildDir, value, nil)
-	if err != nil {
-		return err
-	}
-
-	err = v.fs.Rename(buildDir, secretDir)
-	if err != nil {
-		return fmt.Errorf("failed to move new secret into place: %w", err)
-	}
-
-	return nil
-}
-
-// addVersion saves value as a new version under secretDir, sets the
-// notAfter timestamp of the version it supersedes, if any, and then points
-// current at the new version. Until that last step, current still names the
-// previous version, which stays readable.
-func (v *Vault) addVersion(
-	name, secretDir string, value *memguard.LockedBuffer,
-	previousVersion *secret.Version,
-) error {
 	now := time.Now()
 
 	// Create the new version and save the encrypted value
 	versionName, err := v.createAndSaveVersion(
-		name, secretDir, value, previousVersion, &now)
+		name, secretDir, value, previousVersion, &now, exists)
 	if err != nil {
 		return err
 	}
@@ -278,7 +251,7 @@ func updateVersionMetadata(
 	// Write encrypted metadata
 	metadataPath := filepath.Join(version.Directory, "metadata.age")
 
-	err = secret.WriteFileAtomic(fs, metadataPath, encryptedMetadata)
+	err = afero.WriteFile(fs, metadataPath, encryptedMetadata, secret.FilePerms)
 	if err != nil {
 		return fmt.Errorf("failed to write encrypted version metadata: %w", err)
 	}
@@ -400,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
@@ -436,14 +410,12 @@ func (v *Vault) GetSecretObject(name string) (*secret.Secret, error) {
 	return secretObj, nil
 }
 
-// CopySecretVersion copies a single version from source into destSecretDir
-// in this vault. It decrypts the value using srcIdentity and re-encrypts
-// for this vault.
+// CopySecretVersion copies a single version from source to this vault
+// It decrypts the value using srcIdentity and re-encrypts for this vault
 func (v *Vault) CopySecretVersion(
 	srcVersion *secret.Version,
 	srcIdentity *age.X25519Identity,
 	destSecretName string,
-	destSecretDir string,
 	destVersionName string,
 ) error {
 	secret.DebugWith("Copying secret version to vault",
@@ -469,7 +441,6 @@ func (v *Vault) CopySecretVersion(
 
 	// Create destination version with same name
 	destVersion := secret.NewVersion(v, destSecretName, destVersionName)
-	destVersion.Directory = filepath.Join(destSecretDir, "versions", destVersionName)
 
 	// Copy metadata (preserve original timestamps)
 	destVersion.Metadata = srcVersion.Metadata
@@ -510,11 +481,11 @@ func (v *Vault) CopySecretAllVersions(
 		return fmt.Errorf("failed to get destination vault directory: %w", err)
 	}
 
-	// Refuse to replace an existing destination secret unless forced
+	// Check if destination secret already exists and clear it if forced
 	destStorageName := strings.ReplaceAll(destSecretName, "/", "%")
 	destSecretDir := filepath.Join(destVaultDir, "secrets.d", destStorageName)
 
-	err = v.checkCopyDestination(destSecretDir, destSecretName, force)
+	err = v.prepareCopyDestination(destSecretDir, destSecretName, force)
 	if err != nil {
 		return err
 	}
@@ -550,8 +521,14 @@ func (v *Vault) CopySecretAllVersions(
 		return fmt.Errorf("failed to get current version: %w", err)
 	}
 
-	// Copy each version and the current pointer, then move the copy into place
-	err = v.copyVersions(srcVault, srcIdentity,
+	// Create destination secret directory
+	err = v.fs.MkdirAll(destSecretDir, secret.DirPerms)
+	if err != nil {
+		return fmt.Errorf("failed to create destination secret directory: %w", err)
+	}
+
+	// Copy each version and set the current pointer, rolling back on error
+	err = v.copyVersionsWithRollback(srcVault, srcIdentity,
 		srcSecretName, destSecretName, destSecretDir, versions, currentVersion)
 	if err != nil {
 		return err
@@ -566,10 +543,10 @@ func (v *Vault) CopySecretAllVersions(
 	return nil
 }
 
-// checkExistingSecret reports whether the secret already exists, refuses to
-// overwrite it unless force is set, and returns its current version, which
-// the new version supersedes, if any.
-func (v *Vault) checkExistingSecret(
+// prepareSecretDir checks for an existing secret directory and prepares it
+// for a new version. It returns whether the secret already existed and the
+// current version to be superseded, if any.
+func (v *Vault) prepareSecretDir(
 	name, secretDir string, force bool,
 ) (bool, *secret.Version, error) {
 	// Check if secret already exists
@@ -586,6 +563,19 @@ func (v *Vault) checkExistingSecret(
 	secret.Debug("Secret existence check complete", "exists", exists)
 
 	if !exists {
+		// Create secret directory for new secret
+		secret.Debug("Creating secret directory", "secret_dir", secretDir)
+
+		err = v.fs.MkdirAll(secretDir, secret.DirPerms)
+		if err != nil {
+			secret.Debug("Failed to create secret directory",
+				"error", err, "secret_dir", secretDir)
+
+			return false, nil, fmt.Errorf("failed to create secret directory: %w", err)
+		}
+
+		secret.Debug("Created secret directory successfully")
+
 		return false, nil, nil
 	}
 
@@ -654,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
@@ -727,11 +715,11 @@ func (v *Vault) resolveSecretVersion(name, version string) (string, error) {
 }
 
 // createAndSaveVersion generates a new version name, sets the version
-// timestamps, and saves the encrypted value under secretDir, which is a
-// temporary directory while a new secret is being assembled.
+// timestamps, and saves the encrypted value. When saving fails for a newly
+// created secret, the secret directory is removed again.
 func (v *Vault) createAndSaveVersion(
 	name, secretDir string, value *memguard.LockedBuffer,
-	previousVersion *secret.Version, now *time.Time,
+	previousVersion *secret.Version, now *time.Time, exists bool,
 ) (string, error) {
 	// Generate new version name
 	versionName, err := secret.GenerateVersionName(v.fs, secretDir)
@@ -745,7 +733,6 @@ func (v *Vault) createAndSaveVersion(
 
 	// Create new version
 	newVersion := secret.NewVersion(v, name, versionName)
-	newVersion.Directory = filepath.Join(secretDir, "versions", versionName)
 
 	// Set version timestamps
 	if previousVersion == nil {
@@ -765,73 +752,57 @@ func (v *Vault) createAndSaveVersion(
 	if err != nil {
 		secret.Debug("Failed to save new version", "error", err, "version", versionName)
 
+		// Clean up the secret directory if this was a new secret
+		if !exists {
+			secret.Debug("Cleaning up secret directory due to save failure",
+				"secret_dir", secretDir)
+
+			_ = v.fs.RemoveAll(secretDir)
+		}
+
 		return "", fmt.Errorf("failed to save version: %w", err)
 	}
 
 	return versionName, nil
 }
 
-// copyVersions copies each version of the source secret and its current
-// pointer into a temporary directory, then moves that directory to
-// destSecretDir, replacing a secret already there. Nothing in this vault
-// changes until the copy is complete, so an interrupted copy leaves only a
-// temporary directory behind.
-func (v *Vault) copyVersions(
+// copyVersionsWithRollback copies each version of the source secret into the
+// destination directory and sets the current version pointer, removing the
+// partial copy when any step fails.
+func (v *Vault) copyVersionsWithRollback(
 	srcVault *Vault, srcIdentity *age.X25519Identity,
 	srcSecretName, destSecretName, destSecretDir string,
 	versions []string, currentVersion string,
 ) error {
-	buildDir, err := secret.TempDirFor(v.fs, destSecretDir)
-	if err != nil {
-		return err
-	}
-
-	// Once the rename below has moved it into place, this finds nothing.
-	defer func() { _ = v.fs.RemoveAll(buildDir) }()
-
+	// Copy each version
 	for _, versionName := range versions {
 		srcVersion := secret.NewVersion(srcVault, srcSecretName, versionName)
 
-		err = v.CopySecretVersion(
-			srcVersion, srcIdentity, destSecretName, buildDir, versionName)
+		err := v.CopySecretVersion(srcVersion, srcIdentity, destSecretName, versionName)
 		if err != nil {
+			// Rollback: remove partial copy
+			secret.Debug("Rolling back partial copy due to error", "error", err)
+
+			_ = v.fs.RemoveAll(destSecretDir)
+
 			return fmt.Errorf("failed to copy version %s: %w", versionName, err)
 		}
 	}
 
-	err = secret.SetCurrentVersion(v.fs, buildDir, currentVersion)
+	// Set current version
+	err := secret.SetCurrentVersion(v.fs, destSecretDir, currentVersion)
 	if err != nil {
+		_ = v.fs.RemoveAll(destSecretDir)
+
 		return fmt.Errorf("failed to set current version: %w", err)
 	}
 
-	// With --force, the secret being replaced goes only now that its
-	// replacement is complete
-	exists, err := afero.DirExists(v.fs, destSecretDir)
-	if err != nil {
-		return fmt.Errorf("failed to check destination: %w", err)
-	}
-
-	if exists {
-		secret.Debug("Removing existing destination secret", "path", destSecretDir)
-
-		err = secret.RemoveDirAtomic(v.fs, destSecretDir)
-		if err != nil {
-			return fmt.Errorf("failed to remove existing destination secret: %w", err)
-		}
-	}
-
-	err = v.fs.Rename(buildDir, destSecretDir)
-	if err != nil {
-		return fmt.Errorf("failed to move copied secret into place: %w", err)
-	}
-
 	return nil
 }
 
-// checkCopyDestination refuses to copy over an existing secret unless force
-// is set. A secret being replaced is removed by copyVersions, once its
-// replacement is complete.
-func (v *Vault) checkCopyDestination(
+// prepareCopyDestination ensures the destination secret directory can be
+// created, removing an existing secret when force is set.
+func (v *Vault) prepareCopyDestination(
 	destSecretDir, destSecretName string, force bool,
 ) error {
 	exists, err := afero.DirExists(v.fs, destSecretDir)
@@ -846,5 +817,15 @@ func (v *Vault) checkCopyDestination(
 		)
 	}
 
+	if exists && force {
+		// Remove existing secret
+		secret.Debug("Removing existing destination secret", "path", destSecretDir)
+
+		err = v.fs.RemoveAll(destSecretDir)
+		if err != nil {
+			return fmt.Errorf("failed to remove existing destination secret: %w", err)
+		}
+	}
+
 	return nil
 }

internal/vault/unlockers.go

@@ -310,16 +310,30 @@ func (v *Vault) SelectUnlocker(unlockerID string) error {
 		return fmt.Errorf("unlocker with ID %s %w", unlockerID, ErrUnlockerNotFound)
 	}
 
-	// Create or replace the current-unlocker file with just the unlocker
-	// name. It is replaced in one rename, so it never goes missing.
+	// Create/update current-unlocker file with just the unlocker name
 	currentUnlockerPath := filepath.Join(vaultDir, "current-unlocker")
 
+	// Remove existing file if it exists
+	exists, err := afero.Exists(v.fs, currentUnlockerPath)
+	if err != nil {
+		return fmt.Errorf("failed to check if current-unlocker file exists: %w", err)
+	}
+
+	if exists {
+		err = v.fs.Remove(currentUnlockerPath)
+		if err != nil {
+			return fmt.Errorf("failed to remove existing current-unlocker file: %w", err)
+		}
+	}
+
 	// Get just the unlocker name (basename of the directory)
 	unlockerName := filepath.Base(targetUnlockerDir)
 
+	// Write just the unlocker name to the file
 	secret.Debug("Writing current-unlocker file", "unlocker_name", unlockerName)
 
-	err = secret.WriteFileAtomic(v.fs, currentUnlockerPath, []byte(unlockerName))
+	err = afero.WriteFile(v.fs, currentUnlockerPath, []byte(unlockerName),
+		secret.FilePerms)
 	if err != nil {
 		return fmt.Errorf("failed to create current-unlocker file: %w", err)
 	}
@@ -337,14 +351,6 @@ func (v *Vault) CreatePassphraseUnlocker(
 		return nil, fmt.Errorf("failed to get vault directory: %w", err)
 	}
 
-	// We need to get the long-term key (either from memory if unlocked, or
-	// derive it). Getting it before anything is written means failing to
-	// get it changes nothing, even when replacing the current unlocker.
-	ltIdentity, err := v.GetOrDeriveLongTermKey()
-	if err != nil {
-		return nil, fmt.Errorf("failed to get long-term key: %w", err)
-	}
-
 	// Create unlocker directory
 	unlockerDir := filepath.Join(vaultDir, "unlockers.d", unlockerTypePassphrase)
 
@@ -365,7 +371,33 @@ func (v *Vault) CreatePassphraseUnlocker(
 		return nil, err
 	}
 
+	// Create metadata
+	metadata := UnlockerMetadata{
+		Type:      unlockerTypePassphrase,
+		CreatedAt: time.Now(),
+		Flags:     []string{},
+	}
+
+	// Write metadata
+	metadataBytes, err := json.MarshalIndent(metadata, "", "  ")
+	if err != nil {
+		return nil, fmt.Errorf("failed to marshal metadata: %w", err)
+	}
+
+	metadataPath := filepath.Join(unlockerDir, "unlocker-metadata.json")
+
+	err = afero.WriteFile(v.fs, metadataPath, metadataBytes, secret.FilePerms)
+	if err != nil {
+		return nil, fmt.Errorf("failed to write unlocker metadata: %w", err)
+	}
+
 	// Encrypt long-term private key to this unlocker
+	// We need to get the long-term key (either from memory if unlocked, or derive it)
+	ltIdentity, err := v.GetOrDeriveLongTermKey()
+	if err != nil {
+		return nil, fmt.Errorf("failed to get long-term key: %w", err)
+	}
+
 	ltPrivKeyBuffer := memguard.NewBufferFromBytes([]byte(ltIdentity.String()))
 	defer ltPrivKeyBuffer.Destroy()
 
@@ -377,31 +409,11 @@ func (v *Vault) CreatePassphraseUnlocker(
 
 	ltPrivKeyPath := filepath.Join(unlockerDir, "longterm.age")
 
-	err = secret.WriteFileAtomic(v.fs, ltPrivKeyPath, encryptedLtPrivKey)
+	err = afero.WriteFile(v.fs, ltPrivKeyPath, encryptedLtPrivKey, secret.FilePerms)
 	if err != nil {
 		return nil, fmt.Errorf("failed to write encrypted long-term private key: %w", err)
 	}
 
-	// Write the metadata last: readers skip an unlocker directory without
-	// it, so an unlocker interrupted before this point is never used.
-	metadata := UnlockerMetadata{
-		Type:      unlockerTypePassphrase,
-		CreatedAt: time.Now(),
-		Flags:     []string{},
-	}
-
-	metadataBytes, err := json.MarshalIndent(metadata, "", "  ")
-	if err != nil {
-		return nil, fmt.Errorf("failed to marshal metadata: %w", err)
-	}
-
-	metadataPath := filepath.Join(unlockerDir, "unlocker-metadata.json")
-
-	err = secret.WriteFileAtomic(v.fs, metadataPath, metadataBytes)
-	if err != nil {
-		return nil, fmt.Errorf("failed to write unlocker metadata: %w", err)
-	}
-
 	// Create the unlocker instance
 	unlocker := secret.NewPassphraseUnlocker(v.fs, unlockerDir, metadata)
 
@@ -455,8 +467,9 @@ func (v *Vault) writeUnlockerKeypair(
 	// Write public key
 	pubKeyPath := filepath.Join(unlockerDir, "pub.age")
 
-	err := secret.WriteFileAtomic(v.fs, pubKeyPath,
-		[]byte(unlockerIdentity.Recipient().String()))
+	err := afero.WriteFile(v.fs, pubKeyPath,
+		[]byte(unlockerIdentity.Recipient().String()),
+		secret.FilePerms)
 	if err != nil {
 		return fmt.Errorf("failed to write unlocker public key: %w", err)
 	}
@@ -475,7 +488,7 @@ func (v *Vault) writeUnlockerKeypair(
 	// Write encrypted private key
 	privKeyPath := filepath.Join(unlockerDir, "priv.age")
 
-	err = secret.WriteFileAtomic(v.fs, privKeyPath, encryptedPrivKey)
+	err = afero.WriteFile(v.fs, privKeyPath, encryptedPrivKey, secret.FilePerms)
 	if err != nil {
 		return fmt.Errorf("failed to write encrypted unlocker private key: %w", err)
 	}

internal/vault/vault_error_test.go

@@ -90,10 +90,4 @@ func TestAddSecretCleansUpOnFailure(t *testing.T) {
 	secretDir := filepath.Join(vaultDir, "secrets.d", testSecretName)
 	exists, _ := afero.DirExists(fs, secretDir)
 	assert.False(t, exists, "Secret directory should not exist after failed AddSecret")
-
-	// Nor is the temporary directory the secret was assembled in left behind
-	entries, err := afero.ReadDir(fs, vaultDir)
-	require.NoError(t, err)
-	require.Len(t, entries, 1)
-	assert.Equal(t, "pub.age", entries[0].Name())
 }