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

@@ -90,7 +90,8 @@ remove things pass `--force`.
 #### `secret init`
 
 Initializes the secret manager with a default vault. Prompts for a BIP39
-mnemonic phrase and creates the initial directory structure.
+mnemonic phrase and creates the initial directory structure. When the vault
+`default` already exists, it fails before asking for the mnemonic or passphrase.
 
 **Environment Variables:**
 
@@ -105,7 +106,8 @@ Lists all available vaults. The current vault is marked.
 
 #### `secret vault create <name>`
 
-Creates a new vault with the specified name.
+Creates a new vault with the specified name. When a vault of that name already
+exists, it fails before asking for the mnemonic or passphrase.
 
 **Vault Name Format:** only lowercase ASCII letters, digits, `.`, `-` and `_`
 are allowed, and a name must not be empty, `.` or `..`.
@@ -142,7 +144,13 @@ contain `//`, or have `..` as a path segment.
 
 #### `secret get <secret-name> [--version <version>]`
 
-Retrieves and outputs a secret value to stdout.
+Retrieves and outputs a secret value to stdout. A failed write of the value,
+such as to a file on a full disk, prints the error and exits 1, with two
+exceptions. If the program reading from a pipe has already exited, as in
+`secret get s | true`, the broken-pipe signal (SIGPIPE) ends `secret` with no
+message and the shell reports exit status 141. If the shell closed stdout before
+`secret` started (`secret get s >&-`), the value is written to `/dev/null` and
+`secret get` exits 0.
 
 - `--version, -v`: Get a specific version (default: current)
 
@@ -243,7 +251,7 @@ A vault has one passphrase unlocker: adding one replaces the one the vault has,
 which is removed only once the new one is the current unlocker.
 
 `secret unlocker add passphrase` asks on the terminal for the new passphrase
-even when `SB_UNLOCK_PASSPHRASE` is set, since the variable only unlocks the
+even when `SB_UNLOCK_PASSPHRASE` is set, and uses the variable to unlock the
 vault. With no controlling terminal, as under cron or in a CI job, the new
 unlocker takes its passphrase from `SB_UNLOCK_PASSPHRASE`, and the command says
 so.

internal/cli/create_vault_test.go

