diff --git a/plugins/codex/scripts/lib/git.mjs b/plugins/codex/scripts/lib/git.mjs index 08290cbf..161f9b9b 100644 --- a/plugins/codex/scripts/lib/git.mjs +++ b/plugins/codex/scripts/lib/git.mjs @@ -328,15 +328,31 @@ export function createWorktree(repoRoot) { ); } - // Ensure .worktrees/ is excluded from the target repo without modifying tracked files. - // Use git rev-parse to resolve the real git dir (handles linked worktrees where .git is a file). - const rawGitDir = gitChecked(repoRoot, ["rev-parse", "--git-dir"]).stdout.trim(); - const gitDir = path.resolve(repoRoot, rawGitDir); - const excludePath = path.join(gitDir, "info", "exclude"); - const excludeContent = fs.existsSync(excludePath) ? fs.readFileSync(excludePath, "utf8") : ""; - if (!excludeContent.includes(".worktrees")) { - fs.mkdirSync(path.dirname(excludePath), { recursive: true }); - fs.appendFileSync(excludePath, `${excludeContent.endsWith("\n") || !excludeContent ? "" : "\n"}.worktrees/\n`); + // SECURITY (#16, redesign — eliminate, not detect): the original code wrote + // `.worktrees/` into /info/exclude to keep the worktree dir out of + // `git status`. That write was the entire #16 vulnerability: if gitDir resolved + // outside repoRoot (separate-git-dir, crafted .git gitfile, linked worktree), + // the exclude write landed at an attacker-controlled location. Detection-based + // fixes (boundary checks) broke legit linked worktrees and couldn't distinguish + // them from attack — git introspection can't authenticate external common-dirs. + // + // Eliminate the write entirely: git worktree add works WITHOUT the exclude + // (it was cosmetic, added in openai#137 commit 4bca062). Instead, keep + // `.worktrees/` out of git status via an in-repo `.worktrees/.gitignore` + // containing `*`, written inside the already-validated worktreesDir (#14 guard + // ensures it's inside repoRoot and not a symlink). No git-dir metadata write. + const dotGitignore = path.join(worktreesDir, ".gitignore"); + // SECURITY: if .gitignore already exists as a symlink (crafted repo ships it as + // a dangling symlink to an external path), existsSync follows it and returns + // false, then writeFileSync would create a file at the symlink target outside + // repoRoot. lstat (not stat/existsSync) detects the symlink itself. + if (fs.existsSync(dotGitignore) && fs.lstatSync(dotGitignore).isSymbolicLink()) { + throw new Error( + `Refusing to create worktree: ${dotGitignore} is a symlink (possible redirect attempt). Remove it manually if expected.` + ); + } + if (!fs.existsSync(dotGitignore)) { + fs.writeFileSync(dotGitignore, "*\n", "utf8"); } const worktreePath = path.join(worktreesDir, `codex-${ts}`); diff --git a/tests/worktree.test.mjs b/tests/worktree.test.mjs index 6f3bd530..25ac5947 100644 --- a/tests/worktree.test.mjs +++ b/tests/worktree.test.mjs @@ -415,3 +415,51 @@ test("keep applies a retargeted existing symlink as a regular file (no host redi removeSession(session); } }); + +// SECURITY regression (#16, redesign — eliminate info/exclude write): createWorktree +// no longer writes to /info/exclude. The #16 vulnerability (git-dir outside +// repoRoot → exclude write at attacker location) is eliminated by not writing to +// git-dir metadata. A separate-git-dir repo (previously the attack case) now works. +test("createWorktreeSession works with separate-git-dir (no info/exclude write)", () => { + const repoRoot = makeTempDir(); + const externalGitDir = makeTempDir("external-gitdir-"); + run("git", ["init", "-q", "-b", "main", `--separate-git-dir=${externalGitDir}`, repoRoot]); + run("git", ["config", "user.name", "Test"], { cwd: repoRoot }); + run("git", ["config", "user.email", "test@test"], { cwd: repoRoot }); + fs.writeFileSync(path.join(repoRoot, "app.js"), "export const v = 1;\n"); + run("git", ["add", "app.js"], { cwd: repoRoot }); + run("git", ["commit", "-q", "-m", "init"], { cwd: repoRoot }); + + const session = createWorktreeSession(repoRoot); + try { + assert.ok(session.worktreePath); + assert.match(session.branch, /^codex\/\d+$/); + const excludePath = path.join(externalGitDir, "info", "exclude"); + if (fs.existsSync(excludePath)) { + assert.doesNotMatch( + fs.readFileSync(excludePath, "utf8"), + /\.worktrees/, + "external info/exclude must not contain .worktrees (no git-dir write)" + ); + } + } finally { + removeSession(session); + } +}); + +// Regression: linked worktree (gitdir in main .git/) works — no git-dir metadata +// write means no boundary check needed, no legit-layout breakage. +test("createWorktreeSession works from a linked worktree", () => { + const { repoRoot } = createRepoWithInitialCommit(); + const linkedWt = makeTempDir("linked-wt-"); + assert.equal(run("git", ["worktree", "add", linkedWt, "-b", "linked"], { cwd: repoRoot }).status, 0); + + const session = createWorktreeSession(linkedWt); + try { + assert.ok(session.worktreePath); + assert.match(session.branch, /^codex\/\d+$/); + } finally { + removeSession(session); + run("git", ["worktree", "remove", "--force", linkedWt], { cwd: repoRoot }); + } +});