fix: rDelete fatals on musl (GLOB_BRACE) - broke ALL deletion and plugin removal #4
+101
-21
@@ -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.
|
||||
|
||||
@@ -1,155 +0,0 @@
|
||||
<?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);
|
||||
|
||||
?>
|
||||
@@ -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) {
|
||||
@@ -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"
|
||||
|
||||
Reference in New Issue
Block a user