diff --git a/TODO.md b/TODO.md index 8d7f08b..d78416a 100644 --- a/TODO.md +++ b/TODO.md @@ -25,6 +25,15 @@ 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. Every vault name given with `vault:` must be one + of the existing vaults by exact name, so `work:x work/:x` is rejected + instead of being taken for a move between two vaults. A move within a + named vault no longer makes that vault the current one, whether it + succeeds or fails. - 2026-10-03: Commands that change the state directory hold one lock (`flock` on `lock` in the state directory; a mutex on the in-memory test filesystem), so concurrent commands no longer lose versions or diff --git a/internal/cli/move_test.go b/internal/cli/move_test.go new file mode 100644 index 0000000..c0f5d03 --- /dev/null +++ b/internal/cli/move_test.go @@ -0,0 +1,99 @@ +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, also when "work" was spelled two ways, 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", + }, + // Each of these spells "work" a second way. The spelling is not an + // existing vault name, so the move is not taken for a move between + // two vaults, which would delete the destination, here the source. + { + "mv --force work:x work/:x", workX, "work/:x", true, + "vault 'work/' does not exist", + }, + { + "mv --force work/:x work:", "work/:x", "work:", true, + "vault 'work/' does not exist", + }, + { + "mv --force work:x ./work:x", workX, "./work:x", true, + "vault './work' 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 a022e35..87bf092 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 @@ -781,8 +782,8 @@ func (cli *Instance) moveSecret( // with a new name if srcQualified && !destQualified { // Check if dest is actually a vault name - vaults, err := vault.ListVaults(cli.fs, cli.stateDir) - if err == nil && slices.Contains(vaults, dest) { + _, err := cli.existingVault(dest) + if err == nil { // dest is a vault name, use source secret name destVaultName = dest destSecretName = srcSecretName @@ -798,8 +799,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 @@ -810,38 +811,66 @@ 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) - if srcVaultName == destVaultName { - // Select the vault and do a simple move - err = vault.SelectVault(cli.fs, cli.stateDir, srcVaultName) + // Neither name is qualified: a rename within the current vault. + if !srcQualified { + vlt, err := vault.GetCurrentVault(cli.fs, cli.stateDir) if err != nil { - return fmt.Errorf("failed to select vault '%s': %w", srcVaultName, err) + return err } - return cli.moveSecretWithinVault(cmd, srcSecretName, destSecretName, force) + return cli.moveSecretWithinVault( + cmd, vlt, srcSecretName, destSecretName, force) } - // Cross-vault move - return cli.moveSecretCrossVault( - 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) + // Both vaults must be existing vaults by exact name, so that two + // spellings of one vault, such as "work" and "work/", are never taken for + // two vaults. A named vault does not become the current vault. + srcVault, err := cli.existingVault(srcVaultName) if err != nil { return err } - vaultDir, err := currentVlt.GetDirectory() + destVault, err := cli.existingVault(destVaultName) + if err != nil { + return err + } + + if srcVaultName == destVaultName { + return cli.moveSecretWithinVault( + cmd, srcVault, srcSecretName, destSecretName, force) + } + + return cli.moveSecretCrossVault( + cmd, srcVault, srcSecretName, destVault, destSecretName, force) +} + +// 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 nil, fmt.Errorf("failed to list vaults: %w", err) + } + + 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 } @@ -887,57 +916,34 @@ func (cli *Instance) moveSecretWithinVault( return nil } -// moveSecretCrossVault handles moving between different vaults. Its caller, -// MoveSecret, has already checked both secret names. +// moveSecretCrossVault handles moving between two different vaults. Its +// caller, MoveSecret, has already checked both secret names and that both +// vaults exist. func (cli *Instance) moveSecretCrossVault( cmd *cobra.Command, - srcVaultName, srcSecretName, - destVaultName, destSecretName string, + srcVault *vault.Vault, srcSecretName string, + destVault *vault.Vault, destSecretName string, force bool, ) error { - // Get source vault - srcVault := vault.NewVault(cli.fs, cli.stateDir, srcVaultName) - srcVaultDir, err := srcVault.GetDirectory() if err != nil { return fmt.Errorf("failed to get source vault directory: %w", err) } - // Verify source vault exists - exists, err := afero.DirExists(cli.fs, srcVaultDir) - if err != nil || !exists { - return fmt.Errorf("source vault '%s' %w", srcVaultName, errVaultDoesNotExist) - } - // Verify source secret exists srcStorageName := strings.ReplaceAll(srcSecretName, "/", "%") srcSecretDir := filepath.Join(srcVaultDir, "secrets.d", srcStorageName) - exists, err = afero.DirExists(cli.fs, srcSecretDir) + exists, err := afero.DirExists(cli.fs, srcSecretDir) if err != nil || !exists { return fmt.Errorf("secret '%s' %w in vault '%s'", - srcSecretName, errSecretNotFound, srcVaultName) - } - - // Get destination vault - destVault := vault.NewVault(cli.fs, cli.stateDir, destVaultName) - - destVaultDir, err := destVault.GetDirectory() - if err != nil { - return fmt.Errorf("failed to get destination vault directory: %w", err) - } - - // Verify destination vault exists - exists, err = afero.DirExists(cli.fs, destVaultDir) - if err != nil || !exists { - return fmt.Errorf("destination vault '%s' %w", - destVaultName, errVaultDoesNotExist) + srcSecretName, errSecretNotFound, srcVault.Name) } // Unlock destination vault (will fail if neither mnemonic nor unlocker available) _, err = destVault.GetOrDeriveLongTermKey() if err != nil { - return fmt.Errorf("failed to unlock destination vault '%s': %w", destVaultName, err) + return fmt.Errorf("failed to unlock destination vault '%s': %w", destVault.Name, err) } // Count versions for user feedback @@ -957,13 +963,13 @@ func (cli *Instance) moveSecretCrossVault( // Copy succeeded but delete failed - warn but don't fail cmd.Printf("Warning: copied secret but failed to remove source: %v\n", err) cmd.Printf("Moved secret '%s:%s' to '%s:%s' (%d version(s))\n", - srcVaultName, srcSecretName, destVaultName, destSecretName, versionCount) + srcVault.Name, srcSecretName, destVault.Name, destSecretName, versionCount) return nil } cmd.Printf("Moved secret '%s:%s' to '%s:%s' (%d version(s))\n", - srcVaultName, srcSecretName, destVaultName, destSecretName, versionCount) + srcVault.Name, srcSecretName, destVault.Name, destSecretName, versionCount) return nil }