Filter Runner-Guard RGS-012 false positives for local Copilot allow-tools - #54341
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
✅ PR Code Quality Reviewer completed the code quality review.
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check.
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
|
✅ Ponytail Reviewer completed successfully!
|
|
✅ Test Quality Sentinel completed test quality analysis. Test Quality Sentinel skipped because pre-fetch PR data was unavailable: unable to fetch test file diff
|
Comment MemoryNote This comment is managed by comment memory.It stores persistent context for this thread in the code block at the top of this comment.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Overall, this change looks narrowly scoped and the new tests cover the main false-positive cases I would expect.
Review notes
I did not find a changed-line correctness or security regression worth blocking on. The new suppression only applies to RGS-012, still preserves executable curl invocations, and explicitly refuses mixed local/non-local allow-tool contexts.
The requested grumpy-coder sub-agent was unavailable in this environment, so this review is based on my own pass over the diff.
🔎 Code quality review by PR Code Quality Reviewer · gpt54 · 4.54 AIC · ⌖ 6.82 AIC · ⊞ 7K
Comment /review to run again
There was a problem hiding this comment.
The implementation is correct and well-tested. The filter conservatively suppresses RGS-012 only when: (1) the finding falls inside a step named exactly Execute GitHub Copilot CLI, (2) the finding line precedes run:, and (3) all allow-tool hosts in the step are loopback addresses. Any step mixing local and non-local curl targets is preserved as a live finding. Tests cover all the key cases including the evil.example.com mixed scenario. LGTM.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 27.9 AIC · ⌖ 8.79 AIC · ⊞ 5.7K
There was a problem hiding this comment.
Ponytail pass (over-engineering only).
net: -15 lines possible.
Generated by ✂️ Ponytail Reviewer for #54341 · auto · 26.6 AIC · ⌖ 8.19 AIC · ⊞ 7.3K
Comment /ponytail to run again
| func allDigits(value string) bool { | ||
| if value == "" { | ||
| return false | ||
| } | ||
| for _, r := range value { | ||
| if r < '0' || r > '9' { | ||
| return false | ||
| } | ||
| } | ||
| return true | ||
| } |
There was a problem hiding this comment.
L186-196: stdlib: hand-rolled digit-only check. _, err := strconv.Atoi(port); err == nil, 1 line.
| func curlAllowToolCommentHost(line string) (string, bool) { | ||
| trimmed := strings.TrimSpace(line) | ||
| trimmed = strings.TrimPrefix(trimmed, "#") | ||
| trimmed = strings.TrimSpace(trimmed) | ||
| const prefix = "--allow-tool shell(curl " | ||
| if !strings.HasPrefix(trimmed, prefix) || !strings.HasSuffix(trimmed, ")") { | ||
| return "", false | ||
| } | ||
|
|
||
| return curlTargetHost(strings.TrimSuffix(strings.TrimPrefix(trimmed, prefix), ")")) | ||
| } | ||
|
|
||
| func curlAllowToolHosts(line string) []string { | ||
| const prefix = "shell(curl " | ||
| var hosts []string | ||
| remaining := line | ||
| for { | ||
| index := strings.Index(remaining, prefix) | ||
| if index < 0 { | ||
| return hosts | ||
| } | ||
| remaining = remaining[index+len(prefix):] | ||
| end := strings.Index(remaining, ")") | ||
| if end < 0 { | ||
| return hosts | ||
| } | ||
| if host, ok := curlTargetHost(remaining[:end]); ok { | ||
| hosts = append(hosts, host) | ||
| } | ||
| remaining = remaining[end+1:] | ||
| } | ||
| } |
There was a problem hiding this comment.
L126-157: yagni: two near-identical curl-host extractors (comment-single vs line-multi). One regex/loop over shell(curl ...) occurrences handles both; comment form is just the multi-host loop with 1 match expected.
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /tdd — requesting changes on one security-relevant correctness issue.
📋 Key Themes & Highlights
Key Themes
- Fast-path bypass (security):
findingInCopilotLocalCurlAllowToolhas a fast path that suppresses findings for any line matching the local curl allow-tool pattern without verifying the line belongs to a Copilot execution step. The safer slow path below does check step membership — the fast path should either be removed or guarded identically. - Edge case in
curlTargetHost: mixed-wildcard port strings (e.g.3*00) are not stripped, leaving the host unmatched and a finding unexpectedly un-suppressed. Low probability but worth a comment or a guard. - Missing unit test for
isStepBoundaryLine: the tests rely on coincidental fixture structure; an explicit test would pin the contract.
Positive Highlights
- ✅ Excellent test coverage — both positive (suppressed) and negative (preserved) cases are exercised.
- ✅
hasNonLocalCurlAllowToolguard prevents suppression when a step mixes local and remote targets — exactly the right safety boundary. - ✅ Clean, single-responsibility helper functions throughout.
- ✅ File-lines cache (
fileLinesByPath) avoids repeated disk reads for multi-finding workflows.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 38.3 AIC · ⌖ 10.3 AIC · ⊞ 7.8K
Comment /matt to run again
| return true | ||
| } | ||
|
|
||
| stepStart := -1 |
There was a problem hiding this comment.
[/diagnosing-bugs] The fast path in findingInCopilotLocalCurlAllowTool suppresses findings on a matching line without verifying the line is inside a Copilot execution step — inconsistent with the stricter step-scoped check below it.
💡 Details and suggested fix
The fast path fires for any file that has # --allow-tool shell(curl (localhost/redacted) even in non-Copilot steps. The slow path guards against this with isStepNameLine(lines[stepStart], copilotExecutionStepNameMarker)`, but the fast path bypasses that guard:
// line 47 — no step-membership check
if isLocalCurlAllowToolComment(lines[lineIndex]) || isLocalCurlAllowToolArgumentLine(lines[lineIndex]) {
return true
}An attacker (or an accidental workflow) can silence an RGS-012 finding simply by placing `# --allow-tool shell(curl (localhost/redacted) in any step.
Simplest fix: remove the fast path entirely. The slow path already handles these lines correctly since lineIndex < runIndex places them before run:.
@copilot please address this.
| filtered := filterCopilotLocalAllowToolFindings(findings, gitRoot) | ||
|
|
||
| require.Len(t, filtered, 3) | ||
| assert.Equal(t, suspiciousCurlLine, filtered[0].Line) |
There was a problem hiding this comment.
[/tdd] The test uses lineContaining(t, lines, "Execute GitHub Copilot CLI") to find the step name line, but that line is the name: field — not a step boundary marker. The isStepBoundaryLine function may use different criteria. If the test passes because of how step-finding works, it's worth an explicit unit test for isStepBoundaryLine to document what constitutes a "step boundary".
💡 Why this matters
If isStepBoundaryLine looks for - name: or a specific YAML indentation pattern, the test's use of a plain name: line might coincidentally work today but mask a regression if the boundary logic changes. A focused test like:
func TestIsStepBoundaryLine(t *testing.T) {
assert.True(t, isStepBoundaryLine(" - name: Execute GitHub Copilot CLI"))
assert.False(t, isStepBoundaryLine(" name: Execute GitHub Copilot CLI"))
}...would pin the contract and catch future regressions.
@copilot please address this.
| func curlTargetHost(target string) (string, bool) { | ||
| target = strings.TrimSpace(target) | ||
| if fields := strings.Fields(target); len(fields) > 0 { | ||
| target = fields[0] |
There was a problem hiding this comment.
[/diagnosing-bugs] curlTargetHost strips port wildcards (*) and digits but not other wildcard patterns (e.g. (localhost/redacted) The path is already stripped by the /check, but a URL like(localhost/redacted) (unusual but not impossible from a crafted allow-tool line) would leave the port stripping silently skipping: allDigits("3*00") is false and port == "*" is false, so the port stays in the host string, causing isLocalCurlAllowToolHost to return false and the finding to be preserved unexpectedly.
💡 Suggested fix
The risk is low in practice but worth a comment or an additional strings.HasPrefix(port, "*") guard:
if port == "*" || allDigits(port) || strings.ContainsAny(port, "*?") {
target = target[:colon]
}Or simply document that wildcard ports other than * are out of scope.
@copilot please address this.
There was a problem hiding this comment.
Pull request overview
Filters Runner-Guard RGS-012 false positives for generated Copilot local-curl allow-tool declarations, addressing #54157.
Changes:
- Adds filtering for approved local hosts.
- Preserves unrelated findings and executable curl findings.
- Adds focused unit tests.
Show a summary per file
| File | Description |
|---|---|
pkg/cli/runner_guard.go |
Integrates the new filter. |
pkg/cli/runner_guard_copilot_allow_tool.go |
Implements detection and host parsing. |
pkg/cli/runner_guard_copilot_allow_tool_test.go |
Tests suppression boundaries. |
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 3
- Review effort level: Balanced
| if isLocalCurlAllowToolComment(lines[lineIndex]) || isLocalCurlAllowToolArgumentLine(lines[lineIndex]) { | ||
| return true |
| } | ||
| } | ||
|
|
||
| return hasToolCommentHeader && hasLocalCurlAllowTool && !hasNonLocalCurlAllowTool |
| target = strings.Trim(target, "[]") | ||
| if colon := strings.LastIndex(target, ":"); colon >= 0 { | ||
| port := target[colon+1:] | ||
| if port == "*" || allDigits(port) { | ||
| target = target[:colon] | ||
| } | ||
| } |
🏗️ Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (356 new lines in 📄 Draft ADR committed:
📋 What to do next
Once an ADR is linked in the PR body, this gate will re-run and verify the implementation matches the decision. ❓ Why ADRs Matter
ADRs create a searchable, permanent record of why the codebase looks the way it does. Future contributors (and your future self) will thank you. 📋 Michael Nygard ADR Format ReferenceAn ADR must contain these four sections to be considered complete:
All ADRs are stored in
|
|
@copilot run pr-finisher |
|
@copilot PR sous-chef triage:
|
Finish pass complete at ADR review Validation status
Action-required runs on Remaining human actions
|
🔍 PR TriageCategory: Score: 42/100 (impact 22/50 + urgency 12/30 + quality 8/20) Recommended action: CI has 1 failure. Has ADR and tests but needs CI fix before merge. Automated triage — run 32432526976
|
PR Triage
|
|
/q determine who the pr-sous-chef agentic workflow is not adding comments to this PR |
|
🍳 skillet has started processing this pull request comment |
Q Investigation: Why
|
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Fixed in That step runs I rewrote the affected messages to include expected/example guidance and dropped the generic
Substrings asserted by existing tests ( Verified locally by reproducing the exact CI invocation: |
|
🎉 This pull request is included in a new release. Release: |
Runner-Guard flagged generated Copilot allow-tool declarations for local
curltargets as possible secret exfiltration. These lines describe permitted tools for loopback/local dev-server access; they are not executable outbound requests.Runner-Guard filtering
localhost127.0.0.1::1host.docker.internalSafety boundaries
curlcommands, including local targets carrying payloads.Run: https://github.com/github/gh-aw/actions/runs/32420067325> Generated by 👨🍳 PR Sous Chef · gpt54 · 7.36 AIC · ⌖ 8.07 AIC · ⊞ 9.3K · ◷
PR Sous Chef branch refresh requested from run https://github.com/github/gh-aw/actions/runs/32422171255> Generated by 👨🍳 PR Sous Chef · gpt54 · 12.8 AIC · ⌖ 10.5 AIC · ⊞ 9.3K · ◷