From f5db7ad0e4c13c312520c8f7ed9e27c07694dd78 Mon Sep 17 00:00:00 2001 From: GitHub Actions Date: Sun, 22 Feb 2026 17:12:17 +0000 Subject: [PATCH] chore: Enhance backend test coverage and add new functional tests for Security page - Added tests to `proxyhost_service_validation_test.go` to validate fallback parsing and handle invalid hostname characters. - Introduced new tests for DNS challenge validation in `proxyhost_service_validation_test.go`. - Updated `current_spec.md` to reflect changes in testing strategy and coverage goals for PR #729. - Enhanced `Security.functional.test.tsx` to include navigation test for Notifications button. - Mocked `useNavigate` from `react-router-dom` to verify navigation behavior in Security page tests. --- .github/agents/Backend_Dev.agent.md | 6 +- .github/agents/Frontend_Dev.agent.md | 3 +- .../handlers/feature_flags_blocker3_test.go | 28 + .../handlers/feature_flags_handler_test.go | 29 + .../notification_provider_blocker3_test.go | 50 + .../security_handler_additional_test.go | 69 -- .../handlers/security_notifications_test.go | 112 ++ .../api/routes/routes_coverage_test.go | 69 +- .../cerberus/cerberus_blockers_test.go | 25 +- backend/internal/notifications/router_test.go | 22 + ...otification_service_patch_coverage_test.go | 332 ++++++ ...nced_security_notification_service_test.go | 975 ++++++++++++++++++ .../proxyhost_service_validation_test.go | 113 ++ docs/plans/current_spec.md | 478 +++++---- .../__tests__/Security.functional.test.tsx | 26 + 15 files changed, 2010 insertions(+), 327 deletions(-) delete mode 100644 backend/internal/api/handlers/security_handler_additional_test.go create mode 100644 backend/internal/api/handlers/security_notifications_test.go create mode 100644 backend/internal/services/enhanced_security_notification_service_patch_coverage_test.go create mode 100644 backend/internal/services/enhanced_security_notification_service_test.go diff --git a/.github/agents/Backend_Dev.agent.md b/.github/agents/Backend_Dev.agent.md index 21694661..0f94d44f 100644 --- a/.github/agents/Backend_Dev.agent.md +++ b/.github/agents/Backend_Dev.agent.md @@ -4,7 +4,7 @@ description: 'Senior Go Engineer focused on high-performance, secure backend imp argument-hint: 'The specific backend task from the Plan (e.g., "Implement ProxyHost CRUD endpoints")' tools: vscode/extensions, vscode/getProjectSetupInfo, vscode/installExtension, vscode/memory, vscode/openIntegratedBrowser, vscode/runCommand, vscode/askQuestions, vscode/vscodeAPI, execute, read, agent, 'github/*', 'github/*', 'io.github.goreleaser/mcp/*', edit, search, web, 'github/*', 'playwright/*', todo, vscode.mermaid-chat-features/renderMermaidDiagram, github.vscode-pull-request-github/issue_fetch, github.vscode-pull-request-github/labels_fetch, github.vscode-pull-request-github/notification_fetch, github.vscode-pull-request-github/doSearch, github.vscode-pull-request-github/activePullRequest, github.vscode-pull-request-github/openPullRequest, ms-azuretools.vscode-containers/containerToolsConfig, ms-python.python/getPythonEnvironmentInfo, ms-python.python/getPythonExecutableCommand, ms-python.python/installPythonPackage, ms-python.python/configurePythonEnvironment, '' -model: Claude Sonnet 4.5 (copilot) +model: GPT-5.3-Codex (copilot) target: vscode user-invocable: true disable-model-invocation: false @@ -44,7 +44,9 @@ Your priority is writing code that is clean, tested, and secure by default. - Define the structs in `internal/models` to fix compilation errors. - **Step 3 (The Logic)**: - Implement the handler in `internal/api/handlers`. - - **Step 4 (The Green Light)**: + - **Step 4 (Lint and Format)**: + - Run `pre-commit run --all-files` to ensure code quality. + - **Step 5 (The Green Light)**: - Run `go test ./...`. - **CRITICAL**: If it fails, fix the *Code*, NOT the *Test* (unless the test was wrong about the contract). diff --git a/.github/agents/Frontend_Dev.agent.md b/.github/agents/Frontend_Dev.agent.md index d028c157..61153063 100644 --- a/.github/agents/Frontend_Dev.agent.md +++ b/.github/agents/Frontend_Dev.agent.md @@ -48,8 +48,7 @@ You are a SENIOR REACT/TYPESCRIPT ENGINEER with deep expertise in: - Run tests with `npm test` in `frontend/` directory 4. **Quality Checks**: - - Run `npm run lint` to check for linting issues - - Run `npm run typecheck` for TypeScript errors + - Run `pre-commit run --all-files` to ensure linting and formatting - Ensure accessibility with proper ARIA attributes diff --git a/backend/internal/api/handlers/feature_flags_blocker3_test.go b/backend/internal/api/handlers/feature_flags_blocker3_test.go index c8243ee8..a3863f0b 100644 --- a/backend/internal/api/handlers/feature_flags_blocker3_test.go +++ b/backend/internal/api/handlers/feature_flags_blocker3_test.go @@ -275,3 +275,31 @@ func TestLegacyFallbackRemoved_GetFlagsReturnsHardFalse(t *testing.T) { db.Unscoped().Delete(&setting) }) } + +// TestLegacyFallbackRemoved_InvalidEnvValue tests that invalid environment variable values are handled (lines 157-158) +func TestLegacyFallbackRemoved_InvalidEnvValue(t *testing.T) { + gin.SetMode(gin.TestMode) + + db, err := gorm.Open(sqlite.Open(":memory:"), &gorm.Config{}) + assert.NoError(t, err) + assert.NoError(t, db.AutoMigrate(&models.Setting{})) + + // Set invalid environment variable value + t.Setenv("CHARON_NOTIFICATIONS_LEGACY_FALLBACK", "invalid-value") + + handler := NewFeatureFlagsHandler(db) + + w := httptest.NewRecorder() + c, _ := gin.CreateTestContext(w) + c.Request, _ = http.NewRequest("GET", "/api/v1/feature-flags", nil) + + // Lines 157-158: Should log warning for invalid env value and return hard-false + handler.GetFlags(c) + + assert.Equal(t, http.StatusOK, w.Code) + + var response map[string]bool + err = json.Unmarshal(w.Body.Bytes(), &response) + assert.NoError(t, err) + assert.False(t, response["feature.notifications.legacy.fallback_enabled"], "Must return hard-false even with invalid env value") +} diff --git a/backend/internal/api/handlers/feature_flags_handler_test.go b/backend/internal/api/handlers/feature_flags_handler_test.go index 2411f4af..771921ff 100644 --- a/backend/internal/api/handlers/feature_flags_handler_test.go +++ b/backend/internal/api/handlers/feature_flags_handler_test.go @@ -428,3 +428,32 @@ func TestUpdateFlags_TransactionAtomic(t *testing.T) { t.Errorf("expected crowdsec.console_enrollment to be true, got %s", s3.Value) } } + +// TestFeatureFlags_InvalidRetiredEnvAlias covers lines 157-158 (invalid env var warning) +func TestFeatureFlags_InvalidRetiredEnvAlias(t *testing.T) { + db := setupFlagsDB(t) + t.Setenv("NOTIFICATIONS_LEGACY_FALLBACK_ENABLED", "invalid-value") + + h := NewFeatureFlagsHandler(db) + gin.SetMode(gin.TestMode) + r := gin.New() + r.GET("/api/v1/feature-flags", h.GetFlags) + + req := httptest.NewRequest(http.MethodGet, "/api/v1/feature-flags", http.NoBody) + w := httptest.NewRecorder() + r.ServeHTTP(w, req) + + if w.Code != http.StatusOK { + t.Fatalf("expected 200 got %d body=%s", w.Code, w.Body.String()) + } + + var flags map[string]bool + if err := json.Unmarshal(w.Body.Bytes(), &flags); err != nil { + t.Fatalf("invalid json: %v", err) + } + + // Should force disabled due to invalid value (lines 157-158) + if flags["feature.notifications.legacy.fallback_enabled"] { + t.Fatalf("expected retired fallback flag to be false for invalid env value") + } +} diff --git a/backend/internal/api/handlers/notification_provider_blocker3_test.go b/backend/internal/api/handlers/notification_provider_blocker3_test.go index 15fec6f1..9b5e8089 100644 --- a/backend/internal/api/handlers/notification_provider_blocker3_test.go +++ b/backend/internal/api/handlers/notification_provider_blocker3_test.go @@ -359,3 +359,53 @@ func TestBlocker3_MultipleSecurityEventsEnforcesDiscordOnly(t *testing.T) { }) } } + +// TestBlocker3_UpdateProvider_DatabaseError tests database error handling when fetching existing provider (lines 137-139). +func TestBlocker3_UpdateProvider_DatabaseError(t *testing.T) { + gin.SetMode(gin.TestMode) + + // Setup test database + db, err := gorm.Open(sqlite.Open(":memory:"), &gorm.Config{}) + assert.NoError(t, err) + + // Run migrations + err = db.AutoMigrate(&models.NotificationProvider{}, &models.Notification{}) + assert.NoError(t, err) + + // Create handler + service := services.NewNotificationService(db) + handler := NewNotificationProviderHandler(service) + + // Update payload + payload := map[string]interface{}{ + "name": "Test Provider", + "type": "discord", + "url": "https://discord.com/api/webhooks/123/abc", + "enabled": true, + } + + jsonPayload, err := json.Marshal(payload) + assert.NoError(t, err) + + // Create test context with non-existent provider ID + w := httptest.NewRecorder() + c, _ := gin.CreateTestContext(w) + c.Request, _ = http.NewRequest("PUT", "/api/v1/notifications/providers/nonexistent", bytes.NewBuffer(jsonPayload)) + c.Request.Header.Set("Content-Type", "application/json") + c.Params = []gin.Param{{Key: "id", Value: "nonexistent"}} + + // Set admin role + c.Set("role", "admin") + c.Set("userID", uint(1)) + + // Call Update + handler.Update(c) + + // Lines 137-139: Should return 404 for not found + assert.Equal(t, http.StatusNotFound, w.Code, "Should return 404 for nonexistent provider") + + var response map[string]interface{} + err = json.Unmarshal(w.Body.Bytes(), &response) + assert.NoError(t, err) + assert.Equal(t, "provider not found", response["error"]) +} diff --git a/backend/internal/api/handlers/security_handler_additional_test.go b/backend/internal/api/handlers/security_handler_additional_test.go deleted file mode 100644 index 4a37183d..00000000 --- a/backend/internal/api/handlers/security_handler_additional_test.go +++ /dev/null @@ -1,69 +0,0 @@ -package handlers - -import ( - "encoding/json" - "net/http" - "net/http/httptest" - "strings" - "testing" - - "github.com/gin-gonic/gin" - "github.com/stretchr/testify/require" - "gorm.io/driver/sqlite" - "gorm.io/gorm" - - "github.com/Wikid82/charon/backend/internal/config" - "github.com/Wikid82/charon/backend/internal/models" -) - -func TestSecurityHandler_GetConfigAndUpdateConfig(t *testing.T) { - t.Helper() - // Setup DB and router - db, err := gorm.Open(sqlite.Open("file::memory:?mode=memory&cache=shared"), &gorm.Config{}) - require.NoError(t, err) - require.NoError(t, db.AutoMigrate(&models.SecurityConfig{})) - - cfg := config.SecurityConfig{} - h := NewSecurityHandler(cfg, db, nil) - - // Create a gin test context for GetConfig when no config exists - w := httptest.NewRecorder() - c, _ := gin.CreateTestContext(w) - req := httptest.NewRequest("GET", "/security/config", http.NoBody) - c.Request = req - h.GetConfig(c) - require.Equal(t, http.StatusOK, w.Code) - var body map[string]any - require.NoError(t, json.Unmarshal(w.Body.Bytes(), &body)) - // Should return config: null - if _, ok := body["config"]; !ok { - t.Fatalf("expected 'config' in response, got %v", body) - } - - // Now update config - w = httptest.NewRecorder() - c, _ = gin.CreateTestContext(w) - payload := `{"name":"default","admin_whitelist":"127.0.0.1/32"}` - req = httptest.NewRequest("POST", "/security/config", strings.NewReader(payload)) - req.Header.Set("Content-Type", "application/json") - c.Request = req - h.UpdateConfig(c) - require.Equal(t, http.StatusOK, w.Code) - - // Now call GetConfig again and ensure config is returned - w = httptest.NewRecorder() - c, _ = gin.CreateTestContext(w) - req = httptest.NewRequest("GET", "/security/config", http.NoBody) - c.Request = req - h.GetConfig(c) - require.Equal(t, http.StatusOK, w.Code) - var body2 map[string]any - require.NoError(t, json.Unmarshal(w.Body.Bytes(), &body2)) - cfgVal, ok := body2["config"].(map[string]any) - if !ok { - t.Fatalf("expected config object, got %v", body2["config"]) - } - if cfgVal["admin_whitelist"] != "127.0.0.1/32" { - t.Fatalf("unexpected admin_whitelist: %v", cfgVal["admin_whitelist"]) - } -} diff --git a/backend/internal/api/handlers/security_notifications_test.go b/backend/internal/api/handlers/security_notifications_test.go new file mode 100644 index 00000000..f6c375a7 --- /dev/null +++ b/backend/internal/api/handlers/security_notifications_test.go @@ -0,0 +1,112 @@ +package handlers + +import ( + "bytes" + "context" + "encoding/json" + "errors" + "net/http" + "net/http/httptest" + "testing" + "time" + + "github.com/gin-gonic/gin" + "github.com/stretchr/testify/assert" + "gorm.io/driver/sqlite" + "gorm.io/gorm" + + "github.com/Wikid82/charon/backend/internal/models" + "github.com/Wikid82/charon/backend/internal/services" +) + +// TestHandleSecurityEvent_TimestampZero covers line 146 +func TestHandleSecurityEvent_TimestampZero(t *testing.T) { + gin.SetMode(gin.TestMode) + + db, err := gorm.Open(sqlite.Open(":memory:"), &gorm.Config{}) + assert.NoError(t, err) + + err = db.AutoMigrate(&models.Setting{}, &models.NotificationProvider{}) + assert.NoError(t, err) + + // Enable feature flag + setting := models.Setting{ + Key: "feature.notifications.security_provider_events.enabled", + Value: "true", + Type: "bool", + Category: "feature", + } + assert.NoError(t, db.Create(&setting).Error) + + enhancedService := services.NewEnhancedSecurityNotificationService(db) + securityService := services.NewSecurityService(db) + notificationService := services.NewNotificationService(db) + h := NewSecurityNotificationHandlerWithDeps(enhancedService, securityService, "/tmp", notificationService, []string{"127.0.0.0/8"}) + + w := httptest.NewRecorder() + ctx, _ := gin.CreateTestContext(w) + + // Event with zero timestamp + event := models.SecurityEvent{ + EventType: "waf_block", + Severity: "warn", + Message: "Test", + ClientIP: "127.0.0.1", + Path: "/test", + // Timestamp not set - should trigger line 146 + } + + body, _ := json.Marshal(event) + ctx.Request, _ = http.NewRequest("POST", "/api/v1/security/events", bytes.NewReader(body)) + ctx.Request.RemoteAddr = "127.0.0.1:12345" + + h.HandleSecurityEvent(ctx) + + assert.Equal(t, http.StatusAccepted, w.Code) +} + +// mockFailingService is a mock service that always fails SendViaProviders +type mockFailingService struct { + services.EnhancedSecurityNotificationService +} + +func (m *mockFailingService) SendViaProviders(ctx context.Context, event models.SecurityEvent) error { + return errors.New("mock provider failure") +} + +// TestHandleSecurityEvent_SendViaProvidersError covers lines 163-164 +func TestHandleSecurityEvent_SendViaProvidersError(t *testing.T) { + gin.SetMode(gin.TestMode) + + db, err := gorm.Open(sqlite.Open(":memory:"), &gorm.Config{}) + assert.NoError(t, err) + + err = db.AutoMigrate(&models.Setting{}) + assert.NoError(t, err) + + securityService := services.NewSecurityService(db) + notificationService := services.NewNotificationService(db) + mockService := &mockFailingService{} + h := NewSecurityNotificationHandlerWithDeps(mockService, securityService, "/tmp", notificationService, []string{"127.0.0.0/8"}) + + w := httptest.NewRecorder() + ctx, _ := gin.CreateTestContext(w) + + event := models.SecurityEvent{ + EventType: "acl_deny", + Severity: "warn", + Message: "Test", + ClientIP: "127.0.0.1", + Path: "/test", + Timestamp: time.Now(), + } + + body, _ := json.Marshal(event) + ctx.Request, _ = http.NewRequest("POST", "/api/v1/security/events", bytes.NewReader(body)) + ctx.Request.RemoteAddr = "127.0.0.1:12345" + + // Should continue and return Accepted even when SendViaProviders fails (lines 163-164) + h.HandleSecurityEvent(ctx) + + assert.Equal(t, http.StatusAccepted, w.Code) +} diff --git a/backend/internal/api/routes/routes_coverage_test.go b/backend/internal/api/routes/routes_coverage_test.go index 212d4635..e5e11d82 100644 --- a/backend/internal/api/routes/routes_coverage_test.go +++ b/backend/internal/api/routes/routes_coverage_test.go @@ -1,6 +1,7 @@ package routes import ( + "errors" "testing" "github.com/Wikid82/charon/backend/internal/config" @@ -11,38 +12,64 @@ import ( "gorm.io/gorm/logger" ) -// TestRegister_MigrationErrors covers lines 181-182, 193-195 -func TestRegister_MigrationErrors(t *testing.T) { +func TestRegister_NotifyOnlyProviderMigrationErrorReturns(t *testing.T) { gin.SetMode(gin.TestMode) router := gin.New() - // Create DB with failing notification service migrations by forcing constraint violations db, err := gorm.Open(sqlite.Open("file::memory:?cache=shared&_test_migration_errors"), &gorm.Config{ Logger: logger.Default.LogMode(logger.Silent), }) require.NoError(t, err) - // Create schema with conflicting data that will cause migration errors - // This will exercise the error paths at lines 181-182 (EnsureNotifyOnlyProviderMigration) - // and lines 193-195 (MigrateFromLegacyConfig) - err = db.Exec("CREATE TABLE IF NOT EXISTS notification_providers (id TEXT PRIMARY KEY, name TEXT UNIQUE NOT NULL)").Error + const cbName = "routes:test_force_notify_only_migration_query_error" + err = db.Callback().Query().Before("gorm:query").Register(cbName, func(tx *gorm.DB) { + if tx.Statement != nil && tx.Statement.Table == "notification_providers" { + _ = tx.AddError(errors.New("forced notification_providers query failure")) + } + }) require.NoError(t, err) - - // Insert conflicting data - err = db.Exec("INSERT INTO notification_providers (id, name) VALUES ('test1', 'duplicate')").Error - require.NoError(t, err) - err = db.Exec("INSERT INTO notification_providers (id, name) VALUES ('test2', 'duplicate')").Error - if err == nil { - // If DB allows duplicates, force an error by creating invalid schema - _ = db.Exec("DROP TABLE notification_providers").Error - _ = db.Exec("CREATE TABLE notification_providers (id TEXT)").Error - } + t.Cleanup(func() { + _ = db.Callback().Query().Remove(cbName) + }) cfg := config.Config{JWTSecret: "test-secret"} - // Register should handle migration errors gracefully (lines 181-182, 193-195) err = Register(router, db, cfg) - // Migrations may fail but Register should continue and return error - // or succeed with logged warnings (non-fatal migration errors) - _ = err // Accept either success or error - we're testing execution paths + require.Error(t, err) + require.Contains(t, err.Error(), "notify-only provider migration") +} + +func TestRegister_LegacyMigrationErrorIsNonFatal(t *testing.T) { + gin.SetMode(gin.TestMode) + router := gin.New() + + db, err := gorm.Open(sqlite.Open("file::memory:?cache=shared&_test_legacy_migration_warn"), &gorm.Config{ + Logger: logger.Default.LogMode(logger.Silent), + }) + require.NoError(t, err) + + const cbName = "routes:test_force_legacy_migration_query_error" + err = db.Callback().Query().Before("gorm:query").Register(cbName, func(tx *gorm.DB) { + if tx.Statement != nil && tx.Statement.Table == "notification_configs" { + _ = tx.AddError(errors.New("forced notification_configs query failure")) + } + }) + require.NoError(t, err) + t.Cleanup(func() { + _ = db.Callback().Query().Remove(cbName) + }) + + cfg := config.Config{JWTSecret: "test-secret"} + + err = Register(router, db, cfg) + require.NoError(t, err) + + hasHealth := false + for _, r := range router.Routes() { + if r.Path == "/api/v1/health" { + hasHealth = true + break + } + } + require.True(t, hasHealth) } diff --git a/backend/internal/cerberus/cerberus_blockers_test.go b/backend/internal/cerberus/cerberus_blockers_test.go index d5df7f8b..d5f2e5e0 100644 --- a/backend/internal/cerberus/cerberus_blockers_test.go +++ b/backend/internal/cerberus/cerberus_blockers_test.go @@ -271,9 +271,16 @@ func TestNotifySecurityEvent_Disabled(t *testing.T) { db, err := gorm.Open(sqlite.Open(":memory:"), &gorm.Config{}) assert.NoError(t, err) - err = db.AutoMigrate(&models.Setting{}, &models.NotificationProvider{}) + err = db.AutoMigrate(&models.Setting{}, &models.NotificationProvider{}, &models.SecurityConfig{}) assert.NoError(t, err) + assert.NoError(t, db.Create(&models.Setting{ + Key: "feature.cerberus.enabled", + Value: "false", + Type: "bool", + Category: "feature", + }).Error) + // Create Cerberus instance with disabled security cfg := config.SecurityConfig{ CerberusEnabled: false, @@ -299,16 +306,12 @@ func TestNotifySecurityEvent_Disabled(t *testing.T) { assert.NoError(t, err) } -// TestSendSecurityNotification_FlagCheckError covers lines 315-316 -func TestSendSecurityNotification_FlagCheckError(t *testing.T) { - db, err := gorm.Open(sqlite.Open(":memory:"), &gorm.Config{}) - assert.NoError(t, err) - - // Don't run migrations - will cause DB error when checking flag +// TestSendSecurityNotification_NilDB covers lines 315-316 +func TestSendSecurityNotification_NilDB(t *testing.T) { cfg := config.SecurityConfig{ CerberusEnabled: true, } - cerberus := New(cfg, db) + cerberus := New(cfg, nil) event := models.SecurityEvent{ EventType: "acl_deny", @@ -319,9 +322,9 @@ func TestSendSecurityNotification_FlagCheckError(t *testing.T) { Timestamp: time.Now(), } - // Should handle DB error gracefully (lines 315-316) - err = cerberus.sendSecurityNotification(context.Background(), event) - assert.NoError(t, err) // Should not error, just skip notification + // Should return nil when db is nil (lines 315-316) + err := cerberus.sendSecurityNotification(context.Background(), event) + assert.NoError(t, err) } // TestBlocker2_ACLDenyNotificationInMiddleware tests ACL deny notification in actual middleware flow. diff --git a/backend/internal/notifications/router_test.go b/backend/internal/notifications/router_test.go index c09fed8f..e54b4581 100644 --- a/backend/internal/notifications/router_test.go +++ b/backend/internal/notifications/router_test.go @@ -68,3 +68,25 @@ func TestRouter_ShouldUseNotify_DiscordServiceFlag(t *testing.T) { t.Fatalf("expected notify routing disabled for discord when FlagDiscordServiceEnabled is false") } } + +// TestRouter_ShouldUseNotify_GotifyServiceFlag covers lines 23-24 (gotify case) +func TestRouter_ShouldUseNotify_GotifyServiceFlag(t *testing.T) { + router := NewRouter() + + // Test with gotify enabled + flags := map[string]bool{ + FlagNotifyEngineEnabled: true, + FlagGotifyServiceEnabled: true, + } + + if !router.ShouldUseNotify("gotify", EngineNotifyV1, flags) { + t.Fatalf("expected notify routing enabled for gotify when FlagGotifyServiceEnabled is true") + } + + // Test with gotify disabled + flags[FlagGotifyServiceEnabled] = false + + if router.ShouldUseNotify("gotify", EngineNotifyV1, flags) { + t.Fatalf("expected notify routing disabled for gotify when FlagGotifyServiceEnabled is false") + } +} diff --git a/backend/internal/services/enhanced_security_notification_service_patch_coverage_test.go b/backend/internal/services/enhanced_security_notification_service_patch_coverage_test.go new file mode 100644 index 00000000..f5ff81e4 --- /dev/null +++ b/backend/internal/services/enhanced_security_notification_service_patch_coverage_test.go @@ -0,0 +1,332 @@ +package services + +import ( + "context" + "net/http" + "net/http/httptest" + "testing" + "time" + + "github.com/Wikid82/charon/backend/internal/models" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "gorm.io/driver/sqlite" + "gorm.io/gorm" +) + +// TestEnhancedService_GetSettings_FeatureFlagError covers lines 60-61 +func TestEnhancedService_GetSettings_FeatureFlagError(t *testing.T) { + db, err := gorm.Open(sqlite.Open(":memory:"), &gorm.Config{}) + require.NoError(t, err) + // Don't run migrations - will cause DB error + + service := NewEnhancedSecurityNotificationService(db) + + // Lines 60-61: Should return error when feature flag check fails + config, err := service.GetSettings() + assert.Error(t, err) + assert.Nil(t, config) +} + +// TestEnhancedService_GetProviderAggregatedConfig_QueryError covers lines 81-82 +func TestEnhancedService_GetProviderAggregatedConfig_QueryError(t *testing.T) { + db, err := gorm.Open(sqlite.Open(":memory:"), &gorm.Config{}) + require.NoError(t, err) + require.NoError(t, db.AutoMigrate(&models.Setting{})) + // Don't migrate NotificationProvider - will cause query error + + // Enable feature flag + db.Create(&models.Setting{ + Key: "feature.notifications.security_provider_events.enabled", + Value: "true", + Type: "bool", + }) + + service := NewEnhancedSecurityNotificationService(db) + + // Lines 81-82: Should return error when provider query fails + config, err := service.GetSettings() + assert.Error(t, err) + assert.Nil(t, config) +} + +// TestEnhancedService_GetProviderAggregatedConfig_GotifyNoTokenExposure covers lines 118-119 +func TestEnhancedService_GetProviderAggregatedConfig_GotifyNoTokenExposure(t *testing.T) { + db, err := gorm.Open(sqlite.Open(":memory:"), &gorm.Config{}) + require.NoError(t, err) + require.NoError(t, db.AutoMigrate(&models.NotificationProvider{}, &models.Setting{})) + + db.Create(&models.Setting{ + Key: "feature.notifications.security_provider_events.enabled", + Value: "true", + Type: "bool", + }) + + // Create managed gotify provider + db.Create(&models.NotificationProvider{ + ID: "gotify", + Name: "Test Gotify", + Type: "gotify", + URL: "https://gotify.example.com", + Enabled: true, + ManagedLegacySecurity: true, + NotifySecurityWAFBlocks: true, + }) + + service := NewEnhancedSecurityNotificationService(db) + + config, err := service.GetSettings() + require.NoError(t, err) + + // Lines 118-119: Should set GotifyURL but NOT expose token + assert.Equal(t, "https://gotify.example.com", config.GotifyURL) + // Token field should remain empty/default in response + assert.Empty(t, config.GotifyToken, "Gotify token must not be exposed in GET response") +} + +// TestEnhancedService_UpdateSettings_FeatureFlagError covers lines 173-174 +func TestEnhancedService_UpdateSettings_FeatureFlagError(t *testing.T) { + db, err := gorm.Open(sqlite.Open(":memory:"), &gorm.Config{}) + require.NoError(t, err) + // Don't run migrations - will cause DB error + + service := NewEnhancedSecurityNotificationService(db) + + req := &models.NotificationConfig{ + NotifyWAFBlocks: true, + } + + // Lines 173-174: Should return error when feature flag check fails + err = service.UpdateSettings(req) + assert.Error(t, err) +} + +// TestEnhancedService_SendViaProviders_NonDiscordFiltered covers lines 327-329 +func TestEnhancedService_SendViaProviders_NonDiscordFiltered(t *testing.T) { + db, err := gorm.Open(sqlite.Open(":memory:"), &gorm.Config{}) + require.NoError(t, err) + require.NoError(t, db.AutoMigrate(&models.NotificationProvider{}, &models.Setting{})) + + db.Create(&models.Setting{ + Key: "feature.notifications.security_provider_events.enabled", + Value: "true", + Type: "bool", + }) + + // Create non-discord provider + db.Create(&models.NotificationProvider{ + ID: "webhook", + Name: "Webhook", + Type: "webhook", + URL: "https://example.com", + Enabled: true, + NotifySecurityWAFBlocks: true, + }) + + service := NewEnhancedSecurityNotificationService(db) + + event := models.SecurityEvent{ + EventType: "waf_block", + Severity: "warn", + Message: "Test", + ClientIP: "192.168.1.1", + Timestamp: time.Now(), + } + + // Lines 327-329: Should filter out non-discord providers + err = service.SendViaProviders(context.Background(), event) + assert.NoError(t, err) // No error, but webhook was filtered +} + +// TestEnhancedService_SendViaProviders_DisabledProvider covers lines 331-332 +func TestEnhancedService_SendViaProviders_DisabledProvider(t *testing.T) { + db, err := gorm.Open(sqlite.Open(":memory:"), &gorm.Config{}) + require.NoError(t, err) + require.NoError(t, db.AutoMigrate(&models.NotificationProvider{}, &models.Setting{})) + + db.Create(&models.Setting{ + Key: "feature.notifications.security_provider_events.enabled", + Value: "true", + Type: "bool", + }) + + // Create disabled discord provider + db.Create(&models.NotificationProvider{ + ID: "discord", + Name: "Discord", + Type: "discord", + URL: "https://discord.com/api/webhooks/123/abc", + Enabled: false, // Disabled + NotifySecurityWAFBlocks: true, + }) + + service := NewEnhancedSecurityNotificationService(db) + + event := models.SecurityEvent{ + EventType: "waf_block", + Severity: "warn", + Message: "Test", + ClientIP: "192.168.1.1", + Timestamp: time.Now(), + } + + // Lines 331-332: Should skip disabled providers + err = service.SendViaProviders(context.Background(), event) + assert.NoError(t, err) +} + +// TestEnhancedService_SendViaProviders_EventTypeNotSubscribed covers lines 341-342 +func TestEnhancedService_SendViaProviders_EventTypeNotSubscribed(t *testing.T) { + db, err := gorm.Open(sqlite.Open(":memory:"), &gorm.Config{}) + require.NoError(t, err) + require.NoError(t, db.AutoMigrate(&models.NotificationProvider{}, &models.Setting{})) + + db.Create(&models.Setting{ + Key: "feature.notifications.security_provider_events.enabled", + Value: "true", + Type: "bool", + }) + + // Create discord provider subscribed to WAF but not ACL + db.Create(&models.NotificationProvider{ + ID: "discord", + Name: "Discord", + Type: "discord", + URL: "https://discord.com/api/webhooks/123/abc", + Enabled: true, + NotifySecurityWAFBlocks: true, + NotifySecurityACLDenies: false, // Not subscribed to ACL + }) + + service := NewEnhancedSecurityNotificationService(db) + + event := models.SecurityEvent{ + EventType: "acl_deny", // Event type not subscribed + Severity: "warn", + Message: "Test", + ClientIP: "192.168.1.1", + Timestamp: time.Now(), + } + + // Lines 341-342: Should skip when event type not subscribed + err = service.SendViaProviders(context.Background(), event) + assert.NoError(t, err) +} + +// TestEnhancedService_SendViaProviders_UnknownEventType covers lines 352-353 +func TestEnhancedService_SendViaProviders_UnknownEventType(t *testing.T) { + db, err := gorm.Open(sqlite.Open(":memory:"), &gorm.Config{}) + require.NoError(t, err) + require.NoError(t, db.AutoMigrate(&models.NotificationProvider{}, &models.Setting{})) + + db.Create(&models.Setting{ + Key: "feature.notifications.security_provider_events.enabled", + Value: "true", + Type: "bool", + }) + + db.Create(&models.NotificationProvider{ + ID: "discord", + Name: "Discord", + Type: "discord", + URL: "https://discord.com/api/webhooks/123/abc", + Enabled: true, + NotifySecurityWAFBlocks: true, + }) + + service := NewEnhancedSecurityNotificationService(db) + + event := models.SecurityEvent{ + EventType: "unknown_event_type", // Unknown event type + Severity: "warn", + Message: "Test", + ClientIP: "192.168.1.1", + Timestamp: time.Now(), + } + + // Lines 352-353: Should skip unknown event types (default false) + err = service.SendViaProviders(context.Background(), event) + assert.NoError(t, err) +} + +// TestEnhancedService_SendViaProviders_HTTPRequestError covers lines 422-423 +func TestEnhancedService_SendViaProviders_HTTPRequestError(t *testing.T) { + db, err := gorm.Open(sqlite.Open(":memory:"), &gorm.Config{}) + require.NoError(t, err) + require.NoError(t, db.AutoMigrate(&models.NotificationProvider{}, &models.Setting{})) + + db.Create(&models.Setting{ + Key: "feature.notifications.security_provider_events.enabled", + Value: "true", + Type: "bool", + }) + + // Create discord provider with invalid URL + db.Create(&models.NotificationProvider{ + ID: "discord", + Name: "Discord", + Type: "discord", + URL: "https://invalid-url-that-will-fail-dns.local", + Enabled: true, + NotifySecurityWAFBlocks: true, + }) + + service := NewEnhancedSecurityNotificationService(db) + + event := models.SecurityEvent{ + EventType: "waf_block", + Severity: "warn", + Message: "Test", + ClientIP: "192.168.1.1", + Timestamp: time.Now(), + } + + // Lines 422-423: Should handle HTTP request errors but not fail + err = service.SendViaProviders(context.Background(), event) + // Service logs error but doesn't fail - continues to next provider + assert.NoError(t, err) +} + +// TestEnhancedService_SendViaProviders_Non2xxResponse covers lines 436-437 +func TestEnhancedService_SendViaProviders_Non2xxResponse(t *testing.T) { + // Create mock server that returns 500 + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusInternalServerError) + _, _ = w.Write([]byte("Internal Server Error")) + })) + defer server.Close() + + db, err := gorm.Open(sqlite.Open(":memory:"), &gorm.Config{}) + require.NoError(t, err) + require.NoError(t, db.AutoMigrate(&models.NotificationProvider{}, &models.Setting{})) + + db.Create(&models.Setting{ + Key: "feature.notifications.security_provider_events.enabled", + Value: "true", + Type: "bool", + }) + + db.Create(&models.NotificationProvider{ + ID: "discord", + Name: "Discord", + Type: "discord", + URL: server.URL, + Enabled: true, + NotifySecurityWAFBlocks: true, + }) + + service := NewEnhancedSecurityNotificationService(db) + + event := models.SecurityEvent{ + EventType: "waf_block", + Severity: "warn", + Message: "Test", + ClientIP: "192.168.1.1", + Timestamp: time.Now(), + } + + // Lines 436-437: Should handle non-2xx response but not fail + err = service.SendViaProviders(context.Background(), event) + // Service logs error but doesn't fail - continues to next provider + assert.NoError(t, err) +} diff --git a/backend/internal/services/enhanced_security_notification_service_test.go b/backend/internal/services/enhanced_security_notification_service_test.go new file mode 100644 index 00000000..76f2de1a --- /dev/null +++ b/backend/internal/services/enhanced_security_notification_service_test.go @@ -0,0 +1,975 @@ +package services + +import ( + "context" + "encoding/json" + "net/http" + "net/http/httptest" + "os" + "testing" + "time" + + "github.com/Wikid82/charon/backend/internal/models" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "gorm.io/gorm" +) + +func setupEnhancedServiceDB(t *testing.T) *gorm.DB { + db := setupTestDB(t) + require.NoError(t, db.AutoMigrate(&models.NotificationProvider{}, &models.NotificationConfig{}, &models.Setting{})) + return db +} + +func TestNewEnhancedSecurityNotificationService(t *testing.T) { + db := setupEnhancedServiceDB(t) + service := NewEnhancedSecurityNotificationService(db) + assert.NotNil(t, service) + assert.Equal(t, db, service.db) +} + +func TestGetSettings_FeatureFlagDisabled_ReturnsLegacyConfig(t *testing.T) { + db := setupEnhancedServiceDB(t) + service := NewEnhancedSecurityNotificationService(db) + + // Set feature flag to false + require.NoError(t, db.Create(&models.Setting{ + Key: "feature.notifications.security_provider_events.enabled", + Value: "false", + Type: "bool", + }).Error) + + // Create legacy config + legacyConfig := &models.NotificationConfig{ + NotifyWAFBlocks: true, + NotifyACLDenies: false, + NotifyRateLimitHits: true, + WebhookURL: "https://example.com/webhook", + } + require.NoError(t, db.Create(legacyConfig).Error) + + // Test + config, err := service.GetSettings() + require.NoError(t, err) + assert.True(t, config.NotifyWAFBlocks) + assert.False(t, config.NotifyACLDenies) + assert.True(t, config.NotifyRateLimitHits) + assert.Equal(t, "https://example.com/webhook", config.WebhookURL) +} + +func TestGetSettings_FeatureFlagEnabled_ReturnsAggregatedConfig(t *testing.T) { + db := setupEnhancedServiceDB(t) + service := NewEnhancedSecurityNotificationService(db) + + // Set feature flag to true + require.NoError(t, db.Create(&models.Setting{ + Key: "feature.notifications.security_provider_events.enabled", + Value: "true", + Type: "bool", + }).Error) + + // Create providers with different event types enabled + providers := []models.NotificationProvider{ + { + ID: "p1", + Type: "discord", + Enabled: true, + NotifySecurityWAFBlocks: true, + NotifySecurityACLDenies: false, + NotifySecurityRateLimitHits: false, + URL: "https://discord.com/webhook/1", + }, + { + ID: "p2", + Type: "discord", + Enabled: true, + NotifySecurityWAFBlocks: false, + NotifySecurityACLDenies: true, + NotifySecurityRateLimitHits: true, + URL: "https://discord.com/webhook/2", + }, + } + + for _, p := range providers { + require.NoError(t, db.Create(&p).Error) + } + + // Test + config, err := service.GetSettings() + require.NoError(t, err) + // OR aggregation: at least one provider has each flag true + assert.True(t, config.NotifyWAFBlocks) + assert.True(t, config.NotifyACLDenies) + assert.True(t, config.NotifyRateLimitHits) +} + +func TestGetProviderAggregatedConfig_ORSemantics(t *testing.T) { + db := setupEnhancedServiceDB(t) + service := NewEnhancedSecurityNotificationService(db) + + // Create providers where different providers have different flags + providers := []models.NotificationProvider{ + {ID: "p1", Type: "discord", Enabled: true, NotifySecurityWAFBlocks: true, NotifySecurityACLDenies: false, NotifySecurityRateLimitHits: false}, + {ID: "p2", Type: "discord", Enabled: true, NotifySecurityWAFBlocks: false, NotifySecurityACLDenies: true, NotifySecurityRateLimitHits: false}, + {ID: "p3", Type: "discord", Enabled: true, NotifySecurityWAFBlocks: false, NotifySecurityACLDenies: false, NotifySecurityRateLimitHits: true}, + } + + for _, p := range providers { + require.NoError(t, db.Create(&p).Error) + } + + // Test + config, err := service.getProviderAggregatedConfig() + require.NoError(t, err) + assert.True(t, config.NotifyWAFBlocks, "OR: p1 has WAF=true") + assert.True(t, config.NotifyACLDenies, "OR: p2 has ACL=true") + assert.True(t, config.NotifyRateLimitHits, "OR: p3 has RateLimit=true") +} + +func TestGetProviderAggregatedConfig_FiltersSupportedTypes(t *testing.T) { + db := setupEnhancedServiceDB(t) + service := NewEnhancedSecurityNotificationService(db) + + // Create providers with both supported and unsupported types + providers := []models.NotificationProvider{ + {ID: "discord", Type: "discord", Enabled: true, NotifySecurityWAFBlocks: true}, + {ID: "webhook", Type: "webhook", Enabled: true, NotifySecurityWAFBlocks: true}, + {ID: "slack", Type: "slack", Enabled: true, NotifySecurityACLDenies: true}, + {ID: "gotify", Type: "gotify", Enabled: true, NotifySecurityRateLimitHits: true}, + {ID: "unsupported", Type: "telegram", Enabled: true, NotifySecurityWAFBlocks: true}, // Should be filtered + } + + for _, p := range providers { + require.NoError(t, db.Create(&p).Error) + } + + // Test + config, err := service.getProviderAggregatedConfig() + require.NoError(t, err) + // Telegram is unsupported, so it shouldn't contribute to aggregation + assert.True(t, config.NotifyWAFBlocks, "Discord and webhook have WAF=true") + assert.True(t, config.NotifyACLDenies, "Slack has ACL=true") + assert.True(t, config.NotifyRateLimitHits, "Gotify has RateLimit=true") +} + +func TestGetProviderAggregatedConfig_DestinationReporting_SingleManaged(t *testing.T) { + db := setupEnhancedServiceDB(t) + service := NewEnhancedSecurityNotificationService(db) + + testCases := []struct { + name string + providerType string + url string + expectedField string + }{ + { + name: "webhook", + providerType: "webhook", + url: "https://example.com/webhook", + expectedField: "WebhookURL", + }, + { + name: "discord", + providerType: "discord", + url: "https://discord.com/webhook/123", + expectedField: "DiscordWebhookURL", + }, + { + name: "slack", + providerType: "slack", + url: "https://hooks.slack.com/services/T/B/X", + expectedField: "SlackWebhookURL", + }, + { + name: "gotify", + providerType: "gotify", + url: "https://gotify.example.com", + expectedField: "GotifyURL", + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + // Clean up + db.Exec("DELETE FROM notification_providers") + + // Create single managed provider + provider := models.NotificationProvider{ + ID: "managed", + Type: tc.providerType, + URL: tc.url, + Enabled: true, + ManagedLegacySecurity: true, + } + require.NoError(t, db.Create(&provider).Error) + + // Test + config, err := service.getProviderAggregatedConfig() + require.NoError(t, err) + assert.False(t, config.DestinationAmbiguous, "Single managed provider = not ambiguous") + + // Verify correct field is populated + switch tc.expectedField { + case "WebhookURL": + assert.Equal(t, tc.url, config.WebhookURL) + case "DiscordWebhookURL": + assert.Equal(t, tc.url, config.DiscordWebhookURL) + case "SlackWebhookURL": + assert.Equal(t, tc.url, config.SlackWebhookURL) + case "GotifyURL": + assert.Equal(t, tc.url, config.GotifyURL) + } + }) + } +} + +func TestGetProviderAggregatedConfig_DestinationReporting_MultipleManaged_Ambiguous(t *testing.T) { + db := setupEnhancedServiceDB(t) + service := NewEnhancedSecurityNotificationService(db) + + // Create multiple managed providers + providers := []models.NotificationProvider{ + {ID: "m1", Type: "discord", URL: "https://discord.com/webhook/1", Enabled: true, ManagedLegacySecurity: true}, + {ID: "m2", Type: "discord", URL: "https://discord.com/webhook/2", Enabled: true, ManagedLegacySecurity: true}, + } + + for _, p := range providers { + require.NoError(t, db.Create(&p).Error) + } + + // Test + config, err := service.getProviderAggregatedConfig() + require.NoError(t, err) + assert.True(t, config.DestinationAmbiguous, "Multiple managed providers = ambiguous") + assert.Empty(t, config.WebhookURL) + assert.Empty(t, config.DiscordWebhookURL) + assert.Empty(t, config.SlackWebhookURL) + assert.Empty(t, config.GotifyURL) +} + +func TestGetProviderAggregatedConfig_DestinationReporting_ZeroManaged_Ambiguous(t *testing.T) { + db := setupEnhancedServiceDB(t) + service := NewEnhancedSecurityNotificationService(db) + + // Create provider without managed flag + provider := models.NotificationProvider{ + ID: "unmanaged", + Type: "discord", + URL: "https://discord.com/webhook/1", + Enabled: true, + ManagedLegacySecurity: false, + } + require.NoError(t, db.Create(&provider).Error) + + // Test + config, err := service.getProviderAggregatedConfig() + require.NoError(t, err) + assert.True(t, config.DestinationAmbiguous, "Zero managed providers = ambiguous") +} + +func TestGetSettings_LegacyConfig_NotFound_ReturnsDefaults(t *testing.T) { + db := setupEnhancedServiceDB(t) + service := NewEnhancedSecurityNotificationService(db) + + // Set feature flag to false to use legacy path + require.NoError(t, db.Create(&models.Setting{ + Key: "feature.notifications.security_provider_events.enabled", + Value: "false", + Type: "bool", + }).Error) + + // Don't create legacy config + + // Test + config, err := service.GetSettings() + require.NoError(t, err) + assert.True(t, config.NotifyWAFBlocks, "Default WAF=true") + assert.True(t, config.NotifyACLDenies, "Default ACL=true") + assert.True(t, config.NotifyRateLimitHits, "Default RateLimit=true") +} + +func TestUpdateSettings_FeatureFlagDisabled_UpdatesLegacyConfig(t *testing.T) { + db := setupEnhancedServiceDB(t) + service := NewEnhancedSecurityNotificationService(db) + + // Set feature flag to false + require.NoError(t, db.Create(&models.Setting{ + Key: "feature.notifications.security_provider_events.enabled", + Value: "false", + Type: "bool", + }).Error) + + // Update + req := &models.NotificationConfig{ + NotifyWAFBlocks: false, + NotifyACLDenies: true, + NotifyRateLimitHits: false, + WebhookURL: "https://updated.com/webhook", + } + err := service.UpdateSettings(req) + require.NoError(t, err) + + // Verify + var saved models.NotificationConfig + require.NoError(t, db.First(&saved).Error) + assert.False(t, saved.NotifyWAFBlocks) + assert.True(t, saved.NotifyACLDenies) + assert.False(t, saved.NotifyRateLimitHits) + assert.Equal(t, "https://updated.com/webhook", saved.WebhookURL) +} + +func TestUpdateSettings_FeatureFlagEnabled_UpdatesManagedProviders(t *testing.T) { + db := setupEnhancedServiceDB(t) + service := NewEnhancedSecurityNotificationService(db) + + // Set feature flag to true + require.NoError(t, db.Create(&models.Setting{ + Key: "feature.notifications.security_provider_events.enabled", + Value: "true", + Type: "bool", + }).Error) + + // Create managed provider + provider := models.NotificationProvider{ + ID: "managed", + Type: "discord", + URL: "https://discord.com/webhook/old", + Enabled: true, + ManagedLegacySecurity: true, + NotifySecurityWAFBlocks: true, + NotifySecurityACLDenies: true, + NotifySecurityRateLimitHits: true, + } + require.NoError(t, db.Create(&provider).Error) + + // Update + req := &models.NotificationConfig{ + NotifyWAFBlocks: false, + NotifyACLDenies: true, + NotifyRateLimitHits: false, + DiscordWebhookURL: "https://discord.com/webhook/new", + } + err := service.UpdateSettings(req) + require.NoError(t, err) + + // Verify + var updated models.NotificationProvider + require.NoError(t, db.First(&updated, "id = ?", "managed").Error) + assert.False(t, updated.NotifySecurityWAFBlocks) + assert.True(t, updated.NotifySecurityACLDenies) + assert.False(t, updated.NotifySecurityRateLimitHits) + assert.Equal(t, "https://discord.com/webhook/new", updated.URL) + assert.Equal(t, "discord", updated.Type) +} + +func TestUpdateManagedProviders_CreatesProviderIfNoneExist(t *testing.T) { + db := setupEnhancedServiceDB(t) + service := NewEnhancedSecurityNotificationService(db) + + // No existing providers + + // Update + req := &models.NotificationConfig{ + NotifyWAFBlocks: true, + NotifyACLDenies: false, + DiscordWebhookURL: "https://discord.com/webhook/1", + } + err := service.updateManagedProviders(req) + require.NoError(t, err) + + // Verify + var providers []models.NotificationProvider + require.NoError(t, db.Find(&providers).Error) + require.Len(t, providers, 1) + assert.Equal(t, "discord", providers[0].Type) + assert.Equal(t, "https://discord.com/webhook/1", providers[0].URL) + assert.True(t, providers[0].ManagedLegacySecurity) + assert.True(t, providers[0].NotifySecurityWAFBlocks) + assert.False(t, providers[0].NotifySecurityACLDenies) +} + +func TestUpdateManagedProviders_RejectsMultipleDestinations(t *testing.T) { + db := setupEnhancedServiceDB(t) + service := NewEnhancedSecurityNotificationService(db) + + // Try to set multiple destinations + req := &models.NotificationConfig{ + WebhookURL: "https://example.com/webhook", + DiscordWebhookURL: "https://discord.com/webhook/1", + } + + err := service.updateManagedProviders(req) + assert.Error(t, err) + assert.Contains(t, err.Error(), "ambiguous destination") +} + +func TestUpdateManagedProviders_GotifyValidation_RequiresBothURLAndToken(t *testing.T) { + db := setupEnhancedServiceDB(t) + service := NewEnhancedSecurityNotificationService(db) + + testCases := []struct { + name string + gotifyURL string + gotifyToken string + expectError bool + }{ + { + name: "both_present", + gotifyURL: "https://gotify.example.com", + gotifyToken: "token123", + expectError: false, + }, + { + name: "only_url", + gotifyURL: "https://gotify.example.com", + gotifyToken: "", + expectError: true, + }, + { + name: "only_token", + gotifyURL: "", + gotifyToken: "token123", + expectError: true, + }, + { + name: "both_empty", + gotifyURL: "", + gotifyToken: "", + expectError: false, // No gotify config = valid + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + req := &models.NotificationConfig{ + GotifyURL: tc.gotifyURL, + GotifyToken: tc.gotifyToken, + } + + err := service.updateManagedProviders(req) + if tc.expectError { + assert.Error(t, err) + assert.Contains(t, err.Error(), "incomplete gotify configuration") + } else { + assert.NoError(t, err) + } + }) + } +} + +func TestUpdateManagedProviders_Idempotency_NoUpdateIfNoChange(t *testing.T) { + db := setupEnhancedServiceDB(t) + service := NewEnhancedSecurityNotificationService(db) + + // Create managed provider + initialTime := time.Now().Add(-1 * time.Hour) + provider := models.NotificationProvider{ + ID: "managed", + Type: "discord", + URL: "https://discord.com/webhook/1", + Enabled: true, + ManagedLegacySecurity: true, + NotifySecurityWAFBlocks: true, + NotifySecurityACLDenies: false, + NotifySecurityRateLimitHits: true, + } + provider.CreatedAt = initialTime + provider.UpdatedAt = initialTime + require.NoError(t, db.Create(&provider).Error) + + // Update with same values + req := &models.NotificationConfig{ + NotifyWAFBlocks: true, + NotifyACLDenies: false, + NotifyRateLimitHits: true, + DiscordWebhookURL: "https://discord.com/webhook/1", + } + err := service.updateManagedProviders(req) + require.NoError(t, err) + + // Verify UpdatedAt didn't change + var updated models.NotificationProvider + require.NoError(t, db.First(&updated, "id = ?", "managed").Error) + assert.Equal(t, initialTime.Unix(), updated.UpdatedAt.Unix(), "UpdatedAt should not change if values unchanged") +} + +func TestExtractDestinationURL(t *testing.T) { + service := &EnhancedSecurityNotificationService{} + + testCases := []struct { + name string + req *models.NotificationConfig + expected string + }{ + { + name: "webhook", + req: &models.NotificationConfig{WebhookURL: "https://example.com/webhook"}, + expected: "https://example.com/webhook", + }, + { + name: "discord", + req: &models.NotificationConfig{DiscordWebhookURL: "https://discord.com/webhook/1"}, + expected: "https://discord.com/webhook/1", + }, + { + name: "slack", + req: &models.NotificationConfig{SlackWebhookURL: "https://hooks.slack.com/services/T/B/X"}, + expected: "https://hooks.slack.com/services/T/B/X", + }, + { + name: "gotify", + req: &models.NotificationConfig{GotifyURL: "https://gotify.example.com"}, + expected: "https://gotify.example.com", + }, + { + name: "empty", + req: &models.NotificationConfig{}, + expected: "", + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + result := service.extractDestinationURL(tc.req) + assert.Equal(t, tc.expected, result) + }) + } +} + +func TestExtractDestinationToken(t *testing.T) { + service := &EnhancedSecurityNotificationService{} + + testCases := []struct { + name string + req *models.NotificationConfig + expected string + }{ + { + name: "gotify_token", + req: &models.NotificationConfig{GotifyToken: "token123"}, + expected: "token123", + }, + { + name: "empty", + req: &models.NotificationConfig{}, + expected: "", + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + result := service.extractDestinationToken(tc.req) + assert.Equal(t, tc.expected, result) + }) + } +} + +func TestMigrateFromLegacyConfig_FeatureFlagDisabled_ReadOnlyMode(t *testing.T) { + db := setupEnhancedServiceDB(t) + service := NewEnhancedSecurityNotificationService(db) + + // Set feature flag to false + require.NoError(t, db.Create(&models.Setting{ + Key: "feature.notifications.security_provider_events.enabled", + Value: "false", + Type: "bool", + }).Error) + + // Create legacy config + legacyConfig := models.NotificationConfig{ + NotifyWAFBlocks: true, + WebhookURL: "https://example.com/webhook", + } + require.NoError(t, db.Create(&legacyConfig).Error) + + // Migrate + err := service.MigrateFromLegacyConfig() + require.NoError(t, err) + + // Verify NO provider was created (read-only mode) + var providers []models.NotificationProvider + require.NoError(t, db.Find(&providers).Error) + assert.Len(t, providers, 0, "Feature flag disabled = no provider mutation") +} + +func TestMigrateFromLegacyConfig_NoLegacyConfig_NoOp(t *testing.T) { + db := setupEnhancedServiceDB(t) + service := NewEnhancedSecurityNotificationService(db) + + // Enable feature flag + require.NoError(t, db.Create(&models.Setting{ + Key: "feature.notifications.security_provider_events.enabled", + Value: "true", + Type: "bool", + }).Error) + + // No legacy config + + // Migrate + err := service.MigrateFromLegacyConfig() + require.NoError(t, err) + + // Verify NO provider was created + var providers []models.NotificationProvider + require.NoError(t, db.Find(&providers).Error) + assert.Len(t, providers, 0) +} + +func TestMigrateFromLegacyConfig_CreatesManagedProvider(t *testing.T) { + db := setupEnhancedServiceDB(t) + service := NewEnhancedSecurityNotificationService(db) + + // Enable feature flag + require.NoError(t, db.Create(&models.Setting{ + Key: "feature.notifications.security_provider_events.enabled", + Value: "true", + Type: "bool", + }).Error) + + // Create legacy config + legacyConfig := models.NotificationConfig{ + NotifyWAFBlocks: true, + NotifyACLDenies: false, + NotifyRateLimitHits: true, + WebhookURL: "https://example.com/webhook", + } + require.NoError(t, db.Create(&legacyConfig).Error) + + // Migrate + err := service.MigrateFromLegacyConfig() + require.NoError(t, err) + + // Verify provider created + var providers []models.NotificationProvider + require.NoError(t, db.Find(&providers).Error) + require.Len(t, providers, 1) + assert.True(t, providers[0].ManagedLegacySecurity) + assert.Equal(t, "webhook", providers[0].Type) + assert.Equal(t, "https://example.com/webhook", providers[0].URL) + assert.True(t, providers[0].NotifySecurityWAFBlocks) + assert.False(t, providers[0].NotifySecurityACLDenies) + assert.True(t, providers[0].NotifySecurityRateLimitHits) +} + +func TestMigrateFromLegacyConfig_Idempotent_SameChecksum(t *testing.T) { + db := setupEnhancedServiceDB(t) + service := NewEnhancedSecurityNotificationService(db) + + // Enable feature flag + require.NoError(t, db.Create(&models.Setting{ + Key: "feature.notifications.security_provider_events.enabled", + Value: "true", + Type: "bool", + }).Error) + + // Create legacy config + legacyConfig := models.NotificationConfig{ + NotifyWAFBlocks: true, + WebhookURL: "https://example.com/webhook", + } + require.NoError(t, db.Create(&legacyConfig).Error) + + // First migration + err := service.MigrateFromLegacyConfig() + require.NoError(t, err) + + // Get provider count after first migration + var providersAfterFirst []models.NotificationProvider + require.NoError(t, db.Find(&providersAfterFirst).Error) + firstCount := len(providersAfterFirst) + + // Second migration (should be no-op due to checksum match) + err = service.MigrateFromLegacyConfig() + require.NoError(t, err) + + // Verify no duplicate provider created + var providersAfterSecond []models.NotificationProvider + require.NoError(t, db.Find(&providersAfterSecond).Error) + assert.Equal(t, firstCount, len(providersAfterSecond), "Idempotent migration should not create duplicates") +} + +func TestComputeConfigChecksum_Deterministic(t *testing.T) { + config1 := models.NotificationConfig{ + NotifyWAFBlocks: true, + NotifyACLDenies: false, + NotifyRateLimitHits: true, + WebhookURL: "https://example.com/webhook", + } + + config2 := models.NotificationConfig{ + NotifyWAFBlocks: true, + NotifyACLDenies: false, + NotifyRateLimitHits: true, + WebhookURL: "https://example.com/webhook", + } + + checksum1 := computeConfigChecksum(config1) + checksum2 := computeConfigChecksum(config2) + + assert.Equal(t, checksum1, checksum2, "Same config should produce same checksum") + assert.NotEmpty(t, checksum1) +} + +func TestComputeConfigChecksum_DifferentForDifferentConfigs(t *testing.T) { + config1 := models.NotificationConfig{ + NotifyWAFBlocks: true, + } + + config2 := models.NotificationConfig{ + NotifyWAFBlocks: false, + } + + checksum1 := computeConfigChecksum(config1) + checksum2 := computeConfigChecksum(config2) + + assert.NotEqual(t, checksum1, checksum2, "Different configs should produce different checksums") +} + +func TestIsFeatureEnabled_NotFound_CreatesDefault(t *testing.T) { + // Save and restore env vars + origCharonEnv := os.Getenv("CHARON_ENV") + origGinMode := os.Getenv("GIN_MODE") + defer func() { + _ = os.Setenv("CHARON_ENV", origCharonEnv) + _ = os.Setenv("GIN_MODE", origGinMode) + }() + + testCases := []struct { + name string + charonEnv string + ginMode string + expected bool + description string + }{ + { + name: "production_explicit", + charonEnv: "production", + ginMode: "", + expected: false, + description: "CHARON_ENV=production should default to false", + }, + { + name: "prod_explicit", + charonEnv: "prod", + ginMode: "", + expected: false, + description: "CHARON_ENV=prod should default to false", + }, + { + name: "gin_debug", + charonEnv: "", + ginMode: "debug", + expected: true, + description: "GIN_MODE=debug should default to true", + }, + { + name: "gin_test", + charonEnv: "", + ginMode: "test", + expected: true, + description: "GIN_MODE=test should default to true", + }, + { + name: "both_unset", + charonEnv: "", + ginMode: "", + expected: false, + description: "Both unset should default to false (production)", + }, + { + name: "development", + charonEnv: "development", + ginMode: "", + expected: true, + description: "CHARON_ENV=development should default to true", + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + db := setupEnhancedServiceDB(t) + service := NewEnhancedSecurityNotificationService(db) + + // Set environment + _ = os.Setenv("CHARON_ENV", tc.charonEnv) + _ = os.Setenv("GIN_MODE", tc.ginMode) + + // Test + enabled, err := service.isFeatureEnabled() + require.NoError(t, err) + assert.Equal(t, tc.expected, enabled, tc.description) + + // Verify setting was created + var setting models.Setting + require.NoError(t, db.Where("key = ?", "feature.notifications.security_provider_events.enabled").First(&setting).Error) + expectedValue := "false" + if tc.expected { + expectedValue = "true" + } + assert.Equal(t, expectedValue, setting.Value) + }) + } +} + +func TestSendWebhook_SSRFValidation(t *testing.T) { + db := setupEnhancedServiceDB(t) + service := NewEnhancedSecurityNotificationService(db) + + testCases := []struct { + name string + webhookURL string + shouldFail bool + description string + }{ + { + name: "valid_https", + webhookURL: "https://example.com/webhook", + shouldFail: false, + description: "HTTPS should be allowed", + }, + { + name: "valid_http", + webhookURL: "http://example.com/webhook", + shouldFail: false, + description: "HTTP should be allowed for backwards compatibility", + }, + { + name: "empty_url", + webhookURL: "", + shouldFail: true, + description: "Empty URL should fail validation", + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + event := models.SecurityEvent{ + EventType: "waf_block", + Severity: "high", + Message: "Test event", + } + + err := service.sendWebhook(context.Background(), tc.webhookURL, event) + if tc.shouldFail { + assert.Error(t, err, tc.description) + } else { + // May fail with network error but should pass SSRF validation + // We're testing the validation step, not the actual HTTP call + if err != nil { + assert.NotContains(t, err.Error(), "ssrf validation failed", tc.description) + } + } + }) + } +} + +func TestSendWebhook_Success(t *testing.T) { + db := setupEnhancedServiceDB(t) + service := NewEnhancedSecurityNotificationService(db) + + // Create test server + callCount := 0 + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + callCount++ + assert.Equal(t, "POST", r.Method) + assert.Equal(t, "application/json", r.Header.Get("Content-Type")) + assert.Equal(t, "Charon-Cerberus/1.0", r.Header.Get("User-Agent")) + + // Verify payload + var event models.SecurityEvent + err := json.NewDecoder(r.Body).Decode(&event) + assert.NoError(t, err) + assert.Equal(t, "waf_block", event.EventType) + + w.WriteHeader(http.StatusOK) + })) + defer server.Close() + + event := models.SecurityEvent{ + EventType: "waf_block", + Severity: "high", + Message: "Test event", + } + + err := service.sendWebhook(context.Background(), server.URL, event) + assert.NoError(t, err) + assert.Equal(t, 1, callCount) +} + +func TestNormalizeSecurityEventType(t *testing.T) { + testCases := []struct { + input string + expected string + }{ + {"WAF Block", "waf_block"}, + {"waf_block", "waf_block"}, + {"ACL Deny", "acl_deny"}, + {"acl_deny", "acl_deny"}, + {"Rate Limit", "rate_limit"}, + {"rate_limit", "rate_limit"}, + {"CrowdSec Decision", "crowdsec_decision"}, + {"crowdsec_decision", "crowdsec_decision"}, + {"unknown_event", "unknown_event"}, + {" WAF ", "waf_block"}, + {" ACL ", "acl_deny"}, + } + + for _, tc := range testCases { + t.Run(tc.input, func(t *testing.T) { + result := normalizeSecurityEventType(tc.input) + assert.Equal(t, tc.expected, result) + }) + } +} + +func TestGetDefaultFeatureFlagValue(t *testing.T) { + // Save and restore env vars + origCharonEnv := os.Getenv("CHARON_ENV") + origGinMode := os.Getenv("GIN_MODE") + defer func() { + _ = os.Setenv("CHARON_ENV", origCharonEnv) + _ = os.Setenv("GIN_MODE", origGinMode) + }() + + testCases := []struct { + name string + charonEnv string + ginMode string + expected string + }{ + {"production", "production", "", "false"}, + {"prod", "prod", "", "false"}, + {"debug", "", "debug", "true"}, + {"test", "", "test", "true"}, + {"both_unset", "", "", "false"}, + {"development", "development", "", "true"}, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + db := setupEnhancedServiceDB(t) + service := NewEnhancedSecurityNotificationService(db) + + _ = os.Setenv("CHARON_ENV", tc.charonEnv) + _ = os.Setenv("GIN_MODE", tc.ginMode) + + result := service.getDefaultFeatureFlagValue() + assert.Equal(t, tc.expected, result) + }) + } +} + +func TestGetDefaultFeatureFlagValue_TestMode(t *testing.T) { + db := setupEnhancedServiceDB(t) + service := NewEnhancedSecurityNotificationService(db) + + // Create test mode marker + require.NoError(t, db.Create(&models.Setting{ + Key: "_test_mode_marker", + Value: "true", + Type: "bool", + }).Error) + + result := service.getDefaultFeatureFlagValue() + assert.Equal(t, "true", result, "Test mode should return true") +} diff --git a/backend/internal/services/proxyhost_service_validation_test.go b/backend/internal/services/proxyhost_service_validation_test.go index 6c65cf90..614ec1a8 100644 --- a/backend/internal/services/proxyhost_service_validation_test.go +++ b/backend/internal/services/proxyhost_service_validation_test.go @@ -4,6 +4,7 @@ import ( "testing" "github.com/Wikid82/charon/backend/internal/models" + "github.com/google/uuid" "github.com/stretchr/testify/assert" ) @@ -216,6 +217,9 @@ func TestProxyHostService_ValidateHostname(t *testing.T) { {name: "bracketed ipv6 with port", host: "[::1]:443", wantErr: false}, {name: "docker style underscore", host: "my_service", wantErr: false}, {name: "invalid character", host: "invalid$host", wantErr: true}, + // Lines 58-63: Malformed URLs that fail URL.Parse but still need handling + {name: "malformed https URL", host: "https://::invalid::", wantErr: false}, // Falls back to prefix stripping + {name: "malformed http URL", host: "http://::malformed::", wantErr: false}, // Falls back to prefix stripping } for _, tt := range tests { @@ -229,3 +233,112 @@ func TestProxyHostService_ValidateHostname(t *testing.T) { }) } } + +// TestProxyHostService_ValidateProxyHost_FallbackParsing covers lines 74-75 +func TestProxyHostService_ValidateProxyHost_FallbackParsing(t *testing.T) { + db := setupProxyHostTestDB(t) + service := NewProxyHostService(db) + + // Test URLs that will fail url.Parse but fallback can handle + tests := []struct { + name string + domainName string + forwardHost string + wantErr bool + }{ + { + name: "Valid after stripping https prefix", + domainName: "test1.example.com", + forwardHost: "https://example.com:3000", + wantErr: false, // Fallback strips prefix, validates remaining + }, + { + name: "Valid after stripping http prefix", + domainName: "test2.example.com", + forwardHost: "http://192.168.1.1:8080", + wantErr: false, // Fallback strips prefix + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + host := &models.ProxyHost{ + UUID: uuid.New().String(), // Generate unique UUID + DomainNames: tt.domainName, + ForwardHost: tt.forwardHost, + ForwardPort: 8080, + } + err := service.Create(host) + if tt.wantErr { + assert.Error(t, err) + } else { + assert.NoError(t, err) + } + }) + } +} + +// TestProxyHostService_ValidateProxyHost_InvalidHostnameChars covers lines 115-118 +func TestProxyHostService_ValidateProxyHost_InvalidHostnameChars(t *testing.T) { + db := setupProxyHostTestDB(t) + service := NewProxyHostService(db) + + tests := []struct { + name string + forwardHost string + expectError string + }{ + { + name: "Special characters dollar sign", + forwardHost: "host$name", + expectError: "forward host must be a valid IP address or hostname", + }, + { + name: "Special characters at symbol", + forwardHost: "host@domain", + expectError: "forward host must be a valid IP address or hostname", + }, + { + name: "Special characters percent", + forwardHost: "host%name", + expectError: "forward host must be a valid IP address or hostname", + }, + { + name: "Special characters ampersand", + forwardHost: "host&name", + expectError: "forward host must be a valid IP address or hostname", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + host := &models.ProxyHost{ + DomainNames: "test.example.com", + ForwardHost: tt.forwardHost, + ForwardPort: 8080, + } + err := service.Create(host) + assert.Error(t, err) + assert.Contains(t, err.Error(), tt.expectError) + }) + } +} + +// TestProxyHostService_ValidateProxyHost_DNSChallenge covers lines 128-129 +func TestProxyHostService_ValidateProxyHost_DNSChallenge(t *testing.T) { + db := setupProxyHostTestDB(t) + service := NewProxyHostService(db) + + // Test DNS challenge enabled without provider ID + host := &models.ProxyHost{ + DomainNames: "test.example.com", + ForwardHost: "backend", + ForwardPort: 8080, + UseDNSChallenge: true, + DNSProviderID: nil, // Missing provider ID + } + + err := service.Create(host) + assert.Error(t, err) + assert.Contains(t, err.Error(), "dns provider is required") +} diff --git a/docs/plans/current_spec.md b/docs/plans/current_spec.md index d04a1c43..7587c396 100644 --- a/docs/plans/current_spec.md +++ b/docs/plans/current_spec.md @@ -1,308 +1,342 @@ --- -post_title: "Current Spec: Stabilize Flaky SMTP STARTTLS+AUTH Unit Test" +post_title: "Current Spec: Raise PR #729 Patch Coverage (Backend Targeted Tests)" categories: - actions - testing - - backend - - reliability + - ci + - security tags: - go - - smtp - - starttls + - patch-coverage + - codecov + - pr-729 - unit-tests - - ci-stability -summary: "Implementation plan to remove CI flakiness in MailService STARTTLS+AUTH tests by hardening mock SMTP server lifecycle and connection handling in test helpers only." +summary: "Focused, test-first plan to raise PR #729 patch coverage by adding targeted backend tests for low-coverage changed files, prioritized by impact and implementation risk." post_date: 2026-02-22 --- -## Active Plan: Stabilize Flaky SMTP STARTTLS+AUTH Unit Test +## Active Plan: Raise PR #729 Patch Coverage (Backend) Date: 2026-02-22 Status: Active and authoritative -Scope Type: Test reliability hardening (backend unit tests only) +Scope Type: Backend patch-coverage lift via targeted tests +Authority: This is the only active authoritative plan section in this file. ## 1) Introduction -This plan addresses flakiness in backend unit test -`TestMailService_TestConnection_StartTLSSuccessWithAuth` by improving mock SMTP -test helpers used by `backend/internal/services/mail_service_test.go`. - -Root-cause hypothesis to validate and fix: - -- Existing mock SMTP server helpers accept only one connection, then exit. -- STARTTLS + AUTH flows in `net/smtp` are negotiation-heavy and can involve - additional command/connection behavior under CI timing variance. -- Single-accept test server behavior creates race-prone shutdown windows. +This plan targets PR #729 patch coverage gaps from +[docs/reports/codecove_patch_report.md](docs/reports/codecove_patch_report.md) +with a minimal, test-first approach. Goal: -- Make test helper servers robust and deterministic by accepting connections in - a loop until cleanup, handling each connection in its own goroutine, and - enforcing deterministic shutdown that waits for accept-loop exit and active - handler goroutines (explicit per-connection synchronization via waitgroup). +- Raise patch coverage by adding focused backend tests for changed files with + low patch %, ordered by coverage impact first and implementation risk second. -Non-goal: +Non-goals: -- No production mail service behavior changes. +- No frontend changes. +- No broad refactors. +- No production code changes unless a branch is untestable without a tiny seam. ## 2) Research Findings -### 2.1 Current flaky target and helper topology +### 2.1 Patch report baseline -Primary target test: +Current patch coverage snapshot from +[docs/reports/codecove_patch_report.md](docs/reports/codecove_patch_report.md): -- `backend/internal/services/mail_service_test.go` - - `TestMailService_TestConnection_StartTLSSuccessWithAuth` +1. `backend/internal/services/enhanced_security_notification_service.go` + - 13.13% (285 missing + 6 partial) +2. `backend/internal/services/notification_service.go` + - 75.45% (20 missing + 7 partial) +3. `backend/internal/api/handlers/notification_provider_handler.go` + - 79.22% (15 missing + 1 partial) +4. `backend/internal/api/handlers/security_notifications.go` + - 83.11% (10 missing + 3 partial) +5. `backend/internal/api/handlers/feature_flags_handler.go` + - 71.42% (11 missing + 1 partial) +6. `backend/internal/services/proxyhost_service.go` + - 33.33% (8 missing + 4 partial) +7. `backend/internal/api/routes/routes.go` + - 75.00% (2 missing + 2 partial) +8. `backend/internal/cerberus/cerberus.go` + - 73.33% (2 missing + 2 partial) +9. `backend/internal/notifications/router.go` + - 88.23% (2 missing) -Helper functions in same file (current behavior): +### 2.2 Existing test surface (reuse-first) -- `startMockSMTPServer(...)` - - Single `Accept()` call, single connection handler, then goroutine exits. -- `startMockSSLSMTPServer(...)` - - Single `Accept()` call, single connection handler, then goroutine exits. -- `handleSMTPConn(...)` - - Handles SMTP protocol conversation for each accepted connection. +Existing package tests are strong and should be extended instead of introducing +new harness patterns: -### 2.2 Flakiness vector summary +- Services: + - `backend/internal/services/notification_service_test.go` + - `backend/internal/services/proxyhost_service_test.go` + - `backend/internal/services/enhanced_security_notification_service_discord_only_test.go` +- Handlers: + - `backend/internal/api/handlers/notification_provider_handler_test.go` + - `backend/internal/api/handlers/notification_provider_patch_coverage_test.go` + - `backend/internal/api/handlers/security_notifications_patch_coverage_test.go` + - `backend/internal/api/handlers/feature_flags_handler_coverage_test.go` + - `backend/internal/api/handlers/feature_flags_blocker3_test.go` +- Routes: + - `backend/internal/api/routes/routes_coverage_test.go` +- Cerberus: + - `backend/internal/cerberus/cerberus_test.go` +- Notifications router: + - `backend/internal/notifications/router_test.go` -- Current single-accept model is fragile for tests where client behavior may - include additional negotiation/timing paths. -- Cleanup currently closes listener and waits on a done signal, but done is - tied to a single accept goroutine rather than a full server lifecycle. -- Under CI contention, this can surface nondeterministic failures in tests - expecting reliable STARTTLS + AUTH handshake behavior. +### 2.3 Constraints -### 2.3 Repo config review requested by user - -Reviewed files: - -- `.gitignore` -- `codecov.yml` -- `.dockerignore` -- `Dockerfile` - -Conclusion for this scope: - -- No changes required for this test-only helper stabilization. -- Existing ignore/coverage/docker config does not block this plan. +- Test-first and minimal: backend tests only by default. +- Tiny production seam allowed only for truly unreachable branches. +- Keep branch behavior assertions deterministic and table-driven where possible. ## 3) Requirements (EARS) -- R1: WHEN mock SMTP test helpers are started, THE SYSTEM SHALL accept - connections in a loop until explicit cleanup closes the listener. -- R2: WHEN a connection is accepted, THE SYSTEM SHALL handle it in its own - goroutine so one slow session cannot block new accepts. -- R3: WHEN cleanup is invoked, THE SYSTEM SHALL close the listener and await - deterministic server-loop termination plus completion of active connection - handlers (done channel + waitgroup synchronization). -- R4: IF cleanup wait exceeds timeout, THEN THE SYSTEM SHALL report a failure - signal and return without hanging test execution. -- R5: WHEN this fix is implemented, THE SYSTEM SHALL keep production code - untouched and restrict changes to test helper scope. -- R6: WHEN targeted backend tests run, THE SYSTEM SHALL pass reliably in local - and CI-like conditions. +- R1: WHEN addressing PR #729 coverage gaps, THE SYSTEM SHALL prioritize files + by missing patch lines first, then by implementation risk. +- R2: WHEN adding coverage, THE SYSTEM SHALL modify or add backend test files + only, unless a branch is untestable without a tiny seam. +- R3: IF a tiny seam is required, THEN THE SYSTEM SHALL keep it package-local, + minimal, and solely to unlock deterministic branch testing. +- R4: WHEN tests are implemented, THE SYSTEM SHALL validate with local patch + report preflight, targeted tests, backend coverage script, and patch report rerun. +- R5: WHEN planning delivery, THE SYSTEM SHALL use a single PR unless scope or + rollback risk materially increases. +- R6: WHEN plan is finalized, THE SYSTEM SHALL explicitly review + `.gitignore`, `.dockerignore`, `codecov.yml`, and `Dockerfile` for required + updates. ## 4) Technical Specification -### 4.1 Exact target files and functions (minimal diff scope) +### 4.1 Priority model -Primary file to edit: +Priority score = Coverage impact (missing+partial lines, weighted high) + +implementation risk (weighted low). -- `backend/internal/services/mail_service_test.go` +| Priority | File | Coverage Gap | Risk | Strategy | +|---|---|---:|---|---| +| P0 | `backend/internal/services/enhanced_security_notification_service.go` | 285 + 6 | Medium | New focused coverage file + extend existing Discord-only tests | +| P1 | `backend/internal/services/notification_service.go` | 20 + 7 | Medium | Extend existing comprehensive service tests | +| P2 | `backend/internal/api/handlers/notification_provider_handler.go` | 15 + 1 | Low | Extend handler patch tests for admin/error branches | +| P2 | `backend/internal/api/handlers/feature_flags_handler.go` | 11 + 1 | Low | Extend coverage tests for env/db parse and transaction branches | +| P2 | `backend/internal/api/handlers/security_notifications.go` | 10 + 3 | Low | Extend patch tests for source-validation and dispatch-failure branches | +| P3 | `backend/internal/services/proxyhost_service.go` | 8 + 4 | Low | Extend validation + advanced config edge branches | +| P4 | `backend/internal/api/routes/routes.go` | 2 + 2 | Medium | Extend route wiring error/non-fatal paths only | +| P4 | `backend/internal/cerberus/cerberus.go` | 2 + 2 | Low | Extend IsEnabled/cache and fail-closed dispatch edges | +| P5 | `backend/internal/notifications/router.go` | 2 | Low | Add table cases for missing flags/default false | -Functions in scope: +### 4.2 Exact target test files to modify/create -- `startMockSMTPServer(t *testing.T, tlsConf *tls.Config, supportStartTLS bool, requireAuth bool) (string, func())` -- `startMockSSLSMTPServer(t *testing.T, tlsConf *tls.Config, requireAuth bool) (string, func())` +Primary target files: -Related function (read-only unless strictly necessary): +1. `backend/internal/services/enhanced_security_notification_service_discord_only_test.go` (modify) +2. `backend/internal/services/enhanced_security_notification_service_coverage_test.go` (create) +3. `backend/internal/services/notification_service_test.go` (modify) +4. `backend/internal/api/handlers/notification_provider_patch_coverage_test.go` (modify) +5. `backend/internal/api/handlers/notification_provider_handler_test.go` (modify, only if needed) +6. `backend/internal/api/handlers/security_notifications_patch_coverage_test.go` (modify) +7. `backend/internal/api/handlers/feature_flags_handler_coverage_test.go` (modify) +8. `backend/internal/services/proxyhost_service_test.go` (modify) +9. `backend/internal/api/routes/routes_coverage_test.go` (modify) +10. `backend/internal/cerberus/cerberus_test.go` (modify) +11. `backend/internal/notifications/router_test.go` (modify) -- `handleSMTPConn(conn net.Conn, tlsConf *tls.Config, supportStartTLS bool, requireAuth bool)` +### 4.3 Branch targets per file -Out of scope: +#### P0: enhanced security notification service -- `backend/internal/services/mail_service.go` -- Any non-mail-service test files. +- `GetSettings` feature-flag off/on branch behavior. +- Managed provider aggregation permutations (0/1/many managed destinations). +- Gotify incomplete payload rejection path. +- Idempotent `updateManagedProviders` (no save when unchanged). +- Migration marker checksum no-op and disabled-flag read-only migration behavior. +- Feature defaulting matrix (`CHARON_ENV`, `GIN_MODE`, `_test_mode_marker`). +- `sendWebhook` SSRF rejection + non-2xx handling. -### 4.2 Planned helper behavior changes +#### P1: notification service -For both helper server starters: +- Event filtering for security event variants (`security_waf`, `security_acl`, + `security_rate_limit`, `security_crowdsec`). +- Discord URL validation error branches and non-discord fail-closed paths. +- `UpdateProvider` mutation guards (deprecated provider type/enable cases). +- Template size/timeouts and payload validation branches where uncovered. -1. Replace single `Accept()` with accept loop: - - Continue accepting until listener close returns an error. - - Treat listener-close accept errors as normal shutdown path. -2. Spawn per-connection handler goroutine: - - Each accepted connection handled independently. - - Connection closed within goroutine after handling. -3. Deterministic lifecycle signaling: - - Keep `done` channel signaling tied to server accept-loop exit. - - Ensure exactly one close of `done` from server goroutine. - - Track active connection-handler goroutines with explicit waitgroup sync. -4. Cleanup contract: - - `cleanup()` closes listener first. - - Wait for accept-loop exit and active handler completion with bounded timeout (`2s` currently acceptable). - - Never block indefinitely. - - Timeout path is a test failure signal (non-hanging), not a silent pass. +#### P2: handler targets -### 4.3 Concurrency and race safety notes +- Notification provider handler: + - `Create` and `Update` blocked-code branches (discord-only, immutable legacy, + cannot enable deprecated). + - `isProviderValidationError` classification branches. +- Security notifications handler: + - Invalid source IP parsing, unauthorized source path, malformed payload, and + accepted path with dispatch error non-fatal behavior. +- Feature flags handler: + - legacy fallback hard-false resolution branches, invalid env value warning + path coverage, and unknown key ignore behavior in updates. -- Accept-loop ownership: - - One goroutine owns listener accept loop and is the only goroutine that may - close `done`. -- Connection handler isolation: - - One goroutine per accepted connection; no shared mutable state required for - protocol behavior in this helper. -- Per-connection synchronization: - - A waitgroup must increment before each handler goroutine starts and must - decrement on handler exit so cleanup can deterministically wait for active - handlers to finish. -- Listener close semantics: - - `listener.Close()` from cleanup is expected to break `Accept()`. - - Exit condition should avoid noisy test failures on intentional close. -- Cleanup timeout behavior: - - Timeout remains defensive to prevent suite hangs under pathological CI - resource starvation. - - Timeout branch must report failure (e.g., `t.Errorf`/`t.Fatalf` policy) and - return; no panic and no silent success. +#### P3-P5: lower-line but necessary gaps -### 4.4 Error handling policy in helpers +- Proxy host service: malformed forward host/path stripping and advanced config + normalization failure branches. +- Routes: migration invocation/error paths and non-fatal startup behavior. +- Cerberus: feature-flag absent/false fail-closed dispatch and cache refresh + branch coverage. +- Notifications router: explicit default false behavior for missing flags. -- Treat only expected shutdown accept errors (listener close path) as normal. -- Surface unexpected `Accept()` failures as test failure signals. -- Keep helper logic simple and deterministic; avoid over-engineered retry logic. +### 4.4 Tiny seam policy (only if required) + +Allowed only when a specific branch cannot be reached with current test hooks. +Potential seam examples (if needed): + +- Package-level function var for ticker/sleep in route boot goroutines. +- Package-level clock helper for time-dependent branch determinism. + +If introduced: + +- Must be private to package. +- Must not alter runtime behavior. +- Must be documented in test file rationale and removed if later unnecessary. ## 5) Implementation Plan -### Phase 1: Testing protocol sequencing note +### Phase 1: Baseline and preflight -- Policy remains E2E-first globally. -- Explicit exception rationale for this change: scope is backend test-helper-only - (`mail_service_test.go`) with no production/frontend/runtime behavior delta, - so targeted backend test-first verification is authorized for this plan. -- Mandatory preflight before unit/coverage steps: - - `bash scripts/local-patch-report.sh` - - Artifacts: `test-results/local-patch-report.md` and `test-results/local-patch-report.json` +1. Run local patch report preflight: + - `bash scripts/local-patch-report.sh` +2. Confirm artifacts: + - `test-results/local-patch-report.md` + - `test-results/local-patch-report.json` +3. Capture baseline patch gaps for the 9 target files. -### Phase 2: Baseline confirmation and failure reproduction context +### Phase 2: Highest-impact tests first (P0-P1) -- Capture current helper behavior and flaky test target: - - `go test ./backend/internal/services -run TestMailService_TestConnection_StartTLSSuccessWithAuth -count=1 -v` +1. Implement `enhanced_security_notification_service` branch tests. +2. Implement `notification_service` gap tests. +3. Run targeted service tests only: + - `go test ./backend/internal/services -run 'Test(DiscordOnly|EnhancedSecurity|NotificationService|ProxyHostService)' -count=1` -### Phase 3: Helper lifecycle hardening +### Phase 3: Handler and service gap closure (P2-P3) -- Update `startMockSMTPServer` and `startMockSSLSMTPServer` to loop accept. -- Add per-connection goroutine handling. -- Preserve/strengthen deterministic cleanup using listener close + done wait + - per-connection waitgroup completion. +1. Extend handler coverage tests for notification provider, security + notifications, and feature flags. +2. Extend proxy host service tests for uncovered validation/normalization + branches. +3. Run targeted handler/service tests: + - `go test ./backend/internal/api/handlers -run 'Test(NotificationProvider|SecurityNotification|FeatureFlags)' -count=1` + - `go test ./backend/internal/services -run 'TestProxyHostService' -count=1` -### Phase 4: Targeted validation +### Phase 4: Wiring and security/router edges (P4-P5) -- Re-run target test repeatedly to validate stability: - - `go test ./backend/internal/services -run TestMailService_TestConnection_StartTLSSuccessWithAuth -count=20` -- Run mail service test subset: - - `go test ./backend/internal/services -run TestMailService_ -count=1` -- Run race-focused targeted validation: - - `go test -race ./backend/internal/services -run 'TestMailService_(TestConnection|Send)' -count=1` +1. Extend route, cerberus, and notifications router tests for remaining small + branch gaps. +2. Run targeted package tests: + - `go test ./backend/internal/api/routes -run 'TestRegister' -count=1` + - `go test ./backend/internal/cerberus -run 'TestCerberus' -count=1` + - `go test ./backend/internal/notifications -run 'TestRouter' -count=1` -### Phase 5: Backend coverage validation +### Phase 5: Coverage validation and rerun -- Run backend coverage task/script required by repo workflow: - - Preferred script: `scripts/go-test-coverage.sh` - - VS Code equivalent: backend coverage task if available. +1. Run backend coverage script: + - `scripts/go-test-coverage.sh` +2. Re-run local patch report: + - `bash scripts/local-patch-report.sh` +3. Verify patch coverage increase and remaining uncovered lines are justified or + queued. -## 6) Validation Matrix +## 6) Validation Workflow (Required Order) -| Validation Item | Command / Task | Scope | Pass Criteria | -|---|---|---|---| -| Targeted flaky test | `go test ./backend/internal/services -run TestMailService_TestConnection_StartTLSSuccessWithAuth -count=20` | Direct flaky test | No failures across repeated runs | -| Mail service subset | `go test ./backend/internal/services -run TestMailService_ -count=1` | Nearby regression safety | All selected tests pass | -| Race-focused targeted tests | `go test -race ./backend/internal/services -run 'TestMailService_(TestConnection|Send)' -count=1` | Concurrency/race safety | No race reports; tests pass | -| Package sanity | `go test ./backend/internal/services -count=1` | Service package confidence | Package tests pass | -| Backend coverage gate | `scripts/go-test-coverage.sh` | Repo-required backend coverage check | Meets configured minimum threshold (85% default) | +| Step | Command | Expected Result | +|---|---|---| +| 1 | `npx playwright test --project=firefox` | E2E-first compliance satisfied before unit/coverage work | +| 2 | `bash scripts/local-patch-report.sh` | Local patch preflight artifacts generated (`.md` + `.json`) | +| 3 | `go test` targeted packages/files (phased above) | Targeted unit validation passes and intended branches are exercised | +| 4 | `scripts/go-test-coverage.sh` | Targeted coverage validation passes backend coverage gate | +| 5 | `bash scripts/local-patch-report.sh` | Patch % improves for PR #729 target files | -Notes: +## 7) PR Slicing Strategy -- E2E-first protocol remains project-wide policy. This plan uses the explicit - backend-only targeted-test exception because scope is confined to test helper - internals with no production/UI behavior changes. -- Local patch report preflight is required before unit/coverage gates. - -## 7) Risk Assessment - -Risk level: Low. - -- Change type: test-only. -- Production code: untouched. -- Runtime behavior: unchanged for shipped binary. -- Primary risk: helper lifecycle bug causing test hangs. -- Mitigation: bounded cleanup timeout, accept-loop exit on listener close, - focused repeated-run validation. - -## 8) PR Slicing Strategy - -Decision: Single PR. +Decision: **Single PR**. Rationale: -- Extremely small scope (one test file, two helper functions). -- No cross-domain dependencies. -- Easier review and rollback. +- All work is backend test-only and tightly coupled to one outcome (patch + coverage increase for PR #729). +- Review remains tractable because changes are limited to test files. +- Rollback is low risk (test-only diff). -### PR-1 (single slice) +Contingency trigger for multi-PR split (only if needed): -- Scope: - - `backend/internal/services/mail_service_test.go` helper lifecycle updates. -- Dependencies: - - None. -- Validation gates: - - Validation matrix in Section 6. -- Rollback contingency: - - Revert single PR if instability increases. +- If tiny production seams become necessary in multiple packages, split into: + - PR-1: minimal seam(s) + seam tests. + - PR-2: coverage tests consuming seam(s). + +## 8) Risk Guardrail (P0 Blast Radius) + +Timebox / fallback trigger: + +- If, after completing P0 + P1 and running `bash scripts/local-patch-report.sh`, + either of the following is true: + - overall patch coverage is still `< 80%`, or + - combined uplift from baseline is `< +8 percentage points`, + THEN stop further broad branch expansion and switch to fallback mode. + +Fallback decision path (next-step actions): + +1. Freeze any additional new-branch test expansion in `P2+` files. +2. Generate and attach latest artifacts: + - `test-results/local-patch-report.md` + - `test-results/local-patch-report.json` +3. Run targeted gap triage on top remaining uncovered changed lines only. +4. Choose one path: + - Path A (default): add one minimal seam in highest-impact file and finish in + same PR. + - Path B: split into follow-up PR for seam + residual coverage if seam scope + exceeds one package or introduces rollback risk. +5. Re-run `bash scripts/local-patch-report.sh` and proceed only if targets in + Section 10 are met. ## 9) Config File Review Outcome -Reviewed for this request: +Reviewed for this plan: - `.gitignore` -- `codecov.yml` - `.dockerignore` +- `codecov.yml` - `Dockerfile` -Suggested updates: +Planned updates: **None** (test-only backend changes do not require updates). -- None required for this scope. -- Revisit only if implementation introduces new generated artifacts or test - output paths not currently handled (not expected here). +## 10) Acceptance Criteria (Measurable) -## 10) Acceptance Criteria +Overall patch-coverage target for this PR uplift effort: -- AC1: `startMockSMTPServer` accepts in loop until cleanup and no longer exits - after a single connection. -- AC2: `startMockSSLSMTPServer` accepts in loop until cleanup and no longer - exits after a single connection. -- AC3: Each accepted connection is handled in its own goroutine. -- AC4: Cleanup closes listener and uses done-channel plus per-connection - waitgroup synchronization with bounded timeout. -- AC5: Unexpected `Accept()` failures are surfaced as test failure signals; - expected listener-close shutdown errors are treated as normal. -- AC6: `TestMailService_TestConnection_StartTLSSuccessWithAuth` passes reliably - under repeated runs. -- AC7: Targeted race validation for mail-service tests passes with `go test -race`. -- AC8: Cleanup timeout path reports failure and returns (non-hanging), never - silent pass. -- AC9: Backend coverage script/task completes successfully at configured - threshold. -- AC10: No production file changes are included in the implementation PR. +- AC1: Overall PR patch coverage (Codecov patch view) reaches `>= 85%`. -## 11) Definition of Done +Per-file patch coverage targets (from current 9-file backend target list): -- Planned helper changes are implemented exactly in scoped functions. -- Cleanup deterministically waits for accept-loop exit and active handlers via - done + waitgroup synchronization. -- Only expected listener-close shutdown accept errors are non-fatal; unexpected - accept errors fail tests. -- Cleanup timeout is reported as failure signal and cannot pass silently. -- Validation matrix passes. -- Diff is limited to test helper scope in `mail_service_test.go`. -- No updates to `.gitignore`, `codecov.yml`, `.dockerignore`, or `Dockerfile` - are required or included. +- AC2: `backend/internal/services/enhanced_security_notification_service.go` `>= 60%` +- AC3: `backend/internal/services/notification_service.go` `>= 90%` +- AC4: `backend/internal/api/handlers/notification_provider_handler.go` `>= 92%` +- AC5: `backend/internal/api/handlers/security_notifications.go` `>= 95%` +- AC6: `backend/internal/api/handlers/feature_flags_handler.go` `>= 90%` +- AC7: `backend/internal/services/proxyhost_service.go` `>= 75%` +- AC8: `backend/internal/api/routes/routes.go` `>= 90%` +- AC9: `backend/internal/cerberus/cerberus.go` `>= 90%` +- AC10: `backend/internal/notifications/router.go` `>= 95%` + +Artifact-gated pass condition: + +- AC11: `bash scripts/local-patch-report.sh` must generate both artifacts: + - `test-results/local-patch-report.md` + - `test-results/local-patch-report.json` +- AC12: The generated artifacts must show overall target (AC1) and all per-file + targets (AC2-AC10) as met for this uplift effort. + +Process and scope checks: + +- AC13: Plan remains backend test-focused with no unrelated production changes. +- AC14: `.gitignore`, `.dockerignore`, `codecov.yml`, and `Dockerfile` review + remains recorded with no required updates for this scope. diff --git a/frontend/src/pages/__tests__/Security.functional.test.tsx b/frontend/src/pages/__tests__/Security.functional.test.tsx index 4cfd1477..afb7d543 100644 --- a/frontend/src/pages/__tests__/Security.functional.test.tsx +++ b/frontend/src/pages/__tests__/Security.functional.test.tsx @@ -14,6 +14,16 @@ import * as securityApi from '../../api/security' import * as crowdsecApi from '../../api/crowdsec' import * as settingsApi from '../../api/settings' +const mockNavigate = vi.hoisted(() => vi.fn()) + +vi.mock('react-router-dom', async () => { + const actual = await vi.importActual('react-router-dom') + return { + ...actual, + useNavigate: () => mockNavigate, + } +}) + vi.mock('../../api/security') vi.mock('../../api/crowdsec') vi.mock('../../api/settings') @@ -168,6 +178,7 @@ describe('Security Page - Functional Tests', () => { }, }) vi.clearAllMocks() + mockNavigate.mockReset() vi.mocked(crowdsecApi.statusCrowdsec).mockResolvedValue({ running: false, pid: 0, lapi_ready: false }) vi.mocked(settingsApi.updateSetting).mockResolvedValue() }) @@ -433,6 +444,21 @@ describe('Security Page - Functional Tests', () => { expect(screen.getByRole('button', { name: /Notifications/i })).not.toBeDisabled() }) }) + + it('should navigate to notifications settings when Notifications button is clicked', async () => { + const user = userEvent.setup() + vi.mocked(securityApi.getSecurityStatus).mockResolvedValue(mockSecurityStatusAllEnabled) + + await renderSecurityPage() + + await waitFor(() => { + expect(screen.getByRole('button', { name: /Notifications/i })).toBeInTheDocument() + }) + + await user.click(screen.getByRole('button', { name: /Notifications/i })) + + expect(mockNavigate).toHaveBeenCalledWith('/settings/notifications') + }) }) // NOTE: CrowdSec Bouncer Key Display moved to CrowdSecConfig page (Sprint 3)