diff --git a/pkg/cli/firewall_policy.go b/pkg/cli/firewall_policy.go index 36a48667b2..6d5477fbca 100644 --- a/pkg/cli/firewall_policy.go +++ b/pkg/cli/firewall_policy.go @@ -411,7 +411,7 @@ func enrichWithPolicyRules(entries []AuditLogEntry, manifest *PolicyManifest) *P // 2. agent/sandbox/firewall/audit/ — non-flattened unified agent artifact (new structure) // 3. agent/tmp/gh-aw/sandbox/firewall/audit/ — non-flattened unified agent artifact (old structure) // 4. firewall-audit*/ — legacy separate firewall-audit-logs artifact (backward compat) -func detectFirewallAuditArtifacts(runDir string) (manifestPath, auditJSONLPath string) { +func detectFirewallAuditArtifacts(runDir string) (manifestPath, auditJSONLPath string, err error) { firewallPolicyLog.Printf("Detecting firewall audit artifacts in: %s", runDir) // checkDir probes dir for policy-manifest.json and audit.jsonl, populating the @@ -456,15 +456,16 @@ func detectFirewallAuditArtifacts(runDir string) (manifestPath, auditJSONLPath s checkDir(filepath.Join(agentDir, "tmp", "gh-aw", "sandbox", "firewall", "audit"), agentBase+"/tmp/gh-aw/sandbox/firewall/audit") } if manifestPath != "" && auditJSONLPath != "" { - return + return manifestPath, auditJSONLPath, nil } } } // 4. Legacy separate firewall-audit-logs artifact (backward compat for older runs that // uploaded the audit directory as a standalone artifact named firewall-audit-logs). - entries, err := os.ReadDir(runDir) - if err != nil { + entries, readErr := os.ReadDir(runDir) + if readErr != nil { + err = fmt.Errorf("detectFirewallAuditArtifacts: reading run dir %s: %w", runDir, readErr) return } for _, entry := range entries { @@ -477,13 +478,16 @@ func detectFirewallAuditArtifacts(runDir string) (manifestPath, auditJSONLPath s } } - return manifestPath, auditJSONLPath + return manifestPath, auditJSONLPath, nil } // analyzeFirewallPolicy loads policy manifest and audit JSONL, then enriches with rule attribution. // Returns nil if no policy artifacts are found (graceful fallback to basic analysis). func analyzeFirewallPolicy(runDir string, verbose bool) (*PolicyAnalysis, error) { - manifestPath, auditJSONLPath := detectFirewallAuditArtifacts(runDir) + manifestPath, auditJSONLPath, err := detectFirewallAuditArtifacts(runDir) + if err != nil { + return nil, fmt.Errorf("failed to detect firewall audit artifacts: %w", err) + } if manifestPath == "" { firewallPolicyLog.Print("No policy manifest found, skipping policy analysis") diff --git a/pkg/cli/firewall_policy_test.go b/pkg/cli/firewall_policy_test.go index 347288dda3..b8e3ae39d3 100644 --- a/pkg/cli/firewall_policy_test.go +++ b/pkg/cli/firewall_policy_test.go @@ -561,7 +561,8 @@ func TestDetectFirewallAuditArtifacts(t *testing.T) { require.NoError(t, os.WriteFile(manifestPath, []byte("{}"), 0644)) require.NoError(t, os.WriteFile(auditPath, []byte(""), 0644)) - foundManifest, foundAudit := detectFirewallAuditArtifacts(dir) + foundManifest, foundAudit, err := detectFirewallAuditArtifacts(dir) + require.NoError(t, err, "Should not error with valid run dir") assert.Equal(t, manifestPath, foundManifest, "Should find policy manifest") assert.Equal(t, auditPath, foundAudit, "Should find audit JSONL") }) @@ -579,7 +580,8 @@ func TestDetectFirewallAuditArtifacts(t *testing.T) { require.NoError(t, os.WriteFile(manifestPath, []byte("{}"), 0644)) require.NoError(t, os.WriteFile(auditPath, []byte(""), 0644)) - foundManifest, foundAudit := detectFirewallAuditArtifacts(dir) + foundManifest, foundAudit, err := detectFirewallAuditArtifacts(dir) + require.NoError(t, err, "Should not error with valid run dir") assert.Equal(t, manifestPath, foundManifest, "Should find policy manifest in agent/sandbox/firewall/audit") assert.Equal(t, auditPath, foundAudit, "Should find audit JSONL in agent/sandbox/firewall/audit") }) @@ -596,7 +598,8 @@ func TestDetectFirewallAuditArtifacts(t *testing.T) { require.NoError(t, os.WriteFile(manifestPath, []byte("{}"), 0644)) require.NoError(t, os.WriteFile(auditPath, []byte(""), 0644)) - foundManifest, foundAudit := detectFirewallAuditArtifacts(dir) + foundManifest, foundAudit, err := detectFirewallAuditArtifacts(dir) + require.NoError(t, err, "Should not error with valid run dir") assert.Equal(t, manifestPath, foundManifest, "Should find policy manifest in agent/tmp/gh-aw/sandbox/firewall/audit") assert.Equal(t, auditPath, foundAudit, "Should find audit JSONL in agent/tmp/gh-aw/sandbox/firewall/audit") }) @@ -612,7 +615,8 @@ func TestDetectFirewallAuditArtifacts(t *testing.T) { require.NoError(t, os.WriteFile(manifestPath, []byte("{}"), 0644)) require.NoError(t, os.WriteFile(auditPath, []byte(""), 0644)) - foundManifest, foundAudit := detectFirewallAuditArtifacts(dir) + foundManifest, foundAudit, err := detectFirewallAuditArtifacts(dir) + require.NoError(t, err, "Should not error with valid run dir") assert.Equal(t, manifestPath, foundManifest, "Should find policy manifest in agent-artifacts/sandbox/firewall/audit") assert.Equal(t, auditPath, foundAudit, "Should find audit JSONL in agent-artifacts/sandbox/firewall/audit") }) @@ -629,7 +633,8 @@ func TestDetectFirewallAuditArtifacts(t *testing.T) { require.NoError(t, os.WriteFile(manifestPath, []byte("{}"), 0644)) require.NoError(t, os.WriteFile(auditPath, []byte(""), 0644)) - foundManifest, foundAudit := detectFirewallAuditArtifacts(dir) + foundManifest, foundAudit, err := detectFirewallAuditArtifacts(dir) + require.NoError(t, err, "Should not error with valid run dir") assert.Equal(t, manifestPath, foundManifest, "Should find policy manifest in prefixed-agent/sandbox/firewall/audit") assert.Equal(t, auditPath, foundAudit, "Should find audit JSONL in prefixed-agent/sandbox/firewall/audit") }) @@ -644,14 +649,16 @@ func TestDetectFirewallAuditArtifacts(t *testing.T) { require.NoError(t, os.WriteFile(manifestPath, []byte("{}"), 0644)) require.NoError(t, os.WriteFile(auditPath, []byte(""), 0644)) - foundManifest, foundAudit := detectFirewallAuditArtifacts(dir) + foundManifest, foundAudit, err := detectFirewallAuditArtifacts(dir) + require.NoError(t, err, "Should not error with valid run dir") assert.Equal(t, manifestPath, foundManifest, "Should find policy manifest in firewall-audit-logs") assert.Equal(t, auditPath, foundAudit, "Should find audit JSONL in firewall-audit-logs") }) t.Run("no artifacts", func(t *testing.T) { dir := t.TempDir() - foundManifest, foundAudit := detectFirewallAuditArtifacts(dir) + foundManifest, foundAudit, err := detectFirewallAuditArtifacts(dir) + require.NoError(t, err, "Should not error with empty run dir") assert.Empty(t, foundManifest, "Should not find manifest") assert.Empty(t, foundAudit, "Should not find audit JSONL") }) @@ -662,10 +669,24 @@ func TestDetectFirewallAuditArtifacts(t *testing.T) { dir := t.TempDir() require.NoError(t, os.WriteFile(filepath.Join(dir, "agent"), []byte("not a directory"), 0644)) - foundManifest, foundAudit := detectFirewallAuditArtifacts(dir) + foundManifest, foundAudit, err := detectFirewallAuditArtifacts(dir) + require.NoError(t, err, "Should not error when 'agent' is a file") assert.Empty(t, foundManifest, "Should not find manifest when 'agent' is a file") assert.Empty(t, foundAudit, "Should not find audit JSONL when 'agent' is a file") }) + + t.Run("unreadable run directory returns error", func(t *testing.T) { + if os.Getuid() == 0 { + t.Skip("root can read any directory; skipping permission test") + } + dir := t.TempDir() + require.NoError(t, os.Chmod(dir, 0000), "Should be able to remove read permission") + t.Cleanup(func() { _ = os.Chmod(dir, 0755) }) + + _, _, err := detectFirewallAuditArtifacts(dir) + require.Error(t, err, "Should return an error when run dir is unreadable") + assert.Contains(t, err.Error(), "detectFirewallAuditArtifacts", "Error should identify the function") + }) } func TestAnalyzeFirewallPolicy(t *testing.T) {