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.
This commit is contained in:
2026-09-29 16:22:25 +10:00
parent 8ea645386f
commit 976b3f0005
+50
View File
@@ -170,3 +170,53 @@ 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 **Rule of thumb:** if a plugin was ported from Codiad, assume its `Common::` calls are stale and
verify them before trusting it. 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.