Stop vault creation when a vault's metadata cannot be read (closes #140)

GetNextDerivationIndex passed over a vault whose vault-metadata.json could
not be read or parsed, so the next vault made from the same mnemonic could
get that vault's derivation index and so the same long-term key. It now
returns an error naming the file, so vault create and init stop with that
error and leave the state directory as it was. A vault directory with no
metadata file is still skipped.

The regression test breaks one vault's metadata, once with content that
does not parse and once with a test filesystem that fails to open it, and
requires CreateVault with the same mnemonic to fail, naming the file, and
leave no new vault.

Model: opus-5-5

README.md

@@ -204,6 +204,12 @@ Moves or renames a secret within the current vault.
 Lists all versions of a secret showing creation time, status, and validity
 period.
 
+`STATUS` is `current` for the secret's current version, `expired` for another
+version whose `NOT_AFTER` has passed, and `inactive` for any other version.
+Adding a version sets `NOT_AFTER` on the version that was current;
+`secret version promote` sets no times. A version that is not current therefore
+shows `inactive` when no version was added while it was current.
+
 #### `secret version promote <secret-name> <version>`
 
 Promotes a specific version to current by rewriting the secret's `current` file
@@ -544,7 +550,11 @@ secret decrypt encryption/mykey --input document.txt.age --output document.txt
 ### Vault Management
 
 - **Derivation Index**: Each vault uses a unique derivation index from the
-  mnemonic, and thus a unique key pair
+  mnemonic, and thus a unique key pair; creating a vault (`secret init` and
+  `secret vault create`) fails, with an error naming the file, when any vault's
+  `vault-metadata.json` exists but cannot be read or parsed; a vault directory
+  with no `vault-metadata.json` is skipped, so the index it uses can be given to
+  a new vault
 - **Public Key Hash**: Double SHA-256 hash of the vault's public key; the same
   hash of the index-0 public key identifies vaults from the same mnemonic
 - **Automatic Key Derivation**: When creating vaults with a mnemonic, keys are

internal/cli/unlockers.go

@@ -314,7 +314,8 @@ func (cli *Instance) UnlockersList(jsonOutput bool) error {
 		return err
 	}
 
-	var unlockers []UnlockerInfo
+	// Not nil, so that a vault with no unlockers prints [] in JSON, not null
+	unlockers := make([]UnlockerInfo, 0, len(unlockerMetadata))
 
 	for _, unlockerID := range slices.Sorted(maps.Keys(unlockerMetadata)) {
 		metadata := unlockerMetadata[unlockerID]

internal/cli/unlockers_list_test.go

@@ -1,13 +1,14 @@
 // Unlocker List Tests
 //
-// Tests for `secret unlocker list` behavior when an unlocker's metadata
-// cannot be read or used:
+// Tests for `secret unlocker list`:
 //
 //   - 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.
+//   - TestUnlockersListEmptyJSON: a vault with no unlockers lists them as
+//     an empty JSON list.
 
 //nolint:testpackage // white-box test of unexported internals
 package cli
@@ -321,3 +322,36 @@ func TestUnlockersListSkipsUnreadableMetadata(t *testing.T) {
 		})
 	}
 }
+
+// TestUnlockersListEmptyJSON is a regression test for
+// https://git.eeqj.de/sneak/secret/issues/148: listing the unlockers of a
+// vault that has none, as after its last unlocker is removed, printed
+// "unlockers": null in JSON form instead of an empty list.
+func TestUnlockersListEmptyJSON(t *testing.T) {
+	t.Parallel()
+
+	fs := afero.NewMemMapFs()
+	vaultDir := filepath.Join(listTestStateDir, "vaults.d", listTestVaultName)
+
+	require.NoError(t, afero.WriteFile(
+		fs, filepath.Join(listTestStateDir, "currentvault"),
+		[]byte(listTestVaultName), listTestFilePerm,
+	))
+	require.NoError(t, fs.MkdirAll(
+		filepath.Join(vaultDir, listTestUnlockersDirName), listTestDirPerm,
+	))
+
+	var buf bytes.Buffer
+
+	cmd := &cobra.Command{}
+	cmd.SetOut(&buf)
+
+	instance := &Instance{fs: fs, stateDir: listTestStateDir, cmd: cmd}
+	require.NoError(t, instance.UnlockersList(true))
+
+	var decoded map[string]any
+
+	require.NoError(t, json.Unmarshal(buf.Bytes(), &decoded))
+	assert.Equal(t, []any{}, decoded["unlockers"],
+		"no unlockers must be printed as [], not %s", buf.String())
+}

internal/cli/vault_list_test.go

