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

A Delivery carries its Event and Target structs in memory for the
delivery engine, so GORM's automatic association save upserted the
whole target row -- config included, which holds destination URLs
and bearer credentials -- into the per-webhook event database with
an empty webhook_id. Event databases are the files most likely to be
backed up or handed to someone else, so they shipped the credentials
with them.

Register a create and update callback on every per-webhook
connection that omits associations, rather than fixing the one call
site: it covers writes inside a transaction and write paths added
later. Sweep any rows already written, before the migration on each
open, so it is idempotent and a no-op on a database with no targets
table.

Encryption of target config at rest in webhooker.db is deliberately
not part of this: it is tracked separately.

internal/database/event_db_isolation.go

@@ -0,0 +1,92 @@
+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
+}
+
+// purgeTargetRows deletes target rows that an earlier build's
+// association upsert wrote into a per-webhook database. 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.
+//
+// It runs before every migration, so it is idempotent, and it is a
+// no-op on a database that has no targets table at all.
+func purgeTargetRows(
+	db *gorm.DB, log *slog.Logger, webhookID string,
+) error {
+	if !db.Migrator().HasTable("targets") {
+		return nil
+	}
+
+	res := db.Exec("DELETE FROM targets")
+	if res.Error != nil {
+		return fmt.Errorf(
+			"purging target rows from webhook database %s: %w",
+			webhookID, res.Error,
+		)
+	}
+
+	if res.RowsAffected > 0 {
+		log.Warn(
+			"purged leaked target rows from per-webhook database",
+			"webhook_id", webhookID,
+			"rows", res.RowsAffected,
+		)
+	}
+
+	return nil
+}

internal/database/event_db_isolation_test.go

@@ -0,0 +1,213 @@
+package database_test
+
+import (
+	"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
+}
+
+// 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))
+	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))
+	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))
+}
+
+// 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,22 @@ 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, 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{},

internal/delivery/event_db_isolation_test.go

@@ -0,0 +1,157 @@
+package delivery_test
+
+import (
+	"context"
+	"database/sql"
+	"fmt"
+	"net/http"
+	"net/http/httptest"
+	"testing"
+
+	"github.com/google/uuid"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+	_ "modernc.org/sqlite"
+	"sneak.berlin/go/webhooker/internal/database"
+)
+
+// assertNoTargetRows opens the per-webhook database file directly,
+// outside GORM, and fails if its targets table holds any rows.
+// Target config is the credential for slack and http targets, and
+// event databases are the files that get backed up and handed
+// around.
+func assertNoTargetRows(t *testing.T, dbPath string) {
+	t.Helper()
+
+	sqlDB, err := sql.Open(
+		"sqlite", fmt.Sprintf("file:%s?mode=ro", dbPath),
+	)
+	require.NoError(t, err)
+
+	defer func() { _ = sqlDB.Close() }()
+
+	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
+	}
+
+	var rows int
+
+	require.NoError(t, sqlDB.QueryRowContext(
+		t.Context(), "SELECT count(*) FROM targets",
+	).Scan(&rows))
+
+	assert.Zero(
+		t, rows,
+		"per-webhook event database must hold no target rows",
+	)
+}
+
+// TestEventDBHoldsNoTargetRows drives a delivery and then a retry
+// through the real engine write paths and asserts neither leaves a
+// target row behind in events-*.db.
+func TestEventDBHoldsNoTargetRows(t *testing.T) {
+	t.Parallel()
+
+	s := newISetup(t)
+
+	ts := httptest.NewServer(http.HandlerFunc(
+		func(w http.ResponseWriter, _ *http.Request) {
+			w.WriteHeader(http.StatusOK)
+		},
+	))
+	defer ts.Close()
+
+	cfg := iHTTPConfig(ts.URL)
+	targetID := uuid.New().String()
+	dbPath := s.DBMgr.DBPath(s.WebhookID)
+
+	event := iSeedEvent(
+		t, s.WebhookDB, s.WebhookID, `{"leak":"none"}`,
+	)
+	body := event.Body
+
+	// A new delivery.
+	d := iSeedDelivery(
+		t, s.WebhookDB, event.ID, targetID,
+		database.DeliveryStatusPending,
+	)
+	task := iTask(
+		d, event, s.WebhookID, targetID,
+		"leaky-target", cfg, 5, 1, &body,
+	)
+
+	s.Engine.ExportProcessNewTask(context.TODO(), &task)
+
+	iAssertStatus(
+		t, s.WebhookDB, d.ID,
+		database.DeliveryStatusDelivered,
+	)
+	assertNoTargetRows(t, dbPath)
+
+	// A retry.
+	rd := iSeedDelivery(
+		t, s.WebhookDB, event.ID, targetID,
+		database.DeliveryStatusRetrying,
+	)
+	rTask := iTask(
+		rd, event, s.WebhookID, targetID,
+		"leaky-target", cfg, 5, 2, &body,
+	)
+
+	s.Engine.ExportProcessRetryTask(context.TODO(), &rTask)
+
+	iAssertStatus(
+		t, s.WebhookDB, rd.ID,
+		database.DeliveryStatusDelivered,
+	)
+	assertNoTargetRows(t, dbPath)
+}
+
+// TestEventDBHoldsNoTargetRowsOnFailedDelivery covers the failure
+// write path, which updates the delivery to failed and records a
+// result, rather than the success path above.
+func TestEventDBHoldsNoTargetRowsOnFailedDelivery(t *testing.T) {
+	t.Parallel()
+
+	s := newISetup(t)
+
+	ts := httptest.NewServer(http.HandlerFunc(
+		func(w http.ResponseWriter, _ *http.Request) {
+			w.WriteHeader(http.StatusInternalServerError)
+		},
+	))
+	defer ts.Close()
+
+	cfg := iHTTPConfig(ts.URL)
+	targetID := uuid.New().String()
+
+	event := iSeedEvent(
+		t, s.WebhookDB, s.WebhookID, `{"leak":"none"}`,
+	)
+	body := event.Body
+
+	d := iSeedDelivery(
+		t, s.WebhookDB, event.ID, targetID,
+		database.DeliveryStatusPending,
+	)
+	task := iTask(
+		d, event, s.WebhookID, targetID,
+		"leaky-target", cfg, 0, 1, &body,
+	)
+
+	s.Engine.ExportProcessNewTask(context.TODO(), &task)
+
+	iAssertStatus(
+		t, s.WebhookDB, d.ID,
+		database.DeliveryStatusFailed,
+	)
+	assertNoTargetRows(t, s.DBMgr.DBPath(s.WebhookID))
+}