Files
ripe-cidr-collector/docs/review-2026-09-21.md
T
ayurishchevandClaude Sonnet 5 aaf41adc79 Stage B of review fixes: daemon state lock, image build check, RIPEstat retries
Job state is changed under one RLock, the Dockerfile copies all root
modules and imports them at build time, RIPEstat requests go through a
retrying session with the sourceapp parameter (ripestat_sourceapp) and a
capped Retry-After. Adds the summary and marks review findings 5-10 fixed.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
2026-09-21 10:12:48 +03:00

7.5 KiB
Raw Blame History

Ревью кодовой базы (2026-09-21)

Метод: структура по графу знаний (405 узлов; код: 7 модулей, 1382 строки), затем чтение кода. Часть находок подтверждена запуском (отмечено). Состояние на момент ревью: коммит 02f7b49, 20 тестов.

Структура по графу

  • Связанность модулей: api_server ↔ db (13 связей), api_server ↔ cidr_collector (11), cidr_collector ↔ collector_daemon (8). Зависимости направлены в основном одним способом, слои читаемы.
  • Хабы: api_server.py (посредничество 0,38), db.py (0,23), cidr_collector.py (0,20).
  • Циклическая зависимость: cidr_collector импортирует db, а db берёт пути и настройки из cidr_collector отложенным импортом внутри функций.
  • Мёртвый код не найден. Узлы кода со степенью не более 1 - обработчики, регистрируемые декораторами, и функции через словарь диспетчеризации (_frr, _mikrotik): ложные срабатывания.

Находки

# Серьёзность Находка Где Статус
1 Высокая Пустой список после порчи базы без копий (подтверждено запуском). Первый запрос даёт 503, затем создаётся новая пустая база, и /addresses отвечает 200 []. Если демон жив, /health показывает ok при counts = 0. Потребители, забирающие список по расписанию, могут стереть свои списки. db.py:83-112 Исправлено (summary-loss-guard.md)
2 Средняя 500 вместо 401 при нелатинском X-API-Key (подтверждено). secrets.compare_digest для str работает только с ASCII. Обхода авторизации нет. api_server.py:105 Исправлено (summary-input-hardening.md)
3 Средняя 500 при некорректном since (подтверждено): since=² (str.isdigit() истинно для символов юникода, а int() их не принимает), число длиннее 4300 цифр (лимит int), крайние даты с поясом (0001-01-01T00:00:00+05:00, 9999-12-31T23:59:59-05:00: OverflowError). api_server.py:152 Исправлено (summary-input-hardening.md)
4 Средняя DNS-адреса без фильтрации (подтверждено). В списки попадают любые ответы: 127.0.0.1, 10.x, 0.0.0.0. Оставлять нужно только глобальные адреса. cidr_collector.py:176-179 Исправлено (summary-input-hardening.md)
5 Низкая /addresses открывает три соединения и три разных снимка (курсор, ASN, FQDN): части ответа могут относиться к разным моментам, соединения тратят время на PRAGMA и проверку схемы. Достаточно одной сессии с одной транзакцией. api_server.py:133-141 Исправлено (summary-review-fixes-5-10.md)
6 Низкая /health без авторизации отдаёт внутренние пути и текст ошибки (last_restore). api_server.py:214-226 Исправлено (summary-review-fixes-5-10.md)
7 Низкая run_job меняет job_state вне _state_lock, хотя write_status читает под блокировкой (гонка безвредна благодаря GIL). collector_daemon.py:72 Исправлено (summary-review-fixes-5-10.md)
8 Низкая Dockerfile копирует модули явным списком: новый модуль не попадёт в образ, ошибка проявится при запуске (healthcheck поймает, но поздно). Dockerfile:12 Исправлено (summary-review-fixes-5-10.md)
9 Низкая Запросы к RIPEstat без повторов и без параметра sourceapp. cidr_collector.py:119 Исправлено (summary-review-fixes-5-10.md)
10 Низкая get_changes: по запросу на каждое значение при проверке наличия; на десятках тысяч изменений станет заметно. db.py Исправлено (summary-review-fixes-5-10.md)

Архитектура и сопровождение

  • Цикл db ↔ cidr_collector обходится отложенным импортом; чище вынести пути и значения по умолчанию в settings.py.
  • cidr_collector.py совмещает четыре роли: помощники конфигурации, файловый обмен с демоном, два сборщика, CLI (связность сообщества 0,09).
  • api_server.py растёт: схемы, проверки и все эндпоинты в одном модуле (326 строк, наблюдение 11 анализа).

Что сделано хорошо

  • Токен проверяется безопасным сравнением и отказывает по умолчанию (503 без токена).
  • Запись атомарна везде (JSON через os.replace, SQLite в транзакциях, копии через .tmp).
  • Сетевые запросы выполняются до транзакции, блокировка записи короткая.
  • Ошибки хранилища сведены к одному типу (StorageError).
  • Порча данных не приводит к потере: карантин и копии.

Пробелы тестов

Не было проверок задания backup в демоне, поля last_restore в /health и граничных значений X-API-Key и since; закрыты доработками input-hardening и review-fixes-5-10.

Ограничения ревью

  • Граф не заменяет чтение кода: 14 связей типа INFERRED между модулями по отдельности не проверялись.
  • Сборка MikroTik и FRR на реальном ПО не проверялась (риск 4 анализа).

Рекомендуемый порядок

  1. Доработка «Безопасность ввода и данных»: п. 2, 3, 4 и тесты (plan-input-hardening.md).
  2. Находка 1: после порчи без копий отвечать 503 до первого успешного сбора (plan-loss-guard.md, выполнено).
  3. Пункты 5-10 исправлены отдельной доработкой (plan-review-fixes-5-10.md, выполнено); из ревью остаются только архитектурные замечания (вынос путей в settings.py, разделение api_server.py и cidr_collector.py).