From 1912c4e8b231d74474234e686ecd5bd3e30bd907 Mon Sep 17 00:00:00 2001 From: Seth Johnson Date: Fri, 17 Jul 2026 11:31:44 -0400 Subject: [PATCH] feat: lock down the fan-control API MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The control API was unauthenticated on 0.0.0.0:8086 — any LAN host could pin fans at 1% indefinitely, and overrides bypassed the min/max clamp. - Bearer-token auth (api.token config key / API_TOKEN env) on all mutating endpoints (POST/DELETE override, POST/DELETE hint) via middleware with constant-time comparison. Read-only endpoints and dashboard stay open. - No-token mode: mutating requests restricted to loopback peers (403 otherwise, judged from RemoteAddr — forwarded headers ignored and spoof-proof), with a loud startup warning. - Overrides clamped to min_speed/max_speed at the controller layer (plus defense-in-depth in calculateTarget); duration capped at 24h so overrides can no longer be infinite. Critical-temp emergency ramp still beats any override. - Stored XSS fixed: hint/GPU/CPU fields escaped before innerHTML in the dashboard; server-side hint validation (charset, length, closed sets) rejects script payloads outright. - api.token excluded from /api/config responses; no request logging that could leak it. - Docs: README API Security + migration note, config.example.yaml exposure comments, unraid template masked API_TOKEN field, hint-client.sh sends the bearer token (and its lost exec bit is restored). Tests: httptest coverage for the full auth matrix, loopback/spoofing, clamping, validation, token non-leakage. go vet and go test -race clean. Task: dex ttm1g3x0 - Tier 2: API security Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01EQmqj1Hj8DL3GGAcBDNm8v --- README.md | 49 ++++++++ cmd/controller/main.go | 5 + config.example.yaml | 15 +++ internal/api/server.go | 152 ++++++++++++++++++++++-- internal/api/server_test.go | 197 ++++++++++++++++++++++++++++++++ internal/api/static/index.html | 28 +++-- internal/config/config.go | 5 + internal/controller/fan.go | 43 +++++-- internal/controller/fan_test.go | 70 ++++++++++++ scripts/hint-client.sh | 22 +++- unraid/only-fan-controller.xml | 1 + 11 files changed, 558 insertions(+), 29 deletions(-) create mode 100644 internal/api/server_test.go diff --git a/README.md b/README.md index a762bf7..e84df26 100644 --- a/README.md +++ b/README.md @@ -62,6 +62,54 @@ Shows: - Temperature history graph with threshold lines - Active workload hints +## API Security + +The API binds `0.0.0.0` by default (required for container/bridge networking), so +the dashboard and API are reachable from every host on your LAN. Fan control is +protected by a **bearer token**, not by the bind address. + +- **Mutating endpoints** — `POST`/`DELETE /api/override` and `POST /api/hint`, + `DELETE /api/hint/:source` — require the token. +- **Read-only endpoints** — `/api/status`, `/api/history`, `/api/config`, and the + dashboard — stay open. + +Set the token via `api.token` in the config (or the `API_TOKEN` env var), then +send it as an `Authorization: Bearer ` header: + +```bash +curl -X POST http://localhost:8086/api/override \ + -H "Authorization: Bearer $API_TOKEN" \ + -H "Content-Type: application/json" \ + -d '{"speed": 50, "duration": 300, "reason": "testing"}' +``` + +`scripts/hint-client.sh` reads `API_TOKEN` from the environment and adds the +header automatically. + +**If no token is configured**, mutating endpoints are accepted **only from the +local host (loopback)** and a warning is logged at startup. This keeps a +single-host setup convenient without silently exposing fan control to the LAN. +Loopback is determined from the real connection peer — a spoofed +`X-Forwarded-For` cannot bypass it. + +There is no built-in TLS. To expose the controller beyond a trusted LAN, run it +behind a reverse proxy (nginx / Caddy / Traefik) that terminates TLS. + +> **Upgrading from an earlier version?** No breaking changes: add `api.token` to +> your config (or set `API_TOKEN`) to enable off-host control. Existing +> docker-compose / Unraid deployments keep working unchanged — without a token +> they simply restrict control to loopback. + +Other safety behavior worth knowing: + +- Manual overrides are **clamped** to the configured `min_speed`/`max_speed` + band and **capped at 24 hours** (an override with `duration: 0` is treated as + 24h, never truly indefinite). A critical temperature still ramps fans to max, + overriding any manual override. +- Hint `source`/`type` are restricted to `[A-Za-z0-9_.-]` (max 64 chars), + `intensity` to `low`/`medium`/`high`, and `action` to `start`/`stop`; + everything else is rejected with `400`. + ## API Endpoints ### GET /api/status @@ -205,6 +253,7 @@ All config options can be overridden via environment variables: | `FAN_GPU_THRESHOLD` | GPU temp threshold (°C) | 60 | | `CHECK_INTERVAL` | Seconds between checks | 10 | | `API_PORT` | API/Dashboard port | 8086 | +| `API_TOKEN` | Bearer token for mutating endpoints | - (loopback-only) | ## Unraid Installation diff --git a/cmd/controller/main.go b/cmd/controller/main.go index 1c75d02..2f156bc 100644 --- a/cmd/controller/main.go +++ b/cmd/controller/main.go @@ -87,6 +87,11 @@ func applyEnvOverrides(cfg *config.Config) { cfg.API.Port = i } } + + // API bearer token for mutating endpoints + if v := os.Getenv("API_TOKEN"); v != "" { + cfg.API.Token = v + } } func main() { diff --git a/config.example.yaml b/config.example.yaml index de29ba2..ba25dc1 100644 --- a/config.example.yaml +++ b/config.example.yaml @@ -27,8 +27,23 @@ fan_control: cooldown_delay: 60 # Seconds below threshold before ramping down api: + # host 0.0.0.0 binds every interface — REQUIRED for container/bridge + # networking, but it means the API/dashboard is reachable from every host on + # your LAN. The control surface is protected by the token below, NOT by the + # bind address. There is no built-in TLS; if you expose this beyond a trusted + # LAN, put it behind a reverse proxy (nginx/Caddy/Traefik) that terminates TLS. host: "0.0.0.0" port: 8086 + # Bearer token required on mutating endpoints (POST/DELETE /api/override and + # /api/hint). Read-only endpoints (/api/status, /api/history, /api/config) and + # the dashboard stay open. Env override: API_TOKEN. + # + # If left empty, mutating endpoints are accepted ONLY from the local host + # (loopback) and a warning is logged at startup — convenient for a single-host + # setup, but it means no other LAN host can change fan speed. Set a token to + # control fans from another machine (and pass it to scripts/hint-client.sh via + # the API_TOKEN env var). + token: "" dashboard: enabled: true diff --git a/internal/api/server.go b/internal/api/server.go index 702de2c..39f9950 100644 --- a/internal/api/server.go +++ b/internal/api/server.go @@ -1,11 +1,16 @@ package api import ( + "crypto/subtle" "embed" "fmt" "io/fs" + "log" + "net" "net/http" + "regexp" "strconv" + "strings" "time" "github.com/gin-gonic/gin" @@ -14,6 +19,21 @@ import ( "github.com/sethpjohnson/only-fan-controller/internal/storage" ) +// maxHintFieldLen bounds free-form hint identifiers (source/type). Kept small: +// these are process names, not prose. +const maxHintFieldLen = 64 + +// hintFieldPattern is the allowed charset for hint source/type. Restricting to +// this set means no server-derived hint string can carry HTML/script even if a +// dashboard interpolation is ever missed. +var hintFieldPattern = regexp.MustCompile(`^[A-Za-z0-9_.-]+$`) + +// allowedIntensities is the closed set of intensity values AddHint understands. +var allowedIntensities = map[string]bool{"": true, "low": true, "medium": true, "high": true} + +// allowedHintActions is the closed set of hint actions the controller acts on. +var allowedHintActions = map[string]bool{"start": true, "stop": true} + //go:embed static/* var staticFiles embed.FS @@ -50,6 +70,11 @@ func NewServer(cfg *config.Config, ctrl *controller.FanController, store *storag router: router, } + if cfg.API.Token == "" { + log.Println("WARNING: no api.token configured (env API_TOKEN); mutating endpoints " + + "(override/hint) are restricted to loopback only. Set a token to control fans from other LAN hosts.") + } + s.setupRoutes() return s } @@ -58,13 +83,20 @@ func (s *Server) setupRoutes() { // API routes api := s.router.Group("/api") { + // Read-only endpoints stay open: they expose no control surface. api.GET("/status", s.handleStatus) api.GET("/history", s.handleHistory) - api.POST("/hint", s.handleHint) - api.DELETE("/hint/:source", s.handleRemoveHint) - api.POST("/override", s.handleOverride) - api.DELETE("/override", s.handleClearOverride) api.GET("/config", s.handleGetConfig) + + // Mutating endpoints are gated by requireAuth (bearer token, or loopback + // when no token is configured). + mutate := api.Group("", s.requireAuth()) + { + mutate.POST("/hint", s.handleHint) + mutate.DELETE("/hint/:source", s.handleRemoveHint) + mutate.POST("/override", s.handleOverride) + mutate.DELETE("/override", s.handleClearOverride) + } } // Dashboard static files @@ -84,6 +116,99 @@ func (s *Server) Run() error { return s.router.Run(addr) } +// requireAuth guards the mutating endpoints. +// +// - When a token is configured, the request must carry a matching +// "Authorization: Bearer " header (compared in constant time). Any +// other case is 401. +// - When no token is configured, only requests whose connection peer is a +// loopback address are accepted (403 otherwise). This preserves single-user, +// same-host convenience without silently exposing fan control to the LAN. +// +// Loopback is decided from the real connection peer (c.Request.RemoteAddr), NOT +// from X-Forwarded-For / X-Real-IP: those are client-supplied and trivially +// spoofable, so trusting them would defeat the check. +func (s *Server) requireAuth() gin.HandlerFunc { + return func(c *gin.Context) { + token := s.cfg.API.Token + + if token == "" { + if !isLoopbackAddr(c.Request.RemoteAddr) { + c.AbortWithStatusJSON(http.StatusForbidden, gin.H{ + "error": "mutating endpoints require a loopback connection or a configured api.token", + }) + return + } + c.Next() + return + } + + if !bearerTokenMatches(c.GetHeader("Authorization"), token) { + c.AbortWithStatusJSON(http.StatusUnauthorized, gin.H{ + "error": "missing or invalid bearer token", + }) + return + } + c.Next() + } +} + +// isLoopbackAddr reports whether a net.Conn RemoteAddr string ("host:port") +// refers to a loopback address. +func isLoopbackAddr(remoteAddr string) bool { + host, _, err := net.SplitHostPort(remoteAddr) + if err != nil { + // RemoteAddr is normally host:port; fall back to treating the whole + // string as a bare host so an unexpected format fails closed via ParseIP. + host = remoteAddr + } + ip := net.ParseIP(host) + return ip != nil && ip.IsLoopback() +} + +// bearerTokenMatches reports whether an Authorization header carries a bearer +// token equal to expected. The comparison is constant-time to avoid leaking the +// token via response timing. +func bearerTokenMatches(header, expected string) bool { + got, ok := strings.CutPrefix(header, "Bearer ") + if !ok { + return false + } + return subtle.ConstantTimeCompare([]byte(got), []byte(expected)) == 1 +} + +// validateHintRequest enforces length, charset, and closed-set bounds on the +// client-controlled hint fields before they are stored or echoed back. This is +// the server-side half of the XSS defense (the dashboard escapes on render). +func validateHintRequest(req *HintRequest) error { + if err := validateHintField("source", req.Source); err != nil { + return err + } + if err := validateHintField("type", req.Type); err != nil { + return err + } + if !allowedHintActions[req.Action] { + return fmt.Errorf("action must be one of start, stop") + } + if !allowedIntensities[req.Intensity] { + return fmt.Errorf("intensity must be one of low, medium, high") + } + if req.DurationEstimate < 0 { + return fmt.Errorf("duration_estimate must not be negative") + } + return nil +} + +func validateHintField(name, value string) error { + if len(value) > maxHintFieldLen { + return fmt.Errorf("%s exceeds %d characters", name, maxHintFieldLen) + } + if !hintFieldPattern.MatchString(value) { + return fmt.Errorf("%s must match [A-Za-z0-9_.-]", name) + } + return nil +} + // GET /api/status func (s *Server) handleStatus(c *gin.Context) { status := s.ctrl.GetStatus() @@ -119,6 +244,11 @@ func (s *Server) handleHint(c *gin.Context) { return } + if err := validateHintRequest(&req); err != nil { + c.JSON(http.StatusBadRequest, gin.H{"error": err.Error()}) + return + } + if req.Action == "stop" { s.ctrl.RemoveHint(req.Source) c.JSON(http.StatusOK, gin.H{"status": "hint removed", "source": req.Source}) @@ -162,7 +292,7 @@ func (s *Server) handleOverride(c *gin.Context) { duration := time.Duration(req.Duration) * time.Second s.ctrl.SetOverride(req.Speed, duration, req.Reason) - + c.JSON(http.StatusOK, gin.H{ "status": "override set", "speed": req.Speed, @@ -180,11 +310,11 @@ func (s *Server) handleClearOverride(c *gin.Context) { func (s *Server) handleGetConfig(c *gin.Context) { // Return sanitized config (no passwords) c.JSON(http.StatusOK, gin.H{ - "idrac_host": s.cfg.IDRAC.Host, - "gpu_enabled": s.cfg.GPU.Enabled, - "interval": s.cfg.Monitoring.Interval, - "zones": s.cfg.Zones, - "fan_control": s.cfg.FanControl, - "api_port": s.cfg.API.Port, + "idrac_host": s.cfg.IDRAC.Host, + "gpu_enabled": s.cfg.GPU.Enabled, + "interval": s.cfg.Monitoring.Interval, + "zones": s.cfg.Zones, + "fan_control": s.cfg.FanControl, + "api_port": s.cfg.API.Port, }) } diff --git a/internal/api/server_test.go b/internal/api/server_test.go new file mode 100644 index 0000000..9bceedf --- /dev/null +++ b/internal/api/server_test.go @@ -0,0 +1,197 @@ +package api + +import ( + "bytes" + "encoding/json" + "net/http" + "net/http/httptest" + "testing" + + "github.com/gin-gonic/gin" + "github.com/sethpjohnson/only-fan-controller/internal/config" + "github.com/sethpjohnson/only-fan-controller/internal/controller" +) + +func init() { gin.SetMode(gin.TestMode) } + +// newTestServer builds a Server backed by a hardware-free controller. The +// controller's SetOverride/AddHint/GetStatus paths touch no external commands, +// so nil monitors and a nil store are safe for the endpoints exercised here. +func newTestServer(t *testing.T, token string) *Server { + t.Helper() + cfg := config.Default() + cfg.API.Token = token + cfg.Dashboard.Enabled = false + ctrl := controller.NewFanController(cfg, nil, nil, nil) + return NewServer(cfg, ctrl, nil) +} + +// doRequest issues a request against the server's router. A non-empty token is +// sent as a bearer header; a non-empty remoteAddr overrides the connection peer +// (httptest defaults to a non-loopback TEST-NET address). +func doRequest(s *Server, method, path, token, remoteAddr string, body []byte) *httptest.ResponseRecorder { + var req *http.Request + if body != nil { + req = httptest.NewRequest(method, path, bytes.NewReader(body)) + req.Header.Set("Content-Type", "application/json") + } else { + req = httptest.NewRequest(method, path, nil) + } + if token != "" { + req.Header.Set("Authorization", "Bearer "+token) + } + if remoteAddr != "" { + req.RemoteAddr = remoteAddr + } + w := httptest.NewRecorder() + s.router.ServeHTTP(w, req) + return w +} + +const validOverrideBody = `{"speed":50,"duration":60,"reason":"test"}` + +func TestMutatingRequiresTokenWhenConfigured(t *testing.T) { + s := newTestServer(t, "s3cret") + body := []byte(validOverrideBody) + + 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()) + } + }) + } +} + +func TestReadOnlyEndpointsStayOpen(t *testing.T) { + s := newTestServer(t, "s3cret") + for _, path := range []string{"/api/status", "/api/config"} { + w := doRequest(s, http.MethodGet, path, "", "203.0.113.7:5555", nil) + if w.Code != http.StatusOK { + t.Fatalf("GET %s should be open: got %d", path, w.Code) + } + } +} + +func TestConfigNeverLeaksToken(t *testing.T) { + s := newTestServer(t, "s3cret") + w := doRequest(s, http.MethodGet, "/api/config", "", "203.0.113.7:5555", nil) + if w.Code != http.StatusOK { + t.Fatalf("GET /api/config: got %d", w.Code) + } + if bytes.Contains(w.Body.Bytes(), []byte("s3cret")) { + t.Fatalf("/api/config leaked the api token: %s", w.Body.String()) + } +} + +func TestNoTokenLoopbackOnly(t *testing.T) { + s := newTestServer(t, "") // no token configured + body := []byte(validOverrideBody) + + 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) + } + }) + } +} + +// A spoofed X-Forwarded-For must not turn a LAN request into an accepted one: +// loopback is decided from the real connection peer only. +func TestForwardedHeaderCannotSpoofLoopback(t *testing.T) { + s := newTestServer(t, "") + req := httptest.NewRequest(http.MethodPost, "/api/override", bytes.NewReader([]byte(validOverrideBody))) + req.Header.Set("Content-Type", "application/json") + req.Header.Set("X-Forwarded-For", "127.0.0.1") + req.Header.Set("X-Real-IP", "127.0.0.1") + req.RemoteAddr = "203.0.113.9:5000" // real peer is off-host + w := httptest.NewRecorder() + s.router.ServeHTTP(w, req) + if w.Code != http.StatusForbidden { + t.Fatalf("spoofed forwarded headers granted access: got %d, want %d", w.Code, http.StatusForbidden) + } +} + +func TestHintValidation(t *testing.T) { + s := newTestServer(t, "") // loopback path used below + + cases := []struct { + name string + body string + want int + }{ + {"valid hint", `{"type":"gpu_load","action":"start","intensity":"high","source":"whisper"}`, http.StatusOK}, + {"xss source rejected", `{"type":"gpu_load","action":"start","intensity":"high","source":""}`, http.StatusBadRequest}, + {"bad type rejected", `{"type":"gpu load!","action":"start","source":"whisper"}`, http.StatusBadRequest}, + {"bad intensity rejected", `{"type":"gpu_load","action":"start","intensity":"EXTREME","source":"whisper"}`, http.StatusBadRequest}, + {"bad action rejected", `{"type":"gpu_load","action":"launch","source":"whisper"}`, http.StatusBadRequest}, + {"overlong source rejected", `{"type":"gpu_load","action":"start","source":"aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"}`, http.StatusBadRequest}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + w := doRequest(s, http.MethodPost, "/api/hint", "", "127.0.0.1:4000", []byte(tc.body)) + if w.Code != tc.want { + t.Fatalf("POST /api/hint (%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) { + s := newTestServer(t, "") + s.cfg.FanControl.MinSpeed = 20 + s.cfg.FanControl.MaxSpeed = 80 + + body := []byte(`{"speed":100,"duration":0,"reason":"pin high"}`) + w := doRequest(s, http.MethodPost, "/api/override", "", "127.0.0.1:4000", body) + if w.Code != http.StatusOK { + t.Fatalf("POST /api/override: got %d, want 200", w.Code) + } + + w = doRequest(s, http.MethodGet, "/api/status", "", "127.0.0.1:4000", nil) + if w.Code != http.StatusOK { + t.Fatalf("GET /api/status: got %d", w.Code) + } + var status struct { + Override *struct { + Speed int `json:"speed"` + ExpiresAt string `json:"expires_at"` + } `json:"override"` + } + if err := json.Unmarshal(w.Body.Bytes(), &status); err != nil { + t.Fatalf("decode status: %v", err) + } + if status.Override == nil { + t.Fatal("expected an active override in status") + } + if status.Override.Speed != 80 { + t.Fatalf("override speed not clamped to max: got %d, want 80", status.Override.Speed) + } + if status.Override.ExpiresAt == "" || status.Override.ExpiresAt == "0001-01-01T00:00:00Z" { + t.Fatalf("override expiry must be finite (not infinite), got %q", status.Override.ExpiresAt) + } +} diff --git a/internal/api/static/index.html b/internal/api/static/index.html index 93b8360..2e4ad6f 100644 --- a/internal/api/static/index.html +++ b/internal/api/static/index.html @@ -334,6 +334,20 @@

