Warn when TRUSTED_PROXIES is empty in production (closes #149)

With no trusted proxies configured, every client behind the reverse proxy production requires shares one rate-limit bucket per limit, so five POSTs per minute from anywhere holds the login limit full and denies the admin login until restart. The default is still correct; it was the consequence that was invisible. Startup now warns, and the README no longer claims the login limit is per-IP unconditionally.

.dockerignore

@@ -1,3 +1,6 @@
+# .ci-fingerprint is deliberately NOT excluded: it is the CI cache barrier
+# that keeps the check stages from replaying a cached pass. See the lint
+# stage of the Dockerfile.
 .git/
 bin/
 *.md

.gitea/workflows/check.yml

@@ -11,5 +11,53 @@ jobs:
     steps:
       - name: Checkout
         uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2 2024-10-23
+        with:
+          # The fingerprint step below needs history to find the last commit
+          # that touched the Docker build context.
+          fetch-depth: 0
+
+      - name: Neutralize superseded run statuses
+        # Gitea cancels the in-flight run when another commit is pushed to the
+        # same branch and records the cancellation as `failure`, so a commit
+        # that was never tested reads red. The cancellation is unconditional
+        # server-side for push events and cannot be disabled from a workflow
+        # file, so the superseding run rewrites those statuses to `skipped`.
+        # Only the exact cancellation status is touched; a real failure is
+        # left alone.
+        env:
+          GITEA_TOKEN: ${{ secrets.GITEA_TOKEN }}
+        run: |
+          set -eu
+          api="${GITHUB_API_URL}/repos/${GITHUB_REPOSITORY}"
+          ctx='check / check (push)'
+          for sha in $(git rev-list --max-count=20 "${GITHUB_SHA}^" || true); do
+            latest="$(curl -sf "${api}/commits/${sha}/status" | jq -r \
+              --arg c "$ctx" \
+              '[.statuses[] | select(.context == $c)][0] // empty
+               | "\(.status)|\(.description)"')" || continue
+            [ "$latest" = 'failure|Has been cancelled' ] || continue
+            curl -sf -X POST "${api}/statuses/${sha}" \
+              -H "Authorization: token ${GITEA_TOKEN}" \
+              -H 'Content-Type: application/json' \
+              -d "$(jq -nc --arg c "$ctx" '{
+                context: $c,
+                state: "skipped",
+                description: "Superseded by a newer commit; never tested"
+              }')" >/dev/null
+            echo "neutralized superseded status on ${sha}"
+          done
+
+      - name: Fingerprint the build context
+        # `.dockerignore` keeps docs out of the build context, so a docs-only
+        # commit legitimately replays the whole image from cache and stays
+        # cheap. Every other commit writes a new fingerprint into the context,
+        # which invalidates the `COPY . .` layer of both check stages: a
+        # commit that was never linted, formatted-checked, tested and built
+        # cannot report success from cache.
+        run: |
+          set -eu
+          fp="$(git log -1 --format=%H -- . ':!*.md' ':!LICENSE' ':!.editorconfig')"
+          printf '%s\n' "${fp:-$GITHUB_SHA}" > .ci-fingerprint
+
       - name: Build Docker image (runs make check)
         run: script/cibuild

.gitignore

@@ -42,3 +42,6 @@ data/
 # Temporary files
 tmp/
 temp/
+
+# CI cache barrier, written into the build context by the check workflow
+.ci-fingerprint 

Dockerfile

@@ -12,7 +12,11 @@ WORKDIR /src
 COPY go.mod go.sum ./
 RUN go mod download
 
-# Copy source code
+# Copy source code. In CI the context also carries .ci-fingerprint, whose
+# value changes with every commit that touches the build context (see
+# .gitea/workflows/check.yml). That invalidates this layer, so the checks
+# below cannot report success by replaying a cached pass. Do not add it to
+# .dockerignore.
 COPY . .
 
 # Run formatting check and linter
@@ -36,7 +40,8 @@ WORKDIR /build
 COPY go.mod go.sum ./
 RUN go mod download
 
-# Copy source code
+# Copy source code, including the .ci-fingerprint cache barrier described in
+# the lint stage above.
 COPY . .
 
 # Run tests and build

README.md

@@ -93,9 +93,10 @@ TTY detection, and security headers are always applied.
 | `METRICS_USERNAME`      | Basic auth username for `/metrics`  | `""`     |
 | `METRICS_PASSWORD`      | Basic auth password for `/metrics`  | `""`     |
 | `SENTRY_DSN`            | Sentry error reporting DSN          | `""`     |
