From 02d37a110b9b60bf37b6e77d3507946064e5ed48 Mon Sep 17 00:00:00 2001 From: kisfenyo Date: Tue, 7 Jul 2026 18:34:52 +0200 Subject: [PATCH] =?UTF-8?q?fix(config):=20F-C2-1=20=E2=80=94=20stop=20os.E?= =?UTF-8?q?xpandEnv=20corrupting=20the=20bcrypt=20password=5Fhash=20(v0.10?= =?UTF-8?q?3.0)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Claude-Session: https://claude.ai/code/session_01PSK5g6qYLknKj8u3QAFEr6 --- CHANGELOG.md | 18 +++++ controller/internal/config/config.go | 14 ++-- controller/internal/config/config_test.go | 87 +++++++++++++++++++++++ 3 files changed, 113 insertions(+), 6 deletions(-) create mode 100644 controller/internal/config/config_test.go diff --git a/CHANGELOG.md b/CHANGELOG.md index 6a3ed82..d75b059 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,23 @@ ## Changelog +### v0.103.0 — F-C2-1: config loader no longer corrupts a bcrypt password_hash (silent auth bug) (2026-07-07) + +Fixes campaign-2 finding **F-C2-1** (`felhom.eu/documentation/tests/CAMPAIGN-2-2026-07-07.md`). +`loadAndParse` and `LoadFromBytes` ran `os.ExpandEnv` over the **entire** YAML before parse. A bcrypt +hash (`$2a$10$…`) is full of `$word` sequences, so `ExpandEnv` silently replaced each with its (usually +empty) env value — corrupting `web.password_hash` on load (proven: `$2a$10$N9qo8uL…` → `"a0"`). A silent +auth-integrity bug. + +- **Fix:** removed both `os.ExpandEnv` calls (`config.go` :234 loadAndParse, :249 LoadFromBytes) — parse + the raw bytes directly. The sanctioned, typed env path (`applyEnvOverrides` → `FELHOM_WEB_PASSWORD_HASH`, + applied after parse) is unchanged; no shipped `controller.yaml` relies on file-level `${VAR}` + interpolation (only `docker-compose.yml` uses `${DOMAIN}`, which is compose-level). +- **Behavior change:** a literal `${VAR}` in a controller.yaml value is now preserved verbatim (was + expanded). No repo config depends on the old behavior. +- Tests (`config_test.go`): bcrypt hash loads byte-identical (file + bytes paths; red-proof: pre-fix + `ExpandEnv` mangles it to `"a0"` → FAIL, demonstrated + reverted); `FELHOM_WEB_PASSWORD_HASH` override + still wins; literal `${VAR}` preserved. + ### v0.102.0 — async restore family: no more proxy-timeout error page on a succeeding restore (2026-07-06) Re-adjudicates campaign **F4** (`felhom.eu/documentation/audits/RERUN-p1p3-2026-07-06.md`): all three diff --git a/controller/internal/config/config.go b/controller/internal/config/config.go index 11e0d36..cc8bbac 100644 --- a/controller/internal/config/config.go +++ b/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) diff --git a/controller/internal/config/config_test.go b/controller/internal/config/config_test.go new file mode 100644 index 0000000..4bdb88a --- /dev/null +++ b/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) + } +}