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

Follow-ups from the PR 251 review: two comments state the wrong mechanism, and the resubmit route's middleware is untested #252

New Issue
Closed
opened 2026-08-24 00:53:49 +02:00 by clawbot · 2 comments
clawbot commented 2026-08-24 00:53:49 +02:00
Collaborator
Copy Link
Copy Source

Non-blocking follow-ups from the review of #251. The behaviour is correct in every case below; these are accuracy and coverage gaps.

  • loadResubmitSource credits GORM's soft-delete scope for refusing a reaped event, but the retention reaper hard-deletes — internal/database/retention.go:288 uses Unscoped(). Right behaviour, wrong stated mechanism, and a reader who trusts the comment will assume reaped events are still recoverable.
  • createAndFanOut's comment claims it is "the only path by which an event and its deliveries are created". Per-delivery replay creates a delivery without an event, so the claim overreaches.
  • The resubmit tests call HandleEventResubmit() directly, bypassing router middleware, so auth, CSRF, ownership and rate limiting have no CI coverage on this route. All four were verified live during review (unauthenticated 403, missing/garbage/foreign CSRF 403, foreign sourceID/eventID 404, rate limit 429 at ~30 with replay's bucket spending independently), but nothing pins them against regression.

Done when the two comments describe the actual mechanism and the route's middleware chain is covered by tests that go through the router.

Non-blocking follow-ups from the review of https://git.eeqj.de/sneak/webhooker/pulls/251. The behaviour is correct in every case below; these are accuracy and coverage gaps. - `loadResubmitSource` credits GORM's soft-delete scope for refusing a reaped event, but the retention reaper hard-deletes — `internal/database/retention.go:288` uses `Unscoped()`. Right behaviour, wrong stated mechanism, and a reader who trusts the comment will assume reaped events are still recoverable. - `createAndFanOut`'s comment claims it is "the only path by which an event and its deliveries are created". Per-delivery replay creates a delivery without an event, so the claim overreaches. - The resubmit tests call `HandleEventResubmit()` directly, bypassing router middleware, so auth, CSRF, ownership and rate limiting have no CI coverage on this route. All four were verified live during review (unauthenticated 403, missing/garbage/foreign CSRF 403, foreign `sourceID`/`eventID` 404, rate limit 429 at ~30 with replay's bucket spending independently), but nothing pins them against regression. Done when the two comments describe the actual mechanism and the route's middleware chain is covered by tests that go through the router.
clawbot commented 2026-10-02 18:04:01 +02:00
Author
Collaborator
Copy Link
Copy Source

Plan. Check each item against next first (routes are now under /hook/ID).

  • The comment in loadResubmitSource names the real reason a reaped event is refused: the retention reaper deletes it outright.
  • The comment on createAndFanOut no longer claims to be the only path that creates deliveries (per-delivery replay creates one without an event).
  • Tests through the production router pin the resubmit route's middleware: refused without a session, refused without a valid CSRF token, 404 for another webhook's event, and the rate limit refusing past its budget. Each fails when its middleware is removed from the route.

Model: opus-5-5

Plan. Check each item against `next` first (routes are now under `/hook/ID`). - The comment in `loadResubmitSource` names the real reason a reaped event is refused: the retention reaper deletes it outright. - The comment on `createAndFanOut` no longer claims to be the only path that creates deliveries (per-delivery replay creates one without an event). - Tests through the production router pin the resubmit route's middleware: refused without a session, refused without a valid CSRF token, 404 for another webhook's event, and the rate limit refusing past its budget. Each fails when its middleware is removed from the route. Model: opus-5-5
clawbot commented 2026-10-02 18:23:50 +02:00
Author
Collaborator
Copy Link
Copy Source

Done in #457.

  • The comment in loadResubmitSource now says a reaped event is gone because the retention reaper deletes its row outright.
  • The comment on createAndFanOut no longer claims to be the only path that creates deliveries; it names per-delivery replay as the other one.
  • New tests drive the resubmit route through the production router for CSRF, ownership, the rate limit and signed-out requests. Each failed with its check removed.

Disclosures:

  • Judgement call: the handler refuses a signed-out request with the same redirect RequireAuth gives, so the RequireAuth test pins that signed-out requests never spend the rate limit, not the refusal itself.
  • Judgement call: no route change can make the other-webhook's-event case fail, since that event lives in another webhook's database file; it stays as a pin on the URL wiring.
  • The same wrong mechanism in the eventBodyQuery comment is filed separately as #455.

Model: opus-5-5

Done in https://git.eeqj.de/sneak/webhooker/pulls/457. - The comment in `loadResubmitSource` now says a reaped event is gone because the retention reaper deletes its row outright. - The comment on `createAndFanOut` no longer claims to be the only path that creates deliveries; it names per-delivery replay as the other one. - New tests drive the resubmit route through the production router for CSRF, ownership, the rate limit and signed-out requests. Each failed with its check removed. Disclosures: - Judgement call: the handler refuses a signed-out request with the same redirect RequireAuth gives, so the RequireAuth test pins that signed-out requests never spend the rate limit, not the refusal itself. - Judgement call: no route change can make the other-webhook's-event case fail, since that event lives in another webhook's database file; it stays as a pin on the URL wiring. - The same wrong mechanism in the `eventBodyQuery` comment is filed separately as https://git.eeqj.de/sneak/webhooker/issues/455. Model: opus-5-5
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
Labels
Clear labels
critical
Extremely important: data loss, a security hole, production down, or basic use impossible
 merge-ready
needs-checks
needs-rebase
needs-review
needs-rework
No labels
Milestone
No items
No Milestone
Projects
Clear projects
No projects
Assignees
Clear assignees
clawbot sneak (Jeffrey Paul)
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: sneak/webhooker#252