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

@@ -274,6 +274,13 @@ 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. 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
 
 ### Directory Structure

TODO.md

@@ -18,6 +18,15 @@ https://git.eeqj.de/sneak/secret/milestone/12
 
 # Completed Steps
 
+- 2026-10-14: A failed `secret encrypt` or `secret decrypt` leaves the file
+  named by `--output` as it was, and the same file can be given as `--input` and
+  `--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 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

@@ -188,36 +188,24 @@ func (cli *Instance) Encrypt(secretName, inputFile, outputFile string) error {
 		input = file
 	}
 
-	// Set up output writer
-	output := cli.cmd.OutOrStdout()
-
-	if outputFile != "" {
-		file, err := cli.fs.Create(outputFile)
+	return cli.writeOutput(outputFile, func(output io.Writer) error {
+		encryptor, err := age.Encrypt(output, recipient)
 		if err != nil {
-			return fmt.Errorf("failed to create output file: %w", err)
+			return fmt.Errorf("failed to create age encryptor: %w", err)
 		}
-		defer func() { _ = file.Close() }()
 
-		output = file
-	}
+		_, err = io.Copy(encryptor, input)
+		if err != nil {
+			return fmt.Errorf("failed to encrypt data: %w", err)
+		}
 
-	// Encrypt the data
-	encryptor, err := age.Encrypt(output, recipient)
-	if err != nil {
-		return fmt.Errorf("failed to create age encryptor: %w", err)
-	}
+		err = encryptor.Close()
+		if err != nil {
+			return fmt.Errorf("failed to finalize encryption: %w", err)
+		}
 
-	_, err = io.Copy(encryptor, input)
-	if err != nil {
-		return fmt.Errorf("failed to encrypt data: %w", err)
-	}
-
-	err = encryptor.Close()
-	if err != nil {
-		return fmt.Errorf("failed to finalize encryption: %w", err)
-	}
-
-	return nil
+		return nil
+	})
 }
 
 // Decrypt decrypts data using an age secret key stored in a secret
@@ -278,31 +266,45 @@ func (cli *Instance) Decrypt(secretName, inputFile, outputFile string) error {
 		input = file
 	}
 
-	// Set up output writer
-	output := cli.cmd.OutOrStdout()
-
-	if outputFile != "" {
-		file, err := cli.fs.Create(outputFile)
+	return cli.writeOutput(outputFile, func(output io.Writer) error {
+		decryptor, err := age.Decrypt(input, identity)
 		if err != nil {
-			return fmt.Errorf("failed to create output file: %w", err)
+			return fmt.Errorf("failed to create age decryptor: %w", err)
+		}
+
+		_, err = io.Copy(output, decryptor)
+		if err != nil {
+			return fmt.Errorf("failed to decrypt data: %w", err)
+		}
+
+		return nil
+	})
+}
+
+// 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
+// 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 {
+	if outputFile == "" {
+		return write(cli.cmd.OutOrStdout())
+	}
+
+	info, err := cli.fs.Stat(outputFile)
+	if err == nil && !info.Mode().IsRegular() {
+		file, err := cli.fs.OpenFile(outputFile, os.O_WRONLY, 0)
+		if err != nil {
+			return fmt.Errorf("failed to open output file: %w", err)
 		}
 		defer func() { _ = file.Close() }()
 
-		output = file
+		return write(file)
 	}
 
-	// Decrypt the data
-	decryptor, err := age.Decrypt(input, identity)
-	if err != nil {
-		return fmt.Errorf("failed to create age decryptor: %w", err)
-	}
-
-	_, err = io.Copy(output, decryptor)
-	if err != nil {
-		return fmt.Errorf("failed to decrypt data: %w", err)
-	}
-
-	return nil
+	return secret.WriteFileAtomicFunc(cli.fs, outputFile, write)
 }
 
 // isValidAgeSecretKey checks if a string is a valid age secret key by

internal/cli/crypto_test.go

@@ -0,0 +1,135 @@
+//nolint:testpackage // sets the unexported fields of Instance
+package cli
+
+import (
+	"os"
+	"path/filepath"
+	"syscall"
+	"testing"
+
+	"github.com/spf13/afero"
+	"github.com/spf13/cobra"
+	"github.com/stretchr/testify/require"
+	"sneak.berlin/go/secret/internal/vault"
+)
+
+// plainText is the content of plain.txt in the directory newCryptoTest makes.
+const plainText = "document"
+
+// newCryptoTest returns an Instance whose current vault, in a new state
+// directory on the real filesystem, holds the age key secret "key", and a
+// new directory holding only plain.txt and plain.txt.age, its encryption
+// with that key.
+func newCryptoTest(t *testing.T) (*Instance, string) {
+	t.Helper()
+
+	fs := afero.NewOsFs()
+	stateDir := t.TempDir()
+
+	_, err := vault.CreateVault(fs, stateDir, "default", testMnemonicBuffer(t), nil)
+	require.NoError(t, err)
+
+	cli := NewCLIInstanceWithStateDir(fs, stateDir)
+	cli.Mnemonic = testMnemonicBuffer(t)
+	cli.cmd = &cobra.Command{}
+
+	dir := t.TempDir()
+	plain := filepath.Join(dir, "plain.txt")
+	require.NoError(t, afero.WriteFile(fs, plain, []byte(plainText), 0o600))
+	require.NoError(t, cli.Encrypt("key", plain, plain+".age"))
+
+	return cli, dir
+}
+
+// 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.
+func TestFailedCryptoLeavesOutputUnchanged(t *testing.T) {
+	t.Parallel()
+
+	tests := []struct {
+		name string
+		run  func(cli *Instance, dir, output string) error
+	}{
+		// Reading a directory fails once encryption has begun
+		{"encrypt", func(cli *Instance, dir, output string) error {
+			return cli.Encrypt("key", dir, output)
+		}},
+		// plain.txt is not an age file
+		{"decrypt", func(cli *Instance, dir, output string) error {
+			return cli.Decrypt("key", filepath.Join(dir, "plain.txt"), output)
+		}},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(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))
+
+			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)
+			require.Equal(t, "precious", string(content))
+
+			// No new.txt, and no temporary file left behind
+			entries, err := afero.ReadDir(cli.fs, dir)
+			require.NoError(t, err)
+
+			names := make([]string, 0, len(entries))
+			for _, entry := range entries {
+				names = append(names, entry.Name())
+			}
+
+			require.Equal(t, []string{"existing.txt", "plain.txt", "plain.txt.age"}, names)
+		})
+	}
+}
+
+// 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
+// before reading it.
+func TestCryptoInPlace(t *testing.T) {
+	t.Parallel()
+
+	cli, dir := newCryptoTest(t)
+	file := filepath.Join(dir, "plain.txt")
+
+	require.NoError(t, cli.Encrypt("key", file, file))
+	require.NoError(t, cli.Decrypt("key", file, file))
+
+	content, err := afero.ReadFile(cli.fs, file)
+	require.NoError(t, err)
+	require.Equal(t, plainText, string(content))
+}
+
+// TestDecryptToPipe checks that secret decrypt writes into a named pipe
+// given as --output, as it must into a device such as /dev/null, instead
+// of replacing it with a file.
+func TestDecryptToPipe(t *testing.T) {
+	t.Parallel()
+
+	cli, dir := newCryptoTest(t)
+	pipe := filepath.Join(dir, "pipe")
+	require.NoError(t, syscall.Mkfifo(pipe, 0o600))
+
+	received := make(chan []byte, 1)
+
+	go func() {
+		content, _ := afero.ReadFile(cli.fs, pipe)
+		received <- content
+	}()
+
+	require.NoError(t, cli.Decrypt("key", filepath.Join(dir, "plain.txt.age"), pipe))
+
+	info, err := os.Lstat(pipe)
+	require.NoError(t, err)
+	require.Equal(t, os.ModeNamedPipe, info.Mode().Type())
+	require.Equal(t, plainText, string(<-received))
+}

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 is in the name of every temporary file WriteFileAtomic makes,
-// ".NAME.tmp-123", 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,13 +23,26 @@ 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(w io.Writer) error {
+		_, err := w.Write(data)
+
+		return err
+	})
+}
+
+// WriteFileAtomicFunc is WriteFileAtomic for content that write streams
+// 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(w io.Writer) error,
+) error {
 	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 = tmp.Write(data)
+	err = write(tmp)
 	if err == nil {
 		err = tmp.Sync()
 	}