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

@@ -279,8 +279,8 @@ Both write the result to a new file next to `--output` and rename it over
 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.
+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
 

TODO.md

@@ -29,6 +29,14 @@ https://git.eeqj.de/sneak/secret/milestone/12
   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
+  written before the vault was unlocked to set `notAfter` on the version it
+  replaces, so each failed add left one more version that `secret version list`
+  showed. The vault is now unlocked, and that version's metadata read, before
+  anything is written. `generate secret --force` and `import --force` take the
+  same path.
 - 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

@@ -314,12 +314,9 @@ 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)
+	path, err := outputPath(outputFile)
 	if err != nil {
-		path = outputFile
+		return fmt.Errorf("failed to open output file: %w", err)
 	}
 
 	err = secret.WriteFileAtomicFunc(cli.fs, path, func(file afero.File) error {
@@ -334,6 +331,39 @@ func (cli *Instance) writeOutput(
 	return err
 }
 
+// outputPath returns the path of the file that writing to outputFile
+// 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) {
+		return path, err
+	}
+
+	// No file is at outputFile, or at the end of its links, yet
+	target, err := os.Readlink(outputFile)
+	if errors.Is(err, os.ErrNotExist) {
+		return outputFile, nil
+	}
+
+	if err != nil {
+		return "", err
+	}
+
+	if !filepath.IsAbs(target) {
+		// Not filepath.Join, which would remove a ".." in target together
+		// with the name before it, even when that name is a link to a
+		// directory
+		dir, _ := filepath.Split(outputFile)
+		target = dir + target
+	}
+
+	return outputPath(target)
+}
+
 // isValidAgeSecretKey checks if a string is a valid age secret key by
 // attempting to parse it
 func isValidAgeSecretKey(key string) bool {

internal/cli/crypto_test.go

@@ -172,6 +172,56 @@ func TestDecryptThroughLink(t *testing.T) {
 	require.Len(t, entries, 1)
 }
 
+// TestDecryptThroughLinkToMissingFile checks that secret decrypt given a
+// symbolic link to a file that does not exist yet as --output creates that
+// file and keeps the link, as writing through the link would. Here --output
+// is a relative link to a second link, which points into another directory.
+func TestDecryptThroughLinkToMissingFile(t *testing.T) {
+	t.Parallel()
+
+	cli, dir := newCryptoTest(t)
+	targetDir := t.TempDir()
+	target := filepath.Join(targetDir, "target.txt")
+
+	link := filepath.Join(dir, "link")
+	require.NoError(t, os.Symlink("second-link", link))
+	require.NoError(t, os.Symlink(target, filepath.Join(dir, "second-link")))
+
+	require.NoError(t, cli.Decrypt("key", filepath.Join(dir, "plain.txt.age"), link))
+
+	for _, name := range []string{"link", "second-link"} {
+		info, err := os.Lstat(filepath.Join(dir, name))
+		require.NoError(t, err)
+		require.Equal(t, os.ModeSymlink, info.Mode().Type(), name)
+	}
+
+	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)
+}
+
+// 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.
+func TestDecryptThroughLoopingLink(t *testing.T) {
+	t.Parallel()
+
+	cli, dir := newCryptoTest(t)
+	link := filepath.Join(dir, "link")
+	require.NoError(t, os.Symlink("link", link))
+
+	require.Error(t, cli.Decrypt("key", filepath.Join(dir, "plain.txt.age"), link))
+
+	target, err := os.Readlink(link)
+	require.NoError(t, err)
+	require.Equal(t, "link", target)
+}
+
 // 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.

internal/vault/secrets.go

