ipam_control/docs/reviews/2026-09-27-codebase-review.md

Ревью кодовой базы — 2026-09-27

Объём: рабочее дерево поверх 5210ba3 (незакоммиченные изменения 031–032, миграции 0010–0012). Метод: сверка с docs/reviews/2026-09-27-changes-032-review.md → построчное чтение app/, alembic/versions/0010–0012, diff web/app.js → воспроизведение на стенде ipam_control_006 (API + SQL) → pytest. Временные данные удалены.

Итог

Находки ревью 032 не исправлены, все подтверждены повторно (pytest: 3 failed / 11 passed — те же три теста). Собственное ревью добавляет одну уязвимость изоляции организаций (№ 9, подтверждена на стенде) и несколько мелких замечаний. Изменение 031 существует только как план — в коде не реализовано.

Рекомендация: не коммитить 032 до исправления № 1–4 и № 9.

Статус находок ревью 032

#	Серьёзность	Статус	Проверка
1	Критическая	Не исправлено	FK audit_log.organization_id без ON DELETE SET NULL (модель + 0012). Уточнение: одного SET NULL по существующим строкам недостаточно — delete_org сам пишет audit(..., organization_id=id) в той же транзакции перед DELETE; на стенде даже после ручного обнуления — 409. Нужен именно ondelete="SET NULL"
2	Высокая	Не исправлено	UserIn(role="admin") без ключа organization_id проходит валидацию (проверено в venv); явный null — отклоняется
3	Высокая	Не исправлено	update_user не реконсилирует organization_id при смене роли
4	Средняя	Не исправлено	refuse_delete без organization_id
5–8	Низкая/инфо	Не исправлено	Без изменений

Побочный эффект № 1: на стенде накоплено 18 организаций test-* — фикстура org не может их удалить. После исправления — почистить.

Новые находки

#	Серьёзность	Файл	Кратко
9	Высокая	app/api/v1/prefixes.py:286	GET /prefixes/{id}/subnets/next без require_org — чтение чужой организации
10	Средняя	app/api/v1/users.py, app/services.py	Несуществующий organization_id / нарушение CHECK → 409 «логин уже существует» или обезличенный 409
11	Низкая	prefixes.py:241,355, refs.py:354-355	Различимые ответы 404/422 раскрывают существование объектов чужой организации
12	Низкая	app/api/v1/refs.py:174	/device-types отдаёт devices_count по всем организациям
13	Низкая	app/models.py, 0011	CheckConstraint в модели без имени, в миграции — ck_users_role_org_scope
14	Инфо	app/api/v1/journal.py	Админ организации видит IP и User-Agent суперадминистратора в событиях своей организации
15	Инфо	README.md	Устаревшие/неточные формулировки

9. Утечка данных чужой организации через предпросмотр подсети — ВЫСОКАЯ

preview_subnet не принимает user и не вызывает require_org — единственный GET по id префикса, пропущенный в 032. Воспроизведено: viewer организации B: GET /prefixes/{id_A} → 404 (изоляция), GET /prefixes/{id_A}/subnets/next?length=24 → 200 {"prefix":"10.77.0.0/24","length_min":17,"length_max":32}; несуществующий id → 404. Раскрывается: существование префикса, его длина (length_min − 1), занятость (перебором length восстанавливается раскладка подсетей). Исправление: user: User = Depends(current_user) + require_org(user, p.organization_id, "Префикс").

10. Ошибки целостности маскируются под «логин уже существует»

create_user оборачивает flush/commit текстом «Пользователь с таким логином уже существует» — им же подписываются нарушение FK (organization_id несуществующей организации) и CHECK (№ 2). update_user при тех же причинах отдаёт общий CONFLICT_MSG. Отдельно: повторная активация мигрированного viewer (organization_id=NULL) через «Разрешить доступ» (в т. ч. групповое) → непонятный 409. Исправление: в create_user/update_user — get_or_404(Organization, organization_id) до записи; проверку инварианта роль/организация делать по итоговому состоянию (роль, организация, активность) с 422 и понятным текстом — это же закрывает № 2–3.

11. Оракул существования объектов чужой организации

Для чужих объектов ответ отличается от несуществующих: create_prefix — 422 «VRF принадлежит другой организации» против 404; _check_device — 422 против 404; update_isp — get_or_404(Organization) до require_org (разные тексты 404). Также create_prefix берёт advisory-lock чужого VRF до require_org. Противоречит принципу 032 «404, не подтверждать существование». Исправление: для не-superadmin отвечать 404 при чужой организации (или вызывать require_org по организации объекта до остальных проверок).

12. Глобальные счётчики типов устройств

list_types считает устройства по всем организациям — admin/viewer видит объём чужих данных (на стенде видно devices_count=1 у типа без устройств своей организации). Решение 2 плана относится к справочнику, не к счётчикам. Исправление: scope_org в подсчёте _type_outs.

13. Имя CHECK-ограничения расходится

Модель: CheckConstraint(...) без name; миграция: ck_users_role_org_scope. alembic check CHECK не сравнивает, поэтому расхождение не видно, но create_all (и любая будущая автогенерация/drop) получит другое имя. Исправление: name="ck_users_role_org_scope" в модели.

14. Метаданные запросов суперадминистратора в журнале организации

События, выполненные superadmin в организации, несут client_ip и meta (User-Agent, путь) и видны админу/просмотрщику этой организации. Не баг плана, но стоит решить осознанно (например, скрывать client_ip/meta чужих акторов для не-superadmin).

15. README

• Строка 135: «Пользователи» (только admin) → только superadmin.
• Строка 96: «защищена от отключения/удаления при наличии других активных superadmin» — смысл обратный: защищён последний активный.
• Нет раздела о 031 (Swagger, TLS-профиль) — ожидаемо, изменение не реализовано.

Прочее

• 031: в docs/changes/031-expose-hardening/ только PLAN.md; docs_enabled, app_bind, Caddyfile, профиль tls отсутствуют — находки пентеста № 1–2 открыты.
• Артефакты 032: нет итогового файла (суммаризации) — ожидаемо до завершения цикла правок.
• Тесты: после исправлений обновить test_users (явный organization_id) и прямую SQL-вставку в test_journal; добавить один тест изоляции (viewer чужой организации → 404 на GET /prefixes/{id} и /subnets/next) — закрывает № 9 от регрессии.

Что проверено без замечаний

• Все мутации (VRF, префиксы, адреса, устройства, операторы, организации) проверяют организацию до изменения; *Update-схемы не принимают organization_id — перенос объектов между организациями невозможен; перенос префикса между VRF проверяет организацию целевого VRF.
• current_user читает роль и организацию из БД на каждый запрос — смена роли/организации действует сразу, без перевыпуска токена.
• Журнал: list/summary/facets/get фильтруются по организации; settings/clear — только superadmin.
• UI: loadOrgs сбрасывает сохранённую в localStorage организацию, если она недоступна пользователю.
• Миграции 0010 (enum в autocommit_block) и 0011 (CHECK после backfill) — корректный порядок.

Порядок исправлений

1. № 1 (ondelete="SET NULL" в модели и новой миграции 0013 — 0012 уже применена на стенде), № 9.
2. № 2, 3, 10 — единая проверка инварианта по итоговому состоянию в users.py + правка userDialog (№ 3, 8).
3. № 4, 11, 12, 13.
4. № 5–7, 15; обновление тестов; очистка организаций test-* на стенде.