Fix Copilot SDK multiword shell prefix matching and denial-guard hang - #61430
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
| if (!name) { | ||
| // No executable name could be extracted (e.g. a bare shell control-flow | ||
| // segment such as "for f in $FILES", "if [ ... ]", "fi", or "done" that | ||
| // resulted from splitting a multi-line script on statement boundaries). | ||
| // These are not independently executable commands, so they are not | ||
| // subject to the allow-list — matching prior behavior where such | ||
| // segments were excluded from the extracted command-name set entirely. | ||
| return true; | ||
| } |
|
✅ Test Quality Sentinel completed test quality analysis. Test Quality Sentinel skipped because pre-fetch PR data was unavailable: unable to fetch test file diff
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "registry.npmjs.org"See Network Configuration for more information.
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. See the comment below for the result and any generated ADR draft. Warning Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding. What happenedThe threat detection engine failed to produce results. Review the workflow run logs for details.
|
|
✅ Ponytail Reviewer completed successfully! Lean already. Ship. Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "ab.chatgpt.com"See Network Configuration for more information.
|
|
Warning Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding. What happenedThe threat detection engine failed to produce results. Review the workflow run logs for details.
|
There was a problem hiding this comment.
🟡 Changes recommended
The matcher permits a control-flow allow-list bypass and changes exact-command semantics, while the unreferenced timer weakens the guaranteed failure exit.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Fixes Copilot SDK shell-prefix matching and bounds denial-guard shutdowns.
Changes:
- Matches multiword shell grants using parsed command segments.
- Adds a timed denial-guard failure path and regression tests.
- Updates mirrored Docker action pins.
File summaries
| File | Description |
|---|---|
actions/setup/js/copilot_sdk_permissions.cjs |
Revises shell permission matching. |
actions/setup/js/copilot_sdk_session.cjs |
Adds bounded denial-guard settlement. |
actions/setup/js/copilot_sdk_driver.test.cjs |
Adds regression coverage. |
.github/aw/actions-lock.json |
Changes canonical Docker action pins. |
pkg/actionpins/data/action_pins.json |
Mirrors action-pin changes. |
pkg/workflow/data/action_pins.json |
Mirrors workflow action pins. |
Review details
- Files reviewed: 6/6 changed files
- Comments generated: 6
- Review effort level: Balanced (auto)
Note
Copilot is running an experiment and ran this review at Balanced.
| // These are not independently executable commands, so they are not | ||
| // subject to the allow-list — matching prior behavior where such | ||
| // segments were excluded from the extracted command-name set entirely. | ||
| return true; |
| */ | ||
| function segmentStartsWithPrefix(segmentText, prefix) { | ||
| if (!prefix) return false; | ||
| return segmentText === prefix || segmentText.startsWith(`${prefix} `); |
| if (!rule.includes(" ")) { | ||
| return identifier === rule; | ||
| return name === rule; | ||
| } | ||
| return false; | ||
| return trimmedSegment === rule; |
| denialGuardReject(catastrophicToolDenialsError); | ||
| } | ||
| }, denialGuardTimeoutMs); | ||
| if (typeof forceExitTimer.unref === "function") forceExitTimer.unref(); |
| "docker/build-push-action@v7.3.0": { | ||
| "repo": "docker/build-push-action", | ||
| "version": "v7.4.0", | ||
| "sha": "c3c9e263c25d99ce0380d002d59b67737d91b0dc" | ||
| "version": "v7.3.0", | ||
| "sha": "53b7df96c91f9c12dcc8a07bcb9ccacbed38856a" |
| "docker/setup-buildx-action@v4.3.0": { | ||
| "repo": "docker/setup-buildx-action", | ||
| "version": "v4.4.0", | ||
| "sha": "594f3bf4285d9ea8dc53c9a0c9c4092420091003" | ||
| "version": "v4.3.0", | ||
| "sha": "37fe631027851001ddb9b187196cc803df7f5f0e" |
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /tdd — requesting changes on a confirmed permission-check bypass introduced by the shell-matching refactor.
📋 Key Themes & Highlights
Key Themes
- Security regression (confirmed by local repro):
isSegmentAllowedByShellRulesreturnstrue(auto-approve) for any pipeline segment whereextractCommandNamesFromPipelinecan't extract a command name — this covers more than control-flow keywords; it also fires for bare redirections (> /tmp/pwn,2>&1), lone!/{/}tokens, and other unparseable fragments. This invertsbash_command_parser.cjs's own documented "fail-closed" invariant and was already flagged by a prior GHAS review comment on this same line. - Test coverage gap: the new multiword-prefix regression test covers the happy path and two plainly-denied subcommands, but not the fail-open branch itself — a test for the exact bypass would prevent silent re-regression.
- Denial-guard fix looks solid: the bounded force-exit timer race against
sendAndWaitis well-reasoned, the timer isunref()'d, and the accompanying test simulates a stuckdisconnect()/sendAndWait()and asserts bounded completion time — good/diagnosing-bugspractice (reproduce → bound → regression test).
Positive Highlights
- ✅ Multiword
:*prefix matching itself (git checkout:*etc.) is correctly implemented and well-documented, with the exact SDK-identifier-unreliability rationale spelled out in comments. - ✅ Ungranted subcommand denial (
git push,git status) is explicitly tested to prevent unintended widening togit:*. - ✅ Denial-guard regression test asserts wall-clock bound (
elapsedMs < 10_000) rather than just checking the promise resolves, catching the actual hang scenario.
Please address the fail-open branch (copilot_sdk_permissions.cjs line ~293) before merge — it can let an unrecognized/unparseable shell segment slip past the allow-list entirely.
Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
registry.npmjs.org
To allow these domains, add them to the network.allowed list in your workflow frontmatter:
network:
allowed:
- defaults
- "registry.npmjs.org"See Network Configuration for more information.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · copilot · sonnet50 · 95 AIC · ⌖ 14.3 AIC · ⊞ 10.4K
Comment /matt to run again
| // These are not independently executable commands, so they are not | ||
| // subject to the allow-list — matching prior behavior where such | ||
| // segments were excluded from the extracted command-name set entirely. | ||
| return true; |
There was a problem hiding this comment.
[/diagnosing-bugs] Fail-open on unparseable segments: any pipeline segment where extractCommandNamesFromPipeline can't extract a name (not just control-flow keywords) is auto-approved regardless of shell rules.
💡 Reproduction and root cause
Verified locally:
const handler = buildCopilotSDKPermissionHandler({ allowedTools: ["shell(echo:*)"] }, approveAll);
handler({ kind: "shell", fullCommandText: "> /tmp/pwn", commands: [] });
// => { kind: 'approve-once' } (should be rejected!)
handler({ kind: "shell", fullCommandText: "2>&1", commands: [] });
// => { kind: 'approve-once' }extractCommandName returns null for any segment whose first token is a bare redirection (>, 2>&1, etc.), a lone !/{/}, or an unparseable subshell fragment — not only for the control-flow keywords the comment above this line describes. Since splitOnPipelineOperators also splits on ;, an attacker-influenced command like echo hi; > /tmp/pwn or rm -rf / 2>&1 (where the redirection becomes/ends its own segment) can smuggle a segment through this return true fail-open path even when no rule grants it.
The comment claims this "match[es] prior behavior where such segments were excluded from the extracted command-name set entirely" — but prior behavior excluded the segment from the set used to build normalizedCommandIdentifiers/parsedNames.every(...), it did not treat exclusion as automatic approval. Excluding a segment from an every() check is very different from returning true for that segment directly: the old code required all extracted names to be allowed, with un-extractable segments contributing nothing (safe), whereas this new helper actively vouches for them.
Given bash_command_parser.cjs's own documented "Security invariant" (line 22: "when parsing is ambiguous ... the caller receives an empty array and the permission checker falls back to denying the request, ensuring a safe default"), this branch inverts that invariant for the multiword-prefix path. Recommend returning false here (deny) unless the segment can be proven to be a genuine non-executable control-flow fragment (e.g. explicitly checking the token is a known keyword), and adding a regression test for a lone redirection segment smuggled via ;.
@copilot please address this.
| expect(handler({ kind: "shell", commands: [{ identifier: "git" }], fullCommandText: "git status" })).toEqual({ | ||
| kind: "reject", | ||
| feedback: "Tool invocation is not allowed by workflow tool permissions.", | ||
| }); |
There was a problem hiding this comment.
[/tdd] The new multiword-prefix regression test only exercises the happy path (allowed subcommands + two plainly-denied subcommands); it doesn't cover the fail-open branch flagged in the other comment.
💡 Suggested regression test
it("denies a segment that cannot be parsed into a command name (e.g. a bare redirection)", async () => {
const handler = await makePermissionHandlerViaSDK(["shell(echo:*)"]);
expect(handler({ kind: "shell", fullCommandText: "> /tmp/pwn", commands: [] })).toEqual({
kind: "reject",
feedback: "Tool invocation is not allowed by workflow tool permissions.",
});
expect(handler({ kind: "shell", fullCommandText: "echo hi; > /tmp/pwn", commands: [] })).toEqual({
kind: "reject",
feedback: "Tool invocation is not allowed by workflow tool permissions.",
});
});Per /tdd, a bug fix (or in this case, a refactor that changes matching semantics) should ship with a test proving the specific failure mode it must not reintroduce — otherwise this exact gap can silently regress again in a future change.
@copilot please address this.
There was a problem hiding this comment.
Added in bb0d9c8. The denies executable content hidden behind control flow or leading redirection test now covers the fail-open branch directly: bare > /tmp/pwn, the numeric fd form 2>&1 rm -rf /important, and redirections appearing as a later stage of a chain whose earlier stages are granted (echo hi; > /tmp/pwn, git status && 2>/dev/null rm -rf /important, with shell(git:*) granted). All are rejected because isSegmentAllowedByShellRules now falls through to isNonExecutableShellSegment instead of returning true when no command name can be extracted.
There was a problem hiding this comment.
Applied mixed_unclear review mode (critique + audit guidance, adapted for this backend logic change) since the diff spans SDK permission-matching logic, a hang-mitigation timeout, and lock-file pin data.
Blocking issues found:
- Security regression (
copilot_sdk_permissions.cjs:294,isSegmentAllowedByShellRules) — the fail-open branch for "no executable name extracted" also fires for segments beginning with a redirection token (2>&1 rm -rf /...), not just genuine control-flow fragments. This lets an attacker-controlled shell segment bypass every configuredshell(...)allow-list rule, even a narrow one likeshell(git:*). Verified locally with a standalone repro. - Broken test — the diff downgrades
docker/build-push-actionanddocker/setup-buildx-actionpins in.github/aw/actions-lock.json/pkg/actionpins/data/action_pins.json/pkg/workflow/data/action_pins.jsonback tov7.3.0/v4.3.0, undoing #61359's fix.go test ./pkg/workflow/... -run TestWasmGolden_CompileFixtures/smoke-copilotfails on this branch as a result. Looks like a stale-branch/merge artifact rather than an intended change — needs a rebase.
The multiword :* prefix-matching fix and the bounded denial-guard timeout are both well-reasoned and well-tested; no concerns there beyond the parser edge case above.
Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
github.com
To allow these domains, add them to the network.allowed list in your workflow frontmatter:
network:
allowed:
- defaults
- "github.com"See Network Configuration for more information.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · copilot · sonnet50 · 127 AIC · ⌖ 14.2 AIC · ⊞ 8.4K
| // subject to the allow-list — matching prior behavior where such | ||
| // segments were excluded from the extracted command-name set entirely. | ||
| return true; | ||
| } |
There was a problem hiding this comment.
Security: fail-open on unparseable segments allows an allow-list bypass via a leading redirection token.
isSegmentAllowedByShellRules returns true (allow) whenever extractCommandNamesFromPipeline(trimmedSegment)[0] is falsy. The comment says this is only meant to cover bare control-flow fragments produced by splitting multi-line scripts (for f in $FILES, if [ ... ], fi, done), but extractCommandName also returns null for any segment whose first token looks like a redirection (/^[<>]/ or /^\d+[<>&]/), even when that segment is a fully executable command with a leading redirect.
This is exploitable even with a restrictive shell allow-list such as ["shell(git:*)"]:
const handler = buildCopilotSDKPermissionHandler({ allowedTools: ["shell(git:*)"] }, approveAll);
handler({ kind: "shell", fullCommandText: "2>&1 rm -rf /important", commands: [] });
// => { kind: "approve-once" } — should be rejected!
handler({ kind: "shell", fullCommandText: "git status && 2>/dev/null rm -rf /important", commands: [] });
// => { kind: "approve-once" } — pipeline-chain "all stages allowed" check is defeated by one bad-parse stageAny attacker-controlled command prefixed with a stdout/stderr redirect (2>&1 ..., >/dev/null ..., 1>foo ...) slips past every configured shell rule, because the segment is treated the same as an inert control-flow fragment instead of an executable command.
Suggested fix: distinguish "no executable name because this is a genuine control-flow fragment" from "no executable name because the segment starts with a redirection/other unparseable executable token." The latter should deny by default (matching the module's own stated security invariant: "when parsing is ambiguous... the caller receives an empty array and the permission checker falls back to denying the request"). For example, only treat a segment as an inert fragment when its first token is a recognized SHELL_KEYWORDS/clause keyword, not whenever extractCommandName returns null for any reason.
@copilot please address this.
There was a problem hiding this comment.
Fixed in 63091b2 (hardened further in 194eec6). isSegmentAllowedByShellRules no longer returns true when no command name can be extracted: it now unwraps recognized leading keywords (if/while/until/time/coproc) and re-checks the wrapped command, otherwise it falls through to isNonExecutableShellSegment, which only accepts inert fragments (assignments, for/select/case headers, then/else/do/fi/done/esac/braces) and rejects anything containing $(, backticks, or process substitution.
Verified against the exact cases in this comment with shell(git:*) granted:
2>&1 rm -rf /important→ rejectgit status && 2>/dev/null rm -rf /important→ reject> /tmp/pwnandecho hi; > /tmp/pwn(withshell(echo:*)) → rejectif rm -rf /tmp/x; then echo ok; fi(withshell(echo)) → reject
while echo hi and git status still approve. Regression coverage is in bb0d9c8 (denies executable content hidden behind control flow or leading redirection).
| "sha": "22d081ff2d3a40755e97629de92e3bcbfa7cf2ed" | ||
| }, | ||
| "docker/build-push-action@v7.4.0": { | ||
| "docker/build-push-action@v7.3.0": { |
There was a problem hiding this comment.
This PR downgrades docker action pins and breaks TestWasmGolden_CompileFixtures.
This diff reverts docker/setup-buildx-action v4.4.0 → v4.3.0 and docker/build-push-action v7.4.0 → v7.3.0 (in .github/aw/actions-lock.json, pkg/actionpins/data/action_pins.json, and pkg/workflow/data/action_pins.json), undoing the bump that #61359 just synced with the wasm golden fixture. These files appear unintentionally stale relative to main (a rebase/merge artifact), not a deliberate change described in the PR body.
Verified locally — this currently fails on this branch:
$ go test ./pkg/workflow/... -run 'TestWasmGolden_CompileFixtures/smoke-copilot'
--- FAIL: TestWasmGolden_CompileFixtures (0.13s)
--- FAIL: TestWasmGolden_CompileFixtures/smoke-copilot (0.13s)
diff:
--- golden
+++ run
@@ -425,9 +425,9 @@
- uses: docker/setup-buildx-action@594f3bf4285d9ea8dc53c9a0c9c4092420091003 # v4.4.0
+ uses: docker/setup-buildx-action@37fe631027851001ddb9b187196cc803df7f5f0e # v4.3.0
- uses: docker/build-push-action@c3c9e263c25d99ce0380d002d59b67737d91b0dc # v7.4.0
+ uses: docker/build-push-action@53b7df96c91f9c12dcc8a07bcb9ccacbed38856a # v7.3.0
Please rebase onto latest main (or manually restore the v4.4.0/v7.4.0 pins) so this branch doesn't reintroduce the drift #61359 fixed.
@copilot please address this.
|
@copilot this PR still needs forward progress.
Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "github.com"See Network Configuration for more information.
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Addressed in |
| function segmentStartsWithPrefix(segmentText, prefix) { | ||
| if (!prefix) return false; | ||
| const normalizedSegment = segmentText.trim().replace(/\s+/g, " "); | ||
| const normalizedPrefix = prefix.trim().replace(/\s+/g, " "); | ||
| return normalizedSegment === normalizedPrefix || normalizedSegment.startsWith(`${normalizedPrefix} `); | ||
| } |
| if (normalizedCommandIdentifiers.length > 0) { | ||
| return normalizedCommandIdentifiers.every(identifier => isSegmentAllowedByShellRules(identifier)); | ||
| } |
|
@copilot clear main, add extensive test suite |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
…dk-issues Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Merged current |
|
@copilot please review the latest state of this PR, refresh from the updated branch if needed, and run the Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "github.com"See Network Configuration for more information.
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Ran Reviews — all flagged matcher issues are closed in the pushed code and verified by direct probe: leading/fd redirections ( Checks (local) — Prior CI failures — One note: CI on the new HEAD is unverified — agent pushes don't trigger workflows, so a maintainer needs to re-trigger before merge. |
|
@copilot please review the latest state of this PR, refresh from the updated branch if needed, and run the
Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "github.com"See Network Configuration for more information.
|
Ran Branch — already contains Reviews — re-verified every flagged matcher case by probing the pushed code directly, and also against
The remaining Checks (local) — Prior CI failures — Nothing further is actionable from here: CI on
Ran Summary: branch already contains No code changes were needed this pass. CI on |
|
🎉 This pull request is included in a new release. Release: |
Git subcommands auto-granted by
safe-outputs.create-pull-request(e.g.shell(git checkout:*),shell(git branch:*)) were rejected by the Copilot SDK driver because the permission matcher relied on unreliable SDK-provided command identifiers. Additionally, once the tool-denial guard tripped, the process could stall indefinitely ifsession.disconnect()or the in-flight SDK request never settled.Multiword
:*prefix matching (copilot_sdk_permissions.cjs)commands[].identifier, which the SDK may populate with just the executable name ("git"), the full command text, or nothing at all — none of which reliably exposes the subcommand word ("checkout") needed to test a rule likeshell(git checkout:*).fullCommandTextinto pipeline segments (via the existing bash parser) and checking each segment directly against shell rules, so multiword prefixes match regardless of what identifiers the SDK reports::*wildcards, pipeline-chain matching (all stages must be allowed), and exact full-command rules retain their existing behavior. Ungranted subcommands (git push,git status) remain denied — no widening to a blanketgit:*.Bounded exit after denial-guard trips (
copilot_sdk_session.cjs)guard.tool_denials_exceededfires, the in-flightsession.sendAndWait()is now raced against a bounded timer (GH_AW_DENIAL_GUARD_TIMEOUT_MS, default 15s).session.disconnect()or the underlying SDK request never resolve, the driver now force-settles with a nonzero exit instead of hanging until the surrounding job's own timeout.guard.tool_denials_exceededevent, denial logging, and failure reporting are unchanged.Run: https://github.com/github/gh-aw/actions/runs/35196972371
Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
github.laiyagushi.comTo allow these domains, add them to the
network.allowedlist in your workflow frontmatter:See Network Configuration for more information.
Run: https://github.com/github/gh-aw/actions/runs/35201512779
Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
github.laiyagushi.comTo allow these domains, add them to the
network.allowedlist in your workflow frontmatter:See Network Configuration for more information.