A cross-vault secret mv that cannot remove the source fails (closes #159)

When `secret mv` copied a secret into another vault but could not remove
the source, it printed a warning and the usual "Moved" line and exited 0.
It now fails. If the source secret's directory is still in place, the
error says the copy holds all versions and the source remains. If the
source left its vault and only the temporary directory it was moved into
could not be deleted, the error says the secret is moved and names that
directory, which the error of RemoveDirAtomic now does.

The tests are in package cli and call moveSecret, because the state
directory lock refuses the wrapped filesystems that make the removal fail.

Model: opus-5-5

README.md

@@ -118,7 +118,11 @@ are allowed, and a name must not be empty, `.` or `..`.
 
 #### `secret vault select <name>`
 
-Switches to the specified vault for subsequent operations.
+Switches to the specified vault for subsequent operations. When `currentvault`
+is empty or names a vault that does not exist, every command that uses the
+current vault fails with an error that names the file and says to run
+`secret vault select <name>`. `secret vault list` still lists the vaults, and
+`secret info` still runs; both show no vault as current.
 
 #### `secret vault remove <name> [--force]` / `secret vault rm` ⚠️ 🛑
 

internal/cli/current_vault_test.go

@@ -0,0 +1,60 @@
+package cli_test
+
+import (
+	"bytes"
+	"path/filepath"
+	"strconv"
+	"testing"
+
+	"github.com/spf13/afero"
+	"github.com/spf13/cobra"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+	"sneak.berlin/go/secret/internal/cli"
+	"sneak.berlin/go/secret/internal/secret"
+	"sneak.berlin/go/secret/internal/vault"
+)
+
+// TestListFailsWhenCurrentVaultDoesNotExist is a regression test for
+// https://git.eeqj.de/sneak/secret/issues/156, where a currentvault file
+// that was empty, held an invalid vault name, or named a vault that does
+// not exist, made `secret list` report an empty vault. Listing must fail
+// with an error that names the file and says to run `secret vault select`,
+// `secret vault list` must still list the vaults, and selecting a vault
+// must make listing work again. The name "." would make vaults.d itself the
+// vault, and "../vaults.d/work" would reach the vault work by a path.
+func TestListFailsWhenCurrentVaultDoesNotExist(t *testing.T) {
+	t.Parallel()
+
+	for _, name := range []string{"", ".", "../vaults.d/work", "gone"} {
+		t.Run(strconv.Quote(name), func(t *testing.T) {
+			t.Parallel()
+
+			fs := newTwoVaultFs(t)
+			currentVaultPath := filepath.Join(testStateDir, "currentvault")
+			require.NoError(t, afero.WriteFile(fs, currentVaultPath,
+				[]byte(name), secret.FilePerms))
+
+			c := cli.NewCLIInstanceWithStateDir(fs, testStateDir)
+
+			var out bytes.Buffer
+
+			cmd := &cobra.Command{}
+			cmd.SetOut(&out)
+
+			err := c.ListSecrets(cmd, false, true, "")
+			require.ErrorIs(t, err, vault.ErrVaultNotFound)
+			require.ErrorContains(t, err, currentVaultPath)
+			require.ErrorContains(t, err, "secret vault select")
+			assert.Empty(t, out.String())
+
+			require.NoError(t, c.ListVaults(cmd, false))
+			assert.Equal(t, "Available vaults:\n  default\n  work\n", out.String())
+
+			require.NoError(t, c.SelectVault(cmd, "default"))
+			out.Reset()
+			require.NoError(t, c.ListSecrets(cmd, false, true, ""))
+			assert.Equal(t, "x\n", out.String())
+		})
+	}
+}

internal/cli/move_cross_vault_test.go

@@ -0,0 +1,152 @@
+//nolint:testpackage // white-box test of unexported internals
+package cli
+
+import (
+	"errors"
+	"path/filepath"
+	"testing"
+
+	"github.com/awnumar/memguard"
+	"github.com/spf13/afero"
+	"github.com/spf13/cobra"
+	"github.com/stretchr/testify/require"
+	"sneak.berlin/go/secret/internal/vault"
+)
+
+// errPermissionDenied is returned by renameFailFs and removeAllFailFs in
+// place of a rename or removal that succeeds.
+var errPermissionDenied = errors.New("permission denied")
+
+// renameFailFs fails every Rename of one path, as a read-only parent
+// directory does.
+type renameFailFs struct {
+	afero.Fs
+
+	path string
+}
+
+func (f *renameFailFs) Rename(oldname, newname string) error {
+	if oldname == f.path {
+		return errPermissionDenied
+	}
+
+	return f.Fs.Rename(oldname, newname)
+}
+
+// removeAllFailFs fails every RemoveAll of an entry of the directory dir,
+// as a file in it that cannot be deleted does, and records that entry's
+// path in failed.
+type removeAllFailFs struct {
+	afero.Fs
+
+	dir    string
+	failed string
+}
+
+func (f *removeAllFailFs) RemoveAll(path string) error {
+	if filepath.Dir(path) == f.dir {
+		f.failed = path
+
+		return errPermissionDenied
+	}
+
+	return f.Fs.RemoveAll(path)
+}
+
+// createCrossVaults creates in fs the vaults "work" and "default", where
+// "default" holds the secret "x" with the value "value".
+func createCrossVaults(t *testing.T, fs afero.Fs) {
+	t.Helper()
+
+	_, err := vault.CreateVault(fs, testStateDir, "work",
+		testMnemonicBuffer(t), nil)
+	require.NoError(t, err)
+
+	source, err := vault.CreateVault(fs, testStateDir, "default",
+		testMnemonicBuffer(t), nil)
+	require.NoError(t, err)
+
+	err = source.AddSecret("x",
+		memguard.NewBufferFromBytes([]byte("value")), false)
+	require.NoError(t, err)
+}
+
+// requireValue requires that the vault vaultName in fs holds secretName
+// with the value createCrossVaults stored.
+func requireValue(t *testing.T, fs afero.Fs, vaultName, secretName string) {
+	t.Helper()
+
+	vlt := vault.NewVault(fs, testStateDir, vaultName)
+	vlt.Mnemonic = testMnemonicBuffer(t)
+
+	value, err := vlt.GetSecret(secretName)
+	require.NoError(t, err, vaultName+":"+secretName)
+
+	defer value.Destroy()
+
+	require.Equal(t, []byte("value"), value.Bytes())
+}
+
+// TestCrossVaultMoveFailsWhenSourceRemains is a regression test for
+// https://git.eeqj.de/sneak/secret/issues/159, where a move between two
+// vaults whose source could not be removed reported the secret moved and
+// succeeded. It must fail, and the copy and the source must both be there.
+// The state directory lock refuses the wrapped filesystem, so the test
+// calls moveSecret, which MoveSecret runs once it holds the lock.
+func TestCrossVaultMoveFailsWhenSourceRemains(t *testing.T) {
+	t.Parallel()
+
+	base := afero.NewMemMapFs()
+	createCrossVaults(t, base)
+
+	instance := NewCLIInstanceWithStateDir(&renameFailFs{
+		Fs:   base,
+		path: testStateDir + "/vaults.d/default/secrets.d/x",
+	}, testStateDir)
+	instance.Mnemonic = testMnemonicBuffer(t)
+
+	err := instance.moveSecret(&cobra.Command{}, "default:x", "work:y", false)
+	require.ErrorIs(t, err, errPermissionDenied)
+	require.ErrorContains(t, err, "secret 'default:x' is copied with all "+
+		"1 version(s) to 'work:y', but the source remains")
+
+	requireValue(t, base, "default", "x")
+	requireValue(t, base, "work", "y")
+}
+
+// TestCrossVaultMoveFailsWhenOldFilesRemain makes the removal of the source
+// fail after the source has been moved out of its vault, when only the
+// temporary directory it was moved into cannot be deleted. The move must
+// fail, say that the secret is moved, and name that directory.
+func TestCrossVaultMoveFailsWhenOldFilesRemain(t *testing.T) {
+	t.Parallel()
+
+	base := afero.NewMemMapFs()
+	createCrossVaults(t, base)
+
+	sourceVaultDir := testStateDir + "/vaults.d/default"
+	fs := &removeAllFailFs{Fs: base, dir: sourceVaultDir}
+
+	instance := NewCLIInstanceWithStateDir(fs, testStateDir)
+	instance.Mnemonic = testMnemonicBuffer(t)
+
+	err := instance.moveSecret(&cobra.Command{}, "default:x", "work:y", false)
+	require.ErrorIs(t, err, errPermissionDenied)
+	require.ErrorContains(t, err, "secret 'default:x' is moved with all "+
+		"1 version(s) to 'work:y', but its old files could not all be deleted")
+
+	// The error names the directory that holds the old files
+	require.NotEmpty(t, fs.failed)
+	require.ErrorContains(t, err, fs.failed)
+
+	oldFiles, err := afero.DirExists(base, filepath.Join(fs.failed, "x"))
+	require.NoError(t, err)
+	require.True(t, oldFiles)
+
+	// The source vault no longer holds the secret
+	remains, err := afero.DirExists(base, sourceVaultDir+"/secrets.d/x")
+	require.NoError(t, err)
+	require.False(t, remains)
+
+	requireValue(t, base, "work", "y")
+}

internal/cli/secrets.go

@@ -1097,12 +1097,9 @@ func (cli *Instance) moveSecretCrossVault(
 	// Delete source secret
 	err = secret.RemoveDirAtomic(cli.fs, srcSecretDir)
 	if err != nil {
-		// Copy succeeded but delete failed - warn but don't fail
+		return cli.sourceRemovalError(err, srcSecretDir,
-		cmd.Printf("Warning: copied secret but failed to remove source: %v\n", err)
+			srcVault.Name+":"+srcSecretName, destVault.Name+":"+destSecretName,
-		cmd.Printf("Moved secret '%s:%s' to '%s:%s' (%d version(s))\n",
+			versionCount)
-			srcVault.Name, srcSecretName, destVault.Name, destSecretName, versionCount)
-
-		return nil
 	}
 
 	cmd.Printf("Moved secret '%s:%s' to '%s:%s' (%d version(s))\n",
@@ -1110,3 +1107,31 @@ func (cli *Instance) moveSecretCrossVault(
 
 	return nil
 }
+
+// sourceRemovalError returns the error of a move between vaults that
+// copied all versionCount versions of the secret srcName to destName, and
+// then failed with removeErr to remove srcSecretDir, the directory of
+// srcName. The removal can fail after the source has left its vault, when
+// only the temporary directory it was moved into cannot be deleted, so the
+// error says the source remains only if srcSecretDir is still there.
+func (cli *Instance) sourceRemovalError(
+	removeErr error, srcSecretDir, srcName, destName string, versionCount int,
+) error {
+	remains, err := afero.DirExists(cli.fs, srcSecretDir)
+
+	switch {
+	case err != nil:
+		return fmt.Errorf("secret '%s' is copied with all %d version(s) "+
+			"to '%s', but removing the source failed and whether it "+
+			"remains is unknown: %w",
+			srcName, versionCount, destName, errors.Join(removeErr, err))
+	case remains:
+		return fmt.Errorf("secret '%s' is copied with all %d version(s) "+
+			"to '%s', but the source remains: %w",
+			srcName, versionCount, destName, removeErr)
+	default:
+		return fmt.Errorf("secret '%s' is moved with all %d version(s) "+
+			"to '%s', but its old files could not all be deleted: %w",
+			srcName, versionCount, destName, removeErr)
+	}
+}

internal/cli/secrets_size_test.go

@@ -36,17 +36,11 @@ func newSizeTestVault(t *testing.T) (afero.Fs, *vault.Vault) {
 
 	fs := afero.NewMemMapFs()
 
-	// Create vault
+	// Create vault, which selects it as the current vault
 	_, err := vault.CreateVault(fs, testStateDir, testVaultName,
 		testMnemonicBuffer(t), nil)
 	require.NoError(t, err)
 
-	// Set current vault
-	currentVaultPath := filepath.Join(testStateDir, "currentvault")
-	vaultPath := filepath.Join(testStateDir, "vaults.d", testVaultName)
-	err = afero.WriteFile(fs, currentVaultPath, []byte(vaultPath), 0o600)
-	require.NoError(t, err)
-
 	// Get vault and set up long-term key
 	vlt, err := vault.GetCurrentVault(fs, testStateDir)
 	require.NoError(t, err)

internal/secret/atomic.go

@@ -149,7 +149,9 @@ func WriteDir(fs afero.Fs, dir string, write func(dir string) error) error {
 
 // RemoveDirAtomic deletes the directory dir so that it disappears in one
 // rename: dir is moved into a new directory from TempDirFor, which is then
-// deleted. A crash part-way leaves only that temporary directory behind.
+// deleted. A crash part-way leaves only that temporary directory behind. If
+// only deleting the temporary directory fails, dir is already gone, and the
+// error names the temporary directory, which holds what is left of it.
 func RemoveDirAtomic(fs afero.Fs, dir string) error {
 	tmp, err := TempDirFor(fs, dir)
 	if err != nil {
@@ -165,7 +167,8 @@ func RemoveDirAtomic(fs afero.Fs, dir string) error {
 
 	err = fs.RemoveAll(tmp)
 	if err != nil {
-		return fmt.Errorf("failed to remove %s: %w", dir, err)
+		return fmt.Errorf("failed to remove %s after moving %s into it: %w",
+			tmp, dir, err)
 	}
 
 	return nil

internal/vault/management.go

@@ -55,7 +55,10 @@ func ValidateVaultName(name string) error {
 }
 
 // ResolveVaultSymlink reads the currentvault file to get the path to the current vault
-// The file contains just the vault name (e.g., "default")
+// The file contains just the vault name (e.g., "default"). An empty or
+// invalid name, or the name of a vault that does not exist, is an error
+// wrapping ErrVaultNotFound that names the file and says to run
+// 'secret vault select <name>'.
 func ResolveVaultSymlink(fs afero.Fs, currentVaultPath string) (string, error) {
 	secret.Debug("resolveVaultSymlink starting", "path", currentVaultPath)
 
@@ -74,6 +77,21 @@ func ResolveVaultSymlink(fs afero.Fs, currentVaultPath string) (string, error) {
 	stateDir := filepath.Dir(currentVaultPath)
 	absolutePath := filepath.Join(stateDir, "vaults.d", vaultName)
 
+	// An empty name would make vaults.d itself the vault's directory
+	exists := false
+	if isValidVaultName(vaultName) {
+		exists, err = afero.DirExists(fs, absolutePath)
+		if err != nil {
+			return "", fmt.Errorf("failed to check if vault exists: %w", err)
+		}
+	}
+
+	if !exists {
+		return "", fmt.Errorf("vault '%s' named in %s %w; run "+
+			"'secret vault select <name>' to select a vault",
+			vaultName, currentVaultPath, ErrVaultNotFound)
+	}
+
 	secret.Debug("Resolved to absolute path", "absolute_path", absolutePath)
 
 	return absolutePath, nil