ayurishchevandClaude Opus 5.5
13e17fbb47
Ревью кодовой базы (docs/reviews/2026-09-26-codebase-review.md) и планы по каждой находке:
011 IP уникален в VRF и хранится в самом узком префиксе (addresses.vrf_id, составной FK
с каскадом при переносе VRF, миграция 0007 с остановкой на дублях).
012 Ограничение попыток входа (login_attempts, 429 + Retry-After), выравнивание времени
ответа, журнал без вытеснения анонимными событиями (миграция 0006).
013 Границы пагинации: отрицательные/чрезмерные limit/offset дают 422 вместо 500.
014 Экран адресов: страница свободных адресов арифметикой, пагинация в SQL.
015 Запрет адреса сети/broadcast, загрузка не выше 100 %.
016 Роль по умолчанию — viewer.
017 Проверка JWT_SECRET/ADMIN_PASSWORD при старте.
018 null в PATCH очищает текстовые поля; нейтральный текст конфликта БД.
019 Пакетная загрузка в списках вместо N+1.
020 Автоназначение адреса вне вложенных префиксов, с блокировкой префикса.
021 Advisory-lock при снятии прав администратора, уникальный lower(username) (миграция 0008).
022 Контейнер не от root, healthcheck, блокировка миграций, requirements.lock.
023 Экранирование LIKE, журнал отказов очистки, заголовки безопасности, учёт force-удаления,
отзыв токенов при смене пароля (claim pv, миграция 0005).
024 Исправление находок ревью 011-023 (docs/reviews/2026-09-26-changes-011-023-review.md):
сериализация попыток входа, запрет переноса адресов в адрес сети/broadcast, журнал входов,
запрет смены своего пароля через PATCH, валидация PATCH устройства, обновлён тест токенов.
Тесты: 14 passed. Документация: README.md, docs/changes/011-024, docs/reviews.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
161 lines
20 KiB
Markdown
161 lines
20 KiB
Markdown
# Ревью кодовой базы 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.
|