admin/felhom.eu
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity
Files
0dbd954fec9043876184ae11ca6ef4fbb2bc7cf6
felhom.eu/documentation/audits/deep-sweep-2026-06-13.md
T
admin 280781115d docs(audits): AGENT-001 -> MERGED + DEPLOYED (agent v0.30.0) 
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
2026-06-13 22:32:23 +02:00

60 KiB
Raw Blame History

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) MERGED + DEPLOYED (agent v0.30.0; verified non-destructively on demo, no rollback) agent main 8572a0f (merge of d96e5bd; REPORT 1c37a16)
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:

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:

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:

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:

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:

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:

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:

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:

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:

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:

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:

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:

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 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:

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:

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":<other>} 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:

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:

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:

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:

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 <db> 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 — </script> 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.
Reference in New Issue View Git Blame Copy Permalink