From da988e3576a74e4ddfe1c28b6102bd2544225de6 Mon Sep 17 00:00:00 2001 From: Seth Johnson Date: Fri, 17 Jul 2026 12:28:05 -0400 Subject: [PATCH] fix: status freshness in sensor fail-safe window, override reason validation Two non-blocking mediums from earlier principal reviews: - controlLoop now records successfully-read temperatures during the 'sensor failsafe recovered but restore unconfirmed' window (mirroring the sticky write-failsafe branch), so /api/status never goes stale while the controller is handing control back to the BMC. No failsafe or restore semantics changed. - /api/override reason field is now validated server-side: max 128 chars, control characters rejected (400), free text otherwise - closing the defense-in-depth asymmetry with hint validation without restricting human-readable prose. Note: a reason containing a literal newline is now rejected with 400 (previously accepted). - Auth middleware tests parameterized across all four mutating routes, so a future route registered outside the auth group fails tests automatically. Tasks: dex tl6vr184, dex 2cka7723 Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01EQmqj1Hj8DL3GGAcBDNm8v --- internal/api/server.go | 26 +++++++ internal/api/server_test.go | 130 ++++++++++++++++++++++++-------- internal/controller/fan.go | 1 + internal/controller/fan_test.go | 51 +++++++++++++ 4 files changed, 175 insertions(+), 33 deletions(-) diff --git a/internal/api/server.go b/internal/api/server.go index 39f9950..b6ea15e 100644 --- a/internal/api/server.go +++ b/internal/api/server.go @@ -12,6 +12,7 @@ import ( "strconv" "strings" "time" + "unicode" "github.com/gin-gonic/gin" "github.com/sethpjohnson/only-fan-controller/internal/config" @@ -34,6 +35,26 @@ var allowedIntensities = map[string]bool{"": true, "low": true, "medium": true, // allowedHintActions is the closed set of hint actions the controller acts on. var allowedHintActions = map[string]bool{"start": true, "stop": true} +// maxOverrideReasonLen bounds the free-text override reason. +const maxOverrideReasonLen = 128 + +// validateOverrideReason enforces a length cap and rejects control characters +// on the override reason. Unlike hint source/type, this is human-readable +// prose (shown to an operator, not interpolated as an identifier), so normal +// punctuation, spaces, and quotes are all valid free text -- only control +// characters and excessive length are rejected. +func validateOverrideReason(reason string) error { + if len(reason) > maxOverrideReasonLen { + return fmt.Errorf("reason exceeds %d characters", maxOverrideReasonLen) + } + for _, r := range reason { + if unicode.IsControl(r) { + return fmt.Errorf("reason must not contain control characters") + } + } + return nil +} + //go:embed static/* var staticFiles embed.FS @@ -290,6 +311,11 @@ func (s *Server) handleOverride(c *gin.Context) { return } + if err := validateOverrideReason(req.Reason); err != nil { + c.JSON(http.StatusBadRequest, gin.H{"error": err.Error()}) + return + } + duration := time.Duration(req.Duration) * time.Second s.ctrl.SetOverride(req.Speed, duration, req.Reason) diff --git a/internal/api/server_test.go b/internal/api/server_test.go index 9bceedf..4b1b580 100644 --- a/internal/api/server_test.go +++ b/internal/api/server_test.go @@ -5,6 +5,7 @@ import ( "encoding/json" "net/http" "net/http/httptest" + "strings" "testing" "github.com/gin-gonic/gin" @@ -49,26 +50,47 @@ func doRequest(s *Server, method, path, token, remoteAddr string, body []byte) * } const validOverrideBody = `{"speed":50,"duration":60,"reason":"test"}` +const validHintBody = `{"type":"gpu_load","action":"start","intensity":"high","source":"whisper"}` + +// mutatingRoutes lists every route registered inside the requireAuth group. +// Parameterizing the auth-middleware tests across all of them (rather than +// just POST /api/override) means a future mutating route registered outside +// the mutate group -- and therefore missing auth -- gets caught by these +// tests instead of shipping unprotected. +var mutatingRoutes = []struct { + name string + method string + path string + body []byte +}{ + {"POST /api/hint", http.MethodPost, "/api/hint", []byte(validHintBody)}, + {"DELETE /api/hint/:source", http.MethodDelete, "/api/hint/whisper", nil}, + {"POST /api/override", http.MethodPost, "/api/override", []byte(validOverrideBody)}, + {"DELETE /api/override", http.MethodDelete, "/api/override", nil}, +} func TestMutatingRequiresTokenWhenConfigured(t *testing.T) { - s := newTestServer(t, "s3cret") - body := []byte(validOverrideBody) + for _, route := range mutatingRoutes { + t.Run(route.name, func(t *testing.T) { + s := newTestServer(t, "s3cret") - cases := []struct { - name string - token string - want int - }{ - {"no token", "", http.StatusUnauthorized}, - {"wrong token", "nope", http.StatusUnauthorized}, - {"correct token", "s3cret", http.StatusOK}, - } - for _, tc := range cases { - t.Run(tc.name, func(t *testing.T) { - // Use a non-loopback peer so only the token can grant access. - w := doRequest(s, http.MethodPost, "/api/override", tc.token, "203.0.113.7:5555", body) - if w.Code != tc.want { - t.Fatalf("POST /api/override: got %d, want %d (body: %s)", w.Code, tc.want, w.Body.String()) + cases := []struct { + name string + token string + want int + }{ + {"no token", "", http.StatusUnauthorized}, + {"wrong token", "nope", http.StatusUnauthorized}, + {"correct token", "s3cret", http.StatusOK}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + // Use a non-loopback peer so only the token can grant access. + w := doRequest(s, route.method, route.path, tc.token, "203.0.113.7:5555", route.body) + if w.Code != tc.want { + t.Fatalf("%s %s: got %d, want %d (body: %s)", route.method, route.path, w.Code, tc.want, w.Body.String()) + } + }) } }) } @@ -96,23 +118,26 @@ func TestConfigNeverLeaksToken(t *testing.T) { } func TestNoTokenLoopbackOnly(t *testing.T) { - s := newTestServer(t, "") // no token configured - body := []byte(validOverrideBody) + for _, route := range mutatingRoutes { + t.Run(route.name, func(t *testing.T) { + s := newTestServer(t, "") // no token configured - cases := []struct { - name string - remoteAddr string - want int - }{ - {"non-loopback rejected", "203.0.113.9:5000", http.StatusForbidden}, - {"ipv4 loopback allowed", "127.0.0.1:5000", http.StatusOK}, - {"ipv6 loopback allowed", "[::1]:5000", http.StatusOK}, - } - for _, tc := range cases { - t.Run(tc.name, func(t *testing.T) { - w := doRequest(s, http.MethodPost, "/api/override", "", tc.remoteAddr, body) - if w.Code != tc.want { - t.Fatalf("POST /api/override from %s: got %d, want %d", tc.remoteAddr, w.Code, tc.want) + cases := []struct { + name string + remoteAddr string + want int + }{ + {"non-loopback rejected", "203.0.113.9:5000", http.StatusForbidden}, + {"ipv4 loopback allowed", "127.0.0.1:5000", http.StatusOK}, + {"ipv6 loopback allowed", "[::1]:5000", http.StatusOK}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + w := doRequest(s, route.method, route.path, "", tc.remoteAddr, route.body) + if w.Code != tc.want { + t.Fatalf("%s %s from %s: got %d, want %d", route.method, route.path, tc.remoteAddr, w.Code, tc.want) + } + }) } }) } @@ -159,6 +184,45 @@ func TestHintValidation(t *testing.T) { } } +// Override reasons are human-readable free text (unlike hint source/type), +// so quotes, spaces, and punctuation must all be accepted -- the hint-client +// script sends reasons containing quotes -- while control characters and +// excessive length are still rejected server-side. +func TestOverrideReasonValidation(t *testing.T) { + s := newTestServer(t, "") // loopback path used below + + type overrideBody struct { + Speed int `json:"speed"` + Duration int `json:"duration"` + Reason string `json:"reason"` + } + + cases := []struct { + name string + reason string + want int + }{ + {"normal free text", "manual override for benchmark", http.StatusOK}, + {"quotes and punctuation allowed", `user said "too loud", quiet it down!`, http.StatusOK}, + {"empty reason allowed", "", http.StatusOK}, + {"newline rejected", "line1\nline2", http.StatusBadRequest}, + {"tab rejected", "reason\twith\ttab", http.StatusBadRequest}, + {"overlong reason rejected", strings.Repeat("a", maxOverrideReasonLen+1), http.StatusBadRequest}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + body, err := json.Marshal(overrideBody{Speed: 50, Duration: 60, Reason: tc.reason}) + if err != nil { + t.Fatalf("marshal body: %v", err) + } + w := doRequest(s, http.MethodPost, "/api/override", "", "127.0.0.1:4000", body) + if w.Code != tc.want { + t.Fatalf("POST /api/override (%s): got %d, want %d (body: %s)", tc.name, w.Code, tc.want, w.Body.String()) + } + }) + } +} + // An override posted through the API must be clamped to the configured band and // carry a finite expiry (never infinite), reflected in /api/status. func TestOverrideClampedViaAPI(t *testing.T) { diff --git a/internal/controller/fan.go b/internal/controller/fan.go index 22433ec..7b68a28 100644 --- a/internal/controller/fan.go +++ b/internal/controller/fan.go @@ -248,6 +248,7 @@ func (fc *FanController) controlLoop() { if fc.currentFailsafeCause() == failsafeSensor { if !fc.isRestoreConfirmed() { log.Printf("Sensors recovered but BMC auto hand-back not yet confirmed; deferring manual reclaim") + fc.recordReadings(cpuReading, gpuReading) return } if err := fc.enableManualMode(); err != nil { diff --git a/internal/controller/fan_test.go b/internal/controller/fan_test.go index 1a000ac..ab95288 100644 --- a/internal/controller/fan_test.go +++ b/internal/controller/fan_test.go @@ -464,6 +464,57 @@ func TestFailsafeRetriesRestoreUntilConfirmed(t *testing.T) { } } +// TestSensorFailsafeUnconfirmedStillRefreshesStatus reproduces sensors +// recovering while the BMC auto hand-back is still unconfirmed (e.g. reads +// work but RestoreAutoMode keeps failing). Manual control must NOT be +// reclaimed yet, but a successful read must still be visible via +// /api/status -- mirroring the sticky write-failsafe branch, which +// deliberately keeps refreshing readings for status visibility. +func TestSensorFailsafeUnconfirmedStillRefreshesStatus(t *testing.T) { + rec := &cmdRecorder{failRestore: true} + cfg := testConfig() + cfg.FanControl.SensorFailureLimit = 3 + cpu := &flakyCPU{failFor: 3, max: 61} + fc := NewFanController(cfg, cpu, staticGPU{max: 40}, newTestStore(t)) + fc.runCommand = rec.run + fc.currentSpeed = 40 + + // 3 failing reads -> enter sensor fail-safe; the restore attempt itself + // fails (BMC unreachable), so restoreConfirmed stays false. + fc.controlLoop() + fc.controlLoop() + fc.controlLoop() + if fc.currentFailsafeCause() != failsafeSensor { + t.Fatalf("expected sensor fail-safe after 3 failures, got %v", fc.currentFailsafeCause()) + } + if fc.isRestoreConfirmed() { + t.Fatal("restore should not be confirmed while BMC is unreachable") + } + + // 4th read succeeds, but the BMC auto hand-back is still unconfirmed, so + // manual control must stay deferred. + fc.controlLoop() + if fc.currentFailsafeCause() != failsafeSensor { + t.Fatalf("fail-safe cleared before restore was confirmed: %v", fc.currentFailsafeCause()) + } + if fc.isRestoreConfirmed() { + t.Fatal("restore should still be unconfirmed (BMC still unreachable)") + } + if got := rec.manualModeCount(); got != 0 { + t.Fatalf("manual mode reclaimed before restore was confirmed: %d toggles", got) + } + + // /api/status must reflect the fresh reading even though we are still + // waiting on restore confirmation. + status := fc.GetStatus() + if !status.RestorePending { + t.Fatal("expected restore_pending while unconfirmed") + } + if status.CPU == nil || status.CPU.Max != 61 { + t.Fatalf("status did not reflect fresh reading during unconfirmed sensor fail-safe: %+v", status.CPU) + } +} + func TestSetFanSpeedOnlyUpdatesCurrentSpeedOnSuccess(t *testing.T) { rec := &cmdRecorder{failOnFanSet: true} fc := NewFanController(testConfig(), nil, nil, nil)