From 80d79c1588962e2b165e39c894558c471e4a5bd3 Mon Sep 17 00:00:00 2001 From: Elliott de Launay Date: Tue, 26 May 2026 16:23:43 +0000 Subject: [PATCH] refactor(ShadowCheckpointService): updating template config, and removed env vars --- pnpm-lock.yaml | 24 ++++-- .../checkpoints/ShadowCheckpointService.ts | 72 +++++++++++++----- .../__tests__/ShadowCheckpointService.spec.ts | 73 +++++++++++++++++++ 3 files changed, 146 insertions(+), 23 deletions(-) diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index ce2940b92f..54d1bded00 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -660,7 +660,7 @@ importers: version: 1.8.3 simple-git: specifier: ^3.27.0 - version: 3.27.0 + version: 3.36.0 sound-play: specifier: ^1.1.0 version: 1.1.0 @@ -3183,6 +3183,12 @@ packages: '@shikijs/vscode-textmate@10.0.2': resolution: {integrity: sha512-83yeghZ2xxin3Nj8z1NMd/NCuca+gsYXswywDy5bHvwlWL8tpTQmzGeUuHd9FC3E/SBEMvzJRwWEOz5gGes9Qg==} + '@simple-git/args-pathspec@1.0.3': + resolution: {integrity: sha512-ngJMaHlsWDTfjyq9F3VIQ8b7NXbBLq5j9i5bJ6XLYtD6qlDXT7fdKY2KscWWUF8t18xx052Y/PUO1K1TRc9yKA==} + + '@simple-git/argv-parser@1.1.1': + resolution: {integrity: sha512-Q9lBcfQ+VQCpQqGJFHe5yooOS5hGdLFFbJ5R+R5aDsnkPCahtn1hSkMcORX65J2Z5lxSkD0lQorMsncuBQxYUw==} + '@sinclair/typebox@0.27.8': resolution: {integrity: sha512-+Fj43pSMwJs4KRrH/938Uf+uAELIgVBmQzg/q1YG10djyfA3TnrU8N8XzqCh/okZdszqBQTZf96idMfE5lnwTA==} @@ -8453,8 +8459,8 @@ packages: simple-get@4.0.1: resolution: {integrity: sha512-brv7p5WgH0jmQJr1ZDDfKDOSeWWg+OVypG99A/5vYGPqJ6pxiaHLy8nxtFjBA7oMa01ebA9gfh1uMCFqOuXxvA==} - simple-git@3.27.0: - resolution: {integrity: sha512-ivHoFS9Yi9GY49ogc6/YAi3Fl9ROnF4VyubNylgCkA+RVqLaKWnDSzXOVzya8csELIaWaYNutsEuAhZrtOjozA==} + simple-git@3.36.0: + resolution: {integrity: sha512-cGQjLjK8bxJw4QuYT7gxHw3/IouVESbhahSsHrX97MzCL1gu2u7oy38W6L2ZIGECEfIBG4BabsWDPjBxJENv9Q==} simple-invariant@2.0.1: resolution: {integrity: sha512-1sbhsxqI+I2tqlmjbz99GXNmZtr6tKIyEgGGnJw/MKGblalqk/XoOYYFJlBzTKZCxx8kLaD3FD5s9BEEjx5Pyg==} @@ -12336,6 +12342,12 @@ snapshots: '@shikijs/vscode-textmate@10.0.2': {} + '@simple-git/args-pathspec@1.0.3': {} + + '@simple-git/argv-parser@1.1.1': + dependencies: + '@simple-git/args-pathspec': 1.0.3 + '@sinclair/typebox@0.27.8': {} '@sindresorhus/merge-streams@4.0.0': {} @@ -18666,11 +18678,13 @@ snapshots: simple-concat: 1.0.1 optional: true - simple-git@3.27.0: + simple-git@3.36.0: dependencies: '@kwsites/file-exists': 1.1.1 '@kwsites/promise-deferred': 1.1.1 - debug: 4.4.1(supports-color@8.1.1) + '@simple-git/args-pathspec': 1.0.3 + '@simple-git/argv-parser': 1.1.1 + debug: 4.4.3 transitivePeerDependencies: - supports-color diff --git a/src/services/checkpoints/ShadowCheckpointService.ts b/src/services/checkpoints/ShadowCheckpointService.ts index 89ae52c435..3e3d3d0653 100644 --- a/src/services/checkpoints/ShadowCheckpointService.ts +++ b/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 = {} - 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 = { 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 diff --git a/src/services/checkpoints/__tests__/ShadowCheckpointService.spec.ts b/src/services/checkpoints/__tests__/ShadowCheckpointService.spec.ts index 5bc43d54ce..3a2eb62a76 100644 --- a/src/services/checkpoints/__tests__/ShadowCheckpointService.spec.ts +++ b/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> = {} + +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")