diff --git a/.env.example b/.env.example index 932e8cf..c675e1f 100644 --- a/.env.example +++ b/.env.example @@ -47,6 +47,16 @@ AUTH_DISABLED=false # Comma-separated CIDR ranges allowed when AUTH_DISABLED=true # TRUSTED_NETWORKS=172.16.0.0/12,10.0.0.0/8,192.168.0.0/16,127.0.0.1,::1 +# Comma-separated CIDR ranges, beyond loopback, allowed to reach /health +# (liveness) and /health/ready (readiness). Empty means loopback only, which is +# all the container's own HEALTHCHECK needs. Set this to let an external uptime +# monitor or load balancer probe Capstan. +# +# Deliberately separate from TRUSTED_NETWORKS: that value is also the +# trusted-proxy list and the AUTH_DISABLED bypass list, so reusing it would +# grant a monitor X-Forwarded-For spoofing and auth bypass just to read health. +# HEALTH_ALLOWED_NETWORKS=10.1.0.0/16 + # CORS origins (comma-separated, empty = same-origin only) # CORS_ORIGINS=https://your-domain.com diff --git a/README.md b/README.md index bc6c73c..c58a25e 100644 --- a/README.md +++ b/README.md @@ -574,11 +574,49 @@ npm run build # production build ## API Endpoints All routes are under `/api/v1` and require authentication (unless -`AUTH_DISABLED=true`), except `/health` and `/api/v1/version`. The web UI is the -primary interface; these are the main REST routes. +`AUTH_DISABLED=true`), except `/health`, `/health/ready` and `/api/v1/version`. +The web UI is the primary interface; these are the main REST routes. ### Health -- `GET /health` — health check (restricted to localhost) + +Liveness and readiness are separate endpoints, because Capstan is a separate +process from Docker. Point restart-on-failure checks at liveness and dependency +monitoring at readiness. + +- `GET /health` — **liveness**. 200 whenever the process is up and serving. Makes + no Docker call, so a Docker daemon restart cannot mark the container unhealthy + and get Capstan bounced for an outage restarting it would not fix. This is what + the container `HEALTHCHECK`, and any orchestrator liveness probe, should use. +- `GET /health/ready` — **readiness**. Reports dependencies, 503 when any is + degraded, naming which. The Docker probe is bounded by a 2-second timeout so a + hung daemon cannot pile up goroutines across repeated probes. Point an uptime + monitor, a load balancer, or a Kubernetes readiness probe here. + +```console +$ curl -s localhost:5001/health +{"status":"healthy"} + +$ curl -s localhost:5001/health/ready # Docker up +{"checks":{"docker":{"status":"ok"}},"status":"ready"} + +$ curl -s localhost:5001/health/ready # Docker daemon stopped -> HTTP 503 +{"checks":{"docker":{"error":"Cannot connect to the Docker daemon at unix:///var/run/docker.sock.", +"status":"unavailable"}},"degraded":["docker"],"status":"degraded"} +``` + +**Reachability.** Loopback reaches both with no configuration, so the container's +own healthcheck needs no setup. Every other address is denied by default — +upgrading does not silently widen exposure. To let an external monitor in, list +its network in `HEALTH_ALLOWED_NETWORKS`: + +```bash +HEALTH_ALLOWED_NETWORKS=10.1.0.0/16,192.168.50.7 +``` + +This is deliberately **not** `TRUSTED_NETWORKS`. That value is Gin's +trusted-proxy list *and* the `AUTH_DISABLED` bypass list, so reusing it would +mean granting an uptime monitor `X-Forwarded-For` spoofing and authentication +bypass just to let it read a health endpoint. ### Version - `GET /api/v1/version` — build identity of the running binary. Public, so an diff --git a/backend/cmd/server/main.go b/backend/cmd/server/main.go index fba3c23..809d38b 100644 --- a/backend/cmd/server/main.go +++ b/backend/cmd/server/main.go @@ -7,7 +7,6 @@ import ( "fmt" "log" "log/slog" - "net" "net/http" "os" "os/signal" @@ -79,14 +78,6 @@ func SecurityHeaders(cfg *config.Config) gin.HandlerFunc { } } -func isLocalhost(c *gin.Context) bool { - ip := net.ParseIP(c.ClientIP()) - if ip == nil { - return false - } - return ip.IsLoopback() -} - func main() { // Bootstrap the logger from the environment before config.Load, so that // config's own startup lines — including the volume-path-identity warning — @@ -224,32 +215,15 @@ func main() { r.Use(middleware.ValidateInput()) r.Use(SecurityHeaders(cfg)) - r.GET("/health", func(c *gin.Context) { - if !isLocalhost(c) { - c.JSON(http.StatusForbidden, gin.H{"error": "Health endpoint restricted to localhost"}) - return - } - - dockerHealthy := false - - if dockerService != nil { - _, err := dockerService.GetContainerList("") - if err == nil { - dockerHealthy = true - } - } - - if !dockerHealthy { - c.JSON(http.StatusServiceUnavailable, gin.H{ - "status": "unhealthy", - }) - return - } - - c.JSON(http.StatusOK, gin.H{ - "status": "healthy", - }) - }) + // Liveness and readiness. dockerService is a typed nil when the daemon was + // unreachable at startup; hand the handler an untyped nil so it reports the + // outage rather than calling through a non-nil interface holding a nil + // pointer (agent-os-69a). + var dockerPinger handlers.DockerPinger + if dockerService != nil { + dockerPinger = dockerService + } + handlers.NewHealthHandler(dockerPinger, cfg.HealthNetworks).RegisterRoutes(r) api := r.Group("/api/v1") diff --git a/backend/internal/config/config.go b/backend/internal/config/config.go index a4915b2..9d06506 100644 --- a/backend/internal/config/config.go +++ b/backend/internal/config/config.go @@ -30,6 +30,14 @@ type Config struct { AuthDisabled bool CORSOrigins string TrustedNetworks string + // HealthNetworks lists the CIDRs, beyond loopback, that may reach /health + // and /health/ready. Deliberately separate from TrustedNetworks: that value + // is Gin's trusted-proxy list *and* the AUTH_DISABLED bypass list, so + // reusing it would force an operator to grant an uptime monitor + // X-Forwarded-For spoofing and auth bypass just to read a health endpoint + // (agent-os-69a). Empty means loopback only, which is the pre-split + // behaviour. + HealthNetworks string ExtraStacksDirs []string // Backup / restic env-var fallbacks (DB settings take precedence at runtime). @@ -57,6 +65,7 @@ func Load() (*Config, error) { GitHTTPSUser: "git", AuthDisabled: os.Getenv("AUTH_DISABLED") == "true", TrustedNetworks: os.Getenv("TRUSTED_NETWORKS"), + HealthNetworks: os.Getenv("HEALTH_ALLOWED_NETWORKS"), } if stacksDir := os.Getenv("STACKS_DIR"); stacksDir != "" { diff --git a/backend/internal/handlers/health.go b/backend/internal/handlers/health.go new file mode 100644 index 0000000..bc70a48 --- /dev/null +++ b/backend/internal/handlers/health.go @@ -0,0 +1,144 @@ +package handlers + +import ( + "context" + "log/slog" + "net" + "net/http" + "time" + + "github.com/gin-gonic/gin" + "github.com/thinkbig1979/capstan/backend/internal/middleware" +) + +// readinessProbeTimeout bounds the dependency check. A hung Docker daemon +// otherwise parks a goroutine per probe, and repeated probes pile them up. +const readinessProbeTimeout = 2 * time.Second + +// DockerPinger is the readiness handler's view of the Docker service: one cheap, +// context-aware round-trip. Narrow on purpose, so the handler is testable +// without a daemon. +type DockerPinger interface { + Ping(ctx context.Context) error +} + +// HealthHandler serves liveness and readiness. +// +// The split exists because Capstan is a separate process from Docker. The old +// combined endpoint returned 503 whenever the daemon was unreachable, so a +// daemon restart marked the container unhealthy and anything that restarts on +// failed health checks would bounce Capstan — which does nothing to fix Docker +// (agent-os-69a). +// +// GET /health liveness — the process is up and serving. No Docker call. +// GET /health/ready readiness — reports dependencies; 503 names what is degraded. +type HealthHandler struct { + // docker is nil when the Docker service could not be created at startup. + // That is a legitimate degraded state, not an error to hide. + docker DockerPinger + allowedNetworks string + probeTimeout time.Duration +} + +// NewHealthHandler builds the handler. Pass a nil docker when the Docker service +// is unavailable; readiness then reports Docker as degraded rather than +// pretending to be ready. +// +// allowedNetworks is HEALTH_ALLOWED_NETWORKS: comma-separated CIDRs, beyond +// loopback, permitted to reach either endpoint. Empty means loopback only, which +// is the pre-split behaviour — an upgrade must not silently widen exposure. +func NewHealthHandler(docker DockerPinger, allowedNetworks string) *HealthHandler { + return &HealthHandler{ + docker: docker, + allowedNetworks: allowedNetworks, + probeTimeout: readinessProbeTimeout, + } +} + +// RegisterRoutes mounts both endpoints on the root router. They sit outside the +// protected group deliberately: a probe has no session. Both paths are listed in +// middleware.PublicPaths so auth and CSRF agree with that. +func (h *HealthHandler) RegisterRoutes(r gin.IRoutes) { + r.GET("/health", h.Live) + r.GET("/health/ready", h.Ready) +} + +// allowed enforces the network policy shared by both endpoints, writing the 403 +// itself. Loopback always passes, so the container's own HEALTHCHECK needs no +// configuration. +func (h *HealthHandler) allowed(c *gin.Context) bool { + clientIP := c.ClientIP() + + // The whole loopback range, not just the literal 127.0.0.1/::1 that + // middleware.IsTrustedIP matches. The handler this replaced used + // net.IP.IsLoopback, and narrowing that here would deny a probe bound to, + // say, 127.0.0.2 that works today. + if ip := net.ParseIP(clientIP); ip != nil && ip.IsLoopback() { + return true + } + + if middleware.IsTrustedIP(clientIP, h.allowedNetworks) { + return true + } + c.JSON(http.StatusForbidden, gin.H{ + "error": "Health endpoint restricted; add this network to HEALTH_ALLOWED_NETWORKS to permit it", + }) + return false +} + +// Live answers whether the process is up and serving. It must not touch any +// dependency: the container HEALTHCHECK points here, and a dependency failure +// answered from this endpoint gets the container restarted for someone else's +// outage. +// +// The body keeps the pre-split {"status":"healthy"} so existing monitors that +// match on it keep working. What changed is that it no longer returns 503. +func (h *HealthHandler) Live(c *gin.Context) { + if !h.allowed(c) { + return + } + c.JSON(http.StatusOK, gin.H{"status": "healthy"}) +} + +// Ready reports the dependencies, 503 when any is degraded, naming which. +func (h *HealthHandler) Ready(c *gin.Context) { + if !h.allowed(c) { + return + } + + timeout := h.probeTimeout + if timeout <= 0 { + timeout = readinessProbeTimeout + } + ctx, cancel := context.WithTimeout(c.Request.Context(), timeout) + defer cancel() + + dockerCheck := gin.H{"status": "ok"} + var degraded []string + + switch { + case h.docker == nil: + degraded = append(degraded, "docker") + dockerCheck = gin.H{"status": "unavailable", "error": "docker service not initialized at startup"} + default: + if err := h.docker.Ping(ctx); err != nil { + degraded = append(degraded, "docker") + dockerCheck = gin.H{"status": "unavailable", "error": err.Error()} + } + } + + body := gin.H{ + "status": "ready", + "checks": gin.H{"docker": dockerCheck}, + } + + if len(degraded) > 0 { + body["status"] = "degraded" + body["degraded"] = degraded + slog.Warn("Readiness probe degraded", "degraded", degraded) + c.JSON(http.StatusServiceUnavailable, body) + return + } + + c.JSON(http.StatusOK, body) +} diff --git a/backend/internal/handlers/health_test.go b/backend/internal/handlers/health_test.go new file mode 100644 index 0000000..0704484 --- /dev/null +++ b/backend/internal/handlers/health_test.go @@ -0,0 +1,277 @@ +package handlers + +import ( + "context" + "encoding/json" + "errors" + "net/http" + "net/http/httptest" + "strings" + "sync/atomic" + "testing" + "time" + + "github.com/gin-gonic/gin" + "github.com/thinkbig1979/capstan/backend/internal/middleware" +) + +// fakePinger records how often it was called, so a test can assert that +// liveness touched no dependency at all. +type fakePinger struct { + calls atomic.Int32 + err error + // block, when non-nil, holds Ping until ctx is done or block is closed — + // the stand-in for a hung daemon. + block chan struct{} +} + +func (f *fakePinger) Ping(ctx context.Context) error { + f.calls.Add(1) + if f.block != nil { + select { + case <-f.block: + case <-ctx.Done(): + return ctx.Err() + } + } + return f.err +} + +func healthRouter(h *HealthHandler) *gin.Engine { + r := gin.New() + h.RegisterRoutes(r) + return r +} + +// requestFrom issues a GET to path as if it came from remoteIP. IPv6 literals +// need bracketing in RemoteAddr or net.SplitHostPort cannot parse them, and gin +// falls back to an empty ClientIP. +func requestFrom(r *gin.Engine, path, remoteIP string) *httptest.ResponseRecorder { + req := httptest.NewRequest(http.MethodGet, path, nil) + if strings.Contains(remoteIP, ":") { + req.RemoteAddr = "[" + remoteIP + "]:40000" + } else { + req.RemoteAddr = remoteIP + ":40000" + } + w := httptest.NewRecorder() + r.ServeHTTP(w, req) + return w +} + +func decodeHealthBody(t *testing.T, w *httptest.ResponseRecorder) map[string]any { + t.Helper() + var body map[string]any + if err := json.Unmarshal(w.Body.Bytes(), &body); err != nil { + t.Fatalf("decoding %s: %v", w.Body.String(), err) + } + return body +} + +// ── Liveness ──────────────────────────────────────────────────────────────── + +// TestLivenessMakesNoDockerCall is the core of the split. The container +// HEALTHCHECK points here; if liveness consulted Docker, a daemon restart would +// mark the container unhealthy and get Capstan bounced for someone else's +// outage. +func TestLivenessMakesNoDockerCall(t *testing.T) { + docker := &fakePinger{} + w := requestFrom(healthRouter(NewHealthHandler(docker, "")), "/health", "127.0.0.1") + + if w.Code != http.StatusOK { + t.Fatalf("status = %d, want %d", w.Code, http.StatusOK) + } + if got := docker.calls.Load(); got != 0 { + t.Errorf("liveness called Docker %d time(s); it must call it none", got) + } + if body := decodeHealthBody(t, w); body["status"] != "healthy" { + t.Errorf("status = %v, want %q", body["status"], "healthy") + } +} + +// TestLivenessStays200WhenDockerIsDown is the behaviour change the bead asks +// for: the old combined endpoint returned 503 here. +func TestLivenessStays200WhenDockerIsDown(t *testing.T) { + docker := &fakePinger{err: errors.New("cannot connect to the Docker daemon")} + w := requestFrom(healthRouter(NewHealthHandler(docker, "")), "/health", "127.0.0.1") + + if w.Code != http.StatusOK { + t.Fatalf("status = %d, want %d — a Docker outage must not mark the process dead", w.Code, http.StatusOK) + } +} + +// TestLivenessStays200WhenDockerServiceIsAbsent covers the nil case: the daemon +// was already unreachable when the process started. +func TestLivenessStays200WhenDockerServiceIsAbsent(t *testing.T) { + w := requestFrom(healthRouter(NewHealthHandler(nil, "")), "/health", "127.0.0.1") + + if w.Code != http.StatusOK { + t.Fatalf("status = %d, want %d", w.Code, http.StatusOK) + } +} + +// ── Readiness ─────────────────────────────────────────────────────────────── + +func TestReadinessReportsReadyWhenDockerAnswers(t *testing.T) { + docker := &fakePinger{} + w := requestFrom(healthRouter(NewHealthHandler(docker, "")), "/health/ready", "127.0.0.1") + + if w.Code != http.StatusOK { + t.Fatalf("status = %d, want %d (body %s)", w.Code, http.StatusOK, w.Body.String()) + } + body := decodeHealthBody(t, w) + if body["status"] != "ready" { + t.Errorf("status = %v, want %q", body["status"], "ready") + } + if docker.calls.Load() != 1 { + t.Errorf("Docker pinged %d time(s), want 1", docker.calls.Load()) + } +} + +// TestReadinessNamesDockerWhenDegraded — 503 alone is not enough; an operator +// reading the probe output has to be able to tell *what* is degraded. +func TestReadinessNamesDockerWhenDegraded(t *testing.T) { + docker := &fakePinger{err: errors.New("cannot connect to the Docker daemon socket")} + w := requestFrom(healthRouter(NewHealthHandler(docker, "")), "/health/ready", "127.0.0.1") + + if w.Code != http.StatusServiceUnavailable { + t.Fatalf("status = %d, want %d", w.Code, http.StatusServiceUnavailable) + } + body := decodeHealthBody(t, w) + if body["status"] != "degraded" { + t.Errorf("status = %v, want %q", body["status"], "degraded") + } + + names, ok := body["degraded"].([]any) + if !ok || len(names) != 1 || names[0] != "docker" { + t.Fatalf("degraded = %v, want [docker] — 503 must name the failing dependency", body["degraded"]) + } + + checks, _ := body["checks"].(map[string]any) + dockerCheck, _ := checks["docker"].(map[string]any) + if dockerCheck["status"] != "unavailable" { + t.Errorf("checks.docker.status = %v, want %q", dockerCheck["status"], "unavailable") + } + if msg, _ := dockerCheck["error"].(string); msg == "" { + t.Error("checks.docker.error is empty; the reason is what makes the probe actionable") + } +} + +// TestReadinessDegradesWhenDockerServiceIsAbsent covers the nil-service branch. +// A nil *DockerService stored in an interface is a non-nil interface value, so +// this is also the regression guard against pinging through it and panicking. +func TestReadinessDegradesWhenDockerServiceIsAbsent(t *testing.T) { + w := requestFrom(healthRouter(NewHealthHandler(nil, "")), "/health/ready", "127.0.0.1") + + if w.Code != http.StatusServiceUnavailable { + t.Fatalf("status = %d, want %d", w.Code, http.StatusServiceUnavailable) + } + body := decodeHealthBody(t, w) + names, ok := body["degraded"].([]any) + if !ok || len(names) != 1 || names[0] != "docker" { + t.Fatalf("degraded = %v, want [docker]", body["degraded"]) + } +} + +// TestReadinessTimesOutOnAHungDaemon: without a bound on the probe, repeated +// 30-second checks against a hung daemon pile up goroutines indefinitely. +func TestReadinessTimesOutOnAHungDaemon(t *testing.T) { + blocked := make(chan struct{}) + t.Cleanup(func() { close(blocked) }) + + h := NewHealthHandler(&fakePinger{block: blocked}, "") + h.probeTimeout = 50 * time.Millisecond + + done := make(chan *httptest.ResponseRecorder, 1) + go func() { done <- requestFrom(healthRouter(h), "/health/ready", "127.0.0.1") }() + + select { + case w := <-done: + if w.Code != http.StatusServiceUnavailable { + t.Fatalf("status = %d, want %d", w.Code, http.StatusServiceUnavailable) + } + body := decodeHealthBody(t, w) + if body["status"] != "degraded" { + t.Errorf("status = %v, want %q", body["status"], "degraded") + } + case <-time.After(3 * time.Second): + t.Fatal("readiness never returned against a hung daemon: the probe has no timeout") + } +} + +// ── Reachability ──────────────────────────────────────────────────────────── + +// TestLoopbackReachesBothWithNoConfiguration — the container's own HEALTHCHECK +// must work out of the box. +func TestLoopbackReachesBothWithNoConfiguration(t *testing.T) { + r := healthRouter(NewHealthHandler(&fakePinger{}, "")) + + for _, tc := range []struct{ path, ip string }{ + {"/health", "127.0.0.1"}, + {"/health/ready", "127.0.0.1"}, + {"/health", "::1"}, + {"/health/ready", "::1"}, + // The replaced handler used net.IP.IsLoopback, so the whole 127.0.0.0/8 + // range reached it. Keep that rather than narrowing to the literal + // 127.0.0.1 that middleware.IsTrustedIP matches. + {"/health", "127.0.0.2"}, + {"/health/ready", "127.0.0.2"}, + } { + if w := requestFrom(r, tc.path, tc.ip); w.Code != http.StatusOK { + t.Errorf("GET %s from %s: status = %d, want %d", tc.path, tc.ip, w.Code, http.StatusOK) + } + } +} + +// TestNonLoopbackDeniedByDefault pins the no-silent-exposure-change requirement: +// an upgrade with no new configuration must keep the pre-split restriction. +func TestNonLoopbackDeniedByDefault(t *testing.T) { + r := healthRouter(NewHealthHandler(&fakePinger{}, "")) + + for _, path := range []string{"/health", "/health/ready"} { + w := requestFrom(r, path, "10.1.2.3") + if w.Code != http.StatusForbidden { + t.Errorf("GET %s from 10.1.2.3: status = %d, want %d — the default must not widen exposure", + path, w.Code, http.StatusForbidden) + } + } +} + +// TestNonLoopbackAllowedOnceItsNetworkIsListed is the other half: the endpoint +// is reachable by an uptime monitor once its network is configured. +func TestNonLoopbackAllowedOnceItsNetworkIsListed(t *testing.T) { + r := healthRouter(NewHealthHandler(&fakePinger{}, "10.1.0.0/16")) + + for _, path := range []string{"/health", "/health/ready"} { + if w := requestFrom(r, path, "10.1.2.3"); w.Code != http.StatusOK { + t.Errorf("GET %s from 10.1.2.3 with 10.1.0.0/16 allowed: status = %d, want %d", + path, w.Code, http.StatusOK) + } + } + + // A host outside the listed range is still denied. + if w := requestFrom(r, "/health", "10.9.2.3"); w.Code != http.StatusForbidden { + t.Errorf("GET /health from 10.9.2.3: status = %d, want %d", w.Code, http.StatusForbidden) + } +} + +// TestDeniedRequestSkipsTheDockerProbe — a rejected caller must not be able to +// drive daemon round-trips. +func TestDeniedRequestSkipsTheDockerProbe(t *testing.T) { + docker := &fakePinger{} + requestFrom(healthRouter(NewHealthHandler(docker, "")), "/health/ready", "10.1.2.3") + + if got := docker.calls.Load(); got != 0 { + t.Errorf("denied request pinged Docker %d time(s), want 0", got) + } +} + +// TestHealthPathsArePublic keeps middleware.PublicPaths truthful: both routes +// are mounted outside the protected group, and that list is read by the CSRF +// middleware as well as auth. +func TestHealthPathsArePublic(t *testing.T) { + for _, path := range []string{"/health", "/health/ready"} { + if !middleware.IsPublicPath(path) { + t.Errorf("%s is served outside the protected group but is not in middleware.PublicPaths", path) + } + } +} diff --git a/backend/internal/middleware/auth.go b/backend/internal/middleware/auth.go index 945cdb4..8e788d2 100644 --- a/backend/internal/middleware/auth.go +++ b/backend/internal/middleware/auth.go @@ -20,7 +20,10 @@ var PublicPaths = []string{ // conversation needs to answer "what is running here?" without a session. // This list is consulted by the CSRF middleware too, so keep the two in step. "/api/v1/version", + // Liveness and readiness. Both carry their own network policy + // (HEALTH_ALLOWED_NETWORKS, loopback always allowed) rather than a session. "/health", + "/health/ready", } func IsPublicPath(path string) bool { @@ -32,16 +35,24 @@ func IsPublicPath(path string) bool { return false } -func isTrustedIP(clientIP string, trustedNetworks string) bool { +// IsTrustedIP reports whether clientIP is loopback or falls inside one of the +// comma-separated CIDRs (or literal addresses) in networks. Loopback is always +// allowed, and an empty list means loopback only. +// +// Two callers with deliberately different lists: the AUTH_DISABLED bypass uses +// TRUSTED_NETWORKS, the health endpoints use HEALTH_ALLOWED_NETWORKS. Only the +// matching logic is shared — see config.Config.HealthNetworks for why the lists +// are not. +func IsTrustedIP(clientIP string, networks string) bool { if clientIP == "127.0.0.1" || clientIP == "::1" || clientIP == "localhost" { return true } - if trustedNetworks == "" { + if networks == "" { return false } - for _, networkStr := range strings.Split(trustedNetworks, ",") { + for _, networkStr := range strings.Split(networks, ",") { networkStr = strings.TrimSpace(networkStr) if networkStr == "" { continue @@ -91,7 +102,7 @@ func AuthMiddleware(db *database.DB, jwtSecret string, authDisabled bool, truste return func(c *gin.Context) { if authDisabled { clientIP := c.ClientIP() - if !isTrustedIP(clientIP, trustedNetworks) { + if !IsTrustedIP(clientIP, trustedNetworks) { slog.Warn("Untrusted IP attempt with auth disabled", "ip", clientIP, "trusted_networks", trustedNetworks) c.JSON(403, models.NewAppError(403, "FORBIDDEN", "Authentication disabled - only local connections allowed")) c.Abort() diff --git a/backend/internal/middleware/logging.go b/backend/internal/middleware/logging.go index 48f7f75..ac7be57 100644 --- a/backend/internal/middleware/logging.go +++ b/backend/internal/middleware/logging.go @@ -13,7 +13,9 @@ func LoggingMiddleware() gin.HandlerFunc { start := time.Now() path := c.Request.URL.Path - if path == "/health" { + // Probes run on a timer; logging every one buries the real traffic. + // Exact matches, not a prefix, so a real route under /health* still logs. + if path == "/health" || path == "/health/ready" { c.Next() return } diff --git a/backend/internal/services/docker.go b/backend/internal/services/docker.go index 8428805..05f0648 100644 --- a/backend/internal/services/docker.go +++ b/backend/internal/services/docker.go @@ -51,6 +51,26 @@ func NewDockerService(cfg *config.Config) (*DockerService, error) { }, nil } +// Ping reports whether the Docker daemon is reachable, honouring ctx's deadline. +// +// It is the readiness probe's dependency check. Ping is a single cheap +// round-trip, unlike the GetContainerList("") the old inline /health handler ran +// every 30 seconds. +// +// The nil receiver and nil client are handled rather than dereferenced: main +// leaves dockerService nil when the daemon was unreachable at startup, and a nil +// *DockerService stored in an interface is a non-nil interface value — calling +// through it would panic instead of reporting the outage it exists to report. +func (s *DockerService) Ping(ctx context.Context) error { + if s == nil || s.client == nil { + return fmt.Errorf("docker client not initialized") + } + if _, err := s.client.Ping(ctx); err != nil { + return err + } + return nil +} + func (s *DockerService) Logs(stack models.Stack, tail int) (string, error) { args := s.buildComposeArgs(stack, "logs", []string{"--tail", fmt.Sprintf("%d", tail), "--timestamps"}) diff --git a/docker-compose.prod.yaml b/docker-compose.prod.yaml index 391a786..0b507aa 100644 --- a/docker-compose.prod.yaml +++ b/docker-compose.prod.yaml @@ -43,7 +43,10 @@ services: labels: - "com.centurylinklabs.watchtower.enable=true" healthcheck: - test: ["CMD", "wget", "--no-verbose", "--tries=1", "--spider", "http://localhost:5001/health"] + # Liveness (no Docker call), and a real GET rather than --spider: --spider + # sends HEAD, which has no route and falls through to the SPA catch-all, + # so it returned 200 even when GET /health returned 503 (agent-os-69a). + test: ["CMD", "wget", "--no-verbose", "--tries=1", "-O", "/dev/null", "http://localhost:5001/health"] interval: 30s timeout: 10s start_period: 5s diff --git a/docker/Dockerfile b/docker/Dockerfile index 85216cf..a4b8ceb 100644 --- a/docker/Dockerfile +++ b/docker/Dockerfile @@ -169,8 +169,15 @@ ENV PGID=1000 EXPOSE 5001 +# Liveness, not readiness: /health makes no Docker call, so a Docker daemon +# outage cannot mark this container unhealthy and get it restarted for an outage +# a restart would not fix (agent-os-69a). +# +# A real GET, not --spider. --spider sends HEAD, no HEAD route is registered, so +# every HEAD falls through to the SPA catch-all and returns 200 — the check +# passed even when GET /health returned 503, making it a bare port check. HEALTHCHECK --interval=30s --timeout=10s --start-period=5s --retries=3 \ - CMD wget --no-verbose --tries=1 --spider http://localhost:5001/health || exit 1 + CMD wget --no-verbose --tries=1 -O /dev/null http://localhost:5001/health || exit 1 ENTRYPOINT ["/entrypoint.sh"] CMD ["/app/server"] diff --git a/docker/compose.yaml b/docker/compose.yaml index e51b6ff..fb2d416 100644 --- a/docker/compose.yaml +++ b/docker/compose.yaml @@ -49,7 +49,10 @@ services: memory: 512M pids_limit: 100 healthcheck: - test: ["CMD", "wget", "--no-verbose", "--tries=1", "--spider", "http://localhost:5001/health"] + # Liveness (no Docker call), and a real GET rather than --spider: --spider + # sends HEAD, which has no route and falls through to the SPA catch-all, + # so it returned 200 even when GET /health returned 503 (agent-os-69a). + test: ["CMD", "wget", "--no-verbose", "--tries=1", "-O", "/dev/null", "http://localhost:5001/health"] interval: 30s timeout: 10s retries: 3