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.