Fix firewall analysis inflating blocked count with internal Squid error entries

actions/setup/js/firewall_blocked_domains.cjs

@@ -152,12 +152,24 @@ function getBlockedDomains(logsDir) {
         continue;
       }
 
+      // Skip internal Squid error entries (client IP ::1, no domain, no destination)
+      // These are internal Squid connection errors (e.g., error:transaction-end-before-headers)
+      // and are not actual external network requests.
+      // Example: 1773003472.027 ::1:52010 - -:- 0.0 - 0 NONE_NONE:HIER_NONE error:transaction-end-before-headers "-"
+      if (entry.clientIpPort.startsWith("::1:") && entry.domain === "-" && (entry.destIpPort === "-:-" || entry.destIpPort === "-")) {
+        continue;
+      }
+
       // Check if request was blocked
       const isBlocked = isRequestBlocked(entry.decision, entry.status);
       if (isBlocked) {
         // When domain is "-" (iptables-dropped traffic not visible to Squid),
         // fall back to dest IP:port so blocked requests show their actual destination instead of "-"
-        const domainField = entry.domain !== "-" ? entry.domain : entry.destIpPort;
+        // Only fall back if destIpPort is a valid host:port (not "-" or "-:-" which are placeholder values)
+        let domainField = entry.domain;
+        if (domainField === "-" && entry.destIpPort !== "-" && entry.destIpPort !== "-:-") {
+          domainField = entry.destIpPort;
+        }
         const sanitizedDomain = extractAndSanitizeDomain(domainField);
         if (sanitizedDomain && sanitizedDomain !== "-") {
           blockedDomainsSet.add(sanitizedDomain);

actions/setup/js/firewall_blocked_domains.test.cjs

@@ -260,6 +260,30 @@ describe("firewall_blocked_domains.cjs", () => {
       expect(result).not.toContain("allowed.example.com");
     });
 
+    it("should filter out internal Squid error entries (::1 client, -:- destination)", () => {
+      const logsDir = path.join(testDir, "logs-squid-internal");
+      fs.mkdirSync(logsDir, { recursive: true });
+
+      // Internal Squid error entries from localhost (::1) should be ignored
+      const logContent = [
+        '1773003472.027 ::1:52010 - -:- 0.0 - 0 NONE_NONE:HIER_NONE error:transaction-end-before-headers "-"',
+        '1773003475.167 172.30.0.30:50232 api.anthropic.com:443 18.64.224.91:443 1.1 CONNECT 200 TCP_TUNNEL:HIER_DIRECT api.anthropic.com:443 "-"',
+        '1773003477.068 ::1:35712 - -:- 0.0 - 0 NONE_NONE:HIER_NONE error:transaction-end-before-headers "-"',
+        '1773003480.123 172.30.0.30:50235 blocked.example.com:443 10.0.0.1:443 1.1 CONNECT 403 NONE_NONE:HIER_NONE blocked.example.com:443 "-"',
+      ].join("\n");
+
+      fs.writeFileSync(path.join(logsDir, "access.log"), logContent);
+
+      const result = getBlockedDomains(logsDir);
+
+      // Real blocked domain should appear
+      expect(result).toContain("blocked.example.com");
+      // Internal Squid error entries should not appear as "-:-"
+      expect(result).not.toContain("-:-");
+      // Allowed domains should not appear
+      expect(result).not.toContain("api.anthropic.com");
+    });
+
     it("should handle invalid log lines gracefully", () => {
       const logsDir = path.join(testDir, "logs6");
       fs.mkdirSync(logsDir, { recursive: true });

actions/setup/js/parse_firewall_logs.cjs

@@ -46,39 +46,23 @@ async function main() {
       const content = fs.readFileSync(filePath, "utf8");
       const lines = content.split("\n").filter(line => line.trim());
 
-      for (const line of lines) {
-        const entry = parseFirewallLogLine(line);
-        if (!entry) {
-          continue;
-        }
-
-        totalRequests++;
-
-        // Determine if request was allowed or blocked
-        const isAllowed = isRequestAllowed(entry.decision, entry.status);
-
-        // When domain is "-" (iptables-dropped traffic not visible to Squid),
-        // fall back to dest IP:port so blocked requests show their actual destination instead of "-"
-        const domainKey = entry.domain !== "-" ? entry.domain : entry.destIpPort !== "-" ? entry.destIpPort : "-";
-
-        if (isAllowed) {
-          allowedRequests++;
-          allowedDomains.add(domainKey);
-        } else {
-          blockedRequests++;
-          blockedDomains.add(domainKey);
-        }
-
-        // Track request count per domain
-        if (!requestsByDomain.has(domainKey)) {
-          requestsByDomain.set(domainKey, { allowed: 0, blocked: 0 });
-        }
-        const domainStats = requestsByDomain.get(domainKey);
-        if (isAllowed) {
-          domainStats.allowed++;
-        } else {
-          domainStats.blocked++;
+      const result = analyzeFirewallLogLines(lines);
+      totalRequests += result.totalRequests;
+      allowedRequests += result.allowedRequests;
+      blockedRequests += result.blockedRequests;
+      for (const domain of result.allowedDomains) {
+        allowedDomains.add(domain);
+      }
+      for (const domain of result.blockedDomains) {
+        blockedDomains.add(domain);
+      }
+      for (const [domain, stats] of result.requestsByDomain) {
+        if (!requestsByDomain.has(domain)) {
+          requestsByDomain.set(domain, { allowed: 0, blocked: 0 });
         }
+        const existing = requestsByDomain.get(domain);
+        existing.allowed += stats.allowed;
+        existing.blocked += stats.blocked;
       }
     }
 
@@ -163,6 +147,67 @@ function isRequestAllowed(decision, status) {
   return false;
 }
 
+/**
+ * Analyzes an array of raw log lines and returns aggregated request statistics.
+ * Internal Squid error entries (client IP ::1, no domain, no destination) are filtered out.
+ * @param {string[]} lines - Raw log lines to analyze
+ * @returns {{totalRequests: number, allowedRequests: number, blockedRequests: number, allowedDomains: Set<string>, blockedDomains: Set<string>, requestsByDomain: Map<string, {allowed: number, blocked: number}>}}
+ */
+function analyzeFirewallLogLines(lines) {
+  let totalRequests = 0;
+  let allowedRequests = 0;
+  let blockedRequests = 0;
+  const allowedDomains = new Set();
+  const blockedDomains = new Set();
+  const requestsByDomain = new Map();
+
+  for (const line of lines) {
+    const entry = parseFirewallLogLine(line);
+    if (!entry) {
+      continue;
+    }
+
+    // Skip internal Squid error entries (client IP ::1, no domain, no destination)
+    // These are internal Squid connection errors (e.g., error:transaction-end-before-headers)
+    // and are not actual external network requests.
+    // Example: 1773003472.027 ::1:52010 - -:- 0.0 - 0 NONE_NONE:HIER_NONE error:transaction-end-before-headers "-"
+    if (entry.clientIpPort.startsWith("::1:") && entry.domain === "-" && (entry.destIpPort === "-:-" || entry.destIpPort === "-")) {
+      continue;
+    }
+
+    totalRequests++;
+
+    // Determine if request was allowed or blocked
+    const isAllowed = isRequestAllowed(entry.decision, entry.status);
+
+    // When domain is "-" (iptables-dropped traffic not visible to Squid),
+    // fall back to dest IP:port so blocked requests show their actual destination instead of "-"
+    // Only fall back if destIpPort is a valid host:port (not "-" or "-:-" which are placeholder values)
+    const domainKey = entry.domain !== "-" ? entry.domain : entry.destIpPort !== "-" && entry.destIpPort !== "-:-" ? entry.destIpPort : "-";
+
+    if (isAllowed) {
+      allowedRequests++;
+      allowedDomains.add(domainKey);
+    } else {
+      blockedRequests++;
+      blockedDomains.add(domainKey);
+    }
+
+    // Track request count per domain
+    if (!requestsByDomain.has(domainKey)) {
+      requestsByDomain.set(domainKey, { allowed: 0, blocked: 0 });
+    }
+    const domainStats = requestsByDomain.get(domainKey);
+    if (isAllowed) {
+      domainStats.allowed++;
+    } else {
+      domainStats.blocked++;
+    }
+  }
+
+  return { totalRequests, allowedRequests, blockedRequests, allowedDomains, blockedDomains, requestsByDomain };
+}
+
 /**
  * Generates markdown summary from firewall log analysis
  * Uses details/summary structure with basic stats in summary and domain table in details
@@ -218,6 +263,7 @@ if (typeof module !== "undefined" && module.exports) {
   module.exports = {
     parseFirewallLogLine,
     isRequestAllowed,
+    analyzeFirewallLogLines,
     generateFirewallSummary,
     main,
   };

actions/setup/js/parse_firewall_logs.test.cjs

@@ -4,7 +4,7 @@ import path from "path";
 const mockCore = { info: vi.fn(), setFailed: vi.fn(), summary: { addRaw: vi.fn().mockReturnThis(), write: vi.fn().mockResolvedValue() } };
 ((global.core = mockCore),
   describe("parse_firewall_logs.cjs", () => {
-    let parseFirewallLogLine, isRequestAllowed, generateFirewallSummary;
+    let parseFirewallLogLine, isRequestAllowed, analyzeFirewallLogLines, generateFirewallSummary;
     (beforeEach(() => {
       vi.clearAllMocks();
       const scriptPath = path.join(process.cwd(), "parse_firewall_logs.cjs"),
@@ -13,9 +13,13 @@ const mockCore = { info: vi.fn(), setFailed: vi.fn(), summary: { addRaw: vi.fn()
           .replace(/if \(typeof module === "undefined".*?\) \{[\s\S]*?main\(\);[\s\S]*?\}/g, "// main() execution disabled for testing")
           .replace(
             "// Export for testing",
-            "global.testParseFirewallLogLine = parseFirewallLogLine;\n        global.testIsRequestAllowed = isRequestAllowed;\n        global.testGenerateFirewallSummary = generateFirewallSummary;\n        // Export for testing"
+            "global.testParseFirewallLogLine = parseFirewallLogLine;\n        global.testIsRequestAllowed = isRequestAllowed;\n        global.testAnalyzeFirewallLogLines = analyzeFirewallLogLines;\n        global.testGenerateFirewallSummary = generateFirewallSummary;\n        // Export for testing"
           );
-      (eval(scriptForTesting), (parseFirewallLogLine = global.testParseFirewallLogLine), (isRequestAllowed = global.testIsRequestAllowed), (generateFirewallSummary = global.testGenerateFirewallSummary));
+      (eval(scriptForTesting),
+        (parseFirewallLogLine = global.testParseFirewallLogLine),
+        (isRequestAllowed = global.testIsRequestAllowed),
+        (analyzeFirewallLogLines = global.testAnalyzeFirewallLogLines),
+        (generateFirewallSummary = global.testGenerateFirewallSummary));
     }),
       describe("parseFirewallLogLine", () => {
         (test("should parse valid firewall log line", () => {
@@ -68,6 +72,50 @@ const mockCore = { info: vi.fn(), setFailed: vi.fn(), summary: { addRaw: vi.fn()
             expect(isRequestAllowed("NONE_NONE:HIER_NONE", "0")).toBe(!1);
           }));
       }),
+      describe("analyzeFirewallLogLines", () => {
+        (test("should skip internal Squid error entries with -:- destination and count only real traffic", () => {
+          const lines = [
+            '1773003472.027 ::1:52010 - -:- 0.0 - 0 NONE_NONE:HIER_NONE error:transaction-end-before-headers "-"',
+            '1773003475.167 172.30.0.30:50232 api.anthropic.com:443 18.64.224.91:443 1.1 CONNECT 200 TCP_TUNNEL:HIER_DIRECT api.anthropic.com:443 "-"',
+            '1773003477.068 ::1:35712 - -:- 0.0 - 0 NONE_NONE:HIER_NONE error:transaction-end-before-headers "-"',
+            '1773003480.123 172.30.0.30:50235 api.anthropic.com:443 18.64.224.91:443 1.1 CONNECT 200 TCP_TUNNEL:HIER_DIRECT api.anthropic.com:443 "-"',
+            '1773003481.456 ::1:41200 - - 0.0 - 0 NONE_NONE:HIER_NONE error:transaction-end-before-headers "-"',
+          ];
+          const result = analyzeFirewallLogLines(lines);
+          (expect(result.totalRequests).toBe(2),
+            expect(result.allowedRequests).toBe(2),
+            expect(result.blockedRequests).toBe(0),
+            expect(result.requestsByDomain.has("-:-")).toBe(!1),
+            expect(result.requestsByDomain.get("api.anthropic.com:443").allowed).toBe(2));
+        }),
+          test("should not inflate blocked count with internal Squid error entries", () => {
+            // Reproduces the scenario described in the bug report (run 22831150866)
+            const lines = [
+              '1773003472.027 ::1:52010 - -:- 0.0 - 0 NONE_NONE:HIER_NONE error:transaction-end-before-headers "-"',
+              '1773003472.028 ::1:52011 - -:- 0.0 - 0 NONE_NONE:HIER_NONE error:transaction-end-before-headers "-"',
+              '1773003475.167 172.30.0.30:50232 api.anthropic.com:443 18.64.224.91:443 1.1 CONNECT 200 TCP_TUNNEL:HIER_DIRECT api.anthropic.com:443 "-"',
+              '1773003475.168 172.30.0.30:50233 blocked.example.com:443 1.2.3.4:443 1.1 CONNECT 403 NONE_NONE:HIER_NONE blocked.example.com:443 "-"',
+            ];
+            const result = analyzeFirewallLogLines(lines);
+            (expect(result.totalRequests).toBe(2),
+              expect(result.blockedRequests).toBe(1),
+              expect(result.allowedRequests).toBe(1),
+              expect(result.blockedDomains.has("-:-")).toBe(!1),
+              expect(result.blockedDomains.has("blocked.example.com:443")).toBe(!0));
+          }),
+          test("should not treat -:- as a real blocked destination (domain fallback fix)", () => {
+            // When domain="-" and destIpPort="-:-", should not fall back to "-:-" as domain key
+            const lines = ['1773003472.027 172.30.0.20:50000 - -:- 0.0 - 0 NONE_NONE:HIER_NONE - "-"'];
+            const result = analyzeFirewallLogLines(lines);
+            (expect(result.totalRequests).toBe(1), expect(result.requestsByDomain.has("-:-")).toBe(!1), expect(result.requestsByDomain.has("-")).toBe(!0));
+          }),
+          test("should still count iptables-dropped traffic with real destIpPort", () => {
+            // When domain="-" but destIpPort is a real IP:port (iptables-dropped), use destIpPort as key
+            const lines = ['1761332531.123 172.30.0.20:35289 - 8.8.8.8:53 - - 0 NONE_NONE:HIER_NONE - "-"', '1761332532.456 172.30.0.20:35290 - 1.2.3.4:443 - - 0 NONE_NONE:HIER_NONE - "-"'];
+            const result = analyzeFirewallLogLines(lines);
+            (expect(result.totalRequests).toBe(2), expect(result.blockedRequests).toBe(2), expect(result.requestsByDomain.get("8.8.8.8:53").blocked).toBe(1), expect(result.requestsByDomain.get("1.2.3.4:443").blocked).toBe(1));
+          }));
+      }),
       describe("generateFirewallSummary", () => {
         (test("should generate summary with details/summary structure", () => {
           const analysis = {

pkg/cli/firewall_log.go

@@ -247,15 +247,24 @@ func parseFirewallLog(logPath string, verbose bool) (*FirewallAnalysis, error) {
 			continue
 		}
 
+		// Skip internal Squid error entries (client IP ::1, no domain, no destination)
+		// These are internal Squid connection errors (e.g., error:transaction-end-before-headers)
+		// and are not actual external network requests.
+		// Example: 1773003472.027 ::1:52010 - -:- 0.0 - 0 NONE_NONE:HIER_NONE error:transaction-end-before-headers "-"
+		if strings.HasPrefix(entry.ClientIPPort, "::1:") && entry.Domain == "-" && (entry.DestIPPort == "-:-" || entry.DestIPPort == "-") {
+			continue
+		}
+
 		analysis.TotalRequests++
 
 		// Determine if request was allowed or blocked
 		isAllowed := isRequestAllowed(entry.Decision, entry.Status)
 
 		// Extract domain - when domain is "-" (iptables-dropped traffic not visible to Squid),
 		// fall back to dest IP:port so blocked requests show their actual destination instead of "-"
+		// Only fall back if destIPPort is a valid host:port (not "-" or "-:-" which are placeholder values)
 		domain := entry.Domain
-		if domain == "-" && entry.DestIPPort != "-" {
+		if domain == "-" && entry.DestIPPort != "-" && entry.DestIPPort != "-:-" {
 			domain = entry.DestIPPort
 		}
 

pkg/cli/firewall_log_test.go

@@ -702,3 +702,95 @@ func TestAnalyzeFirewallLogsWithWorkflowSuffix(t *testing.T) {
 		t.Errorf("BlockedDomains: got %v, want [blocked.example.com:443]", analysis.BlockedDomains)
 	}
 }
+
+func TestParseFirewallLogInternalSquidErrorEntries(t *testing.T) {
+	// Create a temporary directory for the test
+	tempDir := testutil.TempDir(t, "test-*")
+
+	// Simulate internal Squid error entries interleaved with real traffic.
+	// These internal entries (client IP ::1, domain "-", destIPPort "-:-") are internal
+	// Squid connection errors (e.g., error:transaction-end-before-headers) and should be
+	// filtered out entirely and not counted as blocked external requests.
+	testLogContent := `1773003472.027 ::1:52010 - -:- 0.0 - 0 NONE_NONE:HIER_NONE error:transaction-end-before-headers "-"
+1773003475.167 172.30.0.30:50232 api.anthropic.com:443 18.64.224.91:443 1.1 CONNECT 200 TCP_TUNNEL:HIER_DIRECT api.anthropic.com:443 "-"
+1773003477.068 ::1:35712 - -:- 0.0 - 0 NONE_NONE:HIER_NONE error:transaction-end-before-headers "-"
+1773003480.123 172.30.0.30:50235 api.anthropic.com:443 18.64.224.91:443 1.1 CONNECT 200 TCP_TUNNEL:HIER_DIRECT api.anthropic.com:443 "-"
+1773003481.456 ::1:41200 - - 0.0 - 0 NONE_NONE:HIER_NONE error:transaction-end-before-headers "-"
+`
+
+	// Write test log file
+	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)
+	}
+
+	// Test parsing
+	analysis, err := parseFirewallLog(logPath, false)
+	if err != nil {
+		t.Fatalf("Failed to parse firewall log: %v", err)
+	}
+
+	// Internal Squid error entries should be filtered out entirely
+	// Only the 2 real allowed requests should be counted
+	if analysis.TotalRequests != 2 {
+		t.Errorf("TotalRequests: got %d, want 2 (internal Squid entries should be excluded)", analysis.TotalRequests)
+	}
+	if analysis.AllowedRequests != 2 {
+		t.Errorf("AllowedRequests: got %d, want 2", analysis.AllowedRequests)
+	}
+	if analysis.BlockedRequests != 0 {
+		t.Errorf("BlockedRequests: got %d, want 0 (internal Squid entries should not be counted as blocked)", analysis.BlockedRequests)
+	}
+
+	// "-:-" should not appear in blocked domains
+	for _, d := range analysis.BlockedDomains {
+		if d == "-:-" {
+			t.Error("BlockedDomains should not contain \"-:-\" (internal Squid error entries should be filtered out)")
+		}
+	}
+
+	// The real traffic should still be tracked
+	if stats, ok := analysis.RequestsByDomain["api.anthropic.com:443"]; !ok {
+		t.Error("api.anthropic.com:443 should be in RequestsByDomain")
+	} else if stats.Allowed != 2 {
+		t.Errorf("api.anthropic.com:443 Allowed: got %d, want 2", stats.Allowed)
+	}
+}
+
+func TestParseFirewallLogInternalSquidErrorEntriesDashDash(t *testing.T) {
+	// Create a temporary directory for the test
+	tempDir := testutil.TempDir(t, "test-*")
+
+	// Simulate internal Squid error entries where destIPPort is just "-" (not "-:-")
+	// These should also be filtered out
+	testLogContent := `1773003481.456 ::1:41200 - - 0.0 - 0 NONE_NONE:HIER_NONE error:transaction-end-before-headers "-"
+1773003482.123 172.30.0.30:50235 blocked.example.com:443 10.0.0.1:443 1.1 CONNECT 403 NONE_NONE:HIER_NONE blocked.example.com:443 "-"
+`
+
+	// Write test log file
+	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)
+	}
+
+	// Test parsing
+	analysis, err := parseFirewallLog(logPath, false)
+	if err != nil {
+		t.Fatalf("Failed to parse firewall log: %v", err)
+	}
+
+	// Only the 1 real blocked request should be counted
+	if analysis.TotalRequests != 1 {
+		t.Errorf("TotalRequests: got %d, want 1 (internal Squid entry should be excluded)", analysis.TotalRequests)
+	}
+	if analysis.BlockedRequests != 1 {
+		t.Errorf("BlockedRequests: got %d, want 1", analysis.BlockedRequests)
+	}
+
+	// The real blocked domain should appear, not internal error entries
+	if len(analysis.BlockedDomains) != 1 || analysis.BlockedDomains[0] != "blocked.example.com:443" {
+		t.Errorf("BlockedDomains: got %v, want [blocked.example.com:443]", analysis.BlockedDomains)
+	}
+}