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/crowdsec/hub_sync.go b/backend/internal/crowdsec/hub_sync.go index f344d6f3e..8ab2fbbb2 100644 --- a/backend/internal/crowdsec/hub_sync.go +++ b/backend/internal/crowdsec/hub_sync.go @@ -84,11 +84,18 @@ 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 -// 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 { @@ -175,18 +182,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") +} diff --git a/backend/internal/crowdsec/registration.go b/backend/internal/crowdsec/registration.go index f7a89a574..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 validated to be localhost only - ) - 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 validated to be localhost only - ) - 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 validated to be localhost only - ) - 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.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 b9052ec82..baf91b670 100644 --- a/backend/internal/services/update_service_test.go +++ b/backend/internal/services/update_service_test.go @@ -31,6 +31,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. @@ -161,6 +162,22 @@ 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() + if assert.Error(t, err) { + assert.ErrorIs(t, err, network.ErrBlockedAddress) + } +} + func TestUpdateService_CheckForUpdates_RejectsReservedDestinations(t *testing.T) { for _, target := range []string{"http://100.64.0.1:9/", "http://198.18.0.1:9/", "http://[2002::1]:9/"} { svc := NewUpdateService() diff --git a/backend/internal/services/uptime_check.go b/backend/internal/services/uptime_check.go index 058033790..1babccb8b 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.WithAllowCGNAT(), @@ -104,8 +107,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), diff --git a/docs/reports/qa_report.md b/docs/reports/qa_report.md index e3861c353..b6be7b71a 100644 --- a/docs/reports/qa_report.md +++ b/docs/reports/qa_report.md @@ -1,29 +1,27 @@ -# QA Report: Hardening Outbound Client Configuration in Notification Senders +# QA Report -Branch: `fix/notification-sender-client-hardening` (2 commits on top of `origin/development`) -Date: 2026-10-05 -Scope: backend services + docs (`notification_sender_client.go`, security notification services, tests, `docs/features/notifications.md`). +Branch: `fix/localhost-allowance-followups` (4 commits on `development`) -## Gate Results +## Scope + +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`). + +## Results | Gate | Result | Notes | |---|---|---| -| Playwright E2E | N/A | Backend + docs only; no UI/API-contract change | -| GORM security scan | N/A | No models, queries, or migrations touched | -| Local patch coverage preflight | PASS | Patch coverage 100% (2/2 changed lines); artifacts `test-results/local-patch-report.md/.json` present | -| Security scans (CodeQL/Trivy) | DEFERRED | Fix-scoped change; deferred to CI per CLAUDE.md | -| Lefthook pre-commit (`--all-files`) | PASS | All 16 hooks passed, incl. semgrep (0 findings), staticcheck/golangci-lint-fast, go-vet, frontend lint/type-check | -| `make lint-fast` | PASS | 0 issues (backend + agent) | -| Backend coverage (`scripts/go-test-coverage.sh`) | PASS | 92.3% statements / 89.5% line coverage; gate 87% | -| Frontend type-check / coverage | N/A | No frontend changes (type-check ran via lefthook: pass) | | `go build ./...` | PASS | | -| `go test -race ./internal/services/... -count=1` | PASS | No regressions after rebase | -| `go test ./internal/api/...` | PASS | All packages OK | - -## Findings - -No blocking, high, medium, or low findings. Working tree clean after all gates. +| 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. +PASS. No blocking issues. 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.