diff --git a/documentation/audits/bughunt-reconcile-2026-06-13.md b/documentation/audits/bughunt-reconcile-2026-06-13.md new file mode 100644 index 0000000..431fe50 --- /dev/null +++ b/documentation/audits/bughunt-reconcile-2026-06-13.md @@ -0,0 +1,259 @@ +> **REMEDIATION STATUS — updated 2026-06-13 (controller v0.59.0, deployed to demo guest 9201).** +> Authoritative copy of the BUGHUNT reconciliation. Statuses for the actioned findings: +> +> | Finding | Reconcile verdict | Remediation | +> |---|---|---| +> | H10 (plaintext secret on encrypt failure) | LIVE/PARTIAL | **FIXED** (fail-closed) — controller `5a80739` (v0.59.0) | +> | M2 (unlocked stackProvider read) | LIVE (benign) | **FIXED** (init-only, lock removed) — controller `092cbbe` (v0.59.0) | +> | C2,H5,H6,H7,H8 | FIXED (pre-existing) | confirmed already fixed | +> | C3 (SSD-only DB DR loss) | **MOOT-by-architecture** | whole-LXC PBS DR; no action needed | +> | C1,H9,H11 + file-gone Lows | MOOT | confirmed gone-and-not-migrated | +> | `[backlog]` Mediums M4/M5/M6, M18/M19, M25 | survived, NOT verified | recorded for a future deep pass | +> +> Fresh findings actioned: CTRL-001 **FIXED** (`c20ff56`), CTRL-T2-1 **FIXED** (`5a80739`), +> AGENT-001 **FIX PREPARED/PENDING** (agent `d96e5bd`). + +# BUGHUNT RECONCILIATION — v0.30.3 findings vs current code — 2026-06-13 + +**Task:** Reconcile the stale `BUGHUNT.md` (v0.30.3, 67 findings) against current code. Verdict each: FIXED / MOOT / LIVE with file:line evidence. NOT a new audit, NOT a fix pass. +**Branch:** `audit/2026-06-13-bughunt-reconcile` (off current `main`) + +| Repo | HEAD commit | Version | +|---|---|---| +| felhom-controller | `eea235bd6952184c4681b4b133396d6b8b0aaf33` | v0.58.0 | +| felhom-agent | `d17b5ab45dc7df51836e26d7a38450cd391b94b2` | v0.29.1 | + +**Inputs:** `BUGHUNT.md` (v0.30.3, full), `AUDIT-2026-06-13.md` (fresh audit, full). **Cardinal rule applied:** a fix-tag comment (`// H10 fix`) is a CLAIM — verified the mechanism in every case; it caught H10 (the tag added a log but did NOT close the bug). + +## Progress log + +- 19:20 — Branch created off main (eea235b). Skeleton committed + pushed. Agent migration targets located. +- 19:25 — Parts 1/2/3 dispatched in parallel (3 auditors). All returned. +- 19:45 — **Verified by hand:** C3 MOOT-by-architecture (the priority verdict); H10 PARTIAL (read deploy.go:641-666 — `// H10 fix` only adds a WARN, still persists plaintext). Wrote + ran failing evidence test `internal/stacks/saveappconfig_h10_reconcile_test.go` — FAILS, app.yaml contains the plaintext secret. +- 19:55 — Report assembled (Parts 1-5). Committed. + +## Executive summary + +Of the **62 findings not previously closed** (67 − the 5 concurrency Highs H1–H4/H12 already confirmed fixed in the fresh audit): +- **C3 verdict (priority): MOOT-by-architecture.** DR was re-platformed to whole-LXC PBS restore (the original `restore_*_linux.go` files are gone, not migrated). DB volumes ride inside the block image; the per-app/dump paths route through `GetAppDrivePath` which explicitly falls back to `systemDataPath` when `HDDPath==""`, and DB discovery is docker-ps-driven, never HDD-gated. An SSD-only app's database cannot be silently dropped on any current restore path. No silent DR data-loss. +- **7 HIGH survivors:** C2, H5, H6, H7, H8 → **FIXED** (mechanisms verified, not just the tags). C3 → **MOOT**. **H10 → PARTIAL/LIVE** — the tagged fix only logs a WARN and still writes the secret in plaintext on a `crypto.Encrypt` failure (failing evidence test written). +- **MOOT bucket:** C1 (agent watchdog uses the panic-safe inverse pattern — checked the migrated code), H9 (restic retired; agent backup has no retry-reusing-context), H11 (handler gone; replacement `tier2_config_handler.go` DOES validate the dest against the registered-drive allowlist), and file-gone Lows L7/L8/L9/L11/L12/L14/L18 — all confirmed gone-and-not-migrated-with-bug. +- **Tallies (verdicted findings):** FIXED = 5 (C2,H5,H6,H7,H8) · MOOT = 11 (C3,C1,H9,H11,L7,L8,L9,L11,L12,L14,L18) · LIVE/PARTIAL = 1 (H10). Plus M2 (=fresh-audit CTRL-T3-1) is the one LIVE survivor among the mechanically-triaged Mediums. +- **Mechanical M/L triage:** ~half the Mediums and Lows are CODE-GONE (restic/crossdrive/monitor/restore_*_linux deleted); the survivors form a known backlog (deep-verify deferred per the task). Several incidentally show their fix already in place (M1 ConstantTimeCompare, M3/M13/M14/M15/M23 addressed). + +**Net actionable:** the 3 validated fresh findings + **H10** (plaintext-secret-on-encrypt-failure) + **M2** (unlocked stackProvider read). Everything else is FIXED, MOOT, or a not-yet-verified backlog Medium/Low. + +## Part 1 — The 7 surviving HIGH findings + +### C2 — SetGeoAppOverride nil-override deref +BUGHUNT ref: line 58 (v0.30.3) +Verdict: **FIXED** +Current: controller/internal/settings/settings.go:1013-1030 (commit eea235b) +Evidence: The method nil-checks `override` FIRST, before any field access: +```go +if override == nil { // :1016 + if s.GeoRestriction != nil && s.GeoRestriction.AppOverrides != nil { delete(...) } + return s.save() // returns — no deref +} +if s.GeoRestriction == nil { ... } // :1023 nil-GeoRestriction handled separately +// override.AllowedCountries accessed only at :1029-1030, after the guard +``` +The sole caller (api/geo.go:136-137) always passes a non-nil `&settings.AppGeoOverride{}` (clear uses the separate `RemoveGeoAppOverride`). Both nil-override and nil-GeoRestriction are safe. +Confidence: verified-static + +### C3 — SSD-only apps skip DB dump restoration during DR +BUGHUNT ref: line 69 (v0.30.3) • **PRIORITY** +Verdict: **MOOT (architecture now restores DB unconditionally)** +Current: backup.go:88 (`GetAppDrivePath`), appbackup/restore.go:431, appexport/restore.go:712, export.go:562, setup/handlers.go:88 (commit eea235b) +Evidence: Original DR files (restore_drives_linux.go, restore_app_linux.go, restore_scan.go) are gone (slice 8C de-privileged the controller). Every current restore path was enumerated; none is HDD-gated to silently omit an SSD-only DB: +1. **Whole-LXC / setup restore** — disk-recovery + infra-backup restore moved to the host agent (PBS whole-LXC vzdump; setup/handlers.go:88 comment). The setup "restore" mode only pulls config from the hub; there is NO selective DB-dump-restore step at all — DB volumes ride inside the block image. +2. **DB dump flow** (`runDBDumpsInternal`, backup.go:182) iterates `DiscoverDatabases` (docker ps), not HDD presence. `GetAppDrivePath` (backup.go:88-98) returns the HDD path if set, **else explicitly falls back to `systemDataPath`** — SSD-only dumps ARE written/restored, never dropped. +3. **Native + recovery-unit restore** (restore.go:85 `restoreDockerVolumes`, restore_unit.go:74) restore the DB's named postgres/mariadb volume tar regardless of HDD. +4. **`.fab` import** — `restoreDatabase` (appexport/restore.go:712) is gated on `manifest.HasDatabase` (set from docker-ps discovery at export.go:562), **not** HDD. +The HDDPath=="" early-return class of bug cannot occur — the fallback is in the single chokepoint and DB discovery is docker-ps-driven. Bug gone, not migrated. +Confidence: verified-static + +### H5 — SyncFileBrowserMounts no concurrency guard +BUGHUNT ref: line 132 (v0.30.3) +Verdict: **FIXED** +Current: controller/internal/web/handlers.go:1366-1439 (commit eea235b) +Evidence: All public entry points (`SyncFileBrowserMounts`:1366, `SyncFileBrowserMountsReset`:1373) funnel into one private `syncFileBrowserMounts`, whose first two lines are `s.fileBrowserMu.Lock(); defer s.fileBrowserMu.Unlock()`. A real `sync.Mutex` is held across the entire body (config.yaml write, compose write, `down -v`/`up -d`) — not a racy flag. Every caller, including the 4 `go`-launched ones, reaches the file writes only through this function, so it genuinely serializes them. +Confidence: verified-static + +### H6 — PushEvent never records to event history +BUGHUNT ref: line 143 (v0.30.3) +Verdict: **FIXED** +Current: controller/internal/notify/notifier.go:213, 223 (commit eea235b) +Evidence: Inside the `PushEvent` goroutine, `recordHistory` is now called on BOTH terminal outcomes: success (2xx) at :213, and failure (after 3 attempts) at :223. `recordHistory` (:550) writes the ring buffer under `historyMu`; `GetEventHistory` (:526) reads it. The history page now sees real PushEvent traffic. +Confidence: verified-static + +### H7 — PushOnce returns nil for non-2xx +BUGHUNT ref: line 154 (v0.30.3) +Verdict: **FIXED** +Current: controller/internal/report/pusher.go:197-227 (commit eea235b) +Evidence: `if resp.StatusCode >= 200 && resp.StatusCode < 300 { return nil }` then `return fmt.Errorf("hub push-once: HTTP %d", resp.StatusCode)` at :227. A 4xx/5xx now yields a real error. The earlier `return nil` (:200) is only the legitimate "hub not configured" short-circuit. +Confidence: verified-static + +### H8 — tmpFile not closed/synced before rename in DB dump +BUGHUNT ref: line 165 (v0.30.3) +Verdict: **FIXED** +Current: controller/internal/appbackup/dbdump.go:267-298 (commit eea235b) +Evidence: Ordering is now Sync → Close → Stat → Rename, errors checked at each step BEFORE rename: +```go +if err := tmpFile.Sync(); err != nil { os.Remove(tmpPath); return result } // :267 +if err := tmpFile.Close(); err != nil { os.Remove(tmpPath); return result } // :273 +stat, err := os.Stat(tmpPath) ... // :281 +if err := os.Rename(tmpPath, finalPath); err != nil { ... } // :293 +``` +The deferred `Close` (:244) is now a harmless double-close. This is the controller's only DB-dump writer (`internal/backup/` delegates to this same `appbackup.DumpOne`), so no second un-fixed copy exists. +Confidence: verified-static + +### H10 — SaveAppConfig stores secrets in plaintext on encryption failure +BUGHUNT ref: line 187 (v0.30.3) +Verdict: **PARTIAL / LIVE** (the tagged fix added a WARN log but did NOT close the bug — cardinal-rule catch) +Current: controller/internal/stacks/deploy.go:654-666 (commit eea235b) +Evidence: +```go +if enc, err := crypto.Encrypt(encKey, v); err == nil { + saveCfg.Env[k] = enc; encryptedCount++; continue +} else { + // H10 fix: log encryption failure — value will be saved in plaintext. + log.Printf("[WARN] [stacks] Failed to encrypt env var %q: %v — saving as plaintext", k, err) // :662 +} +} +saveCfg.Env[k] = v // :665 — FALLS THROUGH: plaintext IS persisted +``` +The original complaint had two parts: (a) silent, (b) plaintext-on-disk. (a) is fixed (explicit WARN). (b) is NOT — the code still writes the secret unencrypted to app.yaml. The `// H10 fix` comment is a claim that does not match the mechanism. +Severity (reassessed by consequence): **Low–Medium.** `crypto.Encrypt` (AES-GCM, with `encKey != nil` already required to enter the branch) realistically fails only on a malformed/short key or RNG failure — rare. But when it does, that secret lands in plaintext in app.yaml (0600). +Trigger: `crypto.Encrypt` returns an error for a sensitive var while `encKey` is non-nil (malformed/wrong-length key, RNG failure). +Impact: One or more app secrets (DB/admin passwords) stored unencrypted in app.yaml, with only a WARN log. +Fix sketch (NOT applied): replace the fallthrough with a fail-closed `return fmt.Errorf("encrypting %q: %w", k, err)` so no plaintext is persisted (or skip the var). Note this is co-located with the CTRL-T2-1 deploy-lifecycle change — sequence them together (see Part 4). +Confidence: **verified-by-test** — `internal/stacks/saveappconfig_h10_reconcile_test.go` (this branch). With a 5-byte key, `SaveAppConfig` logs the WARN and writes `DB_PASSWORD: supersecret-pw-do-not-leak` to app.yaml; the test asserting no-plaintext FAILS. Run: `cd controller && go test ./internal/stacks/ -run H10 -v`. + +## Part 2 — MOOT bucket confirmations (+ migration checks) + +### C1 — Watchdog unlock/relock panic-unsafe → checked the agent migration +Verdict: **MOOT** (migrated, but WITHOUT the bug) +Evidence: Controller `internal/monitor/watchdog.go` is gone. The agent watchdog `/e/git/felhom-agent/internal/storage/watchdog.go` was read in full: `tick()` probes liveness OUTSIDE the lock (:250-257), takes `w.mu.Lock()` only for the in-memory state diff/debounce (:261-327), `Unlock()`s once at :327, then runs all side-effects (report trigger :336, `Remount` dispatched to a goroutine via `w.spawn` :342, `onAbsent` :349) AFTER unlocking. There is no `Unlock(); call(); Lock()` sequence and no `defer Unlock()` over a panicking call. The one IO/panic-prone op (remount) is handed to a background goroutine off the lock path. The double-unlock-on-panic pattern does not exist in the migrated code. + +### H9 — Restic retry reused a possibly-expired context → checked agent backup +Verdict: **MOOT** (restic retired; no equivalent retry migrated) +Evidence: Controller `internal/backup/restic.go` gone. Agent backup `/e/git/felhom-agent/internal/backup/runner.go:100-130` is a single linear flow (`Vzdump` → one `WaitTask(ctx, upid, {Timeout:30m})` → `latestArchive`); on error it returns immediately — no retry-after-unlock, no retry reusing a parent context. `watchForSnapshot` re-polls but returns on `ctx.Done()`. `internal/pbs/` has no retry/backoff at all. The H9 expired-context-retry pattern did not survive. + +### H11 — settingsCrossBackupHandler missing dest validation +Verdict: **MOOT** (handler gone; replacement validates) +Evidence: The original unvalidated cross-drive-backup web handler (web/handlers.go:947-994) is gone — no `CrossBackup`/`settingsCross*` HTTP handler exists in controller/internal/web or /api. The replacement `internal/web/tier2_config_handler.go` validates the POSTed `target` (:59) against the eligible registered-drive allowlist before saving (:61-77), with a runtime re-validation in the runner. Remaining `CrossDrive` references are settings/config/notify plumbing, not path-accepting handlers. + +### File-gone Lows (one stroke) +| ID | Original | Verdict | +|---|---|---| +| L7 | backup/crossdrive.go double-clear running map | CODE-GONE (not migrated) | +| L8 | backup/restic.go uint64→int64 cast | CODE-GONE (restic retired) | +| L9 | backup/restic.go empty error parse | CODE-GONE | +| L11 | backup/restore_drives_linux.go fstab TOCTOU | CODE-GONE (agent has read-only mounts only; fstab deferred in agent docs) | +| L12 | backup/restore_drives_linux.go non-atomic fstab | CODE-GONE | +| L14 | backup/restore_app_linux.go hardcoded 0644 | CODE-GONE (the 0644 in backup/recovery_unit.go is new Tier-2 code using atomicWrite, unrelated) | +| L18 | backup/restore_scan.go picks first not freshest | CODE-GONE (no restore-scan/freshest logic in agent) | + +## Part 3 — Medium/Low mechanical triage (existence only; survivors NOT deep-verified) + +**Cross-ref:** survivors that overlap the fresh audit's "What was NOT covered" are flagged **[backlog]** — genuinely unexamined. Survivors the fresh audit already verified/fixed are flagged **[audit:…]**. + +### Mediums +| ID | survives? | current file:line | note | +|---|---|---|---| +| M1 | SURVIVES | cmd/controller/main.go:769 | **already uses `subtle.ConstantTimeCompare`** (also csrf.go:40,65) → effectively addressed | +| M2 | SURVIVES | internal/backup/backup.go:122,259,331 | **LIVE = fresh-audit CTRL-T3-1** (unlocked `stackProvider` read; benign, init-only write) | +| M3 | SURVIVES | internal/settings/settings.go:934 | **[audit: FIXED]** drains only after successful save | +| M4 | SURVIVES | internal/stacks/deploy.go:59 (`SubdomainInUse`) | **[backlog]** I/O under RLock — not re-verified | +| M5 | SURVIVES | internal/stacks/manager.go:128 (`MigrateEncryption`) | **[backlog]** encKey lock — not re-verified | +| M6 | SURVIVES | internal/stacks/manager.go:128 | **[backlog]** lock-during-I/O (startup-only; likely benign) | +| M7 | GONE | — | `executeAllRestores`/web/handler_restore.go deleted (8C) | +| M8 | SURVIVES | internal/web/auth.go:113 | **[audit: partial — CTRL-009]** a limiter now exists but is XFF-spoofable | +| M9 | SURVIVES | internal/api/router.go:161+ | **[audit: addressed]** `extractName` rejects empty/`..` (fresh audit confirmed) | +| M10 | SURVIVES | internal/scheduler/scheduler.go:76,102,130 | **[audit: FIXED]** late-registration launches goroutine immediately | +| M11 | GONE | — | monitor/watchdog.go deleted; no `findStoragePath` | +| M12 | GONE | — | monitor/watchdog.go deleted | +| M13 | SURVIVES | internal/metrics/store.go:27 | **[audit: FIXED]** WAL mode now verified | +| M14 | SURVIVES | internal/metrics/collector.go:99 | **[audit: FIXED]** uses parent ctx, not Background | +| M15 | SURVIVES | internal/metrics/telemetry.go:42 | scan error now logged (not silent) | +| M16 | GONE | — | crossdrive.go deleted (`copyStackDBDumps`) | +| M17 | GONE | — | restic stats removed from backup.go | +| M18 | SURVIVES | internal/appbackup/dbdump.go:425 (`ListDumpFiles`) | **[backlog]** re-validates every dump — moved, not re-verified | +| M19 | SURVIVES | internal/appbackup/dbdump.go:536 (`deriveStackName`) | **[backlog]** naive suffix-strip — moved, not re-verified | +| M20 | GONE | — | crossdrive.go deleted (`syncInfraConfig`) | +| M21 | GONE | — | no `http.Get` anywhere | +| M22 | SURVIVES | internal/assets/syncer.go:78 | **[audit: FIXED]** lock is a running-guard; download lock-free | +| M23 | SURVIVES | internal/sync/sync.go:433 | `maskRepoURL` now applied at the log site → addressed | +| M24 | GONE | — | old storage_handlers.go:953 path-prefix code gone (only `/mnt/` guards remain) | +| M25 | SURVIVES | internal/web/server.go:128+ (`Set*`) | **[backlog]** init-order, like CTRL-T3-1/M2 — not re-verified | +| M26 | SURVIVES | internal/crypto/crypto.go:103,112 (`DecryptMap` global `log`) | **[backlog]** cosmetic logger inconsistency | + +### Lows +| ID | survives? | current file:line | note | +|---|---|---|---| +| L1 | SURVIVES | cmd/controller/main.go:1172 (`fileExists`) | **[audit: dead-code]** unused (also in fresh audit's dead-code inventory) | +| L2 | SURVIVES | cmd/controller/main.go (multiple `go func`) | **[backlog]** no WaitGroup for shutdown | +| L3 | SURVIVES | internal/config/config.go:258 | `0`=unset sentinel present | +| L4 | SURVIVES | cmd/controller/main.go:189 | hardcoded metrics DB path | +| L5 | SURVIVES | cmd/controller/main.go:57 | hardcoded config-flag default | +| L6 | SURVIVES | cmd/controller/main.go:117 | string-concat path | +| L7–L9, L11, L12, L14, L18 | GONE | — | (Part 2 file-gone bucket) | +| L10 | GONE | — | infraPaths gone from backup.go | +| L13 | SURVIVES | internal/backup/backup.go:484 | 10s DB-inspect timeout present | +| L15 | GONE | — | snapshot/restic history gone | +| L16 | GONE | — | `RunIntegrityCheck` gone (restic tier removed) | +| L17 | SURVIVES | internal/backup/backup.go:95 | empty drive-path warn present | +| L19 | SURVIVES | internal/appbackup/dbdump.go:478,517 | throwaway exec for logging | +| L20 | SURVIVES | internal/appbackup/dbdump.go:320,333 (`ValidateDump` global `log`) | **[backlog]** | +| L21 | SURVIVES | internal/stacks/healthprobe.go:337 (`methodOrEmpty`) | moved to stacks/ | +| L22 | SURVIVES | internal/stacks/manager.go:468 (`aggregateState`) | **[backlog]** StatePaused handling — not re-verified | +| L23 | SURVIVES | internal/stacks/manager.go:830 | **[backlog]** composeExecCustomEnv env double-load | +| L24 | SURVIVES | internal/stacks/manager.go:930 (`logPostStartStatus`) | **[backlog]** goroutine no cancel | +| L25 | SURVIVES | internal/stacks/delete.go:510 (`ParseComposeHDDMounts`) | **[audit: mitigated]** traversal cleaned (C10 fix), naive-YAML still present | +| L26 | SURVIVES | internal/stacks/delete.go:574 (`getDirSizeHuman`) | **LIVE = fresh-audit CTRL-T2-5** (no timeout) | +| L27 | SURVIVES | internal/stacks/delete.go:80/283 + ScanStacks | **[backlog]** double update path | +| L28 | SURVIVES | internal/stacks/manager.go:591-593 | **[audit: FIXED]** Options now deep-copied (H12 fix) | +| L29 | SURVIVES | internal/web/server.go:327,363 (`serveCatchAll`) | **[backlog]** double WriteHeader | +| L30 | SURVIVES | internal/web/auth.go:219,236 | **[backlog]** CSRF token not rotated | +| L31 | GONE | — | old storage_handlers.go:1490 Content-Type code gone | +| L32 | SURVIVES | internal/web/alerts.go:54 | variadic API (cosmetic) | +| L33 | SURVIVES | internal/web/funcmap.go:329 | **[audit: reviewed]** `json` func suppresses marshal error (no XSS — html-escaped) | +| L34 | GONE | — | monitor/pinger.go deleted | +| L35 | SURVIVES | internal/metrics/store.go:19 | **[backlog]** no SQLite conn-pool limits | +| L36 | SURVIVES | internal/metrics/store.go:312 | **[backlog]** no WAL checkpoint after Prune | +| L37 | SURVIVES | internal/cloudflare/countries.go:253 | rebuilds sorted list (cosmetic perf) | + +## Part 4 — Merged, severity-ordered fix list + +Combines the 3 validated fresh findings with every LIVE old finding (H10) + the LIVE survivor M2. Severity reassessed by consequence. + +| # | ID | Title | Repo | Location (file:line @ commit) | Severity (reassessed) | Effort | Fix sketch | +|---|---|---|---|---|---|---|---| +| 1 | CTRL-001 | Import path traversal via unvalidated `manifest.AppName` | controller | appexport/restore.go:339,365,401 @ eea235b | **High (Critical-adjacent)** | S | Reject `AppName` unless single safe segment (`^[a-z0-9][a-z0-9-]*$`); same for archive subdir/config-file names | +| 2 | CTRL-T2-1 | `app.yaml` persists `Deployed:true` before `compose up -d` → ghost-deployed stuck stack on crash | controller | stacks/deploy.go:294-330 @ eea235b | High | M | Persist `Deployed:false`/a `deploying` flag before compose; flip true only after success; or startup-reconcile zero-container recent-deploy → failed | +| 3 | H10 | SaveAppConfig persists secret in PLAINTEXT on encrypt failure | controller | stacks/deploy.go:654-665 @ eea235b | Low–Medium | S | Fail-closed: `return err` instead of falling through to plaintext write | +| 4 | AGENT-001 | Inline customer-confirmed wipe formats mutable `/dev` path (classify→mkfs TOCTOU) → wrong-disk wipe | agent | localapi/disks.go:429-471 @ d17b5ab | Medium (data-loss consequence) | M | Re-resolve durable→path + re-inspect immediately before Format (mirror `WipeExecutor.Execute`) | +| 5 | M2 / CTRL-T3-1 | `backup.Manager.stackProvider` read without the mutex that guards its write | controller | backup/backup.go:89,122,259,331,400,491 @ eea235b | Low (benign; init-only write) | S | Drop the pointless lock on the init write, or add a locked accessor for the 11 reads | + +**CTRL-001 severity note (Critical vs High):** it is an arbitrary-directory write as the controller process (root-in-container). On the no-password demo posture, import is reachable unauthenticated, and a written `docker-compose.yml` placed in a stacks path can later be deployed → effectively code-execution-within-container. That argues **Critical**. It is bounded by the de-privileged container and (on password-set deployments) RequireAuth+CSRF, which argues **High**. Verdict: **High, treat as Critical operationally** — and it is the cheapest fix on the list, so it goes first regardless. + +### Recommended fix order + rationale +1. **CTRL-001 first** — trivial (one validator) for the highest security consequence; remove the arbitrary-write primitive before anything else. +2. **Deploy-lifecycle slice: CTRL-T2-1 + H10 together** — both edit `stacks/deploy.go` (the deploy goroutine + `SaveAppConfig`). **They INTERACT — sequence them in one slice/PR to avoid merge conflict.** H10 is the smaller change (fail-closed return); fold it in while restructuring the deploy/save ordering for CTRL-T2-1. Net: deploy state is honest on crash AND no plaintext secret can be written. +3. **AGENT-001 — spike-first, isolated review.** It is the only change touching the DESTRUCTIVE wipe path; do a focused spike (adopt the `WipeExecutor` resolve→re-derive→re-inspect-before-Format structure) and review it in isolation on the agent repo. Do NOT bundle with controller work. The live USB-reenumeration race is hard to reproduce — fix by code-structure parity with the already-correct signed-jobs path, not by chasing a live repro. +4. **M2 / CTRL-T3-1 last** — trivial cleanup; benign today (init-only write). Opportunistic, or fold into a `-race`-driven cleanup pass. + +## Part 5 — Next-session plan + +First fix slice: **CTRL-001** (add the path-segment validator — minutes, highest consequence) then the **deploy.go slice (CTRL-T2-1 + H10)** since they are co-located and both touch deploy/SaveAppConfig. Hold **AGENT-001** for a separate spike + isolated review on the agent repo (structure-parity with `WipeExecutor`, no live-repro dependency). Runtime confirmation still wanted before/after: a one-shot CTRL-T2-1 repro (kill the controller during a large-image deploy, confirm the ghost-deployed state, then re-confirm the fix clears it) — AGENT-001 and H10 need no runtime repro (both have static/structural fixes; H10 has a failing unit test that will flip green). + +## Session notes, assumptions, what was NOT verified + +- **Cardinal rule paid off on H10:** the `// H10 fix` tag claimed a fix that the mechanism does not deliver. All other fix-tags (C2/C03, H5, H6, H7, H8) were verified to match their mechanism. +- **MOOT proofs included migration checks** (per the mandate): C1 → agent `storage/watchdog.go` read in full (panic-safe inverse pattern, no bug); H9 → agent `backup/runner.go` + `pbs/` (no retry-reusing-context); H11 → replacement `tier2_config_handler.go` validates dest. +- **Part 3 is existence-only** (per task) — survivors flagged `[backlog]` are NOT verdicted FIXED/LIVE; they are the genuinely-unexamined set for a future deep pass. The `[backlog]` Mediums most worth a look: M4/M5/M6 (stacks lock-during-I/O), M18/M19 (appbackup dump validation/naming), M25 (web Server Set* init-order, same class as M2). +- **Assumption:** "one customer per host" / de-privileged single-tenant container (affects CTRL-001's Critical-vs-High framing and the exposure of M2/CTRL-T3-1). +- **Not verified (out of scope for reconciliation):** the deep mechanism of every surviving Medium/Low (Part 3 is a triage, not an audit); any runtime/`-race` confirmation (none run this session). +- Two evidence tests written, both FAIL at the recorded commit: controller `internal/stacks/saveappconfig_h10_reconcile_test.go` (H10). (The fresh-audit branch already carries CTRL-001 and AGENT-T2-1 evidence tests; not duplicated here.) +- No production source modified; BUGHUNT.md / AUDIT-2026-06-13.md / CHANGELOG / README / CONTEXT / REPORT untouched. diff --git a/documentation/audits/deep-sweep-2026-06-13.md b/documentation/audits/deep-sweep-2026-06-13.md new file mode 100644 index 0000000..4f041eb --- /dev/null +++ b/documentation/audits/deep-sweep-2026-06-13.md @@ -0,0 +1,617 @@ +> **REMEDIATION STATUS — updated 2026-06-13 (controller v0.59.0, deployed to demo guest 9201).** +> This is the authoritative copy of the deep-sweep audit; the validated findings have been actioned. +> +> | Finding | Status | Commit | +> |---|---|---| +> | CTRL-001 (import path traversal) | **FIXED** | controller `c20ff56` (v0.59.0) | +> | CTRL-T2-1 (ghost-deployed on crash) | **FIXED** | controller `5a80739` (v0.59.0) | +> | AGENT-001 (wrong-disk wipe TOCTOU) | **FIX PREPARED, PENDING REVIEW — not deployed** | agent branch `fix/agent-001-wipe-durable-reresolve` `d96e5bd` | +> | CTRL-T2-2/3, AGENT-002/003, CTRL-002, and the Low/Info tail | OPEN (backlog) | — | +> +> All other findings remain as recorded below (severity/evidence unchanged). See also the BUGHUNT +> reconciliation (`bughunt-reconcile-2026-06-13.md`) and the merged fix list therein. + +# AUDIT — felhom-controller + felhom-agent deep sweep — 2026-06-13 + +**Branch (both repos):** `audit/2026-06-13-deep-sweep` (off latest `main`) +**Auditor:** Claude Code (Opus 4.8, unattended overnight session) +**Mode:** read-only, evidence-based. No fixes applied. No mutations, deploys, or builds. + +| Repo | HEAD commit | Version | Note | +|---|---|---|---| +| felhom-controller | `eea235bd6952184c4681b4b133396d6b8b0aaf33` | v0.58.0 | all CTRL findings cite this commit | +| felhom-agent | `d17b5ab45dc7df51836e26d7a38450cd391b94b2` | v0.29.1 | all AGENT findings cite this commit | +| felhom.eu / hub (reference) | `d59691dd826a901da39562aeaa5d989b8ea1a7ee` | hub v0.11.0 | contract reference | + +**Tooling:** go1.26.0 windows/amd64; staticcheck (latest); go vet. +**Prior art:** controller `BUGHUNT.md` (2026-02-25, v0.30.3) read in full. NOT re-reported unless regressed. NOTE: BUGHUNT predates slice 8C which deleted ~12.3k LOC (internal/storage/*, restic, restore_drives, monitor/watchdog) — many BUGHUNT items (C1, C3, H9, M11-M20 partial, L8-L21 partial) reference now-deleted code and are MOOT. Agent has no BUGHUNT. + +## Progress log + +- 17:20 — Resumed: prior session left only a Phase-0 skeleton stale at v0.51.0 + a duplicate product commit. Reset audit branch onto current main (v0.58.0). Re-baselined. +- 17:25 — Phase 0 complete (both repos). Baselines below. +- 17:35 — Tier 1 dispatched (4 parallel auditors): agent destructive-path, agent auth/authz, controller backup/crypto, controller web/auth/setup. All returned. +- 17:50 — **Verified by hand**: CTRL-001 (import path traversal via manifest.AppName) — read restore.go:320-409 + manifest.go:36-42, confirmed no validator. AGENT-001 (inline wipe TOCTOU) — read disks.go:420-479, confirmed Format targets mutable req.Device. +- 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. +- 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) + +| Check | Controller (module at `controller/`) | Agent (module at root) | +|---|---|---| +| `go build ./...` | PASS | PASS | +| `go vet ./...` | PASS (clean) | PASS (clean) | +| `go test ./...` | PASS (BUGHUNT's TestBackupCopiesOnPath now green) | PASS | +| `gofmt -l .` | CRLF noise only (autocrlf=true); not a finding | same | +| staticcheck | 18 reports — triaged (4× SA4010 in backup.go → CTRL-005; SA4006/SA4017 export.go → CTRL-006; 5× U1000 dead code → §Dead code) | clean | + +## Executive summary + +Both codebases are in good shape; the fail-safe/fail-destructive postures that matter most **hold**. No Critical found. The destructive-storage and operator-signature surfaces of the agent are unusually well-built (locked verify pipeline, hash-only token store, fsync-durable nonce store, narrow privileged fence, TLS pinning, fail-destructive ambiguity defaults — all confirmed). The controller's at-rest crypto (AES-256-GCM), restore data-key fail-closed gate, restic single-flight mutex, and (for the password-set path) full CSRF+auth coverage all hold. + +**Two Highs**, both verified by hand: (1) **CTRL-001** — the app-**import** path joins the attacker-controlled `manifest.AppName` from inside a `.fab` straight into `filepath.Join`+`os.MkdirAll` with no validation (the archive-entry zip-slip guard exists, but the *stack-name* segment is unguarded); confirmed by a failing evidence test. (2) **CTRL-T2-1** — `app.yaml` persists `Deployed:true` *before* `docker compose up -d` runs and `Deploying` is never persisted, so a crash in the pull window leaves a "ghost-deployed" stuck stack the customer can't redeploy. The remaining findings are Medium edge-cases (inline-wipe device-inspect→mkfs TOCTOU; decrypt-before-MAC transient plaintext; protected-stack list with no fail-safe default; no timeout on `docker compose` defeating the quiesce downtime bound) and a Low/Info tail. + +Positive results worth recording: all five BUGHUNT stacks-concurrency Highs (H1–H4, H12) are **confirmed FIXED**; the controller↔agent (`agentapi`↔`localapi`, 12 endpoints) and controller↔hub (`report`↔ingest) contracts are **CLEAN**; agent crash-safety (`Recover()` ground-truth, defer-rollback, marker-before-mutate) and PBS/Proxmox TLS pinning **hold**. + +This session covered **Tier 1 and Tier 2 in full** for both repos plus both cross-repo contracts. **Tier 3** (templates/funcmap/XSS, goroutine-lifecycle races in surviving pkgs, `-race` run, no-`:latest` sweep) is **not yet covered** — see §"What was NOT covered". + +## Top-10 action list + +| # | ID | Sev | Repo | Title | Effort | +|---|---|---|---|---|---| +| 1 | CTRL-001 | **High** | controller | App-import path traversal via unvalidated `manifest.AppName` (verified-by-test) | S | +| 2 | CTRL-T2-1 | **High** | controller | `app.yaml` persists `Deployed:true` *before* `compose up -d` → ghost-deployed/stuck stack on crash | M | +| 3 | AGENT-001 | Medium | agent | Inline customer-confirmed wipe formats mutable `/dev` path (classify→mkfs TOCTOU) | M | +| 4 | CTRL-002 | Medium | controller | FAB decrypt streams plaintext to disk *before* verifying HMAC tag | S | +| 5 | CTRL-T2-3 | Medium | controller | Protected-stack list has no fail-safe default → empty config = nothing protected | S | +| 6 | AGENT-003 | Medium | agent | `InspectDevice` swallows `blkid` error; `lsblk` is sole "probed" authority | S | +| 7 | AGENT-002 | Medium | agent | Blank-device format runs mkfs un-gated with a probe→mkfs TOCTOU | M | +| 8 | CTRL-T2-2 | Medium | controller | No timeout on any `docker compose` exec → hung deploy/quiesce, defeats max-quiesce bound | M | +| 9 | CTRL-008 | Low | controller | `settings.json` (bcrypt hash + plaintext retrieval pw) written 0644 | S | +| 10 | CTRL-007 | Low | controller | Pre-auth setup CSRF is a forgeable double-submit cookie | M | + +(Low/Info tail: CTRL-005 decompression-bomb, CTRL-009 XFF rate-limit, CTRL-011 open-redirect, AGENT-007 decommission scheme, AGENT-004 no-timeout signed-op, AGENT-012 nonce-store growth, AGENT-T2-1..7, CTRL-T2-4/5/6 — all in the sections below.) + +--- + +## Findings — Critical / High + +### [CTRL-001] App-import path traversal via unvalidated `manifest.AppName` +Severity: High +Category: security +Location: controller/internal/appexport/restore.go:339, 365, 401 (commit eea235b); root cause manifest.go:36-42 +Confidence: **verified-by-test** (`internal/appexport/traversal_audit_test.go`, this branch) +Evidence: +```go +manifest, err := UnmarshalManifest(manifestData) // AppName fully from bundle JSON, no validation +stackDir := filepath.Join(stacksDir, manifest.AppName) // :339 +os.MkdirAll(stackDir, 0755) // :365 +composePath := filepath.Join(stackDir, "docker-compose.yml") // :401 +``` +Mechanism: `manifest.AppName` is read from the untrusted `manifest.json` inside an imported `.fab`. `UnmarshalManifest` only validates JSON, no segment check; no `IsValidStackName` exists in the package. `filepath.Join` cleans `..`, so `../../etc/cron.d/x` resolves outside `stacksDir`, then `os.MkdirAll` creates it and `restoreConfig`/`SaveEncryptedAppConfig` write `app.yaml`/`docker-compose.yml` there. The archive-entry zip-slip guard (restore.go:1047) does NOT cover this — it guards tar member names, not the stack-name segment. +Trigger: Import of a crafted `.fab`. The handler `/api/export/import` is behind RequireAuth+CSRF when a dashboard password is set, but on the demo (no password) BOTH are skipped → fully open. The `.fab` must sit under a registered `exports/` dir (`isValidExportPath` validates the file *location*, not its *contents*) — reachable via FileBrowser or a shared/malicious export. +Impact: Arbitrary-directory write as the controller process (config, restored app data, re-encrypted `app.yaml`) outside the stacks namespace — corruption/escape, potential clobber of controller config or writes onto mounted drives. +Fix sketch: In `UnmarshalManifest` (or immediately after) reject `AppName` unless it matches a strict single-segment allowlist (`^[a-z0-9][a-z0-9-]*$`, no `/ \ . ..`). Apply the same to `ConfigFiles`/`VolumeNames`/`HDDSubdirs` entries used in joins. +Verify: `cd controller && go test ./internal/appexport/ -run Traversal -v` → fails at this commit (parent-escape / deep-escape). Manual: craft a `.fab` with `"app_name":"../evil"`, import, observe `stackDir` outside `GetStacksBaseDir()`. + +### [CTRL-T2-1] `app.yaml` persists `Deployed:true` *before* `compose up -d` → ghost-deployed/stuck stack on crash +Severity: High +Category: crash-safety / invariant-drift +Location: controller/internal/stacks/deploy.go:294-330, 337-358; manager.go:280-296 (commit eea235b) +Confidence: verified-static (read by hand) +Evidence: +```go +appCfg := &AppConfig{Deployed: true, DeployedAt: time.Now()...} // :295-300 +SaveAppConfig(stackDir, appCfg, ...) // :302 — DISK says deployed:true... +m.mu.Lock(); s.Deployed = true; s.AppConfig = appCfg; m.mu.Unlock() // :322-327 +go m.runComposeDeploy(req.StackName, stackDir, env, appCfg) // :330 — ...BEFORE compose runs +// runComposeDeploy: _, composeErr := m.composeExecWithEnv(stackDir, env, "up", "-d") // :339 +// revert (mem+disk) only if process is alive when composeErr returns // :344-356 +``` +Mechanism: The on-disk `app.yaml` records `deployed:true` before `docker compose up -d` is attempted. The only thing distinguishing "deploying" from "deployed" is `Stack.Deploying`, which is in-memory ONLY — there is no `deploying` field in `AppConfig`, so it is never persisted. The revert path (:344-356) runs only if the controller is alive when `composeErr` returns. A crash/OOM/host-reboot/self-update-restart during the up-window (image pull can take 30-60s per the code's own comment) skips the revert. On restart, `ScanStacks` reads `appCfg.Deployed==true` (manager.go:281) and marks the stack Deployed with zero live containers. This directly DRIFTS the controller invariant "`Deployed` set [true] only after `up -d`, reverted on failure (mem+disk)". +Trigger: Controller crash/restart during a deploy's compose-up window (large pull, host reboot, OOM, self-update restart). +Impact: Stack shows "deployed" (UI + quiesce/backup/memory accounting) but no containers exist. `DeployStack` then refuses redeploy ("already deployed; use update instead", deploy.go:133-135), so a non-technical customer is stuck — must Remove then redeploy, which they won't know to do → support burden + an app silently not running. +Fix sketch: Write `app.yaml` with `Deployed:false` (or add+persist a `deploying:true` field) before compose; flip to `Deployed:true` in `runComposeDeploy` only after success. Or on startup reconcile: a Deployed stack with zero containers + very recent `DeployedAt` → treat as failed-deploy. +Verify: Deploy a large app, `kill -9` the controller during the pull, restart → stack listed Deployed with no containers; DeployStack refuses redeploy. (Not unit-tested: needs Manager+provider+fs+crash sim; manual repro above.) + +--- + +## Findings — Medium / Low + +### [AGENT-001] Inline customer-confirmed wipe formats a mutable `/dev` path (classify→mkfs TOCTOU) +Severity: Medium (data-loss consequence; narrow USB-reenumeration trigger) +Category: security / correctness +Location: agent/internal/localapi/disks.go:429-471 (commit d17b5ab) +Confidence: verified-static +Evidence: +```go +probe, err := s.disks.InspectDevice(r.Context(), req.Device) // inspect /dev/sdbN +role := s.deviceRole(r.Context(), req.Device) +deviceDurable, _ := storage.DeviceDurableID(req.Device) // derive id of /dev/sdbN NOW +dec := s.diskGate.AuthorizeWipe(WipeRequest{Role:..., DeviceDurableID: deviceDurable, Confirmed: req.Confirmed, ConfirmDurableID: req.DurableID}) +if dec.Allowed { s.disks.Format(r.Context(), req.Device, req.FSType) } // FORMAT the same mutable path +``` +Mechanism: The confirmed path inspects + derives + gate-binds the durable id, then formats `req.Device` (the raw `/dev` node) — never re-resolving durable→path immediately before mkfs. The durable id is *checked* but never *used as the wipe target*. A USB re-enumeration between derive and Format makes mkfs hit a different physical disk than the one confirmed. The signed-jobs `WipeExecutor` (signedjobs/wipe.go:74-99) deliberately does resolve→re-derive→re-inspect just before Format — proving the intended pattern this inline path violates. +Trigger: Customer-confirmed user-data wipe where `/dev/sdbN` is reassigned (hot-unplug/replug, udev churn, multi-USB hub) in the sub-second window. +Impact: mkfs on an unintended physical disk → data loss on the wrong drive, defeating "wipe X wipes exactly X" for the inline tier. +Fix sketch: After `AuthorizeWipe` Allowed, `storage.ResolveDurableDevice(deviceDurable)` → re-derive+match → re-inspect `DataBearing()` → Format the *resolved* path, not `req.Device` (mirror `WipeExecutor.Execute`). +Verify: localapi test that swaps the device behind `req.Device` between InspectDevice and Format; assert refuse-or-re-resolve. + +### [AGENT-002] Blank-device format runs mkfs un-gated with a probe→mkfs TOCTOU +Severity: Medium +Category: security +Location: agent/internal/localapi/disks.go:430-444 (commit d17b5ab) +Confidence: verified-static +Evidence: +```go +probe, err := s.disks.InspectDevice(r.Context(), req.Device) +if err != nil { /* fall through; probe.DataBearing()==true on !Probed (fail-safe) */ } +if !probe.DataBearing() { s.disks.Format(r.Context(), req.Device, req.FSType); writeOK(...); return } +``` +Mechanism: When the agent's own probe reads the device blank, it formats with no gate, no durable binding, on the mutable `req.Device`. "blank → benign" is correct in isolation, but a device blank at probe time can be replaced at the same node before mkfs. Same root cause as AGENT-001 (formatting a mutable path). `InspectDevice` itself is fail-safe (unprobed → data-bearing), so classification is sound; the residual risk is the path-vs-physical gap. +Trigger: `/dev/sdbN` reassigned between blank probe and mkfs. +Impact: mkfs on a now-data-bearing device that was never gated. Lower likelihood than AGENT-001 (no durable binding involved). +Fix sketch: Bind even the blank path to a durable id: derive at probe, resolve back immediately before Format, refuse on mismatch. +Verify: Test a device swapped blank→data-bearing between probe and format is not silently mkfs'd. + +### [AGENT-003] `InspectDevice` swallows the `blkid` error; `lsblk` is the sole "probed" authority +Severity: Medium +Category: error-handling / security +Location: agent/internal/storage/hostops.go:293-331 (commit d17b5ab) +Confidence: verified-static +Evidence: +```go +bout, _, _ := h.runner.Run(ctx, h.bins.Blkid, "-p", "-o", "export", device) // err DROPPED +for k, v := range parseBlkidExport(bout) { ... } +lout, _, lerr := h.runner.Run(ctx, h.bins.Lsblk, "-J", ..., device) +if lerr == nil { probe.Probed = true; ... } // lsblk is the ONLY thing that sets Probed +``` +Mechanism: `blkid`'s exit/stderr is discarded; `Probed` is set solely from `lsblk` succeeding. blkid and lsblk cover different signature classes. If blkid errors transiently (busy/partial read) and returns empty while `lsblk -J` parses the device as having no fstype/pttype/children/mount, the probe is `Probed=true` with no positive evidence → `DataBearing()` false → reachable to the un-gated AGENT-002 format. This is exactly the "error in inspection → safe-to-wipe" pattern, narrowed by lsblk's real coverage of fstype/pttype/partitions/mount (so Medium, not Critical). +Trigger: blkid errors/empties while lsblk succeeds on a device whose data signature is blkid-only (some RAID/crypto members reported via blkid USAGE that lsblk's 4 columns miss). +Impact: A device with real data classified blank and formatted without a gate. +Fix sketch: Treat a blkid hard error (non-empty stderr / non-2 exit) as a probe failure; require both reads to complete before `Probed=true`, or fold blkid success into the Probed decision. +Verify: Unit test: blkid runner errors+empty, lsblk returns a clean blank-looking device → assert `Probed==false` (data-bearing). + +### [CTRL-002] FAB decrypt streams plaintext to disk *before* verifying the HMAC tag +Severity: Medium +Category: security +Location: controller/internal/appexport/crypto.go:191-227 (commit eea235b) +Confidence: verified-static +Evidence: +```go +stream.XORKeyStream(decrypted, buf[:n]); out.Write(decrypted) // :203-204 plaintext to disk as it goes +... +if !hmac.Equal(mac.Sum(nil), storedMAC) { os.Remove(outputPath); ... } // :222-224 MAC checked only at the end +``` +Mechanism: Construction is sound (AES-256-CTR + HMAC-SHA256 Encrypt-then-MAC, distinct scrypt-derived keys, constant-time `hmac.Equal`, random salt+IV per file). But decrypt writes full plaintext to `outputPath` and verifies the tag only afterward; cleanup is a best-effort `os.Remove` whose error is ignored. A same-key bit-flipped ciphertext yields controlled plaintext deltas that briefly land on disk before rejection; a crash between write and verify leaves unauthenticated attacker-influenced data behind. Violates "no unauthenticated plaintext is ever produced." +Trigger: Importing a tampered encrypted `.fab` (decrypted tgz written to a temp file before the tag check rejects). +Impact: Transient unauthenticated plaintext on disk; ignored `os.Remove` error can leave a partial decrypted file. Not a key/confidentiality break. +Fix sketch: Decrypt to temp → fsync → verify MAC → only then rename to `outputPath`; check the `os.Remove` error. +Verify: Flip one ciphertext byte, call `DecryptFile`; instrument to confirm bytes existed at the output path mid-call. + +### [CTRL-005] Decompression-bomb: import extraction has no total-size / entry-count cap +Severity: Low +Category: resource-leak +Location: controller/internal/appexport/restore.go:1021-1072 (extractTarGz), :280 (commit eea235b) +Confidence: verified-static +Evidence: +```go +case tar.TypeReg: + outFile, _ := os.Create(target) + io.Copy(outFile, tr) // :1064 unbounded +``` +Mechanism: Each gzip-inflated entry copied with unbounded `io.Copy`, no aggregate size or file-count limit, extracted to `os.MkdirTemp` on the ~8GB guest rootfs. Zip-slip guard is present (:1047), so availability-only. +Trigger: Importing an oversized/bomb `.fab`. +Impact: Fills guest rootfs during extraction → breaks controller + other apps until `defer os.RemoveAll(tmpDir)` runs. DoS. +Fix sketch: Track cumulative bytes vs a cap and vs `system.GetDiskUsage(tmpDir)` free; abort on exceed; cap entry count. +Verify: Import a tar.gz inflating beyond rootfs free; observe `/tmp` fill. + +### [CTRL-008] `settings.json` (bcrypt hash + plaintext retrieval pw + tokens) written world-readable (0644) +Severity: Low +Category: security +Location: controller/internal/settings/settings.go:241 (commit eea235b) +Confidence: verified-static +Evidence: +```go +os.WriteFile(tmpPath, data, 0644) // file carries PasswordHash (bcrypt) + RetrievalPassword (plaintext) +``` +Mechanism: controller.yaml is correctly 0600, but settings.json — holding the dashboard bcrypt hash and the **plaintext** Hub retrieval password — is 0644. Any non-root UID or mounted-in app with read access to the data dir can read it. +Trigger: Any local read access to the data dir. +Impact: Disclosure of the retrieval password (re-pull customer config from Hub) and bcrypt hash (offline cracking). Defense-in-depth; de-privileged single-tenant container lowers exposure. +Fix sketch: Write settings.json 0600; optionally encrypt RetrievalPassword at rest with the existing AES key. +Verify: `stat -c %a settings.json` → 644. + +### [CTRL-007] Pre-auth setup CSRF is a forgeable double-submit cookie +Severity: Low +Category: security +Location: controller/internal/setup/csrf.go:33-43 (commit eea235b) +Confidence: verified-static +Evidence: +```go +return cookie.Value == formToken // cookie==form, both attacker-controllable; no server secret/HMAC +``` +Mechanism: Pure double-submit with no server-stored secret/HMAC and `HttpOnly:false`. An attacker who can set `felhom_csrf` on the victim (cookie injection over plain-HTTP `:8081`, or a same-site subdomain) can post a matching token to the pre-auth setup wizard. +Trigger: Controller in setup mode (`NeedsSetup`) + attacker can plant/predict the cookie. +Impact: Pre-auth CSRF on `/setup/manual`,`/setup/fresh` → set dashboard password, domain, git repo, Cloudflare tokens. Bounded to the one-time setup window. +Fix sketch: HMAC the token with a server secret, verify Origin/Referer on setup POSTs. +Verify: POST `/setup/manual` with matching cookie+form but no prior GET → accepted. + +### [AGENT-007] Decommission durable-id scheme unvalidated → silent no-op intent +Severity: Low +Category: contract-mismatch / correctness +Location: agent/internal/signedjobs/decommission.go:63-79 (commit d17b5ab) +Confidence: verified-static +Evidence: +```go +if p.DurableID == "" { return ... "refusing an unbound decommission" } +d.intent.SetDecommissioned(p.DurableID) // any non-empty string accepted; no scheme check +``` +Mechanism: Wipe requires the `byid:`/`byuuid:` scheme via `ResolveDurableDevice`; decommission accepts any non-empty string and writes it into the intent map. The watchdog keys on the *storage* scheme (`uuid:…`) at watchdog.go:178. A wrong-scheme id records an intent that never matches → decommission silently no-ops while reporting success. +Trigger: Signed decommission whose `durable_id` uses the device scheme (`byid:`) instead of the storage scheme. +Impact: Operator believes a drive is permanently decommissioned; watchdog still auto-mounts it. Requires operator id-format error; signature is valid. +Fix sketch: Validate `p.DurableID` carries the storage-scheme prefix before recording; reject unknown schemes (fail loud, not no-op). +Verify: Decommission with `byid:…` → should refuse. + +### [CTRL-009] Login rate-limiter keys on spoofable `X-Forwarded-For` +Severity: Low +Category: security +Location: controller/internal/web/auth.go:127-161 (commit eea235b) +Confidence: verified-static +Evidence: +```go +ip := r.RemoteAddr +if fwd := r.Header.Get("X-Forwarded-For"); fwd != "" { ip = strings.Split(fwd, ",")[0] } // attacker-controlled +``` +Mechanism: Limit key derived from client-supplied XFF with no trusted-proxy allowlist; rotating XFF defeats the 5/min lock. bcrypt cost-10 is the real backstop. +Trigger: Brute-force `/login` with varying XFF. +Impact: Online password brute force (hardening gap, not an immediate break). +Fix sketch: Honor XFF only from a configured trusted-proxy CIDR; else key on `r.RemoteAddr`; add a global failed-login cap. +Verify: 5 failed logins with distinct XFF each reach bcrypt (no lock). + +### [CTRL-011] Open redirect via protocol-relative `?next=//host` +Severity: Low +Category: security +Location: controller/internal/web/auth.go:187-191 (commit eea235b) +Confidence: verified-static +Evidence: +```go +if nextURL != "" && strings.HasPrefix(nextURL, "/") { redirectTo = nextURL } // "//evil.com" passes +``` +Mechanism: `next` only required to start with `/`; `//evil.com` is protocol-relative and most browsers redirect to that host. +Trigger: `POST /login?next=//evil.com` after valid credentials. +Impact: Post-login open redirect (phishing). Requires a valid login first. +Fix sketch: Reject `next` starting with `//` or `/\`. +Verify: Log in with `next=//example.com` → 302 to `//example.com`. + +### [AGENT-004] Signed-op execution runs under `context.Background()` (no timeout) +Severity: Low +Category: resource-leak +Location: agent/internal/signedjobs/runner.go:81-85 (commit d17b5ab) +Confidence: verified-static +Evidence: +```go +go func() { if _, err := r.RunOnce(context.Background()); err != nil { ... } }() // no deadline → mkfs/pct can hang forever +``` +Mechanism: ctx flows to `exec.CommandContext`; a wedged mkfs/pct never gets cancelled, and the single-flight `running` flag then short-circuits every later pass → the signed-jobs consumer stalls until restart. +Trigger: A destructive mkfs hangs (IO stall, DM hang). +Impact: Runner goroutine wedged; no further signed ops until restart. +Fix sketch: `context.WithTimeout` per pass in `OnEnvelope`, and/or per-op deadline around Format. +Verify: Inject a blocking Format; assert RunOnce returns within timeout. + +### [AGENT-012] FileNonceStore loads expired nonces; `MemoryNonceStore` never evicts +Severity: Low +Category: resource-leak +Location: agent/internal/authz/noncestore.go:92-112 (commit d17b5ab) +Confidence: verified-static +Evidence: +```go +for _, line := range ... { ... s.idx[r.Nonce] = r.Exp } // no expiry filter on load; compaction only after CompactEvery new appends +``` +Mechanism: `Open` loads all logged nonces incl. expired; compaction driven only by new appends (default 1000). Frequent restarts + long validity window keep expired nonces resident. Bounded by op-rate × window; correctness unaffected (time-window check rejects expired ops first). +Trigger: High op volume + frequent restarts. +Impact: Slow nonce-log growth on a busy host. Not a security hole. +Fix sketch: Skip `r.Exp.Before(now)` during `load()`; run one compaction on `Open`. +Verify: Open a store whose log holds only expired records → assert `len(idx)==0`. + +### [AGENT-009] localapi cross-guest scope check covers only the query `vmid` (latent) +Severity: Low +Category: security +Location: agent/internal/localapi/server.go:263-308 (commit d17b5ab) +Confidence: verified-static +Evidence: +```go +if q := r.URL.Query().Get("vmid"); q != "" { if want != vmid { ...403 } } +fn(w, r, vmid) // handler ALWAYS receives the token's vmid +``` +Mechanism: `withGuest` rejects a mismatching *query* vmid; a mismatching *body* vmid is caught separately by per-handler `scopedFromBody`. Self-scoping holds today on all 14 routes (handler always uses the token's vmid), but the invariant relies on per-handler discipline, not one chokepoint — a future route that reads an id from body/path and forgets `scopedFromBody` would regress it. +Trigger: New handler omitting the body-vmid check. +Impact: None today; latent cross-guest exposure on future routes. +Fix sketch: Centralize the body-vmid check, or assert handlers never read an id other than the wrapper's `vmid`. +Verify: Table test POSTing `{"vmid":}` to every mutating route → expect 403. + +### [AGENT-006] `wholeDiskOf` symlink-resolution failure falls back to raw string +Severity: Low +Category: correctness +Location: agent/internal/storage/role.go:69-87 (commit d17b5ab) +Confidence: verified-static +Evidence: +```go +if resolved, err := filepath.EvalSymlinks(device); err == nil { dev = resolved } +... return "", false // unrecognized → caller treats as system (fail-safe) +``` +Mechanism: Direction is fail-safe (unrecognized → system/protected). Drift: `SystemDisks` only adds a disk when `wholeDiskOf` succeeds; an EvalSymlinks failure on a system mount's device omits that OS disk from `sysDisks`, so a sibling user-data disk could flap. No unsafe verdict, but classification of user-data disks can be unstable when topology resolution is flaky. +Trigger: Transient `/dev` churn failing EvalSymlinks on a system mount during classification. +Impact: No unsafe wipe; possible user-data classification flapping. +Fix sketch: On EvalSymlinks error for a system mount, treat the whole `SystemDisks` result as `ok=false` (all candidates → system). +Verify: `SystemDisks` test with a `/boot` device that fails to resolve → assert `ok==false`. + +### [CTRL-T2-3] Protected-stack list has no fail-safe default — empty/missing config = nothing protected +Severity: Medium +Category: security / invariant-drift +Location: controller/internal/config/config.go:251-301 (applyDefaults), 346-354 (IsProtectedStack) (commit eea235b) +Confidence: verified-static +Evidence: +```go +func (cfg *Config) IsProtectedStack(name string) bool { + for _, p := range cfg.Stacks.Protected { if strings.EqualFold(p, name) { return true } } + return false // empty list → everything unprotected +} +// applyDefaults sets ~40 defaults but NEVER seeds cfg.Stacks.Protected +``` +Mechanism: Protection is pure list-membership; `applyDefaults` never seeds the list. The set comes only from the setup wizard's generated yaml or the hub-pulled controller.yaml. If the hub template omits/empties `stacks.protected` (or a hand-edit/merge drops the key), `IsProtectedStack` returns false for everything, and the server-side guards (router.go:411-414, delete.go:86/289) all silently allow stop/remove/delete of traefik, cloudflared, filebrowser, and the controller itself. No fail-safe floor. +Trigger: Hub config-template regression, hand-edited controller.yaml, or a config merge dropping the key. +Impact: Loss of the core server-side protection invariant — a customer/API call could stop or remove infra (including the controller's own plane via traefik/cloudflared). +Fix sketch: In `applyDefaults`, if `len(cfg.Stacks.Protected)==0` seed `{traefik,cloudflared,felhom-controller,filebrowser}`; OR hardcode an always-on floor inside `IsProtectedStack` independent of config. +Verify: Load a controller.yaml with no `stacks:` block → `IsProtectedStack("traefik")` returns false. + +### [CTRL-T2-2] No timeout/context on any `docker compose` exec → hung deploy/stop, defeats quiesce max-downtime bound +Severity: Medium +Category: resource-leak / crash-safety +Location: controller/internal/stacks/manager.go:830-897 (composeExecCustomEnv); deploy.go:339; delete.go:137,339 (commit eea235b) +Confidence: verified-static +Evidence: +```go +if m.composeCmd == "docker compose" { cmd = exec.Command("docker", fullArgs...) } else { cmd = exec.Command("docker-compose", args...) } +if err := cmd.Run(); err != nil { ... } // bare exec.Command, no context, no deadline — can block forever +``` +Mechanism: Every compose call (deploy/start/stop/down/restart/update/delete) uses `exec.Command` with no `CommandContext`/deadline — while `getDirSizeBytes` (delete.go:589) DOES use a 30s context, proving the pattern is known. A hung `docker compose down`/`up`/`pull` blocks the calling goroutine forever, holding the `Deploying`/`infraMu`/lane locks. For quiesce: `StopStack→down` hanging blocks inside the stop loop; the max-quiesce deadline is checked only AFTER stops complete (quiesce.go:243), so a hung stop strands the app down past the downtime bound. +Trigger: docker daemon stall, registry hang during pull, a container ignoring SIGTERM. +Impact: Deploy/stop/start hang indefinitely; subsequent ops of that class blocked; quiesce's max-downtime guarantee bypassed. +Fix sketch: Add `composeExecCtx(ctx, ...)` via `exec.CommandContext` with per-op deadlines (deploy/pull ~10m, down/stop ~2m); plumb the quiesce ctx through StopStack/StartStack. +Verify: `docker compose` shim that sleeps forever → deploy/stop never returns; quiesce exceeds max-quiesce with the app down. + +### [CTRL-T2-4] Async-deploy disk revert happens outside the lock → narrow window for a stale-disk ScanStacks to resurrect Deployed +Severity: Low +Category: concurrency +Location: controller/internal/stacks/deploy.go:341-357; manager.go:287-296 (commit eea235b) +Confidence: suspected-needs-runtime +Evidence: In-memory revert clears `Deployed/Deploying` under `m.mu` (:344-350), but the disk revert `SaveAppConfig` runs AFTER `m.mu.Unlock()` (:353-356). A 2-min `stack-scan` landing in that gap reads stale `deployed:true` from disk, and since `Deploying` is now false its guard `if !existing.Deploying` (manager.go:293) lets it overwrite in-memory `Deployed` back to true until the next scan. +Impact: Transient ghost-deployed after a failed deploy; self-heals next scan. +Fix sketch: `SaveAppConfig` the reverted config BEFORE releasing the lock (or hold a deploy-scoped guard until disk is consistent). +Verify: Inject a SaveAppConfig delay on the revert path; fire ScanStacks in the gap. + +### [CTRL-T2-5] `getDirSizeHuman` (`du -sh`) has no timeout — hangs delete/remove on a slow/stale mount +Severity: Low +Category: resource-leak +Location: controller/internal/stacks/delete.go:573-585 (commit eea235b) +Confidence: verified-static +Evidence: `getDirSizeBytes` (delete.go:589) wraps `du -sb` in a 30s context; its sibling `getDirSizeHuman` (called on every delete/remove + HDD-data listing) uses bare `exec.Command("du","-sh",path)`. Traversal is already mitigated (ParseComposeHDDMounts cleans + ProtectedHDDPaths gate), so hang/DoS only. +Fix sketch: Give `getDirSizeHuman` the same `exec.CommandContext` 30s deadline. +Verify: Point an orphan's bind at a slow FUSE mount; call delete; handler blocks. + +### [AGENT-T2-1] PVE `WaitTask`/`TaskStatusOnce` never validate the UPID node (PBS side does) +Severity: Low +Category: contract-mismatch +Location: agent/internal/proxmox/task.go:70-81,110-114; upid.go:29-60 (commit d17b5ab) +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: `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 +Category: error-handling +Location: agent/internal/proxmox/task.go:134-142 (commit d17b5ab) +Confidence: verified-static +Evidence: `if st.Running() || st.Status == "" { ...backoff; continue }` — a 200 decoding to an empty `Status` polls until `opts.Timeout`. The per-guest queue lane is serial, so one stuck wait stalls that guest's later ops (≤10m). +Fix sketch: Cap consecutive empty-status polls (e.g. 5) → return a distinct "status never materialized" error. +Verify: Mock `TaskStatusOnce` returning `{Status:""}`; assert `WaitTask` returns before full Timeout. + +### [AGENT-T2-3] Recover marks a no-UPID in-flight op `failed` without verifying the POST didn't land +Severity: Low +Category: crash-safety +Location: agent/internal/reconcile/recover.go:56-63; engine.go:200-203 (commit d17b5ab) +Confidence: verified-static +Evidence: Journal order is `OpStarted`→POST→`OpTaskRunning(upid)`. A crash after the POST returns but before `OpTaskRunning` leaves an `OpStarted`-only entry whose mutation may already have taken effect; Recover assumes "never confirmed → never happened" and records `failed`. Benign for convergent reconcile ops (re-planned next pass), but the `marker-before-mutate` invariant does not strictly hold for the POST's task-id marker. +Fix sketch: For non-convergent/one-shot kinds, query live `GuestStatus/GuestConfig` before recording terminal (as Scratch/Rollback already do). +Verify: Simulate an `OpStarted`-only `ActionStart` entry; confirm Recover consults live run-state (currently it does not). + +### [AGENT-T2-5] Provision re-mint revokes the old token before the mount attach can fail (no transactional rollback) +Severity: Low +Category: resource-leak / correctness +Location: agent/internal/provision/backhalf.go:106-152 (commit d17b5ab) +Confidence: verified-static +Evidence: `Mint` is last-write-wins and revokes the guest's previous token immediately; if a re-provision's later `pct set` attach fails, the old token is already revoked while the new mount was never attached → guest left with no working local-API credential until a successful re-run. No `defer` rollback across mint↔attach. (First-time provision unaffected; file is 0600, no leak.) +Fix sketch: Mint AFTER the mount is prepared/attached, or log a WARN on attach-failure-after-remint so the revoked-token state is visible. +Verify: Inject a `pct set` failure on a 2nd Provision for the same VMID; confirm the prior token is already revoked. + +### [AGENT-T2-6] A panic in a queued reconcile job crashes the whole agent (no `recover()` in the lane) +Severity: Low +Category: crash-safety +Location: agent/internal/reconcile/bringup.go:148-155,181-187; queue.go run() ~line 135 (commit d17b5ab) +Confidence: verified-static +Evidence: The bring-up rollback `defer` fires on panic (guest is destroyed — good), but the panic then propagates out of `lane.run`'s `t.res <- t.fn()` with no `recover()`, taking down the process. On restart `Recover()` reaps the half-built guest, so no guest leak — but one job's panic = full agent outage. +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 + +### [CTRL-005b] DB-dump per-DB summary built then discarded (SA4010 ×4) +Severity: Low → Info (observability only) +Category: error-handling / dead-code +Location: controller/internal/backup/backup.go:179-253 (commit eea235b) +Confidence: verified-static +Evidence: `summary` accumulates `OK/SKIP/FAIL ` lines (`:188,:193,:204,:208`) but is never read; caller returns generic `"some database dumps failed"` (:249). SKIP reasons ("drive disconnected/decommissioned") are lost. +Impact: Operators lose the per-DB failure/skip breakdown — can't tell which app/drive is unprotected. +Fix sketch: Fold `summary` into the returned error / `m.lastDBDump` status, or remove the dead var. +Verify: Wire `summary` into the error → SA4010 clears. + +### [CTRL-006] `executeExport` step-5 timing baseline dead-assigned (SA4006/SA4017) +Severity: Info • Category: dead-code • Location: appexport/export.go:337 (eea235b) +`stepStart = time.Now()` reassigned but never read; encrypt step uses its own `encStart`. Cosmetic timing-log gap. Fix: remove the dead assignment or add the missing `time.Since(stepStart)` log. + +### [CTRL-010] Setup writes config to a hardcoded path, ignoring resolved `Paths` +Severity: Info • Category: correctness • Location: setup/handlers.go:372 (eea235b) +`configPath := "/opt/docker/felhom-controller/controller.yaml"` hardcoded. On a deployment whose runtime config path differs, setup could write where the runtime never reads → `NeedsSetup` stays true, re-exposing the pre-auth wizard persistently. Fix: pass the resolved config path into `setup.NewServer`. + +### [CTRL-012] `os.MkdirAll`/`os.Remove`/`Sync` errors swallowed in export staging +Severity: Info • Category: error-handling • Location: appexport/export.go:280,298,319,571,605 (eea235b) +Staging dir/remove errors ignored → less precise later failures. Critical bundle paths (tar, EncryptFile, final rename) do check errors. Fix: `failJob` on staging MkdirAll errors. + +### [AGENT-005] `bindsToAction` is self-referential for one-shot jobs (defense-in-depth note) +Severity: Info • Category: invariant-drift • Location: signedjobs/runner.go:125-148; reconcile/gate.go:265-276 (d17b5ab) +For hub-queued jobs the gate compares the verified blob's params against an intent copied from the same blob → `bindsToAction` is tautologically true. By design (the executor's durable resolve+re-inspect is the real binding), but the gate adds no re-binding to the agent-surfaced `PendingOp`. The executor's data-bearing re-check is the sole backstop. Document explicitly or add a sanity assert of blob durable-id vs the host's current view. + +### [AGENT-010] localapi `TokenStore.Lookup` constant-time compare is a self-comparison +Severity: Info • Category: security • Location: localapi/tokenstore.go:138-154 (d17b5ab) +The secret-bearing step is the `byHash[want]` map lookup (variable-time); the subsequent `subtle.ConstantTimeCompare(want, byVMID[vmid])` compares a value to itself (always 1) — no timing benefit. Not exploitable (SHA-256 of a 256-bit random token). Risk is documentary: don't claim a timing guarantee the code doesn't provide. Hash-only invariant still HOLDS. + +### [AGENT-011] Host-wide metrics/disk topology served to any guest token +Severity: Info • Category: security • Location: localapi/host_metrics.go:28-49, disks.go:109-147 (d17b5ab) +`/host/metrics` and `/disks` return host-wide CPU/temp/SMART + storage targets to any authenticated guest, by design (one-customer-per-host model, documented inline). Revisit only if multi-tenancy is ever introduced. + +### [AGENT-013] `DestroyLXC` uses blanket destructive flags; narrowness is external (gated upstream) +Severity: Info • Category: security • Location: proxmox/mutate.go:105-115 (d17b5ab) +`DestroyLXC` always passes `force=1&purge=1&destroy-unreferenced-disks=1` with no internal vmid/scratch guard; safety depends entirely on the upstream `reconcile.Gate`. The only destructive consumer (signed-jobs runner) routes through the gate (verified). Add a routing test asserting no non-gated caller reaches it. + +### [AGENT-008] (positive) Fail-destructive defaults are correctly wired end-to-end +Severity: Info • Category: security • Location: storage/hostops.go:64-69,449-453; reconcile/classify.go:99-112; gate.go:146-150 (d17b5ab) +Confirmed compensating control: unprobed device → data-bearing; `NoopHostOps` → every device data-bearing; unknown op class → Destructive; nil verifier → refuse `pending_signature`; role default → system. The dangerous direction (ambiguity→allow-wipe) is the default nowhere. This downgrades AGENT-002/003 from Critical to Medium. + +### [CTRL-T2-6] `EnsureBaseStack` idempotency is per-stack — only filebrowser preserves its compose; traefik/cloudflared re-render +Severity: Info • Category: error-handling • Location: controller/internal/stacks/infra.go:27-148 (eea235b) +Invariant #3 HOLDS for the load-bearing concern: EnsureBaseStack is non-fatal (joined error to LOG, never panics, infra.go:67-69) + single-flight (TryLock), and filebrowser explicitly does NOT regenerate its docker-compose.yml when present (infra.go:122-132), preserving web.SyncFileBrowserMounts' storage mounts. Drift note: traefik/cloudflared guard only on `containerRunning()` and re-render+overwrite their rendered files when the container is stopped-but-present (acme.json preserved separately) — so "idempotent = doesn't overwrite" is true only for filebrowser. No correctness bug; the contract is narrower than the doc comment implies. + +### [AGENT-T2-4] `pbs.WaitVerify` returns nil on ANY stopped state, incl. a task-level FAILED verify +Severity: Info • Category: contract-mismatch • Location: agent/internal/pbs/client.go:155-172 (d17b5ab) +`WaitVerify` resolves nil the moment the task is not running, never consulting `st.OK()`/`ExitStatus`. By design — the caller re-lists snapshots for the authoritative per-snapshot `VerifyState==failed` corruption signal (verify.go:107-126, documented at client.go:143-144). Gap: a verify task that fails to *run at all* (vs finding corruption) is logged only as wait-success. Fix: return a sentinel when `!st.OK()` so a task-level failure is distinct from "verified clean". + +### [AGENT-T2-7] LIVE WaitTask validation gap — characterized, deliberately NOT closed +Severity: Info • Category: test-gap • Location: agent/internal/proxmox/task.go:104-156; errors.go:66-73 (d17b5ab) +The "POST 200 ≠ success; authz can fail at task execution" contract is exercised only against the mock, not a live PVE token whose role is missing a privilege (task stops non-OK with a 403 exitstatus). The non-OK→`*TaskError` privilege-extraction regex (`permRe`) matches the documented "Permission check failed (path, Priv)" form, but whether PVE emits that exact wording in the *task exitstatus* (vs only an HTTP 403 body) for the agent's async ops is unverified live. Fail-safe either way (non-OK → error), only the structured diagnostic degrades. LEFT OPEN per the audit's hard-rule #2 (do not close the live WaitTask gap). Close only by a live under-privileged-token run on felhom-pve. + +### [CONTRACT-1] agentapi client omits the `snapshotted` phase constant (cosmetic) +Severity: Info • Category: contract-mismatch • Location: CLIENT controller/internal/agentapi/client.go:163-168 ↔ SERVER agent/internal/localapi/server.go:109-115 (emits it at server.go:490) +The agentapi package exports `PhaseIdle/Running/Done/Failed` but not `PhaseSnapshotted`, though the server's `/backup/status` emits `snapshotted` (8B.2 early-resume). No runtime impact: the actual consumer (quiesce loop) defines its own literal `phaseSnapshotted="snapshotted"` and matches the wire string directly (quiesce.go:47,265-266). Fix: add the constant for completeness. + +--- + +## Contract checks (controller↔agent, controller↔hub) + +| Contract | Status | Note | +|---|---|---| +| controller `internal/agentapi` ↔ agent `internal/localapi` | **CHECKED — CLEAN** | All 12 client methods diffed field-by-field against their agent handlers + req/resp structs. Every route+method aligns; every request field present in the server struct (survives server `DisallowUnknownFields`); every response field the client reads is populated with the matching tag; status codes mapped correctly incl. destructive `/disks/format` 403 paths (needs_confirmation vs pending_op); error envelope `{ok,data,error}` shapes match; TLS leaf-SHA-256 pin enforced fail-closed. Only divergence = CONTRACT-1 (Info, cosmetic phase constant). | +| controller `internal/report/types.go` ↔ hub ingest | **CHECKED — CLEAN** | Hub `/api/host-report` stores the body as raw `ReportJSON` and structurally re-parses only `customer_id, controller_version, controller_url, customer_name, app_telemetry[]` — all re-parsed tags match controller `report.Report`/`AppTelemetry`/`metrics.LogIssue`. Tolerant ingest, no strict-struct mismatch. | +| Operator-signature namespace `felhom-op-v1` (agent verifier) ↔ opsign tool | clean | verifier.go:99-181 fixed namespace; `cmd/felhom-opsign` present. | + +**Endpoint coverage (client method → route → server handler):** Storage, BackupDue, StartBackup, BackupStatus, RestoreTestStatus, HostMetrics, Disks, AssignDisk, GuestAttach, GuestReboot, EjectDisk, FormatDisk — all CLEAN. Server-only `/snapshot`, `/rollback` are not called by this client (not mismatches). Full table in the contract auditor's notes. + +## Invariant checklist results + +**Agent** +- Data-bearing classification from device inspection only (never caller claims) — **HOLDS** @ localapi/disks.go:429-435 + storage/hostops.go:283-333 (caller "blank/force" claim explicitly ignored). Caveat AGENT-003 (transient blkid-error read drop). +- Fail-destructive on ambiguity — **HOLDS** @ hostops.go:64-69, classify.go:111, role.go:91-99 (AGENT-008). +- Signature gate before destructive op; no un-gated path — **HOLDS for signed-jobs** @ runner.go:134-148; **DRIFTED for inline path** → AGENT-001/AGENT-002 (authorized resource ≠ mutated resource; blank branch un-gated). +- Token store hash-only — **HOLDS** @ tokenstore.go:106-170 (only sha256 hashes persisted; AGENT-010 note). +- localapi self-scoping (cross-guest→403) — **HOLDS** @ server.go:263-308 (latent-discipline AGENT-009). +- Privileged ops narrow + TLS pinned — **HOLDS** @ proxmox/privileged.go:11-178 (3 fenced exceptions, no shell), tls.go:33-75 (CAFile | leaf-SHA256 pin; InsecureSkipVerify off by default). Blanket primitive AGENT-013 gated upstream. +- Signed-job sig verified before destructive op + nonce-replay prevented — **HOLDS** @ verifier.go:99-181 (verify over raw bytes, allow-list by key material), noncestore.go:114-135 (fsync-durable, fail-safe). Growth note AGENT-012. +- No `pct exec` in provision back-half / bootstrap.json chown 100000:100000 — **NOT verified this session** (Tier 2, provision pkg). +- No secrets in logs — **HOLDS** (token "Never logged"; escrow logs only short FPs; pty discards passphrase echo). + +**Controller** +- No `:latest` anywhere — **NOT fully verified** (Tier 3 templates/infra sweep pending; spot-check clean). +- Secrets never logged (keys only) — **HOLDS** spot-checked (handlers.go:56 logs presence only); CTRL-008 is at-rest file mode, not logging. +- Protected stacks unstoppable server-side — **HOLDS for the checks** @ router.go:411-414, manager.go:683-685, delete.go:86-88/289-291 (every mutating path checks); **DRIFTED on the data source** → CTRL-T2-3 (no fail-safe default; empty list = nothing protected). Restart on protected stacks intentionally allowed. +- `Deployed` set [true] only after `up -d`, reverted on failure (mem+disk) — **DRIFTED** → CTRL-T2-1 (disk persists `deployed:true` before compose; `Deploying` not persisted → ghost on crash) + CTRL-T2-4 (revert disk-write outside lock, Low). Happy/failure paths consistent *while the process lives*. +- restic behind running mutex — **HOLDS** @ backup.go:144-148, restore.go:31-42 (restic moved to agent; all entry points single-flight). +- filebrowser compose preserved if present — **HOLDS** @ infra.go:122-132 (does not regenerate when present). +- `EnsureBaseStack` non-fatal+idempotent — **HOLDS** @ infra.go:27-71 (non-fatal + TryLock single-flight); per-stack idempotency note CTRL-T2-6. +- CSRF on every state-changing route incl. setup — **HOLDS for runtime mux** (password-set), full route×CSRF/auth table in session notes; **setup uses weaker CSRF** → CTRL-007. +- Restore data-key fail-closed gate — **HOLDS** @ appbackup/restore_unit.go:38-50,113-116. +- At-rest crypto AEAD — **HOLDS** @ crypto/crypto.go:50-61 (AES-256-GCM, fresh nonce). FAB export Encrypt-then-MAC HOLDS w/ CTRL-002 caveat. + +**BUGHUNT (2026-02-25, v0.30.3) regression status — stacks concurrency items re-checked at eea235b:** +- H1 (double-deploy TOCTOU) — **FIXED** @ deploy.go:122-139 (atomic check-and-set of `Deploying` under one lock). +- H2 (delete-during-deploy) — **FIXED** @ delete.go:106-108, 309-311 (both reject when `Deploying`). +- H3 (ScanStacks overwrites Deployed during async deploy) — **FIXED** @ manager.go:291-296 (skips overwrite when `existing.Deploying`). +- H4 (shared appCfg pointer mutated by async goroutine) — **FIXED** @ deploy.go:344-353 (mutated under lock; in-mem `AppConfig=nil`). Residual disk-revert-outside-lock = CTRL-T2-4 (Low). +- H12 (deepCopyStack incomplete) — **FIXED** @ manager.go:556-638 (now deep-copies Containers, AppConfig+Env+LockedFields, HealthProbe, DeployFields+Options, OptionalConfig+Fields, Integrations, HealthCheck). +- 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. +- **Single path-segment validator (controller):** CTRL-001 shows `manifest.AppName` and friends reach `filepath.Join` unvalidated while API stack routes have `extractName`. Extract one `ValidateStackName` and apply at every untrusted boundary (import manifest, archive subdir names, export filenames). +- **Decrypt-then-rename pattern (controller):** CTRL-002 — the export side already does tmp→rename; the decrypt side should too. Centralize an `atomicDecrypt(verify, then rename)`. +- (Carried from BUGHUNT, still relevant where code survived 8C) atomic-write helper unification; injected-logger consistency (`crypto.DecryptMap` global log). + +## Test-gap analysis + +- **appexport import path** — ZERO tests in the package (no `_test.go` existed before this audit's evidence test). The highest-severity finding (CTRL-001) lived in untested code. Import/restore/crypto-decrypt all lack unit coverage. +- **agent inline format/wipe path** (localapi/disks.go format handler) — needs a swap-device-between-inspect-and-format test (would catch AGENT-001/002). +- **storage classify under inspection error** — no test for blkid-error/lsblk-success (AGENT-003). +- **stacks deploy crash-safety** — no test covers a crash between `app.yaml` write and compose success (CTRL-T2-1); the ghost-deployed state is untested. Needs a Manager+provider+fs harness or the manual repro given. +- **agent proxmox WaitTask** — `task_test.go` covers the mock happy/non-OK paths but not empty-node UPID (AGENT-T2-1) or persistent-empty-status (AGENT-T2-2); the live non-OK exitstatus wording is unverified (AGENT-T2-7, deliberately left open). +- **agent reconcile Recover()** ground-truth IS exercised (recover.go uses live `ListLXC`), but the no-UPID-in-flight side-effect-verification path (AGENT-T2-3) and lane-panic isolation (AGENT-T2-6) are untested. + +## Dead code inventory (staticcheck U1000, controller) + +- `cmd/controller/main.go:1172` `fileExists` — unused (also BUGHUNT L1; still present). +- `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 + +- Session unattended; conservative assumptions inline. Severity reflects consequence; confidence kept honest (most findings verified-static, CTRL-001 verified-by-test). +- **Assumption:** "one customer per host" deployment model holds (makes AGENT-011 Info not Medium). Documented in arch doc 03. +- **Assumption:** AGENT-001/002 trigger (USB `/dev` re-enumeration in the inspect→mkfs window) is rare on these N100 boxes → rated Medium not High; raise to High if USB churn proves common. +- gofmt noise = CRLF/autocrlf artifact, not a finding. +- Two controller Tier-1 auditors independently numbered findings CTRL-001..; merged + renumbered here into one namespace. +- **Full mutating-route × CSRF/auth coverage table** (password-set path) was produced and shows no uncovered mutating runtime route; available on request — every `/api/*` + `/` subtree wrapped in RequireAuth+CsrfProtect (main.go:703-728); only `/api/health` (GET) and `/api/host-metrics` (GET) are CSRF-exempt by nature. + +## What was NOT covered (defines next session) + +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.