Refuse to create a vault that already exists (closes #74)

vault.CreateVault now checks for the vault before writing anything and
fails with "vault NAME already exists" (vault.ErrVaultExists). secret
init and secret vault create call it while holding the state directory
lock, so two creates at once cannot both pass the check. Before, either
command over an existing vault replaced its metadata, passphrase
unlocker and longterm.age, so none of its secrets could be decrypted.

Both commands now ask for the unlocker passphrase before creating the
vault, so one stopped at that prompt leaves no vault without an
unlocker behind, which they would then refuse to create again.

The lock tests set up the vault "work" instead of "default", which init
now refuses to create again.

Model: opus-5-5

README.md

@@ -139,6 +139,9 @@ matching.
 
 Moves or renames a secret within the current vault.
 - Fails if the destination already exists
+- Fails if the destination is the source under another name, such as `foo`
+  for `Foo` on a case-insensitive filesystem (the macOS default); there, to
+  change only the case of a name, move the secret to a third name first
 - Preserves all versions and metadata
 
 ### Version Management

TODO.md

@@ -34,6 +34,20 @@ Bring the repo into policy compliance in one commit:
   `longterm.age`, so none of its secrets could be decrypted any more. Both
   commands now ask for the unlocker passphrase before creating the vault,
   so one stopped at that prompt leaves no vault behind.
+- 2026-10-04: The `internal/cli` tests are back to about their time
+  before the state directory lock
+  (https://git.eeqj.de/sneak/secret/issues/80). The test that each
+  changing command waits for the lock releases it as soon as it sees the
+  command waiting there, instead of after a fixed 100 ms. The two vaults
+  with passphrase unlockers that the path and move tests start from are
+  made once and copied for each test.
+- 2026-10-04: `secret mv` rejects a move whose destination is the source
+  under another name, such as `foo` for `Foo` on a case-insensitive
+  filesystem (the macOS default) or a name reached through a symbolic
+  link, before changing anything, with or without `--force`, within a
+  vault and between vaults; before, `--force` removed the destination and
+  so deleted the secret. A rename that changes only letter case works on a
+  case-sensitive filesystem as before.
 - 2026-10-04: Lint runs only in docker: `script/lint` builds
   `Dockerfile.lint`, where golangci-lint is a build step rebuilt on
   every run (`--no-cache-filter`), so an unchanged tree is linted too;

internal/cli/lock_test.go

@@ -2,9 +2,11 @@
 package cli
 
 import (
+	"bytes"
 	"io"
 	"os"
 	"path/filepath"
+	"runtime"
 	"strconv"
 	"strings"
 	"sync"
@@ -25,11 +27,6 @@ const (
 	// 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"
 
@@ -324,10 +321,28 @@ func setupEveryCommand(
 	return versions[1], unlockerID
 }
 
+// waitingForLock reports whether a goroutine is stopped in
+// vault.LockStateDir, waiting for the in-memory filesystem's lock. The
+// stack trace of such a goroutine starts with the reason it waits,
+// "[sync.Mutex.Lock]", and names LockStateDir.
+func waitingForLock() bool {
+	stacks := make([]byte, 1<<20)
+	stacks = stacks[:runtime.Stack(stacks, true)]
+
+	for goroutine := range bytes.SplitSeq(stacks, []byte("\n\n")) {
+		if bytes.Contains(goroutine, []byte("[sync.Mutex.Lock")) &&
+			bytes.Contains(goroutine, []byte("vault.LockStateDir(")) {
+			return true
+		}
+	}
+
+	return false
+}
+
 // 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.
+// nor change anything before it waits for the lock, and must succeed once
+// the lock is released.
 func requireWaitsForLock(
 	t *testing.T,
 	withUnlocker bool,
@@ -356,14 +371,20 @@ func requireWaitsForLock(
 
 	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):
+	timeout := time.After(lockWait)
+
+	for !waitingForLock() {
+		select {
+		case err := <-done:
+			t.Fatalf("finished while the lock was held, with error %v", err)
+		case <-timeout:
+			t.Fatal("never waited for the lock")
+		case <-time.After(time.Millisecond):
+		}
 	}
 
 	assert.Equal(t, before, stateDirModTimes(t, fs),
-		"changed the state directory while the lock was held")
+		"changed the state directory before waiting for the lock")
 
 	release()
 

internal/cli/move_test.go

@@ -1,9 +1,15 @@
 package cli_test
 
 import (
+	"os"
+	"path/filepath"
 	"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"
 )
@@ -97,3 +103,127 @@ func TestMoveWithinOtherVaultKeepsCurrentVault(t *testing.T) {
 	require.Contains(t, after, workSecrets+"y/")
 	require.NotContains(t, after, workSecrets+"x/")
 }
+
+// TestMoveOntoSameSecretUnderAnotherNameIsRejected is a regression test for
+// https://git.eeqj.de/sneak/secret/issues/78: on a case-insensitive
+// filesystem "Foo" and "foo" are one secret, and `secret mv --force Foo foo`
+// removed the destination, which was the source. Symbolic links on the real
+// filesystem give one secret two names here: in "default", "y" is a link to
+// the secret "x", and the secrets.d of "other" is a link to that of
+// "default", so other:x is default:x. Each move must be rejected and leave
+// the secret and the links as they were.
+//
+//nolint:paralleltest // t.Setenv
+func TestMoveOntoSameSecretUnderAnotherNameIsRejected(t *testing.T) {
+	t.Setenv(secret.EnvMnemonic, testMnemonic)
+
+	const isSame = "is the same secret on this filesystem"
+
+	tests := []struct {
+		command      string
+		source, dest string
+		force        bool
+		wantErr      string
+	}{
+		{
+			"mv --force y x", "y", "x", true,
+			"secret 'y' cannot be moved onto itself: 'x' " + isSame,
+		},
+		{
+			"mv --force x y", "x", "y", true,
+			"secret 'x' cannot be moved onto itself: 'y' " + isSame,
+		},
+		{
+			"mv x y", "x", "y", false,
+			"secret 'x' cannot be moved onto itself: 'y' " + isSame,
+		},
+		{
+			"mv --force default:x other:x", "default:x", "other:x", true,
+			"secret 'default:x' cannot be moved onto itself: 'other:x' " +
+				isSame,
+		},
+		{
+			"mv default:x other", "default:x", "other", false,
+			"secret 'default:x' cannot be moved onto itself: 'other:x' " +
+				isSame,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.command, func(t *testing.T) {
+			fs := afero.NewOsFs()
+			stateDir := t.TempDir()
+			vaultsDir := filepath.Join(stateDir, "vaults.d")
+
+			// "default" is created last, so it is the current vault.
+			_, err := vault.CreateVault(fs, stateDir, "other")
+			require.NoError(t, err)
+
+			vlt, err := vault.CreateVault(fs, stateDir, "default")
+			require.NoError(t, err)
+
+			err = vlt.AddSecret("x", memguard.NewBufferFromBytes([]byte("value")), false)
+			require.NoError(t, err)
+
+			defaultSecrets := filepath.Join(vaultsDir, "default", "secrets.d")
+			otherSecrets := filepath.Join(vaultsDir, "other", "secrets.d")
+			link := filepath.Join(defaultSecrets, "y")
+
+			require.NoError(t, os.Symlink("x", link))
+			require.NoError(t, os.Remove(otherSecrets))
+			require.NoError(t, os.Symlink(defaultSecrets, otherSecrets))
+
+			c := cli.NewCLIInstanceWithStateDir(fs, stateDir)
+			moveErr := c.MoveSecret(&cobra.Command{}, tt.source, tt.dest, tt.force)
+
+			value, err := vlt.GetSecret("x")
+			require.NoError(t, err)
+			require.Equal(t, "value", string(value))
+
+			target, err := os.Readlink(link)
+			require.NoError(t, err)
+			require.Equal(t, "x", target)
+
+			target, err = os.Readlink(otherSecrets)
+			require.NoError(t, err)
+			require.Equal(t, defaultSecrets, target)
+
+			require.EqualError(t, moveErr, tt.wantErr)
+		})
+	}
+}
+
+// TestForcedCaseOnlyMoveOnCaseSensitiveFilesystem checks that where "Foo"
+// and "foo" are two secrets, `secret mv --force Foo foo` still replaces "foo"
+// with "Foo".
+func TestForcedCaseOnlyMoveOnCaseSensitiveFilesystem(t *testing.T) {
+	t.Setenv(secret.EnvMnemonic, testMnemonic)
+
+	fs := afero.NewOsFs()
+	stateDir := t.TempDir()
+
+	vlt, err := vault.CreateVault(fs, stateDir, "default")
+	require.NoError(t, err)
+
+	err = vlt.AddSecret("Foo", memguard.NewBufferFromBytes([]byte("upper")), false)
+	require.NoError(t, err)
+
+	_, err = os.Stat(filepath.Join(stateDir, "vaults.d", "default", "secrets.d", "foo"))
+	if err == nil {
+		t.Skip("the temporary directory is on a case-insensitive filesystem")
+	}
+
+	err = vlt.AddSecret("foo", memguard.NewBufferFromBytes([]byte("lower")), false)
+	require.NoError(t, err)
+
+	c := cli.NewCLIInstanceWithStateDir(fs, stateDir)
+	err = c.MoveSecret(&cobra.Command{}, "Foo", "foo", true)
+	require.NoError(t, err)
+
+	value, err := vlt.GetSecret("foo")
+	require.NoError(t, err)
+	require.Equal(t, "upper", string(value))
+
+	_, err = vlt.GetSecret("Foo")
+	require.ErrorIs(t, err, vault.ErrSecretNotFound)
+}

internal/cli/path_traversal_test.go

@@ -6,6 +6,7 @@ import (
 	"os"
 	"slices"
 	"strings"
+	"sync"
 	"testing"
 
 	"git.eeqj.de/sneak/secret/internal/cli"
@@ -32,9 +33,20 @@ const (
 	missingFile = "/no/such/file"
 )
 
+// The state directory newTwoVaultFs copies, recorded by snapshotStateDir.
+// Creating a passphrase unlocker is slow by design, so the vaults are made
+// once, by the first test that needs them.
+//
+//nolint:gochecknoglobals // shared by the tests that use newTwoVaultFs
+var (
+	twoVaultsOnce sync.Once
+	twoVaults     map[string]string
+)
+
 // 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.
+// Every call returns a new copy of the same vaults.
 //
 //nolint:ireturn // afero.Fs is the filesystem abstraction used throughout
 func newTwoVaultFs(t *testing.T) afero.Fs {
@@ -42,21 +54,27 @@ func newTwoVaultFs(t *testing.T) afero.Fs {
 
 	t.Setenv(secret.EnvMnemonic, testMnemonic)
 
-	fs := afero.NewMemMapFs()
+	twoVaultsOnce.Do(func() {
+		fs := afero.NewMemMapFs()
 
-	for _, name := range []string{"work", "default"} {
-		vlt, err := vault.CreateVault(fs, testStateDir, name)
-		require.NoError(t, err)
+		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.AddSecret("x", memguard.NewBufferFromBytes([]byte("value")), false)
+			require.NoError(t, err)
 
-		_, err = vlt.CreatePassphraseUnlocker(
-			memguard.NewBufferFromBytes([]byte(testPassphrase)))
-		require.NoError(t, err)
-	}
+			_, err = vlt.CreatePassphraseUnlocker(
+				memguard.NewBufferFromBytes([]byte(testPassphrase)))
+			require.NoError(t, err)
+		}
 
-	return fs
+		twoVaults = snapshotStateDir(t, fs)
+	})
+
+	require.NotNil(t, twoVaults, "making the vaults failed in an earlier test")
+
+	return newFsFromSnapshot(t, twoVaults)
 }
 
 // snapshotStateDir maps every file under the state directory to its

internal/cli/secrets.go

@@ -6,6 +6,7 @@ import (
 	"fmt"
 	"io"
 	"log"
+	"os"
 	"path/filepath"
 	"slices"
 	"strings"
@@ -890,6 +891,18 @@ func (cli *Instance) moveSecretWithinVault(
 	destEncoded := strings.ReplaceAll(dest, "/", "%")
 	destDir := filepath.Join(vaultDir, "secrets.d", destEncoded)
 
+	// Removing a destination that is the source under another name, such as
+	// "foo" for "Foo" on a case-insensitive filesystem, would delete it too.
+	same, err := cli.sameDirectory(sourceDir, destDir)
+	if err != nil {
+		return err
+	}
+
+	if same {
+		return fmt.Errorf("secret '%s' %w: '%s' is the same secret on "+
+			"this filesystem", source, errMoveOntoItself, dest)
+	}
+
 	exists, err = afero.DirExists(cli.fs, destDir)
 	if err != nil {
 		return fmt.Errorf("failed to check if destination secret exists: %w", err)
@@ -916,6 +929,31 @@ func (cli *Instance) moveSecretWithinVault(
 	return nil
 }
 
+// sameDirectory reports whether the existing directory dir and the path
+// other are one directory under two names, as secrets.d/Foo and
+// secrets.d/foo are on a case-insensitive filesystem, or a directory and a
+// symbolic link to it. Removing other to make room for dir would then delete
+// dir. It is false if other does not exist, and always false on the
+// in-memory filesystem, which has no such aliasing and whose files
+// os.SameFile does not compare.
+func (cli *Instance) sameDirectory(dir, other string) (bool, error) {
+	dirInfo, err := cli.fs.Stat(dir)
+	if err != nil {
+		return false, fmt.Errorf("failed to check %s: %w", dir, err)
+	}
+
+	otherInfo, err := cli.fs.Stat(other)
+	if errors.Is(err, os.ErrNotExist) {
+		return false, nil
+	}
+
+	if err != nil {
+		return false, fmt.Errorf("failed to check %s: %w", other, err)
+	}
+
+	return os.SameFile(dirInfo, otherInfo), nil
+}
+
 // moveSecretCrossVault handles moving between two different vaults. Its
 // caller, MoveSecret, has already checked both secret names and that both
 // vaults exist.
@@ -940,6 +978,27 @@ func (cli *Instance) moveSecretCrossVault(
 			srcSecretName, errSecretNotFound, srcVault.Name)
 	}
 
+	// The source is removed after the copy, so a destination that is the
+	// source under another name would be lost with it.
+	destVaultDir, err := destVault.GetDirectory()
+	if err != nil {
+		return fmt.Errorf("failed to get destination vault directory: %w", err)
+	}
+
+	destStorageName := strings.ReplaceAll(destSecretName, "/", "%")
+	destSecretDir := filepath.Join(destVaultDir, "secrets.d", destStorageName)
+
+	same, err := cli.sameDirectory(srcSecretDir, destSecretDir)
+	if err != nil {
+		return err
+	}
+
+	if same {
+		return fmt.Errorf("secret '%s:%s' %w: '%s:%s' is the same secret on "+
+			"this filesystem", srcVault.Name, srcSecretName, errMoveOntoItself,
+			destVault.Name, destSecretName)
+	}
+
 	// Unlock destination vault (will fail if neither mnemonic nor unlocker available)
 	_, err = destVault.GetOrDeriveLongTermKey()
 	if err != nil {