Reject secret mv onto the same secret under another name (closes #78)

On a case-insensitive filesystem (the macOS default) "Foo" and "foo" name
one secret, so `secret mv --force Foo foo` removed the destination, which
was the source, and lost the secret with every version. Between vaults the
copy replaced the source, and removing the source then removed the copy.

Both kinds of move now compare the two secret directories with
os.SameFile before changing anything and reject the move if they are one,
with or without --force. The tests give one secret two names with
symbolic links on the real filesystem.

Model: opus-5-5

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/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 {