From 0529ea5cd576298c192dacaacc3b890e0339600d Mon Sep 17 00:00:00 2001 From: clawbot <35+clawbot@noreply.example.org> Date: Fri, 2 Oct 2026 18:08:59 +0000 Subject: [PATCH] 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 --- README.md | 26 ++++---- internal/handlers/export_test.go | 8 ++- internal/handlers/handlers_test.go | 18 ++++-- internal/handlers/source_management.go | 84 ++++++++++++++----------- internal/handlers/target_create_test.go | 12 +++- internal/handlers/target_edit.go | 14 +++-- internal/server/alpine_browser_test.go | 22 ++++++- static/js/app.js | 36 ++++++++++- templates/source_detail.html | 41 +++++++----- 9 files changed, 179 insertions(+), 82 deletions(-) diff --git a/README.md b/README.md index 6833653..07b404b 100644 --- a/README.md +++ b/README.md @@ -1326,18 +1326,20 @@ markup. The CSP build runs no expressions, so every Alpine directive in 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 and Next, Next shows only that type's fields (no url field -for `database` or `log`), Cancel closes the form, and saving adds the target; -a refused target comes back with its form open, the values entered and the -reason; the Copy button beside an entrypoint URL reads "Copied" once clicked; an -event 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 `Dockerfile.browser`, -which runs the test in a digest-pinned headless browser image, so the host -needs no browser. +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 event 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 `Dockerfile.browser`, which +runs the test in a digest-pinned headless browser image, so the host needs no +browser. The package's tarball is committed as `3p/alpinejs-csp-3.14.9.tgz`, byte for byte as the npm registry publishes it. It is a dependency, not this repo's build diff --git a/internal/handlers/export_test.go b/internal/handlers/export_test.go index ba3fe5c..cdf26b4 100644 --- a/internal/handlers/export_test.go +++ b/internal/handlers/export_test.go @@ -139,7 +139,7 @@ func (s *Handlers) RenderTemplateForTest( func (s *Handlers) BuildSlackTargetConfigForTest( ctx context.Context, targetURL string, -) (string, string) { +) (string, string, error) { return s.buildSlackTargetConfig(ctx, targetURL) } @@ -149,7 +149,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, @@ -160,6 +160,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) } diff --git a/internal/handlers/handlers_test.go b/internal/handlers/handlers_test.go index 4c5d6d0..bbf4f11 100644 --- a/internal/handlers/handlers_test.go +++ b/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, diff --git a/internal/handlers/source_management.go b/internal/handlers/source_management.go index b556200..37b8bc0 100644 --- a/internal/handlers/source_management.go +++ b/internal/handlers/source_management.go @@ -1457,14 +1457,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) @@ -1479,24 +1485,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 @@ -1505,7 +1513,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{ @@ -1515,7 +1523,7 @@ func (h *Handlers) newTarget( Active: true, Config: configJSON, MaxRetries: maxRetries, - }, "" + }, "", nil } // isValidTargetType checks whether the target type is supported. @@ -1599,13 +1607,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) @@ -1614,9 +1624,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 } } @@ -1626,29 +1636,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, @@ -1656,18 +1668,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, @@ -1718,16 +1732,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 @@ -1735,18 +1747,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. diff --git a/internal/handlers/target_create_test.go b/internal/handlers/target_create_test.go index 6b2c304..253f63e 100644 --- a/internal/handlers/target_create_test.go +++ b/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))+`"`, ) } diff --git a/internal/handlers/target_edit.go b/internal/handlers/target_edit.go index 82f4999..10a1e65 100644 --- a/internal/handlers/target_edit.go +++ b/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 } diff --git a/internal/server/alpine_browser_test.go b/internal/server/alpine_browser_test.go index 4b432eb..6172134 100644 --- a/internal/server/alpine_browser_test.go +++ b/internal/server/alpine_browser_test.go @@ -387,13 +387,16 @@ func chooseTargetType(ctx context.Context, t *testing.T, targetType string) { // checkRefusedTarget submits an http target the server refuses, a // loopback destination, and checks that the page comes back with the -// form open on the http fields, the values entered and the reason. +// form open on the http fields, the values entered and the reason, and +// that after Cancel the next Add starts with an empty form and no +// reason. func checkRefusedTarget(ctx context.Context, t *testing.T, url string) { t.Helper() const ( refusedURL = "http://127.0.0.1/hook" urlField = `form[action$="/targets"] input[name="url"]` + reason = `//div[@class="alert-error"]` ) require.NoError(t, chromedp.Run(ctx, loadPage(url))) @@ -407,7 +410,7 @@ func checkRefusedTarget(ctx context.Context, t *testing.T, url string) { click(ctx, t, saveButton) - assert.True(t, shown(ctx, `//div[@class="alert-error"]`), + assert.True(t, shown(ctx, reason), "a refused target does not show the reason") var name, typed string @@ -426,6 +429,21 @@ func checkRefusedTarget(ctx context.Context, t *testing.T, url string) { "a refused target does not come back with the form open") assert.True(t, hidden(ctx, typeSelect), "a refused target comes back on the type choice") + + click(ctx, t, cancelFields) + chooseTargetType(ctx, t, "http") + + assert.True(t, hidden(ctx, reason), + "after Cancel, the next Add still shows the reason") + + require.NoError(t, chromedp.Run( + ctx, + chromedp.Value(targetName, &name, chromedp.ByQuery), + chromedp.Value(urlField, &typed, chromedp.ByQuery), + )) + + assert.Empty(t, name, "after Cancel, the next Add keeps the name entered") + assert.Empty(t, typed, "after Cancel, the next Add keeps the url entered") } // checkCopy loads a webhook page and checks that the Copy control beside diff --git a/static/js/app.js b/static/js/app.js index 6ac93e4..0418e0d 100644 --- a/static/js/app.js +++ b/static/js/app.js @@ -91,14 +91,36 @@ document.addEventListener("alpine:init", function () { // choosing a type, then filling in that type's fields. targetType // is empty until Next takes it from the type select. // - // A refused submission comes back with the type it was submitted - // with in the section's data-type, and starts on that type's fields. + // The reason and the fields' values come from the properties below + // rather than from the markup, because each type's fields are made + // afresh from the markup whenever that type is chosen. A refused + // submission comes back with its type, reason and values in the + // section's data attributes, and starts on that type's fields with + // them. Cancel empties these properties and resets the form, which + // holds whatever was typed, so the next Add starts with an empty + // form and no reason. window.Alpine.data("targetForm", function () { return { choosing: false, targetType: "", + reason: "", + name: "", + url: "", + headers: "", + timeout: "", + maxRetries: "", + expiry: "", init() { - this.targetType = this.$root.dataset.type; + const refused = this.$root.dataset; + + this.targetType = refused.type; + this.reason = refused.reason; + this.name = refused.name; + this.url = refused.destination; + this.headers = refused.headers; + this.timeout = refused.timeout; + this.maxRetries = refused.maxRetries; + this.expiry = refused.expiry; }, add() { this.choosing = true; @@ -110,6 +132,14 @@ document.addEventListener("alpine:init", function () { cancel() { this.choosing = false; this.targetType = ""; + this.reason = ""; + this.name = ""; + this.url = ""; + this.headers = ""; + this.timeout = ""; + this.maxRetries = ""; + this.expiry = ""; + this.$refs.form.reset(); }, get filling() { return this.targetType !== ""; diff --git a/templates/source_detail.html b/templates/source_detail.html index 9220bbb..3fbb181 100644 --- a/templates/source_detail.html +++ b/templates/source_detail.html @@ -90,8 +90,20 @@ - -
+ +

Targets