From 06b193ba636798fd5f203ae731ca0a87234723dc Mon Sep 17 00:00:00 2001 From: Wikid82 Date: Mon, 5 Oct 2026 16:24:57 -0400 Subject: [PATCH 1/5] fix(security): harden outbound client configuration in update checker --- .../api/handlers/update_handler_test.go | 2 ++ backend/internal/services/update_service.go | 26 ++++++++++++++----- .../internal/services/update_service_test.go | 15 +++++++++++ 3 files changed, 36 insertions(+), 7 deletions(-) diff --git a/backend/internal/api/handlers/update_handler_test.go b/backend/internal/api/handlers/update_handler_test.go index 3e70837dc..a73d22a53 100644 --- a/backend/internal/api/handlers/update_handler_test.go +++ b/backend/internal/api/handlers/update_handler_test.go @@ -28,6 +28,7 @@ func TestUpdateHandler_Check(t *testing.T) { svc := services.NewUpdateService() err := svc.SetAPIURL(server.URL + "/releases/latest") assert.NoError(t, err) + svc.SetHTTPClient(server.Client()) // Setup Handler h := NewUpdateHandler(svc) @@ -58,6 +59,7 @@ func TestUpdateHandler_Check(t *testing.T) { svcError := services.NewUpdateService() err = svcError.SetAPIURL(serverError.URL) assert.NoError(t, err) + svcError.SetHTTPClient(serverError.Client()) hError := NewUpdateHandler(svcError) rError := gin.New() diff --git a/backend/internal/services/update_service.go b/backend/internal/services/update_service.go index 9a13b6581..a77cc5c55 100644 --- a/backend/internal/services/update_service.go +++ b/backend/internal/services/update_service.go @@ -18,7 +18,8 @@ type UpdateService struct { repoName string lastCheck time.Time cachedResult *UpdateInfo - apiURL string // For testing + apiURL string // For testing + httpClient *http.Client // Test-only override; nil selects the default safe client } type UpdateInfo struct { @@ -110,12 +111,9 @@ func (s *UpdateService) CheckForUpdates() (*UpdateInfo, error) { return s.cachedResult, nil } - // Use SSRF-safe HTTP client for defense-in-depth - // Note: SetAPIURL already validates the URL against github.com allowlist - client := network.NewSafeHTTPClient( - network.WithTimeout(5*time.Second), - network.WithAllowLocalhost(), // Allow localhost for testing - ) + // SetAPIURL already validates the URL against the github.com allowlist; + // the default client additionally blocks private and loopback targets. + client := s.outboundClient() req, err := http.NewRequest("GET", s.apiURL, http.NoBody) if err != nil { @@ -162,3 +160,17 @@ func (s *UpdateService) CheckForUpdates() (*UpdateInfo, error) { return info, nil } + +// SetHTTPClient overrides the outbound client. Intended for tests that need +// to reach local httptest servers; production code never calls it. +func (s *UpdateService) SetHTTPClient(c *http.Client) { + s.httpClient = c +} + +// outboundClient returns the injected client, or the default SSRF-safe client. +func (s *UpdateService) outboundClient() *http.Client { + if s.httpClient != nil { + return s.httpClient + } + return network.NewSafeHTTPClient(network.WithTimeout(5 * time.Second)) +} diff --git a/backend/internal/services/update_service_test.go b/backend/internal/services/update_service_test.go index 570856621..42f69bc51 100644 --- a/backend/internal/services/update_service_test.go +++ b/backend/internal/services/update_service_test.go @@ -29,6 +29,7 @@ func TestUpdateService_CheckForUpdates(t *testing.T) { us := NewUpdateService() err := us.SetAPIURL(server.URL + "/releases/latest") assert.NoError(t, err) + us.SetHTTPClient(server.Client()) // us.currentVersion is private, so we can't set it directly in test unless we export it or add a setter. // However, NewUpdateService sets it from version.Version. // We can temporarily change version.Version if it's a var, but it's likely a const or var in another package. @@ -158,3 +159,17 @@ func TestUpdateService_SetAPIURL_GitHubValidation(t *testing.T) { }) } } + +func TestUpdateService_DefaultClientRejectsLoopback(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + _ = json.NewEncoder(w).Encode(githubRelease{TagName: "v1.0.0"}) + })) + defer server.Close() + + us := NewUpdateService() + assert.NoError(t, us.SetAPIURL(server.URL)) + + // No injected client: the default safe client must refuse loopback targets. + _, err := us.CheckForUpdates() + assert.Error(t, err) +} From 69eaac27d8d5d78d1d1a72a330ed5399e2ee50af Mon Sep 17 00:00:00 2001 From: Wikid82 Date: Mon, 5 Oct 2026 16:27:03 -0400 Subject: [PATCH 2/5] fix(security): harden outbound client configuration in hub sync --- backend/internal/crowdsec/hub_sync.go | 16 +++++++++--- backend/internal/crowdsec/hub_sync_test.go | 30 ++++++++++++++++++++++ 2 files changed, 42 insertions(+), 4 deletions(-) diff --git a/backend/internal/crowdsec/hub_sync.go b/backend/internal/crowdsec/hub_sync.go index f344d6f3e..60d8bbd5a 100644 --- a/backend/internal/crowdsec/hub_sync.go +++ b/backend/internal/crowdsec/hub_sync.go @@ -84,6 +84,11 @@ type HubService struct { ApplyTimeout time.Duration } +// hubAllowLoopback is a test-only seam. It is always false in production and is +// only toggled from _test.go files. Tests that toggle it must not call +// t.Parallel(), as it is shared package state. +var hubAllowLoopback bool + // validateHubURL validates a hub URL for security (SSRF protection - HIGH-001). // This function prevents Server-Side Request Forgery by: // 1. Enforcing HTTPS for production hub URLs @@ -175,18 +180,21 @@ func NewHubService(exec CommandExecutor, cache *HubCache, dataDir string) *HubSe // Hub URLs are validated by validateHubURL() which: // - Enforces HTTPS for production // - Allowlists known CrowdSec domains (hub-data.crowdsec.net, hub.crowdsec.net, raw.githubusercontent.com) -// - Allows localhost for testing +// - Blocks loopback unless the test-only hubAllowLoopback seam is set // Using network.NewSafeHTTPClient provides defense-in-depth at the connection level. func newHubHTTPClient(timeout time.Duration) *http.Client { - return network.NewSafeHTTPClient( + opts := []network.Option{ network.WithTimeout(timeout), - network.WithAllowLocalhost(), // Allow localhost for testing network.WithAllowedDomains( "hub-data.crowdsec.net", "hub.crowdsec.net", "raw.githubusercontent.com", ), - ) + } + if hubAllowLoopback { + opts = append(opts, network.WithAllowLocalhost()) + } + return network.NewSafeHTTPClient(opts...) } func normalizeHubBaseURL(raw string) string { diff --git a/backend/internal/crowdsec/hub_sync_test.go b/backend/internal/crowdsec/hub_sync_test.go index 3f3dfbb8f..1fae1f985 100644 --- a/backend/internal/crowdsec/hub_sync_test.go +++ b/backend/internal/crowdsec/hub_sync_test.go @@ -2511,3 +2511,33 @@ func TestFindIndexEntry_EmptySlug(t *testing.T) { _, found := findIndexEntry(idx, " ") require.False(t, found) } + +// setHubAllowLoopbackForTest toggles the package-level seam. Callers must not +// use t.Parallel(). +func setHubAllowLoopbackForTest(t *testing.T, v bool) { + t.Helper() + prev := hubAllowLoopback + hubAllowLoopback = v + t.Cleanup(func() { hubAllowLoopback = prev }) +} + +// Must not run in parallel: relies on shared seam state. +func TestNewHubHTTPClient_LoopbackSeam(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusOK) + })) + defer srv.Close() + + get := func() error { + resp, err := newHubHTTPClient(2 * time.Second).Get(srv.URL) + if err == nil { + _ = resp.Body.Close() + } + return err + } + + require.Error(t, get(), "default client must reject loopback") + + setHubAllowLoopbackForTest(t, true) + require.NoError(t, get(), "seam permits loopback in tests") +} From da44a1f1d04176d72ab267b3b7a5e046e03a2bf3 Mon Sep 17 00:00:00 2001 From: Wikid82 Date: Mon, 5 Oct 2026 16:27:52 -0400 Subject: [PATCH 3/5] docs: clarify intentional loopback allowances in outbound clients --- backend/internal/crowdsec/registration.go | 6 +++--- backend/internal/services/uptime_check.go | 7 +++++-- 2 files changed, 8 insertions(+), 5 deletions(-) diff --git a/backend/internal/crowdsec/registration.go b/backend/internal/crowdsec/registration.go index f7a89a574..b721e4ba0 100644 --- a/backend/internal/crowdsec/registration.go +++ b/backend/internal/crowdsec/registration.go @@ -139,7 +139,7 @@ func CheckLAPIHealth(lapiURL string) bool { // Use SSRF-safe HTTP client with localhost allowed (LAPI is localhost-only) client := network.NewSafeHTTPClient( network.WithTimeout(defaultHealthTimeout), - network.WithAllowLocalhost(), // LAPI validated to be localhost only + network.WithAllowLocalhost(), // LAPI is loopback-only; the client permits no other private ranges ) resp, err := client.Do(req) if err != nil { @@ -187,7 +187,7 @@ func GetLAPIVersion(ctx context.Context, lapiURL string) (string, error) { // Use SSRF-safe HTTP client with localhost allowed (LAPI is localhost-only) client := network.NewSafeHTTPClient( network.WithTimeout(defaultHealthTimeout), - network.WithAllowLocalhost(), // LAPI validated to be localhost only + network.WithAllowLocalhost(), // LAPI is loopback-only; the client permits no other private ranges ) resp, err := client.Do(req) if err != nil { @@ -230,7 +230,7 @@ func checkDecisionsEndpoint(ctx context.Context, lapiURL string) bool { // Use SSRF-safe HTTP client with localhost allowed (LAPI is localhost-only) client := network.NewSafeHTTPClient( network.WithTimeout(defaultHealthTimeout), - network.WithAllowLocalhost(), // LAPI validated to be localhost only + network.WithAllowLocalhost(), // LAPI is loopback-only; the client permits no other private ranges ) resp, err := client.Do(req) if err != nil { diff --git a/backend/internal/services/uptime_check.go b/backend/internal/services/uptime_check.go index bfe40b9bd..c8311e86f 100644 --- a/backend/internal/services/uptime_check.go +++ b/backend/internal/services/uptime_check.go @@ -76,6 +76,9 @@ func newUptimeChecker(svc *UptimeService) *uptimeChecker { network.WithTimeout(20*time.Second), network.WithDialTimeout(3*time.Second), network.WithMaxRedirects(0), + // Intentional: monitors target admin-configured local services, so + // loopback and RFC 1918 are permitted. Link-local, cloud metadata and + // other restricted ranges stay blocked; redirects are not followed. network.WithAllowLocalhost(), network.WithAllowRFC1918(), network.WithKeepAlive(100, 4, 30*time.Second), @@ -103,8 +106,8 @@ func (c *uptimeChecker) probe(ctx context.Context, monitor models.UptimeMonitor) case "http", "https": validatedURL, err := security.ValidateExternalURL( monitor.URL, - // Uptime monitors are an explicit admin-configured feature and - // commonly target loopback in local/dev setups (and in tests). + // Intentional product behavior: uptime monitors are admin-configured + // and legitimately target services on the local host (loopback). security.WithAllowLocalhost(), security.WithAllowHTTP(), security.WithTimeout(3*time.Second), From 70a8ce0d68c0d38a67487007046008d7da1671eb Mon Sep 17 00:00:00 2001 From: Wikid82 Date: Mon, 5 Oct 2026 16:52:36 -0400 Subject: [PATCH 4/5] refactor: remove unused LAPI health helpers and clarify hub client docs --- backend/internal/crowdsec/hub_sync.go | 8 +- backend/internal/crowdsec/registration.go | 146 ------------------ .../internal/crowdsec/registration_test.go | 97 ------------ .../internal/services/update_service_test.go | 5 +- docs/troubleshooting/crowdsec.md | 1 + 5 files changed, 10 insertions(+), 247 deletions(-) diff --git a/backend/internal/crowdsec/hub_sync.go b/backend/internal/crowdsec/hub_sync.go index 60d8bbd5a..8ab2fbbb2 100644 --- a/backend/internal/crowdsec/hub_sync.go +++ b/backend/internal/crowdsec/hub_sync.go @@ -91,9 +91,11 @@ var hubAllowLoopback bool // validateHubURL validates a hub URL for security (SSRF protection - HIGH-001). // This function prevents Server-Side Request Forgery by: -// 1. Enforcing HTTPS for production hub URLs -// 2. Allowlisting known CrowdSec hub domains -// 3. Allowing localhost/test URLs for development and testing +// 1. Enforcing HTTPS for production hub URLs +// 2. Allowlisting known CrowdSec hub domains +// 3. Accepting localhost/test hostnames at this layer; the dial layer +// (network.NewSafeHTTPClient) still blocks loopback and private targets +// unless the test-only hubAllowLoopback seam is set // // Returns: error if URL is invalid or not allowlisted func validateHubURL(rawURL string) error { diff --git a/backend/internal/crowdsec/registration.go b/backend/internal/crowdsec/registration.go index b721e4ba0..d3567118c 100644 --- a/backend/internal/crowdsec/registration.go +++ b/backend/internal/crowdsec/registration.go @@ -5,23 +5,17 @@ import ( "context" "encoding/json" "fmt" - "io" - "net/http" neturl "net/url" "os" "os/exec" "strings" "time" - - "github.com/Wikid82/charon/backend/internal/logger" - "github.com/Wikid82/charon/backend/internal/network" ) const ( // defaultLAPIURL is the default CrowdSec LAPI URL. // Port 8085 is used to avoid conflict with Charon management API on port 8080. defaultLAPIURL = "http://127.0.0.1:8085" - defaultHealthTimeout = 5 * time.Second defaultRegistrationName = "caddy-bouncer" ) @@ -34,12 +28,6 @@ type BouncerRegistration struct { CreatedAt time.Time `json:"created_at,omitempty"` } -// LAPIHealthResponse represents the health check response from CrowdSec LAPI. -type LAPIHealthResponse struct { - Message string `json:"message,omitempty"` - Version string `json:"version,omitempty"` -} - // validateLAPIURL validates a CrowdSec LAPI URL for security (SSRF protection - MEDIUM-001). // CrowdSec LAPI typically runs on localhost or within an internal network. // This function ensures the URL: @@ -120,140 +108,6 @@ func EnsureBouncerRegistered(ctx context.Context, lapiURL string) (string, error return registerBouncer(ctx, defaultRegistrationName) } -// CheckLAPIHealth verifies CrowdSec LAPI is responding. -func CheckLAPIHealth(lapiURL string) bool { - if lapiURL == "" { - lapiURL = defaultLAPIURL - } - - ctx, cancel := context.WithTimeout(context.Background(), defaultHealthTimeout) - defer cancel() - - // Try the /health endpoint first (standard LAPI health check) - healthURL := strings.TrimRight(lapiURL, "/") + "/health" - req, err := http.NewRequestWithContext(ctx, http.MethodGet, healthURL, http.NoBody) - if err != nil { - return false - } - - // Use SSRF-safe HTTP client with localhost allowed (LAPI is localhost-only) - client := network.NewSafeHTTPClient( - network.WithTimeout(defaultHealthTimeout), - network.WithAllowLocalhost(), // LAPI is loopback-only; the client permits no other private ranges - ) - resp, err := client.Do(req) - if err != nil { - // Fallback: try the /v1/decisions endpoint with a HEAD request - return checkDecisionsEndpoint(ctx, lapiURL) - } - defer func() { - if closeErr := resp.Body.Close(); closeErr != nil { - logger.Log().WithError(closeErr).Warn("Failed to close response body") - } - }() - - // Check content-type to ensure we're getting JSON from actual LAPI (not HTML from frontend) - contentType := resp.Header.Get("Content-Type") - if contentType != "" && !strings.Contains(contentType, "application/json") { - // Not JSON response, likely hitting a frontend/proxy - return false - } - - // LAPI returns 200 OK for healthy status - if resp.StatusCode == http.StatusOK { - return true - } - - // If health endpoint returned non-OK, try decisions endpoint fallback - if resp.StatusCode == http.StatusNotFound { - return checkDecisionsEndpoint(ctx, lapiURL) - } - - return false -} - -// GetLAPIVersion retrieves the CrowdSec LAPI version. -func GetLAPIVersion(ctx context.Context, lapiURL string) (string, error) { - if lapiURL == "" { - lapiURL = defaultLAPIURL - } - - versionURL := strings.TrimRight(lapiURL, "/") + "/v1/version" - req, err := http.NewRequestWithContext(ctx, http.MethodGet, versionURL, http.NoBody) - if err != nil { - return "", fmt.Errorf("create version request: %w", err) - } - - // Use SSRF-safe HTTP client with localhost allowed (LAPI is localhost-only) - client := network.NewSafeHTTPClient( - network.WithTimeout(defaultHealthTimeout), - network.WithAllowLocalhost(), // LAPI is loopback-only; the client permits no other private ranges - ) - resp, err := client.Do(req) - if err != nil { - return "", fmt.Errorf("version request failed: %w", err) - } - defer func() { - if closeErr := resp.Body.Close(); closeErr != nil { - logger.Log().WithError(closeErr).Warn("Failed to close response body") - } - }() - - if resp.StatusCode != http.StatusOK { - return "", fmt.Errorf("version request returned status %d", resp.StatusCode) - } - - body, err := io.ReadAll(resp.Body) - if err != nil { - return "", fmt.Errorf("read version response: %w", err) - } - - var versionResp struct { - Version string `json:"version"` - } - if err := json.Unmarshal(body, &versionResp); err != nil { - // Some versions return plain text - return strings.TrimSpace(string(body)), nil - } - - return versionResp.Version, nil -} - -// checkDecisionsEndpoint is a fallback health check using the decisions endpoint. -func checkDecisionsEndpoint(ctx context.Context, lapiURL string) bool { - decisionsURL := strings.TrimRight(lapiURL, "/") + "/v1/decisions" - req, err := http.NewRequestWithContext(ctx, http.MethodGet, decisionsURL, http.NoBody) - if err != nil { - return false - } - - // Use SSRF-safe HTTP client with localhost allowed (LAPI is localhost-only) - client := network.NewSafeHTTPClient( - network.WithTimeout(defaultHealthTimeout), - network.WithAllowLocalhost(), // LAPI is loopback-only; the client permits no other private ranges - ) - resp, err := client.Do(req) - if err != nil { - return false - } - defer func() { - if err := resp.Body.Close(); err != nil { - logger.Log().WithError(err).Warn("Failed to close response body") - } - }() - - // Check content-type to avoid false positives from HTML responses - contentType := resp.Header.Get("Content-Type") - if contentType != "" && !strings.Contains(contentType, "application/json") { - // Not JSON response, likely hitting a frontend/proxy - return false - } - - // 401 is expected without auth, but indicates LAPI is running - // 200 with empty array is also valid (no decisions) - return resp.StatusCode == http.StatusOK || resp.StatusCode == http.StatusUnauthorized -} - // getBouncerAPIKey returns the bouncer API key from environment variables. func getBouncerAPIKey() string { // Check multiple possible env var names for the API key diff --git a/backend/internal/crowdsec/registration_test.go b/backend/internal/crowdsec/registration_test.go index ec9bddb85..da8209129 100644 --- a/backend/internal/crowdsec/registration_test.go +++ b/backend/internal/crowdsec/registration_test.go @@ -3,8 +3,6 @@ package crowdsec import ( "context" "io/fs" - "net/http" - "net/http/httptest" "os" "path/filepath" "testing" @@ -58,67 +56,6 @@ func withPath(t *testing.T, newPath string, fn func()) { fn() } -func TestCheckLAPIHealth_Healthy(t *testing.T) { - // Create a mock LAPI server that returns 200 OK with JSON content-type - server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - if r.URL.Path == "/health" { - w.Header().Set("Content-Type", "application/json") - w.WriteHeader(http.StatusOK) - _, _ = w.Write([]byte(`{"status":"ok"}`)) - return - } - w.WriteHeader(http.StatusNotFound) - })) - defer server.Close() - - healthy := CheckLAPIHealth(server.URL) - assert.True(t, healthy, "LAPI should be healthy") -} - -func TestCheckLAPIHealth_Unhealthy(t *testing.T) { - // Create a mock LAPI server that returns 500 - server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - w.WriteHeader(http.StatusInternalServerError) - })) - defer server.Close() - - healthy := CheckLAPIHealth(server.URL) - assert.False(t, healthy, "LAPI should be unhealthy") -} - -func TestCheckLAPIHealth_Unreachable(t *testing.T) { - // Use an invalid URL that won't connect - healthy := CheckLAPIHealth("http://127.0.0.1:19999") - assert.False(t, healthy, "LAPI should be unreachable") -} - -func TestCheckLAPIHealth_FallbackToDecisions(t *testing.T) { - // Create a mock LAPI server where /health fails but /v1/decisions returns 401 - server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - if r.URL.Path == "/health" { - w.WriteHeader(http.StatusNotFound) - return - } - if r.URL.Path == "/v1/decisions" { - w.Header().Set("Content-Type", "application/json") - w.WriteHeader(http.StatusUnauthorized) // Expected without auth - return - } - w.WriteHeader(http.StatusNotFound) - })) - defer server.Close() - - healthy := CheckLAPIHealth(server.URL) - // Should fallback to decisions endpoint check which returns 401 (indicates running) - assert.True(t, healthy, "LAPI should be healthy via decisions fallback") -} - -func TestCheckLAPIHealth_DefaultURL(t *testing.T) { - // With empty URL, should use default (which won't be running in test) - healthy := CheckLAPIHealth("") - assert.False(t, healthy, "Default LAPI should not be running in test environment") -} - func TestGetBouncerAPIKey_FromEnv(t *testing.T) { // Save and restore original env original := os.Getenv("CROWDSEC_API_KEY") @@ -266,40 +203,6 @@ exit 2 }) } -func TestGetLAPIVersion_JSON(t *testing.T) { - server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - if r.URL.Path != "/v1/version" { - w.WriteHeader(http.StatusNotFound) - return - } - w.Header().Set("Content-Type", "application/json") - w.WriteHeader(http.StatusOK) - _, _ = w.Write([]byte(`{"version":"1.2.3"}`)) - })) - defer server.Close() - - ver, err := GetLAPIVersion(context.Background(), server.URL) - assert.NoError(t, err) - assert.Equal(t, "1.2.3", ver) -} - -func TestGetLAPIVersion_PlainText(t *testing.T) { - server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - if r.URL.Path != "/v1/version" { - w.WriteHeader(http.StatusNotFound) - return - } - w.Header().Set("Content-Type", "text/plain") - w.WriteHeader(http.StatusOK) - _, _ = w.Write([]byte("vX.Y.Z\n")) - })) - defer server.Close() - - ver, err := GetLAPIVersion(context.Background(), server.URL) - assert.NoError(t, err) - assert.Equal(t, "vX.Y.Z", ver) -} - func TestValidateLAPIURL(t *testing.T) { tests := []struct { name string diff --git a/backend/internal/services/update_service_test.go b/backend/internal/services/update_service_test.go index 42f69bc51..800941c47 100644 --- a/backend/internal/services/update_service_test.go +++ b/backend/internal/services/update_service_test.go @@ -7,6 +7,7 @@ import ( "testing" "time" + "github.com/Wikid82/charon/backend/internal/network" "github.com/stretchr/testify/assert" ) @@ -171,5 +172,7 @@ func TestUpdateService_DefaultClientRejectsLoopback(t *testing.T) { // No injected client: the default safe client must refuse loopback targets. _, err := us.CheckForUpdates() - assert.Error(t, err) + if assert.Error(t, err) { + assert.ErrorIs(t, err, network.ErrBlockedAddress) + } } diff --git a/docs/troubleshooting/crowdsec.md b/docs/troubleshooting/crowdsec.md index 5dff31b77..7279858a4 100644 --- a/docs/troubleshooting/crowdsec.md +++ b/docs/troubleshooting/crowdsec.md @@ -14,6 +14,7 @@ Keep Cerberus terminology and the Configuration Packages flow in mind while debu - Docker images (v1.7.4+): cscli is pre-installed. - Bare-metal deployments: install cscli for Hub preset sync or use HTTP fallback with HUB_BASE_URL. - HUB_BASE_URL points to a JSON hub endpoint (default: ). Redirects to HTML will be rejected. + - A loopback hub mirror is no longer reachable from the hub client; RFC 1918 hosts were already blocked. - Proxy env is set when required: HTTP(S)_PROXY and NO_PROXY are respected by the hub client. - For slow or proxied networks, increase HUB_PULL_TIMEOUT_SECONDS (default 25) and HUB_APPLY_TIMEOUT_SECONDS (default 45) to avoid premature timeouts. - Preset workflow: pull from Hub using cache keys/ETags → preview changes → apply with automatic backup and reload flag. From d48bbcee1b94c03db9772d93e330f35f03015428 Mon Sep 17 00:00:00 2001 From: Wikid82 Date: Mon, 5 Oct 2026 17:26:57 -0400 Subject: [PATCH 5/5] docs: add QA report for outbound client hardening follow-ups --- docs/reports/qa_report.md | 192 +++++--------------------------------- 1 file changed, 21 insertions(+), 171 deletions(-) diff --git a/docs/reports/qa_report.md b/docs/reports/qa_report.md index 449dc43f8..b6be7b71a 100644 --- a/docs/reports/qa_report.md +++ b/docs/reports/qa_report.md @@ -1,177 +1,27 @@ -# QA and Security Report - GH #1422 automatic database maintenance +# QA Report -Branch `feat/db-maintenance-1422`, HEAD e937a7dc, range `3437ab88..HEAD`. Spec: docs/plans/current_spec.md rev 7. +Branch: `fix/localhost-allowance-followups` (4 commits on `development`) -## Verdict: PASS (no blocking findings) +## Scope -## Gates +Hardening outbound client configuration in the update checker and hub sync, removing unused LAPI helpers. Backend and documentation changes only (plus a one-line addition to `docs/troubleshooting/crowdsec.md`). -| Gate | Command | Result | -|---|---|---| -| Playwright | `npx playwright test tests/settings/database-maintenance.spec.ts --project=firefox` (after docker-rebuild-e2e) | 14/14 passed | -| GORM scan | `./scripts/scan-gorm-security.sh --check` | 0 CRITICAL, 0 HIGH, 0 MEDIUM | -| Patch coverage | `bash scripts/local-patch-report.sh` | artifacts present; overall 94.8%, backend 94.4%, frontend 100% (uncovered lines are error branches) | -| CodeQL Go+JS | `lefthook run codeql --all-files` (CodeQL 2.26.4) | Go 4 results, all pre-existing and suppressed in codeql-suppressions.yml (none in files of this branch); JS 0; 0 blocking | -| Trivy | `docker build -t charon:local .` + `aquasec/trivy image --severity CRITICAL,HIGH charon:local` | OS 0, app/charon 0, caddy 0; 1 HIGH in each of crowdsec and cscli binaries (CVE-2026-32286, pgproto3, no fixed version), third-party, already tracked in .trivyignore/.grype.yaml/SECURITY.md. No CRITICAL. | -| Pre-commit | `lefthook run pre-commit --all-files` | all 16 hooks passed (incl. semgrep) | -| Lint | `make lint-fast`, `make lint-backend` | 0 issues both | -| Backend coverage | `scripts/go-test-coverage.sh` | statements 92.1%, lines 89.1% (gate 87%) PASS | -| Frontend coverage | `scripts/frontend-test-coverage.sh` | 289 files / 3586 tests passed (4 skipped, 2 todo pre-existing), lines 91.31% (gate 87%) PASS | -| Types / builds | `npm run type-check`, `go build ./...`, `npm run build` | all OK | -| Backend suite | `go test ./...` | all packages ok, 0 failures | -| Race | `go test -race ./internal/dbmaint/... ./cmd/... ./internal/api/routes/... ./internal/services/... ./internal/database/...` | all ok | - -Note: the Trivy container was pointed at the rootless docker socket (/run/user/1001/docker.sock) instead of the stale /var/run/docker.sock hard-coded in `make security-scan-full`. - -## Synthetic real-data run (scratch DB, removed afterwards) - -Live schema via AutoMigrate, 60 monitors, 3,000,000 heartbeats, oldest 1.23M deleted, WAL, auto_vacuum=0, real StartupPlan/Start/Run path: - -- Before: 569,270,272 B main, 138,982 pages, 56,645 free (40.8%), reclaimable 232 MB; Decide = Run. -- Advisor (Reclaimer.AfterPrune on a legacy DB): Pending=true, 232 MB. -- GET /api/v1/system/database before: notice `restart_to_optimize` (info), can_request_optimize true. -- Conversion wall time 5.7 s; during it GET /, /api/v1/system/database and /api/v1/health/db returned 503 and /api/v1/maintenance/status returned 200 `{"active":true,"phase":"converting",...}`. -- After: 318,664,704 B (-44%), integrity_check ok, auto_vacuum=2, journal_mode=wal, 1,770,000 rows intact, last_result converted, notice null, `.tmp` empty and mode 0700. -- Steady state: after pruning 770k more rows, Reclaimer drained 25,291 pages in 13 steps (318.7 MB to 215.1 MB), advice no longer pending. - -## Security audit - -- SQL: every statement in dbmaint, database and the pruner is a constant or parameterised (`?`); the only formatted ones take integer constants/ints (`incremental_vacuum(%d)`, `busy_timeout=%d`); `"PRAGMA "+name` callers pass literals only. No user input reaches DDL/PRAGMA. -- Gate: answers only GET/HEAD on the exact paths /api/v1/health and /api/v1/maintenance/status; /api/v1/health/db, trailing-slash and sub-paths are blocked (503 JSON or HTML) while active and pass through otherwise; the gate sits before all DB-touching middleware; status/health do no DB access. CSP is hash-based (default-src 'none', script/style sha256 computed from the embedded page, verified by test), plus no-store, nosniff, X-Frame-Options DENY. Remote users cannot trigger the gate: phases change only from the boot-time runner. -- Endpoints: GET /system/database and POST/DELETE /system/database/optimize-on-restart are on managementAdmin; tests assert 401 (no token), 403 (role=user). Cookie auth is SameSite Strict/Lax like every other mutating endpoint; no new CSRF surface. POST is idempotent (200 {requested:true}); error bodies are generic or name only the env var. -- Reserved prefix guard (`migration.`, `maintenance.`): normalised (lowercase+trim) prefix check on UpdateSetting and on every flattened PatchConfig key (nested JSON, mixed case, whitespace, empty-segment cases covered); GetSettings and PatchConfig responses filter reserved rows; Category is ignored. Other settings writers use fixed keys. -- Temp dir: `/.tmp` via Lstat (symlink and non-dir refused), owner == euid, forced 0700, filepath.Clean; operator-set SQLITE_TMPDIR honoured untouched; failure falls back with a warning. -- State/file_id: inode-only id, state of another file is ignored/deleted, in-progress marker counts as a failed attempt, 3-attempt back-off, a fresh admin request resets it. The flag cannot force a conversion below the 100 MB floor nor skip the integrity, disk or writer-lock checks. Corrupt state rows are deleted, not trusted. -- Writer exclusion: BEGIN EXCLUSIVE probe with busy_timeout=0 on the pinned single pool connection, retries then skip as database_busy (tested with a real second writer). -- Logs: sizes, counts and reason codes only; no secrets, no paths beyond config. -- DoS: status endpoint is a mutex read plus fmt (no DB, no allocation proportional to input). -- Provenance: no session IDs, claude.ai links, Co-Authored-By or "generated with" in any commit message or diff; all 14 commit subjects use conventional prefixes; none uses a `(security)` scope. Added `nolint` comments are all justified test or gosec-G115 notes. - -## Findings (all non-blocking, informational) - -1. LOW/INFO - The Makefile target `security-scan-full` mounts /var/run/docker.sock, which on this dev host is a stale root daemon; the scan needs the rootless socket. Pre-existing tooling issue, not part of this PR. -2. INFO - FileID is inode-only; a replaced database file that happens to reuse the old inode would inherit stale attempts/last_result. Consequence is bounded (back-off counter or a notice), and Load clears state on mismatch. No action needed. -3. INFO - /api/v1/health (GET/HEAD) is now answered by the gate and so no longer passes cerberus.RateLimitMiddleware; the handler is DB-free and cheap, so this is acceptable. -4. INFO - Trivy HIGH CVE-2026-32286 in bundled crowdsec/cscli binaries has no fixed version and is already tracked. - - ---- - -## Re-run: Tasks > Database page and development merge - -Date: 2026-10-01. Branch `feat/db-maintenance-1422`, HEAD `107241f8`. Scope: merge of `development` (d3625e08) plus Addendum A (Database page under Tasks). Heavy commands ran with TMPDIR/GOTMPDIR under /var/tmp. - -Result: **PASS** (no blocking findings). - -### Gate results - -| Gate | Command | Outcome | -|---|---|---| -| E2E (Database page) | `npx playwright test tests/tasks/database-maintenance.spec.ts --project=firefox` (charon-e2e rebuilt first) | 25 passed | -| E2E (tab bar) | `tests/tasks/logs-viewing.spec.ts --project=firefox` | 25 passed | -| E2E (Settings uptime card) | `tests/monitoring/uptime-monitoring-scale.spec.ts --project=firefox` | 9 passed | -| Patch coverage | `bash scripts/local-patch-report.sh` (baseline origin/development...HEAD) | Overall 94.9% (1532/1614), backend 94.4%, frontend 100% (141/141), agent n/a; all pass; artifacts present | -| CodeQL | `lefthook run codeql --all-files` (CLI 2.26.4) | Go: 4 results, all already suppressed in codeql-suppressions.yml (none in files touched by this branch); JS: 0; 0 blocking | -| Trivy | `docker build -t charon:local .` then `aquasec/trivy image --severity CRITICAL,HIGH` via rootless socket | With .trivyignore: 0 findings. Without it: only CVE-2026-32286 (HIGH, pgproto3/v2, no fix) in crowdsec and cscli, the known accepted item | -| Pre-commit | `lefthook run pre-commit --all-files` | all hooks passed (semgrep 0 findings) | -| Lint | `make lint-fast`; `make lint-backend` | 0 issues; 0 issues | -| Frontend coverage | `scripts/frontend-test-coverage.sh` | 291 files / 3633 tests passed; lines 91.35% (gate 87%) | -| Backend coverage | `scripts/go-test-coverage.sh` | pass; line coverage 89.1% (gate 87%), statements 92.2% | -| Type/build | `npm run type-check`; `go build ./... && go vet ./...`; `npm run build` | all clean | -| Go tests | `go test ./...`; `go test -race ./internal/dbmaint/... ./internal/api/... ./cmd/...` | 0 failures; 0 races | - -### Security / compliance audit - -- Admin gating: `/tasks/database` is wrapped in `RequireRole allowed={['admin']}` (App.tsx:139); Layout nav item (Layout.tsx:178) and Tasks tab (Tasks.tsx:17) are admin-only. E2E confirms non-admin has no nav item, deep link redirects to "/", and no `/system/database` request is made. -- XSS: no `dangerouslySetInnerHTML`/`innerHTML` in the new page, component, hook or API client; all values rendered as React text. -- Accessibility: passive notices use `role="status"`; load error uses an `Alert` with its own accessible name (covered by E2E). -- `grep -rn "systemSettings.database" frontend/src` is empty; no leftover references to removed components. -- Commit hygiene on `3437ab88..HEAD`: no session IDs, claude.ai links, Co-Authored-By or "generated with" lines in messages or diff; all non-merge subjects use conventional prefixes; no `(security)` scope used. -- Docs: links in docs/database-maintenance.md, docs/features.md, ARCHITECTURE.md resolve (the one flagged URL is an external GitHub link); ARCHITECTURE.md and the docs refer to Tasks -> Database. `docs-site/docs/` is git-ignored and untracked (no hand edits). - -### Findings - -None blocking. Informational: local patch coverage shortfalls are confined to existing dbmaint/database error branches (diskspace.go, probe.go, tmpdir.go, database.go), above the 85% backend threshold overall. GH #1426 /tmp leak not exercised as a failure. - ---- +## Results -## Re-run: #1427 follow-ups - -Branch `fix/db-maintenance-followups-1427`, range `a7aa2909^..HEAD` (HEAD d18a541a). Backend + docs only. Heavy commands ran with TMPDIR/GOTMPDIR under /var/tmp (scratch dir removed afterwards). - -**Verdict: PASS. No blocking findings.** - -### Gates - -| Gate | Command | Outcome | -|---|---|---| -| Build/vet/fmt | `cd backend && go build ./... && go vet ./...`; `gofmt -l .` | clean; gofmt lists nothing | -| Full backend suite | `go test -count=1 ./...` | 0 failures | -| Race (2 runs) | `go test -race -count=1 ./internal/dbmaint/... ./cmd/api/... ./internal/database/... ./internal/api/... ./internal/services/...` | run 1 and run 2 both exit 0, no races | -| Slow real-main() test (2 runs, -race, -v) | `./cmd/api -run TestMaintenance_StopDuringConversionIsSafeAndTheNextBootConverts` | PASS twice (SIGTERM ~18s, SIGKILL ~17-19s), no flakiness | -| Patch coverage | `bash scripts/local-patch-report.sh` (re-run after fresh coverage) | artifacts present; backend 51/51 changed lines = 100% (gate 85%); frontend/agent 0 changed lines | -| Backend coverage | `scripts/go-test-coverage.sh` | pass; line coverage 89.1% (gate 87%), statements 92.1% | -| Lefthook | `lefthook run pre-commit --all-files` | all hooks passed (semgrep 0 findings, 367 rules) | -| Lint | `make lint-fast`; `make lint-backend` | 0 issues; 0 issues | -| GORM | `./scripts/scan-gorm-security.sh --check` | PASSED, 0 issues (2 informational suggestions) | -| CodeQL / Trivy | deferred to CI | no `feat:`, no new network surface or endpoint, no new dependency | -| Frontend untouched | `git diff --stat a7aa2909^..HEAD -- frontend tests` | empty; no frontend gates needed | - -Working tree was clean after the runs (changelog.json not modified); nothing committed. - -### Security / compliance audit - -- SQL: `DiscardMarkerIfConverted`, the counters and markers go through the existing `getJSON`/`ClearInProgress`/`ResetAttempts` helpers (parameterised); `PRAGMA journal_size_limit=%d` is built from a compile-time integer constant (64<<20), no user input, no injection path. -- Temp dir: `PrepareTempDir` keeps `filepath.Clean`, `Lstat`, the symlink/non-directory refusal, the owner==euid check and the 0700 check unchanged. The new `ErrTempDirSkipped` path returns early only when euid==0 and the data directory is owned by a non-root uid, and then touches nothing (a symlinked or attacker-owned `.tmp` is simply never reached; the root process does not create or chmod anything). When the process is non-root or root-owned data, the original checks apply. `ApplyTempDir` handles the skip at Debug level only. -- Back-off: the failure limit now also applies when the user's flag is set, and a flag-driven run no longer bypasses it. The flag is only set by `POST /system/database/optimize-on-restart`, registered on `managementAdmin` (routes.go:583, admin-only); that handler also calls `ResetAttempts`. No unauthenticated or non-admin path can force or skip a conversion. Pre-conversion failures count and keep the flag, bounded by the 3-attempt limit (no infinite retry loop, no repeated heavy work). -- Busy classification: `errors.As` on `Code()` matches `code & 0xff == 5` (SQLITE_BUSY and its extended codes only; SQLITE_LOCKED is 6 and is not matched). The text fallback is unchanged. -- Logs: new messages carry only the reason string; no secrets or new paths. -- DoS: retries are capped at 3; WAL cap bounds disk growth; no new unbounded loop or allocation. -- Docs: the chown advice is scoped to `/.tmp`, tells the operator to stop Charon first, and mentions deleting the folder as an alternative. -- Commit hygiene: `git log a7aa2909^..HEAD --format=%B` and the diff contain no session IDs, claude.ai session links, Co-Authored-By or "generated with" lines. All 11 subjects use `fix:`, `refactor:`, `test:` or `docs:` prefixes; no `(security)` scope is used. - -### Findings - -None blocking. Informational: -- Low (docs/comment nit, drain.go:63-64): the doc comment now wraps unevenly after the `spike_test.go` to `driver_behavior_test.go` rename (one short line, one long line). Cosmetic only; no action required. -- Info: GH #1426 (/tmp leak) was not observed as a failure in any gate. - - -## Re-run: #1426 temp-leak guard - -Branch `fix/test-temp-leaks-1426`, range `b9728f94..HEAD` (9 commits), backend only. All heavy commands ran with private `TMPDIR`/`GOTMPDIR` under `/var/tmp` (removed afterwards). - -**Verdict: PASS. No blocking, no should-fix findings.** - -### Gate results - -| # | Command | Outcome | +| Gate | Result | Notes | |---|---|---| -| 1 | `cd backend && go build ./... && go vet ./...`; `gofmt -l .` | build OK, vet OK, gofmt empty | -| 2a | `go test -count=1 -race ./internal/testutil/... ./internal/services/... ./internal/api/handlers/...` (private TMPDIR) | all ok (services 504.9s, handlers 349.2s, tmpguard, testutil, remotestorage); private TMPDIR EMPTY afterwards | -| 2b | `go test -count=1 -p 4 ./...` (second private TMPDIR) | exit 0, zero failures; private TMPDIR EMPTY afterwards | -| 3 | `go test -run Discard` + `-run 'Discard\|RestoreBackup\|RestoreDB'` in services | 8 new discard tests pass (success, pre-restore-backup failure, apply failure, pending-file written keeps pending copy, unrecoverable, sidecars, empty-field no-op, already-removed); legacy `FileExists(restoreDBPath)` tests (backup_service_test.go:86, :1370) pass | -| 4 | Scratch copy (`git archive HEAD` under /var/tmp) with a deliberate-leak TestMain package | package FAILS, report names `deliberate-leak.bin (1234 bytes)`, exit 1. Decoys: young prefix dir, prefix-named symlink (to a dir with a file), old prefix-named plain file, old non-prefix dir all SURVIVED (symlink target content intact); old prefix-named dir was swept | -| 5a | `bash scripts/local-patch-report.sh` | artifacts present (md + json); patch coverage 72/72 = 100% (backend), pass | -| 5b | `scripts/go-test-coverage.sh` | exit 0; statements 92.2%, line coverage 89.2% vs gate 87%: met | -| 5c | `lefthook run pre-commit --all-files` | exit 0 (all hooks incl. semgrep: 0 findings, golangci-lint-fast, go-vet, frontend lint/type-check) | -| 5d | `make lint-fast` / `make lint-backend` | 0 issues / 0 issues | -| 6 | CodeQL / Trivy | Deferred to CI per CLAUDE.md (no feat:, no new dependency, no network surface). `./scripts/scan-gorm-security.sh --check`: PASSED, 0 issues | -| 7 | `go test -count>1` on services/handlers | Not run; known pre-existing, GH #1450 | - -### Security / compliance audit - -- tmpguard sweeper (`backend/internal/testutil/tmpguard/tmpguard.go`): sweeps only entries in `os.TempDir()` whose name has the `charon-gotest-` prefix, `IsDir()` per lstat (symlinks and plain files skipped), and mtime older than 24h. Only entry names are joined to the root, so it cannot reach outside the base (verified empirically in gate 4). `os.RemoveAll` does not follow symlinks. Private root comes from `os.MkdirTemp` (0700, unique). -- Low / informational (tmpguard.go `removeTree`, ~lines 100-107): `os.Chmod` in the pre-removal walk follows symlinks and there is a theoretical check-to-use window between `ReadDir` and the walk. Exploitation needs a hostile local user racing in a shared temp dir, and the chmod would only apply to files the running user owns. Not exploitable in practice; no change requested. If hardened later, use `os.Lstat` before chmod. -- Production `discardRestoreSnapshot` (`backend/internal/services/backup_restore_safe.go:242-255`): removes only `s.restoreDBPath` and its `-wal`/`-shm`. `restoreDBPath` is only ever assigned from `os.CreateTemp` outputs (backup_restore_safe.go:165, backup_service.go:1413-1419), never from user input, and is distinct from the live DB and from the pending-restore file (the pending file is a copy written by `writePendingRestoreFile`; a test asserts the pending copy is kept). The mutex requirement is documented and respected. Missing files are tolerated. -- CONTRIBUTING.md (~lines 407-413): accurate against the code (TestMain wiring in services and handlers, `charon-gotest-` prefix, 24h sweep, `t.TempDir()` advice, `TMPDIR`/`GOTMPDIR` disk-backed example). -- No secrets or tokens in logs or the diff; guard output lists only file names and sizes. -- Commit hygiene: no session IDs, claude.ai links, Co-Authored-By or "generated with" lines in `git log b9728f94..HEAD --format=%B` or in the diff. Subjects use `test:`, `fix:`, `docs:`; no `(security)` scope used (appropriate: the leak fix is not a vulnerability fix). - -### Findings - -None blocking. One Low informational item (tmpguard `removeTree` chmod follows symlinks, see above). - -### Housekeeping - -No test artifacts modified, nothing committed or pushed; `/var/tmp/charon-qa-pr1-*` scratch removed; `/tmp` untouched; `charon` container untouched. +| `go build ./...` | PASS | | +| Local patch coverage preflight | PASS | 18/18 changed lines covered (100%); artifacts in `test-results/` | +| Backend coverage (`scripts/go-test-coverage.sh`) | PASS | 92.4% statements, 89.5% lines; gate met | +| `lefthook run pre-commit --all-files` | PASS | All hooks green, Semgrep 0 findings | +| `make lint-fast` | PASS | 0 issues (backend and agent) | +| Race tests: `services`, `crowdsec` | PASS | `-race -count=1`, all packages ok | +| `go test ./internal/api/...` | PASS | all packages ok | +| GORM scan | N/A | No models, queries or migrations touched | +| Playwright / frontend gates | N/A | Backend and docs only | +| CodeQL / Trivy | Deferred to CI | Fix-scoped change | +| Docs | PASS | Troubleshooting note is accurate; `docs-site/docs/` untouched | + +## Verdict + +PASS. No blocking issues.