@@ -0,0 +1,42 @@
+package cli_test
+
+import (
+	"bytes"
+	"encoding/json"
+	"path/filepath"
+	"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"
+)
+
+// TestVaultListEmptyJSON is a regression test for
+// https://git.eeqj.de/sneak/secret/issues/148: listing the vaults when
+// vaults.d holds none printed "vaults": null in JSON form instead of an
+// empty list.
+func TestVaultListEmptyJSON(t *testing.T) {
+	t.Parallel()
+
+	fs := afero.NewMemMapFs()
+	require.NoError(t, fs.MkdirAll(
+		filepath.Join(testStateDir, "vaults.d"), secret.DirPerms,
+	))
+
+	var buf bytes.Buffer
+
+	cmd := &cobra.Command{}
+	cmd.SetOut(&buf)
+
+	c := cli.NewCLIInstanceWithStateDir(fs, testStateDir)
+	require.NoError(t, c.ListVaults(cmd, true))
+
+	var decoded map[string]any
+
+	require.NoError(t, json.Unmarshal(buf.Bytes(), &decoded))
+	assert.Equal(t, []any{}, decoded["vaults"],
+		"no vaults must be printed as [], not %s", buf.String())
+}

internal/cli/version.go

@@ -229,9 +229,13 @@ func printVersionRow(
 	}
 
 	// Determine status
