diff --git a/CHANGELOG.md b/CHANGELOG.md index 14fd3dc..b98adfb 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,27 @@ Format follows [Keep a Changelog](https://keepachangelog.com/en/1.1.0/); version ## [Unreleased] +## [2.4.1] - 2026-10-12 + +Documentation only; no product code changes. + +### Added + +- **A full-tree code audit** at `2.4.0`, with its plan of work: + [docs/audit-2026-10-12.md](docs/audit-2026-10-12.md). Six zones read file + by file against what `architecture.md` promises; what the readers could not + settle from source was settled on the test server. Four high findings - the + published release images are stamped `VERSION=e2e` because the e2e gate + rebuilds the image it is about to push, so the backup version check has + never checked anything; a renewed TLS certificate is not picked up while + the container runs; the queue-reconcile sweep can mark delivered mail + `bounced` while the log tailer is blind; application logins are + case-sensitive to the panel and case-insensitive to Postfix - twenty-one + medium, thirty-four low, the doc drift and the test gaps, and what was + checked and found sound. The plan is six stages, each a cut of its own; + nothing in it is agreed yet. The published `1.x` tags and images stay as + they are (owner, 2026-10-12): fixes go forward only. + ## [2.4.0] - 2026-10-12 ### Security diff --git a/README.md b/README.md index b2e7931..a3918c6 100644 --- a/README.md +++ b/README.md @@ -112,7 +112,7 @@ docker run --rm -d --name selfpost-try \ -e SELFPOST_HOSTNAME=mail.local.test \ -e PANEL_COOKIE_SECURE=false \ -v selfpost-try-data:/data \ - ghcr.io/mixeme/selfpost:2.4.0 + ghcr.io/mixeme/selfpost:2.4.1 ``` **Get the setup URL** (pick one): diff --git a/deploy/docker-compose.yml b/deploy/docker-compose.yml index cfc1a1f..cd6ea70 100644 --- a/deploy/docker-compose.yml +++ b/deploy/docker-compose.yml @@ -21,7 +21,7 @@ services: selfpost: - image: ghcr.io/mixeme/selfpost:2.4.0 + image: ghcr.io/mixeme/selfpost:2.4.1 restart: unless-stopped environment: SELFPOST_HOSTNAME: "${SELFPOST_HOSTNAME:?set the mail/panel hostname, e.g. mail.example.com}" diff --git a/docs/audit-2026-10-12.md b/docs/audit-2026-10-12.md new file mode 100644 index 0000000..8f0576b --- /dev/null +++ b/docs/audit-2026-10-12.md @@ -0,0 +1,689 @@ +# Code audit — 2026-10-12 + +**What this is.** A full-tree audit of the code and infrastructure at `8f226bd` +(release `2.4.0`): leftovers, unfinished work, defects, performance, doc drift +and test gaps — explicit ones (there are no `TODO`/`FIXME` markers anywhere in +the product code) and the implicit ones that only reading finds. The panel's +visual design (mockups, `panel.css`, layout) is a separate audit and is not +touched here. Nothing was changed in the tree; this file is the only output. + +**Method.** Six parallel readers, one per zone (mail path; background roles; +store/backup/composition root; HTTP surface; view and services; build/deploy/ +CI/e2e), each reading every file of its zone and its tests in full, each +told to find where the code does not do what `architecture.md` says, and to +prove it (a throwaway test, `EXPLAIN QUERY PLAN`, a probe) rather than guess. +Machine checks: `go build`, `go vet`, `gofmt -l`, `go mod tidy`/`verify`, +`staticcheck`, `govulncheck`, `go test ./...`. Five claims the readers could +not settle from source were settled on the test server +(`selfpost.mixfed.ru`, running a locally built `2.3.0-post.c817f08`) and +against GHCR. Two hypotheses were refuted there and are **not** findings: +`saslpasswd2 -d` of a missing login exits 0 (deletion is idempotent, as the +code says), and a duplicate key in a `texthash:` map is a warning with the +first entry winning, not a fatal error. + +**Machine results.** Build, vet, gofmt, tidy, verify: clean. `staticcheck`: +four items (an unused `domainIDSet` in `handlers/authz.go:63`, two unused +assignments in `handlers_outbound_pages_test.go:174,194`, one `S1003` in +`view/pages_test.go:170`). `govulncheck`: nine standard-library findings +(`net/http` HTTP/2, `net/textproto`, `html/template`), all fixed in the +current Go patch release; `golang.org/x/crypto` `0.54.0` has three published +fixes in `0.56.0` that this code does not call. `modernc.org/sqlite` is seven +minor versions behind (`1.53.0` → `1.60.1`). `go test ./...`: green on Linux +semantics; on Windows two tests fail for OS reasons (`TestWriteLoadPrivateKeyRoundtrip` +expects a POSIX mode; `TestFollowTailsAndRotates` renames an open file) — the +tests, not the product, are platform-bound. + +Severity: **critical** — mail or data lost or the security boundary crossed +today; **high** — a documented guarantee is false in a way an operator will +hit; **medium** — wrong under a realistic condition, or a stated guard that is +not there; **low** — wrong under an unlikely condition, or cost without +failure; **nit** — hygiene. Confidence is stated where a finding was not +proven. + +--- + +## Findings + +### High + +**H1. Every published release image is stamped `VERSION=e2e`, so the +backup/restore version check never checks anything.** +[release.yml:75-84](../.github/workflows/release.yml) builds the image with +`VERSION=X.Y.Z` and tags it `selfpost:e2e`; the e2e step then runs +`docker compose build` through +[test/e2e/compose.override.yml:23-32](../test/e2e/compose.override.yml), +which rebuilds **the same tag** with `args: VERSION: e2e`; `docker tag +selfpost:e2e …X.Y.Z-amd64` pushes the rebuilt one. Confirmed: `docker run +ghcr.io/mixeme/selfpost:1.3.0 panel -version` prints `e2e`. Consequences: +the footer and `-version` lie; `backup.CheckRestore` compares a manifest's +version to `e2e`, so a backup of any release restores into any other and the +promise in development.md:270 and guide.md:191 ("the version is the single +source … so they cannot drift apart") is false. Every image this workflow has +ever published is affected; they stay as they are (owner, 2026-10-12: old tags +and images are history and are not touched) — the fix is for the next +publication. + +**H2. Renewed TLS certificates are never picked up while the container +runs.** [postfix-config.sh:75-89](../build/postfix-config.sh) copies the +mounted cert and key into `/etc/postfix/tls-internal/` at start and points +Postfix at the copy; [postfix-cert-reload.sh](../build/postfix-cert-reload.sh) +runs `postfix reload` daily but nothing re-copies. The copy is the only +`tls-internal` reference in the tree; on the test server the copy carries the +container's start time. Scenario: Let's Encrypt renews at day 60, nobody +restarts, from day 90 ports 465/587 serve an expired certificate. +architecture.md:33,154-155 and supervisord.conf:92-95 say the opposite. + +**H3. The queue-reconcile sweep marks delivered mail `bounced` whenever the +tailer is blind.** Four mechanisms, one consequence (a row the log would have +resolved as `sent` is closed as a failure, which the Outbound log shows and +the notifier believes): +- *Tailer stuck, sweep running.* [logtail.go:559-581](../internal/logtail/logtail.go): + `caughtUp` closes after the first drain and never re-opens; `reconcileLoop` + (:174-191) then runs every 5 min regardless. If `openAt` fails after a + rotation (:573-576, e.g. postlogd recreated the file `0600` — entrypoint.sh:132 + says this happens) the loop logs and retries forever on the old inode while + every new delivery line goes unread; two minutes later `reconcile()` finds the + rows absent from `postqueue -p` (they were delivered) and bounces them. +- *Truncated queue listing read as complete.* + [postfix/queue.go:64-68,81-87](../internal/postfix/queue.go): `Queue()` caps + `postqueue -p` at 4 MiB and appends a text note; `QueueIDs()` parses that + text and returns the first 4 MiB of ids with no error. A deferred backlog of + ~30k messages — the outage the sweep exists for — makes every row past the + cut `bounced` while Postfix still holds the mail. `queuejson.go:48-52` handles + the same cap correctly for the notifier; the two drifted. +- *Rotation drain window.* :567-578 drains the renamed inode once more; postlogd + keeps writing to it until master's reload reaches it (seconds), so lines + written in that window are never read. architecture.md:372-373 claims the + drain covers it. +- *No status guard on close.* `UpdateStatus` ([store/sendlog.go:86-98](../internal/store/sendlog.go)) + is `WHERE queue_id = ? AND to_addr = ?`; the reconcile path can overwrite a + `sent` written an instant earlier, and a reused short queue id to the same + recipient rewrites a months-old row. + +**H4. Application logins are case-sensitive in the panel and case-insensitive +in Postfix; a tenant can take another tenant's envelope sender.** +[app/validate.go:35-48](../internal/app/validate.go) accepts `A-Z` and nothing +lowercases a login; `applications.login` is `UNIQUE` under BINARY collation +([0001_init.sql:91](../internal/store/migrations/0001_init.sql)), so `Ops` and +`ops` are two applications. The sender map is written as stored +([postfix/postfix.go:95-115](../internal/postfix/postfix.go)); Postfix's +`reject_sender_login_mismatch` compares logins without regard to case (the +code's own comment, [milter/sender.go:35-36](../internal/milter/sender.go), +and architecture.md:239-240 say so). Scenario: domain A has `Ops` bound to +`@a.com`; a *domain*-role administrator of B creates `ops` in B, authenticates +as `ops`, sends `MAIL FROM:` with `From: y@b.com`. Postfix accepts +the envelope (`@a.com Ops` matches), the sender check judges only the header, +which is B's — the message leaves with A as its SPF and bounce identity. +Confidence: the panel side is confirmed; Postfix's compare is taken from the +code's own statement. Fix shape: lower-case at validation, `COLLATE NOCASE` +on the column (a migration — check the live registry for a pair first). + +### Medium + +Persistence, restore, backup + +**M1. The restore manifest is consumed before anything that can fail.** +[backup.go:436-446](../internal/backup/backup.go) deletes `manifest.json` +inside `CheckRestore`; [main.go:282-300](../cmd/panel/main.go) runs +`store.Open` and `resyncAfterRestore` afterwards. If either fails, supervisord +restarts the panel with `restored == false` and the one-time drift-healing +Resync never runs. Docs say "on the first *successful* boot"; the code says +"on the first attempt". `restore_test.go` passes `testNoReload=true` and never +fails Open/Resync, so nothing covers it. + +**M2. Three queries on the single connection run without the index they +need.** Measured with `EXPLAIN QUERY PLAN` against the real migrations: +- `CountMessages` by domain ([ratelimits.go:186-190](../internal/store/ratelimits.go)) + and `sendStats` ([stats.go:77-94](../internal/store/stats.go)) use + `idx_send_log_domain (domain)` and apply `created_at >=` by scanning every + row of the domain over the retention window — on every `MAIL FROM` of an + application under a domain-level limit, with the milter, tailer and HTTP + queued behind it. The baseline's comment promises a composite index for + "the journal milter … on every MAIL FROM"; only the `app_login` half exists. +- `ListQueuedOlderThan` ([sendlog.go:117-122](../internal/store/sendlog.go)) + is a full `SCAN send_log` every 5 min (no index on `status`; the cutoff + matches nearly every row). Mirror of what `0005` fixed for `rejected`: a + partial index `(created_at) WHERE status = 'queued'` makes it a handful of + rows. +- Scoped `QuerySendLog` ([sendlog.go:214-219](../internal/store/sendlog.go)) + sorts a domain's whole scope with a temp b-tree for every page a domain user + opens. + +**M3. `selfpost-backup` with a wrong `SELFPOST_DB_PATH` creates an empty +database and ships a "valid" backup of nothing.** [backup.go:394-405](../internal/backup/backup.go) +opens the source without `mode=ro` or an existence check; `VACUUM INTO` on a +non-existent path succeeds (measured), creates the file on disk and snapshots a +4 KiB empty database; `Create` and `Check` pass. A restore of it opens the +`/setup` link on a host that still carries live DKIM keys and `sasldb2`. + +**M4. `secretfile`'s KDF cap does not do what its comment says.** +[secretfile.go:108-113,184](../internal/secretfile/secretfile.go): +`maxLogN=20, maxR=32, maxP=16` lets a 44-byte header demand +`128·2²⁰·32` = 4 GiB and sixteen times the CPU, under a comment that says the +cap exists "so a hostile header cannot make the reader allocate its way to an +out-of-memory kill". The domain-import form calls `NewReader` on the upload. +Operator foot-gun rather than attack surface (global role only), but the +stated guarantee is false; the real writer never exceeds N=2¹⁵, r=8, p=1. + +**M5. The CLI enforces no password floor for an encrypted backup.** +[cmd/selfpost-backup/main.go:170-187](../cmd/selfpost-backup/main.go) takes +any non-empty string; the panel enforces `validate.MinSecretFilePasswordLen` +(12); security.md:291 says the minimum applies to these files. (The roadmap's +"empty password accepted" item: an empty password is *not* accepted as an +encryption password — empty means plain output by design; the gap is the +floor.) + +**M6. The compose pin, README and guide point at an image that does not +exist.** `deploy/docker-compose.yml:24`, `README.md:115` and guide.md:187 pin +`ghcr.io/mixeme/selfpost:2.4.0` (`2.4.1` since this cut); GHCR holds `1.0.0` +and `1.3.0` only (checked +through the Packages API and by pulling); no 2.x was ever published, and tag +`v2.0.0` exists in git without a release. The quick start cannot work for +anyone but the owner, who runs locally built `-post.` images. Decided +(owner, 2026-10-12): the old tags and images are history and stay untouched; +the pin points forward — it is made true by the next publication, after H1 is +fixed, and until then the documents should not claim an image that is not +there. + +HTTP surface + +**M7. The last-global-user guard is check-then-act without a transaction.** +[store/users.go:315-341](../internal/store/users.go) (`DeleteUser`: +`CountGlobalUsers` then `DELETE`), [handlers_users.go:277-287,317-322](../internal/web/handlers/handlers_users.go) +(demotion), [users.go:80-97](../internal/store/users.go) (`CreateGlobalUser`: +`UserExists` then `INSERT`). `MaxOpenConns(1)` serialises statements, not +pairs. Two globals deleting or demoting each other at once leave no global; +`/server/*` answers 404 to everyone and recovery is database surgery. +security.md:155 promises the guard. + +**M8. Login discloses whether a username exists by timing.** +[auth/handlers.go:109-121](../internal/web/auth/handlers.go): unknown user +returns at :114 without bcrypt; a known one costs ~250 ms (cost 12). The test +compares bodies only. Rate limiting slows enumeration; a dummy compare stops +it. + +**M9. `submitUserUpdate` is five writes with no transaction.** +[handlers_users.go:304-336](../internal/web/handlers/handlers_users.go): +`UpdateUser` → `endSessionsAfterPasswordSet` → `SetUserRole` → +`ClearUserReach` (error only logged) → `SetUserReach`. A failure after the +first leaves a user renamed and signed out everywhere with the old role, or a +`domain` user with empty reach — the state the "Assign at least one domain" +check exists to prevent. + +**M10. Domain import trusts the export file for everything but the domain +name, selector and password.** [domain/transfer.go:145-190](../internal/domain/transfer.go): +`DMARCRua` is not run through `validate.Email` (the form path is), client IPs +not through `parseIPList`, rate-limit `Mode`/`WindowSeconds`/`MaxMessages` +not through the form's parsers. `WindowSeconds: 0` yields a limit that counts +nothing ([milter/ratelimit.go:61](../internal/milter/ratelimit.go)); garbage +IPs make the imported application refuse every client with no word at import. +[app/validate.go:50-55](../internal/app/validate.go) calls the file "untrusted +input" — for the password only. + +**M11. Polling stops for good after one failed refresh.** +[panel.js:202-257](../internal/web/view/static/panel.js): the scheduled poll +is `htmx.ajax(...)` without `source`, so htmx attributes it to `document.body`; +`htmx:responseError` therefore reaches an element with no `data-poll` and +nothing reschedules; `htmx:sendError` and `htmx:timeout` are not handled at +all. A panel restart while Overview or Health is open freezes the page until +the tab is hidden and shown. Related: an expired session answers an +`HX-Request` with `303 /login` ([auth/middleware.go:14](../internal/web/auth/middleware.go)); +the XHR follows it and the sign-in document is swapped into the box — no +`HX-Redirect` on either side. + +Log tailer and checks + +**M12. `deliveryRe` is unanchored; a stranger's HELO parses as a delivery +result.** [logtail.go:85](../internal/logtail/logtail.go): a port-25 reject +line ending `helo=<41E862C00D9E: to=, status=sent>` yields +`(id, recipient, sent)` (probed), and `UpdateStatus` rewrites the matching row. +The attacker must guess an id+recipient pair, so impact is bounded — but +`lines.go:21-24` and `inbound.go:48-51` anchor at the head for exactly this +reason, and this pattern, the one that writes status, does not. + +**M13. The sender-check deferred counter misses the case it is documented to +see.** [sendercheck.go:50](../internal/logtail/sendercheck.go) matches only +`postfix/…smtpd[pid]`; header/EOH events run in cleanup(8), so the warning for +a panel that accepts the socket but hangs at end-of-headers is logged by +`postfix/cleanup`. The fixture at `sendercheck_test.go:64-68` puts those lines +under `smtpd` and says it was "composed from Postfix's source rather than +seen". architecture.md:286-287 claims the figure sees "a panel that is running +and not answering". Confidence: plausible; one `kill -STOP` on the stand +settles it. + +**M14. Resolver fall-back is hollow when the first resolver drops packets.** +[dnscheck/resolver.go:72-90](../internal/dnscheck/resolver.go) tries resolvers +in turn under one 5 s context; a blackhole eats the budget and the second is +asked with an expired context. The `DefaultResolvers` comment promises +otherwise. **M15.** [dnscheck/server.go:26-33,42-46](../internal/dnscheck/server.go) +turns a resolver timeout into "does not resolve" / "no PTR record" (every +other check routes transient errors to Unknown), and empties `ServerIPs` so +the SPF check goes Unknown in the same minute. architecture.md:339-340. + +Infrastructure + +**M16. `envdoc_test.go` compares three hand-written lists to each other and +reads neither `main.go`, the scripts nor `guide.md`.** +[envdoc_test.go:70-136](../cmd/panel/envdoc_test.go). Already drifted: +`ANTISPAM_DIR`, `INBOUND_JOURNAL_MILTER_SOCKET`, `INBOUND_SPAM_LOG_RETENTION_DAYS`, +`RATE_LIMIT_MESSAGES_PER_IP`, `RATE_LIMIT_WINDOW_SECONDS` are read by +`loadConfig` and absent from `loadConfigKeys`; `postfix-config.sh` and +`postfix-wrapper.sh` read `INBOUND_JOURNAL_MILTER_SOCKET`, `SELFPOST_DATA_DIR`, +`SELFPOST_DB_PATH`, `INBOUND_RELAY_ENABLE`, absent from `buildScriptKeys`. The +tests pass; development.md:353-357 and :503-504 describe a guard that does +not exist. + +**M17. The supervisord control socket is `0770 root:selfpost` and the +`postfix` and `opendkim` users are in `selfpost`.** +[supervisord.conf:26-29](../build/supervisord.conf) ("only root and the two +SelfPost services"), [Dockerfile:68-71](../build/Dockerfile) (three users); +confirmed on the server. A compromised smtpd can `supervisorctl stop panel` +(all submission deferred), `stop opendkim` or `shutdown`. No escalation; a +whole-stack denial from the least trusted process. + +**M18. DMARC maps are never ownership-normalised.** entrypoint.sh:108-125 +normalises five maps; `dmarc_recipients`/`dmarc_transport`/`dmarc_relay_domains` +are created by postfix-config.sh:315-317 as root only when missing. A tar +restore as root (guide, methods 3/4) yields `root:root 0640`, which smtpd +cannot open → `451 table lookup problem` on every report until the panel next +rewrites them. Confidence: plausible, needs a run. + +**M19. logrotate state lives in the container layer.** [logrotate-loop.sh:42](../build/logrotate-loop.sh) +uses the default `/var/lib/logrotate/status`, not under `/data`. On the server +the state file is dated the container's start; with no entry logrotate records +today and does not rotate, so a host that recreates the container daily +(`.env` change, image bump) never rotates `mail.log`. The e2e uses `-f` and +cannot see it. + +**M20. `test.yml` never compiles `test/e2e`.** A type error there surfaces in +`release.yml` after the GitHub Release is published and the tag checked out. + +**M21. `deploy/traefik/extract-cert.sh` writes empty PEM files and exits 0 on +a `jq` miss** (:17-25, `sh` without pipefail; `jq` exits 0 on no match); the +files are `0644` until :27. Postfix TLS breaks at the next restart. + +### Low + +- **L1** `-decrypt -o FILE` truncates the output before the password is + verified ([main.go:132-148](../cmd/selfpost-backup/main.go)); a wrong + password leaves an empty file where a good one may have been. +- **L2** Backups skip symlinks silently and `Check` does not notice + ([backup.go:323-327](../internal/backup/backup.go)); a `certs/` of symlinks + (the Let's Encrypt layout) yields an archive with no TLS material while the + guide promises refusal. +- **L3** Trailing bytes after the last `secretfile` chunk are ignored + (:400-416): two envelopes concatenated open as the first. +- **L4** `parseTrustedProxies` turns a bare `::ffff:10.0.0.1` into `::/32` + silently ([main.go:247-253](../cmd/panel/main.go)). +- **L5** `tls_policy` is keyed by next-hop; two inbound domains to one + `[host]:port` with different TLS modes write duplicate keys and the first + (by domain order) wins with a warning — confirmed on the server. The per- + domain TLS mode in the form is not what Postfix does + ([postfix/inbound.go:69-72](../internal/postfix/inbound.go)). +- **L6** Multi-file map rebuilds are not all-or-nothing + ([inbound.go:42-55](../internal/postfix/inbound.go), `dmarc.go:31-41`); a + write failure mid-sequence leaves `relay_domains` ahead of `transport`. +- **L7** `atomicfile.Write` never fsyncs the directory and the package has no + tests ([atomicfile.go:40-44](../internal/atomicfile/atomicfile.go)); every map + writer goes through it. +- **L8** The refusal reply names the other sender's domain + ([sender.go:334](../internal/milter/sender.go)) — an oracle for the server's + signing names, available to any authenticated application. +- **L9** go-milter accumulates every message's headers for a connection's + life (library `session.go:173-187`, cleared on ABORT only); the sender check + is safe because it ignores `h`, and the test passes `nil` so would not catch + a "simplification" to `h.Values("From")`. +- **L10** `parsePage` is not clamped to `lastPage` for the Outbound log + ([handlers_monitor.go:357-371](../internal/web/handlers/handlers_monitor.go)); + the inbound log clamps. Known roadmap row, confirmed. +- **L11** `parseAutoMultiplier` accepts `NaN` ([handlers_ratelimit.go:44-49](../internal/web/handlers/handlers_ratelimit.go)); + SQLite stores NULL, read-back is 0, recalc silently uses 2.5 (probed). +- **L12** Outbound rows belong to a domain by **name** ([handlers_monitor.go:97,345](../internal/web/handlers/handlers_monitor.go)): + a domain deleted and re-added shows its earlier rows to the new assignee. + security.md:419 accepts this for the inbound log only. +- **L13** `HandleReload` answers failures as plain-text `http.Error` + (handlers_domains.go:173-184), and the flash "Configuration regenerated from + the database" oversells: DMARC maps and filter-list files are skipped + (architecture.md:856-859 says so for DMARC; `Lists.Sync` is documented + nowhere). +- **L14** `http.Server` has only `ReadHeaderTimeout` ([httpserver.go:145-149](../cmd/panel/httpserver.go)). +- **L15** The Account password change shares the per-IP login bucket + ([handlers_account.go:142](../internal/web/handlers/handlers_account.go)): + ten wrong current passwords lock sign-in for that IP 15 min. +- **L16** Login issues a fresh token but does not destroy the session a + presented cookie already holds ([auth/handlers.go:123-130](../internal/web/auth/handlers.go)). +- **L17** `domain.Service.Delete` returns a bare error after the row is gone + when `resync()` fails ([service.go:202-210](../internal/domain/service.go)); + `apps.Resync()` is not reached, so the sender map keeps the purged logins. +- **L18** A `rate-limited` refusal is a grey tag in the Outbound log + ([outbound_log.go:213](../internal/web/view/outbound_log.go), no such status + in `components.go:132-146`); its two siblings are red, and the test pins the + grey. +- **L19** *Recalculate rate limit* on the application form submits the whole + form and discards every unsaved edit ([out_app.html:67](../internal/web/view/templates/out_app.html), + handlers_ratelimit.go:178-189). +- **L20** DMARC parse rewrites `0` to 100 and stores a missing + `date_range` as 1970 ([parse.go:125-136](../internal/dmarc/parse.go)). +- **L21** Several `mailStack` instances per process with no shared lock on + table rebuilds (`run`, `newPanel`, `runNotifier`); the notifier's + `EnsureServerSigned` at start can write a stale table over a domain added + in the same second. **L22** `IncrDMARCParseFailures` is read-modify-write + across concurrent `dmarc-ingest` processes ([dmarc.go:163-173](../internal/store/dmarc.go)). +- **L23** The restore-boot Resync reloads Postfix before Postfix exists + (panel priority 200, postfix 300) — an error line per restore boot; the + DMARC bootstrap works around the same thing with a no-op hook and does not + say why ([main.go:301-307](../cmd/panel/main.go)). +- **L24** `health/processes.go:43`, `supervisor.go:17,30`: `exec.Command` + without context; `/healthz` and the 5 s status poll hang on a wedged + supervisord. **L25** `health/cert.go:70-83` assumes the first PEM block is + the leaf and grades a not-yet-valid certificate OK; `machine.go:193-204` + advances `prevAt` when only one of cpu/net succeeded. +- **L26** Only successful sends count toward the notifier's 60/h ceiling + ([notify.go:196-207](../internal/notify/notify.go)); with sendmail broken, + `CheckLimits` spawns a 20 s process per recipient per minute forever. +- **L27** `v=dmarc1` matched case-insensitively ([records.go:74,121](../internal/dnscheck/records.go)); + RFC 7489 §6.4 says the version must match exactly, so a lower-case record is + graded OK and ignored by receivers. **L28** The domain cache is keyed by + name only ([dnscheck.go:155-173](../internal/dnscheck/dnscheck.go)) though + the verdict depends on `ExpectedDKIM`/`ServerIPs`. +- **L29** `lineTime` normalises a Feb 29 line read the following January to + Mar 1 ([inbound.go:254-261](../internal/logtail/inbound.go)); `isoStampRe` + accepts a zone-less stamp that `time.Parse` then rejects (dated `now`). +- **L30** OpenDKIM: `Syslog no` with no `LogWhy`/`Logfile` — it logs nothing + anywhere; no `Canonicalization relaxed/simple`; no `OversignHeaders From` + (the header-append replay is the class 2.4.0 is about). +- **L31** Postfix: `smtpd_tls_protocols` at the 3.7 default `>=TLSv1`, + `disable_vrfy_command = no` (VRFY on 25 enumerates `relay_recipient_maps`), + the stock `(Debian/GNU)` banner — all confirmed on the server. + `RATE_LIMIT_*`, `*_SIZE_LIMIT`, `MILTER_*_TIMEOUT` reach `postconf -e` + unvalidated; a non-integer passes `postfix check` and kills every smtpd at + connect. +- **L32** `crashexit` fires on FATAL only, which arises only from + `startretries` exhausted within `startsecs` (1 s): a program that crashes + after one second cycles forever and never exits the container. + architecture.md:40-41 reads wider than that. +- **L33** `.dockerignore` excludes `docs`, `dev`, `bin` but not `test/`; a + developer build after an e2e run ships `test/e2e/.stage/data` (DKIM keys, + `sasldb2`) into the build context. Base images pinned by tag, not digest. +- **L34** e2e: `stage.go:77` pulls `alpine:3.20` from the internet; + `hostname_gate_test.go:97-101` runs with the host's real network; + `sender_check_test.go:141` has a fixed `time.Sleep(2s)` against + development.md:409 "no fixed sleep". + +### Nits + +Dead code: `handlers/authz.go:63 domainIDSet`; `view.Facts` +(`components.go:414`); `dmarc.Service.HostedSuggestion`, `ValidateHostedAddress` +(`dmarc/service.go:98-109`); `app.Service.UpdateMode` (`app/service.go:132-145`, +superseded by `SaveSettings`); `Settings.WithResult` (`view/account.go:137-145`) +re-implements `resultFlash`; `KitPage()`'s hard-coded `User: "admin"` +(overwritten by the handler); dead method checks in `handlers_dmarc.go:20-24` +and the `default:` branches of `HandleUserNew`/`HandleUserEdit`/`HandleServerSettings` +(mux patterns are method-bound); the `postfix set-permissions` branch in +entrypoint.sh:170-172 (`postfix check` has already created the tree). +Duplication: `canViewDMARCDomain` repeats `lookupDomain`'s check; `ParseForm` +twice on the user-edit POST; `Lookup` then `Touch` per request in +`auth/middleware.go:17,28`; `retry.go:58-67` repeats the six `postconf` keys as +literals instead of `retryConfKeys`; three header-name canonicalisers with two +behaviours (`milter.go:124`, `inbound.go:89`, `sender.go:248`). Behaviour: +`submitUserCreate` does `CreateUser` then `UpdateUser` for the e-mail; +self-demotion redirects to a page that is now 404; `redirectHome` gives a +domain user with no outbound reach an empty 200 where the inbound list gives +404; `sort.Slice` in `postfix/inbound.go:59` reorders the caller's slice; +`strings.Split(raw, "\n")` in `sender.go:65,104` keeps `\r`; `lists.go:428` +trims the filter key while the Lua side reads it verbatim; `InDomain.WithMX` +labels a `warn` status "NOT CHECKED"; `senderSession` does not override `Body` +(the library discards the session after every accepted message — harmless, +inconsistent with `milter.go:130-134`). The handlers test package takes ~105 s +on this machine (likely bcrypt cost 12 on the user-creation paths). + +### Doc drift + +architecture.md: :167 (§ OpenDKIM) still says the `From:` header is not held +against the login — contradicted by 2.4.0 and by § Sender check in the same +file; :272-274 credits `/run` being tmpfs for a fresh `.alive` mark (it is a +plain image directory; a restarted container keeps the mark); :328-329 "a +deleted domain's row goes with it" — only `dns:` rows, `limit:` lingers up to +48 h; :493 and § Who may open what omit `/inbound/log` and +`/inbound/filter-lists`; :800 names only migration `0002`; :832-836 omits +`inbound_spam_log`, `inbound_spam_list`, `notify_state`, `sender_check_days`; +:838 "the panel is the only writer" — `dmarc-ingest` is a separate process +that opens the store, migrates and writes; :856-859 vs `backup.go:19-23` and +`httpserver.go:52-53` ("the same Resync") — the button skips DMARC maps and +filter lists; the Internal env list omits `ANTISPAM_DIR` and +`INBOUND_JOURNAL_MILTER_SOCKET`; § View engine calls the kit page "fixtures" +(it is production data). schema-migrations.md: :20 "chain head `user_version += 5`" (six files; `TestBaselineSchema` asserts 6); the "Schema at head (v2)" +table omits `sender_check_days` and `send_log.reason`/`header_from`. +guide.md: :214 `INBOUND_JOURNAL_MILTER_SOCKET` "Set in: compose" (it is not); +:1260 lines "deleted together with the row" (at the next six-hourly prune, and +not while a reused id has a row); :720 "first successful boot" (M1). +development.md: :270 version/tag coupling (H1); :353-357, :503 the env guard +(M16); :404 "milter fail-open" as a covered negative (the suite now proves +deferral); :409 "no fixed sleep". roadmap.md: `template-data-typing` describes +`renderDomainDetail` with forty map keys — gone since 2.0 (one typed-before- +render map remains, the health sampler's); `structured-logging` says `logf` +"in five packages" — it is in 21 files of `internal/web`; +`review-2026-08-followups` row "empty password" is misdescribed (M5). Code +comments: `milter.go:7-11` "it never rejects" (it tempfails at `MAIL FROM`); +`store/sendlog.go:19-22,61-65` `StatusRejected`/`InsertRejected` describe the +level-2 case only; `store.go:2-3` "domain-admins"; `schema_test.go:15` +`TestBaselineIsTheOnlyMigration`; `health/machine.go:225` "see roadmap +panel-docs … later" (shipped 1.8.0); `inboundspam.go:349-350` "does not read +the whole journal" (it scans it); `logtail.go:466` mentions `copytruncate`; +supervisord.conf:25 "two services", :107 "7-14 files"; entrypoint.sh:119-120 +"no secrets" in `/data/antispam` (`filter.key`); docker-compose.yml:85,88 cite +a guide section by a title it does not have. + +### Test gaps + +No test for: the H1 stamp (a `panel -version` assertion in the e2e against the +version the stand was told to build); cert re-copy (H2); `follow()` with an +unreadable or recreated log, a truncated `postqueue -p`, lines written to the +old inode after the switch, `UpdateStatus` from `sent` (H3); two logins +differing in case (H4); M1's ordering (restore tests never fail Open/Resync or +exercise the real reload hook); `CheckRestore` with malformed JSON; M3, M5, +L1, L2; `secretfile` concatenation (L3); the sender-check counter with the +real program name (M13); a `LookupAddr` timeout (M15); `TestLimitRefusalsSince` +EXPLAINs a stand-in query, not the real one; `TestOpenMigratesA20Database` +stops at 0004; `Import` with a bad rua / IPs / window 0 / mode (M10); panel.js +has no test at all (M11); login timing (M8); concurrent last-global (M7); +`TestOriginCheck` lacks the Host-with-port vs Origin case; +`TestServerSubtreeIsGlobalOnly` skips any `/server/` route not in +`routes.always`; `internal/atomicfile` and `internal/supervisor` have no +tests; `TestRenderInboundMaps` has no two-routes-one-next-hop case (L5); +`TestSenderCheckResetsBetweenMessages` passes `h = nil` (L9); the handler test +rows of `review-2026-08-followups` (apps/domains/users/dmarc handlers +directly, `internal/dmarc` service) are still open. + +### Checked and sound + +Worth writing down so the next review does not re-read it: the sender check's +layered rule holds against every `From:` shape tried (quoted local parts with +`@`, CFWS, encoded words decoding to addresses, groups, `<>`, address +literals, obs-route) — the "one `@` outside quotes" rule is the real backstop; +extension stripping and case folding match Postfix; every error path of the +check tempfails; level-2 accounting reserves under one lock and cannot be +inflated by multi-RCPT or bypassed by ABORT/reconnect; the inbound verdict is +trusted only under the queue-id-bound HMAC; the port-25 `-o` overrides are +complete and read back by the e2e through `postconf -Ph`; the generated +milter chain is exactly sender(tempfail) → OpenDKIM(tempfail) → +journal(accept); `/server/` is guarded at registration and per handler; +scopes fail closed (`1 = 0`); sessions store the hash only, `__Host-` cookie, +constant-time setup token; `SaveSettings` restores prior state on mid-way +failure; `secretfile`'s AAD binds header, counter and last-flag, and +truncation, reorder and tamper are tested; shutdown order (tailer offset +saved, HTTP drained, DB closed last) is right; rspamd map names, deny message, +symbol prefixes and key-file format match the Go side byte for byte. + +--- + +## Plan + +Six stages, each a self-contained cut that can be taken in any order after +stage 0, each small enough for one sitting, with model routing per +development.md (security/infra → Opus, UI/docs → Sonnet). "Done when" is the +test that proves it, not the edit. Nothing here is agreed until the owner +says so; the order is my recommendation by harm per hour. + +### Stage 0 — release hygiene (no product code) → `2.4.2` + +1. **Stamp the right version** (H1). Make the e2e build take the version from + the environment (`VERSION: ${SELFPOST_E2E_VERSION:-e2e}` in + `compose.override.yml`, set by `release.yml`), or have the e2e reuse the + image already built instead of rebuilding. Done when the e2e asserts + `panel -version` equals what it was told, and `release.yml` runs that + assertion before the push. — **Opus** +2. **Compile the e2e in `test.yml`** (M20): `cd test/e2e && go vet ./... && go + test -run xxx ./...`. — **Haiku** +3. **`.dockerignore` + `test/`** (L33). — **Haiku** +4. **The pin** (M6): old tags and images are not touched (owner, + 2026-10-12). The next publication through the fixed `release.yml` is the + one the pin names; until then README/guide say that the current line is + not yet published rather than name an image that is not there. — **Sonnet** +5. **Toolchain and deps**: `modernc.org/sqlite`, `x/crypto`, `x/sys`; note in + development.md that the image is rebuilt after each Go patch release + (`golang:1.26-bookworm` floats, so a rebuild suffices). — **Haiku** +6. The four `staticcheck` items. — **Haiku** + +### Stage 1 — mail that is shown wrong (H3, H2, M12, M13) → `2.5.0` + +1. **`QueueIDs` on a truncated listing returns an error**, as `postqueueJSON` + does; `reconcile` already treats an error as no information. Test: a 5 MiB + fake `postqueue -p`. — **Opus** +2. **Reconcile only closes `queued` rows** (`AND status = 'queued'` in the + reconcile path's update) and **only while the tailer is reading**: `follow` + publishes "healthy" (last successful drain within N ticks) and + `reconcileLoop` skips a sweep when it is not. Test: reopen fails → no rows + closed. — **Opus** +3. **Rotation window**: after switching inodes keep draining the old one until + it has not grown for one full tick *and* the new file has received a line, + or for a bounded number of ticks. Test on the stand with `postfix reload` + timing. — **Opus** +4. **Anchor `deliveryRe`** at the head of the line like its two siblings; + test with the probed HELO and quoted-sender lines. — **Opus** +5. **TLS re-copy** (H2): `postfix-cert-reload.sh` compares the mounted files + to the copy and re-copies before `postfix reload` (atomically, same modes); + e2e replaces the cert in the stand and reads the new serial off 465. + — **Opus** +6. **Deferred counter** (M13): on the stand, `kill -STOP` the panel mid-message + and record which program logs the warning; widen the matcher accordingly + and replace the composed fixture with the seen lines. — **Opus** +7. Docs: architecture § Log tailer, § Queue reconcile, § Sender check "what it + costs"; guide § Delivery log. — **Sonnet** + +### Stage 2 — the tenant boundary and the registry (H4, M7, M9, M10) → in `2.5.0` (migration `0007`) + +1. **Logins lower-cased** at validation and `applications.login COLLATE + NOCASE` (migration rebuilds the unique index). Before the migration a check + that no two existing logins differ only in case (there is one + installation; look). Tests: `Ops`/`ops` refused; the map written lower- + case. Then verify Postfix's compare on the stand once (`MAIL FROM` as the + other case) so the doc's claim is seen, not assumed. — **Opus** +2. **Last global in one transaction**: `DeleteUser`/`SetUserRole` take a + `tx`, count and act inside it; `CreateGlobalUser` likewise. A concurrency + test with two goroutines. — **Opus** +3. **`submitUserUpdate` in one transaction** (store method that takes the + whole change). — **Opus** +4. **Import validates like the form**: route `DMARCRua`, IPs, limit mode, + window and count through the same parsers; refuse the file with the + validator's text. Tests per field. — **Opus** +5. Docs: security.md § Roles (the guard), guide § Domain export. — **Sonnet** + +### Stage 3 — persistence and restore (M1–M5, L1–L4, L7) → `2.5.1` + +1. **Consume the manifest after the resync succeeds** (`CheckRestore` reports; + `run` deletes after `resyncAfterRestore` returns nil). Test: Resync fails → + manifest kept → second boot resyncs. — **Opus** +2. **Indexes**: `idx_send_log_domain_created_at (domain, created_at)`; + partial `idx_send_log_queued (created_at) WHERE status = 'queued'`; + consider `(domain, id)` for the scoped page. Migration `0008` DDL only; + `TestLimitRefusalsSince`-style EXPLAIN tests against the **real** query + strings. — **Opus** +3. **CLI backup**: open the source `mode=ro`, refuse a missing file; password + floor of 12 shared with the panel (`validate.MinSecretFilePasswordLen`); + `-decrypt -o` verifies the password (first chunk) before opening the + output; `OnWarn` for skipped symlinks and `Check` fails when `certs/` is + named but empty. — **Opus** +4. **`secretfile` caps**: bound `128·N·r` to a stated budget (e.g. 128 MiB) + and `p` to 1-2; refuse the rest with `ErrHeader`; reject trailing bytes + after the last chunk. Tests. — **Opus** +5. **`atomicfile`**: fsync the directory after rename; a test file for the + package. — **Opus** +6. Docs: security.md § Backup; guide § Backup. — **Sonnet** + +### Stage 4 — panel behaviour (M8, M11, L10–L19) → `2.5.2` + +1. Login: dummy bcrypt on unknown user; a test that measures both paths within + a tolerance. Account password change gets its own limiter bucket. Login + destroys the presented session. — **Opus** +2. `http.Server`: `ReadTimeout`, `WriteTimeout`, `IdleTimeout`. — **Opus** +3. **Polling recovery**: schedule from `htmx:afterRequest` on the box (or pass + `source`), handle `sendError`/`timeout` with back-off; `RequireAuth` + answers an `HX-Request` with `HX-Redirect: /login` instead of a 303. A + minimal JS test (node + jsdom) or at least an e2e that restarts the panel + with Overview open. — **Sonnet** +4. `parsePage` clamp; `NaN` refused; `rate-limited` tag red (add the status to + `components.go`, flip the test); the recalc button posts only what it + needs or warns; `HandleReload` either covers DMARC maps and filter lists or + the flash and the two comments say what it covers. — **Sonnet** +5. `domain.Service.Delete`: say the domain is deleted when a later step fails, + and run `apps.Resync()` anyway. — **Opus** + +### Stage 5 — image and Postfix hardening (M17–M19, M21, L30–L32) → `2.6.0` + +1. **Supervisor socket**: a group of its own (`supervisor`), panel only; + `postfix`/`opendkim` keep `selfpost` for the file paths. — **Opus** +2. **OpenDKIM**: `Syslog yes` (supervisord captures it) or `Logfile`; + `Canonicalization relaxed/simple`; `OversignHeaders From`. e2e: DKIM still + verifies; a message with two `From:` is refused before signing anyway. + — **Opus** +3. **Postfix**: `smtpd_tls_protocols = >=TLSv1.2`, `smtpd_tls_mandatory_protocols` + likewise, `disable_vrfy_command = yes`, `smtpd_banner = $myhostname ESMTP`; + integer validation of `RATE_LIMIT_*`, `*_SIZE_LIMIT`, `MILTER_*_TIMEOUT` at + the top of `postfix-config.sh` with a clear exit. — **Opus** +4. **logrotate state under `/data/log/.logrotate.status`** (`-s`), group- + readable as the directory is. — **Opus** +5. **entrypoint**: normalise the three DMARC maps with the other five; + remove the dead `set-permissions` branch. — **Opus** +6. **`extract-cert.sh`**: `set -o pipefail` under `bash`, check both outputs + are non-empty PEM before moving them into place, create `0600` first. + — **Opus** +7. **`crashexit`**: subscribe to `PROCESS_STATE_EXITED` with a restart-count + threshold, or raise `startsecs` so a crash after 1 s still reaches FATAL; + say in architecture.md what actually exits the container. — **Opus** +8. **`envdoc_test` made real**: parse `envDefault|envInt|os.Getenv("…")` out of + `cmd/panel/*.go` and `${VAR` out of `build/*.sh`, parse the key tables out + of guide.md and architecture.md, compare. — **Opus** +9. Docs: architecture § Image and processes, § Configuration; security.md + § Process. — **Sonnet** + +### Stage 6 — documentation and roadmap sweep (no product code, no cut) + +1. Every item under *Doc drift* above, file by file. — **Sonnet** +2. roadmap.md: strike `template-data-typing` (done by 2.0; one sampler map + left, typed before render); reword `structured-logging`; in + `review-2026-08-followups` mark `parsePage` and `secretfile` rows as + addressed by stages 3-4, keep the `offset.go` inode row (still open: + fingerprint of the first 512 bytes only), and the `dnscheck` fan-out row + now points at M14. Add the open tests list above as the test row's new + content. — **Sonnet** +3. Comments named above (`milter.go`, `sendlog.go`, `store.go`, `schema_test.go`, + `machine.go`, `inboundspam.go`, `logtail.go`, supervisord.conf, + entrypoint.sh, docker-compose.yml). — **Haiku** +4. Dead code removal from § Nits, with `staticcheck` added to `test.yml` so it + stays gone. — **Haiku** + +### Not planned, said out loud + +- L5 (`tls_policy` per next-hop): the right fix is a model change (TLS mode + per upstream, not per domain) and a form change — a design item, not a + patch. Until then the guide should say two domains to one upstream share + the first one's TLS mode. +- L12 (rows by domain name): accept it for outbound as security.md already + does for inbound, and write it there. +- L8 (the signing-name oracle): the names are in DNS; shortening the reply + costs a less useful error. Leave, note in security.md § Accepted risks. +- L9 (go-milter header accumulation): a one-line comment on `Headers` and a + test with a polluted `h`; the library behaviour itself is upstream's. +- The Windows-bound tests: not worth platform guards; CI is Linux. diff --git a/docs/development.md b/docs/development.md index a095203..75cc80c 100644 --- a/docs/development.md +++ b/docs/development.md @@ -107,7 +107,7 @@ than the default «meaningful step» rule in [§ Commits and release build](#com 2. Git tag `vX.Y.Z` and publish the GitHub Release only on explicit request (see [§ Release image](#release-image)). -**Planned version cuts** (from pin `2.4.0`; adjust if semver changes mid-track): +**Planned version cuts** (from pin `2.4.1`; adjust if semver changes mid-track): | Stage | ID | Cut | |---|---|---| @@ -129,6 +129,7 @@ than the default «meaningful step» rule in [§ Commits and release build](#com | 16 | delivery-log-storage | `2.2.0` | | 17 | panel-notifications | `2.3.0` | | 18 | from-header-binding | `2.4.0` | +| 19 | code audit 2026-10-12 and its plan (docs only, owner-requested cut) | `2.4.1` (PATCH) | Docs-only prep (checklists in plans, no product code) uses the same per-step commit rule but **no** version cut until the next product stage ships. diff --git a/docs/guide.md b/docs/guide.md index cb54a24..d362fff 100644 --- a/docs/guide.md +++ b/docs/guide.md @@ -184,7 +184,7 @@ cat ./data/setup-token #### Fixed image tag `deploy/docker-compose.yml` pins an explicit version (`ghcr.io/mixeme/selfpost:X.Y.Z`), -deliberately never `:latest`. The current pin is `2.4.0`. Intermediate +deliberately never `:latest`. The current pin is `2.4.1`. Intermediate CHANGELOG sections (`0.2.0`…`0.6.0`) record development cuts from before that image was published. Pinning matters because of the backup version check (see [Full backup and restore](#full-backup-and-restore)): the panel binary's diff --git a/docs/roadmap.md b/docs/roadmap.md index 2a7ee60..9f5c1d5 100644 --- a/docs/roadmap.md +++ b/docs/roadmap.md @@ -42,6 +42,7 @@ in `git log` and [CHANGELOG.md](../CHANGELOG.md). | from-header-binding | Hold a message's `From:` header against the application that sends it | shipped `2.4.0` | 9/9 | [plans/from-header-binding.md](plans/from-header-binding.md) | | panel-redesign | Panel redesign (accepted mockups, design contract) | shipped `2.0.0` | 27/27 | [plans/panel-redesign.md](plans/panel-redesign.md) | | schema-squash | Squash SQLite migrations into a 2.x baseline | built in panel-redesign (stage 1) | — | [plans/panel-redesign.md](plans/panel-redesign.md) | +| audit-2026-10-12 | Follow-ups of the 2026-10-12 full-tree code audit, six stages | candidate | — | [audit-2026-10-12.md](audit-2026-10-12.md) | **Recommended order** (not binding; owner, 2026-10-09: feature order follows development convenience): **panel-redesign** shipped as `2.0.0` (2026-10-11) and