diff --git a/README.md b/README.md index 570f087..63534e1 100644 --- a/README.md +++ b/README.md @@ -11,9 +11,11 @@ with retry support, logging, and observability. Category: infrastructure ### Prerequisites -- Go 1.26+ -- golangci-lint v2.11+ -- Docker (for containerized deployment) +- Go 1.26.1+ (the version in `go.mod`) +- golangci-lint v2.12.2 (the version pinned in `script/bootstrap` and + in the `Dockerfile`'s lint stage; `make bootstrap` installs it) +- Docker (for containerized deployment, and for the lint and test + stages of the CI gate) ### Quick Start @@ -28,8 +30,10 @@ make deps # Run all checks (format, lint, test, build) make check -# Run in development mode (uses SQLite in current directory) -make dev +# Run in development mode. DATA_DIR defaults to /var/lib/webhooker in +# every environment, so set it (in .env or the shell) to a writable +# directory when running from a clone. +DATA_DIR=./data make dev # Build Docker image make docker @@ -45,9 +49,13 @@ make lint # Run golangci-lint make test # Run tests with race detection make check # test + lint + fmt-check (CI gate) make build # Build binary to bin/webhooker +make run # build, then run ./bin/webhooker make dev # go run ./cmd/webhooker +make deps # go mod download + go mod tidy make docker # Build Docker image make hooks # Install git pre-commit hook that runs script/precommit +make css # Regenerate static/css/tailwind.css (needs tailwindcss) +make clean # Remove bin/ ``` ### Configuration @@ -89,7 +97,7 @@ TTY detection, and security headers are always applied. | `PORT` | HTTP listen port | `8080` | | `DATA_DIR` | Directory for all SQLite databases | `/var/lib/webhooker` | | `DEBUG` | Enable debug logging | `false` | -| `MAINTENANCE_MODE` | Serve the maintenance page | `false` | +| `MAINTENANCE_MODE` | Report `maintenanceMode: true` in the healthcheck JSON. It does not change how any request is served — no maintenance page exists | `false` | | `METRICS_USERNAME` | Basic auth username for `/metrics` | `""` | | `METRICS_PASSWORD` | Basic auth password for `/metrics` | `""` | | `SENTRY_DSN` | Sentry error reporting DSN | `""` | @@ -129,8 +137,13 @@ 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. +at startup whenever `TRUSTED_PROXIES` is empty, in every environment — +not only when `WEBHOOKER_ENVIRONMENT=prod`, because that variable +defaults to `dev` and an operator who never set it is precisely the +one at risk. The warning is informational when nothing proxies to the +process: with no proxy in front, the peer address is the client's own +and the buckets are already per-client. 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 @@ -162,6 +175,8 @@ Two operator requirements follow: makes all three limits, including the unauthenticated webhook receiver, silently bypassable by every client in the block. +#### Sessions + Sessions are bounded by two independent clocks, and end at whichever one runs out first: @@ -231,9 +246,11 @@ docker run -d \ The container runs as a non-root user (`webhooker`, UID 1000), exposes port 8080, and includes a health check against `/.well-known/healthcheck`. The `/var/lib/webhooker` volume holds all -SQLite databases: the main application database (`webhooker.db`) and -the per-webhook event databases (`events-{uuid}.db`). Mount this as a -persistent volume to preserve data across container restarts. +SQLite databases: the main application database (`webhooker.db`), the +per-webhook event databases (`events-{uuid}.db`), and any archive +databases written by `database` targets (`archive-{uuid}.db`). Mount +this as a persistent volume to preserve data across container +restarts. ## Entrypoints @@ -330,7 +347,11 @@ It uses: - **[gorilla/csrf](https://github.com/gorilla/csrf)** for CSRF protection (cookie-based double-submit tokens) - **[go-chi/httprate](https://github.com/go-chi/httprate)** for - per-IP login rate limiting (sliding window counter) + sliding-window rate limiting of the login, password-change and + webhook receiver endpoints. The bucket is per client IP only when + `TRUSTED_PROXIES` names the reverse proxy; unset, every client + behind that proxy shares one bucket per limit (see + [Rate Limiting](#rate-limiting)) - **[Prometheus](https://prometheus.io)** for metrics, served at `/metrics` behind basic auth - **[Sentry](https://sentry.io)** for optional error reporting @@ -440,9 +461,20 @@ days (`database.RetentionForeverDays`). The retention reaper recognises that sentinel and skips the webhook entirely, and the web UI displays such a webhook's retention as "forever" rather than as a day count. -A *finite* retention is capped at `database.MaxFiniteRetentionDays` -(106751 days, about 292 years), and a larger one is rejected with a -400. The cap is not arbitrary: the reaper computes its cutoff as a +Submitted `retention_days` values therefore fall into three bands, not +two: + +- `0` up to `database.MaxFiniteRetentionDays` (106751 days, about 292 + years) is accepted as a finite retention. +- Above that ceiling but below the retain-forever sentinel of 365000 + (`database.RetentionForeverDays`) is rejected with a 400. This is the + band the cap exists for. +- `365000` or above is accepted and means retain forever, collapsing to + the sentinel. It is not out of range: the edit form pre-fills the + sentinel for a retain-forever webhook, so submitting that form back + unchanged has to keep meaning "forever". + +The cap is not arbitrary: the reaper computes its cutoff as a `time.Duration`, an int64 nanosecond count, and a longer period overflows it. An overflowed cutoff lands in the future, where it matches every row, so the sweep would delete every event the webhook @@ -534,7 +566,7 @@ data for auditing and for the planned replay capability. | `id` | UUID | Primary key | | `webhook_id` | UUID | Foreign key → Webhook | | `entrypoint_id` | UUID | Foreign key → Entrypoint | -| `method` | string | HTTP method (POST, PUT, etc.) | +| `method` | string | HTTP method of the captured request. Always `POST`: the receiver answers every other method with 405 before an Event is created | | `headers` | JSON | Complete request headers | | `body` | text | Raw request body | | `content_type` | string | Content-Type header value | @@ -934,8 +966,8 @@ opposite directions: 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`. + must set `TRUSTED_PROXIES`; webhooker warns at startup whenever it is + empty, in any environment. 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 @@ -947,17 +979,17 @@ abuse limit later; they are tracked as future work. | Method | Path | Description | | ------ | --------------------------- | ----------- | -| `GET` | `/` | Root redirect (authenticated → `/sources`, unauthenticated → `/pages/login`) | -| `GET` | `/.well-known/healthcheck` | Health check (JSON: status, uptime, version) | -| `GET` | `/s/*` | Static file serving (embedded CSS, JS) | -| `ANY` | `/webhook/{uuid}` | Webhook receiver endpoint (accepts all methods) | +| `GET` | `/` | Root redirect, 303 (authenticated → `/sources`, unauthenticated → `/pages/login`) | +| `GET` | `/.well-known/healthcheck` | Health check (JSON: `status`, `now`, `uptimeSeconds`, `uptimeHuman`, `version`, `appname`, `maintenanceMode`) | +| `GET` | `/s/*` | Static file serving (embedded CSS, JS; `GET` and `HEAD`) | +| `POST` | `/webhook/{uuid}` | Webhook receiver endpoint. `POST` only — every other method is answered `405 Method Not Allowed` with `Allow: POST`. Rate limited (see [Rate Limiting](#rate-limiting)) | #### Authentication Endpoints | Method | Path | Description | | ------ | --------------- | ----------- | -| `GET` | `/pages/login` | Login page | -| `POST` | `/pages/login` | Login form submission | +| `GET` | `/pages/login` | Login page (not rate limited; the limiter applies to POST only) | +| `POST` | `/pages/login` | Login form submission (5 per minute per bucket, then 429) | | `POST` | `/pages/logout` | Logout (destroys session) | #### Authenticated Endpoints @@ -965,6 +997,7 @@ abuse limit later; they are tracked as future work. | Method | Path | Description | | ------ | ------------------------ | ----------- | | `GET` | `/user/{username}` | User profile page | +| `POST` | `/user/{username}/password` | Change the user's password (5 per minute per bucket, then 429) | | `GET` | `/sources` | List user's webhooks | | `GET` | `/sources/new` | Create webhook form | | `POST` | `/sources/new` | Create webhook submission | @@ -974,13 +1007,17 @@ abuse limit later; they are tracked as future work. | `POST` | `/source/{id}/delete` | Delete webhook | | `GET` | `/source/{id}/logs` | Webhook event logs | | `POST` | `/source/{id}/entrypoints` | Add entrypoint to webhook | +| `POST` | `/source/{id}/entrypoints/{entrypointID}/delete` | Delete an entrypoint | +| `POST` | `/source/{id}/entrypoints/{entrypointID}/toggle` | Enable or disable an entrypoint | | `POST` | `/source/{id}/targets` | Add target to webhook | +| `POST` | `/source/{id}/targets/{targetID}/delete` | Delete a target | +| `POST` | `/source/{id}/targets/{targetID}/toggle` | Enable or disable a target | #### Infrastructure Endpoints | Method | Path | Description | | ------ | ---------- | ----------- | -| `GET` | `/metrics` | Prometheus metrics (requires basic auth) | +| `GET` | `/metrics` | Prometheus metrics, behind basic auth. The route is registered only when `METRICS_USERNAME` is set; otherwise it does not exist and returns 404 | #### API (Planned) @@ -994,8 +1031,10 @@ abuse limit later; they are tracked as future work. | `GET` | `/api/v1/webhooks/{id}/events` | List events for webhook | | `POST` | `/api/v1/events/{id}/redeliver`| Redeliver an event | -API authentication will use API keys passed via `Authorization: Bearer -` header. +None of these exist yet. `/api/v1` is mounted with no routes, so every +path under it returns 404 today. API authentication will use API keys +passed via `Authorization: Bearer ` header; no Bearer middleware +is implemented either. ### Package Layout @@ -1023,13 +1062,25 @@ webhooker/ │ │ ├── model_delivery_result.go # DeliveryResult entity (per-webhook DB) │ │ ├── model_apikey.go # APIKey entity │ │ ├── password.go # Argon2id hashing and verification +│ │ ├── retention.go # Retention reaper (per-webhook event expiry) │ │ └── webhook_db_manager.go # Per-webhook DB lifecycle manager │ ├── globals/ │ │ └── globals.go # Build-time variables (appname, version, arch) │ ├── delivery/ │ │ ├── engine.go # Event-driven delivery engine (channel + timer based) │ │ ├── circuit_breaker.go # Per-target circuit breaker for HTTP targets with retries +│ │ ├── target.go # Target interface, Task, Scheduler +│ │ ├── target_http.go # HTTP target (retries, circuit breaker) +│ │ ├── target_slack.go # Slack/Mattermost incoming-webhook target +│ │ ├── target_database.go # Database archive target +│ │ ├── target_database_archive.go # Archive file lifecycle and pruning +│ │ ├── target_log.go # Log target (stdout) +│ │ ├── target_config_view.go # Masked target config for templates +│ │ ├── archive_sweeper.go # Periodic pruning of idle archives +│ │ ├── url_mask.go # Strips credentials from *url.Error │ │ └── ssrf.go # SSRF prevention (IP validation, safe HTTP transport) +│ ├── lifecycle/ +│ │ └── lifecycle.go # Shared fx start/stop hook helpers │ ├── handlers/ │ │ ├── handlers.go # Base handler struct, JSON helpers, template rendering │ │ ├── auth.go # Login, logout handlers @@ -1054,11 +1105,15 @@ webhooker/ │ └── session.go # Cookie-based session management ├── static/ │ ├── static.go # //go:embed directive -│ ├── css/style.css # Custom stylesheet (system font stack, card effects, layout) -│ └── js/app.js # Client-side JavaScript (minimal bootstrap) -├── templates/ # Go HTML templates (base, index, login, etc.) -├── Dockerfile # Multi-stage: lint, build+test, then Alpine runtime -├── Makefile # fmt, lint, test, check, build, docker targets +│ ├── css/input.css # Tailwind input, source for tailwind.css (make css) +│ ├── css/tailwind.css # Generated stylesheet the pages load +│ ├── css/style.css # Older hand-written stylesheet, no longer loaded +│ ├── js/alpine.min.js # Alpine.js, served locally (no CDN) +│ └── js/app.js # Progressive-enhancement copy-to-clipboard +├── templates/ # Go HTML templates (base, login, sources, etc.) +├── script/ # Scripts to Rule Them All entrypoints +├── Dockerfile # Three stages: lint, test+build, Alpine runtime +├── Makefile # Thin shims over script/ ├── go.mod / go.sum └── .golangci.yml # Linter configuration ``` @@ -1074,21 +1129,27 @@ Components are wired via Uber fx in this order: user seed 5. `database.NewWebhookDBManager` — Per-webhook event database lifecycle manager -6. `healthcheck.New` — Health check service -7. `session.New` — Cookie-based session manager (key from database) -8. `handlers.New` — HTTP handlers -9. `middleware.New` — HTTP middleware -10. `delivery.New` — Event-driven delivery engine -11. `delivery.Engine` → `handlers.DeliveryNotifier` — interface bridge -12. `server.New` — HTTP server and router +6. `database.NewRetentionReaper` — Per-webhook event retention sweep +7. `healthcheck.New` — Health check service +8. `session.New` — Cookie-based session manager (key from database) +9. `handlers.New` — HTTP handlers +10. `middleware.New` — HTTP middleware +11. `delivery.New` — Event-driven delivery engine +12. `delivery.NewArchiveSweeper` — Periodic pruning of idle archives +13. `delivery.Engine` → `delivery.Notifier` — interface bridge +14. `delivery.Engine` → `delivery.WebhookEvictor` — interface bridge so + deleting a webhook releases its archive writer +15. `server.New` — HTTP server and router -The server starts via `fx.Invoke(func(*server.Server, *delivery.Engine) -{})` which triggers the fx lifecycle hooks in dependency order. The -`DeliveryNotifier` interface allows the webhook handler to send -self-contained `DeliveryTask` slices to the engine without a direct +The server starts via `fx.Invoke(func(*server.Server, *delivery.Engine, +*database.RetentionReaper, *delivery.ArchiveSweeper) {})`, which +triggers the fx lifecycle hooks in dependency order. The +`delivery.Notifier` interface allows the webhook handler to send +self-contained `delivery.Task` slices to the engine without a direct package dependency. Each task carries all target config and event data -inline (for bodies ≤16KB), so the engine can deliver without reading -from any database — it only writes to record results. +inline (for bodies ≤16KB, `delivery.MaxInlineBodySize`), so the engine +can deliver without reading from any database — it only writes to +record results. ### Middleware Stack @@ -1114,16 +1175,30 @@ CSRF middleware in every one of those route groups, because gorilla/csrf parses the form; if the cap were installed after it, form parsing would run under net/http's 10 MB default and the 1 MB limit would never apply. A request that declares a `Content-Length` over the -limit is answered with `413 Request Entity Too Large` before any other -middleware or handler runs; a chunked request, or one that lies about -its length, is hard-capped by `http.MaxBytesReader` and fails -downstream at form-parse time. +limit is answered with `413 Request Entity Too Large` without its body +being read and without reaching CSRF, the route group's remaining +middleware, or the handler. It is not rejected before *any* other +middleware, though: the eight global entries listed above all run +first, so such a request is still logged, counted in the metrics, and +given the security headers, and the rejection itself is logged at +`WARN` with the method, path and declared length. A chunked request, or +one that lies about its length, is hard-capped by +`http.MaxBytesReader` and fails downstream at form-parse time. + +Those same four route groups then apply **CSRF** and **NoCache** +(`Cache-Control: no-store`, `Pragma: no-cache`), and every group except +`/pages` applies **RequireAuth**. The rate limiters are per-route +rather than global: **LoginRateLimit** on `/pages/login`, +**PasswordChangeRateLimit** on `/user/{username}/password`, and +**ReceiverRateLimit** on `/webhook/{uuid}`. ### Authentication - **Web UI:** Cookie-based sessions using gorilla/sessions with encrypted cookies. Sessions are configured with HttpOnly, SameSite - Lax, and Secure (in production). Session lifetime is 7 days. + Lax, and Secure (in production). Absolute session lifetime is 7 days, + with a sliding idle timeout on top of it (see + [Sessions](#sessions)). - **API (planned):** API key authentication via `Authorization: Bearer` header. API keys are stored per-user with usage tracking (`last_used_at`). @@ -1155,10 +1230,13 @@ downstream at form-parse time. IPs before connecting, preventing DNS rebinding attacks) - **Login rate limiting** via [go-chi/httprate](https://github.com/go-chi/httprate): sliding-window rate limiter on the login endpoint, 5 POST attempts - 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)) + per minute per bucket, to slow brute-force attacks. GET requests to + the login page are not limited. The password-change endpoint carries + the same 5-per-minute limit. 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)). webhooker warns at startup + whenever `TRUSTED_PROXIES` is empty - Prometheus metrics behind basic auth - Static assets embedded in binary (no filesystem access needed at runtime) @@ -1167,21 +1245,32 @@ downstream at form-parse time. ### Docker -The Dockerfile uses a multi-stage build: +The Dockerfile uses a three-stage build. Each stage is pinned by +digest, and the two check stages are separate images so the linter's +version is fixed independently of the compiler's: -1. **Builder stage** (Debian-based `golang:1.24`) — installs - golangci-lint, downloads dependencies, copies source, runs `make - check` (format verification, linting, tests, compilation). -2. **Runtime stage** (`alpine:3.21`) — copies the binary, creates the - `/var/lib/webhooker` directory for all SQLite databases, runs as - non-root user, exposes port 8080, includes a health check. +1. **Lint stage** (`golangci/golangci-lint:v2.12.2`, Debian-based) — + installs `make`, downloads dependencies, copies the source, and runs + `make fmt-check` then `make lint`. +2. **Builder stage** (`golang:1.26.1-bookworm`) — depends on the lint + stage passing (it copies a file from it), then runs `make test` and + `make build`, and finally rebuilds the binary with + `CGO_ENABLED=1` and static linking so it runs on musl. +3. **Runtime stage** (`alpine:3.21`) — copies the static binary, + creates the `/var/lib/webhooker` directory for all SQLite databases, + runs as the non-root `webhooker` user (UID 1000), exposes port 8080, + and includes a health check against `/.well-known/healthcheck`. -The builder uses Debian rather than Alpine because GORM's SQLite -dialect pulls in CGO-dependent headers at compile time. The runtime -binary is statically linked and runs on Alpine. +Both check stages use Debian rather than Alpine because +`gorm.io/driver/sqlite` pulls in `mattn/go-sqlite3`, which needs CGO +and does not compile against musl. Only the final binary is statically +linked, which is what lets it run on the Alpine runtime image. -`docker build .` is the CI gate — if it passes, the code is formatted, -linted, tested, and compiled. +`script/cibuild` — `docker build .` — is the CI gate: the four check +targets run inside the image, so a build that succeeds is a repo that +is formatted, linted, tested and compiled. Local linting goes through +the same container rather than a host golangci-lint, because a host +binary can be a different version from the pinned one. #### CI gate honesty @@ -1197,16 +1286,17 @@ the hash of the last commit that touched the build context, so: `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. + excludes `*.md`, `LICENSE` and `.editorconfig` 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 +A separate workflow step, run before the fingerprint is written, 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. diff --git a/TODO.md b/TODO.md index e4e0296..2ea43e1 100644 --- a/TODO.md +++ b/TODO.md @@ -25,13 +25,17 @@ password change flow (#65), policy compliance (#6), pinned lint tooling (#55), and fail-loud configuration parsing (#80). `next` holds the completed 1.0.0 milestone: every issue in it is closed, -and it is verified green both by CI and by cache-defeated container -runs. The two were only made to mean the same thing this cycle — before -#119, a warm layer cache let the gate report success without executing -anything, and replayed the previous build's console log so the lie -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. +and it is verified green by cache-defeated container runs +(`docker build --no-cache-filter=lint --no-cache-filter=builder`). The +CI status is not independently claimed here: a superseded run is +recorded as `skipped` and still rolls up green, so a commit status on +`next` does not by itself evidence an executed check (#152). Before +#119, a warm layer cache also let the gate report success without +executing anything, and replayed the previous build's console log so +the lie 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 @@ -190,7 +194,9 @@ rate-limit keys should bucket by `/64`). - OpenAPI specification - Analytics dashboard: success rates, response times, volume - A remember-me option at login -- Password change and reset flow +- Password reset flow for a forgotten password. The authenticated + password *change* flow already landed on `main` (#65); reset does not + exist - Later, nice to have - email delivery target type - SNS and S3 delivery targets diff --git a/internal/config/config.go b/internal/config/config.go index 073f97b..a0413b6 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -422,33 +422,43 @@ func loadFromEnv() (*Config, error) { }, nil } -// warnSharedRateLimitBucket logs a startup warning when a production -// deployment leaves TRUSTED_PROXIES empty. +// warnSharedRateLimitBucket logs a startup warning whenever +// TRUSTED_PROXIES is empty, in any environment. // // 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 +// peer's address. Whether that is harmless or dangerous depends on +// what is in front of the process, which this code cannot observe: +// with nothing in front, the peer is the client and the limits are +// per-client as intended; behind a reverse proxy the peer is the 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. +// keep it full, which denies the only administrative login to everyone +// until the process restarts. +// +// The warning is deliberately not gated on WEBHOOKER_ENVIRONMENT. That +// variable defaults to dev, so gating on it would silence the warning +// for exactly the operator who forgot to configure the deployment — +// the case it exists to catch. // // 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 { + if 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.", + "TRUSTED_PROXIES is empty: every rate limit keys on the "+ + "connecting peer's address. With nothing proxying to "+ + "this process that is the client itself and the limits "+ + "are per-client as intended. Behind a reverse proxy the "+ + "peer is the proxy on every request, so all clients "+ + "share one bucket per limit and any remote client can "+ + "keep the login limit full, denying the admin login — "+ + "the only administrative path — until restart. If "+ + "anything proxies to this process, set TRUSTED_PROXIES "+ + "to its address.", "environment", c.Environment, "trustedProxies", len(c.TrustedProxies), ) @@ -491,6 +501,10 @@ func New(lc fx.Lifecycle, params ConfigParams) (*Config, error) { "maintenanceMode", s.MaintenanceMode, "dataDir", s.DataDir, "retentionSweepInterval", s.RetentionSweepInterval.String(), + // Logged because a perfectly valid non-positive value here + // disables idle expiry entirely, and that is worth showing + // back to the operator. + "sessionIdleTimeout", s.SessionIdleTimeout.String(), "receiverRateLimit", s.ReceiverRateLimit, "trustedProxies", len(s.TrustedProxies), "hasSentryDSN", s.SentryDSN != "", diff --git a/internal/config/config_test.go b/internal/config/config_test.go index d6558f6..11b521b 100644 --- a/internal/config/config_test.go +++ b/internal/config/config_test.go @@ -628,10 +628,12 @@ func testTrustedProxiesSuccess( } // 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. +// tells an operator a deployment behind a reverse proxy shares one +// rate-limit bucket between every client, which makes the admin login +// remotely deniable. It must fire whenever TRUSTED_PROXIES is empty, +// in any environment: WEBHOOKER_ENVIRONMENT defaults to dev, so gating +// on it would silence the warning for exactly the operator who never +// configured the deployment. It stays quiet once proxies are named. func TestSharedRateLimitBucketWarning(t *testing.T) { tests := []struct { name string @@ -651,12 +653,19 @@ func TestSharedRateLimitBucketWarning(t *testing.T) { 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", + // The default environment. An internet-exposed + // deployment whose operator never set + // WEBHOOKER_ENVIRONMENT lands here and has exactly + // the exposure the warning announces. + name: "dev without trusted proxies warns", environment: config.EnvironmentDev, - expectWarning: false, + expectWarning: true, + }, + { + name: "dev with trusted proxies is quiet", + environment: config.EnvironmentDev, + trustedProxies: cidrPrivateV4, + expectWarning: false, }, } @@ -697,8 +706,14 @@ func TestSharedRateLimitBucketWarning(t *testing.T) { 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") + assert.Contains(t, logged, "share one bucket") + assert.Contains(t, logged, "denying the admin login") + // The text must stay accurate for a developer with + // nothing in front of the process, where an empty + // list costs nothing. + assert.Contains( + t, logged, "nothing proxying to this process", + ) }) } }