Files
felhom.eu/documentation/audits/bughunt-reconcile-2026-06-13.md
T
admin 4320c01915 docs(audits): authoritative remediation records (deep-sweep + bughunt-reconcile)
Brings the 2026-06-13 audit + reconciliation under documentation/audits/ with
per-finding remediation status (controller v0.59.0 fixes deployed to demo;
AGENT-001 fix prepared/pending review).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
2026-06-13 19:28:25 +02:00

260 lines
26 KiB
Markdown
Raw Blame History

This file contains ambiguous Unicode characters
This file contains Unicode characters that might be confused with other characters. If you think that this is intentional, you can safely ignore this warning. Use the Escape button to reveal them.
> **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 H1H4/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): **LowMedium.** `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 |
| L7L9, 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 | LowMedium | 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.