From 663d988498e9fc6e8947f4fee6fdfd36949fbe40 Mon Sep 17 00:00:00 2001 From: clawbot Date: Fri, 2 Oct 2026 14:34:24 +0000 Subject: [PATCH] Close the retention follow-ups from the August review (closes #99) A retention value above the keep-forever sentinel is now pinned by a test to be stored as the sentinel. parseRetentionDays returns the form's message directly, so the two error values that only existed to pick that message are gone. The sweep no longer checks keep-forever itself; retentionCutoff does, before the webhook's database is opened. The create-form refill test uses HTML-special characters and checks they come back escaped. Model: opus-5-5 --- internal/database/retention.go | 22 +++---- internal/database/retention_test.go | 2 +- internal/handlers/source_management.go | 67 +++++++-------------- internal/handlers/source_management_test.go | 47 +++++++++------ 4 files changed, 60 insertions(+), 78 deletions(-) diff --git a/internal/database/retention.go b/internal/database/retention.go index c34ed99..86544d4 100644 --- a/internal/database/retention.go +++ b/internal/database/retention.go @@ -184,16 +184,6 @@ func (r *RetentionReaper) sweep(ctx context.Context) { wh := webhooks[i] - // Skip retain-forever webhooks before building any query. - // RetainsForever covers both the RetentionForeverDays - // sentinel and the non-positive values that predate it: the - // sentinel is a positive number, so without this the reaper - // would compute a cutoff a thousand years in the past and - // issue a DELETE matching nothing on every single sweep. - if wh.RetainsForever() { - continue - } - // Nothing to reap if the per-webhook database has never // been created. if !r.dbManager.DBExists(wh.ID) { @@ -212,6 +202,13 @@ func (r *RetentionReaper) reapWebhook( webhookID string, retentionDays int, ) { + // A retain-forever webhook has no cutoff, so its database is not + // even opened. + cutoff, ok := retentionCutoff(time.Now(), retentionDays) + if !ok { + return + } + db, err := r.dbManager.GetDB(webhookID) if err != nil { r.log.Error( @@ -223,11 +220,6 @@ func (r *RetentionReaper) reapWebhook( return } - cutoff, ok := retentionCutoff(time.Now(), retentionDays) - if !ok { - return - } - deleted, err := reapExpired(ctx, db, cutoff) if err != nil { r.log.Error( diff --git a/internal/database/retention_test.go b/internal/database/retention_test.go index c2dccef..7097677 100644 --- a/internal/database/retention_test.go +++ b/internal/database/retention_test.go @@ -362,7 +362,7 @@ func TestRetentionReaper_HugeFiniteRetentionRetainsRecentEvents( t, overflowingRetentionDays, database.RetentionForeverDays, - "the test value must not be rescued by the forever skip", + "the test value must not be treated as retain-forever", ) webhookID := createWebhook( diff --git a/internal/handlers/source_management.go b/internal/handlers/source_management.go index b3b4cc8..36a3468 100644 --- a/internal/handlers/source_management.go +++ b/internal/handlers/source_management.go @@ -41,71 +41,51 @@ type WebhookListItem struct { // errMissingURL signals that a required URL was not provided. var errMissingURL = errors.New("missing URL") -// errInvalidRetention signals a retention_days form value that is not -// a non-negative whole number. -var errInvalidRetention = errors.New("invalid retention days") - -// errRetentionTooLarge signals a retention_days form value that is a -// whole number but larger than the reaper's cutoff arithmetic can -// represent. It is distinguished from errInvalidRetention so the form -// can tell the user the actual ceiling instead of implying their input -// was not a number. -var errRetentionTooLarge = errors.New("retention days out of range") - -// retentionErrorMessage returns the message the create and edit forms -// show the user for a rejected retention_days value. Any error other -// than errRetentionTooLarge falls back to the generic wording, so an -// unrecognised parse failure still produces a sensible 400 rather than -// an empty alert. -func retentionErrorMessage(err error) string { - if errors.Is(err, errRetentionTooLarge) { - return "Retention must be at most " + - strconv.Itoa(database.MaxFiniteRetentionDays) + - " days, or 0 to retain events forever." - } - - return "Retention must be a whole number of days, or 0 to " + - "retain events forever." -} - -// parseRetentionDays interprets a retention_days form value. +// parseRetentionDays interprets a retention_days form value. It +// returns the number of days, or, for a value it refuses, the message +// the create and edit forms show; the message is empty when the value +// is accepted. // // An empty value yields fallback, which lets the create path apply the // default and the edit path leave the stored value unchanged. A value // of 0 is returned as 0 and is rewritten to the retain-forever // sentinel by database.Webhook's BeforeSave hook. Anything unparseable -// or negative is an error rather than a silently substituted default. +// or negative is refused rather than silently given a default. // // The upper bound is not cosmetic. The reaper computes its cutoff as a // time.Duration, an int64 nanosecond count, so a day count above // database.MaxFiniteRetentionDays overflows, puts the cutoff in the // future, and deletes every event the webhook has. A finite value -// above that ceiling is therefore a 400. +// above that ceiling is therefore refused, and the message names the +// ceiling rather than implying the input was not a number. // // A value at or above the retain-forever sentinel is not out of range: // it is what the edit form pre-fills for a retain-forever webhook, so // submitting the form back unchanged has to keep meaning "forever" // rather than being rejected. -func parseRetentionDays(raw string, fallback int) (int, error) { +func parseRetentionDays(raw string, fallback int) (int, string) { raw = strings.TrimSpace(raw) if raw == "" { - return fallback, nil + return fallback, "" } v, err := strconv.Atoi(raw) if err != nil || v < 0 { - return 0, errInvalidRetention + return 0, "Retention must be a whole number of days, or 0 to " + + "retain events forever." } if v >= database.RetentionForeverDays { - return database.RetentionForeverDays, nil + return database.RetentionForeverDays, "" } if v > database.MaxFiniteRetentionDays { - return 0, errRetentionTooLarge + return 0, "Retention must be at most " + + strconv.Itoa(database.MaxFiniteRetentionDays) + + " days, or 0 to retain events forever." } - return v, nil + return v, "" } // DeliveryView is the display-safe projection of a delivery @@ -361,16 +341,13 @@ func (h *Handlers) HandleSourceCreateSubmit() http.HandlerFunc { return } - retentionDays, retErr := parseRetentionDays( + retentionDays, errMsg := parseRetentionDays( retentionStr, database.DefaultRetentionDays, ) - if retErr != nil { + if errMsg != "" { h.renderTemplateStatus( w, r, "sources_new.html", - newSourceFormData( - retentionErrorMessage(retErr), - name, description, - ), + newSourceFormData(errMsg, name, description), http.StatusBadRequest, ) @@ -655,13 +632,13 @@ func (h *Handlers) applyWebhookEdit( // An empty field falls back to the stored value, so submitting the // form without touching retention leaves the policy alone. - retentionDays, retErr := parseRetentionDays( + retentionDays, errMsg := parseRetentionDays( r.PostFormValue("retention_days"), webhook.RetentionDays, ) - if retErr != nil { + if errMsg != "" { data := map[string]any{ tmplKeyWebhook: webhook, - tmplKeyError: retentionErrorMessage(retErr), + tmplKeyError: errMsg, } h.renderTemplateStatus(w, r, "source_edit.html", data, http.StatusBadRequest) diff --git a/internal/handlers/source_management_test.go b/internal/handlers/source_management_test.go index f4437dd..cffd391 100644 --- a/internal/handlers/source_management_test.go +++ b/internal/handlers/source_management_test.go @@ -368,31 +368,42 @@ func TestHandleSourceCreateSubmit_OverflowingRetentionIsRejected( // boundary between "too large to represent" and "retain forever": the // sentinel is above MaxFiniteRetentionDays, but it is the value the // edit form pre-fills, so it must be accepted rather than rejected as -// out of range. +// out of range. A value above the sentinel is stored as the sentinel. func TestHandleSourceCreateSubmit_SentinelIsAcceptedAsForever( t *testing.T, ) { t.Parallel() - env := setupSourceTest(t) - sentinel := strconv.Itoa(database.RetentionForeverDays) - - w := submitCreate(t, env.handlers, env.cookies, "forever", &sentinel) - require.Equal(t, http.StatusSeeOther, w.Code) - - wh := onlyWebhook(t, env.db) - assert.Equal( - t, + for _, days := range []int{ database.RetentionForeverDays, - storedRetentionDays(t, env.db, wh.ID), - ) + database.RetentionForeverDays + 1, + } { + raw := strconv.Itoa(days) + + t.Run(raw, func(t *testing.T) { + t.Parallel() + + env := setupSourceTest(t) + + w := submitCreate(t, env.handlers, env.cookies, "forever", &raw) + require.Equal(t, http.StatusSeeOther, w.Code) + + wh := onlyWebhook(t, env.db) + assert.Equal( + t, + database.RetentionForeverDays, + storedRetentionDays(t, env.db, wh.ID), + ) + }) + } } // TestHandleSourceCreateSubmit_RejectedFormKeepsUserInput checks that a // validation failure hands the user's typing back, matching what the // edit form already does. Losing a long description to a mistyped // retention value is the kind of thing that makes people give up on a -// form. +// form. Both values carry HTML-special characters, which must come +// back escaped rather than as markup. func TestHandleSourceCreateSubmit_RejectedFormKeepsUserInput( t *testing.T, ) { @@ -401,8 +412,8 @@ func TestHandleSourceCreateSubmit_RejectedFormKeepsUserInput( env := setupSourceTest(t) const ( - name = "kept-name" - description = "a description worth not losing" + name = `kept">name` + description = `a worth not losing` ) form := url.Values{} @@ -419,8 +430,10 @@ func TestHandleSourceCreateSubmit_RejectedFormKeepsUserInput( body := w.Body.String() - assert.Contains(t, body, `value="`+name+`"`) - assert.Contains(t, body, description) + assert.Contains(t, body, `value="kept"><b>name"`) + assert.Contains(t, body, `a </textarea> worth not losing`) + assert.NotContains(t, body, name) + assert.NotContains(t, body, description) } // submitEdit posts the webhook edit form for the given webhook. -- 2.54.0