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
2026-10-03 00:51:56 +02:00
parent 61371d388e
commit f282c6363d
11 changed files with 467 additions and 269 deletions
@@ -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.