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-*` на стенде.