From 39064a3d6c5316f00d31d7da691172e2882b65eb Mon Sep 17 00:00:00 2001 From: clawbot Date: Mon, 17 Aug 2026 23:44:59 +0200 Subject: [PATCH] Correct release-blocking README and startup-warning inaccuracies (closes #151) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Publishing this README would have shipped false statements about the product. Corrects the eight items on the issue plus everything a full sweep turned up: the Slack circuit-breaker scope, a nonexistent WAL, the wrong config key for slack targets, six undocumented routes, the conditional /metrics registration, wrong retention bands, wrong shutdown mechanism, and a Quick Start that led a new contributor into a red build. The lockout warning now fires whenever TRUSTED_PROXIES is empty rather than only in production, since the variable it was gated on defaults to dev. Rate-limit keying, the limits and the TRUSTED_PROXIES default are untouched — those belong to #150. What /s/* actually serves was settled empirically rather than by reading: all five of GET/HEAD/POST/PUT/DELETE return 200, pinned by TestStaticServesEveryMethod. Restricting it is filed separately. Independently reviewed after three prior rounds. The reviewer re-derived all fifteen claim-table rows against the code, including every row a previous revision had marked "correct, left alone" and got wrong, and found zero false; then verified every route method-by-method, all twelve environment variables, all nine entity tables, and the package tree against git ls-files. The Quick Start was confirmed by running it in a fresh clone. --- README.md | 353 +++++++++++++++++++++++---------- TODO.md | 22 +- internal/config/config.go | 44 ++-- internal/config/config_test.go | 37 +++- internal/server/routes_test.go | 51 +++++ 5 files changed, 365 insertions(+), 142 deletions(-) diff --git a/README.md b/README.md index 6c940f9..936b002 100644 --- a/README.md +++ b/README.md @@ -11,9 +11,14 @@ 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) +- `curl`, used by `script/fetch-assets` to download the third-party + browser assets, which are not committed (`make bootstrap` installs + it if missing) ### Quick Start @@ -22,14 +27,18 @@ with retry support, logging, and observability. Category: infrastructure git clone https://git.eeqj.de/sneak/webhooker.git cd webhooker -# Install Go dependencies -make deps +# Install Go dependencies, the pinned linter, and the third-party +# browser assets. `make deps` alone is not enough: it only runs +# go mod download/tidy, and the checks below need the fetched assets. +make bootstrap -# Run all checks (format, lint, test, build) +# Run all checks (test, lint, format check) 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 @@ -42,13 +51,18 @@ make bootstrap # Install all dependencies (idempotent) make setup # Bootstrap + install git pre-commit hook make assets # Fetch + verify third-party browser assets make fmt # Format code (gofmt + goimports) +make fmt-check # Fail if gofmt would change anything (writes nothing) 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 @@ -90,7 +104,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 | `""` | @@ -130,8 +144,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 @@ -163,6 +182,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: @@ -232,17 +253,20 @@ 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 This repository adheres to the [Scripts to Rule Them All](https://github.com/github/scripts-to-rule-them-all) standard: normalized scripts in `script/` are the entrypoints for the -development workflow, and the Makefile targets are thin shims that call -them. We provide: +development workflow. Ten of the Makefile's sixteen targets are thin +shims that call them; `build`, `run`, `dev`, `deps`, `clean` and `css` +are inline commands with no script behind them. We provide: - `script/bootstrap` — install all dependencies (idempotent) - `script/setup` — make a fresh clone ready for development @@ -354,7 +378,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 @@ -372,7 +400,7 @@ The codebase uses consistent naming throughout (rename completed in ### Data Model -webhooker's data model has eight entities organized into two tiers: the +webhooker's data model has nine entities organized into two tiers: the **application tier** (user and webhook configuration) and the **event tier** (event ingestion, delivery, and logging). @@ -464,9 +492,25 @@ 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: + +- `1` 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. +- `0`, and `365000` or above, are accepted and mean retain forever, + collapsing to the sentinel — `0` in `Webhook.BeforeSave`, the large + values in `parseRetentionDays`. The large values are 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". + +A negative value is in none of the three: `parseRetentionDays` rejects +it with a 400 before `BeforeSave` ever sees it. + +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 @@ -484,7 +528,7 @@ the full request and creates an Event. | -------------- | ------- | ----------- | | `id` | UUID | Primary key | | `webhook_id` | UUID | Foreign key → Webhook | -| `path` | string | Unique URL path (UUID-based, e.g. `/webhook/{uuid}`) | +| `path` | string | Unique bare UUID, generated at creation. The `/webhook/` prefix is route only and is not stored: the receiver matches this column against the raw `{uuid}` path segment | | `description` | string | Optional description | | `active` | boolean | Whether this entrypoint accepts events (default: true) | @@ -508,8 +552,8 @@ events should be forwarded. | `type` | TargetType | One of: `http`, `slack`, `database`, `log` | | `active` | boolean | Whether deliveries are enabled (default: true) | | `config` | JSON text | Type-specific configuration | -| `max_retries` | integer | Maximum retry attempts for HTTP targets (0 = fire-and-forget, >0 = retries with backoff) | -| `max_queue_size` | integer | Maximum queued deliveries (for HTTP targets with retries) | +| `max_retries` | integer | Maximum retry attempts for `http` and `slack` targets (0 = fire-and-forget, >0 = retries with backoff and a circuit breaker). Ignored by `database` and `log` targets | +| `max_queue_size` | integer | Stored and shown on the target's detail view, but not enforced anywhere yet: nothing in the delivery engine consults it. Queue depth is set by the two fixed 10,000-entry channels | **Relations:** Belongs to Webhook. Has many Deliveries. @@ -522,6 +566,11 @@ events should be forwarded. greater than 0, failed deliveries are retried with exponential backoff up to `max_retries` attempts, protected by a per-target circuit breaker. +- **`slack`** — Post the event as a formatted message to a + Slack-compatible incoming webhook URL (`webhookUrl` in `config`). It + is built on the same HTTP core as `http` and honours `max_retries` + identically, circuit breaker included. See the Slack target section + under "Per-Webhook Event Databases" for the message format. - **`database`** — Archive the full event as a row into a separate per-webhook archive database (`archive-{webhookID}.db`) for long-term retention, with an optional creation-validated expiry (default: keep @@ -558,7 +607,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 | @@ -613,7 +662,9 @@ retries) is individually logged for full observability. #### Common Fields -All entities include these fields from `BaseModel`: +Every entity except `Setting` includes these fields from `BaseModel`. +`Setting` is a bare key-value row with no `id`, no timestamps and no +soft delete: | Field | Type | Description | | ------------ | --------- | ----------- | @@ -659,7 +710,7 @@ handles connection pooling, lazy opening, migrations, and cleanup. This separation provides: - **Isolation** — a high-volume webhook won't cause lock contention or - WAL bloat affecting the main application or other webhooks. + journal growth affecting the main application or other webhooks. - **Independent lifecycle** — event databases can be independently backed up, archived, rotated, or size-limited without impacting the application. @@ -669,9 +720,12 @@ This separation provides: - **Per-webhook retention** — the `retention_days` field on each webhook controls automatic cleanup of old events in that webhook's database only, or disables cleanup entirely when set to `0` (retain forever). -- **Performance** — each webhook's database has its own WAL, its own - page cache, and its own lock, so concurrent event ingestion across - webhooks won't contend. +- **Performance** — each webhook's database has its own page cache and + its own lock, so concurrent event ingestion across webhooks won't + contend. No write-ahead log is involved: both DSNs are + `file:{path}?cache=shared&mode=rwc` and no `journal_mode` pragma is + ever issued, so every database runs on SQLite's default rollback + journal. The **database target type** builds on this architecture to provide long-term archiving, separate from the per-webhook event database (which @@ -727,8 +781,9 @@ and other compatible services). Each message includes event metadata pretty-printed in a code block. JSON payloads are automatically formatted with indentation for readability; non-JSON payloads are shown as raw text. Large payloads are truncated to keep messages reasonable. -Config stores `webhook_url` — the Slack/Mattermost incoming webhook -endpoint. +Config stores `webhookUrl` — the Slack/Mattermost incoming webhook +endpoint. That is the JSON key; the error text for a missing one reads +`webhook_url is required`, which is the message, not the key. The database uses the [modernc.org/sqlite](https://pkg.go.dev/modernc.org/sqlite) driver at @@ -750,8 +805,9 @@ External Service 1. Look up Entrypoint by UUID 2. Capture full request as Event 3. Create Delivery records for each active Target - 4. Build self-contained DeliveryTask structs - (target config + event data inline for ≤16KB) + 4. Build self-contained delivery.Task structs + (target config + event data inline for + bodies < 16 KiB) 5. Notify Engine via channel (no DB read needed) │ ▼ @@ -786,7 +842,7 @@ at any time, preventing goroutine explosions regardless of queue depth. a delivery channel (new tasks from the webhook handler) and a retry channel (tasks from backoff timers). Both are buffered to 10,000. - **Fan-out via channel, not goroutines:** When an event arrives with - multiple targets, each `DeliveryTask` is sent to the delivery channel. + multiple targets, each `delivery.Task` is sent to the delivery channel. Workers pick them up and process them — no goroutine-per-target. - **Worker goroutines:** A fixed number of worker goroutines select from both channels. Each worker processes one task at a time, then picks up @@ -810,7 +866,12 @@ This means: - **Independent results** — each worker records its own delivery result in the per-webhook database without coordination. - **Graceful shutdown** — cancel the context, workers finish their - current task and exit. `WaitGroup.Wait()` ensures clean shutdown. + current task and exit. The stop hook waits for the pool via + `lifecycle.WaitForShutdown`, which bounds that wait by fx's stop + timeout rather than blocking forever on a wedged worker. On timeout + it logs at `ERROR` and returns an error, and the goroutines that + did not finish are still running — an unclean shutdown is reported + rather than hidden. **Recovery paths:** @@ -838,12 +899,13 @@ remains stored in the per-webhook event database, there is no way to redeliver it: manual redelivery is planned, not implemented (see [TODO.md](TODO.md)). -### Circuit Breaker (HTTP Targets with Retries) +### Circuit Breaker (HTTP and Slack Targets with Retries) -HTTP targets with `max_retries` > 0 are protected by a **per-target circuit breaker** that -prevents hammering a down target with repeated failed delivery attempts. -The circuit breaker is in-memory only and resets on restart (which is -fine — startup recovery rescans the database anyway). +`http` and `slack` targets with `max_retries` > 0 are protected by a +**per-target circuit breaker** that prevents hammering a down target +with repeated failed delivery attempts. The circuit breaker is +in-memory only and resets on restart (which is fine — startup recovery +rescans the database anyway). **States:** @@ -879,10 +941,12 @@ fine — startup recovery rescans the database anyway). - **Failure threshold:** 5 consecutive failures before opening - **Cooldown:** 30 seconds in open state before probing -**Scope:** Circuit breakers only apply to **HTTP targets with -`max_retries` > 0**. Fire-and-forget HTTP targets (`max_retries` == 0), -Slack targets, database targets (local operations), and log -targets (stdout) do not use circuit breakers. +**Scope:** Circuit breakers apply to **`http` and `slack` targets with +`max_retries` > 0**. The Slack target is built on the same HTTP core +and hands its own `max_retries` to the same retry path, so it gets a +breaker with the same 5-failure / 30-second defaults. Fire-and-forget +targets of either type (`max_retries` == 0), database targets (local +operations), and log targets (stdout) do not use circuit breakers. When a circuit is open and a new delivery arrives, the engine marks the delivery as `retrying` and schedules a retry timer for after the @@ -898,9 +962,11 @@ unpredictable rates, and blanket limits shared with other routes would cause legitimate deliveries to be dropped. The receiver instead has its own dedicated abuse limit, scoped to the -`/webhook/{uuid}` route only and keyed per client IP per entrypoint: one -misbehaving sender is throttled without affecting other senders of the -same entrypoint or the same sender's other entrypoints. The limit is +`/webhook/{uuid}` route only and keyed per client IP per request path +(`httprate.KeyByEndpoint`): one misbehaving sender is throttled without +affecting other senders of the same entrypoint or the same sender's +other entrypoints. Keying on the path rather than on the entrypoint +matters — see the aggregate limit below. The limit is `RECEIVER_RATE_LIMIT` requests per minute (default 120, generous for legitimate webhook senders). Requests over the limit receive HTTP 429 with a `Retry-After` header. A set-but-invalid `RECEIVER_RATE_LIMIT` @@ -958,8 +1024,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 @@ -971,17 +1037,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`) | +| any | `/s/*` | Static file serving (embedded CSS, JS). Mounted for every method, not just `GET`/`HEAD`: chi's `Mount` registers all methods and `http.FileServer` special-cases only `HEAD` (by omitting the body), so a `POST` or `DELETE` to an asset is answered `200` with the file. Pinned by `TestStaticServesEveryMethod` | +| `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 @@ -989,6 +1055,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 | @@ -998,13 +1065,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) @@ -1018,8 +1089,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 @@ -1047,16 +1120,30 @@ 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) +│ │ ├── testing.go # NewTestDatabase: wrapper for tests, no fx lifecycle │ │ └── 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 +│ │ ├── circuit_breaker.go # Per-target circuit breaker for http/slack 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 +│ │ ├── event_log_view.go # Event log projection, byte-capped in SQL │ │ ├── healthcheck.go # Health check handler │ │ ├── index.go # Index page handler │ │ ├── profile.go # User profile handler @@ -1069,20 +1156,27 @@ webhooker/ │ ├── middleware/ │ │ ├── middleware.go # Logging, CORS, Auth, Metrics, MetricsAuth, SecurityHeaders, MaxBodySize │ │ ├── csrf.go # CSRF protection middleware (gorilla/csrf) -│ │ └── ratelimit.go # Per-IP rate limiting middleware (go-chi/httprate) +│ │ ├── ratelimit.go # Per-IP rate limiting middleware (go-chi/httprate) +│ │ └── testing.go # NewForTest: Middleware without the fx lifecycle │ ├── server/ │ │ ├── server.go # Server struct, fx lifecycle, signal handling │ │ ├── http.go # HTTP server setup with timeouts │ │ └── routes.go # All route definitions │ └── session/ -│ └── session.go # Cookie-based session management +│ ├── session.go # Cookie-based session management +│ └── testing.go # NewForTest: Session without the fx lifecycle ├── 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/app.js # Progressive-enhancement copy-to-clipboard +│ ├── js/alpine.min.js # Alpine.js, fetched by script/fetch-assets, not committed +│ └── vendor.sha256 # Pinned hashes the fetched assets are verified against +├── templates/ # Go HTML templates (base, login, sources, etc.) +├── script/ # Scripts to Rule Them All entrypoints +├── Dockerfile # Three stages: lint, test+build, Alpine runtime +├── Makefile # 10 of 16 targets shim script/; 6 are inline ├── go.mod / go.sum └── .golangci.yml # Linter configuration ``` @@ -1098,21 +1192,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 under 16 KiB, `delivery.MaxInlineBodySize`), so the +engine can deliver without reading from any database — it only writes +to record results. ### Middleware Stack @@ -1138,16 +1238,32 @@ 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 global entries listed above all run first, so +such a request is still logged and given the security headers — and +counted in the metrics, on a deployment where `METRICS_USERNAME` is +set and the Metrics middleware is therefore registered at all. 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`). @@ -1179,33 +1295,53 @@ 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) - Container runs as non-root user (UID 1000) -- GORM soft deletes on all entities (data preserved for audit) +- GORM soft deletes on every entity that carries `BaseModel`, which is + all of them but `Setting` (data preserved for audit) ### 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), runs `script/fetch-assets` + to download and verify the third-party browser assets, 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. Only `script/cibuild` and +`script/docker` involve Docker. `script/lint`, and therefore +`make lint` and `make check`, run whatever `golangci-lint` is on the +host, which can be a different version from the pinned one — so the +container is the authoritative lint result +([issue #109](https://git.eeqj.de/sneak/webhooker/issues/109) tracks +routing local linting through it as well). #### CI gate honesty @@ -1221,16 +1357,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", + ) }) } } diff --git a/internal/server/routes_test.go b/internal/server/routes_test.go index 1c5f72d..39bca38 100644 --- a/internal/server/routes_test.go +++ b/internal/server/routes_test.go @@ -24,6 +24,7 @@ import ( "sneak.berlin/go/webhooker/internal/middleware" "sneak.berlin/go/webhooker/internal/server" "sneak.berlin/go/webhooker/internal/session" + "sneak.berlin/go/webhooker/static" ) // csrfCookieName is the cookie gorilla/csrf issues when it runs. Its @@ -246,6 +247,56 @@ func (e *testEnv) storedHash(t *testing.T, username string) string { return user.Password } +// --- /s static group --- + +// TestStaticServesEveryMethod pins what the static mount actually +// answers. chi's Mount registers the handler for all methods and +// http.FileServer only special-cases HEAD (by suppressing the body), +// so a POST or a DELETE to an asset is served the file rather than +// refused. The README documents this; the test is what keeps the two +// from drifting. +func TestStaticServesEveryMethod(t *testing.T) { + t.Parallel() + + env := newTestEnv(t) + + body, err := static.Static.ReadFile("js/app.js") + require.NoError(t, err) + require.NotEmpty(t, body) + + for _, method := range []string{ + http.MethodGet, + http.MethodHead, + http.MethodPost, + http.MethodPut, + http.MethodDelete, + } { + t.Run(method, func(t *testing.T) { + t.Parallel() + + req := httptest.NewRequestWithContext( + context.Background(), method, + "/s/js/app.js", nil, + ) + w := httptest.NewRecorder() + env.router.ServeHTTP(w, req) + + assert.Equal(t, http.StatusOK, w.Code, + "static mount answers every method") + + if method == http.MethodHead { + assert.Empty(t, w.Body.Bytes(), + "HEAD must not carry a body") + + return + } + + assert.Equal(t, body, w.Body.Bytes(), + "the asset itself is returned") + }) + } +} + // --- /pages group --- // TestPagesLogin_OversizeBody_RejectedBeforeCSRF proves the cap runs