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>
68 lines
11 KiB
Markdown
68 lines
11 KiB
Markdown
# Итог: исправление находок ревью изменений 011–023 (изменение 024)
|
||
|
||
Источник: `docs/reviews/2026-09-26-changes-011-023-review.md`, план: `PLAN.md` в этой же папке. Все 7 пунктов выполнены поверх незакоммиченных изменений 011–023 (не откатывались).
|
||
|
||
## Что сделано
|
||
|
||
### 1. Параллельные запросы обходят лимит входа (012)
|
||
`app/api/v1/auth.py`, `login`: в начале обработчика — `pg_advisory_xact_lock(hashtext(логин_в_нижнем_регистре))`, до `_retry_after`. Блокировка держится до `commit`/закрытия сессии, попытки одного логина сериализуются; лимит по IP не тронут.
|
||
|
||
### 2. Перенос адресов может нарушить запрет адреса сети/broadcast (011 × 015)
|
||
`app/api/v1/prefixes.py`: `_unusable_after_rehome(db, p)` — та же логика выбора «самого узкого целевого префикса», что и в `rehome_addresses`, но без переноса: возвращает пары `(адрес, CIDR целевого префикса)`, где адрес окажется сетевым/broadcast (`network_role`).
|
||
- `create_prefix`: проверка после `attach_to_tree`, до `rehome_addresses`; при находках — `db.rollback()` и 422 с текстом и целевым префиксом на каждый адрес.
|
||
- `_move_to_vrf`: та же проверка после смены VRF и `attach_to_tree`, до `rehome_addresses`; исключение до `commit()` — частичных изменений нет (сессия откатывается при закрытии).
|
||
- `allocate_subnet` не тронут.
|
||
- Миграция `0007`: после шага переноса адресов — предупреждение (`print`) со списком адресов, ставших сетевым/broadcast в новом префиксе (SQL-запрос через `network()`/`broadcast()`); миграция не останавливается. Сам перенос в 0007 не переписан.
|
||
|
||
**Отклонение от плана:** функция возвращает `list[tuple[str, str]]` (адрес + фактический целевой префикс), а не `list[str]`, как в тексте плана. Целевой префикс при переносе VRF не всегда совпадает с `p` (может быть уже существующий вложенный префикс целевого VRF) — сообщение вида «адрес X станет сетевым для p» было бы недостоверным; это подтвердилось на сценарии переноса при первом прогоне ручной проверки. Итоговое сообщение перечисляет каждый адрес с его настоящим целевым CIDR.
|
||
|
||
### 3. Устаревший тест токенов (023)
|
||
`tests/test_users.py::test_users_management`: старый токен после `POST /users/me/password` → 401; токен из ответа (`access_token`) → `GET /auth/me` 200. Других тестов не касались.
|
||
|
||
### 4. Полнота журнала входов (012)
|
||
`app/api/v1/auth.py`: `_retry_after` теперь возвращает счётчики по областям отдельно (`{"login": n, "ip": m}`), а не максимум. Решение о записи `session.failed` принимается по числу неудач именно этого логина (`before["login"] == 0`), а не по максимуму с IP. В `diff` записи `session.locked` с областью `ip` добавлено поле `distinct_logins` (число различных логинов с этого IP в окне, `_distinct_logins`).
|
||
`app/rotation.py`: условие удаления «шумных» записей первыми при ротации по количеству расширено с `action == "failed"` до `action.in_(("failed", "locked"))`, чтобы `session.locked` тоже вытеснялся первым.
|
||
|
||
### 5. Сброс своего пароля через PATCH (023)
|
||
`app/api/v1/users.py`, `update_user`: если `u.id == admin.id` и в теле запроса есть `password` — 422 «Свой пароль меняется через /users/me/password (с подтверждением текущего)». Проверка — до извлечения `pwd` из данных.
|
||
|
||
### 6. Нет валидации PATCH устройства (найдено попутно)
|
||
`app/schemas.py`: `DeviceIn._name`/`_mac` вынесены в модульные функции `_device_name`/`_device_mac` (обе принимают `None` и пропускают его — для `DeviceIn` это неактуально, так как поля обязательны). Применены в `DeviceIn` и `DeviceUpdate` через тот же идиом, что и `_blank` (`field_validator(...)( _device_name )`). В `DeviceUpdate` порядок сохранён: сначала `_blank_text` (null → "" для `mac`/`note`), затем проверка формата; пустая строка для `mac` допустима, как в `DeviceIn`.
|
||
|
||
### 7. Мелочи токена (023)
|
||
`app/security.py`: `_password_version` считает целочисленно — `(changed - EPOCH) // timedelta(microseconds=1)`, без `float`. У `iat` в `create_token` добавлен комментарий: поле информационное, отзыв токенов работает по `pv`.
|
||
|
||
## Изменённые файлы
|
||
`app/api/v1/auth.py`, `app/api/v1/prefixes.py`, `app/api/v1/users.py`, `app/schemas.py`, `app/security.py`, `app/rotation.py`, `alembic/versions/0007_address_vrf_unique.py`, `tests/test_users.py`, `README.md`.
|
||
|
||
## Проверка
|
||
1. `venv/bin/python -c 'import app.main'` — успешно; `node --check web/app.js` — успешно.
|
||
2. `docker compose -p ipam_control_006 up -d --build app` — образ пересобран, `ipam_control_006-app-1` в статусе `healthy`; `ipam_control_006-db-1` не тронут, миграция `0007` повторно не выполнялась (была применена ранее; добавленный `print` сработает при применении «с нуля»).
|
||
3. `venv/bin/python -m pytest -q` → `14 passed`.
|
||
4. Ручная проверка скриптом во временной папке (не в репозитории), учётка администратора из `.env`, порт `APP_PORT`:
|
||
- **п.1**: 16 параллельных неверных входов для временного пользователя (`rv-brute-*`) → 4×401, 12×429, в `login_attempts` ровно 5 строк (только первые 5 попыток проверяют пароль — сериализация advisory-lock работает). `login_attempts` очищен после проверки.
|
||
- **п.2**: `.128` в `/24` → создание вложенного `.128/25` → 422 (адрес указан вместе с целевым `198.51.100.128/25`); перенос `/24` с адресом `.128` в VRF, где уже есть `203.0.113.128/25`, → 422 (адрес указан вместе с целевым `203.0.113.128/25`). Временные организация/VRF/префиксы удалены.
|
||
- **п.5**: `PATCH /users/{id}` на себя с `password` → 422 с текстом плана.
|
||
- **п.6**: `PATCH /devices/{id}` с `name="bad name!"` → 422; `mac="zz"` → 422; `mac="aa-bb-cc-dd-ee-ff"` → 200, `mac` в ответе `"AA:BB:CC:DD:EE:FF"`. Временные устройство/тип устройства/организация удалены.
|
||
Все четыре пункта — **PASS**.
|
||
|
||
## Не проверено
|
||
- Снятие блокировки входа по истечении 10 минут и лимит по IP (20 попыток) — вне объёма ручной проверки этого этапа.
|
||
- Полное содержимое `session.locked.diff.distinct_logins` при переборе нескольких разных логинов с одного IP (сериализация по логину проверена; сценарий «шумного IP» с несколькими логинами вручную не воспроизводился).
|
||
- Повторное применение миграции `0007` «с нуля» (на новой базе) с реальными адресами сети/broadcast — предупреждение добавлено в код, но не исполнялось (на стенде миграция уже была применена ранее).
|
||
|
||
## Сверка (независимая проверка после внедрения)
|
||
Проверено повторно на стенде `ipam_control_006`, без участия исполнителя: `pytest -q` — 14 passed; контейнер `healthy`; прежняя поставка `ipam_control-*` не тронута.
|
||
- П.1: 16 параллельных неверных входов → проверено ровно 5 паролей (4×401, остальные 429) — PASS.
|
||
- П.2: вложенный `.128/25` при адресе `.128` у родителя → 422, префикс не создан (откат); обычный вложенный `/25` создаётся и забирает `.5` (регрессии нет) — PASS.
|
||
- П.3: тест токенов обновлён, весь набор проходит — PASS.
|
||
- П.4: по коду — счётчики по областям раздельно, `distinct_logins`, ротация удаляет `failed` и `locked` первыми — соответствует плану (поведение «шумного IP» не прогонялось).
|
||
- П.5: свой пароль через PATCH → 422, чужой → 200 — PASS.
|
||
- П.6: `name="bad name!"`/`mac="zz"` → 422, `aa-bb-…` → `AA:BB:…`, `mac: null` → `""` — PASS.
|
||
- П.7: целочисленная версия пароля, комментарий к `iat` — соответствует плану.
|
||
|
||
Остаточные замечания (не блокируют):
|
||
- Лимит по IP не сериализуется между разными логинами: параллельный перебор многих логинов с одного IP может немного превысить 20 попыток.
|
||
- `_unusable_after_rehome` проверяет все адреса диапазона, включая уже лежащие в своём самом узком префиксе: адрес сети/broadcast, внесённый до изменения 015, заблокирует создание охватывающего префикса (найти такие — `scripts/find_unusable_addresses.py`, на стенде 0).
|
||
- Смена формулы `pv` (float → целое) может однократно разлогинить пользователей, у которых `password_changed_at` уже был задан (на стенде — только временные пользователи проверки).
|