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 and
rename that over it only on success, using the new
`secret.WriteFileAtomicFunc`, which `secret.WriteFileAtomic` now calls.
A device or pipe given as `--output`, such as `/dev/null`, is written to
directly instead of being replaced by a file.

Judgement call: the device and pipe case goes beyond the plan.
Judgement call: a symbolic link given as `--output` is replaced, not followed.

Model: opus-5-5

README.md

@@ -275,12 +275,11 @@ 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 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 and the link kept. A device or a pipe, such as `/dev/null`, is written
-to directly.
+`--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. The file has mode 0600, whatever mode the file it replaced had, and
+when `--output` is a symbolic link to a file, the link itself is replaced. A
+device or a pipe, such as `/dev/null`, is written to directly.
 
 ## Storage Architecture
 

TODO.md

@@ -23,12 +23,10 @@ https://git.eeqj.de/sneak/secret/milestone/12
   `--output` (https://git.eeqj.de/sneak/secret/issues/143). Both emptied the
   output file before reading any input, so a wrong key left it empty, and a file
   given as both was emptied before it was read. They now write the result into a
-  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. 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.
+  temporary file next to it 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. A device or a pipe given as `--output`,
+  such as `/dev/null`, is still written to directly.
 - 2026-10-12: A mnemonic typed with extra spaces, tabs or line breaks gives the
   same long-term key as its words separated by single spaces
   (https://git.eeqj.de/sneak/secret/issues/135). The key was derived from the

internal/cli/crypto.go

@@ -5,12 +5,9 @@ import (
 	"fmt"
 	"io"
 	"os"
-	"path/filepath"
-	"sync/atomic"
 
 	"filippo.io/age"
 	"github.com/awnumar/memguard"
-	"github.com/spf13/afero"
 	"github.com/spf13/cobra"
 	"sneak.berlin/go/secret/internal/secret"
 	"sneak.berlin/go/secret/internal/vault"
@@ -284,18 +281,11 @@ func (cli *Instance) Decrypt(secretName, inputFile, outputFile string) error {
 	})
 }
 
-// temporaryOutput is the name of the temporary file writeOutput is filling,
-// until it is renamed into place. The signal handler in Entry removes it,
-// since for decrypt it holds part of the secret.
-//
-//nolint:gochecknoglobals // read by the signal handler, which has no Instance
-var temporaryOutput atomic.Pointer[string]
-
 // writeOutput calls write with stdout when outputFile is empty, and with
 // outputFile itself when it is a device or a pipe, such as /dev/null.
-// 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.
+// Otherwise write fills a new file of mode 0600 that replaces outputFile
+// only once write has succeeded, so a failure leaves outputFile as it was,
+// and outputFile may be the input file too.
 func (cli *Instance) writeOutput(
 	outputFile string, write func(output io.Writer) error,
 ) error {
@@ -314,24 +304,7 @@ func (cli *Instance) writeOutput(
 		return write(file)
 	}
 
-	// Replace the file a symbolic link points to and keep the link, as
-	// writing through the link would; /dev/stdout with stdout going to a
-	// file is such a link. A path that does not exist yet is used as given.
-	path, err := filepath.EvalSymlinks(outputFile)
-	if err != nil {
-		path = outputFile
-	}
-
-	err = secret.WriteFileAtomicFunc(cli.fs, path, func(file afero.File) error {
-		name := file.Name()
-		temporaryOutput.Store(&name)
-
-		return write(file)
-	})
-
-	temporaryOutput.Store(nil)
-
-	return err
+	return secret.WriteFileAtomicFunc(cli.fs, outputFile, write)
 }
 
 // isValidAgeSecretKey checks if a string is a valid age secret key by

internal/cli/crypto_test.go

@@ -2,10 +2,8 @@
 package cli
 
 import (
-	"io"
 	"os"
 	"path/filepath"
-	"strings"
 	"syscall"
 	"testing"
 
@@ -46,8 +44,7 @@ func newCryptoTest(t *testing.T) (*Instance, string) {
 // TestFailedCryptoLeavesOutputUnchanged is a regression test for
 // https://git.eeqj.de/sneak/secret/issues/143, where secret encrypt and
 // secret decrypt emptied the file named by --output before they failed.
-// A failed command must leave an existing file as it was and create none,
-// and give the same error as without --output.
+// A failed command must leave an existing file as it was and create none.
 func TestFailedCryptoLeavesOutputUnchanged(t *testing.T) {
 	t.Parallel()
 
@@ -73,13 +70,8 @@ func TestFailedCryptoLeavesOutputUnchanged(t *testing.T) {
 			existing := filepath.Join(dir, "existing.txt")
 			require.NoError(t, afero.WriteFile(cli.fs, existing, []byte("precious"), 0o600))
 
-			cli.cmd.SetOut(io.Discard)
-			toStdout := tt.run(cli, dir, "")
-			require.Error(t, toStdout)
-
-			require.EqualError(t, tt.run(cli, dir, existing), toStdout.Error())
-			require.EqualError(t, tt.run(cli, dir, filepath.Join(dir, "new.txt")),
-				toStdout.Error())
+			require.Error(t, tt.run(cli, dir, existing))
+			require.Error(t, tt.run(cli, dir, filepath.Join(dir, "new.txt")))
 
 			content, err := afero.ReadFile(cli.fs, existing)
 			require.NoError(t, err)
@@ -141,51 +133,3 @@ func TestDecryptToPipe(t *testing.T) {
 	require.Equal(t, os.ModeNamedPipe, info.Mode().Type())
 	require.Equal(t, plainText, string(<-received))
 }
-
-// TestDecryptThroughLink checks that secret decrypt given a symbolic link
-// as --output replaces the file the link points to, in another directory,
-// and keeps the link, as writing through the link would.
-func TestDecryptThroughLink(t *testing.T) {
-	t.Parallel()
-
-	cli, dir := newCryptoTest(t)
-	targetDir := t.TempDir()
-	target := filepath.Join(targetDir, "target.txt")
-	require.NoError(t, afero.WriteFile(cli.fs, target, []byte("old"), 0o600))
-
-	link := filepath.Join(dir, "link")
-	require.NoError(t, os.Symlink(target, link))
-
-	require.NoError(t, cli.Decrypt("key", filepath.Join(dir, "plain.txt.age"), link))
-
-	info, err := os.Lstat(link)
-	require.NoError(t, err)
-	require.Equal(t, os.ModeSymlink, info.Mode().Type())
-
-	content, err := afero.ReadFile(cli.fs, target)
-	require.NoError(t, err)
-	require.Equal(t, plainText, string(content))
-
-	// No temporary file left beside the target
-	entries, err := afero.ReadDir(cli.fs, targetDir)
-	require.NoError(t, err)
-	require.Len(t, entries, 1)
-}
-
-// TestDecryptToLongestName checks that secret decrypt writes an --output
-// whose name is as long as a file name can be, 255 bytes on Linux and
-// macOS: the temporary file's name must not be longer.
-func TestDecryptToLongestName(t *testing.T) {
-	t.Parallel()
-
-	const longestName = 255
-
-	cli, dir := newCryptoTest(t)
-	output := filepath.Join(dir, strings.Repeat("a", longestName))
-
-	require.NoError(t, cli.Decrypt("key", filepath.Join(dir, "plain.txt.age"), output))
-
-	content, err := afero.ReadFile(cli.fs, output)
-	require.NoError(t, err)
-	require.Equal(t, plainText, string(content))
-}

internal/cli/entry_test.go

@@ -8,7 +8,6 @@ import (
 	"path/filepath"
 	"strings"
 	"testing"
-	"time"
 
 	"github.com/awnumar/memguard"
 	"github.com/stretchr/testify/assert"
@@ -106,59 +105,3 @@ func TestInterruptExitsThroughMemguard(t *testing.T) {
 	require.ErrorAs(t, err, &exitErr)
 	assert.Equal(t, 1, exitErr.ExitCode(), "add ended with %v", err)
 }
-
-// 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()
-
-	ctx, cancel := context.WithTimeout(t.Context(), commandWait)
-	defer cancel()
-
-	secretPath := secretBinaryPath(t)
-	env := []string{
-		secret.EnvStateDir + "=" + t.TempDir(),
-		secret.EnvMnemonic + "=" + testMnemonic,
-		secret.EnvUnlockPassphrase + "=test-passphrase",
-		"PATH=/usr/bin:/bin",
-	}
-
-	//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)
-
-	// encrypt stores the new key before it starts writing, so decrypt has it
-	for _, command := range []string{"encrypt", "decrypt"} {
-		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
-
-		// 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(os.Interrupt))
-
-		var exitErr *exec.ExitError
-
-		require.ErrorAs(t, cmd.Wait(), &exitErr)
-
-		entries, err := os.ReadDir(outputDir)
-		require.NoError(t, err)
-		assert.Empty(t, entries, "%s left its temporary file", command)
-	}
-}

internal/cli/root.go

@@ -19,8 +19,6 @@ func Entry() int {
 	// 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) {
@@ -28,11 +26,6 @@ func Entry() int {
 		if terminalErr == nil && err == nil && foreground == unix.Getpgrp() {
 			_ = term.Restore(unix.Stdin, terminalState)
 		}
-
-		name := temporaryOutput.Load()
-		if name != nil {
-			_ = os.Remove(*name)
-		}
 	}, os.Interrupt, unix.SIGTERM)
 
 	defer memguard.Purge()

internal/secret/atomic.go

@@ -3,6 +3,7 @@ package secret
 import (
 	"errors"
 	"fmt"
+	"io"
 	"os"
 	"path/filepath"
 	"strings"
@@ -10,8 +11,9 @@ import (
 	"github.com/spf13/afero"
 )
 
-// tempNamePart starts the name of every temporary file WriteFileAtomicFunc
-// makes and every temporary directory TempDirFor makes, ".tmp-123".
+// tempNamePart is in the name of every temporary file WriteFileAtomicFunc
+// makes, ".NAME.tmp-123", and every temporary directory TempDirFor makes,
+// ".tmp-123".
 const tempNamePart = ".tmp-"
 
 // WriteFileAtomic replaces the file at path with data so that a reader, or
@@ -21,38 +23,30 @@ const tempNamePart = ".tmp-"
 // within one filesystem), is synced to disk, and is renamed over path. The
 // temporary file is removed if any step fails.
 func WriteFileAtomic(fs afero.Fs, path string, data []byte) error {
-	return WriteFileAtomicFunc(fs, path, func(file afero.File) error {
-		_, err := file.Write(data)
-		if err != nil {
-			return fmt.Errorf("failed to write %s: %w", path, err)
-		}
+	return WriteFileAtomicFunc(fs, path, func(w io.Writer) error {
+		_, err := w.Write(data)
 
-		return nil
+		return err
 	})
 }
 
 // WriteFileAtomicFunc is WriteFileAtomic for content that write streams
-// into the temporary file, which it is given, instead of holding it in
-// memory. An error from write is returned as it is. The temporary file's
-// name leaves out path's, which may already be as long as a name can be.
+// into the temporary file instead of holding it in memory. An error from
+// write fails it as any other step does.
 func WriteFileAtomicFunc(
-	fs afero.Fs, path string, write func(file afero.File) error,
+	fs afero.Fs, path string, write func(w io.Writer) error,
 ) error {
-	tmp, err := afero.TempFile(fs, filepath.Dir(path), tempNamePart)
+	tmp, err := afero.TempFile(fs, filepath.Dir(path),
+		"."+filepath.Base(path)+tempNamePart+"*")
 	if err != nil {
 		return fmt.Errorf("failed to create temporary file for %s: %w", path, err)
 	}
 
 	err = write(tmp)
-	if err != nil {
-		_ = tmp.Close()
-		_ = fs.Remove(tmp.Name())
-
-		return err
+	if err == nil {
+		err = tmp.Sync()
 	}
 
-	err = tmp.Sync()
-
 	closeErr := tmp.Close()
 	if err == nil {
 		err = closeErr