Fix read-tool workspace permission scoping regression (daily workflows killed by denial threshold) - #49840
Conversation
Always allow read requests for paths at or under GITHUB_WORKSPACE in buildCopilotSDKPermissionHandler. Previously, workflows with any tool restriction but no explicit read grant would deny read($GITHUB_WORKSPACE), exhausting the denial threshold (3/3) and killing the run. - copilot_sdk_permissions.cjs: add workspace-root allowlist in case "read" - copilot_sdk_driver.test.cjs: add regression test + update affected tests Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
✅ Test Quality Sentinel completed test quality analysis. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR does not have the 'implementation' label and has ≤100 new lines of code in business logic directories (0 additions detected in default business logic directories). |
|
|
There was a problem hiding this comment.
Pull request overview
Fixes workspace read denials that prematurely terminate restricted workflows.
Changes:
- Allows SDK reads within
GITHUB_WORKSPACE. - Adds and updates regression tests for workspace-read behavior.
Show a summary per file
| File | Description |
|---|---|
actions/setup/js/copilot_sdk_permissions.cjs |
Adds workspace read authorization. |
actions/setup/js/copilot_sdk_driver.test.cjs |
Tests workspace and external-path decisions. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Balanced
| const normalizedWorkspace = normalizePermissionPath(logOptions.workspaceRoot); | ||
| const normalizedPath = normalizePermissionPath(request.path); | ||
| if (normalizedPath === normalizedWorkspace || normalizedPath.startsWith(normalizedWorkspace + "/")) { | ||
| return true; | ||
| } |
![github-actions[bot]](https://cdn.github.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 29.5 AIC · ⌖ 15 AIC · ⊞ 5.4K
| const normalizedWorkspace = normalizePermissionPath(logOptions.workspaceRoot); | ||
| const normalizedPath = normalizePermissionPath(request.path); | ||
| if (normalizedPath === normalizedWorkspace || normalizedPath.startsWith(normalizedWorkspace + "/")) { | ||
| return true; |
There was a problem hiding this comment.
Security: path-traversal bypass — workspace containment check can be defeated with ../ segments
normalizePermissionPath only strips trailing slashes and normalizes backslashes; it does not resolve .. components. A crafted path like /home/runner/work/gh-aw/gh-aw/../../../etc/passwd passes normalizePermissionPath unchanged and still starts with the workspace prefix, so the early return true fires for a file well outside the checkout.
// Proof
normalizePermissionPath("/home/runner/work/gh-aw/gh-aw/../../../etc/passwd")
// → "/home/runner/work/gh-aw/gh-aw/../../../etc/passwd"
// .startsWith("/home/runner/work/gh-aw/gh-aw/") === true ← incorrectly approvedFix: resolve both paths with path.posix.normalize before the prefix check so .. segments are collapsed:
const { posix } = require("path");
// inside case "read":
if (logOptions?.workspaceRoot && typeof request.path === "string" && request.path.length > 0) {
const normalizedWorkspace = posix.normalize(normalizePermissionPath(logOptions.workspaceRoot));
const normalizedPath = posix.normalize(normalizePermissionPath(request.path));
if (normalizedPath === normalizedWorkspace || normalizedPath.startsWith(normalizedWorkspace + "/")) {
return true;
}
}Add a corresponding test case:
expect(onPermissionRequest({ kind: "read", path: "/home/runner/work/gh-aw/gh-aw/../../../etc/passwd", intention: "" }))
.toEqual({ kind: "reject", feedback: "Tool invocation is not allowed by workflow tool permissions." });@copilot please address this.
🧪 Test Quality Sentinel Report✅ Test Quality Score: 90/100 — Excellent
📊 Metrics (2 tests)
📈 Test Inflation AnalysisTest file: Justification: The new regression test is comprehensive and documents the critical design contract that workspace files must always be readable. This fixes bug #49836 which killed daily workflows when permission denials exceeded a threshold. The detailed test coverage is intentional and justified. Verdict
|
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /tdd — requesting changes on one security correctness issue.
📋 Key Themes & Highlights
Key Themes
- Path traversal bypass (security/correctness):
normalizePermissionPathdoes not collapse..segments, so../../etc/passwd-style paths incorrectly pass the workspace prefix check. This is a security regression introduced by the fix itself — the bypass vector did not exist before this PR. - Missing boundary-condition test: No test covers the
GITHUB_WORKSPACE-unset path, leaving the fallback contract unpinned.
Positive Highlights
- ✅ Root cause correctly identified and fixed at the right layer (
buildCopilotSDKPermissionHandler, not the call site). - ✅ Regression test reproduces the exact production failure scenario (github MCP + restricted bash, no
readgrant). - ✅
try/finallyenv-restore pattern is applied consistently across all three modified test cases. - ✅ Existing test names updated to reflect the new semantics — good specification hygiene.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 50.6 AIC · ⌖ 10.1 AIC · ⊞ 7.1K
Comment /matt to run again
| if (logOptions?.workspaceRoot && typeof request.path === "string" && request.path.length > 0) { | ||
| const normalizedWorkspace = normalizePermissionPath(logOptions.workspaceRoot); | ||
| const normalizedPath = normalizePermissionPath(request.path); | ||
| if (normalizedPath === normalizedWorkspace || normalizedPath.startsWith(normalizedWorkspace + "/")) { |
There was a problem hiding this comment.
[/diagnosing-bugs] Path traversal bypass: normalizePermissionPath strips trailing slashes but does not resolve .. segments. A path like /home/runner/work/gh-aw/gh-aw/../../../etc/passwd passes the startsWith workspace check yet escapes the workspace entirely.
💡 Fix: resolve dot-segments before comparing
Use path.resolve (already available in Node.js) instead of the string-only comparison:
const nodePath = require('path');
// ...
if (logOptions?.workspaceRoot && typeof request.path === 'string' && request.path.length > 0) {
const resolvedWorkspace = nodePath.resolve(logOptions.workspaceRoot);
const resolvedPath = nodePath.resolve(request.path);
if (resolvedPath === resolvedWorkspace || resolvedPath.startsWith(resolvedWorkspace + '/')) {
return true;
}
}Proof of current bypass:
// normalizePermissionPath does NOT collapse ".."
normalize('/home/runner/work/gh-aw/gh-aw/../../../etc/passwd')
// => '/home/runner/work/gh-aw/gh-aw/../../../etc/passwd'
// .startsWith('/home/runner/work/gh-aw/gh-aw/') === true ⚠️This is a security regression — the workspace allowlist must be traversal-safe.
@copilot please address this.
| await runWithCopilotSDK({ | ||
| sdkUri: "http://127.0.0.1:3002", | ||
| prompt: "test prompt", | ||
| logger: () => {}, |
There was a problem hiding this comment.
[/tdd] The regression test for #49836 does not test the case where GITHUB_WORKSPACE is unset (i.e., logOptions.workspaceRoot is undefined). When workspaceRoot is absent, the new early-return is skipped entirely — meaning the pre-existing fallback still applies. A test confirming "when GITHUB_WORKSPACE is not set, behaviour is unchanged" would prevent future regressions where someone accidentally makes the workspace check mandatory.
💡 Suggested additional assertion
it('falls back to prior behaviour when GITHUB_WORKSPACE is unset', async () => {
const prevWorkspace = process.env.GITHUB_WORKSPACE;
delete process.env.GITHUB_WORKSPACE;
try {
const handler = await makePermissionHandlerViaSDK(['shell(ls)']);
// Without a workspace root, workspace-path reads are still denied (no regression in fallback).
expect(handler({ kind: 'read', path: '/home/runner/work/gh-aw/gh-aw/AGENTS.md' })).toEqual({
kind: 'reject',
feedback: 'Tool invocation is not allowed by workflow tool permissions.',
});
} finally {
if (prevWorkspace === undefined) delete process.env.GITHUB_WORKSPACE;
else process.env.GITHUB_WORKSPACE = prevWorkspace;
}
});This pins the contract: the early-return is only active when workspaceRoot is provided.
@copilot please address this.
|
@copilot Please continue triage on this PR. Remaining visible blockers (newest first):
Run details: https://github.com/github/gh-aw/actions/runs/30766084787
|
…relative Replace string-prefix check with path.resolve + path.relative to correctly: - Reject traversal paths (e.g. workspace/../../../../etc/passwd) - Approve relative paths (e.g. AGENTS.md) resolved inside workspace Also update tests: - Add traversal and relative-path assertions to regression test - Add traversal and relative-path assertions to unit permission handler test - Update original scoped-handler test to use /etc/passwd instead of a.txt (relative paths now correctly resolve inside workspace and are approved)" Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Addressed in commit fixing the workspace read containment check. The string-prefix check has been replaced with case "read":
if (logOptions?.workspaceRoot && typeof request.path === "string" && request.path.length > 0) {
const resolvedWorkspace = path.resolve(logOptions.workspaceRoot);
const resolvedPath = path.isAbsolute(request.path)
? path.resolve(request.path)
: path.resolve(resolvedWorkspace, request.path);
const rel = path.relative(resolvedWorkspace, resolvedPath);
if (rel === "" || (!rel.startsWith("..") && !path.isAbsolute(rel))) {
return true;
}
}This correctly:
Tests updated to cover both cases (traversal rejection + relative path approval). |
|
Thanks for the fix! 🎯 This PR looks great — it directly addresses the permission scoping regression that was killing daily workflows (Daily Testify, Linter Miner). The fix is: ✅ Focused — workspace-root early-return added cleanly, with matching test coverage The change is minimal and targeted: if the requested path is under
|
|
🎉 This pull request is included in a new release. Release: |
Workflows with any tool restriction but no explicit
readgrant hadread($GITHUB_WORKSPACE)silently denied by the SDK permission handler, exhausting the 3-denial cap and aborting the run. Reproduced identically in two unrelated scheduled workflows (Daily Testify, Linter Miner) within 30 minutes on 2026-08-02.Root cause
buildCopilotSDKPermissionHandlerincopilot_sdk_permissions.cjsonly approvedreadrequests when either an explicitreadgrant was present,shellwildcard was granted, or the path matched patterns derived from specificcat/ls/xargsshell rules. Workflows using only MCP tools (e.g.,github) or a restricted bash allowlist had no path to approve reads of their own checkout.Changes
copilot_sdk_permissions.cjs— Add a workspace-root early-return at the top ofcase "read"inisAllowed(): ifworkspaceRootis set and the requested path is at or under it, approve unconditionally. Paths outsideGITHUB_WORKSPACEare unaffected.copilot_sdk_driver.test.cjs— Add regression test reproducing the exact failing scenario (github MCP + restricted bash, no read grant → workspace reads must be approved). Update two existing tests whose assertions reflected the now-corrected behavior.Run details: https://github.com/github/gh-aw/actions/runs/30766084787