Write encrypt and decrypt output only once they succeed (closes #143)

`secret encrypt` and `secret decrypt` emptied the file named by
`--output` before reading any input, so a failure left it empty, and a
file given as both `--input` and `--output` was emptied before it was
read. They now write into a temporary file of mode 0600 next to it, or
next to the path it is a symbolic link to, and rename that over it only
on success, using the new `secret.WriteFileAtomicFunc`, which
`secret.WriteFileAtomic` now calls. Ctrl-C or SIGTERM removes the
temporary file. A device or pipe given as `--output`, such as
`/dev/null`, is written to directly.

Judgement call: the device and pipe case goes beyond the plan.

Model: opus-5-5

README.md

@@ -197,8 +197,7 @@ for confirmation, naming the version, the secret and its vault (see
 
 #### `secret generate mnemonic`
 
-Generates a cryptographically secure BIP39 mnemonic phrase and prints it to
-stdout. The warning to keep it safe goes to stderr.
+Generates a cryptographically secure BIP39 mnemonic phrase.
 
 #### `secret generate secret <name> [--length=16] [--type=base58] [--force]`
 
@@ -242,14 +241,6 @@ that the vault then opens only with its mnemonic (see
 [Confirmation Before Removal](#confirmation-before-removal)). An unlocker
 directory that `secret unlocker list` skips with a warning, because its metadata
 cannot be read or parsed, is removed by the directory name the warning gives.
-Removing the current unlocker makes the first of the vault's other unlockers
-that `secret unlocker select` accepts, in the order `secret unlocker list` shows
-them, the current one, and says which; when there is none, the vault is left
-without a current unlocker. `secret unlocker select` refuses an unlocker whose
-type is not `passphrase` or `pgp`. When `current-unlocker` names an unlocker
-that does not exist and the vault has another that `secret unlocker select`
-accepts, opening the vault fails with an error that says to select one with
-`secret unlocker select`.
 
 - `--force, -f`: Remove without asking, even the last unlocker
 - **CRITICAL WARNING**: Without unlockers and without your mnemonic phrase,
@@ -284,17 +275,12 @@ generates a new Age key.
 Decrypts data using an Age key stored as a secret.
 
 Both write the result to a new file next to `--output` and rename it over
-`--output` only once they have succeeded, so a failed command leaves an existing
-`--output` as it was and creates none, and `--input` and `--output` can be the
-same file. When `--output` is a symbolic link, the file it points to is
-replaced, or created if it does not exist yet, and the link kept; the new file
-is then made next to that file. The directory the new file is made in must be
-writable. The new file has mode 0600 and belongs to the user running the
-command, whatever mode and owner the file it replaces had, so a hard link to the
-old file keeps the old content. Ctrl-C, SIGTERM or a hangup removes the new
-file; a command killed any other way, such as with SIGKILL, leaves it there,
-named `.tmp-` and a number. A device or a pipe, such as `/dev/null`, is written
-to directly.
+`--output` only once they have succeeded, so a failed or interrupted command
+leaves an existing `--output` as it was and creates none, and `--input` and
+`--output` can be the same file. The file has mode 0600, whatever mode the file
+it replaced had. When `--output` is a symbolic link, the file it points to is
+replaced, or created if it does not exist yet, and the link kept. A device or a
+pipe, such as `/dev/null`, is written to directly.
 
 ## Storage Architecture
 
@@ -394,11 +380,6 @@ which every command except `secret vault import` offers when the variable is not
 set, is the safer default; `secret vault import` has no prompt and needs both
 variables.
 
-The prompt asks on the terminal the command runs in, not on stdin, so it also
-asks when stdin is a pipe, as in `echo value | secret add name --force` or
-`cat file | secret encrypt key`. With no controlling terminal, as under cron or
-in a CI job, the command fails and names the variable to set.
-
 ## Security Features
 
 ### Encryption

TODO.md

@@ -26,49 +26,9 @@ https://git.eeqj.de/sneak/secret/milestone/12
   temporary file next to it, or next to the file it is a symbolic link to, and
   rename that over it once they have succeeded, with the new
   `secret.WriteFileAtomicFunc`, which `secret.WriteFileAtomic` now calls; the
-  output file has mode 0600. An output file that cannot be written still fails
-  the command. Ctrl-C, SIGTERM or a hangup removes the temporary file, and a
-  command started with `nohup` still ignores a hangup. A device or a pipe given
-  as `--output`, such as `/dev/null`, is still written to directly.
-- 2026-10-14: A wrong passphrase, from `SB_UNLOCK_PASSPHRASE` or typed at the
-  prompt, gives the error `wrong passphrase for unlocker <id>`
-  (https://git.eeqj.de/sneak/secret/issues/157). It used to be age's "no
-  identity matched any of the recipients", followed by advice to give the vault
-  a new unlocker from its mnemonic, which would have replaced an unlocker that
-  works. The error now wraps `secret.ErrWrongPassphrase` and carries no advice
-  to use the mnemonic. A `priv.age` that is not an age file still gets that
-  advice.
-- 2026-10-14: The passphrase and mnemonic prompts ask on the controlling
-  terminal (`/dev/tty`) instead of stdin, so `echo v | secret add s1 --force`,
-  `cat file | secret encrypt key` and `cat file.age | secret decrypt key` ask
-  for the passphrase at a terminal instead of failing
-  (https://git.eeqj.de/sneak/secret/issues/144). Only a process with no
-  controlling terminal gets the error that names the variable to set. On SIGINT
-  or SIGTERM, `Entry()` restores the settings of that terminal, not of stdin,
-  since that is where the prompt turns echo off. `secret init` and
-  `secret vault create` no longer write a newline to stderr after the mnemonic
-  is typed, since the prompt ends that line on the terminal.
-- 2026-10-14: `secret generate mnemonic`, `secret vault list` and
-  `secret unlocker list` print their output to stdout, as `secret list` does, so
-  it can be piped or captured (https://git.eeqj.de/sneak/secret/issues/138).
-  They printed with cobra's `cmd.Println` and `cmd.Printf`, which write to
-  stderr when no output writer is set, so `secret vault list --json | jq`
-  received nothing. "No versions found" from `secret version list` moved to
-  stdout too. Messages such as "Created vault", and the warning after the
-  mnemonic, stay on stderr.
-- 2026-10-14: Removing the current unlocker no longer leaves the vault unable to
-  open through the unlockers it still has
-  (https://git.eeqj.de/sneak/secret/issues/137). `current-unlocker` went on
-  naming the removed unlocker, so every command that opened the vault failed,
-  with advice to use the mnemonic. `secret unlocker remove` of the current
-  unlocker now makes the first of the vault's other unlockers that
-  `secret unlocker select` accepts, in the order `secret unlocker list` shows,
-  current before removing it, and says so; when there is none, it also removes
-  `current-unlocker`. `vault.RemoveUnlocker` returns the ID of the unlocker it
-  made current. When `current-unlocker` names an unlocker that does not exist
-  and the vault has another that `secret unlocker select` accepts, the error
-  wraps `vault.ErrUnlockerNotFound` and says to run `secret unlocker select`,
-  without the advice to use the mnemonic.
+  output file has mode 0600. Ctrl-C or SIGTERM removes the temporary file. A
+  device or a pipe given as `--output`, such as `/dev/null`, is still written to
+  directly.
 - 2026-10-14: A `secret add --force` that cannot unlock the vault, given a wrong
   passphrase or mnemonic, leaves the secret as it was
   (https://git.eeqj.de/sneak/secret/issues/136). The new version used to be

internal/cli/create_vault_test.go

@@ -8,7 +8,6 @@ import (
 	"os/exec"
 	"slices"
 	"strings"
-	"syscall"
 	"testing"
 
 	"github.com/awnumar/memguard"
@@ -198,10 +197,10 @@ func TestStopAtPassphrasePromptLeavesNothing(t *testing.T) {
 
 // TestMnemonicNotReadNamesOnlyMnemonic is a regression test for
 // https://git.eeqj.de/sneak/secret/issues/115: `secret init` without
-// SB_SECRET_MNEMONIC and with no terminal said "failed to read mnemonic:
-// failed to read passphrase: ...". The error must wrap
+// SB_SECRET_MNEMONIC and with a stdin that is not a terminal said "failed to
+// read mnemonic: failed to read passphrase: ...". The error must wrap
 // secret.ErrMnemonicNotRead and name the mnemonic only. The message is
-// pinned on the built binary, run in a new session, which has no terminal.
+// pinned on the built binary, whose stdin is surely not a terminal.
 func TestMnemonicNotReadNamesOnlyMnemonic(t *testing.T) {
 	t.Parallel()
 
@@ -217,15 +216,14 @@ func TestMnemonicNotReadNamesOnlyMnemonic(t *testing.T) {
 		"PATH=" + os.Getenv("PATH"),
 		"HOME=" + os.Getenv("HOME"),
 	}
-	cmd.SysProcAttr = &syscall.SysProcAttr{Setsid: true}
 
 	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 "+
-		"interactively\n", string(output))
+		"Error: failed to read mnemonic: stdin is not a terminal (piped input "+
+		"or script). Please set the SB_SECRET_MNEMONIC environment variable "+
+		"or run interactively\n", string(output))
 }
 
 // TestStopDuringCreateLeavesWholeVaultOrNone is a regression test for

internal/cli/crypto.go

@@ -296,8 +296,6 @@ var temporaryOutput atomic.Pointer[string]
 // Otherwise write fills a new file of mode 0600 that replaces outputFile,
 // or the file it is a symbolic link to, only once write has succeeded, so a
 // failure leaves that file as it was, and it may be the input file too.
-// An existing outputFile that cannot be written fails before write is
-// called.
 func (cli *Instance) writeOutput(
 	outputFile string, write func(output io.Writer) error,
 ) error {
@@ -309,27 +307,16 @@ func (cli *Instance) writeOutput(
 	if err == nil && !info.Mode().IsRegular() {
 		file, err := cli.fs.OpenFile(outputFile, os.O_WRONLY, 0)
 		if err != nil {
-			return fmt.Errorf("failed to create output file: %w", err)
+			return fmt.Errorf("failed to open output file: %w", err)
 		}
 		defer func() { _ = file.Close() }()
 
 		return write(file)
 	}
 
-	// An existing file that cannot be written fails here, as writing into it
-	// directly would. Opened without O_TRUNC, which would empty it.
-	file, err := cli.fs.OpenFile(outputFile, os.O_WRONLY, 0)
-	if err == nil {
-		err = file.Close()
-	}
-
-	if err != nil && !errors.Is(err, os.ErrNotExist) {
-		return fmt.Errorf("failed to create output file: %w", err)
-	}
-
 	path, err := outputPath(outputFile)
 	if err != nil {
-		return fmt.Errorf("failed to create output file: %w", err)
+		return fmt.Errorf("failed to open output file: %w", err)
 	}
 
 	err = secret.WriteFileAtomicFunc(cli.fs, path, func(file afero.File) error {
@@ -345,12 +332,11 @@ func (cli *Instance) writeOutput(
 }
 
 // outputPath returns the path of the file that writing to outputFile
-// replaces, with no symbolic link in it, so that filepath.Dir of it is the
-// directory that file is in. When outputFile is a symbolic link, as
-// /dev/stdout is when stdout goes to a file, that is the path the link
-// points to, whether or not a file is there yet, so the link is kept, as
-// writing through it would keep it. A link that cannot be followed, such as
-// one that points to itself, is an error.
+// replaces. When outputFile is a symbolic link, as /dev/stdout is when
+// stdout goes to a file, that is the path the link points to, whether or
+// not a file is there yet, so the link is kept, as writing through it would
+// keep it. A link that cannot be followed, such as one that points to
+// itself, is an error.
 func outputPath(outputFile string) (string, error) {
 	path, err := filepath.EvalSymlinks(outputFile)
 	if !errors.Is(err, os.ErrNotExist) {
@@ -360,17 +346,7 @@ func outputPath(outputFile string) (string, error) {
 	// No file is at outputFile, or at the end of its links, yet
 	target, err := os.Readlink(outputFile)
 	if errors.Is(err, os.ErrNotExist) {
-		// Nothing is at outputFile itself. Its directory part is resolved
-		// with filepath.EvalSymlinks, since filepath.Dir would remove a link
-		// to a directory followed by ".." as text and give another directory.
-		dir, name := filepath.Split(outputFile)
-
-		dir, err = filepath.EvalSymlinks(dir)
-		if err != nil {
-			return "", err
-		}
-
-		return filepath.Join(dir, name), nil
+		return outputFile, nil
 	}
 
 	if err != nil {

internal/cli/crypto_test.go

@@ -99,44 +99,6 @@ func TestFailedCryptoLeavesOutputUnchanged(t *testing.T) {
 	}
 }
 
-// writeFailFs fails every open of one path for writing, as a file of mode
-// 0444 does for anyone but root, who runs these tests in Docker.
-type writeFailFs struct {
-	afero.Fs
-
-	path string
-}
-
-//nolint:ireturn // afero.File is the interface required by afero.Fs
-func (f *writeFailFs) OpenFile(
-	name string, flag int, perm os.FileMode,
-) (afero.File, error) {
-	if name == f.path && flag&(os.O_WRONLY|os.O_RDWR) != 0 {
-		return nil, &os.PathError{Op: "open", Path: name, Err: syscall.EACCES}
-	}
-
-	return f.Fs.OpenFile(name, flag, perm)
-}
-
-// TestDecryptToUnwritableOutput checks that secret decrypt fails when
-// --output is a file that cannot be written, as writing into it directly
-// does, and leaves it as it was, although its directory can be written.
-func TestDecryptToUnwritableOutput(t *testing.T) {
-	t.Parallel()
-
-	cli, dir := newCryptoTest(t)
-	existing := filepath.Join(dir, "existing.txt")
-	require.NoError(t, afero.WriteFile(cli.fs, existing, []byte("precious"), 0o600))
-	cli.fs = &writeFailFs{Fs: cli.fs, path: existing}
-
-	err := cli.Decrypt("key", filepath.Join(dir, "plain.txt.age"), existing)
-	require.ErrorIs(t, err, os.ErrPermission)
-
-	content, err := afero.ReadFile(cli.fs, existing)
-	require.NoError(t, err)
-	require.Equal(t, "precious", string(content))
-}
-
 // TestCryptoInPlace checks that secret encrypt and secret decrypt given
 // one file as both --input and --output replace it with the right result.
 // Before https://git.eeqj.de/sneak/secret/issues/143 they emptied the file
@@ -243,31 +205,6 @@ func TestDecryptThroughLinkToMissingFile(t *testing.T) {
 	require.Len(t, entries, 1)
 }
 
-// TestDecryptThroughLinkAndParent checks that secret decrypt makes the file
-// --output names in the directory its path leads to when the path holds a
-// symbolic link to a directory followed by "..": dir/link/../sub/new.txt is
-// targetDir/sub/new.txt, and dir/sub, where removing "link/.." as text
-// leads, does not exist.
-func TestDecryptThroughLinkAndParent(t *testing.T) {
-	t.Parallel()
-
-	cli, dir := newCryptoTest(t)
-	targetDir := t.TempDir()
-	require.NoError(t, os.Mkdir(filepath.Join(targetDir, "linked"), 0o700))
-	require.NoError(t, os.Mkdir(filepath.Join(targetDir, "sub"), 0o700))
-	require.NoError(t, os.Symlink(filepath.Join(targetDir, "linked"),
-		filepath.Join(dir, "link")))
-
-	// Not filepath.Join, which would remove "link/.." too
-	output := dir + "/link/../sub/new.txt"
-
-	require.NoError(t, cli.Decrypt("key", filepath.Join(dir, "plain.txt.age"), output))
-
-	content, err := afero.ReadFile(cli.fs, filepath.Join(targetDir, "sub", "new.txt"))
-	require.NoError(t, err)
-	require.Equal(t, plainText, string(content))
-}
-
 // TestDecryptThroughLoopingLink checks that secret decrypt given a symbolic
 // link that points to itself as --output fails, as writing through it
 // would, and leaves the link as it was.

internal/cli/entry_test.go

@@ -7,7 +7,6 @@ import (
 	"os/exec"
 	"path/filepath"
 	"strings"
-	"syscall"
 	"testing"
 	"time"
 
@@ -108,9 +107,9 @@ func TestInterruptExitsThroughMemguard(t *testing.T) {
 	assert.Equal(t, 1, exitErr.ExitCode(), "add ended with %v", err)
 }
 
-// Ctrl-C or a hangup while encrypt or decrypt waits for input must remove
-// the temporary file it is writing next to --output, which for decrypt
-// holds part of the secret.
+// Ctrl-C while encrypt or decrypt waits for input must remove the temporary
+// file it is writing next to --output, which for decrypt holds part of the
+// secret.
 func TestInterruptRemovesTemporaryOutput(t *testing.T) {
 	t.Parallel()
 
@@ -122,7 +121,7 @@ func TestInterruptRemovesTemporaryOutput(t *testing.T) {
 		secret.EnvStateDir + "=" + t.TempDir(),
 		secret.EnvMnemonic + "=" + testMnemonic,
 		secret.EnvUnlockPassphrase + "=test-passphrase",
-		"PATH=" + os.Getenv("PATH"),
+		"PATH=/usr/bin:/bin",
 	}
 
 	//nolint:gosec // G204: test executes the freshly built secret binary
@@ -134,88 +133,32 @@ func TestInterruptRemovesTemporaryOutput(t *testing.T) {
 
 	// encrypt stores the new key before it starts writing, so decrypt has it
 	for _, command := range []string{"encrypt", "decrypt"} {
-		for _, sig := range []os.Signal{os.Interrupt, syscall.SIGHUP} {
-			outputDir := t.TempDir()
+		outputDir := t.TempDir()
 
-			//nolint:gosec // G204: test executes the freshly built secret binary
-			cmd := exec.CommandContext(ctx, secretPath, command, "key",
-				"--output", filepath.Join(outputDir, "out"))
-			cmd.Env = env
+		//nolint:gosec // G204: test executes the freshly built secret binary
+		cmd := exec.CommandContext(ctx, secretPath, command, "key",
+			"--output", filepath.Join(outputDir, "out"))
+		cmd.Env = env
 
-			// Held open and never written, so the command keeps waiting for
-			// input. Wait closes it.
-			_, err := cmd.StdinPipe()
-			require.NoError(t, err)
-			require.NoError(t, cmd.Start())
-
-			require.Eventually(t, func() bool {
-				entries, err := os.ReadDir(outputDir)
-
-				return err == nil && len(entries) > 0
-			}, commandWait, 10*time.Millisecond, "%s made no temporary file", command)
-			require.NoError(t, cmd.Process.Signal(sig))
-
-			var exitErr *exec.ExitError
-
-			require.ErrorAs(t, cmd.Wait(), &exitErr)
+		// Held open and never written, so the command keeps waiting for
+		// input. Wait closes it.
+		_, err := cmd.StdinPipe()
+		require.NoError(t, err)
+		require.NoError(t, cmd.Start())
 
+		require.Eventually(t, func() bool {
 			entries, err := os.ReadDir(outputDir)
-			require.NoError(t, err)
-			assert.Empty(t, entries, "%s left its temporary file on %v", command, sig)
-		}
-	}
-}
 
-// encrypt started with nohup must go on ignoring a hangup, which it catches
-// otherwise, and finish writing --output.
-func TestNohupIgnoresHangup(t *testing.T) {
-	t.Parallel()
+			return err == nil && len(entries) > 0
+		}, commandWait, 10*time.Millisecond, "%s made no temporary file", command)
+		require.NoError(t, cmd.Process.Signal(os.Interrupt))
 
-	ctx, cancel := context.WithTimeout(t.Context(), commandWait)
-	defer cancel()
+		var exitErr *exec.ExitError
 
-	secretPath := secretBinaryPath(t)
-	env := []string{
-		secret.EnvStateDir + "=" + t.TempDir(),
-		secret.EnvMnemonic + "=" + testMnemonic,
-		secret.EnvUnlockPassphrase + "=test-passphrase",
-		"PATH=" + os.Getenv("PATH"),
-	}
+		require.ErrorAs(t, cmd.Wait(), &exitErr)
 
-	//nolint:gosec // G204: test executes the freshly built secret binary
-	initCmd := exec.CommandContext(ctx, secretPath, "init")
-	initCmd.Env = env
-
-	output, err := initCmd.CombinedOutput()
-	require.NoError(t, err, "init should succeed: %s", output)
-
-	outputDir := t.TempDir()
-	outputFile := filepath.Join(outputDir, "out")
-
-	//nolint:gosec // G204: test executes the freshly built secret binary
-	cmd := exec.CommandContext(ctx, "nohup", secretPath, "encrypt", "key",
-		"--output", outputFile)
-	cmd.Env = env
-
-	stdin, err := cmd.StdinPipe()
-	require.NoError(t, err)
-	require.NoError(t, cmd.Start())
-
-	require.Eventually(t, func() bool {
 		entries, err := os.ReadDir(outputDir)
-
-		return err == nil && len(entries) > 0
-	}, commandWait, 10*time.Millisecond, "encrypt made no temporary file")
-	require.NoError(t, cmd.Process.Signal(syscall.SIGHUP))
-
-	// Time for a hangup that is caught to end encrypt before it has its input
-	time.Sleep(500 * time.Millisecond)
-
-	_, err = stdin.Write([]byte("document"))
-	require.NoError(t, err)
-	require.NoError(t, stdin.Close())
-	require.NoError(t, cmd.Wait())
-
-	_, err = os.Stat(outputFile)
-	require.NoError(t, err)
+		require.NoError(t, err)
+		assert.Empty(t, entries, "%s left its temporary file", command)
+	}
 }

internal/cli/generate.go

@@ -103,7 +103,7 @@ func (cli *Instance) GenerateMnemonic(cmd *cobra.Command) error {
 	}
 
 	// Output mnemonic to stdout
-	_, _ = fmt.Fprintln(cmd.OutOrStdout(), mnemonic)
+	cmd.Println(mnemonic)
 
 	// Output helpful information to stderr
 	fmt.Fprintln(os.Stderr, "")

internal/cli/init.go

@@ -5,6 +5,7 @@ import (
 	"fmt"
 	"log"
 	"log/slog"
+	"os"
 	"strings"
 
 	"github.com/awnumar/memguard"
@@ -56,11 +57,13 @@ func (cli *Instance) promptMnemonic() (*memguard.LockedBuffer, func(), error) {
 	// Read mnemonic securely without echo
 	mnemonicBuffer, err := secret.ReadMnemonic("Enter your BIP39 mnemonic phrase: ")
 	if err != nil {
-		secret.Debug("Failed to read mnemonic", "error", err)
+		secret.Debug("Failed to read mnemonic from stdin", "error", err)
 
 		return nil, nil, err
 	}
 
+	fmt.Fprintln(os.Stderr) // Add newline after hidden input
+
 	return mnemonicBuffer, mnemonicBuffer.Destroy, nil
 }
 

internal/cli/leftovers_test.go

@@ -52,10 +52,10 @@ func TestLeftoversRemovedByNextChangingCommand(t *testing.T) {
 	}
 
 	for _, file := range []string{
-		testStateDir + "/.tmp-4",
-		vaultDir + "/.tmp-5",
-		secretDir + "/.tmp-6",
-		secretDir + "/versions/" + versions[0] + "/.tmp-7",
+		testStateDir + "/.currentvault.tmp-4",
+		vaultDir + "/.current-unlocker.tmp-5",
+		secretDir + "/.current.tmp-6",
+		secretDir + "/versions/" + versions[0] + "/.metadata.age.tmp-7",
 	} {
 		require.NoError(t, afero.WriteFile(fs, file,
 			[]byte("partial"), secret.FilePerms))

internal/cli/passphrase_prompt_test.go

@@ -1,204 +0,0 @@
-package cli_test
-
-import (
-	"bufio"
-	"bytes"
-	"context"
-	"io"
-	"os"
-	"os/exec"
-	"strings"
-	"syscall"
-	"testing"
-
-	"filippo.io/age"
-	"github.com/awnumar/memguard"
-	"github.com/creack/pty"
-	"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"
-)
-
-// TestPassphraseAskedAtTerminalWithPipedStdin is a regression test for
-// https://git.eeqj.de/sneak/secret/issues/144: run at a terminal with stdin
-// a pipe, as in `echo v2 | secret add s1 --force`, a command that needed
-// the passphrase failed instead of asking for it. Each command here must ask
-// on the terminal and succeed once the passphrase is typed there.
-func TestPassphraseAskedAtTerminalWithPipedStdin(t *testing.T) {
-	t.Parallel()
-
-	stateDir := t.TempDir()
-
-	passphrase := memguard.NewBufferFromBytes([]byte(testPassphrase))
-	defer passphrase.Destroy()
-
-	vlt, 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))
-
-	identity, err := age.GenerateX25519Identity()
-	require.NoError(t, err)
-
-	key := secret.IdentityToLockedBuffer(identity)
-	defer key.Destroy()
-
-	require.NoError(t, vlt.AddSecret("mykey", key, false))
-
-	runAtTerminal(t, stateDir, "v2\n", "add", "s1", "--force")
-
-	vlt.UnlockPassphrase = passphrase
-
-	added, err := vlt.GetSecret("s1")
-	require.NoError(t, err)
-
-	defer added.Destroy()
-
-	assert.Equal(t, "v2", added.String())
-
-	encrypted := runAtTerminal(t, stateDir, "a document\n", "encrypt", "mykey")
-	decrypted := runAtTerminal(t, stateDir, encrypted, "decrypt", "mykey")
-	assert.Equal(t, "a document\n", decrypted)
-}
-
-// TestInitWritesNoBlankLineAfterMnemonic runs `secret init 2>log` at a
-// terminal: the terminal is its controlling terminal and none of stdin,
-// stdout and stderr. The mnemonic is typed on the terminal and the prompt
-// ends its line there, so stderr must not get a newline for it as well, a
-// blank line in the log. pty.Open is described above
-// TestRemoveIgnoresTerminalOnStdout.
-func TestInitWritesNoBlankLineAfterMnemonic(t *testing.T) {
-	t.Parallel()
-
-	ctx, cancel := context.WithTimeout(t.Context(), commandWait)
-	defer cancel()
-
-	stateDir := t.TempDir()
-
-	//nolint:gosec // G204: test executes the freshly built secret binary
-	cmd := exec.CommandContext(ctx, secretBinaryPath(t), "init")
-	cmd.Env = []string{
-		secret.EnvStateDir + "=" + stateDir,
-		secret.EnvUnlockPassphrase + "=" + testPassphrase,
-		"PATH=" + os.Getenv("PATH"),
-		"HOME=" + os.Getenv("HOME"),
-	}
-
-	ptmx, tty, err := pty.Open()
-	require.NoError(t, err)
-
-	defer func() { _ = ptmx.Close() }()
-
-	deadline, _ := ctx.Deadline()
-	require.NoError(t, ptmx.SetReadDeadline(deadline))
-
-	var stderr strings.Builder
-
-	cmd.Stderr = &stderr
-	cmd.ExtraFiles = []*os.File{tty}
-	// Ctty is a descriptor in the new process: 3, the first of ExtraFiles.
-	cmd.SysProcAttr = &syscall.SysProcAttr{Setsid: true, Setctty: true, Ctty: 3}
-
-	require.NoError(t, cmd.Start())
-
-	_ = tty.Close()
-
-	var (
-		shown []byte
-		char  byte
-	)
-
-	terminal := bufio.NewReader(ptmx)
-	for !bytes.HasSuffix(shown, []byte("Enter your BIP39 mnemonic phrase: ")) {
-		char, err = terminal.ReadByte()
-		require.NoError(t, err, "secret init did not ask on the terminal: %s",
-			shown)
-
-		shown = append(shown, char)
-	}
-
-	_, err = ptmx.WriteString(testMnemonic + "\n")
-	require.NoError(t, err)
-
-	// The read ends once the command has exited and so closed the terminal.
-	rest, _ := io.ReadAll(terminal)
-
-	err = cmd.Wait()
-	require.NoError(t, err, "secret init: %s%s%s", shown, rest, stderr.String())
-
-	assert.True(t, strings.HasPrefix(stderr.String(),
-		"Initialized secrets manager at: "+stateDir+"\n\n"+
-			"Default vault created and configured\n"),
-		"stderr: %q", stderr.String())
-}
-
-// runAtTerminal runs the secret binary with args and input on stdin, in a
-// new session whose controlling terminal is a new terminal, which is also
-// its stderr: a command typed at a terminal with its input piped. pty.Open
-// is described above TestRemoveIgnoresTerminalOnStdout. runAtTerminal waits
-// for the passphrase prompt on the terminal, types the passphrase there,
-// and returns what the command wrote to stdout once it has succeeded.
-func runAtTerminal(t *testing.T, stateDir, input 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.Env = []string{
-		secret.EnvStateDir + "=" + stateDir,
-		"PATH=" + os.Getenv("PATH"),
-		"HOME=" + os.Getenv("HOME"),
-	}
-
-	ptmx, tty, err := pty.Open()
-	require.NoError(t, err)
-
-	defer func() { _ = ptmx.Close() }()
-
-	deadline, _ := ctx.Deadline()
-	require.NoError(t, ptmx.SetReadDeadline(deadline))
-
-	var stdout strings.Builder
-
-	cmd.Stdin = strings.NewReader(input)
-	cmd.Stdout = &stdout
-	cmd.Stderr = tty
-	// Ctty is a descriptor in the new process: 2, its stderr.
-	cmd.SysProcAttr = &syscall.SysProcAttr{Setsid: true, Setctty: true, Ctty: 2}
-
-	require.NoError(t, cmd.Start())
-
-	_ = tty.Close()
-
-	var (
-		shown []byte
-		char  byte
-	)
-
-	terminal := bufio.NewReader(ptmx)
-	for !bytes.HasSuffix(shown, []byte("Enter unlock passphrase: ")) {
-		char, err = terminal.ReadByte()
-		require.NoError(t, err, "secret %v did not ask on the terminal: %s",
-			args, shown)
-
-		shown = append(shown, char)
-	}
-
-	_, err = ptmx.WriteString(testPassphrase + "\n")
-	require.NoError(t, err)
-
-	// The read ends once the command has exited and so closed the terminal.
-	rest, _ := io.ReadAll(terminal)
-
-	require.NoError(t, cmd.Wait(), "secret %v: %s%s", args, shown, rest)
-
-	return stdout.String()
-}

internal/cli/root.go

@@ -2,7 +2,6 @@ package cli
 
 import (
 	"os"
-	"os/signal"
 
 	"github.com/awnumar/memguard"
 	"github.com/spf13/cobra"
@@ -15,46 +14,26 @@ import (
 // every memguard buffer before it returns, so the caller must do nothing
 // but exit with the code.
 func Entry() int {
-	// On SIGINT, SIGTERM or SIGHUP memguard runs this function, wipes every
-	// buffer and exits with status 1. The function removes the temporary
-	// file that encrypt or decrypt is writing. The passphrase prompt turns
-	// echo off on the controlling terminal until the read finishes, so a
-	// signal there would leave echo off. Only a process in the terminal's
-	// foreground process group may reset it: one in the background that
-	// tries is stopped instead of exiting.
-	terminal, terminalErr := os.Open(secret.TerminalPath)
-
-	var terminalState *term.State
-
-	if terminalErr == nil {
-		defer func() { _ = terminal.Close() }()
-
-		terminalState, terminalErr = term.GetState(int(terminal.Fd()))
-	}
-
-	signals := []os.Signal{os.Interrupt, unix.SIGTERM}
-
-	// A command started with nohup must go on ignoring SIGHUP, which
-	// catching it would undo
-	if !signal.Ignored(unix.SIGHUP) {
-		signals = append(signals, unix.SIGHUP)
-	}
+	// On SIGINT or SIGTERM memguard runs this function, wipes every buffer
+	// and exits with status 1. The passphrase prompt turns terminal echo
+	// off until the read finishes, so a signal there would leave echo off.
+	// Only a process in the terminal's foreground process group may reset
+	// it: one in the background that tries is stopped instead of exiting.
+	// The function also removes the temporary file that encrypt or decrypt
+	// is writing.
+	terminalState, terminalErr := term.GetState(unix.Stdin)
 
 	memguard.CatchSignal(func(os.Signal) {
+		foreground, err := unix.IoctlGetInt(unix.Stdin, unix.TIOCGPGRP)
+		if terminalErr == nil && err == nil && foreground == unix.Getpgrp() {
+			_ = term.Restore(unix.Stdin, terminalState)
+		}
+
 		name := temporaryOutput.Load()
 		if name != nil {
 			_ = os.Remove(*name)
 		}
-
-		if terminalErr != nil {
-			return
-		}
-
-		foreground, err := unix.IoctlGetInt(int(terminal.Fd()), unix.TIOCGPGRP)
-		if err == nil && foreground == unix.Getpgrp() {
-			_ = term.Restore(int(terminal.Fd()), terminalState)
-		}
-	}, signals...)
+	}, os.Interrupt, unix.SIGTERM)
 
 	defer memguard.Purge()
 

internal/cli/stdout_stderr_test.go

@@ -2,7 +2,6 @@ package cli_test
 
 import (
 	"bytes"
-	"encoding/json"
 	"os/exec"
 	"path/filepath"
 	"strings"
@@ -10,7 +9,6 @@ import (
 
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
-	"sneak.berlin/go/secret/internal/bip39"
 	"sneak.berlin/go/secret/internal/secret"
 )
 
@@ -92,66 +90,3 @@ func TestGetCommandOutputsToStdout(t *testing.T) {
 	// Nothing should be in stderr
 	assert.Empty(t, stderr.String(), "stderr should be empty")
 }
-
-// TestMnemonicAndListingsOutputToStdout tests that 'secret generate
-// mnemonic', 'secret vault list' and 'secret unlocker list' print their
-// output to stdout, so that it can be piped
-func TestMnemonicAndListingsOutputToStdout(t *testing.T) {
-	t.Parallel()
-
-	tempDir := t.TempDir()
-	secretPath := secretBinaryPath(t)
-
-	// run runs the secret binary and returns what it wrote to stdout and to
-	// stderr
-	run := func(args ...string) (string, string) {
-		//nolint:gosec // G204: test executes the freshly built secret binary
-		cmd := exec.CommandContext(t.Context(), secretPath, args...)
-		cmd.Env = []string{
-			secret.EnvStateDir + "=" + tempDir,
-			secret.EnvMnemonic + "=" + testMnemonic,
-			secret.EnvUnlockPassphrase + "=test-passphrase",
-			"PATH=/usr/bin:/bin",
-		}
-
-		var stdout, stderr bytes.Buffer
-
-		cmd.Stdout = &stdout
-		cmd.Stderr = &stderr
-
-		err := cmd.Run()
-		require.NoError(t, err, "%v should succeed: %s", args, stderr.String())
-
-		return stdout.String(), stderr.String()
-	}
-
-	stdout, stderr := run("generate", "mnemonic")
-	assert.True(t, bip39.IsMnemonicValid(strings.TrimSpace(stdout)),
-		"stdout should hold the mnemonic, not %q", stdout)
-	assert.Contains(t, stderr, "Save this mnemonic phrase securely",
-		"the warning should be on stderr")
-
-	run("init")
-
-	stdout, _ = run("vault", "list", "--json")
-
-	var vaults map[string]any
-
-	require.NoError(t, json.Unmarshal([]byte(stdout), &vaults),
-		"stdout should hold the vaults as JSON, not %q", stdout)
-	assert.Equal(t, "default", vaults["currentVault"])
-
-	stdout, _ = run("unlocker", "list", "--json")
-
-	var unlockers map[string]any
-
-	require.NoError(t, json.Unmarshal([]byte(stdout), &unlockers),
-		"stdout should hold the unlockers as JSON, not %q", stdout)
-	assert.Len(t, unlockers["unlockers"], 1)
-
-	stdout, _ = run("vault", "list")
-	assert.Contains(t, stdout, "default (current)")
-
-	stdout, _ = run("unlocker", "list")
-	assert.Contains(t, stdout, "Total: 1 unlocker(s)")
-}

internal/cli/unlock_failure_test.go

@@ -1,14 +1,12 @@
 // Unlock Failure Tests
 //
 // When a vault cannot be opened through its current unlocker, because a
-// file the unlocker needs is missing, the error keeps its cause and ends by
-// saying that the mnemonic still opens that vault, but only for a vault that
-// the mnemonic does open, and not when the passphrase could not be read at
-// all. A wrong passphrase gives an error that says so, without that advice.
-// When the current unlocker does not exist and the vault has another that
-// `secret unlocker select` accepts, the error says to select one instead.
-// When a secret's current file is missing, the error says how to make a
-// version current again. Each test that pins such advice also follows it.
+// file the unlocker needs is missing or the passphrase is wrong, the error
+// keeps its cause and ends by saying that the mnemonic still opens that
+// vault, but only for a vault that the mnemonic does open, and not when the
+// passphrase could not be read at all. When a secret's current file is
+// missing, the error says how to make a version current again. Each test
+// that pins such advice also follows it.
 
 package cli_test
 
@@ -18,9 +16,9 @@ import (
 	"os"
 	"os/exec"
 	"path/filepath"
-	"syscall"
 	"testing"
 
+	"filippo.io/age"
 	"github.com/awnumar/memguard"
 	"github.com/spf13/afero"
 	"github.com/spf13/cobra"
@@ -164,48 +162,26 @@ func TestUnlockFailureNamesMnemonic(t *testing.T) {
 	}
 }
 
-// TestWrongPassphraseNamesPassphrase checks the error of `secret get` given
-// a passphrase that does not decrypt the passphrase unlocker: it says that
-// the passphrase is wrong, without the advice to use the mnemonic, since the
-// unlocker still opens the vault with the right passphrase. The test then
-// gives the right passphrase.
-func TestWrongPassphraseNamesPassphrase(t *testing.T) {
+// TestWrongPassphraseNamesMnemonic checks the error of `secret get` given a
+// passphrase that does not decrypt the passphrase unlocker: it keeps age's
+// error and ends with the advice that the mnemonic still opens the vault.
+func TestWrongPassphraseNamesMnemonic(t *testing.T) {
 	t.Parallel()
 
-	unlockerDir, c := newUnlockTestCLI(t, newTwoVaultFs(t))
+	_, c := newUnlockTestCLI(t, newTwoVaultFs(t))
 	c.UnlockPassphrase = memguard.NewBufferFromBytes([]byte("wrong passphrase"))
 	t.Cleanup(c.UnlockPassphrase.Destroy)
 
 	err := c.GetSecret(discardCmd(), "x")
 
-	require.ErrorIs(t, err, secret.ErrWrongPassphrase)
-	assert.NotContains(t, err.Error(), mnemonicAdvice)
+	var noMatch *age.NoIdentityMatchError
+
+	require.ErrorAs(t, err, &noMatch)
+
 	require.EqualError(t, err, "failed to unlock vault: "+
 		"failed to get long-term key: failed to get unlocker identity: "+
-		"wrong passphrase for unlocker "+filepath.Base(unlockerDir))
-
-	c.UnlockPassphrase = memguard.NewBufferFromBytes([]byte(testPassphrase))
-	t.Cleanup(c.UnlockPassphrase.Destroy)
-	assert.Equal(t, "value", getSecret(t, c, "x"))
-}
-
-// TestDamagedUnlockerNamesMnemonic checks the error of `secret get` when the
-// passphrase unlocker's priv.age is not an age file: the passphrase is not
-// blamed, and the error ends with the advice that the mnemonic still opens
-// the vault.
-func TestDamagedUnlockerNamesMnemonic(t *testing.T) {
-	t.Parallel()
-
-	fs := newTwoVaultFs(t)
-	unlockerDir, c := newUnlockTestCLI(t, fs)
-
-	require.NoError(t, afero.WriteFile(fs, filepath.Join(unlockerDir, "priv.age"),
-		[]byte("damaged"), secret.FilePerms))
-
-	err := c.GetSecret(discardCmd(), "x")
-
-	require.NotErrorIs(t, err, secret.ErrWrongPassphrase)
-	require.ErrorContains(t, err, mnemonicAdvice)
+		"failed to decrypt unlocker private key: failed to create decryptor: "+
+		noMatch.Error()+mnemonicAdvice)
 }
 
 // TestMoveUnlockFailureNamesVault checks the error of `secret move` into
@@ -246,71 +222,8 @@ func TestMoveUnlockFailureNamesVault(t *testing.T) {
 	assert.Equal(t, "value", getSecret(t, c, "y"))
 }
 
-// TestMissingCurrentUnlockerNamesSelect checks the error of `secret get`
-// when current-unlocker names an unlocker that does not exist and the vault
-// has another: it names the missing unlocker and says to select another
-// one, without the advice to use the mnemonic. The test then follows that
-// advice.
-func TestMissingCurrentUnlockerNamesSelect(t *testing.T) {
-	t.Parallel()
-
-	fs := newTwoVaultFs(t)
-	unlockerDir, c := newUnlockTestCLI(t, fs)
-
-	require.NoError(t, afero.WriteFile(fs,
-		filepath.Join(unlockTestVaultDir, "current-unlocker"),
-		[]byte("missing"), secret.FilePerms))
-
-	err := c.GetSecret(discardCmd(), "x")
-
-	require.ErrorIs(t, err, vault.ErrUnlockerNotFound)
-	require.EqualError(t, err, "failed to unlock vault: "+
-		"failed to get long-term key: failed to get current unlocker: "+
-		"unlocker with ID missing not found; the vault 'default' has other "+
-		"unlockers: run 'secret unlocker select' with the ID of one of them "+
-		"to make it the current unlocker")
-
-	require.NoError(t, c.UnlockerSelect(filepath.Base(unlockerDir)))
-	assert.Equal(t, "value", getSecret(t, c, "x"))
-}
-
-// TestMissingCurrentUnlockerNamesMnemonic checks the error of `secret get`
-// when the unlocker that current-unlocker names does not exist and the
-// vault's only other unlocker is the directory "odd", whose metadata names a
-// type that `secret unlocker select` refuses: the error ends with the advice
-// that the mnemonic still opens the vault, not with advice to select another
-// unlocker. The test then follows that advice.
-func TestMissingCurrentUnlockerNamesMnemonic(t *testing.T) {
-	t.Parallel()
-
-	fs := newTwoVaultFs(t)
-	unlockerDir, c := newUnlockTestCLI(t, fs)
-
-	require.NoError(t, fs.RemoveAll(unlockerDir))
-	require.NoError(t, afero.WriteFile(fs,
-		filepath.Join(unlockTestVaultDir, "unlockers.d", "odd",
-			"unlocker-metadata.json"),
-		[]byte(`{"type": "odd"}`), secret.FilePerms))
-
-	err := c.GetSecret(discardCmd(), "x")
-
-	var cause *os.PathError
-
-	require.ErrorAs(t, err, &cause)
-	require.NotErrorIs(t, err, vault.ErrUnlockerNotFound)
-	require.EqualError(t, err, "failed to unlock vault: "+
-		"failed to get long-term key: failed to get current unlocker: "+
-		"failed to read unlocker metadata: "+cause.Error()+mnemonicAdvice)
-
-	c.Mnemonic = testMnemonicBuffer(t)
-	require.NoError(t, c.UnlockersAdd("passphrase", discardCmd()))
-
-	c.Mnemonic = nil
-	assert.Equal(t, "value", getSecret(t, c, "x"))
-}
-
 // TestPassphraseNotReadNamesNoMnemonic runs `secret get x` on the built
-// binary without SB_UNLOCK_PASSPHRASE and in a new session, which has no
+// binary without SB_UNLOCK_PASSPHRASE and with a stdin that is not a
 // terminal, so the passphrase cannot be read. The unlocker was not tried,
 // and adding one would need a passphrase read the same way, so the error
 // is the cause alone, without the advice to use the mnemonic.
@@ -341,16 +254,15 @@ func TestPassphraseNotReadNamesNoMnemonic(t *testing.T) {
 		"PATH=" + os.Getenv("PATH"),
 		"HOME=" + os.Getenv("HOME"),
 	}
-	cmd.SysProcAttr = &syscall.SysProcAttr{Setsid: true}
 
 	output, err := cmd.CombinedOutput()
 	require.Error(t, err)
 
 	assert.Equal(t, "Error: failed to unlock vault: "+
 		"failed to get long-term key: failed to get unlocker identity: "+
-		"failed to read passphrase: there is no terminal to ask on. Please "+
-		"set the SB_UNLOCK_PASSPHRASE environment variable or run "+
-		"interactively\n", string(output))
+		"failed to read passphrase: stdin is not a terminal (piped input or "+
+		"script). Please set the SB_UNLOCK_PASSPHRASE environment variable or "+
+		"run interactively\n", string(output))
 }
 
 // TestCryptoUnlockFailureNamesMnemonic checks that `secret encrypt` and

internal/cli/unlockers.go

@@ -222,11 +222,8 @@ func newUnlockerRemoveCmd() *cobra.Command {
 		Long: `Remove an unlocker from the current vault. Asks for ` +
 			`confirmation first, saying whether it is the vault's last ` +
 			`unlocker; when stdin is not a terminal, fails unless --force ` +
-			`is given. Removing the current unlocker makes another of the ` +
-			`vault's unlockers current, if it has one that ` +
-			`'secret unlocker select' accepts. Warning: Without ` +
-			`unlockers and without your mnemonic, vault data will be ` +
-			`permanently inaccessible.`,
+			`is given. Warning: Without unlockers and without your ` +
+			`mnemonic, vault data will be permanently inaccessible.`,
 		Args:              cobra.ExactArgs(1),
 		ValidArgsFunction: getUnlockerIDsCompletionFunc(cli.fs, cli.stateDir),
 		RunE: func(cmd *cobra.Command, args []string) error {
@@ -326,25 +323,22 @@ func (cli *Instance) printUnlockersJSON(
 		return fmt.Errorf("failed to marshal JSON: %w", err)
 	}
 
-	_, _ = fmt.Fprintln(cli.cmd.OutOrStdout(), string(jsonBytes))
+	cli.cmd.Println(string(jsonBytes))
 
 	return nil
 }
 
 // printUnlockersTable prints unlockers in a formatted table
 func (cli *Instance) printUnlockersTable(unlockers []UnlockerInfo) error {
-	out := cli.cmd.OutOrStdout()
-
 	if len(unlockers) == 0 {
-		_, _ = fmt.Fprintln(out, "No unlockers found in current vault.")
-		_, _ = fmt.Fprintln(out, "Run 'secret unlocker add passphrase' to create one.")
+		cli.cmd.Println("No unlockers found in current vault.")
+		cli.cmd.Println("Run 'secret unlocker add passphrase' to create one.")
 
 		return nil
 	}
 
-	_, _ = fmt.Fprintf(out, "  %-40s %-12s %-20s %s\n",
-		"UNLOCKER ID", "TYPE", "CREATED", "FLAGS")
-	_, _ = fmt.Fprintf(out, "  %-40s %-12s %-20s %s\n",
+	cli.cmd.Printf("  %-40s %-12s %-20s %s\n", "UNLOCKER ID", "TYPE", "CREATED", "FLAGS")
+	cli.cmd.Printf("  %-40s %-12s %-20s %s\n",
 		strings.Repeat("-", unlockerIDWidth), strings.Repeat("-", unlockerTypeWidth),
 		strings.Repeat("-", unlockerDateWidth), strings.Repeat("-", unlockerFlagsWidth))
 
@@ -359,7 +353,7 @@ func (cli *Instance) printUnlockersTable(unlockers []UnlockerInfo) error {
 			prefix = "* "
 		}
 
-		_, _ = fmt.Fprintf(out, "%s%-40s %-12s %-20s %s\n",
+		cli.cmd.Printf("%s%-40s %-12s %-20s %s\n",
 			prefix,
 			unlocker.ID,
 			unlocker.Type,
@@ -367,7 +361,7 @@ func (cli *Instance) printUnlockersTable(unlockers []UnlockerInfo) error {
 			flags)
 	}
 
-	_, _ = fmt.Fprintf(out, "\nTotal: %d unlocker(s)\n", len(unlockers))
+	cli.cmd.Printf("\nTotal: %d unlocker(s)\n", len(unlockers))
 
 	return nil
 }
@@ -618,17 +612,13 @@ func (cli *Instance) removeUnlocker(
 			"have your mnemonic phrase to access this vault again!")
 	}
 
-	newCurrentID, err := found.vlt.RemoveUnlocker(unlockerID)
+	err := found.vlt.RemoveUnlocker(unlockerID)
 	if err != nil {
 		return err
 	}
 
 	cmd.Printf("Removed unlocker '%s'\n", unlockerID)
 
-	if newCurrentID != "" {
-		cmd.Printf("Switched current unlocker to '%s'\n", newCurrentID)
-	}
-
 	return nil
 }
 

internal/cli/unlockers_remove_test.go

@@ -1,94 +0,0 @@
-//nolint:testpackage // white-box test of unexported internals
-package cli
-
-import (
-	"bytes"
-	"path/filepath"
-	"testing"
-
-	"github.com/awnumar/memguard"
-	"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"
-)
-
-// TestUnlockerRemoveCurrent is a regression test for
-// https://git.eeqj.de/sneak/secret/issues/137. A vault has a passphrase
-// unlocker and a PGP unlocker, the current one. Removing the PGP unlocker
-// makes the passphrase unlocker current and says so, and the vault then
-// opens through it. Removing the passphrase unlocker too removes
-// current-unlocker.
-//
-// The second case also gives the vault the unlocker directory "odd", whose
-// metadata names a type that `secret unlocker select` refuses, and which
-// sorts before the passphrase unlocker: neither removal makes it current.
-//
-//nolint:paralleltest // t.Setenv (GNUPGHOME) forbids parallel tests
-func TestUnlockerRemoveCurrent(t *testing.T) {
-	tests := []struct {
-		name    string
-		withOdd bool
-	}{
-		{name: "two unlockers", withOdd: false},
-		{name: "and the directory odd", withOdd: true},
-	}
-
-	for _, tt := range tests {
-		t.Run(tt.name, func(t *testing.T) {
-			newTestGPGKey(t)
-
-			passphrase := memguard.NewBufferFromBytes([]byte(testPassphrase))
-			t.Cleanup(passphrase.Destroy)
-
-			fs := afero.NewMemMapFs()
-			vlt, err := vault.CreateVault(fs, listTestStateDir,
-				listTestVaultName, testMnemonicBuffer(t), nil)
-			require.NoError(t, err)
-
-			passphraseUnlocker, err := vlt.CreatePassphraseUnlocker(passphrase)
-			require.NoError(t, err)
-
-			if tt.withOdd {
-				require.NoError(t, afero.WriteFile(fs,
-					filepath.Join(testVaultDir(listTestVaultName), "unlockers.d",
-						"odd", "unlocker-metadata.json"),
-					[]byte(`{"type": "odd"}`), secret.FilePerms))
-			}
-
-			instance, cmd := newTestInstance(fs)
-			instance.UnlockPassphrase = passphrase
-
-			cmd.Flags().String("keyid", unreadableTestGPGUserID, "")
-			require.NoError(t, instance.UnlockersAdd(unlockerTypePGP, cmd))
-
-			pgpUnlocker, err := vlt.GetCurrentUnlocker()
-			require.NoError(t, err)
-			require.Equal(t, unlockerTypePGP, pgpUnlocker.GetType())
-
-			var output bytes.Buffer
-
-			cmd.SetOut(&output)
-			require.NoError(t,
-				instance.UnlockersRemove(pgpUnlocker.GetID(), true, cmd))
-			assert.Contains(t, output.String(),
-				"Switched current unlocker to '"+passphraseUnlocker.GetID()+"'")
-
-			reopened := vault.NewVault(fs, listTestStateDir, listTestVaultName)
-			reopened.UnlockPassphrase = passphrase
-
-			_, err = reopened.GetOrDeriveLongTermKey()
-			require.NoError(t, err)
-
-			require.NoError(t,
-				instance.UnlockersRemove(passphraseUnlocker.GetID(), true, cmd))
-
-			exists, err := afero.Exists(fs, filepath.Join(
-				testVaultDir(listTestVaultName), "current-unlocker"))
-			require.NoError(t, err)
-			assert.False(t, exists,
-				"current-unlocker is left after the passphrase unlocker")
-		})
-	}
-}

internal/cli/vault.go

@@ -186,8 +186,6 @@ func (cli *Instance) ListVaults(cmd *cobra.Command, jsonOutput bool) error {
 		return err
 	}
 
-	out := cmd.OutOrStdout()
-
 	if jsonOutput { //nolint:nestif // Separate JSON and text output formatting logic
 		// Get current vault name for context
 		currentVault := ""
@@ -207,13 +205,13 @@ func (cli *Instance) ListVaults(cmd *cobra.Command, jsonOutput bool) error {
 			return err
 		}
 
-		_, _ = fmt.Fprintln(out, string(jsonBytes))
+		cmd.Println(string(jsonBytes))
 	} else {
 		// Text output
-		_, _ = fmt.Fprintln(out, "Available vaults:")
+		cmd.Println("Available vaults:")
 
 		if len(vaults) == 0 {
-			_, _ = fmt.Fprintln(out, "  (none)")
+			cmd.Println("  (none)")
 		} else {
 			// Try to get current vault for marking
 			currentVault := ""
@@ -225,9 +223,9 @@ func (cli *Instance) ListVaults(cmd *cobra.Command, jsonOutput bool) error {
 
 			for _, vaultName := range vaults {
 				if vaultName == currentVault {
-					_, _ = fmt.Fprintf(out, "  %s (current)\n", vaultName)
+					cmd.Printf("  %s (current)\n", vaultName)
 				} else {
-					_, _ = fmt.Fprintf(out, "  %s\n", vaultName)
+					cmd.Printf("  %s\n", vaultName)
 				}
 			}
 		}

internal/cli/version.go

@@ -167,7 +167,7 @@ func (cli *Instance) ListVersions(cmd *cobra.Command, secretName string) error {
 	}
 
 	if len(versions) == 0 {
-		_, _ = fmt.Fprintln(cmd.OutOrStdout(), "No versions found")
+		cmd.Println("No versions found")
 
 		return nil
 	}

internal/secret/constants.go

@@ -17,10 +17,6 @@ const (
 	// EnvGPGKeyID is the environment variable for providing the GPG key ID
 	EnvGPGKeyID = "SB_GPG_KEY_ID"
 
-	// TerminalPath opens the controlling terminal of the process that opens
-	// it, where the passphrase and mnemonic prompts ask.
-	TerminalPath = "/dev/tty"
-
 	// UnlockerTimeFormat is the layout of the time, in UTC, in the name of a
 	// new unlocker's directory. It runs to the nanosecond, so that every new
 	// unlocker, even one added right after another, gets a directory of its

internal/secret/crypto.go

@@ -6,6 +6,7 @@ import (
 	"fmt"
 	"io"
 	"os"
+	"syscall"
 	"unsafe"
 
 	"filippo.io/age"
@@ -15,8 +16,11 @@ import (
 
 var (
 	errNilPassphraseBuffer = errors.New("passphrase buffer is nil")
-	errNoTerminal          = errors.New("there is no terminal to ask on")
-	errNothingEntered      = errors.New("nothing was entered")
+	errStdinNotTerminal    = errors.New(
+		"stdin is not a terminal (piped input or script)")
+	errStderrNotTerminal = errors.New(
+		"stderr is not a terminal (running in non-interactive mode)")
+	errNothingEntered = errors.New("nothing was entered")
 )
 
 // ErrMnemonicNotRead is wrapped in every error of ReadMnemonic: there is no
@@ -189,33 +193,38 @@ func ReadMnemonic(prompt string) (*memguard.LockedBuffer, error) {
 	return readFromTerminal(prompt, ErrMnemonicNotRead, EnvMnemonic)
 }
 
-// readFromTerminal asks on the controlling terminal and reads the answer
-// there without echoing it, whatever stdin is: in
-// `echo value | secret add name --force`, stdin is a pipe. Every error it
-// returns wraps notRead; without a terminal, the error says to set envVar
-// instead.
+// readFromTerminal reads input from the terminal without echoing it. Every
+// error it returns wraps notRead; without a terminal, the error says to set
+// envVar instead.
 func readFromTerminal(
 	prompt string, notRead error, envVar string,
 ) (*memguard.LockedBuffer, error) {
-	terminal, err := os.OpenFile(TerminalPath, os.O_RDWR, 0)
-	if err != nil {
-		Debug("Failed to open the controlling terminal", "error", err)
-
+	// Check if stdin is a terminal
+	if !term.IsTerminal(syscall.Stdin) {
+		// Not a terminal - never read secrets from piped input
+		// for security reasons
 		return nil, fmt.Errorf(
 			"%w: %w. Please set the %s environment variable or run interactively",
-			notRead, errNoTerminal, envVar)
+			notRead, errStdinNotTerminal, envVar)
 	}
-	defer func() { _ = terminal.Close() }()
 
-	_, _ = fmt.Fprint(terminal, prompt)
+	// stdin is a terminal, check if stderr is also a terminal for
+	// interactive prompting
+	if !term.IsTerminal(syscall.Stderr) {
+		return nil, fmt.Errorf("%w: %w. Please set the %s environment variable",
+			notRead, errStderrNotTerminal, envVar)
+	}
 
-	input, err := term.ReadPassword(int(terminal.Fd()))
+	// Both stdin and stderr are terminals - use secure password reading
+	fmt.Fprint(os.Stderr, prompt) // Write prompt to stderr, not stdout
+
+	input, err := term.ReadPassword(syscall.Stdin)
 	if err != nil {
 		return nil, fmt.Errorf("%w: %w", notRead, err)
 	}
 
-	// The newline typed after the input, which ReadPassword did not echo
-	_, _ = fmt.Fprintln(terminal)
+	// Print newline to stderr since ReadPassword doesn't echo
+	fmt.Fprintln(os.Stderr)
 
 	if len(input) == 0 {
 		return nil, fmt.Errorf("%w: %w", notRead, errNothingEntered)

internal/secret/passphraseunlocker.go

@@ -16,10 +16,6 @@ import (
 // empty. A passphrase unlocker that fails with it was not tried.
 var ErrPassphraseNotRead = errors.New("failed to read passphrase")
 
-// ErrWrongPassphrase is wrapped in the error of GetIdentity when the
-// passphrase does not decrypt the unlocker's private key.
-var ErrWrongPassphrase = errors.New("wrong passphrase")
-
 // PassphraseUnlocker represents a passphrase-protected unlocker
 type PassphraseUnlocker struct {
 	Directory  string
@@ -77,12 +73,6 @@ func (p *PassphraseUnlocker) GetIdentity() (*age.X25519Identity, error) {
 		Debug("Failed to decrypt unlocker private key",
 			"error", err, "unlocker_id", p.GetID())
 
-		// age says that no identity matched when the passphrase is wrong
-		var noMatch *age.NoIdentityMatchError
-		if errors.As(err, &noMatch) {
-			return nil, fmt.Errorf("%w for unlocker %s", ErrWrongPassphrase, p.GetID())
-		}
-
 		return nil, fmt.Errorf("failed to decrypt unlocker private key: %w", err)
 	}
 	defer privKeyBuffer.Destroy()

internal/vault/errors_test.go

@@ -81,9 +81,7 @@ func TestVaultErrors(t *testing.T) {
 			return vlt.CopySecretAllVersions(vlt, versionless, "copy", false)
 		}, vault.ErrNoVersions},
 		{"remove a missing unlocker", func(vlt *vault.Vault) error {
-			_, err := vlt.RemoveUnlocker(missingName)
-
-			return err
+			return vlt.RemoveUnlocker(missingName)
 		}, vault.ErrUnlockerNotFound},
 		{"select a missing unlocker", func(vlt *vault.Vault) error {
 			return vlt.SelectUnlocker(missingName)

internal/vault/unlockers.go

@@ -5,10 +5,8 @@ import (
 	"errors"
 	"fmt"
 	"log/slog"
-	"maps"
 	"os"
 	"path/filepath"
-	"slices"
 	"strings"
 	"time"
 
@@ -21,7 +19,6 @@ import (
 // Unlocker metadata type strings.
 const (
 	unlockerTypePassphrase = "passphrase"
-	unlockerTypePGP        = "pgp"
 )
 
 // GetCurrentUnlocker returns the current unlocker for this vault
@@ -55,11 +52,6 @@ func (v *Vault) GetCurrentUnlocker() (secret.Unlocker, error) {
 		return nil, err
 	}
 
-	err = v.checkCurrentUnlockerExists(unlockerDir)
-	if err != nil {
-		return nil, err
-	}
-
 	secret.DebugWith("Resolved unlocker directory",
 		slog.String("unlocker_dir", unlockerDir),
 		slog.String("vault_name", v.Name),
@@ -82,7 +74,7 @@ func (v *Vault) GetCurrentUnlocker() (secret.Unlocker, error) {
 		passphraseUnlocker := secret.NewPassphraseUnlocker(v.fs, unlockerDir, metadata)
 		passphraseUnlocker.Passphrase = v.UnlockPassphrase
 		unlocker = passphraseUnlocker
-	case unlockerTypePGP:
+	case "pgp":
 		secret.Debug("Creating PGP unlocker instance", "unlocker_type", metadata.Type)
 
 		unlocker = secret.NewPGPUnlocker(v.fs, unlockerDir, metadata)
@@ -127,37 +119,6 @@ func (v *Vault) resolveUnlockerDirectory(currentUnlockerPath string) (string, er
 	return absolutePath, nil
 }
 
-// checkCurrentUnlockerExists fails with ErrUnlockerNotFound, saying to
-// select another unlocker, when unlockerDir, the unlocker directory that
-// current-unlocker names, does not exist and the vault has another unlocker
-// that SelectUnlocker accepts. When it has none, it returns nil: reading the
-// missing unlocker then fails, with the advice that the mnemonic still opens
-// the vault.
-func (v *Vault) checkCurrentUnlockerExists(unlockerDir string) error {
-	exists, err := afero.DirExists(v.fs, unlockerDir)
-	if err != nil {
-		return fmt.Errorf("failed to check for the current unlocker: %w", err)
-	}
-
-	if exists {
-		return nil
-	}
-
-	otherID, err := v.firstOtherUnlockerID(filepath.Base(unlockerDir))
-	if err != nil {
-		return err
-	}
-
-	if otherID == "" {
-		return nil
-	}
-
-	return fmt.Errorf("unlocker with ID %s %w; the vault '%s' has other "+
-		"unlockers: run %s with the ID of one of them to make it the current "+
-		"unlocker", filepath.Base(unlockerDir), ErrUnlockerNotFound, v.Name,
-		v.withVaultSelect("'secret unlocker select'"))
-}
-
 // findUnlockerByID finds an unlocker by its ID and returns the unlocker
 // 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
@@ -198,7 +159,7 @@ func (v *Vault) findUnlockerByID(
 		switch metadata.Type {
 		case unlockerTypePassphrase:
 			tempUnlocker = secret.NewPassphraseUnlocker(v.fs, unlockerDirPath, metadata)
-		case unlockerTypePGP:
+		case "pgp":
 			tempUnlocker = secret.NewPGPUnlocker(v.fs, unlockerDirPath, metadata)
 		default:
 			continue
@@ -321,15 +282,10 @@ func (v *Vault) HasUnlocker(unlockerID string) (bool, error) {
 // 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.
-//
-// It returns the ID of the unlocker it made current, or "" when it made none
-// current. If the removed unlocker is the current one, the unlocker
-// firstOtherUnlockerID returns is made current before the removal; with
-// none, current-unlocker is removed after it.
-func (v *Vault) RemoveUnlocker(unlockerID string) (string, error) {
+func (v *Vault) RemoveUnlocker(unlockerID string) error {
 	vaultDir, err := v.GetDirectory()
 	if err != nil {
-		return "", err
+		return err
 	}
 
 	// Find the unlocker directory and create the unlocker instance
@@ -338,92 +294,19 @@ func (v *Vault) RemoveUnlocker(unlockerID string) (string, error) {
 	// Find the unlocker by ID
 	unlocker, unlockerDir, err := v.findUnlockerByID(unlockersDir, unlockerID)
 	if err != nil {
-		return "", err
+		return err
 	}
 
 	if unlockerDir == "" {
-		return "", fmt.Errorf("unlocker with ID %s %w", unlockerID, ErrUnlockerNotFound)
-	}
-
-	currentUnlockerPath := filepath.Join(vaultDir, "current-unlocker")
-
-	currentDir, err := v.resolveUnlockerDirectory(currentUnlockerPath)
-	if err != nil && !errors.Is(err, os.ErrNotExist) {
-		return "", err
-	}
-
-	isCurrent := currentDir == unlockerDir
-	newCurrentID := ""
-
-	if isCurrent {
-		newCurrentID, err = v.selectOtherUnlocker(unlockerID)
-		if err != nil {
-			return "", err
-		}
+		return fmt.Errorf("unlocker with ID %s %w", unlockerID, ErrUnlockerNotFound)
 	}
 
 	if unlocker == nil {
-		err = secret.RemoveDirAtomic(v.fs, unlockerDir)
-	} else {
-		err = unlocker.Remove()
+		return secret.RemoveDirAtomic(v.fs, unlockerDir)
 	}
 
-	if err != nil {
-		return "", err
-	}
-
-	if isCurrent && newCurrentID == "" {
-		err = v.fs.Remove(currentUnlockerPath)
-		if err != nil {
-			return "", fmt.Errorf("failed to remove current-unlocker file: %w", err)
-		}
-	}
-
-	return newCurrentID, nil
-}
-
-// selectOtherUnlocker makes the unlocker firstOtherUnlockerID returns for
-// unlockerID the current unlocker and returns its ID. It returns "" when
-// there is no such unlocker.
-func (v *Vault) selectOtherUnlocker(unlockerID string) (string, error) {
-	otherID, err := v.firstOtherUnlockerID(unlockerID)
-	if err != nil {
-		return "", err
-	}
-
-	if otherID == "" {
-		return "", nil
-	}
-
-	err = v.SelectUnlocker(otherID)
-	if err != nil {
-		return "", err
-	}
-
-	return otherID, nil
-}
-
-// firstOtherUnlockerID returns the ID of the vault's first unlocker other
-// than unlockerID, in ID order, the order `secret unlocker list` shows, that
-// SelectUnlocker accepts, or "" when there is none. SelectUnlocker refuses
-// an unlocker whose metadata names a type other than passphrase or pgp,
-// which ListUnlockers still returns.
-func (v *Vault) firstOtherUnlockerID(unlockerID string) (string, error) {
-	unlockers, err := v.ListUnlockers()
-	if err != nil {
-		return "", err
-	}
-
-	for _, id := range slices.Sorted(maps.Keys(unlockers)) {
-		unlockerType := unlockers[id].Type
-
-		if id != unlockerID && (unlockerType == unlockerTypePassphrase ||
-			unlockerType == unlockerTypePGP) {
-			return id, nil
-		}
-	}
-
-	return "", nil
+	// Use the unlocker's Remove method
+	return unlocker.Remove()
 }
 
 // SelectUnlocker selects an unlocker as current for this vault

internal/vault/vault.go

@@ -291,14 +291,9 @@ func (v *Vault) unlockLongTermKey(
 // without a mnemonic records none, and without its metadata the key cannot
 // be derived. It is not added when the passphrase could not be read: the
 // unlocker was not tried, and adding one would need a passphrase read the
-// same way. Nor is it added when the passphrase is wrong: the unlocker still
-// opens the vault with the right one. Nor is it added when the current
-// unlocker does not exist and the vault has others: the error already says
-// to select one of them.
+// same way.
 func (v *Vault) withMnemonicAdvice(err error) error {
-	if errors.Is(err, secret.ErrPassphraseNotRead) ||
-		errors.Is(err, secret.ErrWrongPassphrase) ||
-		errors.Is(err, ErrUnlockerNotFound) {
+	if errors.Is(err, secret.ErrPassphraseNotRead) {
 		return err
 	}
 
@@ -309,21 +304,15 @@ func (v *Vault) withMnemonicAdvice(err error) error {
 		return err
 	}
 
-	return fmt.Errorf("%w; the vault '%s' still opens with its mnemonic: run "+
-		"%s with %s set to the mnemonic to give it a new unlocker",
-		err, v.Name, v.withVaultSelect("'secret unlocker add passphrase'"),
-		secret.EnvMnemonic)
-}
+	// 'secret unlocker add' acts on the current vault only.
+	steps := "'secret unlocker add passphrase'"
 
-// withVaultSelect returns command, a quoted 'secret unlocker' command, for
-// advice about this vault. Such a command acts on the current vault only,
-// so when this vault is not the current one, the command that selects it
-// comes first.
-func (v *Vault) withVaultSelect(command string) string {
-	current, err := GetCurrentVault(v.fs, v.stateDir)
-	if err != nil || current.Name != v.Name {
-		return fmt.Sprintf("'secret vault select %s', then %s", v.Name, command)
+	current, currentErr := GetCurrentVault(v.fs, v.stateDir)
+	if currentErr != nil || current.Name != v.Name {
+		steps = fmt.Sprintf("'secret vault select %s', then %s", v.Name, steps)
 	}
 
-	return command
+	return fmt.Errorf("%w; the vault '%s' still opens with its mnemonic: run "+
+		"%s with %s set to the mnemonic to give it a new unlocker",
+		err, v.Name, steps, secret.EnvMnemonic)
 }

internal/vault/vault_error_test.go

@@ -5,6 +5,7 @@ import (
 	"path/filepath"
 	"testing"
 
+	"filippo.io/age"
 	"github.com/awnumar/memguard"
 	"github.com/spf13/afero"
 	"github.com/stretchr/testify/assert"
@@ -139,7 +140,9 @@ func TestAddSecretForceWithWrongPassphraseChangesNothing(t *testing.T) {
 
 	err = vlt.AddSecret(testSecretName, second, true)
 
-	require.ErrorIs(t, err, secret.ErrWrongPassphrase)
+	var noMatch *age.NoIdentityMatchError
+
+	require.ErrorAs(t, err, &noMatch)
 	assert.Equal(t, before, dirContents(t, fs, secretDir))
 }