# 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.