Merge branch 'main' into secure-enclave-unlocker

internal/vault/path_traversal_test.go

@@ -0,0 +1,96 @@
+package vault
+
+import (
+	"testing"
+
+	"git.eeqj.de/sneak/secret/internal/secret"
+	"github.com/awnumar/memguard"
+	"github.com/spf13/afero"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+// TestGetSecretVersionRejectsPathTraversal verifies that GetSecretVersion
+// validates the secret name and rejects path traversal attempts.
+// This is a regression test for https://git.eeqj.de/sneak/secret/issues/13
+func TestGetSecretVersionRejectsPathTraversal(t *testing.T) {
+	testMnemonic := "abandon abandon abandon abandon abandon abandon abandon abandon abandon abandon abandon about"
+	t.Setenv(secret.EnvMnemonic, testMnemonic)
+	t.Setenv(secret.EnvUnlockPassphrase, "test-passphrase")
+
+	fs := afero.NewMemMapFs()
+	stateDir := "/test/state"
+
+	vlt, err := CreateVault(fs, stateDir, "test-vault")
+	require.NoError(t, err)
+
+	// Add a legitimate secret so the vault is set up
+	value := memguard.NewBufferFromBytes([]byte("legitimate-secret"))
+	err = vlt.AddSecret("legit", value, false)
+	require.NoError(t, err)
+
+	// These names contain path traversal and should be rejected
+	maliciousNames := []string{
+		"../../../etc/passwd",
+		"..%2f..%2fetc/passwd",
+		".secret",
+		"../sibling-vault/secrets.d/target",
+		"foo/../bar",
+		"a/../../etc/passwd",
+	}
+
+	for _, name := range maliciousNames {
+		t.Run(name, func(t *testing.T) {
+			_, err := vlt.GetSecretVersion(name, "")
+			assert.Error(t, err, "GetSecretVersion should reject malicious name: %s", name)
+			assert.Contains(t, err.Error(), "invalid secret name",
+				"error should indicate invalid name for: %s", name)
+		})
+	}
+}
+
+// TestGetSecretRejectsPathTraversal verifies GetSecret (which calls GetSecretVersion)
+// also rejects path traversal names.
+func TestGetSecretRejectsPathTraversal(t *testing.T) {
+	testMnemonic := "abandon abandon abandon abandon abandon abandon abandon abandon abandon abandon abandon about"
+	t.Setenv(secret.EnvMnemonic, testMnemonic)
+	t.Setenv(secret.EnvUnlockPassphrase, "test-passphrase")
+
+	fs := afero.NewMemMapFs()
+	stateDir := "/test/state"
+
+	vlt, err := CreateVault(fs, stateDir, "test-vault")
+	require.NoError(t, err)
+
+	_, err = vlt.GetSecret("../../../etc/passwd")
+	assert.Error(t, err)
+	assert.Contains(t, err.Error(), "invalid secret name")
+}
+
+// TestGetSecretObjectRejectsPathTraversal verifies GetSecretObject
+// also validates names and rejects path traversal attempts.
+func TestGetSecretObjectRejectsPathTraversal(t *testing.T) {
+	testMnemonic := "abandon abandon abandon abandon abandon abandon abandon abandon abandon abandon abandon about"
+	t.Setenv(secret.EnvMnemonic, testMnemonic)
+	t.Setenv(secret.EnvUnlockPassphrase, "test-passphrase")
+
+	fs := afero.NewMemMapFs()
+	stateDir := "/test/state"
+
+	vlt, err := CreateVault(fs, stateDir, "test-vault")
+	require.NoError(t, err)
+
+	maliciousNames := []string{
+		"../../../etc/passwd",
+		"foo/../bar",
+		"a/../../etc/passwd",
+	}
+
+	for _, name := range maliciousNames {
+		t.Run(name, func(t *testing.T) {
+			_, err := vlt.GetSecretObject(name)
+			assert.Error(t, err, "GetSecretObject should reject: %s", name)
+			assert.Contains(t, err.Error(), "invalid secret name")
+		})
+	}
+}

internal/vault/secrets.go

@@ -67,7 +67,7 @@ func (v *Vault) ListSecrets() ([]string, error) {
 	return secrets, nil
 }
 
-// isValidSecretName validates secret names according to the format [a-z0-9\.\-\_\/]+
+// isValidSecretName validates secret names according to the format [a-zA-Z0-9\.\-\_\/]+
 // but with additional restrictions:
 // - No leading or trailing slashes
 // - No double slashes
@@ -92,8 +92,15 @@ func isValidSecretName(name string) bool {
 		return false
 	}
 
+	// Check for path traversal via ".." components
+	for _, part := range strings.Split(name, "/") {
+		if part == ".." {
+			return false
+		}
+	}
+
 	// Check the basic pattern
-	matched, _ := regexp.MatchString(`^[a-z0-9\.\-\_\/]+$`, name)
+	matched, _ := regexp.MatchString(`^[a-zA-Z0-9\.\-\_\/]+$`, name)
 
 	return matched
 }