@@ -222,12 +222,51 @@ func TestMnemonicNotReadNamesOnlyMnemonic(t *testing.T) {
 	output, err := cmd.CombinedOutput()
 	require.Error(t, err)
 
-	require.Equal(t, "Initialized secrets manager at: "+stateDir+"\n"+
-		"Error: failed to read mnemonic: there is no terminal to ask on. "+
-		"Please set the SB_SECRET_MNEMONIC environment variable or run "+
+	require.Equal(t, "Error: failed to read mnemonic: there is no terminal to "+
+		"ask on. Please set the SB_SECRET_MNEMONIC environment variable or run "+
 		"interactively\n", string(output))
 }
 
+// TestCreateExistingVaultAsksForNothing is a regression test for
+// https://git.eeqj.de/sneak/secret/issues/151: `secret init` and `secret
+// vault create` asked for the mnemonic and the unlocker passphrase before
+// refusing a vault that already exists, and `secret init` printed
+// "Initialized secrets manager at" first. Each runs here as the built binary,
+// in a new session, which has no terminal, and with neither
+// SB_SECRET_MNEMONIC nor SB_UNLOCK_PASSPHRASE set, so asking for either
+// fails. Each must fail saying only that the vault exists.
+func TestCreateExistingVaultAsksForNothing(t *testing.T) {
+	t.Parallel()
+
+	stateDir := t.TempDir()
+	_, err := vault.CreateVault(afero.NewOsFs(), stateDir, "default", nil, nil)
+	require.NoError(t, err)
+
+	// run runs the built binary with args, which must fail, and returns what
+	// it printed
+	run := func(args ...string) string {
+		//nolint:gosec // G204: test executes the freshly built secret binary
+		cmd := exec.CommandContext(t.Context(), secretBinaryPath(t), args...)
+		cmd.Env = []string{
+			secret.EnvStateDir + "=" + stateDir,
+			"PATH=" + os.Getenv("PATH"),
+			"HOME=" + os.Getenv("HOME"),
+		}
+		cmd.SysProcAttr = &syscall.SysProcAttr{Setsid: true}
+
+		output, err := cmd.CombinedOutput()
+		require.Error(t, err, "secret %v", args)
+
+		return string(output)
+	}
+
+	require.Equal(t,
+		"Error: failed to create default vault: vault default already exists\n",
+		run("init"))
+	require.Equal(t, "Error: vault default already exists\n",
+		run("vault", "create", "default"))
+}
+
 // TestStopDuringCreateLeavesWholeVaultOrNone is a regression test for
 // https://git.eeqj.de/sneak/secret/issues/105: `secret init` or `secret vault
 // create` killed after the passphrase prompt but before the unlocker was

internal/cli/init.go

@@ -17,6 +17,9 @@ import (
 // errPassphraseMismatch is returned when passphrase confirmation fails
 var errPassphraseMismatch = errors.New("passphrases do not match")
 
+// defaultVaultName is the name of the vault `secret init` creates
+const defaultVaultName = "default"
+
 // NewInitCmd creates the init command
 func NewInitCmd() *cobra.Command {
 	return &cobra.Command{
@@ -92,8 +95,10 @@ func (cli *Instance) initialize(cmd *cobra.Command) error {
 		return fmt.Errorf("failed to create state directory: %w", err)
 	}
 
-	if cmd != nil {
-		cmd.Printf("Initialized secrets manager at: %s\n", stateDir)
+	// Refuse an existing default vault before asking for anything
+	err = vault.CheckNewVaultName(cli.fs, cli.stateDir, defaultVaultName)
+	if err != nil {
+		return fmt.Errorf("failed to create default vault: %w", err)
 	}
 
 	// Prompt for mnemonic
@@ -133,7 +138,7 @@ func (cli *Instance) initialize(cmd *cobra.Command) error {
 	// Create the default vault with its passphrase unlocker
 	secret.Debug("Creating default vault")
 
-	vlt, err := vault.CreateVault(cli.fs, cli.stateDir, "default",
+	vlt, err := vault.CreateVault(cli.fs, cli.stateDir, defaultVaultName,
 		mnemonic, passphraseBuffer)
 	if err != nil {
 		secret.Debug("Failed to create default vault", "error", err)
@@ -152,6 +157,7 @@ func (cli *Instance) initialize(cmd *cobra.Command) error {
 	}
 
 	if cmd != nil {
+		cmd.Printf("Initialized secrets manager at: %s\n", stateDir)
 		cmd.Printf("\nDefault vault created and configured\n")
 		cmd.Printf("Long-term public key: %s\n", ltIdentity.Recipient().String())
 		cmd.Printf("Unlocker ID: %s\n", unlocker.GetID())

internal/cli/secrets.go

@@ -443,7 +443,10 @@ func (cli *Instance) GetSecret(cmd *cobra.Command, secretName string) error {
 	defer value.Destroy()
 
 	// Write the value straight from locked memory, with no trailing newline
-	_, _ = cmd.OutOrStdout().Write(value.Bytes())
+	_, err = cmd.OutOrStdout().Write(value.Bytes())
+	if err != nil {
+		return fmt.Errorf("failed to write secret value: %w", err)
+	}
 
 	return nil
 }
@@ -478,7 +481,10 @@ func (cli *Instance) GetSecretWithVersion(
 	secret.Debug("Got secret value", "valueLength", value.Size())
 
 	// Write the value straight from locked memory, with no trailing newline
-	_, _ = cmd.OutOrStdout().Write(value.Bytes())
+	_, err = cmd.OutOrStdout().Write(value.Bytes())
+	if err != nil {
+		return fmt.Errorf("failed to write secret value: %w", err)
+	}
 
 	secret.Debug("Printed value to stdout")
 

internal/cli/unlockers.go

@@ -441,7 +441,7 @@ func (cli *Instance) addPassphraseUnlocker(cmd *cobra.Command) error {
 	vlt.Mnemonic, vlt.UnlockPassphrase = cli.Mnemonic, cli.UnlockPassphrase
 
 	// The new passphrase is asked for even when SB_UNLOCK_PASSPHRASE is
-	// set, since that only unlocks the vault. With no terminal to ask on,
+	// set, and the variable unlocks the vault. With no terminal to ask on,
 	// the new unlocker takes the passphrase in the variable.
 	fromVariable := false
 

internal/cli/vault.go

@@ -267,6 +267,12 @@ func (cli *Instance) CreateVault(cmd *cobra.Command, name string) error {
 	}
 	defer release()
 
+	// Refuse an invalid name or an existing vault before asking for anything
+	err = vault.CheckNewVaultName(cli.fs, cli.stateDir, name)
+	if err != nil {
+		return err
+	}
+
 	mnemonic, cleanupMnemonic, err := cli.promptMnemonic()
 	if err != nil {
 		return err

internal/cli/version_test.go

@@ -8,6 +8,7 @@
 // - TestPromoteNonExistentVersion: Tests error handling for invalid promotion
 // - TestGetSecretWithVersion: Tests `secret get --version` flag functionality
 // - TestGetSecretWritesBinaryValue: Tests `secret get` output of binary values
+// - TestGetSecretReturnsWriteError: Tests `secret get` when stdout fails
 // - TestVersionCommandStructure: Tests command structure and help text
 // - TestListVersionsEmptyOutput: Tests edge case with no versions
 //
@@ -20,6 +21,7 @@ package cli
 
 import (
 	"bytes"
+	"os"
 	"path/filepath"
 	"strings"
 	"testing"
@@ -360,6 +362,45 @@ func TestGetSecretWritesBinaryValue(t *testing.T) {
 	assert.Equal(t, value, buf.Bytes())
 }
 
+// TestGetSecretReturnsWriteError checks that `secret get`, with and without
+// a version, returns the error of a failed write of the value.
+func TestGetSecretReturnsWriteError(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)
+
+	addTestSecret(t, vlt, []byte("value"), false)
+
+	vaultDir, err := vlt.GetDirectory()
+	require.NoError(t, err)
+
+	versions, err := secret.ListVersions(fs,
+		filepath.Join(vaultDir, "secrets.d", "test%secret"))
+	require.NoError(t, err)
+	require.Len(t, versions, 1)
+
+	// Stdout is a file closed before the value is written to it
+	out, err := os.Create(filepath.Join(t.TempDir(), "stdout"))
+	require.NoError(t, err)
+	require.NoError(t, out.Close())
+
+	cmd := newRootCmd()
+	cmd.SetOut(out)
+
+	err = cli.GetSecret(cmd, "test/secret")
+	require.ErrorIs(t, err, os.ErrClosed)
+
+	err = cli.GetSecretWithVersion(cmd, "test/secret", versions[0])
+	require.ErrorIs(t, err, os.ErrClosed)
+}
+
 //nolint:paralleltest // reads process environment to determine the state dir
 func TestVersionCommandStructure(t *testing.T) {
 	// Test that version commands are properly structured

internal/vault/management.go

@@ -212,16 +212,40 @@ func processMnemonicForVault(
 	return ltIdentity, derivationIndex, publicKeyHash, familyHash, nil
 }
 
+// CheckNewVaultName returns an error when name cannot be the name of a new
+// vault: one wrapping ErrInvalidVaultName when it is not a valid vault name,
+// and one wrapping ErrVaultExists when that vault already exists. Creating an
+// existing vault again would replace its keys, and its secrets could no
+// longer be decrypted. CreateVault checks this before writing anything; the
+// commands that create a vault also check it before they ask for the
+// mnemonic or passphrase, holding the state directory lock.
+func CheckNewVaultName(fs afero.Fs, stateDir string, name string) error {
+	err := ValidateVaultName(name)
+	if err != nil {
+		return err
+	}
+
+	exists, err := afero.DirExists(fs, filepath.Join(stateDir, "vaults.d", name))
+	if err != nil {
+		return fmt.Errorf("failed to check if vault exists: %w", err)
+	}
+
+	if exists {
+		return fmt.Errorf("vault %s %w", name, ErrVaultExists)
+	}
+
+	return nil
+}
+
 // CreateVault creates a new vault and selects it as the current vault. When
 // mnemonic is not nil, the vault's long-term key is derived from it, and the
 // returned vault has it as its Mnemonic; when it is nil, the vault has no
 // long-term key until one is imported. When passphrase is not nil, the vault
 // gets a passphrase unlocker protected by it, as its current unlocker; that
-// needs a mnemonic. It refuses a vault that already exists before writing
-// anything: creating it again would replace its keys, and its secrets could
-// no longer be decrypted. The commands that call it hold the state directory
-// lock, so no other command can create the vault between the check and the
-// writes.
+// needs a mnemonic. It refuses an invalid name or a vault that already exists,
+// as CheckNewVaultName does, before writing anything. The commands that call
+// it hold the state directory lock, so no other command can create the vault
+// between the check and the writes.
 //
 // The vault is written whole into a temporary directory, which is renamed
 // into vaults.d only once complete, and only then selected: a crash at any
@@ -233,30 +257,17 @@ func CreateVault(
 ) (*Vault, error) {
 	secret.Debug("Creating new vault", "name", name, "state_dir", stateDir)
 
-	err := ValidateVaultName(name)
+	err := CheckNewVaultName(fs, stateDir, name)
 	if err != nil {
-		secret.Debug("Invalid vault name provided", "vault_name", name)
-
 		return nil, err
 	}
 
-	secret.Debug("Vault name validation passed", "vault_name", name)
-
-	vaultDir := filepath.Join(stateDir, "vaults.d", name)
-
-	exists, err := afero.DirExists(fs, vaultDir)
-	if err != nil {
-		return nil, fmt.Errorf("failed to check if vault exists: %w", err)
-	}
-
-	if exists {
-		return nil, fmt.Errorf("vault %s %w", name, ErrVaultExists)
-	}
-
 	if passphrase != nil && mnemonic == nil {
 		return nil, fmt.Errorf("vault %s %w", name, ErrUnlockerWithoutMnemonic)
 	}
 
+	vaultDir := filepath.Join(stateDir, "vaults.d", name)
+
 	secret.Debug("Creating vault directory structure", "vault_dir", vaultDir)
 
 	err = secret.WriteDir(fs, vaultDir, func(dir string) error {