refactor(ShadowCheckpointService): updating template config, and removed env vars

Author: edelauna

src/services/checkpoints/ShadowCheckpointService.ts

@@ -16,6 +16,50 @@ import { t } from "../../i18n"
 import { CheckpointDiff, CheckpointResult, CheckpointEventMap } from "./types"
 import { getExcludePatterns } from "./excludes"
 
+/**
+ * Environment variables stripped before passing the env to simple-git.
+ *
+ * Two categories:
+ *  - Location-override vars (GIT_DIR, GIT_WORK_TREE, GIT_INDEX_FILE, GIT_OBJECT_DIRECTORY,
+ *    GIT_ALTERNATE_OBJECT_DIRECTORIES, GIT_CEILING_DIRECTORIES): redirect git operations to
+ *    unintended repositories or limit where git searches.
+ *  - Code-execution vectors blocked by simple-git ≥3.36's blockUnsafeOperationsPlugin when
+ *    passed via .env(): GIT_EDITOR, GIT_SSH_COMMAND, GIT_PAGER, PREFIX, etc.
+ *
+ * Stripping GIT_CONFIG_COUNT also neutralises the entire GIT_CONFIG_KEY_n / GIT_CONFIG_VALUE_n
+ * family — git ignores those per-key entries when the count key is absent.
+ */
+export const BLOCKED_ENV_KEYS = new Set([
+	"GIT_DIR",
+	"GIT_WORK_TREE",
+	"GIT_INDEX_FILE",
+	"GIT_OBJECT_DIRECTORY",
+	"GIT_ALTERNATE_OBJECT_DIRECTORIES",
+	"GIT_CEILING_DIRECTORIES",
+	"GIT_TEMPLATE_DIR",
+	"GIT_EDITOR",
+	"GIT_SEQUENCE_EDITOR",
+	"GIT_ASKPASS",
+	"GIT_SSH",
+	"GIT_SSH_COMMAND",
+	"GIT_PAGER",
+	"GIT_PROXY_COMMAND",
+	"GIT_EXEC_PATH",
+	"GIT_EXTERNAL_DIFF",
+	"GIT_CONFIG",
+	"GIT_CONFIG_GLOBAL",
+	"GIT_CONFIG_SYSTEM",
+	"GIT_CONFIG_COUNT",
+	"PREFIX",
+	"EDITOR",
+	"PAGER",
+	"SSH_ASKPASS",
+])
+
+// Lowercase set for case-insensitive lookup — the plugin uses toLowerCase() internally,
+// so a var like Git_Editor would bypass an exact-match check on Linux.
+const BLOCKED_ENV_KEYS_LOWER = new Set([...BLOCKED_ENV_KEYS].map((k) => k.toLowerCase()))
+
 /**
  * Creates a SimpleGit instance with sanitized environment variables to prevent
  * interference from inherited git environment variables like GIT_DIR and GIT_WORK_TREE.
@@ -25,43 +69,35 @@ import { getExcludePatterns } from "./excludes"
  * @returns A SimpleGit instance with sanitized environment
  */
 function createSanitizedGit(baseDir: string): SimpleGit {
-	// Create a clean environment by explicitly unsetting git-related environment variables
-	// that could interfere with checkpoint operations
 	const sanitizedEnv: Record<string, string> = {}
-	const removedVars: string[] = []
+	const removedKeys: string[] = []
 
-	// Copy all environment variables except git-specific ones
 	for (const [key, value] of Object.entries(process.env)) {
-		// Skip git environment variables that would override repository location
-		if (
-			key === "GIT_DIR" ||
-			key === "GIT_WORK_TREE" ||
-			key === "GIT_INDEX_FILE" ||
-			key === "GIT_OBJECT_DIRECTORY" ||
-			key === "GIT_ALTERNATE_OBJECT_DIRECTORIES" ||
-			key === "GIT_CEILING_DIRECTORIES" ||
-			key === "GIT_TEMPLATE_DIR"
-		) {
-			removedVars.push(`${key}=${value}`)
+		if (BLOCKED_ENV_KEYS_LOWER.has(key.toLowerCase())) {
+			removedKeys.push(key)
 			continue
 		}
 
-		// Only include defined values
 		if (value !== undefined) {
 			sanitizedEnv[key] = value
 		}
 	}
 
 	// Log which git env vars were removed (helps with debugging Dev Container issues)
-	if (removedVars.length > 0) {
+	if (removedKeys.length > 0) {
 		console.log(
-			`[createSanitizedGit] Removed git environment variables for checkpoint isolation: ${removedVars.join(", ")}`,
+			`[createSanitizedGit] Removed git environment variables for checkpoint isolation: ${removedKeys.join(", ")}`,
 		)
 	}
 
 	const options: Partial<SimpleGitOptions> = {
 		baseDir,
 		config: [],
+		// --template="" stops git copying hooks/templates into the shadow repo (axis 1).
+		// GIT_TEMPLATE_DIR is stripped from the env above to block the env-var path (axis 2).
+		// allowUnsafeTemplateDir opts out of simple-git ≥3.36's blockUnsafeOperationsPlugin
+		// so the --template arg is not rejected before reaching git.
+		unsafe: { allowUnsafeTemplateDir: true },
 	}
 
 	// Create git instance and set the sanitized environment

src/services/checkpoints/__tests__/ShadowCheckpointService.spec.ts

@@ -11,9 +11,34 @@ import { fileExistsAtPath } from "../../../utils/fs"
 import * as fileSearch from "../../../services/search/file-search"
 
 import { RepoPerTaskCheckpointService } from "../RepoPerTaskCheckpointService"
+import { BLOCKED_ENV_KEYS } from "../ShadowCheckpointService"
 
 const tmpDir = path.join(os.tmpdir(), "CheckpointService")
 
+// simple-git ≥3.36 blocks env vars it considers code-execution vectors.
+// Strip them for the duration of this test suite so tests pass for developers
+// who have GIT_EDITOR, GIT_SSH_COMMAND, etc. configured globally.
+// Safe under vitest's default "forks" pool (each worker has its own process.env);
+// would be fragile under "threads" pool where workers share the same process.
+const savedEnv: Partial<Record<string, string>> = {}
+
+beforeAll(() => {
+	for (const key of BLOCKED_ENV_KEYS) {
+		savedEnv[key] = process.env[key]
+		delete process.env[key]
+	}
+})
+
+afterAll(() => {
+	for (const key of BLOCKED_ENV_KEYS) {
+		if (savedEnv[key] !== undefined) {
+			process.env[key] = savedEnv[key]
+		} else {
+			delete process.env[key]
+		}
+	}
+})
+
 const initWorkspaceRepo = async ({
 	workspaceDir,
 	userName = "Roo Code",
@@ -35,6 +60,7 @@ const initWorkspaceRepo = async ({
 	await git.init()
 	await git.addConfig("user.name", userName)
 	await git.addConfig("user.email", userEmail)
+	await git.addConfig("commit.gpgSign", "false")
 
 	// Create test file.
 	const testFile = path.join(workspaceDir, testFileName)
@@ -390,6 +416,7 @@ describe.each([[RepoPerTaskCheckpointService, "RepoPerTaskCheckpointService"]])(
 				await mainGit.init()
 				await mainGit.addConfig("user.name", "Roo Code")
 				await mainGit.addConfig("user.email", "support@roocode.com")
+				await mainGit.addConfig("commit.gpgSign", "false")
 
 				// Create a nested repo inside the workspace.
 				const nestedRepoPath = path.join(workspaceDir, "nested-project")
@@ -398,6 +425,7 @@ describe.each([[RepoPerTaskCheckpointService, "RepoPerTaskCheckpointService"]])(
 				await nestedGit.init()
 				await nestedGit.addConfig("user.name", "Roo Code")
 				await nestedGit.addConfig("user.email", "support@roocode.com")
+				await nestedGit.addConfig("commit.gpgSign", "false")
 
 				// Add a file to the nested repo.
 				const nestedFile = path.join(nestedRepoPath, "nested-file.txt")
@@ -460,6 +488,7 @@ describe.each([[RepoPerTaskCheckpointService, "RepoPerTaskCheckpointService"]])(
 				await mainGit.init()
 				await mainGit.addConfig("user.name", "Roo Code")
 				await mainGit.addConfig("user.email", "support@roocode.com")
+				await mainGit.addConfig("commit.gpgSign", "false")
 
 				// Create a test file in the main workspace.
 				const mainFile = path.join(workspaceDir, "main-file.txt")
@@ -873,6 +902,47 @@ describe.each([[RepoPerTaskCheckpointService, "RepoPerTaskCheckpointService"]])(
 				}
 			})
 
+			it("isolates checkpoint operations from simple-git blocked environment variables", async () => {
+				const testShadowDir = path.join(tmpDir, `shadow-blocked-env-test-${Date.now()}`)
+				const testWorkspaceDir = path.join(tmpDir, `workspace-blocked-env-test-${Date.now()}`)
+				await initWorkspaceRepo({ workspaceDir: testWorkspaceDir })
+
+				// beforeAll strips PREFIX, so it is always undefined here; set it to exercise isolation.
+				process.env.PREFIX = path.join(tmpDir, "git-prefix")
+
+				try {
+					const testService = await klass.create({
+						taskId: `test-blocked-env-${Date.now()}`,
+						shadowDir: testShadowDir,
+						workspaceDir: testWorkspaceDir,
+						log: () => {},
+					})
+					await testService.initShadowGit()
+
+					const testWorkspaceFile = path.join(testWorkspaceDir, "test.txt")
+					await fs.writeFile(testWorkspaceFile, "Modified with PREFIX set")
+					const commit = await testService.saveCheckpoint("Checkpoint with PREFIX set")
+					expect(commit?.commit).toBeTruthy()
+
+					// Verify the checkpoint is accessible and contains the expected change
+					const diff = await testService.getDiff({ to: commit!.commit })
+					expect(diff).toHaveLength(1)
+					expect(diff[0].paths.relative).toBe("test.txt")
+					expect(diff[0].content.after).toBe("Modified with PREFIX set")
+
+					// Verify we can restore the checkpoint
+					await fs.writeFile(testWorkspaceFile, "Another modification")
+					await testService.restoreCheckpoint(commit!.commit)
+					expect(await fs.readFile(testWorkspaceFile, "utf-8")).toBe("Modified with PREFIX set")
+				} finally {
+					// beforeAll guarantees PREFIX was undefined at test start, so always delete it.
+					delete process.env.PREFIX
+
+					await fs.rm(testShadowDir, { recursive: true, force: true })
+					await fs.rm(testWorkspaceDir, { recursive: true, force: true })
+				}
+			})
+
 			it("isolates checkpoint operations from GIT_DIR environment variable", async () => {
 				// This test verifies the fix for the issue where GIT_DIR environment variable
 				// causes checkpoint commits to go to the wrong repository.
@@ -886,6 +956,7 @@ describe.each([[RepoPerTaskCheckpointService, "RepoPerTaskCheckpointService"]])(
 				await externalGit.init()
 				await externalGit.addConfig("user.name", "External User")
 				await externalGit.addConfig("user.email", "external@example.com")
+				await externalGit.addConfig("commit.gpgSign", "false")
 
 				// Create and commit a file in the external repo
 				const externalFile = path.join(externalGitDir, "external.txt")
@@ -976,6 +1047,7 @@ describe("worktree path comparison", () => {
 			await mainGit.init()
 			await mainGit.addConfig("user.name", "Roo Code")
 			await mainGit.addConfig("user.email", "support@roocode.com")
+			await mainGit.addConfig("commit.gpgSign", "false")
 
 			await fs.writeFile(path.join(workspaceDir, "main.txt"), "main content")
 			await mainGit.add("main.txt")
@@ -1011,6 +1083,7 @@ describe("worktree path comparison", () => {
 			await mainGit.init()
 			await mainGit.addConfig("user.name", "Roo Code")
 			await mainGit.addConfig("user.email", "support@roocode.com")
+			await mainGit.addConfig("commit.gpgSign", "false")
 
 			await fs.writeFile(path.join(workspaceDir, "main.txt"), "main content")
 			await mainGit.add("main.txt")