diff --git a/CHANGELOG.md b/CHANGELOG.md index a0720a2..d2b8d30 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,13 +10,13 @@ milter chain, DMARC reports are validated before they are stored, and the auto rate limit is correct for a non-default level-1 window. Schema migration `0010` (index only). Upgrading is a tag bump. +A full-tree code review on 2026-08-19 (architecture, code quality, security, +GUI, tests, documentation, AGPL compliance; 38 findings) produced most of what +follows. Its four open items live in [roadmap.md](docs/roadmap.md); its working +document is not part of the release. + ### Added -- Code review plan (`docs/plans/code-review-2026-08.md`) — full codebase audit - covering architecture, code quality, security, GUI, tests, documentation, and - AGPL compliance. 38 findings across 11 categories with a prioritised - implementation plan in 6 phases, and a status table recording what was - implemented, what was already in place, and what was declined. - Roadmap item and plan for `/preflight` installation check page ([docs/plans/preflight.md](docs/plans/preflight.md)) — instance-level infrastructure verification (rDNS, TLS, ports, HELO banner, proxy headers, @@ -109,8 +109,11 @@ rate limit is correct for a non-default level-1 window. Schema migration `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. + never carried into the review's own phase tables). Each roadmap row stands on + its own — file, symptom, and what "done" means — and the declined finding + (`//go:build linux` for the `/proc` reader) is recorded as declined so it is + not raised again. The review's working document is gone from `docs/plans/` + now that nothing depends on it; git history keeps it. - 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 diff --git a/docs/plans/code-review-2026-08.md b/docs/plans/code-review-2026-08.md deleted file mode 100644 index c3230ac..0000000 --- a/docs/plans/code-review-2026-08.md +++ /dev/null @@ -1,385 +0,0 @@ -# Рецензирование кодовой базы SelfPost — август 2026 - -Дата: 2026-08-19. - ---- - -## Общая оценка - -SelfPost — зрелый, хорошо структурированный проект. Архитектура соответствует масштабу (self-hosted SMTP-реле с веб-панелью), код читабелен и поддерживаем, тестовое покрытие обширное (~60 тест-файлов), документация исключительно качественная, лицензионное соответствие AGPL-3.0 полное. Ниже — конкретные находки и план устранения. - ---- - -## 1. Архитектура и структура проекта - -**Оценка: отлично.** Стандартная Go-компоновка (`cmd/`, `internal/`, `test/e2e/`), чёткое разделение слоёв (store → service → handler → template). Два бинарника (`panel`, `selfpost-backup`), единый Docker-образ с supervisord. Всё соответствует масштабу проекта — нет ни over-engineering, ни архитектурных долгов. - ---- - -## 2. Логические ошибки и баги - -### 2.1 [Medium] SPF-checker не обрабатывает CIDR-нотацию -- **Файл:** `internal/dnscheck/spf.go`, `parseIPs()` -- **Суть:** `ip4:192.168.1.0/24` парсится как единичный IP `192.168.1.0`. Панель ложно предупреждает, что SPF не покрывает сервер. -- **Исправление:** использовать `net.ParseCIDR` + `IPNet.Contains()`. - -### 2.2 [Low] Дублирующаяся проверка в user update (мёртвый код) -- **Файл:** `internal/web/handlers/handlers_users.go`, строки 225–239 -- **Суть:** два последовательных блока проверяют одно условие (понижение последнего глобального админа). Второй блок недостижим. -- **Исправление:** оставить один блок с контекстным сообщением. - -### 2.3 [Low] Вводящее в заблуждение flash-сообщение в настройках -- **Файл:** `internal/web/handlers/handlers_settings.go`, строки 73–94, 246–273 -- **Суть:** при изменении username+email flash говорит «сессии сброшены», хотя сессии сбрасываются только при смене пароля. Мёртвые ветки `"username-email"`, `"password-email"` в `settingsFlash()`. -- **Исправление:** строить flash-сообщение из отдельных boolean-флагов. - -### 2.4 [Low] Утечка резервации inflight при обрыве SMTP -- **Файл:** `internal/milter/inflight.go` -- **Суть:** если SMTP-соединение обрывается без `Abort()`, резервация живёт до TTL (60 сек). При высокой нагрузке возможен ложный отказ. -- **Исправление:** задокументировать компромисс в `docs/security.md`. - ---- - -## 3. Отсутствующие CSS-определения на DMARC-страницах (баги GUI) - -### 3.1 [Bug] 6 CSS-классов используются в шаблонах, но не определены в `panel.css` - -| Класс | Шаблоны | Влияние | -|---|---|---| -| `.pair` | `dmarc.html`, `dmarc_domain.html`, `dmarc_report.html` | Контейнер-сетка не работает — карточки стакаются вертикально | -| `.attn` | `dmarc_report.html` (`.card.attn`) | Нет визуального выделения при fail > 0 | -| `.desk-only` | DMARC-страницы | Таблицы не скрываются на мобильных | -| `.phone-only` / `.phone-list` | `dmarc.html` | Мобильный список виден одновременно с таблицей на всех экранах | -| `.check-row` | `domain_detail.html` | Должно быть `.check` (определённый класс) | - -**Исправление:** добавить отсутствующие правила в `panel.css`, включая media-запросы для `.desk-only`/`.phone-only`. Заменить `.pair` на `.split` или определить отдельно. Исправить `.check-row` → `.check`. - ---- - -## 4. Безопасность - -### 4.1 [Medium] Fail-open milter auth IP при ошибке БД -- **Файл:** `internal/milter/authip.go` -- **Суть:** при недоступности БД `authIPAllowed()` возвращает `true` — любой IP может отправлять. -- **Исправление:** оставить fail-open + добавить warning log. -- **Решение:** (b) fail-open с предупреждением. - -### 4.2 [Medium] Нет ограничения stdin в DMARC-ingest -- **Файл:** `cmd/panel/dmarc_ingest.go` -- **Суть:** нет `io.LimitReader` — злонамеренное сообщение может исчерпать память. -- **Исправление:** `io.LimitReader(os.Stdin, 10<<20)`. - -### 4.3 [Medium] Вытеснение bucket'ов в auth rate limiter -- **Файл:** `internal/web/auth/ratelimit.go`, `makeRoom()` -- **Суть:** при >10000 IP атакующий может вытеснить легитимные rate-limit записи, сбросив счётчики. -- **Исправление:** вытеснять bucket с ближайшим expiry, но заблокированные bucket'ы (failures >= max) не вытесняются до истечения TTL. - -### 4.4 [Medium] Нет CSRF-токенов — только `Sec-Fetch-Site` / `Origin` -- **Файл:** `internal/web/security.go` -- **Суть:** защита зависит от браузера. В `docs/security.md` решение задокументировано с обоснованием, но defense-in-depth рекомендует токен. -- **Решение:** добавить per-session CSRF-токен. - -### 4.5 [Low] CSP `unsafe-inline` для стилей -- **Файл:** `internal/web/security.go` -- **Исправление:** вынести inline-стили в stylesheet, удалить `'unsafe-inline'`. - -### 4.6 [Low] `bcrypt.DefaultCost` (10) — рекомендуется 12+ -- **Исправление:** определить `const bcryptCost = 12` и использовать везде. - -### 4.7 [Low] Нет re-auth перед скачиванием backup -- **Файл:** `internal/web/handlers/handlers_backup.go` -- **Исправление:** запросить текущий пароль в форме backup. - -### 4.8 [Very Low] Cookie без `__Host-` prefix -- **Файл:** `internal/web/auth/handlers.go` -- **Примечание:** в коде уже есть `Host-`-prefixed cookie при Secure, см. `auth_test.go`. Проверить, что покрытие полное. - -### 4.9 [Low] zip-бомба в DMARC parse -- **Файл:** `internal/dmarc/parse.go`, `unzip()` -- **Исправление:** обернуть reader в `io.LimitReader`. - ---- - -## 5. Дублирование кода - -### 5.1 [Low] `writeFileAtomic` — две одинаковые реализации -- **Файлы:** `internal/domain/dkim.go`, `internal/postfix/write.go` -- **Исправление:** извлечь в `internal/atomicfile`. - -### 5.2 [Low] `reloadViaSupervisor` — две реализации -- **Файлы:** `internal/postfix/postfix.go`, `internal/domain/opendkim.go` -- **Исправление:** извлечь в `internal/supervisor`. - -### 5.3 [Low] `parseDomainRateLimitForm` / `parseAppRateLimitForm` — идентичная логика -- **Файл:** `internal/web/handlers/handlers_ratelimit.go` -- **Исправление:** объединить в `parseRateLimitForm`. - -### 5.4 [Low] `applyDomainRateLimit` / `applyAppRateLimit` -- **Исправление:** параметризовать scope и saver. - -### 5.5 [Very Low] `logf = log.Printf` — повторяется в 5+ пакетах -- **Исправление:** при переходе на `log/slog` (см. §7) проблема уйдёт. - -### 5.6 [Very Low] `assertMapSafe` / `assertConfigSafe` — похожие проверки в нескольких пакетах -- **Исправление:** рассмотреть `internal/configsafe`. - ---- - -## 6. Граничные случаи - -### 6.1 [Low] `secretfile` принимает пустой пароль на уровне библиотеки -- **Исправление:** добавить `len(password) == 0` guard или задокументировать, что валидация на вызывающей стороне. - -### 6.2 [Very Low] Fingerprint-коллизия при ротации лога -- **Файл:** `internal/logtail/offset.go` -- **Исправление:** включить inode в fingerprint. - -### 6.3 [Low] Неограниченный вывод `postqueue -p` -- **Файл:** `internal/postfix/queue.go` -- **Исправление:** `io.LimitReader` или context timeout. - -### 6.4 [Very Low] `parsePage` не ограничивает максимум -- **Файл:** `internal/web/handlers/handlers_monitor.go` -- **Исправление:** clamp к `lastPage`. - ---- - -## 7. Рефакторинг и оптимизация - -### 7.1 [Medium] Типизированные struct'ы для template data -- **Затронуто:** все `handlers_*.go` -- **Суть:** `map[string]any` с 40+ ключами (напр. `renderDomainDetail`) — хрупко, опечатки молчаливо дают zero-value. -- **Исправление:** определить struct'ы (`DomainDetailData`, `SettingsData` и т.д.). - -### 7.2 [Medium] Структурированное логирование (`log/slog`) -- **Затронуто:** все пакеты -- **Суть:** нет уровней, структурированных полей, фильтрации. -- **Исправление:** инжектировать `*slog.Logger` через конструкторы. - -### 7.3 [Low] Build tags для `/proc`-кода -- **Файл:** `internal/health/machine.go` -- **Исправление:** `//go:build linux` + stub для других платформ. - -### 7.4 [Very Low] DNS resolver — 4 параллельных запроса вместо fallback -- **Файл:** `internal/dnscheck/resolver.go` -- **Исправление:** последовательный запрос с fallback, или задокументировать обоснование. - ---- - -## 8. Документация - -### 8.1 [Medium] CHANGELOG записывает удаление plan-файлов, которые ещё не удалены -- **Суть:** `[Unreleased]` Removed перечисляет 6 plan-файлов, но файлы ещё на диске. -- **Исправление:** проверить — файлы уже удалены; если да, пункт закрыт. - -### 8.2 [Minor] `schema-migrations.md` — заголовок говорит v9, цепочка на v10 -- **Исправление:** обновить заголовок и добавить индекс из миграции 0010. - -### 8.3 [Low] ~40 ссылок на `spec N` в build/deploy-файлах -- **Суть:** CHANGELOG `[0.5.0]` говорит «код больше не ссылается на спецификацию», но shell-скрипты и Dockerfile не были очищены. -- **Исправление:** удалить ссылки на spec; где уместно — заменить на ссылку на актуальный документ. - -### 8.4 [Low] `dmarc-ingest` symlink не документирован в `architecture.md` -- **Исправление:** добавить абзац в секцию Mail path. - -### 8.5 [Low] Roadmap ссылается на plan-файлы, которые будут удалены -- **Исправление:** убрать ссылки на удалённые планы. - -### 8.6 [Very Low] `dev/workflow.md` — устаревшая базовая версия -- **Исправление:** обновить «база 0.6.0» → актуальную. - ---- - -## 9. Тесты - -### 9.1 Общая оценка: сильное покрытие - -Набор из ~60 тест-файлов покрывает: криптографию (`secretfile`), конкурентность (`milter` — 32 горутины), SQL edge cases (`store`), безопасность (CSRF-матрица, cookie-shadowing, injection-rejection, fail-open семантика), AGPL-compliance guards, e2e-сценарий. Тесты хорошо документированы комментариями. - -### 9.2 Пакеты без тестов - -| Пакет | Риск | -|---|---| -| `cmd/panel` (main.go, httpserver.go, journal.go, dmarc_ingest.go, ratelimit_recalc.go) | Low — startup-код, частично покрыт restore_test | -| `internal/dmarc` (service.go, addresses.go) | Medium — сервисный слой не протестирован | -| `internal/web/handlers` (handlers_apps.go, handlers_domains.go, handlers_users.go, handlers_dmarc.go, handlers_dmarc_reports.go, handlers_status.go, handlers_help.go) | Medium — нет прямых тестов (частично покрыты authz_test) | -| `internal/store` (store.go, sessions.go) | Low — sessions покрыты через auth_test | -| `internal/buildinfo` | Very Low — константы | - -### 9.3 Слабые области тестирования -- `internal/postfix/dmarc_test.go` — только 2 теста, нет проверки injection -- `internal/web/validate/validate_email_test.go` — 4 кейса, тонкое покрытие -- `internal/store/stats_test.go` — нет edge case с нулевым трафиком - -### 9.4 Отсутствуют тесты для -- Error page templates -- Corrupted backup archive handling -- Malformed XML в DMARC parse (oversized reports) -- Pagination edge cases в delivery log -- Concurrent store operations - ---- - -## 10. GUI — дополнительные находки - -### 10.1 [Low] Accessibility: help drawer -- Кнопка закрытия `