Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 33 additions & 1 deletion src/entrypoints/run.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,12 +15,20 @@ import { setupGitHubToken, WorkflowValidationSkipError } from "../github/token";
import { checkWritePermissions } from "../github/validation/permissions";
import { createOctokit } from "../github/api/client";
import type { Octokits } from "../github/api/client";
import { parseGitHubContext, isEntityContext } from "../github/context";
import {
parseGitHubContext,
isEntityContext,
isPullRequestEvent,
isPullRequestReviewEvent,
isPullRequestReviewCommentEvent,
} from "../github/context";
import type { GitHubContext } from "../github/context";
import { detectMode } from "../modes/detector";
import { prepareTagMode } from "../modes/tag";
import { prepareAgentMode } from "../modes/agent";
import { checkContainsTrigger } from "../github/validation/trigger";
import { restoreConfigFromBase } from "../github/operations/restore-config";
import { validateBranchName } from "../github/operations/branch";
import { collectActionInputsPresence } from "./collect-inputs";
import { updateCommentLink } from "./update-comment-link";
import { formatTurnsFromData } from "./format-turns";
Expand Down Expand Up @@ -217,6 +225,30 @@ async function run() {

validateEnvironmentVariables();

// On PRs, .claude/ and .mcp.json in the checkout are attacker-controlled.
// Restore them from the base branch before the CLI reads them.
//
// We read pull_request.base.ref from the payload directly because agent
// mode's branchInfo.baseBranch defaults to "main" rather than the PR's
// actual target (agent/index.ts). For issue_comment on a PR the payload
// lacks base.ref, so we fall back to the mode-provided value — tag mode
// fetches it from GraphQL; agent mode on issue_comment is an edge case
// that at worst restores from the wrong trusted branch (still secure).
if (isEntityContext(context) && context.isPR) {
let restoreBase = baseBranch;
if (
isPullRequestEvent(context) ||
isPullRequestReviewEvent(context) ||
isPullRequestReviewCommentEvent(context)
) {
restoreBase = context.payload.pull_request.base.ref;
validateBranchName(restoreBase);
}
if (restoreBase) {
restoreConfigFromBase(restoreBase);
}
}
Comment on lines +237 to +250
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 restoreConfigFromBase requires its parameter to be pre-validated (per its docstring), and validateBranchName() is called on the pull_request event path (line 245) but not on the issue_comment fallback path where restoreBase = baseBranch from agent mode. Adding validateBranchName(restoreBase) before line 248 would close this defense-in-depth gap, though practical risk is near-zero since the fallback value comes from admin-controlled sources.

Extended reasoning...

The Bug

When the event is issue_comment on a PR, the code at lines 237-250 takes the fallback path where restoreBase is set to baseBranch (from the mode prepare function) rather than reading context.payload.pull_request.base.ref. In agent mode, this baseBranch value comes from process.env.BASE_BRANCH || context.inputs.baseBranch || "main" (agent/index.ts:88-89) and is never passed through validateBranchName().

The Inconsistency

The restoreConfigFromBase docstring at restore-config.ts:25-26 explicitly states: "Must be pre-validated (branch.ts calls validateBranchName on it before returning)." The PR event path correctly calls validateBranchName(restoreBase) at line 245, but the issue_comment fallback skips this validation entirely. For tag mode this is fine because setupBranch validates baseBranch at branch.ts:176, but agent mode does not validate its baseBranch anywhere in its pipeline.

Step-by-Step Proof

  1. An issue_comment event fires on a PR in a repo using agent mode.
  2. prepareAgentMode runs and computes baseBranch = process.env.BASE_BRANCH || context.inputs.baseBranch || "main" (agent/index.ts:88-89).
  3. At run.ts:237, isEntityContext(context) && context.isPR is true, so we enter the block.
  4. At line 238, restoreBase = baseBranch (the unvalidated value from agent mode).
  5. The isPullRequestEvent / isPullRequestReviewEvent / isPullRequestReviewCommentEvent checks all fail (this is an issue_comment event), so the inner block with validateBranchName at line 245 is skipped.
  6. At line 248, restoreConfigFromBase(restoreBase) is called with the unvalidated baseBranch.

Practical Impact

The practical risk is effectively zero. The BASE_BRANCH env var is set in the workflow YAML (which runs from the default branch, not the PR branch), context.inputs.baseBranch resolves to the same process.env.BASE_BRANCH (context.ts:150), and the hardcoded default is "main". None of these are attacker-controlled. Additionally, execFileSync with array args prevents shell injection. However, validateBranchName also blocks leading dashes (preventing git option injection like --upload-pack=...), so including it would be a worthwhile defense-in-depth measure.

Suggested Fix

Add validateBranchName(restoreBase) before the restoreConfigFromBase(restoreBase) call at line 248, or move the validation outside the event-type if block so it applies to all paths uniformly:

if (restoreBase) {
  validateBranchName(restoreBase);
  restoreConfigFromBase(restoreBase);
}


await setupClaudeCodeSettings(process.env.INPUT_SETTINGS);

await installPlugins(
Expand Down
79 changes: 79 additions & 0 deletions src/github/operations/restore-config.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,79 @@
import { execFileSync } from "child_process";
import { rmSync } from "fs";

// Paths that are both PR-controllable and read from cwd at CLI startup.
//
// Deliberately excluded from the CLI's broader auto-edit blocklist:
// .git/ — not tracked by git; PR commits cannot place files there.
// Restoring it would also undo the PR checkout entirely.
// .gitconfig — git reads ~/.gitconfig and .git/config, never cwd/.gitconfig.
// .bashrc etc. — shells source these from $HOME; checkout cannot reach $HOME.
// .vscode/.idea— IDE config; nothing in the CLI's startup path reads them.
const SENSITIVE_PATHS = [
".claude",
".mcp.json",
".claude.json",
".gitmodules",
".ripgreprc",
];

/**
* Restores security-sensitive config paths from the PR base branch.
*
* The CLI's non-interactive mode trusts cwd: it reads `.mcp.json`,
* `.claude/settings.json`, and `.claude/settings.local.json` from the working
* directory and acts on them before any tool-permission gating — executing
* hooks (including SessionStart), setting env vars (NODE_OPTIONS, LD_PRELOAD,
* PATH), running apiKeyHelper/awsAuthRefresh shell commands, and auto-approving
* MCP servers. When this action checks out a PR head, all of these are
* attacker-controlled.
*
* Rather than enumerate every dangerous key, this replaces the entire `.claude/`
* tree and `.mcp.json` with the versions from the PR base branch, which a
* maintainer has reviewed and merged. Paths absent on base are deleted.
*
* Known limitation: if a PR legitimately modifies `.claude/` and the CLI later
* commits with `git add -A`, the revert will be included in that commit. This
* is a narrow UX tradeoff for closing the RCE surface.
*
* @param baseBranch - PR base branch name. Must be pre-validated (branch.ts
* calls validateBranchName on it before returning).
*/
export function restoreConfigFromBase(baseBranch: string): void {
console.log(
`Restoring ${SENSITIVE_PATHS.join(", ")} from origin/${baseBranch} (PR head is untrusted)`,
);

// Fetch base first — if this fails we haven't touched the workspace and the
// caller sees a clean error.
execFileSync("git", ["fetch", "origin", baseBranch, "--depth=1"], {
stdio: "inherit",
});

// Delete PR-controlled versions. If the restore below fails for a given path,
// that path stays deleted — the safe fallback (no attacker-controlled config).
// A bare `git checkout` alone wouldn't remove files the PR added, so nuke first.
for (const p of SENSITIVE_PATHS) {
rmSync(p, { recursive: true, force: true });
}

for (const p of SENSITIVE_PATHS) {
try {
execFileSync("git", ["checkout", `origin/${baseBranch}`, "--", p], {
stdio: "pipe",
});
} catch {
// Path doesn't exist on base — it stays deleted.
}
}

// `git checkout <ref> -- <path>` stages the restored files. Unstage so the
// revert doesn't silently leak into commits the CLI makes later.
try {
execFileSync("git", ["reset", "--", ...SENSITIVE_PATHS], {
stdio: "pipe",
});
} catch {
// Nothing was staged, or paths don't exist on HEAD — either is fine.
}
}
Loading