From f282c6363dff98053e7d4724a07b4aa559edbde7 Mon Sep 17 00:00:00 2001
From: clawbot <35+clawbot@noreply.example.org>
Date: Sat, 3 Oct 2026 00:51:56 +0200
Subject: [PATCH] Keep what was typed when a target or webhook edit is refused
(closes #381)
A refused save on the target edit page answered with a bare text page, losing the form and everything typed, and the webhook edit page came back with the stored values instead of the submitted ones. A refused target edit now shows the edit form again with the reason above it and every value submitted, with the same status codes as before; a refused webhook edit keeps the submitted name, description and retention. Target edits use the same validation as new targets, with no second copy; an encoding or database failure stays a logged 500. The browser test covers a refused save on both pages, and its main function is now a plain list of checks.
Model: opus-5-5
---
README.md | 36 ++--
internal/handlers/source_management.go | 190 ++++++++++--------
internal/handlers/source_management_test.go | 49 +++++
internal/handlers/target_edit.go | 126 ++++++------
internal/handlers/target_edit_test.go | 71 +++++++
.../handlers/target_private_refusal_test.go | 9 +-
internal/handlers/target_retries.go | 30 ---
internal/handlers/ui_copy_test.go | 22 +-
internal/server/alpine_browser_test.go | 181 ++++++++++++-----
templates/source_edit.html | 6 +-
templates/target_edit.html | 16 +-
11 files changed, 467 insertions(+), 269 deletions(-)
diff --git a/README.md b/README.md
index e886758..39cbfa8 100644
--- a/README.md
+++ b/README.md
@@ -1350,23 +1350,25 @@ markup. The CSP build runs no expressions, so every Alpine directive in
`static/js/app.js`: `x-data="collapsible"` and `@click="toggle"`, never
`x-data="{ open: false }"` or `@click="open = !open"`.
-A browser test in `internal/server` loads the webhook page and the event log
-under the real policy and checks that: the add entrypoint form stays hidden
-until Add is clicked; for every target type, the targets section's Add shows
-only a choice of type with Next and Cancel, Next shows only that type's fields
-(no url field for `database` or `log`), Cancel at either step closes the form,
-and saving adds the target; a refused target comes back with its form open, the
-values entered and the reason, and after Cancel the next Add starts with an
-empty form and no reason; the Copy button beside an entrypoint URL reads
-"Copied" once clicked; an entrypoint's Edit button shows its edit form in place
-of its description and hides until the form closes, Cancel hides the form and
-drops what was typed, as does leaving the page and going back to it, and Save
-changes the description; of the recent events on the webhook page only the
-newest starts expanded, each expands and collapses, and Open leads to the
-event's own page; an event in the event log expands and collapses, and so do a
-delivery's attempts inside it; and at phone width the menu button opens and
-closes the mobile menu. It also fails if the browser reports a console warning
-or error, an uncaught exception, or anything the policy refused. `make check`
+A browser test in `internal/server` loads the webhook page, its edit pages and
+the event log under the real policy and checks that: the add entrypoint form
+stays hidden until Add is clicked; for every target type, the targets section's
+Add shows only a choice of type with Next and Cancel, Next shows only that
+type's fields (no url field for `database` or `log`), Cancel at either step
+closes the form, and saving adds the target; a refused target comes back with
+its form open, the values entered and the reason, and after Cancel the next Add
+starts with an empty form and no reason; a refused save on the target edit page
+and on the webhook edit page comes back with the reason and every value
+entered; the Copy button beside an entrypoint URL reads "Copied" once clicked;
+an entrypoint's Edit button shows its edit form in place of its description and
+hides until the form closes, Cancel hides the form and drops what was typed, as
+does leaving the page and going back to it, and Save changes the description;
+of the recent events on the webhook page only the newest starts expanded, each
+expands and collapses, and Open leads to the event's own page; an event in the
+event log expands and collapses, and so do a delivery's attempts inside it; and
+at phone width the menu button opens and closes the mobile menu. It also fails
+if the browser reports a console warning or error, an uncaught exception, or
+anything the policy refused. `make check`
and the image build lint it but do not run it, and `make test` leaves it out
(its file is built only with the `browser` build tag). Run it with
`make test-browser` after changing `templates/` or `static/js/`: that builds
diff --git a/internal/handlers/source_management.go b/internal/handlers/source_management.go
index 5556e27..7c5b7c2 100644
--- a/internal/handlers/source_management.go
+++ b/internal/handlers/source_management.go
@@ -616,13 +616,13 @@ func (h *Handlers) renderSourceDetail(
// Targets are projected to a display-safe view: a
// target's stored config blob holds a credential, and it
// must never reach a template.
- "Entrypoints": entrypointViews,
- "Targets": h.targetRows(&webhook, targets),
- "Events": events,
- "BaseURL": baseURL,
- "Stats": h.loadWebhookStats(webhook.ID, entrypoints, targets),
- "TargetForm": targetForm,
- "TargetError": targetErr,
+ "Entrypoints": entrypointViews,
+ "Targets": h.targetRows(&webhook, targets),
+ "Events": events,
+ "BaseURL": baseURL,
+ "Stats": h.loadWebhookStats(webhook.ID, entrypoints, targets),
+ tmplKeyTargetForm: targetForm,
+ "TargetError": targetErr,
}
status := http.StatusOK
@@ -658,12 +658,12 @@ func (h *Handlers) HandleSourceEdit() http.HandlerFunc {
return
}
- data := map[string]any{
- tmplKeyWebhook: &webhook,
- tmplKeyError: "",
- }
-
- h.renderTemplate(w, r, "source_edit.html", data)
+ h.renderWebhookEdit(
+ w, r, &webhook,
+ webhook.Name, webhook.Description,
+ strconv.Itoa(webhook.RetentionDays),
+ "", http.StatusOK,
+ )
}
}
@@ -709,7 +709,8 @@ func (h *Handlers) HandleSourceEditSubmit() http.HandlerFunc {
}
}
-// applyWebhookEdit validates and saves webhook edits.
+// applyWebhookEdit validates and saves webhook edits. A refused save
+// shows the edit form again with the values submitted and the reason.
func (h *Handlers) applyWebhookEdit(
w http.ResponseWriter,
r *http.Request,
@@ -718,52 +719,52 @@ func (h *Handlers) applyWebhookEdit(
// The body size cap is enforced by the MaxBodySize middleware,
// which runs before CSRF parses the form.
name := r.PostFormValue("name")
- if name == "" {
- data := map[string]any{
- tmplKeyWebhook: webhook,
- tmplKeyError: "Name is required",
- }
+ description := r.PostFormValue("description")
+ retention := r.PostFormValue("retention_days")
- h.renderTemplateStatus(w, r, "source_edit.html", data, http.StatusBadRequest)
+ if name == "" {
+ h.renderWebhookEdit(
+ w, r, webhook, name, description, retention,
+ "Name is required", http.StatusBadRequest,
+ )
return
}
- oldName := webhook.Name
- webhook.Name = name
- webhook.Description = r.PostFormValue("description")
-
// An empty field falls back to the stored value, so submitting the
// form without touching retention leaves the policy alone.
retentionDays, errMsg := parseRetentionDays(
- r.PostFormValue("retention_days"), webhook.RetentionDays,
+ retention, webhook.RetentionDays,
)
if errMsg != "" {
- data := map[string]any{
- tmplKeyWebhook: webhook,
- tmplKeyError: errMsg,
- }
-
- h.renderTemplateStatus(w, r, "source_edit.html", data, http.StatusBadRequest)
+ h.renderWebhookEdit(
+ w, r, webhook, name, description, retention,
+ errMsg, http.StatusBadRequest,
+ )
return
}
- webhook.RetentionDays = retentionDays
+ // edited is the webhook as the submission leaves it; webhook stays
+ // as stored, for the page shown again when the save is refused.
+ edited := *webhook
+ edited.Name = name
+ edited.Description = description
+ edited.RetentionDays = retentionDays
// A new name renames the archive files before it is saved (see
// delivery.Engine.Rename). If either step fails, the same targets'
// archives go back to the name that is still stored, without
// reading the main database again.
targets, err := h.renameWebhookArchives(
- webhook.ID, oldName, webhook.Name,
+ webhook.ID, webhook.Name, edited.Name,
)
if err == nil {
- err = h.db.DB().Save(webhook).Error
+ err = h.db.DB().Save(&edited).Error
}
if err != nil {
- restoreErr := h.renameArchives(targets, oldName)
+ restoreErr := h.renameArchives(targets, webhook.Name)
if restoreErr != nil {
h.log.Error(
"failed to rename archives back",
@@ -773,15 +774,14 @@ func (h *Handlers) applyWebhookEdit(
}
if errors.Is(err, delivery.ErrArchiveNameTaken) {
- data := map[string]any{
- tmplKeyWebhook: webhook,
- tmplKeyError: "Not saved: " + err.Error() +
- ". Move that archive out of the data directory, " +
- "its .db together with any -wal and -shm beside " +
+ h.renderWebhookEdit(
+ w, r, webhook, name, description, retention,
+ "Not saved: "+err.Error()+
+ ". Move that archive out of the data directory, "+
+ "its .db together with any -wal and -shm beside "+
"it, then save again.",
- }
-
- h.renderTemplateStatus(w, r, "source_edit.html", data, http.StatusConflict)
+ http.StatusConflict,
+ )
return
}
@@ -797,6 +797,27 @@ func (h *Handlers) applyWebhookEdit(
)
}
+// renderWebhookEdit renders the webhook edit page for the webhook as
+// stored, its form showing name, description and retentionDays, with
+// an optional error message above it.
+func (h *Handlers) renderWebhookEdit(
+ w http.ResponseWriter,
+ r *http.Request,
+ webhook *database.Webhook,
+ name, description, retentionDays, errMsg string,
+ status int,
+) {
+ data := map[string]any{
+ tmplKeyWebhook: webhook,
+ tmplKeyError: errMsg,
+ "Name": name,
+ "Description": description,
+ "RetentionDays": retentionDays,
+ }
+
+ h.renderTemplateStatus(w, r, "source_edit.html", data, status)
+}
+
// HandleSourceDelete handles webhook deletion.
func (h *Handlers) HandleSourceDelete() http.HandlerFunc {
return func(w http.ResponseWriter, r *http.Request) {
@@ -1668,49 +1689,57 @@ func (h *Handlers) newTarget(
webhookID string,
in targetFormInput,
) (*database.Target, string, error) {
- if in.Name == "" {
- return nil, "Name is required", nil
+ target := &database.Target{
+ WebhookID: webhookID,
+ Type: in.Type,
+ Active: true,
}
- if !isValidTargetType(in.Type) {
- return nil, "Invalid target type", nil
- }
-
- configJSON, errMsg, err := h.buildTargetConfig(ctx, in.Type, in)
+ errMsg, err := h.setTargetFromForm(ctx, target, in)
if err != nil || errMsg != "" {
return nil, errMsg, err
}
- // 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 refused rather than becoming
- // that default.
- maxRetries, err := parseMaxRetries(in.MaxRetries, 0)
- if err != nil {
- return nil, "Invalid max retries: " + retriesErrorMessage(err), nil
- }
-
- return &database.Target{
- WebhookID: webhookID,
- Name: in.Name,
- Type: in.Type,
- Active: true,
- Config: configJSON,
- MaxRetries: maxRetries,
- }, "", nil
+ return target, "", nil
}
-// isValidTargetType checks whether the target type is supported.
-func isValidTargetType(tt database.TargetType) bool {
- switch tt {
- case database.TargetTypeHTTP,
- database.TargetTypeDatabase,
- database.TargetTypeLog,
- database.TargetTypeSlack:
- return true
- default:
- return false
+// setTargetFromForm validates a target form against the target's type
+// and, when it accepts it, sets the target's name, configuration and
+// retry count from it. It returns the message the form shows for
+// anything it refuses, an unknown type among them, and then leaves the
+// target unchanged; an error is the server's fault, as for newTarget.
+// The add target form and the target edit form both go through here,
+// so the two cannot come to disagree about what a target may be.
+func (h *Handlers) setTargetFromForm(
+ ctx context.Context,
+ target *database.Target,
+ in targetFormInput,
+) (string, error) {
+ if in.Name == "" {
+ return "Name is required", nil
}
+
+ configJSON, errMsg, err := h.buildTargetConfig(ctx, target.Type, in)
+ if err != nil || errMsg != "" {
+ return errMsg, err
+ }
+
+ // An empty max_retries keeps the target's count: the
+ // fire-and-forget default of 0 for a new target, and the stored
+ // count for an edited one, since the forms for target types that
+ // do not retry have no such field. A value that is filled in but
+ // invalid is refused rather than becoming that count, so a typo
+ // cannot destroy the count a target is delivering with.
+ maxRetries, err := parseMaxRetries(in.MaxRetries, target.MaxRetries)
+ if err != nil {
+ return "Invalid max retries: " + retriesErrorMessage(err), nil
+ }
+
+ target.Name = in.Name
+ target.Config = configJSON
+ target.MaxRetries = maxRetries
+
+ return "", nil
}
// pageOrFirst parses a paginated page number, answering 1 for
@@ -1732,9 +1761,10 @@ func pageOrFirst(s string) int {
}
// targetFormInput carries the raw values of a target form. Both the
-// create and the edit path fill one and hand it to buildTargetConfig,
-// so neither can come to validate a destination differently from the
-// other. A refused add target form is shown again from it.
+// create and the edit path fill one and hand it to setTargetFromForm,
+// so neither can come to validate a target differently from the
+// other. Both forms are filled from one: the edit form with the
+// stored values, and a refused form with the values submitted.
type targetFormInput struct {
// Name is the target's name.
Name string
diff --git a/internal/handlers/source_management_test.go b/internal/handlers/source_management_test.go
index c6d967a..1021df4 100644
--- a/internal/handlers/source_management_test.go
+++ b/internal/handlers/source_management_test.go
@@ -509,6 +509,51 @@ func TestHandleSourceEditSubmit_InvalidRetentionIsRejected(
)
}
+// TestHandleSourceEditSubmit_RefusedFormComesBack refuses an edit for
+// each reason the form can give and checks that the form comes back
+// with the reason and the name, description and retention submitted,
+// that the page still reports the stored retention, and that nothing
+// is saved.
+func TestHandleSourceEditSubmit_RefusedFormComesBack(t *testing.T) {
+ t.Parallel()
+
+ env := setupSourceTest(t)
+
+ refused := func(name, retention, reason string) {
+ t.Helper()
+
+ wh := seedWebhookWithRetention(t, env.db, 30)
+
+ submitted := wh
+ submitted.Name = name
+ submitted.Description = "a description worth keeping"
+
+ w := submitEdit(t, env, submitted, retention)
+ assert.Equal(t, http.StatusBadRequest, w.Code)
+
+ page := w.Body.String()
+ assert.Contains(t, page, `class="alert-error">`+reason)
+ assert.Contains(t, page, `name="name" value="`+name+`"`)
+ assert.Contains(t, page, ">a description worth keeping")
+ assert.Contains(
+ t, page, `name="retention_days" value="`+retention+`"`,
+ )
+ assert.Contains(t, page, "Currently 30 days.")
+
+ var stored database.Webhook
+
+ require.NoError(
+ t, env.db.DB().First(&stored, "id = ?", wh.ID).Error,
+ )
+ assert.Equal(t, wh.Name, stored.Name)
+ assert.Empty(t, stored.Description)
+ assert.Equal(t, 30, stored.RetentionDays)
+ }
+
+ refused("", "45", "Name is required")
+ refused("kept-name", "nonsense", "Retention must be")
+}
+
func TestHandleSourceEditSubmit_EmptyRetentionLeavesValueUnchanged(
t *testing.T,
) {
@@ -809,6 +854,10 @@ func TestHandleSourceEditSubmit_ArchiveNameTaken(t *testing.T) {
w := submitEdit(t, env, wh, "")
require.Equal(t, http.StatusConflict, w.Code)
assert.Contains(t, w.Body.String(), "archive-taken.db")
+ assert.Contains(
+ t, w.Body.String(), `name="name" value="`+renamedWebhookName+`"`,
+ "the form comes back with the name submitted",
+ )
var stored database.Webhook
diff --git a/internal/handlers/target_edit.go b/internal/handlers/target_edit.go
index 10a1e65..81a553f 100644
--- a/internal/handlers/target_edit.go
+++ b/internal/handlers/target_edit.go
@@ -3,6 +3,7 @@ package handlers
import (
"errors"
"net/http"
+ "strconv"
"github.com/go-chi/chi"
"sneak.berlin/go/webhooker/internal/database"
@@ -13,10 +14,13 @@ import (
const targetEditTemplate = "target_edit.html"
// tmplKeyTarget is the template data key for the target being
-// edited, and tmplKeyMaxTimeout for the timeout ceiling the form
-// tells the user about.
+// edited, tmplKeyTargetForm for the values its form shows, and
+// tmplKeyMaxTimeout for the timeout ceiling the form tells the user
+// about. The add target form on the webhook page takes its values
+// under the same key as the edit form.
const (
tmplKeyTarget = "Target"
+ tmplKeyTargetForm = "TargetForm"
tmplKeyMaxTimeout = "MaxTimeout"
)
@@ -28,20 +32,19 @@ const configUnreadableMessage = "The stored configuration for this " +
"target could not be read. Enter the values below; saving " +
"replaces the stored configuration."
-// targetEditView is the display model for the target edit page.
+// targetEditView is the display model for the target edit page: the
+// target's row fields as stored. The values the form shows, the
+// UNMASKED configuration among them, come separately, as a
+// targetFormInput.
//
-// It carries the target's row fields alongside its UNMASKED
-// configuration, and deliberately omits database.Target's raw
-// Config blob: the form renders named fields, and giving the
-// template the blob as well would put an unreviewed second path to
-// the credential on the page.
+// It deliberately omits database.Target's raw Config blob: the form
+// renders named fields, and giving the template the blob as well
+// would put an unreviewed second path to the credential on the page.
type targetEditView struct {
- ID string
- Name string
- Type database.TargetType
- Active bool
- MaxRetries int
- Config delivery.TargetConfigForm
+ ID string
+ Name string
+ Type database.TargetType
+ Active bool
}
// HandleTargetEdit shows the form to edit a target.
@@ -73,7 +76,18 @@ func (h *Handlers) HandleTargetEdit() http.HandlerFunc {
msg = configUnreadableMessage
}
- h.renderTargetEdit(w, r, webhook, target, cfg, msg)
+ form := targetFormInput{
+ Name: target.Name,
+ URL: cfg.URL,
+ Headers: cfg.Headers,
+ Timeout: cfg.Timeout,
+ MaxRetries: strconv.Itoa(target.MaxRetries),
+ Expiry: cfg.Expiry,
+ }
+
+ h.renderTargetEdit(
+ w, r, webhook, target, form, msg, http.StatusOK,
+ )
}
}
@@ -101,11 +115,12 @@ func (h *Handlers) HandleTargetEditSubmit() http.HandlerFunc {
}
}
-// applyTargetEdit validates and saves target edits.
+// applyTargetEdit validates and saves target edits. A refused save
+// shows the edit form again with the values submitted and the reason.
//
-// The submitted configuration goes through buildTargetConfig, the
-// same builder the create path uses, so an edited destination is
-// SSRF-validated exactly as a new one is.
+// The submission goes through setTargetFromForm, as a new target
+// does, so an edited destination is SSRF-validated exactly as a new
+// one is.
//
// The target's type is not editable. Each type stores a different
// configuration shape and its delivery history is recorded against
@@ -118,16 +133,13 @@ func (h *Handlers) applyTargetEdit(
webhook database.Webhook,
target *database.Target,
) {
- name := r.PostFormValue("name")
- if name == "" {
- http.Error(w, "Name is required", http.StatusBadRequest)
+ in := targetFormInputFrom(r)
- return
- }
+ // edited is the target as the submission leaves it; target stays
+ // as stored, for the page shown again when the save is refused.
+ edited := *target
- configJSON, errMsg, err := h.buildTargetConfig(
- r.Context(), target.Type, targetFormInputFrom(r),
- )
+ errMsg, err := h.setTargetFromForm(r.Context(), &edited, in)
if err != nil {
h.serverError(w, r, "failed to encode target config", err)
@@ -135,45 +147,26 @@ func (h *Handlers) applyTargetEdit(
}
if errMsg != "" {
- http.Error(w, errMsg, http.StatusBadRequest)
+ h.renderTargetEdit(
+ w, r, webhook, target, in, errMsg, http.StatusBadRequest,
+ )
return
}
- // 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") {
- retries, ok := targetMaxRetries(w, r, target.MaxRetries)
- if !ok {
- return
- }
-
- target.MaxRetries = retries
- }
-
- oldName := target.Name
- target.Name = name
- target.Config = configJSON
-
// A new name renames the archive file before it is saved (see
// delivery.Engine.Rename). If either step fails, it goes back to
// the name that is still stored.
- err = h.renameTargetArchive(target, webhook.Name, oldName, name)
+ err = h.renameTargetArchive(
+ target, webhook.Name, target.Name, edited.Name,
+ )
if err == nil {
- err = h.db.DB().Save(target).Error
+ err = h.db.DB().Save(&edited).Error
}
if err != nil {
restoreErr := h.renameTargetArchive(
- target, webhook.Name, name, oldName,
+ target, webhook.Name, edited.Name, target.Name,
)
if restoreErr != nil {
h.log.Error(
@@ -184,8 +177,8 @@ func (h *Handlers) applyTargetEdit(
}
if errors.Is(err, delivery.ErrArchiveNameTaken) {
- http.Error(
- w,
+ h.renderTargetEdit(
+ w, r, webhook, target, in,
"Not saved: "+err.Error()+
". Move that archive out of the data directory, "+
"its .db together with any -wal and -shm beside "+
@@ -222,15 +215,17 @@ func (h *Handlers) renameTargetArchive(
return h.archives.Rename(target.ID, webhookName, newName)
}
-// renderTargetEdit renders the target edit page with an optional
-// error message.
+// renderTargetEdit renders the target edit page for the target as
+// stored, its form showing form's values, with an optional error
+// message above it.
func (h *Handlers) renderTargetEdit(
w http.ResponseWriter,
r *http.Request,
webhook database.Webhook,
target *database.Target,
- cfg delivery.TargetConfigForm,
+ form targetFormInput,
errMsg string,
+ status int,
) {
// The template calls Webhook methods, which take pointer
// receivers; html/template cannot address a value stored in a
@@ -238,18 +233,17 @@ func (h *Handlers) renderTargetEdit(
data := map[string]any{
tmplKeyWebhook: &webhook,
tmplKeyTarget: targetEditView{
- ID: target.ID,
- Name: target.Name,
- Type: target.Type,
- Active: target.Active,
- MaxRetries: target.MaxRetries,
- Config: cfg,
+ ID: target.ID,
+ Name: target.Name,
+ Type: target.Type,
+ Active: target.Active,
},
+ tmplKeyTargetForm: form,
tmplKeyMaxTimeout: delivery.MaxTargetTimeoutSeconds,
tmplKeyError: errMsg,
}
- h.renderTemplate(w, r, targetEditTemplate, data)
+ h.renderTemplateStatus(w, r, targetEditTemplate, data, status)
}
// ownedTarget resolves the request's sourceID and targetID
diff --git a/internal/handlers/target_edit_test.go b/internal/handlers/target_edit_test.go
index 942d765..bc7f652 100644
--- a/internal/handlers/target_edit_test.go
+++ b/internal/handlers/target_edit_test.go
@@ -565,6 +565,73 @@ func assertEditRejectsTimeout(
)
}
+// TestHandleTargetEditSubmit_RefusedFormComesBack refuses an edit of
+// a target of each type and checks that the edit form comes back with
+// the reason and every value submitted, and that nothing is saved.
+func TestHandleTargetEditSubmit_RefusedFormComesBack(t *testing.T) {
+ t.Parallel()
+
+ env := setupSourceTest(t)
+
+ // fields is what the operator submitted, as a query string.
+ cases := []struct {
+ targetType database.TargetType
+ fields string
+ reason string
+ }{
+ {
+ database.TargetTypeHTTP,
+ "name=edited&url=" + editBlockedURL +
+ "&headers=X-Edited:+kept&timeout=12&max_retries=3",
+ "Invalid target URL",
+ },
+ {
+ database.TargetTypeSlack,
+ "name=edited&url=" + editOriginalURL + "&max_retries=25",
+ "Invalid max retries",
+ },
+ {
+ database.TargetTypeDatabase, "name=edited&expiry=7d",
+ "Invalid archive expiry",
+ },
+ {database.TargetTypeLog, "name=", "Name is required"},
+ }
+
+ for _, tc := range cases {
+ t.Run(string(tc.targetType), func(t *testing.T) {
+ t.Parallel()
+
+ webhook := seedWebhookWithRetention(t, env.db, 30)
+ target := seedTarget(t, env.db, webhook.ID, tc.targetType)
+
+ form, err := url.ParseQuery(tc.fields)
+ require.NoError(t, err)
+
+ w := submitTargetEdit(env, webhook.ID, target.ID, form)
+ assert.Equal(t, http.StatusBadRequest, w.Code)
+
+ page := w.Body.String()
+ assert.Contains(t, page, `class="alert-error">`+tc.reason)
+
+ // headers is the form's one textarea; every other field is
+ // an input.
+ for field := range form {
+ shown := `name="` + field + `" value="` + form.Get(field) + `"`
+ if field == "headers" {
+ shown = ">" + form.Get(field) + ""
+ }
+
+ assert.Contains(t, page, shown)
+ }
+
+ assert.Equal(
+ t, target.Name, storedTarget(t, env, target.ID).Name,
+ "a refused edit must save nothing",
+ )
+ })
+ }
+}
+
// TestHandleTargetEdit_Scoping keeps the edit routes scoped the way
// the delete and toggle routes are: ownership is decided by the
// webhook, and the target is then scoped to it.
@@ -700,6 +767,10 @@ func TestHandleTargetEditSubmit_RenamesArchive(t *testing.T) {
w = submitTargetEdit(env, wh.ID, archive.ID, again)
require.Equal(t, http.StatusConflict, w.Code)
assert.Contains(t, w.Body.String(), "archive-taken.db")
+ assert.Contains(
+ t, w.Body.String(), `name="name" value="Again"`,
+ "the form comes back with the name submitted",
+ )
assert.Equal(
t, renamedTargetName, storedTarget(t, env, archive.ID).Name,
)
diff --git a/internal/handlers/target_private_refusal_test.go b/internal/handlers/target_private_refusal_test.go
index 69131ea..a2c6f30 100644
--- a/internal/handlers/target_private_refusal_test.go
+++ b/internal/handlers/target_private_refusal_test.go
@@ -44,9 +44,9 @@ func TestTargetRefusal_PrivateDestinationSaysHowToAllowIt(
form.Set("type", string(targetType))
form.Set("url", editBlockedURL)
- // A refused add shows the webhook page again, where
- // the hint is HTML-escaped; a refused edit answers in
- // plain text.
+ // A refused add shows the webhook page again, and a
+ // refused edit the edit page, where the hint is
+ // HTML-escaped.
added := serveTarget(
env, http.MethodPost, targetsPath, form,
)
@@ -76,7 +76,8 @@ func TestTargetRefusal_PrivateDestinationSaysHowToAllowIt(
)
assert.Equal(t, http.StatusBadRequest, edited.Code)
assert.Contains(
- t, edited.Body.String(), privateRefusalHint,
+ t, edited.Body.String(),
+ html.EscapeString(privateRefusalHint),
)
})
}
diff --git a/internal/handlers/target_retries.go b/internal/handlers/target_retries.go
index 34dadd3..8b68780 100644
--- a/internal/handlers/target_retries.go
+++ b/internal/handlers/target_retries.go
@@ -2,7 +2,6 @@ package handlers
import (
"errors"
- "net/http"
"strconv"
"strings"
)
@@ -89,32 +88,3 @@ func retriesErrorMessage(err error) string {
return errRetriesInvalid.Error() +
", or 0 for fire-and-forget"
}
-
-// targetMaxRetries reads and validates max_retries from a target edit
-// submission, answering the request with a 400 and reporting false
-// when the value is set but invalid.
-//
-// It and the create path (newTarget) both use parseMaxRetries and
-// retriesErrorMessage, 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
-}
diff --git a/internal/handlers/ui_copy_test.go b/internal/handlers/ui_copy_test.go
index 009ec9f..28eca73 100644
--- a/internal/handlers/ui_copy_test.go
+++ b/internal/handlers/ui_copy_test.go
@@ -396,21 +396,21 @@ func TestTargetFormMaxRetriesCopyMatchesBehaviour(t *testing.T) {
)
// A slack target exercises the same max_retries field while needing
- // only Config.URL from the edit template, so the test data stays
- // minimal. The Target key mirrors the field names the template reads
- // off the handler's view value.
+ // only a URL from the edit template, so the test data stays
+ // minimal. The Target and TargetForm keys mirror the field names
+ // the template reads off the handler's values.
editBody := renderPage(
t, h, sess, "target_edit.html", map[string]any{
dataKeyWebhook: webhook,
"Target": map[string]any{
- "ID": "tg-1",
- "Name": "t",
- "Type": "slack",
- "Active": true,
- "MaxRetries": 3,
- "Config": map[string]any{
- "URL": "https://hooks.slack.com/services/x",
- },
+ "ID": "tg-1",
+ "Name": "t",
+ "Type": "slack",
+ "Active": true,
+ },
+ "TargetForm": map[string]any{
+ "URL": "https://hooks.slack.com/services/x",
+ "MaxRetries": "3",
},
dataKeyError: "",
},
diff --git a/internal/server/alpine_browser_test.go b/internal/server/alpine_browser_test.go
index c2ea01f..33892de 100644
--- a/internal/server/alpine_browser_test.go
+++ b/internal/server/alpine_browser_test.go
@@ -59,6 +59,44 @@ func TestAlpineRunsUnderTheSecurityPolicy(t *testing.T) {
t.Cleanup(srv.Close)
userID, _ := env.seedUser(t, "browser", "browser-password")
+ webhook, event, target := seedBrowserWebhook(t, env, userID)
+
+ require.NoError(t, chromedp.Run(
+ ctx, setCookies(srv.URL, env.authCookies(t, userID, "browser")),
+ ))
+
+ page := srv.URL + "/hook/" + webhook.ID
+
+ // The checks share one browser tab, so they run one at a time, in
+ // this order. A new check is one more line here.
+ checkAddEntrypoint(ctx, t, page)
+ checkAddEachTargetType(ctx, t, page)
+ checkRefusedTarget(ctx, t, page)
+ checkTargetDeliveries(ctx, t, page, target.Name,
+ "0 in total, 0 in the last 24 hours",
+ "1 in total, 1 in the last 24 hours")
+ checkRefusedEdits(ctx, t, page, target.ID)
+ checkCopy(ctx, t, page)
+ checkEntrypointEdit(ctx, t, page, page+"/events")
+ checkRecentEvents(ctx, t, page)
+ checkEventLog(ctx, t, page+"/events", event.ID, target.Name)
+ checkArchiveChoice(ctx, t, srv.URL+"/hooks/new", page)
+ checkNewWebhookTargets(ctx, t, env, srv.URL+"/hooks/new")
+ checkRefusedNewWebhook(ctx, t, srv.URL+"/hooks/new")
+ checkMobileMenu(ctx, t, page)
+
+ assert.Empty(t, problems(), "the browser reported problems")
+}
+
+// seedBrowserWebhook seeds the webhook the browser test loads, owned by
+// userID: an entrypoint, two events, and a target whose delivery of the
+// newer event failed once with a 502. It returns the webhook, the newer
+// event and the target.
+func seedBrowserWebhook(
+ t *testing.T, env *testEnv, userID string,
+) (*database.Webhook, *database.Event, *database.Target) {
+ t.Helper()
+
webhook := env.seedWebhook(t, userID)
require.NoError(t, env.db.DB().Omit(clause.Associations).Create(
&database.Entrypoint{
@@ -82,56 +120,7 @@ func TestAlpineRunsUnderTheSecurityPolicy(t *testing.T) {
},
).Error)
- require.NoError(t, chromedp.Run(
- ctx, setCookies(srv.URL, env.authCookies(t, userID, "browser")),
- ))
-
- page := srv.URL + "/hook/" + webhook.ID
-
- checkAddEntrypoint(ctx, t, page)
-
- // Each target type, with the fields its add target form submits, in
- // page order. Only http and slack have a url field.
- targetTypes := []struct {
- name string
- fields string
- values map[string]string
- }{
- {
- "http", "csrf_token name type url headers timeout max_retries",
- map[string]string{"url": publicTargetURL},
- },
- {
- "slack", "csrf_token name type url max_retries",
- map[string]string{"url": publicTargetURL},
- },
- {
- "database", "csrf_token name type expiry",
- map[string]string{"expiry": "720h"},
- },
- {"log", "csrf_token name type", nil},
- }
-
- for _, tt := range targetTypes {
- checkAddTarget(
- ctx, t, page, tt.name, strings.Fields(tt.fields), tt.values,
- )
- }
-
- checkRefusedTarget(ctx, t, page)
- checkTargetDeliveries(ctx, t, page, target.Name,
- "0 in total, 0 in the last 24 hours",
- "1 in total, 1 in the last 24 hours")
- checkCopy(ctx, t, page)
- checkEntrypointEdit(ctx, t, page, page+"/events")
- checkRecentEvents(ctx, t, page)
- checkEventLog(ctx, t, page+"/events", event.ID, target.Name)
- checkArchiveChoice(ctx, t, srv.URL+"/hooks/new", page)
- checkNewWebhookTargets(ctx, t, env, srv.URL+"/hooks/new")
- checkRefusedNewWebhook(ctx, t, srv.URL+"/hooks/new")
- checkMobileMenu(ctx, t, page)
-
- assert.Empty(t, problems(), "the browser reported problems")
+ return webhook, event, target
}
// startBrowser starts a headless browser for one test. It returns the
@@ -310,6 +299,40 @@ const (
document.querySelector('form[action$="/targets"]')).keys()]`
)
+// checkAddEachTargetType runs checkAddTarget on a webhook page for each
+// target type, in page order.
+func checkAddEachTargetType(ctx context.Context, t *testing.T, url string) {
+ t.Helper()
+
+ // Each target type, with the fields its add target form submits, in
+ // page order. Only http and slack have a url field.
+ targetTypes := []struct {
+ name string
+ fields string
+ values map[string]string
+ }{
+ {
+ "http", "csrf_token name type url headers timeout max_retries",
+ map[string]string{"url": publicTargetURL},
+ },
+ {
+ "slack", "csrf_token name type url max_retries",
+ map[string]string{"url": publicTargetURL},
+ },
+ {
+ "database", "csrf_token name type expiry",
+ map[string]string{"expiry": "720h"},
+ },
+ {"log", "csrf_token name type", nil},
+ }
+
+ for _, tt := range targetTypes {
+ checkAddTarget(
+ ctx, t, url, tt.name, strings.Fields(tt.fields), tt.values,
+ )
+ }
+}
+
// checkAddTarget loads a webhook page and walks the add target form for
// one target type. The form shows nothing until Add is clicked; Add
// shows only the type choice; Cancel there closes it; Next shows the
@@ -481,6 +504,64 @@ func checkTargetDeliveries(
"the row of %s does not show %q failed", name, failed)
}
+// checkRefusedEdits fills in the target edit page and the webhook edit
+// page of a webhook page with values the server refuses, a loopback
+// destination and a retention above the longest finite one, which the
+// browser lets through. It saves each and checks that the page comes
+// back with the reason and every value still in its field. The values
+// are keyed by the id of their field.
+func checkRefusedEdits(
+ ctx context.Context, t *testing.T, page, targetID string,
+) {
+ t.Helper()
+
+ const reason = `//div[@class="alert-error"]`
+
+ edits := []struct {
+ url string
+ values map[string]string
+ }{
+ {page + "/targets/" + targetID + "/edit", map[string]string{
+ "#name": "edited-target",
+ "#url": "http://127.0.0.1/hook",
+ "#headers": "X-Edited: kept",
+ "#timeout": "12",
+ "#max_retries": "3",
+ }},
+ {page + "/edit", map[string]string{
+ "#name": "edited-webhook",
+ "#description": "kept description",
+ "#retention_days": "200000",
+ }},
+ }
+
+ for _, edit := range edits {
+ require.NoError(t, chromedp.Run(ctx, loadPage(edit.url)))
+
+ for field, value := range edit.values {
+ require.NoError(t, chromedp.Run(
+ ctx, chromedp.SetValue(field, value, chromedp.ByQuery),
+ ))
+ }
+
+ click(ctx, t, `//button[text()="Save Changes"]`)
+
+ assert.Truef(t, shown(ctx, reason),
+ "%s: a refused save does not show the reason", edit.url)
+
+ for field, value := range edit.values {
+ var kept string
+
+ require.NoError(t, chromedp.Run(
+ ctx, chromedp.Value(field, &kept, chromedp.ByQuery),
+ ))
+ assert.Equalf(t, value, kept,
+ "%s: a refused save does not keep the %s entered",
+ edit.url, field)
+ }
+ }
+}
+
// checkCopy loads a webhook page and checks that the Copy control beside
// its entrypoint's URL is a button, and that clicking it copies the URL
// and says so: the button reads "Copied" only once the copy succeeded.
diff --git a/templates/source_edit.html b/templates/source_edit.html
index 43d2c4f..6605ee7 100644
--- a/templates/source_edit.html
+++ b/templates/source_edit.html
@@ -18,17 +18,17 @@
-
+
-
+
-
+
Currently {{.Webhook.RetentionLabel}}.{{if .Webhook.RetainsForever}} No events are deleted while retention is set to forever.{{else}} A periodic cleanup permanently deletes events older than this, along with their delivery records.{{end}} Enter 0 to retain events forever; leave blank to keep the current setting.
- This form shows the target's stored destination in full, including any credential carried in its URL or headers. It is the only page that does; everywhere else the value is masked.
+ This form shows the target's destination in full, including any credential carried in its URL or headers. It is the only page that does; everywhere else the value is masked.
{{end}}
@@ -26,25 +26,25 @@
-
+
{{if eq .Target.Type "http"}}
-
+
Revalidated on save; destinations that resolve to private or link-local addresses are rejected.
-
+
One Name: value per line, sent with every delivery. Leave blank for none. Host, Content-Length, Transfer-Encoding, Connection, Trailer and User-Agent are set by the delivery engine and are rejected here rather than silently ignored. Headers set here are dropped if a redirect leaves the destination's own origin, so a credential cannot follow one to another host.
-
+
Per-request timeout, at most {{.MaxTimeout}} seconds. Leave blank to use the default.
This is the total number of delivery attempts, not retries on top of the first: a value of 3 makes three attempts in all. 0 means a single attempt with no retries and no circuit breaker.