docs(changelog): v0.59.0 audit fixes (CTRL-001, CTRL-T2-1, H10, M2)

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

CHANGELOG.md

@@ -1,5 +1,34 @@
 ## Changelog
 
+### v0.59.0 — security/crash-safety fixes from the 2026-06-13 audit (2026-06-13)
+
+Fixes the validated findings from the deep-sweep audit + BUGHUNT reconciliation
+(records under `felhom.eu/documentation/audits/`). All shipped with permanent
+regression tests.
+
+- **CTRL-001 (path traversal on `.fab` import) — High.** `appexport.UnmarshalManifest`
+  did zero validation; the attacker-controlled `manifest.AppName` / `HDDSubdirs` /
+  `VolumeNames` reached `filepath.Join`+`MkdirAll`/`extractTar` (restore.go:339/606/678),
+  so `../..` in any escaped the stacks / HDD destination dir (arbitrary write as the
+  controller). New `appexport.ValidateSegment` + `validateManifestPaths`;
+  `UnmarshalManifest` now fails the parse on a traversal segment, with defence-in-depth
+  guards at the HDD-subdir and volume-name join loops. `ConfigFiles` intentionally not
+  validated (holds dotfiles, never used in a restore join).
+- **CTRL-T2-1 (ghost-deployed stack on crash) — High.** `DeployStack` wrote `app.yaml`
+  `deployed:true` to disk *before* the async `docker compose up -d`; a crash during the
+  image-pull window left a ghost-deployed stack with no containers that the app then
+  refused to redeploy. The env is now persisted `deployed:false` (transitional) and
+  flipped to `deployed:true` by `runComposeDeploy` only after `up -d` succeeds. The
+  in-memory flag still goes true during the pull (no stale "Telepítés" button).
+- **H10 (plaintext secret on encrypt failure) — fail-closed.** `SaveAppConfig` logged a
+  WARN then fell through to persist the secret in plaintext on a `crypto.Encrypt` error.
+  Now returns an error instead — never writes plaintext.
+- **M2 (misleading lock).** `backup.Manager.SetStackProvider` was mutex-guarded while all
+  reads were unlocked; it is init-only (one call before any goroutine), so the lock was
+  removed and the contract documented. No behaviour change.
+- **AGENT-001 (wrong-disk wipe race)** is fixed on the agent branch `fix/agent-001-wipe-durable-reresolve`
+  (PENDING REVIEW — not deployed; stored out-of-band per the supervised-merge rule).
+
 ### v0.58.0 — infra-protection prevention layer for the OS/Docker-data split (2026-06-13)
 
 Phase 2 of the storage-split slice (Phase 1 = felhom-agent golden + provision). The OS rootfs and

controller/internal/appexport/manifest.go

@@ -38,5 +38,11 @@ func UnmarshalManifest(data []byte) (*Manifest, error) {
 	if err := json.Unmarshal(data, &m); err != nil {
 		return nil, err
 	}
+	// [CTRL-001] Reject path-traversal in any segment used to build a filesystem
+	// path on import (app_name, hdd_subdirs, volume_names). A hostile .fab must
+	// fail to parse rather than escape the stacks / HDD destination dir.
+	if err := validateManifestPaths(&m); err != nil {
+		return nil, err
+	}
 	return &m, nil
 }

controller/internal/appexport/restore.go

