diff --git a/docs/known-issues.md b/docs/known-issues.md index a516b75..11863dc 100644 --- a/docs/known-issues.md +++ b/docs/known-issues.md @@ -9,7 +9,79 @@ Pinned Atheos commit: `f90cd1806dbe0e25673a77e27390e8b1a240c3f6` (build 611). --- -## 1. ๐Ÿ”ด Plugin market: the **Terminal** plugin does not work as shipped +## 1. ๐Ÿ”ด **Deleting any file or folder was broken** โ€” `GLOB_BRACE` does not exist on musl + +**Symptom:** removing the Terminal plugin from the market **cycled forever in the UI** (never +completed, never errored). Deleting a file or folder in the editor was broken by the same cause. + +**Root cause โ€” a glibc-only constant referenced on musl.** `traits/helpers.php::rDelete()`: + +```php +$files = glob($target . "{*,.[!.]*,..?*}", GLOB_BRACE|GLOB_MARK); +``` + +`GLOB_BRACE` is a **glibc extension**. Alpine uses **musl**, where the constant is *undefined* โ€” +and touching an undefined constant is a **fatal error** in PHP 8 (a warning in older versions): + +``` +PHP Fatal error: Uncaught Error: Undefined constant "GLOB_BRACE" +in /var/www/atheos/traits/helpers.php:138 +#0 components/market/class.market.php(178): Common::rDelete() +#1 components/market/controller.php(46): Market->remove() +``` + +Because the fatal kills the request before `Common::send(200)`, the front end never receives a +settle response โ€” so the UI **spins indefinitely**. A hang, not an error. + +**Blast radius โ€” measured, two call sites:** + +| Caller | What breaks | +|---|---| +| `components/market/class.market.php:178` | removing a plugin from the market | +| `components/filetree/class.filetree.php:59` | **deleting any file or folder in the editor** | + +The second is core editor functionality, so this is **not** a plugin bug โ€” installing the Terminal +plugin is merely what made it visible. Verified directly: with the original code, `rDelete()` fatals +and the target is left on disk. + +**Fix applied** (`patches/helpers-rDelete-GLOB_BRACE.diff`) โ€” expand the brace pattern by hand when +`GLOB_BRACE` is absent, keeping upstream's exact behaviour where it *is* defined: + +```php +if (defined("GLOB_BRACE")) { + $files = glob($target . "{*,.[!.]*,..?*}", GLOB_BRACE | GLOB_MARK); +} else { + $files = array_merge( + glob($target . "/*", GLOB_MARK) ?: [], + glob($target . "/.[!.]*", GLOB_MARK) ?: [], + glob($target . "/..?*", GLOB_MARK) ?: [] + ); +} +``` + +The three patterns are precisely what the brace expansion produces (`*`, dotfiles, and `.`/`..`-like +names), so the directory listing is equivalent. + +**Verified** โ€” `php83 -l` clean, then a real deletion of a tree containing nested dirs and two +dotfiles: + +``` +entries before: 10 +rDelete() returned: true +exists after: gone (OK) +single file: rDelete=true exists=gone (OK) +``` + +**Upstream has NOT fixed this** โ€” the same line is still in `main` (checked directly), so it cannot +be resolved by updating Atheos. Re-apply this patch after any update (`scripts/update-atheos.sh`). + +**Consequence for the platform choice:** this is the real cost of Alpine. A glibc-based image +(Debian/Ubuntu) would not have hit it. It is patchable in one place, so Alpine stays โ€” but it is worth +knowing that `GLOB_BRACE` is the kind of thing musl breaks. + +--- + +## 2. ๐Ÿ”ด Plugin market: the **Terminal** plugin does not work as shipped **Symptom:** installed the Terminal plugin from the market; it failed / did not function. @@ -86,17 +158,30 @@ Because of (1) and (2), the in-app password is treated as a **secret**: - 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. +**Outcome: the plugin has been REMOVED.** Once the `GLOB_BRACE` fault (ยง1) was fixed, removal through +the market succeeded โ€” and that is the right end state: a browser shell is the single largest attack +surface in the container, and **Atheos macros already provide command execution**, so it added little +you did not already have. Removal was done by invoking the market's own `remove()` path, so the plugin +list, the `addons` cache and the generated bundle all stayed consistent: + +``` +{"text":"Plugin successfully removed.","status":200} +plugins/ -> (only README.md remains) +addons cache references to Terminal: 0 +``` + +The notes above are kept because they document what the plugin *is* and how it failed โ€” and because +the same stale-API pattern will recur with any other Codiad-era plugin. To reinstall, use the +market: it will re-introduce the `Common::execute` fault, so `patches/Terminal-terminal.php.patched` +was the fix โ€” **it has been deleted along with the plugin**. If you ever reinstall, re-derive it: +`grep -rhoE 'Common::[a-zA-Z_]+' plugins/Terminal/` and expect `Common::execute` to be missing. `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 +## 3. ๐ŸŸ  The Macro dialog crashed when folder-scoped macros existed **Symptom:** the Settings โ†’ Macro dialog returned HTTP 500. @@ -139,7 +224,7 @@ Worth doing if the dialog starts 500-ing again after editing a macro in the UI. --- -## 3. Checking for plugin problems in future +## 4. 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 @@ -173,22 +258,17 @@ verify them before trusting it. --- -## 4. Follow-ups not done (recorded so they are not forgotten) +## 5. 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) +### 5.1 Remove the Terminal plugin โ€” โœ… DONE -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. +Removed through the market's own code path once ยง1 was fixed (see ยง2). A browser shell was the largest +attack surface in the container and macros already cover command execution. -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 +### 5.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: @@ -205,18 +285,18 @@ turns the editor into a text editor that cannot commit. So it stays **a delibera 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 +### 5.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 +### 5.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 +### 5.5 The real upstream fix for ยง3 -`class.macro.php::save()` should write `[]` instead of `"N/A"` for folder macros. See ยง2 โ€” the fix is +`class.macro.php::save()` should write `[]` instead of `"N/A"` for folder macros. See ยง3 โ€” the fix is currently applied to the data, so it survives updates, at the cost of being reversible from the UI. diff --git a/patches/Terminal-terminal.php.patched b/patches/Terminal-terminal.php.patched deleted file mode 100644 index 395bb5d..0000000 --- a/patches/Terminal-terminal.php.patched +++ /dev/null @@ -1,155 +0,0 @@ -"); -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 diff --git a/patches/helpers-rDelete-GLOB_BRACE.diff b/patches/helpers-rDelete-GLOB_BRACE.diff new file mode 100644 index 0000000..47932fa --- /dev/null +++ b/patches/helpers-rDelete-GLOB_BRACE.diff @@ -0,0 +1,22 @@ +--- /var/www/atheos/traits/helpers.php.orig ++++ /var/www/atheos/traits/helpers.php +@@ -135,7 +135,18 @@ + + if (is_dir($target)) { + +- $files = glob($target . "{*,.[!.]*,..?*}", GLOB_BRACE|GLOB_MARK); //GLOB_MARK adds a slash to directories returned ++ // GLOB_BRACE is a glibc extension and is UNDEFINED under musl (Alpine), where ++ // referencing it is a fatal error in PHP 8. Expand the brace pattern by hand ++ // so this works on both libcs. ++ if (defined("GLOB_BRACE")) { ++ $files = glob($target . "{*,.[!.]*,..?*}", GLOB_BRACE | GLOB_MARK); ++ } else { ++ $files = array_merge( ++ glob($target . "/*", GLOB_MARK) ?: [], ++ glob($target . "/.[!.]*", GLOB_MARK) ?: [], ++ glob($target . "/..?*", GLOB_MARK) ?: [] ++ ); ++ } + // $files = glob($target . "/*"); + + foreach ($files as $file) { diff --git a/scripts/build-and-test.sh b/scripts/build-and-test.sh index b4105c8..44b8b88 100644 --- a/scripts/build-and-test.sh +++ b/scripts/build-and-test.sh @@ -108,7 +108,29 @@ done echo echo "==============================================================" -echo "GUARD: is master PROTECTED on the blog repo? (the writer-branch model depends on it)" +echo "REGRESSION: rDelete works (GLOB_BRACE is glibc-only; musl lacks it)" +echo "==============================================================" +# traits/helpers.php::rDelete() used GLOB_BRACE, which is undefined under musl -> PHP 8 fatal. +# That broke deleting ANY file/folder in the editor AND removing a market plugin (the UI +# would spin forever because the fatal killed the response before Common::send()). +# docs/known-issues.md ยง1. This test fails loudly if the patch is lost (e.g. by an update). +rm -rf /tmp/rdel_guard 2>/dev/null +if mkdir -p /tmp/rdel_guard/a/b/c /tmp/rdel_guard/.hd 2>/dev/null \ + && touch /tmp/rdel_guard/f.txt /tmp/rdel_guard/.dot /tmp/rdel_guard/a/b/c/deep.txt; then + ROUT=$(php83 -r 'require_once("/var/www/atheos/common.php"); echo Common::rDelete("/tmp/rdel_guard/") ? "ok" : "fail";' 2>&1 | grep -viE 'deprecat|warning|notice' | tail -1) + if [ "$ROUT" = "ok" ] && [ ! -e /tmp/rdel_guard ]; then + ok "rDelete removed a nested tree incl. dotfiles (delete works)" + else + if echo "$ROUT" | grep -q 'GLOB_BRACE'; then + bad "rDelete FATALS on GLOB_BRACE - re-apply patches/helpers-rDelete-GLOB_BRACE.diff" + else + bad "rDelete failed (got: $ROUT)" + fi + fi +else + bad "could not build the rDelete test tree" +fi +rm -rf /tmp/rdel_guard 2>/dev/null echo "==============================================================" # The whole branch-based workflow assumes the deploy key CANNOT push master. That is a # repo-side setting we cannot verify from here by inspection - so test it the way an @@ -128,10 +150,22 @@ fi echo echo "==============================================================" -echo "FR-11 GUARD: agent created NO identity or armistace credential" +echo "IDENTITY: git identity is SET by the operator (FR-11 superseded by explicit request)" echo "==============================================================" -chk "git user.name empty" "$(asuser 'git config --get user.name' 2>/dev/null)" "" -chk "git user.email empty" "$(asuser 'git config --get user.email' 2>/dev/null)" "" +# FR-11 originally required the agent to leave the identity EMPTY so it could not impersonate +# the operator. The operator has since explicitly asked for it to be set, so the requirement is +# superseded - and the guard is inverted: the identity must now be present and must be the +# OPERATOR's, not an agent-invented one. Guarding the old expectation would assert the opposite +# of the current design and fail forever (which is what it did). +NAME=$(asuser 'git config --get user.name' 2>/dev/null) +MAIL=$(asuser 'git config --get user.email' 2>/dev/null) +if [ -n "$NAME" ]; then ok "git user.name set ($NAME)"; else bad "git user.name is empty - macro 2 will refuse to commit"; fi +if [ -n "$MAIL" ]; then ok "git user.email set ($MAIL)"; else bad "git user.email is empty"; fi +case "$MAIL" in + *hermes*|*bot*|*noreply@anthropic*|*agent*) bad "identity looks agent-generated: $MAIL" ;; + "") ;; + *) ok "identity is an operator address, not an agent-invented one" ;; +esac if [ -f /home/atheos/.ssh/id_ed25519_blog ]; then ok "deploy key present"; else bad "deploy key missing"; fi chk "deploy key mode" "$(stat -c '%a' /home/atheos/.ssh/id_ed25519_blog 2>/dev/null)" "600" chk "deploy key owner" "$(stat -c '%U' /home/atheos/.ssh/id_ed25519_blog 2>/dev/null)" "atheos"