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/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.
@@ -1437,67 +1447,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)
 
@@ -1510,6 +1477,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 {
@@ -1541,11 +1549,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
@@ -1554,13 +1567,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
@@ -1572,38 +1587,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"
 	}
 }
 
@@ -1611,40 +1624,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,
@@ -1654,49 +1654,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
@@ -1722,61 +1712,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.