@@ -319,6 +326,13 @@ func (v *Vault) GetSecretVersion(name string, version string) ([]byte, error) {
 		slog.String("version", version),
 	)
 
+	// Validate secret name to prevent path traversal
+	if !isValidSecretName(name) {
+		secret.Debug("Invalid secret name provided", "secret_name", name)
+
+		return nil, fmt.Errorf("invalid secret name '%s': must match pattern [a-z0-9.\\-_/]+", name)
+	}
+
 	// Get vault directory
 	vaultDir, err := v.GetDirectory()
 	if err != nil {
@@ -454,6 +468,10 @@ func (v *Vault) UnlockVault() (*age.X25519Identity, error) {
 
 // GetSecretObject retrieves a Secret object with metadata loaded from this vault
 func (v *Vault) GetSecretObject(name string) (*secret.Secret, error) {
+	if !isValidSecretName(name) {
+		return nil, fmt.Errorf("invalid secret name: %s", name)
+	}
+
 	// First check if the secret exists by checking for the metadata file
 	vaultDir, err := v.GetDirectory()
 	if err != nil {

internal/vault/secrets_name_test.go

@@ -0,0 +1,42 @@
+package vault
+
+import "testing"
+
+func TestIsValidSecretNameUppercase(t *testing.T) {
+	tests := []struct {
+		name  string
+		valid bool
+	}{
+		// Lowercase (existing behavior)
+		{"valid-name", true},
+		{"valid.name", true},
+		{"valid_name", true},
+		{"valid/path/name", true},
+		{"123valid", true},
+
+		// Uppercase (new behavior - issue #2)
+		{"Valid-Upper-Name", true},
+		{"2025-11-21-ber1app1-vaultik-test-bucket-AKI", true},
+		{"MixedCase/Path/Name", true},
+		{"ALLUPPERCASE", true},
+		{"ABC123", true},
+
+		// Still invalid
+		{"", false},
+		{"invalid name", false},
+		{"invalid@name", false},
+		{".dotstart", false},
+		{"/leading-slash", false},
+		{"trailing-slash/", false},
+		{"double//slash", false},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			result := isValidSecretName(tt.name)
+			if result != tt.valid {
+				t.Errorf("isValidSecretName(%q) = %v, want %v", tt.name, result, tt.valid)
+			}
+		})
+	}
+}

internal/vault/unlockers.go

@@ -218,7 +218,9 @@ func (v *Vault) ListUnlockers() ([]UnlockerMetadata, error) {
 				return nil, fmt.Errorf("failed to check if metadata exists for unlocker %s: %w", file.Name(), err)
 			}
 			if !exists {
-				return nil, fmt.Errorf("unlocker directory %s is missing metadata file", file.Name())
+				secret.Warn("Skipping unlocker directory with missing metadata file", "directory", file.Name())
+
+				continue
 			}
 
 			metadataBytes, err := afero.ReadFile(v.fs, metadataPath)

internal/vault/vault.go

@@ -225,27 +225,23 @@ func (v *Vault) NumSecrets() (int, error) {
 		return 0, fmt.Errorf("failed to read secrets directory: %w", err)
 	}
 
-	// Count only directories that contain at least one version file
+	// Count only directories that have a "current" version pointer file
 	count := 0
 	for _, entry := range entries {
 		if !entry.IsDir() {
 			continue
 		}
 
-		// Check if this secret directory contains any version files
+		// A valid secret has a "current" file pointing to the active version
 		secretDir := filepath.Join(secretsDir, entry.Name())
-		versionFiles, err := afero.ReadDir(v.fs, secretDir)
+		currentFile := filepath.Join(secretDir, "current")
+		exists, err := afero.Exists(v.fs, currentFile)
 		if err != nil {
 			continue // Skip directories we can't read
 		}
 
-		// Look for at least one version file (excluding "current" symlink)
-		for _, vFile := range versionFiles {
-			if !vFile.IsDir() && vFile.Name() != "current" {
-				count++
-
-				break // Found at least one version, count this secret
-			}
+		if exists {
+			count++
 		}
 	}
 

internal/vault/vault_test.go

@@ -162,6 +162,24 @@ func TestVaultOperations(t *testing.T) {
 		}
 	})
 
+	// Test NumSecrets
+	t.Run("NumSecrets", func(t *testing.T) {
+		vlt, err := GetCurrentVault(fs, stateDir)
+		if err != nil {
+			t.Fatalf("Failed to get current vault: %v", err)
+		}
+
+		numSecrets, err := vlt.NumSecrets()
+		if err != nil {
+			t.Fatalf("Failed to count secrets: %v", err)
+		}
+
+		// We added one secret in SecretOperations
+		if numSecrets != 1 {
+			t.Errorf("Expected 1 secret, got %d", numSecrets)
+		}
+	})
+
 	// Test unlocker operations
 	t.Run("UnlockerOperations", func(t *testing.T) {
 		vlt, err := GetCurrentVault(fs, stateDir)
@@ -225,3 +243,57 @@ func TestVaultOperations(t *testing.T) {
 		}
 	})
 }
