Skip to content
Merged
Show file tree
Hide file tree
Changes from 4 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
10 changes: 6 additions & 4 deletions pkg/workflow/awf_helpers.go
Original file line number Diff line number Diff line change
Expand Up @@ -222,15 +222,17 @@ func BuildAWFArgs(config AWFCommandConfig) []string {
awfHelpersLog.Printf("Added %d custom mounts from agent config", len(sortedMounts))
}

// Add allowed domains. Pass the raw value so shellEscapeArg (via shellJoinArgs)
// single-quotes it, which safely handles wildcards like *.domain.com without
// shell glob expansion and without adding literal double-quote characters.
// Add allowed domains. When the value contains ${{ }} GitHub Actions expressions,
// shellEscapeArg (via shellJoinArgs) double-quotes it so the expression is preserved
// for GA evaluation. Otherwise it escapes or quotes only when needed (typically using
// single quotes for shell-special content), which safely handles wildcards like
// *.domain.com without shell glob expansion.
awfArgs = append(awfArgs, "--allow-domains", config.AllowedDomains)

// Add blocked domains if specified
blockedDomains := formatBlockedDomains(config.WorkflowData.NetworkPermissions)
if blockedDomains != "" {
// Same single-quoting rationale as --allow-domains above
// Same quoting rationale as --allow-domains above
awfArgs = append(awfArgs, "--block-domains", blockedDomains)
awfHelpersLog.Printf("Added blocked domains: %s", blockedDomains)
}
Expand Down
31 changes: 27 additions & 4 deletions pkg/workflow/shell.go
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,9 @@

var shellLog = logger.New("workflow:shell")

// shellJoinArgs joins command arguments with proper shell escaping
// Arguments containing special characters are wrapped in single quotes
// shellJoinArgs joins command arguments with proper shell escaping.
// Arguments containing ${{ }} GitHub Actions expressions are double-quoted;
// other arguments with special shell characters are single-quoted.
func shellJoinArgs(args []string) string {
shellLog.Printf("Joining %d shell arguments with escaping", len(args))
var escapedArgs []string
Expand All @@ -21,9 +22,21 @@
return result
}

// shellEscapeArg escapes a single argument for safe use in shell commands
// Arguments containing special characters are wrapped in single quotes
// shellEscapeArg escapes a single argument for safe use in shell commands.
// Arguments containing ${{ }} GitHub Actions expressions are double-quoted;
// other arguments with special shell characters are single-quoted.
func shellEscapeArg(arg string) string {
// If the argument contains GitHub Actions expressions (${{ }}), use double-quote
// wrapping. GitHub Actions evaluates ${{ }} at the YAML level before the shell runs,
// so single-quoting would mangle the expression syntax (e.g., 'staging' inside
// ${{ env.X == 'staging' }} becomes '\''staging'\'' which GA cannot parse).
// Double-quoting preserves the expression for GA evaluation.
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.

The double-quote wrapping for GA expressions looks correct. One edge case to consider: if the expression itself produces a value with embedded double quotes at runtime, those would break the shell command. The ReplaceAll on line 34 handles literal double quotes in the source, but runtime-evaluated values aren't covered here (that would be a separate concern).

if containsGitHubActionsExpression(arg) {
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.

The double-quote wrapping for GitHub Actions expressions is a great fix! One small note: since GitHub Actions evaluates $\{\{ }} expressions at the YAML level before the shell runs, using double quotes here correctly preserves expression syntax. The comment explaining this is very helpful for future maintainers. 👍

shellLog.Print("Argument contains GitHub Actions expression, using double-quote wrapping")
escaped := strings.ReplaceAll(arg, `"`, `\"`)
return `"` + escaped + `"`
}

// Check if the argument contains special shell characters that need escaping
if strings.ContainsAny(arg, "()[]{}*?$`\"'\\|&;<> \t\n") {
shellLog.Print("Argument contains special characters, applying escaping")
Expand All @@ -36,6 +49,16 @@
return arg
}

// containsGitHubActionsExpression checks if a string contains GitHub Actions
// expressions (${{ ... }}). It verifies that ${{ appears before }}.
func containsGitHubActionsExpression(s string) bool {
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.

The containsGitHubActionsExpression helper is well-implemented. Checking that $\{\{ appears before }} (using index-based verification rather than simple substring search) avoids false positives from standalone }} in args. Clear and correct!

openIdx := strings.Index(s, "${{")
if openIdx < 0 {
return false
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.

Good defensive check: containsGitHubActionsExpression correctly verifies $\{\{ appears before }} rather than just checking both substrings exist. This prevents false positives like }} some text $\{\{.

}
return strings.Index(s[openIdx:], "}}") >= 0

Check failure on line 59 in pkg/workflow/shell.go

View workflow job for this annotation

GitHub Actions / lint-go

S1003: should use strings.Contains(s[openIdx:], "}}") instead (staticcheck)
}

// buildDockerCommandWithExpandableVars builds a properly quoted docker command
// that allows ${GITHUB_WORKSPACE} and $GITHUB_WORKSPACE to be expanded at runtime
func buildDockerCommandWithExpandableVars(cmd string) string {
Expand Down
20 changes: 20 additions & 0 deletions pkg/workflow/shell_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,21 @@ func TestShellEscapeArg(t *testing.T) {
input: "path\\to\\file",
expected: "'path\\to\\file'",
},
{
name: "GitHub Actions expression uses double quotes",
input: "${{ env.MCP_ENV == 'staging' && env.MCP_URL_STAGING || env.MCP_URL_PROD }},errors.code.visualstudio.com",
expected: `"${{ env.MCP_ENV == 'staging' && env.MCP_URL_STAGING || env.MCP_URL_PROD }},errors.code.visualstudio.com"`,
},
{
name: "simple GitHub Actions expression uses double quotes",
input: "${{ env.DOMAINS }}",
expected: `"${{ env.DOMAINS }}"`,
},
{
name: "GitHub Actions expression with embedded double quotes escapes them",
input: `${{ env.X == "test" && env.Y || env.Z }}`,
expected: `"${{ env.X == \"test\" && env.Y || env.Z }}"`,
},
}

for _, tt := range tests {
Expand Down Expand Up @@ -131,6 +146,11 @@ func TestShellJoinArgs(t *testing.T) {
input: []string{"copilot", "--add-dir", "/tmp/gh-aw/", "--prompt", "\"$INSTRUCTION\""},
expected: "copilot --add-dir /tmp/gh-aw/ --prompt '\"$INSTRUCTION\"'",
},
{
name: "allow-domains with GitHub Actions expression uses double quotes",
input: []string{"--allow-domains", "${{ env.MCP_ENV == 'staging' && env.MCP_URL_STAGING || env.MCP_URL_PROD }},errors.code.visualstudio.com"},
expected: `--allow-domains "${{ env.MCP_ENV == 'staging' && env.MCP_URL_STAGING || env.MCP_URL_PROD }},errors.code.visualstudio.com"`,
},
}

for _, tt := range tests {
Expand Down
Loading