diff --git a/internal/handlers/event_resubmit.go b/internal/handlers/event_resubmit.go index 63b51c4..fba7106 100644 --- a/internal/handlers/event_resubmit.go +++ b/internal/handlers/event_resubmit.go @@ -145,8 +145,9 @@ func (h *Handlers) resubmitEvent( // per-webhook database files — a sibling webhook's event is not in the // database being queried at all — and is there so the scoping survives // any future change that puts more than one webhook's events in one -// file. Going through Model applies GORM's soft-delete scope, which is -// what stops a reaped event being resubmitted. +// file. A reaped event is not found because the retention reaper +// deletes its row outright rather than marking it deleted; see +// deleteEvents in internal/database/retention.go. func loadResubmitSource( webhookDB *gorm.DB, webhookID, eventID string, diff --git a/internal/handlers/webhook.go b/internal/handlers/webhook.go index e913057..23c65ec 100644 --- a/internal/handlers/webhook.go +++ b/internal/handlers/webhook.go @@ -272,10 +272,12 @@ func requestEventSource( // createAndFanOut writes the event and one pending delivery per target, // and adds them to the webhook's running totals, in a single -// transaction, then hands the tasks to the delivery engine. It is the -// only path by which an event and its deliveries are created, so a -// resubmitted event is retried, SSRF-guarded and circuit-broken -// exactly as a received one is. +// transaction, then hands the tasks to the delivery engine. Every +// event is created here, received or resubmitted, so a resubmitted +// event is retried, SSRF-guarded and circuit-broken exactly as a +// received one is. Per-delivery replay is the one other path that +// creates a delivery: it adds one to an existing event without +// coming through here. // // The tasks are returned as well as queued, so a caller can report how // many targets the event went to. diff --git a/internal/server/resubmit_test.go b/internal/server/resubmit_test.go new file mode 100644 index 0000000..a5ed43a --- /dev/null +++ b/internal/server/resubmit_test.go @@ -0,0 +1,215 @@ +package server_test + +import ( + "net/http" + "net/url" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// maxResubmits bounds the requests the tests below send to the +// resubmit route. The route's rate limit belongs to the middleware; +// this only has to sit well above it, so that a route without the +// limiter fails its test instead of looping. +const maxResubmits = 100 + +// resubmitPath is the resubmit route for one stored event. +func resubmitPath(webhookID, eventID string) string { + return "/hook/" + webhookID + "/events/" + eventID + "/resubmit" +} + +// csrfForm is a resubmit form carrying the given CSRF token. +func csrfForm(token string) url.Values { + form := url.Values{} + form.Set("csrf_token", token) + + return form +} + +// TestEventResubmit_SignedOutRequestsNeverReachTheRateLimit pins +// RequireAuth on the resubmit route. The handler also turns away a +// request without a session, with the same redirect, so a refusal +// alone would pass without RequireAuth. What RequireAuth adds is that +// it refuses such a request before the route's rate limit, so a +// signed-out client cannot spend the budget a signed-in user +// resubmits from. Each request carries a CSRF token valid for its own +// cookie, so CSRF lets it through to RequireAuth. +func TestEventResubmit_SignedOutRequestsNeverReachTheRateLimit( + t *testing.T, +) { + t.Parallel() + + env := newTestEnv(t) + + userID, _ := env.seedUser(t, "resubmitter", "somepassword") + wh := env.seedWebhook(t, userID) + evt := env.seedEvent(t, wh.ID, `{"resubmit":"me"}`) + path := resubmitPath(wh.ID, evt.ID) + logsPath := "/hook/" + wh.ID + "/events" + + token, signedOut := env.csrfFrom(t, "/pages/login", nil) + + for i := range maxResubmits { + w := env.post(path, csrfForm(token), signedOut) + require.Equal(t, http.StatusSeeOther, w.Code, "request %d", i) + require.Equal( + t, "/pages/login", w.Header().Get("Location"), + "request %d", i, + ) + } + + require.Equal( + t, int64(1), env.countEvents(t, wh.ID), + "a signed-out request must store nothing", + ) + + token, cookies := env.csrfFrom( + t, logsPath, env.authCookies(t, userID, "resubmitter"), + ) + + env.requireNotice( + t, env.post(path, csrfForm(token), cookies), + logsPath, "resubmit-no-targets", + "this source has no active targets", cookies, + ) +} + +// TestEventResubmit_RefusedWithoutAValidCSRFToken pins CSRF on the +// resubmit route: a signed-in user's POST is refused with 403, and +// stores nothing, unless it carries the token issued to that user's +// own browser. +func TestEventResubmit_RefusedWithoutAValidCSRFToken(t *testing.T) { + t.Parallel() + + env := newTestEnv(t) + + userID, _ := env.seedUser(t, "resubmitter", "somepassword") + wh := env.seedWebhook(t, userID) + evt := env.seedEvent(t, wh.ID, `{"resubmit":"me"}`) + path := resubmitPath(wh.ID, evt.ID) + logsPath := "/hook/" + wh.ID + "/events" + + token, cookies := env.csrfFrom( + t, logsPath, env.authCookies(t, userID, "resubmitter"), + ) + otherBrowsers, _ := env.csrfFrom(t, "/pages/login", nil) + + for name, form := range map[string]url.Values{ + "no token": {}, + "a malformed token": csrfForm("not-a-token"), + "another browser's token": csrfForm(otherBrowsers), + } { + assert.Equal( + t, http.StatusForbidden, + env.post(path, form, cookies).Code, name, + ) + } + + assert.Equal( + t, int64(1), env.countEvents(t, wh.ID), + "a refused request must store nothing", + ) + + // The same request with the user's own token goes through, so the + // refusals above were the token's doing. + env.requireNotice( + t, env.post(path, csrfForm(token), cookies), + logsPath, "resubmit-no-targets", + "this source has no active targets", cookies, + ) +} + +// TestEventResubmit_AnotherWebhooksEvent404s pins, on the route as +// registered, that a signed-in user gets 404, and nothing is stored, +// for an event of a webhook another user owns, which the handler's +// ownership check refuses, and for another webhook's event posted +// under a webhook the user does own, which the event lookup refuses. +// The user's own event, posted the same way, is accepted, so the +// second 404 comes from the lookup and not from a route that never +// passed the event ID to the handler. +func TestEventResubmit_AnotherWebhooksEvent404s(t *testing.T) { + t.Parallel() + + env := newTestEnv(t) + + ownerID, _ := env.seedUser(t, "owner", "somepassword") + owners := env.seedWebhook(t, ownerID) + ownersEvent := env.seedEvent(t, owners.ID, `{"owner":"only"}`) + + intruderID, _ := env.seedUser(t, "intruder", "somepassword") + intruders := env.seedWebhook(t, intruderID) + intrudersEvent := env.seedEvent( + t, intruders.ID, `{"intruder":"own"}`, + ) + intrudersLogs := "/hook/" + intruders.ID + "/events" + + token, cookies := env.csrfFrom( + t, intrudersLogs, env.authCookies(t, intruderID, "intruder"), + ) + + for name, path := range map[string]string{ + "another user's webhook": resubmitPath( + owners.ID, ownersEvent.ID, + ), + "another webhook's event": resubmitPath( + intruders.ID, ownersEvent.ID, + ), + } { + w := env.post(path, csrfForm(token), cookies) + assert.Equal(t, http.StatusNotFound, w.Code, name) + } + + assert.Equal(t, int64(1), env.countEvents(t, owners.ID)) + assert.Equal(t, int64(1), env.countEvents(t, intruders.ID)) + + env.requireNotice( + t, + env.post( + resubmitPath(intruders.ID, intrudersEvent.ID), + csrfForm(token), cookies, + ), + intrudersLogs, "resubmit-no-targets", + "this source has no active targets", cookies, + ) +} + +// TestEventResubmit_RateLimited pins the rate limit on the resubmit +// route: a signed-in user's resubmits are accepted until the budget +// is spent, and then refused with 429. +func TestEventResubmit_RateLimited(t *testing.T) { + t.Parallel() + + env := newTestEnv(t) + + userID, _ := env.seedUser(t, "resubmitter", "somepassword") + wh := env.seedWebhook(t, userID) + evt := env.seedEvent(t, wh.ID, `{"resubmit":"me"}`) + path := resubmitPath(wh.ID, evt.ID) + + token, cookies := env.csrfFrom( + t, "/hook/"+wh.ID+"/events", + env.authCookies(t, userID, "resubmitter"), + ) + + limited := false + + for range maxResubmits { + code := env.post(path, csrfForm(token), cookies).Code + if code == http.StatusTooManyRequests { + limited = true + + break + } + + require.Equal( + t, http.StatusSeeOther, code, + "a resubmit within the budget must be accepted", + ) + } + + assert.True( + t, limited, "repeated resubmits must eventually be refused", + ) +} diff --git a/internal/server/routes_test.go b/internal/server/routes_test.go index f9327c2..4ad98bc 100644 --- a/internal/server/routes_test.go +++ b/internal/server/routes_test.go @@ -444,6 +444,23 @@ func (e *testEnv) countDeliveries( return count } +// countEvents reports how many events a webhook's database holds. +func (e *testEnv) countEvents(t *testing.T, webhookID string) int64 { + t.Helper() + + webhookDB, err := e.dbMgr.GetDB(webhookID) + require.NoError(t, err) + + var count int64 + + require.NoError( + t, + webhookDB.Model(&database.Event{}).Count(&count).Error, + ) + + return count +} + // storedHash reads the current password hash for a username. func (e *testEnv) storedHash(t *testing.T, username string) string { t.Helper()