# 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&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`

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