fix(config): F-C2-1 — stop os.ExpandEnv corrupting the bcrypt password_hash (v0.103.0)

loadAndParse + LoadFromBytes ran os.ExpandEnv over the whole YAML before parse;
a bcrypt hash ($2a$10$...) is full of $word sequences that get replaced with
empty env values, silently corrupting web.password_hash on load (a silent
auth-integrity bug: $2a$10$N9qo8uL... -> "a0"). Remove both ExpandEnv calls;
parse raw bytes. The typed applyEnvOverrides path (FELHOM_WEB_PASSWORD_HASH) is
the sanctioned env mechanism and is unchanged. Tests + red-proof.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PSK5g6qYLknKj8u3QAFEr6

controller/internal/config/config.go

@@ -230,11 +230,13 @@ func loadAndParse(path string) (*Config, error) {
 		return nil, fmt.Errorf("reading config file: %w", err)
 	}
 
-	// Expand environment variables in the YAML
-	expanded := os.ExpandEnv(string(data))
-
+	// F-C2-1: parse the RAW bytes — do NOT os.ExpandEnv the whole file. A bcrypt password_hash
+	// ($2a$10$…) is full of `$word` sequences that ExpandEnv silently replaces with (usually empty)
+	// env values, corrupting the stored hash on load (a silent auth-integrity bug). Typed env
+	// overrides are the sanctioned mechanism — applyEnvOverrides (e.g. FELHOM_WEB_PASSWORD_HASH),
+	// applied after parse. No shipped controller.yaml relies on file-level ${VAR} interpolation.
 	cfg := &Config{}
-	if err := yaml.Unmarshal([]byte(expanded), cfg); err != nil {
+	if err := yaml.Unmarshal(data, cfg); err != nil {
 		return nil, fmt.Errorf("parsing config file: %w", err)
 	}
 
@@ -246,9 +248,9 @@ func loadAndParse(path string) (*Config, error) {
 
 // LoadFromBytes parses YAML config from raw bytes (for validation without file I/O).
 func LoadFromBytes(data []byte) (*Config, error) {
-	expanded := os.ExpandEnv(string(data))
+	// F-C2-1: parse RAW bytes — see loadAndParse. os.ExpandEnv would corrupt a bcrypt password_hash.
 	cfg := &Config{}
-	if err := yaml.Unmarshal([]byte(expanded), cfg); err != nil {
+	if err := yaml.Unmarshal(data, cfg); err != nil {
 		return nil, fmt.Errorf("parsing config: %w", err)
 	}
 	applyDefaults(cfg)

controller/internal/config/config_test.go

@@ -0,0 +1,87 @@
+package config
+
+import (
+	"os"
+	"path/filepath"
+	"strings"
+	"testing"
+)
+
+// A real bcrypt hash for "correct horse" — full of `$word` sequences ($2a, $10, and a $-laden salt)
+// that os.ExpandEnv would each replace with an (empty) env value, silently corrupting the hash.
+const bcryptHash = `$2a$10$N9qo8uLOickgx2ZMRZoMyeIjZAgcfl7p92ldGxad68LJZdL17lhWy`
+
+func minimalYAML(passwordHash string) string {
+	return `customer:
+  id: demo
+  domain: demo.example.com
+web:
+  password_hash: "` + passwordHash + `"
+`
+}
+
+// TestLoadFromBytes_BcryptHashIntact is the F-C2-1 headline red-proof: a bcrypt password_hash must
+// survive config load BYTE-IDENTICAL. On the pre-fix code (os.ExpandEnv over the whole file) the
+// `$2a$`/`$10$`/`$…salt` segments get expanded to empty → the stored hash is corrupted → silent
+// auth breakage. This test FAILS on that pre-fix path.
+func TestLoadFromBytes_BcryptHashIntact(t *testing.T) {
+	// Make the corruption visible if ExpandEnv ever creeps back: set env vars matching hash segments.
+	t.Setenv("2a", "CORRUPT")
+	t.Setenv("10", "CORRUPT")
+
+	cfg, err := LoadFromBytes([]byte(minimalYAML(bcryptHash)))
+	if err != nil {
+		t.Fatalf("LoadFromBytes: %v", err)
+	}
+	if cfg.Web.PasswordHash != bcryptHash {
+		t.Fatalf("password_hash corrupted on load:\n  want %q\n  got  %q", bcryptHash, cfg.Web.PasswordHash)
+	}
+}
+
+// TestLoadAndParse_BcryptHashIntact proves the same for the file path (loadAndParse / Load).
+func TestLoadAndParse_BcryptHashIntact(t *testing.T) {
+	t.Setenv("2a", "CORRUPT")
+	dir := t.TempDir()
+	p := filepath.Join(dir, "controller.yaml")
+	if err := os.WriteFile(p, []byte(minimalYAML(bcryptHash)), 0o600); err != nil {
+		t.Fatal(err)
+	}
+	cfg, err := loadAndParse(p)
+	if err != nil {
+		t.Fatalf("loadAndParse: %v", err)
+	}
+	if cfg.Web.PasswordHash != bcryptHash {
+		t.Fatalf("password_hash corrupted on file load:\n  want %q\n  got  %q", bcryptHash, cfg.Web.PasswordHash)
+	}
+}
+
+// TestEnvOverride_PasswordHash proves the sanctioned typed env path still wins (applyEnvOverrides).
+func TestEnvOverride_PasswordHash(t *testing.T) {
+	override := `$2a$10$differentHASHvalueForOverrideTestXXXXXXXXXXXXXXXXXXXXXXXX`
+	t.Setenv("FELHOM_WEB_PASSWORD_HASH", override)
+	cfg, err := LoadFromBytes([]byte(minimalYAML(bcryptHash)))
+	if err != nil {
+		t.Fatalf("LoadFromBytes: %v", err)
+	}
+	if cfg.Web.PasswordHash != override {
+		t.Fatalf("FELHOM_WEB_PASSWORD_HASH override not applied:\n  want %q\n  got  %q", override, cfg.Web.PasswordHash)
+	}
+}
+
+// TestLiteralDollarVarPreserved documents the behavior change: a literal ${VAR} in a value is now
+// kept verbatim (no file-level expansion). Session secrets or comments with `$` survive intact.
+func TestLiteralDollarVarPreserved(t *testing.T) {
+	y := `customer:
+  id: demo
+  domain: demo.example.com
+web:
+  session_secret: "literal-${NOT_EXPANDED}-value"
+`
+	cfg, err := LoadFromBytes([]byte(y))
+	if err != nil {
+		t.Fatalf("LoadFromBytes: %v", err)
+	}
+	if !strings.Contains(cfg.Web.SessionSecret, "${NOT_EXPANDED}") {
+		t.Fatalf("literal ${VAR} was expanded (should be preserved): %q", cfg.Web.SessionSecret)
+	}
+}