Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 19 additions & 1 deletion pkg/cli/firewall_log.go
Original file line number Diff line number Diff line change
Expand Up @@ -372,9 +372,27 @@ func analyzeFirewallLogs(runDir string, verbose bool) (*FirewallAnalysis, error)

// First, check for sandbox/firewall/logs/ directory (new path after artifact download)
// Firewall logs are uploaded from /tmp/gh-aw/sandbox/firewall/logs/ and the common parent
// /tmp/gh-aw/ is stripped during artifact upload, resulting in sandbox/firewall/logs/ after download
// /tmp/gh-aw/ is stripped during artifact upload, resulting in sandbox/firewall/logs/ after download.
// Within that directory, AWF writes Squid access logs to a squid-logs/ subdirectory.
sandboxFirewallLogsDir := filepath.Join(runDir, "sandbox", "firewall", "logs")
if fileutil.DirExists(sandboxFirewallLogsDir) {
// AWF writes the Squid access.log to a squid-logs/ subdirectory within --proxy-logs-dir.
// Check for that subdirectory first before falling back to the parent directory.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[/codebase-design] The string path sandbox/firewall/logs/squid-logs is now duplicated verbatim across firewall_log.go (here) and logs_parsing_firewall.go (line 44). If the artifact layout ever changes again, both sites need updating and it's easy to miss one.

💡 Suggested approach

Define a package-level constant or constructor to centralise the path:

// squidLogsSubDir is the subdirectory within the sandbox firewall logs dir
// where AWF writes the Squid access.log.
const squidLogsSubDir = "squid-logs"

Or extract a helper function that returns the ordered candidate dirs (see also the comment in logs_parsing_firewall.go about divergent fallback chains).

@copilot please address this.

squidSubDir := filepath.Join(sandboxFirewallLogsDir, "squid-logs")
if fileutil.DirExists(squidSubDir) {
firewallLogLog.Printf("Found firewall logs directory: sandbox/firewall/logs/squid-logs")

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hardcoded path literal in log message will mislead debugging: the message emits 'sandbox/firewall/logs/squid-logs' as a literal string rather than the resolved squidSubDir path, so if the run directory is not under CWD the logged location won't match where the file was actually found.

Suggested fix

Every other log site in this function uses the actual resolved path variable. Use squidSubDir here and on the existing fallback line below:

firewallLogLog.Printf("Found firewall logs directory: %s", squidSubDir)
if verbose {
    fmt.Fprintln(os.Stderr, console.FormatInfoMessage("Found firewall logs directory: "+squidSubDir))
}

The existing fallback at line 390-393 has the same issue with sandboxFirewallLogsDir.

if verbose {
fmt.Fprintln(os.Stderr, console.FormatInfoMessage("Found firewall logs directory: sandbox/firewall/logs/squid-logs"))
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[/tdd] No test covers the edge case where sandbox/firewall/logs/squid-logs/ exists but is empty (no *.log files inside). Depending on analyzeMultipleFirewallLogs's behaviour, the result may be nil, nil, causing silent success with no analysis — which could mask a real problem at runtime.

💡 Suggested test

Add TestAnalyzeFirewallLogsSandboxSquidSubdirEmpty that creates the directory without any log files and asserts either a non-nil error or a specific "no logs found" outcome, so the contract is explicit:

func TestAnalyzeFirewallLogsSandboxSquidSubdirEmpty(t *testing.T) {
    tmpDir := testutil.TempDir(t, "test-squid-empty-*")
    squidDir := filepath.Join(tmpDir, "sandbox", "firewall", "logs", "squid-logs")
    os.MkdirAll(squidDir, 0755)
    // no files written
    analysis, err := analyzeFirewallLogs(tmpDir, false)
    // assert expected behaviour: err != nil OR analysis.TotalRequests == 0
}

@copilot please address this.

analysis, err := analyzeMultipleFirewallLogs(squidSubDir, verbose)
if err != nil {
return nil, err
}
if analysis != nil {
return analysis, nil
}
}
// Fall back to direct *.log files at the sandbox/firewall/logs/ level (older AWF layout).
firewallLogLog.Printf("Found firewall logs directory: sandbox/firewall/logs")
if verbose {
fmt.Fprintln(os.Stderr, console.FormatInfoMessage("Found firewall logs directory: sandbox/firewall/logs"))
Expand Down
98 changes: 98 additions & 0 deletions pkg/cli/firewall_log_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -1009,6 +1009,104 @@ func TestFirewallAnalysisAddMetricsMergesDomains(t *testing.T) {
}
}

// TestAnalyzeFirewallLogsSandboxSquidSubdir verifies that analyzeFirewallLogs
// correctly finds access.log in the sandbox/firewall/logs/squid-logs/ subdirectory.
// AWF writes Squid logs to {proxy-logs-dir}/squid-logs/access.log, so after the
// agent artifact is downloaded and flattened the path is:
// {runDir}/sandbox/firewall/logs/squid-logs/access.log
func TestAnalyzeFirewallLogsSandboxSquidSubdir(t *testing.T) {
tmpDir := testutil.TempDir(t, "test-sandbox-squid-*")

squidLogsDir := filepath.Join(tmpDir, "sandbox", "firewall", "logs", "squid-logs")
if err := os.MkdirAll(squidLogsDir, 0755); err != nil {
t.Fatalf("Failed to create squid-logs directory: %v", err)
}

logContent := `1761332530.474 172.30.0.20:35288 api.enterprise.githubcopilot.com:443 140.82.112.22:443 1.1 CONNECT 200 TCP_TUNNEL:HIER_DIRECT api.enterprise.githubcopilot.com:443 "-"
1761332531.123 172.30.0.20:35289 blocked.example.com:443 140.82.112.23:443 1.1 CONNECT 403 NONE_NONE:HIER_NONE blocked.example.com:443 "-"
1761332532.456 172.30.0.20:35290 api.github.com:443 140.82.112.5:443 1.1 CONNECT 200 TCP_TUNNEL:HIER_DIRECT api.github.com:443 "-"
`
if err := os.WriteFile(filepath.Join(squidLogsDir, "access.log"), []byte(logContent), 0644); err != nil {
t.Fatalf("Failed to write access.log: %v", err)
}

analysis, err := analyzeFirewallLogs(tmpDir, false)
if err != nil {
t.Fatalf("analyzeFirewallLogs failed: %v", err)
}
if analysis == nil {
t.Fatal("Expected firewall analysis but got nil - access.log in squid-logs/ subdirectory was not found")
}

if analysis.TotalRequests != 3 {
t.Errorf("TotalRequests: got %d, want 3", analysis.TotalRequests)
}
if analysis.AllowedRequests != 2 {
t.Errorf("AllowedRequests: got %d, want 2", analysis.AllowedRequests)
}
if analysis.BlockedRequests != 1 {
t.Errorf("BlockedRequests: got %d, want 1", analysis.BlockedRequests)
}
if len(analysis.BlockedDomains) != 1 || analysis.BlockedDomains[0] != "blocked.example.com:443" {
t.Errorf("BlockedDomains: got %v, want [blocked.example.com:443]", analysis.BlockedDomains)
}
}

// TestAnalyzeFirewallLogsSandboxFallbackToTopLevel verifies that when sandbox/firewall/logs/
// exists but has no squid-logs/ subdirectory, analyzeFirewallLogs falls back to looking for
// *.log files directly in sandbox/firewall/logs/ (backward compatibility for older AWF layout).
func TestAnalyzeFirewallLogsSandboxFallbackToTopLevel(t *testing.T) {
tmpDir := testutil.TempDir(t, "test-sandbox-fallback-*")

sandboxLogsDir := filepath.Join(tmpDir, "sandbox", "firewall", "logs")
if err := os.MkdirAll(sandboxLogsDir, 0755); err != nil {
t.Fatalf("Failed to create sandbox/firewall/logs directory: %v", err)
}

logContent := `1761332530.474 172.30.0.20:35288 api.github.com:443 140.82.112.5:443 1.1 CONNECT 200 TCP_TUNNEL:HIER_DIRECT api.github.com:443 "-"
`
if err := os.WriteFile(filepath.Join(sandboxLogsDir, "access.log"), []byte(logContent), 0644); err != nil {
t.Fatalf("Failed to write access.log: %v", err)
}

analysis, err := analyzeFirewallLogs(tmpDir, false)
if err != nil {
t.Fatalf("analyzeFirewallLogs failed: %v", err)
}
if analysis == nil {
t.Fatal("Expected firewall analysis but got nil")
}
if analysis.TotalRequests != 1 {
t.Errorf("TotalRequests: got %d, want 1", analysis.TotalRequests)
}
}

func TestAnalyzeFirewallLogsSandboxEmptySquidSubdirFallsBackToTopLevel(t *testing.T) {
tmpDir := testutil.TempDir(t, "test-sandbox-empty-squid-subdir-*")

sandboxLogsDir := filepath.Join(tmpDir, "sandbox", "firewall", "logs")
if err := os.MkdirAll(filepath.Join(sandboxLogsDir, "squid-logs"), 0755); err != nil {
t.Fatalf("Failed to create sandbox/firewall/logs directories: %v", err)
}

logContent := `1761332530.474 172.30.0.20:35288 api.github.com:443 140.82.112.5:443 1.1 CONNECT 200 TCP_TUNNEL:HIER_DIRECT api.github.com:443 "-"
`
if err := os.WriteFile(filepath.Join(sandboxLogsDir, "access.log"), []byte(logContent), 0644); err != nil {
t.Fatalf("Failed to write access.log: %v", err)
}

analysis, err := analyzeFirewallLogs(tmpDir, false)
if err != nil {
t.Fatalf("analyzeFirewallLogs failed: %v", err)
}
if analysis == nil {
t.Fatal("Expected firewall analysis but got nil")
}
if analysis.TotalRequests != 1 {
t.Errorf("TotalRequests: got %d, want 1", analysis.TotalRequests)
}
}

func TestFirewallAnalysisAddMetricsDeduplicatesDomains(t *testing.T) {
base := &FirewallAnalysis{
TotalRequests: 1,
Expand Down
23 changes: 23 additions & 0 deletions pkg/cli/logs_firewall_parse_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -26,3 +26,26 @@ func TestParseFirewallLogsNoLogs(t *testing.T) {
t.Errorf("firewall.md should not be created when no logs are present")
}
}

func TestFindFirewallLogsDirSandboxFallbackToTopLevel(t *testing.T) {
tempDir := testutil.TempDir(t, "test-firewall-parse-*")

sandboxLogsDir := filepath.Join(tempDir, "sandbox", "firewall", "logs")
if err := os.MkdirAll(filepath.Join(sandboxLogsDir, "squid-logs"), 0755); err != nil {
t.Fatalf("Failed to create firewall log directories: %v", err)
}

logContent := `1761332531.123 172.30.0.20:35289 blocked.example.com:443 140.82.112.23:443 1.1 CONNECT 403 NONE_NONE:HIER_NONE blocked.example.com:443 "-"
`
if err := os.WriteFile(filepath.Join(sandboxLogsDir, "access.log"), []byte(logContent), 0644); err != nil {
t.Fatalf("Failed to write access.log: %v", err)
}

logsDir, err := findFirewallLogsDir(tempDir)
if err != nil {
t.Fatalf("findFirewallLogsDir failed: %v", err)
}
if logsDir != sandboxLogsDir {
t.Fatalf("findFirewallLogsDir returned %q, want %q", logsDir, sandboxLogsDir)
}
}
71 changes: 56 additions & 15 deletions pkg/cli/logs_parsing_firewall.go
Original file line number Diff line number Diff line change
Expand Up @@ -40,21 +40,13 @@ func parseFirewallLogs(runDir string, verbose bool) error {
}

// Check if squid logs directory exists in the run directory
// The logs could be in workflow-logs subdirectory or directly in the run directory
squidLogsDir := filepath.Join(runDir, "squid-logs")

// Also check for squid logs in workflow-logs directory
workflowLogsSquidDir := filepath.Join(runDir, "workflow-logs", "squid-logs")

// Determine which directory to use
var logsDir string
if fileutil.DirExists(squidLogsDir) {
logsDir = squidLogsDir
logsParsingFirewallLog.Printf("Found firewall logs in squid-logs directory")
} else if fileutil.DirExists(workflowLogsSquidDir) {
logsDir = workflowLogsSquidDir
logsParsingFirewallLog.Printf("Found firewall logs in workflow-logs/squid-logs directory")
} else {
// The logs could be in several locations depending on the AWF version and artifact structure.

logsDir, err := findFirewallLogsDir(runDir)
if err != nil {
return err
}
if logsDir == "" {
logsParsingFirewallLog.Print("No firewall logs found, skipping parsing")
// No firewall logs found - this is not an error, just skip parsing
if verbose {
Expand Down Expand Up @@ -239,3 +231,52 @@ originalMain();

return nil
}

func dirHasMatchingFiles(dir string, globPattern string) (bool, error) {
if !fileutil.DirExists(dir) {
return false, nil
}

files, err := filepath.Glob(filepath.Join(dir, globPattern))
if err != nil {
return false, fmt.Errorf("failed to find firewall log files in %s: %w", dir, err)
}

return len(files) > 0, nil
}

func findFirewallLogsDir(runDir string) (string, error) {
for _, candidate := range []struct {
path string
description string
}{
{
path: filepath.Join(runDir, "sandbox", "firewall", "logs", "squid-logs"),
description: "sandbox/firewall/logs/squid-logs",
},
{
path: filepath.Join(runDir, "sandbox", "firewall", "logs"),
description: "sandbox/firewall/logs",
},
{
path: filepath.Join(runDir, "squid-logs"),
description: "squid-logs",
},
{
path: filepath.Join(runDir, "workflow-logs", "squid-logs"),
description: "workflow-logs/squid-logs",
},
} {
hasLogs, err := dirHasMatchingFiles(candidate.path, "*.log")
if err != nil {
return "", err
}
if !hasLogs {
continue
}
logsParsingFirewallLog.Printf("Found firewall logs in %s directory", candidate.description)
return candidate.path, nil
}

return "", nil
}
Loading