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

# Ревью кодовой базы IPAM Manager — 2026-09-26

**Объём:** `app/` (FastAPI, SQLAlchemy, ~1 700 строк), `web/app.js` (~1 000 строк), `alembic/`, `tests/`, Docker-окружение.
**Состояние:** коммит `cd09ef0` (задачи 001–010).
**Метод:** чтение кода. Подозрительные места проверены запросами к стенду `ipam_control_006` на временных данных, которые потом удалены. Выполнен `alembic check`.

Пометки: ✅ — воспроизведено на стенде, 🔎 — вывод по коду.

## Итог

Кодовая база компактная и последовательная. Бизнес-правила собраны в API. Журнал аудита сделан добротно: ротация под advisory-lock, IP клиента с учётом доверенных прокси. В UI все данные сервера экранируются через `esc()`, XSS-векторов не найдено. Миграции совпадают с моделями (`alembic check`: «No new upgrade operations detected»). Все 14 тестов проходят.

Основные риски:
- **Целостность адресного пространства.** Один IP можно завести дважды в одном VRF. Можно назначить адрес сети и broadcast.
- **Устойчивость API.** Отрицательные `limit`/`offset` дают 500. У `offset` на экране адресов нет верхней границы.
- **Журнал можно вытеснить без авторизации.** Неудачные входы пишутся без ограничений, ротация по количеству удаляет старые записи.

| # | Серьёзность | Область | Кратко |
|---|---|---|---|
| 1 | Высокая | Данные | Один IP дважды в VRF (в родителе и в дочернем префиксе); занятость завышается ✅ |
| 2 | Высокая | Безопасность | Анонимный перебор паролей без ограничений и вытеснение журнала аудита 🔎 |
| 3 | Средняя | API | Отрицательные `limit`/`offset` дают 500 ✅ |
| 4 | Средняя | Производительность / DoS | Экран адресов: неограниченный `offset` для `status=free` и выборка без LIMIT 🔎 |
| 5 | Средняя | Данные | Можно назначить адрес сети и broadcast; `free` и загрузка считаются неверно ✅ |
| 6 | Средняя | Безопасность | Роль по умолчанию в `POST /users` — `admin` ✅ |
| 7 | Средняя | Безопасность | Встроенный `jwt_secret = "dev-only-secret"` без проверки при старте 🔎 |
| 8 | Низкая | API | `null` в PATCH адреса даёт 409 «Запись … уже существует» ✅ |
| 9 | Низкая | Производительность | N+1 запросов в списках устройств, операторов, VRF, типов и на «Обзоре» 🔎 |
| 10 | Низкая | Данные | `next_free` (автоназначение адреса) не учитывает вложенные префиксы 🔎 |
| 11 | Низкая | Конкурентность | Нет блокировок в проверке «последний администратор» и в `allocate_next` 🔎 |
| 12 | Низкая | Эксплуатация | Контейнер работает от root, нет healthcheck у `app`, миграции выполняются при старте каждой реплики 🔎 |
| 13 | Низкая | Мелочи | LIKE без экранирования, нет журналирования неудачных попыток очистки журнала, нет заголовков безопасности 🔎 |

---

## Находки

### 1. Один IP-адрес дважды в VRF ✅
`create_address` (`app/api/v1/prefixes.py:314`) проверяет две вещи: адрес входит в префикс и уникальна пара `(prefix_id, address)` (`app/models.py:119`). Уникальности адреса в пределах VRF нет. Кроме того, адрес можно записать на родителя, даже если он попадает в диапазон дочернего префикса.
**Воспроизведение:** родитель `198.51.100.0/24`, дочерний `/25`. `198.51.100.5` создаётся и в родителе, и в дочернем (оба ответа 201). У родителя `used = 4` при трёх уникальных адресах.
**Последствия:** дубли в реестре. `_usage` суммирует адреса поддерева, поэтому загрузка и «Обзор» завышаются.
**Рекомендация:**
- Хранить адреса только в самом узком префиксе. При создании проверять, что адрес не попадает ни в один дочерний префикс.
- Добавить уникальность `(vrf_id, address)`: денормализовать `vrf_id` в `addresses` или поставить триггер. Иначе инвариант держится только в коде.
- При создании префикса (`attach_to_tree`) переносить в новый дочерний адреса родителя из его диапазона.

### 2. Перебор паролей и вытеснение журнала без авторизации 🔎
`POST /auth/login` (`app/api/v1/auth.py:14`) никак не ограничен. Каждая неудача пишет `session.failed` в `audit_log`. Ротация по количеству (`app/rotation.py:42`) удаляет самые старые записи сверх `max_entries` (по умолчанию 100 000).
**Последствия:**
- неограниченный перебор паролей;
- анонимный клиент может за несколько минут вытеснить из журнала историю реальных изменений.

