diff --git a/README.md b/README.md index 9bcd9fb..1d5d000 100644 --- a/README.md +++ b/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 diff --git a/TODO.md b/TODO.md index d78416a..3cb039f 100644 --- a/TODO.md +++ b/TODO.md @@ -25,6 +25,13 @@ Bring the repo into policy compliance in one commit: # Completed Steps +- 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-03: `secret mv` rejects a move whose destination is the source (`mv --force x x`, `mv --force work:x work:`, or an empty destination, which defaults to the source name) before changing diff --git a/internal/cli/move_test.go b/internal/cli/move_test.go index c0f5d03..40c43c8 100644 --- a/internal/cli/move_test.go +++ b/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) +} diff --git a/internal/cli/secrets.go b/internal/cli/secrets.go index 87bf092..c8ae0ed 100644 --- a/internal/cli/secrets.go +++ b/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 {