@@ -589,6 +589,11 @@ func (e *Exporter) restoreHDDData(tmpDir string, manifest *Manifest, composePath
 	}
 
 	for _, subdir := range manifest.HDDSubdirs {
+		// [CTRL-001] defence-in-depth: refuse any subdir that is not a single
+		// safe segment before it reaches MkdirAll/extractTar on a user drive.
+		if err := ValidateSegment("hdd_subdir", subdir); err != nil {
+			return err
+		}
 		tarPath := filepath.Join(hddDir, subdir+".tar")
 		tarInfo, err := os.Stat(tarPath)
 		if err != nil {
@@ -670,6 +675,11 @@ func (e *Exporter) restoreVolumeData(tmpDir string, manifest *Manifest) error {
 	volDir := filepath.Join(tmpDir, "data", "volumes")
 
 	for _, volName := range manifest.VolumeNames {
+		// [CTRL-001] defence-in-depth: refuse any volume name that is not a
+		// single safe segment before it reaches a tar path / docker volume op.
+		if err := ValidateSegment("volume_name", volName); err != nil {
+			return err
+		}
 		tarPath := filepath.Join(volDir, volName+".tar")
 		tarInfo, err := os.Stat(tarPath)
 		if err != nil {

controller/internal/appexport/traversal_regression_test.go

@@ -0,0 +1,82 @@
+package appexport
+
+import (
+	"encoding/json"
+	"strings"
+	"testing"
+)
+
+// Regression test for [CTRL-001] (path traversal on .fab import). Originated as
+// a failing audit test (audit/2026-06-13-deep-sweep); now a permanent guard.
+// UnmarshalManifest must REJECT any manifest whose AppName / HDDSubdirs /
+// VolumeNames contain a path-traversal or separator, and ACCEPT legitimate
+// single-segment names. Do NOT weaken these assertions.
+
+func mustManifestJSON(t *testing.T, m Manifest) []byte {
+	t.Helper()
+	b, err := json.Marshal(m)
+	if err != nil {
+		t.Fatalf("marshal: %v", err)
+	}
+	return b
+}
+
+func TestUnmarshalManifestRejectsTraversal(t *testing.T) {
+	cases := []struct {
+		name string
+		m    Manifest
+	}{
+		{"appname-parent", Manifest{Version: 1, AppName: "../evil"}},
+		{"appname-deep", Manifest{Version: 1, AppName: "../../etc/cron.d/x"}},
+		{"appname-absolute", Manifest{Version: 1, AppName: "/etc/cron.d/x"}},
+		{"appname-dotdot", Manifest{Version: 1, AppName: ".."}},
+		{"appname-empty", Manifest{Version: 1, AppName: ""}},
+		{"appname-backslash", Manifest{Version: 1, AppName: `..\evil`}},
+		{"hdd-subdir-escape", Manifest{Version: 1, AppName: "romm", HDDSubdirs: []string{"../../mnt"}}},
+		{"volume-escape", Manifest{Version: 1, AppName: "romm", VolumeNames: []string{"../../var/lib"}}},
+	}
+	for _, tc := range cases {
+		t.Run(tc.name, func(t *testing.T) {
+			_, err := UnmarshalManifest(mustManifestJSON(t, tc.m))
+			if err == nil {
+				t.Fatalf("CTRL-001 regression: UnmarshalManifest accepted a traversal manifest %+v; expected rejection", tc.m)
+			}
+		})
+	}
+}
+
+func TestUnmarshalManifestAcceptsLegitNames(t *testing.T) {
+	m := Manifest{
+		Version:     1,
+		AppName:     "paperless-ngx",
+		HDDSubdirs:  []string{"felhom-usb", "romm"},
+		VolumeNames: []string{"adventurelog_postgres_data", "romm_redis-data"},
+		ConfigFiles: []string{".felhom.yml", "docker-compose.yml", "app.yaml"}, // dotfiles must NOT be rejected
+	}
+	got, err := UnmarshalManifest(mustManifestJSON(t, m))
+	if err != nil {
+		t.Fatalf("CTRL-001 regression: UnmarshalManifest rejected a legitimate manifest: %v", err)
+	}
+	if got.AppName != "paperless-ngx" {
+		t.Fatalf("AppName round-trip mismatch: %q", got.AppName)
+	}
+}
+
+func TestValidateSegment(t *testing.T) {
+	good := []string{"romm", "paperless-ngx", "adventurelog_postgres_data", "felhom-usb", "a", "App1.2_3-4"}
+	for _, s := range good {
+		if err := ValidateSegment("x", s); err != nil {
+			t.Errorf("ValidateSegment(%q) = %v; want nil", s, err)
+		}
+	}
+	bad := []string{"", ".", "..", "../x", "a/b", `a\b`, "/abs", ".hidden", "-leadingdash", "a/../b"}
+	for _, s := range bad {
+		if err := ValidateSegment("x", s); err == nil {
+			t.Errorf("ValidateSegment(%q) = nil; want rejection", s)
+		}
+	}
+	// Sanity: a rejected value's message names the kind, for operator clarity.
+	if err := ValidateSegment("app_name", "../x"); err == nil || !strings.Contains(err.Error(), "app_name") {
+		t.Errorf("expected error mentioning app_name, got %v", err)
+	}
+}

controller/internal/appexport/validate.go

@@ -0,0 +1,66 @@
+package appexport
+
+import (
+	"fmt"
+	"path/filepath"
+	"regexp"
+	"strings"
+)
+
+// safeSegment matches a single safe path component: starts with an
+// alphanumeric, then alphanumerics / dot / dash / underscore. It cannot be
+// "." or ".." (must start alnum), cannot contain a path separator, and cannot
+// be an absolute path. This covers the legitimate values these fields hold —
+// app slugs (e.g. "paperless-ngx"), HDD mount basenames (e.g. "felhom-usb"),
+// and docker volume names (e.g. "adventurelog_postgres_data").
+var safeSegment = regexp.MustCompile(`^[a-zA-Z0-9][a-zA-Z0-9._-]*$`)
+
+// ValidateSegment rejects any value that is not a single safe path component.
+// It is the guard for [CTRL-001]: manifest fields that reach filepath.Join with
+// a trusted base (AppName, HDDSubdirs, VolumeNames) are fully attacker-controlled
+// JSON inside an imported .fab, so a value like "../../etc/cron.d/x" must be
+// refused before it can escape the stacks / HDD destination directory.
+//
+// NOTE: this is deliberately NOT applied to manifest.ConfigFiles — those are
+// dotfile-bearing names (e.g. ".felhom.yml") that are never used in a restore
+// join (restoreConfig enumerates the extracted dir via os.ReadDir, whose names
+// are already single components).
+func ValidateSegment(kind, s string) error {
+	if s == "" {
+		return fmt.Errorf("appexport: empty %s", kind)
+	}
+	if s == "." || s == ".." {
+		return fmt.Errorf("appexport: %s %q is a path-traversal segment", kind, s)
+	}
+	if strings.ContainsAny(s, `/\`) || strings.ContainsRune(s, filepath.Separator) {
+		return fmt.Errorf("appexport: %s %q must not contain a path separator", kind, s)
+	}
+	if filepath.IsAbs(s) {
+		return fmt.Errorf("appexport: %s %q must not be an absolute path", kind, s)
+	}
+	if !safeSegment.MatchString(s) {
+		return fmt.Errorf("appexport: %s %q is not a safe single-segment name", kind, s)
+	}
+	return nil
+}
+
+// validateManifestPaths checks every manifest field that is later used as a
+// path segment in a filepath.Join against a trusted base. Called from
+// UnmarshalManifest so a hostile bundle fails the parse, before executeImport
+// can MkdirAll/extract into a traversed location.
+func validateManifestPaths(m *Manifest) error {
+	if err := ValidateSegment("app_name", m.AppName); err != nil {
+		return err
+	}
+	for _, s := range m.HDDSubdirs {
+		if err := ValidateSegment("hdd_subdir", s); err != nil {
+			return err
+		}
+	}
+	for _, v := range m.VolumeNames {
+		if err := ValidateSegment("volume_name", v); err != nil {
+			return err
+		}
+	}
+	return nil
+}

controller/internal/backup/backup.go

@@ -388,11 +388,15 @@ func (m *Manager) releaseRunning() {
 }
 
 // SetStackProvider sets the stack data provider for app data discovery.
-// Write is protected by mutex since stackProvider is read by concurrent goroutines.
+//
+// M2: this MUST be called exactly once during single-threaded startup (main.go),
+// before the scheduler / HTTP server / any backup goroutine starts. That write
+// then happens-before all the (unlocked) reads of m.stackProvider, so no data
+// race exists. The earlier mutex on this write was misleading — it implied
+// runtime concurrency the reads don't honour; removed to make the init-only
+// contract explicit. Do NOT call this after startup.
 func (m *Manager) SetStackProvider(provider StackDataProvider) {
-	m.mu.Lock()
 	m.stackProvider = provider
-	m.mu.Unlock()
 }
 
 // GetStackHDDMounts returns HDD mount paths for the named stack via the stack provider.

controller/internal/stacks/deploy.go

@@ -291,15 +291,24 @@ func (m *Manager) DeployStack(req DeployRequest) (string, error) {
 		}
 	}
 
-	// Save app.yaml
+	// Save app.yaml.
+	// CTRL-T2-1: persist the env now, but mark the ON-DISK state Deployed:false
+	// until `docker compose up -d` actually succeeds (done in runComposeDeploy).
+	// A crash/power-loss during the image-pull window must NOT leave a
+	// ghost-deployed stack on disk (Deployed:true with no containers), which
+	// DeployStack would then refuse to redeploy. The IN-MEMORY Deployed flag is
+	// still set true below to preserve the "no stale Telepítés button during
+	// pull" UX; only the durable record waits for success.
 	appCfg := &AppConfig{
-		Deployed:     true,
+		Deployed:     true, // in-memory truth (see below); the disk write overrides to false
 		DeployedAt:   time.Now().UTC().Format(time.RFC3339),
 		Env:          env,
 		LockedFields: lockedFields,
 	}
 
-	if err := SaveAppConfig(stackDir, appCfg, m.encKey, SensitiveEnvVars(&meta)); err != nil {
+	diskCfg := *appCfg
+	diskCfg.Deployed = false // transitional: env saved, not yet marked deployed
+	if err := SaveAppConfig(stackDir, &diskCfg, m.encKey, SensitiveEnvVars(&meta)); err != nil {
 		clearDeploying()
 		return "", fmt.Errorf("saving app config: %w", err)
 	}
@@ -359,6 +368,25 @@ func (m *Manager) runComposeDeploy(name, stackDir string, env map[string]string,
 
 	m.logger.Printf("[INFO] [stacks] Stack %s deployed successfully (took %.1fs)", name, time.Since(start).Seconds())
 
+	// CTRL-T2-1: compose up -d succeeded — only NOW mark deployed on disk.
+	// (DeployStack wrote the env with Deployed:false; flip it true here so the
+	// durable record matches reality and survives a restart.)
+	meta := LoadMetadata(stackDir)
+	if err := SaveAppConfig(stackDir, appCfg, m.encKey, SensitiveEnvVars(&meta)); err != nil {
+		// Running but not durably recorded as deployed. Revert so the customer
+		// can cleanly redeploy rather than be stuck with a half-recorded stack.
+		m.logger.Printf("[ERROR] [stacks] Stack %s: compose succeeded but persisting deployed state failed: %v — reverting", name, err)
+		m.mu.Lock()
+		if s, ok := m.stacks[name]; ok {
+			s.Deployed = false
+			s.Deploying = false
+			s.DeployError = "deploy succeeded but state could not be saved: " + err.Error()
+			s.AppConfig = nil
+		}
+		m.mu.Unlock()
+		return
+	}
+
 	// Clear deploying flag
 	m.mu.Lock()
 	if s, ok := m.stacks[name]; ok {
@@ -653,14 +681,17 @@ func SaveAppConfig(stackDir string, cfg *AppConfig, encKey []byte, sensitiveVars
 	}
 	for k, v := range cfg.Env {
 		if encKey != nil && sensitiveSet[k] && !crypto.IsEncrypted(v) && v != "" {
-			if enc, err := crypto.Encrypt(encKey, v); err == nil {
+			enc, err := crypto.Encrypt(encKey, v)
+			if err != nil {
+				// H10 (fail-closed): NEVER persist a sensitive value in plaintext.
+				// Earlier code logged a WARN and fell through to a plaintext write;
+				// that leaked the secret to disk. Abort the save instead — callers
+				// already propagate this error and the deploy fails cleanly.
+				return fmt.Errorf("encrypting sensitive env var %q (refusing to persist plaintext): %w", k, err)
+			}
 			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)
-			}
 		}
 		saveCfg.Env[k] = v
 	}

controller/internal/stacks/deploy_crashsafety_regression_test.go

@@ -0,0 +1,101 @@
+package stacks
+
+import (
+	"os"
+	"path/filepath"
+	"strings"
+	"testing"
+)
+
+// Regression tests for the deploy-lifecycle slice (audit/2026-06-13):
+//   - H10: SaveAppConfig must FAIL CLOSED on an encryption error (never persist
+//     a sensitive value in plaintext).
+//   - CTRL-T2-1: the durable record must read as NOT deployed until a deploy
+//     actually completes, so a crash during the image-pull window leaves a
+//     redeployable stack rather than a ghost-deployed one.
+
+// TestSaveAppConfigFailsClosedOnEncryptError — H10. Originated as a failing
+// reconcile test; now a permanent guard. A bad-length encKey makes crypto.Encrypt
+// fail; SaveAppConfig must return an error and write NO plaintext secret.
+func TestSaveAppConfigFailsClosedOnEncryptError(t *testing.T) {
+	dir := t.TempDir()
+	const secret = "supersecret-pw-do-not-leak"
+	cfg := &AppConfig{Deployed: true, Env: map[string]string{"DB_PASSWORD": secret}}
+
+	badKey := []byte("short") // non-nil, invalid AES key length → crypto.Encrypt errors
+
+	err := SaveAppConfig(dir, cfg, badKey, []string{"DB_PASSWORD"})
+	if err == nil {
+		t.Fatalf("H10: SaveAppConfig returned nil on an encrypt failure — expected a fail-closed error")
+	}
+
+	// No app.yaml should have been written; even if one was, it must not contain
+	// the plaintext secret.
+	if data, readErr := os.ReadFile(filepath.Join(dir, "app.yaml")); readErr == nil {
+		if strings.Contains(string(data), secret) {
+			t.Fatalf("H10: app.yaml contains the secret in plaintext after an encrypt failure:\n%s", data)
+		}
+	}
+}
+
+// TestSaveAppConfigEncryptsWithGoodKey — companion: a valid key encrypts the
+// sensitive var (it must not appear in plaintext) and the save succeeds.
+func TestSaveAppConfigEncryptsWithGoodKey(t *testing.T) {
+	dir := t.TempDir()
+	const secret = "supersecret-pw"
+	key := make([]byte, 32) // valid AES-256 key
+	cfg := &AppConfig{Deployed: true, Env: map[string]string{"DB_PASSWORD": secret}}
+
+	if err := SaveAppConfig(dir, cfg, key, []string{"DB_PASSWORD"}); err != nil {
+		t.Fatalf("SaveAppConfig with a valid key failed: %v", err)
+	}
+	data, err := os.ReadFile(filepath.Join(dir, "app.yaml"))
+	if err != nil {
+		t.Fatalf("reading app.yaml: %v", err)
+	}
+	if strings.Contains(string(data), secret) {
+		t.Fatalf("sensitive value was written in plaintext despite a valid key:\n%s", data)
+	}
+}
+
+// TestTransitionalDeployStateReadsNotDeployed — CTRL-T2-1. DeployStack now
+// writes the env with Deployed:false before launching compose, and flips it to
+// Deployed:true only after `up -d` succeeds. This locks the durable-state
+// semantics ScanStacks relies on: a stack persisted in the transitional state
+// (the on-disk state left by a crash mid-pull) MUST read as not-deployed, so the
+// customer can redeploy. The full crash-window is integration-level (needs a
+// real compose); see the audit's manual repro.
+func TestTransitionalDeployStateReadsNotDeployed(t *testing.T) {
+	key := make([]byte, 32)
+	sensitive := []string{"DB_PASSWORD"}
+
+	// Transitional state DeployStack writes BEFORE compose succeeds.
+	transitional := t.TempDir()
+	if err := SaveAppConfig(transitional, &AppConfig{
+		Deployed: false, DeployedAt: "2026-06-13T00:00:00Z",
+		Env: map[string]string{"DB_PASSWORD": "x"},
+	}, key, sensitive); err != nil {
+		t.Fatalf("saving transitional config: %v", err)
+	}
+	got := LoadAppConfig(transitional)
+	if got == nil {
+		t.Fatal("LoadAppConfig returned nil for the transitional state")
+	}
+	// This is exactly the expression ScanStacks uses: deployed := cfg != nil && cfg.Deployed
+	if got.Deployed {
+		t.Fatalf("CTRL-T2-1: a deploy that did not complete reads as Deployed=true — ghost-deployed; "+
+			"DeployStack must persist Deployed:false until compose succeeds")
+	}
+
+	// Completed state (what runComposeDeploy writes on success) reads deployed.
+	completed := t.TempDir()
+	if err := SaveAppConfig(completed, &AppConfig{
+		Deployed: true, DeployedAt: "2026-06-13T00:00:00Z",
+		Env: map[string]string{"DB_PASSWORD": "x"},
+	}, key, sensitive); err != nil {
+		t.Fatalf("saving completed config: %v", err)
+	}
+	if c := LoadAppConfig(completed); c == nil || !c.Deployed {
+		t.Fatalf("CTRL-T2-1: a completed deploy must read as Deployed=true, got %+v", c)
+	}
+}