@@ -219,6 +219,15 @@ func (v *Vault) addVersion(
 	name, secretDir string, value *memguard.LockedBuffer,
 	previousVersion *secret.Version,
 ) error {
+	// Unlock the vault before writing anything, so that a wrong passphrase
+	// or mnemonic leaves the secret as it was
+	err := v.loadPreviousVersionMetadata(previousVersion)
+	if err != nil {
+		return err
+	}
+
+	// Taken after the unlock, which can wait for the user to type the
+	// passphrase, so that the new version starts when it is created
 	now := time.Now()
 
 	// Create the new version and save the encrypted value
@@ -632,8 +641,35 @@ func (v *Vault) checkExistingSecret(
 	return true, previousVersion, nil
 }
 
+// loadPreviousVersionMetadata unlocks the vault and loads the metadata of
+// the version being superseded. It is a no-op when previousVersion is nil.
+func (v *Vault) loadPreviousVersionMetadata(
+	previousVersion *secret.Version,
+) error {
+	if previousVersion == nil {
+		return nil
+	}
+
+	ltIdentity, err := v.GetOrDeriveLongTermKey()
+	if err != nil {
+		secret.Debug("Failed to get long-term key for previous version", "error", err)
+
+		return fmt.Errorf("failed to get long-term key: %w", err)
+	}
+
+	err = previousVersion.LoadMetadata(ltIdentity)
+	if err != nil {
+		secret.Debug("Failed to load previous version metadata", "error", err)
+
+		return fmt.Errorf("failed to load previous version metadata: %w", err)
+	}
+
+	return nil
+}
+
 // updatePreviousVersion sets the notAfter timestamp on the version being
-// superseded. It is a no-op when previousVersion is nil.
+// superseded, whose metadata loadPreviousVersionMetadata has loaded. It is
+// a no-op when previousVersion is nil.
 func (v *Vault) updatePreviousVersion(
 	previousVersion *secret.Version, now *time.Time,
 ) error {
@@ -641,22 +677,13 @@ func (v *Vault) updatePreviousVersion(
 		return nil
 	}
 
-	// Get long-term key to decrypt/encrypt metadata
+	// loadPreviousVersionMetadata unlocked the vault, so this returns the
+	// key it keeps in memory
 	ltIdentity, err := v.GetOrDeriveLongTermKey()
 	if err != nil {
-		secret.Debug("Failed to get long-term key for metadata update", "error", err)
-
 		return fmt.Errorf("failed to get long-term key: %w", err)
 	}
 
-	// Load previous version metadata
-	err = previousVersion.LoadMetadata(ltIdentity)
-	if err != nil {
-		secret.Debug("Failed to load previous version metadata", "error", err)
-
-		return fmt.Errorf("failed to load previous version metadata: %w", err)
-	}
-
 	// Update notAfter timestamp
 	previousVersion.Metadata.NotAfter = now
 

internal/vault/secrets_version_test.go

@@ -10,6 +10,8 @@
 //   version
 // - TestVaultVersionTimestamps: Tests timestamp logic (notBefore/notAfter)
 //   across versions
+// - TestVaultVersionTimestampsAfterUnlock: Tests that those timestamps are
+//   taken after AddSecret unlocks the vault
 // - TestVaultGetNonExistentVersion: Tests error handling for invalid versions
 // - TestUpdateVersionMetadata: Tests metadata update functionality
 //
@@ -295,6 +297,70 @@ func TestVaultVersionTimestamps(t *testing.T) {
 	assert.Nil(t, secondVersion.Metadata.NotAfter) // Current version
 }
 
+// longTermKeyReadFs records when longterm.age, the last file an unlock
+// reads, was last opened.
+type longTermKeyReadFs struct {
+	afero.Fs
+
+	readAt time.Time
+}
+
+//nolint:ireturn // afero.File is the interface required by afero.Fs
+func (f *longTermKeyReadFs) Open(name string) (afero.File, error) {
+	if filepath.Base(name) == "longterm.age" {
+		f.readAt = time.Now()
+	}
+
+	return f.Fs.Open(name)
+}
+
+// TestVaultVersionTimestampsAfterUnlock checks that when AddSecret has to
+// unlock the vault to replace a version, the new version's notBefore and the
+// replaced version's notAfter are taken after the unlock, which can wait for
+// the user to type the passphrase.
+func TestVaultVersionTimestampsAfterUnlock(t *testing.T) {
+	t.Parallel()
+
+	fs := &longTermKeyReadFs{Fs: afero.NewMemMapFs()}
+
+	mnemonic := memguard.NewBufferFromBytes([]byte(testMnemonic))
+	defer mnemonic.Destroy()
+
+	passphrase := memguard.NewBufferFromBytes([]byte("test-passphrase"))
+	defer passphrase.Destroy()
+
+	created, err := CreateVault(fs, testStateDir, "test", mnemonic, passphrase)
+	require.NoError(t, err)
+
+	addTestSecretToVault(t, created, testSecretPath, []byte("version-1"), false)
+
+	// A new instance is locked; AddSecret unlocks it with the passphrase
+	vault := NewVault(fs, testStateDir, "test")
+	vault.SetUnlockPassphrase(passphrase)
+
+	addTestSecretToVault(t, vault, testSecretPath, []byte("version-2"), true)
+	require.False(t, fs.readAt.IsZero(), "AddSecret did not unlock the vault")
+
+	ltIdentity, err := vault.GetOrDeriveLongTermKey()
+	require.NoError(t, err)
+
+	vaultDir, _ := vault.GetDirectory()
+	secretDir := filepath.Join(vaultDir, "secrets.d", "test%secret")
+
+	versions, err := secret.ListVersions(fs, secretDir)
+	require.NoError(t, err)
+	require.Len(t, versions, 2)
+
+	newVersion := secret.NewVersion(vault, testSecretPath, versions[0])
+	require.NoError(t, newVersion.LoadMetadata(ltIdentity))
+
+	replacedVersion := secret.NewVersion(vault, testSecretPath, versions[1])
+	require.NoError(t, replacedVersion.LoadMetadata(ltIdentity))
+
+	assert.False(t, newVersion.Metadata.NotBefore.Before(fs.readAt))
+	assert.False(t, replacedVersion.Metadata.NotAfter.Before(fs.readAt))
+}
+
 func TestVaultGetNonExistentVersion(t *testing.T) {
 	t.Parallel()
 

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"
@@ -103,3 +104,78 @@ func TestAddSecretCleansUpOnFailure(t *testing.T) {
 	require.Len(t, entries, 1)
 	assert.Equal(t, "pub.age", entries[0].Name())
 }
+
+// TestAddSecretForceWithWrongPassphraseChangesNothing checks that adding a
+// version to an existing secret, with a passphrase that does not open the
+// vault, fails and leaves the secret's directory as it was.
+func TestAddSecretForceWithWrongPassphraseChangesNothing(t *testing.T) {
+	t.Parallel()
+
+	fs := afero.NewMemMapFs()
+
+	passphrase := memguard.NewBufferFromBytes([]byte(testPassphrase))
+	t.Cleanup(passphrase.Destroy)
+
+	created, err := vault.CreateVault(fs, testStateDir, testVaultName,
+		testMnemonicBuffer(t), passphrase)
+	require.NoError(t, err)
+
+	first := memguard.NewBufferFromBytes([]byte("first"))
+	t.Cleanup(first.Destroy)
+
+	require.NoError(t, created.AddSecret(testSecretName, first, false))
+
+	wrong := memguard.NewBufferFromBytes([]byte("wrong passphrase"))
+	t.Cleanup(wrong.Destroy)
+
+	vlt := vault.NewVault(fs, testStateDir, testVaultName)
+	vlt.SetUnlockPassphrase(wrong)
+
+	secretDir := filepath.Join(testStateDir, "vaults.d", testVaultName,
+		"secrets.d", testSecretName)
+	before := dirContents(t, fs, secretDir)
+
+	second := memguard.NewBufferFromBytes([]byte("second"))
+	t.Cleanup(second.Destroy)
+
+	err = vlt.AddSecret(testSecretName, second, true)
+
+	var noMatch *age.NoIdentityMatchError
+
+	require.ErrorAs(t, err, &noMatch)
+	assert.Equal(t, before, dirContents(t, fs, secretDir))
+}
+
+// dirContents maps every file under dir to its contents, and every
+// directory, written with a trailing "/", to "".
+func dirContents(t *testing.T, fs afero.Fs, dir string) map[string]string {
+	t.Helper()
+
+	contents := map[string]string{}
+
+	err := afero.Walk(fs, dir, func(
+		path string, info os.FileInfo, err error,
+	) error {
+		if err != nil {
+			return err
+		}
+
+		if info.IsDir() {
+			contents[path+"/"] = ""
+
+			return nil
+		}
+
+		content, err := afero.ReadFile(fs, path)
+		if err != nil {
+			return err
+		}
+
+		contents[path] = string(content)
+
+		return nil
+	})
+	require.NoError(t, err)
+
+	return contents
+}