6fce522016196331ef8817d92bfa358b953ee579
7 Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
| 6fce522016 |
Verify login credentials before spending rate-limit budget (closes #150)
All checks were successful
check / check (push) Successful in 2m45s
With TRUSTED_PROXIES empty behind the reverse proxy production is required to run behind, every login POST keyed on the proxy's address and shared one 5/minute bucket. A stranger sending five POSTs a minute -- 0.08 requests per second, from anywhere -- kept that bucket permanently full, and the operator's own correct password was answered 429 indefinitely with no second administrative path. The login POST no longer has a pre-emptive limiter. The handler verifies credentials first and spends budget only on a FAILED attempt, so a correct password is never throttled whatever the counters hold. Three things follow, and are implemented together because the first is unsafe without the other two: - Failures are counted per (client bucket, submitted username), five per minute, after which further failures get 429 with a Retry-After. A successful login clears the counter, so mistyping and then succeeding does not leave the operator throttled. - Both key sets are capped at 1024 entries. The submitted username is attacker-controlled, so past the first cap failures fall back to a counter keyed on the client alone, and past both caps a failure is answered as throttled without being recorded. Tracked state stays under half a megabyte and does not grow with invented usernames. - Concurrent Argon2id verifications are capped at two, a 128 MB ceiling at 64 MB per hash, and the queue for those slots is capped at 16 waiters. Every password-hashing endpoint takes a slot, including the password-change endpoint, which holds one across both its hashes. A request that waits five seconds without a slot is answered 503, and one that arrives with the queue already full is shed with 503 immediately rather than joining it. Bounding the wait alone would not bound memory, and the queue depth is sized from what a parked waiter measurably retains rather than from the 1 MB body cap, which bounds only the raw body read. The body-cap, CSRF and form-parsing middleware all run before the guard, so a waiter holds its parsed form plus its request header block for the whole wait. Measured on the pinned go1.26.1 toolchain as the HeapAlloc delta across two GCs with 64 waiters parked in the handler: an ordinary two-field login form retains ~0 MB, a 1 MB urlencoded body at Go's 10,000-parameter parse cap retains 2.82 MB (3.09 MB with %41 escapes), and the ~0.9 MB of headers the 1 MB header cap allows takes it to 4.18 MB. The retained parse and the header block dominate, not the raw body. So 16 waiters: 16 x 4.18 MB is about 67 MB of committed queue memory, and two slots drain a full 16-deep queue in about 0.6 s, far inside the deadline. Peak commitment for the endpoint is about 203 MB — 128 MB of Argon2id plus the 18 requests holding a parsed form, 16 queued and the 2 being hashed, at about 75 MB. That 203 MB is live commitment, not resident size: the Go collector lets the heap reach roughly twice the live set before collecting, and the review measured a peak HeapAlloc of 392 MB under 18 adversarial requests, so the README says to provision on the order of 400 MB. An unknown username is verified against a dummy hash instead of returning early, so a nonexistent account costs the same time as a real one and the response cannot be used to enumerate usernames. The password-change limiter is unchanged: RequireAuth runs ahead of it, so only a request already carrying a valid session reaches its bucket. Two consequences are documented rather than fixed, because they follow from the shape the issue asks for. Online guessing throughput rises from 5 a minute to roughly 27 a second, about 2.3 million a day: the credential check always precedes the counter, so the 429 is a label on the response rather than a gate in front of the hash, and what bounds brute force is the semaphore. And under a sustained flood the residual exposure is a loss of login availability, not merely of latency -- above about 27 requests a second most attempts are shed with 503, so a determined flood still denies login for as long as it runs. It costs roughly 400x more to run, nothing accumulates, and the first attempt after it stops succeeds. Restarting the service does not help: the counters a restart clears are not what is saturated. Also adds the missing test for the third bucketKey call site, where the peer is a trusted proxy but the forwarded chain names no client. Every existing test of that fallback uses an IPv4 proxy, where bucketKey is the identity function, so dropping the /64 masking there left the suite green. README and the TRUSTED_PROXIES startup warning updated: a shared bucket now costs precision, not the availability of the admin path. |
|||
| 279effb4c2 |
Bound the event log's rendered bodies in the query (closes #135)
All checks were successful
check / check (push) Successful in 3m0s
The event log rendered stored bodies untruncated. Since buffered rendering landed (#123) that became resident memory per concurrent viewer, up to tens of MB, driven by payloads unauthenticated clients supply to the public receiver. Bound in the query rather than the template, via substr(cast(body as blob), 1, ?) plus length(cast(body as blob)), so an oversized body never becomes a Go string at all. Adds an EventLogView projection carrying the true byte count, and trims a partial UTF-8 tail without rewriting bodies that are merely invalid UTF-8. Independently reviewed. The generated SQL was dumped under GORM DryRun to confirm the cap is a bound parameter, both casts are present, and no other path selects the full column; soft-delete scope, ordering and pagination are unchanged. Correction to the PR body: its quoted mutation output was produced by removing the bound from eventLogColumns, not by raising the cap to 1<<30 as the text claimed. The reviewer reproduced the real mutation and confirmed the tests do catch removal of the bound. Follow-up #157 restores in-app retrieval of bodies above the cap. |
|||
| 0b457ea713 |
Render templates via a buffer, not the ResponseWriter (closes #123)
Some checks failed
check / check (push) Superseded by a newer commit; never tested
|
|||
| 734606b7af |
Update golangci-lint to v2.12.2 with canonical config (#86)
All checks were successful
check / check (push) Successful in 3s
Bumps golangci-lint from v2.11.3 to v2.12.2 and adopts the canonical lint config. ## Version pins - `Dockerfile`: `golangci/golangci-lint:v2.12.2` Debian image, pinned by digest, dated `2026-08-07` - `script/bootstrap`: `GOLANGCI_LINT_VERSION=2.12.2` with updated sha256 pins for the `linux-amd64` and `linux-arm64` release archives ## Config `.golangci.yml` replaced with the canonical config. The previous file kept `lll`/`funlen`/`cyclop`/`dupl` settings under the top-level `linters-settings` key, which the v2 schema ignores; the canonical config nests them under `linters.settings`, so those thresholds now actually apply. The unsupported `issues.exclude-use-default` key was dropped. ## Lint fixes (32 findings) - `lll` (7): wrapped or shortened over-length lines (struct tag comments moved above fields, test logger construction split, `session.NewForTest` signature wrapped, shortened a `#nosec` comment) - `goconst` (17): replaced repeated `"POST"`/`"PUT"` literals with `http.MethodPost`/`http.MethodPut`, added shared test constants for `webhooker-test`/`test`/`application/json`, and added `tmplKeyError`/`tmplKeyWebhook` constants for template data keys in `internal/handlers` - `dupl` (8): merged `buildHTTPTargetConfig` and `buildSlackTargetConfig` into a parameterized `buildURLTargetConfig`; removed the duplicate `iWebhookDB` test helper in favor of `testWebhookDB`; extracted shared helpers in middleware and session tests No `//nolint` directives were added and behavior is unchanged. `make check` (fmt-check, tests, lint) passes. Note: golangci-lint v2.12 deprecates the `gomodguard` linter in favor of `gomodguard_v2`; the canonical config change for that is left for a future coordinated update. Co-authored-by: sneak <sneak@sneak.berlin> Reviewed-on: #86 Co-authored-by: clawbot <clawbot@noreply.example.org> Co-committed-by: clawbot <clawbot@noreply.example.org> |
|||
| ee7c626071 |
Implement the database archiving target (closes #43) (#84)
All checks were successful
check / check (push) Successful in 4s
Implements the `databaseTarget` as a real archiving target, replacing the always-successful stub. Delivering to a `database` target now writes the full event into a per-webhook archive SQLite file for long-term storage.
## Archive-writer semantics
- **Separate file:** each webhook's full events are written as rows into `archive-{webhookID}.db` under the data dir, distinct from the per-webhook event DB (`events-{webhookID}.db`). The file and its schema are created on first write if missing. Each row carries the full event: body, headers, method, content type, webhook id, entrypoint id, event id, and an archived-at timestamp.
- **Close/reopen with debounce:** after each write the archive handle is closed and reopened, unless the last (re)open was less than one second ago. This lets an operator move the archive file away for offline archiving while bounding file churn under load. A per-webhook `archiveWriter` owns this debounce state and serialises writes.
- **Auto-recreate:** the file is opened create-if-missing (`mode=rwc`) and its schema re-migrated on every open, so if the archive was moved or removed since the last open, the next write recreates it. The writer also detects a missing file before writing and reopens first, so a moved-away file is recreated rather than lost.
- **Optional expiry, validated at creation:** an optional `expiry` in the target's config JSON (e.g. `{"expiry":"720h"}`) is validated when the target is created (`ValidateArchiveExpiry`; bad values are rejected with a 400 at the add-target form, the Slack URL precedent). The default (missing, empty, or `"never"`) keeps rows forever with no pruning. When a positive duration is set, rows older than it (measured from each row's archived-at time) are pruned on every (re)open; because the file is reopened after writes, prune-on-open keeps the archive swept without a separate background sweeper. A set-but-invalid expiry in a stored config (unparseable, zero, or negative) is an error at delivery time too — never a silent default.
- **No-retry, fail-loud:** the target performs a single attempt with no retries. On success it records one successful attempt and marks the delivery delivered. If the archive write fails, the attempt is recorded as failed with the error and the delivery is marked failed — archiving errors never report success.
## Scope
- `internal/delivery/target_database.go` — the `databaseTarget` (no-retry) archives via a per-webhook writer registry; an archive error records a failed attempt and marks the delivery failed.
- `internal/delivery/target_database_archive.go` (new) — the `archiveWriter`, the archived-row model, config/expiry parsing (fail-loud on set-but-invalid values), `ValidateArchiveExpiry`, and prune-on-open.
- `internal/handlers/source_management.go` — database targets get a creation-validated `expiry` config (`buildDatabaseTargetConfig`); the expiry form value is read where the request body is bounded and bad values are rejected with a 400 at target creation.
- `templates/source_detail.html` — the add-target form shows an expiry field for database targets.
- `README.md` — the database-target documentation describes the archiving semantics.
- `internal/delivery/export_test.go`, `internal/delivery/target_database_test.go`, `internal/handlers` tests — tests and their exported shims.
No changes to the `Target` interface or other targets.
## Tests
- a row is archived (both at the writer level and end-to-end through `Deliver`)
- a forced archive failure (bad stored expiry config) yields a `Failed` delivery with a non-success `DeliveryResult` carrying the error and no archive file created
- the file is recreated after removal, with only the post-removal row
- the one-second reopen debounce (rapid writes reopen once; a write after the window reopens again)
- expiry pruning removes rows older than the configured expiry
- expiry config parsing (empty / `never` / duration accepted; unparseable, zero, and negative values error)
- expiry validation at target creation (`TestValidateArchiveExpiry`; valid values build the config, bad values get a 400)
## Validation
`docker build .` exits 0 (fmt-check, lint, test, build all pass).
Closes #43
Co-authored-by: sneak <sneak@sneak.berlin>
Reviewed-on: #84
Co-authored-by: clawbot <clawbot@noreply.example.org>
Co-committed-by: clawbot <clawbot@noreply.example.org>
|
|||
| 752d6beead |
Validate Slack target URLs at creation time (closes #68) (#73)
All checks were successful
check / check (push) Successful in 4s
Slack delivery targets were only checked by the request-time dialer guard, not at creation, giving them a weaker SSRF gate than HTTP targets. This validates the Slack incoming-webhook URL with `delivery.ValidateTargetURL` in the Slack target creation path (`buildSlackTargetConfig`), before persisting, mirroring the existing HTTP-target path. On failure the create is rejected with the same clear, non-leaking user-facing error the HTTP path uses. Adds handlers-package tests covering both an accepted public URL and a rejected private/reserved URL. Confined to `internal/handlers/`; `internal/delivery/` is unchanged. Closes #68 Co-authored-by: sneak <sneak@sneak.berlin> Co-authored-by: Jeffrey Paul <sneak@noreply.example.org> Reviewed-on: #73 Co-authored-by: clawbot <clawbot@noreply.example.org> Co-committed-by: clawbot <clawbot@noreply.example.org> |
|||
| afe88c601a |
refactor: use pinned golangci-lint Docker image for linting (#55)
All checks were successful
check / check (push) Successful in 5s
Closes [issue #50](#50) ## Summary Refactors the Dockerfile to use a separate lint stage with a pinned golangci-lint Docker image, following the pattern used by [sneak/pixa](https://git.eeqj.de/sneak/pixa). This replaces the previous approach of installing golangci-lint via curl in the builder stage. ## Changes ### Dockerfile - **New `lint` stage** using `golangci/golangci-lint:v2.11.3` (Debian-based, pinned by sha256 digest) as a separate build stage - **Builder stage** depends on lint via `COPY --from=lint /src/go.sum /dev/null` — build won't proceed unless linting passes - **Go bumped** from 1.24 to 1.26.1 (`golang:1.26.1-bookworm`, pinned by sha256) - **golangci-lint bumped** from v1.64.8 to v2.11.3 - All three Docker images (golangci-lint, golang, alpine) pinned by sha256 digest - Debian-based golangci-lint image used (not Alpine) because mattn/go-sqlite3 CGO does not compile on musl (off64_t) ### Linter Config (.golangci.yml) - Migrated from v1 to v2 format (`version: "2"` added) - Removed linters no longer available in v2: `gofmt` (handled by `make fmt-check`), `gosimple` (merged into `staticcheck`), `typecheck` (always-on in v2) - Same set of linters enabled — no rules weakened ### Code Fixes (all lint issues from v2 upgrade) - Added package comments to all packages - Added doc comments to all exported types, functions, and methods - Fixed unchecked errors flagged by `errcheck` (sqlDB.Close, os.Setenv in tests, resp.Body.Close, fmt.Fprint) - Fixed unused parameters flagged by `revive` (renamed to `_`) - Fixed `gosec` G120 warnings: added `http.MaxBytesReader` before `r.ParseForm()` calls - Fixed `staticcheck` QF1012: replaced `WriteString(fmt.Sprintf(...))` with `fmt.Fprintf` - Fixed `staticcheck` QF1003: converted if/else chain to tagged switch - Renamed `DeliveryTask` → `Task` to avoid package stutter (`delivery.Task` instead of `delivery.DeliveryTask`) - Renamed shadowed builtin `max` parameter to `upperBound` in `cryptoRandInt` - Used `t.Setenv` instead of `os.Setenv` in tests (auto-restores) ### README.md - Updated version requirements: Go 1.26+, golangci-lint v2.11+ - Updated Dockerfile description in project structure ## Verification `docker build .` passes cleanly — formatting check, linting, all tests, and build all succeed. Co-authored-by: clawbot <clawbot@noreply.git.eeqj.de> Reviewed-on: #55 Co-authored-by: clawbot <clawbot@noreply.example.org> Co-committed-by: clawbot <clawbot@noreply.example.org> |
