From 974b1b6dc563fcea69b0fdc3e04b63fe916941d1 Mon Sep 17 00:00:00 2001 From: sneak Date: Sat, 3 Oct 2026 23:47:57 +0000 Subject: [PATCH] Stop `secret mv` deleting a secret moved onto itself (closes #73) `secret mv --force x x` deleted the secret: a move within one vault removes an existing destination before renaming the source onto it. The same happened for `work:x work:`, `work:x work` and `work:x ""`, where an empty destination defaults to the source name. moveSecretWithinVault now rejects a move whose two names are the same before touching anything. A move within a named vault works in that vault directly instead of selecting it, so the current vault never changes; the vault must be one of the existing vaults. The test runs each rejected move on a copy of two in-memory vaults and requires the exact error and an unchanged state directory. Model: opus-5-5 --- TODO.md | 6 +++ internal/cli/move_test.go | 83 +++++++++++++++++++++++++++++++++++++++ internal/cli/secrets.go | 63 +++++++++++++++++++---------- 3 files changed, 131 insertions(+), 21 deletions(-) create mode 100644 internal/cli/move_test.go diff --git a/TODO.md b/TODO.md index e4781c4..8f332c8 100644 --- a/TODO.md +++ b/TODO.md @@ -25,6 +25,12 @@ Bring the repo into policy compliance in one commit: # Completed Steps +- 2026-10-03: `secret mv` rejects a move whose destination is the + source (`mv --force x x`, `mv --force work:x work:`, or an empty + destination, which defaults to the source name) before changing + anything; before, `--force` removed the destination first and so + deleted the secret. A move within a named vault no longer makes that + vault the current one, whether it succeeds or fails. - 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 diff --git a/internal/cli/move_test.go b/internal/cli/move_test.go new file mode 100644 index 0000000..58d9e6a --- /dev/null +++ b/internal/cli/move_test.go @@ -0,0 +1,83 @@ +package cli_test + +import ( + "testing" + + "git.eeqj.de/sneak/secret/internal/cli" + "github.com/spf13/cobra" + "github.com/stretchr/testify/require" +) + +// TestRejectedMoveWithinVaultLeavesStateUnchanged is a regression test for +// https://git.eeqj.de/sneak/secret/issues/73, where a forced move of a secret +// onto itself deleted it, and a failed move within "work" left "work" the +// current vault. "default" is the current vault in every case, and each case +// runs on its own copy of the state directory. +// +//nolint:paralleltest // newTwoVaultFs uses t.Setenv +func TestRejectedMoveWithinVaultLeavesStateUnchanged(t *testing.T) { + before := snapshotStateDir(t, newTwoVaultFs(t)) + require.Equal(t, "default", before[testStateDir+"/currentvault"]) + + const ( + ontoItself = "secret 'x' cannot be moved onto itself" + workX = "work:x" + ) + + tests := []struct { + command string + source, dest string + force bool + wantErr string + }{ + {"mv x x", "x", "x", false, ontoItself}, + {"mv --force x x", "x", "x", true, ontoItself}, + {"mv --force work:x work:", workX, "work:", true, ontoItself}, + // An empty destination name defaults to the source name. + {`mv --force work:x ""`, workX, "", true, ontoItself}, + // "work" is a vault name, so the destination is work:x. + {"mv --force work:x work", workX, "work", true, ontoItself}, + { + "mv work:nosuch work:y", "work:nosuch", "work:y", false, + "secret 'nosuch' not found", + }, + // Only an existing vault is used, so ".." cannot reach the state + // directory itself. + { + "mv --force ..:x ..:y", "..:x", "..:y", true, + "vault '..' does not exist", + }, + } + + for _, tt := range tests { + t.Run(tt.command, func(t *testing.T) { + fs := newFsFromSnapshot(t, before) + c := cli.NewCLIInstanceWithStateDir(fs, testStateDir) + + err := c.MoveSecret(&cobra.Command{}, tt.source, tt.dest, tt.force) + + require.Equal(t, before, snapshotStateDir(t, fs)) + require.EqualError(t, err, tt.wantErr) + }) + } +} + +// TestMoveWithinOtherVaultKeepsCurrentVault checks that `secret mv work:x +// work:y`, with "default" the current vault, renames "x" to "y" in "work" and +// leaves "default" the current vault. +// +//nolint:paralleltest // newTwoVaultFs uses t.Setenv +func TestMoveWithinOtherVaultKeepsCurrentVault(t *testing.T) { + fs := newTwoVaultFs(t) + c := cli.NewCLIInstanceWithStateDir(fs, testStateDir) + + err := c.MoveSecret(&cobra.Command{}, "work:x", "work:y", false) + require.NoError(t, err) + + after := snapshotStateDir(t, fs) + workSecrets := testStateDir + "/vaults.d/work/secrets.d/" + + require.Equal(t, "default", after[testStateDir+"/currentvault"]) + require.Contains(t, after, workSecrets+"y/") + require.NotContains(t, after, workSecrets+"x/") +} diff --git a/internal/cli/secrets.go b/internal/cli/secrets.go index 7fbf6f0..46443cb 100644 --- a/internal/cli/secrets.go +++ b/internal/cli/secrets.go @@ -40,6 +40,7 @@ var ( errVaultDoesNotExist = errors.New("does not exist") errCrossVaultSourceUnqualified = errors.New( "source must specify vault (e.g., vault:secret) for cross-vault move") + errMoveOntoItself = errors.New("cannot be moved onto itself") ) // bufferInfo tracks a protected buffer and the number of bytes used in it @@ -740,8 +741,8 @@ func (cli *Instance) MoveSecret( 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. + // Check both names, for every form of the move, before building any path + // from them. err := vault.ValidateSecretName(srcSecretName) if err != nil { return err @@ -752,20 +753,24 @@ func (cli *Instance) MoveSecret( 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) + // A move within one vault: the current vault when neither name is + // qualified (both vault names are then empty), else the named vault, + // which does not become the current vault. if srcVaultName == destVaultName { - // Select the vault and do a simple move - err = vault.SelectVault(cli.fs, cli.stateDir, srcVaultName) - if err != nil { - return fmt.Errorf("failed to select vault '%s': %w", srcVaultName, err) + var vlt *vault.Vault + + if srcQualified { + vlt, err = cli.existingVault(srcVaultName) + } else { + vlt, err = vault.GetCurrentVault(cli.fs, cli.stateDir) } - return cli.moveSecretWithinVault(cmd, srcSecretName, destSecretName, force) + if err != nil { + return err + } + + return cli.moveSecretWithinVault( + cmd, vlt, srcSecretName, destSecretName, force) } // Cross-vault move @@ -773,17 +778,33 @@ func (cli *Instance) MoveSecret( cmd, srcVaultName, srcSecretName, destVaultName, destSecretName, force) } -// 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 { - currentVlt, err := vault.GetCurrentVault(cli.fs, cli.stateDir) +// existingVault returns the vault with the given name, or an error if there +// is none. Unlike vault.SelectVault, it leaves the current vault as it is. +func (cli *Instance) existingVault(name string) (*vault.Vault, error) { + vaults, err := vault.ListVaults(cli.fs, cli.stateDir) if err != nil { - return err + return nil, fmt.Errorf("failed to list vaults: %w", err) } - vaultDir, err := currentVlt.GetDirectory() + if !slices.Contains(vaults, name) { + return nil, fmt.Errorf("vault '%s' %w", name, errVaultDoesNotExist) + } + + return vault.NewVault(cli.fs, cli.stateDir, name), nil +} + +// moveSecretWithinVault renames a secret within the vault vlt. Its caller, +// MoveSecret, has already checked both secret names. +func (cli *Instance) moveSecretWithinVault( + cmd *cobra.Command, vlt *vault.Vault, source, dest string, force bool, +) error { + // With --force the destination is removed before the source is renamed + // onto it, which would delete the secret. + if source == dest { + return fmt.Errorf("secret '%s' %w", source, errMoveOntoItself) + } + + vaultDir, err := vlt.GetDirectory() if err != nil { return err }