diff --git a/plugins/compound-engineering/skills/ce-code-review/SKILL.md b/plugins/compound-engineering/skills/ce-code-review/SKILL.md index f574bec79..bd24b72a0 100644 --- a/plugins/compound-engineering/skills/ce-code-review/SKILL.md +++ b/plugins/compound-engineering/skills/ce-code-review/SKILL.md @@ -298,9 +298,10 @@ Then detect the review base branch and compute the merge-base. Run the bundled ` RESOLVE_SCRIPT="" if [ -n "${CLAUDE_SKILL_DIR:-}" ] && [ -f "${CLAUDE_SKILL_DIR}/scripts/resolve-base.sh" ]; then real_base=$(cd "${CLAUDE_SKILL_DIR}" 2>/dev/null && pwd -P) || real_base="" - real_cwd=$(pwd -P) + repo_top=$(git rev-parse --show-toplevel 2>/dev/null) || repo_top="" + if [ -n "$repo_top" ]; then real_repo=$(cd "$repo_top" 2>/dev/null && pwd -P) || real_repo=""; else real_repo=$(pwd -P); fi case "$real_base" in - ""|"$real_cwd"|"$real_cwd"/*) ;; + ""|"$real_repo"|"$real_repo"/*) ;; *) RESOLVE_SCRIPT="$real_base/scripts/resolve-base.sh" ;; esac fi @@ -328,9 +329,10 @@ Detect the review base branch and compute the merge-base using the same bundled RESOLVE_SCRIPT="" if [ -n "${CLAUDE_SKILL_DIR:-}" ] && [ -f "${CLAUDE_SKILL_DIR}/scripts/resolve-base.sh" ]; then real_base=$(cd "${CLAUDE_SKILL_DIR}" 2>/dev/null && pwd -P) || real_base="" - real_cwd=$(pwd -P) + repo_top=$(git rev-parse --show-toplevel 2>/dev/null) || repo_top="" + if [ -n "$repo_top" ]; then real_repo=$(cd "$repo_top" 2>/dev/null && pwd -P) || real_repo=""; else real_repo=$(pwd -P); fi case "$real_base" in - ""|"$real_cwd"|"$real_cwd"/*) ;; + ""|"$real_repo"|"$real_repo"/*) ;; *) RESOLVE_SCRIPT="$real_base/scripts/resolve-base.sh" ;; esac fi diff --git a/tests/review-skill-contract.test.ts b/tests/review-skill-contract.test.ts index c3f338f3b..254d22be7 100644 --- a/tests/review-skill-contract.test.ts +++ b/tests/review-skill-contract.test.ts @@ -680,6 +680,10 @@ describe("ce-code-review contract", () => { // the base is unresolved. expect(content).toContain('"${CLAUDE_SKILL_DIR:-}"') expect(content).toContain('[ -f "${CLAUDE_SKILL_DIR}/scripts/resolve-base.sh" ]') + // Reject helper paths against the reviewed-repo top-level, not just CWD -- + // otherwise a launch from a monorepo subdirectory leaves repo-controlled + // sibling paths (e.g., `/.evil`) accepted. + expect(content).toContain("git rev-parse --show-toplevel") // No bare relative-path fallback to `scripts/resolve-base.sh` -- that would resolve // against the reviewed repo's CWD and could execute a repo-controlled script. expect(content).not.toContain('RESOLVE_SCRIPT="scripts/resolve-base.sh"') @@ -750,6 +754,32 @@ describe("ce-code-review contract", () => { expect(evil.status).not.toBe(0) expect(evil.output).toContain("Re-run with `base:`") expect(evil.output).not.toContain("from-cwd-evil") + + // Monorepo subdir attack: reviewer launched from a subdirectory of the reviewed + // repo. CLAUDE_SKILL_DIR points at a sibling of the subdir (e.g., `/.evil`) + // -- it lives outside the CWD subdir but is still repo-controlled. The guard must + // reject it by comparing against the repo top-level, not just the CWD. + const monorepoRoot = fs.mkdtempSync(path.join(os.tmpdir(), "ce-code-review-monorepo-")) + try { + const subDir = path.join(monorepoRoot, "services", "foo") + const sneakSkill = path.join(monorepoRoot, ".evil") + fs.mkdirSync(subDir, { recursive: true }) + fs.mkdirSync(path.join(sneakSkill, "scripts"), { recursive: true }) + fs.writeFileSync( + path.join(sneakSkill, "scripts", "resolve-base.sh"), + "echo BASE:from-monorepo-evil\n", + ) + execSync("git init -q", { cwd: monorepoRoot }) + + const realSubDir = fs.realpathSync(subDir) + const realSneakSkill = fs.realpathSync(sneakSkill) + const monorepo = runResolveProbe(snippet, realSubDir, { CLAUDE_SKILL_DIR: realSneakSkill }) + expect(monorepo.status).not.toBe(0) + expect(monorepo.output).toContain("Re-run with `base:`") + expect(monorepo.output).not.toContain("from-monorepo-evil") + } finally { + fs.rmSync(monorepoRoot, { recursive: true, force: true }) + } } finally { fs.rmSync(tempDir, { recursive: true, force: true }) }