From 0aa7318a41dd74bd472b050e40a85f10bdc6e7ec Mon Sep 17 00:00:00 2001 From: Mikhail Yenuchenko Date: Wed, 19 Aug 2026 15:33:43 +0300 Subject: [PATCH] docs: add full codebase review plan 38 findings across architecture, code quality, security, GUI, tests, documentation, and AGPL compliance. Prioritised 6-phase implementation plan with model recommendations. Co-Authored-By: Claude Sonnet 4.6 Co-authored-by: Cursor --- CHANGELOG.md | 7 + docs/plans/code-review-2026-08.md | 311 ++++++++++++++++++++++++++++++ 2 files changed, 318 insertions(+) create mode 100644 docs/plans/code-review-2026-08.md diff --git a/CHANGELOG.md b/CHANGELOG.md index d29cbc1..b13c739 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,6 +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. +### 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. + ### Removed - Plan documents for already-shipped features: `domain-stats-auto-ratelimit`, diff --git a/docs/plans/code-review-2026-08.md b/docs/plans/code-review-2026-08.md new file mode 100644 index 0000000..e89f94d --- /dev/null +++ b/docs/plans/code-review-2026-08.md @@ -0,0 +1,311 @@ +# Рецензирование кодовой базы 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 сек). При высокой нагрузке возможен ложный отказ. +- **Исправление:** TTL достаточен, но стоит задокументировать компромисс. + +--- + +## 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-closed режим. + +### 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, а не самый старый. + +### 4.4 [Medium] Нет CSRF-токенов — только `Sec-Fetch-Site` / `Origin` +- **Файл:** `internal/web/security.go` +- **Суть:** защита зависит от браузера. В `docs/security.md` решение задокументировано с обоснованием, но defense-in-depth рекомендует токен. +- **Статус:** осознанный компромисс; задокументирован. Оставить как есть или добавить per-session токен. + +### 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-файлов, но файлы ещё на диске. +- **Исправление:** удалить файлы или убрать запись из CHANGELOG. + +### 8.2 [Minor] `schema-migrations.md` — заголовок говорит v9, цепочка на v10 +- **Исправление:** обновить заголовок и добавить индекс из миграции 0010. + +### 8.3 [Low] ~40 ссылок на `spec N` в build/deploy-файлах +- **Суть:** CHANGELOG `[0.5.0]` говорит «код больше не ссылается на спецификацию», но shell-скрипты и Dockerfile не были очищены. +- **Исправление:** удалить или заменить ссылки на spec в build/, deploy/, Makefile, ранних миграциях. + +### 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 +- Кнопка закрытия `