Targets section: one Add, then a type and Next, then its fields (closes #370)

The webhook page's targets section lists only its targets until Add is
clicked. Add shows a choice of target type with Next; Next shows only
that type's fields, with Save and Cancel. The `database` and `log`
types have no URL field, and the `slack` form gains max retries.

A refused target now shows the webhook page again with the form open on
its type, the values entered and the reason, instead of a bare text
page. Target validation returns that message rather than writing the
response; newTarget validates a whole new target for reuse by the
new-webhook page. The edit page still answers a refusal in plain text.

Model: opus-5-5

internal/handlers/export_test.go

@@ -137,22 +137,20 @@ func (s *Handlers) RenderTemplateForTest(
 // BuildSlackTargetConfigForTest exposes
 // buildSlackTargetConfig for use in the handlers_test package.
 func (s *Handlers) BuildSlackTargetConfigForTest(
-	w http.ResponseWriter,
-	r *http.Request,
+	ctx context.Context,
 	targetURL string,
-) (string, error) {
-	return s.buildSlackTargetConfig(w, r, targetURL)
+) (string, string) {
+	return s.buildSlackTargetConfig(ctx, targetURL)
 }
 
 // BuildHTTPTargetConfigForTest exposes buildHTTPTargetConfig
 // for use in the handlers_test package, taking the form fields
 // an HTTP target's configuration is built from.
 func (s *Handlers) BuildHTTPTargetConfigForTest(
-	w http.ResponseWriter,
-	r *http.Request,
+	ctx context.Context,
 	targetURL, headers, timeout string,
-) (string, error) {
-	return s.buildHTTPTargetConfig(w, r, targetFormInput{
+) (string, string) {
+	return s.buildHTTPTargetConfig(ctx, targetFormInput{
 		URL:     targetURL,
 		Headers: headers,
 		Timeout: timeout,
@@ -162,9 +160,6 @@ func (s *Handlers) BuildHTTPTargetConfigForTest(
 // BuildDatabaseTargetConfigForTest exposes
 // buildDatabaseTargetConfig for use in the handlers_test
 // package.
-func (s *Handlers) BuildDatabaseTargetConfigForTest(
-	w http.ResponseWriter,
-	expiry string,
-) (string, error) {
-	return s.buildDatabaseTargetConfig(w, newRequestForTest(), expiry)
+func BuildDatabaseTargetConfigForTest(expiry string) (string, string) {
+	return buildDatabaseTargetConfig(expiry)
 }

internal/handlers/handlers_test.go

@@ -314,16 +314,11 @@ func TestBuildSlackTargetConfig_AcceptsPublicURL(t *testing.T) {
 
 	t.Cleanup(app.RequireStop)
 
-	req := httptest.NewRequestWithContext(
-		context.Background(), http.MethodPost, "/", nil)
-	w := httptest.NewRecorder()
-
-	cfg, err := h.BuildSlackTargetConfigForTest(
-		w, req, "http://93.184.216.34/services/T00/B00/xxx",
+	cfg, errMsg := h.BuildSlackTargetConfigForTest(
+		t.Context(), "http://93.184.216.34/services/T00/B00/xxx",
 	)
 
-	require.NoError(t, err)
-	assert.Equal(t, http.StatusOK, w.Code)
+	assert.Empty(t, errMsg)
 	assert.Contains(t, cfg, "webhookUrl")
 }
 
@@ -337,17 +332,12 @@ func TestBuildSlackTargetConfig_RejectsReservedURL(t *testing.T) {
 
 	t.Cleanup(app.RequireStop)
 
-	req := httptest.NewRequestWithContext(
-		context.Background(), http.MethodPost, "/", nil)
-	w := httptest.NewRecorder()
-
-	cfg, err := h.BuildSlackTargetConfigForTest(
-		w, req, "http://169.254.169.254/latest/meta-data/",
+	cfg, errMsg := h.BuildSlackTargetConfigForTest(
+		t.Context(), "http://169.254.169.254/latest/meta-data/",
 	)
 
-	require.Error(t, err)
+	assert.Contains(t, errMsg, "Invalid target URL")
 	assert.Empty(t, cfg)
-	assert.Equal(t, http.StatusBadRequest, w.Code)
 }
 
 func TestRenderTemplate(t *testing.T) {
@@ -444,29 +434,19 @@ func TestRenderTemplateMidRenderErrorSendsNoPartialBody(t *testing.T) {
 func TestBuildDatabaseTargetConfig_Valid(t *testing.T) {
 	t.Parallel()
 
-	var h *handlers.Handlers
-
-	app := newTestApp(t, &h)
-	app.RequireStart()
-
-	t.Cleanup(app.RequireStop)
-
 	// Empty expiry: the keep-forever default, empty config.
-	w := httptest.NewRecorder()
-	cfg, err := h.BuildDatabaseTargetConfigForTest(w, "")
-	require.NoError(t, err)
+	cfg, errMsg := handlers.BuildDatabaseTargetConfigForTest("")
+	assert.Empty(t, errMsg)
 	assert.Empty(t, cfg)
 
 	// Explicit never is stored as config.
-	w = httptest.NewRecorder()
-	cfg, err = h.BuildDatabaseTargetConfigForTest(w, "never")
-	require.NoError(t, err)
+	cfg, errMsg = handlers.BuildDatabaseTargetConfigForTest("never")
+	assert.Empty(t, errMsg)
 	assert.JSONEq(t, `{"expiry":"never"}`, cfg)
 
 	// A positive duration is stored as config.
-	w = httptest.NewRecorder()
-	cfg, err = h.BuildDatabaseTargetConfigForTest(w, "720h")
-	require.NoError(t, err)
+	cfg, errMsg = handlers.BuildDatabaseTargetConfigForTest("720h")
+	assert.Empty(t, errMsg)
 	assert.JSONEq(t, `{"expiry":"720h"}`, cfg)
 }
 
@@ -475,22 +455,13 @@ func TestBuildDatabaseTargetConfig_RejectsBadExpiry(
 ) {
 	t.Parallel()
 
-	var h *handlers.Handlers
-
-	app := newTestApp(t, &h)
-	app.RequireStart()
-
-	t.Cleanup(app.RequireStop)
-
 	for _, bad := range []string{"nonsense", "7d", "-5h"} {
-		w := httptest.NewRecorder()
-		cfg, err := h.BuildDatabaseTargetConfigForTest(w, bad)
+		cfg, errMsg := handlers.BuildDatabaseTargetConfigForTest(bad)
 
-		require.Error(t, err, "expiry %q", bad)
-		assert.Empty(t, cfg)
-		assert.Equal(
-			t, http.StatusBadRequest, w.Code,
-			"expiry %q should be rejected with 400", bad,
+		assert.Contains(
+			t, errMsg, "Invalid archive expiry",
+			"expiry %q should be refused", bad,
 		)
+		assert.Empty(t, cfg)
 	}
 }

internal/handlers/source_management.go

@@ -1,6 +1,7 @@
 package handlers
 
 import (
+	"context"
 	"encoding/json"
 	"errors"
 	"fmt"
@@ -38,9 +39,6 @@ type WebhookListItem struct {
 	EventsUnreadable  bool
 }
 
-// errMissingURL signals that a required URL was not provided.
-var errMissingURL = errors.New("missing URL")
-
 // 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
@@ -460,15 +458,20 @@ func (h *Handlers) HandleSourceDetail() http.HandlerFunc {
 			return
 		}
 
-		h.renderSourceDetail(w, r, webhook)
+		h.renderSourceDetail(w, r, webhook, targetFormInput{}, "")
 	}
 }
 
-// renderSourceDetail loads and renders a source detail page.
+// renderSourceDetail loads and renders a source detail page. With a
+// targetErr, it is the page shown again for a refused add target
+// form: it answers 400, and the form opens on targetForm's type with
+// its values and the message.
 func (h *Handlers) renderSourceDetail(
 	w http.ResponseWriter,
 	r *http.Request,
 	webhook database.Webhook,
+	targetForm targetFormInput,
+	targetErr string,
 ) {
 	var entrypoints []database.Entrypoint
 
@@ -525,9 +528,16 @@ func (h *Handlers) renderSourceDetail(
 		"Events":      events,
 		"BaseURL":     baseURL,
 		"Stats":       h.loadWebhookStats(webhook.ID, entrypoints, targets),
+		"TargetForm":  targetForm,
+		"TargetError": targetErr,
 	}
 
-	h.renderTemplate(w, r, "source_detail.html", data)
+	status := http.StatusOK
+	if targetErr != "" {
+		status = http.StatusBadRequest
+	}
+
+	h.renderTemplateStatus(w, r, "source_detail.html", data, status)
 }
 
 // HandleSourceEdit shows the form to edit a webhook.
@@ -1501,67 +1511,24 @@ func (h *Handlers) HandleTargetCreate() http.HandlerFunc {
 	}
 }
 
-// processTargetCreate validates and creates a new target.
+// processTargetCreate validates and creates a new target. A refused
+// submission shows the webhook page again, with the add target form
+// open on the chosen type, the values entered, and the reason.
 func (h *Handlers) processTargetCreate(
 	w http.ResponseWriter,
 	r *http.Request,
 	webhook database.Webhook,
 ) {
-	// The body size cap is enforced by the MaxBodySize middleware,
-	// which runs before CSRF parses the form.
-	//
-	// Every field here is read with PostFormValue, not FormValue.
-	// FormValue falls back to the query string, which would let
-	// `POST /hook/{id}/targets?url=https://hooks.slack.com/...`
-	// configure a target from a value the request line carries — and
-	// the request line, unlike the body, is what logs, proxies,
-	// Referer headers and error trackers record.
-	name := r.PostFormValue("name")
-	targetType := database.TargetType(r.PostFormValue("type"))
+	in := targetFormInputFrom(r)
 
-	if name == "" {
-		http.Error(
-			w, "Name is required", http.StatusBadRequest,
-		)
+	target, errMsg := h.newTarget(r.Context(), webhook.ID, in)
+	if errMsg != "" {
+		h.renderSourceDetail(w, r, webhook, in, errMsg)
 
 		return
 	}
 
-	if !isValidTargetType(targetType) {
-		http.Error(
-			w, "Invalid target type",
-			http.StatusBadRequest,
-		)
-
-		return
-	}
-
-	configJSON, err := h.buildTargetConfig(
-		w, r, targetType, targetFormInputFrom(r),
-	)
-	if err != nil {
-		return
-	}
-
-	// 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 rejected rather than becoming
-	// that default.
-	maxRetries, ok := targetMaxRetries(w, r, 0)
-	if !ok {
-		return
-	}
-
-	target := &database.Target{
-		WebhookID:  webhook.ID,
-		Name:       name,
-		Type:       targetType,
-		Active:     true,
-		Config:     configJSON,
-		MaxRetries: maxRetries,
-	}
-
-	err = h.db.DB().Create(target).Error
+	err := h.db.DB().Create(target).Error
 	if err != nil {
 		h.serverError(w, r, "failed to create target", err)
 
@@ -1574,6 +1541,47 @@ func (h *Handlers) processTargetCreate(
 	)
 }
 
+// newTarget validates a new target for a webhook and returns the row
+// to create, or, when it refuses the target, the message the form
+// shows. Every form that creates a target goes through here, so they
+// all accept and refuse the same things.
+func (h *Handlers) newTarget(
+	ctx context.Context,
+	webhookID string,
+	in targetFormInput,
+) (*database.Target, string) {
+	if in.Name == "" {
+		return nil, "Name is required"
+	}
+
+	if !isValidTargetType(in.Type) {
+		return nil, "Invalid target type"
+	}
+
+	configJSON, errMsg := h.buildTargetConfig(ctx, in.Type, in)
+	if errMsg != "" {
+		return nil, errMsg
+	}
+
+	// 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)
+	}
+
+	return &database.Target{
+		WebhookID:  webhookID,
+		Name:       in.Name,
+		Type:       in.Type,
+		Active:     true,
+		Config:     configJSON,
+		MaxRetries: maxRetries,
+	}, ""
+}
+
 // isValidTargetType checks whether the target type is supported.
 func isValidTargetType(tt database.TargetType) bool {
 	switch tt {
@@ -1605,11 +1613,16 @@ func pageOrFirst(s string) int {
 	return v
 }
 
-// targetFormInput carries the raw form values describing a target's
-// configuration. 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.
+// 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.
 type targetFormInput struct {
+	// Name is the target's name.
+	Name string
+	// Type is the type chosen on the add target form. The edit form
+	// has none: a target's stored type decides.
+	Type database.TargetType
 	// URL is the destination for an HTTP target and the webhook URL
 	// for a Slack target.
 	URL string
@@ -1618,13 +1631,15 @@ type targetFormInput struct {
 	Headers string
 	// Timeout is an HTTP target's per-request timeout in seconds.
 	Timeout string
+	// MaxRetries is an HTTP or Slack target's max_retries.
+	MaxRetries string
 	// Expiry is a database (archive) target's row expiry.
 	Expiry string
 }
 
-// targetFormInputFrom reads the configuration fields from a request
-// body. The body size cap is enforced by the MaxBodySize middleware,
-// which runs before CSRF parses the form.
+// targetFormInputFrom reads a target form from a request body. The
+// body size cap is enforced by the MaxBodySize middleware, which runs
+// before CSRF parses the form.
 //
 // Every field is read with PostFormValue, not FormValue. FormValue
 // falls back to the query string, which would let
@@ -1636,38 +1651,36 @@ type targetFormInput struct {
 // tokens.
 func targetFormInputFrom(r *http.Request) targetFormInput {
 	return targetFormInput{
-		URL:     r.PostFormValue("url"),
-		Headers: r.PostFormValue("headers"),
-		Timeout: r.PostFormValue("timeout"),
-		Expiry:  r.PostFormValue("expiry"),
+		Name:       r.PostFormValue("name"),
+		Type:       database.TargetType(r.PostFormValue("type")),
+		URL:        r.PostFormValue("url"),
+		Headers:    r.PostFormValue("headers"),
+		Timeout:    r.PostFormValue("timeout"),
+		MaxRetries: r.PostFormValue("max_retries"),
+		Expiry:     r.PostFormValue("expiry"),
 	}
 }
 
 // buildTargetConfig builds the JSON config string for a target from
-// the submitted form values, writing its own 4xx response on
-// rejection. Which fields of in apply depends on the target type.
+// the submitted form values, or returns the message the form shows
+// for a value it refuses. Which fields of in apply depends on the
+// target type; a type without a URL ignores any URL submitted.
 func (h *Handlers) buildTargetConfig(
-	w http.ResponseWriter,
-	r *http.Request,
+	ctx context.Context,
 	targetType database.TargetType,
 	in targetFormInput,
-) (string, error) {
+) (string, string) {
 	switch targetType {
 	case database.TargetTypeHTTP:
-		return h.buildHTTPTargetConfig(w, r, in)
+		return h.buildHTTPTargetConfig(ctx, in)
 	case database.TargetTypeSlack:
-		return h.buildSlackTargetConfig(w, r, in.URL)
+		return h.buildSlackTargetConfig(ctx, in.URL)
 	case database.TargetTypeDatabase:
-		return h.buildDatabaseTargetConfig(w, r, in.Expiry)
+		return buildDatabaseTargetConfig(in.Expiry)
 	case database.TargetTypeLog:
-		return "", nil
+		return "", ""
 	default:
-		http.Error(
-			w, "Invalid target type",
-			http.StatusBadRequest,
-		)
-
-		return "", errMissingURL
+		return "", "Invalid target type"
 	}
 }
 
@@ -1675,40 +1688,27 @@ func (h *Handlers) buildTargetConfig(
 // SSRF-validated destination plus the optional headers and timeout
 // the delivery path honours.
 func (h *Handlers) buildHTTPTargetConfig(
-	w http.ResponseWriter,
-	r *http.Request,
+	ctx context.Context,
 	in targetFormInput,
-) (string, error) {
-	err := h.validateTargetURL(
-		w, r, in.URL, "URL is required for HTTP targets",
+) (string, string) {
+	errMsg := h.validateTargetURL(
+		ctx, in.URL, "URL is required for HTTP targets",
 	)
-	if err != nil {
-		return "", err
+	if errMsg != "" {
+		return "", errMsg
 	}
 
 	headers, err := delivery.ParseTargetHeaders(in.Headers)
 	if err != nil {
-		http.Error(
-			w,
-			"Invalid headers: "+err.Error(),
-			http.StatusBadRequest,
-		)
-
-		return "", err
+		return "", "Invalid headers: " + err.Error()
 	}
 
 	timeout, err := delivery.ParseTargetTimeout(in.Timeout)
 	if err != nil {
-		http.Error(
-			w,
-			"Invalid timeout: "+err.Error(),
-			http.StatusBadRequest,
-		)
-
-		return "", err
+		return "", "Invalid timeout: " + err.Error()
 	}
 
-	return h.marshalTargetConfig(w, r, delivery.HTTPTargetConfig{
+	return marshalTargetConfig(delivery.HTTPTargetConfig{
 		URL:     in.URL,
 		Headers: headers,
 		Timeout: timeout,
@@ -1718,49 +1718,39 @@ func (h *Handlers) buildHTTPTargetConfig(
 // buildSlackTargetConfig builds config JSON for a Slack target,
 // whose whole configuration is one SSRF-validated webhook URL.
 func (h *Handlers) buildSlackTargetConfig(
-	w http.ResponseWriter,
-	r *http.Request,
+	ctx context.Context,
 	targetURL string,
-) (string, error) {
-	err := h.validateTargetURL(
-		w, r, targetURL,
+) (string, string) {
+	errMsg := h.validateTargetURL(
+		ctx, targetURL,
 		"Webhook URL is required for Slack targets",
 	)
-	if err != nil {
-		return "", err
+	if errMsg != "" {
+		return "", errMsg
 	}
 
-	return h.marshalTargetConfig(w, r, delivery.SlackTargetConfig{
+	return marshalTargetConfig(delivery.SlackTargetConfig{
 		WebhookURL: targetURL,
 	})
 }
 
-// validateTargetURL rejects an empty or SSRF-blocked destination,
-// writing the 400 itself. missingMsg is the error shown when no URL
-// is given.
+// validateTargetURL refuses an empty or SSRF-blocked destination,
+// returning the message the form shows, or "" when the destination
+// is accepted. missingMsg is the message for no URL at all.
 //
 // It is the single point at which a user-supplied destination enters
 // the SSRF guard, on create and on edit alike. An edit path that
 // reached storage without passing through here would reopen the hole
 // the guard closes.
 func (h *Handlers) validateTargetURL(
-	w http.ResponseWriter,
-	r *http.Request,
+	ctx context.Context,
 	targetURL, missingMsg string,
-) error {
+) string {
 	if targetURL == "" {
-		http.Error(
-			w,
-			missingMsg,
-			http.StatusBadRequest,
-		)
-
-		return errMissingURL
+		return missingMsg
 	}
 
-	err := h.ssrf.ValidateTargetURL(
-		r.Context(), targetURL,
-	)
+	err := h.ssrf.ValidateTargetURL(ctx, targetURL)
 	if err != nil {
 		// The submitted URL can be a credential (a Slack
 		// incoming webhook URL is a bearer token), so the log
@@ -1786,61 +1776,41 @@ func (h *Handlers) validateTargetURL(
 				"egress to your own network\" in the README)."
 		}
 
-		http.Error(w, msg, http.StatusBadRequest)
-
-		return err
+		return msg
 	}
 
-	return nil
+	return ""
 }
 
 // marshalTargetConfig serialises a target configuration for storage,
-// writing a 500 itself if it cannot.
-func (h *Handlers) marshalTargetConfig(
-	w http.ResponseWriter,
-	r *http.Request,
-	cfg any,
-) (string, error) {
+// or returns the message the form shows if it cannot.
+func marshalTargetConfig(cfg any) (string, string) {
 	configBytes, err := json.Marshal(cfg)
 	if err != nil {
-		h.serverError(w, r, "failed to encode target config", err)
-
-		return "", err
+		return "", "Could not encode the target configuration: " +
+			err.Error()
 	}
 
-	return string(configBytes), nil
+	return string(configBytes), ""
 }
 
 // buildDatabaseTargetConfig builds config JSON for a database
-// (archive) target. The optional expiry (a form value read by
-// the caller, which bounds the request body) is validated here,
-// at creation time, so an unparseable value is rejected with a
-// 400 instead of failing every subsequent delivery. An empty
-// expiry yields an empty config (the keep-forever default).
-func (h *Handlers) buildDatabaseTargetConfig(
-	w http.ResponseWriter,
-	r *http.Request,
-	expiry string,
-) (string, error) {
+// (archive) target. The optional expiry is validated here, at
+// creation time, so an unparseable value is refused instead of
+// failing every subsequent delivery. An empty expiry yields an
+// empty config (the keep-forever default).
+func buildDatabaseTargetConfig(expiry string) (string, string) {
 	expiry = strings.TrimSpace(expiry)
 	if expiry == "" {
-		return "", nil
+		return "", ""
 	}
 
 	err := delivery.ValidateArchiveExpiry(expiry)
 	if err != nil {
-		http.Error(
-			w,
-			"Invalid archive expiry: "+err.Error(),
-			http.StatusBadRequest,
-		)
-
-		return "", err
+		return "", "Invalid archive expiry: " + err.Error()
 	}
 
-	return h.marshalTargetConfig(
-		w, r, map[string]any{"expiry": expiry},
-	)
+	return marshalTargetConfig(map[string]any{"expiry": expiry})
 }
 
 // HandleEntrypointDelete handles deleting an entrypoint.

internal/handlers/target_create_test.go

@@ -0,0 +1,144 @@
+package handlers_test
+
+import (
+	"html"
+	"net/http"
+	"net/url"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+	"sneak.berlin/go/webhooker/internal/database"
+)
+
+// TestHandleTargetCreate_EveryType adds a target of each type. Each
+// submission carries a url: only the http and slack types store one.
+func TestHandleTargetCreate_EveryType(t *testing.T) {
+	t.Parallel()
+
+	env := setupSourceTest(t)
+
+	// fields is the rest of each submission, as a query string.
+	cases := []struct {
+		targetType  database.TargetType
+		fields      string
+		wantConfig  string
+		wantRetries int
+	}{
+		{
+			database.TargetTypeHTTP, "timeout=12&max_retries=3",
+			`{"url":"` + editOriginalURL + `","timeout":12}`, 3,
+		},
+		{
+			database.TargetTypeSlack, "max_retries=4",
+			`{"webhookUrl":"` + editOriginalURL + `"}`, 4,
+		},
+		{
+			database.TargetTypeDatabase, "expiry=720h",
+			`{"expiry":"720h"}`, 0,
+		},
+		{database.TargetTypeLog, "", "", 0},
+	}
+
+	for _, tc := range cases {
+		t.Run(string(tc.targetType), func(t *testing.T) {
+			t.Parallel()
+
+			webhook := seedWebhookWithRetention(t, env.db, 30)
+
+			form, err := url.ParseQuery(tc.fields)
+			require.NoError(t, err)
+			form.Set("name", "every-type")
+			form.Set("type", string(tc.targetType))
+			form.Set("url", editOriginalURL)
+
+			w := serveTarget(
+				env, http.MethodPost,
+				"/hook/"+webhook.ID+"/targets", form,
+			)
+			require.Equal(t, http.StatusSeeOther, w.Code, w.Body.String())
+
+			targets := targetsForWebhook(t, env.db, webhook.ID)
+			require.Len(t, targets, 1)
+			assert.Equal(t, tc.targetType, targets[0].Type)
+			assert.Equal(t, tc.wantRetries, targets[0].MaxRetries)
+
+			if tc.wantConfig == "" {
+				assert.Empty(t, targets[0].Config)
+			} else {
+				assert.JSONEq(t, tc.wantConfig, targets[0].Config)
+			}
+		})
+	}
+}
+
+// TestHandleTargetCreate_RefusedFormComesBack refuses a target of each
+// type and checks that the webhook page comes back with the add target
+// form open on that type, the values entered, and the reason.
+func TestHandleTargetCreate_RefusedFormComesBack(t *testing.T) {
+	t.Parallel()
+
+	env := setupSourceTest(t)
+
+	// fields is what the operator typed, as a query string.
+	cases := []struct {
+		targetType database.TargetType
+		fields     string
+		reason     string
+	}{
+		{
+			database.TargetTypeHTTP,
+			"name=private&url=" + editBlockedURL +
+				"&timeout=12&max_retries=3",
+			"Invalid target URL",
+		},
+		{
+			database.TargetTypeSlack, "name=no-url&max_retries=4",
+			"Webhook URL is required for Slack targets",
+		},
+		{
+			database.TargetTypeDatabase, "name=archive&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)
+
+			typed, err := url.ParseQuery(tc.fields)
+			require.NoError(t, err)
+
+			form := url.Values{}
+			form.Set("type", string(tc.targetType))
+
+			for field := range typed {
+				form.Set(field, typed.Get(field))
+			}
+
+			w := serveTarget(
+				env, http.MethodPost,
+				"/hook/"+webhook.ID+"/targets", form,
+			)
+			assert.Equal(t, http.StatusBadRequest, w.Code)
+
+			page := w.Body.String()
+			assert.Contains(
+				t, page, `data-type="`+string(tc.targetType)+`"`,
+			)
+			assert.Contains(t, page, html.EscapeString(tc.reason))
+
+			for field := range typed {
+				assert.Contains(
+					t, page, `name="`+field+`" value="`+
+						html.EscapeString(typed.Get(field))+`"`,
+				)
+			}
+
+			assert.Empty(t, targetsForWebhook(t, env.db, webhook.ID))
+		})
+	}
+}

internal/handlers/target_edit.go

@@ -127,11 +127,12 @@ func (h *Handlers) applyTargetEdit(
 		return
 	}
 
-	configJSON, err := h.buildTargetConfig(
-		w, r, target.Type, targetFormInputFrom(r),
+	configJSON, errMsg := h.buildTargetConfig(
+		r.Context(), target.Type, targetFormInputFrom(r),
 	)
-	if err != nil {
-		// buildTargetConfig has already written the response.
+	if errMsg != "" {
+		http.Error(w, errMsg, http.StatusBadRequest)
+
 		return
 	}
 
@@ -161,7 +162,7 @@ func (h *Handlers) applyTargetEdit(
 	// 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, oldName, name)
 	if err == nil {
 		err = h.db.DB().Save(target).Error
 	}

internal/handlers/target_private_refusal_test.go

@@ -1,6 +1,7 @@
 package handlers_test
 
 import (
+	"html"
 	"net/http"
 	"net/url"
 	"testing"
@@ -43,12 +44,16 @@ 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.
 			added := serveTarget(
 				env, http.MethodPost, targetsPath, form,
 			)
 			assert.Equal(t, http.StatusBadRequest, added.Code)
 			assert.Contains(
-				t, added.Body.String(), privateRefusalHint,
+				t, added.Body.String(),
+				html.EscapeString(privateRefusalHint),
 			)
 
 			form.Set("url", editOriginalURL)

internal/handlers/target_retries.go

@@ -90,13 +90,14 @@ func retriesErrorMessage(err error) string {
 		", or 0 for fire-and-forget"
 }
 
-// targetMaxRetries reads and validates max_retries from a target form
+// 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.
 //
-// Both the create and the edit path go through here, so the two
-// cannot come to disagree about what a valid retry count is. The
-// wording matches the timeout control on the same submission.
+// 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,