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 file 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.
Judgement call: a link to a file that does not exist yet is replaced.

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, 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.
+replaced and the link kept. A device or a pipe, such as `/dev/null`, is written
+to directly.
 
 ## Storage Architecture
 

TODO.md

@@ -29,14 +29,6 @@ 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,9 +314,12 @@ func (cli *Instance) writeOutput(
 		return write(file)
 	}
 
-	path, err := outputPath(outputFile)
+	// 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 {
-		return fmt.Errorf("failed to open output file: %w", err)
+		path = outputFile
 	}
 
 	err = secret.WriteFileAtomicFunc(cli.fs, path, func(file afero.File) error {
@@ -331,39 +334,6 @@ 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,56 +172,6 @@ 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,15 +219,6 @@ 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
@@ -641,35 +632,8 @@ 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, whose metadata loadPreviousVersionMetadata has loaded. It is
-// a no-op when previousVersion is nil.
+// superseded. It is a no-op when previousVersion is nil.
 func (v *Vault) updatePreviousVersion(
 	previousVersion *secret.Version, now *time.Time,
 ) error {
@@ -677,13 +641,22 @@ func (v *Vault) updatePreviousVersion(
 		return nil
 	}
 
-	// loadPreviousVersionMetadata unlocked the vault, so this returns the
-	// key it keeps in memory
+	// Get long-term key to decrypt/encrypt metadata
 	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,8 +10,6 @@
 //   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
 //
@@ -297,70 +295,6 @@ 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,7 +5,6 @@ import (
 	"path/filepath"
 	"testing"
 
-	"filippo.io/age"
 	"github.com/awnumar/memguard"
 	"github.com/spf13/afero"
 	"github.com/stretchr/testify/assert"
@@ -104,78 +103,3 @@ 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
-}