Попутно: при несуществующем логине argon2 не вызывается, поэтому по времени ответа можно понять, существует ли логин. У `LoginIn` нет ограничения длины пароля.
**Рекомендация:**
- ограничить частоту попыток по IP и логину (таблица попыток по образцу `ClearAttempt` или прокси);
- при серии неудач агрегировать их в одну запись журнала;
- выполнять фиктивную проверку argon2 для несуществующего логина;
- ограничить `password` до `max_length=128`;
- рассмотреть отдельную квоту ротации для событий `session.*`.

### 3. Отрицательные `limit`/`offset` дают 500 ✅
У всех списков объявлено `limit: int = Query(…, le=…)` и `offset: int = 0` без нижней границы. `GET /audit?limit=-1` и `GET /isps?offset=-1` возвращают **500**: PostgreSQL отвергает отрицательный LIMIT/OFFSET.
**Рекомендация:** `Query(…, ge=1, le=…)` и `Query(0, ge=0)` во всех списках (`refs.py`, `prefixes.py`, `journal.py`, `users.py`). Удобно завести общую зависимость пагинации.

### 4. `GET /prefixes/{id}/addresses`: ресурсоёмкие ветки 🔎
В `list_addresses` (`app/api/v1/prefixes.py:272`) две проблемы.
- При `status=free` цикл собирает `offset + limit` свободных адресов в список (`need = offset + limit`). У `offset` нет верхней границы. На IPv6-префиксе любой пользователь с ролью `viewer` одним запросом с `offset=10^8` занимает процессор и память воркера.
- Без фильтров основной запрос (строка 289) выполняется **без LIMIT/OFFSET**: все адреса префикса загружаются и нарезаются в Python. На крупных подсетях это десятки тысяч объектов на каждую страницу.

**Рекомендация:**
- для `free` генерировать нужную страницу арифметически, пропуская занятые диапазоны, без материализации `offset` элементов, и ограничить `offset`;
- в ветке без свободных адресов перенести пагинацию в SQL;
- смешанный режим (занятые + свободные) оставить только для подсетей ≤ `FREE_LISTING_LIMIT`, как сейчас.

### 5. Назначаются адрес сети и broadcast ✅
`create_address` проверяет только `ip in network`. В `198.51.100.0/25` успешно назначаются `.0` и `.127`. При этом `capacity()` для IPv4 до `/30` вычитает эти два адреса. В итоге `free = cap - stored` занижается, а загрузка может превысить 100 %.
**Рекомендация:** для IPv4 с длиной ≤ 30 отклонять адрес сети и broadcast (422). Вариант — считать их занятыми только в `capacity`, но это хуже.

### 6. Роль по умолчанию — администратор ✅
`UserIn.role` по умолчанию `Role.admin` (`app/schemas.py:79`), и `POST /users` без `role` создаёт администратора. UI передаёт `viewer` явно, но у клиентов API и скриптов по умолчанию наибольшие права.
Та же картина в модели: `User.role` — `default=Role.admin`.
**Рекомендация:** по умолчанию `viewer`.

### 7. Встроенный JWT-секрет 🔎
`Settings.jwt_secret = "dev-only-secret"` (`app/config.py:8`). Если приложение запущено без `JWT_SECRET` (локально или другим способом развёртывания), токены подписываются общеизвестным ключом, и их можно подделать для любого логина.
В docker-compose пустое значение даёт не небезопасный режим, а 500 при входе: PyJWT отвергает пустой HMAC-ключ.
**Рекомендация:** убрать значение по умолчанию. При старте проверять длину секрета, например ≥ 32 байт, и не запускаться без него. То же для `ADMIN_PASSWORD` при пустой БД: сейчас без него админ молча не создаётся.

### 8. `null` в `PATCH /addresses/{id}` даёт ложный 409 ✅
`update_address` использует `model_dump(exclude_unset=True)` без `exclude_none`, поэтому `{"description": null}` пишет NULL в колонку NOT NULL. Приходит `IntegrityError`, и ответ — 409 «Запись с такими значениями уже существует».
Остальные PATCH-эндпоинты используют `exclude_none=True`.
**Рекомендация:** `exclude_none=True`. Если нужна очистка поля, явно приводить `None` к `""`.

### 9. N+1 запросов 🔎
Счётчики и связанные объекты догружаются отдельными запросами на каждую строку:
- `_device_out`: 2 запроса на устройство, до 1 000 на странице из 500;
- `_isp_out`: организация на каждого оператора;
- `_vrf_out`, `_type_out`, `_org_out`: счётчики на каждую строку;
- `_prefix_outs` на «Обзоре»: `_depths` и `_capacities` по каждой организации.

При текущих объёмах это незаметно, но растёт линейно.
**Рекомендация:** агрегаты одним `GROUP BY` на страницу и `selectinload` для связей.

### 10. Автоназначение адреса не учитывает вложенные префиксы 🔎
`next_free` (`app/services.py:309`) исключает только адреса самого пула. Если внутри пула есть дочерний префикс, выданный адрес может попасть в его диапазон. Это частный случай п. 1.
Кроме того, функция перебирает хосты подряд и загружает в память все занятые адреса. Для крупных пулов лучше использовать тот же приём, что и в `next_free_subnet` (перескок за занятые диапазоны).

