diff --git a/CHANGELOG.md b/CHANGELOG.md index e88a646..a0720a2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -103,6 +103,14 @@ rate limit is correct for a non-default level-1 window. Schema migration ### Changed +- [roadmap.md](docs/roadmap.md) now carries everything the 2026-08-19 code + review left unimplemented, so no finding lives only in a plan file: + `csrf-tokens` (the recorded §4.4 decision that never became a task), + `template-data-typing`, `structured-logging`, and + `review-2026-08-followups` for the small remainder (backup re-auth, the + missing service/handler/error-page tests, and nine minor findings that were + never in the phase tables). The review plan's status table links each row to + its roadmap item; the one declined finding is marked as declined in both. - Extracted `internal/configsafe` — the empty-value and forbidden-character check every generated config file shares. Each writer keeps its own forbidden set next to the file it protects (an OpenDKIM table and a Postfix map break on @@ -414,7 +422,7 @@ Plex. Upgrading from 1.2.x is a tag bump; no migration. - docs: agreed roadmap item **queue-retries** — show this Postfix's retry policy (first delay, backoff cap, queue lifetime) on Mail queue and on a delivery's history, reading `postconf -h` once at panel start so a manual - override is visible. Plan: [docs/plans/queue-retries.md](docs/plans/queue-retries.md). + override is visible. Plan: `docs/plans/queue-retries.md` (removed once shipped). Explanation only; no attempt counter and no panel knobs. Not yet implemented. @@ -606,7 +614,7 @@ Plex. Upgrading from 1.2.x is a tag bump; no migration. RBAC in the doc-alignment pass above). Corrected stale `admin.dmarc_report_email` references in [roadmap.md](docs/roadmap.md) and - [docs/plans/dmarc-reports.md](docs/plans/dmarc-reports.md) to the setting's + `docs/plans/dmarc-reports.md` (removed once shipped) to the setting's actual home after migration `0005`. No behaviour change. - panel: code-review P6 cleanup — the unused `auth.RequireGlobal` middleware is diff --git a/docs/plans/code-review-2026-08.md b/docs/plans/code-review-2026-08.md index 30c36d8..c3230ac 100644 --- a/docs/plans/code-review-2026-08.md +++ b/docs/plans/code-review-2026-08.md @@ -336,18 +336,18 @@ SelfPost — зрелый, хорошо структурированный пр | 14 | Ссылки на spec в build/deploy | сделано: `build/`, `deploy/`, миграции, workflow | | 15 | `dmarc-ingest` symlink | не требовалось — уже описан в `architecture.md` | | 16 | Ссылки в roadmap | сделано | -| 17 | Типизированные template data structs | **не сделано** | -| 18 | Миграция на `log/slog` | **не сделано** | +| 17 | Типизированные template data structs | **не сделано** → `template-data-typing` | +| 18 | Миграция на `log/slog` | **не сделано** → `structured-logging` | | 19 | Build tags для `machine.go` | отклонено, см. ниже | | 20 | Guide URL → `{{.SourceURL}}` | сделано | -| 21 | Тесты DMARC service layer | **не сделано** | -| 22 | Тесты хендлеров | **не сделано** | +| 21 | Тесты DMARC service layer | **не сделано** → `review-2026-08-followups` | +| 22 | Тесты хендлеров | **не сделано** → `review-2026-08-followups` | | 23 | postfix/dmarc injection cases | сделано | | 24 | `validate_email_test` | сделано | -| 25 | Тесты error pages / битого бэкапа | **не сделано** | +| 25 | Тесты error pages / битого бэкапа | **не сделано** → `review-2026-08-followups` | | 26 | CSP `unsafe-inline` | не требовалось — директивы в CSP нет | | 27 | bcrypt cost 12 | сделано, включая setup-аккаунт (`auth.BcryptCost`) | -| 28 | Re-auth перед бэкапом | **не сделано** | +| 28 | Re-auth перед бэкапом | **не сделано** → `review-2026-08-followups` | | 29 | Accessibility help drawer | сделано | | 30 | Ограничение `postqueue -p` | сделано: таймаут 5 с + лимит вывода 4 МиБ | | 31 | `internal/configsafe` | сделано | @@ -361,9 +361,25 @@ SelfPost — зрелый, хорошо структурированный пр **Открытый вопрос.** §4.4: решение «добавить per-session CSRF-токен» записано, но задачи под него в таблицах фаз нет, и токенов в коде нет. Нужно либо -завести задачу, либо вернуть §4.4 в статус осознанного компромисса. +реализовать токены, либо вернуть §4.4 в статус осознанного компромисса и +оставить это решение в одном месте — в `security.md`. **Вне фазовых таблиц** (в план не переносились и не делались): §5.5, §6.1, §6.2, §6.4, §7.4, §8.6, §10.3, §10.4, §10.5, а также §9.3–9.4 сверх задач 23–25. +--- + +## Что перенесено в roadmap + +Всё незакрытое заведено в [roadmap.md](../roadmap.md) — этот файл больше не +единственное место, где живут остатки: + +| Пункт плана | Элемент roadmap | +|---|---| +| §4.4 CSRF-токен | [csrf-tokens](../roadmap.md#csrf-tokens) | +| Задача 17 (§7.1) | [template-data-typing](../roadmap.md#template-data-typing) | +| Задача 18 (§7.2), §5.5 | [structured-logging](../roadmap.md#structured-logging) | +| Задачи 21, 22, 25, 28 и §6.1, §6.2, §6.4, §7.4, §8.6, §9.3, §10.3–10.5 | [review-2026-08-followups](../roadmap.md#review-2026-08-followups) | +| Задача 19 (§7.3) | не переносится — отклонено, обоснование выше | + diff --git a/docs/roadmap.md b/docs/roadmap.md index fe7cfc5..fe81ac2 100644 --- a/docs/roadmap.md +++ b/docs/roadmap.md @@ -32,6 +32,10 @@ in `git log` and [CHANGELOG.md](../CHANGELOG.md). | contributing | `CONTRIBUTING.md` | candidate | — | — | | inbound-antispam-panel | Inbound antispam journal + allow/deny lists | agreed | 1/12 | [plans/inbound-antispam-panel.md](plans/inbound-antispam-panel.md) | | inbound-quarantine | Inbound spam quarantine (hold / review / release) | candidate | — | [plans/inbound-quarantine.md](plans/inbound-quarantine.md) | +| csrf-tokens | Per-session CSRF tokens on state-changing forms | candidate | — | — | +| template-data-typing | Typed structs for template data instead of `map[string]any` | candidate | — | — | +| structured-logging | `log/slog` with levels and fields | candidate | — | — | +| review-2026-08-followups | Remaining findings of the 2026-08-19 code review | candidate | — | [plans/code-review-2026-08.md](plans/code-review-2026-08.md) | | schema-squash | Squash SQLite migrations into a 2.x baseline | **2.x** | — | — | **Recommended order** (not binding): the next feature is @@ -42,6 +46,14 @@ in `git log` and [CHANGELOG.md](../CHANGELOG.md). queue-retries in `[1.3.1]`; the 2026-08-13 full-tree review follow-ups are in `[1.3.0]`. Candidates need explicit agreement before they join the queue. +The 2026-08-19 code review ([plans/code-review-2026-08.md](plans/code-review-2026-08.md)) +is closed except for four items, all listed above and none of them urgent: +[csrf-tokens](#csrf-tokens), [template-data-typing](#template-data-typing), +[structured-logging](#structured-logging) and +[review-2026-08-followups](#review-2026-08-followups). What that review +already produced is in [CHANGELOG.md](../CHANGELOG.md) `[Unreleased]`; the +per-task status table is at the end of its plan file. + After a context reset, pick an item marked `agreed` or `in progress`, then work the **Implementation checklist** in its linked plan. The `Progress` column above is `done/total` checklist steps in that plan ([development.md](development.md) @@ -163,6 +175,117 @@ falls back to `postmaster@SELFPOST_HOSTNAME`. --- +## csrf-tokens + +**Goal:** a per-session CSRF token on every state-changing form, checked +server-side in addition to the `Sec-Fetch-Site` / `Origin` checks the panel +relies on today. + +**Boundary:** defence in depth, not a replacement — the header checks stay and +keep covering requests that carry no form. The token is bound to the existing +session record; no new session model, no cookie changes. + +**Done when:** every POST form and htmx fragment that changes state carries a +token, a missing or mismatched token is refused with a test that proves it, and +[security.md](security.md) describes the layered model instead of arguing that +the header checks alone are sufficient. + +**Dependencies / risks:** the current documented decision in +[security.md](security.md) is that header checks suffice; this item exists +because the 2026-08-19 review recorded the opposite decision without ever +turning it into a task (§4.4). Confirm which of the two stands before writing +code — the honest outcomes are "implement tokens" or "keep headers and say so +in one place only". Token plumbing through htmx swaps is where the work is. + +**Version:** `1.x` MINOR; `candidate`. + +--- + +## template-data-typing + +**Goal:** replace `map[string]any` template payloads in `internal/web/handlers` +with typed structs (`DomainDetailData`, `SettingsData`, …), so a renamed or +mistyped key is a compile error instead of a silently empty page section. + +**Boundary:** handler-to-template plumbing only; no template redesign, no route +or behaviour change. `renderDomainDetail` (~40 keys) is the worst case and the +one that motivates the item. + +**Done when:** the page handlers pass structs, the shared footer/base fields +still arrive on every page, and the template guard tests pass unchanged. + +**Dependencies / risks:** mechanical but wide — it touches every +`handlers_*.go` and every template that reads those keys; a missed key is +invisible unless a test renders that page. Worth pairing with the handler +tests in [review-2026-08-followups](#review-2026-08-followups) so the pages +are covered before they are rewired. Review estimate: 2–3 hours (§7.1). + +**Version:** no bearing on semver; `candidate`. + +--- + +## structured-logging + +**Goal:** move from `log.Printf` to `log/slog` — levels, structured fields, and +one logger injected through constructors instead of a package-level `logf` in +five packages (§5.5, §7.2). + +**Boundary:** internal logging only. The log destination stays stdout/stderr +under supervisord; no log shipping, no new dependency, and the operator-visible +lines the Status page and the log tailer parse must keep parsing. + +**Done when:** packages take a `*slog.Logger`, the per-package `logf = log.Printf` +aliases are gone, levels are used deliberately (an error is not an info), and +the panel's own journal/log-tailer expectations still hold. + +**Dependencies / risks:** the log tailer reads Postfix's log, not the panel's, +so the risk is contained — but anything that greps the panel's output (e2e, +operator habits) sees a new format. Review estimate: 3–4 hours (§7.2). + +**Version:** no bearing on semver; `candidate`. + +--- + +## review-2026-08-followups + +**Goal:** the leftovers of the 2026-08-19 code review that are too small to be +roadmap items of their own. The full findings and the per-task status table are +in [plans/code-review-2026-08.md](plans/code-review-2026-08.md). + +| § | Item | +|---|---| +| §4.7 | Re-authenticate (current password) before a backup download | +| §9.2 | Tests for the DMARC service layer (`service.go`, `addresses.go`) | +| §9.2 | Tests for the apps / domains / users / dmarc handlers | +| §9.4 | Tests: error page templates, corrupted backup archive, delivery-log pagination edges | +| §9.3 | Thin spots: `store/stats_test` zero-traffic case, concurrent store operations | +| §6.1 | `secretfile` accepts an empty password at library level — guard or document | +| §6.2 | Log-rotation fingerprint collision in `logtail/offset.go` — include the inode | +| §6.4 | `parsePage` does not clamp to `lastPage` | +| §7.4 | `dnscheck` resolver fans out four parallel queries instead of falling back — decide or document | +| §8.6 | `dev/workflow.md` still names an outdated base version | +| §10.3 | `.pair` vs `.split`: two class names for one layout | +| §10.4 | Two `back_link` elements in `dmarc_domain.html` | +| §10.5 | `initShowWhen` is not re-run after an htmx swap (safe today, fragile) | + +**Boundary:** no feature work and no behaviour change an operator would notice, +apart from the backup re-auth prompt. Anything here that grows past an hour +becomes its own roadmap item instead. + +**Done when:** each row is either implemented or struck with a written reason, +and the status table in the plan file says which. + +**Dependencies / risks:** the test rows overlap +[template-data-typing](#template-data-typing) — write the handler tests first +if both are taken up. §7.3 (build tags for the `/proc` reader) was **declined** +during the review pass and is deliberately absent: that code is parameterised by +root, has no platform-specific calls, already degrades gracefully without +`/proc`, and tagging it `linux` would only delete cross-platform test coverage. + +**Version:** no bearing on semver; `candidate`. + +--- + ## schema-squash **Goal:** when 2.x is cut, stop shipping the 1.x migration chain in the binary