-	status := "expired"
-	if version == currentVersion {
+	status := "inactive"
+
+	switch {
+	case version == currentVersion:
 		status = "current"
+	case sv.Metadata.NotAfter != nil && sv.Metadata.NotAfter.Before(time.Now()):
+		status = "expired"
 	}
 
 	// Format timestamps

internal/cli/version_test.go

@@ -5,6 +5,8 @@
 // - TestListVersionsCommand: Tests `secret version list` command output
 // - TestListVersionsNonExistentSecret: Tests error handling for missing secrets
 // - TestPromoteVersionCommand: Tests `secret version promote` command
+// - TestListVersionsStatusAfterPromote: Tests each version's status after
+//   promoting an older version
 // - TestPromoteNonExistentVersion: Tests error handling for invalid promotion
 // - TestPromoteHelpSaysCurrentFileIsRewritten: Tests `secret version promote
 //   --help` text
@@ -244,6 +246,66 @@ func TestPromoteVersionCommand(t *testing.T) {
 	assert.Equal(t, []byte("version-1"), promoted.Bytes())
 }
 
+func TestListVersionsStatusAfterPromote(t *testing.T) {
+	t.Parallel()
+
+	fs := afero.NewMemMapFs()
+	cli := NewCLIInstanceWithStateDir(fs, testStateDir)
+	cli.Mnemonic = testMnemonicBuffer(t)
+
+	setupTestVault(t, fs)
+
+	vlt, err := vault.GetCurrentVault(fs, testStateDir)
+	require.NoError(t, err)
+
+	vlt.Mnemonic = cli.Mnemonic
+
+	addTestSecret(t, vlt, []byte("version-1"), false)
+	addTestSecret(t, vlt, []byte("version-2"), true)
+	addTestSecret(t, vlt, []byte("version-3"), true)
+
+	vaultDir, err := vlt.GetDirectory()
+	require.NoError(t, err)
+
+	// Newest first
+	versions, err := secret.ListVersions(fs,
+		filepath.Join(vaultDir, "secrets.d", "test%secret"))
+	require.NoError(t, err)
+	require.Len(t, versions, 3)
+
+	newest, middle, oldest := versions[0], versions[1], versions[2]
+
+	cmd := newRootCmd()
+
+	var buf bytes.Buffer
+	cmd.SetOut(&buf)
+	cmd.SetErr(&buf)
+
+	err = cli.PromoteVersion(cmd, "test/secret", oldest)
+	require.NoError(t, err)
+
+	buf.Reset()
+
+	err = cli.ListVersions(cmd, "test/secret")
+	require.NoError(t, err)
+
+	// STATUS is the fourth field of a row, since CREATED is a date and a time
+	statuses := map[string]string{}
+
+	for line := range strings.SplitSeq(buf.String(), "\n") {
+		fields := strings.Fields(line)
+		if len(fields) > 3 {
+			statuses[fields[0]] = fields[3]
+		}
+	}
+
+	assert.Equal(t, "current", statuses[oldest])
+	// Its NOT_AFTER was set when the newest version was added
+	assert.Equal(t, "expired", statuses[middle])
+	// It has no NOT_AFTER
+	assert.Equal(t, "inactive", statuses[newest])
+}
+
 func TestPromoteNonExistentVersion(t *testing.T) {
 	t.Parallel()
 

internal/secret/version.go

@@ -32,7 +32,8 @@ type VersionMetadata struct {
 	ID        string     `json:"id"`                  // ULID
 	CreatedAt *time.Time `json:"createdAt,omitempty"` // When version was created
 	NotBefore *time.Time `json:"notBefore,omitempty"` // When this version becomes active
-	NotAfter  *time.Time `json:"notAfter,omitempty"`  // Expiry (nil = current)
+	// NotAfter is set when another version is added while this one is current
+	NotAfter *time.Time `json:"notAfter,omitempty"`
 }
 
 // Version represents a version of a secret

internal/vault/management.go

@@ -140,8 +140,9 @@ func ListVaults(fs afero.Fs, stateDir string) ([]string, error) {
 		return nil, fmt.Errorf("failed to read vaults directory: %w", err)
 	}
 
-	// Extract vault names
-	var vaults []string
+	// Extract vault names. Not nil, so that no vaults prints [] in JSON,
+	// not null.
+	vaults := make([]string, 0, len(entries))
 
 	for _, entry := range entries {
 		if entry.IsDir() {

internal/vault/metadata.go

@@ -4,7 +4,9 @@ import (
 	"crypto/sha256"
 	"encoding/hex"
 	"encoding/json"
+	"errors"
 	"fmt"
+	"os"
 	"path/filepath"
 
 	"github.com/spf13/afero"
@@ -33,7 +35,10 @@ func ComputeDoubleSHA256(data []byte) string {
 }
 
 // GetNextDerivationIndex finds the next available derivation index for a given mnemonic
-// by deriving the public key for index 0 and using its hash to identify related vaults
+// by deriving the public key for index 0 and using its hash to identify related vaults.
+// It fails when a vault's metadata file exists but cannot be read or parsed: the
+// index that vault uses is then unknown, and handing it out again would give a
+// second vault the same long-term key.
 func GetNextDerivationIndex(
 	fs afero.Fs, stateDir string, mnemonic string,
 ) (uint32, error) {
@@ -72,21 +77,23 @@ func GetNextDerivationIndex(
 			continue
 		}
 
-		// Try to read vault metadata
 		metadataPath := filepath.Join(vaultsDir, entry.Name(), "vault-metadata.json")
 
 		metadataBytes, err := afero.ReadFile(fs, metadataPath)
-		if err != nil {
-			// Skip vaults without metadata
+		if errors.Is(err, os.ErrNotExist) {
+			// Skip directories without metadata
 			continue
 		}
 
+		if err != nil {
+			return 0, fmt.Errorf("failed to read vault metadata %s: %w", metadataPath, err)
+		}
+
 		var metadata Metadata
 
 		err = json.Unmarshal(metadataBytes, &metadata)
 		if err != nil {
-			// Skip vaults with invalid metadata
-			continue
+			return 0, fmt.Errorf("failed to parse vault metadata %s: %w", metadataPath, err)
 		}
 
 		// Check if this vault uses the same mnemonic by comparing family hashes

internal/vault/metadata_test.go

@@ -1,11 +1,15 @@
 package vault_test
 
 import (
+	"errors"
 	"path/filepath"
 	"strings"
 	"testing"
 
 	"github.com/spf13/afero"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+	"sneak.berlin/go/secret/internal/secret"
 	"sneak.berlin/go/secret/internal/vault"
 	"sneak.berlin/go/secret/pkg/agehd"
 )
@@ -163,6 +167,88 @@ func testGetNextDerivationIndex(t *testing.T, fs afero.Fs) {
 	}
 }
 
+// errMetadataUnreadable is returned by the test filesystem in place of a
+// successful open of one vault'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
+	}
+
+	return f.Fs.Open(name)
+}
+
+// TestCreateVaultWithCorruptMetadataFails checks that creating a vault from
+// a mnemonic fails, naming the file and leaving no new vault, when another
+// vault's metadata file exists but cannot be parsed or read. That vault's
+// derivation index is then unknown, so the new vault could get the same one
+// and the same key.
+func TestCreateVaultWithCorruptMetadataFails(t *testing.T) {
+	t.Parallel()
+
+	vaultsDir := filepath.Join(testStateDir, "vaults.d")
+	metadataPath := filepath.Join(vaultsDir, "work", "vault-metadata.json")
+
+	tests := []struct {
+		name string
+		// breakMetadata makes the work vault's metadata file unusable on
+		// base and returns the filesystem to create the second vault on.
+		breakMetadata func(t *testing.T, base afero.Fs) afero.Fs
+	}{
+		{
+			name: "parsing the file fails",
+			breakMetadata: func(t *testing.T, base afero.Fs) afero.Fs {
+				t.Helper()
+
+				err := afero.WriteFile(base, metadataPath, []byte("{"),
+					secret.FilePerms)
+				require.NoError(t, err)
+
+				return base
+			},
+		},
+		{
+			name: "reading the file fails",
+			breakMetadata: func(_ *testing.T, base afero.Fs) afero.Fs {
+				return &metadataReadFailFs{Fs: base, unreadablePath: metadataPath}
+			},
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			t.Parallel()
+
+			base := afero.NewMemMapFs()
+
+			_, err := vault.CreateVault(base, testStateDir, "work",
+				testMnemonicBuffer(t), nil)
+			require.NoError(t, err)
+
+			fs := tt.breakMetadata(t, base)
+
+			_, err = vault.CreateVault(fs, testStateDir, "other",
+				testMnemonicBuffer(t), nil)
+			require.ErrorContains(t, err, metadataPath)
+
+			entries, err := afero.ReadDir(base, vaultsDir)
+			require.NoError(t, err)
+			require.Len(t, entries, 1)
+			assert.Equal(t, "work", entries[0].Name())
+		})
+	}
+}
+
 func testMetadataPersistence(t *testing.T, fs afero.Fs) {
 	t.Helper()