From 8c600d8ee40edaec797d01ac5304f309f8129d19 Mon Sep 17 00:00:00 2001 From: pelikhan Date: Mon, 19 Oct 2026 22:30:28 -0700 Subject: [PATCH 1/6] Fix SDK shell permission parsing for comments Skip shell comments without interpreting their quotes or operators, preserve literal hashes and parameter-expansion contents, and keep checking every executable command against the existing grants. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../fix-sdk-shell-comment-permissions.md | 5 +++ actions/setup/js/bash_command_parser.cjs | 37 +++++++++++++++++++ actions/setup/js/bash_command_parser.test.cjs | 33 +++++++++++++++++ actions/setup/js/copilot_sdk_driver.test.cjs | 28 ++++++++++++++ 4 files changed, 103 insertions(+) create mode 100644 .changeset/fix-sdk-shell-comment-permissions.md diff --git a/.changeset/fix-sdk-shell-comment-permissions.md b/.changeset/fix-sdk-shell-comment-permissions.md new file mode 100644 index 00000000000..0a0cfd00781 --- /dev/null +++ b/.changeset/fix-sdk-shell-comment-permissions.md @@ -0,0 +1,5 @@ +--- +"gh-aw": patch +--- + +Fix false Copilot SDK shell permission denials for commands containing comments while preserving literal hashes and checking every executable command against the workflow allowlist. diff --git a/actions/setup/js/bash_command_parser.cjs b/actions/setup/js/bash_command_parser.cjs index bcc2b37cc4f..9f39848ef04 100644 --- a/actions/setup/js/bash_command_parser.cjs +++ b/actions/setup/js/bash_command_parser.cjs @@ -31,6 +31,7 @@ * The split respects: * - Single-quoted strings (no escaping inside) * - Double-quoted strings (backslash-escape aware) + * - Unquoted backslash escapes and shell comments * - $(...) subshell expressions (balanced parentheses) * * Operators embedded inside any of these constructs are not treated as separators. @@ -43,14 +44,42 @@ function splitOnPipelineOperators(commandText) { const segments = []; let current = ""; + let atWordStart = true; + let parameterDepth = 0; let i = 0; const len = commandText.length; while (i < len) { const ch = commandText[i]; + if (ch === "#" && atWordStart && parameterDepth === 0) { + while (i < len && commandText[i] !== "\n" && commandText[i] !== "\r") i++; + continue; + } + + if (ch === "\\" && i + 1 < len) { + current += ch; + i++; + if (commandText[i] !== "\n" && commandText[i] !== "\r") { + current += commandText[i]; + i++; + atWordStart = false; + } + continue; + } + + if (ch === "$" && commandText[i + 1] === "{") { + current += "${"; + parameterDepth++; + atWordStart = false; + i += 2; + continue; + } + if (ch === "}" && parameterDepth > 0) parameterDepth--; + // ── Single-quoted string: no escape sequences, copy verbatim until closing ' ── if (ch === "'") { + atWordStart = false; current += ch; i++; while (i < len && commandText[i] !== "'") { @@ -66,6 +95,7 @@ function splitOnPipelineOperators(commandText) { // ── Double-quoted string: backslash escapes are recognised ── if (ch === '"') { + atWordStart = false; current += ch; i++; while (i < len && commandText[i] !== '"') { @@ -86,6 +116,7 @@ function splitOnPipelineOperators(commandText) { // ── $(...) subshell: skip balanced parentheses as a unit ── if (ch === "$" && i + 1 < len && commandText[i + 1] === "(") { + atWordStart = false; current += ch; i++; let depth = 0; @@ -111,6 +142,7 @@ function splitOnPipelineOperators(commandText) { if (ch === "&" && i + 1 < len && commandText[i + 1] === "&") { segments.push(current); current = ""; + atWordStart = true; i += 2; while (i < len && commandText[i] !== undefined && /\s/.test(commandText[i])) i++; continue; @@ -120,6 +152,7 @@ function splitOnPipelineOperators(commandText) { if (ch === "|" && i + 1 < len && commandText[i + 1] === "|") { segments.push(current); current = ""; + atWordStart = true; i += 2; while (i < len && /\s/.test(commandText[i])) i++; continue; @@ -129,6 +162,7 @@ function splitOnPipelineOperators(commandText) { if (ch === "|") { segments.push(current); current = ""; + atWordStart = true; i++; while (i < len && /\s/.test(commandText[i])) i++; continue; @@ -138,6 +172,7 @@ function splitOnPipelineOperators(commandText) { if (ch === ";") { segments.push(current); current = ""; + atWordStart = true; i++; while (i < len && /\s/.test(commandText[i])) i++; continue; @@ -168,6 +203,7 @@ function splitOnPipelineOperators(commandText) { segments.push(current); current = ""; + atWordStart = true; i++; if (ch === "\r" && i < len && commandText[i] === "\n") { i++; @@ -177,6 +213,7 @@ function splitOnPipelineOperators(commandText) { } current += ch; + atWordStart = /[ \t&()<>]/.test(ch); i++; } diff --git a/actions/setup/js/bash_command_parser.test.cjs b/actions/setup/js/bash_command_parser.test.cjs index 1fdb6dab434..49bc5ddf49c 100644 --- a/actions/setup/js/bash_command_parser.test.cjs +++ b/actions/setup/js/bash_command_parser.test.cjs @@ -110,6 +110,35 @@ describe("splitOnPipelineOperators", () => { expect(segments).toEqual(["pwd", "ls -la", "safeoutputs --help"]); }); + it.each(["\n", "\r\n", "\r"])("ignores standalone comments with %j line endings", newline => { + const command = `# Check attribution data${newline} # Ignore "quotes, && operators; and $(subshells)${newline}cat data.json${newline}# Done`; + expect(splitOnPipelineOperators(command)).toEqual(["cat data.json"]); + }); + + it("ignores inline comments without hiding the next command", () => { + expect(splitOnPipelineOperators('echo ready # Ignore "quotes && operators; too\ncat data.json')).toEqual(["echo ready", "cat data.json"]); + }); + + it("returns no segments for a comment-only script", () => { + expect(splitOnPipelineOperators(" # Nothing to execute\n\t# Still a comment")).toEqual([]); + }); + + it.each(["echo '# literal'", 'echo "# literal"', "echo value#literal", String.raw`echo \#literal`, String.raw`echo value\ #literal`, String.raw`echo \"#literal`])("preserves literal hashes in %s", command => { + expect(splitOnPipelineOperators(`${command} && cat data.json`)).toEqual([command, "cat data.json"]); + }); + + it("recognizes a comment after an escaped backslash and unescaped whitespace", () => { + expect(splitOnPipelineOperators(String.raw`echo \\ # Ignore this && command` + "\ncat data.json")).toEqual([String.raw`echo \\`, "cat data.json"]); + }); + + it.each(["echo ${value:- # literal}", "echo ${outer:-${inner:- # literal}}", "echo ${value:-'quoted }' # literal}"])("preserves literal hashes inside parameter expansions: %s", command => { + expect(splitOnPipelineOperators(`${command} && cat data.json`)).toEqual([command, "cat data.json"]); + }); + + it("does not continue a comment ending in a backslash", () => { + expect(splitOnPipelineOperators("# Comment \\\ncat data.json")).toEqual(["cat data.json"]); + }); + it("does not split on escaped newline continuations", () => { const segments = splitOnPipelineOperators("git log \\\n --oneline \\\n --max-count=1"); expect(segments).toEqual(["git log --oneline --max-count=1"]); @@ -289,6 +318,10 @@ describe("extractCommandNamesFromPipeline", () => { expect(extractCommandNamesFromPipeline(" ")).toEqual([]); }); + it("extracts only executable commands from a commented script", () => { + expect(extractCommandNamesFromPipeline("# Check counts\njq length data.json # Count entries\n# Read the data\ncat data.json")).toEqual(["jq", "cat"]); + }); + it("handles command with $() subshell — does not split inside subshell", () => { const result = extractCommandNamesFromPipeline("cat $(ls /tmp)"); expect(result).toEqual(["cat"]); diff --git a/actions/setup/js/copilot_sdk_driver.test.cjs b/actions/setup/js/copilot_sdk_driver.test.cjs index a7be1d46a6f..1e5219dcb05 100644 --- a/actions/setup/js/copilot_sdk_driver.test.cjs +++ b/actions/setup/js/copilot_sdk_driver.test.cjs @@ -2013,6 +2013,34 @@ describe("copilot_sdk_driver.cjs", () => { expect(result).toEqual({ kind: "approve-once" }); }); + it.each([ + "\n# Query issue #68529 for follow-up references\ngithub issue read 68529 --json body,comments 2>/dev/null | jq '.body, .comments[].body' 2>/dev/null || echo \"Issue details not available via github CLI\"\n", + "\n# Check what data files exist that might have PR closing references\ncat /tmp/gh-aw/agent/community-data/closing_refs_by_issue.json | jq 'keys[] as $k | select($k | test(\"68529|65058|60858\")) | {issue: $k, prs: .[$k]}'\n", + '\n# Count entries in each tier file\necho "=== Tier counts ==="\nwc -l /tmp/gh-aw/agent/community-data/tier0_attributed.json /tmp/gh-aw/agent/community-data/tier1_attributed.json /tmp/gh-aw/agent/community-data/tier2_attributed.json\n\necho ""\necho "=== Checking Tier 1 & 2 ==="\ncat /tmp/gh-aw/agent/community-data/tier1_attributed.json\necho "---"\ncat /tmp/gh-aw/agent/community-data/tier2_attributed.json\n', + "\n# Get count of authors and total issues\njq 'length' /tmp/gh-aw/agent/community-data/attribution_by_author.json\njq '[.[].count] | add' /tmp/gh-aw/agent/community-data/attribution_by_author.json\n", + ])("allows the community-attribution command despite standalone comments: %s", async fullCommandText => { + const handler = await makePermissionHandlerViaSDK(["shell(github:*)", "shell(cat)", "shell(jq)", "shell(echo)", "shell(wc)"]); + expect(handler({ kind: "shell", commands: [], fullCommandText })).toEqual({ kind: "approve-once" }); + }); + + it.each([ + "# An allowed read follows\ncat data.json\n# Python is not allowed\npython3 -c 'print(1)'", + "cat data.json # Ignore \"quotes && operators; only on this line\npython3 -c 'print(1)'", + "cat ${value:- # literal} && python3 -c 'print(1)'", + "cat ${outer:-${inner:- # literal}} && python3 -c 'print(1)'", + String.raw`cat \#literal && python3 -c 'print(1)'`, + String.raw`cat value\ #literal && python3 -c 'print(1)'`, + "# Comment-only requests remain fail-closed", + ])("does not let comments bypass shell permissions: %s", async fullCommandText => { + const handler = await makePermissionHandlerViaSDK(["shell(cat)"]); + expect(handler({ kind: "shell", commands: [], fullCommandText })).toEqual({ kind: "reject", feedback: "Tool invocation is not allowed by workflow tool permissions." }); + }); + + it("still denies the community-attribution Python heredoc", async () => { + const handler = await makePermissionHandlerViaSDK(["shell(github:*)", "shell(cat)", "shell(jq)", "shell(echo)", "shell(wc)"]); + expect(handler({ kind: "shell", commands: [], fullCommandText: "python3 << 'PYEOF'\nimport json\nprint('summary')\nPYEOF\n" })).toEqual({ kind: "reject", feedback: "Tool invocation is not allowed by workflow tool permissions." }); + }); + it("denies a piped command when any stage is not in the allow-list", async () => { const handler = await makePermissionHandlerViaSDK(["shell(ls)", "shell(echo)"]); // cat is NOT in the allow-list From 98a02759cf788f44db4b5671d5a8dc824ee5ee2e Mon Sep 17 00:00:00 2001 From: pelikhan Date: Mon, 19 Oct 2026 22:36:49 -0700 Subject: [PATCH 2/6] Add extensive commented Bash parser tests Cover complete Bash scripts, comment placement across operators and line endings, literal hashes, quoting, escaping, parameter expansions, and fail-closed permission checks with 173 table-driven cases. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../js/bash_command_parser_comments.test.cjs | 280 ++++++++++++++++++ 1 file changed, 280 insertions(+) create mode 100644 actions/setup/js/bash_command_parser_comments.test.cjs diff --git a/actions/setup/js/bash_command_parser_comments.test.cjs b/actions/setup/js/bash_command_parser_comments.test.cjs new file mode 100644 index 00000000000..453386bf932 --- /dev/null +++ b/actions/setup/js/bash_command_parser_comments.test.cjs @@ -0,0 +1,280 @@ +// @ts-check + +import { describe, it, expect } from "vitest"; +import { createRequire } from "module"; + +const require = createRequire(import.meta.url); +const { splitOnPipelineOperators, extractCommandNamesFromPipeline } = require("./bash_command_parser.cjs"); +const { buildCopilotSDKPermissionHandler } = require("./copilot_sdk_permissions.cjs"); + +const checkPermission = buildCopilotSDKPermissionHandler({ allowedTools: ["shell(cat)", "shell(echo)", "shell(jq)", "shell(wc)"] }, () => ({ kind: "approve-once" })); + +const lineEndings = [ + { name: "LF", value: "\n" }, + { name: "CRLF", value: "\r\n" }, + { name: "CR", value: "\r" }, +]; + +const commentBodies = [ + { name: "ordinary text", text: "Read the precomputed attribution data" }, + { name: "operators", text: "ignored_tool && another_tool || third_tool | fourth_tool; fifth_tool" }, + { name: "unclosed quotes", text: `Unclosed single ' and double " quotes` }, + { name: "unclosed substitutions", text: "Ignore $(unclosed and ${unclosed and (( parentheses" }, + { name: "hashes and redirections", text: "### ignored_tool >output 2>&1 { + it.each([ + { + name: "shebang and file header", + script: "#!/usr/bin/env bash\n# Read the data\ncat data.json", + segments: ["cat data.json"], + names: ["cat"], + }, + { + name: "inline comment at EOF", + script: "cat data.json # Read only", + segments: ["cat data.json"], + names: ["cat"], + }, + { + name: "comment-only final line", + script: "cat data.json\n# Finished", + segments: ["cat data.json"], + names: ["cat"], + }, + { + name: "indented comments and blank lines", + script: "\n # Header\n\n\t# Another comment\n\tcat data.json\n\n # Footer\n", + segments: ["cat data.json"], + names: ["cat"], + }, + { + name: "empty inline comments", + script: "echo ready #\ncat data.json\t#\n#", + segments: ["echo ready", "cat data.json"], + names: ["echo", "cat"], + }, + { + name: "comment immediately after semicolon", + script: "echo ready;# Ignore this\ncat data.json", + segments: ["echo ready", "cat data.json"], + names: ["echo", "cat"], + }, + { + name: "comment immediately after pipe", + script: "cat data.json |# Ignore this\nwc -l", + segments: ["cat data.json", "wc -l"], + names: ["cat", "wc"], + }, + { + name: "comment immediately after AND", + script: "cat data.json &&# Ignore this\necho done", + segments: ["cat data.json", "echo done"], + names: ["cat", "echo"], + }, + { + name: "comment immediately after OR", + script: "cat data.json ||# Ignore this\necho missing", + segments: ["cat data.json", "echo missing"], + names: ["cat", "echo"], + }, + { + name: "commented command is not executable", + script: "# python3 -c 'print(1)'\ncat data.json\n# another_tool", + segments: ["cat data.json"], + names: ["cat"], + }, + { + name: "single-quoted hashes and operators", + script: "echo '# literal && not_a_command' # Actual comment\ncat data.json", + segments: ["echo '# literal && not_a_command'", "cat data.json"], + names: ["echo", "cat"], + }, + { + name: "double-quoted hashes and operators", + script: 'echo "# literal | not_a_command" # Actual comment\ncat data.json', + segments: ['echo "# literal | not_a_command"', "cat data.json"], + names: ["echo", "cat"], + }, + { + name: "multiline single-quoted text", + script: "echo 'first line\n# literal second line' # Actual comment\ncat data.json", + segments: ["echo 'first line\n# literal second line'", "cat data.json"], + names: ["echo", "cat"], + }, + { + name: "multiline double-quoted text", + script: 'echo "first line\n# literal second line" # Actual comment\ncat data.json', + segments: ['echo "first line\n# literal second line"', "cat data.json"], + names: ["echo", "cat"], + }, + { + name: "hash inside filename", + script: "cat report#draft.json # Actual comment\nwc -l report#draft.json", + segments: ["cat report#draft.json", "wc -l report#draft.json"], + names: ["cat", "wc"], + }, + { + name: "hash inside URL", + script: "echo https://example.invalid/page#section # Actual comment\ncat data.json", + segments: ["echo https://example.invalid/page#section", "cat data.json"], + names: ["echo", "cat"], + }, + { + name: "hash adjoining empty single quotes", + script: "echo ''#literal # Actual comment\ncat data.json", + segments: ["echo ''#literal", "cat data.json"], + names: ["echo", "cat"], + }, + { + name: "hash adjoining empty double quotes", + script: 'echo ""#literal # Actual comment\ncat data.json', + segments: ['echo ""#literal', "cat data.json"], + names: ["echo", "cat"], + }, + { + name: "escaped hash", + script: String.raw`echo \#literal # Actual comment` + "\ncat data.json", + segments: [String.raw`echo \#literal`, "cat data.json"], + names: ["echo", "cat"], + }, + { + name: "escaped space before literal hash", + script: String.raw`echo value\ #literal # Actual comment` + "\ncat data.json", + segments: [String.raw`echo value\ #literal`, "cat data.json"], + names: ["echo", "cat"], + }, + { + name: "escaped tab before literal hash", + script: "echo value\\\t#literal # Actual comment\ncat data.json", + segments: ["echo value\\\t#literal", "cat data.json"], + names: ["echo", "cat"], + }, + { + name: "escaped quote before literal hash", + script: String.raw`echo \"#literal # Actual comment` + "\ncat data.json", + segments: [String.raw`echo \"#literal`, "cat data.json"], + names: ["echo", "cat"], + }, + { + name: "escaped operator does not start a comment", + script: String.raw`echo \;#literal # Actual comment` + "\ncat data.json", + segments: [String.raw`echo \;#literal`, "cat data.json"], + names: ["echo", "cat"], + }, + { + name: "parameter length and prefix removal", + script: "echo ${#value} ${path##*/} # Actual comment\ncat data.json", + segments: ["echo ${#value} ${path##*/}", "cat data.json"], + names: ["echo", "cat"], + }, + { + name: "space and hash in parameter default", + script: "echo ${value:- # fallback} # Actual comment\ncat data.json", + segments: ["echo ${value:- # fallback}", "cat data.json"], + names: ["echo", "cat"], + }, + { + name: "nested parameter defaults", + script: "echo ${outer:-${inner:- # fallback}} # Actual comment\ncat data.json", + segments: ["echo ${outer:-${inner:- # fallback}}", "cat data.json"], + names: ["echo", "cat"], + }, + { + name: "quoted closing brace in parameter default", + script: "echo ${value:-'}' # fallback} # Actual comment\ncat data.json", + segments: ["echo ${value:-'}' # fallback}", "cat data.json"], + names: ["echo", "cat"], + }, + { + name: "quoted hash inside command substitution", + script: "echo $(printf '%s' '# literal') # Actual comment\ncat data.json", + segments: ["echo $(printf '%s' '# literal')", "cat data.json"], + names: ["echo", "cat"], + }, + { + name: "continued command before inline comment", + script: "jq \\\n '.[]' \\\n data.json # Filter entries\nwc -l data.json", + segments: ["jq '.[]' data.json", "wc -l data.json"], + names: ["jq", "wc"], + }, + { + name: "standalone comment after line continuation", + script: "echo \\\n# This ends the logical command\ncat data.json", + segments: ["echo", "cat data.json"], + names: ["echo", "cat"], + }, + { + name: "assignment with inline comment", + script: '# Configuration\nDATA=data.json # Input path\ncat "$DATA" # Read it', + segments: ["DATA=data.json", 'cat "$DATA"'], + names: ["cat"], + }, + { + name: "for loop with comments around structural keywords", + script: '# Iterate over inputs\nfor file in first.json second.json; do # Loop body\n cat "$file" # Read input\ndone # End loop\necho done', + segments: ["for file in first.json second.json", "do", 'cat "$file"', "done", "echo done"], + names: ["cat", "echo"], + }, + { + name: "conditional with comments on every clause", + script: "if # Condition follows\n grep -q community data.json # Search\nthen # Match\n cat data.json # Read\nelse # No match\n echo missing # Report\nfi # End", + segments: ["if", "grep -q community data.json", "then", "cat data.json", "else", "echo missing", "fi"], + names: ["grep", "cat", "echo"], + }, + { + name: "function declaration and invocation", + script: "function report # Define helper\n{ # Start body\n cat data.json # Read\n} # End body\nreport # Invoke", + segments: ["function report", "{", "cat data.json", "}", "report"], + names: ["cat", "report"], + }, + { + name: "attribution summary script", + script: + '#!/usr/bin/env bash\n# Use precomputed data\nDATA_DIR=/tmp/gh-aw/agent/community-data\n# Count authors\njq length "$DATA_DIR/attribution_by_author.json" # Count\n# Summarize issues\njq \'[.[].count] | add\' "$DATA_DIR/attribution_by_author.json" # Sum\necho done # Finished', + segments: ["DATA_DIR=/tmp/gh-aw/agent/community-data", 'jq length "$DATA_DIR/attribution_by_author.json"', "jq '[.[].count] | add' \"$DATA_DIR/attribution_by_author.json\"", "echo done"], + names: ["jq", "echo"], + }, + ])("parses $name", ({ script, segments, names }) => { + expect(splitOnPipelineOperators(script)).toEqual(segments); + expect(extractCommandNamesFromPipeline(script)).toEqual(names); + }); +}); + +describe.each(lineEndings)("comment placement with $name line endings", ({ value: newline }) => { + for (const separator of ["\n", ";", "&&", "||", "|"]) { + it.each(commentBodies)(`ignores $name around ${JSON.stringify(separator)} separators`, ({ text }) => { + const operator = separator === "\n" ? newline : separator; + const script = [`# ${text}`, `cat data.json ${operator} # ${text}`, `jq length ${operator} # ${text}`, `wc -l # ${text}`, `# ${text}`].join(newline); + expect(splitOnPipelineOperators(script)).toEqual(["cat data.json", "jq length", "wc -l"]); + expect(extractCommandNamesFromPipeline(script)).toEqual(["cat", "jq", "wc"]); + expect(checkPermission({ kind: "shell", fullCommandText: script })).toEqual({ kind: "approve-once" }); + }); + } + + it.each(["#", "# Header", " # Indented", "\t# Tabbed", "#!/usr/bin/env bash", '# Unclosed "quotes && $(subshells'])("keeps comment-only requests fail-closed: %s", comment => { + const script = `${comment}${newline}${comment}`; + expect(splitOnPipelineOperators(script)).toEqual([]); + expect(extractCommandNamesFromPipeline(script)).toEqual([]); + expect(checkPermission({ kind: "shell", fullCommandText: script }).kind).toBe("reject"); + }); + + it.each([ + { name: "standalone comments", prefix: "# Allowed read\ncat data.json\n# Next command is not allowed\n" }, + { name: "inline comments with unmatched quotes", prefix: 'cat data.json # Ignore this "quote and && operator\n' }, + { name: "comment ending in backslash", prefix: "cat data.json # This comment does not continue \\\n" }, + { name: "quoted hash", prefix: "echo '# literal' && " }, + { name: "multiline quoted hash", prefix: 'echo "first line\n# literal" && ' }, + { name: "escaped hash", prefix: String.raw`echo \#literal && ` }, + { name: "escaped space", prefix: String.raw`echo value\ #literal && ` }, + { name: "escaped quote", prefix: String.raw`echo \"#literal && ` }, + { name: "parameter default", prefix: "echo ${value:- # literal} && " }, + { name: "nested parameter defaults", prefix: "echo ${outer:-${inner:- # literal}} && " }, + ])("cannot conceal a disallowed command with $name", ({ prefix }) => { + const script = `${prefix}python3 -c 'print(1)' # Final comment`.replace(/\n/g, newline); + expect(extractCommandNamesFromPipeline(script)).toContain("python3"); + expect(checkPermission({ kind: "shell", fullCommandText: script }).kind).toBe("reject"); + }); +}); From c5e2025e3d6f83cd95049629ade303ac4dadb8e0 Mon Sep 17 00:00:00 2001 From: pelikhan Date: Mon, 19 Oct 2026 22:53:14 -0700 Subject: [PATCH 3/6] Harden shell comment parsing against quote confusion Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../fix-sdk-shell-comment-permissions.md | 2 + actions/setup/js/bash_command_parser.cjs | 95 ++++++++++++------- actions/setup/js/bash_command_parser.test.cjs | 2 +- .../js/bash_command_parser_comments.test.cjs | 48 +++++++++- 4 files changed, 110 insertions(+), 37 deletions(-) diff --git a/.changeset/fix-sdk-shell-comment-permissions.md b/.changeset/fix-sdk-shell-comment-permissions.md index 0a0cfd00781..35d724145fa 100644 --- a/.changeset/fix-sdk-shell-comment-permissions.md +++ b/.changeset/fix-sdk-shell-comment-permissions.md @@ -3,3 +3,5 @@ --- Fix false Copilot SDK shell permission denials for commands containing comments while preserving literal hashes and checking every executable command against the workflow allowlist. + +Keep ambiguous nested shell quoting fail-closed and terminate comments only at line feeds so quote confusion and carriage returns cannot hide disallowed commands. diff --git a/actions/setup/js/bash_command_parser.cjs b/actions/setup/js/bash_command_parser.cjs index 9f39848ef04..e2f2f8fb3b2 100644 --- a/actions/setup/js/bash_command_parser.cjs +++ b/actions/setup/js/bash_command_parser.cjs @@ -24,6 +24,41 @@ * the request, ensuring a safe default. */ +/** + * Find the end of a quoted word, rejecting nested syntax this parser cannot + * safely distinguish from the surrounding quote. + * + * @param {string} commandText + * @param {number} start + * @returns {number} Exclusive end offset, or -1 for unsupported/unclosed quoting. + */ +function findShellQuoteEnd(commandText, start) { + const quote = commandText[start]; + let parameterDepth = 0; + for (let i = start + 1; i < commandText.length; i++) { + const ch = commandText[i]; + if (quote === '"') { + if (ch === "\\") { + i++; + continue; + } + if (ch === "`" || (ch === "$" && commandText[i + 1] === "(")) return -1; + if (ch === "$" && commandText[i + 1] === "{") { + parameterDepth++; + i++; + continue; + } + if (ch === "}" && parameterDepth > 0) { + parameterDepth--; + continue; + } + if (parameterDepth > 0 && (ch === "'" || ch === '"')) return -1; + } + if (ch === quote) return i + 1; + } + return -1; +} + /** * Split a shell command text into individual pipeline segments. * Splits on the following shell operators: &&, ||, |, ; and newlines. @@ -31,10 +66,11 @@ * The split respects: * - Single-quoted strings (no escaping inside) * - Double-quoted strings (backslash-escape aware) - * - Unquoted backslash escapes and shell comments + * - Unquoted backslash escapes and shell comments (terminated by LF, not bare CR) * - $(...) subshell expressions (balanced parentheses) * * Operators embedded inside any of these constructs are not treated as separators. + * Unsupported nested quoting and unclosed constructs fail closed. * * @param {string} commandText - Raw bash command text that may contain pipeline operators * @returns {string[]} Non-empty trimmed segments (operators removed) @@ -53,10 +89,12 @@ function splitOnPipelineOperators(commandText) { const ch = commandText[i]; if (ch === "#" && atWordStart && parameterDepth === 0) { - while (i < len && commandText[i] !== "\n" && commandText[i] !== "\r") i++; + while (i < len && commandText[i] !== "\n") i++; continue; } + if (ch === "`" || (ch === "$" && commandText[i + 1] === "'")) return []; + if (ch === "\\" && i + 1 < len) { current += ch; i++; @@ -77,40 +115,12 @@ function splitOnPipelineOperators(commandText) { } if (ch === "}" && parameterDepth > 0) parameterDepth--; - // ── Single-quoted string: no escape sequences, copy verbatim until closing ' ── - if (ch === "'") { - atWordStart = false; - current += ch; - i++; - while (i < len && commandText[i] !== "'") { - current += commandText[i]; - i++; - } - if (i < len) { - current += commandText[i]; // closing ' - i++; - } - continue; - } - - // ── Double-quoted string: backslash escapes are recognised ── - if (ch === '"') { + if (ch === "'" || ch === '"') { + const end = findShellQuoteEnd(commandText, i); + if (end < 0) return []; atWordStart = false; - current += ch; - i++; - while (i < len && commandText[i] !== '"') { - if (commandText[i] === "\\" && i + 1 < len) { - current += commandText[i] + commandText[i + 1]; - i += 2; - } else { - current += commandText[i]; - i++; - } - } - if (i < len) { - current += commandText[i]; // closing " - i++; - } + current += commandText.slice(i, end); + i = end; continue; } @@ -122,6 +132,19 @@ function splitOnPipelineOperators(commandText) { let depth = 0; while (i < len) { const sc = commandText[i]; + if (sc === "#" || sc === "`" || (sc === "$" && commandText[i + 1] === "'")) return []; + if (sc === "\\" && i + 1 < len) { + current += sc + commandText[i + 1]; + i += 2; + continue; + } + if (sc === "'" || sc === '"') { + const end = findShellQuoteEnd(commandText, i); + if (end < 0) return []; + current += commandText.slice(i, end); + i = end; + continue; + } if (sc === "(") depth++; else if (sc === ")") { depth--; @@ -133,6 +156,7 @@ function splitOnPipelineOperators(commandText) { current += sc; i++; } + if (depth !== 0) return []; continue; } @@ -218,6 +242,7 @@ function splitOnPipelineOperators(commandText) { } // Push the final segment + if (parameterDepth !== 0) return []; if (current.trim()) { segments.push(current); } diff --git a/actions/setup/js/bash_command_parser.test.cjs b/actions/setup/js/bash_command_parser.test.cjs index 49bc5ddf49c..328c84d1510 100644 --- a/actions/setup/js/bash_command_parser.test.cjs +++ b/actions/setup/js/bash_command_parser.test.cjs @@ -110,7 +110,7 @@ describe("splitOnPipelineOperators", () => { expect(segments).toEqual(["pwd", "ls -la", "safeoutputs --help"]); }); - it.each(["\n", "\r\n", "\r"])("ignores standalone comments with %j line endings", newline => { + it.each(["\n", "\r\n"])("ignores standalone comments with %j line endings", newline => { const command = `# Check attribution data${newline} # Ignore "quotes, && operators; and $(subshells)${newline}cat data.json${newline}# Done`; expect(splitOnPipelineOperators(command)).toEqual(["cat data.json"]); }); diff --git a/actions/setup/js/bash_command_parser_comments.test.cjs b/actions/setup/js/bash_command_parser_comments.test.cjs index 453386bf932..7cf6f407289 100644 --- a/actions/setup/js/bash_command_parser_comments.test.cjs +++ b/actions/setup/js/bash_command_parser_comments.test.cjs @@ -12,7 +12,6 @@ const checkPermission = buildCopilotSDKPermissionHandler({ allowedTools: ["shell const lineEndings = [ { name: "LF", value: "\n" }, { name: "CRLF", value: "\r\n" }, - { name: "CR", value: "\r" }, ]; const commentBodies = [ @@ -278,3 +277,50 @@ describe.each(lineEndings)("comment placement with $name line endings", ({ value expect(checkPermission({ kind: "shell", fullCommandText: script }).kind).toBe("reject"); }); }); + +describe("comment parser security boundaries", () => { + it.each([ + 'echo ok "${value:-"x #"}"; forbidden_probe', + 'echo ok "${value:+"x #"}"; forbidden_probe', + 'echo ok "${outer:-${inner:-"x #"}}"; forbidden_probe', + 'echo ok "$(printf "x #")"; forbidden_probe', + 'echo ok "`printf "x #"`"; forbidden_probe', + "echo ok $(printf ')x #'); forbidden_probe", + "echo ok $'x \\' #'; forbidden_probe", + ])("cannot hide a following command using nested shell quoting: %s", fullCommandText => { + for (const grant of ["shell(echo)", "shell(echo ok:*)"]) { + const handler = buildCopilotSDKPermissionHandler({ allowedTools: [grant] }, () => ({ kind: "approve-once" })); + expect(handler({ kind: "shell", fullCommandText }).kind).toBe("reject"); + } + const segments = splitOnPipelineOperators(fullCommandText); + if (segments.length > 0) expect(extractCommandNamesFromPipeline(fullCommandText)).toContain("forbidden_probe"); + }); + + it.each(['#\recho ok "\nforbidden_probe\n#"', '# ignored\recho ok "\nforbidden_probe\n#"', 'echo ok # ignored\recho ok "\nforbidden_probe\n#"', '# ignored\r\r\recho ok "\nforbidden_probe\n#"'])( + "does not let carriage returns terminate a Bash comment: %j", + fullCommandText => { + const handler = buildCopilotSDKPermissionHandler({ allowedTools: ["shell(echo ok:*)"] }, () => ({ kind: "approve-once" })); + expect(extractCommandNamesFromPipeline(fullCommandText)).toContain("forbidden_probe"); + expect(handler({ kind: "shell", fullCommandText }).kind).toBe("reject"); + } + ); + + it.each(["# Header\rcat data.json", "# Header\r # Another comment\rcat data.json", '# Header\r"unclosed quote'])("keeps carriage returns inside comment text: %j", script => { + expect(splitOnPipelineOperators(script)).toEqual([]); + expect(extractCommandNamesFromPipeline(script)).toEqual([]); + expect(checkPermission({ kind: "shell", fullCommandText: script }).kind).toBe("reject"); + }); + + it.each(['echo "${value:-#literal}" # Actual comment\ncat data.json', 'echo "${outer:-${inner:-#literal}}" # Actual comment\ncat data.json', "echo $(printf ${value:-fallback}) # Actual comment\ncat data.json"])( + "preserves simple parameter expansions: %s", + script => { + expect(extractCommandNamesFromPipeline(script)).toEqual(["echo", "cat"]); + expect(checkPermission({ kind: "shell", fullCommandText: script }).kind).toBe("approve-once"); + } + ); + + it.each(["echo ok 'unclosed #", 'echo ok "unclosed #', "echo ok ${value:- #", "echo ok $(printf value"])("rejects unclosed contexts instead of approving a partial parse: %s", fullCommandText => { + expect(splitOnPipelineOperators(fullCommandText)).toEqual([]); + expect(checkPermission({ kind: "shell", fullCommandText }).kind).toBe("reject"); + }); +}); From c7397e57889055521d0799c25704885c46ef92c0 Mon Sep 17 00:00:00 2001 From: pelikhan Date: Mon, 19 Oct 2026 23:08:35 -0700 Subject: [PATCH 4/6] Reject unsupported glob contexts in shell permission parsing Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../fix-sdk-shell-comment-permissions.md | 2 ++ actions/setup/js/bash_command_parser.cjs | 6 ++++- .../js/bash_command_parser_comments.test.cjs | 25 +++++++++++++++++++ 3 files changed, 32 insertions(+), 1 deletion(-) diff --git a/.changeset/fix-sdk-shell-comment-permissions.md b/.changeset/fix-sdk-shell-comment-permissions.md index 35d724145fa..1aa9008dee0 100644 --- a/.changeset/fix-sdk-shell-comment-permissions.md +++ b/.changeset/fix-sdk-shell-comment-permissions.md @@ -5,3 +5,5 @@ Fix false Copilot SDK shell permission denials for commands containing comments while preserving literal hashes and checking every executable command against the workflow allowlist. Keep ambiguous nested shell quoting fail-closed and terminate comments only at line feeds so quote confusion and carriage returns cannot hide disallowed commands. + +Reject unsupported unquoted parenthesis contexts, including extended globs, so literal pattern hashes cannot conceal following commands. diff --git a/actions/setup/js/bash_command_parser.cjs b/actions/setup/js/bash_command_parser.cjs index e2f2f8fb3b2..0804cc46749 100644 --- a/actions/setup/js/bash_command_parser.cjs +++ b/actions/setup/js/bash_command_parser.cjs @@ -70,7 +70,8 @@ function findShellQuoteEnd(commandText, start) { * - $(...) subshell expressions (balanced parentheses) * * Operators embedded inside any of these constructs are not treated as separators. - * Unsupported nested quoting and unclosed constructs fail closed. + * Unsupported nested quoting, unquoted grouping/glob parentheses, and unclosed + * constructs fail closed. * * @param {string} commandText - Raw bash command text that may contain pipeline operators * @returns {string[]} Non-empty trimmed segments (operators removed) @@ -160,6 +161,9 @@ function splitOnPipelineOperators(commandText) { continue; } + // Extended globs can contain literal hashes; their parentheses need a full lexer. + if (ch === "(") return []; + // ── Pipeline operators ── // && (AND-then) diff --git a/actions/setup/js/bash_command_parser_comments.test.cjs b/actions/setup/js/bash_command_parser_comments.test.cjs index 7cf6f407289..1f8bcd70e4d 100644 --- a/actions/setup/js/bash_command_parser_comments.test.cjs +++ b/actions/setup/js/bash_command_parser_comments.test.cjs @@ -279,6 +279,31 @@ describe.each(lineEndings)("comment placement with $name line endings", ({ value }); describe("comment parser security boundaries", () => { + describe.each(["?", "*", "+", "@", "!"])("unsupported %s(...) patterns", operator => { + it.each(["", "\\\n", "\\\n\\\n"])("rejects glob operators separated from parentheses by %j", continuation => { + for (const prefix of ["", "shopt -s extglob\n"]) { + const fullCommandText = `${prefix}echo ok ${operator}${continuation}(x #); forbidden_probe`; + for (const grant of ["shell(echo)", "shell(echo ok:*)"]) { + const handler = buildCopilotSDKPermissionHandler({ allowedTools: ["shell(shopt)", grant] }, () => ({ kind: "approve-once" })); + expect(handler({ kind: "shell", fullCommandText }).kind).toBe("reject"); + } + expect(splitOnPipelineOperators(fullCommandText)).toEqual([]); + expect(extractCommandNamesFromPipeline(fullCommandText)).toEqual([]); + } + }); + }); + + it.each([ + "echo '@(x #)' # Actual comment\ncat data.json", + 'echo "!(x #)" # Actual comment\ncat data.json', + String.raw`echo @\(x\ \#\) # Actual comment` + "\ncat data.json", + "# Ignore @(x #) and $(unclosed\necho ok\ncat data.json", + "echo $(printf '%s' '@(x #)') # Actual comment\ncat data.json", + ])("preserves quoted, escaped, and commented glob syntax: %s", fullCommandText => { + expect(extractCommandNamesFromPipeline(fullCommandText)).toEqual(["echo", "cat"]); + expect(checkPermission({ kind: "shell", fullCommandText }).kind).toBe("approve-once"); + }); + it.each([ 'echo ok "${value:-"x #"}"; forbidden_probe', 'echo ok "${value:+"x #"}"; forbidden_probe', From ede93703385ed7f59a9727f49dffe74d63922ebd Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Tue, 20 Oct 2026 07:26:30 +0000 Subject: [PATCH 5/6] Harden shell parser boundaries and regressions Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com> --- actions/setup/js/bash_command_parser.cjs | 135 +++++++++++++----- .../js/bash_command_parser_comments.test.cjs | 42 ++++-- 2 files changed, 129 insertions(+), 48 deletions(-) diff --git a/actions/setup/js/bash_command_parser.cjs b/actions/setup/js/bash_command_parser.cjs index 0804cc46749..28a094d7224 100644 --- a/actions/setup/js/bash_command_parser.cjs +++ b/actions/setup/js/bash_command_parser.cjs @@ -1,6 +1,20 @@ // @ts-check "use strict"; +const SHELL_BLANK_RE = /^[ \t]$/; + +function trimShellWhitespace(value) { + return value.replace(/^(?:[ \t]|\r?\n)+|(?:[ \t]|\r?\n)+$/g, ""); +} + +function findShellExpansionOpener(commandText, dollarIndex) { + let openerIndex = dollarIndex + 1; + while (commandText[openerIndex] === "\\" && commandText[openerIndex + 1] === "\n") { + openerIndex += 2; + } + return ["{", "(", "'"].includes(commandText[openerIndex]) ? openerIndex : -1; +} + /** * bash_command_parser.cjs * @@ -42,11 +56,16 @@ function findShellQuoteEnd(commandText, start) { i++; continue; } - if (ch === "`" || (ch === "$" && commandText[i + 1] === "(")) return -1; - if (ch === "$" && commandText[i + 1] === "{") { - parameterDepth++; - i++; - continue; + if (ch === "`") return -1; + if (ch === "$") { + const openerIndex = findShellExpansionOpener(commandText, i); + if (openerIndex > i + 1) return -1; + if (openerIndex === i + 1 && (commandText[openerIndex] === "(" || commandText[openerIndex] === "'")) return -1; + if (openerIndex === i + 1 && commandText[openerIndex] === "{") { + parameterDepth++; + i = openerIndex; + continue; + } } if (ch === "}" && parameterDepth > 0) { parameterDepth--; @@ -94,14 +113,24 @@ function splitOnPipelineOperators(commandText) { continue; } - if (ch === "`" || (ch === "$" && commandText[i + 1] === "'")) return []; + if (ch === "`") return []; + + if (ch === "$") { + const openerIndex = findShellExpansionOpener(commandText, i); + if (openerIndex > i + 1) return []; + if (openerIndex === i + 1 && commandText[openerIndex] === "'") return []; + } if (ch === "\\" && i + 1 < len) { - current += ch; - i++; - if (commandText[i] !== "\n" && commandText[i] !== "\r") { - current += commandText[i]; - i++; + if (commandText[i + 1] === "\n") { + i += 2; + while (i < len && SHELL_BLANK_RE.test(commandText[i])) i++; + if (current && !/[ \t]$/.test(current)) { + current += " "; + } + } else { + current += ch + commandText[i + 1]; + i += 2; atWordStart = false; } continue; @@ -131,9 +160,21 @@ function splitOnPipelineOperators(commandText) { current += ch; i++; let depth = 0; + let subshellParameterDepth = 0; while (i < len) { const sc = commandText[i]; - if (sc === "#" || sc === "`" || (sc === "$" && commandText[i + 1] === "'")) return []; + if (sc === "#" || sc === "`") return []; + if (sc === "$") { + const openerIndex = findShellExpansionOpener(commandText, i); + if (openerIndex > i + 1) return []; + if (openerIndex === i + 1 && commandText[openerIndex] === "'") return []; + if (openerIndex === i + 1 && commandText[openerIndex] === "{") { + current += commandText.slice(i, openerIndex + 1); + subshellParameterDepth++; + i = openerIndex + 1; + continue; + } + } if (sc === "\\" && i + 1 < len) { current += sc + commandText[i + 1]; i += 2; @@ -142,10 +183,13 @@ function splitOnPipelineOperators(commandText) { if (sc === "'" || sc === '"') { const end = findShellQuoteEnd(commandText, i); if (end < 0) return []; + if (subshellParameterDepth > 0 && /[()]/.test(commandText.slice(i, end))) return []; current += commandText.slice(i, end); i = end; continue; } + if (sc === "}" && subshellParameterDepth > 0) subshellParameterDepth--; + if (subshellParameterDepth > 0 && (sc === "(" || sc === ")")) return []; if (sc === "(") depth++; else if (sc === ")") { depth--; @@ -157,13 +201,19 @@ function splitOnPipelineOperators(commandText) { current += sc; i++; } - if (depth !== 0) return []; + if (depth !== 0 || subshellParameterDepth !== 0) return []; continue; } // Extended globs can contain literal hashes; their parentheses need a full lexer. if (ch === "(") return []; + if (parameterDepth > 0) { + current += ch; + i++; + continue; + } + // ── Pipeline operators ── // && (AND-then) @@ -172,7 +222,7 @@ function splitOnPipelineOperators(commandText) { current = ""; atWordStart = true; i += 2; - while (i < len && commandText[i] !== undefined && /\s/.test(commandText[i])) i++; + while (i < len && SHELL_BLANK_RE.test(commandText[i])) i++; continue; } @@ -182,7 +232,7 @@ function splitOnPipelineOperators(commandText) { current = ""; atWordStart = true; i += 2; - while (i < len && /\s/.test(commandText[i])) i++; + while (i < len && SHELL_BLANK_RE.test(commandText[i])) i++; continue; } @@ -192,7 +242,7 @@ function splitOnPipelineOperators(commandText) { current = ""; atWordStart = true; i++; - while (i < len && /\s/.test(commandText[i])) i++; + while (i < len && SHELL_BLANK_RE.test(commandText[i])) i++; continue; } @@ -202,14 +252,29 @@ function splitOnPipelineOperators(commandText) { current = ""; atWordStart = true; i++; - while (i < len && /\s/.test(commandText[i])) i++; + while (i < len && SHELL_BLANK_RE.test(commandText[i])) i++; continue; } // Newline (sequential) — treat line breaks as command separators, // except when escaped as a shell line continuation ("\\" + newline). - // Handles LF, CRLF, and CR forms. - if (ch === "\n" || ch === "\r") { + // Handles LF and CRLF; a bare CR remains part of the shell word. + if (ch === "\r" && commandText[i + 1] === "\n") { + let backslashRunLength = 0; + for (let j = current.length - 1; j >= 0 && current[j] === "\\"; j--) { + backslashRunLength++; + } + if (backslashRunLength % 2 === 1) { + current += ch; + i++; + atWordStart = false; + } else { + i++; + } + continue; + } + + if (ch === "\n") { let backslashRunLength = 0; for (let j = current.length - 1; j >= 0 && current[j] === "\\"; j--) { backslashRunLength++; @@ -219,11 +284,8 @@ function splitOnPipelineOperators(commandText) { if (backslashRunLength % 2 === 1) { current = current.slice(0, -1); i++; - if (ch === "\r" && i < len && commandText[i] === "\n") { - i++; - } - while (i < len && (commandText[i] === " " || commandText[i] === "\t")) i++; - if (current && !/\s$/.test(current)) { + while (i < len && SHELL_BLANK_RE.test(commandText[i])) i++; + if (current && !/[ \t]$/.test(current)) { current += " "; } continue; @@ -233,25 +295,22 @@ function splitOnPipelineOperators(commandText) { current = ""; atWordStart = true; i++; - if (ch === "\r" && i < len && commandText[i] === "\n") { - i++; - } - while (i < len && /\s/.test(commandText[i])) i++; + while (i < len && SHELL_BLANK_RE.test(commandText[i])) i++; continue; } current += ch; - atWordStart = /[ \t&()<>]/.test(ch); + atWordStart = /[ \t&<>]/.test(ch); i++; } // Push the final segment if (parameterDepth !== 0) return []; - if (current.trim()) { + if (trimShellWhitespace(current)) { segments.push(current); } - return segments.map(s => s.trim()).filter(s => s.length > 0); + return segments.map(trimShellWhitespace).filter(s => s.length > 0); } /** @@ -274,7 +333,7 @@ const SHELL_KEYWORDS = new Set([...CLAUSE_KEYWORDS, ...STRUCTURE_KEYWORDS]); // - "(...)" double-quoted text (supports escapes like \") // - '(...)' single-quoted text // - an unquoted non-space token -const ENV_ASSIGNMENT_PREFIX_RE = /^[A-Za-z_][A-Za-z0-9_]*=(?:"(?:\\.|[^"\\])*"|'[^']*'|\S*)\s*/; +const ENV_ASSIGNMENT_PREFIX_RE = /^[A-Za-z_][A-Za-z0-9_]*=(?:"(?:\\.|[^"\\])*"|'[^']*'|[^ \t\n]*)[ \t]*/; /** * Extract the executable command name from a single shell command segment. @@ -294,21 +353,21 @@ const ENV_ASSIGNMENT_PREFIX_RE = /^[A-Za-z_][A-Za-z0-9_]*=(?:"(?:\\.|[^"\\])*"|' function extractCommandName(segment) { if (!segment || typeof segment !== "string") return null; - let remaining = segment.trim(); + let remaining = trimShellWhitespace(segment); if (!remaining) return null; // Skip leading env-var assignments: IDENTIFIER=anything (repeat) for (;;) { const m = remaining.match(ENV_ASSIGNMENT_PREFIX_RE); if (!m) break; - remaining = remaining.slice(m[0].length).trim(); + remaining = trimShellWhitespace(remaining.slice(m[0].length)); } if (!remaining) return null; for (;;) { // Get the first word - const wordMatch = remaining.match(/^(\S+)/); + const wordMatch = remaining.match(/^([^ \t\n]+)/); if (!wordMatch) return null; const word = wordMatch[1]; @@ -320,7 +379,7 @@ function extractCommandName(segment) { // Shell negation / grouping — skip and continue scanning if (word === "!" || word === "{" || word === "}") { - remaining = remaining.slice(word.length).trim(); + remaining = trimShellWhitespace(remaining.slice(word.length)); if (!remaining) return null; continue; } @@ -328,7 +387,7 @@ function extractCommandName(segment) { // Flow-control keywords are not executable commands if (SHELL_KEYWORDS.has(word)) { if (CLAUSE_KEYWORDS.has(word)) { - remaining = remaining.slice(word.length).trim(); + remaining = trimShellWhitespace(remaining.slice(word.length)); if (!remaining) return null; continue; } @@ -356,7 +415,7 @@ function extractCommandName(segment) { function extractCommandNamesFromPipeline(commandText) { if (!commandText || typeof commandText !== "string") return []; - const text = commandText.trim(); + const text = trimShellWhitespace(commandText); if (!text) return []; const segments = splitOnPipelineOperators(text); diff --git a/actions/setup/js/bash_command_parser_comments.test.cjs b/actions/setup/js/bash_command_parser_comments.test.cjs index 1f8bcd70e4d..c73dd3077d7 100644 --- a/actions/setup/js/bash_command_parser_comments.test.cjs +++ b/actions/setup/js/bash_command_parser_comments.test.cjs @@ -14,15 +14,6 @@ const lineEndings = [ { name: "CRLF", value: "\r\n" }, ]; -const commentBodies = [ - { name: "ordinary text", text: "Read the precomputed attribution data" }, - { name: "operators", text: "ignored_tool && another_tool || third_tool | fourth_tool; fifth_tool" }, - { name: "unclosed quotes", text: `Unclosed single ' and double " quotes` }, - { name: "unclosed substitutions", text: "Ignore $(unclosed and ${unclosed and (( parentheses" }, - { name: "hashes and redirections", text: "### ignored_tool >output 2>&1 { it.each([ { @@ -244,7 +235,8 @@ describe("commented Bash script fixtures", () => { describe.each(lineEndings)("comment placement with $name line endings", ({ value: newline }) => { for (const separator of ["\n", ";", "&&", "||", "|"]) { - it.each(commentBodies)(`ignores $name around ${JSON.stringify(separator)} separators`, ({ text }) => { + it(`ignores inert comments around ${JSON.stringify(separator)} separators`, () => { + const text = "inert comment"; const operator = separator === "\n" ? newline : separator; const script = [`# ${text}`, `cat data.json ${operator} # ${text}`, `jq length ${operator} # ${text}`, `wc -l # ${text}`, `# ${text}`].join(newline); expect(splitOnPipelineOperators(script)).toEqual(["cat data.json", "jq length", "wc -l"]); @@ -279,6 +271,35 @@ describe.each(lineEndings)("comment placement with $name line endings", ({ value }); describe("comment parser security boundaries", () => { + it.each(["echo ok ${value:-foo|bar} && cat data.json", "echo ok ${value:-foo;bar} && cat data.json"])("keeps command separators inside parameter expansions: %s", fullCommandText => { + expect(splitOnPipelineOperators(fullCommandText)).toEqual([fullCommandText.slice(0, fullCommandText.indexOf(" && ")), "cat data.json"]); + expect(extractCommandNamesFromPipeline(fullCommandText)).toEqual(["echo", "cat"]); + expect(checkPermission({ kind: "shell", fullCommandText })).toEqual({ kind: "approve-once" }); + }); + + it.each(["echo ok\r#; forbidden_probe", "echo ok\\\r#; forbidden_probe", "echo ok;\v#; forbidden_probe", "echo ok;\u00a0#; forbidden_probe"])("does not treat non-Bash whitespace as a comment boundary: %j", fullCommandText => { + expect(extractCommandNamesFromPipeline(fullCommandText)).toContain("forbidden_probe"); + for (const grant of ["shell(echo)", "shell(echo ok:*)"]) { + const handler = buildCopilotSDKPermissionHandler({ allowedTools: [grant] }, () => ({ kind: "approve-once" })); + expect(handler({ kind: "shell", fullCommandText }).kind).toBe("reject"); + } + }); + + it.each(["echo ok $\\\n{value:-x #}; forbidden_probe", "echo ok $\\\n'x #'; forbidden_probe", 'echo ok "$\\\n(echo x)"; forbidden_probe', 'echo ok "$\\\n{value:-x #}"; forbidden_probe'])( + "rejects expansion openers separated by escaped newlines: %s", + fullCommandText => { + for (const grant of ["shell(echo)", "shell(echo ok:*)"]) { + const handler = buildCopilotSDKPermissionHandler({ allowedTools: [grant] }, () => ({ kind: "approve-once" })); + expect(handler({ kind: "shell", fullCommandText }).kind).toBe("reject"); + } + } + ); + + it("does not reinterpret escaped-newline openers in comments or single quotes", () => { + const fullCommandText = "# $\\\n# {comment text\ncat data.json && echo '$\\\n{literal}'"; + expect(extractCommandNamesFromPipeline(fullCommandText)).toEqual(["cat", "echo"]); + }); + describe.each(["?", "*", "+", "@", "!"])("unsupported %s(...) patterns", operator => { it.each(["", "\\\n", "\\\n\\\n"])("rejects glob operators separated from parentheses by %j", continuation => { for (const prefix of ["", "shopt -s extglob\n"]) { @@ -311,6 +332,7 @@ describe("comment parser security boundaries", () => { 'echo ok "$(printf "x #")"; forbidden_probe', 'echo ok "`printf "x #"`"; forbidden_probe', "echo ok $(printf ')x #'); forbidden_probe", + "echo ok $(printf %s ${value:-)x #}); forbidden_probe", "echo ok $'x \\' #'; forbidden_probe", ])("cannot hide a following command using nested shell quoting: %s", fullCommandText => { for (const grant of ["shell(echo)", "shell(echo ok:*)"]) { From acc6faaf3fcbb7b86f30fbfefc5fa25c967282e8 Mon Sep 17 00:00:00 2001 From: pelikhan Date: Tue, 20 Oct 2026 06:21:18 -0700 Subject: [PATCH 6/6] Check background commands in shell permission parsing Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../fix-sdk-shell-comment-permissions.md | 2 + actions/setup/js/bash_command_parser.cjs | 27 ++++++-- actions/setup/js/bash_command_parser.test.cjs | 19 ++++++ .../js/bash_command_parser_comments.test.cjs | 63 ++++++++++++++++++- actions/setup/js/copilot_sdk_driver.test.cjs | 10 +++ .../fuzz_bash_command_parser_harness.test.cjs | 2 +- 6 files changed, 116 insertions(+), 7 deletions(-) diff --git a/.changeset/fix-sdk-shell-comment-permissions.md b/.changeset/fix-sdk-shell-comment-permissions.md index 1aa9008dee0..d99833ffaa2 100644 --- a/.changeset/fix-sdk-shell-comment-permissions.md +++ b/.changeset/fix-sdk-shell-comment-permissions.md @@ -7,3 +7,5 @@ Fix false Copilot SDK shell permission denials for commands containing comments Keep ambiguous nested shell quoting fail-closed and terminate comments only at line feeds so quote confusion and carriage returns cannot hide disallowed commands. Reject unsupported unquoted parenthesis contexts, including extended globs, so literal pattern hashes cannot conceal following commands. + +Check commands after background (`&`) and stderr-pipe (`|&`) operators while preserving redirection ampersands, and retain Bash-specific word boundaries across escaped newlines and non-Bash whitespace. diff --git a/actions/setup/js/bash_command_parser.cjs b/actions/setup/js/bash_command_parser.cjs index 28a094d7224..0a47c95ce97 100644 --- a/actions/setup/js/bash_command_parser.cjs +++ b/actions/setup/js/bash_command_parser.cjs @@ -21,7 +21,7 @@ function findShellExpansionOpener(commandText, dollarIndex) { * Dedicated bash command parser for permission checking in the Copilot SDK driver. * * Provides utilities to: - * - Split a shell command text on pipeline operators (&&, ||, |, ;) + * - Split a shell command text on pipeline operators (&&, ||, |, |&, ;, &) * - Extract the executable command name from a shell segment * - Extract all command names from a complex piped/chained command * @@ -80,7 +80,7 @@ function findShellQuoteEnd(commandText, start) { /** * Split a shell command text into individual pipeline segments. - * Splits on the following shell operators: &&, ||, |, ; and newlines. + * Splits on the following shell operators: &&, ||, |, |&, ;, & and newlines. * * The split respects: * - Single-quoted strings (no escaping inside) @@ -216,6 +216,14 @@ function splitOnPipelineOperators(commandText) { // ── Pipeline operators ── + // Keep redirection ampersands distinct from background separators. + if (((ch === ">" || ch === "<") && commandText[i + 1] === "&") || (ch === "&" && commandText[i + 1] === ">")) { + current += ch + commandText[i + 1]; + atWordStart = true; + i += 2; + continue; + } + // && (AND-then) if (ch === "&" && i + 1 < len && commandText[i + 1] === "&") { segments.push(current); @@ -226,6 +234,15 @@ function splitOnPipelineOperators(commandText) { continue; } + if (ch === "&") { + segments.push(current); + current = ""; + atWordStart = true; + i++; + while (i < len && SHELL_BLANK_RE.test(commandText[i])) i++; + continue; + } + // || (OR-else) — must be checked before lone | if (ch === "|" && i + 1 < len && commandText[i + 1] === "|") { segments.push(current); @@ -236,12 +253,12 @@ function splitOnPipelineOperators(commandText) { continue; } - // | (pipe) + // | and |& (pipe stdout, or stdout and stderr) if (ch === "|") { segments.push(current); current = ""; atWordStart = true; - i++; + i += commandText[i + 1] === "&" ? 2 : 1; while (i < len && SHELL_BLANK_RE.test(commandText[i])) i++; continue; } @@ -401,7 +418,7 @@ function extractCommandName(segment) { /** * Extract all unique command names from a bash pipeline or command sequence. * - * Splits the text on &&, ||, |, and ; and extracts the executable command name + * Splits the text on &&, ||, |, |&, ;, and & and extracts the executable command name * from each resulting segment. Returns a deduplicated array preserving * first-occurrence order. * diff --git a/actions/setup/js/bash_command_parser.test.cjs b/actions/setup/js/bash_command_parser.test.cjs index 328c84d1510..038f6d0ee0b 100644 --- a/actions/setup/js/bash_command_parser.test.cjs +++ b/actions/setup/js/bash_command_parser.test.cjs @@ -35,6 +35,25 @@ describe("splitOnPipelineOperators", () => { expect(splitOnPipelineOperators("ls -la | grep pattern")).toEqual(["ls -la", "grep pattern"]); }); + it.each([ + ["echo ok & cat data.json", ["echo ok", "cat data.json"]], + ["echo ok&cat data.json&", ["echo ok", "cat data.json"]], + ["echo ok |& cat data.json", ["echo ok", "cat data.json"]], + ["echo ok 2>&1&cat data.json", ["echo ok 2>&1", "cat data.json"]], + ["echo ok 2<&0 & cat data.json", ["echo ok 2<&0", "cat data.json"]], + ["echo ok &>output & cat data.json", ["echo ok &>output", "cat data.json"]], + ["echo ok &>>output&cat data.json", ["echo ok &>>output", "cat data.json"]], + [String.raw`echo ok \>&cat data.json`, [String.raw`echo ok \>`, "cat data.json"]], + ])("separates background and stderr-pipe commands in %s", (script, segments) => { + expect(splitOnPipelineOperators(script)).toEqual(segments); + expect(extractCommandNamesFromPipeline(script)).toEqual(["echo", "cat"]); + }); + + it.each(["echo ok 2>&1", "echo ok 1>&2", "echo ok 2<&0", "echo ok &>output", "echo ok &>>output", "echo 'a & b |& c'", 'echo "a & b |& c"', String.raw`echo a\&b`])("preserves redirections and literal ampersands in %s", script => { + expect(splitOnPipelineOperators(script)).toEqual([script]); + expect(extractCommandNamesFromPipeline(script)).toEqual(["echo"]); + }); + it("splits on ; (sequential)", () => { expect(splitOnPipelineOperators("echo a; echo b")).toEqual(["echo a", "echo b"]); }); diff --git a/actions/setup/js/bash_command_parser_comments.test.cjs b/actions/setup/js/bash_command_parser_comments.test.cjs index c73dd3077d7..c0d4c0eacf1 100644 --- a/actions/setup/js/bash_command_parser_comments.test.cjs +++ b/actions/setup/js/bash_command_parser_comments.test.cjs @@ -2,6 +2,7 @@ import { describe, it, expect } from "vitest"; import { createRequire } from "module"; +import { spawnSync } from "node:child_process"; const require = createRequire(import.meta.url); const { splitOnPipelineOperators, extractCommandNamesFromPipeline } = require("./bash_command_parser.cjs"); @@ -234,7 +235,7 @@ describe("commented Bash script fixtures", () => { }); describe.each(lineEndings)("comment placement with $name line endings", ({ value: newline }) => { - for (const separator of ["\n", ";", "&&", "||", "|"]) { + for (const separator of ["\n", ";", "&&", "||", "|", "&", "|&"]) { it(`ignores inert comments around ${JSON.stringify(separator)} separators`, () => { const text = "inert comment"; const operator = separator === "\n" ? newline : separator; @@ -271,6 +272,58 @@ describe.each(lineEndings)("comment placement with $name line endings", ({ value }); describe("comment parser security boundaries", () => { + it.each([ + "echo ok & forbidden_probe", + "echo ok&forbidden_probe", + "echo ok & # Background command\nforbidden_probe", + "echo ok 2>&1&forbidden_probe", + String.raw`echo ok \>&forbidden_probe`, + 'echo ok "${value:-"x #"}"; forbidden_probe', + '#\recho ok "\nforbidden_probe\n#"', + "echo ok\r#; forbidden_probe", + "echo ok $\\\n{value:-x #}; forbidden_probe", + "shopt -s extglob\necho ok @(x #); forbidden_probe", + ])("rejects scripts whose disallowed command is reached by the Bash oracle: %j", fullCommandText => { + const result = spawnSync("bash", ["--noprofile", "--norc", "-c", 'forbidden_probe() { builtin printf "\\nMOCK_REACHED\\n"; }\n' + fullCommandText], { + env: {}, + encoding: "utf8", + timeout: 2000, + }); + expect(result.error).toBeUndefined(); + expect(result.status).toBe(0); + expect(result.stdout).toContain("\nMOCK_REACHED\n"); + for (const grant of ["shell(echo)", "shell(echo ok:*)"]) { + const handler = buildCopilotSDKPermissionHandler({ allowedTools: ["shell(shopt)", grant] }, () => ({ kind: "approve-once" })); + expect(handler({ kind: "shell", fullCommandText }).kind).toBe("reject"); + } + }); + + it.each([ + "echo ok & forbidden_probe", + "echo ok&forbidden_probe", + "echo ok & # Background command\nforbidden_probe", + "echo ok # Commented ampersand &\nforbidden_probe", + "echo ok 2>&1&forbidden_probe", + "echo ok 2<&0 & forbidden_probe", + "echo ok &>output & forbidden_probe", + "echo ok &>>output&forbidden_probe", + String.raw`echo ok \>&forbidden_probe`, + "echo ok |& forbidden_probe", + ])("checks every command around ampersands: %s", fullCommandText => { + expect(extractCommandNamesFromPipeline(fullCommandText)).toContain("forbidden_probe"); + for (const grant of ["shell(echo)", "shell(echo:*)", "shell(echo ok:*)"]) { + const handler = buildCopilotSDKPermissionHandler({ allowedTools: [grant] }, () => ({ kind: "approve-once" })); + expect(handler({ kind: "shell", fullCommandText }).kind).toBe("reject"); + } + }); + + it.each(["echo ok & echo done &", "echo ok & # Background command\ncat data.json", "echo ok 2>&1", "echo ok &>output", "echo ok |& cat data.json", "echo ${value:-a&b} # Actual comment"])( + "preserves permitted commands and ampersand redirections: %s", + fullCommandText => { + expect(checkPermission({ kind: "shell", fullCommandText }).kind).toBe("approve-once"); + } + ); + it.each(["echo ok ${value:-foo|bar} && cat data.json", "echo ok ${value:-foo;bar} && cat data.json"])("keeps command separators inside parameter expansions: %s", fullCommandText => { expect(splitOnPipelineOperators(fullCommandText)).toEqual([fullCommandText.slice(0, fullCommandText.indexOf(" && ")), "cat data.json"]); expect(extractCommandNamesFromPipeline(fullCommandText)).toEqual(["echo", "cat"]); @@ -285,6 +338,14 @@ describe("comment parser security boundaries", () => { } }); + it.each(["\r", "\v", "\f", "\u00a0", "\u2003", "\u2028", "\u2029", "\ufeff"])("preserves non-Bash word characters after separators: %j", character => { + for (const separator of [";", "&&", "||", "|", "&", "\n"]) { + const fullCommandText = `echo ok${separator}${character}#; forbidden_probe`; + expect(extractCommandNamesFromPipeline(fullCommandText)).toContain("forbidden_probe"); + expect(checkPermission({ kind: "shell", fullCommandText }).kind).toBe("reject"); + } + }); + it.each(["echo ok $\\\n{value:-x #}; forbidden_probe", "echo ok $\\\n'x #'; forbidden_probe", 'echo ok "$\\\n(echo x)"; forbidden_probe', 'echo ok "$\\\n{value:-x #}"; forbidden_probe'])( "rejects expansion openers separated by escaped newlines: %s", fullCommandText => { diff --git a/actions/setup/js/copilot_sdk_driver.test.cjs b/actions/setup/js/copilot_sdk_driver.test.cjs index 1e5219dcb05..d47ce27f143 100644 --- a/actions/setup/js/copilot_sdk_driver.test.cjs +++ b/actions/setup/js/copilot_sdk_driver.test.cjs @@ -2197,6 +2197,16 @@ describe("copilot_sdk_driver.cjs", () => { }); }); + it("checks background commands even when SDK identifiers omit them", async () => { + const handler = await makePermissionHandlerViaSDK(["shell(echo)"]); + for (const fullCommandText of ["echo ok & forbidden_probe", "echo ok 2>&1&forbidden_probe", "echo ok & # Comment\nforbidden_probe", "echo ok |& forbidden_probe"]) { + expect(handler({ kind: "shell", fullCommandText, commands: [{ identifier: "echo" }] }).kind).toBe("reject"); + expect(handler({ kind: "shell", fullCommandText: "", commands: [{ identifier: fullCommandText }] }).kind).toBe("reject"); + } + expect(handler({ kind: "shell", fullCommandText: "echo ok & echo done &", commands: [] })).toEqual({ kind: "approve-once" }); + expect(handler({ kind: "shell", fullCommandText: "echo ok 2>&1", commands: [] })).toEqual({ kind: "approve-once" }); + }); + it("denies executable content hidden behind control flow or leading redirection", async () => { const handler = await makePermissionHandlerViaSDK(["shell(echo)", "shell(git:*)"]); // A segment whose first token is a redirection yields no extractable diff --git a/actions/setup/js/fuzz_bash_command_parser_harness.test.cjs b/actions/setup/js/fuzz_bash_command_parser_harness.test.cjs index baebcd74316..187d2f78810 100644 --- a/actions/setup/js/fuzz_bash_command_parser_harness.test.cjs +++ b/actions/setup/js/fuzz_bash_command_parser_harness.test.cjs @@ -33,7 +33,7 @@ const { testSplitOnPipelineOperators, testExtractCommandName, testExtractCommand // ───────────────────────────────────────────────────────────────────────────── /** Pipeline operators that should split segments */ -const OPERATORS = ["&&", "||", "|", ";"]; +const OPERATORS = ["&&", "||", "|", "|&", ";", "&"]; /** Commands that should be recognised as safe (typically in the workflow allow-list) */ const SAFE_COMMANDS = ["ls", "cat", "echo", "grep", "wc", "find", "jq", "printf", "pwd", "date", "head", "tail", "sort", "uniq", "yq", "safeoutputs", "gh", "git"];