diff --git a/docs/known-issues.md b/docs/known-issues.md new file mode 100644 index 0000000..a516b75 --- /dev/null +++ b/docs/known-issues.md @@ -0,0 +1,222 @@ +# 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`). Two things matter: + +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. + +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. + +--- + +## 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. + +--- + +## 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. 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..395bb5d --- /dev/null +++ b/patches/Terminal-terminal.php.patched @@ -0,0 +1,155 @@ +"); +define("ROOT", Common::getWorkspacePath($project)); +define("BLOCKED", "ssh,telnet"); +// define("ENABLED", "java,ls,cd,echo,javac"); +define("ENABLED", false); + +//////////////////////////////////////////////////////////////////////////////80 +// Terminal Class +//////////////////////////////////////////////////////////////////////////////80 + +class Terminal { + + //////////////////////////////////////////////////////////////////////////80 + // Constructor + //////////////////////////////////////////////////////////////////////////80 + public function __construct() { + if (!isset($_SESSION["activeDir"]) || !isset($_SESSION["activeDir"][$_SESSION["project"]]) || empty($_SESSION["activeDir"][$_SESSION["project"]])) { + if (ROOT === "") { + $output = Common::raw_execute("pwd"); + $_SESSION["activeDir"] = array([$_SESSION["project"]] => 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