Skip to content

Commit 0c41f8a

Browse files
CopilotMossaka
andcommitted
Address code review: improve security of path handling
Co-authored-by: Mossaka <5447827+Mossaka@users.noreply.github.com>
1 parent 7b8c792 commit 0c41f8a

File tree

2 files changed

+26
-9
lines changed

2 files changed

+26
-9
lines changed

pkg/workflow/copilot_engine.go

Lines changed: 16 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@ package workflow
22

33
import (
44
"fmt"
5+
"path/filepath"
56
"sort"
67
"strings"
78

@@ -876,17 +877,22 @@ func generateAWFInstallationStep(version string) GitHubActionStep {
876877
}
877878

878879
// resolveAWFPath handles path resolution for absolute and relative paths
880+
// It returns the path to use for the AWF binary, resolving relative paths against GITHUB_WORKSPACE
879881
func resolveAWFPath(customPath string) string {
880882
if customPath == "" {
881883
return "/usr/local/bin/awf"
882884
}
883885

884-
if strings.HasPrefix(customPath, "/") {
885-
return customPath // Absolute path
886+
// Clean the path to normalize separators and remove redundant elements
887+
// Note: filepath.Clean is used for normalization only, the path is not resolved against the filesystem
888+
cleanPath := filepath.Clean(customPath)
889+
890+
if strings.HasPrefix(cleanPath, "/") {
891+
return cleanPath // Absolute path
886892
}
887893

888894
// Relative path - resolve against GITHUB_WORKSPACE
889-
return fmt.Sprintf("${GITHUB_WORKSPACE}/%s", customPath)
895+
return fmt.Sprintf("${GITHUB_WORKSPACE}/%s", cleanPath)
890896
}
891897

892898
// getAWFBinaryPath returns appropriate AWF binary path for execution
@@ -900,20 +906,21 @@ func getAWFBinaryPath(firewallConfig *FirewallConfig) string {
900906
// generateAWFPathValidationStep creates a validation step to verify custom AWF binary
901907
func generateAWFPathValidationStep(customPath string) GitHubActionStep {
902908
resolvedPath := resolveAWFPath(customPath)
909+
escapedPath := shellEscapeArg(resolvedPath)
903910

904911
stepLines := []string{
905912
" - name: Validate custom AWF binary",
906913
" run: |",
907-
fmt.Sprintf(" echo \"Validating custom AWF binary at: %s\"", resolvedPath),
908-
fmt.Sprintf(" if [ ! -f %s ]; then", shellEscapeArg(resolvedPath)),
909-
fmt.Sprintf(" echo \"Error: AWF binary not found at %s\"", resolvedPath),
914+
fmt.Sprintf(" echo \"Validating custom AWF binary at: %s\"", escapedPath),
915+
fmt.Sprintf(" if [ ! -f %s ]; then", escapedPath),
916+
fmt.Sprintf(" echo \"Error: AWF binary not found at %s\"", escapedPath),
910917
" exit 1",
911918
" fi",
912-
fmt.Sprintf(" if [ ! -x %s ]; then", shellEscapeArg(resolvedPath)),
913-
fmt.Sprintf(" echo \"Error: AWF binary at %s is not executable\"", resolvedPath),
919+
fmt.Sprintf(" if [ ! -x %s ]; then", escapedPath),
920+
fmt.Sprintf(" echo \"Error: AWF binary at %s is not executable\"", escapedPath),
914921
" exit 1",
915922
" fi",
916-
fmt.Sprintf(" %s --version", shellEscapeArg(resolvedPath)),
923+
fmt.Sprintf(" %s --version", escapedPath),
917924
}
918925

919926
return GitHubActionStep(stepLines)

pkg/workflow/firewall_custom_path_test.go

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -41,6 +41,16 @@ func TestResolveAWFPath(t *testing.T) {
4141
customPath: "tools/bin/awf",
4242
expectedPath: "${GITHUB_WORKSPACE}/tools/bin/awf",
4343
},
44+
{
45+
name: "path with redundant separators cleaned",
46+
customPath: "bin//awf",
47+
expectedPath: "${GITHUB_WORKSPACE}/bin/awf",
48+
},
49+
{
50+
name: "path with dot components cleaned",
51+
customPath: "./bin/awf",
52+
expectedPath: "${GITHUB_WORKSPACE}/bin/awf",
53+
},
4454
}
4555

4656
for _, tt := range tests {

0 commit comments

Comments
 (0)