From 27d4f1c5767d113bb53b57e384130f4f369c6458 Mon Sep 17 00:00:00 2001 From: sneak Date: Mon, 24 Aug 2026 00:14:58 +0000 Subject: [PATCH] Check every statement in the webhook deletion transaction (closes #262) deleteWebhookResources issued three deletes without checking any of them. A failing delete sets .Error on the returned session but leaves the transaction usable, so the handler committed whatever succeeded, redirected to /sources as though deletion had worked, and then hard-deleted the per-webhook event database anyway. A webhook could end up with its entrypoints or targets still in the main database and its entire event history permanently gone, reported as a success. The transaction moves into commitWebhookDeletion, which checks each statement and rolls back on any failure, matching commitWebhook in the same file. deleteWebhookResources reports the failure with h.serverError and leaves the event database alone. The configuration commit deliberately precedes DeleteDB: no transaction spans SQLite and the filesystem, and a failure after the commit leaves an unreferenced event database file, which the operator can remove, rather than destroying history for a webhook that still exists. That failure is reported with h.serverError too instead of a success redirect. --- internal/handlers/source_delete_test.go | 226 ++++++++++++++++++++++++ internal/handlers/source_management.go | 109 +++++++----- 2 files changed, 293 insertions(+), 42 deletions(-) diff --git a/internal/handlers/source_delete_test.go b/internal/handlers/source_delete_test.go index 2b493bd..5441139 100644 --- a/internal/handlers/source_delete_test.go +++ b/internal/handlers/source_delete_test.go @@ -2,6 +2,7 @@ package handlers_test import ( "context" + "errors" "net/http" "net/http/httptest" "os" @@ -11,6 +12,7 @@ import ( "github.com/go-chi/chi" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "gorm.io/gorm" "gorm.io/gorm/clause" "sneak.berlin/go/webhooker/internal/database" "sneak.berlin/go/webhooker/internal/handlers" @@ -73,6 +75,77 @@ func seedTarget( return tgt } +// errInjectedDelete is the failure failDeleteOnTable reports +// from a delete statement. +var errInjectedDelete = errors.New("injected delete failure") + +// seedEntrypoint inserts an entrypoint for a webhook. +func seedEntrypoint( + t *testing.T, + db *database.Database, + webhookID string, +) { + t.Helper() + + ep := &database.Entrypoint{ + WebhookID: webhookID, + Path: "ep-" + webhookID, + Active: true, + } + + require.NoError( + t, + db.DB().Omit(clause.Associations).Create(ep).Error, + ) +} + +// countRows counts the live (not soft-deleted) rows of a model +// matching column = value. +func countRows( + t *testing.T, + db *database.Database, + model any, + column, value string, +) int64 { + t.Helper() + + var n int64 + + require.NoError( + t, + db.DB().Model(model). + Where(column+" = ?", value). + Count(&n).Error, + ) + + return n +} + +// failDeleteOnTable makes every delete against the named table +// fail the way a database-level error does: the statement +// reports an error but leaves the surrounding transaction +// usable, so a caller that does not check it can go on to +// commit the statements that did succeed. +func failDeleteOnTable( + t *testing.T, + db *database.Database, + table string, +) { + t.Helper() + + require.NoError(t, db.DB().Callback().Delete(). + Before("gorm:delete"). + Register( + "test:fail_delete_"+table, + func(tx *gorm.DB) { + if tx.Statement.Table == table { + _ = tx.AddError(errInjectedDelete) + } + }, + ), + ) +} + // archivePathFor returns the archive database path the // delivery engine would use for a webhook: beside the webhook's // event database in the data directory. @@ -209,6 +282,159 @@ func TestHandleSourceDelete_KeepsArchiveFile(t *testing.T) { ) } +// TestHandleSourceDelete_FailedDeleteKeepsEverything proves +// that a failing delete statement loses nothing: the +// configuration is rolled back whole, the event database +// survives, and the operator is told the deletion failed +// instead of being redirected as though it worked. +func TestHandleSourceDelete_FailedDeleteKeepsEverything( + t *testing.T, +) { + t.Parallel() + + var ( + h *handlers.Handlers + sess *session.Session + db *database.Database + mgr *database.WebhookDBManager + ) + + app := newTestApp(t, &h, &sess, &db, &mgr) + app.RequireStart() + + t.Cleanup(app.RequireStop) + + wh := seedWebhook(t, db) + seedEntrypoint(t, db, wh.ID) + seedTarget(t, db, wh.ID, database.TargetTypeDatabase) + + require.NoError(t, mgr.CreateDB(wh.ID)) + + eventDBPath := mgr.DBPath(wh.ID) + require.FileExists(t, eventDBPath) + + // The entrypoint delete runs first and succeeds; the target + // delete then fails, which is what the whole transaction has + // to be rolled back over. + failDeleteOnTable(t, db, "targets") + + cookies := authenticatedCookies( + t, sess, deleteTestUserID, deleteTestUsername, + ) + + req := postRequest( + "/source/"+wh.ID+"/delete", + cookies, + map[string]string{paramSourceID: wh.ID}, + ) + w := httptest.NewRecorder() + + h.HandleSourceDelete().ServeHTTP(w, req) + + assert.Equal( + t, http.StatusInternalServerError, w.Code, + "a failed deletion must be reported, not redirected", + ) + assert.Empty( + t, w.Header().Get("Location"), + "a failed deletion must not redirect to /sources", + ) + + assert.Equal( + t, int64(1), + countRows(t, db, &database.Webhook{}, "id", wh.ID), + "the webhook must survive a failed deletion", + ) + assert.Equal( + t, int64(1), + countRows( + t, db, &database.Entrypoint{}, "webhook_id", wh.ID, + ), + "the entrypoint delete must be rolled back", + ) + assert.Equal( + t, int64(1), + countRows( + t, db, &database.Target{}, "webhook_id", wh.ID, + ), + "the target must survive a failed deletion", + ) + + assert.FileExists( + t, eventDBPath, + "event history must not be destroyed when the "+ + "configuration delete did not commit", + ) +} + +// TestHandleSourceDelete_RemovesConfigAndEventDatabase is the +// positive control for the rollback above: an ordinary deletion +// still removes the webhook, its children and its event +// database. +func TestHandleSourceDelete_RemovesConfigAndEventDatabase( + t *testing.T, +) { + t.Parallel() + + var ( + h *handlers.Handlers + sess *session.Session + db *database.Database + mgr *database.WebhookDBManager + ) + + app := newTestApp(t, &h, &sess, &db, &mgr) + app.RequireStart() + + t.Cleanup(app.RequireStop) + + wh := seedWebhook(t, db) + seedEntrypoint(t, db, wh.ID) + seedTarget(t, db, wh.ID, database.TargetTypeDatabase) + + require.NoError(t, mgr.CreateDB(wh.ID)) + + eventDBPath := mgr.DBPath(wh.ID) + require.FileExists(t, eventDBPath) + + cookies := authenticatedCookies( + t, sess, deleteTestUserID, deleteTestUsername, + ) + + req := postRequest( + "/source/"+wh.ID+"/delete", + cookies, + map[string]string{paramSourceID: wh.ID}, + ) + w := httptest.NewRecorder() + + h.HandleSourceDelete().ServeHTTP(w, req) + + require.Equal(t, http.StatusSeeOther, w.Code) + assert.Equal(t, "/sources", w.Header().Get("Location")) + + assert.Equal( + t, int64(0), + countRows(t, db, &database.Webhook{}, "id", wh.ID), + ) + assert.Equal( + t, int64(0), + countRows( + t, db, &database.Entrypoint{}, "webhook_id", wh.ID, + ), + ) + assert.Equal( + t, int64(0), + countRows( + t, db, &database.Target{}, "webhook_id", wh.ID, + ), + ) + assert.NoFileExists( + t, eventDBPath, + "a successful deletion removes the event database", + ) +} + // TestHandleTargetDelete_EvictsWhenLastDatabaseTargetGone // proves that removing the last database target releases the // archive writer. diff --git a/internal/handlers/source_management.go b/internal/handlers/source_management.go index e375f97..e01adf9 100644 --- a/internal/handlers/source_management.go +++ b/internal/handlers/source_management.go @@ -625,43 +625,27 @@ func (h *Handlers) deleteWebhookResources( webhook database.Webhook, userID string, ) { - tx := h.db.DB().Begin() - if tx.Error != nil { - h.log.Error( - "failed to begin transaction", - "error", tx.Error, - ) - http.Error( - w, "Internal server error", - http.StatusInternalServerError, - ) - - return - } - - tx.Where( - "webhook_id = ?", webhook.ID, - ).Delete(&database.Entrypoint{}) - - tx.Where( - "webhook_id = ?", webhook.ID, - ).Delete(&database.Target{}) - - tx.Delete(&webhook) - - err := tx.Commit().Error + // The configuration delete commits before the event database + // is touched. No transaction spans the main database and the + // filesystem, so one side has to go first: committing the + // configuration first means a later failure leaves an unused + // event database file on disk, while removing the event + // database first would mean a failed commit destroys the + // history of a webhook that still exists. A leftover file can + // be removed by hand; deleted history cannot be recovered. + err := h.commitWebhookDeletion(&webhook) if err != nil { - h.log.Error( - "failed to commit deletion", "error", err, - ) - http.Error( - w, "Internal server error", - http.StatusInternalServerError, - ) + h.serverError(w, "failed to delete webhook", err) return } + h.log.Info( + "webhook deleted", + "webhook_id", webhook.ID, + "user_id", userID, + ) + // Release the delivery engine's per-webhook archiving state // so a deleted webhook's archive writer (and any handle open // within its debounce window) does not linger for the @@ -671,22 +655,63 @@ func (h *Handlers) deleteWebhookResources( err = h.dbMgr.DeleteDB(webhook.ID) if err != nil { - h.log.Error( - "failed to delete webhook event database", - "webhook_id", webhook.ID, - "error", err, + // The configuration is committed, so the webhook is gone, + // but its event database file is still on disk with + // nothing referencing it. Report the failure rather than + // redirecting as though everything succeeded: the file + // needs removing by hand, and the logged error names it. + h.serverError( + w, "failed to delete webhook event database", err, ) + + return } - h.log.Info( - "webhook deleted", - "webhook_id", webhook.ID, - "user_id", userID, - ) - http.Redirect(w, r, "/sources", http.StatusSeeOther) } +// commitWebhookDeletion soft-deletes a webhook's entrypoints, +// targets and the webhook row in one transaction. Every +// statement is checked and any failure rolls the whole +// transaction back, so a caller that gets an error knows the +// configuration is untouched and the event database must be +// left alone. +func (h *Handlers) commitWebhookDeletion( + webhook *database.Webhook, +) error { + tx := h.db.DB().Begin() + if tx.Error != nil { + return tx.Error + } + + err := tx.Where( + "webhook_id = ?", webhook.ID, + ).Delete(&database.Entrypoint{}).Error + if err != nil { + tx.Rollback() + + return err + } + + err = tx.Where( + "webhook_id = ?", webhook.ID, + ).Delete(&database.Target{}).Error + if err != nil { + tx.Rollback() + + return err + } + + err = tx.Delete(webhook).Error + if err != nil { + tx.Rollback() + + return err + } + + return tx.Commit().Error +} + // evictArchiveWriter asks the delivery engine to drop its // cached archive writer for a webhook, closing the archive file // handle. -- 2.49.1