### 11. Гонки без блокировок 🔎
- **Последний администратор.** `update_user` и `delete_user` проверяют «последний активный администратор» через `count()` без блокировки. Два администратора, одновременно отключающие друг друга, могут оставить систему без админа. Нужен `SELECT … FOR UPDATE` по активным администраторам или advisory-lock.
- **Автоназначение адреса.** В `allocate_next` при параллельных запросах один из них получает 409 «повторите запрос». Корректно, но можно блокировать префикс так же, как в `allocate_subnet`.
- **Логин без учёта регистра.** Уникальность проверяется запросом, а в БД `username` уникален с учётом регистра. Нужен индекс `unique(lower(username))`, как у VRF.

### 12. Эксплуатация 🔎
- `Dockerfile`: процесс работает от root. Нет `USER`, нет `HEALTHCHECK`, у сервиса `app` в compose нет `healthcheck`, хотя `/healthz` есть.
- `alembic upgrade head` в `CMD` выполняется при старте каждой реплики. При масштабировании нужен отдельный job или advisory-lock в `env.py`.
- Приложение слушает `0.0.0.0:8088` по HTTP, JWT и пароли идут открытым текстом по LAN. Для эксплуатации нужен TLS-прокси (Caddy на хосте уже есть) и `TRUSTED_PROXIES`.
- Зависимости заданы диапазонами без lock-файла, поэтому сборки не воспроизводимы.

### 13. Мелочи 🔎
- **LIKE без экранирования.** В `list_prefixes`, `list_users` и `_like()` (`refs.py`) `%` и `_` в поиске не экранируются (в журнале это сделано, `_escape_like`).
- **Очистка журнала.** Неудачные попытки подтверждения пароля пишутся только в `clear_attempts`, в журнал они не попадают, хотя событие значимо для безопасности.
- **Ответы без заголовков безопасности.** Нет `Content-Security-Policy`, `X-Frame-Options`, `X-Content-Type-Options`, поэтому UI можно встроить во фрейм.
- **Удаление префикса с `force=true`.** Запись журнала не содержит числа удалённых вместе с ним адресов.
- **Хранение токена.** Токен лежит в `sessionStorage`: при XSS его можно украсть. XSS сейчас не найден, но CSP снизил бы риск.
- **Смена пароля.** Уже выданные токены не отзываются (задокументировано). Можно хранить `password_changed_at` и сверять с `iat` токена.

---

## Что сделано хорошо
- **Целостность VRF и организации.** Обеспечена в БД: составной FK `fk_prefixes_vrf_org`, уникальный индекс `lower(name)`.
- **Ошибки API.** Единый формат `{"code", "message", "fields"}`. Нарушения ограничений БД переводятся в 409, а не в 500.
- **Журнал.** Ротация под `pg_try_advisory_xact_lock`. IP берётся из `X-Forwarded-For` только от доверенных прокси, разбор идёт справа налево. Мусор в заголовке игнорируется.
- **Пароли.** Хранятся в argon2. Очистка журнала подтверждается паролем с блокировкой после пяти неудач. Отключение учётной записи действует немедленно.
- **UI без сборки и зависимостей.** Всё, что приходит с сервера, экранируется, `innerHTML` используется только с экранированным содержимым.
- **Тесты интеграционные, по реальному стеку.** Они короткие и проверяют бизнес-правила, а не реализацию.

## Планы доработок
Одна находка — один план в `docs/changes/`:

| № | План |
|---|---|
| 1 | `011-address-unique-in-vrf` |
| 2 | `012-login-rate-limit` |
| 3 | `013-pagination-bounds` |
| 4 | `014-addresses-listing-performance` |
| 5 | `015-network-broadcast-addresses` |
| 6 | `016-default-role-viewer` |
| 7 | `017-jwt-secret-validation` |
| 8 | `018-patch-null-handling` |
| 9 | `019-n-plus-one-queries` |
| 10 | `020-next-free-address` (после 011) |
| 11 | `021-concurrency-locks` |
| 12 | `022-ops-hardening` |
| 13 | `023-minor-hardening` |

## Предлагаемый порядок работ
1. **Быстрые правки, около часа, без миграций:** пп. 3, 5, 6, 8, экранирование LIKE из п. 13.
2. **Безопасность:** п. 2 (ограничение частоты входа, агрегирование неудач в журнале), п. 7 (проверка секрета при старте), п. 12 (non-root, healthcheck, TLS через прокси).
3. **Целостность данных:** пп. 1 и 10 — модель «адрес в самом узком префиксе» и уникальность в VRF. Нужны миграция и проверка существующих дублей.
4. **Производительность:** пп. 4 и 9, когда объёмы станут заметными. Ограничение `offset` из п. 4 стоит сделать сразу вместе с п. 3.