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, so a script could not tell that the secret was now in both vaults. It now prints no "Moved" line and fails with an error saying that the copy holds all versions and the source remains. The test is in package cli and calls moveSecret, because the state directory lock refuses the wrapped filesystem that makes the removal fail. Judgement call: if the source is moved aside but the temporary directory holding it cannot then be deleted, the error still says the source remains. Model: opus-5-5
2026-10-15 04:09:02 +00:00
6 changed files with 36 additions and 196 deletions
@@ -67,10 +67,6 @@ and unlocker set. This allows for complete isolation between different contexts

 ## Commands Reference

-Every time `secret` prints, in a table or in `--json` output, is in UTC,
-whatever `TZ` or the system's time zone is set to. The date in a version's name
-is the UTC date too.
-
 ### Confirmation Before Removal

 `secret rm`, `secret version rm`, `secret vault remove` and
@@ -3,9 +3,6 @@ package cli_test
 import (
 	"bytes"
 	"encoding/json"
-	"os"
-	"os/exec"
-	"path/filepath"
 	"testing"
 	"time"

@@ -128,57 +125,6 @@ func TestMovedSecretKeepsItsTimes(t *testing.T) {
 		"updated %s, then %s", beforeMove.UpdatedAt, afterMove.UpdatedAt)
 }

