Empty the add target form on Cancel; encoding failures stay a 500

The form's reason and values now come from the targetForm component,
loaded from the section's data attributes after a refusal and emptied
by Cancel, which also resets the form. Each type's fields used to be
recreated with the refused values written into the markup, so they
came back after Cancel. The browser test checks this after a refusal.

A target configuration that cannot be encoded is again a logged 500
with the generic error page, on the add and the edit path; only
refusals of submitted values come back on the form.

The README paragraph on the browser test is re-wrapped at 80 columns
and names Cancel at the type step.

Model: opus-5-5

internal/handlers/export_test.go

@@ -145,7 +145,7 @@ func (s *Handlers) RenderTemplateForTest(
 func (s *Handlers) BuildSlackTargetConfigForTest(
 	ctx context.Context,
 	targetURL string,
-) (string, string) {
+) (string, string, error) {
 	return s.buildSlackTargetConfig(ctx, targetURL)
 }
 
@@ -155,7 +155,7 @@ func (s *Handlers) BuildSlackTargetConfigForTest(
 func (s *Handlers) BuildHTTPTargetConfigForTest(
 	ctx context.Context,
 	targetURL, headers, timeout string,
-) (string, string) {
+) (string, string, error) {
 	return s.buildHTTPTargetConfig(ctx, targetFormInput{
 		URL:     targetURL,
 		Headers: headers,
@@ -166,6 +166,8 @@ func (s *Handlers) BuildHTTPTargetConfigForTest(
 // BuildDatabaseTargetConfigForTest exposes
 // buildDatabaseTargetConfig for use in the handlers_test
 // package.
-func BuildDatabaseTargetConfigForTest(expiry string) (string, string) {
+func BuildDatabaseTargetConfigForTest(
+	expiry string,
+) (string, string, error) {
 	return buildDatabaseTargetConfig(expiry)
 }

internal/handlers/handlers_test.go

@@ -314,10 +314,11 @@ func TestBuildSlackTargetConfig_AcceptsPublicURL(t *testing.T) {
 
 	t.Cleanup(app.RequireStop)
 
-	cfg, errMsg := h.BuildSlackTargetConfigForTest(
+	cfg, errMsg, err := h.BuildSlackTargetConfigForTest(
 		t.Context(), "http://93.184.216.34/services/T00/B00/xxx",
 	)
 
+	require.NoError(t, err)
 	assert.Empty(t, errMsg)
 	assert.Contains(t, cfg, "webhookUrl")
 }
@@ -332,10 +333,11 @@ func TestBuildSlackTargetConfig_RejectsReservedURL(t *testing.T) {
 
 	t.Cleanup(app.RequireStop)
 
-	cfg, errMsg := h.BuildSlackTargetConfigForTest(
+	cfg, errMsg, err := h.BuildSlackTargetConfigForTest(
 		t.Context(), "http://169.254.169.254/latest/meta-data/",
 	)
 
+	require.NoError(t, err)
 	assert.Contains(t, errMsg, "Invalid target URL")
 	assert.Empty(t, cfg)
 }
@@ -435,17 +437,20 @@ func TestBuildDatabaseTargetConfig_Valid(t *testing.T) {
 	t.Parallel()
 
 	// Empty expiry: the keep-forever default, empty config.
-	cfg, errMsg := handlers.BuildDatabaseTargetConfigForTest("")
+	cfg, errMsg, err := handlers.BuildDatabaseTargetConfigForTest("")
+	require.NoError(t, err)
 	assert.Empty(t, errMsg)
 	assert.Empty(t, cfg)
 
 	// Explicit never is stored as config.
-	cfg, errMsg = handlers.BuildDatabaseTargetConfigForTest("never")
+	cfg, errMsg, err = handlers.BuildDatabaseTargetConfigForTest("never")
+	require.NoError(t, err)
 	assert.Empty(t, errMsg)
 	assert.JSONEq(t, `{"expiry":"never"}`, cfg)
 
 	// A positive duration is stored as config.
-	cfg, errMsg = handlers.BuildDatabaseTargetConfigForTest("720h")
+	cfg, errMsg, err = handlers.BuildDatabaseTargetConfigForTest("720h")
+	require.NoError(t, err)
 	assert.Empty(t, errMsg)
 	assert.JSONEq(t, `{"expiry":"720h"}`, cfg)
 }
@@ -456,8 +461,9 @@ func TestBuildDatabaseTargetConfig_RejectsBadExpiry(
 	t.Parallel()
 
 	for _, bad := range []string{"nonsense", "7d", "-5h"} {
-		cfg, errMsg := handlers.BuildDatabaseTargetConfigForTest(bad)
+		cfg, errMsg, err := handlers.BuildDatabaseTargetConfigForTest(bad)
 
+		require.NoError(t, err)
 		assert.Contains(
 			t, errMsg, "Invalid archive expiry",
 			"expiry %q should be refused", bad,

internal/handlers/source_management.go

@@ -1538,14 +1538,20 @@ func (h *Handlers) processTargetCreate(
 ) {
 	in := targetFormInputFrom(r)
 
-	target, errMsg := h.newTarget(r.Context(), webhook.ID, in)
+	target, errMsg, err := h.newTarget(r.Context(), webhook.ID, in)
+	if err != nil {
+		h.serverError(w, r, "failed to encode target config", err)
+
+		return
+	}
+
 	if errMsg != "" {
 		h.renderSourceDetail(w, r, webhook, in, errMsg)
 
 		return
 	}
 
-	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)
 
@@ -1560,24 +1566,26 @@ 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.
+// shows. An error is the server's fault, not a refusal: the accepted
+// configuration could not be encoded. 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) {
+) (*database.Target, string, error) {
 	if in.Name == "" {
-		return nil, "Name is required"
+		return nil, "Name is required", nil
 	}
 
 	if !isValidTargetType(in.Type) {
-		return nil, "Invalid target type"
+		return nil, "Invalid target type", nil
 	}
 
-	configJSON, errMsg := h.buildTargetConfig(ctx, in.Type, in)
-	if errMsg != "" {
-		return nil, errMsg
+	configJSON, errMsg, err := h.buildTargetConfig(ctx, in.Type, in)
+	if err != nil || errMsg != "" {
+		return nil, errMsg, err
 	}
 
 	// A new target has no stored retry count, so an absent field
@@ -1586,7 +1594,7 @@ func (h *Handlers) newTarget(
 	// that default.
 	maxRetries, err := parseMaxRetries(in.MaxRetries, 0)
 	if err != nil {
-		return nil, "Invalid max retries: " + retriesErrorMessage(err)
+		return nil, "Invalid max retries: " + retriesErrorMessage(err), nil
 	}
 
 	return &database.Target{
@@ -1596,7 +1604,7 @@ func (h *Handlers) newTarget(
 		Active:     true,
 		Config:     configJSON,
 		MaxRetries: maxRetries,
-	}, ""
+	}, "", nil
 }
 
 // isValidTargetType checks whether the target type is supported.
@@ -1680,13 +1688,15 @@ func targetFormInputFrom(r *http.Request) targetFormInput {
 
 // buildTargetConfig builds the JSON config string for a target from
 // 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.
+// for a value it refuses. An error is the server's fault, not a
+// refusal: the accepted configuration could not be encoded. Which
+// fields of in apply depends on the target type; a type without a URL
+// ignores any URL submitted.
 func (h *Handlers) buildTargetConfig(
 	ctx context.Context,
 	targetType database.TargetType,
 	in targetFormInput,
-) (string, string) {
+) (string, string, error) {
 	switch targetType {
 	case database.TargetTypeHTTP:
 		return h.buildHTTPTargetConfig(ctx, in)
@@ -1695,9 +1705,9 @@ func (h *Handlers) buildTargetConfig(
 	case database.TargetTypeDatabase:
 		return buildDatabaseTargetConfig(in.Expiry)
 	case database.TargetTypeLog:
-		return "", ""
+		return "", "", nil
 	default:
-		return "", "Invalid target type"
+		return "", "Invalid target type", nil
 	}
 }
 
@@ -1707,29 +1717,31 @@ func (h *Handlers) buildTargetConfig(
 func (h *Handlers) buildHTTPTargetConfig(
 	ctx context.Context,
 	in targetFormInput,
-) (string, string) {
+) (string, string, error) {
 	errMsg := h.validateTargetURL(
 		ctx, in.URL, "URL is required for HTTP targets",
 	)
 	if errMsg != "" {
-		return "", errMsg
+		return "", errMsg, nil
 	}
 
 	headers, err := delivery.ParseTargetHeaders(in.Headers)
 	if err != nil {
-		return "", "Invalid headers: " + err.Error()
+		return "", fmt.Sprintf("Invalid headers: %v", err), nil
 	}
 
 	timeout, err := delivery.ParseTargetTimeout(in.Timeout)
 	if err != nil {
-		return "", "Invalid timeout: " + err.Error()
+		return "", fmt.Sprintf("Invalid timeout: %v", err), nil
 	}
 
-	return marshalTargetConfig(delivery.HTTPTargetConfig{
+	configJSON, err := marshalTargetConfig(delivery.HTTPTargetConfig{
 		URL:     in.URL,
 		Headers: headers,
 		Timeout: timeout,
 	})
+
+	return configJSON, "", err
 }
 
 // buildSlackTargetConfig builds config JSON for a Slack target,
@@ -1737,18 +1749,20 @@ func (h *Handlers) buildHTTPTargetConfig(
 func (h *Handlers) buildSlackTargetConfig(
 	ctx context.Context,
 	targetURL string,
-) (string, string) {
+) (string, string, error) {
 	errMsg := h.validateTargetURL(
 		ctx, targetURL,
 		"Webhook URL is required for Slack targets",
 	)
 	if errMsg != "" {
-		return "", errMsg
+		return "", errMsg, nil
 	}
 
-	return marshalTargetConfig(delivery.SlackTargetConfig{
+	configJSON, err := marshalTargetConfig(delivery.SlackTargetConfig{
 		WebhookURL: targetURL,
 	})
+
+	return configJSON, "", err
 }
 
 // validateTargetURL refuses an empty or SSRF-blocked destination,
@@ -1799,16 +1813,14 @@ func (h *Handlers) validateTargetURL(
 	return ""
 }
 
-// marshalTargetConfig serialises a target configuration for storage,
-// or returns the message the form shows if it cannot.
-func marshalTargetConfig(cfg any) (string, string) {
+// marshalTargetConfig serialises a target configuration for storage.
+func marshalTargetConfig(cfg any) (string, error) {
 	configBytes, err := json.Marshal(cfg)
 	if err != nil {
-		return "", "Could not encode the target configuration: " +
-			err.Error()
+		return "", err
 	}
 
-	return string(configBytes), ""
+	return string(configBytes), nil
 }
 
 // buildDatabaseTargetConfig builds config JSON for a database
@@ -1816,18 +1828,20 @@ func marshalTargetConfig(cfg any) (string, string) {
 // 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) {
+func buildDatabaseTargetConfig(expiry string) (string, string, error) {
 	expiry = strings.TrimSpace(expiry)
 	if expiry == "" {
-		return "", ""
+		return "", "", nil
 	}
 
 	err := delivery.ValidateArchiveExpiry(expiry)
 	if err != nil {
-		return "", "Invalid archive expiry: " + err.Error()
+		return "", fmt.Sprintf("Invalid archive expiry: %v", err), nil
 	}
 
-	return marshalTargetConfig(map[string]any{"expiry": expiry})
+	configJSON, err := marshalTargetConfig(map[string]any{"expiry": expiry})
+
+	return configJSON, "", err
 }
 
 // HandleEntrypointDelete handles deleting an entrypoint.

internal/handlers/target_create_test.go

@@ -4,6 +4,7 @@ import (
 	"html"
 	"net/http"
 	"net/url"
+	"strings"
 	"testing"
 
 	"github.com/stretchr/testify/assert"
@@ -131,9 +132,18 @@ func TestHandleTargetCreate_RefusedFormComesBack(t *testing.T) {
 			)
 			assert.Contains(t, page, html.EscapeString(tc.reason))
 
+			// Each value comes back in a data attribute of the targets
+			// section named after its field (max_retries as
+			// data-max-retries), except url, which comes back in
+			// data-destination; templates/source_detail.html says why.
 			for field := range typed {
+				attr := "data-" + strings.ReplaceAll(field, "_", "-")
+				if field == "url" {
+					attr = "data-destination"
+				}
+
 				assert.Contains(
-					t, page, `name="`+field+`" value="`+
+					t, page, attr+`="`+
 						html.EscapeString(typed.Get(field))+`"`,
 				)
 			}

internal/handlers/target_edit.go

@@ -120,16 +120,20 @@ func (h *Handlers) applyTargetEdit(
 ) {
 	name := r.PostFormValue("name")
 	if name == "" {
-		http.Error(
-			w, "Name is required", http.StatusBadRequest,
-		)
+		http.Error(w, "Name is required", http.StatusBadRequest)
 
 		return
 	}
 
-	configJSON, errMsg := h.buildTargetConfig(
+	configJSON, errMsg, err := h.buildTargetConfig(
 		r.Context(), target.Type, targetFormInputFrom(r),
 	)
+	if err != nil {
+		h.serverError(w, r, "failed to encode target config", err)
+
+		return
+	}
+
 	if errMsg != "" {
 		http.Error(w, errMsg, http.StatusBadRequest)
 
@@ -162,7 +166,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
 	}