From 92c41bdb0cf6ae1842494877577a10f643cb1a00 Mon Sep 17 00:00:00 2001
From: sneak <sneak@sneak.berlin>
Date: Sat, 26 Jul 2025 22:03:31 +0200
Subject: [PATCH] Fix error handling in AddSecret to clean up on failure

- Clean up secret directory if Save() fails for new secrets
- Add tests to verify cleanup behavior
- Ensures failed secret additions don't leave orphaned directories
---
 internal/vault/secrets.go          |  6 +++
 internal/vault/vault_error_test.go | 87 ++++++++++++++++++++++++++++++
 2 files changed, 93 insertions(+)
 create mode 100644 internal/vault/vault_error_test.go

diff --git a/internal/vault/secrets.go b/internal/vault/secrets.go
index a41357c..f836c9f 100644
--- a/internal/vault/secrets.go
+++ b/internal/vault/secrets.go
@@ -204,6 +204,12 @@ func (v *Vault) AddSecret(name string, value *memguard.LockedBuffer, force bool)
 	if err := newVersion.Save(value); err != nil {
 		secret.Debug("Failed to save new version", "error", err, "version", versionName)
 
+		// Clean up the secret directory if this was a new secret
+		if !exists {
+			secret.Debug("Cleaning up secret directory due to save failure", "secret_dir", secretDir)
+			_ = v.fs.RemoveAll(secretDir)
+		}
+
 		return fmt.Errorf("failed to save version: %w", err)
 	}
 
diff --git a/internal/vault/vault_error_test.go b/internal/vault/vault_error_test.go
new file mode 100644
index 0000000..72be622
--- /dev/null
+++ b/internal/vault/vault_error_test.go
@@ -0,0 +1,87 @@
+package vault_test
+
+import (
+	"path/filepath"
+	"testing"
+
+	"git.eeqj.de/sneak/secret/internal/secret"
+	"git.eeqj.de/sneak/secret/internal/vault"
+	"github.com/awnumar/memguard"
+	"github.com/spf13/afero"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+func TestAddSecretFailsWithMissingPublicKey(t *testing.T) {
+	// Create in-memory filesystem
+	fs := afero.NewMemMapFs()
+	stateDir := "/test/state"
+
+	// Create a vault directory without a public key (simulating the error condition)
+	vaultDir := filepath.Join(stateDir, "vaults.d", "broken")
+	require.NoError(t, fs.MkdirAll(vaultDir, secret.DirPerms))
+
+	// Create currentvault symlink
+	currentVaultPath := filepath.Join(stateDir, "currentvault")
+	require.NoError(t, afero.WriteFile(fs, currentVaultPath, []byte(vaultDir), secret.FilePerms))
+
+	// Create vault instance
+	vlt := vault.NewVault(fs, stateDir, "broken")
+
+	// Try to add a secret - this should fail
+	secretName := "test-secret"
+	value := memguard.NewBufferFromBytes([]byte("test-value"))
+	defer value.Destroy()
+
+	err := vlt.AddSecret(secretName, value, false)
+	require.Error(t, err, "AddSecret should fail when public key is missing")
+	assert.Contains(t, err.Error(), "failed to read long-term public key")
+
+	// Verify that the secret directory was NOT created
+	secretDir := filepath.Join(vaultDir, "secrets.d", secretName)
+	exists, _ := afero.DirExists(fs, secretDir)
+	assert.False(t, exists, "Secret directory should not exist after failed AddSecret")
+
+	// Verify the secrets.d directory is empty or doesn't exist
+	secretsDir := filepath.Join(vaultDir, "secrets.d")
+	if exists, _ := afero.DirExists(fs, secretsDir); exists {
+		entries, err := afero.ReadDir(fs, secretsDir)
+		require.NoError(t, err)
+		assert.Empty(t, entries, "secrets.d directory should be empty after failed AddSecret")
+	}
+}
+
+func TestAddSecretCleansUpOnFailure(t *testing.T) {
+	// Create in-memory filesystem
+	fs := afero.NewMemMapFs()
+	stateDir := "/test/state"
+
+	// Create a vault directory with public key
+	vaultDir := filepath.Join(stateDir, "vaults.d", "test")
+	require.NoError(t, fs.MkdirAll(vaultDir, secret.DirPerms))
+
+	// Create a mock public key that will cause encryption to fail
+	// by using an invalid age public key format
+	pubKeyPath := filepath.Join(vaultDir, "pub.age")
+	require.NoError(t, afero.WriteFile(fs, pubKeyPath, []byte("invalid-public-key"), secret.FilePerms))
+
+	// Create currentvault symlink
+	currentVaultPath := filepath.Join(stateDir, "currentvault")
+	require.NoError(t, afero.WriteFile(fs, currentVaultPath, []byte(vaultDir), secret.FilePerms))
+
+	// Create vault instance
+	vlt := vault.NewVault(fs, stateDir, "test")
+
+	// Try to add a secret - this should fail during encryption
+	secretName := "test-secret"
+	value := memguard.NewBufferFromBytes([]byte("test-value"))
+	defer value.Destroy()
+
+	err := vlt.AddSecret(secretName, value, false)
+	require.Error(t, err, "AddSecret should fail with invalid public key")
+
+	// Verify that the secret directory was NOT created
+	secretDir := filepath.Join(vaultDir, "secrets.d", secretName)
+	exists, _ := afero.DirExists(fs, secretDir)
+	assert.False(t, exists, "Secret directory should not exist after failed AddSecret")
+}
\ No newline at end of file