+| `RETENTION_SWEEP_INTERVAL` | How often the retention reaper and archive sweeper run (Go duration, must be positive) | `1h` |
 | `SESSION_IDLE_TIMEOUT`  | Idle session timeout (Go duration)  | `24h`    |
 | `RECEIVER_RATE_LIMIT`   | Receiver requests/minute per IP per entrypoint (10x that per IP across the route) | `120` |
-| `TRUSTED_PROXIES`       | CIDRs whose forwarded headers are trusted | `""` (none) |
+| `TRUSTED_PROXIES`       | CIDRs whose forwarded headers are trusted (unset: all clients behind a proxy share one rate-limit bucket) | `""` (none) |
 
 #### Trusted proxies
 
@@ -114,6 +115,23 @@ or draining someone else's. Set it to the address of your reverse
 proxy, and to nothing wider. A set but unparseable value aborts
 startup.
 
+That default is safe against forged headers, but leaving it unset in
+production has a cost you must know about. Production runs behind a
+TLS-terminating reverse proxy, so with `TRUSTED_PROXIES` unset every
+request keys on the proxy's own address and all clients share a single
+bucket per limit. For the login and password-change limits that is a
+denial of service anyone can perform: a steady five POSTs per minute
+from any address on the internet keeps the shared login bucket full,
+and the operator's own login then returns HTTP 429 for as long as the
+trickle continues. There is no second administrative path and no
+bypass. Restarting the service clears the in-memory buckets, but a
+sustained trickle re-locks them immediately.
+
+The remedy is to set `TRUSTED_PROXIES` to your reverse proxy's
+address, which restores per-client buckets. webhooker logs a warning
+at startup when `WEBHOOKER_ENVIRONMENT=prod` and `TRUSTED_PROXIES` is
+empty. See [Rate Limiting](#rate-limiting) for what each limit shares.
+
 `X-Real-IP` and `True-Client-IP` are **never** read, from any peer.
 Reverse proxies append to `X-Forwarded-For` but forward other client
 headers verbatim, so a single-valued header is client-controlled even
@@ -173,8 +191,12 @@ its value and refuses to start, rather than silently running with a
 substituted default. `PORT=eighty`, `DEBUG=ture`, and
 `RETENTION_SWEEP_INTERVAL=1 hour` all abort startup. `PORT` must
 additionally be a number in the range 1–65535,
-`RECEIVER_RATE_LIMIT` must be at least 1, and every entry in
+`RECEIVER_RATE_LIMIT` must be at least 1,
-`TRUSTED_PROXIES` must be a CIDR block or a bare IP address.
+`RETENTION_SWEEP_INTERVAL` must be greater than zero (it is a ticker
+period, so `0s` or a negative value would crash the reaper after
+startup), and every entry in `TRUSTED_PROXIES` must be a CIDR block or
+a bare IP address. `SESSION_IDLE_TIMEOUT` is the exception: a
+non-positive value there means idle expiry is disabled, not invalid.
 
 Boolean variables (`DEBUG`, `MAINTENANCE_MODE`) accept exactly the
 spellings Go's `strconv.ParseBool` accepts — `1`, `t`, `T`, `TRUE`,
@@ -260,9 +282,10 @@ webhooker solves this by acting as a durable intermediary:
    targets simultaneously. This enables patterns like forwarding a
    GitHub webhook to both a deployment service and a Slack channel.
 
-5. **Replay** — Stored events can be manually redelivered for debugging
+5. **Replay** (not yet implemented) — Every received event is stored in
-   or testing, without requiring the original sender to fire the webhook
+   full, which is what manual redelivery for debugging or testing will
-   again.
+   be built on. No redelivery exists today, in the web UI or the API;
+   see [TODO.md](TODO.md).
 
 ### Use Cases
 
@@ -272,6 +295,7 @@ webhooker solves this by acting as a durable intermediary:
   size, and delivery performance
 - **Debugging** and introspection of webhook payloads in the web UI
 - **Replay** of webhook events for application testing and development
+  (planned; not yet implemented)
 - **Fan-out** delivery of a single webhook to multiple downstream
   targets
 - **High-availability ingestion** for delivery to less reliable backend
@@ -497,7 +521,7 @@ A programmatic access credential for API authentication.
 #### Event
 
 A captured incoming webhook request. Stores the complete HTTP request
-data for replay and auditing.
+data for auditing and for the planned replay capability.
 
 | Field          | Type   | Description |
 | -------------- | ------ | ----------- |
@@ -779,9 +803,10 @@ unknown) one while one of its deliveries is still `retrying`, both
 recovery paths above terminally mark that delivery `failed` and record a
 `DeliveryResult` naming the current target type as the reason, logging it
 at warn level. The delivery is not re-dispatched under the new type — the
