c4a4de1da7271f2530aa6b92b1b6a2b0e9b81fbd
4
Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 hermes
|
c4a4de1da7 |
fix: rDelete fataled on musl (GLOB_BRACE) - broke ALL deletion and plugin removal
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.
|
||
|
hermes
|
976b3f0005 |
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.
|
||
|
hermes
|
8ea645386f |
fix: rotate the terminal plugin password, remove it from the docs
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.
|
||
|
hermes
|
5ff6a06e82 |
feat: document plugin failures and fix the Terminal plugin + macro dialog crash
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
|