diff --git a/internal/delivery/target_config_edit.go b/internal/delivery/target_config_edit.go
new file mode 100644
index 0000000..42da682
--- /dev/null
+++ b/internal/delivery/target_config_edit.go
@@ -0,0 +1,119 @@
+package delivery
+
+import (
+ "encoding/json"
+ "errors"
+ "fmt"
+
+ "sneak.berlin/go/webhooker/internal/database"
+)
+
+// errUnknownTargetTypeForEdit is returned when a stored target has a
+// type the edit form has no field set for.
+var errUnknownTargetTypeForEdit = errors.New(
+ "unknown target type",
+)
+
+// TargetConfigForm is the UNMASKED projection of a target's stored
+// configuration, for pre-filling the target edit form.
+//
+// It is the deliberate exception to the rule TargetView enforces
+// everywhere else: TargetView exists so that no template can render
+// a target's stored blob, because a destination URL's path segments
+// and a header value are both routinely the credential. An operator
+// cannot correct a value they cannot see, so the edit form — and
+// only the edit form — is shown the full value.
+//
+// Everything that keeps that exception narrow lives at the call
+// site: the route is behind RequireAuth and the webhook's ownership
+// check, and its group sets NoCache so the rendered secret is not
+// written to a shared cache. Do not reach for this type from any
+// other page.
+type TargetConfigForm struct {
+ // URL is the destination for an HTTP target and the webhook
+ // URL for a Slack target.
+ URL string
+ // Headers is the HTTP target's configured headers in the
+ // textarea representation, one "Name: value" per line.
+ Headers string
+ // Timeout is the HTTP target's per-request timeout in seconds,
+ // empty when unset.
+ Timeout string
+ // Expiry is the database (archive) target's row expiry.
+ Expiry string
+}
+
+// NewTargetConfigForm parses a target's stored configuration into
+// the edit form's fields.
+//
+// A configuration that does not parse is an error rather than a
+// zero-valued form that silently looks like a target with no
+// settings. The caller shows the operator that the stored value
+// could not be read, so that saving the form is understood as
+// replacing it rather than preserving it.
+func NewTargetConfigForm(
+ t *database.Target,
+) (TargetConfigForm, error) {
+ switch t.Type {
+ case database.TargetTypeHTTP:
+ cfg, err := parseHTTPConfig(t.Config)
+ if err != nil {
+ return TargetConfigForm{}, err
+ }
+
+ return TargetConfigForm{
+ URL: cfg.URL,
+ Headers: FormatTargetHeaders(cfg.Headers),
+ Timeout: FormatTargetTimeout(cfg.Timeout),
+ }, nil
+ case database.TargetTypeSlack:
+ cfg, err := parseSlackConfig(t.Config)
+ if err != nil {
+ return TargetConfigForm{}, err
+ }
+
+ return TargetConfigForm{URL: cfg.WebhookURL}, nil
+ case database.TargetTypeDatabase:
+ return databaseConfigForm(t.Config)
+ case database.TargetTypeLog:
+ // The log target takes no configuration.
+ return TargetConfigForm{}, nil
+ default:
+ return TargetConfigForm{}, fmt.Errorf(
+ "%w: %q", errUnknownTargetTypeForEdit, t.Type,
+ )
+ }
+}
+
+// databaseConfigForm parses an archive target's optional expiry.
+// An absent or empty configuration is the keep-forever default and
+// yields an empty field, so re-saving the form unchanged stores the
+// same empty configuration it started with. An expiry that is set
+// but not a valid duration is an error, not a blank field.
+func databaseConfigForm(
+ configJSON string,
+) (TargetConfigForm, error) {
+ if configJSON == "" {
+ return TargetConfigForm{}, nil
+ }
+
+ var cfg databaseTargetConfig
+
+ err := json.Unmarshal([]byte(configJSON), &cfg)
+ if err != nil {
+ return TargetConfigForm{}, fmt.Errorf(
+ "parsing config JSON: %w", err,
+ )
+ }
+
+ if cfg.Expiry == "" || cfg.Expiry == archiveExpiryNever {
+ return TargetConfigForm{}, nil
+ }
+
+ err = ValidateArchiveExpiry(cfg.Expiry)
+ if err != nil {
+ return TargetConfigForm{}, err
+ }
+
+ return TargetConfigForm{Expiry: cfg.Expiry}, nil
+}
diff --git a/internal/delivery/target_headers.go b/internal/delivery/target_headers.go
new file mode 100644
index 0000000..aff657e
--- /dev/null
+++ b/internal/delivery/target_headers.go
@@ -0,0 +1,256 @@
+package delivery
+
+import (
+ "errors"
+ "fmt"
+ "net/http"
+ "slices"
+ "strconv"
+ "strings"
+)
+
+// MaxTargetTimeoutSeconds bounds a per-target request timeout.
+// A delivery attempt holds a worker for its whole duration, so an
+// unbounded timeout lets one misconfigured target stall the queue
+// indefinitely. Five minutes is far beyond any healthy webhook
+// receiver and still finite.
+const MaxTargetTimeoutSeconds = 300
+
+// Errors returned when a target's header or timeout form input
+// cannot be turned into a configuration.
+//
+// None of these ever quotes a header VALUE. A target header value
+// is routinely an authorization token, and these messages are shown
+// to the user in an error page body.
+var (
+ errHeaderLineMalformed = errors.New(
+ `each header line must be "Name: value"`,
+ )
+ errHeaderNameInvalid = errors.New(
+ "header name must be a valid HTTP token",
+ )
+ errHeaderValueInvalid = errors.New(
+ "header value must not contain control characters",
+ )
+ errHeaderDuplicate = errors.New(
+ "header given more than once",
+ )
+ errHeaderReserved = errors.New(
+ "header is set by the delivery engine and cannot be " +
+ "overridden",
+ )
+ errTimeoutInvalid = errors.New(
+ "timeout must be a whole number of seconds",
+ )
+ errTimeoutOutOfRange = errors.New(
+ "timeout is out of range",
+ )
+)
+
+// isReservedTargetHeader reports whether name (canonicalised) is a
+// header a target configuration may not set, because the delivery
+// path or net/http itself writes it regardless.
+//
+// These are rejected rather than accepted-and-ignored. Storing a
+// header that provably never reaches the wire tells the operator
+// their configuration took effect when it did not, which is the
+// same failure mode as silently substituting a default for an
+// invalid value.
+func isReservedTargetHeader(name string) bool {
+ switch name {
+ case "Host", "Content-Length", "Transfer-Encoding", "Connection":
+ return true
+ case "User-Agent":
+ // applyRequestHeaders sets the User-Agent after it applies
+ // the configured headers, so a configured one would always
+ // be overwritten.
+ return true
+ default:
+ return false
+ }
+}
+
+// ParseTargetHeaders turns the target form's headers field — one
+// "Name: value" pair per line, blank lines ignored — into the map
+// stored in HTTPTargetConfig.Headers. Names are canonicalised, so a
+// name repeated in a different case is still a duplicate rather than
+// one pair silently overwriting the other.
+//
+// An input with no pairs yields an empty map, which omitempty drops
+// from the stored config: a target configured with no headers keeps
+// the same config JSON it had before this field existed.
+func ParseTargetHeaders(raw string) (map[string]string, error) {
+ headers := make(map[string]string)
+
+ for i, line := range strings.Split(raw, "\n") {
+ lineNum := i + 1
+
+ line = strings.TrimSpace(line)
+ if line == "" {
+ continue
+ }
+
+ name, value, err := parseHeaderLine(line)
+ if err != nil {
+ return nil, fmt.Errorf("line %d: %w", lineNum, err)
+ }
+
+ if _, dup := headers[name]; dup {
+ return nil, fmt.Errorf(
+ "line %d: %w: %q", lineNum,
+ errHeaderDuplicate, name,
+ )
+ }
+
+ headers[name] = value
+ }
+
+ return headers, nil
+}
+
+// parseHeaderLine splits and validates one "Name: value" line,
+// returning the canonicalised name and the trimmed value.
+func parseHeaderLine(line string) (string, string, error) {
+ rawName, value, found := strings.Cut(line, ":")
+ if !found {
+ return "", "", errHeaderLineMalformed
+ }
+
+ rawName = strings.TrimSpace(rawName)
+ if !validHeaderName(rawName) {
+ return "", "", fmt.Errorf(
+ "%w: %q", errHeaderNameInvalid, rawName,
+ )
+ }
+
+ name := http.CanonicalHeaderKey(rawName)
+ if isReservedTargetHeader(name) {
+ return "", "", fmt.Errorf(
+ "%w: %q", errHeaderReserved, name,
+ )
+ }
+
+ value = strings.TrimSpace(value)
+ if !validHeaderValue(value) {
+ return "", "", fmt.Errorf(
+ "%w: %q", errHeaderValueInvalid, name,
+ )
+ }
+
+ return name, value, nil
+}
+
+// validHeaderName reports whether name is a non-empty RFC 9110
+// field name. Rejecting anything else here is what keeps a value
+// containing CR or LF from being smuggled in as part of a name and
+// injecting a second header into the outbound request.
+func validHeaderName(name string) bool {
+ if name == "" {
+ return false
+ }
+
+ for i := range len(name) {
+ if !isTokenByte(name[i]) {
+ return false
+ }
+ }
+
+ return true
+}
+
+// isTokenByte reports whether c is a "tchar" per RFC 9110 5.6.2.
+func isTokenByte(c byte) bool {
+ switch {
+ case c >= 'a' && c <= 'z',
+ c >= 'A' && c <= 'Z',
+ c >= '0' && c <= '9':
+ return true
+ }
+
+ return strings.IndexByte("!#$%&'*+-.^_`|~", c) >= 0
+}
+
+// validHeaderValue reports whether value is a legal field value:
+// no control characters, which is the other half of the header
+// injection guard. An empty value is legal.
+func validHeaderValue(value string) bool {
+ for i := range len(value) {
+ c := value[i]
+ if c < 0x20 || c == 0x7f {
+ return false
+ }
+ }
+
+ return true
+}
+
+// FormatTargetHeaders renders a stored header map back into the
+// form's textarea representation, one "Name: value" per line.
+//
+// Names are sorted so that loading the edit form twice without
+// saving produces identical text; Go map iteration order would
+// otherwise reshuffle the field on every render.
+func FormatTargetHeaders(headers map[string]string) string {
+ if len(headers) == 0 {
+ return ""
+ }
+
+ names := make([]string, 0, len(headers))
+ for name := range headers {
+ names = append(names, name)
+ }
+
+ slices.Sort(names)
+
+ var b strings.Builder
+
+ for _, name := range names {
+ b.WriteString(name)
+ b.WriteString(": ")
+ b.WriteString(headers[name])
+ b.WriteString("\n")
+ }
+
+ return b.String()
+}
+
+// ParseTargetTimeout interprets the target form's timeout field as
+// a whole number of seconds. An empty field means "unset" and yields
+// 0, which omitempty drops from the stored config and which the
+// delivery path reads as "use the shared client's timeout".
+//
+// Anything else that is not a whole number in range is an error, not
+// a silently substituted default: a target whose timeout was typed
+// wrong must say so at the form rather than deliver on a timeout its
+// operator did not choose.
+func ParseTargetTimeout(raw string) (int, error) {
+ raw = strings.TrimSpace(raw)
+ if raw == "" {
+ return 0, nil
+ }
+
+ v, err := strconv.Atoi(raw)
+ if err != nil || v < 0 {
+ return 0, errTimeoutInvalid
+ }
+
+ if v > MaxTargetTimeoutSeconds {
+ return 0, fmt.Errorf(
+ "%w: at most %d seconds",
+ errTimeoutOutOfRange, MaxTargetTimeoutSeconds,
+ )
+ }
+
+ return v, nil
+}
+
+// FormatTargetTimeout renders a stored timeout for the form field.
+// An unset timeout renders as an empty field rather than "0", so the
+// placeholder can describe the default the target actually uses.
+func FormatTargetTimeout(timeout int) string {
+ if timeout <= 0 {
+ return ""
+ }
+
+ return strconv.Itoa(timeout)
+}
diff --git a/internal/delivery/target_headers_test.go b/internal/delivery/target_headers_test.go
new file mode 100644
index 0000000..c7db5f8
--- /dev/null
+++ b/internal/delivery/target_headers_test.go
@@ -0,0 +1,273 @@
+package delivery_test
+
+import (
+ "encoding/json"
+ "strconv"
+ "testing"
+
+ "github.com/stretchr/testify/assert"
+ "github.com/stretchr/testify/require"
+ "sneak.berlin/go/webhooker/internal/database"
+ "sneak.berlin/go/webhooker/internal/delivery"
+)
+
+// Literals these tests repeat, named so that the header name and the
+// keep-forever archive config each have one definition.
+const (
+ headerAuthorization = "Authorization"
+ bearerValue = "Bearer abc"
+ archiveConfigNever = "{\"expiry\":\"never\"}"
+)
+
+func TestParseTargetHeaders_AcceptsPairs(t *testing.T) {
+ t.Parallel()
+
+ got, err := delivery.ParseTargetHeaders(
+ " Authorization: Bearer abc \n\n" +
+ "x-tenant:acme\r\n" +
+ "X-Empty:\n",
+ )
+ require.NoError(t, err)
+
+ assert.Equal(
+ t,
+ map[string]string{
+ headerAuthorization: bearerValue,
+ "X-Tenant": "acme",
+ "X-Empty": "",
+ },
+ got,
+ )
+}
+
+// A configuration with no headers must stay indistinguishable from
+// one written before the field existed, so omitempty drops the key.
+func TestParseTargetHeaders_EmptyInputYieldsNoHeaders(t *testing.T) {
+ t.Parallel()
+
+ got, err := delivery.ParseTargetHeaders("\n \n")
+ require.NoError(t, err)
+ assert.Empty(t, got)
+
+ encoded, err := json.Marshal(delivery.HTTPTargetConfig{
+ URL: "https://example.com/h",
+ Headers: got,
+ })
+ require.NoError(t, err)
+ assert.JSONEq(
+ t, `{"url":"https://example.com/h"}`, string(encoded),
+ )
+}
+
+func TestParseTargetHeaders_Rejects(t *testing.T) {
+ t.Parallel()
+
+ cases := map[string]string{
+ "no colon": "Authorization Bearer abc",
+ "empty name": ": value",
+ "space in name": "X Bad: value",
+ "reserved host": "Host: evil.example",
+ "reserved ua": "User-Agent: curl/8",
+ "reserved length": "Content-Length: 0",
+ "duplicate any case": "X-A: 1\nx-a: 2",
+ }
+
+ for name, input := range cases {
+ t.Run(name, func(t *testing.T) {
+ t.Parallel()
+
+ _, err := delivery.ParseTargetHeaders(input)
+ require.Error(t, err)
+ })
+ }
+}
+
+// A header value is routinely a bearer token and these errors are
+// rendered into a 400 body, so no message may quote one.
+func TestParseTargetHeaders_ErrorsNeverQuoteAValue(t *testing.T) {
+ t.Parallel()
+
+ const secret = "QQNEVERINAMESSAGEQQ"
+
+ _, err := delivery.ParseTargetHeaders(
+ "X-A: " + secret + "\nx-a: " + secret,
+ )
+ require.Error(t, err)
+ assert.NotContains(t, err.Error(), secret)
+
+ _, err = delivery.ParseTargetHeaders(
+ "X Bad Name: " + secret,
+ )
+ require.Error(t, err)
+ assert.NotContains(t, err.Error(), secret)
+}
+
+// Loading the edit form twice without saving must not reshuffle
+// the textarea, which Go's map iteration order would otherwise do.
+func TestFormatTargetHeaders_IsSorted(t *testing.T) {
+ t.Parallel()
+
+ got := delivery.FormatTargetHeaders(map[string]string{
+ "X-Zed": "z",
+ headerAuthorization: bearerValue,
+ "X-Alpha": "a",
+ })
+
+ assert.Equal(
+ t,
+ "Authorization: Bearer abc\nX-Alpha: a\nX-Zed: z\n",
+ got,
+ )
+ assert.Empty(t, delivery.FormatTargetHeaders(nil))
+}
+
+func TestFormatTargetHeaders_RoundTripsThroughParse(t *testing.T) {
+ t.Parallel()
+
+ want := map[string]string{
+ headerAuthorization: bearerValue,
+ "X-Tenant": "acme",
+ }
+
+ got, err := delivery.ParseTargetHeaders(
+ delivery.FormatTargetHeaders(want),
+ )
+ require.NoError(t, err)
+ assert.Equal(t, want, got)
+}
+
+func TestParseTargetTimeout(t *testing.T) {
+ t.Parallel()
+
+ got, err := delivery.ParseTargetTimeout(" 30 ")
+ require.NoError(t, err)
+ assert.Equal(t, 30, got)
+
+ got, err = delivery.ParseTargetTimeout("")
+ require.NoError(t, err)
+ assert.Zero(t, got)
+
+ for _, bad := range []string{"soon", "-1", "1e3", "100000"} {
+ _, err = delivery.ParseTargetTimeout(bad)
+ require.Error(t, err, bad)
+ }
+}
+
+func TestFormatTargetTimeout(t *testing.T) {
+ t.Parallel()
+
+ assert.Equal(t, "30", delivery.FormatTargetTimeout(30))
+ assert.Empty(t, delivery.FormatTargetTimeout(0))
+ assert.Empty(t, delivery.FormatTargetTimeout(-1))
+}
+
+func TestNewTargetConfigForm(t *testing.T) {
+ t.Parallel()
+
+ form, err := delivery.NewTargetConfigForm(&database.Target{
+ Type: database.TargetTypeHTTP,
+ Config: `{"url":"https://example.com/h",` +
+ `"headers":{"Authorization":"Bearer abc"},` +
+ `"timeout":9}`,
+ })
+ require.NoError(t, err)
+ assert.Equal(t, "https://example.com/h", form.URL)
+ assert.Equal(t, "Authorization: Bearer abc\n", form.Headers)
+ assert.Equal(t, "9", form.Timeout)
+
+ form, err = delivery.NewTargetConfigForm(&database.Target{
+ Type: database.TargetTypeSlack,
+ Config: `{"webhookUrl":"https://hooks.example/s"}`,
+ })
+ require.NoError(t, err)
+ assert.Equal(t, "https://hooks.example/s", form.URL)
+
+ form, err = delivery.NewTargetConfigForm(&database.Target{
+ Type: database.TargetTypeDatabase,
+ Config: `{"expiry":"720h"}`,
+ })
+ require.NoError(t, err)
+ assert.Equal(t, "720h", form.Expiry)
+
+ form, err = delivery.NewTargetConfigForm(&database.Target{
+ Type: database.TargetTypeLog,
+ })
+ require.NoError(t, err)
+ assert.Empty(t, form.URL)
+}
+
+// A keep-forever archive target must pre-fill as an empty field, so
+// saving the form back unchanged stores the same empty config.
+func TestNewTargetConfigForm_DatabaseNeverIsBlank(t *testing.T) {
+ t.Parallel()
+
+ for _, cfg := range []string{"", `{}`, archiveConfigNever} {
+ form, err := delivery.NewTargetConfigForm(
+ &database.Target{
+ Type: database.TargetTypeDatabase,
+ Config: cfg,
+ },
+ )
+ require.NoError(t, err, cfg)
+ assert.Empty(t, form.Expiry, cfg)
+ }
+}
+
+// An unreadable stored config is an error rather than a blank form
+// that looks like a target with no settings, so the caller can tell
+// the operator that saving replaces the stored value.
+func TestNewTargetConfigForm_UnreadableConfigErrors(t *testing.T) {
+ t.Parallel()
+
+ cases := []*database.Target{
+ {Type: database.TargetTypeHTTP, Config: "not json"},
+ {Type: database.TargetTypeHTTP, Config: `{}`},
+ {Type: database.TargetTypeSlack, Config: ""},
+ {
+ Type: database.TargetTypeDatabase,
+ Config: `{"expiry":"soon"}`,
+ },
+ {Type: database.TargetType("nope")},
+ }
+
+ for _, target := range cases {
+ _, err := delivery.NewTargetConfigForm(target)
+ require.Error(t, err, target.Type)
+ }
+}
+
+// The ceiling exists so one misconfigured target cannot hold a
+// delivery worker indefinitely, and it is inclusive.
+func TestParseTargetTimeout_CeilingIsInclusive(t *testing.T) {
+ t.Parallel()
+
+ assert.Positive(t, delivery.MaxTargetTimeoutSeconds)
+
+ got, err := delivery.ParseTargetTimeout(
+ strconv.Itoa(delivery.MaxTargetTimeoutSeconds),
+ )
+ require.NoError(t, err)
+ assert.Equal(t, delivery.MaxTargetTimeoutSeconds, got)
+
+ _, err = delivery.ParseTargetTimeout(
+ strconv.Itoa(delivery.MaxTargetTimeoutSeconds + 1),
+ )
+ require.Error(t, err)
+}
+
+// Control characters in a value are how a second header would be
+// smuggled into the outbound request.
+func TestParseTargetHeaders_RejectsControlCharactersInValues(
+ t *testing.T,
+) {
+ t.Parallel()
+
+ for _, bad := range []string{
+ "X-A: one\x01two",
+ "X-A: one\ttwo",
+ "X-A: one\x7ftwo",
+ } {
+ _, err := delivery.ParseTargetHeaders(bad)
+ require.Error(t, err, bad)
+ }
+}
diff --git a/internal/handlers/export_test.go b/internal/handlers/export_test.go
index 1d420a2..d2671c5 100644
--- a/internal/handlers/export_test.go
+++ b/internal/handlers/export_test.go
@@ -69,18 +69,29 @@ func (s *Handlers) RenderTemplateForTest(
s.renderTemplate(w, r, pageTemplate, data)
}
-// BuildSlackTargetConfigForTest exposes buildURLTargetConfig
-// with the Slack target parameters for use in the
-// handlers_test package.
+// BuildSlackTargetConfigForTest exposes
+// buildSlackTargetConfig for use in the handlers_test package.
func (s *Handlers) BuildSlackTargetConfigForTest(
w http.ResponseWriter,
r *http.Request,
targetURL string,
) (string, error) {
- return s.buildURLTargetConfig(
- w, r, targetURL, "webhookUrl",
- "Webhook URL is required for Slack targets",
- )
+ return s.buildSlackTargetConfig(w, r, 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,
+ targetURL, headers, timeout string,
+) (string, error) {
+ return s.buildHTTPTargetConfig(w, r, targetFormInput{
+ URL: targetURL,
+ Headers: headers,
+ Timeout: timeout,
+ })
}
// BuildDatabaseTargetConfigForTest exposes
diff --git a/internal/handlers/handlers.go b/internal/handlers/handlers.go
index 9100867..35efb1d 100644
--- a/internal/handlers/handlers.go
+++ b/internal/handlers/handlers.go
@@ -127,6 +127,7 @@ func New(
"source_detail.html": parsePageTemplate("source_detail.html"),
"source_edit.html": parsePageTemplate("source_edit.html"),
"source_logs.html": parsePageTemplate("source_logs.html"),
+ "target_edit.html": parsePageTemplate("target_edit.html"),
}
lc.Append(fx.Hook{
diff --git a/internal/handlers/source_management.go b/internal/handlers/source_management.go
index 5e4602c..024a9a1 100644
--- a/internal/handlers/source_management.go
+++ b/internal/handlers/source_management.go
@@ -1029,9 +1029,7 @@ func (h *Handlers) processTargetCreate(
// Referer headers and error trackers record.
name := r.PostFormValue("name")
targetType := database.TargetType(r.PostFormValue("type"))
- targetURL := r.PostFormValue("url")
maxRetriesStr := r.PostFormValue("max_retries")
- expiry := r.PostFormValue("expiry")
if name == "" {
http.Error(
@@ -1051,7 +1049,7 @@ func (h *Handlers) processTargetCreate(
}
configJSON, err := h.buildTargetConfig(
- w, r, targetType, targetURL, expiry,
+ w, r, targetType, targetFormInputFrom(r),
)
if err != nil {
return
@@ -1108,28 +1106,60 @@ func parseNonNegativeInt(s string) int {
return 0
}
-// buildTargetConfig builds the JSON config string for a target.
-// The expiry form value is read by the caller (which bounds the
-// request body) and applies to database targets only.
+// 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.
+type targetFormInput struct {
+ // URL is the destination for an HTTP target and the webhook URL
+ // for a Slack target.
+ URL string
+ // Headers is an HTTP target's headers, one "Name: value" per
+ // line.
+ Headers string
+ // Timeout is an HTTP target's per-request timeout in seconds.
+ Timeout 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.
+//
+// Every field is read with PostFormValue, not FormValue. FormValue
+// falls back to the query string, which would let
+// `POST /source/{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. The headers field is under the
+// same rule and for the same reason: its values are authorization
+// 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"),
+ }
+}
+
+// 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.
func (h *Handlers) buildTargetConfig(
w http.ResponseWriter,
r *http.Request,
targetType database.TargetType,
- targetURL, expiry string,
+ in targetFormInput,
) (string, error) {
switch targetType {
case database.TargetTypeHTTP:
- return h.buildURLTargetConfig(
- w, r, targetURL, "url",
- "URL is required for HTTP targets",
- )
+ return h.buildHTTPTargetConfig(w, r, in)
case database.TargetTypeSlack:
- return h.buildURLTargetConfig(
- w, r, targetURL, "webhookUrl",
- "Webhook URL is required for Slack targets",
- )
+ return h.buildSlackTargetConfig(w, r, in.URL)
case database.TargetTypeDatabase:
- return h.buildDatabaseTargetConfig(w, expiry)
+ return h.buildDatabaseTargetConfig(w, in.Expiry)
case database.TargetTypeLog:
return "", nil
default:
@@ -1142,14 +1172,83 @@ func (h *Handlers) buildTargetConfig(
}
}
-// buildURLTargetConfig builds config JSON for a target whose
-// configuration is a single SSRF-validated URL stored under
-// configKey. missingMsg is the error shown when no URL is given.
-func (h *Handlers) buildURLTargetConfig(
+// buildHTTPTargetConfig builds config JSON for an HTTP target: an
+// SSRF-validated destination plus the optional headers and timeout
+// the delivery path honours.
+func (h *Handlers) buildHTTPTargetConfig(
w http.ResponseWriter,
r *http.Request,
- targetURL, configKey, missingMsg string,
+ in targetFormInput,
) (string, error) {
+ err := h.validateTargetURL(
+ w, r, in.URL, "URL is required for HTTP targets",
+ )
+ if err != nil {
+ return "", err
+ }
+
+ headers, err := delivery.ParseTargetHeaders(in.Headers)
+ if err != nil {
+ http.Error(
+ w,
+ "Invalid headers: "+err.Error(),
+ http.StatusBadRequest,
+ )
+
+ return "", err
+ }
+
+ timeout, err := delivery.ParseTargetTimeout(in.Timeout)
+ if err != nil {
+ http.Error(
+ w,
+ "Invalid timeout: "+err.Error(),
+ http.StatusBadRequest,
+ )
+
+ return "", err
+ }
+
+ return marshalTargetConfig(w, delivery.HTTPTargetConfig{
+ URL: in.URL,
+ Headers: headers,
+ Timeout: timeout,
+ })
+}
+
+// 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,
+ targetURL string,
+) (string, error) {
+ err := h.validateTargetURL(
+ w, r, targetURL,
+ "Webhook URL is required for Slack targets",
+ )
+ if err != nil {
+ return "", err
+ }
+
+ return marshalTargetConfig(w, 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.
+//
+// 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,
+ targetURL, missingMsg string,
+) error {
if targetURL == "" {
http.Error(
w,
@@ -1157,7 +1256,7 @@ func (h *Handlers) buildURLTargetConfig(
http.StatusBadRequest,
)
- return "", errMissingURL
+ return errMissingURL
}
err := delivery.ValidateTargetURL(
@@ -1178,11 +1277,18 @@ func (h *Handlers) buildURLTargetConfig(
http.StatusBadRequest,
)
- return "", err
+ return err
}
- cfg := map[string]any{configKey: targetURL}
+ return nil
+}
+// marshalTargetConfig serialises a target configuration for storage,
+// writing a 500 itself if it cannot.
+func marshalTargetConfig(
+ w http.ResponseWriter,
+ cfg any,
+) (string, error) {
configBytes, err := json.Marshal(cfg)
if err != nil {
http.Error(
@@ -1222,19 +1328,9 @@ func (h *Handlers) buildDatabaseTargetConfig(
return "", err
}
- cfg := map[string]any{"expiry": expiry}
-
- configBytes, err := json.Marshal(cfg)
- if err != nil {
- http.Error(
- w, "Internal server error",
- http.StatusInternalServerError,
- )
-
- return "", err
- }
-
- return string(configBytes), nil
+ return marshalTargetConfig(
+ w, map[string]any{"expiry": expiry},
+ )
}
// HandleEntrypointDelete handles deleting an entrypoint.
diff --git a/internal/handlers/target_edit.go b/internal/handlers/target_edit.go
new file mode 100644
index 0000000..d8ec6ee
--- /dev/null
+++ b/internal/handlers/target_edit.go
@@ -0,0 +1,221 @@
+package handlers
+
+import (
+ "net/http"
+
+ "github.com/go-chi/chi"
+ "sneak.berlin/go/webhooker/internal/database"
+ "sneak.berlin/go/webhooker/internal/delivery"
+)
+
+// targetEditTemplate is the page the target edit form renders.
+const targetEditTemplate = "target_edit.html"
+
+// tmplKeyTarget is the template data key for the target being
+// edited, and tmplKeyMaxTimeout for the timeout ceiling the form
+// tells the user about.
+const (
+ tmplKeyTarget = "Target"
+ tmplKeyMaxTimeout = "MaxTimeout"
+)
+
+// configUnreadableMessage is shown when a target's stored
+// configuration does not parse. It says plainly that saving replaces
+// the stored value rather than preserving it, because the form
+// cannot pre-fill what it could not read.
+const configUnreadableMessage = "The stored configuration for this " +
+ "target could not be read. Enter the values below; saving " +
+ "replaces the stored configuration."
+
+// targetEditView is the display model for the target edit page.
+//
+// It carries the target's row fields alongside its UNMASKED
+// configuration, and deliberately omits database.Target's raw
+// Config blob: the form renders named fields, and giving the
+// template the blob as well would put an unreviewed second path to
+// the credential on the page.
+type targetEditView struct {
+ ID string
+ Name string
+ Type database.TargetType
+ Active bool
+ MaxRetries int
+ Config delivery.TargetConfigForm
+}
+
+// HandleTargetEdit shows the form to edit a target.
+//
+// This page is the one place the full destination URL and header
+// values are shown. It is reachable only through the
+// /source/{sourceID} route group, which supplies RequireAuth and
+// NoCache, and only for a target of a webhook the session's user
+// owns; masking (delivery.TargetView) is unchanged everywhere else.
+func (h *Handlers) HandleTargetEdit() http.HandlerFunc {
+ return func(w http.ResponseWriter, r *http.Request) {
+ webhook, target, ok := h.ownedTarget(w, r)
+ if !ok {
+ return
+ }
+
+ cfg, err := delivery.NewTargetConfigForm(target)
+ msg := ""
+
+ if err != nil {
+ // The error carries the parse failure, never the
+ // blob, so it is safe to log against the target id.
+ h.log.Warn(
+ "stored target config could not be read for editing",
+ "target_id", target.ID,
+ "error", err,
+ )
+
+ msg = configUnreadableMessage
+ }
+
+ h.renderTargetEdit(w, r, webhook, target, cfg, msg)
+ }
+}
+
+// HandleTargetEditSubmit handles the target edit form submission.
+func (h *Handlers) HandleTargetEditSubmit() http.HandlerFunc {
+ return func(w http.ResponseWriter, r *http.Request) {
+ webhook, target, ok := h.ownedTarget(w, r)
+ if !ok {
+ return
+ }
+
+ // The body size cap is enforced by the MaxBodySize
+ // middleware, which runs before CSRF parses the form.
+ err := r.ParseForm()
+ if err != nil {
+ http.Error(
+ w, "Bad request", http.StatusBadRequest,
+ )
+
+ return
+ }
+
+ h.applyTargetEdit(w, r, webhook, target)
+ }
+}
+
+// applyTargetEdit validates and saves target edits.
+//
+// The submitted configuration goes through buildTargetConfig, the
+// same builder the create path uses, so an edited destination is
+// SSRF-validated exactly as a new one is.
+//
+// The target's type is not editable. Each type stores a different
+// configuration shape and its delivery history is recorded against
+// the target row, so changing the type of an existing target is
+// really the creation of a different one. The stored type decides
+// which fields the form offers and which builder runs.
+func (h *Handlers) applyTargetEdit(
+ w http.ResponseWriter,
+ r *http.Request,
+ webhook database.Webhook,
+ target *database.Target,
+) {
+ name := r.PostFormValue("name")
+ if name == "" {
+ http.Error(
+ w, "Name is required", http.StatusBadRequest,
+ )
+
+ return
+ }
+
+ configJSON, err := h.buildTargetConfig(
+ w, r, target.Type, targetFormInputFrom(r),
+ )
+ if err != nil {
+ // buildTargetConfig has already written the response.
+ return
+ }
+
+ target.Name = name
+ target.Config = configJSON
+
+ // Retries are offered only by the forms for target types that
+ // retry, so an absent field means "this form does not edit
+ // retries" rather than "set them to zero". Reading it
+ // unconditionally would silently disable retries on any target
+ // saved from a form that does not render the input.
+ if r.PostForm.Has("max_retries") {
+ target.MaxRetries = parseNonNegativeInt(
+ r.PostFormValue("max_retries"),
+ )
+ }
+
+ err = h.db.DB().Save(target).Error
+ if err != nil {
+ h.serverError(w, "failed to update target", err)
+
+ return
+ }
+
+ http.Redirect(
+ w, r, "/source/"+webhook.ID, http.StatusSeeOther,
+ )
+}
+
+// renderTargetEdit renders the target edit page with an optional
+// error message.
+func (h *Handlers) renderTargetEdit(
+ w http.ResponseWriter,
+ r *http.Request,
+ webhook database.Webhook,
+ target *database.Target,
+ cfg delivery.TargetConfigForm,
+ errMsg string,
+) {
+ // The template calls Webhook methods, which take pointer
+ // receivers; html/template cannot address a value stored in a
+ // map.
+ data := map[string]any{
+ tmplKeyWebhook: &webhook,
+ tmplKeyTarget: targetEditView{
+ ID: target.ID,
+ Name: target.Name,
+ Type: target.Type,
+ Active: target.Active,
+ MaxRetries: target.MaxRetries,
+ Config: cfg,
+ },
+ tmplKeyMaxTimeout: delivery.MaxTargetTimeoutSeconds,
+ tmplKeyError: errMsg,
+ }
+
+ h.renderTemplate(w, r, targetEditTemplate, data)
+}
+
+// ownedTarget resolves the request's sourceID and targetID
+// parameters to a target of a webhook the session's user owns.
+//
+// Ownership is decided by the webhook, and the target is then
+// scoped to that webhook, so a target id belonging to someone
+// else's webhook is a 404 rather than an edit of their target. It
+// reports false once it has written the response.
+func (h *Handlers) ownedTarget(
+ w http.ResponseWriter,
+ r *http.Request,
+) (database.Webhook, *database.Target, bool) {
+ webhook, ok := h.ownedWebhook(w, r)
+ if !ok {
+ return database.Webhook{}, nil, false
+ }
+
+ var target database.Target
+
+ err := h.db.DB().Where(
+ "id = ? AND webhook_id = ?",
+ chi.URLParam(r, "targetID"), webhook.ID,
+ ).First(&target).Error
+ if err != nil {
+ http.NotFound(w, r)
+
+ return database.Webhook{}, nil, false
+ }
+
+ return webhook, &target, true
+}
diff --git a/internal/handlers/target_edit_test.go b/internal/handlers/target_edit_test.go
new file mode 100644
index 0000000..64a93e4
--- /dev/null
+++ b/internal/handlers/target_edit_test.go
@@ -0,0 +1,637 @@
+package handlers_test
+
+import (
+ "context"
+ "encoding/json"
+ "net/http"
+ "net/http/httptest"
+ "net/url"
+ "strings"
+ "testing"
+
+ "github.com/go-chi/chi"
+ "github.com/stretchr/testify/assert"
+ "github.com/stretchr/testify/require"
+ "gorm.io/gorm/clause"
+ "sneak.berlin/go/webhooker/internal/database"
+ "sneak.berlin/go/webhooker/internal/delivery"
+)
+
+// The destinations the target edit tests configure. Both are literal
+// public addresses rather than hostnames so the SSRF check resolves
+// nothing: with a hostname, a sandbox without DNS would reject the
+// URL for the wrong reason and a test asserting rejection would pass
+// even with the guard removed.
+const (
+ editOriginalURL = "https://93.184.216.34/hooks/original"
+ editReplacedURL = "https://93.184.216.34/hooks/replaced"
+ // editBlockedURL resolves to loopback, which the SSRF guard
+ // refuses. It is what proves the guard runs on the edit path.
+ editBlockedURL = "http://127.0.0.1/hooks/internal"
+)
+
+// editAuthHeader carries a bearer credential, the case the headers
+// field exists for.
+const (
+ editBearerSecret = "QQEDITSECRETQQ"
+ editAuthHeader = "Authorization: Bearer " + editBearerSecret
+)
+
+// targetRouter mounts the target create and edit routes on a chi
+// router so the handlers see the URL parameters they read.
+func targetRouter(env *sourceTestEnv) *chi.Mux {
+ router := chi.NewRouter()
+ router.Post(
+ "/source/{sourceID}/targets",
+ env.handlers.HandleTargetCreate(),
+ )
+ router.Get(
+ "/source/{sourceID}/targets/{targetID}/edit",
+ env.handlers.HandleTargetEdit(),
+ )
+ router.Post(
+ "/source/{sourceID}/targets/{targetID}/edit",
+ env.handlers.HandleTargetEditSubmit(),
+ )
+
+ return router
+}
+
+// serveTarget drives one request through the target routes as the
+// authenticated test user.
+func serveTarget(
+ env *sourceTestEnv,
+ method, path string,
+ form url.Values,
+) *httptest.ResponseRecorder {
+ body := ""
+ if form != nil {
+ body = form.Encode()
+ }
+
+ req := httptest.NewRequestWithContext(
+ context.Background(), method, path,
+ strings.NewReader(body),
+ )
+ if form != nil {
+ req.Header.Set(
+ "Content-Type",
+ "application/x-www-form-urlencoded",
+ )
+ }
+
+ for _, c := range env.cookies {
+ req.AddCookie(c)
+ }
+
+ w := httptest.NewRecorder()
+ targetRouter(env).ServeHTTP(w, req)
+
+ return w
+}
+
+// seedHTTPTarget creates a webhook and an HTTP target on it through
+// the real create handler, so every case starts from a target the
+// production path produced rather than a hand-written row.
+//
+// Standing the fx app up is what a handler test mostly costs, and
+// internal/handlers is already the slowest package in the suite, so
+// the tests below share one env per test function and give each case
+// its own webhook rather than its own app.
+func seedHTTPTarget(
+ t *testing.T,
+ env *sourceTestEnv,
+ headers, timeout string,
+) (database.Webhook, database.Target) {
+ t.Helper()
+
+ webhook := seedWebhookWithRetention(t, env.db, 30)
+
+ form := url.Values{}
+ form.Set("name", "original-name")
+ form.Set("type", string(database.TargetTypeHTTP))
+ form.Set("url", editOriginalURL)
+ form.Set("headers", headers)
+ form.Set("timeout", timeout)
+ form.Set("max_retries", "3")
+
+ w := serveTarget(
+ env, http.MethodPost,
+ "/source/"+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)
+
+ return webhook, targets[0]
+}
+
+// storedTarget reloads a target row.
+func storedTarget(
+ t *testing.T,
+ env *sourceTestEnv,
+ targetID string,
+) database.Target {
+ t.Helper()
+
+ var target database.Target
+
+ require.NoError(
+ t,
+ env.db.DB().Where("id = ?", targetID).
+ First(&target).Error,
+ )
+
+ return target
+}
+
+// storedHTTPConfig reloads a target and parses its stored HTTP
+// configuration.
+func storedHTTPConfig(
+ t *testing.T,
+ env *sourceTestEnv,
+ targetID string,
+) delivery.HTTPTargetConfig {
+ t.Helper()
+
+ var cfg delivery.HTTPTargetConfig
+
+ require.NoError(
+ t,
+ json.Unmarshal(
+ []byte(storedTarget(t, env, targetID).Config), &cfg,
+ ),
+ )
+
+ return cfg
+}
+
+// editForm is the fully populated edit submission for an HTTP
+// target.
+func editForm(targetURL, headers, timeout string) url.Values {
+ form := url.Values{}
+ form.Set("name", "edited-name")
+ form.Set("url", targetURL)
+ form.Set("headers", headers)
+ form.Set("timeout", timeout)
+ form.Set("max_retries", "5")
+
+ return form
+}
+
+// submitTargetEdit posts the edit form for a target.
+func submitTargetEdit(
+ env *sourceTestEnv,
+ webhookID, targetID string,
+ form url.Values,
+) *httptest.ResponseRecorder {
+ return serveTarget(
+ env, http.MethodPost,
+ "/source/"+webhookID+"/targets/"+targetID+"/edit",
+ form,
+ )
+}
+
+// TestHandleTargetCreate_Configuration covers the half of the gap
+// that is not about editing at all: HTTPTargetConfig has carried
+// Headers and Timeout, and the delivery path has honoured them, but
+// the create form wrote {"url":...} and nothing else, so a
+// destination needing an Authorization header could not be
+// configured through the UI at all.
+func TestHandleTargetCreate_Configuration(t *testing.T) {
+ t.Parallel()
+
+ env := setupSourceTest(t)
+
+ t.Run("stores headers and timeout", func(t *testing.T) {
+ t.Parallel()
+ assertCreateStoresHeadersAndTimeout(t, env)
+ })
+
+ t.Run("without them keeps a url-only config", func(t *testing.T) {
+ t.Parallel()
+ assertCreateKeepsURLOnlyConfig(t, env)
+ })
+}
+
+func assertCreateStoresHeadersAndTimeout(
+ t *testing.T, env *sourceTestEnv,
+) {
+ t.Helper()
+
+ _, target := seedHTTPTarget(
+ t, env, editAuthHeader+"\nX-Tenant: acme\n", "12",
+ )
+
+ cfg := storedHTTPConfig(t, env, target.ID)
+
+ assert.Equal(t, editOriginalURL, cfg.URL)
+ assert.Equal(t, 12, cfg.Timeout)
+ assert.Equal(
+ t,
+ map[string]string{
+ "Authorization": "Bearer " + editBearerSecret,
+ "X-Tenant": "acme",
+ },
+ cfg.Headers,
+ )
+}
+
+// Without the new fields the stored shape must be the same
+// {"url":...} the create form wrote before they existed, so no
+// existing target's configuration is rewritten by this change.
+func assertCreateKeepsURLOnlyConfig(
+ t *testing.T, env *sourceTestEnv,
+) {
+ t.Helper()
+
+ _, target := seedHTTPTarget(t, env, "", "")
+
+ assert.JSONEq(
+ t, `{"url":"`+editOriginalURL+`"}`, target.Config,
+ )
+}
+
+// TestHandleTargetEditSubmit_Saves is the round trip the issue asks
+// for: create a target, edit it, and confirm the stored config
+// changed.
+func TestHandleTargetEditSubmit_Saves(t *testing.T) {
+ t.Parallel()
+
+ env := setupSourceTest(t)
+
+ t.Run("changes the destination URL", func(t *testing.T) {
+ t.Parallel()
+ assertEditChangesDestination(t, env)
+ })
+
+ t.Run("round trips headers and timeout", func(t *testing.T) {
+ t.Parallel()
+ assertEditRoundTripsHeadersAndTimeout(t, env)
+ })
+
+ t.Run("clearing them removes them", func(t *testing.T) {
+ t.Parallel()
+ assertEditClearingRemovesThem(t, env)
+ })
+
+ t.Run("absent max_retries is not zeroed", func(t *testing.T) {
+ t.Parallel()
+ assertEditKeepsAbsentMaxRetries(t, env)
+ })
+}
+
+func assertEditChangesDestination(
+ t *testing.T, env *sourceTestEnv,
+) {
+ t.Helper()
+
+ webhook, target := seedHTTPTarget(t, env, "", "")
+
+ w := submitTargetEdit(
+ env, webhook.ID, target.ID,
+ editForm(editReplacedURL, "", ""),
+ )
+ require.Equal(t, http.StatusSeeOther, w.Code, w.Body.String())
+
+ assert.Equal(
+ t,
+ editReplacedURL,
+ storedHTTPConfig(t, env, target.ID).URL,
+ )
+
+ reloaded := storedTarget(t, env, target.ID)
+ assert.Equal(t, "edited-name", reloaded.Name)
+ assert.Equal(t, 5, reloaded.MaxRetries)
+ assert.Equal(
+ t, database.TargetTypeHTTP, reloaded.Type,
+ "the edit form must not change a target's type",
+ )
+}
+
+// The two previously unreachable fields must survive create,
+// pre-fill and save.
+func assertEditRoundTripsHeadersAndTimeout(
+ t *testing.T, env *sourceTestEnv,
+) {
+ t.Helper()
+
+ webhook, target := seedHTTPTarget(t, env, editAuthHeader, "7")
+
+ w := submitTargetEdit(
+ env, webhook.ID, target.ID,
+ editForm(
+ editOriginalURL,
+ "Authorization: Bearer rotated\nX-Trace: on",
+ "21",
+ ),
+ )
+ require.Equal(t, http.StatusSeeOther, w.Code, w.Body.String())
+
+ cfg := storedHTTPConfig(t, env, target.ID)
+
+ assert.Equal(t, 21, cfg.Timeout)
+ assert.Equal(
+ t,
+ map[string]string{
+ "Authorization": "Bearer rotated",
+ "X-Trace": "on",
+ },
+ cfg.Headers,
+ )
+}
+
+// The direction a naive "only set what was submitted" implementation
+// gets wrong: an emptied field must remove the stored value, not
+// leave the previous one in place.
+func assertEditClearingRemovesThem(
+ t *testing.T, env *sourceTestEnv,
+) {
+ t.Helper()
+
+ webhook, target := seedHTTPTarget(t, env, editAuthHeader, "7")
+
+ w := submitTargetEdit(
+ env, webhook.ID, target.ID,
+ editForm(editOriginalURL, "", ""),
+ )
+ require.Equal(t, http.StatusSeeOther, w.Code, w.Body.String())
+
+ cfg := storedHTTPConfig(t, env, target.ID)
+
+ assert.Empty(t, cfg.Headers)
+ assert.Zero(t, cfg.Timeout)
+}
+
+// Retries are offered only by the forms for target types that retry.
+// An absent field means the form does not edit retries, not that
+// they should be turned off.
+func assertEditKeepsAbsentMaxRetries(
+ t *testing.T, env *sourceTestEnv,
+) {
+ t.Helper()
+
+ webhook, target := seedHTTPTarget(t, env, "", "")
+ require.Equal(t, 3, target.MaxRetries)
+
+ form := editForm(editOriginalURL, "", "")
+ form.Del("max_retries")
+
+ w := submitTargetEdit(env, webhook.ID, target.ID, form)
+ require.Equal(t, http.StatusSeeOther, w.Code, w.Body.String())
+
+ assert.Equal(
+ t, 3, storedTarget(t, env, target.ID).MaxRetries,
+ )
+}
+
+// TestHandleTargetEdit_PrefillsTheStoredValuesUnmasked covers the
+// deliberate exception to the masking rule. The operator cannot
+// correct a value they cannot see, so this page — and only this page
+// — renders the destination and the header values in full.
+func TestHandleTargetEdit_PrefillsTheStoredValuesUnmasked(
+ t *testing.T,
+) {
+ t.Parallel()
+
+ env := setupSourceTest(t)
+
+ webhook, target := seedHTTPTarget(t, env, editAuthHeader, "7")
+
+ w := serveTarget(
+ env, http.MethodGet,
+ "/source/"+webhook.ID+"/targets/"+target.ID+"/edit",
+ nil,
+ )
+ require.Equal(t, http.StatusOK, w.Code)
+
+ page := w.Body.String()
+
+ assert.Contains(t, page, editOriginalURL)
+ assert.Contains(t, page, "Bearer "+editBearerSecret)
+ assert.Contains(t, page, `value="7"`)
+ assert.Contains(t, page, "original-name")
+}
+
+// TestHandleTargetEditSubmit_Rejects covers every submission that
+// must not reach storage.
+//
+// The SSRF case is the most important assertion on this change: the
+// edited destination goes through the same guard the create path
+// uses. An edit that stored an unvalidated URL would reopen a closed
+// hole, since a target could then be created public and edited to
+// point at loopback.
+//
+// The header and timeout cases keep input that could not be
+// delivered as written out of storage: a stored value that provably
+// never reaches the wire reports a configuration that did not take
+// effect.
+func TestHandleTargetEditSubmit_Rejects(t *testing.T) {
+ t.Parallel()
+
+ env := setupSourceTest(t)
+
+ t.Run("an SSRF-blocked destination", func(t *testing.T) {
+ t.Parallel()
+ assertEditRejectsBlockedDestination(t, env)
+ })
+
+ t.Run("a query-string destination", func(t *testing.T) {
+ t.Parallel()
+ assertEditIgnoresQueryString(t, env)
+ })
+
+ headerCases := map[string]string{
+ "no colon": "Authorization Bearer token",
+ "empty name": ": value",
+ "invalid name": "X Bad Name: value",
+ "reserved header": "User-Agent: curl/8",
+ "duplicate name": "X-A: one\nx-a: two",
+ }
+
+ for name, headers := range headerCases {
+ t.Run("headers: "+name, func(t *testing.T) {
+ t.Parallel()
+ assertEditRejectsHeaders(t, env, headers)
+ })
+ }
+
+ timeoutCases := map[string]string{
+ "not a number": "soon",
+ "negative": "-1",
+ "over ceiling": "100000",
+ }
+
+ for name, timeout := range timeoutCases {
+ t.Run("timeout: "+name, func(t *testing.T) {
+ t.Parallel()
+ assertEditRejectsTimeout(t, env, timeout)
+ })
+ }
+}
+
+func assertEditRejectsBlockedDestination(
+ t *testing.T, env *sourceTestEnv,
+) {
+ t.Helper()
+
+ webhook, target := seedHTTPTarget(t, env, "", "")
+
+ w := submitTargetEdit(
+ env, webhook.ID, target.ID,
+ editForm(editBlockedURL, "", ""),
+ )
+
+ assert.Equal(t, http.StatusBadRequest, w.Code)
+ assert.Contains(t, w.Body.String(), "Invalid target URL")
+ assert.Equal(
+ t, editOriginalURL,
+ storedHTTPConfig(t, env, target.ID).URL,
+ "a rejected edit must leave the stored config alone",
+ )
+}
+
+// The ingress rule the create path already follows applies to the
+// edit path too: reading a field with FormValue would let the request
+// line carry the credential, and the request line is what logs,
+// proxies and Referer headers record.
+func assertEditIgnoresQueryString(
+ t *testing.T, env *sourceTestEnv,
+) {
+ t.Helper()
+
+ webhook, target := seedHTTPTarget(t, env, "", "")
+
+ form := url.Values{}
+ form.Set("name", "edited-name")
+
+ w := serveTarget(
+ env, http.MethodPost,
+ "/source/"+webhook.ID+"/targets/"+target.ID+
+ "/edit?url="+url.QueryEscape(editReplacedURL)+
+ "&headers="+url.QueryEscape(editAuthHeader),
+ form,
+ )
+
+ assert.Equal(t, http.StatusBadRequest, w.Code)
+
+ cfg := storedHTTPConfig(t, env, target.ID)
+ assert.Equal(t, editOriginalURL, cfg.URL)
+ assert.Empty(t, cfg.Headers)
+}
+
+func assertEditRejectsHeaders(
+ t *testing.T, env *sourceTestEnv, headers string,
+) {
+ t.Helper()
+
+ webhook, target := seedHTTPTarget(t, env, "", "")
+
+ w := submitTargetEdit(
+ env, webhook.ID, target.ID,
+ editForm(editOriginalURL, headers, ""),
+ )
+
+ assert.Equal(t, http.StatusBadRequest, w.Code)
+ assert.Contains(t, w.Body.String(), "Invalid headers")
+ assert.Empty(
+ t, storedHTTPConfig(t, env, target.ID).Headers,
+ "a rejected header must not be stored",
+ )
+}
+
+func assertEditRejectsTimeout(
+ t *testing.T, env *sourceTestEnv, timeout string,
+) {
+ t.Helper()
+
+ webhook, target := seedHTTPTarget(t, env, "", "9")
+
+ w := submitTargetEdit(
+ env, webhook.ID, target.ID,
+ editForm(editOriginalURL, "", timeout),
+ )
+
+ assert.Equal(t, http.StatusBadRequest, w.Code)
+ assert.Contains(t, w.Body.String(), "Invalid timeout")
+ assert.Equal(
+ t, 9, storedHTTPConfig(t, env, target.ID).Timeout,
+ "a rejected timeout must leave the stored one alone",
+ )
+}
+
+// TestHandleTargetEdit_Scoping keeps the edit routes scoped the way
+// the delete and toggle routes are: ownership is decided by the
+// webhook, and the target is then scoped to it.
+func TestHandleTargetEdit_Scoping(t *testing.T) {
+ t.Parallel()
+
+ env := setupSourceTest(t)
+
+ t.Run("a target of another webhook", func(t *testing.T) {
+ t.Parallel()
+ assertTargetOfAnotherWebhook404s(t, env)
+ })
+
+ t.Run("a webhook of another user", func(t *testing.T) {
+ t.Parallel()
+ assertWebhookOfAnotherUser404s(t, env)
+ })
+}
+
+// A target id from elsewhere must not become editable by pairing it
+// with a webhook the user does own.
+func assertTargetOfAnotherWebhook404s(
+ t *testing.T, env *sourceTestEnv,
+) {
+ t.Helper()
+
+ mine := seedWebhookWithRetention(t, env.db, 30)
+ _, target := seedHTTPTarget(t, env, "", "")
+
+ get := serveTarget(
+ env, http.MethodGet,
+ "/source/"+mine.ID+"/targets/"+target.ID+"/edit", nil,
+ )
+ assert.Equal(t, http.StatusNotFound, get.Code)
+
+ post := submitTargetEdit(
+ env, mine.ID, target.ID,
+ editForm(editReplacedURL, "", ""),
+ )
+ assert.Equal(t, http.StatusNotFound, post.Code)
+
+ assert.Equal(
+ t, editOriginalURL,
+ storedHTTPConfig(t, env, target.ID).URL,
+ )
+}
+
+func assertWebhookOfAnotherUser404s(
+ t *testing.T, env *sourceTestEnv,
+) {
+ t.Helper()
+
+ other := &database.Webhook{
+ UserID: "some-other-user",
+ Name: "not mine",
+ RetentionDays: 30,
+ }
+ require.NoError(
+ t,
+ env.db.DB().Omit(clause.Associations).Create(other).Error,
+ )
+
+ target := seedConfiguredTarget(
+ t, env.db, other.ID, database.TargetTypeHTTP,
+ `{"url":"`+editOriginalURL+`"}`,
+ )
+
+ w := serveTarget(
+ env, http.MethodGet,
+ "/source/"+other.ID+"/targets/"+target.ID+"/edit", nil,
+ )
+
+ assert.Equal(t, http.StatusNotFound, w.Code)
+}
diff --git a/internal/server/routes.go b/internal/server/routes.go
index 6f3d4c7..1d77b52 100644
--- a/internal/server/routes.go
+++ b/internal/server/routes.go
@@ -214,6 +214,20 @@ func (s *Server) setupSourceRoutes() {
s.h.HandleEntrypointToggle(),
)
r.Post("/targets", s.h.HandleTargetCreate())
+ // The edit form is the one page that renders a target's
+ // destination URL and header values in full; see
+ // delivery.TargetConfigForm. It belongs to this group for
+ // its RequireAuth and NoCache, which are what keep that
+ // exception from reaching an unauthenticated request or a
+ // shared cache.
+ r.Get(
+ "/targets/{targetID}/edit",
+ s.h.HandleTargetEdit(),
+ )
+ r.Post(
+ "/targets/{targetID}/edit",
+ s.h.HandleTargetEditSubmit(),
+ )
r.Post(
"/targets/{targetID}/delete",
s.h.HandleTargetDelete(),
diff --git a/templates/source_detail.html b/templates/source_detail.html
index 197ddcb..dc3177d 100644
--- a/templates/source_detail.html
+++ b/templates/source_detail.html
@@ -110,6 +110,14 @@
+
+
+
Optional request headers, one Name: value per line, sent with every delivery.