Stop secret mv deleting a secret moved onto itself (closes #73)
check / check (push) Successful in 1m32s

`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
This commit is contained in:
2026-10-03 23:47:57 +00:00
parent a5faec0466
commit 974b1b6dc5
3 changed files with 131 additions and 21 deletions
+42 -21
View File
@@ -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
}