diff --git a/backend/go.sum b/backend/go.sum index 576d192e..7a3151b8 100644 --- a/backend/go.sum +++ b/backend/go.sum @@ -207,8 +207,6 @@ golang.org/x/arch v0.22.0 h1:c/Zle32i5ttqRXjdLyyHZESLD/bB90DCU1g9l/0YBDI= golang.org/x/arch v0.22.0/go.mod h1:dNHoOeKiyja7GTvF9NJS1l3Z2yntpQNzgrjh1cU103A= golang.org/x/crypto v0.46.0 h1:cKRW/pmt1pKAfetfu+RCEvjvZkA9RimPbh7bhFjGVBU= golang.org/x/crypto v0.46.0/go.mod h1:Evb/oLKmMraqjZ2iQTwDwvCtJkczlDuTmdJXoZVzqU0= -golang.org/x/net v0.47.0 h1:Mx+4dIFzqraBXUugkia1OOvlD6LemFo1ALMHjrXDOhY= -golang.org/x/net v0.47.0/go.mod h1:/jNxtkgq5yWUGYkaZGqo27cfGZ1c5Nen03aYrrKpVRU= golang.org/x/net v0.48.0 h1:zyQRTTrjc33Lhh0fBgT/H3oZq9WuvRR5gPC70xpDiQU= golang.org/x/net v0.48.0/go.mod h1:+ndRgGjkh8FGtu1w1FGbEC31if4VrNVMuKTgcAAnQRY= golang.org/x/sys v0.0.0-20220715151400-c0bba94af5f8/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg= diff --git a/backend/internal/api/handlers/settings_handler_test.go b/backend/internal/api/handlers/settings_handler_test.go index 86c69dd5..728ebfbc 100644 --- a/backend/internal/api/handlers/settings_handler_test.go +++ b/backend/internal/api/handlers/settings_handler_test.go @@ -1,10 +1,15 @@ package handlers_test import ( + "bufio" "bytes" "encoding/json" + "fmt" + "net" "net/http" "net/http/httptest" + "strings" + "sync" "testing" "github.com/gin-gonic/gin" @@ -16,6 +21,101 @@ import ( "github.com/Wikid82/charon/backend/internal/models" ) +func startTestSMTPServer(t *testing.T) (host string, port int) { + t.Helper() + + ln, err := net.Listen("tcp", "127.0.0.1:0") + if err != nil { + t.Fatalf("failed to listen for smtp test server: %v", err) + } + + var wg sync.WaitGroup + acceptDone := make(chan struct{}) + go func() { + defer close(acceptDone) + for { + conn, err := ln.Accept() + if err != nil { + return + } + wg.Add(1) + go func(c net.Conn) { + defer wg.Done() + defer func() { _ = c.Close() }() + handleSMTPConnection(c) + }(conn) + } + }() + + t.Cleanup(func() { + _ = ln.Close() + <-acceptDone + wg.Wait() + }) + + host, portStr, err := net.SplitHostPort(ln.Addr().String()) + if err != nil { + t.Fatalf("failed to split smtp listener addr: %v", err) + } + if _, err := fmt.Sscanf(portStr, "%d", &port); err != nil { + t.Fatalf("failed to parse smtp listener port: %v", err) + } + + return host, port +} + +func handleSMTPConnection(conn net.Conn) { + r := bufio.NewReader(conn) + w := bufio.NewWriter(conn) + + writeLine := func(line string) { + _, _ = w.WriteString(line + "\r\n") + _ = w.Flush() + } + + writeLine("220 localhost ESMTP test") + + for { + line, err := r.ReadString('\n') + if err != nil { + return + } + cmd := strings.TrimSpace(line) + upper := strings.ToUpper(cmd) + + switch { + case strings.HasPrefix(upper, "EHLO") || strings.HasPrefix(upper, "HELO"): + writeLine("250-localhost") + writeLine("250 OK") + case strings.HasPrefix(upper, "MAIL FROM:"): + writeLine("250 OK") + case strings.HasPrefix(upper, "RCPT TO:"): + writeLine("250 OK") + case strings.HasPrefix(upper, "DATA"): + writeLine("354 End data with .") + for { + dataLine, err := r.ReadString('\n') + if err != nil { + return + } + if strings.TrimRight(dataLine, "\r\n") == "." { + break + } + } + writeLine("250 OK") + case strings.HasPrefix(upper, "RSET"): + writeLine("250 OK") + case strings.HasPrefix(upper, "NOOP"): + writeLine("250 OK") + case strings.HasPrefix(upper, "QUIT"): + writeLine("221 Bye") + return + default: + writeLine("250 OK") + } + } +} + func setupSettingsTestDB(t *testing.T) *gorm.DB { dsn := "file:" + t.Name() + "?mode=memory&cache=shared" db, err := gorm.Open(sqlite.Open(dsn), &gorm.Config{}) @@ -400,6 +500,35 @@ func TestSettingsHandler_TestSMTPConfig_NotConfigured(t *testing.T) { assert.Equal(t, false, resp["success"]) } +func TestSettingsHandler_TestSMTPConfig_Success(t *testing.T) { + gin.SetMode(gin.TestMode) + handler, db := setupSettingsHandlerWithMail(t) + + host, port := startTestSMTPServer(t) + + // Seed SMTP config for local test server. + db.Create(&models.Setting{Key: "smtp_host", Value: host, Category: "smtp", Type: "string"}) + db.Create(&models.Setting{Key: "smtp_port", Value: fmt.Sprintf("%d", port), Category: "smtp", Type: "number"}) + db.Create(&models.Setting{Key: "smtp_encryption", Value: "none", Category: "smtp", Type: "string"}) + + router := gin.New() + router.Use(func(c *gin.Context) { + c.Set("role", "admin") + c.Next() + }) + router.POST("/settings/smtp/test", handler.TestSMTPConfig) + + req, _ := http.NewRequest("POST", "/settings/smtp/test", http.NoBody) + w := httptest.NewRecorder() + router.ServeHTTP(w, req) + + assert.Equal(t, http.StatusOK, w.Code) + + var resp map[string]any + _ = json.Unmarshal(w.Body.Bytes(), &resp) + assert.Equal(t, true, resp["success"]) +} + func TestSettingsHandler_SendTestEmail_NonAdmin(t *testing.T) { gin.SetMode(gin.TestMode) handler, _ := setupSettingsHandlerWithMail(t) @@ -464,6 +593,38 @@ func TestSettingsHandler_SendTestEmail_NotConfigured(t *testing.T) { assert.Equal(t, false, resp["success"]) } +func TestSettingsHandler_SendTestEmail_Success(t *testing.T) { + gin.SetMode(gin.TestMode) + handler, db := setupSettingsHandlerWithMail(t) + + host, port := startTestSMTPServer(t) + + // Seed SMTP config for local test server. + db.Create(&models.Setting{Key: "smtp_host", Value: host, Category: "smtp", Type: "string"}) + db.Create(&models.Setting{Key: "smtp_port", Value: fmt.Sprintf("%d", port), Category: "smtp", Type: "number"}) + db.Create(&models.Setting{Key: "smtp_from_address", Value: "noreply@example.com", Category: "smtp", Type: "string"}) + db.Create(&models.Setting{Key: "smtp_encryption", Value: "none", Category: "smtp", Type: "string"}) + + router := gin.New() + router.Use(func(c *gin.Context) { + c.Set("role", "admin") + c.Next() + }) + router.POST("/settings/smtp/send-test", handler.SendTestEmail) + + body := map[string]string{"to": "test@example.com"} + jsonBody, _ := json.Marshal(body) + req, _ := http.NewRequest("POST", "/settings/smtp/send-test", bytes.NewBuffer(jsonBody)) + req.Header.Set("Content-Type", "application/json") + w := httptest.NewRecorder() + router.ServeHTTP(w, req) + + assert.Equal(t, http.StatusOK, w.Code) + var resp map[string]any + _ = json.Unmarshal(w.Body.Bytes(), &resp) + assert.Equal(t, true, resp["success"]) +} + func TestMaskPassword(t *testing.T) { // Empty password assert.Equal(t, "", handlers.MaskPasswordForTest("")) @@ -768,6 +929,35 @@ func TestSettingsHandler_TestPublicURL_DNSFailure(t *testing.T) { "Expected DNS error message, got: %s", errorMsg) } +func TestSettingsHandler_TestPublicURL_ConnectivityError(t *testing.T) { + gin.SetMode(gin.TestMode) + handler, _ := setupSettingsHandlerWithMail(t) + + router := gin.New() + router.Use(func(c *gin.Context) { + c.Set("role", "admin") + c.Next() + }) + router.POST("/settings/test-url", handler.TestPublicURL) + + // 192.0.2.0/24 is reserved for documentation/testing and is not considered private by + // network.IsPrivateIP(). Using a closed port should trigger a deterministic connect error + // after passing SSRF validation. + body := map[string]string{"url": "http://192.0.2.1:1"} + jsonBody, _ := json.Marshal(body) + req, _ := http.NewRequest("POST", "/settings/test-url", bytes.NewBuffer(jsonBody)) + req.Header.Set("Content-Type", "application/json") + w := httptest.NewRecorder() + router.ServeHTTP(w, req) + + assert.Equal(t, http.StatusOK, w.Code) + var resp map[string]any + _ = json.Unmarshal(w.Body.Bytes(), &resp) + assert.Equal(t, false, resp["reachable"]) + _, ok := resp["error"].(string) + assert.True(t, ok) +} + // ============= SSRF Protection Tests ============= func TestSettingsHandler_TestPublicURL_SSRFProtection(t *testing.T) { diff --git a/backend/internal/caddy/client_test.go b/backend/internal/caddy/client_test.go index 3b036eab..3fc664ae 100644 --- a/backend/internal/caddy/client_test.go +++ b/backend/internal/caddy/client_test.go @@ -6,6 +6,7 @@ import ( "fmt" "net/http" "net/http/httptest" + "net/url" "testing" "github.com/stretchr/testify/require" @@ -193,3 +194,34 @@ func TestClient_Ping_TransportError(t *testing.T) { require.Error(t, err) require.Contains(t, err.Error(), "caddy unreachable") } + +func TestClient_GetConfig_BaseURLNil_ReturnsError(t *testing.T) { + client := &Client{ + baseURL: nil, + httpClient: http.DefaultClient, + initErr: nil, + } + + _, err := client.GetConfig(context.Background()) + require.Error(t, err) + require.Contains(t, err.Error(), "base URL is not configured") +} + +func TestClient_RequestCreationErrors_FromInvalidResolvedURL(t *testing.T) { + client := &Client{ + baseURL: &url.URL{Scheme: "http", Host: "example.com\n"}, + initErr: nil, + } + + err := client.Load(context.Background(), &Config{}) + require.Error(t, err) + require.Contains(t, err.Error(), "create request") + + _, err = client.GetConfig(context.Background()) + require.Error(t, err) + require.Contains(t, err.Error(), "create request") + + err = client.Ping(context.Background()) + require.Error(t, err) + require.Contains(t, err.Error(), "create request") +} diff --git a/backend/internal/caddy/config_patch_coverage_test.go b/backend/internal/caddy/config_patch_coverage_test.go index 1743dd23..b62251de 100644 --- a/backend/internal/caddy/config_patch_coverage_test.go +++ b/backend/internal/caddy/config_patch_coverage_test.go @@ -3,20 +3,50 @@ package caddy import ( "os" "testing" + "time" "github.com/Wikid82/charon/backend/internal/models" + "github.com/Wikid82/charon/backend/pkg/dnsprovider" "github.com/stretchr/testify/require" _ "github.com/Wikid82/charon/backend/pkg/dnsprovider/builtin" // Auto-register DNS providers ) +type multiCredTestProvider struct{} + +func (p *multiCredTestProvider) Type() string { return "testmulti" } +func (p *multiCredTestProvider) Metadata() dnsprovider.ProviderMetadata { + return dnsprovider.ProviderMetadata{Type: p.Type(), Name: "Test Multi", IsBuiltIn: true} +} +func (p *multiCredTestProvider) Init() error { return nil } +func (p *multiCredTestProvider) Cleanup() error { return nil } +func (p *multiCredTestProvider) RequiredCredentialFields() []dnsprovider.CredentialFieldSpec { + return nil +} +func (p *multiCredTestProvider) OptionalCredentialFields() []dnsprovider.CredentialFieldSpec { + return nil +} +func (p *multiCredTestProvider) ValidateCredentials(creds map[string]string) error { return nil } +func (p *multiCredTestProvider) TestCredentials(creds map[string]string) error { return nil } +func (p *multiCredTestProvider) SupportsMultiCredential() bool { return true } +func (p *multiCredTestProvider) BuildCaddyConfig(creds map[string]string) map[string]any { + return map[string]any{"name": p.Type(), "token": creds["token"]} +} +func (p *multiCredTestProvider) BuildCaddyConfigForZone(baseDomain string, creds map[string]string) map[string]any { + return map[string]any{"name": p.Type(), "zone": baseDomain, "token": creds["token"]} +} +func (p *multiCredTestProvider) PropagationTimeout() time.Duration { return 2 * time.Second } +func (p *multiCredTestProvider) PollingInterval() time.Duration { return 1 * time.Second } + +func mustProviderID(v uint) *uint { return &v } + func TestGenerateConfig_DNSChallenge_LetsEncrypt_StagingCAAndPropagationTimeout(t *testing.T) { providerID := uint(1) host := models.ProxyHost{ Enabled: true, DomainNames: "*.example.com,example.com", DNSProvider: &models.DNSProvider{ID: providerID, ProviderType: "cloudflare"}, - DNSProviderID: func() *uint { v := providerID; return &v }(), + DNSProviderID: mustProviderID(providerID), } conf, err := GenerateConfig( @@ -86,7 +116,7 @@ func TestGenerateConfig_DNSChallenge_ZeroSSL_IssuerShape(t *testing.T) { Enabled: true, DomainNames: "*.example.net", DNSProvider: &models.DNSProvider{ID: providerID, ProviderType: "cloudflare"}, - DNSProviderID: func() *uint { v := providerID; return &v }(), + DNSProviderID: mustProviderID(providerID), } conf, err := GenerateConfig( @@ -137,7 +167,7 @@ func TestGenerateConfig_DNSChallenge_SkipsPolicyWhenProviderConfigMissing(t *tes Enabled: true, DomainNames: "*.example.org", DNSProvider: &models.DNSProvider{ID: providerID, ProviderType: "cloudflare"}, - DNSProviderID: func() *uint { v := providerID; return &v }(), + DNSProviderID: mustProviderID(providerID), } conf, err := GenerateConfig( @@ -229,7 +259,7 @@ func TestGenerateConfig_MultiCredential_ZoneSpecificPolicies(t *testing.T) { Enabled: true, DomainNames: "*.zone1.com,zone1.com,*.zone2.com,zone2.com", DNSProvider: &models.DNSProvider{ID: providerID, ProviderType: "cloudflare"}, - DNSProviderID: func() *uint { v := providerID; return &v }(), + DNSProviderID: mustProviderID(providerID), } conf, err := GenerateConfig( @@ -284,7 +314,7 @@ func TestGenerateConfig_MultiCredential_ZeroSSL_Issuer(t *testing.T) { Enabled: true, DomainNames: "*.zerossl-test.com", DNSProvider: &models.DNSProvider{ID: providerID, ProviderType: "cloudflare"}, - DNSProviderID: func() *uint { v := providerID; return &v }(), + DNSProviderID: mustProviderID(providerID), } conf, err := GenerateConfig( @@ -337,7 +367,7 @@ func TestGenerateConfig_MultiCredential_BothIssuers(t *testing.T) { Enabled: true, DomainNames: "*.both-test.com", DNSProvider: &models.DNSProvider{ID: providerID, ProviderType: "cloudflare"}, - DNSProviderID: func() *uint { v := providerID; return &v }(), + DNSProviderID: mustProviderID(providerID), } conf, err := GenerateConfig( @@ -394,7 +424,7 @@ func TestGenerateConfig_MultiCredential_ACMEStaging(t *testing.T) { Enabled: true, DomainNames: "*.staging-test.com", DNSProvider: &models.DNSProvider{ID: providerID, ProviderType: "cloudflare"}, - DNSProviderID: func() *uint { v := providerID; return &v }(), + DNSProviderID: mustProviderID(providerID), } conf, err := GenerateConfig( @@ -449,7 +479,7 @@ func TestGenerateConfig_MultiCredential_NoMatchingDomains(t *testing.T) { Enabled: true, DomainNames: "*.actual.com", DNSProvider: &models.DNSProvider{ID: providerID, ProviderType: "cloudflare"}, - DNSProviderID: func() *uint { v := providerID; return &v }(), + DNSProviderID: mustProviderID(providerID), } conf, err := GenerateConfig( @@ -496,7 +526,7 @@ func TestGenerateConfig_MultiCredential_ProviderTypeNotFound(t *testing.T) { Enabled: true, DomainNames: "*.unknown-provider.com", DNSProvider: &models.DNSProvider{ID: providerID, ProviderType: "nonexistent_provider"}, - DNSProviderID: func() *uint { v := providerID; return &v }(), + DNSProviderID: mustProviderID(providerID), } conf, err := GenerateConfig( @@ -525,3 +555,338 @@ func TestGenerateConfig_MultiCredential_ProviderTypeNotFound(t *testing.T) { require.NoError(t, err) require.NotNil(t, conf) } + +func TestGenerateConfig_MultiCredential_SupportsMultiCredential_UsesZoneConfigAndStagingBothIssuers(t *testing.T) { + if err := dnsprovider.Global().Register(&multiCredTestProvider{}); err == nil { + t.Cleanup(func() { dnsprovider.Global().Unregister("testmulti") }) + } + + providerID := uint(16) + host := models.ProxyHost{ + Enabled: true, + DomainNames: "*.z1.com,z1.com", + DNSProvider: &models.DNSProvider{ID: providerID, ProviderType: "testmulti"}, + DNSProviderID: mustProviderID(providerID), + } + + conf, err := GenerateConfig( + []models.ProxyHost{host}, + t.TempDir(), + "acme@example.com", + "", + "both", + true, + false, false, false, false, + "", + nil, + nil, + nil, + &models.SecurityConfig{}, + []DNSProviderConfig{{ + ID: providerID, + ProviderType: "testmulti", + UseMultiCredentials: true, + ZoneCredentials: map[string]map[string]string{ + "z1.com": {"token": "tok-z1"}, + }, + }}, + ) + require.NoError(t, err) + require.NotNil(t, conf) + require.NotNil(t, conf.Apps.TLS) + require.NotNil(t, conf.Apps.TLS.Automation) + require.NotEmpty(t, conf.Apps.TLS.Automation.Policies) + + var foundCA string + var foundProvider map[string]any + for _, p := range conf.Apps.TLS.Automation.Policies { + if p == nil { + continue + } + for _, it := range p.IssuersRaw { + issuer, ok := it.(map[string]any) + if !ok || issuer["module"] != "acme" { + continue + } + if ca, ok := issuer["ca"].(string); ok { + foundCA = ca + } + ch, _ := issuer["challenges"].(map[string]any) + dnsCh, _ := ch["dns"].(map[string]any) + prov, _ := dnsCh["provider"].(map[string]any) + foundProvider = prov + break + } + if foundProvider != nil { + break + } + } + + require.Equal(t, "https://acme-staging-v02.api.letsencrypt.org/directory", foundCA) + require.NotNil(t, foundProvider) + require.Equal(t, "z1.com", foundProvider["zone"], "expected zone-specific provider config") +} + +func TestGenerateConfig_DNSChallenge_SingleCredential_BothIssuers_ACMEStaging(t *testing.T) { + providerID := uint(17) + host := models.ProxyHost{ + Enabled: true, + DomainNames: "*.both-staging.com", + DNSProvider: &models.DNSProvider{ID: providerID, ProviderType: "cloudflare"}, + DNSProviderID: mustProviderID(providerID), + } + + conf, err := GenerateConfig( + []models.ProxyHost{host}, + t.TempDir(), + "acme@example.com", + "", + "both", + true, + false, false, false, false, + "", + nil, + nil, + nil, + &models.SecurityConfig{}, + []DNSProviderConfig{{ + ID: providerID, + ProviderType: "cloudflare", + PropagationTimeout: 1, + Credentials: map[string]string{"api_token": "tok"}, + }}, + ) + require.NoError(t, err) + require.NotNil(t, conf) + + var foundIssuer map[string]any + for _, p := range conf.Apps.TLS.Automation.Policies { + if p == nil { + continue + } + for _, s := range p.Subjects { + if s != "*.both-staging.com" { + continue + } + for _, it := range p.IssuersRaw { + if m, ok := it.(map[string]any); ok && m["module"] == "acme" { + foundIssuer = m + break + } + } + } + if foundIssuer != nil { + break + } + } + + require.NotNil(t, foundIssuer) + require.Equal(t, "https://acme-staging-v02.api.letsencrypt.org/directory", foundIssuer["ca"]) +} + +func TestGenerateConfig_DNSChallenge_SingleCredential_ProviderTypeNotFound_SkipsPolicy(t *testing.T) { + providerID := uint(18) + host := models.ProxyHost{ + Enabled: true, + DomainNames: "*.missing-registry.com", + DNSProvider: &models.DNSProvider{ID: providerID, ProviderType: "not_registered"}, + DNSProviderID: mustProviderID(providerID), + } + + conf, err := GenerateConfig( + []models.ProxyHost{host}, + t.TempDir(), + "acme@example.com", + "", + "letsencrypt", + false, + false, false, false, false, + "", + nil, + nil, + nil, + &models.SecurityConfig{}, + []DNSProviderConfig{{ + ID: providerID, + ProviderType: "not_registered", + PropagationTimeout: 1, + Credentials: map[string]string{"token": "x"}, + }}, + ) + require.NoError(t, err) + require.NotNil(t, conf) +} + +func TestGenerateConfig_DefaultPolicy_LetsEncrypt_StagingCA(t *testing.T) { + host := models.ProxyHost{Enabled: true, DomainNames: "192.0.2.1"} + + conf, err := GenerateConfig( + []models.ProxyHost{host}, + t.TempDir(), + "acme@example.com", + "", + "letsencrypt", + true, + false, false, false, false, + "", + nil, + nil, + nil, + &models.SecurityConfig{}, + nil, + ) + require.NoError(t, err) + require.NotNil(t, conf) + require.NotNil(t, conf.Apps.TLS) + require.NotNil(t, conf.Apps.TLS.Automation) + require.NotEmpty(t, conf.Apps.TLS.Automation.Policies) + + found := false + for _, p := range conf.Apps.TLS.Automation.Policies { + if p == nil { + continue + } + for _, it := range p.IssuersRaw { + if m, ok := it.(map[string]any); ok && m["module"] == "acme" { + require.Equal(t, "https://acme-staging-v02.api.letsencrypt.org/directory", m["ca"]) + found = true + break + } + } + if found { + break + } + } + require.True(t, found) +} + +func TestGenerateConfig_DefaultPolicy_ZeroSSL_Issuer(t *testing.T) { + host := models.ProxyHost{Enabled: true, DomainNames: "192.0.2.2"} + + conf, err := GenerateConfig( + []models.ProxyHost{host}, + t.TempDir(), + "acme@example.com", + "", + "zerossl", + false, + false, false, false, false, + "", + nil, + nil, + nil, + &models.SecurityConfig{}, + nil, + ) + require.NoError(t, err) + require.NotNil(t, conf) + require.NotNil(t, conf.Apps.TLS) + require.NotNil(t, conf.Apps.TLS.Automation) + + found := false + for _, p := range conf.Apps.TLS.Automation.Policies { + if p == nil { + continue + } + for _, it := range p.IssuersRaw { + if m, ok := it.(map[string]any); ok && m["module"] == "zerossl" { + found = true + break + } + } + if found { + break + } + } + require.True(t, found) +} + +func TestGenerateConfig_DefaultPolicy_BothIssuers_StagingCA(t *testing.T) { + host := models.ProxyHost{Enabled: true, DomainNames: "192.0.2.3"} + + conf, err := GenerateConfig( + []models.ProxyHost{host}, + t.TempDir(), + "acme@example.com", + "", + "", + true, + false, false, false, false, + "", + nil, + nil, + nil, + &models.SecurityConfig{}, + nil, + ) + require.NoError(t, err) + require.NotNil(t, conf) + + caFound := false + zeroFound := false + for _, p := range conf.Apps.TLS.Automation.Policies { + if p == nil { + continue + } + for _, it := range p.IssuersRaw { + m, ok := it.(map[string]any) + if !ok { + continue + } + switch m["module"] { + case "acme": + if m["ca"] == "https://acme-staging-v02.api.letsencrypt.org/directory" { + caFound = true + } + case "zerossl": + zeroFound = true + } + } + } + require.True(t, caFound) + require.True(t, zeroFound) +} + +func TestGenerateConfig_IPSubjects_InitializesTLSAppAndAutomation(t *testing.T) { + providerID := uint(19) + host := models.ProxyHost{ + Enabled: true, + UUID: "ip-host", + DomainNames: "1.2.3.4", + ForwardHost: "app", + ForwardPort: 8080, + DNSProvider: &models.DNSProvider{ID: providerID, ProviderType: "cloudflare"}, + } + + conf, err := GenerateConfig( + []models.ProxyHost{host}, + t.TempDir(), + "", + "", + "", + false, + false, false, false, false, + "", + nil, + nil, + nil, + &models.SecurityConfig{}, + nil, + ) + require.NoError(t, err) + require.NotNil(t, conf) + require.NotNil(t, conf.Apps.TLS) + require.NotNil(t, conf.Apps.TLS.Automation) + require.NotEmpty(t, conf.Apps.TLS.Automation.Policies) +} + +func TestGetAccessLogPath_DockerEnv_UsesCrowdSecPath(t *testing.T) { + const dockerMarker = "/.dockerenv" + if err := os.WriteFile(dockerMarker, []byte("test"), 0o600); err != nil { + t.Skipf("cannot create %s: %v", dockerMarker, err) + } + t.Cleanup(func() { _ = os.Remove(dockerMarker) }) + + path := getAccessLogPath(t.TempDir(), false) + require.Equal(t, "/var/log/caddy/access.log", path) +} diff --git a/docs/plans/current_spec.md b/docs/plans/current_spec.md index fac4f1f4..7baf2719 100644 --- a/docs/plans/current_spec.md +++ b/docs/plans/current_spec.md @@ -1,2522 +1,297 @@ -# Plan: Add CHARON_ENCRYPTION_KEY to Docker Compose Files and README +# Patch Coverage Remediation Plan (Codecov) — Backend -**Created:** January 8, 2026 -**Status:** 🟢 READY FOR IMPLEMENTATION -**Priority:** P1 - Security Enhancement +**Created:** 2026-01-09 ---- +## Goal -## Overview +Restore **Codecov patch coverage** to green by ensuring **100% of modified lines** are executed by tests. -Add the `CHARON_ENCRYPTION_KEY` environment variable to all Docker Compose files and update README.md documentation examples. This variable is required for encrypting sensitive data at rest. +- **Codecov patch coverage:** 92.93866% +- **Reported missing patch lines:** ~99 ---- +Hard constraints: +- Do **not** lower Codecov thresholds. +- Fix with **targeted tests** (only add micro “test hooks” if absolutely unavoidable). -## Files to Modify +## Scope (two workstreams; both in-scope) -| # | File Path | Has Charon Service | Needs Update | -|---|-----------|-------------------|--------------| -| 1 | `.docker/compose/docker-compose.yml` | ✅ Yes | ✅ Yes | -| 2 | `.docker/compose/docker-compose.dev.yml` | ✅ Yes (as `app`) | ✅ Yes | -| 3 | `.docker/compose/docker-compose.local.yml` | ✅ Yes | ✅ Yes | -| 4 | `.docker/compose/docker-compose.remote.yml` | ❌ No (docker-socket-proxy only) | ❌ No | -| 5 | `docker-compose.test.yml` (root) | ✅ Yes | ✅ Yes | -| 6 | `README.md` | N/A (documentation) | ✅ Yes | -| 7 | `.docker/compose/README.md` | N/A (documentation) | ❌ No (no env examples) | +### Workstream A (required): Patch coverage fixes for 10 backend files -**Note:** `docker-compose.remote.yml` only contains the `docker-socket-proxy` service for remote Docker socket access. It does NOT run Charon itself, so no environment variable is needed. +This workstream fixes Codecov **patch coverage** by adding targeted tests (and only minimal seams if unavoidable) for the following backend files: ---- +1. backend/internal/api/handlers/plugin_handler.go +2. backend/internal/api/handlers/encryption_handler.go +3. backend/internal/api/handlers/credential_handler.go +4. backend/internal/api/handlers/settings_handler.go +5. backend/internal/api/handlers/crowdsec_handler.go +6. backend/internal/api/handlers/proxy_host_handler.go +7. backend/internal/api/handlers/security_handler.go +8. backend/internal/caddy/config.go +9. backend/internal/caddy/client.go +10. backend/internal/api/handlers/testdb.go (special: ignored in `.codecov.yml` but may still show up) -## Detailed Changes +### Workstream B (required): Prevention updates (instructions + agent files) -### 1. `.docker/compose/docker-compose.yml` +This workstream updates the following files to ensure future production-code changes include the patch-coverage triage + tests needed to keep Codecov patch coverage green: -**Current environment section (lines 10-35):** -```yaml - environment: - - CHARON_ENV=production # CHARON_ preferred; CPM_ values still supported - - TZ=UTC # Set timezone (e.g., America/New_York) - - CHARON_HTTP_PORT=8080 - - CHARON_DB_PATH=/app/data/charon.db - ... -``` - -**Insert after:** `- TZ=UTC # Set timezone (e.g., America/New_York)` (line 12) - -**Snippet to add:** -```yaml - # Generate with: openssl rand -base64 32 - - CHARON_ENCRYPTION_KEY=your-32-byte-base64-key-here -``` - -**Full context for edit:** -```yaml - environment: - - CHARON_ENV=production # CHARON_ preferred; CPM_ values still supported - - TZ=UTC # Set timezone (e.g., America/New_York) - # Generate with: openssl rand -base64 32 - - CHARON_ENCRYPTION_KEY=your-32-byte-base64-key-here - - CHARON_HTTP_PORT=8080 -``` - ---- - -### 2. `.docker/compose/docker-compose.dev.yml` - -**Current environment section (lines 13-25):** -```yaml - environment: - - CHARON_ENV=development - - CPM_ENV=development - - CHARON_HTTP_PORT=8080 - - CPM_HTTP_PORT=80 - - CHARON_DB_PATH=/app/data/charon.db - ... -``` - -**Insert after:** `- CPM_HTTP_PORT=80` (line 17) - -**Snippet to add:** -```yaml - # Generate with: openssl rand -base64 32 - - CHARON_ENCRYPTION_KEY=your-32-byte-base64-key-here -``` - -**Full context for edit:** -```yaml - environment: - - CHARON_ENV=development - - CPM_ENV=development - - CHARON_HTTP_PORT=8080 - - CPM_HTTP_PORT=80 - # Generate with: openssl rand -base64 32 - - CHARON_ENCRYPTION_KEY=your-32-byte-base64-key-here - - CHARON_DB_PATH=/app/data/charon.db -``` - ---- - -### 3. `.docker/compose/docker-compose.local.yml` - -**Current environment section (lines 14-26):** -```yaml - environment: - - CHARON_ENV=development - - CHARON_DEBUG=1 - - TZ=America/New_York - - CHARON_HTTP_PORT=8080 - - CHARON_DB_PATH=/app/data/charon.db - ... -``` - -**Insert after:** `- TZ=America/New_York` (line 17) - -**Snippet to add:** -```yaml - # Generate with: openssl rand -base64 32 - - CHARON_ENCRYPTION_KEY=your-32-byte-base64-key-here -``` - -**Full context for edit:** -```yaml - environment: - - CHARON_ENV=development - - CHARON_DEBUG=1 - - TZ=America/New_York - # Generate with: openssl rand -base64 32 - - CHARON_ENCRYPTION_KEY=your-32-byte-base64-key-here - - CHARON_HTTP_PORT=8080 -``` - ---- - -### 4. `docker-compose.test.yml` (project root) - -**Current environment section (lines 14-28):** -```yaml - environment: - - CHARON_ENV=development - - CHARON_DEBUG=1 - - TZ=America/New_York - - CHARON_HTTP_PORT=8080 - - CHARON_DB_PATH=/app/data/charon.db - ... -``` - -**Insert after:** `- TZ=America/New_York` (line 17) - -**Snippet to add:** -```yaml - # Generate with: openssl rand -base64 32 - - CHARON_ENCRYPTION_KEY=your-32-byte-base64-key-here -``` - -**Full context for edit:** -```yaml - environment: - - CHARON_ENV=development - - CHARON_DEBUG=1 - - TZ=America/New_York - # Generate with: openssl rand -base64 32 - - CHARON_ENCRYPTION_KEY=your-32-byte-base64-key-here - - CHARON_HTTP_PORT=8080 -``` - ---- - -### 5. `README.md` - Docker Compose Example - -**Location:** Around lines 107-123 (Quick Start section) - -**Current:** -```yaml -services: - charon: - image: ghcr.io/wikid82/charon:latest - container_name: charon - restart: unless-stopped - ports: - - "80:80" - - "443:443" - - "443:443/udp" - - "8080:8080" - volumes: - - ./charon-data:/app/data - - /var/run/docker.sock:/var/run/docker.sock:ro - environment: - - CHARON_ENV=production - -``` - -**Replace with:** -```yaml -services: - charon: - image: ghcr.io/wikid82/charon:latest - container_name: charon - restart: unless-stopped - ports: - - "80:80" - - "443:443" - - "443:443/udp" - - "8080:8080" - volumes: - - ./charon-data:/app/data - - /var/run/docker.sock:/var/run/docker.sock:ro - environment: - - CHARON_ENV=production - # Generate with: openssl rand -base64 32 - - CHARON_ENCRYPTION_KEY=your-32-byte-base64-key-here - -``` - ---- - -### 6. `README.md` - Docker Run One-Liner - -**Location:** Around lines 130-141 (Docker Run section) - -**Current:** -```bash -docker run -d \ - --name charon \ - -p 80:80 \ - -p 443:443 \ - -p 443:443/udp \ - -p 8080:8080 \ - -v ./charon-data:/app/data \ - -v /var/run/docker.sock:/var/run/docker.sock:ro \ - -e CHARON_ENV=production \ - ghcr.io/wikid82/charon:latest -``` - -**Replace with:** -```bash -docker run -d \ - --name charon \ - -p 80:80 \ - -p 443:443 \ - -p 443:443/udp \ - -p 8080:8080 \ - -v ./charon-data:/app/data \ - -v /var/run/docker.sock:/var/run/docker.sock:ro \ - -e CHARON_ENV=production \ - -e CHARON_ENCRYPTION_KEY=your-32-byte-base64-key-here \ - ghcr.io/wikid82/charon:latest -``` - -**Note:** For the one-liner, we cannot include the comment inline. Users can generate the key using `openssl rand -base64 32` as shown in the compose example above it. - ---- - -## Files NOT Modified (with Justification) - -### `.docker/compose/docker-compose.remote.yml` -This file contains ONLY the `docker-socket-proxy` service using the `alpine/socat` image. It is deployed on **remote servers** to expose their Docker socket to Charon. Charon itself does NOT run from this compose file, so no `CHARON_ENCRYPTION_KEY` is needed. - -**File contents:** -```yaml -services: - docker-socket-proxy: - image: alpine/socat - container_name: docker-socket-proxy - # ... no environment section for Charon -``` - -### `.docker/compose/README.md` -This file contains usage documentation for compose files. It does NOT include environment variable examples or configuration snippets, so no update is needed. - ---- - -## Summary Table - -| File | Line to Insert After | Snippet | -|------|---------------------|---------| -| `.docker/compose/docker-compose.yml` | `- TZ=UTC # Set timezone...` | 2-line block with comment | -| `.docker/compose/docker-compose.dev.yml` | `- CPM_HTTP_PORT=80` | 2-line block with comment | -| `.docker/compose/docker-compose.local.yml` | `- TZ=America/New_York` | 2-line block with comment | -| `docker-compose.test.yml` | `- TZ=America/New_York` | 2-line block with comment | -| `README.md` (compose example) | `- CHARON_ENV=production` | 2-line block with comment | -| `README.md` (docker run) | `-e CHARON_ENV=production \` | Single `-e` flag line | - ---- - -## Implementation Order - -1. `.docker/compose/docker-compose.yml` — Primary production file -2. `.docker/compose/docker-compose.dev.yml` — Development override -3. `.docker/compose/docker-compose.local.yml` — Local development -4. `docker-compose.test.yml` — Test environment (project root) -5. `README.md` — User-facing documentation (both examples) - ---- - -## Validation Checklist - -After implementation, verify: - -- [ ] All 4 compose files have the `CHARON_ENCRYPTION_KEY` variable -- [ ] All have the comment `# Generate with: openssl rand -base64 32` above the variable -- [ ] README.md docker-compose example includes the variable with comment -- [ ] README.md docker run example includes `-e CHARON_ENCRYPTION_KEY=...` -- [ ] YAML syntax is valid (run `docker compose -f config` on each file) -- [ ] Variable placement is consistent across all files (after TZ or early env vars) - ---- - -## Exact Edit Snippets for Implementation Agent - -### Edit 1: `.docker/compose/docker-compose.yml` - -**Find this block:** -```yaml - environment: - - CHARON_ENV=production # CHARON_ preferred; CPM_ values still supported - - TZ=UTC # Set timezone (e.g., America/New_York) - - CHARON_HTTP_PORT=8080 -``` - -**Replace with:** -```yaml - environment: - - CHARON_ENV=production # CHARON_ preferred; CPM_ values still supported - - TZ=UTC # Set timezone (e.g., America/New_York) - # Generate with: openssl rand -base64 32 - - CHARON_ENCRYPTION_KEY=your-32-byte-base64-key-here - - CHARON_HTTP_PORT=8080 -``` - ---- - -### Edit 2: `.docker/compose/docker-compose.dev.yml` - -**Find this block:** -```yaml - environment: - - CHARON_ENV=development - - CPM_ENV=development - - CHARON_HTTP_PORT=8080 - - CPM_HTTP_PORT=80 - - CHARON_DB_PATH=/app/data/charon.db -``` - -**Replace with:** -```yaml - environment: - - CHARON_ENV=development - - CPM_ENV=development - - CHARON_HTTP_PORT=8080 - - CPM_HTTP_PORT=80 - # Generate with: openssl rand -base64 32 - - CHARON_ENCRYPTION_KEY=your-32-byte-base64-key-here - - CHARON_DB_PATH=/app/data/charon.db -``` - ---- - -### Edit 3: `.docker/compose/docker-compose.local.yml` - -**Find this block:** -```yaml - environment: - - CHARON_ENV=development - - CHARON_DEBUG=1 - - TZ=America/New_York - - CHARON_HTTP_PORT=8080 -``` - -**Replace with:** -```yaml - environment: - - CHARON_ENV=development - - CHARON_DEBUG=1 - - TZ=America/New_York - # Generate with: openssl rand -base64 32 - - CHARON_ENCRYPTION_KEY=your-32-byte-base64-key-here - - CHARON_HTTP_PORT=8080 -``` - ---- - -### Edit 4: `docker-compose.test.yml` (project root) - -**Find this block:** -```yaml - environment: - - CHARON_ENV=development - - CHARON_DEBUG=1 - - TZ=America/New_York - - CHARON_HTTP_PORT=8080 -``` - -**Replace with:** -```yaml - environment: - - CHARON_ENV=development - - CHARON_DEBUG=1 - - TZ=America/New_York - # Generate with: openssl rand -base64 32 - - CHARON_ENCRYPTION_KEY=your-32-byte-base64-key-here - - CHARON_HTTP_PORT=8080 -``` - ---- - -### Edit 5: `README.md` - Docker Compose Example - -**Find this block:** -```yaml - environment: - - CHARON_ENV=production - -``` - -**Replace with:** -```yaml - environment: - - CHARON_ENV=production - # Generate with: openssl rand -base64 32 - - CHARON_ENCRYPTION_KEY=your-32-byte-base64-key-here - -``` - ---- - -### Edit 6: `README.md` - Docker Run One-Liner - -**Find this block:** -```bash - -e CHARON_ENV=production \ - ghcr.io/wikid82/charon:latest -``` - -**Replace with:** -```bash - -e CHARON_ENV=production \ - -e CHARON_ENCRYPTION_KEY=your-32-byte-base64-key-here \ - ghcr.io/wikid82/charon:latest -``` - ---- - -## Ready for Implementation - -This plan is complete. An implementation agent can now execute these changes using the exact edit snippets provided above - -### Remediation Phases - -**Phase 0: Pre-Implementation Verification (NEW - P0)** -- Capture exact error messages with verbose test output -- Verify package structure at `backend/pkg/dnsprovider/builtin/` -- Confirm `init()` function exists in `builtin.go` -- Check current test imports for builtin package -- Establish coverage baseline - -**Phase 1: DNS Provider Registry Initialization (P0)** -- **Option 1A (TRY FIRST):** Add blank import `_ "github.com/Wikid82/charon/backend/pkg/dnsprovider/builtin"` to test files -- **Option 1B (FALLBACK):** Create test helper if blank imports fail -- Leverage existing `init()` in builtin package (already registers all providers) -- Update 4 test files with correct import path - -**Phase 2: Fix Credential Field Names (P1)** -- Update `TestAllProviderTypes` field names (3 providers) -- Update `TestDNSProviderService_TestCredentials_AllProviders` (3 providers) -- Update `TestDNSProviderService_List_OrderByDefault` (hetzner) -- Update `TestDNSProviderService_AuditLogging_Delete` (digitalocean) - -**Phase 3: Security Handler Input Validation (P2)** -- Add IP format validation (`net.ParseIP`, `net.ParseCIDR`) -- Add action enum validation (block, allow, captcha) -- Add string sanitization (remove control characters, enforce length limits) -- Return 400 for invalid inputs BEFORE database operations -- Preserve parameterized queries for valid inputs - -**Phase 4: Security Settings Database Override Fix (P1)** -- Fix `GetStatus` handler to check `settings` table for overrides -- Update handler to properly fallback when `security_configs` record not found -- Ensure settings-based overrides work for WAF, Rate Limit, ACL, and CrowdSec -- Fix 5 failing tests +- .github/instructions/testing.instructions.md +- .github/instructions/copilot-instructions.md +- .github/instructions/taming-copilot.instructions.md +- .github/agents/Backend_Dev.agent.md +- .github/agents/QA_Security.agent.md +- .github/agents/Supervisor.agent.md +- .github/agents/Frontend_Dev.agent.md (only if frontend changes are involved; otherwise leave untouched) -**Phase 5: Certificate Deletion Race Condition Fix (P2)** -- Add transaction handling or retry logic for certificate deletion -- Prevent database lock errors during backup + delete operations -- Fix 1 failing test +Non-goals: +- No unrelated refactors. +- No new integration tests requiring external services. -**Phase 6: Frontend Test Timeout Fix (P2)** -- Split `waitFor` assertions in LiveLogViewer test -- Use `findBy` queries for cleaner async handling -- Increase test timeout if needed -- Fix 1 failing test +## Missing Files Table (copy from Codecov patch report) -**Phase 7: Validation (P0)** -- Run individual test suites for each fix -- Full test suite validation -- Coverage verification (target ≥85%) +Codecov “Patch” view is the source of truth. Paste the **exact missing/partial line ranges** into this table. -### Detailed Remediation Plan +Note: Codecov “Patch” view is the source of truth. This table is only for tracking what you copied from Codecov and what test you’ll add. -**📄 Full plan available in this file below** (scroll down for complete analysis) +| File | Missing patch line ranges (Codecov) | Partial patch line ranges (Codecov) | Primary test strategy | +|------|-------------------------------------|-------------------------------------|-----------------------| +| backend/internal/api/handlers/plugin_handler.go | (paste) | (paste) | Gin handler tests (httptest) to hit error + warn-but-200 branches | +| backend/internal/api/handlers/encryption_handler.go | (paste) | (paste) | Handler tests for rotate/validate error + success branches | +| backend/internal/api/handlers/credential_handler.go | (paste) | (paste) | Handler tests for parse/notfound/service-error branches | +| backend/internal/api/handlers/settings_handler.go | (paste) | (paste) | Handler tests for URL test/SSRF + “reachable=false” mapping | +| backend/internal/api/handlers/crowdsec_handler.go | (paste) | (paste) | Handler tests using httptest.Server to force non-200/non-JSON branches | +| backend/internal/api/handlers/proxy_host_handler.go | (paste) | (paste) | Handler tests for JSON type coercion and invalid payload validation | +| backend/internal/api/handlers/security_handler.go | (paste) | (paste) | Handler tests for effective-status branches + validation branches | +| backend/internal/caddy/config.go | (paste) | (paste) | Unit tests for helper branches + config generation edge cases | +| backend/internal/caddy/client.go | (paste) | (paste) | Unit tests for HTTP non-200 + endpoint/parse branches | +| backend/internal/api/handlers/testdb.go | (paste) | (paste) | Prefer moving helpers into *_test.go; else fix ignore/path mismatch | ---- +## Why patch coverage missed (common patterns) -## Section 1: ARCHIVED - React 19 Production Error Resolution Plan +Patch coverage misses are usually caused by newly-changed lines being in branches that existing tests don’t naturally hit: -**Status:** 🔴 CRITICAL - Production Error Confirmed -**Created:** January 7, 2026 -**Priority:** P0 (Blocking) -**Issue:** React 19 production build fails with "Cannot set properties of undefined (setting 'Activity')" in lucide-react +- Error-only branches (DB errors, JSON decode failures, upstream non-200) +- “Warn but succeed” flows (HTTP 200 with a warning field) +- JSON type coercion (e.g., `map[string]any` decoding numbers vs strings) +- Env-driven branches (missing `t.Setenv` in tests) +- Codecov ignore/path normalization mismatch (repo-relative path vs module path in coverprofile) -### Evidence-Based Investigation (Corrected) +## Handling partial (yellow) patch lines -#### npm Registry Findings +Partial patch lines (yellow) usually mean the line executed, but **not all branches associated with that line** did. -**lucide-react Latest Version Check:** -```bash -Latest version: 0.562.0 (currently installed) -Peer Dependencies: ^16.5.1 || ^17.0.0 || ^18.0.0 || ^19.0.0 -Recent versions: 0.554.0 through 0.562.0 -``` - -**Critical Finding:** No newer version of lucide-react exists beyond 0.562.0 that might have React 19 fixes. - -#### Current Installation State - -**Verified Installed Versions (Jan 7, 2026):** -- React: `19.2.3` (latest) -- React-DOM: `19.2.3` (latest) -- lucide-react: `0.562.0` (latest) -- All dependencies claim React 19 support in peerDependencies - -**Production Build Test:** -- ✅ Build succeeds without errors -- ✅ No compile-time warnings -- ⚠️ **Runtime error confirmed by user in browser console** - -#### Timeline: When React 19 Was Introduced - -**CONFIRMED:** React 19 was introduced **November 20, 2025** via automatic Renovate bot update: -- Commit: `c60beec` - "fix(deps): update react monorepo to v19" -- Previous version: React 18.3.1 -- Project age at upgrade: 1 day old -- **Time since upgrade: 48 days (6+ weeks)** - -#### Why User Didn't See Error Until Now - -**CRITICAL INSIGHT:** This is likely the **FIRST time user has tested a production build** in a real browser. - -**Evidence:** -1. **Development mode (npm run dev) hides the error** - React DevTools and HMR mask the issue -2. **Fresh Docker build with --no-cache** eliminates cache as root cause -3. **User has active production error RIGHT NOW** with fresh build -4. **No prior production testing documented** - all testing was in dev mode - -**Root Cause:** lucide-react 0.562.0 has a module bundling issue with React 19 that only manifests in **production builds** where code is minified and tree-shaken. - -The error "Cannot set properties of undefined (setting 'Activity')" indicates lucide-react is trying to register icon components on an undefined object during module initialization in production mode. - -### Alternative Icon Library Research - -#### Current: Lucide React -- **Version:** 0.562.0 -- **Peer Deps:** `^16.5.1 || ^17.0.0 || ^18.0.0 || ^19.0.0` ✅ React 19 compatible -- **Bundle Size:** ~50KB (tree-shakeable) -- **Icons Used:** 50+ icons across 20+ components -- **Status:** **VERIFIED WORKING** with React 19.2.3 - -**Icons in Use:** -- Activity, AlertCircle, AlertTriangle, Archive, Bell, CheckCircle, CheckCircle2 -- ChevronLeft, ChevronRight, Clock, Cloud, Copy, Download, Edit2, ExternalLink -- FileCode2, FileKey, FileText, Filter, Gauge, Globe, Info, Key, LayoutGrid -- LayoutList, Loader2, Lock, Mail, Package, Pencil, Plus, RefreshCw, RotateCcw -- Save, ScrollText, Send, Server, Settings, Shield, ShieldAlert, ShieldCheck -- Sparkles, TestTube2, Trash2, User, UserCheck, X, XCircle - -#### Option 1: Heroicons (by Tailwind Labs) -- **Version:** 2.2.0 -- **Peer Deps:** `>= 16 || ^19.0.0-rc` ✅ React 19 compatible -- **Bundle Size:** ~30KB (smaller than lucide-react) -- **Icon Coverage:** ⚠️ **MISSING CRITICAL ICONS** - - Missing: `Activity`, `RotateCcw`, `TestTube2`, `Gauge`, `ScrollText`, `Sparkles` - - Have equivalents: Shield, Server, Mail, User, Bell, Key, Globe, etc. -- **Migration Effort:** HIGH - Need to find replacements for 6+ icons -- **Verdict:** ❌ Incomplete icon coverage - -#### Option 2: React Icons (Aggregator) -- **Version:** 5.5.0 -- **Peer Deps:** `*` (accepts any React version) ✅ React 19 compatible -- **Bundle Size:** ~100KB+ (includes multiple icon sets) -- **Icon Coverage:** ✅ Comprehensive (includes Feather, Font Awesome, Lucide, etc.) -- **Migration Effort:** MEDIUM - Import from different sub-packages -- **Cons:** Larger bundle, inconsistent design across sets -- **Verdict:** ⚠️ Overkill for our needs - -#### Option 3: Radix Icons -- **Version:** 1.3.2 -- **Peer Deps:** `^16.x || ^17.x || ^18.x || ^19.0.0 || ^19.0.0-rc` ✅ React 19 compatible -- **Bundle Size:** ~5KB (very lightweight!) -- **Icon Coverage:** ❌ **SEVERELY LIMITED** - - Only ~300 icons vs lucide-react's 1400+ - - Missing most icons we need: Activity, Gauge, TestTube2, Sparkles, RotateCcw, etc. -- **Integration:** ✅ Already using Radix UI components -- **Verdict:** ❌ Too limited for our needs - -#### Option 4: Phosphor Icons -- **Version:** 1.4.1 (⚠️ Last updated 2 years ago) -- **Peer Deps:** Not clearly defined -- **Bundle Size:** ~60KB -- **Icon Coverage:** ✅ Comprehensive -- **React 19 Compatibility:** ⚠️ **UNVERIFIED** (package appears unmaintained) -- **Verdict:** ❌ Stale package, risky for React 19 - -#### Option 5: Keep lucide-react (RECOMMENDED) -- **Version:** 0.562.0 -- **Status:** ✅ **VERIFIED WORKING** with React 19.2.3 -- **Evidence:** CHANGELOG confirms no runtime errors, all tests passing -- **Action:** No library change needed -- **Fix Required:** None - issue was user-side (cache) - -### Recommended Fix Strategy - -#### ✅ OPTION A: User-Side Cache Clear (IMMEDIATE) - -**Verdict:** Issue already resolved via cache clear. - -**Steps:** -1. Hard refresh browser (Ctrl+Shift+R or Cmd+Shift+R) -2. Clear browser cache completely -3. Docker: `docker compose down && docker compose up -d --build` -4. Verify production build works - -**Risk:** None - already verified working -**Effort:** 5 minutes -**Status:** ✅ COMPLETE per user confirmation - ---- - -#### ⚠️ OPTION B: Downgrade to React 18 (USER-REQUESTED FALLBACK) - -**Use Case:** If cache clear doesn't work OR if user wants to revert for stability. - -**Specific Versions:** -```json -{ - "react": "^18.3.1", - "react-dom": "^18.3.1", - "@types/react": "^18.3.12", - "@types/react-dom": "^18.3.1" -} -``` - -**Steps:** -1. Edit `frontend/package.json`: - ```bash - cd frontend - npm install react@18.3.1 react-dom@18.3.1 @types/react@18.3.12 @types/react-dom@18.3.1 --save-exact - ``` - -2. Update `package.json` to lock versions: - ```json - "react": "18.3.1", - "react-dom": "18.3.1" - ``` - -3. Test locally: - ```bash - npm run build - npm run preview - ``` - -4. Test in Docker: - ```bash - docker build --no-cache -t charon:local . - docker compose -f docker-compose.test.yml up -d - ``` - -5. Run full test suite: - ```bash - npm run test:coverage - npm run e2e:test - ``` - -**Compatibility Concerns:** -- ✅ lucide-react@0.562.0 supports React 18 -- ✅ @radix-ui components support React 18 -- ✅ @tanstack/react-query supports React 18 -- ✅ react-router-dom v7 supports React 18 - -**Rollback Procedure:** -```bash -# Create rollback branch -git checkout -b rollback/react-18-downgrade - -# Apply changes -cd frontend -npm install react@18.3.1 react-dom@18.3.1 @types/react@18.3.12 @types/react-dom@18.3.1 --save-exact - -# Test -npm run test:coverage -npm run build - -# Commit -git add frontend/package.json frontend/package-lock.json -git commit -m "fix: downgrade React to 18.3.1 for production stability" -``` - -**Risk Assessment:** -- **HIGH:** React 19 has been stable for 48 days -- **MEDIUM:** Downgrade may introduce new issues -- **LOW:** All dependencies support React 18 - -**Effort:** 30 minutes -**Testing Time:** 1 hour -**Recommendation:** ❌ **NOT RECOMMENDED** - Issue is already resolved - ---- - -#### ❌ OPTION C: Switch Icon Library - -**Verdict:** NOT RECOMMENDED - lucide-react is working correctly. - -**Analysis:** -- Heroicons: Missing 6+ critical icons -- React Icons: Overkill, larger bundle -- Radix Icons: Too limited (only ~300 icons) -- Phosphor: Unmaintained, React 19 compatibility unknown - -**Migration Effort:** 8-12 hours (50+ icons across 20+ files) -**Bundle Impact:** Minimal savings (-20KB max) -**Recommendation:** ❌ **WASTE OF TIME** - lucide-react is verified working - ---- - -### Final Recommendation - -**Action:** ✅ **NO CODE CHANGES NEEDED** - -**Rationale:** -1. React 19.2.3 + lucide-react@0.562.0 is **verified working** -2. Issue was user-side (browser/Docker cache) -3. All 1403 tests passing, production build succeeds -4. Alternative icon libraries are worse (missing icons, larger bundles, or unmaintained) -5. Downgrading React 19 is risky and unnecessary - -**If User Still Sees Errors:** -1. Clear browser cache: `Ctrl+Shift+R` (hard refresh) -2. Rebuild Docker image: `docker compose down && docker build --no-cache -t charon:local . && docker compose up -d` -3. Clear Docker build cache: `docker builder prune -a` -4. Test in incognito/private browsing window - -**Fallback Plan (if cache clear fails):** -- Implement Option B (React 18 downgrade) -- Estimated time: 2 hours including testing -- All dependencies confirmed compatible - -### Answers to User's Questions - -#### Q1: "React 19 was released well before I started work on Charon, so haven't I been using React 19 this whole time? Why all of a sudden are we having this issue?" - -**Answer:** - -No, you were **not** using React 19 from the start. - -- **Project Started:** November 19, 2025 with **React 18.3.1** - - Initial commit (`945b18a`): "feat: Implement User Authentication and Fix Frontend Startup" - - Used React 18.3.1 and React-DOM 18.3.1 - -- **React 19 Upgrade:** November 20, 2025 (next day) - - Commit `c60beec`: "fix(deps): update react monorepo to v19" - - Renovate bot automatically upgraded to React 19.2.0 - - Later updated to React 19.2.3 - -- **Why Failing Now:** - 1. **Vite Code-Splitting Change (Dec 5, 2025):** Added icon chunk splitting in `vite.config.ts` (33 days after React 19 upgrade) - 2. **Docker Cache:** Stale build layers with mismatched React versions - 3. **Browser Cache:** Mixing old React 18 assets with new React 19 code - 4. **Recent Dependency Updates:** lucide-react, Radix UI, TypeScript updates - -**Timeline:** -- Nov 19: Project starts with React 18 -- Nov 20: Auto-upgrade to React 19 (worked fine for 48 days) -- Dec 5: Vite config changed (icon code-splitting added) -- Jan 7: Error reported (likely triggered by cache issues) - -**Why It's Failing Now (Not Earlier):** -- React 19 was working fine for 6 weeks -- Recent Docker rebuild exposed cached layer issues -- Browser cache mixing old and new assets -- The issue is **environmental**, not a code problem - -**Verification:** -- CHANGELOG.md confirms React 19.2.3 + lucide-react@0.562.0 is verified working -- All 1403 tests pass -- Production build succeeds without errors - -#### Q2: "Is there a different option than Lucide that might work better with our project?" - -**Answer:** - -**No** - lucide-react is the best option for this project, and it's **verified working** with React 19. - -**Why lucide-react is the right choice:** - -1. **Verified Working:** CHANGELOG confirms no runtime errors with React 19.2.3 -2. **Best Icon Coverage:** 1400+ icons, we use 50+ unique icons -3. **React 19 Compatible:** Peer dependencies explicitly support React 19 -4. **Tree-Shakeable:** Only bundles icons you import (~50KB for our usage) -5. **Consistent Design:** All icons match visually -6. **Well-Maintained:** Active development, frequent updates - -**Alternatives Evaluated:** - -| Library | React 19 Support | Icon Coverage | Bundle Size | Verdict | -|---------|-----------------|---------------|-------------|---------| -| **Lucide React** | ✅ Yes | ✅ 1400+ icons | ~50KB | ✅ **KEEP** | -| Heroicons | ✅ Yes | ❌ Missing 6+ icons | ~30KB | ❌ Incomplete | -| React Icons | ✅ Yes | ✅ Comprehensive | ~100KB+ | ❌ Too large | -| Radix Icons | ✅ Yes | ❌ Only ~300 icons | ~5KB | ❌ Too limited | -| Phosphor Icons | ⚠️ Unknown | ✅ Comprehensive | ~60KB | ❌ Unmaintained | - -**Heroicons Missing Icons:** -- `Activity` (used in Dashboard, SystemSettings) -- `RotateCcw` (used in Backups) -- `TestTube2` (used in AccessLists) -- `Gauge` (used in RateLimiting) -- `ScrollText` (used in Logs) -- `Sparkles` (used in WafConfig) - -**Migration Effort if Switching:** -- 50+ icon imports across 20+ files -- Find equivalent icons or redesign UI -- Update all icon usages -- Test every page -- **Estimated time:** 8-12 hours -- **Benefit:** None (lucide-react already works) - -**Recommendation:** -- ✅ **KEEP lucide-react@0.562.0** -- ❌ Don't switch libraries -- ❌ Don't downgrade React -- ✅ Clear cache and rebuild - -**The error you experienced was NOT caused by lucide-react or React 19 incompatibility. It was a cache issue that's now resolved.** - ---- - -### Implementation Steps (If Fallback Required) - -**ONLY if user confirms cache clear didn't work:** - -1. **Backup Current State:** - ```bash - git checkout -b backup/react-19-state - git push origin backup/react-19-state - ``` - -2. **Create Downgrade Branch:** - ```bash - git checkout development - git checkout -b fix/react-18-downgrade - ``` - -3. **Downgrade React:** - ```bash - cd frontend - npm install react@18.3.1 react-dom@18.3.1 @types/react@18.3.12 @types/react-dom@18.3.1 --save-exact - ``` - -4. **Test Locally:** - ```bash - npm run test:coverage - npm run build - npm run preview - ``` - -5. **Test Docker Build:** - ```bash - docker build --no-cache -t charon:react18-test . - docker compose -f docker-compose.test.yml up -d - ``` - -6. **Verify All Features:** - - Test login/logout - - Test proxy host creation - - Test certificate management - - Test settings pages - - Test dashboard metrics - -7. **Commit and Push:** - ```bash - git add frontend/package.json frontend/package-lock.json - git commit -m "fix: downgrade React to 18.3.1 for production stability" - git push origin fix/react-18-downgrade - ``` - -8. **Create PR:** - - Title: "fix: downgrade React to 18.3.1 for production stability" - - Description: Link to this plan document - - Request review - -### Testing Checklist - -- [ ] All 1403+ unit tests pass -- [ ] Frontend coverage ≥85% -- [ ] Production build succeeds without warnings -- [ ] Docker image builds successfully -- [ ] Application loads in browser -- [ ] Login/logout works -- [ ] All icon components render correctly -- [ ] No console errors in production -- [ ] No React warnings in development -- [ ] Lighthouse score unchanged (≥90) - -### Monitoring & Verification - -**Post-Implementation:** -1. Monitor browser console for errors -2. Check Docker logs: `docker compose logs -f` -3. Verify Lighthouse performance scores -4. Monitor bundle sizes (should be stable) - -**Success Criteria:** -- ✅ No "Cannot set properties of undefined" errors -- ✅ All tests passing -- ✅ Production build succeeds -- ✅ Application loads without errors -- ✅ Icons render correctly - ---- - -**Status:** ✅ **RESOLVED** - Issue was user-side cache problem -**Next Action:** None required unless user confirms cache clear failed -**Fallback Ready:** React 18 downgrade plan documented above - ---- - -# DETAILED REMEDIATION PLAN: Test Failures - PR #461 (REVISED) - -**Generated**: 2026-01-07 (REVISED: Critical Issues Fixed) -**PR**: #461 - DNS Challenge Support for Wildcard Certificates -**Context**: Multi-credential DNS provider implementation causing test failures across handlers, caddy manager, and services - -**CRITICAL CORRECTIONS:** -- ✅ Fixed incorrect package path: `pkg/dnsprovider/builtin` (NOT `internal/dnsprovider/*`) -- ✅ Simplified registry initialization: Use blank imports (registry already has `init()`) -- ✅ Enhanced security handler validation: Comprehensive IP/action validation + 200/400 handling - -## Complete Test Failure Analysis - -### Category 1: API Handler Failures (476.752s total) - -#### 1.1 Credential Handler Tests (ALL FAILING) -**Files:** -- Test: `/projects/Charon/backend/internal/api/handlers/credential_handler_test.go` -- Handler: `/projects/Charon/backend/internal/api/handlers/credential_handler.go` (EXISTS) -- Service: `/projects/Charon/backend/internal/services/credential_service.go` (EXISTS) - -**Failing Tests:** -- `TestCredentialHandler_Create` (line 82) -- `TestCredentialHandler_List` (line 129) -- `TestCredentialHandler_Get` (line 159) -- `TestCredentialHandler_Update` (line 197) -- `TestCredentialHandler_Delete` (line 234) -- `TestCredentialHandler_Test` (line 260) - -**Root Cause:** -Handler exists but DNS provider registry not initialized during test setup. Error: `"invalid provider type"` - the credential validation runs but fails because provider type "cloudflare" not found in registry. - -**Evidence:** -``` -credential_handler_test.go:143: Received unexpected error: invalid provider type -``` - -**Fix Required:** -1. Initialize DNS provider registry in test setup -2. Verify `setupCredentialHandlerTest()` properly initializes all dependencies +Common causes: +- Short-circuit boolean logic (e.g., `a && b`, `a || b`) where tests only exercise one side. +- Multi-branch conditionals (`if/else if/else`, `switch`) where only one case is hit. +- Error wrapping/logging paths that run only when an upstream returns an error. -#### 1.2 Security Handler Tests (MIXED) -**Files:** -- Test: `/projects/Charon/backend/internal/api/handlers/security_handler_audit_test.go` +Guidance: +- Treat yellow lines like “missing branch coverage”, not “missing statement coverage”. +- Write the smallest additional test that triggers the unhit branch (invalid input, not-found, service error, upstream non-200, etc.). +- Prefer deterministic seams: `httptest.Server` for upstream failures; fake services/mocks for DB/service errors; explicit `t.Setenv` for env-driven branches. -**Failing:** `TestSecurityHandler_CreateDecision_SQLInjection` (3/4 sub-tests) -- Payloads 1, 2, 3 returning 500 instead of expected 200/400 +## How to locate exact missing lines in Codecov (triage) -**Passing:** `TestSecurityHandler_UpsertRuleSet_XSSInContent` ✓ +1. Open the PR in GitHub. +2. Open the Codecov check details (“Details” / “View report”). +3. Switch to **Patch** (not Project). +4. Filter to the 10 scoped files. +5. For each file, copy the exact missing (red) and partial (yellow) line ranges. +6. Map each range to a minimal test that executes the branch. -**Root Cause:** -Missing input validation causes 500 errors for malicious payloads. +Local assist (for understanding branches; Codecov still authoritative): +- Run VS Code task: **Test: Backend with Coverage**. +- View coverage HTML: `go tool cover -html=backend/coverage.txt`. -**Fix:** Add input sanitization returning 400 for invalid inputs. +## Remediation Plan (tight, test-first) ---- +1. Extract missing patch line ranges from Codecov and fill the table above. +2. For each missing range: + - Identify the branch (if/switch/early return) causing it. + - Add the shortest test that executes it. +3. Re-run VS Code task **Test: Backend with Coverage**. +4. Confirm Codecov patch view is green (100% patch coverage). -### Category 2: Caddy Manager Failures (2.334s total) +## Per-File Testing Strategy (scoped) -#### 2.1 DNS Challenge Config Generation Failures -**Failing Tests:** -1. `TestGenerateConfig_DNSChallenge_LetsEncrypt_StagingCAAndPropagationTimeout` -2. `TestApplyConfig_SingleCredential_BackwardCompatibility` -3. `TestApplyConfig_MultiCredential_ExactMatch` -4. `TestApplyConfig_MultiCredential_WildcardMatch` -5. `TestApplyConfig_MultiCredential_CatchAll` +Only add tests that hit the lines Codecov marks missing. -**Root Cause:** -DNS provider registry not initialized. Credential matching succeeds but config generation skips DNS challenge setup: -``` -time="2026-01-07T07:10:14Z" level=warning msg="DNS provider type not found in registry" provider_type=cloudflare -manager_multicred_integration_test.go:125: Expected value not to be nil. -Messages: DNS challenge policy should exist for *.example.com -``` +### 1) backend/internal/api/handlers/plugin_handler.go -**Fix:** Initialize DNS provider registry before config generation tests. +- Prioritize handler tests that cover: + - invalid `:id` parsing → 400 + - not found → 404 + - DB update failures → 500 + - loader reload/load failures (often warn-but-200) → assert response body, not just status ---- +### 2) backend/internal/api/handlers/encryption_handler.go -### Category 3: Service Layer Failures (115.206s total) +- Add/extend tests to cover: + - request bind/validation errors → 400 + - service-layer failures → 500 + - success path + any new audit/log branches -#### 3.1 DNS Provider Credential Validation Failures +### 3) backend/internal/api/handlers/credential_handler.go -**Failing Tests:** -1. `TestAllProviderTypes` (line 755) - 3 providers failing -2. `TestDNSProviderService_TestCredentials_AllProviders` (line 1128) - 3 providers failing -3. `TestDNSProviderService_List_OrderByDefault` (line 1221) - hetzner failing -4. `TestDNSProviderService_AuditLogging_Delete` (line 1670) - digitalocean failing +- Add/extend tests to cover: + - invalid ID parse → 400 + - not found → 404 + - create/update invalid provider / credential validation failures (where applicable) + - “test credentials” endpoint: service error mapping -**Root Cause:** Credential field name mismatches +### 4) backend/internal/api/handlers/settings_handler.go -| Provider | Test Uses | Should Use | -|---------------|-----------------|------------------| -| hetzner | `api_key` | `api_token` | -| digitalocean | `auth_token` | `api_token` | -| dnsimple | `oauth_token` | `api_token` | +- Add/extend tests to cover: + - invalid URL input → 400 + - SSRF-blocked / unreachable cases that return 200 but set `reachable=false` + - network failures and how they’re represented in the response -**Fix:** Update test credential data field names (8 locations). +### 5) backend/internal/api/handlers/crowdsec_handler.go ---- +- Use `httptest.Server` to deterministically cover: + - upstream non-200 responses + - upstream non-JSON responses + - query param forwarding (capture server request URL) -### Category 4: Security Settings Database Override Failures (NEW - PR #460) +### 6) backend/internal/api/handlers/proxy_host_handler.go -#### 4.1 Security Settings Table Override Tests -**Files:** -- Test: `/projects/Charon/backend/internal/api/handlers/security_handler_settings_test.go` -- Test: `/projects/Charon/backend/internal/api/handlers/security_handler_clean_test.go` -- Handler: `/projects/Charon/backend/internal/api/handlers/security_handler.go` +- Add/extend tests that submit JSON payloads exercising type coercion: + - wrong types (string vs number) → 400 + - boundary values (negative ports, missing required fields) + - normalization branches (if Codecov points to them) -**Failing Tests (5 total):** -1. `TestSecurityHandler_ACL_DBOverride` (line 86 in security_handler_clean_test.go) -2. `TestSecurityHandler_CrowdSec_Mode_DBOverride` (line 196 in security_handler_clean_test.go) -3. `TestSecurityHandler_GetStatus_RespectsSettingsTable` (5 sub-tests): - - "WAF enabled via settings overrides disabled config" - - "Rate Limit enabled via settings overrides disabled config" - - "CrowdSec enabled via settings overrides disabled config" - - "All modules enabled via settings" - - "No settings - falls back to config (enabled)" -4. `TestSecurityHandler_GetStatus_WAFModeFromSettings` (line 187) -5. `TestSecurityHandler_GetStatus_RateLimitModeFromSettings` (line 218) +### 7) backend/internal/api/handlers/security_handler.go -**Root Cause:** -Handler's `GetStatus` method queries `security_configs` table: -```go -// Line 68 in security_handler.go -var secConfig models.SecurityConfig -if err := h.db.Where("name = ?", "default").First(&secConfig).Error; err != nil { - // Returns error: "record not found" -} -``` +- Add/extend tests to cover: + - effective-status branch selection (settings vs DB vs defaults) + - validation branches (IP/CIDR parsing, enum validation, etc.) -Tests insert settings into `settings` table: -```go -db.Create(&models.Setting{Key: "security.acl.enabled", Value: "true"}) -``` +### 8) backend/internal/caddy/config.go -But handler never checks the `settings` table for overrides. +- Prefer helper-level unit tests (cheaper and more targeted) to cover: + - header normalization helpers + - CSP/security header composition + - CIDR parsing and bypass list validation + - skip/empty branches (e.g., “missing provider config → skip policy”) -**Expected Behavior:** -1. Handler should first check `settings` table for keys like: - - `security.waf.enabled` - - `security.rate_limit.enabled` - - `security.acl.enabled` - - `security.crowdsec.enabled` -2. If settings exist, use them as overrides -3. Otherwise, fall back to `security_configs` table -4. If `security_configs` doesn't exist, fall back to config file +### 9) backend/internal/caddy/client.go -**Fix Required:** -Update `GetStatus` handler to implement 3-tier priority: -1. Runtime settings (`settings` table) - highest priority -2. Saved config (`security_configs` table) - medium priority -3. Config file - lowest priority (fallback) +- Add/extend tests for: + - endpoint validation failures + - HTTP non-200 response branches + - parse/marshal error branches (only if Codecov points to them) ---- +### 10) backend/internal/api/handlers/testdb.go (special) -### Category 5: Certificate Deletion Race Condition (NEW - PR #460) +If Codecov patch view includes this file: -#### 5.1 Database Lock During Certificate Deletion -**File:** `/projects/Charon/backend/internal/api/handlers/certificate_handler_test.go` - -**Failing Test:** -- `TestDeleteCertificate_CreatesBackup` (line 87) - -**Error:** -``` -database table is locked: ssl_certificates -expected 200 OK, got 500, body={"error":"failed to delete certificate"} -``` +What’s happening: +- `.codecov.yml` currently ignores `backend/internal/api/handlers/testdb.go`, but Go coverprofiles can report paths as module-import paths (example from `backend/coverage.txt`): `github.com/Wikid82/charon/backend/internal/.../testdb.go`. +- If Codecov is matching against the coverprofile path form, the repo-relative ignore may not apply. -**Root Cause:** -Test creates a backup service that creates a backup, then immediately tries to delete the certificate: -```go -mockBackupService := &mockBackupService{ - createFunc: func() (string, error) { - backupCalled = true - return "backup-test.tar.gz", nil - }, -} - -// Handler calls: -// 1. backupService.Create() → locks DB for backup -// 2. db.Delete(&cert) → tries to lock DB again → LOCKED -``` +Make it actionable: +1. Confirm what path form the coverprofile is using: + - `grep -n "testdb.go" backend/coverage.txt` +2. If the result is a module/import-path form (example: `github.com/Wikid82/charon/backend/internal/api/handlers/testdb.go`), add one additional ignore entry to `.codecov.yml` so it matches what Codecov is actually seeing. + - Minimal, explicit (exact repo/module path): `"**/github.com/Wikid82/charon/backend/internal/api/handlers/testdb.go"` + - More resilient (still narrow): `"**/github.com/**/backend/internal/api/handlers/testdb.go"` -SQLite in-memory databases have limited concurrency. The backup operation holds a read lock while the delete tries to acquire a write lock. +Note: +- Do not add `"**/backend/internal/api/handlers/testdb.go"` unless it’s missing; the repo-relative ignore is already present. -**Fix Required:** -Option 1: Add transaction with proper lock handling -Option 2: Add retry logic with exponential backoff -Option 3: Mock the backup more properly to avoid actual DB operations -Option 4: Use `?mode=memory&cache=shared&_txlock=immediate` for better transaction handling +Why this is minimal: +- `.codecov.yml` already ignores the repo-relative path, but ignore matching can fail if Codecov consumes coverprofile paths that include the module/import prefix. +- Only add the extra ignore if the grep confirms the import-path form is present. ---- +Preferred approach (in order): +1. Move test-only helpers into a `_test.go` file. + - Caution: this is only safe if **no other packages’ tests import those helpers**. Anything in `*_test.go` cannot be imported by other packages. +2. Otherwise, keep the helper in non-test code but rely on `.codecov.yml` ignores that match the coverprofile path form. -### Category 6: Frontend Test Timeout (NEW - CI #20773147447) +## Prevention: required instruction/agent updates (diff-style) -#### 6.1 LiveLogViewer Security Mode Test -**File:** `/projects/Charon/frontend/src/components/__tests__/LiveLogViewer.test.tsx` +These are the minimal guardrails to prevent future patch-coverage regressions. -**Failing Test:** -- Line 374: "displays blocked requests with special styling" (Security Mode suite) +### A) .github/instructions/testing.instructions.md -**Error:** -``` -Test timed out in 5000ms +```diff +@@ + ## 3. Coverage & Completion + * **Coverage Gate:** A task is not "Complete" until a coverage report is generated. + * **Threshold Compliance:** You must compare the final coverage percentage against the project's threshold (Default: 85% unless specified otherwise). If coverage drops, you must identify the "uncovered lines" and add targeted tests. ++* **Patch Coverage Gate (Codecov):** If production code is modified, Codecov **patch coverage must be 100%** for the modified lines. Do not relax thresholds; add targeted tests. ++* **Patch Triage Requirement:** Plans must include the exact missing/partial patch line ranges copied from Codecov’s **Patch** view. ``` -**Root Cause:** -Test has race condition between async state updates and DOM queries: -```typescript -await act(async () => { - mockOnSecurityMessage(blockedLog); -}); - -await waitFor(() => { - const wafElements = screen.getAllByText('WAF'); - expect(wafElements.length).toBeGreaterThanOrEqual(2); - expect(screen.getByText('10.0.0.1')).toBeTruthy(); - expect(screen.getByText(/BLOCKED: SQL injection detected/)).toBeTruthy(); - expect(screen.getByText(/\[SQL injection detected\]/)).toBeTruthy(); -}); -``` +### B) .github/instructions/taming-copilot.instructions.md -**Issues:** -1. Multiple assertions in single `waitFor` - if any fails, all retry -2. Complex regex patterns may not match rendered content -3. `act()` completes before component state update finishes -4. 5000ms timeout insufficient for CI environment - -**Fix Required:** -Option A (Quick): Increase timeout to 10000ms, split assertions -Option B (Better): Use `findBy` queries which wait automatically: -```typescript -await screen.findByText('10.0.0.1'); -await screen.findByText(/BLOCKED: SQL injection detected/); +```diff +@@ + ## Surgical Code Modification +- **Focus on the Core Request**: Generate code that directly addresses the user's request, without adding extra features or handling edge cases that were not mentioned. ++ **Focus on the Core Request**: Generate code that directly addresses the user's request, without adding extra features or handling edge cases that were not mentioned. ++ **Spec Hygiene**: When asked to update a plan/spec file, do not append unrelated/archived plans; keep it strictly scoped to the current task. ``` ---- - -## Implementation Plan - -### Phase 0: Pre-Implementation Verification (NEW - P0) - -**Estimated Time:** 30 minutes -**Purpose:** Establish factual baseline before making changes +### C) .github/instructions/copilot-instructions.md -#### Step 0.1: Capture Exact Error Messages -```bash -# Run failing tests with verbose output -go test -v ./backend/internal/api/handlers -run "TestCredentialHandler_Create" 2>&1 | tee phase0_credential_errors.txt -go test -v ./backend/internal/caddy -run "TestGenerateConfig_DNSChallenge_LetsEncrypt" 2>&1 | tee phase0_caddy_errors.txt -go test -v ./backend/internal/services -run "TestAllProviderTypes" 2>&1 | tee phase0_service_errors.txt -go test -v ./backend/internal/api/handlers -run "TestSecurityHandler_CreateDecision_SQLInjection" 2>&1 | tee phase0_security_errors.txt +```diff +@@ + 3. **Coverage Testing** (MANDATORY - Non-negotiable): +- - **MANDATORY**: Patch coverage must cover 100% of new/modified code. This prevents CodeCov Report failing CI. ++ - **MANDATORY**: Patch coverage must cover 100% of modified lines (Codecov Patch view must be green). If patch coverage fails, add targeted tests for the missing patch line ranges. ``` -#### Step 0.2: Verify DNS Provider Package Structure -```bash -# Confirm correct package location -ls -la backend/pkg/dnsprovider/builtin/ -cat backend/pkg/dnsprovider/builtin/builtin.go | grep -A 20 "func init" +### D) .github/agents/Backend_Dev.agent.md -# Verify providers exist -ls backend/pkg/dnsprovider/builtin/*.go | grep -v _test.go +```diff +@@ + 3. **Verification (Definition of Done)**: +@@ +- - **Coverage (MANDATORY)**: Run the coverage script explicitly. This is NOT run by pre-commit automatically. ++ - **Coverage (MANDATORY)**: Run the coverage task/script explicitly and confirm Codecov patch view is green for modified lines. ``` -**Expected Output:** -``` -backend/pkg/dnsprovider/builtin/builtin.go -backend/pkg/dnsprovider/builtin/cloudflare.go -backend/pkg/dnsprovider/builtin/route53.go -backend/pkg/dnsprovider/builtin/digitalocean.go -backend/pkg/dnsprovider/builtin/hetzner.go -backend/pkg/dnsprovider/builtin/dnsimple.go -backend/pkg/dnsprovider/builtin/vultr.go -backend/pkg/dnsprovider/builtin/godaddy.go -backend/pkg/dnsprovider/builtin/namecheap.go -backend/pkg/dnsprovider/builtin/googleclouddns.go -backend/pkg/dnsprovider/builtin/azure.go -``` +### E) .github/agents/Frontend_Dev.agent.md (only if frontend changes are involved) -#### Step 0.3: Check Current Test Imports -```bash -# Check if any test already imports builtin -grep -r "import.*builtin" backend/**/*_test.go - -# Check what packages credential tests import -head -30 backend/internal/api/handlers/credential_handler_test.go +```diff +@@ + 3. **Verification (Quality Gates)**: +@@ + - **Gate 3: Coverage (MANDATORY)**: +@@ + - **MANDATORY**: Patch coverage must cover 100% of new/modified code. This prevents CodeCov Report failing CI. ++ - If patch coverage fails, identify missing patch line ranges in Codecov Patch view and add targeted tests. ``` -#### Step 0.4: Establish Coverage Baseline -```bash -# Capture current coverage before changes -go test -coverprofile=baseline_coverage.out ./backend/internal/... -go tool cover -func=baseline_coverage.out | tail -1 -``` +### F) .github/agents/QA_Security.agent.md -**Success Criteria:** -- [ ] All error logs captured with exact messages -- [ ] Package structure at `pkg/dnsprovider/builtin` confirmed -- [ ] `init()` function in `builtin.go` verified -- [ ] Current test imports documented -- [ ] Baseline coverage recorded - ---- - -### Phase 1: DNS Provider Registry Initialization (CRITICAL - P0) - -**Estimated Time:** 1-2 hours -**CORRECTED APPROACH:** Use blank imports to trigger existing `init()` function - -#### Step 1.1: Try Blank Import Approach First (SIMPLEST) - -The `backend/pkg/dnsprovider/builtin/builtin.go` file ALREADY contains: -```go -func init() { - providers := []dnsprovider.ProviderPlugin{ - &CloudflareProvider{}, - &Route53Provider{}, - // ... all 10 providers - } - for _, provider := range providers { - dnsprovider.Global().Register(provider) - } -} +```diff +@@ +- - When creating tests, if there are folders that don't require testing make sure to update `.codecov.yml` to exclude them from coverage reports or this throws off the difference between local and CI coverage. ++ - Prefer fixing patch coverage with tests. Only adjust `.codecov.yml` ignores when code is truly non-production (e.g., test-only helpers), and document why. ``` -**This means we only need to import the package to trigger registration.** - -##### Option 1A: Add Blank Import to Test Files - -**File:** `/projects/Charon/backend/internal/api/handlers/credential_handler_test.go` -**Line:** Add to import block (around line 5) - -```go -import ( - "bytes" - "encoding/json" - // ... existing imports ... +### G) .github/agents/Supervisor.agent.md - _ "github.com/Wikid82/charon/backend/pkg/dnsprovider/builtin" // Auto-register DNS providers -) +```diff +@@ +- - **Plan Completeness**: Does the plan cover all edge cases? Are there any missing components or unclear requirements? ++ - **Plan Completeness**: Does the plan cover all edge cases? Are there any missing components or unclear requirements? ++ - **Patch Coverage Completeness**: If coverage is in scope, does the plan include Codecov Patch missing/partial line ranges and the exact tests needed to execute them? ``` -**File:** `/projects/Charon/backend/internal/caddy/manager_multicred_integration_test.go` - -Add same blank import to import block. - -**File:** `/projects/Charon/backend/internal/caddy/config_patch_coverage_test.go` - -Add same blank import to import block. - -**File:** `/projects/Charon/backend/internal/services/dns_provider_service_test.go` - -Add same blank import to import block. - -##### Step 1.2: Validate Blank Import Approach -```bash -# Test if blank imports fix the issue -go test -v ./backend/internal/api/handlers -run "TestCredentialHandler_Create" -go test -v ./backend/internal/caddy -run "TestGenerateConfig_DNSChallenge_LetsEncrypt" -``` +## Validation Checklist (patch-coverage scope) -**If blank imports work → DONE. Skip to Phase 2.** - ---- - -##### Option 1B: Create Test Helper (ONLY IF BLANK IMPORTS FAIL) - -**File:** `/projects/Charon/backend/internal/services/dns_provider_test_helper.go` (NEW) - -```go -package services - -import ( - _ "github.com/Wikid82/charon/backend/pkg/dnsprovider/builtin" // Triggers init() -) - -// InitDNSProviderRegistryForTests ensures the builtin DNS provider registry -// is initialized. This is a no-op if the package is already imported elsewhere, -// but provides an explicit call point for test setup. -// -// The actual registration happens in builtin.init(). -func InitDNSProviderRegistryForTests() { - // No-op: The blank import above triggers builtin.init() - // which calls dnsprovider.Global().Register() for all providers. -} -``` - -**Then update test files to call the helper:** - -**File:** `/projects/Charon/backend/internal/api/handlers/credential_handler_test.go` -**Line:** 21 (in `setupCredentialHandlerTest`) - -```go -func setupCredentialHandlerTest(t *testing.T) (*gin.Engine, *gorm.DB, *models.DNSProvider) { - // Initialize DNS provider registry (triggers builtin.init()) - services.InitDNSProviderRegistryForTests() - - gin.SetMode(gin.TestMode) - // ... rest of setup -} -``` - -**File:** `/projects/Charon/backend/internal/caddy/manager_multicred_integration_test.go` - -Add at package level: -```go -func init() { - services.InitDNSProviderRegistryForTests() -} -``` - -**File:** `/projects/Charon/backend/internal/caddy/config_patch_coverage_test.go` - -Same init() function. - -**File:** `/projects/Charon/backend/internal/services/dns_provider_service_test.go` - -In `setupDNSProviderTestDB`: -```go -func setupDNSProviderTestDB(t *testing.T) (*gorm.DB, *crypto.EncryptionService) { - InitDNSProviderRegistryForTests() - // ... rest of setup -} -``` - -**Decision Point:** Start with Option 1A (blank imports). Only implement Option 1B if blank imports don't work. - ---- - -### Phase 2: Fix Credential Field Names (P1) - -**Estimated Time:** 30 minutes - -#### Step 2.1: Update TestAllProviderTypes -**File:** `/projects/Charon/backend/internal/services/dns_provider_service_test.go` -**Lines:** 762-774 - -Change: -- Line 768: `"hetzner": {"api_key": "key"}` → `"hetzner": {"api_token": "key"}` -- Line 765: `"digitalocean": {"auth_token": "token"}` → `"digitalocean": {"api_token": "token"}` -- Line 774: `"dnsimple": {"oauth_token": "token", ...}` → `"dnsimple": {"api_token": "token", ...}` - -#### Step 2.2: Update TestDNSProviderService_TestCredentials_AllProviders -**File:** Same -**Lines:** 1142-1152 - -Apply identical changes (3 providers). - -#### Step 2.3: Update TestDNSProviderService_List_OrderByDefault -**File:** Same -**Line:** ~1236 - -```go -_, err = service.Create(ctx, CreateDNSProviderRequest{ - Name: "A Provider", - ProviderType: "hetzner", - Credentials: map[string]string{"api_token": "key"}, // CHANGED -}) -``` - -#### Step 2.4: Update TestDNSProviderService_AuditLogging_Delete -**File:** Same -**Line:** ~1679 - -```go -provider, err := service.Create(ctx, CreateDNSProviderRequest{ - Name: "To Be Deleted", - ProviderType: "digitalocean", - Credentials: map[string]string{"api_token": "test-token"}, // CHANGED -}) -``` - ---- - -### Phase 3: Fix Security Handler (P2) - -**Estimated Time:** 1-2 hours - -**CORRECTED UNDERSTANDING:** -- Test expects EITHER 200 OR 400, not just 400 -- Current issue: Returns 500 on malicious inputs (database errors) -- Root cause: Missing input validation allows malicious data to reach database layer -- Fix: Add comprehensive validation returning 400 BEFORE database operations - -**File:** `/projects/Charon/backend/internal/api/handlers/security_handler.go` - -Locate `CreateDecision` method and add input validation: -```go -func (h *SecurityHandler) CreateDecision(c *gin.Context) { - var req struct { - IP string `json:"ip" binding:"required"` - Action string `json:"action" binding:"required"` - Details string `json:"details"` - } - - if err := c.ShouldBindJSON(&req); err != nil { - c.JSON(http.StatusBadRequest, gin.H{"error": "Invalid input"}) - return - } - - // CRITICAL: Validate IP format to prevent SQL injection via IP field - // Must accept both single IPs and CIDR ranges - if !isValidIP(req.IP) && !isValidCIDR(req.IP) { - c.JSON(http.StatusBadRequest, gin.H{"error": "Invalid IP address format"}) - return - } - - // CRITICAL: Validate action enum - // Only accept known action types to prevent injection via action field - validActions := []string{"block", "allow", "captcha"} - if !contains(validActions, req.Action) { - c.JSON(http.StatusBadRequest, gin.H{"error": "Invalid action"}) - return - } - - // Sanitize details field (limit length, strip control characters) - req.Details = sanitizeString(req.Details, 1000) - - // Now proceed with database operation - // Parameterized queries are already used, but validation prevents malicious data - // from ever reaching the database layer - - // ... existing code for database insertion ... -} - -// isValidIP validates that s is a valid IPv4 or IPv6 address -func isValidIP(s string) bool { - return net.ParseIP(s) != nil -} - -// isValidCIDR validates that s is a valid CIDR notation -func isValidCIDR(s string) bool { - _, _, err := net.ParseCIDR(s) - return err == nil -} - -// contains checks if a string exists in a slice -func contains(slice []string, item string) bool { - for _, s := range slice { - if s == item { - return true - } - } - return false -} - -// sanitizeString removes control characters and enforces max length -func sanitizeString(s string, maxLen int) string { - // Remove null bytes and other control characters - s = strings.Map(func(r rune) rune { - if r == 0 || (r < 32 && r != '\n' && r != '\r' && r != '\t') { - return -1 // Remove character - } - return r - }, s) - - // Enforce max length - if len(s) > maxLen { - return s[:maxLen] - } - return s -} -``` - -**Add required imports at top of file:** -```go -import ( - "net" - "strings" - // ... existing imports ... -) -``` - -**Why This Fixes the Test:** -1. **SQL Injection Payload 1:** Invalid IP format → Returns 400 (valid response per test) -2. **SQL Injection Payload 2:** Invalid characters in action → Returns 400 (valid response per test) -3. **SQL Injection Payload 3:** Null bytes in details → Sanitized, returns 200 (valid response per test) -4. **Legitimate Requests:** Valid IP + action → Returns 200 (existing behavior preserved) - -**Test expects status in [200, 400]:** -```go -// From security_handler_audit_test.go -if status != http.StatusOK && status != http.StatusBadRequest { - t.Errorf("Payload %d: Expected 200 or 400, got %d", i+1, status) -} -``` - ---- - -### Phase 4: Security Settings Database Override Fix (P1) - -**Estimated Time:** 2-3 hours - -#### Step 4.1: Update GetStatus Handler to Check Settings Table - -**File:** `/projects/Charon/backend/internal/api/handlers/security_handler.go` -**Method:** `GetStatus` (around line 68) - -**Current Code Pattern:** -```go -var secConfig models.SecurityConfig -if err := h.db.Where("name = ?", "default").First(&secConfig).Error; err != nil { - // Fails with "record not found" in tests - logger.Log().WithError(err).Error("Failed to load security config") - // Uses config file as fallback -} -``` - -**Required Changes:** - -1. Create helper function to check settings table first: -```go -// getSettingBool retrieves a boolean setting from the settings table -func (h *SecurityHandler) getSettingBool(key string, defaultVal bool) bool { - var setting models.Setting - err := h.db.Where("key = ?", key).First(&setting).Error - if err != nil { - return defaultVal - } - return setting.Value == "true" -} -``` - -2. Update `GetStatus` to use 3-tier priority: -```go -func (h *SecurityHandler) GetStatus(c *gin.Context) { - // Tier 1: Check runtime settings table (highest priority) - var secConfig models.SecurityConfig - configExists := true - if err := h.db.Where("name = ?", "default").First(&secConfig).Error; err != nil { - if errors.Is(err, gorm.ErrRecordNotFound) { - configExists = false - // Not an error - just means we'll use settings + config file - } else { - logger.Log().WithError(err).Error("Database error loading security config") - c.JSON(500, gin.H{"error": "failed to load security config"}) - return - } - } - - // Start with config file values (Tier 3 - lowest priority) - wafEnabled := h.config.WAFMode != "disabled" - rateLimitEnabled := h.config.RateLimitMode != "disabled" - aclEnabled := h.config.ACLMode != "disabled" - crowdSecEnabled := h.config.CrowdSecMode != "disabled" - cerberusEnabled := h.config.CerberusEnabled - - // Override with security_configs table if exists (Tier 2) - if configExists { - wafEnabled = secConfig.WAFEnabled - rateLimitEnabled = secConfig.RateLimitEnabled - aclEnabled = secConfig.ACLEnabled - crowdSecEnabled = secConfig.CrowdSecEnabled - } - - // Override with settings table if exists (Tier 1 - highest priority) - wafEnabled = h.getSettingBool("security.waf.enabled", wafEnabled) - rateLimitEnabled = h.getSettingBool("security.rate_limit.enabled", rateLimitEnabled) - aclEnabled = h.getSettingBool("security.acl.enabled", aclEnabled) - crowdSecEnabled = h.getSettingBool("security.crowdsec.enabled", crowdSecEnabled) - - // Build response - response := gin.H{ - "cerberus": gin.H{ - "enabled": cerberusEnabled, - }, - "waf": gin.H{ - "enabled": wafEnabled, - "mode": h.config.WAFMode, - }, - "rate_limit": gin.H{ - "enabled": rateLimitEnabled, - "mode": h.config.RateLimitMode, - }, - "acl": gin.H{ - "enabled": aclEnabled, - "mode": h.config.ACLMode, - }, - "crowdsec": gin.H{ - "enabled": crowdSecEnabled, - "mode": h.config.CrowdSecMode, - }, - } - - c.JSON(200, response) -} -``` - -#### Step 4.2: Update Tests Affected - -No test changes needed - tests are correct, handler was wrong. - -**Verify these tests now pass:** -```bash -go test -v ./backend/internal/api/handlers -run "TestSecurityHandler_ACL_DBOverride" -go test -v ./backend/internal/api/handlers -run "TestSecurityHandler_CrowdSec_Mode_DBOverride" -go test -v ./backend/internal/api/handlers -run "TestSecurityHandler_GetStatus_RespectsSettingsTable" -go test -v ./backend/internal/api/handlers -run "TestSecurityHandler_GetStatus_WAFModeFromSettings" -go test -v ./backend/internal/api/handlers -run "TestSecurityHandler_GetStatus_RateLimitModeFromSettings" -``` +Required vs optional alignment: +- Required for this plan to be considered complete: Workstream A + Workstream B changes landed. +- This validation checklist is intentionally focused on Workstream A (patch coverage remediation). For additional repo-wide Definition of Done items, follow `.github/instructions/copilot-instructions.md`. +- Workstream B validation is “diff applied + reviewer confirmation” (it doesn’t impact Go patch coverage directly). ---- - -### Phase 5: Certificate Deletion Race Condition Fix (P2) - -**Estimated Time:** 1-2 hours - -#### Step 5.1: Fix Database Lock in Certificate Deletion - -**File:** `/projects/Charon/backend/internal/api/handlers/certificate_handler.go` -**Method:** `Delete` handler - -**Option A: Use Immediate Transaction Mode (RECOMMENDED)** - -**File:** Test setup in `certificate_handler_test.go` -**Change:** Update SQLite connection string to use immediate transactions - -```go -func TestDeleteCertificate_CreatesBackup(t *testing.T) { - // OLD: - // db, err := gorm.Open(sqlite.Open(fmt.Sprintf("file:%s?mode=memory&cache=shared", t.Name())), &gorm.Config{}) - - // NEW: Add _txlock=immediate to prevent lock contention - db, err := gorm.Open(sqlite.Open(fmt.Sprintf("file:%s?mode=memory&cache=shared&_txlock=immediate", t.Name())), &gorm.Config{}) - if err != nil { - t.Fatalf("failed to open db: %v", err) - } - // ... rest of test unchanged -} -``` - -**Option B: Add Retry Logic to Delete Handler** - -**File:** `/projects/Charon/backend/internal/services/certificate_service.go` -**Method:** `Delete` - -```go -func (s *CertificateService) Delete(id uint) error { - var cert models.SSLCertificate - if err := s.db.First(&cert, id).Error; err != nil { - return err - } - - // Retry delete up to 3 times if database is locked - maxRetries := 3 - var deleteErr error - for i := 0; i < maxRetries; i++ { - deleteErr = s.db.Delete(&cert).Error - if deleteErr == nil { - return nil - } - // Check if error is database locked - if strings.Contains(deleteErr.Error(), "database table is locked") || - strings.Contains(deleteErr.Error(), "database is locked") { - // Wait with exponential backoff - time.Sleep(time.Duration(50*(i+1)) * time.Millisecond) - continue - } - // Non-lock error, return immediately - return deleteErr - } - return fmt.Errorf("delete failed after %d retries: %w", maxRetries, deleteErr) -} -``` - -**Recommended:** Use Option A (simpler and more reliable) - -#### Step 5.2: Verify Test Passes - -```bash -go test -v ./backend/internal/api/handlers -run "TestDeleteCertificate_CreatesBackup" -``` - ---- - -### Phase 6: Frontend Test Timeout Fix (P2) - -**Estimated Time:** 30 minutes - 1 hour - -#### Step 6.1: Fix LiveLogViewer Test Timeout - -**File:** `/projects/Charon/frontend/src/components/__tests__/LiveLogViewer.test.tsx` -**Line:** 374 ("displays blocked requests with special styling" test) - -**Option A: Use findBy Queries (RECOMMENDED)** - -```typescript -it('displays blocked requests with special styling', async () => { - render(); - - // Wait for connection to establish - await screen.findByText('Connected'); - - const blockedLog: logsApi.SecurityLogEntry = { - timestamp: '2025-12-12T10:30:00Z', - level: 'warn', - logger: 'http.handlers.waf', - client_ip: '10.0.0.1', - method: 'POST', - uri: '/admin', - status: 403, - duration: 0.001, - size: 0, - user_agent: 'Attack/1.0', - host: 'example.com', - source: 'waf', - blocked: true, - block_reason: 'SQL injection detected', - }; - - // Send message inside act to properly handle state updates - await act(async () => { - if (mockOnSecurityMessage) { - mockOnSecurityMessage(blockedLog); - } - }); - - // Use findBy for automatic waiting - cleaner and more reliable - await screen.findByText('10.0.0.1'); - await screen.findByText(/BLOCKED: SQL injection detected/); - await screen.findByText(/\[SQL injection detected\]/); - - // For getAllByText, wrap in waitFor since findAllBy isn't used for counts - await waitFor(() => { - const wafElements = screen.getAllByText('WAF'); - expect(wafElements.length).toBeGreaterThanOrEqual(2); - }); -}); -``` - -**Option B: Split Assertions with Individual waitFor (Fallback)** - -```typescript -it('displays blocked requests with special styling', async () => { - render(); - - await waitFor(() => expect(screen.getByText('Connected')).toBeTruthy()); - - const blockedLog: logsApi.SecurityLogEntry = { /* ... */ }; - - await act(async () => { - if (mockOnSecurityMessage) { - mockOnSecurityMessage(blockedLog); - } - }); - - // Split assertions into separate waitFor calls to isolate failures - await waitFor(() => { - expect(screen.getByText('10.0.0.1')).toBeTruthy(); - }, { timeout: 10000 }); - - await waitFor(() => { - expect(screen.getByText(/BLOCKED: SQL injection detected/)).toBeTruthy(); - }, { timeout: 10000 }); - - await waitFor(() => { - expect(screen.getByText(/\[SQL injection detected\]/)).toBeTruthy(); - }, { timeout: 10000 }); - - await waitFor(() => { - const wafElements = screen.getAllByText('WAF'); - expect(wafElements.length).toBeGreaterThanOrEqual(2); - }, { timeout: 10000 }); -}, 30000); // Increase overall test timeout -``` - -**Recommended:** Use Option A (cleaner, more React Testing Library idiomatic) - -#### Step 6.2: Verify Test Passes - -```bash -cd frontend && npm test -- LiveLogViewer.test.tsx -``` - ---- - -### Phase 7: Validation & Testing (P0) - -#### Step 7.1: Test Individual Suites -```bash -# Phase 0: Capture baseline -go test -v ./backend/internal/api/handlers -run "TestCredentialHandler_Create" 2>&1 | tee test_output_phase1.txt - -# After Phase 1: Test registry initialization fix -go test -v ./backend/internal/api/handlers -run "TestCredentialHandler_" 2>&1 | tee test_phase1_credentials.txt -go test -v ./backend/internal/caddy -run "TestGenerateConfig_DNSChallenge|TestApplyConfig_" 2>&1 | tee test_phase1_caddy.txt - -# After Phase 2: Test credential field name fixes -go test -v ./backend/internal/services -run "TestAllProviderTypes" 2>&1 | tee test_phase2_providers.txt -go test -v ./backend/internal/services -run "TestDNSProviderService_TestCredentials_AllProviders" 2>&1 | tee test_phase2_validation.txt -go test -v ./backend/internal/services -run "TestDNSProviderService_List_OrderByDefault" 2>&1 | tee test_phase2_list.txt -go test -v ./backend/internal/services -run "TestDNSProviderService_AuditLogging_Delete" 2>&1 | tee test_phase2_audit.txt - -# After Phase 3: Test security handler fix -go test -v ./backend/internal/api/handlers -run "TestSecurityHandler_CreateDecision_SQLInjection" 2>&1 | tee test_phase3_security.txt -``` - -#### Step 7.2: Full Test Suite -```bash -# Run all affected test suites -go test -v ./backend/internal/api/handlers 2>&1 | tee test_full_handlers.txt -go test -v ./backend/internal/caddy 2>&1 | tee test_full_caddy.txt -go test -v ./backend/internal/services 2>&1 | tee test_full_services.txt - -# Verify no new failures introduced -grep -E "(FAIL|PASS)" test_full_*.txt | sort -``` - -#### Step 7.3: Coverage Check -```bash -# Generate coverage for all modified packages -go test -coverprofile=coverage_final.out ./backend/internal/api/handlers ./backend/internal/caddy ./backend/internal/services - -# Compare with baseline -go tool cover -func=coverage_final.out | tail -1 -go tool cover -func=baseline_coverage.out | tail -1 - -# Generate HTML report -go tool cover -html=coverage_final.out -o coverage_final.html - -# Target: ≥85% coverage maintained -``` - -#### Step 7.4: Verify All 30 Tests Pass -```bash -# Phase 1: DNS Provider Registry (18 tests) -go test -v ./backend/internal/api/handlers -run "TestCredentialHandler_Create|TestCredentialHandler_List|TestCredentialHandler_Get|TestCredentialHandler_Update|TestCredentialHandler_Delete|TestCredentialHandler_Test" -go test -v ./backend/internal/caddy -run "TestGenerateConfig_DNSChallenge_LetsEncrypt_StagingCAAndPropagationTimeout|TestApplyConfig_SingleCredential_BackwardCompatibility|TestApplyConfig_MultiCredential_ExactMatch|TestApplyConfig_MultiCredential_WildcardMatch|TestApplyConfig_MultiCredential_CatchAll" - -# Phase 2: Credential Field Names (4 tests) -go test -v ./backend/internal/services -run "TestAllProviderTypes|TestDNSProviderService_TestCredentials_AllProviders|TestDNSProviderService_List_OrderByDefault|TestDNSProviderService_AuditLogging_Delete" - -# Phase 3: Security Handler Input Validation (1 test) -go test -v ./backend/internal/api/handlers -run "TestSecurityHandler_CreateDecision_SQLInjection" - -# Phase 4: Security Settings Override (5 tests) -go test -v ./backend/internal/api/handlers -run "TestSecurityHandler_ACL_DBOverride|TestSecurityHandler_CrowdSec_Mode_DBOverride|TestSecurityHandler_GetStatus_RespectsSettingsTable|TestSecurityHandler_GetStatus_WAFModeFromSettings|TestSecurityHandler_GetStatus_RateLimitModeFromSettings" - -# Phase 5: Certificate Deletion (1 test) -go test -v ./backend/internal/api/handlers -run "TestDeleteCertificate_CreatesBackup" - -# Phase 6: Frontend Timeout (1 test) -cd frontend && npm test -- LiveLogViewer.test.tsx -t "displays blocked requests with special styling" - -# All 30 tests should report PASS -``` - ---- - -## Execution Order - -### Critical Path (Sequential) -1. **Phase 0** - Pre-implementation verification (capture baseline, verify package structure) -2. **Phase 1.1** - Try blank imports first (simplest approach) -3. **Phase 1.2** - Validate if blank imports work -4. **Phase 1.3** - IF blank imports fail, create test helper (Option 1B) -5. **Phase 7.1** - Verify credential handler & Caddy tests pass (18 tests) -6. **Phase 2** - Fix credential field names -7. **Phase 7.1** - Verify service tests pass (4 tests) -8. **Phase 3** - Fix security handler with comprehensive validation -9. **Phase 7.1** - Verify security SQL injection test passes (1 test) -10. **Phase 4** - Fix security settings database override -11. **Phase 7.1** - Verify security settings tests pass (5 tests) -12. **Phase 5** - Fix certificate deletion race condition -13. **Phase 7.1** - Verify certificate deletion test passes (1 test) -14. **Phase 6** - Fix frontend test timeout -15. **Phase 7.1** - Verify frontend test passes (1 test) -16. **Phase 7.2** - Full validation -17. **Phase 7.3** - Coverage check -18. **Phase 7.4** - Verify all 30 tests pass - -### Parallelization Opportunities -- Phase 2 can be prepared during Phase 1 (but don't commit until Phase 1 validated) -- Phase 3, 4, 5, 6 are independent and can be developed in parallel (but test after Phase 1-2 complete) -- Phase 4 (security settings) and Phase 5 (certificate) don't conflict -- Phase 6 (frontend) can be done completely independently - ---- - -## Files Requiring Changes - -### Potentially New Files (1) -1. `/projects/Charon/backend/internal/services/dns_provider_test_helper.go` (ONLY IF blank imports fail) - -### Files to Edit (8-9) - -**Phase 1 (Blank Import Approach):** -1. `/projects/Charon/backend/internal/api/handlers/credential_handler_test.go` (add import) -2. `/projects/Charon/backend/internal/caddy/manager_multicred_integration_test.go` (add import) -3. `/projects/Charon/backend/internal/caddy/config_patch_coverage_test.go` (add import) -4. `/projects/Charon/backend/internal/services/dns_provider_service_test.go` (add import) - -**Phase 2 (Credential Field Names):** -5. `/projects/Charon/backend/internal/services/dns_provider_service_test.go` (8 locations: lines 768, 765, 774, 1142-1152, ~1236, ~1679) - -**Phase 3 (Security Handler):** -6. `/projects/Charon/backend/internal/api/handlers/security_handler.go` (add validation + helper functions) - -**Phase 4 (Security Settings Override):** -7. `/projects/Charon/backend/internal/api/handlers/security_handler.go` (update GetStatus to check settings table) - -**Phase 5 (Certificate Deletion):** -8. `/projects/Charon/backend/internal/api/handlers/certificate_handler_test.go` (update SQLite connection string) - -**Phase 6 (Frontend Timeout):** -9. `/projects/Charon/frontend/src/components/__tests__/LiveLogViewer.test.tsx` (fix async assertions) - ---- - -## Success Criteria - -✓ **Phase 0:** Baseline established, package structure verified -✓ **Phase 1:** All 18 credential/Caddy tests pass (registry initialized via blank import OR helper) -✓ **Phase 2:** All 4 DNS provider service tests pass (credential field names corrected) -✓ **Phase 3:** Security SQL injection test passes 4/4 sub-tests (comprehensive validation) -✓ **Phase 4:** All 5 security settings override tests pass (GetStatus checks settings table) -✓ **Phase 5:** Certificate deletion test passes (database lock resolved) -✓ **Phase 6:** Frontend LiveLogViewer test passes (timeout resolved) -✓ **Coverage:** Test coverage remains ≥85% -✓ **CI:** All 30 failing tests now pass (PR #461: 24, PR #460: 5, CI: 1) -✓ **No Regressions:** No new test failures introduced -✓ **PR #461:** Ready for merge - ---- - -## Risk Mitigation - -### High Risk: Blank Imports Don't Trigger init() -**Mitigation:** Phase 0 verification step confirms init() exists. If blank imports fail, fall back to explicit helper (Option 1B). - -### Medium Risk: Package Structure Different Than Expected -**Mitigation:** Phase 0.2 explicitly verifies `backend/pkg/dnsprovider/builtin/` structure before implementation. - -### Medium Risk: Provider Implementation Uses Different Field Names -**Mitigation:** Phase 0.2 includes checking actual provider code for field names, not just tests. - -### Low Risk: Security Validation Too Strict -**Mitigation:** Test expectations allow both 200 and 400. Validation only blocks clearly invalid inputs (bad IP format, invalid action enum, control characters). - -### Low Risk: Merge Conflicts During Implementation -**Mitigation:** Rebase on latest main before starting work. - ---- - -## Pre-Implementation Checklist - -Before starting implementation, verify: -- [ ] All import paths reference `github.com/Wikid82/charon/backend/pkg/dnsprovider/builtin` -- [ ] No references to `internal/dnsprovider/*` anywhere -- [ ] Phase 0 verification steps documented and ready to execute -- [ ] Understand that blank imports are the FIRST approach (simpler than helper) -- [ ] Security handler fix includes IP validation, action validation, AND string sanitization -- [ ] Test expectations confirmed: 200 OR 400 are both valid responses - ---- - -## Package Path Reference (CRITICAL) - -**CORRECT:** `github.com/Wikid82/charon/backend/pkg/dnsprovider/builtin` -**INCORRECT:** `github.com/Wikid82/charon/backend/internal/dnsprovider/*` ❌ - -**File Locations:** -- Registry & init(): `backend/pkg/dnsprovider/builtin/builtin.go` -- Providers: `backend/pkg/dnsprovider/builtin/*.go` -- Base interface: `backend/pkg/dnsprovider/provider.go` - -**Key Insight:** The `builtin` package already handles registration. Tests just need to import it. - ---- - -**Plan Status:** ✅ REVISED - ALL TEST FAILURES IDENTIFIED - READY FOR IMPLEMENTATION -**Next Action:** Execute Phase 0 verification, then begin Phase 1.1 (blank imports) -**Estimated Total Time:** 7-10 hours total -- Phases 0-3 (DNS + SQL injection): 3-5 hours (PR #461 original scope) -- Phase 4 (Security settings): 2-3 hours (PR #460) -- Phase 5 (Certificate lock): 1-2 hours (PR #460) -- Phase 6 (Frontend timeout): 0.5-1 hour (CI #20773147447) - -**CRITICAL CORRECTIONS SUMMARY:** -1. ✅ Fixed all package paths: `pkg/dnsprovider/builtin` (not `internal/dnsprovider/*`) -2. ✅ Simplified Phase 1: Blank imports FIRST, helper only if needed -3. ✅ Added Phase 0: Pre-implementation verification with evidence gathering -4. ✅ Enhanced Phase 3: Comprehensive validation (IP + action + string sanitization) -5. ✅ Corrected test expectations: 200 OR 400 are both valid (not just 400) - -## Executive Summary - -**Current Status**: 72.96% coverage (457 lines missing) -**Target**: 85%+ coverage -**Gap**: ~12% coverage increase needed -**Impact**: Approximately 90-110 additional test lines needed to reach target - -### Priority Breakdown - -| Priority | File | Missing Lines | Partials | Impact | Effort | -|----------|------|---------------|----------|--------|--------| -| **CRITICAL** | plugin_handler.go | 173 | 0 | 38% of gap | High | -| **HIGH** | credential_handler.go | 70 | 20 | 15% of gap | Medium | -| **HIGH** | caddy/config.go | 38 | 9 | 8% of gap | High | -| **MEDIUM** | caddy/manager_helpers.go | 28 | 10 | 6% of gap | Medium | -| **MEDIUM** | encryption_handler.go | 24 | 4 | 5% of gap | Low | -| **MEDIUM** | caddy/manager.go | 13 | 8 | 3% of gap | Medium | -| **MEDIUM** | audit_log_handler.go | 10 | 6 | 2% of gap | Low | -| **LOW** | settings_handler.go | 7 | 2 | 2% of gap | Low | -| **LOW** | crowdsec/hub_sync.go | 4 | 4 | 1% of gap | Low | -| **LOW** | routes/routes.go | 6 | 1 | 1% of gap | Low | - ---- - -## Phase 1: Critical Priority - Plugin Handler (0% Coverage) - -### File: `backend/internal/api/handlers/plugin_handler.go` - -**Status**: 0.00% coverage (173 lines missing) -**Priority**: CRITICAL - Highest impact -**Existing Tests**: None found -**New Test File**: `backend/internal/api/handlers/plugin_handler_test.go` - -#### Uncovered Functions - -1. **`NewPluginHandler(db, pluginLoader)`** - Constructor -2. **`ListPlugins(c *gin.Context)`** - GET /admin/plugins -3. **`GetPlugin(c *gin.Context)`** - GET /admin/plugins/:id -4. **`EnablePlugin(c *gin.Context)`** - POST /admin/plugins/:id/enable -5. **`DisablePlugin(c *gin.Context)`** - POST /admin/plugins/:id/disable -6. **`ReloadPlugins(c *gin.Context)`** - POST /admin/plugins/reload - -#### Test Strategy - -**Test Infrastructure Needed**: -- Mock `PluginLoaderService` for testing without filesystem -- Mock `dnsprovider.Global()` registry -- Test fixtures for plugin database records -- Gin test context helpers - -**Test Cases** (estimated 15-20 tests): - -##### ListPlugins Tests (5 tests) -```go -TestListPlugins_EmptyDatabase -TestListPlugins_BuiltInProvidersOnly -TestListPlugins_MixedBuiltInAndExternal -TestListPlugins_FailedPluginWithError -TestListPlugins_DatabaseReadError -``` - -##### GetPlugin Tests (4 tests) -```go -TestGetPlugin_Success -TestGetPlugin_InvalidID -TestGetPlugin_NotFound -TestGetPlugin_DatabaseError -``` - -##### EnablePlugin Tests (4 tests) -```go -TestEnablePlugin_Success -TestEnablePlugin_AlreadyEnabled -TestEnablePlugin_PluginLoadFailure -TestEnablePlugin_DatabaseError -``` - -##### DisablePlugin Tests (4 tests) -```go -TestDisablePlugin_Success -TestDisablePlugin_AlreadyDisabled -TestDisablePlugin_InUseByDNSProvider -TestDisablePlugin_DatabaseError -``` - -##### ReloadPlugins Tests (3 tests) -```go -TestReloadPlugins_Success -TestReloadPlugins_LoadError -TestReloadPlugins_NoPluginsDirectory -``` - -**Mocks Needed**: -```go -type MockPluginLoader struct { - LoadPluginFunc func(path string) error - UnloadPluginFunc func(providerType string) error - LoadAllFunc func() error - ListLoadedFunc func() []string -} - -type MockDNSProviderRegistry struct { - ListFunc func() []dnsprovider.ProviderPlugin - GetFunc func(providerType string) (dnsprovider.ProviderPlugin, bool) -} -``` - -**Estimated Coverage Gain**: +38% (173 lines) - ---- - -## Phase 2: High Priority - Credential Handler (32.83% Coverage) - -### File: `backend/internal/api/handlers/credential_handler.go` - -**Status**: 32.83% coverage (70 missing, 20 partials) -**Priority**: HIGH -**Existing Tests**: None found -**New Test File**: `backend/internal/api/handlers/credential_handler_test.go` - -#### Uncovered Functions - -All functions have partial coverage - error paths not tested: - -1. **`List(c *gin.Context)`** - GET /api/v1/dns-providers/:id/credentials -2. **`Create(c *gin.Context)`** - POST /api/v1/dns-providers/:id/credentials -3. **`Get(c *gin.Context)`** - GET /api/v1/dns-providers/:id/credentials/:cred_id -4. **`Update(c *gin.Context)`** - PUT /api/v1/dns-providers/:id/credentials/:cred_id -5. **`Delete(c *gin.Context)`** - DELETE /api/v1/dns-providers/:id/credentials/:cred_id -6. **`Test(c *gin.Context)`** - POST /api/v1/dns-providers/:id/credentials/:cred_id/test -7. **`EnableMultiCredentials(c *gin.Context)`** - POST /api/v1/dns-providers/:id/enable-multi-credentials - -#### Missing Coverage Areas - -- Invalid ID parameter handling -- Provider not found errors -- Multi-credential mode disabled errors -- Encryption failures -- Service layer error propagation - -#### Test Strategy - -**Test Cases** (estimated 21 tests): - -##### List Tests (3 tests) -```go -TestListCredentials_Success -TestListCredentials_InvalidProviderID -TestListCredentials_ProviderNotFound -TestListCredentials_MultiCredentialNotEnabled -``` - -##### Create Tests (4 tests) -```go -TestCreateCredential_Success -TestCreateCredential_InvalidProviderID -TestCreateCredential_InvalidCredentials -TestCreateCredential_EncryptionFailure -``` - -##### Get Tests (3 tests) -```go -TestGetCredential_Success -TestGetCredential_InvalidCredentialID -TestGetCredential_NotFound -``` - -##### Update Tests (4 tests) -```go -TestUpdateCredential_Success -TestUpdateCredential_InvalidCredentialID -TestUpdateCredential_InvalidCredentials -TestUpdateCredential_EncryptionFailure -``` - -##### Delete Tests (3 tests) -```go -TestDeleteCredential_Success -TestDeleteCredential_InvalidCredentialID -TestDeleteCredential_NotFound -``` - -##### Test Tests (3 tests) -```go -TestTestCredential_Success -TestTestCredential_InvalidCredentialID -TestTestCredential_TestFailure -``` - -##### EnableMultiCredentials Tests (1 test) -```go -TestEnableMultiCredentials_Success -TestEnableMultiCredentials_ProviderNotFound -``` - -**Mock Requirements**: -```go -type MockCredentialService struct { - ListFunc func(ctx context.Context, providerID uint) ([]models.DNSProviderCredential, error) - CreateFunc func(ctx context.Context, providerID uint, req CreateCredentialRequest) (*models.DNSProviderCredential, error) - GetFunc func(ctx context.Context, providerID, credentialID uint) (*models.DNSProviderCredential, error) - UpdateFunc func(ctx context.Context, providerID, credentialID uint, req UpdateCredentialRequest) (*models.DNSProviderCredential, error) - DeleteFunc func(ctx context.Context, providerID, credentialID uint) error - TestFunc func(ctx context.Context, providerID, credentialID uint) (*TestResult, error) -} -``` - -**Estimated Coverage Gain**: +15% (70 lines + 20 partials) - ---- - -## Phase 3: High Priority - Caddy Config Generation (79.82% Coverage) - -### File: `backend/internal/caddy/config.go` - -**Status**: 79.82% coverage (38 missing, 9 partials) -**Priority**: HIGH - Complex business logic -**Existing Tests**: Partial coverage exists -**Test File**: `backend/internal/caddy/config_test.go` (exists, needs expansion) - -#### Missing Coverage Areas - -**Functions with gaps**: -1. `GenerateConfig()` - Multi-credential DNS challenge logic (lines 140-230) -2. `buildWAFHandler()` - WAF ruleset selection logic (lines 850-920) -3. `buildRateLimitHandler()` - Bypass list parsing (lines 1020-1050) -4. `buildACLHandler()` - Geo-blocking CEL expression logic (lines 700-780) -5. `buildSecurityHeadersHandler()` - CSP/Permissions Policy building (lines 950-1010) - -#### Test Strategy - -**Test Cases** (estimated 12 tests): - -##### Multi-Credential DNS Challenge Tests (4 tests) -```go -TestGenerateConfig_MultiCredentialDNSChallenge_ZoneMatching -TestGenerateConfig_MultiCredentialDNSChallenge_WildcardMatching -TestGenerateConfig_MultiCredentialDNSChallenge_CatchAllCredential -TestGenerateConfig_MultiCredentialDNSChallenge_NoMatchingCredential -``` - -##### WAF Handler Tests (3 tests) -```go -TestBuildWAFHandler_RulesetPrioritySelection -TestBuildWAFHandler_PerRulesetModeOverride -TestBuildWAFHandler_EmptyDirectivesReturnsNil -``` - -##### Rate Limit Handler Tests (2 tests) -```go -TestBuildRateLimitHandler_WithBypassList -TestBuildRateLimitHandler_InvalidBypassCIDRs -``` - -##### ACL Handler Tests (2 tests) -```go -TestBuildACLHandler_GeoWhitelistCEL -TestBuildACLHandler_GeoBlacklistCEL -``` - -##### Security Headers Tests (1 test) -```go -TestBuildSecurityHeadersHandler_CSPAndPermissionsPolicy -``` - -**Test Fixtures**: -```go -type ConfigTestFixture struct { - Hosts []models.ProxyHost - DNSProviders []DNSProviderConfig - Rulesets []models.SecurityRuleSet - SecurityConfig *models.SecurityConfig - RulesetPaths map[string]string -} -``` - -**Estimated Coverage Gain**: +8% (38 lines + 9 partials) - ---- - -## Phase 4: Medium Priority - Remaining Handlers - -### Summary Table - -| File | Coverage | Missing | Priority | Tests | Gain | -|------|----------|---------|----------|-------|------| -| manager_helpers.go | 59.57% | 28+10 | Medium | 8 | +6% | -| encryption_handler.go | 78.29% | 24+4 | Medium | 6 | +5% | -| manager.go | 76.13% | 13+8 | Medium | 5 | +3% | -| audit_log_handler.go | 78.08% | 10+6 | Medium | 4 | +2% | -| settings_handler.go | 84.48% | 7+2 | Low | 3 | +2% | -| hub_sync.go | 80.48% | 4+4 | Low | 2 | +1% | -| routes.go | 89.06% | 6+1 | Low | 2 | +1% | - -### Details - -#### manager_helpers.go -**Functions**: `extractBaseDomain()`, `matchesZoneFilter()`, `getCredentialForDomain()` -**Strategy**: Edge case testing for domain matching logic - -#### encryption_handler.go -**Functions**: All functions - admin check errors -**Strategy**: Non-admin user tests, error path tests - -#### manager.go -**Functions**: `ApplyConfig()`, `computeEffectiveFlags()` -**Strategy**: Error path coverage for rollback scenarios - -#### audit_log_handler.go -**Functions**: All functions - error paths -**Strategy**: Service layer error propagation tests - -#### settings_handler.go -**Functions**: `TestPublicURL()` - SSRF validation -**Strategy**: Security validation edge cases - -#### hub_sync.go -**Functions**: `validateHubURL()` - edge cases -**Strategy**: URL validation security tests - -#### routes.go -**Functions**: `Register()` - error paths -**Strategy**: Initialization error handling tests - ---- - -## Test Infrastructure & Patterns - -### Existing Test Helpers - -1. **`testutil.GetTestTx(t, db)`** - Transaction-based test isolation -2. **`testutil.WithTx(t, db, fn)`** - Transaction wrapper for tests -3. Shared DB pattern for fast parallel tests - -### Required New Test Infrastructure - -#### 1. Gin Test Helpers -```go -// backend/internal/testutil/gin.go -func NewTestGinContext() (*gin.Context, *httptest.ResponseRecorder) -func SetGinContextUser(c *gin.Context, userID uint, role string) -func SetGinContextParam(c *gin.Context, key, value string) -``` - -#### 2. Mock DNS Provider Registry -```go -// backend/internal/testutil/dns_mocks.go -type MockDNSProviderRegistry struct { ... } -func NewMockDNSProviderRegistry() *MockDNSProviderRegistry -``` - -#### 3. Mock Plugin Loader -```go -// backend/internal/testutil/plugin_mocks.go -type MockPluginLoader struct { ... } -func NewMockPluginLoader() *MockPluginLoader -``` - -#### 4. Test Fixtures -```go -// backend/internal/testutil/fixtures.go -func CreateTestDNSProvider(tx *gorm.DB) *models.DNSProvider -func CreateTestProxyHost(tx *gorm.DB) *models.ProxyHost -func CreateTestPlugin(tx *gorm.DB) *models.Plugin -``` - ---- - -## Implementation Plan - -### Week 1: Critical Priority -- **Day 1-2**: Set up test infrastructure (Gin helpers, mocks) -- **Day 3-5**: Implement `plugin_handler_test.go` (173 lines) -- **Target**: +38% coverage - -### Week 2: High Priority Part 1 -- **Day 1-3**: Implement `credential_handler_test.go` (70 lines + 20 partials) -- **Day 4-5**: Start `config_test.go` expansion (38 lines + 9 partials) -- **Target**: +20% coverage (cumulative: 58%) - -### Week 3: High Priority Part 2 & Medium Priority -- **Day 1-2**: Complete `config_test.go` -- **Day 3-5**: Implement remaining medium priority handlers -- **Target**: +15% coverage (cumulative: 73%) - -### Week 4: Low Priority & Buffer -- **Day 1-2**: Implement low priority handlers -- **Day 3-5**: Buffer time for test failures, refactoring, CI integration -- **Target**: +12% coverage (cumulative: 85%+) - ---- - -## Coverage Validation Strategy - -### CI Integration -1. Add coverage threshold check to GitHub Actions workflow -2. Fail builds if coverage drops below 85% -3. Generate coverage reports as PR comments - -### Coverage Verification Commands -```bash -# Run full test suite with coverage -go test -v -race -coverprofile=coverage.out ./... - -# Generate HTML coverage report -go tool cover -html=coverage.out -o coverage.html - -# Check coverage by file -go tool cover -func=coverage.out | grep -E "(plugin_handler|credential_handler|config)" - -# Verify 85% threshold -COVERAGE=$(go tool cover -func=coverage.out | tail -1 | awk '{print $3}' | sed 's/%//') -if (( $(echo "$COVERAGE < 85" | bc -l) )); then - echo "Coverage $COVERAGE% is below 85% threshold" - exit 1 -fi -``` - ---- - -## Risk Assessment & Mitigation - -### Risks - -1. **Plugin Handler Complexity** - No existing test patterns for plugin system - - *Mitigation*: Start with simple mock-based tests, iterate - -2. **Caddy Config Generation Complexity** - 1000+ line function with many branches - - *Mitigation*: Focus on untested branches only, use table tests - -3. **Test Flakiness** - Network/filesystem dependencies - - *Mitigation*: Use mocks for external dependencies, in-memory DB for tests - -4. **Time Constraints** - 457 lines to cover in 4 weeks - - *Mitigation*: Prioritize high-impact files first, parallelize work - -### Success Criteria - -- [ ] 85%+ overall coverage achieved -- [ ] All critical files (plugin_handler, credential_handler, config) have >80% coverage -- [ ] All tests pass on CI with race detection enabled -- [ ] No test flakiness observed over 10 consecutive CI runs -- [ ] Coverage reports integrated into PR workflow - ---- - -## Notes - -- **Test Philosophy**: Focus on business logic and error paths, not just happy paths -- **Performance**: Use transaction-based test isolation for speed (testutil.GetTestTx) -- **Security**: Ensure SSRF validation and auth checks are thoroughly tested -- **Documentation**: Add godoc comments to test functions explaining what they test - ---- - -## Appendix: Quick Reference - -### Test File Locations -``` -backend/internal/api/handlers/ - plugin_handler_test.go (NEW - Phase 1) - credential_handler_test.go (NEW - Phase 2) - encryption_handler_test.go (EXPAND - Phase 4) - audit_log_handler_test.go (EXPAND - Phase 4) - settings_handler_test.go (EXPAND - Phase 4) - -backend/internal/caddy/ - config_test.go (EXPAND - Phase 3) - manager_helpers_test.go (NEW - Phase 4) - manager_test.go (EXPAND - Phase 4) - -backend/internal/crowdsec/ - hub_sync_test.go (EXPAND - Phase 4) - -backend/internal/api/routes/ - routes_test.go (NEW - Phase 4) -``` - -### Command Reference -```bash -# Run tests for specific file -go test -v ./backend/internal/api/handlers -run TestPluginHandler - -# Run tests with race detection -go test -race ./... - -# Generate coverage for specific package -go test -coverprofile=plugin.cover ./backend/internal/api/handlers -go tool cover -html=plugin.cover - -# Run all tests and check threshold -make test-coverage-check -``` +Run these tasks in order: ---- +1. **Test: Backend with Coverage** + - Pass criteria: task succeeds; `backend/coverage.txt` generated; zero failing tests. + - Outcome criteria: Codecov patch status becomes green (100% patch coverage). -**Document Status**: Draft v1.0 -**Created**: 2026-01-07 -**Last Updated**: 2026-01-07 -**Next Review**: After Phase 1 completion +2. **Lint: Pre-commit (All Files)** (optional; general DoD) + - Pass criteria: all hooks pass. diff --git a/docs/reports/qa_report.md b/docs/reports/qa_report.md index dd01df18..8fc49447 100644 --- a/docs/reports/qa_report.md +++ b/docs/reports/qa_report.md @@ -1,261 +1,115 @@ -# QA Report: Test Failure Resolution and Coverage Boost +# QA/Security DoD Validation Report -**Date**: January 7, 2026 -**PR**: #461 - DNS Challenge Support for Wildcard Certificates -**Branch**: feature/beta-release -**Status**: ✅ PASS +**Date**: 2026-01-09 +**Scope**: DoD validation rerun (backend tests + lint + security scans) +**Overall Status**: ❌ FAIL ---- +## Summary -## Executive Summary +All requested tasks completed successfully (no task execution failures). However, DoD fails due to **HIGH/CRITICAL security findings** in CodeQL and Trivy outputs. -All 30 originally failing tests have been fixed, backend coverage boosted from 82.7% to 85.2%, and all security scans passed with zero HIGH/CRITICAL findings. The codebase is ready for merge. +## Frontend Change Check ---- +**Result**: No frontend files detected as changed (no paths under `frontend/` in current workspace changes). -## Test Coverage Results +**Action**: Per request, skipped: +- Test: Frontend with Coverage +- Lint: TypeScript Check -### Backend Coverage: 85.2% ✅ +Note: the pre-commit run includes a frontend TypeScript check hook, but it is not a substitute for the explicit “Frontend with Coverage” task if frontend source changes are present. -- **Target**: 85% -- **Achieved**: 85.2% (+0.2% margin) -- **Tests Run**: All backend packages -- **Status**: PASSED +## Task Results (Required) -**Improvements Made**: -- Excluded `pkg/dnsprovider/builtin` from coverage (integration-tested, not unit-tested) -- Added comprehensive tests to `internal/services` and `internal/api/handlers` -- Focus on error paths, edge cases, and validation logic +### 1) Test: Backend with Coverage -**Key Package Coverage**: -- `internal/api/handlers`: 85%+ (was 81.9%) -- `internal/services`: 85%+ (was 80.7%) -- `internal/caddy`: 94.4% -- `internal/cerberus`: 100% -- `internal/config`: 100% -- `internal/models`: 96.4% +**Pass/Fail Criteria**: +- PASS if task exits successfully and produces a coverage result. -### Frontend Coverage: 85.65% ✅ +**Result**: ✅ PASS (task completed) -- **Target**: 85% -- **Achieved**: 85.65% (+0.65% margin) -- **Tests Run**: 119 tests across 5 test files -- **Status**: PASSED +**Coverage**: +- Backend total coverage (from `go tool cover -func backend/coverage.txt`): **86.6%** +- Task output included: `coverage: 63.2% of statements` (package `backend/cmd/seed`) ---- +### 2) Lint: Pre-commit (All Files) -## Test Fixes Summary +**Pass/Fail Criteria**: +- PASS if all hooks complete successfully. -### Phase 1: DNS Provider Registry Initialization (18 tests) -**Files Modified**: -- `backend/internal/api/handlers/credential_handler_test.go` -- `backend/internal/caddy/manager_multicred_integration_test.go` -- `backend/internal/caddy/config_patch_coverage_test.go` -- `backend/internal/services/dns_provider_service_test.go` +**Result**: ✅ PASS -**Fix**: Added blank import `_ "github.com/Wikid82/charon/backend/pkg/dnsprovider/builtin"` to trigger DNS provider registry initialization +### 3) Security: CodeQL All (CI-Aligned) -### Phase 2: Credential Field Name Corrections (4 tests) -**File**: `backend/internal/services/dns_provider_service_test.go` +**Pass/Fail Criteria**: +- PASS if no HIGH/CRITICAL findings are present. -**Fixes**: -- Hetzner: `api_key` → `api_token` -- DigitalOcean: `auth_token` → `api_token` -- DNSimple: `oauth_token` → `api_token` +**Result**: ❌ FAIL -### Phase 3: Security Handler Input Validation (1 test) -**File**: `backend/internal/api/handlers/security_handler.go` +**Findings**: +- Go SARIF (`codeql-results-go.sarif`): **3 CRITICAL** (security severity 9.8) + - Rule: `go/email-injection` (“Email content injection”) + - Location: `backend/internal/services/mail_service.go` (lines ~222, ~340, ~393) +- JS SARIF (`codeql-results-js.sarif`): **1 HIGH** (security severity 7.8) + - Rule: `js/incomplete-hostname-regexp` (“Incomplete regular expression for hostnames”) + - Location: `frontend/src/pages/__tests__/ProxyHosts-extra.test.tsx` (line ~252) -**Fix**: Added comprehensive input validation: -- `isValidIP()` - IP format validation -- `isValidCIDR()` - CIDR notation validation -- `isValidAction()` - Action enum validation (block/allow/captcha) -- `sanitizeString()` - Input sanitization +### 4) Security: Trivy Scan -### Phase 4: Security Settings Database Override (5 tests) -**File**: `backend/internal/testutil/db.go` +**Pass/Fail Criteria**: +- PASS if no HIGH/CRITICAL findings are present. -**Fix**: Added SQLite `_txlock=immediate` parameter to prevent database lock contention +**Result**: ❌ FAIL -### Phase 5: Certificate Deletion Race Condition (1 test) -**File**: Already fixed in previous PR +**Counts (from existing artifacts)**: +- `trivy-scan-output.txt`: **CRITICAL=1**, **HIGH=7** +- `trivy-image-scan.txt`: **CRITICAL=0**, **HIGH=1** -### Phase 6: Frontend LiveLogViewer Timeout (1 test) -**Status**: Already fixed in previous PR +## Root Cause (Why DoD Failed) -### Coverage Boost Tests -**Files Created/Modified**: -- `backend/internal/services/coverage_boost_test.go` - Service accessor and error path tests -- `backend/internal/api/handlers/plugin_handler_test.go` - Complete plugin handler coverage +### CodeQL -**New Tests Added**: 40+ test cases covering: -- Service accessors (DB(), Get*(), List*()) -- Error handling for missing resources -- Plugin enable/disable/reload operations -- Notification provider lifecycle -- Security service configuration -- Mail service SMTP error paths -- GeoIP service validation +1) **CRITICAL** `go/email-injection` in `backend/internal/services/mail_service.go` ---- +**Likely cause**: user-controlled or otherwise untrusted values are being used to build email content (and potentially headers) without robust validation/normalization, enabling header/body injection (e.g., newline injection). -## Security Scan Results +2) **HIGH** `js/incomplete-hostname-regexp` in a frontend test -### CodeQL Analysis ✅ +**Likely cause**: a regex used for host matching in tests does not escape `.`, so it matches more than intended. -**Go Scan**: -- Queries Run: 61 -- Errors: 0 -- Warnings: 0 -- Notes: 0 -- **Status**: PASSED +### Trivy -**JavaScript Scan**: -- Queries Run: 88 -- Errors: 0 -- Warnings: 0 -- Notes: 1 (regex pattern in test file - non-blocking) -- **Status**: PASSED - -**Total Findings**: 0 blocking issues - -### Trivy Container Scan -**Status**: Not run (Docker build verified locally, no containers built for this QA run) +**Likely cause**: one or more dependencies in the repo (Go modules and/or image contents) are pinned to vulnerable versions. -### Go Vulnerability Check (govulncheck) -**Status**: Not run (can be run in CI) - ---- - -## Pre-commit Hooks ✅ - -**Status**: PASSED - -**Hooks Verified**: -- ✅ Fix end of files -- ✅ Trim trailing whitespace -- ✅ Check YAML -- ✅ Check for added large files -- ✅ Dockerfile validation -- ✅ Go Vet -- ✅ Check .version matches Git tag -- ✅ Prevent large files not tracked by LFS -- ✅ Prevent committing CodeQL DB artifacts -- ✅ Prevent committing data/backups files -- ✅ Frontend TypeScript Check -- ✅ Frontend Lint (Fix) +Examples extracted from `trivy-scan-output.txt` / `trivy-image-scan.txt` include (non-exhaustive): +- `golang.org/x/crypto` (CVE-2024-45337 CRITICAL; CVE-2025-22869 HIGH) +- `golang.org/x/net` (CVE-2023-39325 HIGH) +- `golang.org/x/oauth2` (CVE-2025-22868 HIGH) +- `gopkg.in/yaml.v3` (CVE-2022-28948 HIGH) +- `github.com/quic-go/quic-go` (CVE-2025-59530 HIGH) +- `github.com/expr-lang/expr` (CVE-2025-68156 HIGH) ---- +## Proposed Remediation (No changes applied) -## Type Safety ✅ +Per instruction: **no fixes were made**. Suggested remediation steps: -### Backend (Go) -- **Status**: PASSED -- All packages compile successfully -- No type errors +### For CodeQL `go/email-injection` -### Frontend (TypeScript) -- **Status**: PASSED -- TypeScript 5.x type check passed -- All imports resolve correctly -- No type errors +- Validate/normalize any untrusted values used in mail headers/body (especially ensuring values do not contain `\r`/`\n`). +- Use strict email address parsing/validation (e.g., Go `net/mail`) and explicit header encoding. +- Ensure subject/from/to/reply-to fields are constructed via safe libraries and reject control characters. ---- +### For CodeQL `js/incomplete-hostname-regexp` -## Issues Found and Resolved +- Update the test regex to escape `.` and/or use a safer matcher; rerun CodeQL JS scan. -### Issue 1: Mock DNS Provider Missing Interface Methods -**Severity**: High (compilation error) -**Location**: `backend/internal/api/handlers/plugin_handler_test.go` -**Root Cause**: `mockDNSProvider` was missing `Init()`, `Cleanup()`, and other interface methods -**Resolution**: Added all required `ProviderPlugin` interface methods to mock -**Status**: FIXED +### For Trivy findings -### Issue 2: Time Package Import Missing -**Severity**: Low (compilation error) -**Location**: `backend/internal/api/handlers/plugin_handler_test.go` -**Root Cause**: Mock methods return `time.Duration` but package not imported -**Resolution**: Added `time` to imports -**Status**: FIXED +- Upgrade impacted Go modules to versions containing fixes (follow Trivy “Fixed Version” guidance) and run `go mod tidy`. +- Re-run Trivy scan after dependency upgrades. +- If image findings remain: rebuild the image after base image upgrades and/or OS package updates. ---- +## Artifacts -## Files Modified - -### Configuration Files -- `.codecov.yml` - Added DNS provider builtin package exclusion -- `scripts/go-test-coverage.sh` - Added DNS provider to exclusion list - -### Test Files -- `backend/internal/api/handlers/credential_handler_test.go` - Added blank import -- `backend/internal/caddy/manager_multicred_integration_test.go` - Added blank import -- `backend/internal/caddy/config_patch_coverage_test.go` - Added blank import -- `backend/internal/services/dns_provider_service_test.go` - Fixed credential fields + blank import -- `backend/internal/services/coverage_boost_test.go` - NEW (service tests) -- `backend/internal/api/handlers/plugin_handler_test.go` - NEW (handler tests) - -### Source Files -- `backend/internal/api/handlers/security_handler.go` - Added input validation -- `backend/internal/api/handlers/security_handler_audit_test.go` - Fixed test action value -- `backend/internal/testutil/db.go` - Added SQLite txlock parameter - ---- - -## Test Execution Summary - -### Backend -- **Total Packages Tested**: 25+ -- **Coverage**: 85.2% -- **All Tests**: PASSED -- **Execution Time**: ~30s - -### Frontend -- **Test Files**: 5 -- **Tests Run**: 119 -- **Tests Passed**: 119 -- **Tests Failed**: 0 -- **Coverage**: 85.65% -- **Execution Time**: ~12 minutes - ---- - -## Deployment Readiness Checklist - -- [x] All original failing tests fixed (30/30) -- [x] Backend coverage >= 85% (85.2%) -- [x] Frontend coverage >= 85% (85.65%) -- [x] Security scans passed (0 HIGH/CRITICAL) -- [x] Pre-commit hooks passed -- [x] Type checks passed (Go + TypeScript) -- [x] No compilation errors -- [x] Code follows project conventions -- [x] Tests are meaningful and maintainable - ---- - -## Recommendations - -1. **Merge Ready**: All blocking issues resolved, code is production-ready -2. **Monitor CI**: Verify Docker build passes in CI (tested locally) -3. **Follow-up**: Consider adding more integration tests for DNS provider implementations in a future PR -4. **Documentation**: Update user-facing docs to mention DNS challenge support for wildcards - ---- - -## Conclusion - -**FINAL VERDICT**: ✅ PASS - -All Definition of Done criteria met: -- ✅ Coverage tests passed (backend 85.2%, frontend 85.65%) -- ✅ Type safety verified -- ✅ Pre-commit hooks passed -- ✅ Security scans clean (0 HIGH/CRITICAL findings) -- ✅ All tests passing - -The PR is approved for merge from a quality assurance perspective. - ---- - -**QA Engineer**: Engineering Director (Management Mode) -**Sign-off Date**: January 7, 2026 +- Backend coverage profile: `backend/coverage.txt` +- CodeQL results: `codeql-results-go.sarif`, `codeql-results-js.sarif`, `codeql-results-javascript.sarif` +- Trivy results: `trivy-scan-output.txt`, `trivy-image-scan.txt`