Skip to content
Merged
Show file tree
Hide file tree
Changes from 3 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
9 changes: 5 additions & 4 deletions pkg/workflow/awf_helpers.go
Original file line number Diff line number Diff line change
Expand Up @@ -222,15 +222,16 @@ 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 single-quotes, safely handling wildcards like
Copy link

Copilot AI Apr 11, 2026

Choose a reason for hiding this comment

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

This comment implies non-expression values are always single-quoted, but shellEscapeArg only quotes when it sees shell-special characters (e.g., it won’t quote a normal comma-separated domain list like github.com,api.github.com). Please tweak the wording to reflect that quoting is conditional (single-quote only when needed; double-quote when a ${{ }} expression is present).

Suggested change
// for GA evaluation. Otherwise it single-quotes, safely handling wildcards like
// for GA evaluation. Otherwise it escapes or quotes only when needed (typically using
// single quotes for shell-special content), which safely handles wildcards like

Copilot uses AI. Check for mistakes.
// *.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
21 changes: 21 additions & 0 deletions pkg/workflow/shell.go
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,17 @@
// shellEscapeArg escapes a single argument for safe use in shell commands
// Arguments containing special characters are wrapped in single quotes
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

Copilot AI Apr 11, 2026

Choose a reason for hiding this comment

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

The function comment says arguments with special characters are wrapped in single quotes, but shellEscapeArg now sometimes returns a double-quoted string when ${{ ... }} is present. Please update the doc comment (and any nearby comments that make the same claim) to reflect the new dual-quoting behavior so callers don’t assume single-quote escaping in all cases.

This issue also appears in the following locations of the same file:

  • line 27
  • line 50

Copilot uses AI. Check for mistakes.
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 +47,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 57 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