-operator never asked for that delivery — and the event itself remains
+operator never asked for that delivery — and while the event itself
-stored in the per-webhook event database, so it can be redelivered
+remains stored in the per-webhook event database, there is no way to
-manually.
+redeliver it: manual redelivery is planned, not implemented (see
+[TODO.md](TODO.md)).
 
 ### Circuit Breaker (HTTP Targets with Retries)
 
@@ -861,9 +886,20 @@ requests still costs an entrypoint lookup before it 404s. The aggregate
 limit leaves room for one address to drive several entrypoints at their
 full rate, and it is not configurable separately.
 
-Requests to a `/webhook/` path that names no entrypoint are logged at
+What that aggregate limit bounds is the database work an invented path
-`DEBUG` only, since the path is attacker-controlled; the request itself
+costs; log volume it caps rather than eliminates. A path that names no
-still appears in the access log.
+entrypoint is recorded by the handler at `DEBUG`, and the aggregate
+limiter logs its own rejections at `DEBUG` and without the path, so
+neither appears at all under the default level. The per-entrypoint
+limiter is the loud one: it still logs every rejection at `WARN` with
+the request path, which on this route is attacker-controlled text. A
+client hammering a single invented path is served `RECEIVER_RATE_LIMIT`
+requests and has the rest of its aggregate budget rejected there, so
+the aggregate limit is what bounds those `WARN` lines — to under ten
+times `RECEIVER_RATE_LIMIT` per minute per client IP, 1080 at the
+defaults, where before it there was no bound at all. The access log is
+bounded by neither limit: every request is recorded once at `INFO` with
+its full URL, served or rejected alike.
 
 Every limiter here — receiver, login, and password change — identifies
 the client the same way, through one shared key function: the
