Review follow-ups: status freshness in fail-safe window, override reason validation

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)
 

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) {

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 {

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)