ros_control/docs/reviews/2026-09-27-codebase-review.md

# Ревью кодовой базы ros_control — 2026-09-27

Состояние на коммит `26dd1f8`. Тесты: 20/20 проходят.

## Назначение

Централизованное управление парком MikroTik RouterOS: опрос статуса, бэкапы в S3 (Yandex Object Storage),
обновление ROS и прошивки, группы устройств, журнал событий. Объём — около 2,5 тыс. строк (Python и шаблоны).

## Архитектура

| Слой | Модули | Назначение |
|---|---|---|
| Вход | `app/main.py` | FastAPI; при старте запускает опрос устройств, сверку бэкапов с бакетом и ротацию журнала; ошибки переводятся в HTTP-коды 404, 400 и 502 |
| API | `app/api/v1.py` | REST под Bearer-токеном |
| UI | `app/ui/routes.py`, шаблоны | Jinja2 + HTMX, сессия в cookie, один администратор |
| Сервисы | `app/services/*` | `devices`, `groups`, `jobs` (фоновые задачи), `ops` (связка устройство + S3 + БД), `poller`, `backups`, `events`, `settings` |
| Интеграции | `app/ros/client.py`, `app/ros/operations.py`, `app/s3.py` | REST-клиент RouterOS на httpx; boto3 вызывается через `to_thread` |
| Данные | `app/models.py`, `app/db.py`, `app/migrations.py`, `app/ids.py` | SQLite; ID вида `<префикс>_<uuid7>`; версия схемы в `PRAGMA user_version` |

API и UI используют один и тот же сервисный слой, поэтому логика не дублируется. Операции с устройствами
(`app/ros/operations.py`) не зависят от БД и S3, поэтому их легко тестировать. Паролей устройств нет в ответах
API, в БД они зашифрованы (Fernet).

## Сильные стороны

- **Бэкап:** файлы скачиваются с устройства по REST блоками в base64 со сверкой размера, затем уходят в S3.
  В `finally` файлы удаляются с устройства и при успехе, и при сбое.
- **Опрос:** быстрый режим — один запрос `system/resource`. Полная проверка обновлений идёт по отдельному
  интервалу. Короткий таймаут соединения позволяет быстро замечать недоступные устройства.
- **Журнал событий:** автор действия передаётся через ContextVar. Запись события идёт в одной транзакции
  с изменением. Есть ротация и постраничный вывод по ID.
- **Миграция на новые ID:** перед ней делается копия файла БД, всё выполняется одной транзакцией
  со сверкой числа строк.

## Замечания

### Безопасность

| № | Замечание | Где | Приоритет |
|---|---|---|---|
| 1 | Небезопасные значения по умолчанию не блокируются: `API_TOKEN`, `ADMIN_PASSWORD`, `SESSION_SECRET` = `change-me`. Если `.env` не заполнен, API открыт с известным токеном. Стоит отказываться запускаться при таких значениях | `app/config.py` | Высокий |
| 2 | Вход в UI без защиты от перебора паролей. Блокировка после неверных попыток (`security.register_failure`) есть только при очистке журнала | `app/ui/routes.py`, `login` | Высокий |
| 3 | Сессионная cookie без флага Secure (`https_only=False`). Нужен TLS-терминатор перед сервисом и флаг, включаемый через настройки | `app/main.py` | Средний |
| 4 | Открытый редирект в `/ui/move`: значение `next=/\evil.com` проходит проверку, а браузер воспринимает его как адрес другого сайта. Риск низкий: запрос только POST, cookie с `SameSite=strict` | `app/ui/routes.py`, `move` | Низкий |

### Корректность и согласованность

| № | Замечание | Где | Приоритет |
|---|---|---|---|
| 5 | Одиночное удаление бэкапа в UI вызывает `s3.delete_object` напрямую, в обход `backups.delete_many`. В итоге не обновляются метаданные (`deleted_at`) и не пишется событие, в отличие от API и группового удаления | `app/ui/routes.py`, `backup_delete` | Средний |
| 6 | Две системы миграций: `db._migrate` (ручные `ALTER`) и `migrations.py` (`user_version`). Лучше свести их в одну | `app/db.py`, `app/migrations.py` | Низкий |
| 7 | Смена канала обновлений у группы устройств идёт последовательно и синхронно внутри HTTP-запроса. На большом парке запрос упрётся в таймаут, а ошибка на одном устройстве прерывает остальные | `/api/v1/batch/channel`, `/ui/batch/channel` | Средний |

### Производительность и масштабирование

| № | Замечание | Где | Приоритет |
|---|---|---|---|
| 8 | Каждое открытие страницы бэкапов перечитывает весь бакет и запускает `sync_rows` по всей таблице `backups`. Нагрузка растёт линейно с числом копий и оплачивается запросами к S3 | `app/services/backups.py`, `search` | Средний |
| 9 | Синхронные запросы к БД внутри async-обработчиков. На SQLite и небольших объёмах это терпимо, но под нагрузкой блокирует event loop | сервисный слой | Низкий |
| 10 | Состояние хранится в памяти процесса: семафор задач, `_sync_lock`, счётчики неудачных попыток пароля. Приложение рассчитано на один процесс, `--workers > 1` сломает эти гарантии. Стоит явно указать это в README | `jobs`, `backups`, `security` | Низкий |

### Эксплуатация

| № | Замечание | Где | Приоритет |
|---|---|---|---|
| 11 | Нет healthcheck, логирование не настроено (используется стандартный `logging`) | `Dockerfile`, `docker-compose.yml` | Низкий |
| 12 | Тесты собраны в одном файле (570 строк). Это соответствует требованию минимума тестов, но при росте проекта файл станет трудно поддерживать | `tests/test_app.py` | Низкий |

## Рекомендуемый порядок исправлений

1. Пункты 1 и 2: проверка небезопасных значений при старте и защита входа от перебора. Это небольшие изменения,
   которые сильнее всего снижают риск.
2. Пункт 5: одиночное удаление через `delete_many`.
3. Пункт 7: групповая смена канала через фоновые задачи (`jobs`).
4. Пункт 8: кэшировать список объектов бакета или вызывать `sync_rows` только после изменений.

Каждое исправление оформляется отдельным изменением в `docs/changes/` (план → доработка → итоги → README).