-// TestListedTimesAreUTC is a regression test for
-// https://git.eeqj.de/sneak/secret/issues/169, where times were printed
-// in the local time zone. It runs `secret list` on the built binary with
-// TZ set to New York, where the secret's last update, 03:04:05 UTC on 2
-// January, is 22:04:05 the day before.
-func TestListedTimesAreUTC(t *testing.T) {
-	t.Parallel()
-
-	stateDir := t.TempDir()
-
-	vlt, err := vault.CreateVault(
-		afero.NewOsFs(), stateDir, "default", testMnemonicBuffer(t), nil)
-	require.NoError(t, err)
-
-	vlt.Mnemonic = testMnemonicBuffer(t)
-	err = vlt.AddSecret("x", memguard.NewBufferFromBytes([]byte("one")), false)
-	require.NoError(t, err)
-
-	updated := time.Date(2026, time.January, 2, 3, 4, 5, 0, time.UTC)
-	current := filepath.Join(stateDir, "vaults.d", "default", "secrets.d", "x",
-		"current")
-	require.NoError(t, os.Chtimes(current, updated, updated))
-
-	tests := []struct {
-		flags []string
-		want  string
-	}{
-		{flags: nil, want: "2026-01-02 03:04"},
-		{
-			flags: []string{"--json"},
-			want:  `"updated_at": "2026-01-02T03:04:05Z"`,
-		},
-	}
-
-	for _, tt := range tests {
-		args := append([]string{"list"}, tt.flags...)
-
-		//nolint:gosec // G204: test executes the freshly built secret binary
-		cmd := exec.CommandContext(t.Context(), secretBinaryPath(t), args...)
-		cmd.Env = []string{
-			secret.EnvStateDir + "=" + stateDir,
-			"TZ=America/New_York",
-			"PATH=" + os.Getenv("PATH"),
-		}
-
-		output, err := cmd.CombinedOutput()
-		require.NoError(t, err, "%v: %s", args, output)
-		assert.Contains(t, string(output), tt.want, "%v", args)
-	}
-}
-
 // listSecrets returns the output of `secret list`, or of
 // `secret list --json` when jsonOutput is set.
 func listSecrets(t *testing.T, c *cli.Instance, jsonOutput bool) string {
@@ -3,7 +3,6 @@ package cli

 import (
 	"errors"
-	"path/filepath"
 	"testing"

 	"github.com/awnumar/memguard"
@@ -13,9 +12,9 @@ import (
 	"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")
+// errRenameFailed is returned by renameFailFs in place of a successful
+// rename.
+var errRenameFailed = errors.New("permission denied")

 // renameFailFs fails every Rename of one path, as a read-only parent
 // directory does.
@@ -27,66 +26,12 @@ type renameFailFs struct {

 func (f *renameFailFs) Rename(oldname, newname string) error {
 	if oldname == f.path {
-		return errPermissionDenied
+		return errRenameFailed
 	}

 	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
@@ -97,7 +42,18 @@ func TestCrossVaultMoveFailsWhenSourceRemains(t *testing.T) {
 	t.Parallel()

 	base := afero.NewMemMapFs()
-	createCrossVaults(t, base)
+
+	_, err := vault.CreateVault(base, testStateDir, "work",
+		testMnemonicBuffer(t), nil)
+	require.NoError(t, err)
+
+	source, err := vault.CreateVault(base, testStateDir, "default",
+		testMnemonicBuffer(t), nil)
+	require.NoError(t, err)
+
+	err = source.AddSecret("x",
+		memguard.NewBufferFromBytes([]byte("value")), false)
+	require.NoError(t, err)

 	instance := NewCLIInstanceWithStateDir(&renameFailFs{
 		Fs:   base,
@@ -105,48 +61,25 @@ func TestCrossVaultMoveFailsWhenSourceRemains(t *testing.T) {
 	}, testStateDir)
 	instance.Mnemonic = testMnemonicBuffer(t)

-	err := instance.moveSecret(&cobra.Command{}, "default:x", "work:y", false)
-	require.ErrorIs(t, err, errPermissionDenied)
+	err = instance.moveSecret(&cobra.Command{}, "default:x", "work:y", false)
+	require.ErrorIs(t, err, errRenameFailed)
 	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")
-}
+	// requireValue requires that the vault vaultName holds secretName with
+	// the value stored above.
+	requireValue := func(vaultName, secretName string) {
+		vlt := vault.NewVault(base, testStateDir, vaultName)
+		vlt.Mnemonic = testMnemonicBuffer(t)

-// 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()
+		value, err := vlt.GetSecret(secretName)
+		require.NoError(t, err, vaultName+":"+secretName)

-	base := afero.NewMemMapFs()
-	createCrossVaults(t, base)
+		defer value.Destroy()

-	sourceVaultDir := testStateDir + "/vaults.d/default"
-	fs := &removeAllFailFs{Fs: base, dir: sourceVaultDir}
+		require.Equal(t, []byte("value"), value.Bytes())
+	}

-	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")
+	requireValue("default", "x")
+	requireValue("work", "y")
 }
@@ -2,7 +2,6 @@ package cli

 import (
 	"os"
-	"time"

 	"github.com/awnumar/memguard"
 	"github.com/spf13/cobra"
@@ -15,10 +14,6 @@ import (
 // every memguard buffer before it returns, so the caller must do nothing
 // but exit with the code.
 func Entry() int {
-	// Every time secret prints or writes is in UTC, whatever TZ or the
-	// system's time zone is set to.
-	time.Local = time.UTC //nolint:gosmopolitan // this makes local time UTC
-
 	raiseLockedMemoryLimit()

 	// On SIGINT or SIGTERM memguard runs this function, wipes every buffer
@@ -1097,9 +1097,10 @@ func (cli *Instance) moveSecretCrossVault(
 	// Delete source secret
 	err = secret.RemoveDirAtomic(cli.fs, srcSecretDir)
 	if err != nil {
-		return cli.sourceRemovalError(err, srcSecretDir,
-			srcVault.Name+":"+srcSecretName, destVault.Name+":"+destSecretName,
-			versionCount)
+		return fmt.Errorf("secret '%s:%s' is copied with all %d version(s) "+
+			"to '%s:%s', but the source remains: %w",
+			srcVault.Name, srcSecretName, versionCount,
+			destVault.Name, destSecretName, err)
 	}

 	cmd.Printf("Moved secret '%s:%s' to '%s:%s' (%d version(s))\n",
@@ -1107,31 +1108,3 @@ 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)
-	}
-}
@@ -149,9 +149,7 @@ 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. 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.
+// deleted. A crash part-way leaves only that temporary directory behind.
 func RemoveDirAtomic(fs afero.Fs, dir string) error {
 	tmp, err := TempDirFor(fs, dir)
 	if err != nil {
@@ -167,8 +165,7 @@ func RemoveDirAtomic(fs afero.Fs, dir string) error {

 	err = fs.RemoveAll(tmp)
 	if err != nil {
-		return fmt.Errorf("failed to remove %s after moving %s into it: %w",
-			tmp, dir, err)
+		return fmt.Errorf("failed to remove %s: %w", dir, err)
 	}

 	return nil