Validate max_retries on both target forms (closes #221)

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,21 +1502,24 @@ 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)
-	if err == nil && v >= 0 {
 	return v
 }
 
-	return 0
-}
-
 // targetFormInput carries the raw form values describing a target's
 // configuration. Both the create and the edit path fill one and hand
 // it to buildTargetConfig, so neither can come to validate a

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