From bc845badf1733b22d03f3d1da3dfbd20a00c8f76 Mon Sep 17 00:00:00 2001 From: kisfenyo Date: Sat, 13 Jun 2026 17:53:17 +0200 Subject: [PATCH] audit: Tier-3 (templates clean, BUGHUNT concurrency re-check) + finalize - Templates/funcmap/XSS/no-:latest all CLEAN; 5/6 surviving BUGHUNT concurrency items FIXED - CTRL-T3-1 (Low): backup.Manager.stackProvider unlocked read (M2 still present, benign) - AGENT-T2-1 upgraded to verified-by-test (agent branch evidence test) - Final report: exec summary, top-10, invariants, contracts (CLEAN), dead-code, test-gaps, not-covered Co-Authored-By: Claude Opus 4.8 (1M context) --- AUDIT-2026-06-13.md | 36 +++++++++++++++++++++++++++--------- 1 file changed, 27 insertions(+), 9 deletions(-) diff --git a/AUDIT-2026-06-13.md b/AUDIT-2026-06-13.md index 1b8b4fc..cad3157 100644 --- a/AUDIT-2026-06-13.md +++ b/AUDIT-2026-06-13.md @@ -22,7 +22,9 @@ - 17:55 — Wrote + ran failing evidence test `internal/appexport/traversal_audit_test.go` (CTRL-001). FAILS as expected. Committed + force-pushed Tier-1 checkpoint (`1f06029`). - 18:15 — Tier 2 dispatched (3 parallel): controller stacks lifecycle, agent reconcile crash-safety + provision + proxmox/pbs, agentapi↔localapi contract diff. All returned. - 18:30 — **Verified by hand**: CTRL-T2-1 (ghost-deployed on crash) — read deploy.go:288-358, confirmed `app.yaml` persists `Deployed:true` at :302 before `compose up -d` at :339; `Deploying` is in-memory only. BUGHUNT H1/H2/H3/H4/H12 all confirmed FIXED. agentapi↔localapi contract = CLEAN (12 endpoints diffed). Committed Tier-2. -- (next) Tier 3: templates/funcmap/XSS, goroutine-lifecycle races in surviving pkgs, `-race` run on build server, no-`:latest` sweep. +- 18:55 — Tier 3 dispatched (templates/funcmap/XSS + no-`:latest` + surviving BUGHUNT concurrency re-check). Returned very clean: templates correct, 5/6 BUGHUNT concurrency items FIXED (only M2→CTRL-T3-1 lingers, benign), scheduler panic-isolation present. +- 19:05 — Wrote + ran failing evidence test `agent: internal/proxmox/upid_audit_test.go` (AGENT-T2-1, empty-node UPID). FAILS as expected. Folded Tier-3 into report. Final commit + push (both branches). +- DONE for this session. Resume point if continued: deeper read of `selfupdate`/`integrations`/`metrics`/`cloudflare` (controller) + `lanresolver`/`hub`/`desired` (agent); `-race` on build server; live read-only inspection. ## Baseline (Phase 0) @@ -392,10 +394,10 @@ Verify: Point an orphan's bind at a slow FUSE mount; call delete; handler bl Severity: Low Category: contract-mismatch Location: agent/internal/proxmox/task.go:70-81,110-114; upid.go:29-60 (commit d17b5ab) -Confidence: verified-static -Evidence: `ParseUPID` accepts an empty `parts[1]` → `u.Node==""` → request to `/nodes//tasks/...`; PVE queries `u.Node` not its pinned `c.node`. The sibling PBS client guards `node==""` (client.go:128-132); PVE does not. In Recover an unreadable status is treated fail-safe (left in-flight), bounding blast radius. +Confidence: **verified-by-test** (agent branch: `internal/proxmox/upid_audit_test.go`) +Evidence: `ParseUPID` (upid.go:50-59) sets `Node: parts[1]` with no empty/validity check → an empty-node UPID parses cleanly (`u.Node==""`) → `TaskStatusOnce` requests `/nodes//tasks/...` and queries `u.Node` not its pinned `c.node`. The sibling PBS client guards `node==""`; PVE does not. In Recover an unreadable status is treated fail-safe (left in-flight), bounding blast radius. Fix sketch: After `ParseUPID`, reject `u.Node==""` and assert `u.Node==c.node`, mirroring PBS. -Verify: `TaskStatusOnce(ctx,"UPID::0:0:0:x:9:root@pam:")` → expect a node-validation error. +Verify: `cd /e/git/felhom-agent && go test ./internal/proxmox/ -run RejectsEmptyNode -v` → FAILS at this commit (empty node accepted). ### [AGENT-T2-2] `WaitTask` treats empty task status as "running" → burns full 10m timeout on a persistently-empty status Severity: Low @@ -433,6 +435,20 @@ Evidence: The bring-up rollback `defer` fires on panic (guest is destroyed — Fix sketch: Wrap `t.fn()` in `lane.run` with `defer recover()` converting a panic into an error on `t.res`, isolating it to that op. Verify: Submit a panicking job; assert the queue delivers an error and other lanes keep running. +### [CTRL-T3-1] `backup.Manager.stackProvider` read without the mutex that guards its write (BUGHUNT M2 — still present, benign) +Severity: Low +Category: concurrency +Location: controller/internal/backup/backup.go:89,122,259,331,400,491 (reads); :392 (locked write) (commit eea235b) +Confidence: verified-static +Evidence: +```go +func (m *Manager) SetStackProvider(p StackDataProvider) { m.mu.Lock(); m.stackProvider = p; m.mu.Unlock() } // :392 +if m.stackProvider != nil { ... m.stackProvider.GetStackHDDPath(stackName) ... } // :89 unlocked read (×11) +``` +Mechanism: The single write is locked (with a comment claiming concurrent reads) but all 11 reads are unlocked. Compensating control: `main.go:225` calls `SetStackProvider` exactly once during single-threaded init, before the scheduler/HTTP server start → write happens-before all reads. Latent: a second runtime call would introduce a real data race; the locked write + "concurrent" comment is misleading. +Fix sketch: Drop the lock on the init-only write, OR add a locked `getStackProvider()` accessor and route the 11 reads through it. +Verify: `go test -race ./internal/backup/...` with a test calling SetStackProvider concurrently with a read; or confirm it stays init-only. + --- ## Findings — Info @@ -541,6 +557,10 @@ The agentapi package exports `PhaseIdle/Running/Done/Failed` but not `PhaseSnaps - Agent provision invariants: no `pct exec` — **HOLDS** (grep clean); bootstrap.json chown 100000:100000 — **HOLDS** @ backhalf.go:32,137. - Agent crash-safety: marker-before-mutate — **MOSTLY HOLDS** (OpStarted pre-POST, fsync'd; task-id marker post-POST → AGENT-T2-3 Low); `Recover()` ground-truth via live `ListLXC` — **HOLDS** @ recover.go:103,163; defer-unquiesce/rollback — **HOLDS** @ bringup.go:182-187 (lane-panic isolation gap AGENT-T2-6). PBS fingerprint pinning — **HOLDS** @ pbs/pin.go:22-48. +**Tier-3 results (templates + surviving BUGHUNT concurrency, controller @ eea235b):** +- Templates/funcmap — **CLEAN.** Every template func invoked is registered (no parse-panic risk); no XSS bypass (only raw-HTML sites are `csrf.go:98` with explicit `HTMLEscapeString` and the `json` func returning `template.JS` via `json.Marshal`'s default HTML-escaping — `` breakout neutralized); all 11 container states resolve in `stateColor`/`stateLabel`; **no `:latest`** anywhere (traefik:v3.6.7, cloudflared:2026.6.0, filebrowser:1.3.3-stable pinned); no emoji, no dead `{{define}}` blocks. (Several *unused* funcmap entries — `stateIcon, stateStr, statusText, seq, shortID, fmtDuration, pruneLabel, nextPruneLabel` — harmless dead code.) +- Surviving BUGHUNT concurrency: **M10** (scheduler late-registration) — **FIXED** @ scheduler.go:94-97,123-126; **M2** (stackProvider unlocked read) — **STILL-PRESENT** → CTRL-T3-1 (Low, benign); **M3** (DrainPendingEvents loss) — **FIXED** @ settings.go:946-950; **M22** (assets mutex during download) — **FIXED** @ syncer.go:79-91; **M13** (WAL not verified) — **FIXED** @ store.go:27-34; **M14** (sampleContainers context.Background) — **FIXED** @ collector.go:99-100. Scheduler **panic isolation PRESENT** @ scheduler.go:268-276 (each job under `defer recover()`). + ## Refactor & shared-code opportunities - **Durable-resolve-before-mutate helper (agent):** AGENT-001/002 + AGENT-007 all stem from formatting/recording a device by a *path/string* instead of re-resolving the *durable id* at the moment of action. A single `resolveAndReinspect(durableID) (path, probe, error)` used by both the inline localapi format path and `WipeExecutor` would make "act on exactly the confirmed device" structural. @@ -563,6 +583,7 @@ The agentapi package exports `PhaseIdle/Running/Done/Failed` but not `PhaseSnaps - `internal/system/info.go:11` `debugf` — unused. - `internal/web/alerts.go:233` `countLevel` — unused. - `internal/web/handlers.go:1083` `(*Server).countAppsUsingPath` — unused. +- `internal/web/funcmap.go` — unused registered template funcs: `stateIcon, stateStr, statusText, seq, shortID, fmtDuration, pruneLabel, nextPruneLabel` (registered but never invoked by any template; harmless dead funcmap entries). - (agent: staticcheck clean — no dead-code reports.) ## Session notes, assumptions, open questions @@ -576,11 +597,8 @@ The agentapi package exports `PhaseIdle/Running/Done/Failed` but not `PhaseSnaps ## What was NOT covered (defines next session) -Tiers 1 & 2 are complete for both repos + both cross-repo contracts. NOT yet done (defines the next session, roughly in priority order): -- **Tier 3 templates (controller):** funcmap completeness (every template func referenced exists), XSS via any raw-HTML/`json` escape of user-influenced data, every container state has a label/color/icon, leftover emoji vs the minimal-UI rule, dead template blocks. `internal/web/funcmap.go` + `templates/`. -- **Tier 3 cross-cutting concurrency:** goroutine/ticker lifecycle & shared-state races in the surviving pkgs. BUGHUNT flagged several still-live ones NOT re-checked this session — **M10 scheduler late-registration** (jobs added after Start() never run), **M2 stackProvider unlocked read**, **M3 DrainPendingEvents loses events on save fail**, **M22 assets syncer holds mutex during network I/O**, **M13 SQLite WAL not verified**, **M14 sampleContainers uses context.Background**. Confirm FIXED/PRESENT at eea235b. -- **Tier 3 panic isolation:** does one bad scheduler/loop job kill the process? (agent lane-panic AGENT-T2-6 is the analogue — check controller `scheduler.go`.) -- **`-race` run:** not executed (needs the build server; CGO/sqlite won't `-race` on this Windows box). Recommended next session in a throwaway `mktemp -d` on `kisfenyo@192.168.0.180`, then delete. +Tiers 1, 2 AND the Tier-3 templates + surviving-BUGHUNT-concurrency re-check are complete for both repos + both cross-repo contracts. NOT yet done (defines the next session, roughly in priority order): +- **`-race` run:** not executed (needs the build server; CGO/sqlite won't `-race` on this Windows box). Highest-value next step — would empirically confirm/deny CTRL-T3-1 (M2) and any uncaught shared-state races. Recommended in a throwaway `mktemp -d` on `kisfenyo@192.168.0.180`, then delete. - **Remaining Tier-1/2 pkgs not deeply read:** controller `internal/selfupdate` (signed-update verification, restart safety), `internal/notify`, `internal/integrations` (compose patching — OnlyOffice→FileBrowser/Nextcloud), `internal/cloudflare` (WAF rule identity), `internal/metrics` (SQLite stmt/conn handling). Agent `internal/lanresolver`, `internal/hub`, `internal/desired`, `internal/config`. - **Evidence tests:** only CTRL-001 has a failing test. Cheap wins for next session: AGENT-T2-1 (empty-node UPID — pure unit test), AGENT-001/002 (swap-device-between-inspect-and-format), AGENT-003 (blkid-error/lsblk-success classification). - **Live read-only inspection** (docker logs/df/findmnt on demo) — skipped; static-only this session.