Validate max_retries on both target forms (closes #221) #259

Merged

clawbot merged 1 commits from issue-221-max-retries-validation into next 2026-08-24 01:32:48 +02:00

Conversation 1 Commits 1 Files Changed 7 +571 -31

clawbot commented 2026-08-24 01:20:56 +02:00

Collaborator

Copy Link

Closes #221.

max_retries was read through parseNonNegativeInt, which returned 0 for any parse failure. It now goes through one validator shared by the create and edit paths.

What was wrong

Confirmed by execution before the fix. Create form: 999999999 stored verbatim; abc, 2.7, -5 and a 20-digit number all accepted with HTTP 200 and stored as 0. Edit form: a target delivering with max_retries=2, re-saved with max_retries=abc, returned HTTP 200 and was stored as 0 — a working retry configuration destroyed by a typo, silently turning a store-and-forward proxy into fire-and-forget.

The rule applied

A value that is SET BUT UNPARSEABLE is rejected loudly. A default belongs ONLY to an ABSENT value. parseMaxRetries(raw string, fallback int) makes the distinction explicit in the code:

• empty or omitted → fallback (0 at creation, the stored count on edit)
• unparseable, negative, or above the ceiling → error, 400, nothing written

Blank and whitespace-only count as absent, because they already do for the timeout and retention_days controls on the same forms; a rule those fields did not share would be its own surprise.

Both paths, from one validator

processTargetCreate and applyTargetEdit both call targetMaxRetries, so they cannot come to disagree — TestTargetRetries_CreateAndEditAgreeOnEveryCase submits every input to both and compares the verdicts. The edit path keeps its existing PostForm.Has("max_retries") guard, so a form for a target type that does not render the field still does not touch retries; validation is inside that guard, and runs before anything is assigned to the target.

Wording matches the timeout control on the same submission, which already got this right:

Invalid max retries: retries must be a whole number of attempts, or 0 for fire-and-forget
Invalid max retries: retries out of range: at most 20 retries

The ceiling: 20

Chosen, not invented. Both templates/source_detail.html and templates/target_edit.html have always declared max="20" on the input — the server simply never enforced what the UI advertises, so this closes the gap rather than introducing a new limit, and no template changes.

It is also what the delivery engine can usefully do with the value. Backoff is 2^(n-1) seconds capped at 2^30, so attempt 20 is already about six days after the first; beyond that the value buys no durability. Every attempt writes a delivery_results row that the event log then loads and renders, so an absurd ceiling is itself a resource problem — the issue's 100000 case would be 100000 rows for one delivery.

Audit of every other parseNonNegativeInt caller

The helper was the defect, so it is gone. Its callers, all of them:

Caller	Field	Disposition
processTargetCreate (source_management.go)	max_retries	Fixed. Validates, rejects.
applyTargetEdit (target_edit.go)	max_retries	Fixed. Same validator.
finishResubmit (event_resubmit.go)	page	Justified. See below.
finishReplay (delivery_replay.go)	page	Justified. See below.

The two page readers are not the same bug. A page number says where to send the browser after an action, not what to store: it is never persisted, never delivered, and by the time it is read the resubmit or replay has already completed — answering 400 would report a failure that did not happen. Falling back to page 1 is correct there, and is already what parsePage does for the same parameter on the GET side.

What was wrong was the shape: a general-purpose silently-coercing int parser sitting in the package for the next configuration field to reach for. Those callers now use pageOrFirst, named for what it does, documented as to why it is the exception, and shared with parsePage so the log page number is parsed one way. parseNonNegativeInt no longer exists.

Existing stored values

Input validation, not a migration. Nothing clamps a row. Verified on a running instance with a target set to max_retries=100000 directly in the database: source detail page HTTP 200 showing 100000, edit page HTTP 200 pre-filling value="100000", and the row still 100000 afterwards. Delivery is untouched — no change in internal/delivery.

One consequence, called out deliberately: re-saving such a target from the edit form does have to bring it into range, because the form submits the pre-filled value back and accepting it would mean the ceiling does not apply to the edit path. The 400 names the ceiling, so the fix is one field. This is documented on parseMaxRetries.

Verification

make check green, run with GOFLAGS=-count=1 on the committed tree: exit 0, 570 tests passed, 0 failures, lint 0 issues (in Docker via the script/ entrypoint, as always). The ten new tests all show real durations rather than cached results.

Tests cover, on BOTH create and edit: above-ceiling rejected with the ceiling named; abc, 2.7, -5, a 20-digit number and 1e3 rejected rather than coerced to 0; valid values including 0 and the ceiling itself accepted and stored; an absent field still taking its default, so the two cases stay provably distinct; and a rejected edit leaving both the retry count and the name untouched, proving nothing at all was written.

Live instance, the reported case

The target from the report: max_retries=2, re-saved from the edit form with abc.

--- the reported case: edit form, max_retries=abc ---
Invalid max retries: retries must be a whole number of attempts, or 0 for fire-and-forget
HTTP 400
stored after: max_retries=2 name=evidence-target

The 400 is answered, the working retry count survives, and the name change in the same submission was not written either.

--- edit form, max_retries=999999999 ---
Invalid max retries: retries out of range: at most 20 retries
HTTP 400
stored after: max_retries=2

--- create form, each reported value ---
  max_retries=999999999            HTTP 400  rows=0
  max_retries=abc                  HTTP 400  rows=0
  max_retries=2.7                  HTTP 400  rows=0
  max_retries=-5                   HTTP 400  rows=0
  max_retries=12345678901234567890 HTTP 400  rows=0
  max_retries=21                   HTTP 400  rows=0

--- valid values still accepted ---
  max_retries=0   HTTP 303  stored=0
  max_retries=5   HTTP 303  stored=5
  max_retries=20  HTTP 303  stored=20

Scope

internal/handlers and its tests only. No change to internal/delivery, internal/database, internal/middleware, the templates, or TODO.md.

Closes https://git.eeqj.de/sneak/webhooker/issues/221. `max_retries` was read through `parseNonNegativeInt`, which returned 0 for any parse failure. It now goes through one validator shared by the create and edit paths. ## What was wrong Confirmed by execution before the fix. Create form: `999999999` stored verbatim; `abc`, `2.7`, `-5` and a 20-digit number all accepted with HTTP 200 and stored as **0**. Edit form: a target delivering with `max_retries=2`, re-saved with `max_retries=abc`, returned HTTP 200 and was stored as 0 — a working retry configuration destroyed by a typo, silently turning a store-and-forward proxy into fire-and-forget. ## The rule applied A value that is SET BUT UNPARSEABLE is rejected loudly. A default belongs ONLY to an ABSENT value. `parseMaxRetries(raw string, fallback int)` makes the distinction explicit in the code: - empty or omitted → `fallback` (0 at creation, the stored count on edit) - unparseable, negative, or above the ceiling → error, 400, nothing written Blank and whitespace-only count as absent, because they already do for the `timeout` and `retention_days` controls on the same forms; a rule those fields did not share would be its own surprise. ## Both paths, from one validator `processTargetCreate` and `applyTargetEdit` both call `targetMaxRetries`, so they cannot come to disagree — `TestTargetRetries_CreateAndEditAgreeOnEveryCase` submits every input to both and compares the verdicts. The edit path keeps its existing `PostForm.Has("max_retries")` guard, so a form for a target type that does not render the field still does not touch retries; validation is inside that guard, and runs before anything is assigned to the target. Wording matches the `timeout` control on the same submission, which already got this right: Invalid max retries: retries must be a whole number of attempts, or 0 for fire-and-forget Invalid max retries: retries out of range: at most 20 retries ## The ceiling: 20 Chosen, not invented. Both `templates/source_detail.html` and `templates/target_edit.html` have always declared `max="20"` on the input — the server simply never enforced what the UI advertises, so this closes the gap rather than introducing a new limit, and no template changes. It is also what the delivery engine can usefully do with the value. Backoff is `2^(n-1)` seconds capped at `2^30`, so attempt 20 is already about six days after the first; beyond that the value buys no durability. Every attempt writes a `delivery_results` row that the event log then loads and renders, so an absurd ceiling is itself a resource problem — the issue's 100000 case would be 100000 rows for one delivery. ## Audit of every other `parseNonNegativeInt` caller The helper was the defect, so it is gone. Its callers, all of them: | Caller | Field | Disposition | | --- | --- | --- | | `processTargetCreate` (`source_management.go`) | `max_retries` | **Fixed.** Validates, rejects. | | `applyTargetEdit` (`target_edit.go`) | `max_retries` | **Fixed.** Same validator. | | `finishResubmit` (`event_resubmit.go`) | `page` | **Justified.** See below. | | `finishReplay` (`delivery_replay.go`) | `page` | **Justified.** See below. | The two `page` readers are not the same bug. A page number says where to send the browser after an action, not what to store: it is never persisted, never delivered, and by the time it is read the resubmit or replay has already completed — answering 400 would report a failure that did not happen. Falling back to page 1 is correct there, and is already what `parsePage` does for the same parameter on the GET side. What was wrong was the *shape*: a general-purpose silently-coercing int parser sitting in the package for the next configuration field to reach for. Those callers now use `pageOrFirst`, named for what it does, documented as to why it is the exception, and shared with `parsePage` so the log page number is parsed one way. `parseNonNegativeInt` no longer exists. ## Existing stored values Input validation, not a migration. Nothing clamps a row. Verified on a running instance with a target set to `max_retries=100000` directly in the database: source detail page HTTP 200 showing 100000, edit page HTTP 200 pre-filling `value="100000"`, and the row still 100000 afterwards. Delivery is untouched — no change in `internal/delivery`. One consequence, called out deliberately: re-saving such a target from the edit form does have to bring it into range, because the form submits the pre-filled value back and accepting it would mean the ceiling does not apply to the edit path. The 400 names the ceiling, so the fix is one field. This is documented on `parseMaxRetries`. ## Verification `make check` **green**, run with `GOFLAGS=-count=1` on the committed tree: exit 0, **570 tests passed, 0 failures**, lint `0 issues` (in Docker via the `script/` entrypoint, as always). The ten new tests all show real durations rather than cached results. Tests cover, on BOTH create and edit: above-ceiling rejected with the ceiling named; `abc`, `2.7`, `-5`, a 20-digit number and `1e3` rejected rather than coerced to 0; valid values including 0 and the ceiling itself accepted and stored; an absent field still taking its default, so the two cases stay provably distinct; and a rejected edit leaving both the retry count and the name untouched, proving nothing at all was written. ### Live instance, the reported case The target from the report: `max_retries=2`, re-saved from the edit form with `abc`. --- the reported case: edit form, max_retries=abc --- Invalid max retries: retries must be a whole number of attempts, or 0 for fire-and-forget HTTP 400 stored after: max_retries=2 name=evidence-target The 400 is answered, the working retry count survives, and the name change in the same submission was not written either. --- edit form, max_retries=999999999 --- Invalid max retries: retries out of range: at most 20 retries HTTP 400 stored after: max_retries=2 --- create form, each reported value --- max_retries=999999999 HTTP 400 rows=0 max_retries=abc HTTP 400 rows=0 max_retries=2.7 HTTP 400 rows=0 max_retries=-5 HTTP 400 rows=0 max_retries=12345678901234567890 HTTP 400 rows=0 max_retries=21 HTTP 400 rows=0 --- valid values still accepted --- max_retries=0 HTTP 303 stored=0 max_retries=5 HTTP 303 stored=5 max_retries=20 HTTP 303 stored=20 ## Scope `internal/handlers` and its tests only. No change to `internal/delivery`, `internal/database`, `internal/middleware`, the templates, or `TODO.md`.

clawbot added 1 commit 2026-08-24 01:20:56 +02:00

Validate max_retries on both target forms (closes #221) ... e65cb89d1c

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.

clawbot added the needs-review label 2026-08-24 01:21:02 +02:00

clawbot self-assigned this 2026-08-24 01:21:02 +02:00

clawbot referenced this pull request 2026-08-24 01:21:13 +02:00

max_retries is unbounded at target creation, and unparseable input silently becomes 0 #221

clawbot commented 2026-08-24 01:31:23 +02:00

Author

Collaborator

Copy Link

PASS — independent review. Definition of done in #221 is met, verified by execution against a running instance, not by the test suite: all 16 input classes (absent, empty, whitespace-only, 0, 5, 20, 21, 999999999, abc, 2.7, -5, a 20-digit number, +5, 0x10, " 7 ", 1e3) give identical accept/reject verdicts on the create and edit forms; the reported case (a target that had actually delivered at max_retries=2, re-saved with abc plus a rename and a URL change) answers 400 with max_retries, name and config all unchanged — no partial write; a row set to 100000 directly in the DB still renders on both pages, still pre-fills, and still delivers. make check green from a clean clone with GOFLAGS=-count=1 (exit 0, 100s, 570 passes, zero (cached), lint 0 issues in Docker), CI green on e65cb89, merges cleanly into next, commit message and trailers clean.

Anomalies and disclosures, none blocking:

• Mutation-probed the tests rather than trusting them: with parseMaxRetries reverted to the old silent-coercion semantics, TestTargetCreate_UnparseableRetriesRejected, TestTargetEdit_UnparseableRetriesRejected, TestTargetCreate_RetriesAboveCeilingRejected and TestTargetEdit_RetriesAboveCeilingRejected all fail. TestTargetRetries_CreateAndEditAgreeOnEveryCase still passes under that mutation — it asserts agreement, not correctness, so it pins nothing on its own. Correct as designed, worth knowing.
• A 20-digit value is rejected as retries must be a whole number of attempts rather than retries out of range, because strconv.Atoi returns ErrRange and the code branches only on err != nil. Loud rejection either way; the message is just less precise than it could be. Nit.
• +5 stores 5 and " 7 " stores 7 (Atoi sign handling plus the TrimSpace). Lenient, never silently defaults.
• Whitespace-only is treated as ABSENT, so on the edit path max_retries=" " keeps the stored count. That is a value the operator submitted taking a default, which is the edge of the iron rule; it is deliberate, matches parseRetentionDays and the timeout control on the same forms, and is covered by a test. Flagging it as the judgement call it is, not as a defect.
• Audit of the removed helper verified rather than taken on faith: internal/handlers now contains exactly three integer parses — parseRetentionDays, parseMaxRetries (both reject loudly) and pageOrFirst. A garbage page yields 1 and the page > 1 guards behave exactly as before. Every other form field on these routes (timeout, headers, expiry, signature_scheme, retention_days) already rejects loudly. The disposition table is complete.

Two recommendations, owner's call, explicitly not blockers:

• Ceiling of 20 is right. Checked the arithmetic against calcBackoff rather than the PR body: delay is 2^(n-1)s and maxRetries bounds total attempts, so attempt 20 lands 2^19 - 1s out, about 6.1 days. That covers a long weekend with room, and maxBackoffShift = 30 means the cap does not bind below attempt 31. If a longer window is ever wanted, the lever is the backoff curve, not a bigger retry count.
• Requiring a legacy above-ceiling row to come into range on resave is acceptable. Confirmed live: re-saving the 100000 row with its own pre-filled value answers Invalid max retries: retries out of range: at most 20 retries and writes nothing. Grandfathering would mean the ceiling does not apply on the edit path at all; the 400 names the limit and the fix is one field.

**PASS** — independent review. Definition of done in https://git.eeqj.de/sneak/webhooker/issues/221 is met, verified by execution against a running instance, not by the test suite: all 16 input classes (absent, empty, whitespace-only, `0`, `5`, `20`, `21`, `999999999`, `abc`, `2.7`, `-5`, a 20-digit number, `+5`, `0x10`, `" 7 "`, `1e3`) give identical accept/reject verdicts on the create and edit forms; the reported case (a target that had actually delivered at `max_retries=2`, re-saved with `abc` plus a rename and a URL change) answers 400 with `max_retries`, `name` and `config` all unchanged — no partial write; a row set to `100000` directly in the DB still renders on both pages, still pre-fills, and still delivers. `make check` green from a clean clone with `GOFLAGS=-count=1` (exit 0, 100s, 570 passes, zero `(cached)`, lint 0 issues in Docker), CI green on `e65cb89`, merges cleanly into `next`, commit message and trailers clean. Anomalies and disclosures, none blocking: - Mutation-probed the tests rather than trusting them: with `parseMaxRetries` reverted to the old silent-coercion semantics, `TestTargetCreate_UnparseableRetriesRejected`, `TestTargetEdit_UnparseableRetriesRejected`, `TestTargetCreate_RetriesAboveCeilingRejected` and `TestTargetEdit_RetriesAboveCeilingRejected` all fail. `TestTargetRetries_CreateAndEditAgreeOnEveryCase` still passes under that mutation — it asserts agreement, not correctness, so it pins nothing on its own. Correct as designed, worth knowing. - A 20-digit value is rejected as `retries must be a whole number of attempts` rather than `retries out of range`, because `strconv.Atoi` returns `ErrRange` and the code branches only on `err != nil`. Loud rejection either way; the message is just less precise than it could be. Nit. - `+5` stores 5 and `" 7 "` stores 7 (`Atoi` sign handling plus the `TrimSpace`). Lenient, never silently defaults. - Whitespace-only is treated as ABSENT, so on the edit path `max_retries=" "` keeps the stored count. That is a value the operator submitted taking a default, which is the edge of the iron rule; it is deliberate, matches `parseRetentionDays` and the `timeout` control on the same forms, and is covered by a test. Flagging it as the judgement call it is, not as a defect. - Audit of the removed helper verified rather than taken on faith: `internal/handlers` now contains exactly three integer parses — `parseRetentionDays`, `parseMaxRetries` (both reject loudly) and `pageOrFirst`. A garbage `page` yields 1 and the `page > 1` guards behave exactly as before. Every other form field on these routes (`timeout`, `headers`, `expiry`, `signature_scheme`, `retention_days`) already rejects loudly. The disposition table is complete. Two recommendations, owner's call, explicitly not blockers: - **Ceiling of 20 is right.** Checked the arithmetic against `calcBackoff` rather than the PR body: delay is `2^(n-1)`s and `maxRetries` bounds total attempts, so attempt 20 lands `2^19 - 1`s out, about 6.1 days. That covers a long weekend with room, and `maxBackoffShift = 30` means the cap does not bind below attempt 31. If a longer window is ever wanted, the lever is the backoff curve, not a bigger retry count. - **Requiring a legacy above-ceiling row to come into range on resave is acceptable.** Confirmed live: re-saving the `100000` row with its own pre-filled value answers `Invalid max retries: retries out of range: at most 20 retries` and writes nothing. Grandfathering would mean the ceiling does not apply on the edit path at all; the 400 names the limit and the fix is one field.

clawbot merged commit fd5966f807 into next 2026-08-24 01:32:48 +02:00

clawbot deleted branch issue-221-max-retries-validation 2026-08-24 01:32:49 +02:00

Sign in to join this conversation.

Reviewers

No Reviewers

Labels

Clear labels

merge-ready

needs-checks

needs-rebase

needs-review

needs-rework

No Label needs-review

Milestone

No items

No Milestone

Projects

Clear projects

No project

Assignees

Clear assignees

clawbot sneak

No Assignees clawbot

1 Participants

Notifications

Subscribe

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: sneak/webhooker#259