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