@@ -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:<x@a.com>` 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.<sha>` 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=<victim@…>, 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 `<pct>0</pct>` 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– – `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– `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– – `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.
Claude Fable 5.1