Ask for the new passphrase in unlocker add when the variable is set (closes #154)

With SB_UNLOCK_PASSPHRASE set, `secret unlocker add passphrase` gave the
new unlocker the passphrase in the variable without asking. It now asks
on the terminal for the new passphrase and uses the variable to unlock
the vault. With no controlling terminal the new unlocker takes the
variable's passphrase, and the output says so.

secret.ErrNoTerminal is exported so that the command can tell a missing
terminal from other failures to read, such as nothing typed or a
mismatched confirmation, which still fail the command.

The in-process tests that add a passphrase unlocker with the variable
set rely on the Docker test phase having no controlling terminal; at a
terminal they would ask.

Model: opus-5-5

README.md

@@ -204,12 +204,6 @@ 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

internal/cli/passphrase_prompt_test.go

@@ -143,8 +143,8 @@ func TestInitWritesNoBlankLineAfterMnemonic(t *testing.T) {
 // `secret unlocker add passphrase` gave the new unlocker the passphrase in
 // the variable without asking. At a terminal it must ask for the new
 // passphrase there, twice, and unlock the vault with the variable, so that
-// `secret get` then opens the vault with the passphrase typed. pty.Open is
+// the vault then opens with the passphrase typed. pty.Open is described
-// described above TestRemoveIgnoresTerminalOnStdout.
+// above TestRemoveIgnoresTerminalOnStdout.
 func TestUnlockerAddAsksForNewPassphrase(t *testing.T) {
 	t.Parallel()
 
@@ -155,15 +155,10 @@ func TestUnlockerAddAsksForNewPassphrase(t *testing.T) {
 	passphrase := memguard.NewBufferFromBytes([]byte(testPassphrase))
 	defer passphrase.Destroy()
 
-	vlt, err := vault.CreateVault(afero.NewOsFs(), stateDir, "default",
+	_, err := vault.CreateVault(afero.NewOsFs(), stateDir, "default",
 		testMnemonicBuffer(t), passphrase)
 	require.NoError(t, err)
 
-	value := memguard.NewBufferFromBytes([]byte("v1"))
-	defer value.Destroy()
-
-	require.NoError(t, vlt.AddSecret("s1", value, false))
-
 	ctx, cancel := context.WithTimeout(t.Context(), commandWait)
 	defer cancel()
 
@@ -222,17 +217,21 @@ func TestUnlockerAddAsksForNewPassphrase(t *testing.T) {
 	require.NoError(t, cmd.Wait(),
 		"secret unlocker add passphrase: %s%s", shown, rest)
 
-	// The binary, not this test process, opens the vault: the new unlocker
+	typed := memguard.NewBufferFromBytes([]byte(newPassphrase))
-	// has age's full scrypt work factor, which is slow under the race
+	defer typed.Destroy()
-	// detector.
+
-	assert.Equal(t, "v1",
+	reopened := vault.NewVault(afero.NewOsFs(), stateDir, "default")
-		runWithoutTerminal(t, stateDir, newPassphrase, "get", "s1"))
+	reopened.UnlockPassphrase = typed
+
+	_, err = reopened.GetOrDeriveLongTermKey()
+	require.NoError(t, err, "the vault does not open with the passphrase typed")
 }
 
 // TestUnlockerAddWithoutTerminalSaysPassphraseIsVariable is a regression
 // test for https://git.eeqj.de/sneak/secret/issues/154: with no terminal to
 // ask on, `secret unlocker add passphrase` gives the new unlocker the
-// passphrase in SB_UNLOCK_PASSPHRASE, and its output must say so.
+// passphrase in SB_UNLOCK_PASSPHRASE, and its output must say so. The binary
+// runs in a new session, which has no terminal.
 func TestUnlockerAddWithoutTerminalSaysPassphraseIsVariable(t *testing.T) {
 	t.Parallel()
 
@@ -245,39 +244,26 @@ func TestUnlockerAddWithoutTerminalSaysPassphraseIsVariable(t *testing.T) {
 		testMnemonicBuffer(t), passphrase)
 	require.NoError(t, err)
 
-	output := runWithoutTerminal(t, stateDir, testPassphrase,
-		"unlocker", "add", "passphrase")
-
-	assert.Contains(t, output, "The new unlocker takes its passphrase "+
-		"from SB_UNLOCK_PASSPHRASE, since there is no terminal to ask for a "+
-		"new one on\n")
-}
-
-// runWithoutTerminal runs the secret binary with args and SB_UNLOCK_PASSPHRASE
-// set to passphrase, in a new session, which has no terminal, and returns
-// what it printed once it has succeeded.
-func runWithoutTerminal(
-	t *testing.T, stateDir, passphrase string, args ...string,
-) string {
-	t.Helper()
-
 	ctx, cancel := context.WithTimeout(t.Context(), commandWait)
 	defer cancel()
 
 	//nolint:gosec // G204: test executes the freshly built secret binary
-	cmd := exec.CommandContext(ctx, secretBinaryPath(t), args...)
+	cmd := exec.CommandContext(ctx, secretBinaryPath(t),
+		"unlocker", "add", "passphrase")
 	cmd.Env = []string{
 		secret.EnvStateDir + "=" + stateDir,
-		secret.EnvUnlockPassphrase + "=" + passphrase,
+		secret.EnvUnlockPassphrase + "=" + testPassphrase,
 		"PATH=" + os.Getenv("PATH"),
 		"HOME=" + os.Getenv("HOME"),
 	}
 	cmd.SysProcAttr = &syscall.SysProcAttr{Setsid: true}
 
 	output, err := cmd.CombinedOutput()
-	require.NoError(t, err, "secret %v: %s", args, output)
+	require.NoError(t, err, "secret unlocker add passphrase: %s", output)
 
-	return string(output)
+	assert.Contains(t, string(output), "The new unlocker takes its passphrase "+
+		"from SB_UNLOCK_PASSPHRASE, since there is no terminal to ask for a "+
+		"new one on\n")
 }
 
 // runAtTerminal runs the secret binary with args and input on stdin, in a

internal/cli/unlockers.go

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

internal/cli/unlockers_list_test.go

@@ -1,14 +1,13 @@
 // Unlocker List Tests
 //
-// Tests for `secret unlocker list`:
+// Tests for `secret unlocker list` behavior when an unlocker's metadata
+// cannot be read or used:
 //
 //   - 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
@@ -322,36 +321,3 @@ 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

@@ -1,42 +0,0 @@
-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,13 +229,9 @@ func printVersionRow(
 	}
 
 	// Determine status
-	status := "inactive"
+	status := "expired"
-
+	if version == currentVersion {
-	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,8 +5,6 @@
 // - 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
@@ -246,66 +244,6 @@ 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,8 +32,7 @@ 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 is set when another version is added while this one is current
+	NotAfter  *time.Time `json:"notAfter,omitempty"`  // Expiry (nil = current)
-	NotAfter *time.Time `json:"notAfter,omitempty"`
 }
 
 // Version represents a version of a secret

internal/vault/management.go

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