fix(audit): exclude - placeholder from blocked domains and allow-list recommendations

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(

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")
+}

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
 			}
 		}

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) {