From e7b9301bd2f5bccff8f96c9e870b14e1a9eeb16f Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Wed, 15 Apr 2026 06:09:24 +0000 Subject: [PATCH 1/2] Initial plan From 5adbdbcf0a06e35098922e1f1a15f74694d12e87 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Wed, 15 Apr 2026 06:32:44 +0000 Subject: [PATCH 2/2] fix: filter '-' placeholder domain from firewall audit report and recommendations - In firewall_log.go: replace '-' with '(unknown)' sentinel when both domain and destIPPort are placeholders; exclude sentinel from BlockedDomains/AllowedDomains sets so it never appears in allow-list recommendations - In audit_report_analysis.go: add filterActionableDomains() helper that strips '-' and '(unknown)' before building findings descriptions and network allow-list recommendation examples (defense-in-depth) - Update TestParseFirewallLogIptablesDropped to expect '(unknown)' in RequestsByDomain and verify '-' is absent from BlockedDomains - Add TestParseFirewallLogUnknownAllowedDomain for the allowed-traffic edge case - Add TestGenerateRecommendationsFiltersDashPlaceholder and TestGenerateRecommendationsFiltersUnknownSentinel in audit_report_test.go Fixes #" Agent-Logs-Url: https://github.com/github/gh-aw/sessions/dd8a9c21-b6de-4bcd-bf2f-910a20093667 Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- pkg/cli/audit_report_analysis.go | 18 ++++++++-- pkg/cli/audit_report_test.go | 60 ++++++++++++++++++++++++++++++++ pkg/cli/firewall_log.go | 17 +++++++-- pkg/cli/firewall_log_test.go | 59 ++++++++++++++++++++++++++++--- 4 files changed, 146 insertions(+), 8 deletions(-) diff --git a/pkg/cli/audit_report_analysis.go b/pkg/cli/audit_report_analysis.go index 46e03aeb442..72b13b82539 100644 --- a/pkg/cli/audit_report_analysis.go +++ b/pkg/cli/audit_report_analysis.go @@ -10,6 +10,20 @@ import ( "github.com/github/gh-aw/pkg/timeutil" ) +// filterActionableDomains removes placeholder values from a domain list. +// "-" and unknownDomain ("(unknown)") can appear when iptables drops traffic +// before Squid identifies the destination; they are not real domains and should +// not appear in allow-list recommendations. +func filterActionableDomains(domains []string) []string { + result := make([]string, 0, len(domains)) + for _, d := range domains { + if d != "-" && d != unknownDomain { + result = append(result, d) + } + } + return result +} + // generateFindings creates key findings from workflow run data func generateFindings(processedRun ProcessedRun, metrics MetricsData, errors []ErrorInfo) []Finding { auditReportLog.Printf("Generating findings: errors=%d, conclusion=%s", len(errors), processedRun.Run.Conclusion) @@ -143,7 +157,7 @@ func generateFindings(processedRun ProcessedRun, metrics MetricsData, errors []E // Firewall findings if processedRun.FirewallAnalysis != nil && processedRun.FirewallAnalysis.BlockedRequests > 0 { - blockedDomains := processedRun.FirewallAnalysis.GetBlockedDomains() + blockedDomains := filterActionableDomains(processedRun.FirewallAnalysis.GetBlockedDomains()) var desc string switch { case len(blockedDomains) == 1: @@ -253,7 +267,7 @@ func generateRecommendations(processedRun ProcessedRun, metrics MetricsData, fin // Recommendations for firewall blocks – trigger on any block so even a single // domain denial (e.g. Codex CLI reporting one blocked domain) surfaces an action. if processedRun.FirewallAnalysis != nil && processedRun.FirewallAnalysis.BlockedRequests > 0 { - blockedDomains := processedRun.FirewallAnalysis.GetBlockedDomains() + blockedDomains := filterActionableDomains(processedRun.FirewallAnalysis.GetBlockedDomains()) var example string if len(blockedDomains) > 0 { example = fmt.Sprintf( diff --git a/pkg/cli/audit_report_test.go b/pkg/cli/audit_report_test.go index f2d3416e92a..fa3a4a19c3b 100644 --- a/pkg/cli/audit_report_test.go +++ b/pkg/cli/audit_report_test.go @@ -1623,3 +1623,63 @@ func TestGenerateRecommendationsFirewallSingleBlock(t *testing.T) { assert.Contains(t, networkRec.Example, "chatgpt.com", "Recommendation example should include the blocked domain name") } + +func TestGenerateRecommendationsFiltersDashPlaceholder(t *testing.T) { + // Defense-in-depth: even if "-" somehow appears in BlockedDomains (e.g. from + // extractFirewallFromAgentLog or a future code path), the recommendation must + // not include it as an allow-list entry. In practice, parseFirewallLog now + // replaces "-" with the unknownDomain sentinel, but other ingestion paths may + // still produce "-" entries. + pr := createTestProcessedRun() + fw := &FirewallAnalysis{ + TotalRequests: 1, + BlockedRequests: 1, + AllowedRequests: 0, + RequestsByDomain: map[string]DomainRequestStats{"-": {Blocked: 1}}, + } + fw.SetBlockedDomains([]string{"-"}) + pr.FirewallAnalysis = fw + + findings := []Finding{{Category: "network", Severity: "medium", Title: "Blocked Network Requests"}} + recs := generateRecommendations(pr, MetricsData{}, findings) + + var networkRec *Recommendation + for i := range recs { + if strings.Contains(recs[i].Action, "network") || strings.Contains(recs[i].Action, "blocked") { + networkRec = &recs[i] + break + } + } + require.NotNil(t, networkRec, "Should still generate a network recommendation when blocked requests exist") + assert.NotContains(t, networkRec.Example, " - -", + "Recommendation example should not include the \"-\" placeholder as an allow-list entry") +} + +func TestGenerateRecommendationsFiltersUnknownSentinel(t *testing.T) { + // When blocked domains only contain the unknownDomain sentinel (from iptables drops + // where no destination info is available), the recommendation should not include the + // sentinel in the allow-list example. + pr := createTestProcessedRun() + fw := &FirewallAnalysis{ + TotalRequests: 1, + BlockedRequests: 1, + AllowedRequests: 0, + RequestsByDomain: map[string]DomainRequestStats{unknownDomain: {Blocked: 1}}, + } + fw.SetBlockedDomains([]string{unknownDomain}) + pr.FirewallAnalysis = fw + + findings := []Finding{{Category: "network", Severity: "medium", Title: "Blocked Network Requests"}} + recs := generateRecommendations(pr, MetricsData{}, findings) + + var networkRec *Recommendation + for i := range recs { + if strings.Contains(recs[i].Action, "network") || strings.Contains(recs[i].Action, "blocked") { + networkRec = &recs[i] + break + } + } + require.NotNil(t, networkRec, "Should still generate a network recommendation when blocked requests exist") + assert.NotContains(t, networkRec.Example, unknownDomain, + "Recommendation example should not include the unknownDomain sentinel as an allow-list entry") +} diff --git a/pkg/cli/firewall_log.go b/pkg/cli/firewall_log.go index 2860a79a5b5..7b0057b4b79 100644 --- a/pkg/cli/firewall_log.go +++ b/pkg/cli/firewall_log.go @@ -17,6 +17,13 @@ import ( var firewallLogLog = logger.New("cli:firewall_log") +// unknownDomain is a sentinel value used in RequestsByDomain when iptables drops +// traffic before Squid can identify the destination, and no fallback IP:port is +// available (both domain and destIPPort are "-"). These entries are tracked for +// informational purposes but excluded from BlockedDomains/AllowedDomains since +// they carry no actionable domain information. +const unknownDomain = "(unknown)" + // Pre-compiled regexes for firewall log parsing (performance optimization) var ( firewallLogFieldSplitter = regexp.MustCompile(`(?:[^\s"]+|"[^"]*")+`) @@ -312,16 +319,22 @@ func parseFirewallLog(logPath string, verbose bool) (*FirewallAnalysis, error) { domain := entry.Domain if domain == "-" && entry.DestIPPort != "-" && entry.DestIPPort != "-:-" { domain = entry.DestIPPort + } else if domain == "-" { + // Both domain and destIPPort are placeholders: iptables dropped the traffic before + // Squid could identify the destination. Use a sentinel so the entry appears in + // RequestsByDomain for informational purposes, but do NOT add it to the domain sets + // since "-" is not an actionable domain name. + domain = unknownDomain } if isAllowed { analysis.AllowedRequests++ - if !allowedDomainsSet[domain] { + if domain != unknownDomain && !allowedDomainsSet[domain] { allowedDomainsSet[domain] = true } } else { analysis.BlockedRequests++ - if !blockedDomainsSet[domain] { + if domain != unknownDomain && !blockedDomainsSet[domain] { blockedDomainsSet[domain] = true } } diff --git a/pkg/cli/firewall_log_test.go b/pkg/cli/firewall_log_test.go index a2a8cd047d6..4cc3f977051 100644 --- a/pkg/cli/firewall_log_test.go +++ b/pkg/cli/firewall_log_test.go @@ -503,11 +503,16 @@ func TestParseFirewallLogIptablesDropped(t *testing.T) { t.Errorf("1.2.3.4:443 Blocked: got %d, want 2", stats.Blocked) } - // "-" should only appear for entries where both domain and destIPPort are "-" - if stats, ok := analysis.RequestsByDomain["-"]; !ok { - t.Error("\"-\" should be in RequestsByDomain for truly-unknown entries") + // Entries where both domain and destIPPort are "-" should use the unknownDomain sentinel. + if stats, ok := analysis.RequestsByDomain[unknownDomain]; !ok { + t.Errorf("%q should be in RequestsByDomain for truly-unknown entries", unknownDomain) } else if stats.Blocked != 1 { - t.Errorf("\"-\" Blocked: got %d, want 1", stats.Blocked) + t.Errorf("%q Blocked: got %d, want 1", unknownDomain, stats.Blocked) + } + + // "-" should NOT appear as a key in RequestsByDomain after the sentinel replacement. + if _, ok := analysis.RequestsByDomain["-"]; ok { + t.Error("\"-\" should not be in RequestsByDomain; it should be replaced by the unknownDomain sentinel") } // BlockedDomains should include the real IPs, not just "-" @@ -521,6 +526,52 @@ func TestParseFirewallLogIptablesDropped(t *testing.T) { if !blockedSet["1.2.3.4:443"] { t.Error("BlockedDomains should contain 1.2.3.4:443 (iptables-dropped fallback)") } + // Neither the "-" placeholder nor the unknownDomain sentinel should appear in BlockedDomains. + if blockedSet["-"] { + t.Error("BlockedDomains should NOT contain \"-\" placeholder") + } + if blockedSet[unknownDomain] { + t.Errorf("BlockedDomains should NOT contain %q sentinel", unknownDomain) + } +} + +func TestParseFirewallLogUnknownAllowedDomain(t *testing.T) { + // Verify that when both domain and destIPPort are "-" and the request is classified as + // allowed, the unknownDomain sentinel is excluded from AllowedDomains. + // This is an unlikely but possible edge case (e.g. Squid internally marks a + // packet as allowed before iptables drops it at the network layer). + tempDir := testutil.TempDir(t, "test-*") + + // Status 200 + TCP_TUNNEL:HIER_DIRECT = allowed; domain and destIPPort both "-" + testLogContent := `1761332530.474 172.30.0.20:35288 - - - - 200 TCP_TUNNEL:HIER_DIRECT - "-" +` + logPath := filepath.Join(tempDir, "firewall.log") + err := os.WriteFile(logPath, []byte(testLogContent), 0644) + if err != nil { + t.Fatalf("Failed to create test firewall.log: %v", err) + } + + analysis, err := parseFirewallLog(logPath, false) + if err != nil { + t.Fatalf("Failed to parse firewall log: %v", err) + } + + if analysis.AllowedRequests != 1 { + t.Errorf("AllowedRequests: got %d, want 1", analysis.AllowedRequests) + } + + // The unknownDomain sentinel should appear in RequestsByDomain but NOT in AllowedDomains. + if stats, ok := analysis.RequestsByDomain[unknownDomain]; !ok { + t.Errorf("%q should be in RequestsByDomain", unknownDomain) + } else if stats.Allowed != 1 { + t.Errorf("%q Allowed: got %d, want 1", unknownDomain, stats.Allowed) + } + + for _, d := range analysis.AllowedDomains { + if d == unknownDomain || d == "-" { + t.Errorf("AllowedDomains should NOT contain %q placeholder/sentinel", d) + } + } } func TestAnalyzeMultipleFirewallLogs(t *testing.T) {