Compare commits

..

1 Commits

Author SHA1 Message Date
Claude
80591ffc11 fix: prevent path traversal in delete_files MCP tool
The delete_files tool only validated absolute paths starting with "/",
allowing relative paths with traversal sequences like "./../../sensitive"
to bypass validation.

Now all paths are normalized using path.resolve() before validation,
ensuring both absolute and relative paths with ".." sequences are
properly blocked from accessing files outside the repository root.
2025-12-12 00:14:12 +00:00
7 changed files with 27 additions and 336 deletions

View File

@@ -195,7 +195,7 @@ runs:
# Install Claude Code if no custom executable is provided
if [ -z "$PATH_TO_CLAUDE_CODE_EXECUTABLE" ]; then
CLAUDE_CODE_VERSION="2.0.69"
CLAUDE_CODE_VERSION="2.0.62"
echo "Installing Claude Code v${CLAUDE_CODE_VERSION}..."
for attempt in 1 2 3; do
echo "Installation attempt $attempt..."

View File

@@ -121,7 +121,7 @@ runs:
PATH_TO_CLAUDE_CODE_EXECUTABLE: ${{ inputs.path_to_claude_code_executable }}
run: |
if [ -z "$PATH_TO_CLAUDE_CODE_EXECUTABLE" ]; then
CLAUDE_CODE_VERSION="2.0.69"
CLAUDE_CODE_VERSION="2.0.62"
echo "Installing Claude Code v${CLAUDE_CODE_VERSION}..."
for attempt in 1 2 3; do
echo "Installation attempt $attempt..."

View File

