Summary
pkg/workflow/engine_firewall_support.go contains two functions that both refer to the firewall logs directory, but they disagree on whether to use the constant. Result: any future change to constants.AWFProxyLogsDir will silently miss one of the two sites, generating divergent YAML output.
This is a sergo-tracked finding (Run 10, strategy constants-adoption-audit-plus-context-propagation-scan, ref #aw_sgar2). Tracking issue auto-expires after 7d.
Location
| File |
Line |
Code |
pkg/workflow/engine_firewall_support.go |
104 |
firewallLogsDir := "/tmp/gh-aw/sandbox/firewall/logs/" |
pkg/workflow/engine_firewall_support.go |
123 |
firewallLogsDir := constants.AWFProxyLogsDir |
The constant (pkg/constants/constants.go:143) is "/tmp/gh-aw/sandbox/firewall/logs" — note that the literal at line 104 also adds a trailing slash that the constant does not include.
Why this is a real bug, not just style
Both functions emit YAML steps that reference the firewall logs path:
generateSquidLogsUploadStep (line 104) writes the path into the path: field of an actions/upload-artifact step.
generateFirewallLogParsingStep (line 123) writes the path into the AWF_LOGS_DIR env var of a run: step.
If the firewall logs directory ever moves (e.g. for sandbox path consolidation), only line 123 gets updated by changing the constant — line 104 keeps writing the old path into the generated workflow, and the upload step silently uploads an empty/missing directory.
Recommendation
Replace the literal at line 104 with the constant. Centralize the trailing-slash convention either by appending in the use site or by defining a separate AWFProxyLogsDirSlash constant.
Before (engine_firewall_support.go:100-104):
func generateSquidLogsUploadStep(workflowName string) GitHubActionStep {
sanitizedName := strings.ToLower(SanitizeWorkflowName(workflowName))
artifactName := "firewall-logs-" + sanitizedName
// Firewall logs are now at a known location in the sandbox folder structure
firewallLogsDir := "/tmp/gh-aw/sandbox/firewall/logs/"
After:
func generateSquidLogsUploadStep(workflowName string) GitHubActionStep {
sanitizedName := strings.ToLower(SanitizeWorkflowName(workflowName))
artifactName := "firewall-logs-" + sanitizedName
// Firewall logs are now at a known location in the sandbox folder structure
firewallLogsDir := constants.AWFProxyLogsDir + "/"
Related minor finding (not requiring a separate issue)
pkg/cli/firewall_policy.go:459 constructs the same audit path via filepath.Join(agentDir, "tmp", "gh-aw", "sandbox", "firewall", "audit") plus agentBase+"/tmp/gh-aw/sandbox/firewall/audit". Defensible because the join form is needed for OS-portable path construction inside an artifact directory, but worth a comment cross-referencing constants.AWFAuditDir.
Validation
Impact / Severity
Medium. No current bug — the literal happens to match the constant. But the file is a textbook example of the helper_underadoption_failure_mode pattern flagged in sergo-stats established_patterns, and the divergence will eventually bite during a sandbox-path migration. Cheap fix (one-line edit), high prevention value.
Related historical findings
- Run 8 (#aw_sg8a1): file-mode octal literals — fixed by introducing
FilePerm{Sensitive,Public} and DirPerm{Sensitive,Public} at pkg/constants/constants.go:73-91
- Run 9 (#aw_sg9a1):
.github/workflows literal at 27 sites; GetWorkflowDir() env-var override (now appears fixed at constants.go:339-341)
- Pattern: helper or constant exists in
pkg/constants but call sites bypass it via literal duplication. Auditing Default*/Get* helpers for adoption percentage each run
Filed by Sergo run §25901108807.
Generated by 🤖 Sergo - Serena Go Expert · ● 19.1M · ◷
Summary
pkg/workflow/engine_firewall_support.gocontains two functions that both refer to the firewall logs directory, but they disagree on whether to use the constant. Result: any future change toconstants.AWFProxyLogsDirwill silently miss one of the two sites, generating divergent YAML output.This is a
sergo-tracked finding (Run 10, strategyconstants-adoption-audit-plus-context-propagation-scan, ref #aw_sgar2). Tracking issue auto-expires after 7d.Location
pkg/workflow/engine_firewall_support.gofirewallLogsDir := "/tmp/gh-aw/sandbox/firewall/logs/"pkg/workflow/engine_firewall_support.gofirewallLogsDir := constants.AWFProxyLogsDirThe constant (
pkg/constants/constants.go:143) is"/tmp/gh-aw/sandbox/firewall/logs"— note that the literal at line 104 also adds a trailing slash that the constant does not include.Why this is a real bug, not just style
Both functions emit YAML steps that reference the firewall logs path:
generateSquidLogsUploadStep(line 104) writes the path into thepath:field of anactions/upload-artifactstep.generateFirewallLogParsingStep(line 123) writes the path into theAWF_LOGS_DIRenv var of arun:step.If the firewall logs directory ever moves (e.g. for sandbox path consolidation), only line 123 gets updated by changing the constant — line 104 keeps writing the old path into the generated workflow, and the upload step silently uploads an empty/missing directory.
Recommendation
Replace the literal at line 104 with the constant. Centralize the trailing-slash convention either by appending in the use site or by defining a separate
AWFProxyLogsDirSlashconstant.Before (
engine_firewall_support.go:100-104):After:
Related minor finding (not requiring a separate issue)
pkg/cli/firewall_policy.go:459constructs the same audit path viafilepath.Join(agentDir, "tmp", "gh-aw", "sandbox", "firewall", "audit")plusagentBase+"/tmp/gh-aw/sandbox/firewall/audit". Defensible because the join form is needed for OS-portable path construction inside an artifact directory, but worth a comment cross-referencingconstants.AWFAuditDir.Validation
firewallLogsDiringenerateSquidLogsUploadStepusesconstants.AWFProxyLogsDir/tmp/gh-aw/sandbox/firewall/logsoutsidepkg/constants/Impact / Severity
Medium. No current bug — the literal happens to match the constant. But the file is a textbook example of the
helper_underadoption_failure_modepattern flagged in sergo-statsestablished_patterns, and the divergence will eventually bite during a sandbox-path migration. Cheap fix (one-line edit), high prevention value.Related historical findings
FilePerm{Sensitive,Public}andDirPerm{Sensitive,Public}atpkg/constants/constants.go:73-91.github/workflowsliteral at 27 sites;GetWorkflowDir()env-var override (now appears fixed atconstants.go:339-341)pkg/constantsbut call sites bypass it via literal duplication. AuditingDefault*/Get*helpers for adoption percentage each runFiled by Sergo run §25901108807.