[q] Fix bash syntax error by escaping parentheses in Copilot shell commands #2493
Conversation
Implements Option 1 from issue #2485: Escape parentheses in shellEscapeCommandString to prevent bash from interpreting them as subshell syntax when commands are wrapped in double quotes. The issue occurred when tool names like 'shell(cat)' were wrapped in single quotes, then the entire command was wrapped in double quotes. Bash treats single quotes as literal characters inside double quotes, leaving parentheses unprotected. Changes: - Added parentheses escaping to shellEscapeCommandString function - Updated tests to expect escaped parentheses in command strings Fixes #2485
|
@copilot backticks around |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| // Escape parentheses (to prevent subshell interpretation inside double quotes) | ||
| // BUT preserve command substitution syntax: \$(...) should remain as \$(...) | ||
| // We need to escape ( and ) except when they immediately follow \$ (which was $ before escaping) | ||
| result := make([]byte, 0, len(escaped)*2) |
Check failure
Code scanning / CodeQL
Size computation for allocation may overflow High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 8 days ago
The best solution is to introduce a length check in shellEscapeCommandString before making the allocation for result. We should defensively check that len(escaped) is below a reasonable safe maximum (e.g., 64MB, which is standard for preventing overflow like in the CodeQL example), and fail gracefully if it exceeds that value. The check should return an empty quoted shell string or some visually obvious error string if the length is excessive, or, better, panic with a clear, descriptive error (or perhaps log and return a safe error). Since only pkg/workflow/shell.go is shown and permitted for editing, these checks must be implemented there.
Specifically:
- Edit
shellEscapeCommandStringinpkg/workflow/shell.goto check thatlen(escaped)is below a threshold (e.g., 6410241024); otherwise, return an error string. - Optionally, you could also log the error to aid future diagnostics, but as logging facilities may not be imported here, prefer minimal intervention.
- You might define a
constfor the threshold for clarity and future maintainability.
| @@ -44,6 +44,7 @@ | ||
| // Special case: Parentheses immediately following $ (i.e., $(...)) are NOT escaped | ||
| // to preserve command substitution syntax. | ||
| func shellEscapeCommandString(cmd string) string { | ||
| const MaxCmdLength = 64 * 1024 * 1024 // 64MB safeguard | ||
| // Escape backslashes first (must be done before other escapes) | ||
| escaped := strings.ReplaceAll(cmd, "\\", "\\\\") | ||
| // Escape double quotes | ||
| @@ -56,6 +57,10 @@ | ||
| // Escape parentheses (to prevent subshell interpretation inside double quotes) | ||
| // BUT preserve command substitution syntax: \$(...) should remain as \$(...) | ||
| // We need to escape ( and ) except when they immediately follow \$ (which was $ before escaping) | ||
| if len(escaped) > MaxCmdLength { | ||
| // Defensive fail: do not attempt to allocate, return a safe shell string | ||
| return "\"\"" // Or optionally return an error string | ||
| } | ||
| result := make([]byte, 0, len(escaped)*2) | ||
| for i := 0; i < len(escaped); i++ { | ||
| ch := escaped[i] |
🔍 Smoke Test Investigation - Run #7SummaryNEW ISSUE DISCOVERED: The fix in this PR introduces backslash escaping ( Failure Details
Root Cause AnalysisThe original issue was a bash syntax error when parentheses in tool names like This PR attempted to fix it by escaping parentheses in the shell escape function, resulting in: However, this creates a new problem: The Copilot CLI's rule parser validates tool permission formats using regex pattern: When the CLI receives Key Finding: The issue occurs at line 265 in the agent stdio log: --allow-tool 'shell\(cat\)' --allow-tool 'shell\(date\)' ...The backslashes are being passed through to Copilot CLI as literal characters, not consumed by bash as escape sequences. Failed Jobs and ErrorsAgent Job: Failed in 48 seconds
Downstream Jobs: All skipped (detection, create_issue, missing_tool) Investigation FindingsThis failure reveals a critical incompatibility between three layers:
The Problem: When you use single quotes in bash, backslashes are literal characters, not escape sequences. So:
Comparison with Original Issue
Recommended ActionsThe escaping approach needs refinement. Here are the options: Option 1: Double Quoting Strategy (Recommended)Use double quotes with proper escaping in the workflow compiler: --allow-tool "shell(cat)" # Double quotes protect parentheses from bashPros:
Cons:
Option 2: Conditional Escaping for Firewall WrapperOnly escape when wrapping in AWF's docker command (which uses double quotes), but not in single-quoted contexts: # For AWF wrapper (double-quoted context):
"... --allow-tool 'shell\\(cat\\)' ..."
# For direct execution (single-quoted context):
... --allow-tool 'shell(cat)' ...Pros:
Cons:
Option 3: Fix AWF Entrypoint ParsingModify the AWF container's entrypoint to handle the nested quoting correctly so parentheses don't trigger subshell interpretation. Pros:
Cons:
Prevention Strategies
Historical ContextThis is the 3rd iteration of this issue:
Pattern ID: Next Steps
Investigation completed at: 2025-10-26T04:30:42Z
|
|
@Mossaka might need to do changes in awf to solve this. |
Fix: Bash Syntax Error in Copilot Workflows (Issue #2485)
Problem
Copilot smoke tests were failing with bash syntax errors when executing workflows with shell tool permissions:
The workflow compiler generated commands like:
"npx `@github/copilot` --allow-tool 'shell(cat)' --allow-tool 'shell(date)' ..."When bash executes this, the single quotes inside double quotes are treated as literal quote characters (not delimiters), leaving the parentheses unprotected. Bash then interprets
(cat)as subshell syntax, causing a syntax error.Solution (Option 1 from #2485)
Escape parentheses in
shellEscapeCommandStringfunction by adding:Now the generated command becomes:
"npx `@github/copilot` --allow-tool 'shell\\(cat\\)' --allow-tool 'shell\\(date\\)' ..."The escaped parentheses
\(and\)are treated as literal characters by bash, preventing subshell interpretation.Changes Made
Modified Files
pkg/workflow/shell.go
shellEscapeCommandStringfunctionpkg/workflow/shell_test.go
Testing
All unit tests updated to expect the new escaping behavior. The fix affects only the
shellEscapeCommandStringfunction, which is specifically used when wrapping commands in double quotes for programs like AWF.Impact
Related
Fixes #2485COPILOT_BASH_SHELL_SYNTAX_PARENTHESESImplementation: Option 1 - Escape parentheses in compiler (as recommended in #2485)