From dfe01f3dbbe3ee3dc91114380bf40b02ed9d2742 Mon Sep 17 00:00:00 2001 From: Jeremy Hatfield Date: Mon, 9 Nov 2026 15:17:10 +0000 Subject: [PATCH 1/4] test: pin the routes generated for a proxy host --- backend/internal/caddy/config.go | 55 ++++---- .../internal/caddy/config_crowdsec_test.go | 10 +- backend/internal/caddy/config_extra_test.go | 26 ++-- .../caddy/config_generate_additional_test.go | 26 ++-- .../internal/caddy/config_generate_test.go | 11 +- .../internal/caddy/config_redirect_test.go | 4 +- .../caddy/config_routes_pipeline_test.go | 129 ++++++++++++++++++ .../caddy/config_security_headers_test.go | 2 +- backend/internal/caddy/config_test.go | 54 ++++---- .../caddy/config_waf_rules_required_test.go | 4 +- backend/internal/caddy/self_upstream_test.go | 8 +- 11 files changed, 231 insertions(+), 98 deletions(-) create mode 100644 backend/internal/caddy/config_routes_pipeline_test.go diff --git a/backend/internal/caddy/config.go b/backend/internal/caddy/config.go index bef2e008f..a9d7e6ec8 100644 --- a/backend/internal/caddy/config.go +++ b/backend/internal/caddy/config.go @@ -6,6 +6,7 @@ import ( "net" "os" "path/filepath" + "slices" "strconv" "strings" @@ -32,6 +33,14 @@ const serverShutdownGracePeriod = "5s" // but no bouncer key exists yet, so CrowdSec is left out of the proxy config. const crowdSecNoKeyWarning = "CrowdSec is on but no bouncer key exists yet; CrowdSec is left out of the proxy configuration until a key is saved" +// resetRecoveryPaths are the only paths the recovery route for the reset endpoint +// matches. They are exact paths with no wildcard, so nothing under the prefix +// (token endpoints included) takes the route. +var resetRecoveryPaths = []string{ + "/api/v1/emergency/security-reset", + "/emergency/security-reset", +} + // GenerateConfig creates a Caddy JSON configuration from proxy hosts. // This is the core transformation layer from our database model to Caddy config. func GenerateConfig(hosts []models.ProxyHost, storageDir, acmeEmail, frontendDir, sslProvider string, acmeStaging, crowdsecEnabled, wafEnabled, rateLimitEnabled, aclEnabled bool, adminWhitelist string, rulesets []models.SecurityRuleSet, rulesetPaths map[string]string, decisions []models.SecurityDecision, secCfg *models.SecurityConfig, dnsProviderConfigs []DNSProviderConfig, opts ...GenerateConfigOption) (*Config, error) { @@ -662,31 +671,31 @@ func GenerateConfig(hosts []models.ProxyHost, storageDir, acmeEmail, frontendDir // Build main handlers: security pre-handlers, other host-level handlers, then reverse proxy // Determine if standard headers should be enabled (default true if nil) enableStdHeaders := host.EnableStandardHeaders == nil || *host.EnableStandardHeaders - emergencyPaths := []string{ - "/api/v1/emergency/security-reset", - "/api/v1/emergency/*", - "/emergency/security-reset", - "/emergency/*", - } - emergencyHandlers := append(append([]Handler{}, handlers...), resolvedOpts.proxyHandler(dial, resolvedOpts.isRemoteHost(host.UUID), host.Application, enableStdHeaders)) - emergencyRoute := &Route{ - Match: []Match{ - { - Host: uniqueDomains, - Path: emergencyPaths, + // Recovery route for the reset endpoint. It skips the security pre-handlers, + // so it is emitted only for a host whose upstream is Charon itself, and it + // matches the exact reset paths only. The reset endpoint performs its own + // token and management-network checks. + if resolvedOpts.selfHop.isSelfUpstream(dial, resolvedOpts.isRemoteHost(host.UUID)) { + recoveryHandlers := append(append([]Handler{}, handlers...), resolvedOpts.proxyHandler(dial, resolvedOpts.isRemoteHost(host.UUID), host.Application, enableStdHeaders)) + recoveryRoute := &Route{ + Match: []Match{ + { + Host: uniqueDomains, + Path: slices.Clone(resetRecoveryPaths), + }, }, - }, - Handle: emergencyHandlers, - Terminal: true, + Handle: recoveryHandlers, + Terminal: true, + } + logger.Log().WithFields(map[string]any{ + "host_id": host.ID, + "host_uuid": host.UUID, + "unique_domains": uniqueDomains, + "has_paths": true, + "path_count": len(resetRecoveryPaths), + }).Debug("[CONFIG DEBUG] Creating recovery route for the reset endpoint") + routes = append(routes, recoveryRoute) } - logger.Log().WithFields(map[string]any{ - "host_id": host.ID, - "host_uuid": host.UUID, - "unique_domains": uniqueDomains, - "has_paths": true, - "path_count": len(emergencyPaths), - }).Debug("[CONFIG DEBUG] Creating EMERGENCY route") - routes = append(routes, emergencyRoute) mainHandlers := append(append([]Handler{}, securityHandlers...), handlers...) mainHandlers = append(mainHandlers, resolvedOpts.proxyHandler(dial, resolvedOpts.isRemoteHost(host.UUID), host.Application, enableStdHeaders)) diff --git a/backend/internal/caddy/config_crowdsec_test.go b/backend/internal/caddy/config_crowdsec_test.go index 96f734b1c..088b61341 100644 --- a/backend/internal/caddy/config_crowdsec_test.go +++ b/backend/internal/caddy/config_crowdsec_test.go @@ -134,10 +134,10 @@ func TestGenerateConfig_WithCrowdSec(t *testing.T) { assert.Contains(t, server.TrustedProxies.Ranges, "10.0.0.0/8", "Should trust private networks") assert.Contains(t, server.TrustedProxies.Ranges, "192.168.0.0/16", "Should trust private networks") - // Check handler is minimal (2 routes: emergency + main) - require.Len(t, server.Routes, 2) + // Check handler is minimal (1 route: main) + require.Len(t, server.Routes, 1) - route := server.Routes[1] // Main route is at index 1 + route := server.Routes[0] // Main route is at index 0 // Handlers should include crowdsec + reverse_proxy require.GreaterOrEqual(t, len(route.Handle), 2) @@ -177,9 +177,9 @@ func TestGenerateConfig_CrowdSecDisabled(t *testing.T) { server := config.Apps.HTTP.Servers["charon_server"] require.NotNil(t, server) - require.Len(t, server.Routes, 2) // 2 routes: emergency + main + require.Len(t, server.Routes, 1) // 1 route: main - route := server.Routes[1] // Main route is at index 1 + route := server.Routes[0] // Main route is at index 0 // Verify no crowdsec handler for _, h := range route.Handle { diff --git a/backend/internal/caddy/config_extra_test.go b/backend/internal/caddy/config_extra_test.go index b8bbe3a40..f33f23cf5 100644 --- a/backend/internal/caddy/config_extra_test.go +++ b/backend/internal/caddy/config_extra_test.go @@ -37,9 +37,9 @@ func TestGenerateConfig_AdvancedInvalidJSON(t *testing.T) { require.NoError(t, err) server := cfg.Apps.HTTP.Servers["charon_server"] require.NotNil(t, server) - // Main route should still have ReverseProxy as last handler (2 routes: emergency + main) - require.Len(t, server.Routes, 2) - route := server.Routes[1] // Main route is at index 1 + // Main route should still have ReverseProxy as last handler (1 route: main) + require.Len(t, server.Routes, 1) + route := server.Routes[0] // Main route is at index 0 last := route.Handle[len(route.Handle)-1] require.Equal(t, "reverse_proxy", last["handler"]) } @@ -68,7 +68,7 @@ func TestGenerateConfig_AdvancedArrayHandler(t *testing.T) { require.NoError(t, err) server := cfg.Apps.HTTP.Servers["charon_server"] require.NotNil(t, server) - route := server.Routes[1] // Main route is at index 1 (after emergency route) + route := server.Routes[0] // Main route is at index 0 (no emergency route for a non-Charon upstream) // First handler should be our headers handler first := route.Handle[0] require.Equal(t, "headers", first["handler"]) @@ -80,7 +80,7 @@ func TestGenerateConfig_LowercaseDomains(t *testing.T) { } cfg, err := GenerateConfig(hosts, "/tmp/caddy-data", "", "", "", false, false, false, false, false, "", nil, nil, nil, nil, nil) require.NoError(t, err) - route := cfg.Apps.HTTP.Servers["charon_server"].Routes[1] + route := cfg.Apps.HTTP.Servers["charon_server"].Routes[0] // Debug prints removed require.Equal(t, []string{"upper.example.com"}, route.Match[0].Host) } @@ -96,7 +96,7 @@ func TestGenerateConfig_AdvancedObjectHandler(t *testing.T) { } cfg, err := GenerateConfig([]models.ProxyHost{host}, "/tmp/caddy-data", "", "", "", false, false, false, false, true, "", nil, nil, nil, nil, nil) require.NoError(t, err) - route := cfg.Apps.HTTP.Servers["charon_server"].Routes[1] + route := cfg.Apps.HTTP.Servers["charon_server"].Routes[0] // First handler should be headers first := route.Handle[0] require.Equal(t, "headers", first["handler"]) @@ -113,7 +113,7 @@ func TestGenerateConfig_AdvancedHeadersStringToArray(t *testing.T) { } cfg, err := GenerateConfig([]models.ProxyHost{host}, "/tmp/caddy-data", "", "", "", false, false, false, false, true, "", nil, nil, nil, nil, nil) require.NoError(t, err) - route := cfg.Apps.HTTP.Servers["charon_server"].Routes[1] + route := cfg.Apps.HTTP.Servers["charon_server"].Routes[0] // Debug prints removed first := route.Handle[0] require.Equal(t, "headers", first["handler"]) @@ -174,7 +174,7 @@ func TestGenerateConfig_ACLWhitelistIncluded(t *testing.T) { require.NotNil(t, aclH) cfg, err := GenerateConfig([]models.ProxyHost{host}, "/tmp/caddy-data", "", "", "", false, false, false, false, false, "", nil, nil, nil, nil, nil) require.NoError(t, err) - route := cfg.Apps.HTTP.Servers["charon_server"].Routes[1] + route := cfg.Apps.HTTP.Servers["charon_server"].Routes[0] // Accept either a subroute (ACL) or reverse_proxy as first handler first := route.Handle[0] if first["handler"] != "subroute" { @@ -186,7 +186,7 @@ func TestGenerateConfig_SkipsEmptyDomainEntries(t *testing.T) { hosts := []models.ProxyHost{{UUID: "u1", DomainNames: ", test.example.com", ForwardHost: "a", ForwardPort: 80, Enabled: true}} cfg, err := GenerateConfig(hosts, "/tmp/caddy-data", "", "", "", false, false, false, false, false, "", nil, nil, nil, nil, nil) require.NoError(t, err) - route := cfg.Apps.HTTP.Servers["charon_server"].Routes[1] + route := cfg.Apps.HTTP.Servers["charon_server"].Routes[0] require.Equal(t, []string{"test.example.com"}, route.Match[0].Host) } @@ -194,7 +194,7 @@ func TestGenerateConfig_AdvancedNoHandlerKey(t *testing.T) { host := models.ProxyHost{UUID: "adv3", DomainNames: "nohandler.example.com", ForwardHost: "app", ForwardPort: 8080, Enabled: true, AdvancedConfig: `{"foo":"bar"}`} cfg, err := GenerateConfig([]models.ProxyHost{host}, "/tmp/caddy-data", "", "", "", false, false, false, false, false, "", nil, nil, nil, nil, nil) require.NoError(t, err) - route := cfg.Apps.HTTP.Servers["charon_server"].Routes[1] + route := cfg.Apps.HTTP.Servers["charon_server"].Routes[0] // No headers handler appended; last handler is reverse_proxy last := route.Handle[len(route.Handle)-1] require.Equal(t, "reverse_proxy", last["handler"]) @@ -204,7 +204,7 @@ func TestGenerateConfig_AdvancedUnexpectedJSONStructure(t *testing.T) { host := models.ProxyHost{UUID: "adv4", DomainNames: "struct.example.com", ForwardHost: "app", ForwardPort: 8080, Enabled: true, AdvancedConfig: `42`} cfg, err := GenerateConfig([]models.ProxyHost{host}, "/tmp/caddy-data", "", "", "", false, false, false, false, false, "", nil, nil, nil, nil, nil) require.NoError(t, err) - route := cfg.Apps.HTTP.Servers["charon_server"].Routes[1] + route := cfg.Apps.HTTP.Servers["charon_server"].Routes[0] // Expect main reverse proxy handler exists but no appended advanced handler last := route.Handle[len(route.Handle)-1] require.Equal(t, "reverse_proxy", last["handler"]) @@ -232,7 +232,7 @@ func TestGenerateConfig_SecurityPipeline_Order(t *testing.T) { secCfg := &models.SecurityConfig{CrowdSecMode: "local", RateLimitRequests: 100, RateLimitWindowSec: 60} cfg, err := GenerateConfig([]models.ProxyHost{host}, "/tmp/caddy-data", "", "", "", false, true, true, true, true, "", rulesets, rulesetPaths, nil, secCfg, nil, WithBouncerKeyPath(keyPath)) require.NoError(t, err) - route := cfg.Apps.HTTP.Servers["charon_server"].Routes[1] + route := cfg.Apps.HTTP.Servers["charon_server"].Routes[0] // Extract handler names names := []string{} @@ -255,7 +255,7 @@ func TestGenerateConfig_SecurityPipeline_OmitWhenDisabled(t *testing.T) { host := models.ProxyHost{UUID: "pipe2", DomainNames: "pipe2.example.com", Enabled: true, ForwardHost: "app", ForwardPort: 8080} cfg, err := GenerateConfig([]models.ProxyHost{host}, "/tmp/caddy-data", "", "", "", false, false, false, false, false, "", nil, nil, nil, nil, nil) require.NoError(t, err) - route := cfg.Apps.HTTP.Servers["charon_server"].Routes[1] + route := cfg.Apps.HTTP.Servers["charon_server"].Routes[0] // Extract handler names names := []string{} diff --git a/backend/internal/caddy/config_generate_additional_test.go b/backend/internal/caddy/config_generate_additional_test.go index b785e171f..c7575056f 100644 --- a/backend/internal/caddy/config_generate_additional_test.go +++ b/backend/internal/caddy/config_generate_additional_test.go @@ -118,7 +118,7 @@ func TestGenerateConfig_ACLHandlerIncluded(t *testing.T) { require.NoError(t, err) server := cfg.Apps.HTTP.Servers["charon_server"] require.NotNil(t, server) - route := server.Routes[1] // Main route is at index 1 (after emergency route) + route := server.Routes[0] // Main route is at index 0 (no emergency route for a non-Charon upstream) // Extract handler names names := []string{} @@ -144,7 +144,7 @@ func TestGenerateConfig_DecisionsBlockWithAdminExclusion(t *testing.T) { dec := models.SecurityDecision{Action: "block", IP: "1.2.3.4"} cfg, err := GenerateConfig([]models.ProxyHost{host}, "/tmp/caddy-data", "", "", "", false, false, false, false, false, "10.0.0.1/32", nil, nil, []models.SecurityDecision{dec}, nil, nil) require.NoError(t, err) - route := cfg.Apps.HTTP.Servers["charon_server"].Routes[1] // Main route is at index 1 + route := cfg.Apps.HTTP.Servers["charon_server"].Routes[0] // Main route is at index 0 b, _ := json.MarshalIndent(route.Handle, "", " ") t.Logf("handles: %s", string(b)) // Expect first security handler is a subroute that includes both remote_ip and a 'not' exclusion for adminWhitelist @@ -176,7 +176,7 @@ func TestGenerateConfig_WAFModeAndRulesetReference(t *testing.T) { require.NoError(t, err) // Since a ruleset name was requested but none exists, NO waf handler should be created // (Bug fix: don't create a no-op WAF handler without directives) - route := cfg.Apps.HTTP.Servers["charon_server"].Routes[1] // Main route after emergency + route := cfg.Apps.HTTP.Servers["charon_server"].Routes[0] // Main route for _, h := range route.Handle { if hn, ok := h["handler"].(string); ok && hn == "waf" { t.Fatalf("expected NO waf handler when referenced ruleset does not exist, but found: %v", h) @@ -189,7 +189,7 @@ func TestGenerateConfig_WAFModeAndRulesetReference(t *testing.T) { sec2 := &models.SecurityConfig{WAFMode: "block", WAFLearning: true} cfg2, err := GenerateConfig([]models.ProxyHost{host}, "/tmp/caddy-data", "", "", "", false, false, true, false, false, "", rulesets, rulesetPaths, nil, sec2, nil) require.NoError(t, err) - route2 := cfg2.Apps.HTTP.Servers["charon_server"].Routes[1] // Main route + route2 := cfg2.Apps.HTTP.Servers["charon_server"].Routes[0] // Main route monitorFound := false for _, h := range route2.Handle { if hn, ok := h["handler"].(string); ok && hn == "waf" { @@ -204,7 +204,7 @@ func TestGenerateConfig_WAFModeDisabledSkipsHandler(t *testing.T) { sec := &models.SecurityConfig{WAFMode: "disabled", WAFRulesSource: "owasp-crs"} cfg, err := GenerateConfig([]models.ProxyHost{host}, "/tmp/caddy-data", "", "", "", false, false, true, false, false, "", nil, nil, nil, sec, nil) require.NoError(t, err) - route := cfg.Apps.HTTP.Servers["charon_server"].Routes[1] // Main route after emergency + route := cfg.Apps.HTTP.Servers["charon_server"].Routes[0] // Main route for _, h := range route.Handle { if hn, ok := h["handler"].(string); ok && hn == "waf" { t.Fatalf("expected NO waf handler when WAFMode disabled, found: %v", h) @@ -219,7 +219,7 @@ func TestGenerateConfig_WAFSelectedSetsContentAndMode(t *testing.T) { rulesetPaths := map[string]string{"owasp-crs": "/tmp/owasp-crs.conf"} cfg, err := GenerateConfig([]models.ProxyHost{host}, "/tmp/caddy-data", "", "", "", false, false, true, false, false, "", []models.SecurityRuleSet{rs}, rulesetPaths, nil, sec, nil) require.NoError(t, err) - route := cfg.Apps.HTTP.Servers["charon_server"].Routes[1] // Main route after emergency + route := cfg.Apps.HTTP.Servers["charon_server"].Routes[0] // Main route found := false for _, h := range route.Handle { if hn, ok := h["handler"].(string); ok && hn == "waf" { @@ -238,7 +238,7 @@ func TestGenerateConfig_DecisionAdminPartsEmpty(t *testing.T) { // Provide an adminWhitelist with an empty segment to trigger p == "" cfg, err := GenerateConfig([]models.ProxyHost{host}, "/tmp/caddy-data", "", "", "", false, false, false, false, false, ", 10.0.0.1/32", nil, nil, []models.SecurityDecision{dec}, nil, nil) require.NoError(t, err) - route := cfg.Apps.HTTP.Servers["charon_server"].Routes[1] // Main route is at index 1 + route := cfg.Apps.HTTP.Servers["charon_server"].Routes[0] // Main route is at index 0 found := false for _, h := range route.Handle { b, _ := json.Marshal(h) @@ -275,7 +275,7 @@ func TestGenerateConfig_WAFUsesRuleSet(t *testing.T) { rulesetPaths := map[string]string{"owasp-crs": "/tmp/owasp-crs.conf"} cfg, err := GenerateConfig([]models.ProxyHost{host}, "/tmp/caddy-data", "", "", "", false, false, true, false, false, "", []models.SecurityRuleSet{rs}, rulesetPaths, nil, nil, nil) require.NoError(t, err) - route := cfg.Apps.HTTP.Servers["charon_server"].Routes[1] // Main route after emergency + route := cfg.Apps.HTTP.Servers["charon_server"].Routes[0] // Main route // check waf handler present with directives containing Include found := false for _, h := range route.Handle { @@ -299,7 +299,7 @@ func TestGenerateConfig_WAFUsesRuleSetFromAdvancedConfig(t *testing.T) { rulesetPaths := map[string]string{"host-rs": "/tmp/host-rs.conf"} cfg, err := GenerateConfig([]models.ProxyHost{host}, "/tmp/caddy-data", "", "", "", false, false, true, false, false, "", []models.SecurityRuleSet{rs}, rulesetPaths, nil, nil, nil) require.NoError(t, err) - route := cfg.Apps.HTTP.Servers["charon_server"].Routes[1] // Main route after emergency + route := cfg.Apps.HTTP.Servers["charon_server"].Routes[0] // Main route // check waf handler present with directives containing Include from host AdvancedConfig found := false for _, h := range route.Handle { @@ -320,7 +320,7 @@ func TestGenerateConfig_WAFUsesRuleSetFromAdvancedConfig_Array(t *testing.T) { rulesetPaths := map[string]string{"host-rs-array": "/tmp/host-rs-array.conf"} cfg, err := GenerateConfig([]models.ProxyHost{host}, "/tmp/caddy-data", "", "", "", false, false, true, false, false, "", []models.SecurityRuleSet{rs}, rulesetPaths, nil, nil, nil) require.NoError(t, err) - route := cfg.Apps.HTTP.Servers["charon_server"].Routes[1] // Main route after emergency + route := cfg.Apps.HTTP.Servers["charon_server"].Routes[0] // Main route // check waf handler present with directives containing Include from host AdvancedConfig array found := false for _, h := range route.Handle { @@ -345,7 +345,7 @@ func TestGenerateConfig_WAFUsesRulesetFromSecCfgFallback(t *testing.T) { cfg, err := GenerateConfig([]models.ProxyHost{host}, "/tmp/caddy-data", "", "", "", false, false, true, false, false, "", nil, rulesetPaths, nil, sec, nil) require.NoError(t, err) // since secCfg requested owasp-crs and we have a path, the waf handler should include the path in directives - route := cfg.Apps.HTTP.Servers["charon_server"].Routes[1] // Main route after emergency + route := cfg.Apps.HTTP.Servers["charon_server"].Routes[0] // Main route found := false for _, h := range route.Handle { if hn, ok := h["handler"].(string); ok && hn == "waf" { @@ -363,7 +363,7 @@ func TestGenerateConfig_RateLimitFromSecCfg(t *testing.T) { sec := &models.SecurityConfig{RateLimitRequests: 10, RateLimitWindowSec: 60, RateLimitBurst: 5} cfg, err := GenerateConfig([]models.ProxyHost{host}, "/tmp/caddy-data", "", "", "", false, false, false, true, false, "", nil, nil, nil, sec, nil) require.NoError(t, err) - route := cfg.Apps.HTTP.Servers["charon_server"].Routes[1] // Main route after emergency + route := cfg.Apps.HTTP.Servers["charon_server"].Routes[0] // Main route found := false for _, h := range route.Handle { if hn, ok := h["handler"].(string); ok && hn == "rate_limit" { @@ -402,7 +402,7 @@ func TestGenerateConfig_CrowdSecHandlerFromSecCfg(t *testing.T) { require.Contains(t, server.TrustedProxies.Ranges, "172.16.0.0/12", "Should trust Docker networks") // Check handler is minimal - route := cfg.Apps.HTTP.Servers["charon_server"].Routes[1] // Main route after emergency + route := cfg.Apps.HTTP.Servers["charon_server"].Routes[0] // Main route found := false for _, h := range route.Handle { if hn, ok := h["handler"].(string); ok && hn == "crowdsec" { diff --git a/backend/internal/caddy/config_generate_test.go b/backend/internal/caddy/config_generate_test.go index 3fad63fa8..9ae420fda 100644 --- a/backend/internal/caddy/config_generate_test.go +++ b/backend/internal/caddy/config_generate_test.go @@ -94,14 +94,9 @@ func TestGenerateConfig_EmergencyRoutesBypassSecurity(t *testing.T) { } } - require.NotNil(t, emergencyRoute, "expected emergency bypass route") - - for _, handler := range emergencyRoute.Handle { - name, _ := handler["handler"].(string) - require.NotEqual(t, "rate_limit", name) - require.NotEqual(t, "waf", name) - require.NotEqual(t, "crowdsec", name) - } + // The host does not proxy Charon, so it gets no recovery route. Security + // handlers therefore cannot be skipped for it on any path. + require.Nil(t, emergencyRoute, "a host that does not proxy Charon must not get a recovery route") } func TestApplyOptionalServerKeepalive_OmitsWhenUnset(t *testing.T) { diff --git a/backend/internal/caddy/config_redirect_test.go b/backend/internal/caddy/config_redirect_test.go index 8eaa4cfdc..f0701fefe 100644 --- a/backend/internal/caddy/config_redirect_test.go +++ b/backend/internal/caddy/config_redirect_test.go @@ -62,8 +62,8 @@ func TestGenerateConfig_RedirectHostsPrecedeProxyHostRoutes(t *testing.T) { require.NoError(t, err) server := cfg.Apps.HTTP.Servers["charon_server"] require.NotNil(t, server) - // Redirect route first, then ProxyHost's emergency + main routes. - require.Len(t, server.Routes, 3) + // Redirect route first, then ProxyHost's main route. + require.Len(t, server.Routes, 2) assert.Equal(t, []string{"redirected.example.com"}, server.Routes[0].Match[0].Host) assert.Equal(t, "static_response", server.Routes[0].Handle[0]["handler"]) } diff --git a/backend/internal/caddy/config_routes_pipeline_test.go b/backend/internal/caddy/config_routes_pipeline_test.go new file mode 100644 index 000000000..4ebf38a87 --- /dev/null +++ b/backend/internal/caddy/config_routes_pipeline_test.go @@ -0,0 +1,129 @@ +package caddy + +import ( + "slices" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/Wikid82/charon/backend/internal/models" +) + +// handlerIndex returns the position of the first handler named name, or -1. +func handlerIndex(handle []Handler, name string) int { + for i, h := range handle { + if h["handler"] == name { + return i + } + } + return -1 +} + +// pipelineRoutesFor generates routes with one block decision in place so every +// host carries a security pre-handler, which lets the ordering check see it. +func pipelineRoutesFor(t *testing.T, hosts []models.ProxyHost, opts ...GenerateConfigOption) []*Route { + t.Helper() + decisions := []models.SecurityDecision{{Action: "block", IP: "203.0.113.7"}} + cfg, err := GenerateConfig(hosts, "/tmp/caddy-data", "admin@example.com", "", "", false, false, false, false, true, "", nil, nil, decisions, nil, nil, opts...) + require.NoError(t, err) + require.NotNil(t, cfg.Apps.HTTP) + server := cfg.Apps.HTTP.Servers["charon_server"] + require.NotNil(t, server) + return server.Routes +} + +// TestGeneratedRoutesKeepPipelineOrder checks, for each host kind, that the +// routes serving the host match the host and that the security pre-handlers +// run before the reverse proxy on the main route and on every location route. +func TestGeneratedRoutesKeepPipelineOrder(t *testing.T) { + secret := testHopSecret(t) + tests := []struct { + name string + domain string + host models.ProxyHost + opts []GenerateConfigOption + locationPaths [][]string + }{ + { + name: "plain upstream", + domain: "plain.example.com", + host: models.ProxyHost{ + UUID: "plain", DomainNames: "plain.example.com", ForwardHost: "10.0.0.5", ForwardPort: 3000, Enabled: true, + }, + }, + { + name: "self upstream", + domain: "self.example.com", + host: models.ProxyHost{ + UUID: "self", DomainNames: "self.example.com", ForwardHost: "127.0.0.1", ForwardPort: 8080, Enabled: true, + }, + opts: []GenerateConfigOption{WithSelfHop(secret, "8080")}, + }, + { + name: "host with locations", + domain: "locs.example.com", + host: models.ProxyHost{ + UUID: "locs", DomainNames: "locs.example.com", ForwardHost: "10.0.0.6", ForwardPort: 3001, Enabled: true, + Locations: []models.Location{{Path: "/api", ForwardHost: "10.0.0.7", ForwardPort: 9000}}, + }, + locationPaths: [][]string{{"/api", "/api/*"}}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + routes := pipelineRoutesFor(t, []models.ProxyHost{tt.host}, tt.opts...) + + // Routes with no path matcher are the main route. Location routes are + // matched by their configured paths; any other path-scoped route for + // the host is a recovery route and is outside this check. + var mainRoutes, locationRoutes []*Route + for _, r := range routes { + if len(r.Match) == 0 || len(r.Match[0].Host) != 1 || r.Match[0].Host[0] != tt.domain { + continue + } + if len(r.Match[0].Path) == 0 { + mainRoutes = append(mainRoutes, r) + } else if isLocationPath(tt.locationPaths, r.Match[0].Path) { + locationRoutes = append(locationRoutes, r) + } + } + + require.Len(t, mainRoutes, 1, "exactly one main route for the host") + main := mainRoutes[0] + assert.Equal(t, []string{tt.domain}, main.Match[0].Host) + assertPipelineOrder(t, main.Handle) + + require.Len(t, locationRoutes, len(tt.locationPaths)) + for _, r := range locationRoutes { + assert.Equal(t, []string{tt.domain}, r.Match[0].Host) + assertPipelineOrder(t, r.Handle) + } + }) + } +} + +// isLocationPath reports whether path is the path matcher of one of the +// configured locations. +func isLocationPath(locationPaths [][]string, path []string) bool { + for _, want := range locationPaths { + if slices.Equal(want, path) { + return true + } + } + return false +} + +// assertPipelineOrder requires a security pre-handler and exactly one reverse +// proxy, with the pre-handler first and the proxy last. +func assertPipelineOrder(t *testing.T, handle []Handler) { + t.Helper() + proxyAt := handlerIndex(handle, "reverse_proxy") + require.GreaterOrEqual(t, proxyAt, 0, "route has a reverse_proxy handler") + assert.Equal(t, len(handle)-1, proxyAt, "reverse_proxy is the final handler") + + securityAt := handlerIndex(handle, "subroute") + require.GreaterOrEqual(t, securityAt, 0, "route has a security pre-handler") + assert.Less(t, securityAt, proxyAt, "security pre-handler runs before the proxy") +} diff --git a/backend/internal/caddy/config_security_headers_test.go b/backend/internal/caddy/config_security_headers_test.go index f1e3c388a..9dca96eac 100644 --- a/backend/internal/caddy/config_security_headers_test.go +++ b/backend/internal/caddy/config_security_headers_test.go @@ -595,7 +595,7 @@ func TestGenerateConfig_SecurityHeadersHandlerPrecedesReverseProxy_Deferred(t *t // The pair must be contiguous and precede reverse_proxy. assert.Less(t, headersIdx+1, proxyIdx) } - assert.Equal(t, 3, found, "location, emergency and main routes must carry the headers pair") + assert.Equal(t, 2, found, "location and main routes must carry the headers pair") } func TestGenerateConfig_LocationRouteHeadersHandlerDeferred(t *testing.T) { diff --git a/backend/internal/caddy/config_test.go b/backend/internal/caddy/config_test.go index 938d7cc29..326f6fe86 100644 --- a/backend/internal/caddy/config_test.go +++ b/backend/internal/caddy/config_test.go @@ -51,9 +51,9 @@ func TestGenerateConfig_SingleHost(t *testing.T) { require.NotNil(t, server) require.Contains(t, server.Listen, ":80") require.Contains(t, server.Listen, ":443") - require.Len(t, server.Routes, 2) // Emergency + main route + require.Len(t, server.Routes, 1) // Main route only: no recovery route for a non-Charon upstream - route := server.Routes[1] // Main route is at index 1 + route := server.Routes[0] // Main route is at index 0 require.Len(t, route.Match, 1) require.Equal(t, []string{"media.example.com"}, route.Match[0].Host) require.Len(t, route.Handle, 1) @@ -83,8 +83,8 @@ func TestGenerateConfig_MultipleHosts(t *testing.T) { config, err := GenerateConfig(hosts, "/tmp/caddy-data", "admin@example.com", "", "", false, false, false, false, true, "", nil, nil, nil, nil, nil) require.NoError(t, err) - require.Len(t, config.Apps.HTTP.Servers["charon_server"].Routes, 4) // 2 hosts × 2 routes each (emergency + main) - require.Len(t, config.Apps.HTTP.Servers["charon_server"].Routes, 4) // 2 hosts × 2 routes each + require.Len(t, config.Apps.HTTP.Servers["charon_server"].Routes, 2) // 2 hosts × 1 main route each + require.Len(t, config.Apps.HTTP.Servers["charon_server"].Routes, 2) // 2 hosts × 1 main route each } func TestGenerateConfig_WebSocketEnabled(t *testing.T) { @@ -100,7 +100,7 @@ func TestGenerateConfig_WebSocketEnabled(t *testing.T) { } config, err := GenerateConfig(hosts, "/tmp/caddy-data", "admin@example.com", "", "", false, false, false, false, true, "", nil, nil, nil, nil, nil) require.NoError(t, err) - route := config.Apps.HTTP.Servers["charon_server"].Routes[1] // Main route is at index 1 + route := config.Apps.HTTP.Servers["charon_server"].Routes[0] // Main route is at index 0 handler := route.Handle[0] // Check WebSocket headers are present @@ -210,16 +210,16 @@ func TestGenerateConfig_Advanced(t *testing.T) { server := config.Apps.HTTP.Servers["charon_server"] require.NotNil(t, server) - // Should have 3 routes: location /api, emergency, main - require.Len(t, server.Routes, 3) + // Should have 2 routes: location /api, main + require.Len(t, server.Routes, 2) // Check Location Route (first as it's most specific) locRoute := server.Routes[0] require.Equal(t, []string{"/api", "/api/*"}, locRoute.Match[0].Path) require.Equal(t, []string{"advanced.example.com"}, locRoute.Match[0].Host) - // Check Main Route (after emergency route) - mainRoute := server.Routes[2] + // Check Main Route (after location route) + mainRoute := server.Routes[1] require.Nil(t, mainRoute.Match[0].Path) // No path means all paths require.Equal(t, []string{"advanced.example.com"}, mainRoute.Match[0].Host) @@ -454,9 +454,9 @@ func TestGenerateConfig_WithRateLimiting(t *testing.T) { server := config.Apps.HTTP.Servers["charon_server"] require.NotNil(t, server) - require.Len(t, server.Routes, 2) // Emergency + main route + require.Len(t, server.Routes, 1) // Main route only - route := server.Routes[1] // Main route is at index 1 + route := server.Routes[0] // Main route is at index 0 // Handlers should include rate_limit + reverse_proxy require.GreaterOrEqual(t, len(route.Handle), 2) @@ -790,8 +790,8 @@ func TestGenerateConfig_DuplicateDomains(t *testing.T) { server := config.Apps.HTTP.Servers["charon_server"] require.NotNil(t, server) - // Should only have 4 routes (2 hosts × emergency + main, one duplicate filtered out) - require.Len(t, server.Routes, 4) + // Should only have 2 routes (2 hosts × main, one duplicate filtered out) + require.Len(t, server.Routes, 2) // Verify unique.example.com is present var foundUnique bool @@ -867,9 +867,9 @@ func TestGenerateConfig_CrowdSecHandlerAdded(t *testing.T) { server := config.Apps.HTTP.Servers["charon_server"] require.NotNil(t, server) - require.Len(t, server.Routes, 2) // Emergency + main route + require.Len(t, server.Routes, 1) // Main route only - route := server.Routes[1] // Main route is at index 1 + route := server.Routes[0] // Main route is at index 0 // Should have CrowdSec handler + reverse_proxy handler require.GreaterOrEqual(t, len(route.Handle), 2) @@ -907,9 +907,9 @@ func TestGenerateConfig_WithSecurityDecisions(t *testing.T) { server := config.Apps.HTTP.Servers["charon_server"] require.NotNil(t, server) - require.Len(t, server.Routes, 2) // Emergency + main route + require.Len(t, server.Routes, 1) // Main route only - route := server.Routes[1] // Main route is at index 1 + route := server.Routes[0] // Main route is at index 0 // Marshal to JSON for inspection b, err := json.Marshal(route.Handle) @@ -1345,7 +1345,7 @@ func TestGenerateConfig_WithWAFPerHostDisabled(t *testing.T) { server := config.Apps.HTTP.Servers["charon_server"] require.NotNil(t, server) - require.Len(t, server.Routes, 4) // 2 hosts × 2 routes each (emergency + main) + require.Len(t, server.Routes, 2) // 2 hosts × 1 main route each // Check waf-enabled host has WAF handler var wafEnabledRoute, wafDisabledRoute *Route @@ -1402,9 +1402,9 @@ func TestGenerateConfig_WithDisabledHost(t *testing.T) { server := config.Apps.HTTP.Servers["charon_server"] require.NotNil(t, server) - // Only 2 routes for the enabled host (emergency + main) - require.Len(t, server.Routes, 2) - require.Equal(t, []string{"enabled.example.com"}, server.Routes[1].Match[0].Host) // Main route at index 1 + // Only 1 route for the enabled host (main) + require.Len(t, server.Routes, 1) + require.Equal(t, []string{"enabled.example.com"}, server.Routes[0].Match[0].Host) // Main route at index 0 } // TestGenerateConfig_WithFrontendDir verifies catch-all route with frontend @@ -1424,11 +1424,11 @@ func TestGenerateConfig_WithFrontendDir(t *testing.T) { server := config.Apps.HTTP.Servers["charon_server"] require.NotNil(t, server) - // Should have 3 routes: emergency + main for the host + catch-all for frontend - require.Len(t, server.Routes, 3) + // Should have 2 routes: main for the host + catch-all for frontend + require.Len(t, server.Routes, 2) // Last route should be catch-all with file_server - catchAll := server.Routes[2] + catchAll := server.Routes[1] require.Nil(t, catchAll.Match) require.True(t, catchAll.Terminal) @@ -1568,9 +1568,9 @@ func TestGenerateConfig_NormalizeAdvancedConfig(t *testing.T) { server := config.Apps.HTTP.Servers["charon_server"] require.NotNil(t, server) - require.Len(t, server.Routes, 2) // Emergency + main route + require.Len(t, server.Routes, 1) // Main route only - route := server.Routes[1] // Main route is at index 1 + route := server.Routes[0] // Main route is at index 0 // Should have headers handler + reverse_proxy require.GreaterOrEqual(t, len(route.Handle), 2) @@ -1627,7 +1627,7 @@ func TestGenerateConfig_SecurityDecisionsWithAdminWhitelist(t *testing.T) { server := config.Apps.HTTP.Servers["charon_server"] require.NotNil(t, server) - route := server.Routes[1] // Main route is at index 1 + route := server.Routes[0] // Main route is at index 0 b, _ := json.Marshal(route.Handle) s := string(b) diff --git a/backend/internal/caddy/config_waf_rules_required_test.go b/backend/internal/caddy/config_waf_rules_required_test.go index a5292a9dc..1ad0f0b94 100644 --- a/backend/internal/caddy/config_waf_rules_required_test.go +++ b/backend/internal/caddy/config_waf_rules_required_test.go @@ -83,9 +83,9 @@ func TestGenerateConfig_AdvancedRulesetNameSkippedLeavesHandlerUntouched(t *test cfgPlain, err := GenerateConfig([]models.ProxyHost{withoutName}, "/tmp/caddy-data", "", "", "", false, false, true, false, false, "", rulesets, map[string]string{}, nil, nil, nil) require.NoError(t, err) - namedHandlers, err := json.Marshal(cfgNamed.Apps.HTTP.Servers["charon_server"].Routes[1].Handle) + namedHandlers, err := json.Marshal(cfgNamed.Apps.HTTP.Servers["charon_server"].Routes[0].Handle) require.NoError(t, err) - plainHandlers, err := json.Marshal(cfgPlain.Apps.HTTP.Servers["charon_server"].Routes[1].Handle) + plainHandlers, err := json.Marshal(cfgPlain.Apps.HTTP.Servers["charon_server"].Routes[0].Handle) require.NoError(t, err) assert.Equal(t, string(plainHandlers), string(namedHandlers)) assert.False(t, strings.Contains(string(namedHandlers), "Include"), "no Include directive may point at the skipped set") diff --git a/backend/internal/caddy/self_upstream_test.go b/backend/internal/caddy/self_upstream_test.go index 9e866ec10..d87bade39 100644 --- a/backend/internal/caddy/self_upstream_test.go +++ b/backend/internal/caddy/self_upstream_test.go @@ -112,7 +112,7 @@ func TestGenerateConfig_SelfHopHeadersOnlyOnSelfUpstream(t *testing.T) { _, hasSecret := p.set[selfhop.HeaderSecret] switch p.dial { case "127.0.0.1:8080", "localhost:8080": - // u-self (main + emergency + location) and u-remote (never self). + // u-self (main + recovery + location) and u-remote (never self). if hasSecret { selfCount++ assert.Equal(t, []string{secret.Reveal()}, p.set[selfhop.HeaderSecret]) @@ -127,7 +127,7 @@ func TestGenerateConfig_SelfHopHeadersOnlyOnSelfUpstream(t *testing.T) { assert.NotContains(t, p.set, selfhop.HeaderClient) } } - // main route, emergency route and one self location of u-self. + // main route, recovery route and one self location of u-self. assert.Equal(t, 3, selfCount) assert.Positive(t, otherCount) @@ -272,8 +272,8 @@ func TestManager_ApplyConfig_SelfHopWiringAndSnapshotRedaction(t *testing.T) { without++ } } - assert.Equal(t, 2, withProof) // main + emergency route of the self host - assert.Equal(t, 2, without) // main + emergency route of the agent host + assert.Equal(t, 2, withProof) // main + recovery route of the self host + assert.Equal(t, 1, without) // main route of the agent host (no recovery route) // Snapshots on disk never hold the secret. entries, err := os.ReadDir(dir) From 49e78b9155f3e641eff2c78beef6865a473e5ec7 Mon Sep 17 00:00:00 2001 From: Jeremy Hatfield Date: Mon, 9 Nov 2026 15:17:33 +0000 Subject: [PATCH 2/4] fix(security): harden route handling in the proxy layer --- ARCHITECTURE.md | 35 ++-- .../caddy/config_recovery_route_test.go | 195 ++++++++++++++++++ 2 files changed, 210 insertions(+), 20 deletions(-) create mode 100644 backend/internal/caddy/config_recovery_route_test.go diff --git a/ARCHITECTURE.md b/ARCHITECTURE.md index 72fd30ecf..d3d4e28ef 100644 --- a/ARCHITECTURE.md +++ b/ARCHITECTURE.md @@ -723,36 +723,31 @@ export const useProxyHosts = () => { 5. Caddy validates and applies new configuration 6. Traffic flows through new proxy route -**Route Pattern: Emergency + Main** +**Route Pattern: Main Route (plus a recovery route for Charon's own host)** -For each proxy host, Charon generates **two routes** with the same domain: +For each proxy host, Charon generates a **main route** with the domain and no path matchers. Its handlers run the full Cerberus security pipeline before the reverse proxy. Locations are generated as separate path-matched routes that run the same pipeline. -1. **Emergency Route** (with path matchers): - - Matches: `/api/v1/emergency/*` paths - - Purpose: Bypass security features for administrative access - - Priority: Evaluated first (more specific match) - - Handlers: No WAF, ACL, or Rate Limiting - -2. **Main Route** (without path matchers): - - Matches: All other paths for the domain +1. **Main Route** (without path matchers): + - Matches: All paths for the domain - Purpose: Normal application traffic with full security - - Priority: Evaluated second (catch-all) - - Handlers: Full Cerberus security suite + - Handlers: Full Cerberus security suite, then the reverse proxy + +2. **Recovery Route** (exact path matchers, only for a host that proxies Charon itself): + - Matches: exactly `/api/v1/emergency/security-reset` and `/emergency/security-reset`, with no wildcards + - Purpose: Recovery route for the reset endpoint. The endpoint performs its own token and management-network checks. + - Handlers: Skips the security pre-handlers; no other path under `/emergency/` takes this route, token endpoints included -This pattern is **intentional and valid**: +A host that proxies Charon gets a recovery route for the reset endpoint. Every other host gets only its main route and its locations. -- Emergency route provides break-glass access to security controls -- Main route protects application with enterprise security features -- Caddy processes routes in order (emergency matches first) -- Validator allows duplicate hosts when one has paths and one doesn't +Caddy processes routes in order, so the recovery route (when present) is evaluated before the main route. Validator allows duplicate hosts when one has paths and one doesn't. **Example:** ```json -// Emergency Route (evaluated first) +// Recovery Route (only for a host whose upstream is Charon; evaluated first) { - "match": [{"host": ["app.example.com"], "path": ["/api/v1/emergency/*"]}], - "handle": [/* Emergency handlers - no security */], + "match": [{"host": ["charon.example.com"], "path": ["/api/v1/emergency/security-reset", "/emergency/security-reset"]}], + "handle": [/* reverse proxy only */], "terminal": true } diff --git a/backend/internal/caddy/config_recovery_route_test.go b/backend/internal/caddy/config_recovery_route_test.go new file mode 100644 index 000000000..6bf2678f9 --- /dev/null +++ b/backend/internal/caddy/config_recovery_route_test.go @@ -0,0 +1,195 @@ +package caddy + +import ( + "slices" + "strings" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/Wikid82/charon/backend/internal/models" +) + +// recoveryProbePaths are paths near the reset endpoint that must never be +// served by a route that skips the security pre-handlers, for any host. +var recoveryProbePaths = []string{ + "/emergency/..;/admin", + "/emergency/x", + "/api/v1/emergency/token/x", + "/api/v1/emergency/security-reset/extra", + "/emergency/security-reset/", + "/api/v1/emergency/security-reset;x", +} + +// matchesRoutePattern follows the path matcher rules: an exact match, or a +// prefix match when the pattern ends in "*". +func matchesRoutePattern(pattern, path string) bool { + if prefix, ok := strings.CutSuffix(pattern, "*"); ok { + return strings.HasPrefix(path, prefix) + } + return pattern == path +} + +func routeMatchesPath(patterns []string, path string) bool { + return slices.ContainsFunc(patterns, func(p string) bool { + return matchesRoutePattern(p, path) + }) +} + +// checkFreePathRoutes returns the path-scoped routes for domain that run no +// security pre-handler. Location routes and the main route both carry the +// pre-handler, so only a recovery route can appear here. +func checkFreePathRoutes(routes []*Route, domain string) []*Route { + var out []*Route + for _, r := range routes { + if len(r.Match) == 0 || !slices.Equal(r.Match[0].Host, []string{domain}) || len(r.Match[0].Path) == 0 { + continue + } + if handlerIndex(r.Handle, "subroute") >= 0 { + continue + } + out = append(out, r) + } + return out +} + +// recoveryHostCase describes one host kind for the recovery route checks. +type recoveryHostCase struct { + name string + domain string + host models.ProxyHost + opts []GenerateConfigOption + selfUpstream bool +} + +func recoveryHostCases(t *testing.T) []recoveryHostCase { + t.Helper() + secret := testHopSecret(t) + return []recoveryHostCase{ + { + name: "plain upstream", + domain: "plain.example.com", + host: models.ProxyHost{ + UUID: "plain", DomainNames: "plain.example.com", ForwardHost: "10.0.0.5", ForwardPort: 3000, Enabled: true, + }, + }, + { + name: "plain upstream with locations", + domain: "locs.example.com", + host: models.ProxyHost{ + UUID: "locs", DomainNames: "locs.example.com", ForwardHost: "10.0.0.6", ForwardPort: 3001, Enabled: true, + Locations: []models.Location{{Path: "/api", ForwardHost: "10.0.0.7", ForwardPort: 9000}}, + }, + }, + { + name: "loopback port that is not the API port", + domain: "other.example.com", + host: models.ProxyHost{ + UUID: "other", DomainNames: "other.example.com", ForwardHost: "127.0.0.1", ForwardPort: 9090, Enabled: true, + }, + opts: []GenerateConfigOption{WithSelfHop(secret, "8080")}, + }, + { + name: "self port on a remote host", + domain: "remote.example.com", + host: models.ProxyHost{ + UUID: "remote", DomainNames: "remote.example.com", ForwardHost: "127.0.0.1", ForwardPort: 8080, Enabled: true, + }, + opts: []GenerateConfigOption{ + WithSelfHop(secret, "8080"), + WithRemoteHosts(map[string]struct{}{"remote": {}}), + }, + }, + { + name: "self upstream", + domain: "self.example.com", + host: models.ProxyHost{ + UUID: "self", DomainNames: "self.example.com", ForwardHost: "127.0.0.1", ForwardPort: 8080, Enabled: true, + }, + opts: []GenerateConfigOption{WithSelfHop(secret, "8080")}, + selfUpstream: true, + }, + } +} + +// selfUpstreamCase returns the self upstream host kind. +func selfUpstreamCase(t *testing.T) recoveryHostCase { + t.Helper() + for _, tt := range recoveryHostCases(t) { + if tt.selfUpstream { + return tt + } + } + require.Fail(t, "no self upstream host case") + return recoveryHostCase{} +} + +// TestRecoveryRouteAbsentForNonSelfUpstream checks that a host whose upstream +// is not Charon itself gets no route that skips the security pre-handlers. +func TestRecoveryRouteAbsentForNonSelfUpstream(t *testing.T) { + for _, tt := range recoveryHostCases(t) { + if tt.selfUpstream { + continue + } + t.Run(tt.name, func(t *testing.T) { + routes := pipelineRoutesFor(t, []models.ProxyHost{tt.host}, tt.opts...) + assert.Empty(t, checkFreePathRoutes(routes, tt.domain)) + }) + } +} + +// TestRecoveryRouteMatchesOnlyExactResetPaths checks that a self upstream host +// gets exactly one check-free route, that it lists the two exact reset paths +// with no wildcard, and that it is terminal and proxies last. +func TestRecoveryRouteMatchesOnlyExactResetPaths(t *testing.T) { + selfCase := selfUpstreamCase(t) + routes := pipelineRoutesFor(t, []models.ProxyHost{selfCase.host}, selfCase.opts...) + recovery := checkFreePathRoutes(routes, selfCase.domain) + require.Len(t, recovery, 1, "a self upstream host gets exactly one recovery route") + + route := recovery[0] + assert.Equal(t, []string{"/api/v1/emergency/security-reset", "/emergency/security-reset"}, route.Match[0].Path) + assert.True(t, route.Terminal) + require.NotEmpty(t, route.Handle) + assert.Equal(t, "reverse_proxy", route.Handle[len(route.Handle)-1]["handler"], "the recovery route proxies last") + + assert.True(t, routeMatchesPath(route.Match[0].Path, "/emergency/security-reset")) + assert.True(t, routeMatchesPath(route.Match[0].Path, "/api/v1/emergency/security-reset")) + for _, p := range route.Match[0].Path { + assert.False(t, strings.HasSuffix(p, "*"), "recovery path %q must not use a wildcard", p) + } +} + +// TestRecoveryRouteProbePathsNeverMatch checks that paths near the reset +// endpoint, token endpoints included, match no check-free route for any host +// kind. Token endpoints run through the main route with the pre-handlers. +func TestRecoveryRouteProbePathsNeverMatch(t *testing.T) { + for _, tt := range recoveryHostCases(t) { + t.Run(tt.name, func(t *testing.T) { + routes := pipelineRoutesFor(t, []models.ProxyHost{tt.host}, tt.opts...) + for _, route := range checkFreePathRoutes(routes, tt.domain) { + for _, probe := range recoveryProbePaths { + assert.False(t, routeMatchesPath(route.Match[0].Path, probe), + "probe path %q must not match a check-free route", probe) + } + } + }) + } +} + +// TestRecoveryRouteTokenPathUsesMainRoute checks that a token endpoint on a self +// upstream host is served by the main route, which runs the pre-handlers first. +func TestRecoveryRouteTokenPathUsesMainRoute(t *testing.T) { + selfCase := selfUpstreamCase(t) + routes := pipelineRoutesFor(t, []models.ProxyHost{selfCase.host}, selfCase.opts...) + + var mainRoute *Route + for _, r := range routes { + if len(r.Match) > 0 && slices.Equal(r.Match[0].Host, []string{selfCase.domain}) && len(r.Match[0].Path) == 0 { + mainRoute = r + } + } + require.NotNil(t, mainRoute, "self upstream host keeps a main route") + assertPipelineOrder(t, mainRoute.Handle) +} From 17eef751c46db8b826971d1b7ab2152793a317d1 Mon Sep 17 00:00:00 2001 From: Jeremy Hatfield Date: Mon, 9 Nov 2026 15:30:47 +0000 Subject: [PATCH 3/4] test: cover route handling end to end --- .../acl-enforcement.spec.ts | 169 ++++++++++++++++++ 1 file changed, 169 insertions(+) diff --git a/tests/security-enforcement/acl-enforcement.spec.ts b/tests/security-enforcement/acl-enforcement.spec.ts index de4e080e0..a0681f10b 100644 --- a/tests/security-enforcement/acl-enforcement.spec.ts +++ b/tests/security-enforcement/acl-enforcement.spec.ts @@ -207,4 +207,173 @@ test.describe('ACL Enforcement', () => { console.log(`Note: Could not create test ACL: ${errorBody}`); } }); + + test.describe('Route handling on proxy hosts', () => { + // An IP allow-list cannot express a non-listed runner here: the runner is + // admin-whitelisted, and the proxy ACL merges the admin whitelist into every + // IP allow-list. A geo allow-list with no country match for the runner's + // private address refuses it without that merge. + const ROUTE_PATHS = [ + '/', + '/emergency/security-reset', + '/emergency/x', + '/emergency/..;/admin', + '/api/v1/emergency/token/x', + '/api/v1/emergency/security-reset', + ]; + // Paths under the reset prefix that the reset endpoint does not serve. + const NON_RESET_PATHS = ROUTE_PATHS.filter( + (path) => path !== '/emergency/security-reset' && path !== '/api/v1/emergency/security-reset' + ); + const APP_BASE_URL = process.env.PLAYWRIGHT_BASE_URL || 'http://127.0.0.1:8080'; + const RESET_API_PATH = '/api/v1/emergency/security-reset'; + const RESET_APP_PATH = '/emergency/security-reset'; + + let proxyContext: APIRequestContext; + let aclUuid: string; + let allowListedProxyUuid: string; + let allowListedDomain: string; + let selfProxyUuid: string; + let selfProxyDomain: string; + + /** Caddy proxy origin, derived the same way caddyProxyOrigin() does (no page here). */ + function proxyOrigin(): string { + const url = new URL(APP_BASE_URL); + return `${url.protocol}//${url.hostname}:${process.env.PLAYWRIGHT_CADDY_PROXY_PORT || '80'}`; + } + + /** Send a request through the proxy hop with the given Host header. */ + async function proxied(domain: string, path: string, method = 'GET') { + return proxyContext.fetch(`${proxyOrigin()}${path}`, { + method, + headers: { Host: domain }, + data: method === 'POST' ? {} : undefined, + failOnStatusCode: false, + maxRedirects: 0, + }); + } + + async function createProxyHost( + domain: string, + forwardPort: number, + name: string, + accessListUuid: string + ): Promise { + const response = await requestContext.post('/api/v1/proxy-hosts', { + data: { + domain_names: domain, + forward_host: '127.0.0.1', + forward_port: forwardPort, + forward_scheme: 'http', + websocket_support: false, + enabled: true, + name, + access_list_id: accessListUuid, + }, + }); + expect(response.ok(), `create proxy host ${domain}`).toBe(true); + const body = await response.json(); + return body.uuid ?? body.id; + } + + test.beforeAll(async () => { + proxyContext = await request.newContext(); + + // Allow-list that only admits one country; the runner has no country, so it is + // not on the list. Attached to both hosts. + const aclResponse = await requestContext.post('/api/v1/access-lists', { + data: { + name: `Route Handling Allow List ${Date.now()}`, + type: 'geo_whitelist', + country_codes: 'US', + enabled: true, + }, + }); + expect(aclResponse.ok(), 'create allow list').toBe(true); + aclUuid = (await aclResponse.json()).uuid; + + // Plain allow-listed host: upstream is a port nothing listens on, so any + // request that gets past the ACL would fail upstream with 502. + allowListedDomain = `route-allow-${Date.now()}.example.test`; + allowListedProxyUuid = await createProxyHost(allowListedDomain, 3001, 'Allow-listed host', aclUuid); + + // Self-proxy host: upstream is Charon's own listener. + selfProxyDomain = `route-self-${Date.now()}.example.test`; + selfProxyUuid = await createProxyHost(selfProxyDomain, 8080, 'Self-proxy host', aclUuid); + + // Wait for both routes to be loaded by the proxy. The ACL answers 403 on '/'. + for (const domain of [allowListedDomain, selfProxyDomain]) { + await expect + .poll(async () => (await proxied(domain, '/')).status(), { + timeout: 30_000, + message: `route for ${domain} should be loaded`, + }) + .toBe(403); + } + }); + + test.afterAll(async () => { + for (const uuid of [selfProxyUuid, allowListedProxyUuid]) { + if (uuid) { + await requestContext.delete(`/api/v1/proxy-hosts/${uuid}`); + } + } + if (aclUuid) { + await requestContext.delete(`/api/v1/access-lists/${aclUuid}`); + } + await proxyContext.dispose(); + }); + + test('refuses a non-listed client on every route handling path of an allow-listed host', async () => { + const results = await Promise.all( + ROUTE_PATHS.map(async (path) => { + const response = await proxied(allowListedDomain, path); + return { path, status: response.status() }; + }) + ); + + expect(results).toEqual(ROUTE_PATHS.map((path) => ({ path, status: 403 }))); + }); + + test('keeps the reset endpoint reachable on a self-proxy host while other paths stay under the ACL', async () => { + await test.step('Non-reset paths under the prefix are refused by the ACL', async () => { + const results = await Promise.all( + NON_RESET_PATHS.map(async (path) => { + const response = await proxied(selfProxyDomain, path); + return { path, status: response.status() }; + }) + ); + + expect(results).toEqual(NON_RESET_PATHS.map((path) => ({ path, status: 403 }))); + }); + + await test.step('API reset path answers from the reset handler without a token', async () => { + const response = await proxied(selfProxyDomain, RESET_API_PATH, 'POST'); + const direct = await proxyContext.fetch(`${APP_BASE_URL}${RESET_API_PATH}`, { + method: 'POST', + data: {}, + failOnStatusCode: false, + }); + + // The reset endpoint's own token check answers (401), the same as the app directly. + expect(response.status()).toBe(401); + expect(response.status()).toBe(direct.status()); + expect(await response.json()).toMatchObject({ error: 'unauthorized' }); + }); + + await test.step('Exact app-level reset path is answered by the app, not the ACL', async () => { + // The app has no API route at this path; its own router answers it. The + // proxied answer must match the direct answer and must not be the ACL refusal. + const response = await proxied(selfProxyDomain, RESET_APP_PATH, 'POST'); + const direct = await proxyContext.fetch(`${APP_BASE_URL}${RESET_APP_PATH}`, { + method: 'POST', + data: {}, + failOnStatusCode: false, + }); + + expect(response.status()).not.toBe(403); + expect(response.status()).toBe(direct.status()); + }); + }); + }); }); From 8ad8c4eb1e86178e163b607d03a13705a4e8deb6 Mon Sep 17 00:00:00 2001 From: Jeremy Hatfield Date: Mon, 9 Nov 2026 16:13:10 +0000 Subject: [PATCH 4/4] docs: add the QA report --- docs/reports/qa_report.md | 122 +++++++++++++++++--------------------- 1 file changed, 56 insertions(+), 66 deletions(-) diff --git a/docs/reports/qa_report.md b/docs/reports/qa_report.md index 89f7a7ea6..ddbd6495f 100644 --- a/docs/reports/qa_report.md +++ b/docs/reports/qa_report.md @@ -1,87 +1,77 @@ -# QA Report: Single-Backup Bulk Certificate Delete +# QA Report: Route Handling in the Proxy Layer -- Branch: `fix/cert-bulk-delete-single-backup` (3 commits over `development` `9765dce2b`, HEAD `08870cca9`) -- Scope: new admin-only `POST /api/v1/certificates/bulk-delete`, refactor of `CertificateHandler.Delete`, frontend `bulkDeleteCertificates` client, `useBulkDeleteCertificates` hook, and ProxyHosts orphaned-certificate cleanup. -- Change type: `fix:` with no new feature surface. CodeQL and Trivy are deferred to CI per `CLAUDE.md`. -- Date: 2026-11-07 +- Branch: `fix/proxy-route-handling` (3 commits over `origin/development` `d7a3af60a`, HEAD `17eef751c`) +- Scope: how the proxy configuration generator builds routes for a proxy host (`backend/internal/caddy/config.go`), the unit tests that pin those routes, one Playwright case in `tests/security-enforcement/acl-enforcement.spec.ts`, and the matching `ARCHITECTURE.md` section. +- Change type: `fix:` (security scope). CodeQL Go was run locally as the spec requires; Trivy is deferred to CI (no dependency or image input changed). +- Date: 2026-11-09 ## Verdict -**PASS. No blockers.** All gates run locally passed. Findings below are low severity or informational. +**PASS. No blockers.** Every gate run locally passed. -## Gate Results - -| Gate | Result | Evidence | -|---|---|---| -| GORM security scan | Skipped (not triggered) | No changes under `backend/internal/models/**`, no new GORM queries or migrations | -| Backend coverage (`scripts/go-test-coverage.sh`) | PASS | Statement 93.2%, line 92.1% (gate 87%). `internal/api/handlers` ran fresh (not cached): 92.3%. Zero failures | -| `go build ./...` | PASS | Clean | -| `go vet ./internal/api/...` | PASS | Clean | -| `make lint-fast` | PASS | 0 issues (backend and agent) | -| Frontend coverage (`scripts/frontend-test-coverage.sh`) | PASS | 328 files, 4332 tests passed (2 todo). Lines 92.5%, statements 91.6%, branches 86.31% | -| `npm run type-check` | PASS | Clean | -| `npm run build` | PASS | Built | -| Local patch coverage (`scripts/local-patch-report.sh`, strict) | PASS | Overall 95.2% (100/105), backend 94.7% (89/94), frontend 100% (11/11) | -| Targeted E2E (`tests/certificate-bulk-delete.spec.ts`) | Deferred to CI | Requires a rebuilt E2E container for the new route; Docker was out of scope for this run | -| CodeQL / Trivy | Deferred to CI | `fix:`-scoped change | - -Coverage freshness: HEAD commit time 2026-11-07 03:39:23 UTC. `backend/coverage.txt` written 03:52:18 UTC and `frontend/coverage/lcov.info` written 04:05:54 UTC, both after HEAD, so the patch report used fresh coverage. Patch baseline `origin/development...HEAD`; `origin/development` equals local `development` and the merge base. - -## Patch Coverage Gaps (non-blocking) - -| File | Uncovered lines | What they are | -|---|---|---| -| `backend/internal/api/handlers/certificate_handler.go` | 469-471 | `Delete` (UUID path): `DeleteCertificate` returns an error after the usage check passed | -| `backend/internal/api/handlers/certificate_handler.go` | 535-536 | `respondCertificateDeleteError` default branch (generic 500 "failed to delete certificate") | - -Both are the race/DB-failure branch of single delete (a host gets assigned, or the row vanishes, or the DB fails, between the usage check and the conditional delete). Recommended test: in a `Delete` test, use the existing `gormfault` test helper (or a callback that fails the `Delete` on `ssl_certificates`) so `IsCertificateInUseByUUID` succeeds and `DeleteCertificate` fails, then assert 500 `{"error":"failed to delete certificate"}` and that the raw DB error is not in the body. +## Generated Routes (behaviour) -## Security Review +For each enabled proxy host the generator now emits: -### Authorization: PASS -- `routes.go:1151` registers `POST /certificates/bulk-delete` on the authenticated `management` group with `middleware.RequireRole(models.RoleAdmin)`, matching single `DELETE /certificates/:uuid`. -- The static `bulk-delete` segment does not collide with `/:uuid` routes (POST `/:uuid/export` has an extra segment); route registration ran in the backend suite without panic. -- `TestBulkDeleteCertificates_RoleGating` confirms a `user` role gets 403, no backup is taken, and nothing is deleted. +1. One route per custom location, matching the location path. Handlers: security pre-handlers, host-level handlers, then the reverse proxy. +2. **Only when the host's main upstream is Charon itself** (same detection the generator already uses for the proxy hop headers; upstreams marked remote never qualify): a recovery route matching exactly `/api/v1/emergency/security-reset` and `/emergency/security-reset`, with no wildcard. Handlers: host-level handlers, then the reverse proxy. The reset endpoint performs its own token and management-network checks. +3. The main route, matching the host with no path matcher. Handlers: security pre-handlers, host-level handlers, then the reverse proxy. -### Input validation: PASS -- Body bound to a typed struct; bind errors return a fixed `{"error":"invalid request"}` (no decoder text). -- 1 to 200 entries enforced (`maxBulkDeleteCertificates`); each entry parsed with `uuid.Parse` and canonicalized; duplicates (including case variants) dropped while keeping order. Numeric IDs are rejected, consistent with the UUID-only rule. -- Validation failures return before the backup, so a bad request cannot trigger a backup. +Every other path, including everything else under `/emergency/` and `/api/v1/emergency/` (token endpoints included), is served by the main route or a location route and runs the security pre-handlers first. Hosts whose upstream is not Charon get no recovery route. -### No raw error text in responses: PASS -- 400 messages come from fixed sentinel errors (`errBulkDeleteCount`, `errBulkDeleteUUID`). -- Per-certificate failures map to the fixed enum `in_use` / `not_found` / `error`; `TestBulkDeleteCertificates_UsageCheckErrorReportsGenericError` asserts the SQLite "no such table" text does not leak. -- Backup and disk-space failures reuse the existing fixed messages. +Route table observed in the throwaway container (three hosts: allow-listed host, allow-listed host proxying Charon, WAF-only host): -### Log injection: PASS -- Every log field carrying a UUID goes through `util.SanitizeForLog`, and the values are already canonical UUIDs from `uuid.Parse`. `reason` is from a fixed set. `sendDeleteNotification` re-validates via `sanitizeCertRef`. +| Host | Path matcher | Handlers | +|---|---|---| +| WAF-only host | none | waf, headers, headers, vars, reverse_proxy | +| Host proxying Charon | `/api/v1/emergency/security-reset`, `/emergency/security-reset` | headers, headers, vars, reverse_proxy | +| Host proxying Charon | none | waf, access list, headers, headers, vars, reverse_proxy | +| Allow-listed host | none | waf, access list, headers, headers, vars, reverse_proxy | -### DoS bounds: PASS with one low finding -- Work per request is capped at 200 certificates and one backup. The backup service's non-blocking lock still prevents concurrent backups. -- **QA-1 (Low):** the handler does not cap the request body size before decoding. The 200-entry limit is checked only after the whole JSON body has been read, so an authenticated admin can make the server buffer an arbitrarily large body. No global body limit exists in the server, and most JSON handlers share this gap, so the risk is small (admin-only, authenticated). Remediation: wrap `c.Request.Body` with `http.MaxBytesReader` (for example 64 KiB, which is ample for 200 UUIDs) before `ShouldBindJSON`, the same pattern as `crowdsec_body_limit.go`. A server-wide default JSON body limit would close it for every handler. +## Gate Results -### `Delete` behavior unchanged: PASS -- Numeric-ID path: same order (usage check, backup, delete), same status codes and messages; the backup block moved into `backupBeforeDelete` unchanged. -- UUID path: usage check still runs before the backup; 404 / 409 / 500 "failed to check certificate usage" / 507 / 500 backup / 404, 409, 500 on delete map to the same statuses and messages as before via `respondCertificateDeleteError`. `errors.Is` works through the `%w: %w` wrap (Go 1.20+). -- Only difference: the UUID path's low-disk branch now logs a warning it previously did not. No response change. +| Gate | Result | Evidence | +|---|---|---| +| Diff review against the design (spec section 3, D1) | PASS | Recovery route only behind the self-upstream check; exact paths from one `resetRecoveryPaths` list; locations and main route unchanged and still start with the security pre-handlers; remote and agent upstreams excluded by the existing remote flag; redirect and location tests updated, not removed | +| `go build ./...`, `go vet ./...` | PASS | Clean | +| `go test ./internal/caddy/... ./internal/api/... ./internal/cerberus/... -count=1` | PASS | All packages `ok` (see note on one non-reproducible handlers failure below) | +| Backend coverage (`scripts/go-test-coverage.sh`) | PASS | Statement 93.2%, line 92.1% (gate 87%). Zero failures | +| Local patch coverage (`scripts/local-patch-report.sh`, strict) | PASS | Backend 100% (20/20 changed lines), overall 100% | +| `make lint-fast` | PASS | 0 issues (backend and agent) | +| `make lint-backend` (golangci-lint v2.15.0, full) | PASS | 0 issues | +| GORM security scan | Not triggered | No model, query or migration changes | +| CodeQL Go (local, CLI 2.27.2, `go-security-and-quality`) | PASS | 4 results, all matched by non-expired entries in `.github/codeql/codeql-suppressions.yml`; 0 blocking; extraction parity 394/394. None in the diff | +| Container gate (throwaway container built from this branch, own network, loopback-only ports, removed afterwards) | PASS | See below | +| Playwright `acl-enforcement.spec.ts --project=security-tests --retries=0` | PASS | 9 passed (5.2 s) | +| `npm run lint:e2e` | PASS | 124 warnings, 0 errors (budget 124); none in the touched spec | -## Informational Findings (no action required for this PR) +### Container gate -- **QA-2 (Info):** `BulkDelete` takes the backup before the per-certificate usage checks, so a batch where every certificate is in use or missing still takes a backup. Single delete checks usage first. Acceptable trade-off (one backup per batch); a pre-pass that skips the backup when nothing is deletable would avoid wasted backups. -- **QA-3 (Low):** neither the certificates page bulk action (`useBulkDeleteCertificates`) nor the ProxyHosts cleanup splits selections larger than 200. Above 200 the request returns 400 and the whole batch is reported as failed, with nothing deleted. Remediation: chunk into groups of 200 in `bulkDeleteCertificates` (one backup per chunk), or cap selection in the UI. -- **QA-4 (Info):** one external "Certificate Deleted" notification is still sent per deleted certificate (up to 200 per request). This matches the old sequential behavior; a single summary notification for a batch would be quieter. +Requests were sent from a separate container whose address is on no list. -## Documentation Review (`docs/api.md`) +| Check | Request | Result | +|---|---|---| +| (a) Allow-listed host refuses a non-listed client | `/`, `/emergency/x`, `/emergency/..;/admin`, `/api/v1/emergency/token/x`, `POST /api/v1/emergency/security-reset`, `POST /emergency/security-reset` | 403 on every path | +| (b) Host proxying Charon, other paths | `/`, `/emergency/x`, `/emergency/..;/admin`, `/api/v1/emergency/token/x`, `/api/v1/emergency/token/status` | 403 on every path | +| (b) Host proxying Charon, reset path without a token | `POST /api/v1/emergency/security-reset` | 401 from the reset handler | +| (b) Host proxying Charon, reset path with a valid token | `POST /api/v1/emergency/security-reset` | 200; modules disabled as expected; the allow-listed host then stops refusing | +| (b) Host proxying Charon, trailing-slash form | `POST /api/v1/emergency/security-reset/` | 403 (main route) | +| (c) WAF host, test payload under the prefix | `/emergency/x?q=`, `/api/v1/emergency/`, `POST /api/v1/emergency/security-reset?q=` | 403 on every path; `/` without the payload reaches the (absent) upstream: 502 | -The new "Bulk Delete Certificates" section matches the code: path, admin-only POST, request shape, 1 to 200 UUID limit, duplicate handling, response shape with `deleted` and `failed` arrays (both always arrays), reason enum, and the exact 400 / 500 / 507 messages. +On the host proxying Charon, the exact reset path is answered by Charon itself (route 2 above), so it is not subject to the proxy WAF or access list on that one host; the endpoint's own checks apply. This is the approved design (D1). -- **QA-5 (Low, docs):** the new section was inserted between the single "Delete Certificate" responses and that section's trailing paragraph, "**Note:** A backup is automatically created before deletion. The certificate files are removed from disk along with the database record." That note now reads as part of the bulk section. Move it back above `#### Bulk Delete Certificates`. -- Minor gaps: the section does not say the endpoint is admin-only (403 for other roles), and does not mention that UUIDs are returned in canonical lowercase form. +## CodeQL suppression re-review (D11) -## Gotify Token Review +| Rule | Location | Review by | Verdict | +|---|---|---|---| +| `go/cookie-secure-not-set` | `backend/internal/api/handlers/auth_handler.go:201` | 2026-12-05 | Still justified. `Secure` is false only for a plain-HTTP request whose connecting peer is local or private; every other request gets `Secure=true`. Behaviour and doc comment unchanged | +| `go/log-injection` (two taint paths, one sink) | `backend/internal/api/handlers/remote_server_handler.go:148` | 2026-11-27 | Still justified. The logged value is a `uint` ID; `RemoteServer.ID` is `json:"-"`, so the request body cannot set it | +| `go/log-injection` | `backend/internal/services/uptime_service.go:1551` | 2026-11-27 | Still justified. Logs the `uint remoteServerID` argument only | -No Gotify tokens, notification URLs, or secrets appear in the changed code, tests, or docs. +None has expired. Two entries reach their review date on 2026-11-27 and need renewal (or a fix) before then. -## Blockers +## Notes -None. +- **Non-reproducible test failure:** the first scoped run reported `FAIL` for `internal/api/handlers` (209 s) without the failing test in the captured tail. The package then passed twice (a standalone rerun and the full coverage run). The package has no changes in this branch. Recorded for follow-up if it recurs. +- **Branch base moved during the run:** the branch was rebased onto `origin/development` (`d7a3af60a`). The only tree difference from the earlier HEAD is one CI skill script, so results apply to the current HEAD. +- **Wording:** commit subjects, the commit bodies (empty), code comments, test names and this report describe behaviour only.