diff --git a/docs/scratchpads/AMD1213-D-transaction-and-helper-hardening.md b/docs/scratchpads/AMD1213-D-transaction-and-helper-hardening.md new file mode 100644 index 00000000..893a546a --- /dev/null +++ b/docs/scratchpads/AMD1213-D-transaction-and-helper-hardening.md @@ -0,0 +1,125 @@ +# AMD1213-D — transaction and helper trust remediation + +- **Task:** AMD1213-D (issue #1213 amendment; controlling packet `comms/20260813T212447Z__from-scrappy__dc43de.md`) +- **Objective:** Address D1–D6 on local `feat/wf-fleet-mvp`, commit-only. Never push or re-author. +- **Scope:** Existing C-fence production/tests only. No provider calls. +- **Standing constraint:** AMEND/HOLD. Do not push, do not update PR #1213, do not merge, do not + re-author. PR #1216 remains independently held for Jason. + +## Where this actually stands (measured 2026-08-15, not inherited from notes) + +Everything below was re-measured against the tree rather than trusted from the previous entries, +which understated progress by roughly two defects. Branch head `b91b702a`. + +| Defect | State | Evidence | +|---|---|---| +| D1 transactional rollback | Substantially implemented | `fleet-launch-command.ts` snapshots links before mutating, prepares the manifest, and has `injectFailure` seams at `prepare-manifest`, `write-manifest`, `close-manifest`, `rename-manifest`; restores `manifestLinksBefore` on failure | +| D2 exact managed-link classes | Substantially implemented | classifier at `fleet-launch-command.ts:598-663`: exact resolved credential, direct one-component plugin/skill only, symlink target and ancestor rejected, `realpath` containment, duplicates rejected | +| D3 ambient-PATH executable resolution | **Closed this pass** (commit `585dac7a`) | see below | +| D4 config check/apply safety | Substantially implemented | `secure_dir` ancestor checks, `read_private` with `O_NOFOLLOW` + fstat, apply via mkstemp + fchmod 0600 + fsync + dev/ino re-check before `os.replace`, compatibility path separated | +| D5 bounded test seam | **Closed this pass** (commit `b91b702a`) | see below | +| D6 validate-by-path then exec-by-path | Substantially implemented | helper runs as a verified snapshot piped to `bash -s`, not executed by pathname; the same binding applied to the runtime in D3 | + +### D2 — one thing worth recording so it is not "fixed" later + +The alias concern in the packet (`duplicates/normalization aliases`) is closed by strictness, not +by normalization. `dirname(link)` is compared literally against the seat root, so `/s/plugins//foo` +(`dirname` → `/s/plugins/`), `/s/plugins/./foo` and `/s/plugins/bar/../foo` all fail the comparison +and are rejected. Verified by direct measurement of `path.dirname` on each form. Anyone who +"improves" this by normalizing the link first would open the alias hole the strict comparison +currently closes. + +### D3 — what was wrong and what was done + +The launcher asked `which` whether a runtime was reachable and then spawned the **bare name**, +letting the OS resolve it a second time against an ambient PATH at a later moment. Two independent +resolutions of an attacker-influenced name with a gap between them. + +Demonstrated against the old code before changing it — a world-writable `codex` shim prepended to +PATH: + +``` +OLD checkRuntime -> PASSED (which found it) +OLD execRuntime -> "SHIM EXECUTED — this is not the real runtime" +``` + +Three exposed call sites, not one: `checkRuntime`'s `which`; `execRuntime` spawning +`codex`/`opencode` by name; and `execLeaseGatedRuntime` spawning `python3` by name — the +interpreter that starts the lease gate, where a shim replaces the process that enforces every other +check. `minimalLaunchEnv` copies ambient PATH straight through. + +Fix: `resolveExecutableFromPath` searches only the PATH the child will actually receive, validates +what the search lands on (regular file, executable, not group/other-writable, owned by the +launching user or root, no group/world-writable non-sticky directory and no foreign-owned directory +on the resolved path), and returns that path pinned to dev/ino. Callers execute the returned path +and never the name again. The fleet lease-gate interpreter comes from the root-owned +`trustedCapability('python3')`. `checkRuntime` is deliberately kept on the operator path, where +"is it reachable from my shell" is the right question. + +**Residuals, stated not engineered around:** + +1. `assertUnchangedSinceValidation` re-confirms dev/ino immediately before spawn. That narrows the + validation→exec window; it does not close it. Closing it means exec by held descriptor, which + Node cannot do portably. Same accepted boundary already documented for the fleet helper. +2. For claude and pi the runtime binary is still re-resolved inside `launch-runtime.py` after the + trusted interpreter starts it. **This change does not cover that path.** + +12 tests, one per hole. One was written wrong first and is worth remembering: +`mkdirSync(path, { mode: 0o777 })` is masked by the umask to 0o755, so the world-writable-directory +case passed while testing nothing. Create at 0o755, then `chmodSync`. + +### D5 — what remained and what was done + +Most of D5 was already closed: `launchFleetRuntimeForTest` is gone, specs enter through the real +`registerFleetLaunchCommand → apply → launchFleetRuntime → launchRuntime` route on a fixture seat, +the ledger points at the fixture and **is** asserted, and the seat-seeded/HOME-empty pass plus +HOME-seeded/seat-empty fail pair both exist. + +What remained was the dead `recordLaunch?: boolean` context field. Nothing in the package set it; +its only effect was to let a caller silently disable recording on the claude branch while codex, +opencode and pi recorded unconditionally. Removed. + +## Not part of D1–D6, fixed because it blocked the required evidence + +The amend requires a green full-package Vitest run. +`install-ordering-guard.spec.ts > defaults to the real leaseEnforcementActivatable()` made that +non-reproducible. `defaultCapabilityProbe` executes `dist/cli.js` out-of-process with a **2000 ms +timeout**; in a full run with 86 spec files scheduled at once, one observation beats the timeout and +the next does not, so the test's two observations of the same predicate disagree and it fails — +reporting machine load as a wiring defect. Passed 3/3 in isolation, failed in three consecutive +full runs. + +Ruled out my own change by reverting only the `recordLaunch` edit and re-running: still failed. +The guard call is now bracketed by two observations, only an agreeing pair is used as ground truth, +a disagreeing pair is retried up to three times, and never holding still is a failure rather than a +skip. Falsified by inverting the guard's default to `!leaseEnforcementActivatable()` → red +(1 failed / 18 passed), then reverted. + +## Verification state + +- typecheck RC=0. +- Full package Vitest, sanitized lease env (`MOSAIC_LEASE_*` + `MOSAIC_RUNTIME_GENERATION` + stripped): **86 files / 1619 tests passed, 0 failed**, three consecutive runs, RC=0. +- Without that sanitization the suite shows 4 failures in `mutator-gate.acceptance.spec.ts`. Those + are the known host lease-identity leak into spawned hooks, **not** a product defect — the same + spec re-run with only those five variables stripped and no code change is 20/20. The standing fix + is the unpushed `fix/lease-test-env-isolation` branch (blocked on the identity blocker below). + +## Still open + +- **Remaining D3 surface:** the packet also asks for a capability-minimal *measured child + environment* — construct child env from scratch, fixed locale, exact vars only, and add + `BASH_ENV`/`ENV`, `PYTHON*`, `NODE_*`, `NPM_CONFIG_*`, unexpected-env-name killers. The + executable-resolution half is closed; the environment half is not. +- **D1/D2/D4/D6 need confirmation, not assumption.** They read as substantially implemented but I + have not run the packet's full RED→GREEN control matrix against each seam. +- Required next evidence per the packet: all D1–D6 observed RED→GREEN controls, framework-shell, + build/lint/Prettier/bash -n, fresh current-next merge-tree. + +## Blocker not solvable inside this branch + +No `fred` principal exists (`tea login list` has no entry; `MOSAIC_GIT_IDENTITY` never reaches the +pane). The only push path on this host is the **retired** mos-dt-0 token. That is why this work is +commit-only beyond scrappy's instruction — even after the hold lifts, the truthful authenticated +push the packet requires cannot be made under a correct identity yet. Raised with mos-claude and +with Jason; awaiting a mint decision.