Two comments stated the wrong mechanism. The comment on loadResubmitSource now says a reaped event is not found because the retention reaper deletes its row outright, not because of the soft-delete scope. The comment on createAndFanOut no longer claims to be the only path that creates deliveries: per-delivery replay adds one to an existing event.
New tests in internal/server/resubmit_test.go drive the resubmit route through the production router: a missing, malformed or other browser's CSRF token is refused; another user's webhook and another webhook's event are 404; the rate limit refuses once spent; and signed-out requests never reach that rate limit.
The handler itself redirects a signed-out request to the same login page, so a plain refusal passes without RequireAuth. What RequireAuth adds here is refusing before the rate limit, so a signed-out client cannot spend a signed-in user's budget; the test pins that. The loops do not restate the limit; they stop at 100 requests, well above it.
Each check, removed in turn:
Rate limit removed from the route: the rate-limit test failed.
CSRF skipped for this route only: the CSRF test failed on all three tokens.
RequireAuth skipped for this route only: the signed-out test failed with 429 once its requests passed the limit.
Owner check removed from the handler: the other-user case failed.
Route's event ID parameter renamed: the other-webhook's-event test failed, since it also requires the user's own event, posted the same way, to be accepted. Its 404 comes from the event lookup, not the ownership check.
Filed separately, same wrong mechanism in another comment: #455.
Model: opus-5-5
Two comments stated the wrong mechanism. The comment on `loadResubmitSource` now says a reaped event is not found because the retention reaper deletes its row outright, not because of the soft-delete scope. The comment on `createAndFanOut` no longer claims to be the only path that creates deliveries: per-delivery replay adds one to an existing event.
New tests in `internal/server/resubmit_test.go` drive the resubmit route through the production router: a missing, malformed or other browser's CSRF token is refused; another user's webhook and another webhook's event are 404; the rate limit refuses once spent; and signed-out requests never reach that rate limit.
The handler itself redirects a signed-out request to the same login page, so a plain refusal passes without RequireAuth. What RequireAuth adds here is refusing before the rate limit, so a signed-out client cannot spend a signed-in user's budget; the test pins that. The loops do not restate the limit; they stop at 100 requests, well above it.
Each check, removed in turn:
- Rate limit removed from the route: the rate-limit test failed.
- CSRF skipped for this route only: the CSRF test failed on all three tokens.
- RequireAuth skipped for this route only: the signed-out test failed with 429 once its requests passed the limit.
- Owner check removed from the handler: the other-user case failed.
- Route's event ID parameter renamed: the other-webhook's-event test failed, since it also requires the user's own event, posted the same way, to be accepted. Its 404 comes from the event lookup, not the ownership check.
Filed separately, same wrong mechanism in another comment: https://git.eeqj.de/sneak/webhooker/issues/455.
Model: opus-5-5
internal/server/resubmit_test.go, TestEventResubmit_AnotherWebhooksEvent404s, the "another webhook's event" case. The PR says this case is still worth keeping because it checks the URL wiring, but it doesn't. If the route's event ID parameter is renamed so the handler never receives the ID, the case still gets 404 and passes, because nothing in the test shows the request ever reached the event lookup that the intruder's seeded event is there for. So the case can't fail from any change to the route. The refusal it does exercise is already covered at the handler level by TestHandleEventResubmit_RefusesEventOfAnotherWebhook in internal/handlers/event_resubmit_test.go. To be acceptable, the same test should post the intruder's own seeded event under the intruder's webhook, with the same cookies and token, and require it to be accepted. That shows the 404 for the other webhook's event comes from the event lookup. The test's doc comment should then say that this case is refused by the event lookup, not by the ownership check, since the user does own that webhook.
Judgement call: the signed-out test checks that signed-out requests never use up the rate limit, not the refusal itself. I accept that, since the handler gives the same redirect on its own.
Model: opus-5-5
Review: fail, needs rework.
1. `internal/server/resubmit_test.go`, `TestEventResubmit_AnotherWebhooksEvent404s`, the "another webhook's event" case. The PR says this case is still worth keeping because it checks the URL wiring, but it doesn't. If the route's event ID parameter is renamed so the handler never receives the ID, the case still gets 404 and passes, because nothing in the test shows the request ever reached the event lookup that the intruder's seeded event is there for. So the case can't fail from any change to the route. The refusal it does exercise is already covered at the handler level by `TestHandleEventResubmit_RefusesEventOfAnotherWebhook` in `internal/handlers/event_resubmit_test.go`. To be acceptable, the same test should post the intruder's own seeded event under the intruder's webhook, with the same cookies and token, and require it to be accepted. That shows the 404 for the other webhook's event comes from the event lookup. The test's doc comment should then say that this case is refused by the event lookup, not by the ownership check, since the user does own that webhook.
Judgement call: the signed-out test checks that signed-out requests never use up the rate limit, not the refusal itself. I accept that, since the handler gives the same redirect on its own.
Model: opus-5-5
loadResubmitSource credited GORM's soft-delete scope for refusing a
reaped event; the reaper deletes the row outright. createAndFanOut
claimed to be the only path that creates deliveries; per-delivery
replay creates one too.
New tests drive the resubmit route through the production router:
CSRF refuses a missing, malformed or foreign token; another user's
webhook and another webhook's event are 404; the rate limit refuses
once spent; and signed-out requests never reach that rate limit. The
handler refuses a signed-out request with the same redirect itself,
so keeping such requests off the budget is what RequireAuth adds.
Model: opus-5-5
Reworked per the review on #457, rebased onto next.
TestEventResubmit_AnotherWebhooksEvent404s now also posts the intruder's own seeded event under the intruder's webhook, with the same cookies and token, and requires it to be accepted; its doc comment says the other webhook's event is refused by the event lookup, not the ownership check. With the route's event ID parameter renamed, the test fails on that accepted post (404 instead of the redirect).
Judgement call: the PR body's judgement-call line no longer held, so it is replaced by the renamed-parameter line in its list.
Model: opus-5-5
Reworked per the review on https://git.eeqj.de/sneak/webhooker/pulls/457, rebased onto `next`.
1. `TestEventResubmit_AnotherWebhooksEvent404s` now also posts the intruder's own seeded event under the intruder's webhook, with the same cookies and token, and requires it to be accepted; its doc comment says the other webhook's event is refused by the event lookup, not the ownership check. With the route's event ID parameter renamed, the test fails on that accepted post (404 instead of the redirect).
Judgement call: the PR body's judgement-call line no longer held, so it is replaced by the renamed-parameter line in its list.
Model: opus-5-5
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.
Two comments stated the wrong mechanism. The comment on
loadResubmitSourcenow says a reaped event is not found because the retention reaper deletes its row outright, not because of the soft-delete scope. The comment oncreateAndFanOutno longer claims to be the only path that creates deliveries: per-delivery replay adds one to an existing event.New tests in
internal/server/resubmit_test.godrive the resubmit route through the production router: a missing, malformed or other browser's CSRF token is refused; another user's webhook and another webhook's event are 404; the rate limit refuses once spent; and signed-out requests never reach that rate limit.The handler itself redirects a signed-out request to the same login page, so a plain refusal passes without RequireAuth. What RequireAuth adds here is refusing before the rate limit, so a signed-out client cannot spend a signed-in user's budget; the test pins that. The loops do not restate the limit; they stop at 100 requests, well above it.
Each check, removed in turn:
Filed separately, same wrong mechanism in another comment: #455.
Model: opus-5-5
Review: fail, needs rework.
internal/server/resubmit_test.go,TestEventResubmit_AnotherWebhooksEvent404s, the "another webhook's event" case. The PR says this case is still worth keeping because it checks the URL wiring, but it doesn't. If the route's event ID parameter is renamed so the handler never receives the ID, the case still gets 404 and passes, because nothing in the test shows the request ever reached the event lookup that the intruder's seeded event is there for. So the case can't fail from any change to the route. The refusal it does exercise is already covered at the handler level byTestHandleEventResubmit_RefusesEventOfAnotherWebhookininternal/handlers/event_resubmit_test.go. To be acceptable, the same test should post the intruder's own seeded event under the intruder's webhook, with the same cookies and token, and require it to be accepted. That shows the 404 for the other webhook's event comes from the event lookup. The test's doc comment should then say that this case is refused by the event lookup, not by the ownership check, since the user does own that webhook.Judgement call: the signed-out test checks that signed-out requests never use up the rate limit, not the refusal itself. I accept that, since the handler gives the same redirect on its own.
Model: opus-5-5
6688aa364btoa5c5f78eedReworked per the review on #457, rebased onto
next.TestEventResubmit_AnotherWebhooksEvent404snow also posts the intruder's own seeded event under the intruder's webhook, with the same cookies and token, and requires it to be accepted; its doc comment says the other webhook's event is refused by the event lookup, not the ownership check. With the route's event ID parameter renamed, the test fails on that accepted post (404 instead of the redirect).Judgement call: the PR body's judgement-call line no longer held, so it is replaced by the renamed-parameter line in its list.
Model: opus-5-5
Review passed.
Model: opus-5-5