From b6fcc1bb492caa7c8a1321ac8fd22ebef8b933cd Mon Sep 17 00:00:00 2001 From: Tom Boucher Date: Sun, 5 Apr 2026 00:51:19 -0400 Subject: [PATCH] fix(gsd): worktree teardown path validation prevents data loss (#3311) * fix(gsd): validate worktree paths before removal to prevent data loss Fixes #2365 removeWorktree() overrides its computed path with whatever git reports from `git worktree list`. When .gsd/ was a symlink, git resolves it at creation time, so the registered path can point to an external directory. Teardown with --force on that path destroys user data. Add isInsideWorktreesDir() containment check that resolves ".." traversals and validates the target is under .gsd/worktrees/ before any destructive operation (nativeWorktreeRemove --force, rmSync). Paths outside containment get a non-force git worktree remove only, with a warning logged. Also guard the fallback rmSync in teardownAutoWorktree() with the same containment check. Co-Authored-By: Claude Opus 4.6 (1M context) * refactor: convert worktree-teardown-safety test to node:test format Replace main() function wrapper pattern with proper describe/it blocks using node:test, matching the project's standard test conventions. Co-Authored-By: Claude Opus 4.6 * fix: replace empty catch blocks with logWarning in worktree-manager * fix(test): increase removeWorktree slice window from 3000 to 6000 chars The PR's path-validation additions expanded removeWorktree beyond the 3000-char snapshot window, pushing the submodule safety section out of scope and causing Tests 3 and 4 to see empty strings. Co-Authored-By: Claude Sonnet 4.6 --------- Co-authored-by: Claude Opus 4.6 (1M context) Co-authored-by: trek-e --- src/resources/extensions/gsd/auto-worktree.ts | 20 ++- .../tests/worktree-submodule-safety.test.ts | 2 +- .../tests/worktree-teardown-safety.test.ts | 148 ++++++++++++++++++ .../extensions/gsd/worktree-manager.ts | 108 +++++++++---- 4 files changed, 241 insertions(+), 37 deletions(-) create mode 100644 src/resources/extensions/gsd/tests/worktree-teardown-safety.test.ts diff --git a/src/resources/extensions/gsd/auto-worktree.ts b/src/resources/extensions/gsd/auto-worktree.ts index 542a684a2..a0275619a 100644 --- a/src/resources/extensions/gsd/auto-worktree.ts +++ b/src/resources/extensions/gsd/auto-worktree.ts @@ -36,6 +36,7 @@ import { removeWorktree, resolveGitDir, worktreePath, + isInsideWorktreesDir, } from "./worktree-manager.js"; import { detectWorktreeName, @@ -1204,12 +1205,19 @@ export function teardownAutoWorktree( `Remove it manually with: rm -rf "${wtDir.replaceAll("\\", "/")}"`, { worktree: milestoneId }, ); - // Attempt a direct filesystem removal as a fallback - try { - rmSync(wtDir, { recursive: true, force: true }); - } catch (err) { - // Non-fatal — the warning above tells the user how to clean up - logWarning("worktree", `worktree directory removal failed: ${err instanceof Error ? err.message : String(err)}`); + // Attempt a direct filesystem removal as a fallback — but ONLY if the + // path is safely inside .gsd/worktrees/ to prevent #2365 data loss. + if (isInsideWorktreesDir(originalBasePath, wtDir)) { + try { + rmSync(wtDir, { recursive: true, force: true }); + } catch (err) { + // Non-fatal — the warning above tells the user how to clean up + logWarning("worktree", `worktree directory removal failed: ${err instanceof Error ? err.message : String(err)}`); + } + } else { + console.error( + `[GSD] REFUSING fallback rmSync — path is outside .gsd/worktrees/: ${wtDir}`, + ); } } } diff --git a/src/resources/extensions/gsd/tests/worktree-submodule-safety.test.ts b/src/resources/extensions/gsd/tests/worktree-submodule-safety.test.ts index c32b8fe80..7414705f5 100644 --- a/src/resources/extensions/gsd/tests/worktree-submodule-safety.test.ts +++ b/src/resources/extensions/gsd/tests/worktree-submodule-safety.test.ts @@ -22,7 +22,7 @@ console.log("\n=== #2337: Worktree teardown preserves submodule state ==="); const removeWorktreeIdx = src.indexOf("export function removeWorktree"); assertTrue(removeWorktreeIdx > 0, "worktree-manager.ts exports removeWorktree"); -const fnBody = src.slice(removeWorktreeIdx, removeWorktreeIdx + 3000); +const fnBody = src.slice(removeWorktreeIdx, removeWorktreeIdx + 6000); // ── Test 2: The function checks for submodules before force removal ───── diff --git a/src/resources/extensions/gsd/tests/worktree-teardown-safety.test.ts b/src/resources/extensions/gsd/tests/worktree-teardown-safety.test.ts new file mode 100644 index 000000000..e6f9ef134 --- /dev/null +++ b/src/resources/extensions/gsd/tests/worktree-teardown-safety.test.ts @@ -0,0 +1,148 @@ +/** + * worktree-teardown-safety.test.ts — Regression test for #2365. + * + * Ensures that removeWorktree() and teardownAutoWorktree() never delete + * directories outside .gsd/worktrees/. The bug: removeWorktree overrides + * the computed worktree path with whatever `git worktree list` reports. + * When .gsd/ was (or is) a symlink, git resolves the symlink at creation + * time, so its registered path can point to an external directory. If that + * external path happens to be a project data directory, teardown destroys it. + * + * The fix adds path validation so rmSync / nativeWorktreeRemove only operate + * on paths that are actually under .gsd/worktrees/. + */ + +import { + mkdtempSync, + mkdirSync, + writeFileSync, + rmSync, + existsSync, + realpathSync, + readFileSync, +} from "node:fs"; +import { join } from "node:path"; +import { tmpdir } from "node:os"; +import { execSync } from "node:child_process"; +import { describe, it, after } from "node:test"; + +import { createWorktree, removeWorktree, worktreePath, isInsideWorktreesDir } from "../worktree-manager.ts"; +import { createTestContext } from "./test-helpers.ts"; + +const { assertEq, assertTrue, report } = createTestContext(); + +// ─── Helpers ────────────────────────────────────────────────────────────── + +function run(command: string, cwd: string): string { + return execSync(command, { cwd, stdio: ["ignore", "pipe", "pipe"], encoding: "utf-8" }).trim(); +} + +function createTempRepo(): string { + const dir = realpathSync(mkdtempSync(join(tmpdir(), "wt-safety-test-"))); + run("git init", dir); + run("git config user.email test@test.com", dir); + run("git config user.name Test", dir); + writeFileSync(join(dir, "README.md"), "# test\n"); + run("git add .", dir); + run("git commit -m init", dir); + run("git branch -M main", dir); + return dir; +} + +// ─── Tests ──────────────────────────────────────────────────────────────── + +describe("worktree-teardown-safety", () => { + const dirs: string[] = []; + + after(() => { + for (const d of dirs) rmSync(d, { recursive: true, force: true }); + report(); + }); + + it("removeWorktree does not delete sibling data directories", () => { + const tempDir = createTempRepo(); + dirs.push(tempDir); + + // Create a project data directory that lives alongside .gsd/ + const dataDir = join(tempDir, "project-data"); + mkdirSync(dataDir, { recursive: true }); + writeFileSync(join(dataDir, "important.db"), "precious data"); + + // Create a worktree normally + const wt = createWorktree(tempDir, "test-wt"); + assertTrue(existsSync(wt.path), "worktree created successfully"); + + // Remove the worktree + removeWorktree(tempDir, "test-wt"); + + // The worktree directory should be gone + assertTrue(!existsSync(wt.path), "worktree directory removed"); + + // The project data directory MUST still exist + assertTrue(existsSync(dataDir), "project data directory survives teardown"); + assertTrue( + existsSync(join(dataDir, "important.db")), + "project data files survive teardown", + ); + }); + + it("path validation rejects paths outside .gsd/worktrees/", () => { + const tempDir = createTempRepo(); + dirs.push(tempDir); + + const externalDir = join(tempDir, "external-state"); + mkdirSync(externalDir, { recursive: true }); + writeFileSync(join(externalDir, "state.json"), '{"critical": true}'); + + // Create and then remove a worktree that has a legitimate path + const wt2 = createWorktree(tempDir, "safe-wt"); + assertTrue(existsSync(wt2.path), "second worktree created"); + + removeWorktree(tempDir, "safe-wt"); + assertTrue(!existsSync(wt2.path), "second worktree removed cleanly"); + + // External directory must be untouched + assertTrue(existsSync(externalDir), "external directory survives second teardown"); + assertEq( + readFileSync(join(externalDir, "state.json"), "utf-8"), + '{"critical": true}', + "external directory contents intact after teardown", + ); + }); + + it("worktreePath always returns paths under .gsd/worktrees/", () => { + const tempDir = createTempRepo(); + dirs.push(tempDir); + + const wtPathResult = worktreePath(tempDir, "anything"); + assertTrue( + wtPathResult.startsWith(join(tempDir, ".gsd", "worktrees")), + "worktreePath returns path under .gsd/worktrees/", + ); + }); + + it("isInsideWorktreesDir rejects path traversal attempts", () => { + const tempDir = createTempRepo(); + dirs.push(tempDir); + + assertTrue( + isInsideWorktreesDir(tempDir, join(tempDir, ".gsd", "worktrees", "my-wt")), + "path inside .gsd/worktrees/ is accepted", + ); + + assertTrue( + !isInsideWorktreesDir(tempDir, join(tempDir, "project-data")), + "path outside .gsd/worktrees/ is rejected", + ); + + assertTrue( + !isInsideWorktreesDir(tempDir, join(tempDir, ".gsd", "worktrees", "..", "..", "project-data")), + "path traversal via .. is rejected", + ); + + assertTrue( + !isInsideWorktreesDir(tempDir, "/tmp/some-other-dir"), + "completely external path is rejected", + ); + }); +}); diff --git a/src/resources/extensions/gsd/worktree-manager.ts b/src/resources/extensions/gsd/worktree-manager.ts index 80be36e41..b73e4e5ee 100644 --- a/src/resources/extensions/gsd/worktree-manager.ts +++ b/src/resources/extensions/gsd/worktree-manager.ts @@ -113,6 +113,22 @@ export function worktreeBranchName(name: string): string { return `worktree/${name}`; } +/** + * Validate that a path is inside the .gsd/worktrees/ directory. + * Resolves symlinks and normalizes ".." traversals before comparison + * so that a symlink-resolved or crafted path cannot escape containment. + * + * Used as a safety gate before any destructive operation (rmSync, + * nativeWorktreeRemove --force) to prevent #2365-style data loss. + */ +export function isInsideWorktreesDir(basePath: string, targetPath: string): boolean { + const wtDir = resolve(worktreesDir(basePath)); + const resolved = resolve(targetPath); + // The resolved path must start with the worktrees dir followed by a separator, + // not merely be a prefix match (e.g. ".gsd/worktrees-extra" must not match). + return resolved === wtDir || resolved.startsWith(wtDir + sep); +} + // ─── Core Operations ─────────────────────────────────────────────────────── /** @@ -370,16 +386,37 @@ export function removeWorktree( // time, so its registered path points to the resolved external location. // If syncStateToProjectRoot later creates a real .gsd/ directory that // shadows the symlink, the computed path diverges from git's record. + let gitReportedPath: string | null = null; try { const entries = nativeWorktreeList(basePath); const entry = entries.find(e => e.branch === branch); if (entry?.path) { - wtPath = entry.path; + gitReportedPath = entry.path; } } catch (e) { logWarning("worktree", `nativeWorktreeList parse failed: ${(e as Error).message}`); } + // Safety gate (#2365): only use the git-reported path if it is actually + // inside .gsd/worktrees/. When .gsd/ was a symlink, git may have resolved + // it to an external directory (e.g. a project data folder). Using that + // path for removal would destroy user data. + if (gitReportedPath && isInsideWorktreesDir(basePath, gitReportedPath)) { + wtPath = gitReportedPath; + } else if (gitReportedPath) { + console.error( + `[GSD] WARNING: git worktree list reported path outside .gsd/worktrees/: ${gitReportedPath}\n` + + ` Refusing to use it for removal — falling back to computed path: ${wtPath}`, + ); + // Still tell git to unregister the worktree entry via its reported path, + // but do NOT use force and do NOT fall back to rmSync on this path. + try { nativeWorktreeRemove(basePath, gitReportedPath, false); } catch (e) { logWarning("worktree", `non-force worktree remove failed for ${gitReportedPath}: ${e instanceof Error ? e.message : String(e)}`); } + } + const resolvedWtPath = existsSync(wtPath) ? realpathSync(wtPath) : wtPath; + // Double-check: the resolved path (after symlink resolution) must also be + // inside .gsd/worktrees/ — a symlink inside the directory could point out. + const resolvedPathSafe = isInsideWorktreesDir(basePath, resolvedWtPath); + // If we're inside the worktree, move out first — git can't remove an in-use directory const cwd = process.cwd(); const resolvedCwd = existsSync(cwd) ? realpathSync(cwd) : cwd; @@ -453,37 +490,48 @@ export function removeWorktree( } } - // Remove worktree: try non-force first when submodules have changes, - // falling back to force only after submodule state has been preserved. - const useForce = hasSubmoduleChanges ? false : force; - try { nativeWorktreeRemove(basePath, resolvedWtPath, useForce); } catch (e) { logWarning("worktree", `nativeWorktreeRemove failed: ${(e as Error).message}`); } + // Remove worktree — only use force/rmSync when the path is safely contained + if (resolvedPathSafe) { + // Remove worktree: try non-force first when submodules have changes, + // falling back to force only after submodule state has been preserved. + const useForce = hasSubmoduleChanges ? false : force; + try { nativeWorktreeRemove(basePath, resolvedWtPath, useForce); } catch (e) { logWarning("worktree", `nativeWorktreeRemove failed: ${(e as Error).message}`); } - // If the directory is still there (e.g. locked), try harder with force - if (existsSync(resolvedWtPath)) { - try { nativeWorktreeRemove(basePath, resolvedWtPath, true); } catch (e) { logWarning("worktree", `nativeWorktreeRemove (force) failed: ${(e as Error).message}`); } - } - - // (#2821) If the worktree directory STILL exists after both native removal - // attempts (e.g. untracked files like ASSESSMENT/UAT-RESULT prevent git - // worktree remove), force-remove the git internal worktree metadata first, - // then remove the filesystem directory. Without this, the .git/worktrees/ - // lock prevents rmSync from cleaning up, and the orphaned worktree directory - // causes every subsequent `/gsd auto` to re-enter the stale worktree. - if (existsSync(resolvedWtPath)) { - try { - const wtInternalDir = join(basePath, ".git", "worktrees", name); - if (existsSync(wtInternalDir)) { - rmSync(wtInternalDir, { recursive: true, force: true }); - } - rmSync(resolvedWtPath, { recursive: true, force: true }); - } catch { - logWarning( - "reconcile", - `Worktree directory could not be removed after git internal cleanup: ${resolvedWtPath}. ` + - `Manual cleanup: rm -rf "${resolvedWtPath.replaceAll("\\", "/")}"`, - { worktree: name }, - ); + // If the directory is still there (e.g. locked), try harder with force + if (existsSync(resolvedWtPath)) { + try { nativeWorktreeRemove(basePath, resolvedWtPath, true); } catch (e) { logWarning("worktree", `nativeWorktreeRemove (force) failed: ${(e as Error).message}`); } } + + // (#2821) If the worktree directory STILL exists after both native removal + // attempts (e.g. untracked files like ASSESSMENT/UAT-RESULT prevent git + // worktree remove), force-remove the git internal worktree metadata first, + // then remove the filesystem directory. Without this, the .git/worktrees/ + // lock prevents rmSync from cleaning up, and the orphaned worktree directory + // causes every subsequent `/gsd auto` to re-enter the stale worktree. + if (existsSync(resolvedWtPath)) { + try { + const wtInternalDir = join(basePath, ".git", "worktrees", name); + if (existsSync(wtInternalDir)) { + rmSync(wtInternalDir, { recursive: true, force: true }); + } + rmSync(resolvedWtPath, { recursive: true, force: true }); + } catch { + logWarning( + "reconcile", + `Worktree directory could not be removed after git internal cleanup: ${resolvedWtPath}. ` + + `Manual cleanup: rm -rf "${resolvedWtPath.replaceAll("\\", "/")}"`, + { worktree: name }, + ); + } + } + } else { + // Path is outside containment — only do a non-force git worktree remove + // (which refuses to delete dirty worktrees) and never fall back to rmSync. + console.error( + `[GSD] WARNING: Resolved worktree path is outside .gsd/worktrees/: ${resolvedWtPath}\n` + + ` Skipping forced removal to prevent data loss.`, + ); + try { nativeWorktreeRemove(basePath, resolvedWtPath, false); } catch (e) { logWarning("worktree", `non-force worktree remove failed for ${resolvedWtPath}: ${e instanceof Error ? e.message : String(e)}`); } } // Prune stale entries so git knows the worktree is gone