Validate max_retries on both target forms (closes #221)

max_retries was read through parseNonNegativeInt, which returned 0 for
any parse failure. On the create form `abc`, `2.7`, `-5` and a
twenty-digit number were all accepted with HTTP 200 and stored as 0,
and `999999999` was stored verbatim with no ceiling. On the edit form
the same input destroyed a working retry configuration: a target
delivering with max_retries=2, re-saved with a typo in the field, was
silently left at 0 — fire-and-forget on a store-and-forward proxy,
with nothing said.

A value that is set but unparseable must be rejected loudly. A default
belongs only to an absent value. parseMaxRetries makes that
distinction explicit: an empty or omitted field yields the caller's
fallback (0 at creation, the stored count on edit), and anything else
that is not a whole number in range is a 400. Both forms go through
one validator, so they cannot come to disagree.

The ceiling is 20, which both target templates have always declared as
max="20" on the input; only the server never enforced it. Backoff is
2^(n-1) seconds, so attempt 20 is already about six days out, and each
attempt writes a delivery_results row the event log then loads and
renders.

The rejection wording matches the timeout control on the same
submission, which already got this right, and names the ceiling when
the value is out of range.

Existing rows above the ceiling are untouched: they still render on
the source and edit pages and still deliver. This is input validation,
not a migration.

parseNonNegativeInt is removed. Its other two callers read the log
page number for a post-action redirect, where falling back to page 1
is correct — it is navigation, not stored configuration, and the
action has already completed. They now use pageOrFirst, named for what
it does and shared with parsePage on the GET side, so no general
silently-coercing int parser is left for a configuration field to
reach for.

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)

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)

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

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
+	return pageOrFirst(r.URL.Query().Get("page"))
-
-	if p := r.URL.Query().Get("page"); p != "" {
-		v, err := strconv.Atoi(p)
-		if err == nil && v > 0 {
-			page = v
-		}
-	}
-
-	return 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,
+// pageOrFirst parses a paginated page number, answering 1 for
-// returning 0 if s is empty or invalid.
+// anything empty, unparseable or out of range.
-func parseNonNegativeInt(s string) int {
+//
-	if s == "" {
+// Falling back rather than rejecting is correct here and only here:
-		return 0
+// 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)
+	return v
-	if err == nil && v >= 0 {
-		return v
-	}
-
-	return 0
 }
 
 // targetFormInput carries the raw form values describing a target's

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(
+		retries, ok := targetMaxRetries(w, r, target.MaxRetries)
-			r.PostFormValue("max_retries"),
+		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)

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
+}

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 "))
+}