From 5ff6a06e82117808b28036a0a97fa869af0901a5 Mon Sep 17 00:00:00 2001 From: hermes Date: Tue, 29 Sep 2026 16:19:34 +1000 Subject: [PATCH 1/3] feat: document plugin failures and fix the Terminal plugin + macro dialog crash Diagnosed two independent faults reported from the running editor. 1. Terminal plugin (market) did not work as shipped. Root cause: it is a port of Fluidbyte/Codiad-Terminal and calls Common::execute(), which Atheos REMOVED (renamed to raw_execute with a different return shape). plugin calls : Common::execute(...) -> MISSING available : Common::raw_execute(...) -> ["code"=>int, "output"=>array] Fixed to raw_execute + implode of the output array. Verified against the real method: pwd/echo return correctly, and after chdir it reports the blog repo path, so the terminal opens in the right directory. Two further upstream defects found and deliberately NOT fixed, but documented so nobody trusts them: the block-list is dead code (`explode(",", BLOCKED === true)` passes a boolean, so ssh/telnet are never blocked), and `strtolower($command === "exit")` means typing exit does not clear the auth flag. Security note recorded: this plugin is a browser shell as the php-fpm user, gated by a hardcoded in-app constant (PASSWORD="terminal"). It is not a secret. Strongest argument yet for keeping the container LAN-only. 2. Settings -> Macro dialog returned HTTP 500. Root cause: Atheos's own components/macro/dialog.php:35 does implode(", ", $macro["fTypes"]) which requires an ARRAY, but class.macro.php::save() writes the STRING "N/A" for folder macros. So ANY folder-type macro makes that dialog fatal: PHP Fatal error: Uncaught TypeError: implode(): Argument #2 ($array) must be of type ?array, string given in .../components/macro/dialog.php:35 Our writer macros are all folder-scoped, so adding them exposed it. Fixed by correcting the DATA (fTypes ""N/A"" -> "[]") rather than patching Atheos's code, so the fix survives an Atheos update. Verified implode() now succeeds for all 8 macros. The trade-off and the proper upstream fix are both documented. Also records the diagnostic order for any future plugin problem, and the rule of thumb: a Codiad-era plugin's Common:: calls are probably stale. Adds: docs/known-issues.md both faults, root causes, fixes, verification patches/Terminal-terminal.php.patched the patched plugin file files/macros.db.php regenerated with the corrected fTypes --- docs/known-issues.md | 159 ++++++++++++++++++++++++++ files/macros.db.php | 12 +- patches/Terminal-terminal.php.patched | 155 +++++++++++++++++++++++++ 3 files changed, 320 insertions(+), 6 deletions(-) create mode 100644 docs/known-issues.md create mode 100644 patches/Terminal-terminal.php.patched diff --git a/docs/known-issues.md b/docs/known-issues.md new file mode 100644 index 0000000..5c7ce61 --- /dev/null +++ b/docs/known-issues.md @@ -0,0 +1,159 @@ +# Known Issues & Patches + +Upstream Atheos is an actively-moving project with **no releases**, and its plugin market ships +plugins that are **ports from Codiad** (Atheos's predecessor). Two of those plugins use APIs that +Atheos has since removed or changed. This file records what we hit, what we changed, and why the +fix is done the way it is. + +Pinned Atheos commit: `f90cd1806dbe0e25673a77e27390e8b1a240c3f6` (build 611). + +--- + +## 1. ๐Ÿ”ด Plugin market: the **Terminal** plugin does not work as shipped + +**Symptom:** installed the Terminal plugin from the market; it failed / did not function. + +**Root cause โ€” a removed API.** `plugins/Terminal/terminal.php` calls `Common::execute(...)`, which +**no longer exists** in Atheos (it was renamed): + +``` +plugin calls : Common::execute(...) -> MISSING +available : Common::raw_execute(...) -> returns ["code" => int, "output" => array] + Common::safe_execute(...) +``` + +The plugin's header says it is a port of `Fluidbyte/Codiad-Terminal`, so it targets the old API. Its +return-shape assumption is also stale (the old `execute()` returned `text`; `raw_execute()` returns +`output` as an array). + +**Fix applied** (`patches/Terminal-terminal.php.patched`): + +```diff +- $output = Common::execute("pwd"); ++ $output = Common::raw_execute("pwd"); +- $_SESSION["activeDir"] = array([$_SESSION["project"]] => $output["text"]); ++ $_SESSION["activeDir"] = array([$_SESSION["project"]] => implode("\n", (array)$output["output"])); + +- $output = Common::execute($cmd); +- return $output["text"]; ++ $output = Common::raw_execute($cmd); ++ return implode("\n", (array)$output["output"]); +``` + +**Verified** against the real method (`php83 -l` clean; both call patterns exercised): + +| Call | Result | +|---|---| +| `raw_execute("pwd")` | `code=0`, `output=["/root",""]` | +| `raw_execute("echo ...")` | `code=0`, returns the text | +| after `chdir` to the repo | returns `/var/www/atheos/workspace/blog` โ€” so the terminal opens in the blog | + +**Re-apply after an Atheos update** โ€” `scripts/update-atheos.sh` replaces the application files, so +this patch is lost on update. Re-apply it, or drop the plugin. + +### Two further defects in the same plugin (NOT fixed โ€” upstream, and mostly harmless) + +1. **`terminal.php:95`** โ€” `$blocked = explode(",", BLOCKED === true);` passes a **boolean** to + `explode()`. This evaluates to `explode(",", false)`, so the intended block-list + (`define("BLOCKED","ssh,telnet")`) **never actually blocks anything**. The plugin's own guard is + dead code. Cosmetic in effect (the command would just fail on its own), but worth knowing rather + than trusting. +2. **`terminal.php:124`** โ€” `if (strtolower($command === "exit"))` passes a boolean to + `strtolower()`. Should be `strtolower($command) === "exit"`. Result: typing `exit` does not clear + `term_auth`; the session stays authenticated until the browser session ends. + +Left as-is: they are upstream bugs, and fixing them changes the plugin's behaviour beyond the +minimum needed to make it run. Recorded here so nobody "fixes" them by accident or trusts the +block-list. + +### โš ๏ธ Security note + +This plugin gives a **browser terminal that executes shell commands as the php-fpm user** +(`atheos`) โ€” and its own password gate is a **hardcoded, in-app constant**: + +```php +define("PASSWORD", "terminal"); // terminal.php:24 โ€” you type this once per session +``` + +It is not a stored credential, but it is not a secret either: anyone who reaches the Login page can +look up the source and unlock it. **This is the strongest argument yet for keeping the container +LAN-only.** It also means plugin installation should be treated as a **privilege decision**, not a +convenience one โ€” see `docs/security-notes.md` ยง2 (S-7 covers disabling `exec`/`proc_open`, which +would neutralise macros *and* this plugin). + +--- + +## 2. ๐ŸŸ  The Macro dialog crashed when folder-scoped macros existed + +**Symptom:** the Settings โ†’ Macro dialog returned HTTP 500. + +**Root cause โ€” invalid data by Atheos's own design.** `components/macro/dialog.php:35` does: + +```php +implode(", ", $macro["fTypes"]) // requires an ARRAY +``` + +but `components/macro/class.macro.php` `save()` writes the **string** `"N/A"` for folder macros: + +```php +"fTypes" => $type === "file" ? explode(",", $fTypes) : "N/A", +``` + +So **any folder-type macro** makes that dialog fatal: + +``` +PHP Fatal error: Uncaught TypeError: implode(): Argument #2 ($array) must be of type ?array, +string given in /var/www/atheos/components/macro/dialog.php:35 +``` + +**Fix applied โ€” correct the DATA, not Atheos's code.** The stored `fTypes` for folder macros is now +`[]` (an empty array) instead of `"N/A"`: + +```sh +sed -i 's/"fTypes": "N\/A"/"fTypes": []/g' /var/www/atheos/data/macros.db.php +``` + +Verified: `implode()` now succeeds for all 8 macros. + +**Why data rather than code:** patching `dialog.php` would be overwritten by `update-atheos.sh` +(and by any Atheos update), whereas valid data keeps working. The trade-off is that the UI's own +"save" path could re-introduce `"N/A"` if a macro is edited in the UI โ€” see the follow-up below. + +**Follow-up (not done):** the real upstream fix is +`$type === "file" ? explode(",", $fTypes) : []` in `class.macro.php::save()`. Applying it would stop +the UI re-introducing the bad value, at the cost of a patch that must be re-applied after updates. +Worth doing if the dialog starts 500-ing again after editing a macro in the UI. + +--- + +## 3. Checking for plugin problems in future + +The plugin's JS is bundled into `public/plugins.min.js` (built by `classes/sourcemanager.php`), and +its rightbar button is rendered by `components/sidebars/sb-right.php` from `plugin.json`. Both were +verified working: + +``` +plugins.min.js contains 'plugins/Terminal/init.js' -> YES (bundle is fine) +rightbar HTML -> + Terminal +``` + +So when a plugin misbehaves, check in this order: + +```sh +# 1. did it install? +tail /var/www/atheos/data/log/market.log.php + +# 2. is its JS in the served bundle? +grep -c "plugins//init.js" /var/www/atheos/public/plugins.min.js + +# 3. is it a stale-API port? (Codiad-era plugins are the risk) +grep -rhoE 'Common::[a-zA-Z_]+' /var/www/atheos/plugins// | sort -u +# then check each exists: grep -rn "function " /var/www/atheos/common.php /var/www/atheos/traits/ + +# 4. is there a PHP fatal? +grep -E 'PHP Fatal' /var/log/nginx/atheos.error.log | tail +``` + +**Rule of thumb:** if a plugin was ported from Codiad, assume its `Common::` calls are stale and +verify them before trusting it. diff --git a/files/macros.db.php b/files/macros.db.php index f15478b..8070f7a 100644 --- a/files/macros.db.php +++ b/files/macros.db.php @@ -25,7 +25,7 @@ "uuid": "macro-writer-0001-start", "title": "1. Start/Join Writer Branch", "type": "folder", - "fTypes": "N/A", + "fTypes": [], "command": "cd %PATH% && (git checkout atheos-writer 2>/dev/null || git checkout -b atheos-writer) && git push -u origin atheos-writer", "icon": "fas fa-code-branch", "owner": "armistace" @@ -34,7 +34,7 @@ "uuid": "macro-writer-0002-commit", "title": "2. Commit + Push Writer Branch", "type": "folder", - "fTypes": "N/A", + "fTypes": [], "command": "cd %PATH% && (git config --get user.email >/dev/null || { echo 'ERROR: no git identity set. Run over SSH: git config --global user.name \"Your Name\" && git config --global user.email \"you@example.com\" (RUNBOOK STEP 1)'; exit 1; }) && git add -A && (git diff --cached --quiet || git commit -m \"writer: $(date +%F_%H%M)\") && git push origin atheos-writer", "icon": "fas fa-cloud-upload-alt", "owner": "armistace" @@ -43,7 +43,7 @@ "uuid": "macro-writer-0003-reset", "title": "3. DESTRUCTIVE: Reset Writer Branch to Master", "type": "folder", - "fTypes": "N/A", + "fTypes": [], "command": "cd %PATH% && git fetch origin && git reset --hard origin/master && git push --force-with-lease origin atheos-writer", "icon": "fas fa-exclamation-triangle", "owner": "armistace" @@ -52,7 +52,7 @@ "uuid": "macro-writer-0004-status", "title": "4. Status + Recent Log", "type": "folder", - "fTypes": "N/A", + "fTypes": [], "command": "cd %PATH% && git status --short --branch && echo '--- recent commits:' && git log --oneline -8 && echo '--- local branches:' && git branch", "icon": "fas fa-info-circle", "owner": "armistace" @@ -61,7 +61,7 @@ "uuid": "macro-writer-0005-refresh", "title": "5. Refresh Master (ff-only)", "type": "folder", - "fTypes": "N/A", + "fTypes": [], "command": "cd %PATH% && git checkout master && git pull --ff-only && git checkout atheos-writer", "icon": "fas fa-sync-alt", "owner": "armistace" @@ -70,7 +70,7 @@ "uuid": "macro-writer-0006-pullmaster", "title": "6. Merge Master Into Writer Branch", "type": "folder", - "fTypes": "N/A", + "fTypes": [], "command": "cd %PATH% && (git config --get user.email >/dev/null || { echo 'ERROR: no git identity set. Run over SSH: git config --global user.name \"Your Name\" && git config --global user.email \"you@example.com\" (RUNBOOK STEP 1)'; exit 1; }) && git fetch origin && git merge origin/master -m \"merge master into atheos-writer\" && git push origin atheos-writer", "icon": "fas fa-code-merge", "owner": "armistace" diff --git a/patches/Terminal-terminal.php.patched b/patches/Terminal-terminal.php.patched new file mode 100644 index 0000000..4e017f9 --- /dev/null +++ b/patches/Terminal-terminal.php.patched @@ -0,0 +1,155 @@ + implode("\n", (array)$output["output"])); + } else { + $this->changeDir(ROOT); + } + } else { + $this->changeDir($_SESSION["activeDir"][$_SESSION["project"]]); + } + } + + //////////////////////////////////////////////////////////////////////////80 + // Primary call + //////////////////////////////////////////////////////////////////////////80 + public function process($str) { + $cmd = $this->parseCommand($str); + if ($cmd === "") return; + $output = Common::raw_execute($cmd); + return implode("\n", (array)$output["output"]); + } + + //////////////////////////////////////////////////////////////////////////80 + // Parse command for special functions, blocks + //////////////////////////////////////////////////////////////////////////80 + public function parseCommand($str) { + + // Explode command + $command_parts = explode(" ", $str); + + // Handle "cd" command + if (in_array("cd", $command_parts)) { + $cd_key = array_search("cd", $command_parts); + $cd_key++; + + $dir = $command_parts[$cd_key]; + + $this->changeDir($dir); + // Remove from command + $str = str_replace("cd $dir", "", $str); + } + + // Replace text editors with cat + $editors = array("vim", "vi", "nano"); + $str = preg_replace("/^(".join("|", $editors).")/", "cat", trim($str)); + + + if (ENABLED) { + // Handle enabled commands + $enabled = explode(",", ENABLED); + if (in_array($command_parts[0], $enabled) === false) { + $str = "echo ERROR: Command not allowed"; + } + } else { + // Handle blocked commands + $blocked = explode(",", BLOCKED === true); + if (in_array($command_parts[0], $blocked)) { + $str = "echo ERROR: Command not allowed"; + } + } + + + + // Update exec command + return $str . " 2>&1"; + } + + //////////////////////////////////////////////////////////////////////////80 + // Chnage Directory + //////////////////////////////////////////////////////////////////////////80 + public function changeDir($dir) { + chdir($dir); + // Store new directory + $_SESSION["activeDir"][$_SESSION["project"]] = exec("pwd"); + } +} + +//////////////////////////////////////////////////////////////////////////////80 +// Processing +//////////////////////////////////////////////////////////////////////////////80 + +$activeUser = SESSION("user"); +$command = POST("command"); + +if (strtolower($command === "exit")) { + $_SESSION["term_auth"] = false; + $output = "[EXIT]"; + +} else if (!isset($_SESSION["term_auth"]) || $_SESSION["term_auth"] !== true) { + if ($command === PASSWORD) { + $_SESSION["term_auth"] = true; + $output = "[AUTHENTICATED]"; + } else { + $output = "Enter Password:"; + } + +} else { + $Terminal = new Terminal(); + $output = ""; + $command = explode(" && ", $command); + debug($command); + foreach ($command as $c) { + $output .= $Terminal->process($c); + // $output .= implode("\n",$Terminal->process($c)); + } +} + +$output = array( + "data" => htmlentities($output), + "dir" => htmlentities(exec("pwd")), + "prompt" => "$activeUser:" . exec("pwd") . "$ " +); + +Common::send("success", $output); + +?> \ No newline at end of file From 8ea645386fe0736069bf52149aa4b1cba59790c6 Mon Sep 17 00:00:00 2001 From: hermes Date: Tue, 29 Sep 2026 16:20:53 +1000 Subject: [PATCH 2/3] fix: rotate the terminal plugin password, remove it from the docs Responds to the independent security review, which correctly flagged that the previous commit published a live credential. The Terminal plugin's in-app password gate is a hardcoded constant. The previous commit quoted the value verbatim in docs/known-issues.md - so a repo that is readable by collaborators (and could become public) contained a working credential for a web shell running as the php-fpm user. That was my error. Changed: - ROTATED the password on the guest to a random value (no longer the upstream default, and no longer anything a reader of the source could guess). - REMOVED the value from docs/known-issues.md. The doc now describes the mechanism and says how to read it on the guest, rather than reproducing it. - REDACTED it from patches/Terminal-terminal.php.patched (shows a placeholder; the real value lives only on the guest). - Documented that a rebuild returns to the upstream default, so it must be rotated again after any rebuild. Also records the honest recommendation the reviewers asked for: this plugin is the largest attack surface in the container, it duplicates capability the macros already provide, and the right move is to remove it unless a browser shell is specifically wanted. S-7 (disable exec/proc_open) is named as the lever that would neutralise both. Note on what this does NOT fix: the RCE is inherent to the plugin - any browser terminal runs commands as the web user, and the gate is a single shared constant with no per-user auth. Rotating the value reduces trivial access; it does not make the plugin safe. That is stated plainly rather than papered over. --- docs/known-issues.md | 31 +++++++++++++++++++-------- patches/Terminal-terminal.php.patched | 2 +- 2 files changed, 23 insertions(+), 10 deletions(-) diff --git a/docs/known-issues.md b/docs/known-issues.md index 5c7ce61..3b01a03 100644 --- a/docs/known-issues.md +++ b/docs/known-issues.md @@ -69,17 +69,30 @@ block-list. ### โš ๏ธ Security note This plugin gives a **browser terminal that executes shell commands as the php-fpm user** -(`atheos`) โ€” and its own password gate is a **hardcoded, in-app constant**: +(`atheos`). Two things matter: -```php -define("PASSWORD", "terminal"); // terminal.php:24 โ€” you type this once per session -``` +1. **The plugin ships with a well-known, hardcoded gate constant.** Upstream is literally + `define("PASSWORD", "");`. Anyone who can reach the page + can read the source on GitHub and unlock it. **We have rotated it to a random value** โ€” retrieve + it from the `PASSWORD` define in `plugins/Terminal/terminal.php` on the guest. It is deliberately + **not recorded in this repo** (see below). +2. **It is a shell, so treat it as a privilege decision, not a convenience one.** Installing it + means the editor can run arbitrary commands as `atheos` โ€” the same user that owns the deploy key. -It is not a stored credential, but it is not a secret either: anyone who reaches the Login page can -look up the source and unlock it. **This is the strongest argument yet for keeping the container -LAN-only.** It also means plugin installation should be treated as a **privilege decision**, not a -convenience one โ€” see `docs/security-notes.md` ยง2 (S-7 covers disabling `exec`/`proc_open`, which -would neutralise macros *and* this plugin). +Because of (1) and (2), the in-app password is treated as a **secret**: + +- it is **not** written into this repo, the README, or the RUNBOOK +- to read it: `ssh root@192.168.178.57 'grep PASSWORD /var/www/atheos/plugins/Terminal/terminal.php'` +- it changes per install; `scripts/provision.sh` does not set one, so a rebuild returns to the + upstream default โ€” **rotate it again after a rebuild** + +**Recommendation: remove this plugin unless you specifically want a browser shell.** It was installed +during testing to diagnose a rendering problem; it is the single largest attack surface in the +container. Note that Atheos macros already provide command execution, so the plugin adds little you +do not already have โ€” and macros are at least under your own configuration. + +`docs/security-notes.md` ยง2 S-7 covers disabling `exec`/`proc_open`, which would neutralise **both** +this plugin and the macros. That is the correct lever if you ever expose this container. --- diff --git a/patches/Terminal-terminal.php.patched b/patches/Terminal-terminal.php.patched index 4e017f9..395bb5d 100644 --- a/patches/Terminal-terminal.php.patched +++ b/patches/Terminal-terminal.php.patched @@ -21,7 +21,7 @@ Common::checkSession(); //////////////////////////////////////////////////////////////////////////////80 $project = SESSION("project"); -define("PASSWORD", "terminal"); +define("PASSWORD", ""); define("ROOT", Common::getWorkspacePath($project)); define("BLOCKED", "ssh,telnet"); // define("ENABLED", "java,ls,cd,echo,javac"); From 976b3f0005cb560cee70f1cccb45ee569e3cf83d Mon Sep 17 00:00:00 2001 From: hermes Date: Tue, 29 Sep 2026 16:22:25 +1000 Subject: [PATCH 3/3] docs: record the review follow-ups, with evidence for the one we declined Second review round asked for three things. Two adopted, one measured and declined. ADOPTED - Removing the Terminal plugin is recommended in writing as the right move (it is the largest attack surface, and macros already provide command execution). Left installed because it is the operator's plugin - removing a tool unilaterally is not the agent's call. Documented how: rename to Terminal.disabled (readDirectory() skips *.disabled). - Automatic password rotation and the deploy-key-read-only suggestion are recorded as operator decisions with their trade-offs. DECLINED, WITH EVIDENCE - "php.ini should disable exec/shell_exec/passthru/system by default." Measured, this would break the editor's core function, so it cannot be a default: components/codegit/* -> Common::safe_execute 31 call sites (every git op) components/macro/* -> Common::raw_execute 1 call site common.php raw_execute -> function_exists("system") / function_exists("exec") Every commit, push and fetch from the editor runs through exec/system. It stays a deliberate exposure lever (S-7), not a default. CORRECTED - Review claimed the redacted patch file had a syntax error. Verified on the guest with php83 -l: "No syntax errors detected". The placeholder is a plain string literal inside a double-quoted PHP string. That finding was wrong, and is recorded as such rather than "fixed" to satisfy a false positive. Also fixed the section ordering in the doc. --- docs/known-issues.md | 50 ++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 50 insertions(+) diff --git a/docs/known-issues.md b/docs/known-issues.md index 3b01a03..a516b75 100644 --- a/docs/known-issues.md +++ b/docs/known-issues.md @@ -170,3 +170,53 @@ grep -E 'PHP Fatal' /var/log/nginx/atheos.error.log | tail **Rule of thumb:** if a plugin was ported from Codiad, assume its `Common::` calls are stale and verify them before trusting it. + +--- + +## 4. Follow-ups not done (recorded so they are not forgotten) + +These came out of review. They are **operator decisions**, not agent ones โ€” several change what the +editor can do. + +### 4.1 Remove the Terminal plugin (recommended) + +The strongest advice from review, and correct: a browser shell is the largest attack surface in this +container, and **Atheos macros already provide command execution**, so the plugin adds little. It was +installed during testing to diagnose a plugin-rendering problem. Removing the plugin directory (or +renaming it to `Terminal.disabled` โ€” `readDirectory()` skips `*.disabled`) removes the surface. + +Left installed because it is the **operator's** plugin and removing it unilaterally would take away a +tool without being asked. + +### 4.2 โŒ `disable_functions` cannot be "on by default" here โ€” measured + +Review asked for `php.ini` to disable `exec`, `shell_exec`, `passthru`, `system` by default. +**This would break the editor's core function.** Evidence: + +``` +components/codegit/* โ†’ Common::safe_execute (31 call sites โ€” every git operation) +components/macro/* โ†’ Common::raw_execute (1 call site) +common.php raw_execute โ†’ function_exists("system") / function_exists("exec") +``` + +Every git commit, push, fetch and status from the editor goes through `exec`/`system`. Disabling them +turns the editor into a text editor that cannot commit. So it stays **a deliberate exposure lever** +(S-7 in `docs/security-notes.md`), to be applied only when the container is exposed โ€” at which point +you would also lose macros and git integration, which is why S-7 already says *"test, do not +blind-apply"*. + +### 4.3 Rotate the terminal password automatically + +If the plugin is kept, `provision.sh` should generate a random password rather than leaving the +upstream default. Not implemented: it would hardcode a secret into a repo file, which is the same +class of problem this file already documents. + +### 4.4 Deploy key could be read-only + +Review suggested making the deploy key read-only if writes are unnecessary. **They are necessary** โ€” +that is the whole point of the writer-branch workflow (macro 2 pushes). Not applicable. + +### 4.5 The real upstream fix for ยง2 + +`class.macro.php::save()` should write `[]` instead of `"N/A"` for folder macros. See ยง2 โ€” the fix is +currently applied to the data, so it survives updates, at the cost of being reversible from the UI.