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

@@ -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", "<the default value in the plugin source>");`. 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                                        -> <a onclick="atheos.Terminal.open();">
+                                                        <i class="fas fa-terminal"></i>Terminal</a>
+```
+
+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/<Name>/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/<Name>/ | sort -u
+#    then check each exists:  grep -rn "function <name>" /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.

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"

patches/Terminal-terminal.php.patched

@@ -0,0 +1,155 @@
+<?php
+//////////////////////////////////////////////////////////////////////////////80
+// Atheos Terminal
+//////////////////////////////////////////////////////////////////////////////80
+// Copyright (c) 2020 Liam Siira (liam@siira.io), distributed as-is and without
+// warranty under the MIT License. See [root]/docs/LICENSE.md for more.
+// This information must remain intact.
+//////////////////////////////////////////////////////////////////////////////80
+// Copyright (c) 2013 Codiad & Kent Safranski
+// Source: https://github.com/Fluidbyte/Codiad-Terminal
+//////////////////////////////////////////////////////////////////////////////80
+require_once("../../common.php");
+
+//////////////////////////////////////////////////////////////////////////////80
+// Verify Session or Key
+//////////////////////////////////////////////////////////////////////////////80
+Common::checkSession();
+
+//////////////////////////////////////////////////////////////////////////////80
+// Globals
+//////////////////////////////////////////////////////////////////////////////80
+$project = SESSION("project");
+
+define("PASSWORD", "<ROTATED - retrieve on the guest>");
+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" => "<span class=\"user\">$activeUser</span>:<span class=\"path\">" . exec("pwd") . "</span>$ "
+);
+
+Common::send("success", $output);
+
+?>