sneak/webhooker
1
0
Fork 0
Code Issues 62 Pull Requests 2 Actions Packages Projects Releases Wiki Activity

Webhook deletion commits a partial delete and reports success when a delete statement fails #262

New Issue
Closed
opened 2026-08-24 01:39:43 +02:00 by clawbot · 2 comments
clawbot commented 2026-08-24 01:39:43 +02:00
Collaborator
Copy Link

Found while auditing every Begin/Commit/Rollback path for #256. Not fixed there: it is a different failure and out of that issue's scope.

deleteWebhookResources (internal/handlers/source_management.go) opens a transaction and issues three deletes without checking any of them:

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

A failing delete sets .Error on the returned session and leaves the transaction usable, so the code commits whatever did succeed. The operator is redirected to /sources as though the deletion worked, and h.dbMgr.DeleteDB then hard-deletes the per-webhook event database anyway — its own error is logged and swallowed too. The outcome is a webhook whose entrypoints or targets survive in the main database while its entire event history is permanently gone, reported as a success.

commitWebhook, twenty lines above in the same file, does check each statement and rolls back, so this reads as an oversight rather than a decision.

Definition of done: check each delete, roll back and return a 500 on any failure, and do not delete the event database unless the configuration delete committed.

Found while auditing every `Begin`/`Commit`/`Rollback` path for https://git.eeqj.de/sneak/webhooker/issues/256. Not fixed there: it is a different failure and out of that issue's scope. `deleteWebhookResources` (`internal/handlers/source_management.go`) opens a transaction and issues three deletes without checking any of them: ```go 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 ``` A failing delete sets `.Error` on the returned session and leaves the transaction usable, so the code commits whatever did succeed. The operator is redirected to `/sources` as though the deletion worked, and `h.dbMgr.DeleteDB` then **hard-deletes the per-webhook event database anyway** — its own error is logged and swallowed too. The outcome is a webhook whose entrypoints or targets survive in the main database while its entire event history is permanently gone, reported as a success. `commitWebhook`, twenty lines above in the same file, does check each statement and rolls back, so this reads as an oversight rather than a decision. Definition of done: check each delete, roll back and return a 500 on any failure, and do not delete the event database unless the configuration delete committed.
clawbot added this to the 1.0.0 milestone 2026-08-24 01:44:21 +02:00
clawbot commented 2026-08-24 02:07:39 +02:00
Author
Collaborator
Copy Link

Plan:

  • Extract the deletion transaction into commitWebhookDeletion, matching commitWebhook's idiom in the same file: check every statement, tx.Rollback() and return the error on any failure.
  • deleteWebhookResources returns h.serverError on that error and does not touch the event database, so a failed configuration delete loses nothing.
  • Ordering choice for the window that has no transaction spanning SQLite and the filesystem: commit the configuration first, then DeleteDB. If DeleteDB then fails the webhook is gone and an unreferenced event database file is left on disk — reported to the operator with h.serverError rather than a success redirect, since a leftover file can be removed by hand while deleted history cannot be recovered.
  • Tests: a failing delete statement (injected on the targets table, which leaves the transaction usable exactly as a real database error does) must leave the webhook, entrypoint, target and event database file all intact and return 500; plus a positive control that a normal deletion still removes all four.
  • Audit: the repo has three Begin() sites. commitWebhook and createAndFanOut (internal/handlers/webhook.go) both check every statement and roll back; this was the only one that did not.
Plan: - Extract the deletion transaction into `commitWebhookDeletion`, matching `commitWebhook`'s idiom in the same file: check every statement, `tx.Rollback()` and return the error on any failure. - `deleteWebhookResources` returns `h.serverError` on that error and does not touch the event database, so a failed configuration delete loses nothing. - Ordering choice for the window that has no transaction spanning SQLite and the filesystem: commit the configuration first, then `DeleteDB`. If `DeleteDB` then fails the webhook is gone and an unreferenced event database file is left on disk — reported to the operator with `h.serverError` rather than a success redirect, since a leftover file can be removed by hand while deleted history cannot be recovered. - Tests: a failing delete statement (injected on the `targets` table, which leaves the transaction usable exactly as a real database error does) must leave the webhook, entrypoint, target and event database file all intact and return 500; plus a positive control that a normal deletion still removes all four. - Audit: the repo has three `Begin()` sites. `commitWebhook` and `createAndFanOut` (`internal/handlers/webhook.go`) both check every statement and roll back; this was the only one that did not.
clawbot commented 2026-08-24 02:16:49 +02:00
Author
Collaborator
Copy Link

Implemented in #273.

commitWebhookDeletion now checks each of the three deletes and rolls back on any failure; deleteWebhookResources reports it with h.serverError and does not touch the event database. The configuration commit runs before DeleteDB, and a DeleteDB failure is reported with h.serverError rather than the old success redirect, so the worst case is a leftover event database file the operator is told about instead of history destroyed behind a 303.

Verified: make check green with GOFLAGS=-count=1, lint 0 issues. A new test injects a failure into the targets delete (.Error set, transaction still usable — the exact reported condition) and asserts a 500 with the webhook, entrypoint, target and event database file all intact; with only the handler reverted it fails on every one of those assertions, including unable to find file .../events-<id>.db. A positive-control test covers normal deletion, and a running instance was driven through create-target-delete end to end: 303 to /sources, webhook delisted, detail 404, event database file gone.

Audit: the repo has three Begin() sites; commitWebhook and createAndFanOut both check every statement, so this was the only one.

Implemented in https://git.eeqj.de/sneak/webhooker/pulls/273. `commitWebhookDeletion` now checks each of the three deletes and rolls back on any failure; `deleteWebhookResources` reports it with `h.serverError` and does not touch the event database. The configuration commit runs before `DeleteDB`, and a `DeleteDB` failure is reported with `h.serverError` rather than the old success redirect, so the worst case is a leftover event database file the operator is told about instead of history destroyed behind a 303. Verified: `make check` green with `GOFLAGS=-count=1`, lint 0 issues. A new test injects a failure into the `targets` delete (`.Error` set, transaction still usable — the exact reported condition) and asserts a 500 with the webhook, entrypoint, target and event database file all intact; with only the handler reverted it fails on every one of those assertions, including `unable to find file .../events-<id>.db`. A positive-control test covers normal deletion, and a running instance was driven through create-target-delete end to end: 303 to `/sources`, webhook delisted, detail 404, event database file gone. Audit: the repo has three `Begin()` sites; `commitWebhook` and `createAndFanOut` both check every statement, so this was the only one.
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
Labels
Clear labels
merge-ready
needs-checks
needs-rebase
needs-review
needs-rework
No Label
Milestone
No items
No Milestone 1.0.0
Projects
Clear projects
No project
Assignees
Clear assignees
clawbot sneak
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: sneak/webhooker#262
Delete Branch "%!s()"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?