+
+func TestListUnlockers_SkipsMissingMetadata(t *testing.T) {
+	// Set test environment variables
+	testMnemonic := "abandon abandon abandon abandon abandon abandon abandon abandon abandon abandon abandon about"
+	t.Setenv(secret.EnvMnemonic, testMnemonic)
+	t.Setenv(secret.EnvUnlockPassphrase, "test-passphrase")
+
+	// Use in-memory filesystem
+	fs := afero.NewMemMapFs()
+	stateDir := "/test/state"
+
+	// Create vault
+	vlt, err := CreateVault(fs, stateDir, "test-vault")
+	if err != nil {
+		t.Fatalf("Failed to create vault: %v", err)
+	}
+
+	// Create a passphrase unlocker so we have at least one valid unlocker
+	passphraseBuffer := memguard.NewBufferFromBytes([]byte("test-passphrase"))
+	defer passphraseBuffer.Destroy()
+	_, err = vlt.CreatePassphraseUnlocker(passphraseBuffer)
+	if err != nil {
+		t.Fatalf("Failed to create passphrase unlocker: %v", err)
+	}
+
+	// Create a bogus unlocker directory with no metadata file
+	vaultDir, err := vlt.GetDirectory()
+	if err != nil {
+		t.Fatalf("Failed to get vault directory: %v", err)
+	}
+	bogusDir := filepath.Join(vaultDir, "unlockers.d", "bogus-no-metadata")
+	err = fs.MkdirAll(bogusDir, 0o700)
+	if err != nil {
+		t.Fatalf("Failed to create bogus directory: %v", err)
+	}
+
+	// ListUnlockers should succeed, skipping the bogus directory
+	unlockers, err := vlt.ListUnlockers()
+	if err != nil {
+		t.Fatalf("ListUnlockers returned error when it should have skipped bad directory: %v", err)
+	}
+
+	// Should still have the valid passphrase unlocker
+	if len(unlockers) == 0 {
+		t.Errorf("Expected at least one unlocker, got none")
+	}
+
+	// Verify we only got the valid unlocker(s), not the bogus one
+	for _, u := range unlockers {
+		if u.Type == "" {
+			t.Errorf("Got unlocker with empty type, likely from bogus directory")
+		}
+	}
+}