@@ -167,8 +167,8 @@ export async function parseAndSetStructuredOutputs(
}
export async function runClaude(promptPath: string, options: ClaudeOptions) {
// Feature flag: use SDK path by default, set USE_AGENT_SDK=false to use CLI
const useAgentSdk = process.env.USE_AGENT_SDK !== "false";
// Feature flag: use SDK path when USE_AGENT_SDK=true
const useAgentSdk = process.env.USE_AGENT_SDK === "true";
console.log(
`Using ${useAgentSdk ? "Agent SDK" : "CLI"} path (USE_AGENT_SDK=${process.env.USE_AGENT_SDK ?? "unset"})`,
);

View File

@@ -53,7 +53,6 @@ jobs:
fromJSON(steps.detect.outputs.structured_output).confidence >= 0.7
env:
GH_TOKEN: ${{ github.token }}
HEAD_BRANCH: ${{ github.event.workflow_run.head_branch }}
run: |
OUTPUT='${{ steps.detect.outputs.structured_output }}'
CONFIDENCE=$(echo "$OUTPUT" | jq -r '.confidence')
@@ -65,7 +64,7 @@ jobs:
echo "Triggering automatic retry..."
gh workflow run "${{ github.event.workflow_run.name }}" \
--ref "$HEAD_BRANCH"
--ref "${{ github.event.workflow_run.head_branch }}"
# Low confidence flaky detection - skip retry
- name: Low confidence detection
@@ -84,14 +83,13 @@ jobs:
if: github.event.workflow_run.event == 'pull_request'
env:
GH_TOKEN: ${{ github.token }}
HEAD_BRANCH: ${{ github.event.workflow_run.head_branch }}
run: |
OUTPUT='${{ steps.detect.outputs.structured_output }}'
IS_FLAKY=$(echo "$OUTPUT" | jq -r '.is_flaky')
CONFIDENCE=$(echo "$OUTPUT" | jq -r '.confidence')
SUMMARY=$(echo "$OUTPUT" | jq -r '.summary')
pr_number=$(gh pr list --head "$HEAD_BRANCH" --json number --jq '.[0].number')
pr_number=$(gh pr list --head "${{ github.event.workflow_run.head_branch }}" --json number --jq '.[0].number')
if [ -n "$pr_number" ]; then
if [ "$IS_FLAKY" = "true" ]; then

View File

@@ -6,112 +6,13 @@
* - For Issues: Create a new branch
*/
import { execFileSync } from "child_process";
import { $ } from "bun";
import * as core from "@actions/core";
import type { ParsedGitHubContext } from "../context";
import type { GitHubPullRequest } from "../types";
import type { Octokits } from "../api/client";
import type { FetchDataResult } from "../data/fetcher";
/**
* Validates a git branch name against a strict whitelist pattern.
* This prevents command injection by ensuring only safe characters are used.
*
* Valid branch names:
* - Start with alphanumeric character (not dash, to prevent option injection)
* - Contain only alphanumeric, forward slash, hyphen, underscore, or period
* - Do not start or end with a period
* - Do not end with a slash
* - Do not contain '..' (path traversal)
* - Do not contain '//' (consecutive slashes)
* - Do not end with '.lock'
* - Do not contain '@{'
* - Do not contain control characters or special git characters (~^:?*[\])
*/
export function validateBranchName(branchName: string): void {
// Check for empty or whitespace-only names
if (!branchName || branchName.trim().length === 0) {
throw new Error("Branch name cannot be empty");
}
// Check for leading dash (prevents option injection like --help, -x)
if (branchName.startsWith("-")) {
throw new Error(
`Invalid branch name: "${branchName}". Branch names cannot start with a dash.`,
);
}
// Check for control characters and special git characters (~^:?*[\])
// eslint-disable-next-line no-control-regex
if (/[\x00-\x1F\x7F ~^:?*[\]\\]/.test(branchName)) {
throw new Error(
`Invalid branch name: "${branchName}". Branch names cannot contain control characters, spaces, or special git characters (~^:?*[\\]).`,
);
}
// Strict whitelist pattern: alphanumeric start, then alphanumeric/slash/hyphen/underscore/period
const validPattern = /^[a-zA-Z0-9][a-zA-Z0-9/_.-]*$/;
if (!validPattern.test(branchName)) {
throw new Error(
`Invalid branch name: "${branchName}". Branch names must start with an alphanumeric character and contain only alphanumeric characters, forward slashes, hyphens, underscores, or periods.`,
);
}
// Check for leading/trailing periods
if (branchName.startsWith(".") || branchName.endsWith(".")) {
throw new Error(
`Invalid branch name: "${branchName}". Branch names cannot start or end with a period.`,
);
}
// Check for trailing slash
if (branchName.endsWith("/")) {
throw new Error(
`Invalid branch name: "${branchName}". Branch names cannot end with a slash.`,
);
}
// Check for consecutive slashes
if (branchName.includes("//")) {
throw new Error(
`Invalid branch name: "${branchName}". Branch names cannot contain consecutive slashes.`,
);
}
// Additional git-specific validations
if (branchName.includes("..")) {
throw new Error(
`Invalid branch name: "${branchName}". Branch names cannot contain '..'`,
);
}
if (branchName.endsWith(".lock")) {
throw new Error(
`Invalid branch name: "${branchName}". Branch names cannot end with '.lock'`,
);
}
if (branchName.includes("@{")) {
throw new Error(
`Invalid branch name: "${branchName}". Branch names cannot contain '@{'`,
);
}
}
/**
* Executes a git command safely using execFileSync to avoid shell interpolation.
*
* Security: execFileSync passes arguments directly to the git binary without
* invoking a shell, preventing command injection attacks where malicious input
* could be interpreted as shell commands (e.g., branch names containing `;`, `|`, `&&`).
*
* @param args - Git command arguments (e.g., ["checkout", "branch-name"])
*/
function execGit(args: string[]): void {
execFileSync("git", args, { stdio: "inherit" });
}
export type BranchInfo = {
baseBranch: string;
claudeBranch?: string;
@@ -152,19 +53,14 @@ export async function setupBranch(
`PR #${entityNumber}: ${commitCount} commits, using fetch depth ${fetchDepth}`,
);
// Validate branch names before use to prevent command injection
validateBranchName(branchName);
// Execute git commands to checkout PR branch (dynamic depth based on PR size)
// Using execFileSync instead of shell template literals for security
execGit(["fetch", "origin", `--depth=${fetchDepth}`, branchName]);
execGit(["checkout", branchName, "--"]);
await $`git fetch origin --depth=${fetchDepth} ${branchName}`;
await $`git checkout ${branchName} --`;
console.log(`Successfully checked out PR branch for PR #${entityNumber}`);
// For open PRs, we need to get the base branch of the PR
const baseBranch = prData.baseRefName;
validateBranchName(baseBranch);
return {
baseBranch,
@@ -222,9 +118,8 @@ export async function setupBranch(
// Ensure we're on the source branch
console.log(`Fetching and checking out source branch: ${sourceBranch}`);
validateBranchName(sourceBranch);
execGit(["fetch", "origin", sourceBranch, "--depth=1"]);
execGit(["checkout", sourceBranch, "--"]);
await $`git fetch origin ${sourceBranch} --depth=1`;
await $`git checkout ${sourceBranch}`;
// Set outputs for GitHub Actions
core.setOutput("CLAUDE_BRANCH", newBranch);
@@ -243,13 +138,11 @@ export async function setupBranch(
// Fetch and checkout the source branch first to ensure we branch from the correct base
console.log(`Fetching and checking out source branch: ${sourceBranch}`);
validateBranchName(sourceBranch);
validateBranchName(newBranch);
execGit(["fetch", "origin", sourceBranch, "--depth=1"]);
execGit(["checkout", sourceBranch, "--"]);
await $`git fetch origin ${sourceBranch} --depth=1`;
await $`git checkout ${sourceBranch}`;
// Create and checkout the new branch from the source branch
execGit(["checkout", "-b", newBranch]);
await $`git checkout -b ${newBranch}`;
console.log(
`Successfully created and checked out local branch: ${newBranch}`,

View File

@@ -4,7 +4,7 @@ import { McpServer } from "@modelcontextprotocol/sdk/server/mcp.js";
import { StdioServerTransport } from "@modelcontextprotocol/sdk/server/stdio.js";
import { z } from "zod";
import { readFile, stat } from "fs/promises";
import { join } from "path";
import { join, resolve, sep } from "path";
import { constants } from "fs";
import fetch from "node-fetch";
import { GITHUB_API_URL } from "../github/api/config";
@@ -474,20 +474,21 @@ server.tool(
throw new Error("GITHUB_TOKEN environment variable is required");
}
// Convert absolute paths to relative if they match CWD
// Normalize all paths and validate they're within the repository root
const cwd = process.cwd();
const processedPaths = paths.map((filePath) => {
if (filePath.startsWith("/")) {
if (filePath.startsWith(cwd)) {
// Strip CWD from absolute path
return filePath.slice(cwd.length + 1);
} else {
throw new Error(
`Path '${filePath}' must be relative to repository root or within current working directory`,
);
}
// Normalize the path to resolve any .. or . sequences
const normalizedPath = resolve(cwd, filePath);
// Validate the normalized path is within the current working directory
if (!normalizedPath.startsWith(cwd + sep)) {
throw new Error(
`Path '${filePath}' resolves outside the repository root`,
);
}
return filePath;
// Convert to relative path by stripping the cwd prefix
return normalizedPath.slice(cwd.length + 1);
});
// 1. Get the branch reference (create if doesn't exist)

View File

@@ -1,201 +0,0 @@
import { describe, expect, it } from "bun:test";
import { validateBranchName } from "../src/github/operations/branch";
describe("validateBranchName", () => {
describe("valid branch names", () => {
it("should accept simple alphanumeric names", () => {
expect(() => validateBranchName("main")).not.toThrow();
expect(() => validateBranchName("feature123")).not.toThrow();
expect(() => validateBranchName("Branch1")).not.toThrow();
});
it("should accept names with hyphens", () => {
expect(() => validateBranchName("feature-branch")).not.toThrow();
expect(() => validateBranchName("fix-bug-123")).not.toThrow();
});
it("should accept names with underscores", () => {
expect(() => validateBranchName("feature_branch")).not.toThrow();
expect(() => validateBranchName("fix_bug_123")).not.toThrow();
});
it("should accept names with forward slashes", () => {
expect(() => validateBranchName("feature/new-thing")).not.toThrow();
expect(() => validateBranchName("user/feature/branch")).not.toThrow();
});
it("should accept names with periods", () => {
expect(() => validateBranchName("v1.0.0")).not.toThrow();
expect(() => validateBranchName("release.1.2.3")).not.toThrow();
});
it("should accept typical branch name formats", () => {
expect(() =>
validateBranchName("claude/issue-123-20250101-1234"),
).not.toThrow();
expect(() => validateBranchName("refs/heads/main")).not.toThrow();
expect(() => validateBranchName("bugfix/JIRA-1234")).not.toThrow();
});
});
describe("command injection attempts", () => {
it("should reject shell command substitution with $()", () => {
expect(() => validateBranchName("$(whoami)")).toThrow();
expect(() => validateBranchName("branch-$(rm -rf /)")).toThrow();
expect(() => validateBranchName("test$(cat /etc/passwd)")).toThrow();
});
it("should reject shell command substitution with backticks", () => {
expect(() => validateBranchName("`whoami`")).toThrow();
expect(() => validateBranchName("branch-`rm -rf /`")).toThrow();
});
it("should reject command chaining with semicolons", () => {
expect(() => validateBranchName("branch; rm -rf /")).toThrow();
expect(() => validateBranchName("test;whoami")).toThrow();
});
it("should reject command chaining with &&", () => {
expect(() => validateBranchName("branch && rm -rf /")).toThrow();
expect(() => validateBranchName("test&&whoami")).toThrow();
});
it("should reject command chaining with ||", () => {
expect(() => validateBranchName("branch || rm -rf /")).toThrow();
expect(() => validateBranchName("test||whoami")).toThrow();
});
it("should reject pipe characters", () => {
expect(() => validateBranchName("branch | cat")).toThrow();
expect(() => validateBranchName("test|grep password")).toThrow();
});
it("should reject redirection operators", () => {
expect(() => validateBranchName("branch > /etc/passwd")).toThrow();
expect(() => validateBranchName("branch < input")).toThrow();
expect(() => validateBranchName("branch >> file")).toThrow();
});
});
describe("option injection attempts", () => {
it("should reject branch names starting with dash", () => {
expect(() => validateBranchName("-x")).toThrow(
/cannot start with a dash/,
);
expect(() => validateBranchName("--help")).toThrow(
/cannot start with a dash/,
);
expect(() => validateBranchName("-")).toThrow(/cannot start with a dash/);
expect(() => validateBranchName("--version")).toThrow(
/cannot start with a dash/,
);
expect(() => validateBranchName("-rf")).toThrow(
/cannot start with a dash/,
);
});
});
describe("path traversal attempts", () => {
it("should reject double dot sequences", () => {
expect(() => validateBranchName("../../../etc")).toThrow();
expect(() => validateBranchName("branch/../secret")).toThrow(/'\.\.'$/);
expect(() => validateBranchName("a..b")).toThrow(/'\.\.'$/);
});
});
describe("git-specific invalid patterns", () => {
it("should reject @{ sequence", () => {
expect(() => validateBranchName("branch@{1}")).toThrow(/@{/);
expect(() => validateBranchName("HEAD@{yesterday}")).toThrow(/@{/);
});
it("should reject .lock suffix", () => {
expect(() => validateBranchName("branch.lock")).toThrow(/\.lock/);
expect(() => validateBranchName("feature.lock")).toThrow(/\.lock/);
});
it("should reject consecutive slashes", () => {
expect(() => validateBranchName("feature//branch")).toThrow(
/consecutive slashes/,
);
expect(() => validateBranchName("a//b//c")).toThrow(
/consecutive slashes/,
);
});
it("should reject trailing slashes", () => {
expect(() => validateBranchName("feature/")).toThrow(
/cannot end with a slash/,
);
expect(() => validateBranchName("branch/")).toThrow(
/cannot end with a slash/,
);
});
it("should reject leading periods", () => {
expect(() => validateBranchName(".hidden")).toThrow();
});
it("should reject trailing periods", () => {
expect(() => validateBranchName("branch.")).toThrow(
/cannot start or end with a period/,
);
});
it("should reject special git refspec characters", () => {
expect(() => validateBranchName("branch~1")).toThrow();
expect(() => validateBranchName("branch^2")).toThrow();
expect(() => validateBranchName("branch:ref")).toThrow();
expect(() => validateBranchName("branch?")).toThrow();
expect(() => validateBranchName("branch*")).toThrow();
expect(() => validateBranchName("branch[0]")).toThrow();
expect(() => validateBranchName("branch\\path")).toThrow();
});
});
describe("control characters and special characters", () => {
it("should reject null bytes", () => {
expect(() => validateBranchName("branch\x00name")).toThrow();
});
it("should reject other control characters", () => {
expect(() => validateBranchName("branch\x01name")).toThrow();
expect(() => validateBranchName("branch\x1Fname")).toThrow();
expect(() => validateBranchName("branch\x7Fname")).toThrow();
});
it("should reject spaces", () => {
expect(() => validateBranchName("branch name")).toThrow();
expect(() => validateBranchName("feature branch")).toThrow();
});
it("should reject newlines and tabs", () => {
expect(() => validateBranchName("branch\nname")).toThrow();
expect(() => validateBranchName("branch\tname")).toThrow();
});
});
describe("empty and whitespace", () => {
it("should reject empty strings", () => {
expect(() => validateBranchName("")).toThrow(/cannot be empty/);
});
it("should reject whitespace-only strings", () => {
expect(() => validateBranchName(" ")).toThrow();
expect(() => validateBranchName("\t\n")).toThrow();
});
});
describe("edge cases", () => {
it("should accept single alphanumeric character", () => {
expect(() => validateBranchName("a")).not.toThrow();
expect(() => validateBranchName("1")).not.toThrow();
});
it("should reject single special characters", () => {
expect(() => validateBranchName(".")).toThrow();
expect(() => validateBranchName("/")).toThrow();
expect(() => validateBranchName("-")).toThrow();
});
});
});