Agreed with the owner, one question at a time; documents only, no code. From from-header-binding's Not done list, each a step of the audit plan: the Sender check column says "Connections turned away"; a stand test of which key OpenDKIM signs with; Health connects to a milter socket instead of stat-ing it; the cards layout orders a phone by the box's number; a refusal the panel did not write is recovered from mail.log; a log rotated while the panel was down is read to its end; the deferred counter keeps a place in the log; downtime is counted when supervisord is silent. "Today, UTC" became the roadmap item local-time (agreed): one instance zone, no labels beside a time, days counted in that zone, with the conditions for per-user zones and offsets, and the trap of mail.log's own stamps written down. The six steps on the log tailer are split off stage 1 into stage 2b, a cut of its own (2.5.1); stages 3 and 4 move to 2.5.2 and 2.5.3. Also: negative e2e cases pass only on their own refusal (stage 0); notifications are owed per recipient (stage 4); a DNS outage is unknown, not wrong (M14, M15 had no step); DMARC reports tested where they come in. review-2026-08-followups is closed, its rows placed in stages or dropped with a reason. Certificate warnings by lifetime: left as is, with the condition for return. The audit's checklist is 61 lines (was 48). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This commit is contained in:
Claude Opus 5.5
+187
-34
@@ -520,12 +520,14 @@ symbol prefixes and key-file format match the Go side byte for byte.
|
||||
|
||||
## Plan
|
||||
|
||||
Six stages, each a self-contained cut, each small enough for one sitting,
|
||||
Seven stages (six, and stage 2b split off stage 1 on 2026-10-14), each a
|
||||
self-contained cut, 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.
|
||||
|
||||
**Agreed with the owner on 2026-10-12** (roadmap row `audit-2026-10-12`):
|
||||
stage 0 first, then stages 1 and 2 as one cut, the rest after in any order.
|
||||
stage 0 first, then stages 1 and 2 as one cut, the rest after in any order
|
||||
(stage 2b after `2.5.0`, before the others).
|
||||
This is the **active plan** in the sense of development.md § Plan checklists:
|
||||
the `- [ ]` lines below are the implementation checklist, a line is ticked in
|
||||
the commit that finishes it, and the roadmap's Progress column counts them.
|
||||
@@ -560,6 +562,16 @@ has already run is never edited — the next file is added; the published
|
||||
development.md that the image is rebuilt after each Go patch release
|
||||
(`golang:1.26-bookworm` floats, so a rebuild suffices). — **Haiku**
|
||||
- [ ] The four `staticcheck` items. — **Haiku**
|
||||
- [ ] **A negative e2e case passes only on the refusal it is about** (owner,
|
||||
2026-10-14; seen, not found by the audit). `negative_level1_ratelimit`
|
||||
sends up to sixty messages and waits for one to be refused; it once
|
||||
passed on "limit tripped on attempt 1/60" when the connection was reset
|
||||
because Postfix was still starting. The case counts as the limit only the
|
||||
reply code of the limit, and only after at least one message was
|
||||
accepted; a reset, a timeout or any other reply fails it and says what
|
||||
came. Then every other negative case of the suite is read for the same
|
||||
mistake - any failure taken for the expected refusal - and held to its
|
||||
own reply code. - **Opus**
|
||||
|
||||
### Stage 1 — mail that is shown wrong (H3, H2, M12, M13) → `2.5.0`
|
||||
|
||||
@@ -597,25 +609,6 @@ has already run is never edited — the next file is added; the published
|
||||
grown for a tick and the new file has a line). Both are fixes to the
|
||||
tailer's blindness and stay on the list whatever the sweep's fate; their
|
||||
priority follows `tailer_blind_seconds`.
|
||||
- [ ] **A refused message's page shows what Postfix wrote** (owner,
|
||||
2026-10-13: "the log box is required" - the box is the reason
|
||||
delivery-log-storage was built, and its absence from the picture he
|
||||
approved for 2.4.0 was an oversight, not a decision). Today every
|
||||
`rejected` row - `wrong From`, `wrong client`, `rate-limited` - has no
|
||||
queue id, and its page shows the box with one sentence,
|
||||
"This message never reached the queue, so Postfix wrote no delivery lines
|
||||
for it" ([handlers_monitor.go](../internal/web/handlers/handlers_monitor.go)).
|
||||
The sentence is untrue: Postfix wrote a line for each (`milter-reject:
|
||||
END-OF-MESSAGE ... 5.7.1 From: ...` from cleanup, `NOQUEUE: reject: ...`
|
||||
from smtpd). The tailer attaches that line to the row it belongs to, by
|
||||
client address, time and the addresses in the line, and stores it in
|
||||
`send_log_lines` like any other; the page shows it. First on the stand:
|
||||
read `mail.log` for the three kinds of refusal and write down what
|
||||
identifies each line. Then the mockup: the refused variant of `out-message`
|
||||
gets the box with the line in it (`design:` commit), then the code. The
|
||||
sentence stays only for a row whose line was truly never written or is
|
||||
older than this change, and says that. Done when the e2e shows the stored
|
||||
line on the page of each of the three refusals. - **Opus**
|
||||
- [ ] **Anchor `deliveryRe`** at the head of the line like its two siblings;
|
||||
test with the probed HELO and quoted-sender lines. — **Opus**
|
||||
- [ ] **TLS re-copy** (H2): `postfix-cert-reload.sh` compares the mounted files
|
||||
@@ -642,7 +635,8 @@ has already run is never edited — the next file is added; the published
|
||||
`tx`, count and act inside it; `CreateGlobalUser` likewise. A concurrency
|
||||
test with two goroutines. — **Opus**
|
||||
- [ ] **`submitUserUpdate` in one transaction** (store method that takes the
|
||||
whole change). — **Opus**
|
||||
whole change). The users pages get direct handler tests while they are
|
||||
rewired - a row of review-2026-08-followups, placed here 2026-10-14. — **Opus**
|
||||
- [ ] **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**
|
||||
@@ -662,17 +656,121 @@ has already run is never edited — the next file is added; the published
|
||||
are still in the global log. — **Opus**
|
||||
- [ ] Docs: security.md § Roles (the guard) and the struck accepted risk, guide
|
||||
§ Domain export, schema-migrations.md. — **Sonnet**
|
||||
- [ ] **Which key OpenDKIM signs with, seen and not reasoned** (owner,
|
||||
2026-10-13, from the Not verified list of from-header-binding). The check
|
||||
and OpenDKIM read `From:` with two parsers; that they agree was argued
|
||||
from the check's two rules, never run. An e2e case sends, as one
|
||||
application, the `From:` lines the check passes - quoted strings,
|
||||
comments, folded lines, groups, escapes, encoded words, an address
|
||||
extension - and reads the `d=` of the signature on what arrives: any
|
||||
domain but the application's fails the test. The lines live in one table
|
||||
shared with the check's unit test, so a line added to one is run by the
|
||||
other. A disagreement found is fixed in the check before the cut, and
|
||||
security.md § Mail path says the claim is tested. - **Opus**
|
||||
- [ ] **The 550 reply names no other domain** (L8): `sender.go:334` stops at
|
||||
"it names another sender of this server"; the refused message's page
|
||||
still shows the `From:` line as written (`header_from`), so the operator
|
||||
loses nothing. Test pins the reply text; e2e's sender-check negatives
|
||||
assert the reply does not contain the other tenant's name. — **Opus**
|
||||
|
||||
### Stage 3 — persistence and restore (M1–M5, L1–L4, L7) → `2.5.1`
|
||||
### Stage 2b — the log tailer, and what the panel shows of a refusal → `2.5.1`
|
||||
|
||||
Split off stage 1 on 2026-10-14 at the owner's word: the six steps below were
|
||||
agreed on 2026-10-13/14, after the plan, and had made stages 1 and 2 a cut of
|
||||
twenty-one steps. Stages 1 and 2 ship as `2.5.0` in their first size; this
|
||||
stage follows as a cut of its own, with its own reviews. Five of the six
|
||||
change one thing, the tailer and the state it saves, and are done in this
|
||||
order - each stands on the one before.
|
||||
|
||||
- [ ] **A log rotated while the panel was down is read to its end** (owner,
|
||||
2026-10-13; the Not done list of from-header-binding had it as "deferrals
|
||||
are lost", and it is wider). At start, a saved offset whose fingerprint
|
||||
is not the current file's means "start on the new file"
|
||||
([offset.go](../internal/logtail/offset.go)); what Postfix wrote between
|
||||
the panel's stop and the rotation is never read - delivery statuses,
|
||||
stored delivery lines, refusals, inbound journal rows, the deferred
|
||||
counter, and the recovery of unwritten refusals above. The tailer first
|
||||
opens the rotated file (`mail.log.1`), checks by fingerprint that it is
|
||||
the one the offset belongs to, reads it from the offset to its end, then
|
||||
goes on to the new file. The fingerprint gains the inode, which closes
|
||||
the roadmap's review-2026-08-followups row on collisions. Not found, or
|
||||
rotated twice: logged, with from when to when nothing was read. A test
|
||||
that stops the tailer, appends, rotates, appends, starts it, and finds
|
||||
both halves once. - **Opus**
|
||||
- [ ] **The deferred counter remembers a place in the log, not a second**
|
||||
(owner, 2026-10-14, from the Not done list of from-header-binding; he
|
||||
chose it over leaving the case written down). After an unclean stop the
|
||||
tailer re-reads lines it had read, and the counter passes over every
|
||||
warning no later than the last one it counted
|
||||
([logtail/sendercheck.go](../internal/logtail/sendercheck.go), `last`);
|
||||
`mail.log` stamps whole seconds, so a warning of that very second, later
|
||||
in the file, is lost. The count and the place in the log it has reached
|
||||
are saved in one transaction, and what to pass over is decided by the
|
||||
place. Done after the rotation step above, on the same saved state - the
|
||||
place must stay true across a rotated file. A test that stops the tailer
|
||||
between two warnings of one second and finds both counted, once each.
|
||||
- **Opus**
|
||||
- [ ] **A refused message's page shows what Postfix wrote** (owner,
|
||||
2026-10-13: "the log box is required" - the box is the reason
|
||||
delivery-log-storage was built, and its absence from the picture he
|
||||
approved for 2.4.0 was an oversight, not a decision). Today every
|
||||
`rejected` row - `wrong From`, `wrong client`, `rate-limited` - has no
|
||||
queue id, and its page shows the box with one sentence,
|
||||
"This message never reached the queue, so Postfix wrote no delivery lines
|
||||
for it" ([handlers_monitor.go](../internal/web/handlers/handlers_monitor.go)).
|
||||
The sentence is untrue: Postfix wrote a line for each (`milter-reject:
|
||||
END-OF-MESSAGE ... 5.7.1 From: ...` from cleanup, `NOQUEUE: reject: ...`
|
||||
from smtpd). The tailer attaches that line to the row it belongs to, by
|
||||
client address, time and the addresses in the line, and stores it in
|
||||
`send_log_lines` like any other; the page shows it. First on the stand:
|
||||
read `mail.log` for the three kinds of refusal and write down what
|
||||
identifies each line. Then the mockup: the refused variant of `out-message`
|
||||
gets the box with the line in it (`design:` commit), then the code. The
|
||||
sentence stays only for a row whose line was truly never written or is
|
||||
older than this change, and says that. Done when the e2e shows the stored
|
||||
line on the page of each of the three refusals. - **Opus**
|
||||
- [ ] **A refusal the panel did not get to write is written from
|
||||
`mail.log`** (owner, 2026-10-13, from the Not done list of
|
||||
from-header-binding; built on the step above). The check answers 550 and
|
||||
writes the row a moment later, through a buffer; a panel that stops in
|
||||
between - a rollout, a crash - leaves an application that was refused
|
||||
and a log that does not know it. When the tailer meets a refusal line
|
||||
that has no row, it makes the row from the line (client, sender,
|
||||
recipient, reason), marked as recovered. The tailer resumes from its
|
||||
saved offset, so this covers a kill as well as a clean stop - which a
|
||||
flush of the buffer at shutdown would not. Done when the e2e refuses a
|
||||
message, kills the panel before the row is written, and finds the row
|
||||
after the restart, once. - **Opus**
|
||||
- [ ] **Downtime is counted when supervisord does not answer at start**
|
||||
(owner, 2026-10-13, from the Not done list of from-header-binding). The
|
||||
panel asks supervisord once since when Postfix runs
|
||||
([sendercheck.go](../cmd/panel/sendercheck.go), `postfixStarted`); an
|
||||
error is logged and that start's downtime is not counted at all - the
|
||||
number the owner will decide by reads low. The question is repeated for
|
||||
about thirty seconds. Still no answer: the downtime is taken from the
|
||||
alive mark alone - an upper bound, Postfix may not have been running -
|
||||
and the day is marked as estimated (a column in `sender_check_days`, the
|
||||
next migration), which the card shows. The mark on the card is drawn in
|
||||
the mockup with the renamed column above (one `design:` commit). - **Opus**
|
||||
- [ ] **The Sender check card names what it counts** (owner, 2026-10-13, from
|
||||
the Not done list of from-header-binding): the column *Messages deferred*
|
||||
becomes *Connections turned away*. Postfix refuses from the connection on
|
||||
while the check does not answer, before any message is offered, so a
|
||||
client that connects three times with one message is three, and a monitor
|
||||
that only connects is one; messages cannot be counted. The mockup `health`
|
||||
and help topic `sender-check` first (`design:` commit), then the page and
|
||||
`view/help.go`, word for word. - **Sonnet**
|
||||
- [ ] Docs: architecture § Log tailer (the rotated file, the saved place, a
|
||||
refusal's line and a recovered row), guide § Outbound log and § Health,
|
||||
schema-migrations.md for the mark on `sender_check_days`. — **Sonnet**
|
||||
|
||||
### Stage 3 — persistence and restore (M1–M5, L1–L4, L7) → `2.5.2`
|
||||
|
||||
- [ ] **Consume the manifest after the resync succeeds** (`CheckRestore` reports;
|
||||
`run` deletes after `resyncAfterRestore` returns nil). Test: Resync fails →
|
||||
manifest kept → second boot resyncs. — **Opus**
|
||||
manifest kept → second boot resyncs. And a corrupted or truncated archive
|
||||
is refused on restore and on import with its message, nothing half
|
||||
written (a row of review-2026-08-followups, placed here 2026-10-14). — **Opus**
|
||||
- [ ] **Indexes**: `idx_send_log_domain_created_at (domain_id, created_at)` —
|
||||
on the id column stage 2 adds, replacing `idx_send_log_domain`;
|
||||
partial `idx_send_log_queued (created_at) WHERE status = 'queued'`;
|
||||
@@ -691,7 +789,7 @@ has already run is never edited — the next file is added; the published
|
||||
package. — **Opus**
|
||||
- [ ] Docs: security.md § Backup; guide § Backup. — **Sonnet**
|
||||
|
||||
### Stage 4 — panel behaviour (M8, M11, L10–L19) → `2.5.2`
|
||||
### Stage 4 — panel behaviour (M8, M11, L10–L19) → `2.5.3`
|
||||
|
||||
- [ ] Login: dummy bcrypt on unknown user; a test that measures both paths within
|
||||
a tolerance. Account password change gets its own limiter bucket. Login
|
||||
@@ -705,7 +803,9 @@ has already run is never edited — the next file is added; the published
|
||||
- [ ] `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**
|
||||
the flash and the two comments say what it covers. The log's paging is
|
||||
tested at its edges: first page, last, past the last, an empty log (a row
|
||||
of review-2026-08-followups, placed here 2026-10-14). — **Sonnet**
|
||||
- [ ] `domain.Service.Delete`: say the domain is deleted when a later step fails,
|
||||
and run `apps.Resync()` anyway. — **Opus**
|
||||
- [ ] **A status filter and a day filter in the Outbound log** (owner, 2026-10-13, from the
|
||||
@@ -720,6 +820,61 @@ has already run is never edited — the next file is added; the published
|
||||
commit). And a filter by day, as the Inbound log has it (owner, the same
|
||||
day): the same control, drawn in the mockup first. Both filters survive
|
||||
paging and the rows' polling. - **Sonnet**
|
||||
- [ ] **Milter sockets: "Listening" is asked of the socket** (owner,
|
||||
2026-10-13, from the Not done list of from-header-binding).
|
||||
`health.CheckSocket` ([socket.go](../internal/health/socket.go)) stats the
|
||||
file and writes "Listening"; a process that died and left its socket
|
||||
reads ok while mail is deferred or goes out unsigned. It connects and
|
||||
closes instead, and a refused connection is an error that says nobody
|
||||
answers. Find out first, on the stand: whether the panel's user may
|
||||
connect to OpenDKIM's socket, and whether a connection closed without a
|
||||
byte ends the sender check (security.md § Accepted risks: the milter
|
||||
library panics on an empty packet) - if it does, that is fixed first, in
|
||||
this step. A test with a socket file nobody listens on. - **Opus**
|
||||
- [ ] **The `cards` layout takes its phone order from the box's number**
|
||||
(owner, 2026-10-13, from the Not done list of from-header-binding). The
|
||||
order on a phone is written in `panel.css` for four boxes to a stack
|
||||
(`.sp-cards > .column:first-child > .box:nth-child(1..4)`); a fifth box
|
||||
gets no rule and lands out of place with nothing failing. The box partial
|
||||
sets an order class from its own number (a class, not a style: the CSP
|
||||
allows no inline styles), the stylesheet orders by that class, and a test
|
||||
checks that the numbers of a rendered `cards` page run in order. A change
|
||||
to a component: the mockup's components page and `theme.css` first
|
||||
(`design:` commit). - **Sonnet**
|
||||
- [ ] **A notification is owed to each recipient, not to the subject**
|
||||
(owner, 2026-10-14; from the reviews of panel-notifications, not found by
|
||||
the audit). `Notifier.send` ([notify.go](../internal/notify/notify.go))
|
||||
counts a subject as said when one recipient's copy was handed to Postfix;
|
||||
a recipient whose copy failed, or was "held back" by the hourly ceiling,
|
||||
is never told. Unseen while the owner is the only recipient; the first
|
||||
domain administrator makes it "the global administrator knows the
|
||||
domain's DNS broke, its administrator does not". The state of a subject
|
||||
is kept per recipient (the next migration), and a later check sends to
|
||||
those not yet told - so the ceiling holds a copy back instead of dropping
|
||||
it. A user who gains the right to hear of a subject while it is already
|
||||
open is not mailed about it. "Cleared" goes to those who were told of the
|
||||
problem. Tests: one copy fails and goes out at the next check, once; the
|
||||
ceiling; a user added mid-subject. - **Opus**
|
||||
- [ ] **A DNS outage is "unknown", not "wrong"** (M14, M15; owner,
|
||||
2026-10-14 - the two findings had no step). Each resolver gets a time
|
||||
budget of its own, so a silent first resolver no longer leaves the second
|
||||
an expired context ([dnscheck/resolver.go](../internal/dnscheck/resolver.go));
|
||||
a timeout is Unknown, not "does not resolve" or "no PTR record"
|
||||
([dnscheck/server.go](../internal/dnscheck/server.go)); a failed lookup
|
||||
keeps the server's addresses it had instead of emptying them, so SPF does
|
||||
not go Unknown with it. Notifications: a domain's DNS is reported broken
|
||||
only on a result that is wrong, never on Unknown - checked in `notify`,
|
||||
and tested. Tests: a first resolver that drops packets, a PTR timeout.
|
||||
architecture.md:339-340 and the `DefaultResolvers` comment say what is
|
||||
true. — **Opus**
|
||||
- [ ] **DMARC reports tested where they come in** (owner, 2026-10-14; rows of
|
||||
review-2026-08-followups). The panel parses reports that arrive by mail
|
||||
from anyone, and the `internal/dmarc` service (`service.go`,
|
||||
`addresses.go`) and the DMARC handlers have no tests; no other step
|
||||
touches them. Tests of the service, and of a report that is malformed,
|
||||
oversized, compressed past its limit, or names a domain the instance does
|
||||
not have: refused or skipped with a reason, nothing stored, the panel
|
||||
alive. A finding is fixed in this step. — **Opus**
|
||||
- [ ] **The last untyped page data** (owner, 2026-10-13): `healthChecks`
|
||||
([handlers_status.go](../internal/web/handlers/handlers_status.go)) returns
|
||||
a struct instead of `map[string]any`, so a mistyped key in `healthPage` /
|
||||
@@ -764,14 +919,12 @@ has already run is never edited — the next file is added; the published
|
||||
### Stage 6 — documentation and roadmap sweep (no product code, no cut)
|
||||
|
||||
- [ ] Every item under *Doc drift* above, file by file. — **Sonnet**
|
||||
- [ ] roadmap.md: `template-data-typing` is struck already (owner, 2026-10-13;
|
||||
its last map is a step of stage 4); `structured-logging` is dropped (owner,
|
||||
2026-10-13, roadmap § Dropped) - nothing to reword; 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**
|
||||
- [ ] roadmap.md: done ahead on 2026-10-13/14 with the owner -
|
||||
`template-data-typing` struck, `structured-logging` dropped,
|
||||
`review-2026-08-followups` closed with each row placed in a stage or
|
||||
dropped with its reason. What is left: check that every *Test gaps* entry
|
||||
above is named by the step that touches its code, and add it to that step
|
||||
where it is not. — **Sonnet**
|
||||
- [ ] 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**
|
||||
|
||||
Reference in New Issue
Block a user