Gate forwarded-header trust behind trusted-proxy config (closes #88)

Every rate limiter keyed on httprate.KeyByRealIP, which believes
True-Client-IP, X-Real-IP and the first X-Forwarded-For entry from
any peer. A client could therefore mint a fresh bucket per request
by rotating a spoofed header, or drain another client's bucket by
claiming its address, which left the receiver, login and password
change limits with no value against a deliberate attacker.

The receiver, login and password change limiters now share one key
function: the connection's own address, unless the direct peer is
inside a network listed in the new TRUSTED_PROXIES CIDR list, in
which case the forwarded client address is used. The list is empty
by default, so nothing is trusted until an operator names their
proxy; a set-but-unparseable value aborts startup, matching the
handling of the other parsed variables. Within a trusted request
X-Forwarded-For is walked right to left and the first hop that is
not itself a trusted proxy wins, so client-prepended entries cannot
be selected.

Also folds in two cleanups from the same review: the 429 responder
shared by all three limiters is extracted, and the
RECEIVER_RATE_LIMIT error-path tests now assert that the failure
names the variable and wraps ErrNonPositiveValue rather than only
that some error occurred.

README.md

@@ -102,43 +102,21 @@ TTY detection, and security headers are always applied.
 `TRUSTED_PROXIES` is a comma-separated list of CIDR blocks (a bare
 address such as `10.0.0.1` is accepted and treated as a single host),
 for example `10.0.0.0/8, 192.168.1.7, 2001:db8::/32`. It decides whose
-`X-Forwarded-For` header the rate limiters believe.
+`X-Forwarded-For`, `X-Real-IP`, and `True-Client-IP` headers the rate
+limiters believe.
 
-`X-Forwarded-For` is honoured **only** when the connecting peer is
+Forwarded headers are honoured **only** when the connecting peer is
 inside one of these blocks; for every other peer the client identity is
-the connection's own address and the header is ignored. The default is
+the connection's own address and the headers are ignored. The default
-the empty list, which trusts nobody — anything else would let any
+is the empty list, which trusts nobody — anything else would let any
 client pick its own rate limit bucket, minting a fresh one per request
 or draining someone else's. Set it to the address of your reverse
-proxy, and to nothing wider. A set but unparseable value aborts
+proxy, and to nothing wider.
-startup.
 
-`X-Real-IP` and `True-Client-IP` are **never** read, from any peer.
+Within a trusted request, `X-Forwarded-For` is read right to left and
-Reverse proxies append to `X-Forwarded-For` but forward other client
+the first hop that is not itself a trusted proxy wins, so entries a
-headers verbatim, so a single-valued header is client-controlled even
+client prepended before reaching the proxy cannot be selected. A set
-behind a trusted proxy.
+but unparseable value aborts startup.
-
-Within a trusted request, `X-Forwarded-For` is read right to left,
-because the rightmost entry is the one the nearest proxy appended and
-everything left of it may have been written by the client. The first
-hop that is not itself a trusted proxy is taken as the client. A hop
-that is not a bare IP address — `ip:port`, a bracketed IPv6 literal,
-the token `unknown` — ends the walk and the peer address is used
-instead, since past such an entry the chain is not the shape assumed
-here.
-
-Two operator requirements follow:
-
-- Your proxy must **append** the peer address to `X-Forwarded-For`
-  (nginx `$proxy_add_x_forwarded_for`, HAProxy `option forwardfor`,
-  Caddy and AWS ALB by default), and must append a bare address with
-  no port.
-- Keep the list narrow. A client whose own address falls inside a
-  broad block such as `10.0.0.0/8` is treated as a proxy: its address
-  is skipped during the walk, so it shares the bucket of whatever lies
-  further left rather than getting one of its own. That is safe — the
-  block is trusted by definition — but surprising if the block covers
-  ordinary clients as well as proxies.
 
 Sessions are bounded by two independent clocks, and end at whichever
 one runs out first:

internal/config/config.go

@@ -47,11 +47,6 @@ const (
 	// maxPort is the highest valid TCP port number. The lower
 	// bound (at least 1) is enforced by envPositiveInt.
 	maxPort = 65535
-
-	// mappedV4Offset is the number of leading bits an IPv4-mapped
-	// IPv6 prefix spends on the ::ffff:0:0/96 wrapper, so a /104
-	// covers the same addresses as an IPv4 /8.
-	mappedV4Offset = 96
 )
 
 // ErrInvalidEnvironment is returned when WEBHOOKER_ENVIRONMENT
@@ -235,10 +230,6 @@ func envDuration(
 // parseCIDR parses one trusted-proxy list entry, which may be a
 // CIDR block ("10.0.0.0/8") or a bare address ("10.0.0.1", treated
 // as a single-host block).
-//
-// Both forms are unmapped, because peer addresses are unmapped
-// before they are matched against the list: an IPv4-mapped prefix
-// left in that form would silently never match.
 func parseCIDR(entry string) (netip.Prefix, error) {
 	if strings.Contains(entry, "/") {
 		prefix, err := netip.ParsePrefix(entry)
@@ -246,13 +237,6 @@ func parseCIDR(entry string) (netip.Prefix, error) {
 			return netip.Prefix{}, err //nolint:wrapcheck // wrapped by caller
 		}
 
-		if addr := prefix.Addr(); addr.Is4In6() &&
-			prefix.Bits() >= mappedV4Offset {
-			prefix = netip.PrefixFrom(
-				addr.Unmap(), prefix.Bits()-mappedV4Offset,
-			)
-		}
-
 		return prefix.Masked(), nil
 	}
 

internal/config/config_test.go

@@ -20,10 +20,6 @@ const (
 	caseUnsetUsesDefault = "unset uses default"
 	caseValidValueParsed = "valid value is parsed"
 	caseUnparseableFails = "unparseable value fails startup"
-
-	// cidrPrivateV4 is the sample trusted-proxy block the
-	// TRUSTED_PROXIES cases are built from.
-	cidrPrivateV4 = "10.0.0.0/8"
 )
 
 func TestEnvironmentConfig(t *testing.T) {
@@ -497,30 +493,21 @@ func TestTrustedProxies(t *testing.T) {
 		{
 			name:  caseValidValueParsed,
 			set:   true,
-			value: cidrPrivateV4 + ", 192.168.1.7 ,2001:db8::/32",
+			value: "10.0.0.0/8, 192.168.1.7 ,2001:db8::/32",
 			expected: []string{
-				cidrPrivateV4, "192.168.1.7/32", "2001:db8::/32",
+				"10.0.0.0/8", "192.168.1.7/32", "2001:db8::/32",
 			},
 		},
 		{
 			name:     "host bits are masked off",
 			set:      true,
 			value:    "10.1.2.3/8",
-			expected: []string{cidrPrivateV4},
+			expected: []string{"10.0.0.0/8"},
-		},
-		{
-			// Peer addresses are unmapped before they are
-			// matched, so an IPv4-mapped prefix kept in that
-			// form could never match anything.
-			name:     "IPv4-mapped prefix is unmapped",
-			set:      true,
-			value:    "::ffff:10.0.0.0/104",
-			expected: []string{cidrPrivateV4},
 		},
 		{
 			name:        caseUnparseableFails,
 			set:         true,
-			value:       cidrPrivateV4 + ",not-an-address",
+			value:       "10.0.0.0/8,not-an-address",
 			expectError: true,
 		},
 		{

internal/delivery/ssrf.go

@@ -92,12 +92,7 @@ func ValidateTargetURL(
 ) error {
 	parsed, err := url.Parse(targetURL)
 	if err != nil {
-		// url.Parse embeds the whole URL in its error, and
+		return fmt.Errorf("invalid URL: %w", err)
-		// this one is logged and shown; mask it. Every other
-		// branch below reports only the hostname.
-		return fmt.Errorf(
-			"invalid URL: %w", maskURLError(err),
-		)
 	}
 
 	err = validateScheme(parsed.Scheme)

internal/delivery/target_config_view.go

@@ -3,6 +3,7 @@ package delivery
 import (
 	"encoding/json"
 	"fmt"
+	"net/url"
 	"strconv"
 
 	"sneak.berlin/go/webhooker/internal/database"
@@ -16,6 +17,9 @@ import (
 // browser history, screenshots and screen shares.
 const configUnavailable = "(unavailable)"
 
+// urlPathElision stands in for a URL's elided path.
+const urlPathElision = "/..."
+
 // ConfigField is one labelled, display-safe value derived
 // from a target's stored configuration.
 type ConfigField struct {
@@ -198,5 +202,23 @@ func databaseConfigFields(configJSON string) []ConfigField {
 // parse into a scheme and host yields the neutral
 // placeholder, never the raw string.
 func (c *SlackTargetConfig) MaskedWebhookURL() string {
-	return MaskURL(c.WebhookURL)
+	return maskURL(c.WebhookURL)
+}
+
+// maskURL renders a URL as scheme plus host with everything
+// that can carry a secret removed.
+func maskURL(raw string) string {
+	parsed, err := url.Parse(raw)
+	if err != nil || parsed.Scheme == "" ||
+		parsed.Host == "" {
+		return configUnavailable
+	}
+
+	masked := parsed.Scheme + "://" + parsed.Host
+
+	if parsed.Path != "" && parsed.Path != "/" {
+		masked += urlPathElision
+	}
+
+	return masked
 }

internal/delivery/target_http.go

@@ -363,8 +363,7 @@ func (t *httpTarget) doHTTPRequest(
 	)
 	if reqErr != nil {
 		return 0, "", 0, fmt.Errorf(
-			"creating request: %w",
+			"creating request: %w", reqErr,
-			maskURLError(reqErr),
 		)
 	}
 
@@ -493,19 +492,8 @@ func applyRequestHeaders(
 // executeHTTPRequest sends an HTTP request using the provided
 // client. URLs are validated by the config parsers and the
 // SSRF-safe transport before reaching here.
-//
-// Transport failures are masked here, at the single point
-// where every target's request errors are born, because the
-// caller stores them in DeliveryResult.Error: an unmasked
-// *url.Error would write the target URL — the credential for
-// a Slack incoming webhook — into the per-webhook database.
 func executeHTTPRequest(
 	client *http.Client, req *http.Request,
 ) (*http.Response, error) {
-	resp, err := client.Do(req) //#nosec G704 -- validated URL, SSRF-safe transport
+	return client.Do(req) //#nosec G704 -- validated URL, SSRF-safe transport
-	if err != nil {
-		return nil, maskURLError(err)
-	}
-
-	return resp, nil
 }

internal/delivery/target_slack.go

@@ -125,7 +125,7 @@ func (t *slackTarget) attempt(
 	if err != nil {
 		return attemptResult{
 			success: false,
-			errMsg:  maskURLError(err).Error(),
+			errMsg:  err.Error(),
 		}
 	}
 

internal/delivery/url_mask.go

@@ -1,61 +0,0 @@
-package delivery
-
-import (
-	"errors"
-	"net/url"
-)
-
-// urlPathElision stands in for a URL's elided path.
-const urlPathElision = "/..."
-
-// MaskURL renders a URL as scheme plus host with everything
-// that can carry a secret removed. A delivery target URL is
-// itself a credential — a Slack incoming webhook URL is a
-// bearer token — so the path, query and userinfo are never
-// reproduced, in a page, a log line or a stored error. A URL
-// that does not parse into a scheme and host yields the
-// neutral placeholder, never the raw string.
-func MaskURL(raw string) string {
-	parsed, err := url.Parse(raw)
-	if err != nil || parsed.Scheme == "" ||
-		parsed.Host == "" {
-		return configUnavailable
-	}
-
-	masked := parsed.Scheme + "://" + parsed.Host
-
-	if parsed.Path != "" && parsed.Path != "/" {
-		masked += urlPathElision
-	}
-
-	return masked
-}
-
-// maskURLError strips the credential from an error raised
-// against a request URL. The net/http and net/url packages
-// embed the full request URL in every *url.Error they return,
-// so an unmodified transport error persisted into
-// DeliveryResult.Error writes the credential to disk.
-//
-// The masked error keeps the operation and the wrapped cause,
-// so a DNS failure still reads differently from a refused
-// connection, a TLS handshake failure or a timeout, and Is,
-// As, Timeout and Temporary keep working on it. Only the
-// path, query and userinfo of the URL are dropped. Errors
-// that carry no URL are returned unchanged.
-//
-// Call it where the error is raised, before any wrapping: it
-// replaces the *url.Error itself, so any context wrapped
-// around it first would be discarded.
-func maskURLError(err error) error {
-	var urlErr *url.Error
-	if !errors.As(err, &urlErr) {
-		return err
-	}
-
-	return &url.Error{
-		Op:  urlErr.Op,
-		URL: MaskURL(urlErr.URL),
-		Err: urlErr.Err,
-	}
-}

internal/delivery/url_mask_test.go

@@ -1,196 +0,0 @@
-package delivery_test
-
-import (
-	"context"
-	"encoding/json"
-	"net/http"
-	"net/http/httptest"
-	"testing"
-
-	"github.com/google/uuid"
-	"github.com/stretchr/testify/assert"
-	"github.com/stretchr/testify/require"
-	"gorm.io/gorm"
-	"sneak.berlin/go/webhooker/internal/database"
-	"sneak.berlin/go/webhooker/internal/delivery"
-)
-
-// The path of a Slack incoming webhook URL is the credential:
-// whoever holds these segments can post to the channel
-// forever. None of them may reach a stored delivery error,
-// which lives on disk in the per-webhook database and is
-// serialized by the JSON tag on DeliveryResult.Error.
-const (
-	maskSecretPath = "/services/T00000000/B00000000/" +
-		"XXXXXXXXXXXXXXXXXXXXXXXX"
-)
-
-// assertNoCredential fails if the whole path or any single
-// segment of it survived into the message, so a partial leak
-// fails the test too.
-func assertNoCredential(t *testing.T, msg string) {
-	t.Helper()
-
-	segments := []string{
-		maskSecretPath,
-		"services",
-		"T00000000",
-		"B00000000",
-		"XXXXXXXXXXXXXXXXXXXXXXXX",
-	}
-
-	for _, segment := range segments {
-		assert.NotContains(t, msg, segment)
-	}
-}
-
-// storedDeliveryError returns the error string persisted for a
-// delivery, which is what an operator and any future API read.
-func storedDeliveryError(
-	t *testing.T, db *gorm.DB, deliveryID string,
-) string {
-	t.Helper()
-
-	var result database.DeliveryResult
-
-	require.NoError(t, db.Where(
-		"delivery_id = ?", deliveryID,
-	).First(&result).Error)
-
-	return result.Error
-}
-
-// deliverSlackTo runs a Slack delivery against webhookURL and
-// returns the error string it persisted.
-func deliverSlackTo(
-	t *testing.T, webhookURL string,
-) string {
-	t.Helper()
-
-	db := testWebhookDB(t)
-	e := testEngine(t, 1)
-	targetID := uuid.New().String()
-
-	slackCfg, err := json.Marshal(
-		delivery.SlackTargetConfig{
-			WebhookURL: webhookURL,
-		},
-	)
-	require.NoError(t, err)
-
-	event := seedEvent(t, db, `{"test":true}`)
-
-	dlv := seedDelivery(
-		t, db, event.ID, targetID,
-		database.DeliveryStatusPending,
-	)
-
-	d := buildSlackDelivery(
-		dlv, event, targetID,
-		"test-slack-mask", string(slackCfg),
-	)
-
-	e.ExportDeliverSlack(context.TODO(), db, d)
-
-	assertDeliveryStatus(t, db, dlv.ID,
-		database.DeliveryStatusFailed,
-	)
-
-	return storedDeliveryError(t, db, dlv.ID)
-}
-
-// TestDeliverSlack_TransportErrorMasksWebhookURL is the
-// load-bearing regression test: a transport failure must not
-// persist the webhook URL's credential into the database, and
-// must still say what went wrong and where.
-func TestDeliverSlack_TransportErrorMasksWebhookURL(
-	t *testing.T,
-) {
-	t.Parallel()
-
-	// A server closed before use gives a deterministic
-	// transport failure against a known host.
-	ts := httptest.NewServer(http.NewServeMux())
-	host := ts.URL
-
-	ts.Close()
-
-	errMsg := deliverSlackTo(t, host+maskSecretPath)
-
-	require.NotEmpty(t, errMsg)
-	assertNoCredential(t, errMsg)
-
-	// The diagnostic value survives: the operation, the host
-	// and the transport failure are all still reported, and
-	// only the path is elided.
-	assert.Contains(t, errMsg, "sending request")
-	assert.Contains(t, errMsg, "Post")
-	assert.Contains(t, errMsg, host+"/...")
-	assert.Contains(t, errMsg, "connection refused")
-}
-
-// TestDeliverSlack_UnparsableURLMasksWebhookURL covers the
-// other error path out of a Slack attempt: url.Parse also
-// embeds the whole URL in the error it returns.
-func TestDeliverSlack_UnparsableURLMasksWebhookURL(
-	t *testing.T,
-) {
-	t.Parallel()
-
-	errMsg := deliverSlackTo(
-		t,
-		"https://hooks.slack.com"+maskSecretPath+"\n",
-	)
-
-	require.NotEmpty(t, errMsg)
-	assertNoCredential(t, errMsg)
-	assert.Contains(t, errMsg, "invalid control character")
-}
-
-// TestDoHTTPRequest_TransportErrorMasksURL proves the HTTP
-// target's transport errors are masked too; its destination
-// URL can carry a token in a query string.
-func TestDoHTTPRequest_TransportErrorMasksURL(t *testing.T) {
-	t.Parallel()
-
-	ts := httptest.NewServer(http.NewServeMux())
-	host := ts.URL
-
-	ts.Close()
-
-	e := testEngine(t, 1)
-
-	cfg, err := e.ExportParseHTTPConfig(
-		newHTTPTargetConfig(host + maskSecretPath),
-	)
-	require.NoError(t, err)
-
-	statusCode, _, _, reqErr := e.ExportDoHTTPRequest(
-		context.TODO(), cfg,
-		&database.Event{Body: `{"test":true}`},
-	)
-	require.Error(t, reqErr)
-	assert.Zero(t, statusCode)
-
-	assertNoCredential(t, reqErr.Error())
-	assert.Contains(t, reqErr.Error(), host+"/...")
-	assert.Contains(
-		t, reqErr.Error(), "connection refused",
-	)
-}
-
-// TestValidateTargetURL_UnparsableURLIsMasked proves the SSRF
-// validator's error does not carry the submitted URL, which
-// the handler both logs and shows.
-func TestValidateTargetURL_UnparsableURLIsMasked(t *testing.T) {
-	t.Parallel()
-
-	err := delivery.ValidateTargetURL(
-		context.TODO(),
-		"https://hooks.slack.com"+maskSecretPath+"\n",
-	)
-	require.Error(t, err)
-
-	assertNoCredential(t, err.Error())
-	assert.Contains(t, err.Error(), "invalid URL")
-}

internal/handlers/source_detail_test.go

@@ -26,14 +26,14 @@ const (
 )
 
 // seedConfiguredTarget inserts a target with a stored config
-// blob and returns it.
+// blob.
 func seedConfiguredTarget(
 	t *testing.T,
 	db *database.Database,
 	webhookID string,
 	targetType database.TargetType,
 	config string,
-) *database.Target {
+) {
 	t.Helper()
 
 	tgt := &database.Target{
@@ -48,8 +48,6 @@ func seedConfiguredTarget(
 		t,
 		db.DB().Omit(clause.Associations).Create(tgt).Error,
 	)
-
-	return tgt
 }
 
 // renderSourceDetailPage runs the real source detail handler

internal/handlers/source_logs_test.go

@@ -1,134 +0,0 @@
-package handlers_test
-
-import (
-	"context"
-	"net/http"
-	"net/http/httptest"
-	"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/handlers"
-	"sneak.berlin/go/webhooker/internal/session"
-)
-
-// seedDeliveredEvent records an event and a delivery for it in
-// the webhook's own database, so the log page has a delivery
-// to render against the target.
-func seedDeliveredEvent(
-	t *testing.T,
-	dbMgr *database.WebhookDBManager,
-	webhookID, targetID string,
-) {
-	t.Helper()
-
-	webhookDB, err := dbMgr.GetDB(webhookID)
-	require.NoError(t, err)
-
-	event := &database.Event{
-		WebhookID:   webhookID,
-		Method:      http.MethodPost,
-		Body:        `{"test":true}`,
-		ContentType: "application/json",
-	}
-
-	require.NoError(t, webhookDB.Omit(
-		clause.Associations,
-	).Create(event).Error)
-
-	dlv := &database.Delivery{
-		EventID:  event.ID,
-		TargetID: targetID,
-		Status:   database.DeliveryStatusDelivered,
-	}
-
-	require.NoError(t, webhookDB.Omit(
-		clause.Associations,
-	).Create(dlv).Error)
-}
-
-// renderSourceLogsPage runs the real event log handler for a
-// webhook and returns the rendered HTML.
-func renderSourceLogsPage(
-	t *testing.T,
-	h *handlers.Handlers,
-	sess *session.Session,
-	webhookID string,
-) string {
-	t.Helper()
-
-	req := httptest.NewRequestWithContext(
-		context.Background(),
-		http.MethodGet,
-		"/source/"+webhookID+"/logs",
-		nil,
-	)
-
-	for _, c := range authenticatedCookies(
-		t, sess, deleteTestUserID, deleteTestUsername,
-	) {
-		req.AddCookie(c)
-	}
-
-	rctx := chi.NewRouteContext()
-	rctx.URLParams.Add(paramSourceID, webhookID)
-
-	req = req.WithContext(
-		context.WithValue(
-			req.Context(), chi.RouteCtxKey, rctx,
-		),
-	)
-
-	w := httptest.NewRecorder()
-	h.HandleSourceLogs().ServeHTTP(w, req)
-
-	require.Equal(t, http.StatusOK, w.Code)
-
-	return w.Body.String()
-}
-
-// TestHandleSourceLogs_MasksSlackWebhookURL proves the event
-// log page is handed a display-safe projection of each target
-// rather than the stored row, so the credential cannot be
-// rendered from its template data.
-func TestHandleSourceLogs_MasksSlackWebhookURL(t *testing.T) {
-	t.Parallel()
-
-	var (
-		h     *handlers.Handlers
-		sess  *session.Session
-		db    *database.Database
-		dbMgr *database.WebhookDBManager
-	)
-
-	app := newTestApp(t, &h, &sess, &db, &dbMgr)
-	app.RequireStart()
-
-	t.Cleanup(app.RequireStop)
-
-	wh := seedWebhook(t, db)
-	tgt := seedConfiguredTarget(
-		t, db, wh.ID,
-		database.TargetTypeSlack,
-		`{"webhookUrl":"`+slackWebhookURL+`"}`,
-	)
-
-	seedDeliveredEvent(t, dbMgr, wh.ID, tgt.ID)
-
-	body := renderSourceLogsPage(t, h, sess, wh.ID)
-
-	assert.NotContains(t, body, slackSecretPath)
-	assert.NotContains(t, body, "T00000000")
-	assert.NotContains(t, body, "B00000000")
-	assert.NotContains(
-		t, body, "XXXXXXXXXXXXXXXXXXXXXXXX",
-	)
-	assert.NotContains(t, body, "webhookUrl")
-
-	// The page still identifies the delivery's target.
-	assert.Contains(t, body, tgt.Name)
-	assert.Contains(t, body, "delivered")
-}

internal/handlers/source_management.go

@@ -96,17 +96,7 @@ func parseRetentionDays(raw string, fallback int) (int, error) {
 type EventWithDeliveries struct {
 	database.Event
 
-	Deliveries []DeliveryView
+	Deliveries []database.Delivery
-}
-
-// DeliveryView is the display-safe projection of a delivery
-// for the event log page. Its target is a TargetView, so the
-// stored configuration blob — which holds the target's
-// credential — has no path to the template.
-type DeliveryView struct {
-	ID     string
-	Status database.DeliveryStatus
-	Target delivery.TargetView
 }
 
 // HandleSourceList shows a list of user's webhooks.
@@ -774,27 +764,22 @@ func (h *Handlers) HandleSourceLogs() http.HandlerFunc {
 	}
 }
 
-// loadTargetMap loads targets into a map of display-safe
+// loadTargetMap loads targets into a map keyed by target ID.
-// views keyed by target ID. The projection happens here so
-// that no caller can hand a raw target, configuration blob
-// and all, to a template.
 func (h *Handlers) loadTargetMap(
 	webhookID string,
-) map[string]delivery.TargetView {
+) map[string]database.Target {
 	var targets []database.Target
 
 	h.db.DB().Where(
 		"webhook_id = ?", webhookID,
 	).Find(&targets)
 
-	views := delivery.NewTargetViews(targets)
-
 	targetMap := make(
-		map[string]delivery.TargetView, len(views),
+		map[string]database.Target, len(targets),
 	)
 
-	for _, v := range views {
+	for _, t := range targets {
-		targetMap[v.ID] = v
+		targetMap[t.ID] = t
 	}
 
 	return targetMap
@@ -819,7 +804,7 @@ func (h *Handlers) parsePage(r *http.Request) int {
 func (h *Handlers) loadEventsWithDeliveries(
 	w http.ResponseWriter,
 	webhook database.Webhook,
-	targetMap map[string]delivery.TargetView,
+	targetMap map[string]database.Target,
 	page int,
 ) ([]EventWithDeliveries, int64) {
 	var totalEvents int64
@@ -858,39 +843,22 @@ func (h *Handlers) loadEventsWithDeliveries(
 	for i := range events {
 		result[i].Event = events[i]
 
-		var deliveries []database.Delivery
-
 		webhookDB.Where(
 			"event_id = ?", events[i].ID,
-		).Find(&deliveries)
+		).Find(&result[i].Deliveries)
 
-		result[i].Deliveries = newDeliveryViews(
+		for j := range result[i].Deliveries {
-			deliveries, targetMap,
+			tid := result[i].Deliveries[j].TargetID
-		)
+
+			if target, ok := targetMap[tid]; ok {
+				result[i].Deliveries[j].Target = target
+			}
+		}
 	}
 
 	return result, totalEvents
 }
 
-// newDeliveryViews projects deliveries for rendering,
-// resolving each one's target to its display-safe view.
-func newDeliveryViews(
-	deliveries []database.Delivery,
-	targetMap map[string]delivery.TargetView,
-) []DeliveryView {
-	views := make([]DeliveryView, len(deliveries))
-
-	for i := range deliveries {
-		views[i] = DeliveryView{
-			ID:     deliveries[i].ID,
-			Status: deliveries[i].Status,
-			Target: targetMap[deliveries[i].TargetID],
-		}
-	}
-
-	return views
-}
-
 // HandleEntrypointCreate handles adding a new entrypoint.
 func (h *Handlers) HandleEntrypointCreate() http.HandlerFunc {
 	return func(w http.ResponseWriter, r *http.Request) {
@@ -1134,12 +1102,9 @@ func (h *Handlers) buildURLTargetConfig(
 		r.Context(), targetURL,
 	)
 	if err != nil {
-		// The submitted URL can be a credential (a Slack
-		// incoming webhook URL is a bearer token), so the log
-		// records only its scheme and host.
 		h.log.Warn(
 			"target URL blocked by SSRF protection",
-			"url", delivery.MaskURL(targetURL),
+			"url", targetURL,
 			"error", err,
 		)
 		http.Error(

internal/middleware/ratelimit.go

@@ -54,38 +54,36 @@ func (m *Middleware) isTrustedProxy(addr netip.Addr) bool {
 }
 
 // forwardedClientAddr returns the client address named by this
-// request's X-Forwarded-For chain. It is consulted only for requests
+// request's forwarded headers. It is consulted only for requests
 // whose direct peer is a trusted proxy.
 //
-// X-Forwarded-For is the only header read. X-Real-IP and
+// True-Client-IP and X-Real-IP are single-valued, and a trusted
-// True-Client-IP are deliberately ignored: the reverse proxies in
+// proxy is expected to overwrite whatever the client sent, so they
-// common use append to X-Forwarded-For and pass any other header the
+// are taken as given. X-Forwarded-For is a chain the client can
-// client sent through untouched, so believing a single-valued header
+// prepend to, so it is walked right to left and the first hop that
-// would let a client behind the trusted proxy name its own bucket —
+// is not itself a trusted proxy wins: entries the client inserted
-// the very bypass this gating exists to close.
+// sit to the left of the proxies' own appends and cannot be picked
-//
+// while the chain is intact.
-// The chain is walked right to left, because the rightmost entry is
-// the one the nearest proxy appended and everything to its left may
-// have been written by the client. The first hop that is not itself
-// a trusted proxy is the client. A hop that cannot be read as a bare
-// address ends the walk: past it the chain is not the shape assumed
-// here, so the caller falls back to the peer address.
 func (m *Middleware) forwardedClientAddr(
 	r *http.Request,
 ) (netip.Addr, bool) {
+	for _, header := range []string{"True-Client-IP", "X-Real-IP"} {
+		addr, err := netip.ParseAddr(
+			strings.TrimSpace(r.Header.Get(header)),
+		)
+		if err == nil {
+			return normalizeAddr(addr), true
+		}
+	}
+
 	hops := strings.Split(
 		strings.Join(r.Header.Values("X-Forwarded-For"), ","), ",",
 	)
 
 	for _, hop := range slices.Backward(hops) {
-		hop = strings.TrimSpace(hop)
+		addr, err := netip.ParseAddr(strings.TrimSpace(hop))
-		if hop == "" {
-			continue
-		}
-
-		addr, err := netip.ParseAddr(hop)
 		if err != nil {
-			return netip.Addr{}, false
+			continue
 		}
 
 		if addr = normalizeAddr(addr); !m.isTrustedProxy(addr) {

internal/middleware/ratelimit_test.go

@@ -368,41 +368,6 @@ const (
 	headerTrue = "True-Client-IP"
 )
 
-// assertSharedBucket drives the login limiter from peer with the
-// trusted-proxy set proxies, sending one more request than the limit
-// allows and varying the headers on each with headers(i). Every
-// request must land in the same bucket, so the last one is rejected:
-// if any of the varying header values reached the key, the run would
-// have minted fresh buckets and nothing would be rejected.
-func assertSharedBucket(
-	t *testing.T,
-	proxies []netip.Prefix,
-	peer string,
-	headers func(i int) map[string]string,
-	msg string,
-) {
-	t.Helper()
-
-	m := rateLimitMiddleware(
-		t, &config.Config{TrustedProxies: proxies},
-	)
-	handler := m.LoginRateLimit()(okHandler())
-
-	for i := range middleware.LoginRateLimitConst {
-		w := postWithHeaders(handler, peer, loginPath, headers(i))
-		assert.Equal(
-			t, http.StatusOK, w.Code,
-			"request %d should pass", i,
-		)
-	}
-
-	w := postWithHeaders(
-		handler, peer, loginPath,
-		headers(middleware.LoginRateLimitConst),
-	)
-	assert.Equal(t, http.StatusTooManyRequests, w.Code, msg)
-}
-
 // TestRateLimitKey_SpoofedForwardedFromUntrustedPeer is the test
 // this gating exists for: with no trusted proxies configured (the
 // default), a client that rotates a forwarded header on every
@@ -420,89 +385,34 @@ func TestRateLimitKey_SpoofedForwardedFromUntrustedPeer(
 		t.Run(header, func(t *testing.T) {
 			t.Parallel()
 
-			assertSharedBucket(
+			m := rateLimitMiddleware(t, &config.Config{})
-				t, nil, "203.0.113.9:44444",
+			handler := m.LoginRateLimit()(okHandler())
-				func(i int) map[string]string {
+
-					return map[string]string{
+			const peer = "203.0.113.9:44444"
+
+			for i := range middleware.LoginRateLimitConst {
+				w := postWithHeaders(
+					handler, peer, loginPath,
+					map[string]string{
 						header: fmt.Sprintf(
 							"198.51.100.%d", i+1,
 						),
-					}
 					},
-				"a spoofed "+header+" from an untrusted peer "+
-					"must not mint a fresh bucket",
 				)
-		})
+				assert.Equal(
-	}
+					t, http.StatusOK, w.Code,
-}
+					"request %d should pass", i,
-
-// TestRateLimitKey_SingleValuedHeadersIgnoredFromTrustedPeer is the
-// regression test for the bypass hiding inside the trusted case.
-// Real reverse proxies (nginx, HAProxy, Caddy, ALB) set only
-// X-Forwarded-For and pass every other client header through
-// verbatim, so a client behind the configured proxy can send its own
-// X-Real-IP or True-Client-IP. Reading either would hand that client
-// a fresh bucket per request from inside exactly the deployment
-// TRUSTED_PROXIES exists to serve, so neither header is read at all.
-func TestRateLimitKey_SingleValuedHeadersIgnoredFromTrustedPeer(
-	t *testing.T,
-) {
-	t.Parallel()
-
-	for _, header := range []string{headerReal, headerTrue} {
-		t.Run(header, func(t *testing.T) {
-			t.Parallel()
-
-			assertSharedBucket(
-				t, trustedProxies("10.0.0.0/8"),
-				"10.0.0.1:44444",
-				func(i int) map[string]string {
-					return map[string]string{
-						header: fmt.Sprintf(
-							"198.51.100.%d", i+1,
-						),
-					}
-				},
-				header+" from a trusted peer must not mint a "+
-					"fresh bucket: only X-Forwarded-For is read",
 				)
-		})
-	}
 			}
 
-// TestRateLimitKey_MalformedRightmostHopFallsBackToPeer covers the
+			w := postWithHeaders(
-// other end of the chain walk. The rightmost X-Forwarded-For entry
+				handler, peer, loginPath,
-// is the one the trusted proxy appended; if it cannot be read as an
+				map[string]string{header: "198.51.100.200"},
-// address the chain is not the shape the walk assumes, and every
+			)
-// entry to its left may have come from the client. The walk must
+			assert.Equal(
-// stop and fall back to the peer rather than select one of them.
+				t, http.StatusTooManyRequests, w.Code,
-func TestRateLimitKey_MalformedRightmostHopFallsBackToPeer(
+				"a spoofed %s from an untrusted peer must "+
-	t *testing.T,
+					"not mint a fresh bucket", header,
-) {
-	t.Parallel()
-
-	// Forms seen in the wild: host:port (Azure Application
-	// Gateway, IIS ARR), a bracketed IPv6 literal, and the
-	// RFC 7239 placeholder token.
-	for _, tail := range []string{
-		"198.51.100.7:1234", "[2001:db8::1]", "unknown",
-	} {
-		t.Run(tail, func(t *testing.T) {
-			t.Parallel()
-
-			assertSharedBucket(
-				t, trustedProxies("10.0.0.0/8"),
-				"10.0.0.1:44444",
-				func(i int) map[string]string {
-					return map[string]string{
-						headerXFF: fmt.Sprintf(
-							"9.9.9.%d, %s", i+1, tail,
-						),
-					}
-				},
-				"an unparseable rightmost hop must fall back "+
-					"to the peer address, not select a "+
-					"client-controlled entry",
 			)
 		})
 	}
@@ -554,15 +464,35 @@ func TestRateLimitKey_ForwardedHonouredFromTrustedPeer(
 func TestRateLimitKey_ChainWalkSkipsClientPrepended(t *testing.T) {
 	t.Parallel()
 
-	assertSharedBucket(
+	m := rateLimitMiddleware(t, &config.Config{
-		t, trustedProxies("10.0.0.0/8"), "10.0.0.1:44444",
+		TrustedProxies: trustedProxies("10.0.0.0/8"),
-		func(i int) map[string]string {
+	})
+	handler := m.LoginRateLimit()(okHandler())
+
+	const peer = "10.0.0.1:44444"
+
+	chain := func(spoof string) map[string]string {
 		return map[string]string{
-				headerXFF: fmt.Sprintf(
+			headerXFF: spoof + ", 198.51.100.7, 10.0.0.2",
-					"9.9.9.%d, 198.51.100.7, 10.0.0.2", i+1,
-				),
 		}
-		},
+	}
+
+	for i := range middleware.LoginRateLimitConst {
+		w := postWithHeaders(
+			handler, peer, loginPath,
+			chain(fmt.Sprintf("9.9.9.%d", i+1)),
+		)
+		assert.Equal(
+			t, http.StatusOK, w.Code,
+			"request %d should pass", i,
+		)
+	}
+
+	w := postWithHeaders(
+		handler, peer, loginPath, chain("9.9.9.200"),
+	)
+	assert.Equal(
+		t, http.StatusTooManyRequests, w.Code,
 		"a client-prepended X-Forwarded-For entry must not "+
 			"mint a fresh bucket",
 	)