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>
108 lines
13 KiB
Markdown
108 lines
13 KiB
Markdown
# Ревью изменений 011–023 (доработки по ревью кодовой базы) — 2026-09-26
|
||
|
||
**Объём:** незакоммиченные изменения рабочего дерева поверх `cd09ef0`: 19 файлов, +514/−149, миграции `0005`–`0008`, скрипты `find_duplicate_addresses.py` и `find_unusable_addresses.py`, `requirements.lock`.
|
||
**Метод:** чтение diff. Подозрительные места проверены на стенде `ipam_control_006` на временных данных, после проверки данные удалены. Все экраны UI открыты в headless Chromium под новым CSP. Выполнены `alembic check` и `pytest`.
|
||
|
||
Пометки: ✅ — воспроизведено на стенде, 🔎 — вывод по коду.
|
||
|
||
## Итог
|
||
|
||
Изменения в целом соответствуют планам и работают. Подтверждено на стенде:
|
||
- `alembic check` без расхождений, миграции 0005–0008 применены;
|
||
- контейнер работает от `uid=10001` и в статусе `healthy`;
|
||
- под CSP все экраны UI открываются без ошибок в консоли, шрифты загружаются;
|
||
- перенос префикса в другой VRF каскадно обновляет `addresses.vrf_id`;
|
||
- поиск по `%`, отзыв токенов, ограничение попыток входа и границы пагинации работают;
|
||
- тесты: 13 из 14 проходят.
|
||
|
||
**Одна существенная находка:** лимит попыток входа обходится параллельными запросами (№ 1). Ещё одно расхождение между 011 и 015 и несколько мелких замечаний перечислены ниже.
|
||
|
||
| # | Серьёзность | Изменение | Кратко |
|
||
|---|---|---|---|
|
||
| 1 | Средняя | 012 | Параллельные запросы обходят лимит входа: 16 одновременных попыток проверили 16 паролей при лимите 5 ✅ |
|
||
| 2 | Низкая | 011 × 015 | Перенос адресов в новый вложенный префикс может сделать адрес его адресом сети/broadcast ✅ |
|
||
| 3 | Низкая | 023 | `tests/test_users.py` падает: тест ожидает прежнее поведение токенов после смены пароля ✅ |
|
||
| 4 | Низкая | 012 | Журнал входов: первые неудачи по новым логинам с «шумного» IP не пишутся; `session.locked` не вытесняется первым при ротации 🔎 |
|
||
| 5 | Низкая | 023 | Сброс администратором **своего** пароля через `PATCH /users/{id}` завершает его сессию без выдачи нового токена 🔎 |
|
||
| 6 | Низкая (выявлено попутно) | — | `PATCH /devices/{id}` не проверяет `name` и `mac` (принимает `"bad name!"`, `"zz"`) ✅ |
|
||
| 7 | Инфо | 023 | Claim `iat` пишется, но не используется; версия пароля считается через `float` 🔎 |
|
||
|
||
---
|
||
|
||
## Находки
|
||
|
||
### 1. Параллельные запросы обходят лимит входа ✅ (012)
|
||
В `login` (`app/api/v1/auth.py`) шаги идут в таком порядке:
|
||
1. `_retry_after` проверяет блокировку;
|
||
2. проверяется пароль (argon2, около 50–100 мс);
|
||
3. попытка записывается;
|
||
4. блокировка проверяется повторно.
|
||
|
||
Параллельные запросы проходят первую проверку до того, как любая из них запишет попытку, поэтому пароль проверяется в каждой.
|
||
**Воспроизведение:** 16 одновременных запросов с неверным паролем для одного логина. Ответы: 4 × 401 и 12 × 429, но в `login_attempts` 16 записей, то есть проверено 16 паролей. Если бы один из них был верным, запрос вернул бы токен: ветка успеха не смотрит на счётчик. Лимит «5 за 10 минут» фактически превращается в «5 + степень параллелизма».
|
||
**Исправление (любое из двух):**
|
||
- сериализовать попытки по логину: `SELECT pg_advisory_xact_lock(hashtext(:login))` в начале обработчика, до `_retry_after`;
|
||
- или записывать попытку **до** проверки пароля (пессимистично), а при успехе удалять её вместе с остальными. Тогда параллельные запросы видят растущий счётчик.
|
||
|
||
Первый вариант проще и не меняет семантику журнала.
|
||
|
||
### 2. Новый вложенный префикс может получить адрес сети или broadcast ✅ (011 × 015)
|
||
`rehome_addresses()` переносит адреса родителя в самый узкий префикс, не проверяя правило из 015. То же делает шаг переноса в миграции `0007`.
|
||
**Воспроизведение:** в `/24` назначен `198.51.100.128`, затем создан `198.51.100.128/25`. Адрес переехал в дочерний префикс и стал его адресом сети, хотя назначить его туда напрямую нельзя (422).
|
||
**Исправление:** в `create_prefix` и при переносе VRF (`_move_to_vrf`) отклонять операцию (422 или 409), если среди адресов, которые перейдут в новый префикс, есть его адрес сети или broadcast, и перечислять такие адреса. `allocate_subnet` это не затрагивает: он уже считает адреса родителя занятыми. В миграции 0007 такие случаи выводить в лог как предупреждение (`find_unusable_addresses.py` их найдёт).
|
||
|
||
### 3. Устаревший тест токенов ✅ (023)
|
||
`tests/test_users.py::test_users_management`, строка 55: `other.get("/auth/me").status_code == 200 # выданный ранее токен продолжает работать`. После 023 ответ 401. Поведение изменено намеренно (отзыв токенов при смене пароля), тест нужно привести к новой семантике:
|
||
- старый токен даёт 401;
|
||
- токен из ответа `POST /users/me/password` работает.
|
||
|
||
### 4. Полнота журнала входов 🔎 (012)
|
||
- В журнал пишется «первая неудача в окне». Счётчик `before` берётся как максимум по логину и по IP, поэтому если с этого IP уже были неудачи по другим логинам, первая неудача по **новому** логину не попадёт в журнал. При переборе логинов с одного IP в журнале останется одна запись `session.failed` и затем `session.locked` с областью `ip`. Число и список логинов из записей не восстановить.
|
||
**Исправление:** считать `before` отдельно по логину для решения о `session.failed`, а в `diff` записи `session.locked` по IP класть число различных логинов.
|
||
- При ротации первыми удаляются только `session.failed`. Записи `session.locked` тоже порождаются анонимными клиентами и вытесняют события данных.
|
||
**Исправление:** добавить `locked` в список удаляемых первыми.
|
||
|
||
### 5. Сброс своего пароля через PATCH 🔎 (023)
|
||
`PATCH /users/{id}` с `password` для собственной учётной записи меняет `password_changed_at` и тем самым отзывает текущий токен администратора, но нового токена в ответе нет. UI такой сценарий не допускает (для своей записи поле пароля скрыто), а клиент API окажется разлогинен.
|
||
**Исправление:** для `id == admin.id` отклонять `password` с 422 «Используйте /users/me/password» (проверка текущего пароля там обязательна), либо возвращать новый токен.
|
||
|
||
### 6. Нет валидации `PATCH /devices/{id}` ✅ (выявлено попутно)
|
||
`DeviceUpdate` не проверяет `name` (FQDN) и `mac` (формат, нормализация), хотя `DeviceIn` проверяет. PATCH с `name="bad name!"` и `mac="zz"` возвращает 200. Проблема существовала и до 011–023, но изменение 018 затронуло эту схему.
|
||
**Исправление:** переиспользовать валидаторы `DeviceIn._name` и `DeviceIn._mac` в `DeviceUpdate`, пропуская `None`.
|
||
|
||
### 7. Мелочи 🔎 (023)
|
||
- `iat` добавлен в токен, но нигде не проверяется. Отзыв работает по `pv`, поэтому `iat` можно оставить как информационное поле или убрать.
|
||
- `_password_version` считает `int(changed.timestamp() * 1_000_000)` через `float`. Результат детерминирован, так как значение в сессии и в БД совпадает до микросекунды, но надёжнее считать целочисленно: `(changed - EPOCH) // timedelta(microseconds=1)`.
|
||
|
||
---
|
||
|
||
## Соответствие планам
|
||
|
||
| Изменение | Статус | Замечания |
|
||
|---|---|---|
|
||
| 011 уникальность IP в VRF | ✅ с замечанием | № 2; перенос в VRF и каскад `vrf_id` проверены |
|
||
| 012 лимит входа | ⚠️ | № 1 (обход параллелизмом), № 4 |
|
||
| 013 границы пагинации | ✅ | Вместо общей зависимости используется константа `MAX_OFFSET` (отражено в SUMMARY) |
|
||
| 014 экран адресов | ✅ | IPv6 `offset=9·10⁶` отвечает за 0,02 с |
|
||
| 015 адрес сети/broadcast | ✅ с замечанием | Обход через перенос адресов (№ 2) |
|
||
| 016 роль по умолчанию | ✅ | — |
|
||
| 017 проверка секретов | ✅ | — |
|
||
| 018 null в PATCH | ✅ | Попутно № 6 |
|
||
| 019 N+1 | ✅ частично | «Обзор» не переписан (отражено в SUMMARY) |
|
||
| 020 автоназначение адреса | ✅ | Параллельный сценарий не проверялся |
|
||
| 021 блокировки | ✅ | Параллельное отключение двух администраторов не проверялось |
|
||
| 022 эксплуатация | ✅ | `APP_BIND` по умолчанию `0.0.0.0` (сознательно, отражено в SUMMARY) |
|
||
| 023 мелкие улучшения | ✅ с замечаниями | № 3, 5, 7; CSP проверен в браузере |
|
||
|
||
## Не проверено (для этапа тестов)
|
||
- Остановка миграции 0007 на реальном дубле и миграции 0008 на логинах, совпадающих без учёта регистра.
|
||
- Снятие блокировки входа через 10 минут; лимит по IP (20 попыток).
|
||
- Параллельные сценарии 020 (разные адреса) и 021 (двое администраторов отключают друг друга).
|
||
- Одновременный запуск двух `alembic upgrade head`.
|
||
|
||
## Рекомендуемые действия до коммита
|
||
1. Исправить № 1: advisory-lock по логину в `login`.
|
||
2. Исправить № 2: проверка адреса сети и broadcast при переносе адресов в новый префикс.
|
||
3. Обновить тест из № 3 на этапе тестов.
|
||
4. № 4–7 — по желанию, вместе с этапом тестов.
|