max_retries is unbounded at target creation, and unparseable input silently becomes 0 #221
No Assignees
Notifications
Due Date
No due date set.
Dependencies
No dependencies set.
Reference: sneak/webhooker#221
Reference in New Issue
Block a user
Found while implementing #202, with a second defect added from its review.
Two problems in the same helper,
parseNonNegativeInt(internal/handlers/source_management.go:1183-1194), whichHandleTargetCreateuses formax_retries:1. No ceiling. Any non-negative value is accepted.
parseRetentionDaysin the same file shows the intended shape: validate against a stated ceiling and answer 400 naming it. A typo'd extra zero givesMaxRetriesof 100000 and the engine will genuinely record that manydelivery_resultsrows, each of which the event log then loads and renders.2. Unparseable input silently returns 0. This is the silent-fallback pattern the owner rejected on #78 — a set-but-invalid value must fail loudly, never degrade into a default the operator did not ask for. Here a garbage
max_retriesquietly means "never retry", which is the opposite of what someone typing a number into that field intends, and nothing tells them.It is operator-chosen rather than sender-chosen, so it is a robustness bug rather than a 1.0 blocker — an operator can only shoot themselves.
Definition of done:
max_retriesis validated at creation and edit against a stated ceiling, rejected with 400 naming the ceilingparseNonNegativeIntfor the same coercion and fix or justify eachmax_retries is unbounded at target creation, so one target can queue arbitrarily many delivery attemptsto max_retries is unbounded at target creation, and unparseable input silently becomes 0Plan.
Modelled on
parseRetentionDays/delivery.ParseTargetTimeout, which already get this right in the same form.parseMaxRetries(raw string, fallback int) (int, error)ininternal/handlers: empty means ABSENT and yieldsfallback; anything unparseable or negative is an error; anything above the ceiling is a distinct error. No path returns a substituted default for a set-but-invalid value.maxTargetRetries = 20. Bothtemplates/source_detail.htmlandtemplates/target_edit.htmlalready declaremax="20"on the input; the server simply never enforced what the UI advertises. Backoff is2^(n-1)seconds, so attempt 20 is already ~6 days out, and each attempt writes adelivery_resultsrow the event log then loads.processTargetCreate(fallback 0) andapplyTargetEdit(fallback = storedMaxRetries, kept inside the existingPostForm.Hasguard so a form that does not render the field still does not touch retries). They cannot disagree because they call the same function.timeoutcontrol on the same submission:Invalid max retries: ..., naming the ceiling when out of range.parseNonNegativeInt: its other two callers (finishResubmit,finishReplay) parse the log page number for a redirect, not configuration. Coercion is correct there and already matchesparsePageon the GET side. I will deleteparseNonNegativeIntand give those callers apageOrFirsthelper shared withparsePage, so no silently-coercing general-purpose int parser is left lying around for a config field to reach for.Tests on both create and edit: above-ceiling rejected with the ceiling named,
abc/2.7/-5/ a 20-digit number rejected rather than coerced to 0, a valid value stored, an absent field still taking its default, and the edit case proving a workingmax_retries=2survives a rejected submission.Done in #259 (base
next).parseNonNegativeIntis removed.max_retrieson both the create and the edit form now goes through oneparseMaxRetries: empty or omitted takes the caller's fallback (0 at creation, the stored count on edit), and anything else that is not a whole number at most 20 is a 400 that names the reason. Ceiling is 20 because both templates have always declaredmax="20"on the input and the server never enforced it; backoff is2^(n-1)seconds, so attempt 20 is already about six days out.The two other callers (
finishResubmit,finishReplay) read the log page number for a post-action redirect, where falling back to page 1 is correct and already matchesparsePageon the GET side. They use apageOrFirstnamed for that, so no general silently-coercing parser is left in the package. Full disposition table in the PR.Verified:
make checkgreen withGOFLAGS=-count=1— 570 tests, 0 failures, lint 0 issues in Docker. Ten new tests cover both paths for above-ceiling,abc,2.7,-5, a 20-digit number, valid values including the ceiling itself, and an absent field still taking its default; one test submits every case to both paths and asserts the verdicts match.On a running instance, the reported edit case — a target delivering with
max_retries=2, re-saved withabc— now answersHTTP 400 Invalid max retries: retries must be a whole number of attempts, or 0 for fire-and-forget, and the stored value is still 2 with the submission's name change not written either. A row set to 100000 directly in the database still renders on both pages and is not clamped.