aace4d77a9bff0afbcce00e8ee14d06348472c76
8 Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
| aace4d77a9 |
Bound every slog line against client-chosen text (closes #176)
Some checks failed
check / check (push) Failing after 2m4s
MaxBodySize logged r.URL.Path untruncated at WARN, and routes.go registers it ahead of RequireAuth, so an unauthenticated POST /source/<8 KB>/edit with an oversize declared Content-Length wrote attacker-chosen text of attacker-chosen length into the operator's log, for the cost of a request with no body. The 2,560-byte per-line budget from #146 did not reach it: that budget lives in the access log's field capping and this is a separate slog call. The capping mechanism moves out of internal/middleware into internal/logfield so there is one budget and one implementation rather than a second ad-hoc truncation. Truncate and EncodedBytes are unchanged; the access log now spends logfield.MaxBytes where it spent maxLogFieldBytes. The sweep the issue asked for found five more call sites of the same shape, all reachable unauthenticated, all now capped: the CSRF 403 (also registered ahead of RequireAuth), the rate limiters' 429 (the per-entrypoint receiver limiter is unauthenticated), RequireAuth's own DEBUG line, the unknown-entrypoint DEBUG line on the receiver, and the failed-login DEBUG lines. DEBUG being off by default is not a bound: an operator turning it on to diagnose a flood must not thereby hand the flood an unbounded write. Every other slog call in the tree was read and judged; the PR body lists all of them, including the ones left alone and why. Two further sites arrived in next with #171 after the first sweep was written and are capped here as well: "login failure limit exceeded" in loginguard.go and "password verification capacity exhausted" in handlers/auth.go, both WARN on the unauthenticated login POST. Neither was ever wide — chi routes that POST on a static pattern, so r.URL.Path is the 12-byte constant /pages/login and each line lands near 120 bytes, and removing either cap breaks no test. They are capped because RecordLoginFailure is exported and takes any *http.Request, so the bound rests on a routing invariant nobody wrote down, and because the same message at handlers/profile.go logs no path at all. MaxBodySize stays ahead of RequireAuth. An oversize body should be refused before the request buys a cookie decrypt and a session load, and rejecting first is what keeps an unauthenticated flood from choosing how much session work the process does. The ordering and what it costs are now written at the registration, on maxFormBodySize. MaxAccessLogLineBytes is restated as the ceiling on every slog line carrying text an UNAUTHENTICATED client supplies, not just the access log's: each of these lines carries strictly fewer client-supplied fields than the access log does, so none can be wider. That is asserted per line under both handlers rather than argued. The claim is qualified rather than universal because three kinds of writer are outside it, and the README and the constant now name all three: lines carrying an authenticated operator's own input, which are not truncated at all (the webhook name on "webhook created" reaches 600 KB on one line from a 100 KB form field, measured; the SSRF-rejection url and the target_name lines are the same shape) and are left uncapped deliberately, since truncating the operator's own configuration echoed back costs debuggability against no adversary; the log delivery target, which exists to emit the whole event; and GORM's default logger, which prints the interpolated SQL to stdout on a record-not-found and is unbounded on the receiver and login lookups. That last one is a real defect this audit turned up and is filed separately as #178, not fixed here. Tests drive 8 KB of client-chosen text at all six sites, through both handlers internal/logger can install and through each character they escape — including a bare C0 control, which costs six bytes on the line against the one it cost to send and is the case a raw-byte budget breaks on first. Each holds the encoded line to the ceiling, holds the whole flood's output to what that ceiling allows, and asserts the markers at the far end of the input are absent, so a value that merely happened to be short cannot pass. The two login lines past the username lookup, capped for uniformity rather than need, are pinned too. internal/logfield gains a test that measures the per-rune charge against what the handlers really emit over roughly 3,000 code points on each, so an undercharged rune fails a test instead of quietly falsifying the ceiling. Verified by mutation: reverting the MaxBodySize cap alone fails 28 subtests with a 16,583-byte line against the 2,560 ceiling; reverting the other five fails 70; uncapping either of the two login lines past the username lookup fails both handlers on its own, so those two are independently pinned rather than jointly; budgeting raw bytes instead of encoded ones fails 23 across three packages. The two login-throttle WARN caps are the exception and are recorded as such: reverting them fails nothing, because the constant path gives the mutation nothing to widen. |
|||
| 977fe87588 |
Verify login credentials before spending rate-limit budget (closes #150)
All checks were successful
check / check (push) Successful in 2m46s
In the shipped default, any stranger denied the operator the only administrative path at 5 requests per minute: TRUSTED_PROXIES is empty, the README requires a reverse proxy, so every login POST shared one bucket keyed on the proxy. Credentials are now verified first and only a FAILED attempt spends budget, so a correct password is never throttled. Failures are counted per (client bucket, submitted username), bounded. Concurrent Argon2id verifications are capped at two, and the queue for them at 16 — because verifying first lets an attacker force a 64 MB hash per request, and bounding the wait alone bounds nothing. The issue's own recommendation was insufficient and is rejected here: keying by username stops an attacker locking out a DIFFERENT account, but this is a single-admin product with a predictable bootstrap username, so flooding the operator's own name still locks them out. This is speculative — it implements a corrected recommendation ahead of the owner's ruling so the decision can be made by merging or reverting. Three things are disclosed rather than glossed: online guessing rises from 5/min to roughly 27/s, because the 429 is a label on the response and not a gate in front of the hash; the residual exposure is a loss of login AVAILABILITY, not latency, and a determined flood still denies login while it runs, at ~400x the cost and clearing the moment it stops; and the endpoint should be provisioned for ~400 MB resident, not the 203 MB of live commitment it itemises. Independently reviewed four times. Reviewers disproved the suspected FIFO starvation by measurement, then caught two successive memory bounds the code did not have — the second by parking waiters and reading the heap rather than checking the arithmetic. |
|||
| 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> |