@@ -871,9 +907,29 @@ connection's own address, unless the peer is listed in
 `TRUSTED_PROXIES`, in which case the forwarded client address is used
 instead. See [Trusted proxies](#trusted-proxies). Deployed without that
 variable set, a client behind a reverse proxy shares one bucket with
-every other client behind the same proxy, which is the safe direction
+every other client behind the same proxy. Set `TRUSTED_PROXIES` to the
-to be wrong in: set `TRUSTED_PROXIES` to the proxy's address to get
+proxy's address to get per-client limits back. What the shared bucket
-per-client limits back.
+costs is not the same for every limiter, and the two cases pull in
+opposite directions:
+
+- For the **receiver** limits it costs throughput, which is the safe
+  direction to be wrong in: sharing can only make a limit bind sooner,
+  never let a sender past it. It matters more for the aggregate limit
+  than for the per-entrypoint one: with `TRUSTED_PROXIES` unset behind
+  the reverse proxy a production deployment is required to run behind,
+  every request keys on the proxy, so the aggregate limit becomes a
+  service-wide ceiling of 1200 requests per minute across all senders
+  and all entrypoints, where the per-entrypoint limit's capacity still
+  grows with the number of entrypoints. Any deployment with more than a
+  handful of busy entrypoints must set `TRUSTED_PROXIES`.
+- For the **login and password-change** limits it costs availability of
+  the only administrative path, which is not safe at all. Five POSTs
+  per minute from any address on the internet keeps the single shared
+  login bucket full, and the operator's own login returns HTTP 429 for
+  as long as that trickle continues. A restart clears the in-memory
+  buckets and a resumed trickle re-locks them. Production deployments
+  must set `TRUSTED_PROXIES`; webhooker warns at startup when it is
+  empty in `prod`.
 
 Finer-grained per-webhook rate limits (configured in the web UI and
 enforced in the webhook handler) can layer on top of this env-level
@@ -1092,8 +1148,11 @@ downstream at form-parse time.
   (custom HTTP transport with SSRF-safe dialer that validates resolved
   IPs before connecting, preventing DNS rebinding attacks)
 - **Login rate limiting** via [go-chi/httprate](https://github.com/go-chi/httprate):
-  per-IP sliding-window rate limiter on the login endpoint (5 POST
+  sliding-window rate limiter on the login endpoint, 5 POST attempts
-  attempts per minute per IP) to prevent brute-force attacks
+  per minute per bucket, to slow brute-force attacks. The bucket is per
+  client IP only when `TRUSTED_PROXIES` names the reverse proxy;
+  unset, every client shares one bucket and the login becomes remotely
+  deniable (see [Rate Limiting](#rate-limiting))
 - Prometheus metrics behind basic auth
 - Static assets embedded in binary (no filesystem access needed at
   runtime)
@@ -1118,6 +1177,34 @@ binary is statically linked and runs on Alpine.
 `docker build .` is the CI gate — if it passes, the code is formatted,
 linted, tested, and compiled.
 
+#### CI gate honesty
+
+A layer cache lets `docker build .` exit 0 in seconds with the lint and
+test stages replayed rather than executed, which would make a green
+check meaningless. The `check` workflow therefore writes
+`.ci-fingerprint` into the build context before building. Its value is
+the hash of the last commit that touched the build context, so:
+
+- Any commit that changes code (including a squash merge whose tree
+  matches an already-built branch) gets a new fingerprint, invalidates
+  the `COPY . .` layer of both check stages, and really runs
+  `make fmt-check`, `make lint`, `make test`, and `make build`. A run
+  that reports success ran them.
+- A docs-only commit leaves the fingerprint unchanged — `.dockerignore`
+  excludes `*.md` and `LICENSE` from the context anyway — so the image
+  replays from cache and costs seconds.
+
+The module download layer sits above `COPY . .` and stays cached either
+way.
+
+The workflow's first step covers a second way the gate lied: Gitea
+cancels an in-flight run when a newer commit lands on the same branch
+and records that cancellation as a `failure` status, marking a commit
+red that was never tested. Cancellation is unconditional server-side for
+push events, so the superseding run rewrites the exact
+`Has been cancelled` status to `skipped`. Genuine failures are never
+touched.
+
 ## TODO
 
 See [TODO.md](TODO.md).

TODO.md

@@ -24,22 +24,52 @@ event retention (#63), the database archiving target (#43), the admin
 password change flow (#65), policy compliance (#6), pinned lint tooling
 (#55), and fail-loud configuration parsing (#80).
 
-`next` (9bfd033) holds the completed 1.0.0 milestone: every issue in it
+`next` holds the completed 1.0.0 milestone: every issue in it is closed,
-is closed, and it is verified green by cache-defeated container runs
+and it is verified green both by CI and by cache-defeated container
-rather than by the CI badge, which can pass without executing anything
+runs. The two were only made to mean the same thing this cycle — before
-(#119). Note: TODO.md was deliberately deleted from this repo in f9a9569
+#119, a warm layer cache let the gate report success without executing
-(2026-03-01, #6); its content was folded into the README TODO section,
+anything, and replayed the previous build's console log so the lie
-which this draft reconstructs as of 2026-07-06.
+looked like a real run. Note: TODO.md was deliberately deleted from this
+repo in f9a9569 (2026-03-01, #6); its content was folded into the README
+TODO section, which this draft reconstructs as of 2026-07-06.
 
 # Next Step
 
-Tag 1.0.0 from `main` once the milestone PR merges, then repair the CI
+Merge the milestone PR to `main` and tag 1.0.0 from it.
-gate (#119) before the next cycle's work lands — a gate that can report
+
-success without running is the one thing every other guarantee here
+Two decisions are open and belong to the owner, neither blocking the
-rests on.
+tag: #115 (mask the `http` target's destination URL, implemented
+speculatively and awaiting a yes or no) and #125 (whether IPv6
+rate-limit keys should bucket by `/64`).
 
 # Completed Steps
 
+- 2026-08-12 Bound the receiver rate limit per client IP across the
+  whole `/webhook/*` route. The existing limiter keyed on the request
+  path and `/webhook/{uuid}` matches any single segment, so a client
+  that invented a fresh path per request minted a fresh bucket per
+  request: the limit on the only unauthenticated endpoint bounded
+  nothing in aggregate, and every request still cost an entrypoint
+  lookup before it 404ed. An outer limiter keyed on the client address
+  alone now bounds that, chained in front of the unchanged
+  per-entrypoint limiter (#139)
+- 2026-08-12 Correct release-blocking documentation inaccuracies: the
+  README promised manual redelivery in the present tense in three
+  places when nothing implements it (the same false claim also sat in
+  the doc comment that was its source text), the env table omitted
+  `RETENTION_SWEEP_INTERVAL`, and `TODO.md` itself omitted five landed
+  units (#141)
+- 2026-08-12 Make the CI gate execute the checks it reports on. The
+  workflow now writes a build-context fingerprint before calling
+  `script/cibuild`, so a code commit invalidates the `COPY` layer of
+  the lint and builder stages while a docs-only commit still replays
+  from cache; a superseding run also rewrites the `failure` status
+  Gitea leaves on commits it cancelled and never tested. Verified by
+  pushing a deliberately broken test and watching CI go red (#119)
+- 2026-08-12 Require a positive `RETENTION_SWEEP_INTERVAL`: a
+  non-positive value reached `time.NewTicker` in both the retention
+  reaper and the archive sweeper, panicking two goroutines with no
+  recover after startup had already reported success (#140)
 - 2026-08-12 Bound the `X-Forwarded-For` scan's allocation to the hop
   cap: the reverse walk cuts entries with `strings.LastIndexByte`
   instead of joining and splitting, so a 1 MB header allocates 16 bytes
@@ -61,6 +91,34 @@ rests on.
   Profile settings placeholder removed, a progressive-enhancement copy
   button for the entrypoint URL, and retention form copy that states the
   actual policy (deletion by the reaper, 0 retains forever) (#57)
+- 2026-08-11 Mask the webhook credential in delivery errors and logs:
+  Go embeds the request URL in `*url.Error`, so every transport failure
+  persisted the full Slack webhook URL into the per-webhook event
+  database via `DeliveryResult.Error`, a field a future REST API would
+  have served. `maskURLError` drops path, query and userinfo while
+  preserving the wrapped cause, so `errors.Is`/`As` and `Timeout()`
+  still work and DNS, TLS and timeout failures still read differently
+  (#118)
+- 2026-08-11 Rate-limit the public webhook receiver endpoint
+  (`RECEIVER_RATE_LIMIT`, default 120/min), keyed on client IP plus
+  entrypoint path so one entrypoint cannot exhaust another's budget;
+  over-limit requests get 429 with `Retry-After`. It was the one
+  unauthenticated, internet-facing endpoint with no limit at all (#64)
+- 2026-08-11 Enforce the body size limit before CSRF parses the form:
+  `MaxBodySize` is now first in all four form-parsing route groups, so
+  an oversized request is rejected with 413 instead of being read in
+  full by the CSRF middleware before any cap applied (#90)
+- 2026-08-11 Mask target config on the source detail page, which
+  rendered the stored blob verbatim and so exposed the Slack
+  incoming-webhook URL — a bearer credential that cannot be revoked
+  per-holder. Config reaches the template only as a `TargetView` of
+  labelled fields, and header values are rendered as a count (#113)
+- 2026-08-11 Allow `retention_days` of 0 to mean retain forever, via a
+  sentinel written in `BeforeSave` so the GORM column default cannot
+  win the race. Also bounds the reaper's cutoff arithmetic: day counts
+  above 106751 overflowed `time.Duration` and wrapped the cutoff into
+  the future, where every row matched and the sweep deleted everything
+  (#79)
 - 2026-08-09 Inactivity-based session timeout: sliding idle expiry
   (`SESSION_IDLE_TIMEOUT`, default `24h`) refreshed on authenticated
   requests, with the 7-day absolute cap kept as an independent
@@ -115,6 +173,9 @@ rests on.
 
 # Future Steps
 
+- Manual event redelivery from the web UI — the "Replay" capability the
+  README describes as planned. No redelivery code exists anywhere in the
+  tree; events are stored in full, which is all it would be built on
 - Delivery status and retry management UI
 - Per-webhook rate limiting in the receiver handler (per-webhook config
   plus handler enforcement; global limits must not apply to receiver

internal/config/config.go

@@ -92,6 +92,7 @@ type Config struct {
 	SentryDSN       string
 
 	// RetentionSweepInterval is how often the retention reaper runs.
+	// Always positive: it becomes a time.NewTicker period.
 	RetentionSweepInterval time.Duration
 
 	// SessionIdleTimeout is the sliding inactivity window after
@@ -235,6 +236,34 @@ func envDuration(
 	return d, nil
 }
 
+// envPositiveDuration returns the value of the named environment
+// variable parsed as a Go duration that must be greater than zero.
+// Returns defaultValue if not set. A set value that is unparseable or
+// non-positive is a hard error naming the key and the bad value.
+//
+// This is for durations that reach time.NewTicker, which panics on a
+// non-positive period, in a goroutine started after startup has
+// already reported success. It is deliberately not used for durations
+// where non-positive means "disabled" (SESSION_IDLE_TIMEOUT).
+func envPositiveDuration(
+	key string,
+	defaultValue time.Duration,
+) (time.Duration, error) {
+	d, err := envDuration(key, defaultValue)
+	if err != nil {
+		return 0, err
+	}
+
+	if d <= 0 {
+		return 0, fmt.Errorf(
+			"%w: %s must be greater than zero, got %s",
+			ErrNonPositiveValue, key, d,
+		)
+	}
+
+	return d, nil
+}
+
 // 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).
@@ -346,7 +375,7 @@ func loadFromEnv() (*Config, error) {
 		return nil, err
 	}
 
-	retentionSweepInterval, err := envDuration(
+	retentionSweepInterval, err := envPositiveDuration(
 		"RETENTION_SWEEP_INTERVAL",
 		defaultRetentionSweepInterval,
 	)
@@ -354,6 +383,8 @@ func loadFromEnv() (*Config, error) {
 		return nil, err
 	}
 
+	// Non-positive is "disabled" here, not invalid, so this stays on
+	// envDuration.
 	sessionIdleTimeout, err := envDuration(
 		"SESSION_IDLE_TIMEOUT",
 		defaultSessionIdleTimeout,
@@ -391,6 +422,38 @@ func loadFromEnv() (*Config, error) {
 	}, nil
 }
 
+// warnSharedRateLimitBucket logs a startup warning when a production
+// deployment leaves TRUSTED_PROXIES empty.
+//
+// With no trusted proxies every rate limiter keys on the connecting
+// peer's address. A production deployment is required to run behind a
+// TLS-terminating reverse proxy, and the peer is then that proxy for
+// every request, so all clients share one bucket per limiter. The
+// login limiter's bucket is the dangerous one: any remote client can
+// keep it full, which denies the only administrative login to
+// everyone until the process restarts.
+//
+// The default of trusting nobody is deliberate — trusting forwarded
+// headers from arbitrary peers lets any client choose its own bucket —
+// so this warns rather than failing startup or changing the key.
+func (c *Config) warnSharedRateLimitBucket(log *slog.Logger) {
+	if !c.IsProd() || len(c.TrustedProxies) > 0 {
+		return
+	}
+
+	log.Warn(
+		"TRUSTED_PROXIES is empty: rate limits key on the "+
+			"connecting peer, so behind the reverse proxy a "+
+			"production deployment runs behind, every client "+
+			"shares one bucket per limit. Any remote client can "+
+			"then keep the login limit full and deny the admin "+
+			"login, the only administrative path, until restart. "+
+			"Set TRUSTED_PROXIES to your reverse proxy's address.",
+		"environment", c.Environment,
+		"trustedProxies", len(c.TrustedProxies),
+	)
+}
+
 // New creates a Config by reading environment variables.
 //
 //nolint:revive // lc parameter is required by fx even if unused.
@@ -435,5 +498,7 @@ func New(lc fx.Lifecycle, params ConfigParams) (*Config, error) {
 		s.MetricsUsername != "" && s.MetricsPassword != "",
 	)
 
+	s.warnSharedRateLimitBucket(log)
+
 	return s, nil
 }

internal/config/config_test.go

@@ -1,6 +1,8 @@
 package config_test
 
 import (
+	"bytes"
+	"log/slog"
 	"os"
 	"testing"
 	"time"
@@ -139,6 +141,10 @@ func TestRetentionSweepInterval(t *testing.T) {
 		set         bool
 		value       string
 		expectError bool
+		// sentinel, when set, must be wrapped by the startup
+		// error; every error case must additionally name the
+		// variable in its message.
+		sentinel error
 		expected time.Duration
 	}{
 		{
@@ -158,6 +164,24 @@ func TestRetentionSweepInterval(t *testing.T) {
 			value:       "not-a-duration",
 			expectError: true,
 		},
+		{
+			// A non-positive period panics the ticker in the
+			// reaper and archive-sweeper goroutines, long after
+			// startup has reported success, so it has to fail
+			// here instead.
+			name:        "zero fails startup",
+			set:         true,
+			value:       "0s",
+			expectError: true,
+			sentinel:    config.ErrNonPositiveValue,
+		},
+		{
+			name:        "negative fails startup",
+			set:         true,
+			value:       "-1h",
+			expectError: true,
+			sentinel:    config.ErrNonPositiveValue,
+		},
 	}
 
 	for _, tt := range tests {
@@ -175,7 +199,9 @@ func TestRetentionSweepInterval(t *testing.T) {
 			}
 
 			if tt.expectError {
-				expectStartupError(t)
+				expectStartupErrorFor(
+					t, "RETENTION_SWEEP_INTERVAL", tt.sentinel,
+				)
 			} else {
 				testRetentionSweepIntervalSuccess(t, tt.expected)
 			}
@@ -281,6 +307,22 @@ func TestSessionIdleTimeout(t *testing.T) {
 			value:       "not-a-duration",
 			expectError: true,
 		},
+		{
+			// Non-positive is "idle expiry disabled" for this
+			// variable, not a configuration error: unlike
+			// RETENTION_SWEEP_INTERVAL it never becomes a ticker
+			// period.
+			name:     "zero disables idle expiry",
+			set:      true,
+			value:    "0s",
+			expected: 0,
+		},
+		{
+			name:     "negative disables idle expiry",
+			set:      true,
+			value:    "-1h",
+			expected: -time.Hour,
+		},
 	}
 
 	for _, tt := range tests {
@@ -584,3 +626,79 @@ func testTrustedProxiesSuccess(
 
 	assert.Equal(t, expected, got)
 }
+
+// TestSharedRateLimitBucketWarning covers the startup warning that
+// tells an operator their production deployment shares one rate-limit
+// bucket between every client, which makes the admin login remotely
+// deniable. It must fire when TRUSTED_PROXIES is empty in production
+// and stay quiet otherwise.
+func TestSharedRateLimitBucketWarning(t *testing.T) {
+	tests := []struct {
+		name           string
+		environment    string
+		trustedProxies string
+		expectWarning  bool
+	}{
+		{
+			name:          "prod without trusted proxies warns",
+			environment:   config.EnvironmentProd,
+			expectWarning: true,
+		},
+		{
+			name:           "prod with trusted proxies is quiet",
+			environment:    config.EnvironmentProd,
+			trustedProxies: cidrPrivateV4,
+			expectWarning:  false,
+		},
+		{
+			// Development is not required to run behind a
+			// reverse proxy, so the shared bucket the warning
+			// describes is not the expected shape there.
+			name:          "dev without trusted proxies is quiet",
+			environment:   config.EnvironmentDev,
+			expectWarning: false,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			// Cannot use t.Parallel() here because t.Setenv
+			// is incompatible with parallel subtests.
+			t.Setenv("WEBHOOKER_ENVIRONMENT", tt.environment)
+
+			if tt.trustedProxies == "" {
+				require.NoError(
+					t, os.Unsetenv("TRUSTED_PROXIES"),
+				)
+			} else {
+				t.Setenv("TRUSTED_PROXIES", tt.trustedProxies)
+			}
+
+			var buf bytes.Buffer
+
+			log := slog.New(slog.NewJSONHandler(
+				&buf, &slog.HandlerOptions{
+					Level: slog.LevelDebug,
+				},
+			))
+
+			require.NoError(
+				t,
+				config.WarnSharedRateLimitBucketForTest(log),
+			)
+
+			if !tt.expectWarning {
+				assert.Empty(t, buf.String())
+
+				return
+			}
+
+			logged := buf.String()
+
+			assert.Contains(t, logged, `"level":"WARN"`)
+			assert.Contains(t, logged, "TRUSTED_PROXIES")
+			assert.Contains(t, logged, "shares one bucket")
+			assert.Contains(t, logged, "deny the admin login")
+		})
+	}
+}

internal/config/export_test.go

@@ -1,9 +1,26 @@
 package config
 
+import "log/slog"
+
 // This file exposes the unexported environment parsing helpers to
 // the external config_test package so each helper can be covered by
 // its own table-driven test without weakening the package API.
 
+// WarnSharedRateLimitBucketForTest loads a Config from the current
+// environment and emits its startup warnings to log. The real logger
+// writes to stdout, so this lets the warning's firing condition be
+// asserted against a handler the test controls.
+func WarnSharedRateLimitBucketForTest(log *slog.Logger) error {
+	c, err := loadFromEnv()
+	if err != nil {
+		return err
+	}
+
+	c.warnSharedRateLimitBucket(log)
+
+	return nil
+}
+
 // EnvBoolForTest exposes envBool.
 func EnvBoolForTest(key string, defaultValue bool) (bool, error) {
 	return envBool(key, defaultValue)

internal/delivery/engine.go

@@ -785,9 +785,9 @@ func (e *Engine) sweepSingleRetry(
 // status retrying themselves. Re-dispatching under the new type
 // would be a delivery the operator never asked for, and leaving
 // the row retrying strands it forever, so the delivery is
-// failed with a recorded reason and can be redelivered
+// failed with a recorded reason. The event stays stored, but
-// manually. Logged at warn, not error: this is operator-caused
+// nothing redelivers it today. Logged at warn, not error: this
-// state, not a system fault.
+// is operator-caused state, not a system fault.
 func (e *Engine) failUnretryableRetry(
 	webhookDB *gorm.DB,
 	webhookID string,

internal/middleware/ratelimit.go

@@ -171,9 +171,11 @@ func (m *Middleware) clientKey(r *http.Request) string {
 	return peer.String()
 }
 
-// tooManyRequests returns the 429 handler shared by every limiter:
+// tooManyRequests returns the 429 handler used by the login,
-// it logs the rejection with logMessage and answers with
+// password-change and per-entrypoint receiver limiters: it logs the
-// responseMessage. httprate adds the Retry-After header (RFC 6585).
+// rejection with logMessage and answers with responseMessage.
+// httprate adds the Retry-After header (RFC 6585). The aggregate
+// receiver limiter uses floodTooManyRequests instead.
 func (m *Middleware) tooManyRequests(
 	logMessage, responseMessage string,
 ) http.HandlerFunc {
@@ -183,6 +185,31 @@ func (m *Middleware) tooManyRequests(
 	}
 }
 
+// floodTooManyRequests returns the 429 handler for a limiter whose
+// rejections are themselves the flood: it logs at DEBUG and without
+// the path, then answers with responseMessage.
+//
+// The aggregate receiver limiter trips exactly when one address is
+// sending faster than the receiver wants to serve, so its rejection
+// log is one line per request of that flood. At WARN with "path" that
+// hands a client a way to write its own text into the operator's log,
+// at a level that trips alerting, once per request — the log-volume
+// problem this limiter exists to bound. DEBUG is off in production by
+// default, so a flood costs nothing here; the path is dropped so that
+// turning DEBUG on to diagnose one does not restore the problem.
+//
+// This limiter bounds the database work an invented path costs, not
+// the number of log lines it produces: the access log in
+// middleware.go still records every request, served or rejected.
+func (m *Middleware) floodTooManyRequests(
+	logMessage, responseMessage string,
+) http.HandlerFunc {
+	return func(w http.ResponseWriter, _ *http.Request) {
+		m.log.Debug(logMessage)
+		http.Error(w, responseMessage, http.StatusTooManyRequests)
+	}
+}
+
 // LoginRateLimit returns middleware that enforces per-IP rate
 // limiting on login attempts using go-chi/httprate. Only POST
 // requests are rate-limited; GET requests (rendering the login
@@ -287,7 +314,7 @@ func (m *Middleware) ReceiverRateLimit() func(http.Handler) http.Handler {
 		receiverAggregateLimit(m.params.Config.ReceiverRateLimit),
 		receiverRateInterval,
 		httprate.WithKeyFuncs(m.rateLimitKey),
-		httprate.WithLimitHandler(m.tooManyRequests(
+		httprate.WithLimitHandler(m.floodTooManyRequests(
 			"webhook receiver aggregate rate limit exceeded",
 			"Too many requests. Please slow down.",
 		)),

internal/middleware/ratelimit_test.go

@@ -723,6 +723,58 @@ func TestReceiverRateLimit_LimitsAggregateAcrossInventedPaths(
 	)
 }
 
+// TestReceiverRateLimit_RejectedRequestsCountTowardAggregate pins the
+// order the two limiters are chained in. The aggregate limiter has to
+// be the outer one, so that it counts requests the per-entrypoint
+// limiter rejects: those requests still arrive, and the aggregate
+// limit exists to bound what one address can make the receiver do.
+//
+// One path is hammered past the per-entrypoint limit, which alone
+// would leave the aggregate budget almost untouched; then a path the
+// client has never used must be rejected, which only the aggregate
+// limiter can do. Swap the two limiters and that last request is
+// served, because the rejected ones never reached the aggregate
+// limiter to be counted.
+func TestReceiverRateLimit_RejectedRequestsCountTowardAggregate(
+	t *testing.T,
+) {
+	t.Parallel()
+
+	const (
+		limit = 3
+		ip    = "6.6.6.8:1234"
+	)
+
+	aggregate := limit * middleware.ReceiverAggregateMultiplierConst
+
+	handler := receiverLimitedHandler(t, limit)
+
+	// Spend the whole aggregate budget on one path. Only the first
+	// limit requests are served; the rest are rejected by the
+	// per-entrypoint limiter but still count against the aggregate.
+	for i := range aggregate {
+		w := receiverPost(handler, ip, "/webhook/exhausted")
+
+		want := http.StatusTooManyRequests
+		if i < limit {
+			want = http.StatusOK
+		}
+
+		assert.Equal(
+			t, want, w.Code,
+			"request %d to the exhausted path", i,
+		)
+	}
+
+	w := receiverPost(handler, ip, "/webhook/never-used")
+	assert.Equal(
+		t, http.StatusTooManyRequests, w.Code,
+		"requests rejected per entrypoint must still count "+
+			"toward the aggregate limit, so the aggregate "+
+			"limiter has to run first",
+	)
+}
+
 // TestReceiverAggregateLimit_SaturatesOnOverflow covers the derived
 // aggregate limit for a configured per-entrypoint limit large enough
 // that multiplying it would wrap negative, which httprate would read

static/js/app.js

@@ -1,5 +1,4 @@
 // Webhooker client-side JavaScript
-console.log("Webhooker loaded");
 
 // Copy-to-clipboard, as progressive enhancement.
 //