Temperature History

} } + // escapeHtml renders any server-derived value inert before it is placed + // into innerHTML. All interpolations below run through it — sensor/hint + // strings (GPU names, hint source/type/intensity) originate outside the + // dashboard and must never be trusted as markup. + function escapeHtml(value) { + return String(value ?? '').replace(/[&<>"']/g, ch => ({ + '&': '&', + '<': '<', + '>': '>', + '"': '"', + "'": ''' + }[ch])); + } + function updateDashboard(data) { // CPUs - show each CPU like GPUs const cpuList = document.getElementById('cpu-list'); @@ -343,7 +357,7 @@

Temperature History

CPU ${idx + 1}
-
${temp}°C
+
${escapeHtml(temp)}°C
`).join(''); } else { @@ -420,12 +434,12 @@

Temperature History

gpuList.innerHTML = data.gpu.Devices.map(gpu => `
-
${gpu.name}
+
${escapeHtml(gpu.name)}
- ${gpu.utilization}% util | ${gpu.power_draw}W + ${escapeHtml(gpu.utilization)}% util | ${escapeHtml(gpu.power_draw)}W
-
${gpu.temp}°C
+
${escapeHtml(gpu.temp)}°C
`).join(''); } else { @@ -437,10 +451,10 @@

Temperature History

if (data.active_hints?.length > 0) { hintsList.innerHTML = data.active_hints.map(hint => `
-
${hint.source}
+
${escapeHtml(hint.source)}
- ${hint.type} | ${hint.intensity} intensity | - min ${hint.min_fan_speed}% + ${escapeHtml(hint.type)} | ${escapeHtml(hint.intensity)} intensity | + min ${escapeHtml(hint.min_fan_speed)}%
`).join(''); diff --git a/internal/config/config.go b/internal/config/config.go index 721dcf8..89dbe91 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -69,6 +69,11 @@ type FanControlConfig struct { type APIConfig struct { Host string `yaml:"host"` Port int `yaml:"port"` + // Token is the bearer token required on mutating API endpoints + // (override/hint create+delete). When empty, mutating endpoints are only + // accepted from loopback and a startup warning is logged. It is never + // exposed by /api/config. Env override: API_TOKEN. + Token string `yaml:"token" json:"-"` } type DashboardConfig struct { diff --git a/internal/controller/fan.go b/internal/controller/fan.go index 85f653f..22433ec 100644 --- a/internal/controller/fan.go +++ b/internal/controller/fan.go @@ -495,10 +495,16 @@ func (fc *FanController) calculateTarget(cpuReading *monitor.CPUReading, gpuRead return speed } - // Check for manual override + // Check for manual override. The override speed is clamped to the configured + // MinSpeed/MaxSpeed band here as well as at SetOverride time — this is the + // single point that decides the applied fan speed, so a clamp here guarantees + // no override (however it was set) can drive fans outside the safe band. + // Note this is reached only when temperatures are NOT critical: the emergency + // ramp above returns first, so a clamped override never blocks critical cooling. if fc.override != nil { - fc.targetSpeed = fc.override.Speed - return fc.override.Speed + speed := fc.clampSpeed(fc.override.Speed) + fc.targetSpeed = speed + return speed } // Simple threshold-based control with hysteresis @@ -721,22 +727,39 @@ func (fc *FanController) RemoveHint(source string) { log.Printf("Hint removed: %s", source) } -// SetOverride sets a manual fan speed override +// maxOverrideDuration caps how long a manual override can stay in force. An +// indefinite override (duration <= 0) is treated as this maximum rather than +// "forever": a forgotten override must not silently hold the fans off the +// automatic curve indefinitely. +const maxOverrideDuration = 24 * time.Hour + +// clampSpeed constrains a fan speed to the configured MinSpeed/MaxSpeed band. +// Callers hold fc.mu. +func (fc *FanController) clampSpeed(speed int) int { + return max(fc.cfg.FanControl.MinSpeed, min(fc.cfg.FanControl.MaxSpeed, speed)) +} + +// SetOverride sets a manual fan speed override. The speed is clamped to the +// configured MinSpeed/MaxSpeed band and the duration is capped at +// maxOverrideDuration (an indefinite/zero duration becomes that cap) so a manual +// override can neither drive fans outside the safe band nor persist forever. func (fc *FanController) SetOverride(speed int, duration time.Duration, reason string) { fc.mu.Lock() defer fc.mu.Unlock() + if duration <= 0 || duration > maxOverrideDuration { + duration = maxOverrideDuration + } + + clamped := fc.clampSpeed(speed) fc.override = &Override{ - Speed: speed, + Speed: clamped, Reason: reason, CreatedAt: time.Now(), + ExpiresAt: time.Now().Add(duration), } - if duration > 0 { - fc.override.ExpiresAt = time.Now().Add(duration) - } - - log.Printf("Override set: %d%% (%s)", speed, reason) + log.Printf("Override set: %d%% (%s), expires in %s", clamped, reason, duration) } // ClearOverride removes the manual override diff --git a/internal/controller/fan_test.go b/internal/controller/fan_test.go index 0d47ae3..1a000ac 100644 --- a/internal/controller/fan_test.go +++ b/internal/controller/fan_test.go @@ -491,3 +491,73 @@ func TestSetFanSpeedOnlyUpdatesCurrentSpeedOnSuccess(t *testing.T) { t.Fatal("lastWriteFailed should be cleared after a successful write") } } + +// --- manual-override clamping (Tier 2 C3) --- + +func TestSetOverrideClampsSpeedAndCapsDuration(t *testing.T) { + cfg := config.Default() + cfg.FanControl.MinSpeed = 20 + cfg.FanControl.MaxSpeed = 80 + fc := NewFanController(cfg, nil, nil, nil) + + // Above max clamps down; an infinite (0) duration is capped, not held forever. + fc.SetOverride(100, 0, "too high") + if fc.override.Speed != 80 { + t.Fatalf("override speed above max not clamped: got %d, want 80", fc.override.Speed) + } + if fc.override.ExpiresAt.IsZero() { + t.Fatal("override with duration 0 must be capped to a finite expiry, not infinite") + } + if d := time.Until(fc.override.ExpiresAt); d > maxOverrideDuration+time.Minute { + t.Fatalf("infinite override not capped to %s: expires in %s", maxOverrideDuration, d) + } + + // Below min clamps up. + fc.SetOverride(1, time.Hour, "too low") + if fc.override.Speed != 20 { + t.Fatalf("override speed below min not clamped: got %d, want 20", fc.override.Speed) + } + + // A duration beyond the cap is capped. + fc.SetOverride(50, 48*time.Hour, "too long") + if d := time.Until(fc.override.ExpiresAt); d > maxOverrideDuration+time.Minute { + t.Fatalf("override duration not capped to %s: expires in %s", maxOverrideDuration, d) + } +} + +// calculateTarget must clamp the override even if an override was set directly +// (defense in depth), while temperatures are non-critical. +func TestCalculateTargetClampsOverride(t *testing.T) { + cfg := config.Default() + cfg.FanControl.MinSpeed = 20 + cfg.FanControl.MaxSpeed = 80 + fc := NewFanController(cfg, nil, nil, nil) + + cpuR, gpuR := cpuGpu(30, 30) // well below any threshold + + fc.override = &Override{Speed: 100} // set directly, bypassing SetOverride's clamp + if got := fc.calculateTarget(cpuR, gpuR); got != 80 { + t.Fatalf("override not clamped to max in calculateTarget: got %d, want 80", got) + } + + fc.override = &Override{Speed: 1} + if got := fc.calculateTarget(cpuR, gpuR); got != 20 { + t.Fatalf("override not clamped to min in calculateTarget: got %d, want 20", got) + } +} + +// A critical temperature must still ramp to MaxSpeed even when a (clamped) low +// manual override is active — the Tier 1 emergency behavior must not regress. +func TestCriticalOverridesClampedOverride(t *testing.T) { + cfg := config.Default() + cfg.FanControl.MinSpeed = 20 + cfg.FanControl.MaxSpeed = 100 + cfg.FanControl.CriticalCPUTemp = 85 + fc := NewFanController(cfg, nil, nil, nil) + + fc.SetOverride(20, time.Hour, "low manual") // clamped, low + cpuR, gpuR := cpuGpu(95, 30) // CPU past critical + if got := fc.calculateTarget(cpuR, gpuR); got != 100 { + t.Fatalf("critical ramp did not override clamped override: got %d, want 100", got) + } +} diff --git a/scripts/hint-client.sh b/scripts/hint-client.sh index e73b3db..12a92f2 100755 --- a/scripts/hint-client.sh +++ b/scripts/hint-client.sh @@ -8,6 +8,18 @@ CONTROLLER_URL="${SMART_FAN_URL:-http://localhost:8086}" +# Bearer token for the mutating endpoints (override / hint). Set API_TOKEN to +# match api.token in the controller config. When empty, the controller only +# accepts mutating requests from the local host (loopback). +API_TOKEN="${API_TOKEN:-}" + +# AUTH_ARGS carries the Authorization header for mutating requests when a token +# is configured; it stays empty for read-only calls and loopback-only setups. +AUTH_ARGS=() +if [ -n "$API_TOKEN" ]; then + AUTH_ARGS=(-H "Authorization: Bearer $API_TOKEN") +fi + case "$1" in start) SOURCE="$2" @@ -20,6 +32,7 @@ case "$1" in fi curl -s -X POST "$CONTROLLER_URL/api/hint" \ + "${AUTH_ARGS[@]}" \ -H "Content-Type: application/json" \ -d "{ \"type\": \"gpu_load\", @@ -39,6 +52,7 @@ case "$1" in fi curl -s -X POST "$CONTROLLER_URL/api/hint" \ + "${AUTH_ARGS[@]}" \ -H "Content-Type: application/json" \ -d "{ \"type\": \"gpu_load\", @@ -62,6 +76,7 @@ case "$1" in fi curl -s -X POST "$CONTROLLER_URL/api/override" \ + "${AUTH_ARGS[@]}" \ -H "Content-Type: application/json" \ -d "{ \"speed\": $SPEED, @@ -71,7 +86,8 @@ case "$1" in ;; clear-override) - curl -s -X DELETE "$CONTROLLER_URL/api/override" | jq . + curl -s -X DELETE "$CONTROLLER_URL/api/override" \ + "${AUTH_ARGS[@]}" | jq . ;; *) @@ -88,6 +104,10 @@ case "$1" in echo "" echo "Intensity: low, medium, high" echo "" + echo "Environment:" + echo " SMART_FAN_URL Controller base URL (default http://localhost:8086)" + echo " API_TOKEN Bearer token for override/hint (required off-host)" + echo "" echo "Examples:" echo " $0 start whisper high 300" echo " $0 stop whisper" diff --git a/unraid/only-fan-controller.xml b/unraid/only-fan-controller.xml index 3402697..65d8076 100644 --- a/unraid/only-fan-controller.xml +++ b/unraid/only-fan-controller.xml @@ -33,6 +33,7 @@ root + true 20 65