diff --git a/CHANGELOG.md b/CHANGELOG.md index 1c50cfe..71a636e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -76,6 +76,12 @@ Format follows [Keep a Changelog](https://keepachangelog.com/en/1.1.0/); version ### Changed +- docs: the 2026-08-13 full-tree review plan is complete — every phase (P0–P7) + is closed — and `docs/plans/code-review.md` is deleted per its own exit + criteria (history in git and in this file). The + [roadmap](docs/roadmap.md)'s recommended order returns to **queue-retries** + and then **inbound-relay**. + - licence: [NOTICE](NOTICE) tells modifiers to update `SourceURL` in `internal/legal/legal.go` (the value the panel footer actually injects), not `layout.html`. `release.yml` refuses a `workflow_dispatch` whose version is @@ -138,7 +144,8 @@ Format follows [Keep a Changelog](https://keepachangelog.com/en/1.1.0/); version prompts existed — and why that is acceptable (the prompt is a mis-click guard, not an authorization boundary). -- docs: full-tree review plan ([docs/plans/code-review.md](docs/plans/code-review.md)) +- docs: full-tree review plan (`docs/plans/code-review.md`, since completed + and deleted — history in git) — architecture, quality, GUI, tests, licence; P0 is domain-admin send-log authorization. Roadmap queues that plan ahead of inbound-relay and records `schema-squash` (replace the 1.x SQLite migration chain with a 2.x baseline; diff --git a/docs/plans/code-review.md b/docs/plans/code-review.md deleted file mode 100644 index a209ccd..0000000 --- a/docs/plans/code-review.md +++ /dev/null @@ -1,672 +0,0 @@ -# Plan: code-review (full-tree review follow-ups) - -**Status:** agreed -**Date:** 2026-08-13 -**Reviewer:** Cursor Grok 4.6 (whole-tree authorship review). This is **not** -the Fable pre-release security audit in [development.md](../development.md) § -Model routing; a Fable pass is a later step on the P0 diff. -**Version:** patch for defects; docs/UI follow-ups have no schema. -**Order:** **P0 before inbound-relay.** P0 is a shipped RBAC hole, not a -feature. Remaining phases after P0, or interleaved with inbound-relay by -agreement. - ---- - -## Goal - -Record the 2026-08-13 full-tree review (architecture, complexity, quality, -docs, maintainability, logic, refactor, licence, legacy, stubs, GUI, disputed -decisions, edge cases, tests, duplication) and a phased implementation -checklist with a recommended model per step, using the routing table in -[development.md](../development.md). - ---- - -## Verdict - -SelfPost is a compact, well-bounded 1.x product: one image, one SQLite file, -thin `domain`/`app` services for multi-store writes, a fail-open journal -milter with a Postfix level-1 backstop, and documentation that is unusually -honest about accepted risks. Complexity matches the scale (~10.7k production -Go lines, ~5.8k unit-test lines, ~1.2k HTML, 749 CSS, 281 JS). Comments -explain decisions rather than restating code. - -The outstanding defect is **send-log authorization for domain administrators** -(confidentiality). After that, the work is tightening a few fail-open paths, -catching docs that froze at “single administrator”, small GUI bugs from the -1.2.x layout pass, and filling test gaps around auth/RBAC. Do **not** use this -review as a licence to rewrite layers, squash migrations in 1.x, or add CSRF tokens -without revisiting the ADR. - ---- - -## How to read this file - -Findings are grouped by the sixteen review questions. Each finding has a -severity (**H**igh / **M**edium / **L**ow / **I**nfo). The implementation -checklist at the end is the work queue; it names the model for each step. - -**Models** (from [development.md](../development.md)): - -| Kind of work | Model | -|---|---| -| Security, infra, mail path, permissions, open-relay risk | **Opus** | -| UI / JS / CSS, templates, documentation (English) | **Sonnet** | -| Trivial mechanics: retarget links, grep, compose bump, comment fixes | **Haiku** | -| Security **review** of a diff (not authorship) | **Fable** | - -Reviewers must not be the author of the code under review. - ---- - -## 1. Architecture / structure - -**Proportionate.** Composition root in `cmd/panel` (HTTP + journal-milter + -log-tailer, one `*store.Store`). `internal/domain` and `internal/app` own -multi-store writes and rollback. Adapters (`postfix`, `milter`, `logtail`, -`dnscheck`, `health`, `backup`, `secretfile`) are the only infrastructure-aware -code. Interfaces exist where they break import cycles or enable fakes -(`domain.Applications`, `app.SenderMaps`, `milter.Store`, `logtail.StatusStore`) -— not as a DI framework. - -Handlers may call `store` directly for single-table reads (documented in -[architecture.md](../architecture.md) § Code layers). That is followed for -sessions, send-log queries, users, and DMARC settings. It is not a layering -violation; it is an incomplete service boundary that will hurt if those -surfaces keep growing. - -No circular Go imports. `MaxOpenConns(1)` on SQLite is an intentional -single-connection trade-off for the three in-process roles. - -| Sev | Finding | -|---|---| -| **M** | Users, send-log listing, and global DMARC settings have no service; handlers talk to `store`. Fine at current size; do not invent a service until a second writer appears. | -| **L** | `auth.RequireGlobal` (`internal/web/auth/middleware.go`) is unused; handlers duplicate `requireGlobal`. Either wire the middleware on `/users`, `/backup`, `/status`, `/mail-queue`, `/system-log` or delete the unused helper. | -| **L** | `HandleAccount` / `handlers_account.go` still use the pre-1.2.3 “account” name while the route is `/settings`. | -| **I** | Package comment on `internal/store` still says “the administrator account” after migration `0005` replaced `admin` with `users`. | - -**Do not:** introduce a repository layer, split the panel binary, or move -SQLite behind an interface “for testability” — the existing fakes are enough. - ---- - -## 2. Complexity vs project scale - -The code is **not over-engineered**. A few files are large because the problem -is large, not because of unused abstraction: - -| File | ~Lines | Note | -|---|---|---| -| `internal/health/machine.go` | 622 | Cohesive `/proc` sampler | -| `internal/logtail/logtail.go` | 528 | Follow + rotate + reconcile + retention | -| `internal/web/handlers/handlers_monitor.go` | 481 | Send-log UI + authz (this is where P0 lives) | -| `internal/web/view/templates/domain_detail.html` | 476 | DNS + apps + limits + export; composition debt | -| `internal/secretfile/secretfile.go` | ~423 | Isolated crypto envelope | -| `internal/web/view/static/panel.css` | 749 | Tokens + layout; comment-heavy by design | - -Comments are long and mostly load-bearing (threat, fail-open, why not the -obvious alternative). The cost is scanability: some files are 30–40% prose. -That matches the project’s disclosed AI-authorship style -([development.md](../development.md) § Authorship). Do not strip comments in -the name of “cleanup”. Update the stale ones (see §4). - ---- - -## 3. Code quality - -Naming matches the docs (`domain` / `application`, level-1 / level-2). Errors -on the mail path log-and-continue (intentional fail-open). Panel paths log and -return 4xx/5xx. `crypto/rand` failure panics in `auth/token.go` — acceptable. - -Context is used for process lifetime and DNS timeouts, not for SQLite (correct -with one connection). Dashboard DNS checks write distinct `rows[i]` from -goroutines; Go 1.22+ loop semantics make that safe (`go.mod` is 1.26). - -Magic numbers are mostly named (`reservationTTL`, `renewThreshold`, CSP/HSTS). -Env defaults live in `loadConfig`. - -| Sev | Finding | -|---|---| -| **M** | `sessionStore.Create` logs a DB error and still returns the token (`internal/web/auth/session.go`). Login sets the cookie; the next request bounces to `/login`. Fail closed: no cookie, error page. | -| **M** | `app.Service.Delete` removes the registry row **before** SASL / rate-limit cleanup. SASL failure → orphaned `sasldb2` account that can still authenticate. Domain delete does SASL first (`domain/service.go`). Align app delete with that order (or compensate: restore the row on SASL failure). | -| **L** | Login/setup `rateLimiter` sweeps expired buckets only when creating a **new** key. Many unique IPs grow the map until restart. Cap the map or sweep on a timer. | -| **L** | `parseTrustedProxies` skips invalid CIDRs instead of refusing to start. Silent misconfiguration of `TRUSTED_PROXY_CIDR`. | - ---- - -## 4. Documentation completeness vs code; comments - -Docs are a first-class artefact (env regression test, architecture as-built, -security accepted-risks). The drift is concentrated where **domain-admin -shipped in 1.2.0** and several files still argue “single-user”. - -| Sev | Finding | -|---|---| -| **M** | [guide.md](../guide.md) Operations never mentions **Users** (`/users`) or the domain-admin role. Architecture and product do. An operator reading only the guide does not know the panel is multi-user. | -| **M** | [security.md](../security.md) CSRF ADR still says the panel is single-user and “revisit if multi-user”. Multi-user shipped. The origin-check decision can stand; the **rationale and revisit trigger must be rewritten**. | -| **M** | [security.md](../security.md) says passwords are “bcrypt (or argon2)”. Code is bcrypt only. | -| **M** | `internal/backup` package comment claims the panel **regenerates** Postfix/OpenDKIM maps from SQLite on every start after restore. Startup only runs `CheckRestore` (`cmd/panel/main.go`). Maps/keys are **in** the tarball. Heal path is the Status **Reload** button. | -| **L** | Architecture route table omits `/license` and the `/account` → `/settings` 308. | -| **L** | Guide Settings section: “change the administrator username and/or password” — global Settings also has the default DMARC `rua=` address. | -| **L** | Guide does not warn that restoring an **older** backup can resurrect sessions (architecture does). | -| **L** | Guide rate-limiting section does not stress that level 2 is **fail-open** (store error or missing client IP → mail continues; level 1 is the backstop). | -| **L** | [roadmap.md](../roadmap.md) and [plans/dmarc-reports.md](dmarc-reports.md) still say `admin.dmarc_report_email` after `0005` moved it to `settings`. | -| **L** | `setupManager` comments still say “admin row”; the fact is `users` / `UserExists()`. “Plan B.1 / C.4” comments are opaque to outsiders; keep them, they are history, not errors. | -| **I** | E2e coverage summary in development.md omits logrotate and supervisor-process checks that actually run. | -| **I** | [plans/logrotate-mode.md](logrotate-mode.md) is **done** but still in `docs/plans/` (active-plans directory). History belongs in git / CHANGELOG. | - -Comments in production code are generally **high quality**. Missing comments -are on domain-admin authorization policy in `sendLogData` (the P0 hole has no -comment stating the intended invariant) and on `rateLimiter` memory bounds. - ---- - -## 5. Human readability and maintainability - -A new maintainer can follow the tree from [architecture.md](../architecture.md) -into `cmd/panel` → `internal/web/web.go` → services. Tests document *why* -(milter in-flight, queue-id anchoring, CSRF matrix). - -Friction: - -- `domain_detail.html` is the hardest HTML file to edit (repeated DNS - host/type/value blocks, checkbox “Edit” panels). -- `panel.css` structure-tied selectors (`.muted + form > select:first-of-type`) - will break on a copy change. -- Dual `CurrentUser` + `Principal` is redundant but works (`withPrincipal` - sets both). -- `assignedDomains` loads **all** domains then filters in Go, while - `store.listUserDomainNames` already exists and is unused by handlers. - -None of this blocks maintenance at current size. Prefer small extractions -(DNS partial, `tryAdmit`) over a layer rewrite. - ---- - -## 6. Logical errors - -### H — Domain-admin send-log list leaks other domains - -Detail page checks membership (`HandleDelivery`). The **list** does not. - -`sendLogData` in `internal/web/handlers/handlers_monitor.go`: - -- Empty `SendLogFilter.Domain` means “all rows” (`internal/store/sendlog.go`). -- For a non-global user, a disallowed `?domain=` is cleared to `""`. The - assigned domain is filled in **only when there is exactly one**. -- A domain-admin with **0 or ≥2** assigned domains and no (or a forged) - domain filter therefore sees **every** send-log row (From, To, Subject). -- `?app=` is applied to SQL **before** it is checked against the user’s - application logins. The allowlist only updates the template’s selected - filter. Forged `?app=` with an empty domain filter returns - that application’s rows. - -The deliveries table’s domain dropdown still lists only assigned domains, so -the leak is silent. - -**Invariant to implement:** a non-global principal’s `QuerySendLog` / -`CountSendLog` are always constrained to assigned domain names; if that set is -empty, the result is empty. Validate `AppLogin` against the allowlist -**before** the query. - -### M — Level-2 check/reserve race - -`enforceLimit` calls `flight.count` then `flight.reserve` under **separate** -mutex acquisitions (`internal/milter/ratelimit.go`, `inflight.go`). Two MAIL -FROM handlers can both observe `n == max-1` and both reserve. In-flight -tracking closes the *stored-count* race (and -`TestRateLimitCountsInFlightMessages` covers the **sequential** case). It does -not close parallel check-then-act. Severity is tempered by fail-open and -Postfix level-1. Fix: one `tryAdmit(key, since, max)` under the inflight -mutex. - -### M — Session create fail-open - -See §3. Not a stolen-session bug (hash never lands in the DB); it is a -logged-in-looking cookie that cannot be looked up. - -### M — App delete ordering - -See §3. Orphaned SASL is a mail-path consistency bug. - -### L — Domain export `Version` ignored on import - -`internal/domain/transfer.go` stamps `buildinfo.Version`; import checks format -only. Lower risk than full-backup `CheckRestore`; still a cross-version footgun. - -### I — Journal milter fail-open; origin CSRF fail-open; queue-reconcile -`bounced` - -Documented accepted risks in [security.md](../security.md). Not defects. -Revisit the CSRF ADR’s *framing* (multi-user), not necessarily the mechanism. - ---- - -## 7. Refactoring and optimisation - -Worth doing, in order: - -1. `tryAdmit` (correctness, not speed). -2. `SendLogFilter` domain IN-list (correctness). -3. DNS field partial + settings credentials partial (drift). -4. One helper for the five `panel.js` show/hide field pairs. -5. `assignedDomains` via SQL for the current user (clarity, not performance). - -Not worth doing now: - -- Service layer for users / send-log. -- Replacing SQLite, HTMX, or the single-container model. -- Squashing migrations `0001`–`0005`. -- CSRF tokens (see §12). -- Rewriting `machine.go` or `logtail.go` for size. - ---- - -## 8. Licence (AGPL-3.0) - -Packaging is largely correct: root `LICENSE` ≡ embedded `internal/legal/LICENSE` -(test), unauthenticated `/license`, footer copyright + Source + “No warranty” -on login/setup, image copies `LICENSE`/`NOTICE`, Go deps are BSD-family, htmx -is 0BSD. Network-use §13 is stated in `NOTICE` and the README. - -| Sev | Finding | -|---|---| -| **M** | IBM Plex WOFF2 files are shipped without the SIL OFL 1.1 text. OFL requires the licence to travel with the font. Add `OFL.txt` next to the fonts (and mention the path in `NOTICE`). | -| **M** | `NOTICE` tells modifiers to change the Source URL in `layout.html`. The URL is `legal.SourceURL` in `internal/legal/legal.go`, injected by `view.go`. | -| **L** | `/license` serves LICENSE only, not NOTICE. Optional: serve NOTICE at `/notice` or append attributions. | -| **L** | SPDX headers only on `cmd/panel` and `cmd/selfpost-backup`. AGPL does not require per-file SPDX; either add them everywhere or drop the two so the convention is consistent. | -| **I** | Debian package licences are pointed at packages.debian.org rather than a pinned list — normal for an image that installs from bookworm. | - -No AGPL-incompatible Go dependency found in `go.mod`. - ---- - -## 9. Legacy code and migrations - -| Migration | Role | Removal | -|---|---|---| -| `0001_init.sql` | Core schema (including historical `admin`) | Keep for all **1.x** (`PRAGMA user_version` chain) | -| `0002_sessions.sql` | DB sessions | Keep for 1.x | -| `0003_logtail_state.sql` | Tailer offset | Keep for 1.x | -| `0004_dmarc_report_email.sql` | DMARC columns on `admin` | Keep for 1.x; `0005` moves the data | -| `0005_panel_users.sql` | `users` / `user_domains`; `DROP TABLE admin` | Keep for 1.x | - -Squash is deferred to **2.x** — [roadmap.md](../roadmap.md) `schema-squash`. -Until then do not delete, rename, or reorder these files. Document the 1.x -rule in architecture § Persistence (one sentence). - -Compat shims to keep until a major: - -- `GET/POST /account` → 308 `/settings`. -- Domain rate-limit rows may still have an unused IP list column; enforcement - ignores it. - -`sessions.username` is a string, not a `user_id` FK. Renames update the column; -a missed rename would orphan sessions. Acceptable; a FK would be a 1.x -migration if usernames become mutable in more places. - -**Delete** `docs/plans/logrotate-mode.md` once this review is the active plan -(status `done`; history is git / CHANGELOG `[1.2.3]`). - ---- - -## 10. Stubs and claimed-but-unimplemented behaviour - -| Item | Status | -|---|---| -| Inbound relay | Agreed plan, **no code stubs**, no `INBOUND_RELAY_*` env. Correct. | -| DMARC report **ingestion** | Candidate. UI copy already promises “a future release will be able to receive reports in the panel”. Settings `rua=` and DNS guidance **are** implemented. | -| `panel-docs` | Candidate. Status blurbs were removed in 1.2.2 in favour of this item. | -| `CONTRIBUTING.md` | Candidate, file absent. Matches roadmap. | -| CSRF tokens | Explicitly not implemented (ADR). | -| `auth.RequireGlobal` | Dead helper, not a feature stub. | - -The DMARC “future release” sentence is the only user-visible promise of -unimplemented behaviour. Soften it to “SelfPost does not receive inbound mail” -or keep it and treat `dmarc-reports` as the fulfilment — product call, Sonnet -copy. - ---- - -## 11. GUI: hacks and layout composition - -The panel is CSP-strict (no inline script/style; `TestNoTemplateUsesInlineScriptOrStyle`). -No `!important`. Progressive enhancement is real (pages work without JS). -Adaptive polling in `panel.js` is a **documented** workaround: HTMX -`hx-trigger="every Ns [expr]"` uses `new Function`, which CSP would break. - -| Sev | Finding | -|---|---| -| **M** | `RateLimitErr` uses `class="flash error"`. `.flash` is the **success** surface; `.error` only recolors text. There is no `.flash.error` rule. Validation failures look like success (red text on green). `domain_detail.html` + `panel.css`. | -| **M** | User **Delete** has no `data-confirm` and no confirm page. App delete / regen / rate-limit clear do; domain delete has `domain_delete.html`. One mis-click removes a panel user. | -| **M** | `domain_detail.html` repeats Host/Type/Value/`code-row` for DNS status **and** publishable records. Extract a partial (same pattern as `encrypt_fields.html`). | -| **M** | `settings.html` duplicates the credentials form (global split vs domain-admin narrow card). Drift already visible in the muted help text. | -| **L** | Adaptive polling: `outerHTML` swap every 5 s can steal clicks / focus; poll failures retry silently. Consider `aria-live="polite"` and a visible retry/error. Do not switch back to `hx-trigger="every"` under this CSP. | -| **L** | Checkbox-driven Edit panels instead of `
` (commented in the template). Works without JS; no `aria-expanded`. | -| **L** | Five near-identical show/hide helpers in `panel.js`. Encrypt/import fields can flash visible before `DOMContentLoaded`. | -| **L** | `hx-get` query params in `deliveries_rows.html` are not `urlquery`-encoded. Safe while domain/app charset is locked down. | -| **L** | Applications on a domain page are unpaginated. Fine until an operator has dozens of apps. | -| **L** | `