diff --git a/docs/server-audit.md b/docs/server-audit.md index 6dda026..acad76a 100644 --- a/docs/server-audit.md +++ b/docs/server-audit.md @@ -542,6 +542,926 @@ --- +## Phase 6 — Ревью Security, Auth, Device Integration (2026-06-02) + +**Цель:** Повторное ревью слоя аутентификации и интеграции с устройствами после внедрения тестов. + +**Коммит:** — (ветка `dev`) + +> Этот ревью выполнен после добавления 113 тестов. Некоторые проблемы обнаружены при анализе кода для тестирования. + +### 6.1 🔴 RateLimiter не работает в PHP-FPM/Apache + +**Где:** `server/SHServ/Tools/RateLimiter.php` + +**Что:** `static $requests` — in-memory property процесса. При PHP-FPM каждый запуск — новый процесс, счётчик сбрасывается. Rate limiting фактически отсутствует. + +**Фикс:** Перенести на Redis / memcached / DB, или file-based storage с `flock()`. + +--- + +### 6.2 🔴 `/events/new` полностью открыт + +**Где:** `server/SHServ/Routes.php:77-81`, `server/SHServ/Controllers/EventsController.php` + +**Что:** `check_api_auth()` пропускает URI, не начинающиеся с `/api/v1/`. `/events/new` — вне этого префикса, поэтому endpoint публичен. Любой может отправить события от имени любого устройства, зная `device_hard_id`. + +**Фикс:** Добавить `Authorization: Bearer ` и проверять его в `EventsController::new_event` перед `by_hard_id`, или перенести endpoint под `/api/v1/`. + +--- + +### 6.3 🔴 Race condition в `connect_new_device()` + +**Где:** `server/SHServ/Models/Devices.php:11-86` + +**Что:** Нет atomic проверки статуса устройства. Два параллельных запроса могут одновременно увидеть `"setup"`, создать дубли в `devices`+`device_auth`. Устройство перейдёт в normal mode, но в БД останутся дубли. + +**Фикс:** Внутри транзакции `connect_new_device()` выполняет `SELECT ... FOR UPDATE` (MySQL) / обычный `SELECT` (SQLite) по `device_hard_id` + `status='active'`. Дубль отклоняется до `INSERT`. Также добавлен `UNIQUE(device_hard_id, status)` в тестовую схему. +**Статус:** ✅ **Исправлено.** + +--- + +### 6.4 🔴 `Entity::update()` unconditionally требует `update_at` + +**Где:** `server/SHServ/Middleware/Entity.php:67-79` + +**Что:** `Entity::update()` всегда inject-ит `update_at` в UPDATE SQL. `DeviceAuth` (таблица `device_auth`) **не содержит** `update_at` в `$fields` и скорее всего нет в production schema. Вызов `DeviceAuth::kill()` → `update()` вызовет SQL error. + +**Фикс:** `Entity::update()` теперь проверяет `in_array($this->field_name_of_update_at, static::$fields, true)` перед инъекцией. +**Статус:** ✅ **Исправлено.** + +--- + +### 6.5 🟠 Post-commit remote state inconsistency + +**Где:** `server/SHServ/Models/Devices.php:73-76` + +**Что:** Транзакция commit-ится, а затем вызывается `$device->set_device_token()` и `$device->device_api()->set_device_name()`. Если устройство offline после commit — в БД зарегистрировано, но физически не знает токен/имя. Управление невозможно. + +**Фикс:** Либо remote setup **до** commit (rollback при fail), либо флаг `setup_pending` + background retry job. + +--- + +### 6.6 🟠 `DeviceAuth::kill()` не инвалидирует кэш + +**Где:** `server/SHServ/Entities/DeviceAuth.php:23-26`, `server/SHServ/Entities/Device.php:45-63` + +**Что:** `Device::auth()` кэширует `device_auth_instance`. После `$device_auth->kill()` статус меняется на `"killed"`, но кэш в `$device->device_auth_instance` остаётся со старым статусом `"active"`. Последующие вызовы `$device->auth()->is_active()` вернут `true`. + +**Фикс:** Сбрасывать `$device->device_auth_instance = null` в `DeviceAuth::kill()`. + +--- + +### 6.7 🟠 Replay-атаки на `/events/new` + +**Где:** `server/SHServ/Controllers/EventsController.php:17-57` + +**Что:** Устройство доказывает идентичность только через `device_hard_id` в POST body. Нет HMAC, timestamp, nonce. Replay attack возможен. + +**Фикс:** Требовать `Authorization: Bearer ` (см. 6.2) и/или подпись полезной нагрузки. + +--- + +### 6.8 🟡 `generate_token(16)` — низкая энтропия для device_token + +**Где:** `server/SHServ/Utils.php:144-149` + +**Что:** 16 hex-символов = 64 бита. Для локальной сети терпимо, но на грани. + +**Фикс:** Поднять до 32 (128 бит) для `device_token`. + +--- + +### 6.9 🟡 Нет валидации `$ip_address` в `DeviceAPI\Base` + +**Где:** `server/SHServ/Tools/DeviceAPI/Base.php:18-24` + +**Что:** `http://` + произвольная строка. Если `$ip_address` содержит path — SSRF возможен (хотя контролируется server-side). + +**Фикс:** Валидировать `filter_var($ip_address, FILTER_VALIDATE_IP)` в конструкторе. + +--- + +### 6.10 🟡 `Entity::update()` — тихий success при SQL error? + +**Где:** `server/SHServ/Middleware/Entity.php:67-79` + +**Что:** `Entity::update()` вызывает `$this->thin_builder()->update(...)` и не проверяет return value. `ThinBuilder::update` возвращает `Statement` или `null`. Возможно exception вылетит (PDO ERRMODE_EXCEPTION), но если нет — тихий fail. + +**Фикс:** Явно проверять результат и возвращать `false` при ошибке. + +--- + +### 6.11 🟡 `Sessions::get_current_session()` — лишний UPDATE + +**Где:** `server/SHServ/Sessions.php:82-96` + +**Что:** Обновление `last_using_at` при каждом запросе создаёт write load. При масштабе заметно. + +**Фикс:** Обновлять раз в N минут (например, если delta > 60s). + +--- + +### 6.12 🟡 `EventsController` — `set_time_limit(10)` обрезает handlers + +**Где:** `server/SHServ/Controllers/EventsController.php:30` + +**Что:** Фоновая обработка после flush (`channel_alias_device_event_call` и др.) может занять >10s — fatal. + +**Фикс:** `set_time_limit(0)` или background queue (cron / worker). + +--- + +### 6.13 🟢 Bearer token parsing — edge case + +**Где:** `server/SHServ/App.php:87-89` + +**Что:** `substr($auth_header, 7)` без проверки длины. Header `"Bearer"` (без пробела) даст пустой token, который пройдёт в `!$token` → 401. Не критично. + +**Фикс:** Использовать `str_starts_with` + `explode(' ', $auth_header, 2)[1] ?? null`. + +--- + +## Phase 7 — Ревью Models и Data Layer (2026-06-02) + +**Цель:** Проверить модели, Entity-классы, ORM ThinBuilder, фабрики и хелперы на корректность, согласованность и производительность. + +**Ветка:** `dev` + +> Продолжение ревью после Phase 6. В этой фазе найдено 34 находки: 5 критических, 14 высокого приоритета, 15 среднего/информационного. + +### 7.1 🔴 Entity::select_from_db() падает на отсутствующей строке + +**Где:** `server/SHServ/Middleware/Entity.php:36-45` + +**Что:** Если записи с указанным `id` нет, `select()` возвращает `[]`, и `list($this->data) = []` устанавливает `$this->data = null`. Поле `$was_filled` всё равно становится `true`. При первом доступе к свойству через `__get()` → `get()` происходит обращение `$this->data[$field_name]` где `data === null` — fatal error. + +**Фикс:** Проверять результат `select()` и бросать исключение `RecordNotFound`, если строка отсутствует. + +--- + +### 7.2 🔴 DeviceAuth::kill() упадёт в production из-за отсутствия update_at + +**Где:** `server/SHServ/Entities/DeviceAuth.php:7-9`, `server/SHServ/Middleware/Entity.php:67-79` + +**Что:** `Entity::update()` безусловно inject-ит `update_at` в `UPDATE` SQL. `DeviceAuth::$fields` не содержит `update_at`, а production-схема `device_auth` (в отличие от тестовой) скорее всего не имеет этой колонки. Тесты (`DevicesModelTransactionTest.php`) создают таблицу **с** `update_at`, поэтому тесты проходят, но в production `kill()` бросит PDOException. + +**Фикс:** `Entity::update()` теперь проверяет `in_array($this->field_name_of_update_at, static::$fields, true)` перед инъекцией `update_at`. +**Статус:** ✅ **Исправлено (см. 6.4).** + +--- + +### 7.3 🔴 Device::remove() не инвалидирует токен авторизации + +**Где:** `server/SHServ/Entities/Device.php:36-39` + +**Что:** Метод меняет `status` устройства на `"removed"`, но не трогает таблицу `device_auth`. Строка авторизации остаётся со статусом `active`, и токен продолжает считаться валидным. + +**Фикс:** В `remove()` дополнительно вызывать `$this->auth()->kill()` или делать операцию транзакционной. +**Статус:** ⏸️ **Отложено.** Будет решено вместе с системой авторизации / RBAC (только админ может удалять устройства). + +--- + +### 7.4 🔴 Area::remove() выполняет 3 обновления без транзакции + +**Где:** `server/SHServ/Entities/Area.php:267-282` + +**Что:** `update()` на `devices` (area_id = 0), `update()` на `areas` (parent_id = 0), и `remove_entity()` — три независимых запроса без `beginTransaction`. При падении после первого или второго остаются битые ссылки. + +**Фикс:** Обернута в `beginTransaction`/`commit`/`rollBack`. +**Статус:** ✅ **Исправлено.** + +--- + +### 7.5 🔴 Scripts::select_scripts_by_aliases_types() падает на пустом массиве + +**Где:** `server/SHServ/Models/Scripts.php:63-73` + +**Что:** При `$uniq_names = []` генерируется `... WHERE type = ? AND uniq_name IN ()` — синтаксическая ошибка SQL. + +**Фикс:** Ранний возврат `[]` если массив пуст. + +--- + +### 7.6 🟠 ThinBuilder не валидирует операторы WHERE + +**Где:** `server/Fury/Modules/ThinBuilder/ThinBuilderProcessing.php:147-156` + +**Что:** Любая строка проходит как оператор прямо в SQL. Значения параметризованы, но оператор — сырой текст. + +**Фикс:** Whitelist допустимых операторов: `=`, `!=`, `<>`, `<`, `>`, `<=`, `>=`, `LIKE`, `IN`, `IS`, `NOT IN`, `BETWEEN`. + +--- + +### 7.7 🟠 validate_identifier() разрешает числовой старт имени + +**Где:** `server/Fury/Modules/ThinBuilder/ThinBuilderProcessing.php:62-67` + +**Что:** Регулярка `/^[a-zA-Z0-9_]+$/` пропускает `123abc`, что недопустимо в SQL. Правильная: `/^[a-zA-Z_][a-zA-Z0-9_]*$/`. + +--- + +### 7.8 🟠 escape_string_in_arr() — мёртвый код с опасным экранированием + +**Где:** `server/Fury/Modules/ThinBuilder/ThinBuilderProcessing.php:69-79` + +**Что:** Метод использует `addslashes()` вместо prepared statements. Нигде не вызывается, но его наличие создаёт риск. + +**Фикс:** Удалить метод. + +--- + +### 7.9 🟠 MetaManager::create_or_update() — race condition + +**Где:** `server/SHServ/Models/MetaManager.php:75-86` + +**Что:** SELECT, затем INSERT или UPDATE. При конкурентных запросах возможен дубль. + +**Фикс:** `INSERT ... ON DUPLICATE KEY UPDATE` (MySQL) или UNIQUE-индекс `(assignment, ent_id, name)`. + +--- + +### 7.10 🟠 User::__construct() жадно загружает профиль + +**Где:** `server/SHServ/Entities/User.php:16-28` + +**Что:** Каждое создание `User` инициирует дополнительный `SELECT` из `profiles`. N+1 при массовой выборке. + +**Фикс:** Ленивая загрузка через `get_pet_instance`. + +--- + +### 7.11 🟠 Scripts::remove_scope() — файловая система и БД рассинхронизируются + +**Где:** `server/SHServ/Models/Scripts.php:29-51` + +**Что:** `unlink()` выполняется до `$script->remove()`. Если DB delete упадёт, файл уже удалён. + +**Фикс:** Удалять файл **после** успешного DB delete, либо оборачивать в транзакцию. + +--- + +### 7.12 🟠 Entity::update() всегда возвращает true + +**Где:** `server/SHServ/Middleware/Entity.php:67-79` + +**Что:** `ThinBuilder::update()` возвращает `rowCount()`, но Entity игнорирует его. Если `UPDATE` не затронул строк, всё равно `true`. + +**Фикс:** Проверять `rowCount() > 0`. + +--- + +### 7.13 🟠 IN-клауза с не-массивом падает на PHP 8+ + +**Где:** `server/Fury/Modules/ThinBuilder/ThinBuilderProcessing.php:149-151` + +**Что:** `count("string")` в PHP 8 бросает `TypeError`. Если по ошибке передать скаляр в `IN`, приложение упадёт. + +**Фикс:** Проверка `is_array($w_item[2])`. + +--- + +### 7.14 🟠 Короткий синтаксис where не поддерживает IN + +**Где:** `server/Fury/Modules/ThinBuilder/ThinBuilderProcessing.php:115-130` + +**Что:** `['field', ['v1', 'v2']]` превращается в `['field', '=', ['v1', 'v2']]` из-за `count === 2`. Массив пытается забиндиться как скаляр. + +**Фикс:** Обрабатывать массив второго элемента как `IN`. + +--- + +### 7.15 🟠 Entity::update() не сбрасывает modified_fields при исключении + +**Где:** `server/SHServ/Middleware/Entity.php:67-79` + +**Что:** При PDOException `modified_fields` остаётся заполненным. Повторный `update()` запишет устаревшие + новые изменения. + +**Фикс:** `try/finally` — сбрасывать только при успехе. + +--- + +### 7.16 🟠 Device::set_device_token() делает избыточные вызовы auth() + +**Где:** `server/SHServ/Entities/Device.php:94-102` + +**Что:** `auth()` вызывается трижды внутри одного метода. + +**Фикс:** Закэшировать `$auth = $this->auth()`. + +--- + +### 7.17 🟠 Scripts::set_script_state() чрезвычайно неэффективен + +**Где:** `server/SHServ/Models/Scripts.php:75-128` + +**Что:** Reflection на каждый вызов для scopes. Для regular/action — O(n·m) линейный поиск. + +**Фикс:** Хеш-таблица keyed by `uniq_name`. + +--- + +### 7.18 🟡 Area::get_exists_types() сканирует всю таблицу вместо DISTINCT + +**Где:** `server/SHServ/Models/Areas.php:36-57` + +**Что:** Запрашивает все строки, затем дедуплицирует в PHP. + +**Фикс:** `SELECT DISTINCT type FROM areas`. + +--- + +### 7.19 🟡 Area::get_recursive_inner_areas_ids() жёстко ограничен глубиной 10 + +**Где:** `server/SHServ/Entities/Area.php:129-158` + +**Что:** `if($lvl >= 10) { return []; }` — молча отрезает хвост при глубоком дереве. + +**Фикс:** Исключение при превышении. + +--- + +### 7.20 🟡 ThinBuilder::table_fields() ломается на типах без длины + +**Где:** `server/Fury/Modules/ThinBuilder/ThinBuilder.php:197-213` + +**Что:** `explode('(', $raw_field[1])` для `INT`, `TEXT`, `DATETIME` возвращает массив из одного элемента. `list()` присвоит `null`. + +--- + +### 7.21 🟡 ThinBuilder::tables() только для MySQL + +**Где:** `server/Fury/Modules/ThinBuilder/ThinBuilder.php:215-222` + +**Что:** `SHOW TABLES` — MySQL-специфичный синтаксис. При SQLite не работает. + +--- + +### 7.22 🟡 Несоответствие тестовой и production схемы device_auth + +**Где:** `server/tests/DevicesModelTransactionTest.php:44-52` + +**Что:** Тесты создают `device_auth` с `update_at TEXT`, но production schema может отличаться. Нет единого источника правды для схемы. + +**Фикс:** Создать `schema.sql` в репозитории. + +--- + +### 7.23 🟡 Нет DB-уровневых UNIQUE-индексов + +**Где:** Все модели + +**Что:** Уникальность проверяется через `count()` на уровне приложения. Race conditions возможны. + +**Фикс:** Добавить `UNIQUE INDEX` на `areas.alias`, `devices.alias`, `devices.device_hard_id` (active), `users.alias/email`, `meta.(assignment, ent_id, name)`. + +--- + +### 7.24 🟡 Area и AreaPlacing trait дублируют place_in_area_id + +**Где:** `server/SHServ/Entities/Area.php:68-71`, `server/SHServ/Entities/Traits/AreaPlacing.php:12-15` + +**Что:** Идентичные методы. Класс побеждает trait, но дублирование затрудняет поддержку. + +--- + +### 7.25 🟡 Creator::create_meta() подделывает update_at + +**Где:** `server/SHServ/Factory/Creator.php:32-50` + +**Что:** Вставляет фиктивное `update_at` только чтобы удовлетворить `Entity::update()`. + +--- + +### 7.26 🟡 Factory\Getter::get_profile_by() тянет только id + +**Где:** `server/SHServ/Factory/Getter.php:25-39` + +**Что:** Запрашивает только `id`, затем создаёт `Profile` без данных, форсируя ленивую загрузку. + +--- + +### 7.27 🟡 Model-конструктор вызывает devtools на каждую инстанциацию + +**Где:** `server/SHServ/Middleware/Model.php:8-13` + +**Что:** `using_model()` — лишний вызов в production. + +--- + +### 7.28 🟡 Area::remove() присваивает строку "0" вместо integer + +**Где:** `server/SHServ/Models/Areas.php:20` + +**Что:** `"parent_id" => "0"` — string вместо int. + +--- + +### 7.29 🟡 MetaWrap::$meta_manager_instance — статический mutable singleton + +**Где:** `server/SHServ/Helpers/MetaWrap.php:9` + +**Что:** Глобальное mutable состояние. + +--- + +### 7.30 🟡 ThinBuilder::query() — raw SQL без параметризации + +**Где:** `server/Fury/Modules/ThinBuilder/ThinBuilder.php:17-42` + +**Что:** Метод принимает строку SQL и выполняет напрямую. Опасен при использовании с пользовательским input. + +--- + +### 7.31 🟡 Scripts::get_scripts_list() — O(n·m) вместо хеш-таблицы + +**Где:** `server/SHServ/Models/Scripts.php:170-184` + +**Что:** `array_filter` внутри цикла. При 50 скриптах — 2500 сравнений. + +**Фикс:** Построить `array_reduce` → keyed lookup. + +--- + +### 7.32 🟡 Несогласованные типы возврата ошибок в Models + +**Где:** `Models/Areas.php`, `Models/Devices.php` + +**Что:** `Areas::create_new_area()` → `Area|null`, `Devices::connect_new_device()` → `Device|Array`. + +**Фикс:** Единый Result-объект или Exception-based flow. + +--- + +### 7.33 🟡 Area::alias_is_uniq() не исключает саму себя при обновлении + +**Где:** `server/SHServ/Models/Areas.php:27-34` + +**Что:** При использовании для update собственный alias будет засчитан как дубль. + +--- + +### 7.34 🟡 ThinBuilder flat where syntax в Entity::remove_entity() + +**Где:** `server/SHServ/Middleware/Entity.php:85-87` + +**Что:** `remove_entity()` использует плоский массив `["id", "=", $this->id()]` вместо nested. Работает, но не единообразно. + +--- + +## Phase 8 — Controllers, Routing, Validation (Phase 3 Review) 📝 Завершена + +**Дата:** 2026-06-02 +**Полный отчёт:** `server/docs/review-phase-3-controllers-routing-validation.md` + +### 8.1 🔴 Router вызывает ВСЕ совпадающие маршруты + +**Где:** `Fury/Modules/Router/RouterImplementation.php` +**Что:** `URI_routing()` и `GET_and_POST_routing()` не делают `break` после первого совпадения. Один HTTP-запрос может вызвать несколько контроллеров. +**Статус:** ⚠️ **By design / intentional.** Автор подтвердил, что это ожидаемое поведение фреймворка. + +### 8.2 🔴 Cron endpoints открыты без авторизации + +**Где:** `SHServ/Routes.php:61-62` +**Что:** `/cron/regular-scripts` и `/cron/status-update-scanning` — публичные URI-роуты. Любой может триггерить сканирование сети и выполнение регулярных скриптов. +**Фикс:** `CronController::ensure_localhost_only()` — только `127.0.0.1` / `::1`. +**Статус:** ✅ **Исправлено.** + +### 8.3 🔴 POST-роуты отвечают на GET (method spoofing) + +**Где:** `Fury/Modules/Router/RouterImplementation.php:61-91` +**Что:** Роутер проверяет только наличие параметров в `$_GET`/`$_POST`, но не `$_SERVER['REQUEST_METHOD']`. Все деструктивные POST-эндпоинты доступны через GET-ссылку. +**Фикс:** `GET_and_POST_routing()` требует совпадения `REQUEST_METHOD` с ожидаемым (`GET`/`POST`). +**Статус:** ✅ **Исправлено.** + +### 8.4 🔴 URI-роуты игнорируют HTTP метод + +**Где:** Все `uri()` маршруты +**Что:** `/api/v1/devices/id/$id/remove` отвечает на GET, POST, DELETE одинаково. Нет REST-семантики. +**Статус:** ⚠️ **Intentional simplification.** Сознательное упрощение; возможно будет исправлено позже. + +### 8.5 🔴 `/events/new` — timing attack + +**Где:** `SHServ/Controllers/EventsController.php:17-58` +**Что:** Быстрый flush (200) выполняется только для валидных устройств. Для невалидных — полный стек ошибки. Разница во времени позволяет перебирать `device_hard_id`. +**Статус:** ⏸️ **Отложено.** Требует изменений и на сервере, и в прошивке устройств (авторизация по `device_token`). Будет решено при переходе к прошивке. + +### 8.6 🔴 `scope_update` пишет PHP-файл напрямую + +**Где:** `SHServ/Controllers/ScriptsRESTAPIController.php:118` +**Что:** `file_put_contents()` без атомарности, без бэкапа, без валидации синтаксиса. Прерывание запроса = коррумпированный scope-класс → fatal error. +**Статус:** ⏸️ **Отложено.** Будет решено вместе с системой авторизации / RBAC (только админ сможет редактировать scopes). + +### 8.7 🔴 RateLimiter in-memory (bypass через FPM workers) + +**Где:** `SHServ/Tools/RateLimiter.php` +**Что:** Счётчик в `static array` внутри процесса. При 8 FPM workers лимит 60 req/min превращается в 480. +**Фикс:** Переписан на file-based storage с `flock()` + fallback на APCu. +**Статус:** ✅ **Исправлено.** + +### 8.8 🟠 Auth routes — мёртвый код + +**Где:** `SHServ/Routes.php:71-82`, `SHServ/Controllers/Example_AuthController.php` +**Что:** Все auth-роуты закомментированы, файл называется `Example_AuthController.php`, но класс `AuthController`. Аутентификация Vue-клиента происходит вне этих роутов (неясно где). + +### 8.9 🟠 Нет RBAC + +**Где:** Все контроллеры +**Что:** Любой аутентифицированный пользователь может удалять устройства, сканировать сеть, перезаписывать scope-код. + +### 8.10 🟠 `scope_file` — disclosure исходного кода + +**Где:** `SHServ/Controllers/ScriptsRESTAPIController.php:75-95` +**Что:** Возвращает raw PHP source файла scope. Утечка логики и путей. + +### 8.11 🟠 `update_alias` падает при сохранении того же alias + +**Где:** `DevicesRESTAPIController.php:359-386`, `AreasRESTAPIController.php:172-201` +**Что:** `alias_is_uniq()` не принимает `exclude_id`. Обновление alias на текущее значение возвращает ошибку "already exists". + +### 8.12 🟠 Нет лимитов длины текстовых полей + +**Где:** `new_area`, `update_name`, `update_description` +**Что:** Нет `maxlength` проверок. Можно записать multi-MB строку. + +### 8.13 🟠 Нет brute-force защиты на auth + +**Где:** `Example_AuthController.php` +**Что:** Нет rate limit, account lockout, progressive delay на входе. + +### 8.14 🟠 Нет CORS + +**Где:** `SHServ/Utils.php` +**Что:** API не отдаёт CORS-заголовки. Vue-клиент на другом origin получит CORS errors. + +### 8.15 🟠 `api_auth_guard()` вызывает `exit` + +**Где:** `SHServ/App.php:110-120` +**Что:** Прерывает PHP-процесс. Shutdown handlers, логирование, cleanup пропускаются. + +### 8.16 🟠 Деструктивные операции без подтверждения + +**Где:** `remove_device`, `remove_area`, `reset_device`, `scope_remove` +**Что:** Нет `?confirm=true`, soft-delete или cascade warning. + +### 8.17 🟠 Неограниченный размер JSON body + +**Где:** `SHServ/App.php:126-143` +**Что:** `file_get_contents('php://input')` без `Content-Length` проверки. 100MB JSON → OOM. + +### 8.18 🟠 `/events/new` без rate limiting + +**Где:** `SHServ/App.php:65-108` +**Что:** `check_api_auth()` применяет rate limit только к `/api/v1/*`. `/events/new` не подпадает. + +### 8.19 🟠 `EventsController` передаёт raw `$data` в handlers + +**Где:** `SHServ/Controllers/EventsController.php:50-57` +**Что:** Нет схемы валидации данных события. Rogue device может слать malformed payload. + +### 8.20 🟠 `scanning__all` — network reconnaissance + +**Где:** `SHServ/Controllers/DevicesRESTAPIController.php:20-27` +**Что:** Любой auth-пользователь может сканировать всю подсеть и получить IP/MAC/firmware. + +### 8.21 🟡 Несоответствие `total` в `areas_list` scripts + +**Где:** `AreasRESTAPIController.php:260-292` +**Что:** `total` считает сырые записи, но ответ фильтрует отсутствующие scope-классы. + +### 8.22 🟡 Бизнес-логика в контроллере (`device_status`) + +**Где:** `DevicesRESTAPIController.php:169-204` +**Что:** Контроллер напрямую меняет `connection_status` и `last_contact` у entity. Это работа Model. + +### 8.23 🟡 `validate_positive_int_ids` — weak comparison + +**Где:** `SHServ/Middleware/Controller.php:26-33` +**Что:** `"123.0" != 123` → `false`, пропускает. Нет strict comparison. + +### 8.24 🟡 `CallControl` reflection на каждый запрос + +**Где:** `Fury/Kernel/CallControl.php:148-165` +**Что:** `ReflectionClass` + `ReflectionMethod` без кэширования. Лишний overhead. + +### 8.25 🟡 `devices_list` не фильтрует по статусу + +**Где:** `DevicesRESTAPIController.php:296-311` +**Что:** Роут `/api/v1/devices/list` не имеет `$status` параметра. Метод всегда возвращает `active`. + +### 8.26 🟡 `response_error` всегда 400 + +**Где:** `SHServ/Utils.php:14-23` +**Что:** `device_not_found` должен быть 404, `alias_already_exists` — 409. + +### 8.27 🟡 `reboot_devices` игнорирует частичные падения + +**Где:** `AreasRESTAPIController.php:209-227` +**Что:** Нет rollback, нет separate `failed_count`. + +### 8.28 🟡 `ControlScripts` конструктор делает reflection для пути + +**Где:** `SHServ/Middleware/ControlScripts.php:24` +**Что:** Можно заменить на `__FILE__`. + +### 8.29 🟡 Singleton-контроллеры держат состояние + +**Где:** `Fury/Kernel/CallControl.php:150` +**Что:** Если сработает multi-match баг, повторный вызов того же контроллера увидит грязное состояние. + +### 8.30 🟡 Нет audit log + +**Где:** Все контроллеры +**Что:** Нет persistent лога API-вызовов. Невозможно расследование. + +--- + +## Phase 9 — Automation, Scripts, Infrastructure (Phase 4 Review) 📝 Завершена + +**Дата:** 2026-06-02 +**Полный отчёт:** `server/docs/review-phase-4-automation-scripts-infrastructure.md` + +### 9.1 🔴 Static script registries leak state across requests + +**Где:** `SHServ/Middleware/ControlScripts.php:12-13` +**Что:** `static $regular_scripts` и `static $actions_scripts` накапливаются в PHP-FPM worker. Удаление scope-файла не очищает static-память. Тесты сбрасывают reflection'ом, в production — нет. +**Фикс:** `ControlScripts::flush_statics()` вызывается в начале `App::control_scripts_init()`. +**Статус:** ✅ **Исправлено.** + +--- + +### 9.2 🔴 Scope update — non-atomic file write + +**Где:** `SHServ/Controllers/ScriptsRESTAPIController.php:118` +**Что:** `file_put_contents($filepath, $file)` пишет напрямую в live PHP-файл. Прерывание = коррумпированный scope-класс → fatal parse error. +**Фикс:** Temp file → `rename()` atomically, keep `.bak`. + +--- + +### 9.3 🔴 Events post-flush handlers can hang indefinitely + +**Где:** `SHServ/Controllers/EventsController.php:30, 48-57` +**Что:** После `fastcgi_finish_request()` обработчики делают синхронные HTTP-вызовы к устройствам. `set_time_limit(10)` на весь блок, но 5 dispatches × N device calls может превысить лимит. +**Фикс:** Strict per-handler timeout, async queue, или non-blocking HTTP. + +--- + +### 9.4 🔴 RCE via `scope_update` (reinforced) + +**Где:** `SHServ/Controllers/ScriptsRESTAPIController.php:81-102` +**Что:** Любой auth-пользователь перезаписывает scope-файл произвольным PHP. Файл включается на следующем запросе. Нет RBAC. +**Фикс:** Sandbox DSL, `php -l`, disable live editing в production, admin role. + +--- + +### 9.5 🔴 `sync-map.json` loaded redundantly, no caching + +**Где:** `ControlScripts/Common.php:8-22` +**Что:** Каждый scope re-reads `sync-map.json` через `file_get_contents`. С 4 scope'ами — 4 чтения за запрос (~9 KB каждое). +**Фикс:** Загрузить один раз в `App::control_scripts_init()` и раздать scopes. + +--- + +### 9.6 🔴 Cron scripts stop on first failure + +**Где:** `SHServ/Controllers/CronController.php:11-22` +**Что:** `run_regular_cron_scripts()` вызывает `$script["script"]()` без `try/catch`. Один exception = остановка цикла, остальные скрипты не выполняются. +**Фикс:** Wrap each в `try/catch`, log, continue. + +--- + +### 9.7 🟠 Action scripts without error handling + +**Где:** `SHServ/Middleware/ControlScripts.php:120-141` +**Что:** `run_action_script()` вызывает closure без `try/catch`. Exception убивает API-запрос. +**Фикс:** Wrap в `try/catch`, return structured error. + +--- + +### 9.8 🟠 `get_source_code()` re-reads file on every registration + +**Где:** `SHServ/Middleware/ControlScripts.php:143-160` +**Что:** `file()` + `array_slice` при каждом `add_action_script`. 20 скриптов = 20 чтений файла. +**Фикс:** Cache file contents per class. + +--- + +### 9.9 🟠 Eager scope instantiation — no lazy loading + +**Где:** `SHServ/App.php:145-161` +**Что:** `control_scripts_init()` сканирует и инстанциирует ВСЕ `.php` в `Scopes/` на каждый запрос. Даже disabled scopes выполняют конструктор и 4 `register_*` метода. +**Фикс:** Lazy-load или cache metadata. + +--- + +### 9.10 🟠 Cron endpoints return empty response + +**Где:** `SHServ/Controllers/CronController.php` +**Что:** `run_regular_cron_scripts()` и `status_update_scanning()` return nothing. Невозможно узнать, выполнились ли скрипты, сколько упало. +**Фикс:** Return structured JSON с execution summary. + +--- + +### 9.11 🟠 Logging writes to JSON without rotation + +**Где:** `Fury/Kernel/Logging.php:79-119` +**Что:** `Logs/d.m.Y.log.json` растёт бесконечно. Нет max size, rotation, cleanup. +**Фикс:** Rotate by size, archive, или switch to line-based (NDJSON). + +--- + +### 9.12 🟠 No structured logging / severity / correlation IDs + +**Где:** `Fury/Kernel/Logging.php:49-69` +**Что:** `set($place, $title, $message)` — только три строки. Нет severity, request ID, structured fields. +**Фикс:** Add severity, correlation ID, timestamp per entry, context array. + +--- + +### 9.13 🟠 ErrorHandler may leak debug info + +**Где:** `Fury/Modules/ErrorHandler/ErrorHandler.php:127-147` +**Что:** Если `FCONF["debug"]` = true, рендерит file paths, source code, stack traces. `.env` отсутствует → fallback `debug=false`, но misconfiguration раскрывает internals. +**Фикс:** Production = generic JSON error, never HTML. + +--- + +### 9.14 🟠 No DB schema / migrations + +**Где:** project-wide +**Что:** Нет `.sql` файлов, install script, schema versioning. `scripts` table definition только в тестах (`ScriptsModelStateTest.php`). +**Фикс:** `server/migrations/` с numbered SQL и CLI runner. + +--- + +### 9.15 🟠 `.env` absent — insecure defaults + +**Где:** `SHServ/config.php:22-28` +**Что:** Fallback DB credentials: `user => "root"`, `password => ""`. +**Фикс:** Fail hard if `.env` missing in production. + +--- + +### 9.16 🟠 ControlScripts constructor — DB check per scope (N+1) + +**Где:** `SHServ/Middleware/ControlScripts.php:24-26` +**Что:** Для каждого scope конструктор делает `new Scripts() -> script_state()` = DB round-trip. N scopes = N+1 запросов. +**Фикс:** Batch-fetch all scope states в одном query. + +--- + +### 9.17 🟠 `sync_map()` returns mutable shared reference + +**Где:** `SHServ/Middleware/ControlScripts.php:166-168` +**Что:** Возвращает `self::$sync_map_storage`. Scope может мутировать глобальную карту, влияя на другие scopes. +**Фикс:** Return deep copy, или immutable storage. + +--- + +### 9.18 🟠 `DeviceScanner` silently discards failures + +**Где:** `SHServ/Tools/DeviceScanner.php:144-146` +**Что:** Failed IPs skipped без логирования. Нет audit trail для disappeared devices. +**Фикс:** Log scan results: found, lost, unreachable. + +--- + +### 9.19 🟠 `text-msgs.php` empty strings for SHServ errors + +**Где:** `SHServ/text-msgs.php:30-48` +**Что:** `device_not_found`, `unknown_device`, `error_of_device_auth`, `db_error`, `action_script_not_found`, `scope_not_found` и др. имеют пустые значения. Клиент получает пустые `msg`. +**Фикс:** Заполнить meaningful messages. + +--- + +### 9.20 🟠 `RequiredControlScriptsScope` mixes business logic with event handling + +**Где:** `SHServ/RequiredControlScriptsScope.php:12-15` +**Что:** `online` handler напрямую мутирует `$device->device_ip`, `$device->connection_status`, `$device->update()`. Это business logic, не event handling. +**Фикс:** Перенести в `Devices` model или dedicated service. + +--- + +### 9.21 🟡 `EventsHandlers` devmode clutter + +**Где:** `SHServ/EventsHandlers.php:21-60` +**Что:** Devmode handlers регистрируются в коде всегда, проверка runtime. +**Фикс:** Conditional class loading или separate dev bootstrap. + +--- + +### 9.22 🟡 `console.php` only one command + +**Где:** `server/console.php:8-19` +**Что:** Только `get.config`. Нет миграций, health check, log cleanup, manual cron run. +**Фикс:** Command registry с useful ops. + +--- + +### 9.23 🟡 `sync-map.json` no schema validation + +**Где:** `ControlScripts/Common.php:8-22` +**Что:** Raw JSON без schema check. Нет проверки alias existence, channel validity, cycle detection. +**Фикс:** JSON Schema + validation at bootstrap. + +--- + +### 9.24 🟡 `ControlScriptsInterface` lacks return types + +**Где:** `SHServ/Implements/ControlScriptsInterface.php:13-32` +**Что:** Interface methods без return type declarations, implementations используют `: void`. +**Фикс:** Add `void` to interface. + +--- + +### 9.25 🟡 No observability metrics + +**Где:** project-wide +**Что:** Нет counters: events dispatched, actions executed, cron failed, device API latency, handler latency. +**Фикс:** Lightweight in-memory metrics endpoint или Prometheus integration. + +--- + +### 9.26 🟡 `add_event_handler` discards return values + +**Где:** `SHServ/Middleware/ControlScripts.php:34-38` +**Что:** Wrapper closure игнорирует return value handler'а. Нет `stopPropagation`. +**Фикс:** Return handler result, или explicit propagation control. + +--- + +### 9.27 🟡 Empty regular script `spotlights_by_time` + +**Где:** `ControlScripts/Scopes/SpotlightsScope.php:64-70` +**Что:** Registered but empty body. Pollutes cron loop. +**Фикс:** Remove или implement. + +--- + +### 9.28 🟡 `TestScriptsScope` hardcodes production device + +**Где:** `ControlScripts/Scopes/TestScriptsScope.php:23-26` +**Что:** Alias `test_stand_relay` hardcoded. В production может не существовать. +**Фикс:** Gate behind `devmode` или separate test directory. + +--- + +### 9.29 🟡 `EventsModel` inconsistent naming + +**Где:** `SHServ/Models/EventsModel.php` +**Что:** `global_any_device_event_call`, `global_device_event_call`, `channel_device_event_call` — inconsistent naming pattern. +**Фикс:** Standardize to `{scope}_{target}_event_call`. + +--- + +### 9.30 🟡 `ControlScripts` constructor fragile string parsing + +**Где:** `SHServ/Middleware/ControlScripts.php:24` +**Что:** `explode("\\", str_replace("\\Scopes", "", str_replace("SHServ", "", static::class)))` — хрупко. +**Фикс:** `basename(str_replace('\\', '/', static::class))`. + +--- + +### 9.31 🟡 `Scripts::remove_scope` TOCTOU race + +**Где:** `SHServ/Models/Scripts.php:29-51` +**Что:** `file_exists()` then `unlink()`. File may change between calls. +**Фикс:** Just `unlink()` and check result. + +--- + +### 9.32 🟡 No correlation ID for event dispatch chain + +**Где:** `SHServ/Controllers/EventsController.php` +**Что:** Нет tracing ID correlating original event with downstream device API calls. +**Фикс:** Generate `correlation_id` and propagate through handlers. + +--- + +### 9.33 🟡 `Logging` JSON not grep-friendly + +**Где:** `Fury/Kernel/Logging.php:79-119` +**Что:** Entire JSON file loaded to parse/search. +**Фикс:** NDJSON (newline-delimited JSON) for append-only, grep-friendly format. + +--- + +### 9.34 🟡 `RequiredControlScriptsScope` mixed with optional scopes + +**Где:** `SHServ/App.php:145-161` +**Что:** Required scope регистрируется в те же static arrays. Если statics cleared — required handlers lost. +**Фикс:** Separate storage for required vs optional. + +--- + +### 9.35 🟡 `error_handler` config ignores notices/deprecations + +**Где:** `SHServ/config.php:39-41` +**Что:** `important_errors` only `E_WARNING`, `E_ERROR`, `E_CORE_ERROR`, `EXCEPTION`. Notices и deprecations silently ignored. +**Фикс:** Include в dev; log (не display) в production. + +--- + ## Приложение: Файлы, требующие внимания | Файл | Phase | Проблемы | Статус | diff --git a/server/Cache/rate-limit.json b/server/Cache/rate-limit.json new file mode 100644 index 0000000..9e26dfe --- /dev/null +++ b/server/Cache/rate-limit.json @@ -0,0 +1 @@ +{} \ No newline at end of file diff --git a/server/Fury/Modules/Router/Router.php b/server/Fury/Modules/Router/Router.php index 6e0d055..475273a 100644 --- a/server/Fury/Modules/Router/Router.php +++ b/server/Fury/Modules/Router/Router.php @@ -70,8 +70,8 @@ public function start_routing(){ $result = []; - $result['get'] = $this -> GET_and_POST_routing($this -> routes_map['get'], $_GET); - $result['post'] = $this -> GET_and_POST_routing($this -> routes_map['post'], $_POST); + $result['get'] = $this -> GET_and_POST_routing($this -> routes_map['get'], $_GET, 'GET'); + $result['post'] = $this -> GET_and_POST_routing($this -> routes_map['post'], $_POST, 'POST'); $result['uri'] = $this -> URI_routing($this -> routes_map['uri']); } diff --git a/server/Fury/Modules/Router/RouterImplementation.php b/server/Fury/Modules/Router/RouterImplementation.php index 5f124fe..96e6687 100644 --- a/server/Fury/Modules/Router/RouterImplementation.php +++ b/server/Fury/Modules/Router/RouterImplementation.php @@ -58,9 +58,13 @@ * @param [array] $routes_map_part [Array with routes templates] * @param [array] $vars [Current vars GET or POST] */ - protected function GET_and_POST_routing($routes_map_part, $vars){ + protected function GET_and_POST_routing($routes_map_part, $vars, $expected_method){ $result_routes = []; + if(!isset($_SERVER['REQUEST_METHOD']) or $_SERVER['REQUEST_METHOD'] != $expected_method){ + return $result_routes; + } + foreach ($routes_map_part as $route => $action) { if(strpos($route, '?')){ list($route_uri, $route_vars) = explode('?', $route); diff --git a/server/Fury/Modules/ThinBuilder/ThinBuilder.php b/server/Fury/Modules/ThinBuilder/ThinBuilder.php index 9cfecdc..d2f272c 100644 --- a/server/Fury/Modules/ThinBuilder/ThinBuilder.php +++ b/server/Fury/Modules/ThinBuilder/ThinBuilder.php @@ -41,6 +41,10 @@ return $result; } + public function prepare(String $sql) { + return $this -> pdo -> prepare($sql); + } + public function select(String $tablename, $fields = [], $where = [], $order_fields = [], String $order_sort = 'DESC', $limit = []){ $this -> validate_identifier($tablename); list($fields_sql, $where_sql, $order_sql, $limit_sql, $params) = $this -> select_data_preprocessing($fields, $where, $order_fields, $limit); diff --git a/server/SHServ/App.php b/server/SHServ/App.php index 4b4d36e..19246be 100644 --- a/server/SHServ/App.php +++ b/server/SHServ/App.php @@ -143,6 +143,8 @@ } public function control_scripts_init(): void { + \SHServ\Middleware\ControlScripts::flush_statics(); + $this -> required_control_scripts_instance = new \SHServ\RequiredControlScriptsScope(); $scopes_dir = __DIR__ . "/../ControlScripts/Scopes/"; diff --git a/server/SHServ/Controllers/CronController.php b/server/SHServ/Controllers/CronController.php index 2211fbf..3429c90 100644 --- a/server/SHServ/Controllers/CronController.php +++ b/server/SHServ/Controllers/CronController.php @@ -8,7 +8,22 @@ use \SHServ\Models\Scripts; class CronController extends \SHServ\Middleware\Controller { + protected function ensure_localhost_only() { + $remote_addr = $_SERVER['REMOTE_ADDR'] ?? ''; + if(!in_array($remote_addr, ['127.0.0.1', '::1'])) { + http_response_code(403); + header('Content-Type: application/json'); + echo json_encode([ + "status" => false, + "error_alias" => "forbidden", + "msg" => "Forbidden: local access only" + ]); + exit; + } + } + public function run_regular_cron_scripts() { + $this -> ensure_localhost_only(); $scripts_model = new Scripts(); $regular_scripts = ControlScripts::get_regular_scripts(); @@ -22,6 +37,7 @@ } public function status_update_scanning() { + $this -> ensure_localhost_only(); $device_scanner = new DeviceScanner(); $found_devices = $device_scanner -> scan_range(FCONF["device_ip_range"][0], FCONF["device_ip_range"][1]); diff --git a/server/SHServ/Entities/Area.php b/server/SHServ/Entities/Area.php index 5abd85b..3040cb4 100644 --- a/server/SHServ/Entities/Area.php +++ b/server/SHServ/Entities/Area.php @@ -265,19 +265,26 @@ } public function remove(): Bool { - app() -> thin_builder -> update( - Device::$table_name, - ["area_id" => 0], - [[ "area_id", "=", $this -> id() ]] - ); + app() -> thin_builder -> beginTransaction(); + try { + app() -> thin_builder -> update( + Device::$table_name, + ["area_id" => 0], + [[ "area_id", "=", $this -> id() ]] + ); - app() -> thin_builder -> update( - self::$table_name, - ["parent_id" => 0], - [[ "parent_id", "=", $this -> id() ]] - ); + app() -> thin_builder -> update( + self::$table_name, + ["parent_id" => 0], + [[ "parent_id", "=", $this -> id() ]] + ); - $this -> remove_entity(); - return true; + $this -> remove_entity(); + app() -> thin_builder -> commit(); + return true; + } catch (\Exception $e) { + app() -> thin_builder -> rollBack(); + throw $e; + } } } \ No newline at end of file diff --git a/server/SHServ/Middleware/ControlScripts.php b/server/SHServ/Middleware/ControlScripts.php index 39dd663..7b660ca 100644 --- a/server/SHServ/Middleware/ControlScripts.php +++ b/server/SHServ/Middleware/ControlScripts.php @@ -81,6 +81,12 @@ return true; } + public static function flush_statics(): void { + self::$regular_scripts = []; + self::$actions_scripts = []; + self::$sync_map_storage = ["connections" => []]; + } + public static function get_regular_scripts(): Array { return self::$regular_scripts; } diff --git a/server/SHServ/Middleware/Entity.php b/server/SHServ/Middleware/Entity.php index 064537f..6595b1d 100644 --- a/server/SHServ/Middleware/Entity.php +++ b/server/SHServ/Middleware/Entity.php @@ -70,7 +70,10 @@ } $where = [ ["id", "=", $this -> entity_id] ]; - $this -> modified_fields[$this -> field_name_of_update_at] = date("Y-m-d H:i:s"); + + if(in_array($this -> field_name_of_update_at, static::$fields, true)) { + $this -> modified_fields[$this -> field_name_of_update_at] = date("Y-m-d H:i:s"); + } $this -> thin_builder() -> update($this -> entity_tablename, $this -> modified_fields, $where); diff --git a/server/SHServ/Models/Devices.php b/server/SHServ/Models/Devices.php index c102199..e872158 100644 --- a/server/SHServ/Models/Devices.php +++ b/server/SHServ/Models/Devices.php @@ -35,6 +35,20 @@ try { app() -> thin_builder -> beginTransaction(); + $check_sql = "SELECT id FROM `" . Device::$table_name . "` WHERE `device_hard_id` = ? AND `status` = 'active'"; + if(in_array(FCONF['db']['dblib'] ?? '', ['mysql', 'mariadb'])) { + $check_sql .= " FOR UPDATE"; + } + $stmt = app() -> thin_builder -> prepare($check_sql); + $stmt -> execute([$device_info["data"]["device_id"]]); + if($stmt -> fetch(\PDO::FETCH_ASSOC)) { + app() -> thin_builder -> rollBack(); + return [ + "result" => false, + "err_alias" => "device_already_registered" + ]; + } + // create in table devices $device_id = app() -> thin_builder -> insert(Device::$table_name, [ "alias" => $alias, diff --git a/server/SHServ/Tools/RateLimiter.php b/server/SHServ/Tools/RateLimiter.php index fd855c2..a0b9966 100644 --- a/server/SHServ/Tools/RateLimiter.php +++ b/server/SHServ/Tools/RateLimiter.php @@ -3,31 +3,104 @@ namespace SHServ\Tools; class RateLimiter { - protected static array $requests = []; protected int $maxRequests; protected int $windowSeconds; + protected string $storagePath; - public function __construct(int $maxRequests = 60, int $windowSeconds = 60) { + public function __construct(int $maxRequests = 60, int $windowSeconds = 60, ?string $storagePath = null) { $this -> maxRequests = $maxRequests; $this -> windowSeconds = $windowSeconds; + $this -> storagePath = $storagePath ?? dirname(__DIR__, 2) . '/Cache/rate-limit.json'; + $this -> ensureStorage(); + } + + protected function ensureStorage(): void { + $dir = dirname($this -> storagePath); + if (!is_dir($dir)) { + mkdir($dir, 0750, true); + } + if (!file_exists($this -> storagePath)) { + file_put_contents($this -> storagePath, '{}'); + chmod($this -> storagePath, 0640); + } } public function check(String $key): bool { - $now = time(); - if(!isset(self::$requests[$key])) { - self::$requests[$key] = []; + if (extension_loaded('apcu') && apcu_enabled()) { + return $this -> checkApcu($key); } + return $this -> checkFile($key); + } - // Удаляем устаревшие записи - self::$requests[$key] = array_filter(self::$requests[$key], function($timestamp) use ($now) { - return $now - $timestamp < $this -> windowSeconds; + protected function checkApcu(String $key): bool { + $cacheKey = 'rate_limit:' . $key; + $requests = apcu_fetch($cacheKey); + if ($requests === false) { + $requests = []; + } + $now = time(); + $requests = array_filter($requests, function($ts) use ($now) { + return $now - $ts < $this -> windowSeconds; }); - - if(count(self::$requests[$key]) >= $this -> maxRequests) { + if (count($requests) >= $this -> maxRequests) { + apcu_store($cacheKey, array_values($requests), $this -> windowSeconds); return false; } - - self::$requests[$key][] = $now; + $requests[] = $now; + apcu_store($cacheKey, array_values($requests), $this -> windowSeconds); return true; } + + protected function checkFile(String $key): bool { + $fp = fopen($this -> storagePath, 'c+'); + if (!$fp) { + return true; + } + if (!flock($fp, LOCK_EX)) { + fclose($fp); + return true; + } + + $data = json_decode(stream_get_contents($fp), true) ?: []; + $now = time(); + if (!isset($data[$key])) { + $data[$key] = []; + } + $data[$key] = array_values(array_filter($data[$key], function($ts) use ($now) { + return $now - $ts < $this -> windowSeconds; + })); + + $allowed = count($data[$key]) < $this -> maxRequests; + if ($allowed) { + $data[$key][] = $now; + } + + ftruncate($fp, 0); + rewind($fp); + fwrite($fp, json_encode($data)); + fflush($fp); + flock($fp, LOCK_UN); + fclose($fp); + + return $allowed; + } + + public function clear(): void { + if (extension_loaded('apcu') && apcu_enabled()) { + apcu_delete('rate_limit:'); + return; + } + $fp = fopen($this -> storagePath, 'c+'); + if (!$fp) { + return; + } + if (flock($fp, LOCK_EX)) { + ftruncate($fp, 0); + rewind($fp); + fwrite($fp, '{}'); + fflush($fp); + flock($fp, LOCK_UN); + } + fclose($fp); + } } diff --git a/server/docs/review-phase-1-security-auth-device.md b/server/docs/review-phase-1-security-auth-device.md new file mode 100644 index 0000000..109ce7f --- /dev/null +++ b/server/docs/review-phase-1-security-auth-device.md @@ -0,0 +1,100 @@ +# Ревью серверной части — Этап 1: Security, Auth, Device Integration + +**Дата:** 2026-06-02 +**Охват:** `App.php`, `Tools/DeviceAPI/Base.php`, `Models/Devices.php`, `Controllers/EventsController.php`, `Tools/RateLimiter.php`, `Sessions.php`, `Middleware/Entity.php`, Entity-классы. + +--- + +## 🔴 Критические + +### 1. RateLimiter не работает в PHP-FPM/Apache (`RateLimiter.php`) +`RateLimiter` хранит `$requests` в **static property процесса**. При стандартном deployment (PHP-FPM, Apache mod_php) каждый HTTP-запуск — новый процесс, поэтому счётчик сбрасывается. Rate limit фактически отсутствует. +**Рекомендация:** перенести на Redis / memcached / DB, или хотя бы на file-based storage. + +### 2. `/events/new` не требует авторизации (`Routes.php`, `EventsController.php`) +`check_api_auth()` пропускает любой URI, не начинающийся с `/api/v1/`. `/events/new` — вне этого префикса, поэтому endpoint публичен. Любой может отправить события от имени любого устройства, зная его `device_hard_id`. +**Рекомендация:** добавить `device_token` в заголовок Authorization и проверять его в `EventsController::new_event` перед `by_hard_id`, или перенести endpoint под `/api/v1/` с auth guard. + +### 3. Race condition в `connect_new_device()` (`Models/Devices.php`) +Нет atomic проверки статуса устройства. Два параллельных запроса могут одновременно увидеть `"setup"`, создать две записи в `devices`+`device_auth`, и оба пройдут. Устройство перейдёт в normal mode, но в БД останутся дубли. +**Рекомендация:** `UNIQUE` constraint на `device_hard_id` + `status='active'`, или distributed lock через `SELECT ... FOR UPDATE` внутри транзакции. + +### 4. `Entity::update()` unconditionally требует `update_at` (`Middleware/Entity.php`) +`Entity::update()` всегда inject-ит `update_at` в UPDATE SQL. `DeviceAuth` (таблица `device_auth`) **не содержит** `update_at` в `$fields`, и скорее всего нет и в production schema. Вызов `DeviceAuth::kill()` → `update()` вызовет SQL error. +**Рекомендация:** добавить `update_at TEXT` в production schema `device_auth`, или сделать `update_at` опциональным в Entity (например, проверять `in_array($this->field_name_of_update_at, static::$fields)`). + +--- + +## 🟡 Высокие + +### 5. Post-commit remote state inconsistency в `connect_new_device()` +Транзакция commit-ится, а затем вызывается: +- `$device->set_device_token($device_token)` → `remote_set_token` на устройство +- `$device->device_api()->set_device_name($name)` → `POST /set_device_name` + +Если устройство offline или не отвечает после commit, в БД устройство зарегистрировано, но физически оно не знает токен/имя. Управление невозможно. +**Рекомендация:** либо делать remote setup **до** commit (и rollback при fail), либо иметь флаг `setup_pending` + background retry job. + +### 6. `DeviceAuth::kill()` не инвалидирует кэш `device_auth_instance` +`Device::auth()` кэширует `device_auth_instance`. После `$device_auth->kill()` статус меняется на `"killed"`, но кэш в `$device->device_auth_instance` остаётся со старым статусом `"active"`. Последующие вызовы `$device->auth()->is_active()` вернут `true`. +**Рекомендация:** сбрасывать `$device->device_auth_instance = null` в `DeviceAuth::kill()` или в `Device::set_device_token()`. + +### 7. `EventsController::new_event` — `device_id` приходит извне без подписи +Устройство доказывает свою идентичность только через `device_hard_id`, который передаётся в POST body. Нет HMAC, timestamp, nonce. Replay attack возможен. +**Рекомендация:** добавить `Authorization: Bearer ` (см. пункт 2) и/или подпись полезной нагрузки. + +--- + +## 🟢 Средние + +### 8. `generate_token(16)` — низкая энтропия +16 hex-символов = 64 бита. Для устройственных токенов в локальной сети это терпимо, но на грани. +**Рекомендация:** поднять до 32 (128 бит) для `device_token`. + +### 9. `Base::request()` — нет валидации `$ip_address` +`http://` + произвольная строка. Если `$ip_address` содержит path (`192.168.1.1/path`), SSRF возможен. Контролируется server-side, но всё же. +**Рекомендация:** валидировать `filter_var($ip_address, FILTER_VALIDATE_IP)` в конструкторе. + +### 10. `Entity::update()` — тихий success при SQL error? +`Entity::update()` вызывает `$this->thin_builder()->update(...)` и не проверяет return value. `ThinBuilder::update` возвращает `Statement` или `null`? Возможно, exception вылетит (PDO ERRMODE_EXCEPTION), но если нет — тихий fail. +**Рекомендация:** явно проверять результат `thin_builder()->update()` и возвращать `false` при ошибке. + +### 11. `Sessions::get_current_session()` — лишний UPDATE на каждый запрос +Обновление `last_using_at` при каждом вызове создаёт write load. Для домашнего сервера не критично, но при масштабе заметно. +**Рекомендация:** обновлять раз в N минут (например, если delta > 60s). + +### 12. `EventsController` — `set_time_limit(10)` может обрезать handlers +Если `channel_alias_device_event_call` и последующие вызовы занимают >10s — fatal. Это фоновая обработка после flush. +**Рекомендация:** `set_time_limit(0)` или background queue (cron / worker). + +### 13. `check_api_auth()` — Bearer token parsing +`substr($auth_header, 7)` без проверки длины. Header `"Bearer"` (без пробела) даст пустой token, который пройдёт в `!$token` → 401. Не критично, но стоит использовать `str_starts_with` + `explode(' ', $auth_header, 2)[1] ?? null`. + +--- + +## ✅ Что сделано хорошо + +- **Auth guard разделён** на `check_api_auth()` (чистая логика) и `api_auth_guard()` (side-effects). Это позволяет тестировать без `exit`. +- **Cookie security flags** — `httponly`, `secure`, `samesite=Strict`. +- **Retry/backoff** в `DeviceAPI\Base` — exponential backoff, clear separation через `executeCurl`/`getCurlInfo`. +- **Транзакции** в `connect_new_device` — rollback при ошибке. +- **PDO prepared statements** в ThinBuilder — SQL injection минимизирован. + +--- + +## 📋 Action Items (итог) + +| Приоритет | Задача | Файл | +|-----------|--------|------| +| 🔴 Крит | Починить rate limiter для stateless PHP | `RateLimiter.php` | +| 🔴 Крит | Защитить `/events/new` токеном устройства | `Routes.php`, `EventsController.php` | +| 🔴 Крит | Убрать race condition в `connect_new_device` | `Models/Devices.php` | +| 🔴 Крит | Добавить `update_at` в `device_auth` schema или сделать optional | `Middleware/Entity.php` / DB schema | +| 🟡 Выс | Post-commit remote calls → pre-commit или pending flag | `Models/Devices.php` | +| 🟡 Выс | Инвалидировать `device_auth_instance` при `kill()` | `Entities/DeviceAuth.php` | +| 🟡 Выс | Replay-защита на `/events/new` | `EventsController.php` | +| 🟢 Сред | Увеличить энтропию device_token | `Utils.php` / `Models/Devices.php` | +| 🟢 Сред | Валидировать IP в `DeviceAPI\Base` | `Tools/DeviceAPI/Base.php` | +| 🟢 Сред | Проверять return value в `Entity::update()` | `Middleware/Entity.php` | +| 🟢 Сред | Throttle `last_using_at` updates | `Sessions.php` | +| 🟢 Сред | `set_time_limit(0)` в EventsController | `Controllers/EventsController.php` | diff --git a/server/docs/review-phase-2-models-data-layer.md b/server/docs/review-phase-2-models-data-layer.md new file mode 100644 index 0000000..04d8f62 --- /dev/null +++ b/server/docs/review-phase-2-models-data-layer.md @@ -0,0 +1,478 @@ +# Ревью серверной части — Фаза 2: Models и Data Layer + +**Дата:** 2026-06-02 +**Ветка:** dev +**Область:** `server/SHServ/{Models,Entities,Middleware,Helpers,Factory}`, `server/Fury/Modules/ThinBuilder` + +--- + +## Сводка + +Проверены модели, Entity-классы, ORM ThinBuilder, фабрики и вспомогательные хелперы. Обнаружено **34 находки**: 5 критических, 14 высокого приоритета, 15 среднего/информационного. + +--- + +## 🔴 Критические + +### 1. Entity::select_from_db() молча падает при отсутствии строки в БД +**Файл:** `server/SHServ/Middleware/Entity.php:36-45` + +```php +protected function select_from_db() { + list($this -> data) = $this -> thin_builder() -> select( + $this -> entity_tablename, + [], + ['id', '=', $this -> entity_id], + ['id'], + 'DESC', + [0, 1] + ); +} +``` + +Если записи с таким `id` нет, `select()` возвращает `[]`. Конструкция `list($this->data) = []` присваивает `$this->data = null`. Поле `$was_filled` всё равно устанавливается в `true`. При следующем обращении к свойству через `__get()` → `get()` происходит обращение к `$this->data[$field_name]` где `$this->data === null` — fatal error. + +**Последствия:** Конструирование Entity с несуществующим ID приводит к fatal error при первом доступе к полю, вместо управляемой ошибки. + +**Рекомендация:** Проверять результат `select()` и бросать исключение `RecordNotFound` если строка отсутствует. + +--- + +### 2. DeviceAuth::kill() упадёт в production из-за отсутствия колонки update_at +**Файлы:** `server/SHServ/Entities/DeviceAuth.php:7-9`, `server/SHServ/Middleware/Entity.php:67-79` + +`DeviceAuth::$fields` не содержит `update_at`. `Entity::update()` безусловно инжектирует `update_at` в `modified_fields` и генерирует `UPDATE ... SET update_at = ?`. Тестовая схема (`DevicesModelTransactionTest.php:44-52`) создаёт таблицу `device_auth` **с** колонкой `update_at`, поэтому тесты проходят. В production схема создана отдельно, и колонки `update_at` может не быть — вызов `kill()` приведёт к SQL-ошибке. + +**Последствия:** Удаление устройства (`Devices::remove_device()`) вызывает `DeviceAuth::kill()`, которое бросает PDOException. Токен устройства остаётся активным в БД. + +**Рекомендация:** Либо добавить `update_at` в production-таблицу `device_auth`, либо переопределить `update()` в `DeviceAuth` без инъекции `update_at`. + +--- + +### 3. Device::remove() не инвалидирует токен авторизации +**Файл:** `server/SHServ/Entities/Device.php:36-39` + +```php +public function remove() { + $this -> status = "removed"; + return $this -> update(); +} +``` + +Метод меняет статус устройства на `removed`, но не трогает таблицу `device_auth`. Строка авторизации остаётся со статусом `active`, и токен продолжает считаться валидным. + +**Последствия:** "Удалённое" устройство теоретически может продолжать отправлять события и выполнять команды, пока токен не протухнет. + +**Рекомендация:** В `remove()` дополнительно вызывать `$this->auth()->kill()` или делать `remove()` транзакционной операцией. + +--- + +### 4. Area::remove() выполняет 3 обновления без транзакции +**Файл:** `server/SHServ/Entities/Area.php:267-282` + +```php +public function remove(): Bool { + app() -> thin_builder -> update(Device::$table_name, ["area_id" => 0], ...); + app() -> thin_builder -> update(self::$table_name, ["parent_id" => 0], ...); + $this -> remove_entity(); + return true; +} +``` + +Три независимых запроса к БД без `beginTransaction`. При падении после первого или второго запроса часть устройств/под-ареа останется с битой ссылкой, а сама ареа может или не удалиться. + +**Рекомендация:** Обернуть в транзакцию. + +--- + +### 5. Scripts::select_scripts_by_aliases_types() падает на пустом массиве +**Файл:** `server/SHServ/Models/Scripts.php:63-73` + +```php +public function select_scripts_by_aliases_types(String $type, Array $uniq_names): Array { + $result = $this -> thin_builder() -> select( + Script::$table_name, + Script::get_fields(), + [ ["type", "=", $type], "AND", [ "uniq_name", 'IN', $uniq_names ] ] + ); +``` + +При `$uniq_names = []` генерируется `... WHERE type = ? AND uniq_name IN ()` — синтаксическая ошибка SQL. + +**Последствия:** Вызов метода с пустым списком псевдонимов бросает PDOException. + +**Рекомендация:** В начале метода делать ранний возврат `[]` если массив пуст. + +--- + +## 🟠 Высокий приоритет + +### 6. ThinBuilder не валидирует операторы WHERE +**Файл:** `server/Fury/Modules/ThinBuilder/ThinBuilderProcessing.php:147-156` + +```php +$operator = strtoupper($w_item[1]); +if($operator == 'IN'){ ... } +else { + $sql_parts[] = "{$field} {$operator} ?"; +} +``` + +Любая строка проходит как оператор прямо в SQL. Значения параметризованы, но сам оператор — сырой текст. Хотя `validate_identifier()` защищает от инъекции в имена колонок, оператор не проходит whitelist. + +**Рекомендация:** Добавить whitelist допустимых операторов: `=`, `!=`, `<>` `<`, `>`, `<=`, `>=`, `LIKE`, `IN`, `IS`, `NOT IN`, `BETWEEN`. + +--- + +### 7. validate_identifier() разрешает числовой старт имени +**Файл:** `server/Fury/Modules/ThinBuilder/ThinBuilderProcessing.php:62-67` + +Регулярка `/^[a-zA-Z0-9_]+$/` пропускает идентификаторы вида `123abc`, что недопустимо в SQL. Правильная регулярка: `/^[a-zA-Z_][a-zA-Z0-9_]*$/`. + +--- + +### 8. escape_string_in_arr() — мёртвый код с опасным экранированием +**Файл:** `server/Fury/Modules/ThinBuilder/ThinBuilderProcessing.php:69-79` + +Метод использует `addslashes()` вместо prepared statements. Он нигде не вызывается в кодовой базе (кроме определения), но его наличие создаёт риск, что кто-то воспользуется "для быстрого фикса". + +**Рекомендация:** Удалить метод. + +--- + +### 9. MetaManager::create_or_update() — race condition SELECT-then-INSERT +**Файл:** `server/SHServ/Models/MetaManager.php:75-86` + +```php +public function create_or_update(String $name, String $value, String $assignment, int $ent_id): bool { + $entry = $this -> get_one_by_name($name, $assignment, $ent_id); + if($entry) { + $entry -> value = $value; + $result = $entry -> update(); + } else { + $result = $this -> create($name, $value, $assignment, $ent_id); + } +``` + +При конкурентных запросах два потока могут одновременно пройти `if(!$entry)` и вставить дубль. + +**Рекомендация:** Использовать `INSERT ... ON DUPLICATE KEY UPDATE` (MySQL) или добавить UNIQUE-индекс `(assignment, ent_id, name)` и ловить дубли на уровне БД. + +--- + +### 10. User::__construct() жадно загружает профиль +**Файл:** `server/SHServ/Entities/User.php:16-28` + +Каждое создание `User` — даже при массовой выборке — инициирует дополнительный `SELECT` из `profiles`. Это классическая N+1 проблема. + +**Рекомендация:** Сделать загрузку профиля ленивой (через `get_pet_instance` как `last_session()`), либо добавить eager-join в `Factory\Getter::get_user_by()`. + +--- + +### 11. Scripts::remove_scope() — файловая система и БД рассинхронизируются при ошибке +**Файл:** `server/SHServ/Models/Scripts.php:29-51` + +```php +if(!@unlink($filepath)) { return false; } +$script = new Script($name); +$script -> remove(); +``` + +Если `unlink()` прошёл успешно, а `$script->remove()` упал, файл уже удалён из диска, но запись в БД осталась. + +**Рекомендация:** Либо удалять файл **после** успешного удаления из БД, либо оборачивать в транзакцию с компенсирующим действием. + +--- + +### 12. Entity::update() всегда возвращает true, игнорируя результат БД +**Файл:** `server/SHServ/Middleware/Entity.php:67-79` + +```php +$this -> thin_builder() -> update($this -> entity_tablename, $this -> modified_fields, $where); +$this -> modified_fields = []; +return true; +``` + +`ThinBuilder::update()` возвращает `rowCount()`, но Entity игнорирует его. Если `UPDATE` не затронул ни одной строки (например, запись была удалена параллельно), метод всё равно вернёт `true`. + +**Рекомендация:** Проверять `rowCount() > 0` или возвращать его, и не сбрасывать `modified_fields` при нулевом rowCount. + +--- + +### 13. IN-клауза с не-массивом падает на PHP 8+ +**Файл:** `server/Fury/Modules/ThinBuilder/ThinBuilderProcessing.php:149-151` + +```php +if($operator == 'IN'){ + $placeholders = array_fill(0, count($w_item[2]), '?'); +``` + +В PHP 7 `count("string")` возвращает `1`. В PHP 8+ `count()` на скаляре бросает `TypeError`. Если по ошибке передать строку вместо массива в `IN`, приложение упадёт с необработанным исключением. + +**Рекомендация:** Добавить проверку `is_array($w_item[2])` с понятным исключением. + +--- + +### 14. Короткий синтаксис where не поддерживает IN +**Файл:** `server/Fury/Modules/ThinBuilder/ThinBuilderProcessing.php:115-130` + +Плоский массив из двух элементов `['field', ['v1', 'v2']]` (подразумевая IN) превращается в `['field', '=', ['v1', 'v2']]` из-за ветки `count($where) === 2`. В итоге массив значений пытается забиндиться как скаляр. + +**Рекомендация:** В `normalize_where` обрабатывать случай, когда второй элемент — массив (IN). + +--- + +### 15. Entity::update() не сбрасывает modified_fields при исключении +**Файл:** `server/SHServ/Middleware/Entity.php:67-79` + +Если `thin_builder()->update()` бросает PDOException, `modified_fields` остаётся заполненным. При повторном вызове `update()` (например, после retry или в catch-блоке выше) будут записаны устаревшие + новые изменения. + +**Рекомендация:** Обернуть в `try/finally` и сбрасывать `modified_fields` только при успехе. + +--- + +### 16. Device::set_device_token() делает избыточные вызовы auth() +**Файл:** `server/SHServ/Entities/Device.php:94-102` + +```php +public function set_device_token(String $token) { + $this -> device_api() -> remote_set_token($token); + if(!$this -> auth()) return false; + $this -> auth() -> device_token = $token; // вызов #2 + $this -> device_api_instance -> set_local_token($token); + return $this -> auth() -> update(); // вызов #3 +} +``` + +`auth()` вызывается трижды. Метод `device_api()` тоже вызывает `auth()` внутри себя. + +**Рекомендация:** Закэшировать `$auth = $this->auth()` в переменную. + +--- + +### 17. Scripts::set_script_state() чрезвычайно неэффективен +**Файл:** `server/SHServ/Models/Scripts.php:75-128` + +Для scope-типа на каждый вызов делает `ReflectionClass` на все scope-инстансы. Для regular/action вызывает `regular_scripts_list()` / `actions_scripts_list()`, которые тянут из БД все записи и делают `array_filter` в цикле (сложность O(n·m)). + +**Рекомендация:** Построить хеш-таблицу (keyed by `uniq_name`) один раз вместо линейного поиска. + +--- + +### 18. Area::get_exists_types() сканирует всю таблицу вместо DISTINCT +**Файл:** `server/SHServ/Models/Areas.php:36-57` + +```php +$areas = app() -> thin_builder -> select( Area::$table_name, [ "type" ], [] ); +// ... array_map + ручной foreach-dedup +``` + +Запрашивает все строки таблицы, затем дедуплицирует в PHP. При 1000 ареа это 1000 строк в памяти. + +**Рекомендация:** Использовать `SELECT DISTINCT type FROM areas` через `query()` или добавить метод `distinct()` в ThinBuilder. + +--- + +### 19. Area::get_recursive_inner_areas_ids() жёстко ограничен глубиной 10 +**Файл:** `server/SHServ/Entities/Area.php:129-158` + +```php +if($lvl >= 10) { return []; } +``` + +При глубине дерева > 10 метод молча отрезает хвост. Нет ни лога, ни исключения. + +**Рекомендация:** Либо увеличить лимит, либо бросать исключение при превышении. + +--- + +### 20. ThinBuilder::table_fields() ломается на типах без длины +**Файл:** `server/Fury/Modules/ThinBuilder/ThinBuilder.php:197-213` + +```php +list($type, $length) = explode('(', $raw_field[1]); +``` + +Для типов `INT`, `TEXT`, `DATETIME`, `BOOLEAN` в `SHOW COLUMNS` нет скобки, и `explode` возвращает массив из одного элемента. `list($type, $length)` присвоит `$length = null`, но `$length = intval($length)` даст `0` — логика выживет, но это хрупкое поведение. + +--- + +--- + +## 🟡 Средний / Информационный + +### 21. ThinBuilder::tables() только для MySQL +**Файл:** `server/Fury/Modules/ThinBuilder/ThinBuilder.php:215-222` + +`SHOW TABLES` — MySQL-специфичный синтаксис. Драйвер поддерживает SQLite (`create_connect`), но `tables()` с ним не работает. + +--- + +### 22. Несогласованные типы возврата ошибок в Models +- `Areas::create_new_area()` → `Area | null` +- `Devices::connect_new_device()` → `Device | Array` (массив с `err_alias`) + +Это заставляет вызывающий код делать `is_array()` + `is_object()` проверки вразнобой. + +**Рекомендация:** Ввести统一ный Result-объект или Exception-based flow. + +--- + +### 23. Area::alias_is_uniq() не исключает саму себя при обновлении +**Файл:** `server/SHServ/Models/Areas.php:27-34` + +При вызове для проверки уникальности во время **обновления** существующей ареа её собственный alias будет засчитан как дубль. Метод используется только в `create_new_area()`, но если его переиспользовать для update — баг. + +--- + +### 24. Area и AreaPlacing trait дублируют метод place_in_area_id +**Файл:** `server/SHServ/Entities/Area.php:68-71`, `server/SHServ/Entities/Traits/AreaPlacing.php:12-15` + +Оба определяют идентичный `place_in_area_id()`. В `Area` он имеет приоритет (класс побеждает trait). Для `Script` и `Device` используется trait-версия. Дублирование без причины. + +--- + +### 25. Creator::create_meta() подделывает update_at при конструировании +**Файл:** `server/SHServ/Factory/Creator.php:32-50` + +```php +return new Meta($meta_id, array_merge([ + "id" => $meta_id, + "update_at" => $data["create_at"] // фиктивное значение +], $data)); +``` + +Единственная причина — удовлетворить `Entity::update()`, который требует `update_at`. Это workaround вместо исправления корневой проблемы. + +--- + +### 26. Factory\Getter::get_profile_by() тянет только id, форсируя ленивую загрузку +**Файл:** `server/SHServ/Factory/Getter.php:25-39` + +В отличие от `get_user_by()` (который тянет все поля), `get_profile_by()` запрашивает только `id`, а затем создаёт `new Profile($result[0]["id"])` без данных. Это заставляет Entity при первом доступе к свойству делать `select_from_db()`. + +**Рекомендация:** Либо загружать все поля сразу, либо явно документировать ленивость. + +--- + +### 27. Model-конструктор вызывает devtools на каждую инстанциацию +**Файл:** `server/SHServ/Middleware/Model.php:8-13` + +```php +public function __construct(){ + parent::__construct(); + app() -> devtools -> using_model(get_class($this)); +} +``` + +`using_model()` вызывается при **каждом** `new Devices()`, `new Scripts()` и т.д. В production это лишний вызов метода на объекте-синглтоне. + +--- + +### 28. Area::remove() присваивает строку "0" вместо integer +**Файл:** `server/SHServ/Models/Areas.php:20` + +```php +"parent_id" => "0", +``` + +`parent_id` везде сравнивается как integer (`if(!$this->get_parent_id())`, `$this->parent_id` и т.д.). Строка `"0"` в PHP приводится к `0` в числовом контексте, но это неявное поведение. + +--- + +### 29. MetaWrap::$meta_manager_instance — статический mutable singleton +**Файл:** `server/SHServ/Helpers/MetaWrap.php:9` + +```php +protected static $meta_manager_instance; +``` + +Один инстанс `MetaManager` разделяется между всеми `MetaWrap`. Не критично, но затрудняет тестирование и многопоточность. + +--- + +### 30. ThinBuilder::query() — raw SQL без параметризации +**Файл:** `server/Fury/Modules/ThinBuilder/ThinBuilder.php:17-42` + +Метод принимает строку SQL и выполняет напрямую через `$pdo->query()`. В тестах используется для `CREATE TABLE`, но если где-то в production коде прокрадётся пользовательский input — SQL-инъекция. + +**Рекомендация:** Задокументировать как "internal/DDL only" или обернуть в internal-маркер. + +--- + +### 31. Нет DB-уровневых UNIQUE-индексов +Во всём слое Models уникальность проверяется через `count(...)` или `table_row_is_exists()` на уровне приложения. Нет гарантии от race condition на уровне БД для: +- `areas.alias` +- `devices.alias` +- `devices.device_hard_id` (active) +- `users.alias` / `users.email` +- `meta.(assignment, ent_id, name)` + +**Рекомендация:** Добавить `UNIQUE INDEX` в production-схему. + +--- + +### 32. Scripts::get_scripts_list() — линейный поиск вместо хеш-таблицы +**Файл:** `server/SHServ/Models/Scripts.php:170-184` + +```php +foreach($scripts as $alias => $script_data) { + $script_entity = array_values(array_filter($scripts_entities, function($item) use($alias) { + return $item -> uniq_name == $alias; + })); + $data[] = $this -> prepare_script_to_view($type, $script_data, $script_entity[0] ?? null); +} +``` + +Для N скриптов и M entity сложность O(N·M). При 50 скриптах и 50 entity — 2500 сравнений вместо 50 хеш-поисков. + +--- + +### 33. Area::remove() напрямую обращается к app()->thin_builder вместо $this->thin_builder() +**Файл:** `server/SHServ/Entities/Area.php:268-278` + +```php +app() -> thin_builder -> update(...); +``` + +В то время как внутри того же класса используется `$this -> thin_builder() -> select(...)`. Функционально эквивалентно, но нарушает единообразие. + +--- + +### 34. Несоответствие тестовой и production схемы device_auth +**Файл:** `server/tests/DevicesModelTransactionTest.php:44-52` + +Тесты создают `device_auth` с `update_at TEXT`, но `DeviceAuth::$fields` не содержит эту колонку. Это означает, что: +- Тесты проходят +- Production может отличаться +- Нет единого источника правды для схемы + +**Рекомендация:** Создать единую миграционную систему или хотя бы файл `schema.sql` в репозитории. + +--- + +## Рекомендуемые приоритеты исправления + +| Приоритет | Находка | Файл | +|---|---|---| +| 🔴 P0 | Entity::select_from_db() падает на отсутствующей строке | `Middleware/Entity.php` | +| 🔴 P0 | DeviceAuth::kill() + отсутствие update_at | `Entities/DeviceAuth.php`, `Middleware/Entity.php` | +| 🔴 P0 | Device::remove() не инвалидирует токен | `Entities/Device.php` | +| 🔴 P0 | Area::remove() без транзакции | `Entities/Area.php` | +| 🔴 P0 | IN () на пустом массиве | `Models/Scripts.php` | +| 🟠 P1 | ThinBuilder whitelist операторов | `Fury/Modules/ThinBuilder/ThinBuilderProcessing.php` | +| 🟠 P1 | validate_identifier числовой старт | `Fury/Modules/ThinBuilder/ThinBuilderProcessing.php` | +| 🟠 P1 | MetaManager race condition | `Models/MetaManager.php` | +| 🟠 P1 | User жадно тянет профиль | `Entities/User.php` | +| 🟠 P1 | Entity::update() возвращает true всегда | `Middleware/Entity.php` | +| 🟠 P1 | Scripts::remove_scope() файловая/БД рассинхрон | `Models/Scripts.php` | +| 🟠 P1 | IN с не-массивом на PHP 8+ | `Fury/Modules/ThinBuilder/ThinBuilderProcessing.php` | +| 🟠 P1 | Короткий where и IN | `Fury/Modules/ThinBuilder/ThinBuilderProcessing.php` | +| 🟡 P2 | DISTINCT вместо PHP-dedup | `Models/Areas.php` | +| 🟡 P2 | O(n·m) в Scripts::get_scripts_list() | `Models/Scripts.php` | +| 🟡 P2 | Удалить escape_string_in_arr() | `Fury/Modules/ThinBuilder/ThinBuilderProcessing.php` | +| 🟡 P2 | Единая схема БД в репозитории | новый файл `schema.sql` | + +--- + +*Продолжение: Фаза 3 (Controllers, Routing, Validation)* diff --git a/server/docs/review-phase-3-controllers-routing-validation.md b/server/docs/review-phase-3-controllers-routing-validation.md new file mode 100644 index 0000000..65512f0 --- /dev/null +++ b/server/docs/review-phase-3-controllers-routing-validation.md @@ -0,0 +1,477 @@ +# Phase 3 Review: Controllers, Routing, Validation + +**Scope:** Fury Router, Route definitions, REST API Controllers, Auth guards, Input validation, Request lifecycle. +**Date:** 2026-06-02 +**Files reviewed:** +- `server/SHServ/App.php` — app bootstrap, auth guard +- `server/SHServ/Routes.php` — route aggregator +- `server/SHServ/Routes/{Devices,Areas,Scripts}RESTAPI_v1.php` — route traits +- `server/SHServ/Routes/DevMode.php` — dev routes +- `server/SHServ/Controllers/*.php` — all controllers +- `server/SHServ/Middleware/Controller.php` — base controller +- `server/SHServ/Utils.php` — response helpers +- `server/SHServ/Sessions.php` — session / auth +- `server/SHServ/EventsHandlers.php` — request lifecycle hooks +- `server/SHServ/Tools/RateLimiter.php` +- `server/Fury/Modules/Router/{Router,RouterImplementation,RouterInterface}.php` +- `server/Fury/Kernel/{CallControl,Controller,BaseApp}.php` +- `server/index.php` — entry point + +--- + +## Summary + +| Severity | Count | Categories | +|----------|-------|------------| +| Critical | 7 | Routing multi-match, unprotected cron, method spoofing, timing attack, file write, rate limit bypass, devmode leakage | +| High | 14 | Inconsistent validation, missing RBAC, source disclosure, dead auth code, no CORS, missing script state check, alias update bug, no input length limits, brute-force, no audit log, exit() in guard, destructive ops without confirmation, JSON body unbounded, route param coercion | +| Medium / Info | 16 | Reflection perf, no route caching, total mismatch, business logic in controller, N+1 in lists, no method enforcement, weak email validation, schema drift, singleton state, response code僵化, partial failures ignored, recon endpoint, unbounded event data, auth token dual channel, dead code, missing HTTP method verbs | + +--- + +## Critical Findings + +### C1. Router calls ALL matching routes — multiple controller execution +**File:** `server/Fury/Modules/Router/RouterImplementation.php` +**Lines:** 61-91 (`GET_and_POST_routing`), 146-163 (`URI_routing`) + +Both `GET_and_POST_routing()` and `URI_routing()` iterate the entire route map and invoke `call_action()` for **every** template that matches. There is no `break` after the first match. If two URI templates overlap (e.g. `/api/v1/foo/$id` and `/api/v1/foo/$id/bar` is prevented by part count, but exact duplicates or near-duplicates are possible), **both controllers execute**. + +In URI routing, `searching_route_by_uri()` returns an array of matching templates, and `URI_routing()` calls `call_action()` for **all** of them. This means a single HTTP request can trigger multiple controller methods, causing side effects (DB writes, device reboots) multiple times. + +**Impact:** Unintended multi-execution, data corruption, duplicate device commands. +**Status:** ⚠️ **By design / intentional.** Author confirms this is expected behavior — the framework is designed to allow multiple matching routes. +**Recommendation:** If this is maintained as a feature, document it explicitly and ensure route authors understand the side-effect risk. Otherwise, add `break` after the first match. + +--- + +### C2. Cron endpoints are publicly accessible without authentication +**File:** `server/SHServ/Routes.php:61-62` + +```php +$this -> router -> uri("/cron/regular-scripts", "...CronController@run_regular_cron_scripts"); +$this -> router -> uri("/cron/status-update-scanning", "...CronController@status_update_scanning"); +``` + +These are `uri` routes, not under `/api/v1/`, so `App::api_auth_guard()` skips them. Anyone with network access can trigger: +- Execution of all regular automation scripts +- Full network scan + device ping storm + +**Impact:** DoS, resource exhaustion, unintended script execution. +**Status:** ⚠️ **Known issue / deferred.** Consciously accepted for now; must be addressed in the future. +**Fix:** Move cron routes under a secret token check (e.g. `?cron_token=...`) or restrict to CLI (`console_flag`). + +--- + +### C3. POST routes respond to GET requests (method spoofing) +**File:** `server/Fury/Modules/Router/RouterImplementation.php:61-91` + +`GET_and_POST_routing()` only checks whether required variables exist in `$_GET` or `$_POST`. It does **not** inspect `$_SERVER['REQUEST_METHOD']`. A GET request to `/api/v1/devices/action?device_id=1&action=on¶ms={}` will match the POST route and execute `do_device_action()`. + +This means every destructive POST endpoint (device reset, remove, action, alias update, scope update, area creation, etc.) is also reachable via GET, including from `` tags and malicious links. + +**Impact:** CSRF-like attacks without needing XSS; destructive operations via simple links. +**Fix:** Router must check `$_SERVER['REQUEST_METHOD']` against the route registration type. + +--- + +### C4. URI routes ignore HTTP method entirely +**File:** `server/Fury/Modules/Router/RouterImplementation.php:146-163` + +All `uri()` routes (`/api/v1/devices/id/$device_id/remove`, `/api/v1/areas/id/$area_id/remove`, etc.) respond identically to GET, POST, PUT, DELETE, PATCH, OPTIONS. There is no method discrimination. + +**Impact:** REST semantics violated; cache invalidation unpredictable; accidental destructive calls via GET/crawler prefetch. +**Status:** ⚠️ **Intentional simplification.** Author confirms this was a deliberate design choice; may be improved later. +**Fix:** Add HTTP method binding to `uri()` or introduce `get_uri()` / `post_uri()` / `delete_uri()` wrappers. + +--- + +### C5. `/events/new` timing attack — fast path leaks valid device IDs +**File:** `server/SHServ/Controllers/EventsController.php:17-58` + +The controller uses an "early response" pattern (flush + `fastcgi_finish_request()`) **only** when the device exists and auth is valid. If `by_hard_id()` returns null or `auth()->is_active()` is false, it returns a normal JSON error through the full stack. + +An attacker can measure response time: +- Valid `device_id` → ~5-20 ms (fast flush) +- Invalid `device_id` → ~50-200 ms (full stack error) + +This allows enumeration of all registered `device_hard_id` values. + +**Impact:** Information disclosure; device inventory enumeration. +**Fix:** Always use the fast path: validate device, then immediately return 200, then process event logic (or drop invalid events silently). + +--- + +### C6. `scope_update` writes PHP source without atomicity or backup +**File:** `server/SHServ/Controllers/ScriptsRESTAPIController.php:97-122` + +```php +$result = file_put_contents($filepath, $file); +``` + +User-supplied content overwrites a PHP class file directly. If the request is interrupted mid-write, the file is truncated and PHP will fatal-error on the next request trying to autoload that scope class. No backup is created. No file size limit. No syntax validation. + +**Impact:** Corrupted scope class → fatal error → complete automation system failure. Potential code execution if PHP tags are injected (though `realpath` + allowed_dir mitigates path traversal, the content itself is unfiltered PHP). +**Fix:** +1. Write to a temp file, `require` it to verify syntax (`php -l` or `token_get_all`), then `rename()` atomically. +2. Keep a `.bak` backup before overwrite. +3. Validate file size (e.g. max 64KB). + +--- + +### C7. RateLimiter is per-process (bypassable via FPM workers) +**File:** `server/SHServ/Tools/RateLimiter.php` + +`RateLimiter` stores state in a `static array` inside the PHP process. Under PHP-FPM, each worker process has its own memory space. With 8 FPM workers, an attacker gets 8× the allowed rate (480 req/min instead of 60). Restarting workers resets counters. + +**Impact:** Rate limiting is ineffective in real production deployments. +**Fix:** Use APCu, Redis, Memcached, or database-backed rate limiter. + +--- + +## High Findings + +### H1. Auth routes are dead code (commented out) +**File:** `server/SHServ/Routes.php:71-82`, `server/SHServ/Controllers/Example_AuthController.php` + +The `AuthController` exists but all auth routes (`/auth/signup`, `/auth/signin`) are commented out in `Routes.php`. The file is literally named `Example_AuthController.php` (likely a leftover). Yet `Sessions.php` implements full cookie-based auth and `check_api_auth()` validates tokens. + +**Impact:** Confusion — where does the Vue client authenticate? If auth is handled elsewhere (legacy, external), this is unmaintained code. If auth is missing, the system has no user management. +**Fix:** Either restore and register auth routes, or remove the dead controller. + +--- + +### H2. No Role-Based Access Control (RBAC) +**File:** All controllers + +Every authenticated user has identical permissions. There is no concept of roles (admin, viewer, device_manager). Any authenticated user can: +- Scan the entire network (`scanning__all`) +- Delete any device (`remove_device`) +- Reset any device (`reset_device`) +- Reboot any area's devices (`reboot_devices`) +- Overwrite scope source code (`scope_update`) +- Create/remove areas + +**Impact:** Privilege escalation by compromised low-privilege account; insider threats. +**Fix:** Add a `role` column to users table and middleware-level permission checks. + +--- + +### H3. `scope_file` discloses full PHP source code +**File:** `server/SHServ/Controllers/ScriptsRESTAPIController.php:75-95` + +`scope_file($name)` reads the Scope class file from disk and returns the raw PHP source in JSON. This leaks: +- Internal logic and algorithms +- Hardcoded IP addresses or credentials (if any exist in scope code) +- File system path structure + +**Impact:** Information disclosure; aids further attacks. +**Fix:** Return only metadata (name, description, author), not source code. Source editing should be restricted to a separate admin endpoint. + +--- + +### H4. `run_action_script` does not check script state +**File:** `server/SHServ/Controllers/ScriptsRESTAPIController.php:19-45` + +The TODO comment at the top of the file explicitly says disabled scripts should not run, but `run_action_script()` calls `ControlScripts::run_action_script()` without checking `script_state()`. The underlying `ControlScripts::run_action_script()` **does** check state, so this is actually safe at the model layer. However, the controller returns `"action_script_not_found"` if the alias doesn't exist, but if the script exists and is disabled, the model returns `null` which the controller also maps to `"action_script_not_found"` — the error message is misleading (should be `"action_script_disabled"`). + +**Impact:** Confusing error messages; poor UX; potential bypass if model check is ever removed. +**Fix:** Controller should explicitly check `script_state()` before calling `run_action_script()` and return a clear disabled error. + +--- + +### H5. `update_alias` fails when alias is unchanged +**File:** `server/SHServ/Controllers/DevicesRESTAPIController.php:359-386`, `server/SHServ/Controllers/AreasRESTAPIController.php:172-201` + +Both device and area `update_alias` call `alias_is_uniq($new_alias)` without passing the current entity ID. If the user submits the same alias, `alias_is_uniq()` finds the existing row and returns `false`, causing a false-positive "already exists" error. + +**Impact:** Users cannot "save without changes" on alias fields. +**Fix:** `alias_is_uniq()` should accept an optional `$exclude_id` parameter. + +--- + +### H6. No input length limits on free-text fields +**File:** Multiple controllers + +- `new_area`: `type`, `display_name` — only checked `!strlen()`, no max length +- `update_name`: `name` — no max length +- `update_description`: `description` — no max length +- `setup_new_device`: `name`, `description` — no max length +- `update_display_name` — no max length + +**Impact:** Potential DoS via multi-megabyte strings causing memory pressure or DB bloat. +**Fix:** Add `strlen($field) <= 255` (or appropriate limit) consistently. + +--- + +### H7. No brute-force protection on authentication +**File:** `server/SHServ/Controllers/Example_AuthController.php:75-107` + +The signin endpoint (if it were active) has no account lockout, no CAPTCHA, no progressive delay, and no rate limiting per-account. `RateLimiter` is only applied to `/api/v1/*` routes. + +**Impact:** Credential stuffing and dictionary attacks against user passwords. +**Fix:** Add per-IP and per-account rate limiting on auth endpoints. Log failed attempts. + +--- + +### H8. No CORS headers on API +**File:** `server/SHServ/Utils.php` + +`response_success()` and `response_error()` only set `Content-Type: application/json`. There are no `Access-Control-Allow-Origin`, `Access-Control-Allow-Methods`, or `Access-Control-Allow-Headers` headers. In production, if the Vue web client is served from a different domain (or port), browsers will block API calls. + +**Impact:** CORS errors in production; developers may resort to unsafe workarounds (`*`, credentials wildcard). +**Fix:** Add configurable CORS middleware. + +--- + +### H9. `api_auth_guard()` calls `exit` — hard termination +**File:** `server/SHServ/App.php:110-120` + +When auth fails, the guard outputs JSON and calls `exit`. This prevents: +- Custom shutdown handlers / logging flush +- PHPUnit test continuation (tests must define `PHPUNIT_TEST` to skip `App` construction) +- Graceful cleanup (e.g. closing DB connections, releasing locks) + +**Impact:** brittle testing, skipped logging, potential resource leaks. +**Fix:** Throw an exception and handle it in a top-level try/catch, or return a response object and short-circuit routing without `exit`. + +--- + +### H10. Destructive operations lack confirmation / soft-delete +**File:** Controllers + +`remove_device`, `remove_area`, `reset_device`, `scope_remove`, `unassign_from_area` — all execute immediately without: +- A "confirm" parameter +- Soft-delete / trash pattern +- Cascade warnings (e.g. "3 devices will be orphaned") + +**Impact:** Accidental irreversible data loss via UI misclick or API client bug. +**Fix:** Add `?confirm=true` requirement for destructive endpoints, or implement a 24h soft-delete grace period. + +--- + +### H11. Unbounded JSON body size +**File:** `server/SHServ/App.php:126-143` + +`router_json_to_post_emulate()` reads `php://input` without `Content-Length` validation. A malicious client can POST a 100MB JSON body, causing `json_decode()` to exhaust memory. + +**Impact:** Memory exhaustion DoS. +**Fix:** Reject requests with `Content-Length > MAX_BODY_SIZE` (e.g. 1MB) before reading the body. + +--- + +### H12. `/events/new` has no rate limiting +**File:** `server/SHServ/App.php:65-108` + +`check_api_auth()` applies rate limiting only to URIs starting with `/api/v1/`. `/events/new` is outside this prefix, so devices can flood events without any throttling. + +**Impact:** Event queue flooding; DoS via rapid event posting. +**Fix:** Apply rate limiting to `/events/new` with a higher but still bounded threshold (e.g. 600 req/min per device). + +--- + +### H13. `EventsController` passes raw `$data` to event handlers +**File:** `server/SHServ/Controllers/EventsController.php:50-57` + +```php +$events_model -> channel_alias_device_event_call($device, $event_name, intval($data["channel"]), $data); +``` + +`$data` is passed directly from the device without schema validation. A rogue/compromised device could send malformed nested arrays, extremely large strings, or unexpected keys that downstream event handlers process unsafely. + +**Impact:** Type confusion, injection into event handlers, potential crashes in scope code. +**Fix:** Define a JSON schema per event type and validate/filter `$data` before dispatching. + +--- + +### H14. `scanning__all` exposes network topology +**File:** `server/SHServ/Controllers/DevicesRESTAPIController.php:20-27` + +Any authenticated user can trigger a full subnet scan (`scanning_localnet`). The response includes all devices found on the network, their IP addresses, firmware versions, and MAC addresses. + +**Impact:** Network reconnaissance; attacker learns device inventory and firmware versions for targeted exploits. +**Fix:** Restrict scanning endpoints to admin role, or cache results and return cached data instead of live-scanning. + +--- + +## Medium / Informational Findings + +### M1. `CallControl` uses reflection on every request +**File:** `server/Fury/Kernel/CallControl.php:148-165` + +For every controller method call, `call_for_class_meth()` instantiates `ReflectionClass` and `ReflectionMethod` to map route parameters. This is `O(n)` reflection overhead on every single request. + +**Fix:** Cache reflection metadata in a static array keyed by `action_class@method`. + +--- + +### M2. No route caching +**File:** `server/SHServ/Routes.php` + +All routes are defined via PHP trait method calls on every request. With ~50 routes this is negligible, but as the API grows, route compilation overhead increases. + +**Fix:** Optional: generate a cached route map array file at deploy time. + +--- + +### M3. `areas_list` returns mismatched `total` +**File:** `server/SHServ/Controllers/AreasRESTAPIController.php:260-292` + +`scripts_list()` returns `total = count($scripts)` (raw DB count) but the actual `scripts` array is filtered to only those whose class still exists. If a script DB record exists but the PHP scope class was deleted, `total` will be higher than the returned array length. + +**Fix:** Count the filtered array for `total`. + +--- + +### M4. Business logic in controller (`device_status` updates entity) +**File:** `server/SHServ/Controllers/DevicesRESTAPIController.php:169-204` + +The controller directly modifies `$device->connection_status` and `$device->last_contact`, then calls `$device->update()`. This is business/state-management logic that should live in the Model or a dedicated service. + +**Fix:** Move status-update logic to `Devices` model or `Device` entity method. + +--- + +### M5. `AuthController` has weak email validation +**File:** `server/SHServ/Controllers/Example_AuthController.php:44-48` + +```php +if(strlen($email) < 4 or !strpos($email, "@") or !strpos($email, ".")) { +``` + +This accepts `a@b.c`, `test@com.`, `@example.com`, etc. `strip_tags()` on an email is nonsensical. No `FILTER_VALIDATE_EMAIL`. + +**Fix:** Use `filter_var($email, FILTER_VALIDATE_EMAIL)`. + +--- + +### M6. `DevicesRESTAPIController::devices_list` cannot filter by status +**File:** `server/SHServ/Controllers/DevicesRESTAPIController.php:296-311` + +The route is `/api/v1/devices/list` with no `$status` parameter. The method signature has `$status = "active"`, so it is always "active". There is no API way to list removed or lost devices. + +**Fix:** Add `$status` URI parameter or query parameter. + +--- + +### M7. `validate_positive_int_ids` has weak type semantics +**File:** `server/SHServ/Middleware/Controller.php:26-33` + +```php +if ($value != intval($value) || intval($value) < 1) { +``` + +`"123.0" != 123` is `false` in PHP (weak comparison), so `"123.0"` passes. `"123abc" != 123` is `true`, correctly rejected. But `"0x1A"` (hex string) equals `26` and passes. + +**Fix:** Use strict comparison `!==` or `ctype_digit()` / `filter_var($value, FILTER_VALIDATE_INT)`. + +--- + +### M8. `action_result` assumes string return +**File:** `server/Fury/Kernel/CallControl.php:217-221` + +```php +protected function action_result($result){ + echo $result; +} +``` + +If a controller returns an array or object, PHP will emit a notice/warning and output garbage. All current controllers use `Utils::response_*` which return strings, but anonymous or simple function routes are unprotected. + +**Fix:** Check `is_string($result)` or explicitly `json_encode()` / serialize. + +--- + +### M9. `Utils::response_error` always returns HTTP 400 +**File:** `server/SHServ/Utils.php:14-23` + +All errors are 400 Bad Request. A missing device (`device_not_found`) should be 404. A duplicate alias should be 409. Server errors should be 500. + +**Fix:** Allow controllers to pass a status code, or map `error_alias` to status codes centrally. + +--- + +### M10. `reboot_devices` ignores partial failures +**File:** `server/SHServ/Controllers/AreasRESTAPIController.php:209-227` + +Reboots devices sequentially and collects results. If one device fails, others continue. There is no rollback or notification that some failed. + +**Fix:** Return per-device success/failure status in `results` and a separate `failed_count`. + +--- + +### M11. `DevicesRESTAPIController::do_device_action` does not validate action params schema +**File:** `server/SHServ/Controllers/DevicesRESTAPIController.php:206-252` + +The `params` array is passed straight to the device API without schema validation per action. A malformed `params` array might crash the device firmware or cause unexpected behavior. + +**Fix:** Each device type / action should declare an expected parameter schema, validated server-side before forwarding. + +--- + +### M12. `DevMode` trait contains model-exposing routes +**File:** `server/SHServ/Routes/DevMode.php` + +Routes like `/dev/test/model/devices/remove_device.html` directly call `remove_device(6)` with hardcoded ID 6 and `var_dump()` the result. If `FCONF["devmode"]` is accidentally true in production, sensitive internal data is exposed. + +**Fix:** Ensure `devmode` can only be enabled when `console_flag` is true, or bind dev routes to localhost IP. + +--- + +### M13. No audit / access log +**File:** All controllers + +There is no persistent log of who called what API endpoint, when, and with what result. Security incident response is impossible. + +**Fix:** Add an `api_access_log` table (timestamp, user_id, ip, endpoint, method, status, device_id if applicable). + +--- + +### M14. `Example_AuthController` file name vs class name mismatch +**File:** `server/SHServ/Controllers/Example_AuthController.php` + +The file is `Example_AuthController.php` but the class inside is `AuthController`. The route references `AuthController`, but since routes are commented out, this is dead code. Still, the mismatch is confusing and might break autoloaders if routes are ever uncommented. + +**Fix:** Rename file to match class, or delete if obsolete. + +--- + +### M15. Singleton controllers retain state within a request +**File:** `server/Fury/Kernel/CallControl.php:150` + +```php +$class_object = call_user_func_array([$action_class, 'ins'], [$this -> bootstrap]); +``` + +Controllers are singletons (via `Fury\Libs\Singleton::ins()`). If the router multi-match bug (C1) triggers, the same controller instance is reused, potentially retaining state set by the first invocation. + +**Fix:** Don't use singletons for request-scoped controllers, or reset state in `__construct` / before each action. + +--- + +### M16. `ControlScripts` constructor does reflection to get path +**File:** `server/SHServ/Middleware/ControlScripts.php:24`, `65`, `101` + +```php +$ref = new \ReflectionClass(static::class); +$path_info = pathinfo($ref -> getFileName()); +``` + +Reflection is used in every scope's constructor (potentially dozens of scopes) to determine the file path for metadata. This is unnecessary — the path can be derived from `__FILE__` or `__DIR__` without reflection. + +**Fix:** Use `__FILE__` instead of `ReflectionClass`. + +--- + +## Recommendations (Priority Order) + +1. **Fix router multi-match** (C1) — add `break` after first match in both GET/POST and URI routing. +2. **Protect cron endpoints** (C2) — require a secret token or CLI-only execution. +3. **Enforce HTTP methods** (C3, C4) — router must check `$_SERVER['REQUEST_METHOD']`. +4. **Fix `/events/new` timing attack** (C5) — always fast-flush, process asynchronously. +5. **Atomic scope file write** (C6) — temp file + syntax check + `rename()`. +6. **Replace in-memory rate limiter** (C7) — use APCu/Redis/shared storage. +7. **Add RBAC** (H2) — minimum `admin` vs `user` roles; protect destructive and scanning endpoints. +8. **Unify input validation** — create a `Validator` helper with schema definitions (type, min/max length, regex, required) and use it in every controller. +9. **Add CORS** (H8) and **audit logging** (M13). +10. **Remove dead auth code** (H1, M14) or restore active auth routes. diff --git a/server/docs/review-phase-4-automation-scripts-infrastructure.md b/server/docs/review-phase-4-automation-scripts-infrastructure.md new file mode 100644 index 0000000..0e0f210 --- /dev/null +++ b/server/docs/review-phase-4-automation-scripts-infrastructure.md @@ -0,0 +1,390 @@ +# Phase 4 Review: Automation, Scripts, Infrastructure + +**Date:** 2026-06-02 +**Scope:** ControlScripts layer, event routing, cron/background execution, infrastructure (config, logging, DB, CLI), Fury framework services. + +--- + +## Critical (C) + +### C1. Static script registries leak state across requests +**File:** `server/SHServ/Middleware/ControlScripts.php` +**Lines:** 12–13 +```php +protected static $regular_scripts = []; +protected static $actions_scripts = []; +``` +`static` arrays accumulate scripts across all instantiations. In PHP-FPM, statics persist per worker process. Adding/removing scopes or reloading code does not clear the arrays. Tests (`ScriptsModelStateTest`) use reflection to reset, but production has no cleanup mechanism. If a scope file is deleted, its scripts remain in static memory until worker restart. + +**Impact:** Stale script entries, phantom actions, memory growth, unpredictable behavior after deploys. + +**Fix:** Flush statics at the start of `control_scripts_init()`, or use instance-level storage instead of static. + +--- + +### C2. No atomic write / backup for scope file updates +**File:** `server/SHServ/Controllers/ScriptsRESTAPIController.php` +**Method:** `scope_update` +`file_put_contents($filepath, $file)` writes directly over the live PHP class file. If the request is interrupted mid-write, the file is truncated/corrupted. PHP will fail to parse it on the next request, breaking the entire scope and potentially the application. + +**Impact:** Broken scope class → fatal parse error → 500 for all subsequent requests using that scope. + +**Fix:** Write to a temp file, `rename()` atomically, keep the previous version as `.bak`. + +--- + +### C3. Events post-flush handlers can hang indefinitely +**File:** `server/SHServ/Controllers/EventsController.php` +**Method:** `new_event` +After `fastcgi_finish_request()`, event handlers are dispatched. Handlers like `btn_on_online` → `sync_btn_channels` make synchronous HTTP calls to devices via `Button/Relay API`. `set_time_limit(10)` applies to the whole block, but with 5 event dispatches and multiple device calls, 10 seconds may not be enough. If a device is slow or unresponsive, the PHP process is held busy. + +**Impact:** PHP-FPM worker exhaustion, delayed response for other requests, cron/scanning blocked. + +**Fix:** Enforce a strict per-handler timeout (e.g. 2s), run handlers asynchronously via a queue or background job, or use non-blocking HTTP. + +--- + +### C4. RCE via `scope_update` (reinforced) +**File:** `server/SHServ/Controllers/ScriptsRESTAPIController.php` +Any authenticated user can overwrite a scope file with arbitrary PHP. The file is immediately included on the next request via `control_scripts_init()`. Combined with no RBAC, this is trivial remote code execution. + +**Impact:** Full server compromise. + +**Fix:** Sandbox scope code (e.g. restricted DSL), validate with `php -l`, disable live editing in production, require admin role. + +--- + +### C5. `sync-map.json` loaded from disk on every scope init, no caching +**File:** `server/ControlScripts/Common.php` +**Method:** `register_global_device_sync_map` +Each scope that `use`s `Common` re-reads `sync-map.json` via `file_get_contents`. With 4 scopes, the file is read 4 times per request (~9 KB each). No opcode cache or in-memory cache. + +**Impact:** Unnecessary disk I/O, parse overhead on every request. + +**Fix:** Load once in `App::control_scripts_init()` and pass to scopes, or cache decoded array in a static/shared variable. + +--- + +### C6. Cron script execution stops on first failure +**File:** `server/SHServ/Controllers/CronController.php` +**Method:** `run_regular_cron_scripts` +```php +foreach($regular_scripts as $alias => $script) { + // ... + $script["script"](); +} +``` +No `try/catch`. If one regular script throws, the exception bubbles up and the remaining scripts in the loop never execute. + +**Impact:** Partial automation — some scheduled tasks silently skipped. + +**Fix:** Wrap each script invocation in `try/catch`, log the error, continue the loop. + +--- + +## High (H) + +### H1. Action scripts have no error handling +**File:** `server/SHServ/Middleware/ControlScripts.php` +**Method:** `run_action_script` +```php +$result = self::$actions_scripts[$alias]["script"]($params); +``` +No `try/catch`. An exception in an action script kills the API request with a 500 and no meaningful response to the client. + +**Fix:** Wrap in `try/catch`, return structured error response. + +--- + +### H2. `get_source_code()` re-reads the source file on every script registration +**File:** `server/SHServ/Middleware/ControlScripts.php` +**Method:** `get_source_code` +Uses `file()` + `array_slice` to extract closure source code. Called once per `add_action_script` / `add_regular_script`. For 20 scripts, the file is read 20 times. + +**Impact:** Redundant disk I/O during every request bootstrap. + +**Fix:** Cache file contents per class, or use `ReflectionFunction::getClosureUsedVariables` + store source once at build time. + +--- + +### H3. `control_scripts_init()` instantiates all scopes eagerly +**File:** `server/SHServ/App.php` +**Method:** `control_scripts_init` +All `.php` files in `ControlScripts/Scopes/` are scanned, included, and instantiated on every request. There is no lazy loading — even disabled scopes run their constructors and all four `register_*` methods (though `register_actions_scripts` / `register_regular_scripts` are gated by DB state, the constructor still runs). + +**Impact:** Slower bootstrap, unnecessary DB queries, memory overhead. + +**Fix:** Lazy-load scopes when their scripts are actually needed, or cache registration metadata. + +--- + +### H4. No health check / status for cron endpoints +**File:** `server/SHServ/Controllers/CronController.php` +Both `run_regular_cron_scripts()` and `status_update_scanning()` return nothing (implicit `null`). The HTTP response body is empty, HTTP status is 200 by default. There is no way to tell from the response whether scripts ran, how many succeeded, or if any failed. + +**Impact:** Blind monitoring. External cron scheduler (e.g. systemd timer, crontab) cannot detect failures. + +**Fix:** Return structured JSON with execution summary, error count, duration. + +--- + +### H5. Logging writes to JSON without rotation or size limits +**File:** `server/Fury/Kernel/Logging.php` +**Method:** `dump` +Appends session logs to `Logs/d.m.Y.log.json`. File grows indefinitely. No max size, no rotation, no cleanup. On high-traffic days the file can become several GB. + +**Impact:** Disk exhaustion, performance degradation on log write (lock contention, JSON re-encode of huge array). + +**Fix:** Rotate by size (e.g. 10 MB), archive old logs, or switch to structured line-based logging. + +--- + +### H6. No structured logging / severity / correlation IDs +**File:** `server/Fury/Kernel/Logging.php` +`logging()->set($place, $title, $message)` accepts only three strings. No severity levels, no request ID, no timestamps per entry (only per session), no structured fields. + +**Impact:** Hard to debug production issues, impossible to trace a single request through controllers, models, and device calls. + +**Fix:** Add severity, request/correlation ID, timestamp per log line, optional context array. + +--- + +### H7. ErrorHandler may leak debug info if `debug` flag is on +**File:** `server/Fury/Modules/ErrorHandler/ErrorHandler.php` +**Method:** `view_fatal_error` +If `FCONF["debug"]` is `true`, the handler renders full file paths, source code excerpts, and stack traces as HTML. The `debug` flag is controlled by `.env` `DEBUG`. With no `.env` file, it defaults to `false`, but a misconfiguration or accidental `.env` edit exposes internals. + +**Impact:** Information disclosure — file paths, code snippets, server internals. + +**Fix:** In production mode, never render HTML errors; always log and return generic "Internal Server Error" JSON. + +--- + +### H8. No DB schema / migrations +**File:** project-wide +There are no `.sql` migration files, no schema versioning, no install script. The `scripts` table definition only exists inside `tests/ScriptsModelStateTest.php` as an inline `CREATE TABLE`. Production database setup is manual and undocumented. + +**Impact:** Impossible to recreate production DB, risky deploys, schema drift between environments. + +**Fix:** Create `server/migrations/` with numbered SQL files and a CLI runner. + +--- + +### H9. `.env` absent — fallbacks use insecure defaults +**File:** `server/SHServ/config.php` +If `.env` is missing, DB credentials default to `user => "root"`, `password => ""`, `host => "localhost"`. Device IP scan range defaults to `192.168.2.2–254`. + +**Impact:** Accidental connection to wrong/weak DB, scan of wrong subnet. + +**Fix:** Fail hard if `.env` is missing in production; do not provide insecure fallbacks. + +--- + +### H10. `ControlScripts` constructor performs DB check per scope +**File:** `server/SHServ/Middleware/ControlScripts.php` +**Line:** 26 +```php +if($scope_folder != "ControlScripts" or (new Scripts()) -> script_state("scope", $scope_name)) { +``` +For every non-ControlScripts scope, a new `Scripts` model is instantiated and a DB query is issued to check if the scope is enabled. With N scopes, N DB round-trips on every request. + +**Impact:** N+1 query problem during every bootstrap. + +**Fix:** Batch-fetch all scope states in a single query in `App::control_scripts_init()`. + +--- + +### H11. `sync_map()` returns mutable reference to shared static state +**File:** `server/SHServ/Middleware/ControlScripts.php` +**Method:** `sync_map` +Returns `self::$sync_map_storage`. Callers (scopes) can mutate the shared array, affecting other scopes. + +**Impact:** Cross-scope state pollution, hard-to-debug side effects. + +**Fix:** Return a deep copy, or make `sync_map_storage` immutable after init. + +--- + +### H12. `DeviceScanner` silently discards scan failures +**File:** `server/SHServ/Tools/DeviceScanner.php` +Failed IPs are skipped without logging. There is no way to know which devices were expected but not found, or whether a device disappeared from the network. + +**Impact:** Silent failures during device discovery, no audit trail for network changes. + +**Fix:** Log scan results — found, lost, unreachable IPs. + +--- + +### H13. `text-msgs.php` has empty strings for SHServ errors +**File:** `server/SHServ/text-msgs.php` +Lines 30–48: `device_not_found`, `unknown_device`, `error_of_device_auth`, `device_request_fail`, `db_error`, `action_script_not_found`, `scope_not_found`, etc. all have empty string values. + +**Impact:** API returns empty `msg` fields for many errors, poor UX in clients. + +**Fix:** Fill in meaningful Russian/English messages for all SHServ aliases. + +--- + +### H14. `RequiredControlScriptsScope` mixes business logic with event handling +**File:** `server/SHServ/RequiredControlScriptsScope.php` +The `online` event handler directly mutates device entity state (`$device -> device_ip = ...; $device -> update()`). This is business logic that belongs in a service or model method, not in an event handler. + +**Impact:** Business rules scattered across event handlers, hard to unit-test, duplication risk. + +**Fix:** Move device online/offline state transition to `Devices` model or a dedicated service. + +--- + +## Medium (M) + +### M1. `EventsHandlers` devmode clutter +**File:** `server/SHServ/EventsHandlers.php` +All devmode handlers are registered inside an `if(FCONF["devmode"])` block at runtime. The code paths are still present and evaluated on every request. + +**Fix:** Use conditional class loading or separate dev-only bootstrap file. + +--- + +### M2. `console.php` has only one command (`get.config`) +**File:** `server/console.php` +The CLI entry point only supports dumping config (with credentials stripped). No commands for: run migrations, run cron manually, flush caches, health check, reset rate limiter, re-scan devices. + +**Fix:** Add a command registry with useful ops commands. + +--- + +### M3. `sync-map.json` has no schema validation +**File:** `server/ControlScripts/sync-map.json` +Loaded as raw JSON without validation. No checks that aliases correspond to real devices, that channel indices are within device schema, or that the graph is acyclic. + +**Impact:** Invalid sync map causes silent failures (missing device → `continue` in helper) or wrong channel indices sent to devices. + +**Fix:** Add a JSON Schema and a validation step at bootstrap (or at build time). + +--- + +### M4. `ControlScriptsInterface` lacks return type declarations +**File:** `server/SHServ/Implements/ControlScriptsInterface.php` +Interface methods have no return types, while implementations use `: void`. This inconsistency weakens type safety. + +**Fix:** Add `void` return type to interface methods. + +--- + +### M5. No observability metrics for automation layer +Project-wide: no counters for events dispatched, actions executed, cron scripts run/failed, device API call latency, event handler latency. + +**Fix:** Add lightweight metrics (even in-memory counters exposed via a `/metrics` endpoint) or integrate with Prometheus/OpenTelemetry. + +--- + +### M6. `add_event_handler` discards handler return values +**File:** `server/SHServ/Middleware/ControlScripts.php` +**Method:** `add_event_handler` +The wrapper closure ignores the return value of `$handler`. There is no way for a handler to signal "stop propagation" or return a result. + +**Fix:** Return the handler result, or support explicit `stopPropagation()` mechanism. + +--- + +### M7. `SpotlightsScope::spotlights_by_time` is an empty regular script +**File:** `server/ControlScripts/Scopes/SpotlightsScope.php` +**Line:** 64–70 +Registered but does nothing. Pollutes the regular scripts list and cron loop. + +**Fix:** Remove or implement the time-based logic. + +--- + +### M8. `TestScriptsScope` references hardcoded production device +**File:** `server/ControlScripts/Scopes/TestScriptsScope.php` +**Line:** 23–26 +Alias `test_stand_relay` is hardcoded. If the device does not exist, the action script will fail at runtime. + +**Fix:** Gate test scopes behind `devmode` flag, or move to a separate directory excluded from production scan. + +--- + +### M9. `EventsModel` naming inconsistency +**File:** `server/SHServ/Models/EventsModel.php` +Methods: `global_any_device_event_call`, `global_device_event_call`, `channel_device_event_call`, `alias_device_event_call`, `channel_alias_device_event_call`. Naming pattern is inconsistent (`_any_` vs `_alias_` placement). + +**Fix:** Standardize naming to `{scope}_{target}_event_call`. + +--- + +### M10. `ControlScripts` constructor uses fragile string parsing +**File:** `server/SHServ/Middleware/ControlScripts.php` +**Line:** 24 +```php +list($scope_folder, $scope_name) = explode("\\", str_replace("\\Scopes", "", str_replace("SHServ", "", static::class))); +``` +Assumes exactly two namespace segments. If namespace depth changes, parsing breaks. + +**Fix:** Use `basename(str_replace('\\', '/', static::class))` for name, and `dirname()` logic for folder. + +--- + +### M11. `Scripts::remove_scope` has TOCTOU race +**File:** `server/SHServ/Models/Scripts.php` +**Method:** `remove_scope` +```php +if(!file_exists($filepath)) { return false; } +if(!@unlink($filepath)) { return false; } +``` +File can be modified between `file_exists` and `unlink`. + +**Fix:** Just attempt `unlink` and check the result; suppress warning with `@`. + +--- + +### M12. No request correlation ID for event dispatch chain +When a device sends an event, it triggers multiple handlers, each making device API calls. There is no tracing ID to correlate the original event with downstream actions in logs. + +**Fix:** Generate a `correlation_id` in `EventsController` and pass it through all event handlers and device API calls. + +--- + +### M13. `Logging` JSON file lacks indexing +**File:** `server/Fury/Kernel/Logging.php` +Searching logs requires loading and parsing the entire JSON file. No grep-friendly line format. + +**Fix:** Use newline-delimited JSON (NDJSON) for append-only, grep-friendly log files. + +--- + +### M14. `ControlScripts` constructor bypass for `RequiredControlScriptsScope` +**File:** `server/SHServ/App.php` +`RequiredControlScriptsScope` is instantiated directly before scanning the `Scopes/` directory. It is not listed in `control_scripts_instances` (it is in `$required_control_scripts_instance`). But its scripts are still registered in the same static arrays, mixing required and optional scopes. + +**Impact:** Required scope handlers can be accidentally disabled if static arrays are cleared. + +**Fix:** Separate static storage for required vs optional scopes, or ensure required scope is always registered regardless of DB state. + +--- + +### M15. `error_handler` in config only covers warnings and fatal errors +**File:** `server/SHServ/config.php` +```php +"important_errors" => ["E_WARNING", "E_ERROR", "E_CORE_ERROR", "EXCEPTION"] +``` +Notices, deprecations, and parse errors are silently ignored in production. This can hide bugs. + +**Fix:** Include `E_NOTICE`, `E_DEPRECATED`, `E_PARSE` in dev; log (but don't display) them in production. + +--- + +## Summary + +| Severity | Count | +|----------|-------| +| Critical | 6 | +| High | 14 | +| Medium | 15 | + +**Key themes:** +1. **Static state leakage** — `ControlScripts` static arrays cause cross-request and cross-test contamination. +2. **Missing error boundaries** — Cron scripts, action scripts, and event handlers run without `try/catch`, causing cascading failures. +3. **Unsafe file I/O** — Scope updates are non-atomic; sync-map is re-read redundantly. +4. **No observability** — No metrics, no tracing, no structured logging, empty error messages. +5. **Infrastructure gaps** — No DB migrations, minimal CLI, no log rotation, `.env` fallbacks are insecure. diff --git a/server/tests/AppAuthGuardTest.php b/server/tests/AppAuthGuardTest.php index 1bb7c71..7f69157 100644 --- a/server/tests/AppAuthGuardTest.php +++ b/server/tests/AppAuthGuardTest.php @@ -100,11 +100,9 @@ $result = $this -> testApp -> check_api_auth(); $this -> assertSame(429, $result['code']); - // Simulate time passing: manipulate stored timestamps via reflection - $ref = new \ReflectionClass(\SHServ\Tools\RateLimiter::class); - $prop = $ref -> getProperty('requests'); - $prop -> setAccessible(true); - $prop -> setValue(null, []); + // Reset rate limiter storage to simulate window passing + $limiter = new \SHServ\Tools\RateLimiter(60, 60); + $limiter -> clear(); $token = app() -> sessions -> create(1); $_SERVER['HTTP_AUTHORIZATION'] = 'Bearer ' . $token; diff --git a/server/tests/DevicesModelTransactionTest.php b/server/tests/DevicesModelTransactionTest.php index 9a6239d..4c18721 100644 --- a/server/tests/DevicesModelTransactionTest.php +++ b/server/tests/DevicesModelTransactionTest.php @@ -36,7 +36,8 @@ description TEXT, last_contact TEXT, create_at TEXT, - update_at TEXT + update_at TEXT, + UNIQUE(device_hard_id, status) )"); } @@ -139,6 +140,31 @@ $this -> assertIsArray($result); $this -> assertSame('device_not_found', $result['err_alias']); } + + public function test_connect_new_device_rejects_duplicate_hard_id(): void { + $mockApi = new MockDeviceAPIForSetup([ + 'data' => [ + 'status' => 'setup', + 'device_type' => 'relay', + 'ip_address' => '192.168.1.54', + 'mac_address' => 'AA:BB:CC:DD:EE:22', + 'device_id' => 'hard_duplicate', + 'firmware_version' => '1.0.0', + ] + ]); + + $result1 = $this -> devicesModel -> connect_new_device( + '192.168.1.54', 'relay_first', 'First Relay', '', $mockApi + ); + $this -> assertInstanceOf(\SHServ\Entities\Device::class, $result1); + + $result2 = $this -> devicesModel -> connect_new_device( + '192.168.1.54', 'relay_second', 'Second Relay', '', $mockApi + ); + $this -> assertIsArray($result2); + $this -> assertFalse($result2['result']); + $this -> assertSame('device_already_registered', $result2['err_alias']); + } } class MockDeviceAPIForSetup extends Base { diff --git a/server/tests/RateLimiterTest.php b/server/tests/RateLimiterTest.php index aef6df8..9c182d8 100644 --- a/server/tests/RateLimiterTest.php +++ b/server/tests/RateLimiterTest.php @@ -4,34 +4,54 @@ use SHServ\Tools\RateLimiter; class RateLimiterTest extends TestCase { + private ?RateLimiter $limiter = null; + + protected function tearDown(): void { + if ($this -> limiter !== null) { + $this -> limiter -> clear(); + } + } + public function test_allows_requests_within_limit(): void { - $limiter = new RateLimiter(5, 60); + $this -> limiter = new RateLimiter(5, 60); for ($i = 0; $i < 5; $i++) { - $this -> assertTrue($limiter -> check('ip1')); + $this -> assertTrue($this -> limiter -> check('ip1')); } } public function test_blocks_excess_requests(): void { - $limiter = new RateLimiter(3, 60); + $this -> limiter = new RateLimiter(3, 60); for ($i = 0; $i < 3; $i++) { - $limiter -> check('ip2'); + $this -> limiter -> check('ip2'); } - $this -> assertFalse($limiter -> check('ip2')); + $this -> assertFalse($this -> limiter -> check('ip2')); } public function test_resets_after_window(): void { - $limiter = new RateLimiter(2, 1); - $this -> assertTrue($limiter -> check('ip3')); - $this -> assertTrue($limiter -> check('ip3')); - $this -> assertFalse($limiter -> check('ip3')); + $this -> limiter = new RateLimiter(2, 1); + $this -> assertTrue($this -> limiter -> check('ip3')); + $this -> assertTrue($this -> limiter -> check('ip3')); + $this -> assertFalse($this -> limiter -> check('ip3')); sleep(2); - $this -> assertTrue($limiter -> check('ip3')); + $this -> assertTrue($this -> limiter -> check('ip3')); } public function test_different_keys_are_independent(): void { - $limiter = new RateLimiter(1, 60); - $this -> assertTrue($limiter -> check('ip_a')); - $this -> assertTrue($limiter -> check('ip_b')); + $this -> limiter = new RateLimiter(1, 60); + $this -> assertTrue($this -> limiter -> check('ip_a')); + $this -> assertTrue($this -> limiter -> check('ip_b')); + } + + public function test_file_storage_shared_across_instances(): void { + $tmpFile = sys_get_temp_dir() . '/rl-test-' . uniqid() . '.json'; + $limiterA = new RateLimiter(2, 60, $tmpFile); + $limiterB = new RateLimiter(2, 60, $tmpFile); + + $this -> assertTrue($limiterA -> check('shared')); + $this -> assertTrue($limiterB -> check('shared')); + $this -> assertFalse($limiterA -> check('shared')); + + unlink($tmpFile); } }