Keep unlocker list working when unlocker metadata is corrupt (closes #42)

PGPUnlocker.GetID() panicked when its metadata could not be read or
parsed, which took down `secret unlocker list` for every unlocker. It
now warns with the unlocker's directory and returns `pgp-unknown`;
metadata with an empty GPG key ID counts as corrupt too.
ListUnlockers now skips, with a warning, an unlocker whose metadata
file cannot be checked for, read or parsed, as it already did for a
missing one. The listing's ID lookup skips such a directory without
warning again.

This is the first half of the issue only. Passing the mnemonic in
memory moved to #60.

Model: opus-5-5

TODO.md

@@ -25,6 +25,12 @@ Bring the repo into policy compliance in one commit:
 
 # Completed Steps
 
+- 2026-10-04: A PGP unlocker whose metadata has no usable GPG key ID
+  no longer panics: `GetID()` warns with the unlocker's directory and
+  returns `pgp-unknown`. `ListUnlockers` skips, with a warning, an
+  unlocker whose metadata file cannot be checked for, read or parsed
+  instead of failing, so `secret unlocker list` still lists the others;
+  the listing's ID lookup no longer warns about that directory again.
 - 2026-10-03: `version rm`, `version promote` and `get --version`
   accept a version only if it is one of the versions `version list`
   lists for that secret, compared as typed before any path is built
@@ -125,8 +131,6 @@ Bring the repo into policy compliance in one commit:
     - Timing attacks: bytes.Equal passphrase compare (cli/init.go:
       209-216); non-constant-time public key compare (vault.go:95-100).
 - High priority:
-    - Return errors instead of panicking on corrupted metadata
-      (pgpunlocker.go:116, keychainunlocker.go:141).
     - Secure temporary file handling and cleanup.
     - Print cobra usage only for argument errors, not internal
       failures.

internal/cli/unlockers.go

@@ -349,6 +349,10 @@ func unlockerIDFromDir(
 // itself cannot be read. Callers must distinguish the two: an unreadable
 // directory means the unlocker's real ID is unknowable, so the entry has
 // to be skipped rather than reported under a synthesized ID.
+//
+// A metadata file that cannot be read or parsed is skipped without a
+// warning: every caller gets metadata from vault.ListUnlockers first,
+// which has already warned about that directory.
 func findUnlockerIDByMetadata(
 	fs afero.Fs, unlockersDir string, metadata secret.UnlockerMetadata,
 	includeSecureEnclave bool,
@@ -371,9 +375,6 @@ func findUnlockerIDByMetadata(
 		// Check if this is the right unlocker by comparing metadata
 		metadataBytes, err := afero.ReadFile(fs, metadataPath)
 		if err != nil {
-			secret.Warn("Could not read unlocker metadata file",
-				"path", metadataPath, "error", err)
-
 			continue
 		}
 
@@ -381,9 +382,6 @@ func findUnlockerIDByMetadata(
 
 		err = json.Unmarshal(metadataBytes, &diskMetadata)
 		if err != nil {
-			secret.Warn("Could not parse unlocker metadata file",
-				"path", metadataPath, "error", err)
-
 			continue
 		}
 

internal/cli/unlockers_list_test.go

@@ -1,13 +1,19 @@
 // Unlocker List Tests
 //
-// Tests for `secret unlocker list` behavior when the unlockers.d directory
-// cannot be read while the listing is being rendered:
+// Tests for `secret unlocker list` behavior when the unlockers.d directory,
+// or an unlocker's metadata in it, cannot be read while the listing is
+// being rendered:
 //
 //   - TestUnlockersListSkipsUnreadableUnlockersDir: an unreadable
 //     unlockers.d yields no rows rather than rows bearing synthesized IDs.
 //   - TestUnlockersListSkipsOnlyUnreadableEntries: a readable entry is
 //     still listed, with its real ID and its current-unlocker marker,
 //     when a later entry's scan fails.
+//   - TestUnlockersListToleratesCorruptMetadata: one unlocker's corrupt
+//     metadata does not stop the others from being listed.
+//   - TestUnlockersListSkipsUnreadableMetadata: an unlocker whose metadata
+//     file cannot be checked for or read is left out, and the other is
+//     still listed.
 //
 // The listing resolves each unlocker's real ID by rescanning unlockers.d
 // after the vault has already enumerated it. If that rescan fails the ID
@@ -22,6 +28,7 @@ import (
 	"bytes"
 	"encoding/json"
 	"errors"
+	"os"
 	"path/filepath"
 	"testing"
 	"time"
@@ -92,6 +99,49 @@ func (f *unlockersDirFailFs) Open(name string) (afero.File, error) {
 	return f.Fs.Open(name)
 }
 
+// errMetadataUnreadable is returned by the test filesystem in place of a
+// successful open of one unlocker's metadata file.
+var errMetadataUnreadable = errors.New("input/output error")
+
+// metadataReadFailFs fails every open of the file at unreadablePath. The
+// file still exists, so checking for it succeeds and only reading it fails.
+type metadataReadFailFs struct {
+	afero.Fs
+
+	unreadablePath string
+}
+
+//nolint:ireturn // afero.File is the interface required by afero.Fs
+func (f *metadataReadFailFs) Open(name string) (afero.File, error) {
+	if name == f.unreadablePath {
+		return nil, errMetadataUnreadable
+	}
+
+	//nolint:wrapcheck // test double must return the wrapped Fs error as-is
+	return f.Fs.Open(name)
+}
+
+// errMetadataUncheckable is returned by the test filesystem in place of a
+// successful check for one unlocker's metadata file.
+var errMetadataUncheckable = errors.New("permission denied")
+
+// metadataStatFailFs fails every check for whether the file at
+// uncheckablePath exists, as when its unlocker directory cannot be entered.
+type metadataStatFailFs struct {
+	afero.Fs
+
+	uncheckablePath string
+}
+
+func (f *metadataStatFailFs) Stat(name string) (os.FileInfo, error) {
+	if name == f.uncheckablePath {
+		return nil, errMetadataUncheckable
+	}
+
+	//nolint:wrapcheck // test double must return the wrapped Fs error as-is
+	return f.Fs.Stat(name)
+}
+
 // writePGPUnlocker writes a PGP unlocker directory with metadata that
 // yields the real ID "pgp-<keyID>".
 func writePGPUnlocker(
@@ -227,3 +277,102 @@ func TestUnlockersListReadableEntriesAreListed(t *testing.T) {
 	assert.True(t, unlockers[0].IsCurrent)
 	assert.False(t, unlockers[1].IsCurrent)
 }
+
+// TestUnlockersListToleratesCorruptMetadata asserts that one unlocker with
+// corrupt metadata does not stop the listing. Metadata that is not JSON
+// leaves that unlocker out; PGP metadata without a usable GPG key ID lists
+// it as "pgp-unknown". The healthy unlocker is listed with its real ID.
+func TestUnlockersListToleratesCorruptMetadata(t *testing.T) {
+	t.Parallel()
+
+	healthyID := "pgp-" + listTestGPGKeyID + "A"
+
+	tests := []struct {
+		name     string
+		metadata string
+		wantIDs  []string
+	}{
+		{
+			name:     "not JSON",
+			metadata: "not json",
+			wantIDs:  []string{healthyID},
+		},
+		{
+			name:     "GPG key ID of the wrong type",
+			metadata: `{"type": "pgp", "gpgKeyId": 42}`,
+			wantIDs:  []string{healthyID, "pgp-unknown"},
+		},
+		{
+			name:     "GPG key ID missing",
+			metadata: `{"type": "pgp"}`,
+			wantIDs:  []string{healthyID, "pgp-unknown"},
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			t.Parallel()
+
+			fs := newListTestVault(t, 2)
+			metadataPath := filepath.Join(listTestStateDir, "vaults.d",
+				listTestVaultName, listTestUnlockersDirName,
+				listTestUnlockerDirTwo, listTestMetadataFileName)
+			require.NoError(t, afero.WriteFile(
+				fs, metadataPath, []byte(tt.metadata), listTestFilePerm,
+			))
+
+			unlockers := listUnlockersJSON(t, fs)
+			require.Len(t, unlockers, len(tt.wantIDs))
+
+			for i, wantID := range tt.wantIDs {
+				assert.Equal(t, wantID, unlockers[i].ID)
+			}
+		})
+	}
+}
+
+// TestUnlockersListSkipsUnreadableMetadata asserts that an unlocker whose
+// metadata file cannot be checked for or cannot be read is left out of the
+// listing, and the other unlocker is still listed with its real ID. The
+// failing one sorts first, so finding the other's ID has to step past it
+// as well.
+func TestUnlockersListSkipsUnreadableMetadata(t *testing.T) {
+	t.Parallel()
+
+	failingPath := filepath.Join(listTestStateDir, "vaults.d",
+		listTestVaultName, listTestUnlockersDirName,
+		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()
+
+			fs := tt.wrap(newListTestVault(t, 2))
+
+			unlockers := listUnlockersJSON(t, fs)
+
+			require.Len(t, unlockers, 1,
+				"only the unlocker with usable metadata may be listed")
+			assert.Equal(t, "pgp-"+listTestGPGKeyID+"B", unlockers[0].ID,
+				"the listed row must carry the real unlocker ID")
+		})
+	}
+}

internal/secret/pgpunlocker.go

@@ -155,14 +155,18 @@ func (p *PGPUnlocker) GetDirectory() string {
 	return p.Directory
 }
 
-// GetID implements Unlocker interface - generates ID from GPG key ID
+// GetID implements Unlocker interface - generates ID from GPG key ID.
+// If the metadata has no usable GPG key ID, it warns with the unlocker's
+// directory and returns "pgp-unknown", so listing the other unlockers
+// still works.
 func (p *PGPUnlocker) GetID() string {
 	// Generate ID using GPG key ID: pgp-<keyid>
 	gpgKeyID, err := p.GetGPGKeyID()
 	if err != nil {
-		// The vault metadata is corrupt - this is a fatal error
-		// We cannot continue with a fallback ID as that would mask data corruption
-		panic(fmt.Sprintf("PGP unlocker metadata is corrupt or missing GPG key ID: %v", err))
+		Warn("PGP unlocker metadata is corrupt or missing its GPG key ID",
+			"directory", p.Directory, "error", err)
+
+		return "pgp-unknown"
 	}
 
 	return "pgp-" + gpgKeyID
@@ -197,6 +201,10 @@ func (p *PGPUnlocker) GetGPGKeyID() (string, error) {
 		return "", fmt.Errorf("failed to parse PGP metadata: %w", err)
 	}
 
+	if pgpMetadata.GPGKeyID == "" {
+		return "", fmt.Errorf("PGP metadata: %w", errGPGKeyIDEmpty)
+	}
+
 	return pgpMetadata.GPGKeyID, nil
 }
 

internal/vault/unlockers.go

@@ -233,9 +233,10 @@ func (v *Vault) ListUnlockers() ([]UnlockerMetadata, error) {
 
 			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)
+				secret.Warn("Skipping unlocker directory whose metadata file cannot be checked",
+					"directory", file.Name(), "error", err)
+
+				continue
 			}
 
 			if !exists {
@@ -247,16 +248,20 @@ func (v *Vault) ListUnlockers() ([]UnlockerMetadata, error) {
 
 			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)
+				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 {
-				return nil, fmt.Errorf(
-					"failed to parse metadata for unlocker %s: %w", file.Name(), err)
+				secret.Warn("Skipping unlocker directory with corrupt metadata file",
+					"directory", file.Name(), "error", err)
+
+				continue
 			}
 
 			unlockers = append(unlockers, metadata)