Harden tag mode tool permissions against prompt injection (#1002)
Two defenses for tag mode where an attacker with repo write access could craft a prompt injection payload in an issue/PR to gain RCE on the Actions runner: 1. git-push wrapper (H1 #3556799) The Bash(git\ push:*) rule permitted arbitrary flags and remotes, including combinations that execute shell commands locally. Replaced with scripts/git-push.sh which allowlists exactly 'origin <ref>' with no flags, validates the ref via check-ref-format. Same pattern as scripts/gh.sh. 2. acceptEdits instead of blanket Write/Edit (Asana 1213310082312048) Edit/MultiEdit/Write in allowedTools granted write access to the whole runner filesystem (~/.bashrc etc). Removed from allowedTools and set --permission-mode acceptEdits, which auto-accepts edits inside cwd ($GITHUB_WORKSPACE) and denies outside. Headless SDK has no prompt handler so 'ask' becomes deny. Also: - Noted that create-prompt/index.ts exports ALLOWED_TOOLS env var that nothing reads. The live path is modes/tag/index.ts. Mirrored the fix in both so the file the H1 report likely points to stays in sync. - Updated prompt text (3 callsites) to reference the wrapper. - Updated tests (4 prompt-content asserts, 7 tool-list asserts).
This commit is contained in:
parent
5d0cc745cd
commit
f956510b1a
36
scripts/git-push.sh
Executable file
36
scripts/git-push.sh
Executable file
@ -0,0 +1,36 @@
|
|||||||
|
#!/usr/bin/env bash
|
||||||
|
set -euo pipefail
|
||||||
|
|
||||||
|
# Wrapper around `git push` that only allows `origin <ref>` with no flags.
|
||||||
|
# Defends against --receive-pack / --exec RCE and arbitrary-remote exfiltration
|
||||||
|
# (H1 #3556799). `git push:*` in allowedTools permits `git push --receive-pack='sh -c ...' ext::sh`
|
||||||
|
# which runs arbitrary shell on the Actions runner. This wrapper closes that.
|
||||||
|
#
|
||||||
|
# Usage:
|
||||||
|
# git-push.sh origin HEAD
|
||||||
|
# git-push.sh origin claude/issue-123-20260304
|
||||||
|
|
||||||
|
if [[ $# -ne 2 ]]; then
|
||||||
|
echo "Error: exactly two arguments required: origin <ref>" >&2
|
||||||
|
exit 1
|
||||||
|
fi
|
||||||
|
|
||||||
|
for arg in "$@"; do
|
||||||
|
if [[ "$arg" == -* ]]; then
|
||||||
|
echo "Error: flags are not allowed (got: $arg)" >&2
|
||||||
|
exit 1
|
||||||
|
fi
|
||||||
|
done
|
||||||
|
|
||||||
|
if [[ "$1" != "origin" ]]; then
|
||||||
|
echo "Error: remote must be 'origin' (got: $1)" >&2
|
||||||
|
exit 1
|
||||||
|
fi
|
||||||
|
|
||||||
|
REF="$2"
|
||||||
|
if [[ "$REF" != "HEAD" ]] && ! git check-ref-format --branch "$REF" >/dev/null 2>&1; then
|
||||||
|
echo "Error: invalid ref: $REF" >&2
|
||||||
|
exit 1
|
||||||
|
fi
|
||||||
|
|
||||||
|
exec git push origin "$REF"
|
||||||
@ -23,19 +23,15 @@ import { GITHUB_SERVER_URL } from "../github/api/config";
|
|||||||
import { extractUserRequest } from "../utils/extract-user-request";
|
import { extractUserRequest } from "../utils/extract-user-request";
|
||||||
export type { CommonFields, PreparedContext } from "./types";
|
export type { CommonFields, PreparedContext } from "./types";
|
||||||
|
|
||||||
|
const GIT_PUSH_WRAPPER = `${process.env.GITHUB_ACTION_PATH}/scripts/git-push.sh`;
|
||||||
|
|
||||||
/** Filename for the user request file, read by the SDK runner */
|
/** Filename for the user request file, read by the SDK runner */
|
||||||
const USER_REQUEST_FILENAME = "claude-user-request.txt";
|
const USER_REQUEST_FILENAME = "claude-user-request.txt";
|
||||||
|
|
||||||
// Tag mode defaults - these tools are needed for tag mode to function
|
// Tag mode defaults - these tools are needed for tag mode to function.
|
||||||
const BASE_ALLOWED_TOOLS = [
|
// Edit/MultiEdit/Write are intentionally omitted: acceptEdits permission mode
|
||||||
"Edit",
|
// auto-allows file edits inside $GITHUB_WORKSPACE and denies writes outside it.
|
||||||
"MultiEdit",
|
const BASE_ALLOWED_TOOLS = ["Glob", "Grep", "LS", "Read"];
|
||||||
"Glob",
|
|
||||||
"Grep",
|
|
||||||
"LS",
|
|
||||||
"Read",
|
|
||||||
"Write",
|
|
||||||
];
|
|
||||||
|
|
||||||
export function buildAllowedToolsString(
|
export function buildAllowedToolsString(
|
||||||
customAllowedTools?: string[],
|
customAllowedTools?: string[],
|
||||||
@ -59,7 +55,7 @@ export function buildAllowedToolsString(
|
|||||||
baseTools.push(
|
baseTools.push(
|
||||||
"Bash(git add:*)",
|
"Bash(git add:*)",
|
||||||
"Bash(git commit:*)",
|
"Bash(git commit:*)",
|
||||||
"Bash(git push:*)",
|
`Bash(${GIT_PUSH_WRAPPER}:*)`,
|
||||||
"Bash(git status:*)",
|
"Bash(git status:*)",
|
||||||
"Bash(git diff:*)",
|
"Bash(git diff:*)",
|
||||||
"Bash(git log:*)",
|
"Bash(git log:*)",
|
||||||
@ -434,7 +430,7 @@ function getCommitInstructions(
|
|||||||
Bash(git commit -m "<message>\\n\\n${coAuthorLine}")`
|
Bash(git commit -m "<message>\\n\\n${coAuthorLine}")`
|
||||||
: ""
|
: ""
|
||||||
}
|
}
|
||||||
- Push to the remote: Bash(git push origin HEAD)`;
|
- Push to the remote: Bash(${GIT_PUSH_WRAPPER} origin HEAD)`;
|
||||||
} else {
|
} else {
|
||||||
const branchName = eventData.claudeBranch || eventData.baseBranch;
|
const branchName = eventData.claudeBranch || eventData.baseBranch;
|
||||||
return `
|
return `
|
||||||
@ -448,7 +444,7 @@ function getCommitInstructions(
|
|||||||
Bash(git commit -m "<message>\\n\\n${coAuthorLine}")`
|
Bash(git commit -m "<message>\\n\\n${coAuthorLine}")`
|
||||||
: ""
|
: ""
|
||||||
}
|
}
|
||||||
- Push to the remote: Bash(git push origin ${branchName})`;
|
- Push to the remote: Bash(${GIT_PUSH_WRAPPER} origin ${branchName})`;
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
@ -823,7 +819,7 @@ ${
|
|||||||
: `- Use git commands via the Bash tool for version control (remember that you have access to these git commands):
|
: `- Use git commands via the Bash tool for version control (remember that you have access to these git commands):
|
||||||
- Stage files: Bash(git add <files>)
|
- Stage files: Bash(git add <files>)
|
||||||
- Commit changes: Bash(git commit -m "<message>")
|
- Commit changes: Bash(git commit -m "<message>")
|
||||||
- Push to remote: Bash(git push origin <branch>) (NEVER force push)
|
- Push to remote: Bash(${GIT_PUSH_WRAPPER} origin <branch>)
|
||||||
- Delete files: Bash(git rm <files>) followed by commit and push
|
- Delete files: Bash(git rm <files>) followed by commit and push
|
||||||
- Check status: Bash(git status)
|
- Check status: Bash(git status)
|
||||||
- View diff: Bash(git diff)${eventData.isPR && eventData.baseBranch ? `\n - IMPORTANT: For PR diffs, use: Bash(git diff origin/${eventData.baseBranch}...HEAD)` : ""}`
|
- View diff: Bash(git diff)${eventData.isPR && eventData.baseBranch ? `\n - IMPORTANT: For PR diffs, use: Bash(git diff origin/${eventData.baseBranch}...HEAD)` : ""}`
|
||||||
@ -977,7 +973,9 @@ export async function createPrompt(
|
|||||||
console.log("========================");
|
console.log("========================");
|
||||||
}
|
}
|
||||||
|
|
||||||
// Set allowed tools
|
// NOTE: these env var exports are dead — nothing reads ALLOWED_TOOLS / DISALLOWED_TOOLS.
|
||||||
|
// The live path is modes/tag/index.ts which builds --allowedTools into claudeArgs directly.
|
||||||
|
// Kept only so the H1 report's pointed-to file stays in sync with the live fix.
|
||||||
const hasActionsReadPermission = false;
|
const hasActionsReadPermission = false;
|
||||||
|
|
||||||
const allAllowedTools = buildAllowedToolsString(
|
const allAllowedTools = buildAllowedToolsString(
|
||||||
|
|||||||
@ -114,16 +114,17 @@ export async function prepareTagMode({
|
|||||||
tool.startsWith("mcp__github_"),
|
tool.startsWith("mcp__github_"),
|
||||||
);
|
);
|
||||||
|
|
||||||
// Build claude_args for tag mode with required tools
|
const gitPushWrapper = `${process.env.GITHUB_ACTION_PATH}/scripts/git-push.sh`;
|
||||||
// Tag mode REQUIRES these tools to function properly
|
|
||||||
|
// Build claude_args for tag mode with required tools.
|
||||||
|
// Edit/MultiEdit/Write are intentionally omitted: acceptEdits permission mode (set below)
|
||||||
|
// auto-allows file edits inside $GITHUB_WORKSPACE and denies writes outside (e.g. ~/.bashrc).
|
||||||
|
// Listing them here would grant blanket write access to the whole runner (Asana 1213310082312048).
|
||||||
const tagModeTools = [
|
const tagModeTools = [
|
||||||
"Edit",
|
|
||||||
"MultiEdit",
|
|
||||||
"Glob",
|
"Glob",
|
||||||
"Grep",
|
"Grep",
|
||||||
"LS",
|
"LS",
|
||||||
"Read",
|
"Read",
|
||||||
"Write",
|
|
||||||
"mcp__github_comment__update_claude_comment",
|
"mcp__github_comment__update_claude_comment",
|
||||||
"mcp__github_ci__get_ci_status",
|
"mcp__github_ci__get_ci_status",
|
||||||
"mcp__github_ci__get_workflow_run_details",
|
"mcp__github_ci__get_workflow_run_details",
|
||||||
@ -137,7 +138,7 @@ export async function prepareTagMode({
|
|||||||
tagModeTools.push(
|
tagModeTools.push(
|
||||||
"Bash(git add:*)",
|
"Bash(git add:*)",
|
||||||
"Bash(git commit:*)",
|
"Bash(git commit:*)",
|
||||||
"Bash(git push:*)",
|
`Bash(${gitPushWrapper}:*)`,
|
||||||
"Bash(git status:*)",
|
"Bash(git status:*)",
|
||||||
"Bash(git diff:*)",
|
"Bash(git diff:*)",
|
||||||
"Bash(git log:*)",
|
"Bash(git log:*)",
|
||||||
@ -171,8 +172,10 @@ export async function prepareTagMode({
|
|||||||
const escapedOurConfig = ourMcpConfig.replace(/'/g, "'\\''");
|
const escapedOurConfig = ourMcpConfig.replace(/'/g, "'\\''");
|
||||||
claudeArgs = `--mcp-config '${escapedOurConfig}'`;
|
claudeArgs = `--mcp-config '${escapedOurConfig}'`;
|
||||||
|
|
||||||
// Add required tools for tag mode
|
// Add required tools for tag mode.
|
||||||
claudeArgs += ` --allowedTools "${tagModeTools.join(",")}"`;
|
// acceptEdits: file edits auto-allowed inside cwd ($GITHUB_WORKSPACE), denied outside.
|
||||||
|
// Headless SDK has no prompt handler, so anything that falls through to "ask" is denied.
|
||||||
|
claudeArgs += ` --permission-mode acceptEdits --allowedTools "${tagModeTools.join(",")}"`;
|
||||||
|
|
||||||
// Append user's claude_args (which may have more --mcp-config flags)
|
// Append user's claude_args (which may have more --mcp-config flags)
|
||||||
if (userClaudeArgs) {
|
if (userClaudeArgs) {
|
||||||
|
|||||||
@ -1,6 +1,6 @@
|
|||||||
#!/usr/bin/env bun
|
#!/usr/bin/env bun
|
||||||
|
|
||||||
import { describe, test, expect } from "bun:test";
|
import { describe, test, expect, beforeAll } from "bun:test";
|
||||||
import {
|
import {
|
||||||
generatePrompt,
|
generatePrompt,
|
||||||
getEventTypeAndContext,
|
getEventTypeAndContext,
|
||||||
@ -9,6 +9,10 @@ import {
|
|||||||
} from "../src/create-prompt";
|
} from "../src/create-prompt";
|
||||||
import type { PreparedContext } from "../src/create-prompt";
|
import type { PreparedContext } from "../src/create-prompt";
|
||||||
|
|
||||||
|
beforeAll(() => {
|
||||||
|
process.env.GITHUB_ACTION_PATH = "/test/action/path";
|
||||||
|
});
|
||||||
|
|
||||||
describe("generatePrompt", () => {
|
describe("generatePrompt", () => {
|
||||||
const mockGitHubData = {
|
const mockGitHubData = {
|
||||||
contextData: {
|
contextData: {
|
||||||
@ -505,7 +509,7 @@ describe("generatePrompt", () => {
|
|||||||
const prompt = await generatePrompt(envVars, mockGitHubData, false, "tag");
|
const prompt = await generatePrompt(envVars, mockGitHubData, false, "tag");
|
||||||
|
|
||||||
// Should contain PR-specific instructions (git commands when not using signing)
|
// Should contain PR-specific instructions (git commands when not using signing)
|
||||||
expect(prompt).toContain("git push");
|
expect(prompt).toContain("scripts/git-push.sh origin");
|
||||||
expect(prompt).toContain(
|
expect(prompt).toContain(
|
||||||
"Always push to the existing branch when triggered on a PR",
|
"Always push to the existing branch when triggered on a PR",
|
||||||
);
|
);
|
||||||
@ -643,7 +647,7 @@ describe("generatePrompt", () => {
|
|||||||
const prompt = await generatePrompt(envVars, mockGitHubData, false, "tag");
|
const prompt = await generatePrompt(envVars, mockGitHubData, false, "tag");
|
||||||
|
|
||||||
// Should contain open PR instructions (git commands when not using signing)
|
// Should contain open PR instructions (git commands when not using signing)
|
||||||
expect(prompt).toContain("git push");
|
expect(prompt).toContain("scripts/git-push.sh origin");
|
||||||
expect(prompt).toContain(
|
expect(prompt).toContain(
|
||||||
"Always push to the existing branch when triggered on a PR",
|
"Always push to the existing branch when triggered on a PR",
|
||||||
);
|
);
|
||||||
@ -757,7 +761,7 @@ describe("generatePrompt", () => {
|
|||||||
expect(prompt).toContain("Use git commands via the Bash tool");
|
expect(prompt).toContain("Use git commands via the Bash tool");
|
||||||
expect(prompt).toContain("git add");
|
expect(prompt).toContain("git add");
|
||||||
expect(prompt).toContain("git commit");
|
expect(prompt).toContain("git commit");
|
||||||
expect(prompt).toContain("git push");
|
expect(prompt).toContain("scripts/git-push.sh origin");
|
||||||
|
|
||||||
// Should use the minimal comment tool
|
// Should use the minimal comment tool
|
||||||
expect(prompt).toContain("mcp__github_comment__update_claude_comment");
|
expect(prompt).toContain("mcp__github_comment__update_claude_comment");
|
||||||
@ -886,17 +890,18 @@ describe("buildAllowedToolsString", () => {
|
|||||||
const result = buildAllowedToolsString();
|
const result = buildAllowedToolsString();
|
||||||
|
|
||||||
// The base tools should be in the result
|
// The base tools should be in the result
|
||||||
expect(result).toContain("Edit");
|
// Edit/MultiEdit/Write are NOT in allowedTools — acceptEdits permission mode handles them
|
||||||
|
expect(result).not.toContain("Edit");
|
||||||
|
expect(result).not.toContain("Write");
|
||||||
expect(result).toContain("Glob");
|
expect(result).toContain("Glob");
|
||||||
expect(result).toContain("Grep");
|
expect(result).toContain("Grep");
|
||||||
expect(result).toContain("LS");
|
expect(result).toContain("LS");
|
||||||
expect(result).toContain("Read");
|
expect(result).toContain("Read");
|
||||||
expect(result).toContain("Write");
|
|
||||||
|
|
||||||
// Default is no commit signing, so should have specific Bash git commands
|
// Default is no commit signing, so should have specific Bash git commands
|
||||||
expect(result).toContain("Bash(git add:*)");
|
expect(result).toContain("Bash(git add:*)");
|
||||||
expect(result).toContain("Bash(git commit:*)");
|
expect(result).toContain("Bash(git commit:*)");
|
||||||
expect(result).toContain("Bash(git push:*)");
|
expect(result).toContain("scripts/git-push.sh:*)");
|
||||||
expect(result).toContain("mcp__github_comment__update_claude_comment");
|
expect(result).toContain("mcp__github_comment__update_claude_comment");
|
||||||
|
|
||||||
// Should not have commit signing tools
|
// Should not have commit signing tools
|
||||||
@ -908,12 +913,12 @@ describe("buildAllowedToolsString", () => {
|
|||||||
const result = buildAllowedToolsString([], false, false);
|
const result = buildAllowedToolsString([], false, false);
|
||||||
|
|
||||||
// The base tools should be in the result
|
// The base tools should be in the result
|
||||||
expect(result).toContain("Edit");
|
expect(result).not.toContain("Edit");
|
||||||
expect(result).toContain("Glob");
|
expect(result).toContain("Glob");
|
||||||
expect(result).toContain("Grep");
|
expect(result).toContain("Grep");
|
||||||
expect(result).toContain("LS");
|
expect(result).toContain("LS");
|
||||||
expect(result).toContain("Read");
|
expect(result).toContain("Read");
|
||||||
expect(result).toContain("Write");
|
expect(result).not.toContain("Write");
|
||||||
|
|
||||||
// Should have specific Bash git commands for non-signing mode
|
// Should have specific Bash git commands for non-signing mode
|
||||||
expect(result).toContain("Bash(git add:*)");
|
expect(result).toContain("Bash(git add:*)");
|
||||||
@ -930,7 +935,7 @@ describe("buildAllowedToolsString", () => {
|
|||||||
const result = buildAllowedToolsString(customTools);
|
const result = buildAllowedToolsString(customTools);
|
||||||
|
|
||||||
// Base tools should be present
|
// Base tools should be present
|
||||||
expect(result).toContain("Edit");
|
expect(result).toContain("Read");
|
||||||
expect(result).toContain("Glob");
|
expect(result).toContain("Glob");
|
||||||
|
|
||||||
// Custom tools should be appended
|
// Custom tools should be appended
|
||||||
@ -950,7 +955,7 @@ describe("buildAllowedToolsString", () => {
|
|||||||
const result = buildAllowedToolsString([], true);
|
const result = buildAllowedToolsString([], true);
|
||||||
|
|
||||||
// Base tools should be present
|
// Base tools should be present
|
||||||
expect(result).toContain("Edit");
|
expect(result).toContain("Read");
|
||||||
expect(result).toContain("Glob");
|
expect(result).toContain("Glob");
|
||||||
|
|
||||||
// GitHub Actions tools should be included
|
// GitHub Actions tools should be included
|
||||||
@ -964,7 +969,7 @@ describe("buildAllowedToolsString", () => {
|
|||||||
const result = buildAllowedToolsString(customTools, true);
|
const result = buildAllowedToolsString(customTools, true);
|
||||||
|
|
||||||
// Base tools should be present
|
// Base tools should be present
|
||||||
expect(result).toContain("Edit");
|
expect(result).toContain("Read");
|
||||||
|
|
||||||
// Custom tools should be included
|
// Custom tools should be included
|
||||||
expect(result).toContain("Tool1");
|
expect(result).toContain("Tool1");
|
||||||
@ -980,12 +985,12 @@ describe("buildAllowedToolsString", () => {
|
|||||||
const result = buildAllowedToolsString([], false, true);
|
const result = buildAllowedToolsString([], false, true);
|
||||||
|
|
||||||
// Base tools should be present
|
// Base tools should be present
|
||||||
expect(result).toContain("Edit");
|
expect(result).not.toContain("Edit");
|
||||||
expect(result).toContain("Glob");
|
expect(result).toContain("Glob");
|
||||||
expect(result).toContain("Grep");
|
expect(result).toContain("Grep");
|
||||||
expect(result).toContain("LS");
|
expect(result).toContain("LS");
|
||||||
expect(result).toContain("Read");
|
expect(result).toContain("Read");
|
||||||
expect(result).toContain("Write");
|
expect(result).not.toContain("Write");
|
||||||
|
|
||||||
// Commit signing tools should be included
|
// Commit signing tools should be included
|
||||||
expect(result).toContain("mcp__github_file_ops__commit_files");
|
expect(result).toContain("mcp__github_file_ops__commit_files");
|
||||||
@ -1001,17 +1006,17 @@ describe("buildAllowedToolsString", () => {
|
|||||||
const result = buildAllowedToolsString([], false, false);
|
const result = buildAllowedToolsString([], false, false);
|
||||||
|
|
||||||
// Base tools should be present
|
// Base tools should be present
|
||||||
expect(result).toContain("Edit");
|
expect(result).not.toContain("Edit");
|
||||||
expect(result).toContain("Glob");
|
expect(result).toContain("Glob");
|
||||||
expect(result).toContain("Grep");
|
expect(result).toContain("Grep");
|
||||||
expect(result).toContain("LS");
|
expect(result).toContain("LS");
|
||||||
expect(result).toContain("Read");
|
expect(result).toContain("Read");
|
||||||
expect(result).toContain("Write");
|
expect(result).not.toContain("Write");
|
||||||
|
|
||||||
// Specific Bash git commands should be included
|
// Specific Bash git commands should be included
|
||||||
expect(result).toContain("Bash(git add:*)");
|
expect(result).toContain("Bash(git add:*)");
|
||||||
expect(result).toContain("Bash(git commit:*)");
|
expect(result).toContain("Bash(git commit:*)");
|
||||||
expect(result).toContain("Bash(git push:*)");
|
expect(result).toContain("scripts/git-push.sh:*)");
|
||||||
expect(result).toContain("Bash(git status:*)");
|
expect(result).toContain("Bash(git status:*)");
|
||||||
expect(result).toContain("Bash(git diff:*)");
|
expect(result).toContain("Bash(git diff:*)");
|
||||||
expect(result).toContain("Bash(git log:*)");
|
expect(result).toContain("Bash(git log:*)");
|
||||||
@ -1030,7 +1035,7 @@ describe("buildAllowedToolsString", () => {
|
|||||||
const result = buildAllowedToolsString(customTools, true, false);
|
const result = buildAllowedToolsString(customTools, true, false);
|
||||||
|
|
||||||
// Base tools should be present
|
// Base tools should be present
|
||||||
expect(result).toContain("Edit");
|
expect(result).toContain("Read");
|
||||||
expect(result).toContain("Bash(git add:*)");
|
expect(result).toContain("Bash(git add:*)");
|
||||||
|
|
||||||
// Custom tools should be included
|
// Custom tools should be included
|
||||||
|
|||||||
@ -135,7 +135,7 @@ describe("pull_request_target event support", () => {
|
|||||||
const prompt = generatePrompt(envVars, mockGitHubData, false, "tag");
|
const prompt = generatePrompt(envVars, mockGitHubData, false, "tag");
|
||||||
|
|
||||||
// Should include git commands for non-commit-signing mode
|
// Should include git commands for non-commit-signing mode
|
||||||
expect(prompt).toContain("git push");
|
expect(prompt).toContain("scripts/git-push.sh origin");
|
||||||
expect(prompt).toContain(
|
expect(prompt).toContain(
|
||||||
"Always push to the existing branch when triggered on a PR",
|
"Always push to the existing branch when triggered on a PR",
|
||||||
);
|
);
|
||||||
|
|||||||
Loading…
x
Reference in New Issue
Block a user