diff --git a/internal/handlers/delivery_replay.go b/internal/handlers/delivery_replay.go index 51b7e3b..ee9425b 100644 --- a/internal/handlers/delivery_replay.go +++ b/internal/handlers/delivery_replay.go @@ -368,7 +368,7 @@ func (h *Handlers) finishReplay( // The page is read from the form rather than the query string: // this is a POST, and its query string is what logs and Referer // headers record. - if page := parseNonNegativeInt( + if page := pageOrFirst( r.PostFormValue("page"), ); page > 1 { dest += "&page=" + strconv.Itoa(page) diff --git a/internal/handlers/event_resubmit.go b/internal/handlers/event_resubmit.go index d1b677b..0d898b5 100644 --- a/internal/handlers/event_resubmit.go +++ b/internal/handlers/event_resubmit.go @@ -267,7 +267,7 @@ func (h *Handlers) finishResubmit( // The page is read from the form rather than the query string: // this is a POST, and its query string is what logs and Referer // headers record. - if page := parseNonNegativeInt( + if page := pageOrFirst( r.PostFormValue("page"), ); page > 1 { dest += "&page=" + strconv.Itoa(page) diff --git a/internal/handlers/export_test.go b/internal/handlers/export_test.go index 75b4fe2..4612e94 100644 --- a/internal/handlers/export_test.go +++ b/internal/handlers/export_test.go @@ -27,6 +27,17 @@ const MaxRenderedResponseBytesForTest = maxRenderedResponseBytes // per-delivery attempt ceiling to the handlers_test package. const MaxRenderedAttemptsForTest = maxRenderedAttempts +// MaxTargetRetriesForTest exposes the target max_retries ceiling to +// the handlers_test package, so the tests assert against the constant +// the handlers enforce rather than a number copied beside it. +const MaxTargetRetriesForTest = maxTargetRetries + +// PageOrFirstForTest exposes pageOrFirst for use in the handlers_test +// package. +func PageOrFirstForTest(s string) int { + return pageOrFirst(s) +} + // DummyVerificationsForTest reports how many equivalent-cost // verifications were charged for usernames that do not exist. It // lets a test prove the anti-enumeration path ran without timing diff --git a/internal/handlers/source_management.go b/internal/handlers/source_management.go index a48a2bc..b83e843 100644 --- a/internal/handlers/source_management.go +++ b/internal/handlers/source_management.go @@ -905,16 +905,7 @@ func (h *Handlers) loadTargetMap( // parsePage extracts a page number from the query string. func (h *Handlers) parsePage(r *http.Request) int { - page := 1 - - if p := r.URL.Query().Get("page"); p != "" { - v, err := strconv.Atoi(p) - if err == nil && v > 0 { - page = v - } - } - - return page + return pageOrFirst(r.URL.Query().Get("page")) } // loadEventsWithDeliveries loads paginated events and their @@ -1443,7 +1434,6 @@ func (h *Handlers) processTargetCreate( // Referer headers and error trackers record. name := r.PostFormValue("name") targetType := database.TargetType(r.PostFormValue("type")) - maxRetriesStr := r.PostFormValue("max_retries") if name == "" { http.Error( @@ -1469,7 +1459,14 @@ func (h *Handlers) processTargetCreate( return } - maxRetries := parseNonNegativeInt(maxRetriesStr) + // A new target has no stored retry count, so an absent field + // takes the fire-and-forget default. A field the operator filled + // in with something invalid is rejected rather than becoming + // that default. + maxRetries, ok := targetMaxRetries(w, r, 0) + if !ok { + return + } target := &database.Target{ WebhookID: webhook.ID, @@ -1505,19 +1502,22 @@ func isValidTargetType(tt database.TargetType) bool { } } -// parseNonNegativeInt parses s as a non-negative integer, -// returning 0 if s is empty or invalid. -func parseNonNegativeInt(s string) int { - if s == "" { - return 0 +// pageOrFirst parses a paginated page number, answering 1 for +// anything empty, unparseable or out of range. +// +// Falling back rather than rejecting is correct here and only here: +// a page number is where to send the browser next, not configuration +// the operator is storing, and the actions that submit one have +// already completed by the time it is read — answering 400 would +// report a failure that did not happen. Anything an operator SETS +// must be validated instead; see parseMaxRetries. +func pageOrFirst(s string) int { + v, err := strconv.Atoi(strings.TrimSpace(s)) + if err != nil || v < 1 { + return 1 } - v, err := strconv.Atoi(s) - if err == nil && v >= 0 { - return v - } - - return 0 + return v } // targetFormInput carries the raw form values describing a target's diff --git a/internal/handlers/target_edit.go b/internal/handlers/target_edit.go index d8ec6ee..8ff03b5 100644 --- a/internal/handlers/target_edit.go +++ b/internal/handlers/target_edit.go @@ -133,20 +133,28 @@ func (h *Handlers) applyTargetEdit( return } - target.Name = name - target.Config = configJSON - // Retries are offered only by the forms for target types that // retry, so an absent field means "this form does not edit // retries" rather than "set them to zero". Reading it // unconditionally would silently disable retries on any target // saved from a form that does not render the input. + // + // A field that IS submitted but does not parse is a 400, through + // the same validator the create path uses. It is rejected before + // anything is written, so a typo cannot destroy the retry count + // the target is already delivering with. if r.PostForm.Has("max_retries") { - target.MaxRetries = parseNonNegativeInt( - r.PostFormValue("max_retries"), - ) + retries, ok := targetMaxRetries(w, r, target.MaxRetries) + if !ok { + return + } + + target.MaxRetries = retries } + target.Name = name + target.Config = configJSON + err = h.db.DB().Save(target).Error if err != nil { h.serverError(w, "failed to update target", err) diff --git a/internal/handlers/target_retries.go b/internal/handlers/target_retries.go new file mode 100644 index 0000000..9f571c5 --- /dev/null +++ b/internal/handlers/target_retries.go @@ -0,0 +1,119 @@ +package handlers + +import ( + "errors" + "net/http" + "strconv" + "strings" +) + +// maxTargetRetries bounds a target's max_retries. +// +// Both target forms already declare max="20" on the input, so this +// enforces server-side what the UI has always advertised rather than +// introducing a new limit. +// +// The number is not cosmetic. Every attempt writes a delivery_results +// row that the event log then loads and renders, and the engine backs +// off by 2^(n-1) seconds, so attempt 20 is already about six days +// after the first. A value beyond this buys no additional durability +// and only costs rows. +const maxTargetRetries = 20 + +// Errors returned when a max_retries form value cannot be turned into +// a retry count. +var ( + // errRetriesInvalid signals a max_retries form value that is not + // a non-negative whole number. + errRetriesInvalid = errors.New( + "retries must be a whole number of attempts", + ) + + // errRetriesTooLarge signals a max_retries form value that is a + // whole number but above maxTargetRetries. It is distinguished + // from errRetriesInvalid so the message can name the ceiling + // instead of implying the input was not a number. + errRetriesTooLarge = errors.New("retries out of range") +) + +// parseMaxRetries interprets a max_retries form value. +// +// An ABSENT value — the field empty or not submitted — yields +// fallback, which lets the create path apply its default and the edit +// path leave the stored value alone. A value that is SET BUT INVALID +// is an error: unparseable, negative, or above maxTargetRetries. +// +// The distinction is the whole point of this function. max_retries=0 +// means fire-and-forget, so returning 0 for input the operator typed +// but that did not parse silently disables retries on a +// store-and-forward proxy — and on the edit path it destroys a +// working retry configuration over a typo. A default answers a +// question that was not asked; it never answers one that was asked +// badly. +// +// A target stored with a count above the ceiling before this +// validation existed keeps rendering and keeps delivering — nothing +// clamps the row. Re-saving it from the edit form does have to bring +// it into range, because the form submits the pre-filled value back +// and accepting it would be the ceiling not applying to the edit +// path. The 400 names the ceiling, so the fix is one field. +func parseMaxRetries(raw string, fallback int) (int, error) { + raw = strings.TrimSpace(raw) + if raw == "" { + return fallback, nil + } + + v, err := strconv.Atoi(raw) + if err != nil || v < 0 { + return 0, errRetriesInvalid + } + + if v > maxTargetRetries { + return 0, errRetriesTooLarge + } + + return v, nil +} + +// retriesErrorMessage returns the message the create and edit forms +// show for a rejected max_retries value. Any error other than +// errRetriesTooLarge falls back to the generic wording, so an +// unrecognised parse failure still produces a sensible 400. +func retriesErrorMessage(err error) string { + if errors.Is(err, errRetriesTooLarge) { + return errRetriesTooLarge.Error() + + ": at most " + strconv.Itoa(maxTargetRetries) + + " retries" + } + + return errRetriesInvalid.Error() + + ", or 0 for fire-and-forget" +} + +// targetMaxRetries reads and validates max_retries from a target form +// submission, answering the request with a 400 and reporting false +// when the value is set but invalid. +// +// Both the create and the edit path go through here, so the two +// cannot come to disagree about what a valid retry count is. The +// wording matches the timeout control on the same submission. +func targetMaxRetries( + w http.ResponseWriter, + r *http.Request, + fallback int, +) (int, bool) { + retries, err := parseMaxRetries( + r.PostFormValue("max_retries"), fallback, + ) + if err != nil { + http.Error( + w, + "Invalid max retries: "+retriesErrorMessage(err), + http.StatusBadRequest, + ) + + return 0, false + } + + return retries, true +} diff --git a/internal/handlers/target_retries_test.go b/internal/handlers/target_retries_test.go new file mode 100644 index 0000000..51840c5 --- /dev/null +++ b/internal/handlers/target_retries_test.go @@ -0,0 +1,402 @@ +package handlers_test + +import ( + "net/http" + "net/url" + "strconv" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "sneak.berlin/go/webhooker/internal/database" + "sneak.berlin/go/webhooker/internal/handlers" +) + +// retriesTargetURL is the destination the retry-validation targets +// point at. It is a literal public address rather than a hostname so +// the SSRF check resolves nothing and a sandbox without DNS cannot +// make these cases pass or fail for the wrong reason. +const retriesTargetURL = "https://93.184.216.34/hooks/retries" + +const ( + // wayAboveCeiling is the typo'd-extra-zero case from the report. + wayAboveCeiling = "999999999" + + // notANumber is the plainest garbage an operator can type, and + // the value the report submitted on the edit form. + notANumber = "abc" + + // workingRetries is the retry count a seeded target is already + // delivering with, which a rejected submission must not disturb. + workingRetries = 2 +) + +// aboveCeiling is the smallest rejected whole number. +func aboveCeiling() string { + return strconv.Itoa(handlers.MaxTargetRetriesForTest + 1) +} + +// overCeilingRetries is whole-number input past the limit, which is +// rejected with the limit named. +func overCeilingRetries() []string { + return []string{aboveCeiling(), wayAboveCeiling} +} + +// unparseableRetries is input an operator can type into the field +// that is not a retry count. Each must be REJECTED: silently reading +// any of them as 0 turns a store-and-forward proxy into +// fire-and-forget without saying so. +// +// The twenty-digit case is here because it parses as digits but +// overflows int, which is the one failure the field's own min/max +// attributes cannot describe. +func unparseableRetries() []string { + return []string{ + notANumber, + "2.7", + "-5", + "12345678901234567890", + "1e3", + } +} + +// createRetriesForm is a complete, otherwise-valid HTTP target +// creation, so the only thing any case below varies is max_retries. +func createRetriesForm(retries string) url.Values { + form := url.Values{} + form.Set("name", "retries-target") + form.Set("type", string(database.TargetTypeHTTP)) + form.Set("url", retriesTargetURL) + + if retries != absentField { + form.Set("max_retries", retries) + } + + return form +} + +// absentField marks a field the form does not submit at all, which is +// the case that legitimately takes a default and must stay distinct +// from a field submitted with garbage in it. +const absentField = "\x00absent" + +// absentRetries is every way of saying "the operator did not set +// this", each of which takes the default rather than a 400. Blank and +// whitespace-only count as absent here because they do in the timeout +// and retention controls on the same forms; a rule the fields do not +// share would be its own surprise. +func absentRetries() []string { + return []string{absentField, "", " "} +} + +// createWithRetries posts the target create form for a fresh webhook +// and returns the webhook and the response. +func createWithRetries( + t *testing.T, + env *sourceTestEnv, + retries string, +) (database.Webhook, int, string) { + t.Helper() + + webhook := seedWebhookWithRetention(t, env.db, 30) + + w := serveTarget( + env, http.MethodPost, + "/source/"+webhook.ID+"/targets", + createRetriesForm(retries), + ) + + return webhook, w.Code, w.Body.String() +} + +// TestTargetCreate_RetriesAboveCeilingRejected proves the create form +// enforces a ceiling at all, and that the 400 names it — a rejection +// that does not say what the limit is leaves the operator guessing. +func TestTargetCreate_RetriesAboveCeilingRejected(t *testing.T) { + t.Parallel() + + env := setupSourceTest(t) + ceiling := strconv.Itoa(handlers.MaxTargetRetriesForTest) + + for _, retries := range overCeilingRetries() { + webhook, code, body := createWithRetries(t, env, retries) + + assert.Equal(t, http.StatusBadRequest, code, + "max_retries=%s should be rejected", retries) + assert.Contains(t, body, ceiling, + "the rejection for %s should name the ceiling", + retries) + assert.Empty(t, + targetsForWebhook(t, env.db, webhook.ID), + "no target should be created for %s", retries) + } +} + +// TestTargetCreate_UnparseableRetriesRejected is the core of the +// defect: each of these was accepted with HTTP 200 and stored as 0. +func TestTargetCreate_UnparseableRetriesRejected(t *testing.T) { + t.Parallel() + + env := setupSourceTest(t) + + for _, retries := range unparseableRetries() { + webhook, code, body := createWithRetries(t, env, retries) + + assert.Equal(t, http.StatusBadRequest, code, + "max_retries=%q should be rejected, not coerced", + retries) + assert.Contains(t, body, "whole number", + "the rejection for %q should say why", retries) + assert.Empty(t, + targetsForWebhook(t, env.db, webhook.ID), + "no target should be created for %q", retries) + } +} + +// TestTargetCreate_ValidRetriesStored covers the accepting half, +// including the ceiling itself: a bound that rejects its own limit +// would make the advertised maximum unreachable. +func TestTargetCreate_ValidRetriesStored(t *testing.T) { + t.Parallel() + + env := setupSourceTest(t) + + for _, want := range []int{0, 3, handlers.MaxTargetRetriesForTest} { + webhook, code, body := createWithRetries( + t, env, strconv.Itoa(want), + ) + + require.Equal(t, http.StatusSeeOther, code, body) + + targets := targetsForWebhook(t, env.db, webhook.ID) + require.Len(t, targets, 1) + assert.Equal(t, want, targets[0].MaxRetries) + } +} + +// TestTargetCreate_AbsentRetriesTakesDefault keeps the two cases +// distinct. An omitted field is not an operator asking for something +// invalid, so it still gets the fire-and-forget default rather than a +// 400 — otherwise the fix above would make the form unusable. +func TestTargetCreate_AbsentRetriesTakesDefault(t *testing.T) { + t.Parallel() + + env := setupSourceTest(t) + + for _, retries := range absentRetries() { + webhook, code, body := createWithRetries(t, env, retries) + + require.Equal(t, http.StatusSeeOther, code, body) + + targets := targetsForWebhook(t, env.db, webhook.ID) + require.Len(t, targets, 1) + assert.Equal(t, 0, targets[0].MaxRetries, + "an absent max_retries should take the default") + } +} + +// seedRetriesTarget creates an HTTP target already delivering with +// workingRetries retries, through the real create handler. +func seedRetriesTarget( + t *testing.T, + env *sourceTestEnv, +) (database.Webhook, database.Target) { + t.Helper() + + webhook, code, body := createWithRetries( + t, env, strconv.Itoa(workingRetries), + ) + require.Equal(t, http.StatusSeeOther, code, body) + + targets := targetsForWebhook(t, env.db, webhook.ID) + require.Len(t, targets, 1) + require.Equal(t, workingRetries, targets[0].MaxRetries) + + return webhook, targets[0] +} + +// editRetriesForm is a complete edit submission that changes the +// target's name as well, so a rejected submission can be shown to +// have written nothing at all rather than merely to have left +// max_retries alone. +func editRetriesForm(retries string) url.Values { + form := url.Values{} + form.Set("name", "renamed-by-edit") + form.Set("url", retriesTargetURL) + + if retries != absentField { + form.Set("max_retries", retries) + } + + return form +} + +// assertEditRejectedAndUnchanged submits an edit expected to fail and +// checks both halves of the requirement: the 400 explains itself, and +// the target it was submitted against is untouched. +func assertEditRejectedAndUnchanged( + t *testing.T, + env *sourceTestEnv, + retries, wantReason string, +) { + t.Helper() + + webhook, target := seedRetriesTarget(t, env) + + w := submitTargetEdit( + env, webhook.ID, target.ID, editRetriesForm(retries), + ) + + assert.Equal(t, http.StatusBadRequest, w.Code, + "max_retries=%q should be rejected on edit", retries) + assert.Contains(t, w.Body.String(), wantReason, + "the rejection for %q should say why", retries) + + stored := storedTarget(t, env, target.ID) + assert.Equal(t, workingRetries, stored.MaxRetries, + "a rejected edit must not destroy the working retry "+ + "count with %q", retries) + assert.Equal(t, "retries-target", stored.Name, + "a rejected edit must write nothing at all") +} + +// TestTargetEdit_UnparseableRetriesRejected is the damaging half of +// the defect. A target delivering with two retries, re-saved with a +// typo in the field, returned 200 and was left with retries disabled +// and nothing said. +func TestTargetEdit_UnparseableRetriesRejected(t *testing.T) { + t.Parallel() + + env := setupSourceTest(t) + + for _, retries := range unparseableRetries() { + assertEditRejectedAndUnchanged( + t, env, retries, "whole number", + ) + } +} + +// TestTargetEdit_RetriesAboveCeilingRejected proves the ceiling +// applies to the edit path too, naming itself, so the two paths +// cannot disagree about what is storable. +func TestTargetEdit_RetriesAboveCeilingRejected(t *testing.T) { + t.Parallel() + + env := setupSourceTest(t) + ceiling := strconv.Itoa(handlers.MaxTargetRetriesForTest) + + for _, retries := range overCeilingRetries() { + assertEditRejectedAndUnchanged(t, env, retries, ceiling) + } +} + +// TestTargetEdit_ValidRetriesStored covers the accepting half of the +// edit path, so the ceiling cannot be enforced by simply refusing +// every submission. +func TestTargetEdit_ValidRetriesStored(t *testing.T) { + t.Parallel() + + env := setupSourceTest(t) + + for _, want := range []int{0, 9, handlers.MaxTargetRetriesForTest} { + webhook, target := seedRetriesTarget(t, env) + + w := submitTargetEdit( + env, webhook.ID, target.ID, + editRetriesForm(strconv.Itoa(want)), + ) + require.Equal(t, + http.StatusSeeOther, w.Code, w.Body.String(), + ) + + assert.Equal(t, want, + storedTarget(t, env, target.ID).MaxRetries) + } +} + +// TestTargetEdit_AbsentRetriesLeavesStoredValue is the edit path's +// absent-versus-invalid case. Retries are only offered by the forms +// for types that retry, so a submission without the field must leave +// the stored count alone rather than be rejected or zeroed. +func TestTargetEdit_AbsentRetriesLeavesStoredValue(t *testing.T) { + t.Parallel() + + env := setupSourceTest(t) + + for _, retries := range absentRetries() { + webhook, target := seedRetriesTarget(t, env) + + w := submitTargetEdit( + env, webhook.ID, target.ID, + editRetriesForm(retries), + ) + require.Equal(t, + http.StatusSeeOther, w.Code, w.Body.String(), + ) + + assert.Equal(t, workingRetries, + storedTarget(t, env, target.ID).MaxRetries, + "an absent max_retries must leave the stored "+ + "count alone (%q)", retries) + } +} + +// TestTargetRetries_CreateAndEditAgreeOnEveryCase proves the two +// paths cannot disagree, which is what let the create form and the +// edit form drift apart in the first place. Every input is submitted +// to both and the accept/reject verdicts are compared. +func TestTargetRetries_CreateAndEditAgreeOnEveryCase(t *testing.T) { + t.Parallel() + + env := setupSourceTest(t) + + accepted := []string{ + "0", "1", + strconv.Itoa(handlers.MaxTargetRetriesForTest), + } + overCeiling := overCeilingRetries() + unparseable := unparseableRetries() + + cases := make( + []string, 0, + len(accepted)+len(overCeiling)+len(unparseable), + ) + cases = append(cases, accepted...) + cases = append(cases, overCeiling...) + cases = append(cases, unparseable...) + + for _, retries := range cases { + _, createCode, _ := createWithRetries(t, env, retries) + + webhook, target := seedRetriesTarget(t, env) + editCode := submitTargetEdit( + env, webhook.ID, target.ID, + editRetriesForm(retries), + ).Code + + assert.Equal(t, + createCode == http.StatusBadRequest, + editCode == http.StatusBadRequest, + "create and edit must agree on max_retries=%q "+ + "(create %d, edit %d)", + retries, createCode, editCode, + ) + } +} + +// TestPageOrFirst_CoercesRatherThanRejects pins the one place a +// non-numeric form value legitimately falls back. A page number says +// where to send the browser after an action that has already +// happened, so it is not configuration and rejecting it would report +// a failure that did not occur. +func TestPageOrFirst_CoercesRatherThanRejects(t *testing.T) { + t.Parallel() + + for _, s := range []string{"", "abc", "0", "-1", "2.7", " "} { + assert.Equal(t, 1, handlers.PageOrFirstForTest(s), + "%q should fall back to the first page", s) + } + + assert.Equal(t, 4, handlers.PageOrFirstForTest("4")) + assert.Equal(t, 4, handlers.PageOrFirstForTest(" 4 ")) +}