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.