Skip corrupt unlocker metadata in unlocker select and remove (closes #72)

findUnlockerByID failed on the first unlocker directory whose metadata
could not be checked, read or parsed, so `secret unlocker select` and
`remove` failed when one sorted before the unlocker asked for. It now
skips such a directory with the warning ListUnlockers gives, through
the code both now share. A skipped directory is removed by its
directory name, with RemoveDirAtomic, and cannot be selected. Removing
one whose metadata file is missing or corrupt never counts as removing
the last unlocker; removing one whose metadata file cannot be checked
for or read does, since it may be the only working unlocker.

Model: opus-5-5

internal/cli/unlockers.go

@@ -744,14 +744,43 @@ func (cli *Instance) removeUnlocker(
 		return err
 	}
 
-	// Get list of unlockers
+	// Get list of unlockers. It leaves out a directory whose metadata file
+	// is missing or cannot be checked for, read or parsed.
 	unlockers, err := vlt.ListUnlockers()
 	if err != nil {
 		return fmt.Errorf("failed to list unlockers: %w", err)
 	}
 
+	vaultDir, err := vlt.GetDirectory()
+	if err != nil {
+		return fmt.Errorf("failed to get vault directory: %w", err)
+	}
+
+	unlockersDir := filepath.Join(vaultDir, "unlockers.d")
+
 	// Check if we're removing the last unlocker
+	removingLast := false
+
 	if len(unlockers) == 1 {
+		lastID, err := findUnlockerIDByMetadata(
+			cli.fs, unlockersDir, unlockers[0], true)
+		if err != nil {
+			return err
+		}
+
+		removingLast = lastID == unlockerID
+	}
+
+	// unlockerID may instead name a directory left out of the list. If its
+	// metadata file is missing or corrupt it is not a working unlocker, so
+	// removing it never removes the last one. If the file cannot be checked
+	// for or read, the unlocker may be the only working one, so removing it
+	// counts as removing the last unlocker.
+	if metadataUnreadable(cli.fs, filepath.Join(unlockersDir, unlockerID)) {
+		removingLast = true
+	}
+
+	if removingLast {
 		// Check if vault has secrets
 		numSecrets, err := vlt.NumSecrets()
 		if err != nil {
@@ -785,6 +814,20 @@ func (cli *Instance) removeUnlocker(
 	return nil
 }
 
+// metadataUnreadable reports whether checking for or reading the metadata
+// file in the unlocker directory unlockerDir fails. A missing file is not
+// a failure.
+func metadataUnreadable(fs afero.Fs, unlockerDir string) bool {
+	metadataPath := filepath.Join(unlockerDir, "unlocker-metadata.json")
+
+	exists, err := afero.Exists(fs, metadataPath)
+	if err == nil && exists {
+		_, err = afero.ReadFile(fs, metadataPath)
+	}
+
+	return err != nil
+}
+
 // UnlockerSelect selects an unlocker as current
 func (cli *Instance) UnlockerSelect(unlockerID string) error {
 	release, err := vault.LockStateDir(cli.fs, cli.stateDir)

internal/cli/unlockers_corrupt_test.go

@@ -0,0 +1,167 @@
+// Corrupt Unlocker Tests
+//
+// `secret unlocker select` and `secret unlocker remove` find an unlocker
+// by its ID. These tests give the first unlocker, which sorts before the
+// one the commands act on, metadata that is not JSON, and check that the
+// commands step past it, and that it can itself be removed by its
+// directory name, which `secret unlocker list` names in its warning. A
+// last test checks that an unlocker whose metadata file cannot be read is
+// removed by its directory name only as the last unlocker is.
+
+//nolint:testpackage // white-box test of unexported internals
+package cli
+
+import (
+	"path/filepath"
+	"testing"
+
+	"git.eeqj.de/sneak/secret/internal/vault"
+	"github.com/spf13/afero"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+// newCorruptUnlockerVault returns the two-unlocker test vault with the
+// metadata of the first unlocker replaced by text that is not JSON.
+func newCorruptUnlockerVault(t *testing.T) *afero.MemMapFs {
+	t.Helper()
+
+	fs := newListTestVault(t, 2)
+	require.NoError(t, afero.WriteFile(fs,
+		filepath.Join(testVaultDir(listTestVaultName), listTestUnlockersDirName,
+			listTestUnlockerDirOne, listTestMetadataFileName),
+		[]byte("not json"), listTestFilePerm))
+
+	return fs
+}
+
+// TestUnlockerSelectSkipsCorruptUnlocker asserts that the second unlocker
+// can be selected, and that the corrupt one, having no type to be used as,
+// cannot be selected by its directory name.
+func TestUnlockerSelectSkipsCorruptUnlocker(t *testing.T) {
+	t.Parallel()
+
+	fs := newCorruptUnlockerVault(t)
+	instance, _ := newTestInstance(fs)
+
+	require.NoError(t, instance.UnlockerSelect("pgp-"+listTestGPGKeyID+"B"))
+
+	current, err := afero.ReadFile(fs,
+		filepath.Join(testVaultDir(listTestVaultName), "current-unlocker"))
+	require.NoError(t, err)
+	assert.Equal(t, listTestUnlockerDirTwo, string(current))
+
+	err = instance.UnlockerSelect(listTestUnlockerDirOne)
+	require.ErrorIs(t, err, vault.ErrUnlockerNotFound)
+}
+
+// TestUnlockerRemoveWithCorruptUnlocker asserts that the second unlocker
+// can be removed, unless the vault holds secrets: the corrupt unlocker
+// cannot unlock the vault, so the second is its last. The corrupt one can
+// be removed by its directory name without --force even then.
+func TestUnlockerRemoveWithCorruptUnlocker(t *testing.T) {
+	t.Parallel()
+
+	tests := []struct {
+		name        string
+		unlockerID  string
+		withSecret  bool
+		wantErr     error
+		wantEntries []string
+	}{
+		{
+			name:        "the other unlocker",
+			unlockerID:  "pgp-" + listTestGPGKeyID + "B",
+			wantEntries: []string{listTestUnlockerDirOne},
+		},
+		{
+			name:       "the other unlocker, the last one, with secrets",
+			unlockerID: "pgp-" + listTestGPGKeyID + "B",
+			withSecret: true,
+			wantErr:    errLastUnlocker,
+			wantEntries: []string{
+				listTestUnlockerDirOne, listTestUnlockerDirTwo,
+			},
+		},
+		{
+			name:        "the corrupt unlocker by its directory name",
+			unlockerID:  listTestUnlockerDirOne,
+			withSecret:  true,
+			wantEntries: []string{listTestUnlockerDirTwo},
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			t.Parallel()
+
+			fs := newCorruptUnlockerVault(t)
+			if tt.withSecret {
+				writeTestSecret(t, fs, testVaultDir(listTestVaultName))
+			}
+
+			instance, cmd := newTestInstance(fs)
+
+			err := instance.UnlockersRemove(tt.unlockerID, false, cmd)
+			require.ErrorIs(t, err, tt.wantErr)
+
+			assertDirEntries(t, fs,
+				filepath.Join(testVaultDir(listTestVaultName),
+					listTestUnlockersDirName),
+				tt.wantEntries...)
+		})
+	}
+}
+
+// TestUnlockerRemoveWithUnreadableMetadata asserts that removing the only
+// unlocker of a vault with secrets by its directory name, when its
+// metadata file cannot be checked for or read, is refused without --force:
+// listing leaves it out, but it may still be the vault's only working
+// unlocker. With --force it is removed. The state directory lock refuses
+// the failing filesystem, so the test calls removeUnlocker, which
+// UnlockersRemove runs once it holds the lock.
+func TestUnlockerRemoveWithUnreadableMetadata(t *testing.T) {
+	t.Parallel()
+
+	vaultDir := testVaultDir(listTestVaultName)
+	unlockersDir := filepath.Join(vaultDir, listTestUnlockersDirName)
+	failingPath := filepath.Join(unlockersDir, listTestUnlockerDirOne,
+		listTestMetadataFileName)
+
+	tests := []struct {
+		name string
+		wrap func(base afero.Fs) afero.Fs
+	}{
+		{
+			name: "checking for the file fails",
+			wrap: func(base afero.Fs) afero.Fs {
+				return &metadataStatFailFs{Fs: base, uncheckablePath: failingPath}
+			},
+		},
+		{
+			name: "reading the file fails",
+			wrap: func(base afero.Fs) afero.Fs {
+				return &metadataReadFailFs{Fs: base, unreadablePath: failingPath}
+			},
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			t.Parallel()
+
+			base := newListTestVault(t, 1)
+			writeTestSecret(t, base, vaultDir)
+
+			instance, cmd := newTestInstance(tt.wrap(base))
+
+			err := instance.removeUnlocker(listTestUnlockerDirOne, false, cmd)
+			require.ErrorIs(t, err, errLastUnlocker)
+			assertDirEntries(t, base, unlockersDir, listTestUnlockerDirOne)
+
+			require.NoError(t,
+				instance.removeUnlocker(listTestUnlockerDirOne, true, cmd))
+			assertDirEntries(t, base, unlockersDir)
+		})
+	}
+}

internal/vault/unlockers.go

@@ -126,7 +126,11 @@ func (v *Vault) resolveUnlockerDirectory(currentUnlockerPath string) (string, er
 }
 
 // findUnlockerByID finds an unlocker by its ID and returns the unlocker
-// instance and its directory path
+// instance and its directory path. A directory that ListUnlockers skips is
+// skipped here too, with the same warning. Such a directory has no ID: if
+// no unlocker has the ID unlockerID but such a directory is named
+// unlockerID, that directory is returned with a nil unlocker, so that
+// RemoveUnlocker can remove it.
 //
 //nolint:ireturn // returns one of several concrete unlocker implementations
 func (v *Vault) findUnlockerByID(
@@ -137,42 +141,24 @@ func (v *Vault) findUnlockerByID(
 		return nil, "", fmt.Errorf("failed to read unlockers directory: %w", err)
 	}
 
+	skippedDirPath := ""
+
 	for _, file := range files {
 		if !file.IsDir() {
 			continue
 		}
 
-		// Read metadata file
-		metadataPath := filepath.Join(unlockersDir, file.Name(), "unlocker-metadata.json")
+		unlockerDirPath := filepath.Join(unlockersDir, file.Name())
 
-		exists, err := afero.Exists(v.fs, metadataPath)
-		if err != nil {
-			return nil, "", fmt.Errorf(
-				"failed to check if metadata exists for unlocker %s: %w",
-				file.Name(), err)
-		}
+		metadata, ok := v.readUnlockerMetadataOrWarn(unlockersDir, file.Name())
+		if !ok {
+			if file.Name() == unlockerID {
+				skippedDirPath = unlockerDirPath
+			}
 
-		if !exists {
-			// Skip directories without metadata - they might not be unlockers
 			continue
 		}
 
-		metadataBytes, err := afero.ReadFile(v.fs, metadataPath)
-		if err != nil {
-			return nil, "", fmt.Errorf(
-				"failed to read metadata for unlocker %s: %w", file.Name(), err)
-		}
-
-		var metadata UnlockerMetadata
-
-		err = json.Unmarshal(metadataBytes, &metadata)
-		if err != nil {
-			return nil, "", fmt.Errorf(
-				"failed to parse metadata for unlocker %s: %w", file.Name(), err)
-		}
-
-		unlockerDirPath := filepath.Join(unlockersDir, file.Name())
-
 		// Create the appropriate unlocker instance
 		var tempUnlocker secret.Unlocker
 
@@ -195,7 +181,7 @@ func (v *Vault) findUnlockerByID(
 		}
 	}
 
-	return nil, "", nil
+	return nil, skippedDirPath, nil
 }
 
 // ListUnlockers returns a list of available unlockers for this vault
@@ -226,44 +212,12 @@ func (v *Vault) ListUnlockers() ([]UnlockerMetadata, error) {
 	var unlockers []UnlockerMetadata
 
 	for _, file := range files {
-		if file.IsDir() {
-			// Read metadata file
-			metadataPath := filepath.Join(unlockersDir, file.Name(),
-				"unlocker-metadata.json")
-
-			exists, err := afero.Exists(v.fs, metadataPath)
-			if err != nil {
-				secret.Warn("Skipping unlocker directory whose metadata file cannot be checked",
-					"directory", file.Name(), "error", err)
-
-				continue
-			}
-
-			if !exists {
-				secret.Warn("Skipping unlocker directory with missing metadata file",
-					"directory", file.Name())
-
-				continue
-			}
-
-			metadataBytes, err := afero.ReadFile(v.fs, metadataPath)
-			if err != nil {
-				secret.Warn("Skipping unlocker directory with unreadable metadata file",
-					"directory", file.Name(), "error", err)
-
-				continue
-			}
-
-			var metadata UnlockerMetadata
-
-			err = json.Unmarshal(metadataBytes, &metadata)
-			if err != nil {
-				secret.Warn("Skipping unlocker directory with corrupt metadata file",
-					"directory", file.Name(), "error", err)
-
-				continue
-			}
+		if !file.IsDir() {
+			continue
+		}
 
+		metadata, ok := v.readUnlockerMetadataOrWarn(unlockersDir, file.Name())
+		if ok {
 			unlockers = append(unlockers, metadata)
 		}
 	}
@@ -271,7 +225,54 @@ func (v *Vault) ListUnlockers() ([]UnlockerMetadata, error) {
 	return unlockers, nil
 }
 
-// RemoveUnlocker removes an unlocker from this vault
+// readUnlockerMetadataOrWarn reads the metadata of the unlocker directory
+// name in unlockersDir. If the metadata file cannot be checked for, is
+// missing, or cannot be read or parsed, it warns, naming the directory,
+// and returns false: the caller skips that directory.
+func (v *Vault) readUnlockerMetadataOrWarn(
+	unlockersDir, name string,
+) (UnlockerMetadata, bool) {
+	metadataPath := filepath.Join(unlockersDir, name, "unlocker-metadata.json")
+
+	var metadata UnlockerMetadata
+
+	exists, err := afero.Exists(v.fs, metadataPath)
+	if err != nil {
+		secret.Warn("Skipping unlocker directory whose metadata file cannot be checked",
+			"directory", name, "error", err)
+
+		return metadata, false
+	}
+
+	if !exists {
+		secret.Warn("Skipping unlocker directory with missing metadata file",
+			"directory", name)
+
+		return metadata, false
+	}
+
+	metadataBytes, err := afero.ReadFile(v.fs, metadataPath)
+	if err != nil {
+		secret.Warn("Skipping unlocker directory with unreadable metadata file",
+			"directory", name, "error", err)
+
+		return metadata, false
+	}
+
+	err = json.Unmarshal(metadataBytes, &metadata)
+	if err != nil {
+		secret.Warn("Skipping unlocker directory with corrupt metadata file",
+			"directory", name, "error", err)
+
+		return metadata, false
+	}
+
+	return metadata, true
+}
+
+// RemoveUnlocker removes an unlocker from this vault. An unlocker
+// directory that ListUnlockers skips is removed by its directory name; its
+// type is unknown, so only the directory is removed.
 func (v *Vault) RemoveUnlocker(unlockerID string) error {
 	vaultDir, err := v.GetDirectory()
 	if err != nil {
@@ -282,15 +283,19 @@ func (v *Vault) RemoveUnlocker(unlockerID string) error {
 	unlockersDir := filepath.Join(vaultDir, "unlockers.d")
 
 	// Find the unlocker by ID
-	unlocker, _, err := v.findUnlockerByID(unlockersDir, unlockerID)
+	unlocker, unlockerDir, err := v.findUnlockerByID(unlockersDir, unlockerID)
 	if err != nil {
 		return err
 	}
 
-	if unlocker == nil {
+	if unlockerDir == "" {
 		return fmt.Errorf("unlocker with ID %s %w", unlockerID, ErrUnlockerNotFound)
 	}
 
+	if unlocker == nil {
+		return secret.RemoveDirAtomic(v.fs, unlockerDir)
+	}
+
 	// Use the unlocker's Remove method
 	return unlocker.Remove()
 }
@@ -306,12 +311,14 @@ func (v *Vault) SelectUnlocker(unlockerID string) error {
 	unlockersDir := filepath.Join(vaultDir, "unlockers.d")
 
 	// Find the unlocker by ID
-	_, targetUnlockerDir, err := v.findUnlockerByID(unlockersDir, unlockerID)
+	unlocker, targetUnlockerDir, err := v.findUnlockerByID(unlockersDir, unlockerID)
 	if err != nil {
 		return err
 	}
 
-	if targetUnlockerDir == "" {
+	// A directory found without an unlocker is one ListUnlockers skips; it
+	// cannot be selected.
+	if unlocker == nil {
 		return fmt.Errorf("unlocker with ID %s %w", unlockerID, ErrUnlockerNotFound)
 	}