MMOFramework · 2026-09-code-review

# MMOFramework — Full Codebase Review & Bug Fix Report Autonomous expert-level review of every PHP file in `src/`'s bootstrap dependency chain (233 files, ~40,000 lines): bugs fixed with verified reproductions, full docblock + `@example` documentation added, dead code flagged rather than touched, and deep/architectural issues spawned as background tasks for your decision rather than guessed at. Test suite (923 tests, 2679 assertions) stayed green after every single edit — zero regressions across the entire effort. ## Table of Contents - [Critical Security Finding](#critical-security-finding) - [Background Tasks Requiring Your Decision](#background-tasks-requiring-your-decision) - [Bugs Fixed, By Layer](#bugs-fixed-by-layer) - [functions/](#functions) - [classes/](#classes) - [helpers/](#helpers) - [documents/ (incl. xlsx/)](#documents-incl-xlsx) - [printers/](#printers) - [schemas/](#schemas) - [entities/](#entities) - [server/](#server) - [controllers/](#controllers) - [Top-level modules](#top-level-modules) - [Notable Non-Bugs (investigated, confirmed correct)](#notable-non-bugs-investigated-confirmed-correct) - [Dead Code Identified (flagged, not touched)](#dead-code-identified-flagged-not-touched) - [Testing Methodology](#testing-methodology) - [Other Observations](#other-observations) --- ## Critical Security Finding ### `\MMO\core\shell\escape()` does not prevent command injection — ✅ FIXED **File:** `src/functions/shell.php` · **Task:** `task_2737e937` (marked SECURITY, now resolved and dismissed) The framework's core shell-argument-escaping function wrapped values in **double quotes**, but POSIX shells still perform `$(...)`/backtick command substitution *inside* double-quoted strings — only single-quote wrapping (`escapeshellarg()`-style) fully neutralizes that. **Proven, not theoretical:** ```php $evil = "hello\$(touch /tmp/PWNED_test_marker)world"; $escaped = \MMO\core\shell\escape($evil); // $escaped === "hello$(touch /tmp/PWNED_test_marker)world" ``` Running `echo ` through a real `sh` process **actually created the file** — the escaping provided zero protection. **Fix applied:** `escape()` now wraps POSIX (`$isWin = false`, the default off Windows) arguments in **single** quotes with `'\''`-style escaping of any embedded single quote — the same technique `escapeshellarg()` uses, which fully neutralizes shell interpretation of the value's content. The Windows path (`$isWin = true`) is untouched byte-for-byte: cmd.exe has no `$(...)`/backtick substitution to begin with (a POSIX shell feature), so it was never exposed to this specific vulnerability, and cmd.exe doesn't support single-quote escaping at all. Re-verified after the fix: the exact PoC above (file no longer created), all 6 of `escape()`'s own docblock `@example`s, a 9-value real-shell round-trip covering embedded quotes/backticks/newlines/pipes/etc. (all come back byte-identical), the Windows path (confirmed byte-identical output to before), every real caller in the codebase (`Cron.php`, `Process.php`), and the full 923-test suite — zero regressions. `Cron.php::addJob()` (which didn't call `escape()` on `$command`/`$email` at all before this review — trivially broken by a bare single quote) now benefits from both fixes: it consistently escapes, and `escape()` itself is actually safe. --- ## Background Tasks Requiring Your Decision Each of these was spawned as a background task chip (visible in your session) rather than guessed at, because the correct fix needs information or a design call only you have. Task IDs are stable — reference them directly. || Task ID | Title | Why it wasn't just fixed | |---|---|---|---| |[x]| `task_2737e937` | **SECURITY:** `shell\escape()` doesn't prevent command substitution injection | **Done** — fixed directly (see above), task dismissed | |[ ]| `task_03489439` | `Query::$total` doesn't reflect the true collection-wide match count | Needs an architectural decision: add an expensive unbounded count pass, or is the documented contract itself wrong? | |[ ]| `task_8de86b8f` | `ImportSocket::run()` skips every importer when importers is an unnamed list | Two conflicting features (`'keys'`-command filter vs. list-to-data remapping) share one variable; needs a decision on intended behavior | |[x]| `task_7fafee18` | Define the missing `iWorkflow` interface | `WorkflowController` is entirely non-functional (rejects every workflow class) because the interface it requires was never created — needs Marc's intended contract, not a guess | |[x]| `task_d1d1019e` | Remove abandoned configurable/connectable/middleware/registrable feature set | 6 dead files self-annotated `@todo remove` by you, plus one file with unsafe top-level demo code — deletion is your call | |[ ]| `task_e7e5ae9e` | Fix `HtmlPrinter` XSS without breaking raw-markup embedding | Confirmed real XSS in `exportString()`; a naive fix broke intentional raw-markup rendering elsewhere | |[ ]| `task_a555a68e` | Implement `iEntity::hasReference()` for `Commit::sortEntities()` | Pre-existing; needs real business logic, not a guess | |[ ]| `task_7e98f5ff` | Investigate `ShellPrinter`'s dead `__wakeup()` indent styling | Observed anomaly, root cause outside reviewed scope | --- ## Bugs Fixed, By Layer ### functions/ - **`strings.php` — `csvEncode()`**: `$headers === false` never resolved actual column keys, producing a PHP warning and garbage CSV output. Fixed. ### classes/ - No functional bugs found in the base classes reviewed (`aParser`, `aRequest`, `aResponse`, `aMessage`, etc.) beyond what's captured below in the files that use them. ### helpers/ - **`XmlNode.php` / `XmlNodes.php` — `offsetGet()`**: both declared reference-returning and directly indexed a possibly-missing array key — PHP auto-vivifies a phantom `null` entry as a side effect of merely reading it (rendered as a stray bare HTML attribute for `XmlNode`). Fixed to check `array_key_exists()` first. ### documents/ (incl. xlsx/) - **`Document.php`**: `addLink()`'s per-rel grouping broke `getLinksByRel()`/`getLinkByRel()` (crash) and `removeLink()` (silent no-op) for 2+ links sharing a rel — simplified to a flat array. `import()` never called `extract()` on `$_VARS`. Missing `implements \JsonSerializable` meant native `json_encode()` ignored the class's own `jsonSerialize()`. - **`HtmlDocument.php`**: `setFavicon()`'s `strrpos()` args were swapped. `createBody()`'s SERVER CONSOLE block was built when *not* raw but only included in body when raw (inverted). `getMeta()` returned raw XmlNode objects instead of arrays, silently stringified into rendered markup. - **`xlsx/styles/Alignement.php`**: crashed on missing horizontal/vertical; read wrong attribute names (`wrap`/`rotation` instead of `wrapText`/`textRotation`, always lost on round-trip); wrote misspelled root tag `` instead of the OOXML-spec ``. - **`xlsx/styles/Border.php`**: `set()` missing the `instanceof BorderSide` guard present in `__construct()`. - **`xlsx/styles/Fill.php`**: `FromXml()` used strict `===` comparing a raw string to an enum case (never true), corrupting every solid-fill round-trip. - **`xlsx/styles/Font.php`**: constructor's `$size` typed `?int` vs. property `?float`, truncating fractional point sizes. - **`xlsx/styles/Style.php`**: `FormatValue()`'s decimal-count logic mishandled `strpos()` returning `false` (no decimal point), producing spurious decimals like "50.0%" instead of "50%". - **`xlsx/Cell.php`**: `__clone()` tried to `clone` a PHP enum (uncloneable singleton) — crashed on any populated cell. - **`xlsx/Sheet.php`**: `getColumnXML()`'s XPath condition inverted; `updateFormulae()`'s force-refresh loop always skipped the last row/column; `FromFile()`'s row-height parsing was off-by-one and mishandled missing `ht`; `buildXML()` crashed on any not-yet-registered style (missing `$create: true`). - **`xlsx/Document.php`** (most bug-dense file): `Column::toXML()` had the same missing-`$create:true` bug; `buildXML()` crashed cryptically on zero sheets; three `addElement('xmlns:...')` calls used a fake namespace prefix, always throwing; the rels-renumbering XPath used an invalid double-colon axis, silently no-op'ing and risking duplicate relationship IDs on re-save. All caught via a full build→render→reload→modify→re-save round-trip test. ### printers/ - **`XmlPrinter.php`**: `exportLog()` called `render()` on a `Log` object with no such method — always crashed. Missing all 8 log-level templates. - **`JsonPrinter.php`**: `exportException()` only registered for `\Exception`, so `\Error` types leaked file paths via the reflection exporter — a real information-disclosure bug. - **`XlsxPrinter.php`**: `hash()`'s cyclic-reference formula always assumed a header row, producing a wrong cell reference when there wasn't one. - **`CsvPrinter.php`**: `Encode()` produced a correct header row but completely blank data rows (wrong key mapping). - **`HtmlPrinter.php`**: `exportFilename()` had a real, fixed XSS (unescaped href/text). `exportString()`'s non-dump branch is *also* XSS-vulnerable (verified with a live `` payload) but a fix broke intentional raw-markup rendering — reverted and flagged as `task_e7e5ae9e`. ### schemas/ - **`Schema.php`**: `isNullable()`/`toRegex()` missing `??[]` fallback, crashed on any schema with no explicit `type`. `has($id)` checked the wrong object's `anchors`, causing false negatives on non-root schemas. ### entities/ - **`Entity.php`**: `loopReferences()`'s deletion-safety check used strict `===` unconditionally, which can never match a list/object-type reference property — meaning `removePermanently()`/`replaceBy()` never detected references held in array properties. **Severe**: entity deletion silently succeeded when it should have been blocked. - **`Query.php`**: `setSorts()`/`setFields()` checked the wrong loop variable, so the `-field` descending/hide syntax only worked for the first field in a list. - **`Repository.php`**: `dropCollection()` returned `hasCollection()` instead of actually removing anything. - **`Importer.php`**: `ignore()`'s list-relation branch never stripped a `/*` suffix before lookup, and even fixed, mutated a throwaway copy instead of the real array (by-value vs by-reference helper). ### server/ - **`Cookie.php`**: `toNetscapeLine()` wrote `httpOnly` into the column meant for `secure`, losing `secure` entirely and never encoding `httpOnly`. - **`Link.php`**: `getRels()` returned `['']` instead of `[]` for a link with no rel, violating its own contract. - **`ShellRequest.php`**: `withBody()` broke its own class invariant, returning a raw array instead of a `ShellArguments` — crashed every subsequent call on the resulting object. - **`Request.php`**: `withQueryParams()` *always* threw (the request's URI is always constructed immutable) and separately discarded its own clone's mutation by returning `$this`. Multi-file upload in `Request::Server()` always crashed (malformed original `$_FILES` entry left behind after normalization). - **`Response.php`**: `FromMime()` crashed with a confusing internal `TypeError` for html/xml + array bodies — now a clear `eInvalid`. - **`Route.php`**: `setMiddlewareOrder()`, `setMiddlewareParameters()`, `addMiddlewareParameters()` were all silent no-ops (by-value `foreach` instead of by-reference). `getMiddlewareParameters()`'s declared return type (`?int` instead of `?array`) crashed whenever a match was found. `setFormat()` silently discarded its own `$charset` argument, hardcoding `'utf-8'` instead. - **`Websocket.php`**: `close()`'s close-frame send was outside its own try/catch, and the stream was never cleaned up on failure — a peer disconnecting mid-close cascaded through nested `close()` calls into an **uncaught exception escaping `__destruct()` during PHP shutdown**. - **`Curl.php`**: an operator-precedence bug in `DebugOptions()` (`??` vs `===`) made JSON-vs-urlencoded detection always true. `FromString()`'s `-H`/`--header` parsing stored custom headers under the wrong CURLOPT constant, **silently dropping every custom header and corrupting the response body**. The same wrong constant existed independently in `DebugOptions()`'s switch. - **`Mail.php`**: `setPriority(null)` was supposed to remove a header but unconditionally re-set it right after, leaving a stray `null`-valued key that would render as a malformed header in real outgoing email. `setConfirmReading()` called a nonexistent method (crashed immediately). `replaceInlineImage()`'s greedy regex silently destroyed content between the first and last of 2+ inline images in one email body — the most serious of the three. ### controllers/ - **`MCPController.php`**: entirely dead code — never included in `bootstrap.php` despite being committed today (unlike its 5 sibling controllers, all explicitly included). Also, `routeMCP()` crashed with an uncaught `TypeError` on any unrecognized JSON-RPC method instead of returning a proper error response. - **`WorkflowController.php`**: entirely non-functional — the `iWorkflow` interface it requires doesn't exist anywhere in the codebase, so every workflow class is rejected even when correctly implemented. Made the failure mode honest (see [Background Tasks](#background-tasks-requiring-your-decision)). - **`RepositoryController.php`**: `getRepositoryName()` had the same falsy-key `array_search()`/`?:` bug found elsewhere. The `first` pagination link's formula was simply wrong (neither the true first page nor the aligned current page). Deeper `Query::$total` semantics issue flagged separately. ### Top-level modules - **`Configuration.php`**: `load()`'s `uksort()` comparator was logically inconsistent (only special-cased `Application::class` on one side of the comparison), meaning the app's own config could silently fail to load last, depending on what else was configured. Also removed a stray unused import from an unrelated PECL extension namespace. - **`Cron.php`**: `addJob()` interpolated `$command`/`$email` into a shell command with **no escaping at all** (see the security finding above). - **`ImportSocket.php`**: `run()`'s "no configuration defined" guard checked `=== null`, but the property defaults to `[]` — the check never fired, silently doing nothing instead of erroring. A deeper bug (every importer skipped when importers are an unnamed list) was diagnosed and flagged rather than guessed at. - **`JsonParser.php`** — the most bug-dense file of the whole review, 5 distinct crashes fixed: 1. `decodeObject()`/`decodeArray()`'s "get to `,` or `}`/`]`" sections dereferenced a possibly-null token. 2. The `eJson` exception class the parser's own `addError()` throws **did not exist anywhere in the codebase** — every single parse error crashed with "Class not found" instead of the intended message. Created `src/exceptions/eJson.php` and wired it into `bootstrap.php`. 3. Single-quoted strings were warned-about-but-tolerated in name only — never actually converted to valid JSON, so any single-quoted string crashed with a raw `\JsonException`. Fixed with a careful byte-by-byte quote conversion (verified against embedded quotes, escaped backslashes, `\t`/`\uXXXX` escapes). 4. `decodeAny()` itself dereferenced a null token before its own null check, crashing on fully empty input. - **`XmlParser.php`**: 4 null-dereference crashes on truncated/malformed HTML (a trailing `<`, ``), all violating this parser's own designed-to-be-tolerant behavior. - **`Application.php`** — the framework's HTTP kernel, 4 real bugs: 1. `getJobTrigger()` and `getTriggerJob()` both had a classic `foreach`-shadows-parameter bug — the loop variable silently overwrote the search parameter, so both **always returned null** regardless of input. 2. `getPaths()`'s `$resolve` parameter was inverted, and — since this method is the `#[Serialize]` target for the app's own `paths` config — a save/reload cycle permanently flattened every `{xxx}`-style path cross-reference into a static absolute path. 3. `findRoute()` had the same `??` vs `===` precedence bug as Curl.php, making its group-id fallback permanently dead code. - **`defines.php`**: `LANGUAGE` defaulted to `'en_EN'` — not a real locale (`setlocale()` silently fails on it, confirmed directly), and an invalid Facebook OpenGraph value for the `og:locale` meta tag every `HtmlDocument` renders by default. Fixed to `'en_US'`, verified safe against the one place that treats `en_EN`/`en_US`/`en_GB` as interchangeable "already English" sentinels. - **`bootstrap.php`**: was missing `MCPController.php`'s include (see above); added it, plus `eJson.php`. --- ## Notable Non-Bugs (investigated, confirmed correct) A few things looked suspicious but were verified correct via real execution — noted so the reasoning isn't lost: - **`Stream::getContentType()` / `StreamResponse::getContentType()`**: declared return type didn't include `false` even though the method can return it — harmless (PHP's weak typing coerces it to `''`, and every real caller checks loosely). Widened the type on `Stream` for clarity; **could not** do the same on `StreamResponse` because it overrides a parent method and PHP requires covariant return types — confirmed via a real fatal-error reproduction. Documented instead. - **`Process.php`**'s `exec`-wrapping technique only supports single real executables, not shell builtins or compound syntax (`cmd1; cmd2`) — initially mistaken for a bug during testing, then understood as an intentional, documented constraint of the signal-handling technique (adapted from Symfony's Process component). - **`StatisticController::mailGraph()`**'s `self::MailTendency()`/`self::MailHistogram()` calls (wrong case, static syntax on instance methods) are completely legal in PHP when called from an active object context — verified with an isolated minimal repro, not just the framework. - **`Route::has()`**'s internal bitmask computation is unobservable dead complexity, since PHP's weak typing coerces its declared `bool` return down from whatever int it actually computes. --- ## Dead Code Identified (flagged, not touched) Per policy, dead/unused code was flagged for your decision rather than silently deleted or activated: - `src/entities/Issue.php` — entirely commented out, never included. - `src/classes/tConfigurable.php`, `tMiddleware.php`, `src/interfaces/iConfigurable.php`, `iConnectable.php`, `iMiddleware.php`, `iRegistrable.php` — a self-contained abandoned feature set, each self-annotated `@todo remove` by you in their own docblocks, referenced nowhere (see `task_d1d1019e`). - `src/documents/xlsx/Image.php` — worse than dead: contains unconditional top-level executable demo code (hardcoded file paths, an `echo`) that would run immediately if the file were ever included. --- ## Testing Methodology - Every suspected bug was confirmed via **real PHP execution** (scratch scripts) before being fixed — never reasoning alone. - Every fix, and every `@example` added to documentation, was **re-verified by running it**. - After every single edit: `php -l ` then the full test suite. Baseline **923 tests, 2679 assertions**, maintained with **zero regressions** across the entire multi-hour effort. - Where full end-to-end testing was impractical without side effects (e.g. `Cron.php`'s real crontab mutations, `Mail.php`'s real delivery), safe alternatives were used instead — a fake-sendmail capture script for `Mail.php`/`MailController.php`, read-only crontab inspection plus isolated logic replication for `Cron.php`, and a full raw-socket WebSocket client/server harness for `Websocket.php`. - Real SSH end-to-end authentication testing was deliberately **not** attempted, since it would have required modifying your actual `~/.ssh/authorized_keys` — judged out of scope for autonomous action. --- ## Other Observations **Shared working directory.** This working directory is shared with at least one other active process: git history shows a commit `"MCP controller"` by you and, 18 seconds later, another commit `"fix by claude on shell and schema"` touching `src/functions/shell.php` and `src/schemas/{Coercer,Validator}.php` — both landing while this session was running, and only one worktree is registered for this repo. This session only ever ran read-only git commands, so nothing here was put at risk, but it's worth knowing the checkout isn't exclusively yours-and-this-session's right now.