diff --git a/CHANGELOG.md b/CHANGELOG.md index 7b3e92c..5208b22 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,26 @@ ## Changelog +### v0.76.0 — campaign-#3 hardening: settings recovery, restore-name validation, quiesce-marker quarantine (2026-06-22) + +Three controller findings from chaos campaign #3, all small, all controller-side. + +- **S1 [MEDIUM] — no more crash-loop on a corrupt `settings.json`.** `internal/settings/settings.go`: + `save()` now writes a last-known-good `.bak` **after** the primary rename succeeds (best-effort); + `Load()` on a JSON-parse error recovers from `.bak` (re-promotes it to primary) and, failing that, + **preserves** the corrupt file as `*.corrupt-` and starts on safe defaults — never returns the + error that made `main.go` `Fatalf`/crash-loop. New `Settings.LoadWarning` surfaced as a dashboard + banner. (`main.go`'s `Fatalf` stays — now only the genuine IO-unreadable path is fatal.) Recovery is + safe: an empty `PasswordHash` falls back to `controller.yaml`, the storage registry re-discovers. +- **F2 [MEDIUM, defense-in-depth] — validate `stack_name` against path traversal.** New + `web/validate.go` `validStackName` (single segment; rejects `/`, `\`, `..`, NUL). Gated in + `backupRestoreHandler` (`handlers.go`) and `apiExportStart` (`handler_export.go`) before any + restore/export work. (Storage `where=` was already validated by `gateWhere`.) +- **S3 [LOW] — quarantine a corrupt quiesce marker.** `quiesce/quiesce.go` `readMarker` now logs a + `[WARN]` + renames a bad-JSON marker to `*.corrupt-` instead of silently dropping it (still + returns "no marker" → no recovery, the correct contract). +- Tests: T-S1a-d (settings recovery), T-F2a-c (validation + both handlers), T-S3a/b (quarantine), all + red-proofed against the pre-fix code. Agent/hub untouched. + ### v0.75.0 — gate userdata MkdirAll on a live mountpoint (no writes into an absent drive) (2026-06-22) **Bugfix — two `MkdirAll`-into-`/userdata` sites fired without checking the drive was mounted**, diff --git a/controller/internal/quiesce/quiesce.go b/controller/internal/quiesce/quiesce.go index 6eab642..92440f1 100644 --- a/controller/internal/quiesce/quiesce.go +++ b/controller/internal/quiesce/quiesce.go @@ -319,7 +319,12 @@ func (l *Loop) readMarker() (Marker, bool) { return Marker{}, false } var m Marker - if json.Unmarshal(data, &m) != nil { + if err := json.Unmarshal(data, &m); err != nil { + // S3: a corrupt marker is NOT silently dropped — log it LOUD and quarantine the bad file (a real + // corrupted-mid-quiesce marker would otherwise skip stack-recovery with no trace). Still return + // false: "no usable marker" ⇒ no recovery is the correct contract. + l.logger.Printf("[WARN] [quiesce] marker at %s is corrupt (%v) — quarantining; stacks not auto-recovered from it", l.markerPath, err) + _ = os.Rename(l.markerPath, fmt.Sprintf("%s.corrupt-%d", l.markerPath, l.now().Unix())) return Marker{}, false } return m, true diff --git a/controller/internal/quiesce/quiesce_marker_test.go b/controller/internal/quiesce/quiesce_marker_test.go new file mode 100644 index 0000000..dec671a --- /dev/null +++ b/controller/internal/quiesce/quiesce_marker_test.go @@ -0,0 +1,56 @@ +package quiesce + +import ( + "bytes" + "log" + "os" + "path/filepath" + "strings" + "testing" + "time" +) + +// T-S3a: a corrupt marker is quarantined + logged (not silently dropped). Still returns false. +// Companion: pre-fix readMarker (silent return false) → no *.corrupt-* + no log → this fails. +func TestReadMarker_QuarantinesCorrupt(t *testing.T) { + mp := filepath.Join(t.TempDir(), "quiesce-state.json") + if err := os.WriteFile(mp, []byte(`{ "active": true, BROKEN not json`), 0o644); err != nil { + t.Fatal(err) + } + var buf bytes.Buffer + l := New(Options{ + MarkerPath: mp, Poll: time.Hour, StatusPoll: time.Millisecond, MaxQuiesce: time.Second, + Logger: log.New(&buf, "", 0), + }) + if _, ok := l.readMarker(); ok { + t.Fatal("corrupt marker must return ok=false") + } + if m, _ := filepath.Glob(mp + ".corrupt-*"); len(m) == 0 { + t.Error("corrupt marker must be quarantined as *.corrupt-*") + } + if !strings.Contains(buf.String(), "corrupt") { + t.Errorf("expected a WARN about the corrupt marker, got: %q", buf.String()) + } +} + +// T-S3b: valid marker → (marker, true), no quarantine (regression). +func TestReadMarker_HappyPath(t *testing.T) { + mp := filepath.Join(t.TempDir(), "quiesce-state.json") + if err := os.WriteFile(mp, []byte(`{"active":true,"stopped_stacks":["romm","komga"]}`), 0o644); err != nil { + t.Fatal(err) + } + l := New(Options{ + MarkerPath: mp, Poll: time.Hour, StatusPoll: time.Millisecond, MaxQuiesce: time.Second, + Logger: log.New(&bytes.Buffer{}, "", 0), + }) + m, ok := l.readMarker() + if !ok { + t.Fatal("valid marker must return ok=true") + } + if len(m.StoppedStacks) != 2 { + t.Errorf("expected 2 stopped stacks, got %d", len(m.StoppedStacks)) + } + if g, _ := filepath.Glob(mp + ".corrupt-*"); len(g) != 0 { + t.Error("happy path must NOT quarantine") + } +} diff --git a/controller/internal/settings/settings.go b/controller/internal/settings/settings.go index 81b0675..37ffd47 100644 --- a/controller/internal/settings/settings.go +++ b/controller/internal/settings/settings.go @@ -21,6 +21,10 @@ type Settings struct { log *log.Logger `json:"-"` debug bool `json:"-"` + // LoadWarning is set (non-empty, Hungarian) when Load recovered from a corrupt settings.json — + // surfaced to the dashboard as a persistent banner. Not persisted. + LoadWarning string `json:"-"` + // Auth PasswordHash string `json:"password_hash,omitempty"` // bcrypt hash, overrides controller.yaml @@ -198,7 +202,24 @@ func Load(path string, logger *log.Logger) (*Settings, error) { } if err := json.Unmarshal(data, s); err != nil { - return nil, fmt.Errorf("parsing settings file: %w", err) + // CORRUPT primary — never crash-loop. Recover from the last-known-good .bak; failing that, + // preserve the corrupt file for forensics and start on safe defaults (recoverable: an empty + // PasswordHash falls back to controller.yaml; the storage registry re-discovers on startup). + logger.Printf("[ERROR] [settings] primary settings corrupt (%v) — attempting recovery from .bak", err) + if bak, berr := os.ReadFile(path + ".bak"); berr == nil { + s2 := &Settings{path: path, log: logger} + if json.Unmarshal(bak, s2) == nil { + logger.Printf("[WARN] [settings] recovered settings from .bak; re-promoting to primary") + _ = os.WriteFile(path, bak, 0644) // best-effort promote + s2.LoadWarning = "settings.json volt sérült — visszaállítva biztonsági másolatból" + s2.migrateResticToRsync() + return s2, nil + } + } + corrupt := fmt.Sprintf("%s.corrupt-%d", path, time.Now().Unix()) + _ = os.Rename(path, corrupt) + logger.Printf("[ERROR] [settings] settings unrecoverable — preserved as %s; starting with safe defaults", corrupt) + return &Settings{path: path, log: logger, LoadWarning: "settings.json sérült és helyreállíthatatlan — alapértelmezett beállítások"}, nil } logger.Printf("[INFO] [settings] Loaded settings from %s", path) @@ -266,6 +287,12 @@ func (s *Settings) save() error { return fmt.Errorf("renaming settings file: %w", err) } + // last-known-good: written AFTER the primary rename succeeds, so .bak only ever holds settings that + // parsed + saved cleanly. Best-effort — a failed .bak must NOT fail the save. + if err := os.WriteFile(s.path+".bak", data, 0644); err != nil && s.log != nil { + s.log.Printf("[WARN] [settings] could not write .bak: %v", err) + } + if s.debug { s.log.Printf("[DEBUG] [settings] saved to %s (%d bytes)", s.path, len(data)) } diff --git a/controller/internal/settings/settings_recovery_test.go b/controller/internal/settings/settings_recovery_test.go new file mode 100644 index 0000000..c3478b7 --- /dev/null +++ b/controller/internal/settings/settings_recovery_test.go @@ -0,0 +1,93 @@ +package settings + +import ( + "io" + "log" + "os" + "path/filepath" + "strings" + "testing" +) + +func discardLog() *log.Logger { return log.New(io.Discard, "", 0) } + +// T-S1a: a corrupt primary recovers from the last-known-good .bak (no error, no crash-loop). +// Companion: pre-fix Load (return error on parse) → err != nil → this fails. +func TestLoad_RecoversFromBak(t *testing.T) { + p := filepath.Join(t.TempDir(), "settings.json") + if err := os.WriteFile(p+".bak", []byte(`{"password_hash":"bak-hash"}`), 0644); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(p, []byte(`{"password_hash": BROKEN not json`), 0644); err != nil { + t.Fatal(err) + } + s, err := Load(p, discardLog()) + if err != nil { + t.Fatalf("Load must not error (no crash-loop): %v", err) + } + if s.GetPasswordHash() != "bak-hash" { + t.Fatalf("want recovered hash bak-hash, got %q", s.GetPasswordHash()) + } + if s.LoadWarning == "" { + t.Error("expected LoadWarning to be set after recovery") + } + if b, _ := os.ReadFile(p); !strings.Contains(string(b), "bak-hash") { + t.Error("primary should be re-promoted from .bak") + } +} + +// T-S1b: unrecoverable (corrupt primary, no .bak) → safe defaults, NO error, corrupt file preserved. +func TestLoad_UnrecoverableNoBak(t *testing.T) { + p := filepath.Join(t.TempDir(), "settings.json") + if err := os.WriteFile(p, []byte(`{ BROKEN not json`), 0644); err != nil { + t.Fatal(err) + } + s, err := Load(p, discardLog()) + if err != nil { + t.Fatalf("Load must not error (no crash-loop): %v", err) + } + if s == nil { + t.Fatal("expected non-nil safe-default settings") + } + if s.LoadWarning == "" { + t.Error("expected LoadWarning to be set") + } + if m, _ := filepath.Glob(p + ".corrupt-*"); len(m) == 0 { + t.Error("corrupt file must be preserved as *.corrupt-*") + } + if _, e := os.Stat(p); e == nil { + t.Error("original corrupt primary should have been renamed away") + } +} + +// T-S1c: save() writes a .bak equal to the primary (last-known-good). +func TestSave_WritesBak(t *testing.T) { + p := filepath.Join(t.TempDir(), "settings.json") + s, err := Load(p, discardLog()) // no file → defaults + if err != nil { + t.Fatal(err) + } + if err := s.SetPasswordHash("h1"); err != nil { // SetPasswordHash persists via save() + t.Fatal(err) + } + prim, _ := os.ReadFile(p) + bak, e := os.ReadFile(p + ".bak") + if e != nil { + t.Fatalf(".bak missing after save: %v", e) + } + if string(prim) != string(bak) { + t.Error(".bak must equal the primary after save") + } +} + +// T-S1d: happy path — valid primary loads normally, no warning (regression). +func TestLoad_HappyPath(t *testing.T) { + p := filepath.Join(t.TempDir(), "settings.json") + if err := os.WriteFile(p, []byte(`{"password_hash":"good"}`), 0644); err != nil { + t.Fatal(err) + } + s, err := Load(p, discardLog()) + if err != nil || s.GetPasswordHash() != "good" || s.LoadWarning != "" { + t.Fatalf("happy load failed: err=%v hash=%q warn=%q", err, s.GetPasswordHash(), s.LoadWarning) + } +} diff --git a/controller/internal/web/handler_export.go b/controller/internal/web/handler_export.go index b1d8869..5928584 100644 --- a/controller/internal/web/handler_export.go +++ b/controller/internal/web/handler_export.go @@ -166,6 +166,11 @@ func (s *Server) apiExportStart(w http.ResponseWriter, r *http.Request) { jsonError(w, "Missing stack_name or dest_drive", http.StatusBadRequest) return } + // F2 (defense-in-depth): reject a traversal/escape stack_name before any export work. + if !validStackName(req.StackName) { + jsonError(w, "Invalid stack_name", http.StatusBadRequest) + return + } if !s.isValidDrivePath(req.DestDrive) { s.logger.Printf("[DEBUG] [web] apiExportStart: invalid drive path %q", req.DestDrive) diff --git a/controller/internal/web/handlers.go b/controller/internal/web/handlers.go index 192f50a..c857113 100644 --- a/controller/internal/web/handlers.go +++ b/controller/internal/web/handlers.go @@ -151,6 +151,7 @@ func (s *Server) dashboardHandler(w http.ResponseWriter, r *http.Request) { sysInfo := system.GetInfo(s.primaryHDDPath(), s.cpuCollector) data := s.baseData("dashboard", "Vezérlőpult") + data["SettingsWarning"] = s.settings.LoadWarning // non-empty if settings.json was recovered from corruption data["Stacks"] = deployedStacks data["MissingStorage"] = s.missingStorageMap(deployedStacks) data["RunningCount"] = running @@ -795,6 +796,13 @@ func (s *Server) backupRestoreHandler(w http.ResponseWriter, r *http.Request) { http.Redirect(w, r, "/backups?flash_error=Hi%C3%A1nyz%C3%B3+param%C3%A9terek", http.StatusFound) return } + // F2 (defense-in-depth): a stack name is a single segment, never a path. Reject traversal before any + // restore work — never let it reach RestoreFromRecoveryUnit. + if !validStackName(stackName) { + s.logger.Printf("[WARN] [web] restore rejected: invalid stack_name %q from %s", stackName, r.RemoteAddr) + http.Redirect(w, r, "/backups?flash_error=%C3%89rv%C3%A9nytelen+alkalmaz%C3%A1sn%C3%A9v", http.StatusFound) + return + } if s.backupMgr == nil { http.Redirect(w, r, "/backups?flash_error=Ment%C3%A9s+nincs+be%C3%A1ll%C3%ADtva", http.StatusFound) diff --git a/controller/internal/web/templates/dashboard.html b/controller/internal/web/templates/dashboard.html index e6b45c3..7a87afb 100644 --- a/controller/internal/web/templates/dashboard.html +++ b/controller/internal/web/templates/dashboard.html @@ -87,6 +87,14 @@ {{end}} {{end}} + {{if .SettingsWarning}} +
+
+ + {{.SettingsWarning}} +
+
+ {{end}} {{if .DiskWarnings}}
{{range .DiskWarnings}} diff --git a/controller/internal/web/validate.go b/controller/internal/web/validate.go new file mode 100644 index 0000000..88feeec --- /dev/null +++ b/controller/internal/web/validate.go @@ -0,0 +1,19 @@ +package web + +import ( + "path/filepath" + "strings" +) + +// validStackName reports whether name is a safe stack identifier: a single path segment, never a path. +// Rejects traversal/escape (`..`, `/`, `\`, NUL) so a stack_name can never be turned into a filesystem +// path that escapes the stacks/userdata tree. (Storage `where=` is validated separately by gateWhere.) +func validStackName(name string) bool { + if name == "" || name == "." || name == ".." { + return false + } + if strings.ContainsAny(name, "/\\\x00") { + return false + } + return name == filepath.Clean(name) +} diff --git a/controller/internal/web/validate_test.go b/controller/internal/web/validate_test.go new file mode 100644 index 0000000..9af8a92 --- /dev/null +++ b/controller/internal/web/validate_test.go @@ -0,0 +1,61 @@ +package web + +import ( + "io" + "log" + "net/http/httptest" + "strings" + "testing" + + "gitea.dooplex.hu/admin/felhom-controller/internal/appexport" + "gitea.dooplex.hu/admin/felhom-controller/internal/config" +) + +// T-F2a: validStackName rejects traversal/escape, accepts real single-segment names. (pure) +func TestValidStackName(t *testing.T) { + for _, bad := range []string{"../../etc", "a/b", "..", ".", "", "a\x00b", "a\\b", "../x", "/etc"} { + if validStackName(bad) { + t.Errorf("validStackName(%q) = true, want false", bad) + } + } + for _, good := range []string{"paperless-ngx", "immich", "romm", "uptime-kuma"} { + if !validStackName(good) { + t.Errorf("validStackName(%q) = false, want true", good) + } + } +} + +// T-F2b: the restore handler rejects a traversal stack_name with the flash-error redirect and never +// reaches RestoreFromRecoveryUnit (backupMgr is nil — if the gate failed to fire, it would nil-panic +// or fall through to the "Mentés nincs beállítva" path, not the "Érvénytelen" one). +// Companion: drop the gate → traversal falls through to the nil-backupMgr path → different redirect → fails. +func TestBackupRestoreHandler_RejectsTraversal(t *testing.T) { + s := &Server{cfg: &config.Config{}, logger: log.New(io.Discard, "", 0)} // backupMgr == nil + form := "stack_name=../../../etc&snapshot_id=x" + req := httptest.NewRequest("POST", "/backup/restore", strings.NewReader(form)) + req.Header.Set("Content-Type", "application/x-www-form-urlencoded") + rec := httptest.NewRecorder() + + s.backupRestoreHandler(rec, req) // must not panic + + loc := rec.Header().Get("Location") + if !strings.Contains(loc, "rv%C3%A9nytelen") { // "Érvénytelen" (invalid name) — the gate's redirect + t.Fatalf("expected traversal rejection redirect, got Location=%q", loc) + } +} + +// T-F2c: the export handler rejects a traversal stack_name with 400 (before any export work). +func TestExportStart_RejectsTraversal(t *testing.T) { + // non-nil exporter so we pass the "not available" guard and reach the stack_name gate (the gate + // returns before the exporter is ever used). + s := &Server{cfg: &config.Config{}, logger: log.New(io.Discard, "", 0), appExporter: &appexport.Exporter{}} + body := `{"stack_name":"../../etc","dest_drive":"/mnt/x"}` + req := httptest.NewRequest("POST", "/api/export/start", strings.NewReader(body)) + rec := httptest.NewRecorder() + + s.apiExportStart(rec, req) // must not panic + + if rec.Code != 400 { + t.Fatalf("expected 400 for traversal stack_name, got %d", rec.Code) + } +}