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

$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&params={} 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 <img> 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

$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

$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

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

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

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

$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

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