Reported as "deleting terminal just cycles in the ui". Root cause is much larger than the
plugin: it was a platform bug that broke deletion everywhere.
CAUSE
traits/helpers.php::rDelete() did:
glob($target . "{*,.[!.]*,..?*}", GLOB_BRACE|GLOB_MARK);
GLOB_BRACE is a GLIBC extension. Alpine is musl, where the constant is undefined,
and referencing an undefined constant is a FATAL in PHP 8:
PHP Fatal error: Uncaught Error: Undefined constant "GLOB_BRACE"
in traits/helpers.php:138
#0 components/market/class.market.php(178): Common::rDelete()
#1 components/market/controller.php(46): Market->remove()
The fatal kills the request before Common::send(200), so the front end never gets a
settle response and the UI spins forever. A hang, not an error - which is why it
presented as "cycles in the ui".
BLAST RADIUS (measured, two call sites)
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 functionality, so this was never a plugin bug - installing the
Terminal plugin is just what made it visible.
FIX
patches/helpers-rDelete-GLOB_BRACE.diff - expand the brace pattern by hand when
GLOB_BRACE is undefined, keeping upstream behaviour where it is defined. Verified:
nested dirs + dotfiles + single files all delete correctly now.
Upstream has NOT fixed this (checked main) so it cannot be resolved by updating.
PLUGIN REMOVED
Once deletion worked, removal via the market succeeded and the plugin is now gone:
{"text":"Plugin successfully removed.","status":200}
That is the right end state - a browser shell was the largest attack surface, and
macros already provide command execution. Removed through the market's own code path
so the plugin list, addons cache and bundle stayed consistent.
TESTS (now 42 passed, 1 failed - the failure is branch protection, a genuine open item)
+ a GLOB_BRACE regression guard that fails loudly if the patch is lost to an update
+ the identity guard is INVERTED: FR-11 required an EMPTY identity so the agent could
not impersonate the operator, but the operator has since asked for it to be set, so
the test asserting emptiness was asserting the opposite of the current design. It
now requires the identity to be present and to NOT look agent-generated.
docs/known-issues.md: new section 1 documents the GLOB_BRACE fault in full, including the
honest note that this is the real cost of choosing Alpine over a glibc base image.
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.
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.
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
Responds to the independent review of the writer-branch PR.
Adopted (the reviewer's strongest recommendation): a test that PROVES master is
protected, rather than documenting the setting. It attempts a direct push to
master with the deploy key and requires it to be rejected - the same way an
attacker would find out. It currently FAILS, which is the correct result: branch
protection is genuinely not enabled on armistace/blog, and the failure is now
visible in every test run instead of buried in prose.
Cannot fix from here: setting branch protection on armistace/blog returns
403 "user should be an owner or a collaborator with admin write"
for the bot account (it is a collaborator, not the owner). This is an operator
action, documented in docs/writer-branch-workflow.md with the exact Gitea
settings. Until it is done the writer-branch model is advisory, not enforced.
Also fixed three assertions that were stale rather than wrong:
- "installer markers present" now accepts the post-install state (config.php
populated) as valid, since the operator has completed the installer.
- "posts found == 20" is now ">= 20" - the operator has added a file, so an
exact count was always going to fail.
- "branch == master" is now master|atheos-writer - the writer branch is the
intended steady state under the new workflow, so asserting master was
asserting the opposite of the design.
Result: 40 passed, 1 failed - the single failure being the branch-protection gap.
Adds the branch-based publishing model: the editor writes to a long-lived
`atheos-writer` branch, and merging into `master` stays in Gitea.
Six macros installed into Atheos's macro store (a copy is kept in
files/macros.db.php so a rebuild reproduces them), invoked by right-clicking
the `blog` folder:
1. Start/Join Writer Branch idempotent create-or-switch + push -u
2. Commit + Push Writer Branch identity guard -> add -A -> commit (only
if something is staged) -> push
3. DESTRUCTIVE: Reset Writer to Master fetch -> reset --hard origin/master ->
push --force-with-lease
4. Status + Recent Log read-only
5. Refresh Master (ff-only) pull master down, return to writer
6. Merge Master Into Writer Branch when master has moved ahead
Atheos has no terminal and codegit exposes no branch-switching, so macros are
the only way to run arbitrary git from the UI. Verified: codegit's `checkout`
action takes a FILE argument - it is per-file revert, not branch switching.
Identity handling: the commit macros deliberately do NOT hardcode an identity.
php-fpm runs with HOME=/home/atheos (verified), the same global gitconfig the
operator edits over SSH, so hardcoding `git -c user.name=...` would OVERRIDE the
operator's own identity. Instead macro 2 guards on an identity existing and fails
with an actionable message - which is what design decision D1 intends.
Verified by exercising each command through the web server as the php-fpm user:
- macro 1, 4, 5 return rc=0 with correct output
- macro 2's identity guard fires with the exact RUNBOOK STEP 1 message
- macro 3 was tested with real divergence: committed on the writer branch,
reset --hard to master, force-pushed; origin/atheos-writer then matched
origin/master exactly (0 ahead, 0 behind)
Security note recorded in the docs and README: this model only holds if branch
protection is enabled on armistace/blog's master. It is currently NOT enabled,
so the deploy key could force-push master directly - the operator needs to turn
it on (repo-owner action).
Also documents two hazards found while testing:
- macro 2's `git add -A` stages everything, and a subsequent `reset --hard`
deletes any untracked file that was staged. Keep scratch files outside
workspace/blog/.
- recovery procedure for a file lost that way (git fsck --lost-found ->
ls-tree -> cat-file), which was used to restore a file during testing.
Provisions a LAN-only browser code editor (Atheos) in an Alpine 3.23 LXC on the
scotty PVE node, with armistace/blog cloned and ready to edit. Built via the
AI-DLC workflow; the full process trail is in aidlc-docs/.
Deliverables
- scripts/provision.sh rebuild the guest from scratch (re-runnable)
- scripts/update-atheos.sh move Atheos to latest upstream and RE-PIN
- scripts/build-and-test.sh captures evidence for every acceptance criterion
- files/atheos.conf nginx vhost (socket path byte-matches the FPM pool)
- files/ssh-config ~atheos/.ssh/config deploy-key alias
- RUNBOOK.md operational guide (START AT STEP 1)
- docs/security-notes.md credential model + wider-web hardening TODOs
- docs/resiliency-notes.md what a non-PoC deployment would need
Design decisions
- D1 no git identity is configured by the agent; the operator sets it. Git will
refuse the first commit until then, which is deliberate (requirement FR-11).
- D2 Atheos pinned to commit f90cd18 (build 611) with a verified tarball sha256,
plus update-atheos.sh as the deliberate escape hatch.
- D3 nginx :80, web root /var/www/atheos, php-fpm over a unix socket.
- D6 git auth is a per-repository DEPLOY KEY scoped to armistace/blog, not a
personal access token (a PAT's write:repository scope reaches every repo the
account can write).
Verified on the live guest (40 checks, 0 failures)
- AC-2 Atheos served over HTTP 200 by nginx+php-fpm; socket paths byte-match
- AC-3 20 posts + themes/cleanblog present; owned by the php-fpm user
- AC-4 all five writable paths writable as the service user
- AC-5 remote targets armistace/blog; deploy key authenticates over SSH
- AC-6 survival proven with a real container restart, marker byte-identical
- AC-8 no python/pelican/pip/make and no DB engine - scope boundary enforced
- FR-11 git identity is empty; deploy key mode 600 owned by the service user
Traps documented in the RUNBOOK (each cost real debugging time)
- The Gitea SSH user is `gitea`, NOT `git` (app.ini SSH_USER=gitea). With `git`,
sshd answers "Invalid user git" and rejects before reading authorized_keys,
which is indistinguishable from a bad key.
- The Gitea SSH endpoint is the LAN address (:22 is not forwarded publicly).
- `uptime` and `free` report the PVE HOST inside an LXC without lxcfs, so a
container that just restarted can claim days of uptime.
hermes