Stop target credentials leaking into event databases (closes #206) (#223)

GORM's association upsert copied whole targets rows -- plaintext
credential-bearing config -- into the per-webhook event databases with an
empty webhook_id. The leak was in updateDeliveryStatus, not the create
path: Update leaves Statement.Model pointing at a Delivery whose Target
the engine populated, so save_before_associations upserts it.

A connection-level callback now appends clause.Associations to
Statement.Omits on the create and update chains of every per-webhook
connection, so every write path is covered rather than one call site.

Existing files are swept on first open: the leaked rows are deleted and
the file is VACUUMed, because DELETE alone only unlinks the pages and
leaves the credential recoverable in the file's free space. The sweep is
recorded in PRAGMA user_version only after the VACUUM returns, so a sweep
that fails or is interrupted fails the open and is retried on the next
one, rather than being marked done.

Encryption of target config at rest is deliberately out of scope and
deferred to #212.

internal/database/event_db_isolation.go

@@ -0,0 +1,159 @@
+package database
+
+import (
+	"fmt"
+	"log/slog"
+
+	"gorm.io/gorm"
+	"gorm.io/gorm/clause"
+)
+
+// omitAssociationsCallback is the name the association guard is
+// registered under on a per-webhook database's create and update
+// callback chains.
+const omitAssociationsCallback = "webhooker:omit_associations"
+
+// omitAssociations makes every create and update issued against a
+// per-webhook database skip GORM's automatic association save.
+//
+// A per-webhook database holds the event tier only, but Delivery
+// declares belongs-to Event and Target and the delivery engine fills
+// both in memory before writing. Without this guard GORM upserts
+// those parent rows here on the delivery and retry write paths,
+// copying targets.config, which holds destination URLs and bearer
+// credentials, into the file most likely to be backed up or handed
+// to someone else. Registering the guard on the connection covers
+// every write path, including writes inside a transaction and write
+// paths added later. Every event-tier row this file holds is written
+// explicitly, so nothing depends on the automatic save.
+func omitAssociations(db *gorm.DB) error {
+	omit := func(tx *gorm.DB) {
+		tx.Statement.Omits = append(
+			tx.Statement.Omits, clause.Associations,
+		)
+	}
+
+	err := db.Callback().Create().
+		Before("gorm:save_before_associations").
+		Register(omitAssociationsCallback, omit)
+	if err != nil {
+		return fmt.Errorf(
+			"registering create association guard: %w", err,
+		)
+	}
+
+	err = db.Callback().Update().
+		Before("gorm:save_before_associations").
+		Register(omitAssociationsCallback, omit)
+	if err != nil {
+		return fmt.Errorf(
+			"registering update association guard: %w", err,
+		)
+	}
+
+	return nil
+}
+
+// eventDBSweptVersion is the PRAGMA user_version purgeTargetRows
+// stamps into a per-webhook database once it has removed any leaked
+// target rows *and* the VACUUM that removes their bytes has returned.
+// Nothing else in the tree uses user_version, so 0 means "not swept
+// by this build".
+//
+// The stamp, not the DELETE, is what records that a file is done. A
+// DELETE commits on its own, so a sweep that is interrupted or whose
+// VACUUM fails leaves a file whose rows are gone but whose credential
+// bytes are still in the free pages -- indistinguishable, by row
+// count, from a file that never leaked. Both leave the stamp unset,
+// so the next open sweeps again.
+const eventDBSweptVersion = 1
+
+// purgeTargetRows deletes target rows that an earlier build's
+// association upsert wrote into a per-webhook database, and rewrites
+// the file so their bytes are gone with them. AutoMigrate creates a
+// targets table in every one of these files because Delivery declares
+// a belongs-to Target, but nothing in the event tier may put rows in
+// it. The rows it did put there are junk, not history: they carry an
+// empty webhook_id, and delivery rows resolve their target against
+// the main database, so nothing here refers to them.
+//
+// The DELETE only unlinks the rows: modernc.org/sqlite leaves
+// secure_delete at SQLite's default of off, so the credential bytes
+// stay readable in the file's free pages and a backup of a swept file
+// would still hand them over. VACUUM rewrites the file without them.
+//
+// This runs before every migration and is gated on
+// eventDBSweptVersion, so a file pays for the rewrite once, on the
+// first open that finds it unstamped, and every open after that is a
+// PRAGMA read. A file this build created is stamped before its
+// targets table exists, so it never vacuums at all. A failure here
+// fails the open with the stamp left unset, so the sweep is retried
+// rather than skipped -- a webhook whose file cannot be swept stays
+// unusable instead of quietly serving from a file that still holds
+// recoverable credentials.
+func purgeTargetRows(
+	db *gorm.DB, log *slog.Logger, webhookID string,
+) error {
+	var version int
+
+	// Row().Scan, not (*gorm.DB).Scan: see internal/gormlog.
+	err := db.Raw("PRAGMA user_version").Row().Scan(&version)
+	if err != nil {
+		return fmt.Errorf(
+			"reading sweep marker of webhook database %s: %w",
+			webhookID, err,
+		)
+	}
+
+	if version >= eventDBSweptVersion {
+		return nil
+	}
+
+	var purged int64
+
+	if db.Migrator().HasTable("targets") {
+		res := db.Exec("DELETE FROM targets")
+		if res.Error != nil {
+			return fmt.Errorf(
+				"purging target rows from webhook database %s: %w",
+				webhookID, res.Error,
+			)
+		}
+
+		purged = res.RowsAffected
+
+		// Unconditional: a zero row count here does not mean there is
+		// nothing to remove, only that no *live* row is left. See
+		// eventDBSweptVersion.
+		err = db.Exec("VACUUM").Error
+		if err != nil {
+			return fmt.Errorf(
+				"purged %d leaked target rows from webhook database "+
+					"%s but vacuuming it failed, so the deleted "+
+					"target credentials are still recoverable from "+
+					"the file; it stays marked unswept and the next "+
+					"open retries: %w",
+				purged, webhookID, err,
+			)
+		}
+	}
+
+	err = db.Exec(fmt.Sprintf(
+		"PRAGMA user_version = %d", eventDBSweptVersion,
+	)).Error
+	if err != nil {
+		return fmt.Errorf(
+			"marking webhook database %s swept: %w", webhookID, err,
+		)
+	}
+
+	if purged > 0 {
+		log.Warn(
+			"purged leaked target rows from per-webhook database",
+			"webhook_id", webhookID,
+			"rows", purged,
+		)
+	}
+
+	return nil
+}

internal/database/event_db_isolation_test.go

@@ -0,0 +1,438 @@
+package database_test
+
+import (
+	"bytes"
+	"database/sql"
+	"fmt"
+	"os"
+	"path/filepath"
+	"testing"
+
+	"github.com/google/uuid"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+	_ "modernc.org/sqlite"
+	"sneak.berlin/go/webhooker/internal/database"
+)
+
+// testDataDirPerm is the mode the test data directory is created
+// with.
+const testDataDirPerm = 0o750
+
+// eventDBDataDir returns a data directory that a WebhookDBManager
+// can be pointed at.
+func eventDBDataDir(t *testing.T) string {
+	t.Helper()
+
+	dir := filepath.Join(t.TempDir(), "events")
+	require.NoError(t, os.MkdirAll(dir, testDataDirPerm))
+
+	return dir
+}
+
+// openRawEventDB opens the per-webhook database file directly,
+// without the manager, so a test can put a file on disk in a state
+// the manager has to cope with, or inspect one afterwards.
+func openRawEventDB(
+	t *testing.T, dataDir, webhookID string,
+) *sql.DB {
+	t.Helper()
+
+	path := filepath.Join(
+		dataDir, fmt.Sprintf("events-%s.db", webhookID),
+	)
+
+	sqlDB, err := sql.Open(
+		"sqlite",
+		fmt.Sprintf("file:%s?mode=rwc", path),
+	)
+	require.NoError(t, err)
+
+	t.Cleanup(func() { _ = sqlDB.Close() })
+
+	return sqlDB
+}
+
+// eventDBFileBytes reads a per-webhook database file off disk, so a
+// test can assert on what the file itself still holds rather than on
+// what a query returns.
+func eventDBFileBytes(t *testing.T, dataDir, webhookID string) []byte {
+	t.Helper()
+
+	//nolint:gosec // reads a file the test just created under t.TempDir()
+	raw, err := os.ReadFile(filepath.Join(
+		dataDir, fmt.Sprintf("events-%s.db", webhookID),
+	))
+	require.NoError(t, err)
+
+	return raw
+}
+
+// eventDBUserVersion returns the PRAGMA user_version of a per-webhook
+// database file, which is the marker purgeTargetRows stamps once it
+// has swept and vacuumed.
+func eventDBUserVersion(t *testing.T, sqlDB *sql.DB) int {
+	t.Helper()
+
+	var version int
+
+	require.NoError(t, sqlDB.QueryRowContext(
+		t.Context(), "PRAGMA user_version",
+	).Scan(&version))
+
+	return version
+}
+
+// clearEventDBSweptMarker resets the sweep marker to 0, which is what
+// a file written by a build without the sweep looks like. Tests that
+// seed a leaked row have to create the file through the manager to
+// get the real targets table shape, and that stamps it.
+func clearEventDBSweptMarker(t *testing.T, sqlDB *sql.DB) {
+	t.Helper()
+
+	_, err := sqlDB.ExecContext(t.Context(), "PRAGMA user_version = 0")
+	require.NoError(t, err)
+}
+
+// countTargetRows returns the number of rows in the targets table of
+// a per-webhook database file, or -1 if the table does not exist.
+func countTargetRows(t *testing.T, sqlDB *sql.DB) int {
+	t.Helper()
+
+	var tables int
+
+	require.NoError(t, sqlDB.QueryRowContext(
+		t.Context(),
+		"SELECT count(*) FROM sqlite_master "+
+			"WHERE type = 'table' AND name = 'targets'",
+	).Scan(&tables))
+
+	if tables == 0 {
+		return -1
+	}
+
+	var rows int
+
+	require.NoError(t, sqlDB.QueryRowContext(
+		t.Context(), "SELECT count(*) FROM targets",
+	).Scan(&rows))
+
+	return rows
+}
+
+// TestOpenPurgesLeakedTargetRows covers the sweep for event
+// databases written by a build that let GORM upsert target rows
+// into them: opening the database clears them, and opening it again
+// is a no-op.
+func TestOpenPurgesLeakedTargetRows(t *testing.T) {
+	t.Parallel()
+
+	dataDir := eventDBDataDir(t)
+	webhookID := uuid.New().String()
+
+	// Create the file the way the application does, so the targets
+	// table has exactly the shape AutoMigrate gives it, then write
+	// a leaked row into it the way the association upsert did.
+	initial := database.NewTestWebhookDBManager(dataDir)
+
+	_, err := initial.GetDB(webhookID)
+	require.NoError(t, err)
+	require.NoError(t, initial.CloseAll())
+
+	seed := openRawEventDB(t, dataDir, webhookID)
+
+	_, err = seed.ExecContext(
+		t.Context(),
+		"INSERT INTO targets "+
+			"(id, webhook_id, name, type, config) "+
+			"VALUES (?, '', ?, ?, ?)",
+		uuid.New().String(),
+		"leaked-target",
+		"slack",
+		`{"webhookUrl":"https://hooks.example/T000/B000/secret"}`,
+	)
+	require.NoError(t, err)
+	require.Equal(t, 1, countTargetRows(t, seed))
+	clearEventDBSweptMarker(t, seed)
+	require.NoError(t, seed.Close())
+
+	mgr := database.NewTestWebhookDBManager(dataDir)
+
+	_, err = mgr.GetDB(webhookID)
+	require.NoError(t, err)
+	require.NoError(t, mgr.CloseAll())
+
+	check := openRawEventDB(t, dataDir, webhookID)
+	assert.Zero(t, countTargetRows(t, check))
+	assert.Equal(
+		t, 1, eventDBUserVersion(t, check),
+		"a completed sweep must mark the file so later opens skip it",
+	)
+	require.NoError(t, check.Close())
+
+	// Idempotent: a second open leaves it at zero and does not
+	// error.
+	again := database.NewTestWebhookDBManager(dataDir)
+
+	_, err = again.GetDB(webhookID)
+	require.NoError(t, err)
+	require.NoError(t, again.CloseAll())
+
+	recheck := openRawEventDB(t, dataDir, webhookID)
+	assert.Zero(t, countTargetRows(t, recheck))
+}
+
+// TestOpenPurgeRemovesCredentialBytes covers the sweep at the level
+// that matters for a backup handed to someone else: the leaked
+// credential must be gone from the raw bytes of the file, not merely
+// unreachable by query. A bare DELETE unlinks the row and leaves the
+// bytes readable in the free pages, so this fails without the VACUUM
+// in purgeTargetRows.
+func TestOpenPurgeRemovesCredentialBytes(t *testing.T) {
+	t.Parallel()
+
+	dataDir := eventDBDataDir(t)
+	webhookID := uuid.New().String()
+	credential := "T00000000/B00000000/" + uuid.New().String()
+
+	initial := database.NewTestWebhookDBManager(dataDir)
+
+	_, err := initial.GetDB(webhookID)
+	require.NoError(t, err)
+	require.NoError(t, initial.CloseAll())
+
+	seed := openRawEventDB(t, dataDir, webhookID)
+
+	_, err = seed.ExecContext(
+		t.Context(),
+		"INSERT INTO targets "+
+			"(id, webhook_id, name, type, config) "+
+			"VALUES (?, '', ?, ?, ?)",
+		uuid.New().String(),
+		"leaked-target",
+		"slack",
+		fmt.Sprintf(
+			`{"webhookUrl":"https://hooks.example/%s"}`, credential,
+		),
+	)
+	require.NoError(t, err)
+	clearEventDBSweptMarker(t, seed)
+	require.NoError(t, seed.Close())
+
+	// The seed has to be in the file for its absence later to mean
+	// anything.
+	require.True(
+		t,
+		bytes.Contains(
+			eventDBFileBytes(t, dataDir, webhookID),
+			[]byte(credential),
+		),
+		"seeded credential is not in the file, so this test proves nothing",
+	)
+
+	mgr := database.NewTestWebhookDBManager(dataDir)
+
+	_, err = mgr.GetDB(webhookID)
+	require.NoError(t, err)
+	require.NoError(t, mgr.CloseAll())
+
+	assert.NotContains(
+		t,
+		string(eventDBFileBytes(t, dataDir, webhookID)),
+		credential,
+		"leaked credential is still recoverable from the raw file",
+	)
+}
+
+// TestOpenRevacuumsAfterIncompleteSweep covers the case a row count
+// cannot see: the rows are already deleted but the file was never
+// vacuumed, because an earlier sweep died between the two or its
+// VACUUM failed. The credential bytes are still recoverable, and the
+// unset marker is the only thing that says so, so the next open must
+// vacuum rather than conclude from the empty table that there is
+// nothing to do.
+func TestOpenRevacuumsAfterIncompleteSweep(t *testing.T) {
+	t.Parallel()
+
+	dataDir := eventDBDataDir(t)
+	webhookID := uuid.New().String()
+	credential := "T00000000/B00000000/" + uuid.New().String()
+
+	initial := database.NewTestWebhookDBManager(dataDir)
+
+	_, err := initial.GetDB(webhookID)
+	require.NoError(t, err)
+	require.NoError(t, initial.CloseAll())
+
+	seed := openRawEventDB(t, dataDir, webhookID)
+
+	_, err = seed.ExecContext(
+		t.Context(),
+		"INSERT INTO targets "+
+			"(id, webhook_id, name, type, config) "+
+			"VALUES (?, '', ?, ?, ?)",
+		uuid.New().String(),
+		"leaked-target",
+		"slack",
+		fmt.Sprintf(
+			`{"webhookUrl":"https://hooks.example/%s"}`, credential,
+		),
+	)
+	require.NoError(t, err)
+
+	// Exactly the state an interrupted sweep leaves: rows gone,
+	// marker unset, bytes still in the free pages.
+	_, err = seed.ExecContext(t.Context(), "DELETE FROM targets")
+	require.NoError(t, err)
+	require.Zero(t, countTargetRows(t, seed))
+	clearEventDBSweptMarker(t, seed)
+	require.NoError(t, seed.Close())
+
+	require.True(
+		t,
+		bytes.Contains(
+			eventDBFileBytes(t, dataDir, webhookID),
+			[]byte(credential),
+		),
+		"the deleted row's bytes must still be in the file, or this "+
+			"test proves nothing",
+	)
+
+	mgr := database.NewTestWebhookDBManager(dataDir)
+
+	_, err = mgr.GetDB(webhookID)
+	require.NoError(t, err)
+	require.NoError(t, mgr.CloseAll())
+
+	assert.NotContains(
+		t,
+		string(eventDBFileBytes(t, dataDir, webhookID)),
+		credential,
+		"an interrupted sweep was not retried, so the credential is "+
+			"still recoverable from the raw file",
+	)
+
+	check := openRawEventDB(t, dataDir, webhookID)
+	assert.Equal(t, 1, eventDBUserVersion(t, check))
+}
+
+// TestOpenSkipsSweptDatabase covers the other half of the marker: a
+// file this build created is marked without ever being vacuumed, and
+// a marked file is not swept again.
+func TestOpenSkipsSweptDatabase(t *testing.T) {
+	t.Parallel()
+
+	dataDir := eventDBDataDir(t)
+	webhookID := uuid.New().String()
+
+	mgr := database.NewTestWebhookDBManager(dataDir)
+
+	_, err := mgr.GetDB(webhookID)
+	require.NoError(t, err)
+	require.NoError(t, mgr.CloseAll())
+
+	marked := openRawEventDB(t, dataDir, webhookID)
+	assert.Equal(t, 1, eventDBUserVersion(t, marked))
+
+	// A marked file is left alone, so a row written into it survives
+	// a reopen. Nothing writes target rows any more; this stands in
+	// for the sweep having run.
+	_, err = marked.ExecContext(
+		t.Context(),
+		"INSERT INTO targets "+
+			"(id, webhook_id, name, type, config) "+
+			"VALUES (?, '', ?, ?, ?)",
+		uuid.New().String(), "sentinel", "slack", `{}`,
+	)
+	require.NoError(t, err)
+	require.NoError(t, marked.Close())
+
+	again := database.NewTestWebhookDBManager(dataDir)
+
+	_, err = again.GetDB(webhookID)
+	require.NoError(t, err)
+	require.NoError(t, again.CloseAll())
+
+	check := openRawEventDB(t, dataDir, webhookID)
+	assert.Equal(
+		t, 1, countTargetRows(t, check),
+		"a marked file must not be swept again",
+	)
+}
+
+// TestOpenSucceedsWithoutTargetsTable covers an existing event
+// database that never grew a targets table. The sweep must not fail
+// startup on it.
+func TestOpenSucceedsWithoutTargetsTable(t *testing.T) {
+	t.Parallel()
+
+	dataDir := eventDBDataDir(t)
+	webhookID := uuid.New().String()
+
+	seed := openRawEventDB(t, dataDir, webhookID)
+
+	_, err := seed.ExecContext(
+		t.Context(),
+		"CREATE TABLE events (id text PRIMARY KEY)",
+	)
+	require.NoError(t, err)
+	require.NoError(t, seed.Close())
+
+	mgr := database.NewTestWebhookDBManager(dataDir)
+
+	db, err := mgr.GetDB(webhookID)
+	require.NoError(t, err)
+	assert.NotNil(t, db)
+	require.NoError(t, mgr.CloseAll())
+}
+
+// TestEventDBCreateOmitsAssociations covers the connection-level
+// guard directly: a Delivery carrying its Event and Target in
+// memory, written through the manager's handle, must store only the
+// delivery row.
+func TestEventDBCreateOmitsAssociations(t *testing.T) {
+	t.Parallel()
+
+	dataDir := eventDBDataDir(t)
+	webhookID := uuid.New().String()
+
+	mgr := database.NewTestWebhookDBManager(dataDir)
+
+	db, err := mgr.GetDB(webhookID)
+	require.NoError(t, err)
+
+	target := database.Target{
+		WebhookID: webhookID,
+		Name:      "leaky-target",
+		Type:      database.TargetTypeSlack,
+		Config:    `{"webhookUrl":"https://hooks.example/secret"}`,
+	}
+	target.ID = uuid.New().String()
+
+	event := database.Event{
+		WebhookID:    webhookID,
+		EntrypointID: uuid.New().String(),
+		Method:       "POST",
+		Headers:      `{}`,
+		Body:         `{}`,
+	}
+	event.ID = uuid.New().String()
+
+	d := &database.Delivery{
+		EventID:  event.ID,
+		TargetID: target.ID,
+		Status:   database.DeliveryStatusPending,
+		Event:    event,
+		Target:   target,
+	}
+	d.ID = uuid.New().String()
+
+	require.NoError(t, db.Create(d).Error)
+	require.NoError(t, db.Model(d).
+		Update("status", database.DeliveryStatusDelivered).
+		Error)
+	require.NoError(t, mgr.CloseAll())
+
+	check := openRawEventDB(t, dataDir, webhookID)
+	assert.Zero(t, countTargetRows(t, check))
+}

internal/database/webhook_db_manager.go

@@ -262,6 +262,25 @@ func (m *WebhookDBManager) openDB(
 		)
 	}
 
+	// Keep main-database rows out of this file. See
+	// event_db_isolation.go.
+	err = omitAssociations(db)
+	if err != nil {
+		_ = sqlDB.Close()
+
+		return nil, fmt.Errorf(
+			"guarding webhook database %s: %w",
+			webhookID, err,
+		)
+	}
+
+	err = purgeTargetRows(db, m.log, webhookID)
+	if err != nil {
+		_ = sqlDB.Close()
+
+		return nil, err
+	}
+
 	// Run migrations for event-tier models only
 	err = db.AutoMigrate(
 		&Event{}, &Delivery{}, &DeliveryResult{},