harden sandbox escape vectors for bash disabled/restricted modes (#257)
* harden sandbox escape vectors for bash disabled/restricted modes block git config injection (-c flag as subcommand), dangerous subcommands (config, submodule, rebase, bisect), code-executing arg flags (--exec, --extcmd), .gitattributes/.gitmodules writes, and package lifecycle scripts. add retry logic to test runner for transient failures. add security unit tests and adhoc attack tests. Co-authored-by: Cursor <cursoragent@cursor.com> * only filter subcommands in nobash, remove nobash from ui * use regex matching * iterate on tests * simplify githooks --------- Co-authored-by: Cursor <cursoragent@cursor.com>
This commit is contained in:
committed by
pullfrog[bot]
parent
f37d02b292
commit
bc28c658f2
+51
-44
@@ -1,5 +1,6 @@
|
||||
import { Octokit } from "@octokit/rest";
|
||||
import { describe, expect, it } from "vitest";
|
||||
import { acquireNewToken } from "../utils/github.ts";
|
||||
import { fetchAndFormatPrDiff } from "./checkout.ts";
|
||||
|
||||
/**
|
||||
@@ -20,50 +21,56 @@ function parseTocEntries(toc: string) {
|
||||
return entries;
|
||||
}
|
||||
|
||||
async function getToken(): Promise<string> {
|
||||
// prefer explicit GH_TOKEN, fall back to acquiring one via GitHub App credentials
|
||||
if (process.env.GH_TOKEN) return process.env.GH_TOKEN;
|
||||
return await acquireNewToken();
|
||||
}
|
||||
|
||||
describe("fetchAndFormatPrDiff", () => {
|
||||
it("generates accurate TOC line numbers for pullfrog/test-repo#1", async () => {
|
||||
const token = process.env.GH_TOKEN;
|
||||
if (!token) {
|
||||
throw new Error("GH_TOKEN not set in .env");
|
||||
it(
|
||||
"generates accurate TOC line numbers for pullfrog/test-repo#1",
|
||||
{ timeout: 30000 },
|
||||
async () => {
|
||||
const token = await getToken();
|
||||
const octokit = new Octokit({ auth: token });
|
||||
const result = await fetchAndFormatPrDiff({
|
||||
octokit,
|
||||
owner: "pullfrog",
|
||||
repo: "test-repo",
|
||||
pullNumber: 1,
|
||||
});
|
||||
|
||||
// verify content includes TOC at the start
|
||||
expect(result.content.startsWith(result.toc)).toBe(true);
|
||||
|
||||
// parse TOC and validate every entry's line numbers against actual content
|
||||
const contentLines = result.content.split("\n");
|
||||
const tocEntries = parseTocEntries(result.toc);
|
||||
expect(tocEntries.length).toBeGreaterThan(0);
|
||||
|
||||
for (const entry of tocEntries) {
|
||||
// line numbers are 1-indexed, arrays are 0-indexed
|
||||
const firstLine = contentLines[entry.startLine - 1];
|
||||
expect(firstLine).toBeDefined();
|
||||
// first line of each file section should be the diff header
|
||||
expect(firstLine).toBe(`diff --git a/${entry.filename} b/${entry.filename}`);
|
||||
|
||||
// endLine should be within bounds
|
||||
expect(entry.endLine).toBeLessThanOrEqual(contentLines.length);
|
||||
}
|
||||
|
||||
// verify adjacent files don't overlap and are contiguous
|
||||
for (let i = 1; i < tocEntries.length; i++) {
|
||||
const prev = tocEntries[i - 1];
|
||||
const curr = tocEntries[i];
|
||||
// current file starts right after previous file ends
|
||||
expect(curr.startLine).toBe(prev.endLine + 1);
|
||||
}
|
||||
|
||||
// snapshot the full output for regression detection
|
||||
expect(result.toc).toMatchSnapshot("toc");
|
||||
expect(result.content).toMatchSnapshot("content");
|
||||
}
|
||||
|
||||
const octokit = new Octokit({ auth: token });
|
||||
const result = await fetchAndFormatPrDiff({
|
||||
octokit,
|
||||
owner: "pullfrog",
|
||||
repo: "test-repo",
|
||||
pullNumber: 1,
|
||||
});
|
||||
|
||||
// verify content includes TOC at the start
|
||||
expect(result.content.startsWith(result.toc)).toBe(true);
|
||||
|
||||
// parse TOC and validate every entry's line numbers against actual content
|
||||
const contentLines = result.content.split("\n");
|
||||
const tocEntries = parseTocEntries(result.toc);
|
||||
expect(tocEntries.length).toBeGreaterThan(0);
|
||||
|
||||
for (const entry of tocEntries) {
|
||||
// line numbers are 1-indexed, arrays are 0-indexed
|
||||
const firstLine = contentLines[entry.startLine - 1];
|
||||
expect(firstLine).toBeDefined();
|
||||
// first line of each file section should be the diff header
|
||||
expect(firstLine).toBe(`diff --git a/${entry.filename} b/${entry.filename}`);
|
||||
|
||||
// endLine should be within bounds
|
||||
expect(entry.endLine).toBeLessThanOrEqual(contentLines.length);
|
||||
}
|
||||
|
||||
// verify adjacent files don't overlap and are contiguous
|
||||
for (let i = 1; i < tocEntries.length; i++) {
|
||||
const prev = tocEntries[i - 1];
|
||||
const curr = tocEntries[i];
|
||||
// current file starts right after previous file ends
|
||||
expect(curr.startLine).toBe(prev.endLine + 1);
|
||||
}
|
||||
|
||||
// snapshot the full output for regression detection
|
||||
expect(result.toc).toMatchSnapshot("toc");
|
||||
expect(result.content).toMatchSnapshot("content");
|
||||
});
|
||||
);
|
||||
});
|
||||
|
||||
+11
-5
@@ -182,7 +182,7 @@ export async function checkoutPrBranch(
|
||||
pullNumber: number,
|
||||
params: CheckoutPrBranchParams
|
||||
): Promise<CheckoutPrBranchResult> {
|
||||
const { octokit, owner, name, gitToken, toolState, restricted } = params;
|
||||
const { octokit, owner, name, gitToken, toolState, bash } = params;
|
||||
log.info(`» checking out PR #${pullNumber}...`);
|
||||
|
||||
// fetch PR metadata
|
||||
@@ -215,7 +215,10 @@ export async function checkoutPrBranch(
|
||||
} else {
|
||||
// fetch base branch so origin/<base> exists for diff operations
|
||||
log.debug(`» fetching base branch (${baseBranch})...`);
|
||||
$git("fetch", ["--no-tags", "origin", baseBranch], { token: gitToken, restricted });
|
||||
$git("fetch", ["--no-tags", "origin", baseBranch], {
|
||||
token: gitToken,
|
||||
restricted: bash !== "enabled",
|
||||
});
|
||||
|
||||
// checkout base branch first to avoid "refusing to fetch into current branch" error
|
||||
// -B creates or resets the branch to match origin/baseBranch
|
||||
@@ -225,7 +228,7 @@ export async function checkoutPrBranch(
|
||||
log.debug(`» fetching PR #${pullNumber} (${localBranch})...`);
|
||||
$git("fetch", ["--no-tags", "origin", `pull/${pullNumber}/head:${localBranch}`], {
|
||||
token: gitToken,
|
||||
restricted,
|
||||
restricted: bash !== "enabled",
|
||||
});
|
||||
|
||||
// checkout the branch
|
||||
@@ -237,7 +240,10 @@ export async function checkoutPrBranch(
|
||||
// fetch if we skipped checkout (already on branch) - otherwise already fetched above
|
||||
if (alreadyOnBranch) {
|
||||
log.debug(`» fetching base branch (${baseBranch})...`);
|
||||
$git("fetch", ["--no-tags", "origin", baseBranch], { token: gitToken, restricted });
|
||||
$git("fetch", ["--no-tags", "origin", baseBranch], {
|
||||
token: gitToken,
|
||||
restricted: bash !== "enabled",
|
||||
});
|
||||
}
|
||||
|
||||
// configure push remote for this branch
|
||||
@@ -310,7 +316,7 @@ export function CheckoutPrTool(ctx: ToolContext) {
|
||||
name: ctx.repo.name,
|
||||
gitToken: ctx.gitToken,
|
||||
toolState: ctx.toolState,
|
||||
restricted: ctx.payload.bash === "restricted",
|
||||
bash: ctx.payload.bash,
|
||||
postCheckoutScript: ctx.postCheckoutScript,
|
||||
});
|
||||
|
||||
|
||||
+8
-2
@@ -1,5 +1,5 @@
|
||||
import { type } from "arktype";
|
||||
import type { PrepResult } from "../prep/index.ts";
|
||||
import type { PrepOptions, PrepResult } from "../prep/index.ts";
|
||||
import { runPrepPhase } from "../prep/index.ts";
|
||||
import type { ToolContext } from "./server.ts";
|
||||
import { execute, tool } from "./shared.ts";
|
||||
@@ -75,8 +75,14 @@ function startInstallation(ctx: ToolContext): void {
|
||||
return;
|
||||
}
|
||||
|
||||
// SECURITY: when bash is disabled, suppress lifecycle scripts to prevent
|
||||
// agents from using package.json scripts as a backdoor for code execution
|
||||
const prepOptions: PrepOptions = {
|
||||
ignoreScripts: ctx.payload.bash === "disabled",
|
||||
};
|
||||
|
||||
// initialize state and start installation
|
||||
const promise = runPrepPhase();
|
||||
const promise = runPrepPhase(prepOptions);
|
||||
ctx.toolState.dependencyInstallation = {
|
||||
status: "in_progress",
|
||||
promise,
|
||||
|
||||
+43
-9
@@ -91,13 +91,38 @@ function resolveAndValidatePath(filePath: string): string {
|
||||
return resolved;
|
||||
}
|
||||
|
||||
function validateWritePath(filePath: string): string {
|
||||
const resolved = resolveAndValidatePath(filePath);
|
||||
// SECURITY: files that git interprets and can trigger code execution.
|
||||
// .gitattributes can define filter drivers (clean/smudge) that execute arbitrary commands.
|
||||
// .gitmodules can reference malicious submodule URLs that execute code on update.
|
||||
// only blocked when bash is disabled — in restricted mode the agent already has bash
|
||||
// and could write these files via shell, so blocking via MCP is redundant.
|
||||
const GIT_INTERPRETED_FILES = [".gitattributes", ".gitmodules"];
|
||||
|
||||
type BashPermission = "disabled" | "restricted" | "enabled";
|
||||
|
||||
type ValidateWritePathParams = {
|
||||
filePath: string;
|
||||
bashPermission: BashPermission;
|
||||
};
|
||||
|
||||
function validateWritePath(params: ValidateWritePathParams): string {
|
||||
const resolved = resolveAndValidatePath(params.filePath);
|
||||
const cwd = realpathSync(process.cwd());
|
||||
const relative = resolved.slice(cwd.length + 1);
|
||||
if (relative === ".git" || relative.startsWith(".git/")) {
|
||||
throw new Error(`writing to .git is not allowed: ${filePath}`);
|
||||
throw new Error(`writing to .git is not allowed: ${params.filePath}`);
|
||||
}
|
||||
|
||||
// block git-interpreted files only when bash is disabled (no shell = no other way to create them)
|
||||
if (params.bashPermission === "disabled") {
|
||||
const basename = relative.split("/").pop() || "";
|
||||
if (GIT_INTERPRETED_FILES.includes(basename)) {
|
||||
throw new Error(
|
||||
`writing to ${basename} is not allowed when bash is ${params.bashPermission} (can trigger code execution via git filter drivers): ${params.filePath}`
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
return resolved;
|
||||
}
|
||||
|
||||
@@ -128,13 +153,16 @@ export function FileReadTool(_ctx: ToolContext) {
|
||||
});
|
||||
}
|
||||
|
||||
export function FileWriteTool(_ctx: ToolContext) {
|
||||
export function FileWriteTool(ctx: ToolContext) {
|
||||
return tool({
|
||||
name: "file_write",
|
||||
description: `Write content to a file in the repository. Path is relative to the repository root. Only paths within the current repository are allowed. Writes to .git/ are blocked. Creates parent directories if needed.`,
|
||||
parameters: FileWriteParams,
|
||||
execute: execute(async (params) => {
|
||||
const resolved = validateWritePath(params.path);
|
||||
const resolved = validateWritePath({
|
||||
filePath: params.path,
|
||||
bashPermission: ctx.payload.bash,
|
||||
});
|
||||
const dir = dirname(resolved);
|
||||
mkdirSync(dir, { recursive: true });
|
||||
writeFileSync(resolved, params.content, "utf-8");
|
||||
@@ -143,7 +171,7 @@ export function FileWriteTool(_ctx: ToolContext) {
|
||||
});
|
||||
}
|
||||
|
||||
export function FileEditTool(_ctx: ToolContext) {
|
||||
export function FileEditTool(ctx: ToolContext) {
|
||||
return tool({
|
||||
name: "file_edit",
|
||||
description: `Replace text in a file. old_string must match exactly (including whitespace and indentation). By default replaces a single unique occurrence — set replace_all to replace every occurrence. Path is relative to the repository root. Writes to .git/ are blocked.`,
|
||||
@@ -156,7 +184,10 @@ export function FileEditTool(_ctx: ToolContext) {
|
||||
throw new Error("old_string and new_string are identical");
|
||||
}
|
||||
|
||||
const resolved = validateWritePath(params.path);
|
||||
const resolved = validateWritePath({
|
||||
filePath: params.path,
|
||||
bashPermission: ctx.payload.bash,
|
||||
});
|
||||
const content = readFileSync(resolved, "utf-8");
|
||||
const count = content.split(params.old_string).length - 1;
|
||||
|
||||
@@ -179,13 +210,16 @@ export function FileEditTool(_ctx: ToolContext) {
|
||||
});
|
||||
}
|
||||
|
||||
export function FileDeleteTool(_ctx: ToolContext) {
|
||||
export function FileDeleteTool(ctx: ToolContext) {
|
||||
return tool({
|
||||
name: "file_delete",
|
||||
description: `Delete a file from the repository. Path is relative to the repository root. Only paths within the current repository are allowed. Deletes to .git/ are blocked. Cannot delete directories.`,
|
||||
parameters: FileDeleteParams,
|
||||
execute: execute(async (params) => {
|
||||
const resolved = validateWritePath(params.path);
|
||||
const resolved = validateWritePath({
|
||||
filePath: params.path,
|
||||
bashPermission: ctx.payload.bash,
|
||||
});
|
||||
unlinkSync(resolved);
|
||||
return { path: params.path, deleted: true };
|
||||
}),
|
||||
|
||||
+68
-6
@@ -1,3 +1,4 @@
|
||||
import { regex } from "arkregex";
|
||||
import { type } from "arktype";
|
||||
import { log } from "../utils/cli.ts";
|
||||
import { $git } from "../utils/gitAuth.ts";
|
||||
@@ -132,7 +133,7 @@ export function PushBranchTool(ctx: ToolContext) {
|
||||
}
|
||||
$git("push", pushArgs, {
|
||||
token: ctx.gitToken,
|
||||
restricted: ctx.payload.bash === "restricted",
|
||||
restricted: ctx.payload.bash !== "enabled",
|
||||
});
|
||||
|
||||
return {
|
||||
@@ -155,12 +156,51 @@ const AUTH_REQUIRED_REDIRECT: Record<string, string> = {
|
||||
clone: "Repository already cloned. Use checkout_pr for PR branches.",
|
||||
};
|
||||
|
||||
// SECURITY: subcommands blocked when bash is disabled.
|
||||
// in disabled mode the agent has NO shell access, so these subcommands are the
|
||||
// primary escape vectors for arbitrary code execution. in restricted mode the
|
||||
// agent already has bash in a stripped sandbox, so blocking these is redundant.
|
||||
const NOBASH_BLOCKED_SUBCOMMANDS: Record<string, string> = {
|
||||
config: "Blocked: git config can set up filter drivers or hooks that execute arbitrary code.",
|
||||
submodule:
|
||||
"Blocked: git submodule can reference malicious repositories and execute code on update.",
|
||||
"update-index":
|
||||
"Blocked: git update-index can modify index entries in ways that bypass file protections.",
|
||||
"filter-branch": "Blocked: git filter-branch executes arbitrary code on repository history.",
|
||||
replace: "Blocked: git replace can redirect object lookups.",
|
||||
// subcommands that accept --exec or similar flags for arbitrary code execution
|
||||
rebase: "Blocked: git rebase --exec can execute arbitrary shell commands.",
|
||||
bisect: "Blocked: git bisect run can execute arbitrary shell commands.",
|
||||
};
|
||||
|
||||
// SECURITY: subcommand-specific arg flags that execute code.
|
||||
// only blocked when bash is disabled — in restricted mode the agent already
|
||||
// has shell access in a stripped sandbox, so these provide no additional security.
|
||||
//
|
||||
// NOTE: global git flags like -c and --config-env are NOT included here
|
||||
// because they only work before the subcommand. in the MCP tool, the
|
||||
// subcommand is always first, so -c in args is parsed as a subcommand flag
|
||||
// (e.g., git log -c = combined diff format), not config injection.
|
||||
// the subcommand check (rejecting "-" prefix) already blocks that attack.
|
||||
//
|
||||
// matched as: arg === flag OR arg starts with flag + "="
|
||||
// (avoids false positives like --exclude matching --exec)
|
||||
const NOBASH_BLOCKED_ARGS = ["--exec", "--extcmd", "--upload-pack", "--receive-pack"];
|
||||
|
||||
// SECURITY: subcommand must match [a-z][a-z0-9-]* to reject flags passed as the subcommand.
|
||||
// this blocks injection of global git options like -c, -C, --exec-path, --config-env, etc.
|
||||
//
|
||||
// critical attack: git -c "alias.x=!evil-command" x
|
||||
// -> sets alias "x" to a shell command via -c config injection, then runs it
|
||||
// -> achieves arbitrary code execution even with bash=disabled
|
||||
const subcommandPattern = regex("^[a-z][a-z0-9-]*$");
|
||||
|
||||
const Git = type({
|
||||
subcommand: type.string.describe("Git subcommand (e.g., 'status', 'log', 'diff')"),
|
||||
subcommand: type(subcommandPattern).describe("Git subcommand (e.g., 'status', 'log', 'diff')"),
|
||||
args: type.string.array().describe("Additional arguments for the git command").optional(),
|
||||
});
|
||||
|
||||
export function GitTool(_ctx: ToolContext) {
|
||||
export function GitTool(ctx: ToolContext) {
|
||||
return tool({
|
||||
name: "git",
|
||||
description:
|
||||
@@ -175,6 +215,28 @@ export function GitTool(_ctx: ToolContext) {
|
||||
throw new Error(`git ${subcommand} requires authentication. ${redirect}`);
|
||||
}
|
||||
|
||||
// SECURITY: block dangerous subcommands when bash is disabled.
|
||||
// in restricted mode the agent has bash in a stripped sandbox, so blocking
|
||||
// these through the MCP tool is redundant (agent can do it via bash).
|
||||
if (ctx.payload.bash === "disabled") {
|
||||
const blocked = NOBASH_BLOCKED_SUBCOMMANDS[subcommand];
|
||||
if (blocked) {
|
||||
throw new Error(blocked);
|
||||
}
|
||||
|
||||
// block subcommand-specific flags that execute arbitrary code
|
||||
for (const arg of args) {
|
||||
const isBlocked = NOBASH_BLOCKED_ARGS.some(
|
||||
(flag) => arg === flag || arg.startsWith(flag + "=")
|
||||
);
|
||||
if (isBlocked) {
|
||||
throw new Error(
|
||||
`Blocked: '${arg}' flag can execute arbitrary code and is not allowed.`
|
||||
);
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
const output = $("git", [subcommand, ...args]);
|
||||
return { success: true, output };
|
||||
}),
|
||||
@@ -198,7 +260,7 @@ export function GitFetchTool(ctx: ToolContext) {
|
||||
}
|
||||
$git("fetch", fetchArgs, {
|
||||
token: ctx.gitToken,
|
||||
restricted: ctx.payload.bash === "restricted",
|
||||
restricted: ctx.payload.bash !== "enabled",
|
||||
});
|
||||
return { success: true, ref: params.ref };
|
||||
}),
|
||||
@@ -226,7 +288,7 @@ export function DeleteBranchTool(ctx: ToolContext) {
|
||||
|
||||
$git("push", ["origin", "--delete", params.branchName], {
|
||||
token: ctx.gitToken,
|
||||
restricted: ctx.payload.bash === "restricted",
|
||||
restricted: ctx.payload.bash !== "enabled",
|
||||
});
|
||||
return { success: true, deleted: params.branchName };
|
||||
}),
|
||||
@@ -256,7 +318,7 @@ export function PushTagsTool(ctx: ToolContext) {
|
||||
const pushArgs = [...(params.force ? ["-f"] : []), "origin", `refs/tags/${params.tag}`];
|
||||
$git("push", pushArgs, {
|
||||
token: ctx.gitToken,
|
||||
restricted: ctx.payload.bash === "restricted",
|
||||
restricted: ctx.payload.bash !== "enabled",
|
||||
});
|
||||
return { success: true, tag: params.tag };
|
||||
}),
|
||||
|
||||
@@ -1,5 +1,6 @@
|
||||
import { Octokit } from "@octokit/rest";
|
||||
import { describe, expect, it } from "vitest";
|
||||
import { acquireNewToken } from "../utils/github.ts";
|
||||
import {
|
||||
buildThreadBlocks,
|
||||
formatReviewThreads,
|
||||
@@ -10,13 +11,15 @@ import {
|
||||
type ReviewThreadsQueryResponse,
|
||||
} from "./reviewComments.ts";
|
||||
|
||||
describe("formatReviewThreads", () => {
|
||||
it("formats thread blocks with TOC and correct line numbers", async () => {
|
||||
const token = process.env.GH_TOKEN;
|
||||
if (!token) {
|
||||
throw new Error("GH_TOKEN is not set");
|
||||
}
|
||||
async function getToken(): Promise<string> {
|
||||
// prefer explicit GH_TOKEN, fall back to acquiring one via GitHub App credentials
|
||||
if (process.env.GH_TOKEN) return process.env.GH_TOKEN;
|
||||
return await acquireNewToken();
|
||||
}
|
||||
|
||||
describe("formatReviewThreads", () => {
|
||||
it("formats thread blocks with TOC and correct line numbers", { timeout: 30000 }, async () => {
|
||||
const token = await getToken();
|
||||
const octokit = new Octokit({ auth: token });
|
||||
const pullNumber = 49;
|
||||
const reviewId = 3485940013;
|
||||
|
||||
@@ -0,0 +1,556 @@
|
||||
import { describe, expect, it } from "vitest";
|
||||
|
||||
// ─── git tool security tests ────────────────────────────────────────────
|
||||
|
||||
// re-create the validation logic from git.ts for unit testing
|
||||
const AUTH_REQUIRED_REDIRECT: Record<string, string> = {
|
||||
push: "Use push_branch tool instead.",
|
||||
fetch: "Use git_fetch tool instead.",
|
||||
pull: "Use git_fetch + git merge instead.",
|
||||
clone: "Repository already cloned. Use checkout_pr for PR branches.",
|
||||
};
|
||||
|
||||
// only blocked when bash is disabled — in restricted mode the agent has bash
|
||||
// in a stripped sandbox so blocking these is redundant
|
||||
const NOBASH_BLOCKED_SUBCOMMANDS: Record<string, string> = {
|
||||
config: "Blocked: git config can set up filter drivers or hooks that execute arbitrary code.",
|
||||
submodule:
|
||||
"Blocked: git submodule can reference malicious repositories and execute code on update.",
|
||||
"update-index":
|
||||
"Blocked: git update-index can modify index entries in ways that bypass file protections.",
|
||||
"filter-branch": "Blocked: git filter-branch executes arbitrary code on repository history.",
|
||||
replace: "Blocked: git replace can redirect object lookups.",
|
||||
rebase: "Blocked: git rebase --exec can execute arbitrary shell commands.",
|
||||
bisect: "Blocked: git bisect run can execute arbitrary shell commands.",
|
||||
};
|
||||
|
||||
const NOBASH_BLOCKED_ARGS = ["--exec", "--extcmd", "--upload-pack", "--receive-pack"];
|
||||
|
||||
type BashPermission = "disabled" | "restricted" | "enabled";
|
||||
|
||||
type ValidateGitParams = {
|
||||
subcommand: string;
|
||||
args: string[];
|
||||
bashPermission: BashPermission;
|
||||
};
|
||||
|
||||
// matches the arkregex pattern used in the Git schema
|
||||
const SUBCOMMAND_PATTERN = /^[a-z][a-z0-9-]*$/;
|
||||
|
||||
// mirrors the validation logic in GitTool.execute
|
||||
function validateGitCommand(params: ValidateGitParams): string | null {
|
||||
// schema-level regex validation — applies in ALL modes
|
||||
if (!SUBCOMMAND_PATTERN.test(params.subcommand)) {
|
||||
return `subcommand must be Git subcommand (was "${params.subcommand}")`;
|
||||
}
|
||||
|
||||
const redirect = AUTH_REQUIRED_REDIRECT[params.subcommand];
|
||||
if (redirect) {
|
||||
return `git ${params.subcommand} requires authentication. ${redirect}`;
|
||||
}
|
||||
|
||||
// subcommand and arg blocking only applies when bash is disabled
|
||||
if (params.bashPermission === "disabled") {
|
||||
const blocked = NOBASH_BLOCKED_SUBCOMMANDS[params.subcommand];
|
||||
if (blocked) {
|
||||
return blocked;
|
||||
}
|
||||
|
||||
for (const arg of params.args) {
|
||||
const isBlocked = NOBASH_BLOCKED_ARGS.some(
|
||||
(flag) => arg === flag || arg.startsWith(flag + "=")
|
||||
);
|
||||
if (isBlocked) {
|
||||
return `Blocked: '${arg}' flag can execute arbitrary code and is not allowed.`;
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
return null; // no error
|
||||
}
|
||||
|
||||
describe("git tool security - subcommand regex validation", () => {
|
||||
it("blocks -c flag as subcommand in ALL modes (alias injection)", () => {
|
||||
const modes: BashPermission[] = ["disabled", "restricted", "enabled"];
|
||||
for (const mode of modes) {
|
||||
const error = validateGitCommand({
|
||||
subcommand: "-c",
|
||||
args: ["alias.x=!evil-command", "x"],
|
||||
bashPermission: mode,
|
||||
});
|
||||
expect(error).toContain("Git subcommand");
|
||||
}
|
||||
});
|
||||
|
||||
it("blocks --exec-path as subcommand", () => {
|
||||
const error = validateGitCommand({
|
||||
subcommand: "--exec-path=/malicious",
|
||||
args: ["status"],
|
||||
bashPermission: "disabled",
|
||||
});
|
||||
expect(error).toContain("Git subcommand");
|
||||
});
|
||||
|
||||
it("blocks -C as subcommand (change directory)", () => {
|
||||
const error = validateGitCommand({
|
||||
subcommand: "-C",
|
||||
args: ["/tmp", "init"],
|
||||
bashPermission: "disabled",
|
||||
});
|
||||
expect(error).toContain("Git subcommand");
|
||||
});
|
||||
|
||||
it("blocks --config-env as subcommand", () => {
|
||||
const error = validateGitCommand({
|
||||
subcommand: "--config-env",
|
||||
args: ["core.pager=PATH", "log"],
|
||||
bashPermission: "disabled",
|
||||
});
|
||||
expect(error).toContain("Git subcommand");
|
||||
});
|
||||
|
||||
it("blocks all flags starting with - as subcommand", () => {
|
||||
const flags = ["-c", "-C", "-p", "--paginate", "--git-dir", "--work-tree", "--bare"];
|
||||
for (const flag of flags) {
|
||||
const error = validateGitCommand({
|
||||
subcommand: flag,
|
||||
args: [],
|
||||
bashPermission: "disabled",
|
||||
});
|
||||
expect(error).toContain("Git subcommand");
|
||||
}
|
||||
});
|
||||
|
||||
it("blocks uppercase subcommands", () => {
|
||||
const error = validateGitCommand({
|
||||
subcommand: "STATUS",
|
||||
args: [],
|
||||
bashPermission: "disabled",
|
||||
});
|
||||
expect(error).toContain("Git subcommand");
|
||||
});
|
||||
|
||||
it("blocks subcommands with special characters", () => {
|
||||
const bad = ["git;evil", "status$(cmd)", "log|cat", "diff&bg"];
|
||||
for (const sub of bad) {
|
||||
const error = validateGitCommand({
|
||||
subcommand: sub,
|
||||
args: [],
|
||||
bashPermission: "disabled",
|
||||
});
|
||||
expect(error).toContain("Git subcommand");
|
||||
}
|
||||
});
|
||||
|
||||
it("allows valid subcommands", () => {
|
||||
const safe = ["status", "log", "diff", "show", "branch", "tag", "stash", "blame"];
|
||||
for (const sub of safe) {
|
||||
const error = validateGitCommand({
|
||||
subcommand: sub,
|
||||
args: [],
|
||||
bashPermission: "disabled",
|
||||
});
|
||||
expect(error).toBeNull();
|
||||
}
|
||||
});
|
||||
|
||||
it("allows hyphenated subcommands", () => {
|
||||
const safe = ["filter-branch", "update-index", "ls-remote", "ls-files", "rev-parse"];
|
||||
for (const sub of safe) {
|
||||
const error = validateGitCommand({
|
||||
subcommand: sub,
|
||||
args: [],
|
||||
bashPermission: "enabled",
|
||||
});
|
||||
expect(error).toBeNull();
|
||||
}
|
||||
});
|
||||
});
|
||||
|
||||
describe("git tool security - blocked subcommands (disabled mode only)", () => {
|
||||
it("blocks config in disabled mode", () => {
|
||||
const error = validateGitCommand({
|
||||
subcommand: "config",
|
||||
args: ["core.hooksPath", "./hooks"],
|
||||
bashPermission: "disabled",
|
||||
});
|
||||
expect(error).toContain("git config");
|
||||
});
|
||||
|
||||
it("allows config in restricted mode (agent has bash)", () => {
|
||||
const error = validateGitCommand({
|
||||
subcommand: "config",
|
||||
args: ["filter.evil.clean", "bash -c 'evil'"],
|
||||
bashPermission: "restricted",
|
||||
});
|
||||
expect(error).toBeNull();
|
||||
});
|
||||
|
||||
it("blocks submodule in disabled mode", () => {
|
||||
const error = validateGitCommand({
|
||||
subcommand: "submodule",
|
||||
args: ["add", "https://evil.com/repo.git"],
|
||||
bashPermission: "disabled",
|
||||
});
|
||||
expect(error).toContain("submodule");
|
||||
});
|
||||
|
||||
it("allows submodule in restricted mode", () => {
|
||||
const error = validateGitCommand({
|
||||
subcommand: "submodule",
|
||||
args: ["add", "https://example.com/repo.git"],
|
||||
bashPermission: "restricted",
|
||||
});
|
||||
expect(error).toBeNull();
|
||||
});
|
||||
|
||||
it("blocks rebase in disabled mode", () => {
|
||||
const error = validateGitCommand({
|
||||
subcommand: "rebase",
|
||||
args: ["--exec", "evil-command", "HEAD~1"],
|
||||
bashPermission: "disabled",
|
||||
});
|
||||
expect(error).toContain("rebase");
|
||||
});
|
||||
|
||||
it("allows rebase in restricted mode", () => {
|
||||
const error = validateGitCommand({
|
||||
subcommand: "rebase",
|
||||
args: ["main"],
|
||||
bashPermission: "restricted",
|
||||
});
|
||||
expect(error).toBeNull();
|
||||
});
|
||||
|
||||
it("blocks bisect in disabled mode", () => {
|
||||
const error = validateGitCommand({
|
||||
subcommand: "bisect",
|
||||
args: ["run", "evil-command"],
|
||||
bashPermission: "disabled",
|
||||
});
|
||||
expect(error).toContain("bisect");
|
||||
});
|
||||
|
||||
it("blocks filter-branch in disabled mode", () => {
|
||||
const error = validateGitCommand({
|
||||
subcommand: "filter-branch",
|
||||
args: ["--tree-filter", "evil-command", "HEAD"],
|
||||
bashPermission: "disabled",
|
||||
});
|
||||
expect(error).toContain("filter-branch");
|
||||
});
|
||||
|
||||
it("allows blocked subcommands in enabled mode", () => {
|
||||
const blocked = ["config", "submodule", "rebase", "bisect", "filter-branch"];
|
||||
for (const sub of blocked) {
|
||||
const error = validateGitCommand({
|
||||
subcommand: sub,
|
||||
args: [],
|
||||
bashPermission: "enabled",
|
||||
});
|
||||
expect(error).toBeNull();
|
||||
}
|
||||
});
|
||||
|
||||
it("allows blocked subcommands in restricted mode (stripped env is security boundary)", () => {
|
||||
const blocked = ["config", "submodule", "rebase", "bisect", "filter-branch"];
|
||||
for (const sub of blocked) {
|
||||
const error = validateGitCommand({
|
||||
subcommand: sub,
|
||||
args: [],
|
||||
bashPermission: "restricted",
|
||||
});
|
||||
expect(error).toBeNull();
|
||||
}
|
||||
});
|
||||
});
|
||||
|
||||
describe("git tool security - blocked arg flags (disabled mode only)", () => {
|
||||
it("blocks --exec in args (disabled)", () => {
|
||||
const error = validateGitCommand({
|
||||
subcommand: "log",
|
||||
args: ["--exec", "evil-command"],
|
||||
bashPermission: "disabled",
|
||||
});
|
||||
expect(error).toContain("arbitrary code");
|
||||
});
|
||||
|
||||
it("blocks --exec= in args (disabled)", () => {
|
||||
const error = validateGitCommand({
|
||||
subcommand: "log",
|
||||
args: ["--exec=evil-command"],
|
||||
bashPermission: "disabled",
|
||||
});
|
||||
expect(error).toContain("arbitrary code");
|
||||
});
|
||||
|
||||
it("blocks --extcmd in args (disabled)", () => {
|
||||
const error = validateGitCommand({
|
||||
subcommand: "difftool",
|
||||
args: ["--extcmd=evil-command", "HEAD~1"],
|
||||
bashPermission: "disabled",
|
||||
});
|
||||
expect(error).toContain("arbitrary code");
|
||||
});
|
||||
|
||||
it("blocks --upload-pack in args (disabled)", () => {
|
||||
const error = validateGitCommand({
|
||||
subcommand: "ls-remote",
|
||||
args: ["--upload-pack=evil"],
|
||||
bashPermission: "disabled",
|
||||
});
|
||||
expect(error).toContain("arbitrary code");
|
||||
});
|
||||
|
||||
it("allows --exec in restricted mode (agent has bash)", () => {
|
||||
const error = validateGitCommand({
|
||||
subcommand: "rebase",
|
||||
args: ["--exec", "npm test", "HEAD~1"],
|
||||
bashPermission: "restricted",
|
||||
});
|
||||
expect(error).toBeNull();
|
||||
});
|
||||
|
||||
it("allows --extcmd in restricted mode", () => {
|
||||
const error = validateGitCommand({
|
||||
subcommand: "difftool",
|
||||
args: ["--extcmd=less"],
|
||||
bashPermission: "restricted",
|
||||
});
|
||||
expect(error).toBeNull();
|
||||
});
|
||||
|
||||
it("allows blocked args in enabled mode", () => {
|
||||
const error = validateGitCommand({
|
||||
subcommand: "difftool",
|
||||
args: ["--extcmd=less"],
|
||||
bashPermission: "enabled",
|
||||
});
|
||||
expect(error).toBeNull();
|
||||
});
|
||||
|
||||
it("allows normal args in disabled mode", () => {
|
||||
const error = validateGitCommand({
|
||||
subcommand: "log",
|
||||
args: ["--oneline", "-10", "--format=%H %s"],
|
||||
bashPermission: "disabled",
|
||||
});
|
||||
expect(error).toBeNull();
|
||||
});
|
||||
|
||||
it("does not false-positive on --exclude-standard (not --exec)", () => {
|
||||
const error = validateGitCommand({
|
||||
subcommand: "ls-files",
|
||||
args: ["--exclude-standard"],
|
||||
bashPermission: "disabled",
|
||||
});
|
||||
expect(error).toBeNull();
|
||||
});
|
||||
|
||||
it("does not false-positive on --execute (not --exec=)", () => {
|
||||
const error = validateGitCommand({
|
||||
subcommand: "log",
|
||||
args: ["--execute-something"],
|
||||
bashPermission: "disabled",
|
||||
});
|
||||
expect(error).toBeNull();
|
||||
});
|
||||
|
||||
it("does not false-positive on -c (combined diff format for git log)", () => {
|
||||
const error = validateGitCommand({
|
||||
subcommand: "log",
|
||||
args: ["-c", "--oneline"],
|
||||
bashPermission: "disabled",
|
||||
});
|
||||
expect(error).toBeNull();
|
||||
});
|
||||
});
|
||||
|
||||
describe("git tool security - auth redirect", () => {
|
||||
it("redirects push in all modes", () => {
|
||||
const modes: BashPermission[] = ["disabled", "restricted", "enabled"];
|
||||
for (const mode of modes) {
|
||||
const error = validateGitCommand({
|
||||
subcommand: "push",
|
||||
args: [],
|
||||
bashPermission: mode,
|
||||
});
|
||||
expect(error).toContain("authentication");
|
||||
}
|
||||
});
|
||||
|
||||
it("redirects fetch", () => {
|
||||
const error = validateGitCommand({
|
||||
subcommand: "fetch",
|
||||
args: [],
|
||||
bashPermission: "enabled",
|
||||
});
|
||||
expect(error).toContain("authentication");
|
||||
});
|
||||
|
||||
it("redirects pull", () => {
|
||||
const error = validateGitCommand({
|
||||
subcommand: "pull",
|
||||
args: [],
|
||||
bashPermission: "enabled",
|
||||
});
|
||||
expect(error).toContain("authentication");
|
||||
});
|
||||
|
||||
it("redirects clone", () => {
|
||||
const error = validateGitCommand({
|
||||
subcommand: "clone",
|
||||
args: [],
|
||||
bashPermission: "enabled",
|
||||
});
|
||||
expect(error).toContain("authentication");
|
||||
});
|
||||
});
|
||||
|
||||
// ─── file tool security tests ───────────────────────────────────────────
|
||||
|
||||
const GIT_INTERPRETED_FILES = [".gitattributes", ".gitmodules"];
|
||||
|
||||
type ValidateWritePathResult = {
|
||||
allowed: boolean;
|
||||
error?: string;
|
||||
};
|
||||
|
||||
// simplified path validation that mirrors the security checks in file.ts
|
||||
// without requiring real filesystem operations (for unit testing)
|
||||
function validateWritePathSecurity(
|
||||
relative: string,
|
||||
bashPermission: BashPermission
|
||||
): ValidateWritePathResult {
|
||||
if (relative === ".git" || relative.startsWith(".git/")) {
|
||||
return { allowed: false, error: `writing to .git is not allowed: ${relative}` };
|
||||
}
|
||||
|
||||
// only blocked when bash is disabled
|
||||
if (bashPermission === "disabled") {
|
||||
const basename = relative.split("/").pop() || "";
|
||||
if (GIT_INTERPRETED_FILES.includes(basename)) {
|
||||
return {
|
||||
allowed: false,
|
||||
error: `writing to ${basename} is not allowed when bash is ${bashPermission}`,
|
||||
};
|
||||
}
|
||||
}
|
||||
|
||||
return { allowed: true };
|
||||
}
|
||||
|
||||
describe("file tool security - .git protection", () => {
|
||||
it("blocks .git directory in all modes", () => {
|
||||
const modes: BashPermission[] = ["disabled", "restricted", "enabled"];
|
||||
for (const mode of modes) {
|
||||
const result = validateWritePathSecurity(".git", mode);
|
||||
expect(result.allowed).toBe(false);
|
||||
}
|
||||
});
|
||||
|
||||
it("blocks .git/config", () => {
|
||||
const result = validateWritePathSecurity(".git/config", "enabled");
|
||||
expect(result.allowed).toBe(false);
|
||||
});
|
||||
|
||||
it("blocks .git/hooks/pre-commit", () => {
|
||||
const result = validateWritePathSecurity(".git/hooks/pre-commit", "enabled");
|
||||
expect(result.allowed).toBe(false);
|
||||
});
|
||||
|
||||
it("blocks deeply nested .git paths", () => {
|
||||
const result = validateWritePathSecurity(".git/objects/ab/cd1234", "enabled");
|
||||
expect(result.allowed).toBe(false);
|
||||
});
|
||||
});
|
||||
|
||||
describe("file tool security - git-interpreted files (disabled mode only)", () => {
|
||||
it("blocks .gitattributes in disabled mode", () => {
|
||||
const result = validateWritePathSecurity(".gitattributes", "disabled");
|
||||
expect(result.allowed).toBe(false);
|
||||
expect(result.error).toContain(".gitattributes");
|
||||
});
|
||||
|
||||
it("allows .gitattributes in restricted mode (agent has bash)", () => {
|
||||
const result = validateWritePathSecurity(".gitattributes", "restricted");
|
||||
expect(result.allowed).toBe(true);
|
||||
});
|
||||
|
||||
it("allows .gitattributes in enabled mode", () => {
|
||||
const result = validateWritePathSecurity(".gitattributes", "enabled");
|
||||
expect(result.allowed).toBe(true);
|
||||
});
|
||||
|
||||
it("blocks .gitmodules in disabled mode", () => {
|
||||
const result = validateWritePathSecurity(".gitmodules", "disabled");
|
||||
expect(result.allowed).toBe(false);
|
||||
});
|
||||
|
||||
it("allows .gitmodules in restricted mode", () => {
|
||||
const result = validateWritePathSecurity(".gitmodules", "restricted");
|
||||
expect(result.allowed).toBe(true);
|
||||
});
|
||||
|
||||
it("allows .gitmodules in enabled mode", () => {
|
||||
const result = validateWritePathSecurity(".gitmodules", "enabled");
|
||||
expect(result.allowed).toBe(true);
|
||||
});
|
||||
|
||||
it("blocks subdirectory .gitattributes in disabled mode", () => {
|
||||
const result = validateWritePathSecurity("src/.gitattributes", "disabled");
|
||||
expect(result.allowed).toBe(false);
|
||||
});
|
||||
|
||||
it("blocks deeply nested .gitattributes in disabled mode", () => {
|
||||
const result = validateWritePathSecurity("a/b/c/.gitattributes", "disabled");
|
||||
expect(result.allowed).toBe(false);
|
||||
});
|
||||
|
||||
it("allows subdirectory .gitattributes in restricted mode", () => {
|
||||
const result = validateWritePathSecurity("src/.gitattributes", "restricted");
|
||||
expect(result.allowed).toBe(true);
|
||||
});
|
||||
|
||||
it("allows normal files in all modes", () => {
|
||||
const files = ["README.md", "src/index.ts", "package.json", ".env", ".gitignore"];
|
||||
const modes: BashPermission[] = ["disabled", "restricted", "enabled"];
|
||||
for (const file of files) {
|
||||
for (const mode of modes) {
|
||||
const result = validateWritePathSecurity(file, mode);
|
||||
expect(result.allowed).toBe(true);
|
||||
}
|
||||
}
|
||||
});
|
||||
|
||||
it("does not block .gitignore (not a code execution vector)", () => {
|
||||
const result = validateWritePathSecurity(".gitignore", "disabled");
|
||||
expect(result.allowed).toBe(true);
|
||||
});
|
||||
|
||||
it("does not block .gitkeep (not a code execution vector)", () => {
|
||||
const result = validateWritePathSecurity("dir/.gitkeep", "disabled");
|
||||
expect(result.allowed).toBe(true);
|
||||
});
|
||||
});
|
||||
|
||||
// ─── dependency install security tests ──────────────────────────────────
|
||||
|
||||
// mirrors the logic in dependencies.ts startInstallation()
|
||||
function shouldIgnoreScripts(bashPermission: BashPermission): boolean {
|
||||
return bashPermission === "disabled";
|
||||
}
|
||||
|
||||
describe("dependency install - ignore-scripts logic", () => {
|
||||
it("ignoreScripts is true when bash is disabled", () => {
|
||||
expect(shouldIgnoreScripts("disabled")).toBe(true);
|
||||
});
|
||||
|
||||
it("ignoreScripts is false when bash is restricted (scripts run in stripped env)", () => {
|
||||
expect(shouldIgnoreScripts("restricted")).toBe(false);
|
||||
});
|
||||
|
||||
it("ignoreScripts is false when bash is enabled", () => {
|
||||
expect(shouldIgnoreScripts("enabled")).toBe(false);
|
||||
});
|
||||
});
|
||||
Reference in New Issue
Block a user