diff --git a/README.md b/README.md index 2d18612..239c168 100644 --- a/README.md +++ b/README.md @@ -1323,13 +1323,14 @@ markup. The CSP build runs no expressions, so every Alpine directive in `x-data="{ open: false }"` or `@click="open = !open"`. A browser test in `internal/server` loads the webhook page and the event log -under the real policy and checks that: both add forms stay hidden until Add is -clicked; choosing Slack in the add target form leaves the HTTP fields out of -what it submits, also after leaving the page and going back to it, when the -browser restores the choice; 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, +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` diff --git a/internal/handlers/export_test.go b/internal/handlers/export_test.go index a86e5bd..ba3fe5c 100644 --- a/internal/handlers/export_test.go +++ b/internal/handlers/export_test.go @@ -137,22 +137,20 @@ func (s *Handlers) RenderTemplateForTest( // BuildSlackTargetConfigForTest exposes // buildSlackTargetConfig for use in the handlers_test package. func (s *Handlers) BuildSlackTargetConfigForTest( - w http.ResponseWriter, - r *http.Request, + ctx context.Context, targetURL string, -) (string, error) { - return s.buildSlackTargetConfig(w, r, targetURL) +) (string, string) { + return s.buildSlackTargetConfig(ctx, targetURL) } // BuildHTTPTargetConfigForTest exposes buildHTTPTargetConfig // for use in the handlers_test package, taking the form fields // an HTTP target's configuration is built from. func (s *Handlers) BuildHTTPTargetConfigForTest( - w http.ResponseWriter, - r *http.Request, + ctx context.Context, targetURL, headers, timeout string, -) (string, error) { - return s.buildHTTPTargetConfig(w, r, targetFormInput{ +) (string, string) { + return s.buildHTTPTargetConfig(ctx, targetFormInput{ URL: targetURL, Headers: headers, Timeout: timeout, @@ -162,9 +160,6 @@ func (s *Handlers) BuildHTTPTargetConfigForTest( // BuildDatabaseTargetConfigForTest exposes // buildDatabaseTargetConfig for use in the handlers_test // package. -func (s *Handlers) BuildDatabaseTargetConfigForTest( - w http.ResponseWriter, - expiry string, -) (string, error) { - return s.buildDatabaseTargetConfig(w, newRequestForTest(), expiry) +func BuildDatabaseTargetConfigForTest(expiry string) (string, string) { + return buildDatabaseTargetConfig(expiry) } diff --git a/internal/handlers/handlers_test.go b/internal/handlers/handlers_test.go index 1088419..4c5d6d0 100644 --- a/internal/handlers/handlers_test.go +++ b/internal/handlers/handlers_test.go @@ -314,16 +314,11 @@ func TestBuildSlackTargetConfig_AcceptsPublicURL(t *testing.T) { t.Cleanup(app.RequireStop) - req := httptest.NewRequestWithContext( - context.Background(), http.MethodPost, "/", nil) - w := httptest.NewRecorder() - - cfg, err := h.BuildSlackTargetConfigForTest( - w, req, "http://93.184.216.34/services/T00/B00/xxx", + cfg, errMsg := h.BuildSlackTargetConfigForTest( + t.Context(), "http://93.184.216.34/services/T00/B00/xxx", ) - require.NoError(t, err) - assert.Equal(t, http.StatusOK, w.Code) + assert.Empty(t, errMsg) assert.Contains(t, cfg, "webhookUrl") } @@ -337,17 +332,12 @@ func TestBuildSlackTargetConfig_RejectsReservedURL(t *testing.T) { t.Cleanup(app.RequireStop) - req := httptest.NewRequestWithContext( - context.Background(), http.MethodPost, "/", nil) - w := httptest.NewRecorder() - - cfg, err := h.BuildSlackTargetConfigForTest( - w, req, "http://169.254.169.254/latest/meta-data/", + cfg, errMsg := h.BuildSlackTargetConfigForTest( + t.Context(), "http://169.254.169.254/latest/meta-data/", ) - require.Error(t, err) + assert.Contains(t, errMsg, "Invalid target URL") assert.Empty(t, cfg) - assert.Equal(t, http.StatusBadRequest, w.Code) } func TestRenderTemplate(t *testing.T) { @@ -444,29 +434,19 @@ func TestRenderTemplateMidRenderErrorSendsNoPartialBody(t *testing.T) { func TestBuildDatabaseTargetConfig_Valid(t *testing.T) { t.Parallel() - var h *handlers.Handlers - - app := newTestApp(t, &h) - app.RequireStart() - - t.Cleanup(app.RequireStop) - // Empty expiry: the keep-forever default, empty config. - w := httptest.NewRecorder() - cfg, err := h.BuildDatabaseTargetConfigForTest(w, "") - require.NoError(t, err) + cfg, errMsg := handlers.BuildDatabaseTargetConfigForTest("") + assert.Empty(t, errMsg) assert.Empty(t, cfg) // Explicit never is stored as config. - w = httptest.NewRecorder() - cfg, err = h.BuildDatabaseTargetConfigForTest(w, "never") - require.NoError(t, err) + cfg, errMsg = handlers.BuildDatabaseTargetConfigForTest("never") + assert.Empty(t, errMsg) assert.JSONEq(t, `{"expiry":"never"}`, cfg) // A positive duration is stored as config. - w = httptest.NewRecorder() - cfg, err = h.BuildDatabaseTargetConfigForTest(w, "720h") - require.NoError(t, err) + cfg, errMsg = handlers.BuildDatabaseTargetConfigForTest("720h") + assert.Empty(t, errMsg) assert.JSONEq(t, `{"expiry":"720h"}`, cfg) } @@ -475,22 +455,13 @@ func TestBuildDatabaseTargetConfig_RejectsBadExpiry( ) { t.Parallel() - var h *handlers.Handlers - - app := newTestApp(t, &h) - app.RequireStart() - - t.Cleanup(app.RequireStop) - for _, bad := range []string{"nonsense", "7d", "-5h"} { - w := httptest.NewRecorder() - cfg, err := h.BuildDatabaseTargetConfigForTest(w, bad) + cfg, errMsg := handlers.BuildDatabaseTargetConfigForTest(bad) - require.Error(t, err, "expiry %q", bad) - assert.Empty(t, cfg) - assert.Equal( - t, http.StatusBadRequest, w.Code, - "expiry %q should be rejected with 400", bad, + assert.Contains( + t, errMsg, "Invalid archive expiry", + "expiry %q should be refused", bad, ) + assert.Empty(t, cfg) } } diff --git a/internal/handlers/source_management.go b/internal/handlers/source_management.go index 377eac8..55d2251 100644 --- a/internal/handlers/source_management.go +++ b/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. diff --git a/internal/handlers/target_create_test.go b/internal/handlers/target_create_test.go new file mode 100644 index 0000000..6b2c304 --- /dev/null +++ b/internal/handlers/target_create_test.go @@ -0,0 +1,144 @@ +package handlers_test + +import ( + "html" + "net/http" + "net/url" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "sneak.berlin/go/webhooker/internal/database" +) + +// TestHandleTargetCreate_EveryType adds a target of each type. Each +// submission carries a url: only the http and slack types store one. +func TestHandleTargetCreate_EveryType(t *testing.T) { + t.Parallel() + + env := setupSourceTest(t) + + // fields is the rest of each submission, as a query string. + cases := []struct { + targetType database.TargetType + fields string + wantConfig string + wantRetries int + }{ + { + database.TargetTypeHTTP, "timeout=12&max_retries=3", + `{"url":"` + editOriginalURL + `","timeout":12}`, 3, + }, + { + database.TargetTypeSlack, "max_retries=4", + `{"webhookUrl":"` + editOriginalURL + `"}`, 4, + }, + { + database.TargetTypeDatabase, "expiry=720h", + `{"expiry":"720h"}`, 0, + }, + {database.TargetTypeLog, "", "", 0}, + } + + for _, tc := range cases { + t.Run(string(tc.targetType), func(t *testing.T) { + t.Parallel() + + webhook := seedWebhookWithRetention(t, env.db, 30) + + form, err := url.ParseQuery(tc.fields) + require.NoError(t, err) + form.Set("name", "every-type") + form.Set("type", string(tc.targetType)) + form.Set("url", editOriginalURL) + + w := serveTarget( + env, http.MethodPost, + "/hook/"+webhook.ID+"/targets", form, + ) + require.Equal(t, http.StatusSeeOther, w.Code, w.Body.String()) + + targets := targetsForWebhook(t, env.db, webhook.ID) + require.Len(t, targets, 1) + assert.Equal(t, tc.targetType, targets[0].Type) + assert.Equal(t, tc.wantRetries, targets[0].MaxRetries) + + if tc.wantConfig == "" { + assert.Empty(t, targets[0].Config) + } else { + assert.JSONEq(t, tc.wantConfig, targets[0].Config) + } + }) + } +} + +// TestHandleTargetCreate_RefusedFormComesBack refuses a target of each +// type and checks that the webhook page comes back with the add target +// form open on that type, the values entered, and the reason. +func TestHandleTargetCreate_RefusedFormComesBack(t *testing.T) { + t.Parallel() + + env := setupSourceTest(t) + + // fields is what the operator typed, as a query string. + cases := []struct { + targetType database.TargetType + fields string + reason string + }{ + { + database.TargetTypeHTTP, + "name=private&url=" + editBlockedURL + + "&timeout=12&max_retries=3", + "Invalid target URL", + }, + { + database.TargetTypeSlack, "name=no-url&max_retries=4", + "Webhook URL is required for Slack targets", + }, + { + database.TargetTypeDatabase, "name=archive&expiry=7d", + "Invalid archive expiry", + }, + {database.TargetTypeLog, "name=", "Name is required"}, + } + + for _, tc := range cases { + t.Run(string(tc.targetType), func(t *testing.T) { + t.Parallel() + + webhook := seedWebhookWithRetention(t, env.db, 30) + + typed, err := url.ParseQuery(tc.fields) + require.NoError(t, err) + + form := url.Values{} + form.Set("type", string(tc.targetType)) + + for field := range typed { + form.Set(field, typed.Get(field)) + } + + w := serveTarget( + env, http.MethodPost, + "/hook/"+webhook.ID+"/targets", form, + ) + assert.Equal(t, http.StatusBadRequest, w.Code) + + page := w.Body.String() + assert.Contains( + t, page, `data-type="`+string(tc.targetType)+`"`, + ) + assert.Contains(t, page, html.EscapeString(tc.reason)) + + for field := range typed { + assert.Contains( + t, page, `name="`+field+`" value="`+ + html.EscapeString(typed.Get(field))+`"`, + ) + } + + assert.Empty(t, targetsForWebhook(t, env.db, webhook.ID)) + }) + } +} diff --git a/internal/handlers/target_edit.go b/internal/handlers/target_edit.go index 3f2f124..82f4999 100644 --- a/internal/handlers/target_edit.go +++ b/internal/handlers/target_edit.go @@ -127,11 +127,12 @@ func (h *Handlers) applyTargetEdit( return } - configJSON, err := h.buildTargetConfig( - w, r, target.Type, targetFormInputFrom(r), + configJSON, errMsg := h.buildTargetConfig( + r.Context(), target.Type, targetFormInputFrom(r), ) - if err != nil { - // buildTargetConfig has already written the response. + if errMsg != "" { + http.Error(w, errMsg, http.StatusBadRequest) + return } @@ -161,7 +162,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/handlers/target_private_refusal_test.go b/internal/handlers/target_private_refusal_test.go index 51b6bad..69131ea 100644 --- a/internal/handlers/target_private_refusal_test.go +++ b/internal/handlers/target_private_refusal_test.go @@ -1,6 +1,7 @@ package handlers_test import ( + "html" "net/http" "net/url" "testing" @@ -43,12 +44,16 @@ func TestTargetRefusal_PrivateDestinationSaysHowToAllowIt( form.Set("type", string(targetType)) form.Set("url", editBlockedURL) + // A refused add shows the webhook page again, where + // the hint is HTML-escaped; a refused edit answers in + // plain text. added := serveTarget( env, http.MethodPost, targetsPath, form, ) assert.Equal(t, http.StatusBadRequest, added.Code) assert.Contains( - t, added.Body.String(), privateRefusalHint, + t, added.Body.String(), + html.EscapeString(privateRefusalHint), ) form.Set("url", editOriginalURL) diff --git a/internal/handlers/target_retries.go b/internal/handlers/target_retries.go index 9f571c5..34dadd3 100644 --- a/internal/handlers/target_retries.go +++ b/internal/handlers/target_retries.go @@ -90,13 +90,14 @@ func retriesErrorMessage(err error) string { ", or 0 for fire-and-forget" } -// targetMaxRetries reads and validates max_retries from a target form +// targetMaxRetries reads and validates max_retries from a target edit // submission, answering the request with a 400 and reporting false // when the value is set but invalid. // -// Both the create and the edit path go through here, so the two -// cannot come to disagree about what a valid retry count is. The -// wording matches the timeout control on the same submission. +// It and the create path (newTarget) both use parseMaxRetries and +// retriesErrorMessage, so the two cannot come to disagree about what a +// valid retry count is. The wording matches the timeout control on +// the same submission. func targetMaxRetries( w http.ResponseWriter, r *http.Request, diff --git a/internal/server/alpine_browser_test.go b/internal/server/alpine_browser_test.go index 7637cf8..4b432eb 100644 --- a/internal/server/alpine_browser_test.go +++ b/internal/server/alpine_browser_test.go @@ -83,8 +83,37 @@ func TestAlpineRunsUnderTheSecurityPolicy(t *testing.T) { page := srv.URL + "/hook/" + webhook.ID - checkAddForms(ctx, t, page) - checkTargetType(ctx, t, page+"/events") + 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) checkCopy(ctx, t, page) checkEventLog(ctx, t, page+"/events", event.ID, target.Name) checkMobileMenu(ctx, t, page) @@ -227,115 +256,176 @@ func click(ctx context.Context, t *testing.T, xpath string) { )) } -// checkAddForms loads a webhook page and checks that each section's add -// form stays hidden until the Add button beside its heading is clicked. -// The click looks for a button element there, so it also checks that -// Add is one. -func checkAddForms(ctx context.Context, t *testing.T, url string) { +// checkAddEntrypoint loads a webhook page and checks that the add +// entrypoint form stays hidden until the Add button beside its heading +// is clicked. The click looks for a button element there, so it also +// checks that Add is one. +func checkAddEntrypoint(ctx context.Context, t *testing.T, url string) { + t.Helper() + + form := `form[action$="/entrypoints"]` + + require.NoError(t, chromedp.Run(ctx, loadPage(url))) + + assert.True(t, hidden(ctx, form), + "the add entrypoint form shows before Add is clicked") + + click(ctx, t, `//h2[text()="Entrypoints"]/following-sibling::button`) + + assert.True(t, shown(ctx, form), + "the add entrypoint form stays hidden when Add is clicked") +} + +// publicTargetURL is a destination the server accepts for an http or +// slack target. It is a literal public address, so accepting it needs +// no DNS. +const publicTargetURL = "https://93.184.216.34/hook" + +// The parts of the targets section's add target form the checks below +// find and click. Add is the button beside the Targets heading; each +// Cancel is found from the button beside it, since both are on the +// page at once. +const ( + addTarget = `//h2[text()="Targets"]/following-sibling::button` + typeSelect = `//select[@aria-label="Target type"]` + nextButton = `//button[text()="Next"]` + cancelChoice = nextButton + `/following-sibling::button[text()="Cancel"]` + saveButton = `//form[contains(@action, "/targets")]//button[text()="Save"]` + cancelFields = saveButton + `/following-sibling::button[text()="Cancel"]` + targetName = `form[action$="/targets"] input[name="name"]` + submittedKeys = `[...new FormData( + document.querySelector('form[action$="/targets"]')).keys()]` +) + +// 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 +// type's own fields in place of the choice, and the form then submits +// exactly fields, so a field another type uses, such as url, is absent; +// Cancel closes it again. It then adds a target of the type, filling in +// values, and checks that the section lists it with that type. +func checkAddTarget( + ctx context.Context, + t *testing.T, + url, targetType string, + fields []string, + values map[string]string, +) { t.Helper() require.NoError(t, chromedp.Run(ctx, loadPage(url))) - sections := []struct{ heading, form string }{ - {"Entrypoints", `form[action$="/entrypoints"]`}, - {"Targets", `form[action$="/targets"]`}, + assert.Truef(t, hidden(ctx, typeSelect), + "%s: the type choice shows before Add is clicked", targetType) + assert.Truef(t, hidden(ctx, targetName), + "%s: the fields show before Add is clicked", targetType) + + click(ctx, t, addTarget) + assert.Truef(t, shown(ctx, typeSelect), + "%s: Add does not show the type choice", targetType) + assert.Truef(t, hidden(ctx, targetName), + "%s: Add shows the fields before Next", targetType) + + click(ctx, t, cancelChoice) + assert.Truef(t, hidden(ctx, typeSelect), + "%s: Cancel does not close the type choice", targetType) + + chooseTargetType(ctx, t, targetType) + + var submitted []string + + require.NoError(t, chromedp.Run( + ctx, chromedp.Evaluate(submittedKeys, &submitted), + )) + assert.Equalf(t, fields, submitted, + "%s: the form does not submit exactly the type's fields", targetType) + + click(ctx, t, cancelFields) + assert.Truef(t, hidden(ctx, targetName), + "%s: Cancel does not close the fields", targetType) + assert.Truef(t, shown(ctx, addTarget), + "%s: Add does not come back after Cancel", targetType) + + name := "added-" + targetType + + chooseTargetType(ctx, t, targetType) + require.NoError(t, chromedp.Run( + ctx, chromedp.SetValue(targetName, name, chromedp.ByQuery), + )) + + for field, value := range values { + require.NoError(t, chromedp.Run(ctx, chromedp.SetValue( + `form[action$="/targets"] [name="`+field+`"]`, value, + chromedp.ByQuery, + ))) } - for _, s := range sections { - assert.Truef( - t, hidden(ctx, s.form), - "%s: the add form shows before Add is clicked", s.heading, - ) - - click(ctx, t, `//h2[text()="`+s.heading+ - `"]/following-sibling::button`) - - assert.Truef( - t, shown(ctx, s.form), - "%s: the add form stays hidden when Add is clicked", s.heading, - ) - } + click(ctx, t, saveButton) + assert.Truef(t, shown(ctx, `//span[text()="`+name+ + `"]/following-sibling::div/span[text()="`+targetType+`"]`), + "%s: the added target is not listed with its type", targetType) } -// checkTargetType chooses Slack in the open add target form and checks -// what the form would then submit: one url field, the Slack one, and -// not the HTTP url, headers or timeout, which are hidden and disabled. -// -// It then opens the page at elsewhere and goes back. The browser loads -// the webhook page again and restores the form as it was left, Slack -// chosen, without a change event; the form must again show and submit -// Slack's fields, not the HTTP ones. -func checkTargetType(ctx context.Context, t *testing.T, elsewhere string) { +// chooseTargetType clicks Add, picks targetType and clicks Next, and +// checks that the type's fields then show in place of the type choice. +func chooseTargetType(ctx context.Context, t *testing.T, targetType string) { + t.Helper() + + click(ctx, t, addTarget) + require.NoError(t, chromedp.Run( + ctx, chromedp.SetValue(typeSelect, targetType, chromedp.BySearch), + )) + click(ctx, t, nextButton) + + assert.Truef(t, shown(ctx, targetName), + "%s: Next does not show the fields", targetType) + assert.Truef(t, hidden(ctx, typeSelect), + "%s: Next leaves the type choice showing", targetType) + assert.Truef(t, hidden(ctx, addTarget), + "%s: Add still shows while the form is open", targetType) +} + +// 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. +func checkRefusedTarget(ctx context.Context, t *testing.T, url string) { t.Helper() const ( - chooseSlack = `(() => { - const type = document.querySelector('select[name="type"]'); - type.value = "slack"; - type.dispatchEvent(new Event("change")); - })()` - chosen = `document.querySelector('select[name="type"]').value` - howLoaded = `performance.getEntriesByType("navigation")[0].type` - submitted = `[...new FormData( - document.querySelector('form[action$="/targets"]')).keys()]` - slackURL = `input[placeholder^="https://hooks.slack.com/"]` - httpURL = `input[placeholder="https://example.com/webhook"]` + refusedURL = "http://127.0.0.1/hook" + urlField = `form[action$="/targets"] input[name="url"]` ) - slackFields := strings.Fields("csrf_token name type max_retries url") + require.NoError(t, chromedp.Run(ctx, loadPage(url))) - var fields []string + chooseTargetType(ctx, t, "http") + require.NoError(t, chromedp.Run( + ctx, + chromedp.SetValue(targetName, "refused", chromedp.ByQuery), + chromedp.SetValue(urlField, refusedURL, chromedp.ByQuery), + )) + + click(ctx, t, saveButton) + + assert.True(t, shown(ctx, `//div[@class="alert-error"]`), + "a refused target does not show the reason") + + var name, typed string require.NoError(t, chromedp.Run( ctx, - chromedp.Evaluate(chooseSlack, nil), - chromedp.Evaluate(submitted, &fields), + chromedp.Value(targetName, &name, chromedp.ByQuery), + chromedp.Value(urlField, &typed, chromedp.ByQuery), )) - assert.Equal( - t, slackFields, fields, - "with Slack chosen, the HTTP fields must not be submitted", - ) - - var loaded, restored string - - // Going back waits for the load event, after which the browser has - // restored the form. - require.NoError(t, chromedp.Run( - ctx, - loadPage(elsewhere), - chromedp.NavigateBack(), - chromedp.WaitNotPresent("[x-cloak]", chromedp.ByQuery), - chromedp.Evaluate(howLoaded, &loaded), - chromedp.Evaluate(chosen, &restored), - )) - - // A page the browser kept in memory and showed again as it was - // would prove nothing here. - require.Equal( - t, "back_forward", loaded, - "going back, the browser did not load the page again", - ) - require.Equal( - t, "slack", restored, - "going back, the browser did not restore the chosen type", - ) - - click(ctx, t, `//h2[text()="Targets"]/following-sibling::button`) - - assert.True(t, shown(ctx, slackURL), - "going back with Slack chosen, the Slack fields are not shown") - assert.True(t, hidden(ctx, httpURL), - "going back with Slack chosen, the HTTP fields are shown") - - require.NoError(t, chromedp.Run( - ctx, chromedp.Evaluate(submitted, &fields), - )) - - assert.Equal( - t, slackFields, fields, - "going back with Slack chosen, the HTTP fields must not be submitted", - ) + assert.Equal(t, "refused", name, + "a refused target does not keep the name entered") + assert.Equal(t, refusedURL, typed, + "a refused target does not keep the url entered") + assert.True(t, shown(ctx, targetName), + "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") } // 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 4f76da9..6ac93e4 100644 --- a/static/js/app.js +++ b/static/js/app.js @@ -87,24 +87,35 @@ document.addEventListener("alpine:init", function () { }; }); - // The add target form. Only the chosen type's fields show, and the - // others are disabled so that the form does not submit them. + // The targets section's add target form, in three steps: closed, + // choosing a type, then filling in that type's fields. targetType + // is empty until Next takes it from the type select. // - // The type is read from the type select when Alpine starts, when the - // select changes, and on pageshow. Going back to the page, the - // browser restores the type chosen before without a change event, - // in some browsers only after Alpine has started, but always before - // pageshow. + // 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. window.Alpine.data("targetForm", function () { return { + choosing: false, targetType: "", init() { - this.readType(); + this.targetType = this.$root.dataset.type; }, - readType() { - this.targetType = this.$root.querySelector( - 'select[name="type"]' - ).value; + add() { + this.choosing = true; + }, + next() { + this.targetType = this.$refs.type.value; + this.choosing = false; + }, + cancel() { + this.choosing = false; + this.targetType = ""; + }, + get filling() { + return this.targetType !== ""; + }, + get closed() { + return !this.choosing && !this.filling; }, get isHttp() { return this.targetType === "http"; @@ -115,14 +126,8 @@ document.addEventListener("alpine:init", function () { get isDatabase() { return this.targetType === "database"; }, - get notHttp() { - return !this.isHttp; - }, - get notSlack() { - return !this.isSlack; - }, - get notDatabase() { - return !this.isDatabase; + get isLog() { + return this.targetType === "log"; }, }; }); diff --git a/templates/source_detail.html b/templates/source_detail.html index b37cda6..0f12d9c 100644 --- a/templates/source_detail.html +++ b/templates/source_detail.html @@ -91,10 +91,10 @@ -
+

Targets

-
- -
-
- -
- - -
-
- -
-
- -

Optional request headers, one Name: value per line, sent with every delivery.

-
-
- - -
-
-
- - + + + +
+ + + +
+
+ {{if .TargetError}} +
{{.TargetError}}
+ {{end}} + + + + + +
+ +
-
- -

Slack or Mattermost incoming webhook URL. Payloads are pretty-printed in code blocks.

-
-
- -

Archive expiry: "never" (default) keeps rows forever, or a duration like "720h" prunes older rows.

-
- - -
+
+
{{range .Targets}}