From 2dddbb379a29619be320e399cc74d3a38019a392 Mon Sep 17 00:00:00 2001 From: JATMN Date: Mon, 26 Oct 2026 08:27:04 -0700 Subject: [PATCH] fix(plugins): suppress hooks throughout sparse clones (#2426) --- src/__tests__/security-hardening.test.ts | 42 +++++++++- src/test/fixtures/pluginGitHooks.fixture.ts | 86 +++++++++++++++++++++ src/utils/plugins/marketplaceManager.ts | 4 +- 3 files changed, 129 insertions(+), 3 deletions(-) create mode 100644 src/test/fixtures/pluginGitHooks.fixture.ts diff --git a/src/__tests__/security-hardening.test.ts b/src/__tests__/security-hardening.test.ts index d22ef604d..1b7395481 100644 --- a/src/__tests__/security-hardening.test.ts +++ b/src/__tests__/security-hardening.test.ts @@ -10,7 +10,9 @@ */ import { describe, test, expect } from 'bun:test' -import { resolve } from 'path' +import { resolve, join } from 'path' +import { mkdtempSync, rmSync, writeFileSync } from 'node:fs' +import { tmpdir } from 'node:os' const SRC = resolve(import.meta.dir, '..') const file = (relative: string) => Bun.file(resolve(SRC, relative)) @@ -92,6 +94,44 @@ describe('Plugin git operations disable hooks', () => { }) }) +for (const scenario of ['normal', 'sparse', 'pull-default', 'pull-ref']) { + // The child has a 30-second deadline; leave time for diagnostics and cleanup. + test(`plugin Git operations suppress configured hooks: ${scenario}`, async () => { + const root = mkdtempSync(join(tmpdir(), 'plugin-git-hooks-')) + const config = join(root, 'gitconfig') + try { + writeFileSync(config, '') + const child = Bun.spawn({ + cmd: [process.execPath, resolve(SRC, 'test/fixtures/pluginGitHooks.fixture.ts'), root, scenario], + cwd: root, + env: { + ...process.env, + GIT_DIR: undefined, GIT_WORK_TREE: undefined, GIT_COMMON_DIR: undefined, + GIT_INDEX_FILE: undefined, GIT_OBJECT_DIRECTORY: undefined, GIT_ALTERNATE_OBJECT_DIRECTORIES: undefined, + GIT_CONFIG: undefined, GIT_CONFIG_PARAMETERS: undefined, GIT_CONFIG_COUNT: '2', + GIT_CONFIG_KEY_0: 'core.hooksPath', GIT_CONFIG_VALUE_0: join(root, 'hooks'), + GIT_CONFIG_KEY_1: 'protocol.file.allow', GIT_CONFIG_VALUE_1: 'always', + GIT_CONFIG_GLOBAL: config, GIT_CONFIG_NOSYSTEM: '1', GIT_CEILING_DIRECTORIES: root, + GIT_AUTHOR_NAME: 'Fixture', GIT_AUTHOR_EMAIL: 'fixture@example.invalid', + GIT_COMMITTER_NAME: 'Fixture', GIT_COMMITTER_EMAIL: 'fixture@example.invalid', + PLUGIN_HOOK_MARKER: join(root, 'hook-ran'), + OPENCLAUDE_CONFIG_DIR: join(root, 'config'), CLAUDE_CONFIG_DIR: join(root, 'config'), + CLAUDE_CODE_PLUGIN_GIT_TIMEOUT_MS: '5000', + }, + stdin: 'ignore', stdout: 'pipe', stderr: 'pipe', timeout: 30_000, + killSignal: process.platform === 'win32' ? undefined : 'SIGKILL', + }) + const [code, stdout, stderr] = await Promise.all([ + child.exited, new Response(child.stdout).text(), new Response(child.stderr).text(), + ]) + expect(code, stdout + stderr).toBe(0) + expect(stdout).toContain(`verified ${scenario}: content, hooks, and`) + } finally { + rmSync(root, { recursive: true, force: true }) + } + }, 35_000) +} + // --------------------------------------------------------------------------- // Fix 4: ANTHROPIC_FOUNDRY_API_KEY not in SAFE_ENV_VARS // --------------------------------------------------------------------------- diff --git a/src/test/fixtures/pluginGitHooks.fixture.ts b/src/test/fixtures/pluginGitHooks.fixture.ts new file mode 100644 index 000000000..8c770e539 --- /dev/null +++ b/src/test/fixtures/pluginGitHooks.fixture.ts @@ -0,0 +1,86 @@ +// Run only in a disposable process: real Git operations must not inherit module mocks. +import assert from 'node:assert/strict' +import { existsSync, mkdirSync, readFileSync, rmSync, writeFileSync } from 'node:fs' +import { join } from 'node:path' +import { pathToFileURL } from 'node:url' +import { mock } from 'bun:test' + +const root = process.argv[2]! +const scenario = process.argv[3]! +const upstream = join(root, 'upstream') +const dependency = join(root, 'dependency') +const target = join(root, 'target') +const marker = join(root, 'hook-ran') +const hooks = join(root, 'hooks') +mkdirSync(hooks) +for (const name of ['post-checkout', 'post-merge']) { + writeFileSync(join(hooks, name), '#!/bin/sh\nprintf "hook ran\\n" >> "$PLUGIN_HOOK_MARKER"\n', { mode: 0o755 }) +} + +function git(cwd: string, ...args: string[]): string { + const result = Bun.spawnSync(['git', ...args], { + cwd, env: process.env, stdin: 'ignore', stdout: 'pipe', stderr: 'pipe', timeout: 5_000, + }) + assert.equal(result.exitCode, 0, result.stderr.toString()) + return result.stdout.toString().trim() +} +function commit(cwd: string, message: string): void { + git(cwd, 'add', '.') + git(cwd, '-c', 'core.hooksPath=/dev/null', 'commit', '-m', message) +} +for (const repo of [upstream, dependency]) { + mkdirSync(repo) + git(repo, 'init', '-b', 'main') +} +writeFileSync(join(dependency, 'value.txt'), 'dependency v1') +commit(dependency, 'first dependency') +mkdirSync(join(upstream, 'selected')) +mkdirSync(join(upstream, 'omitted')) +writeFileSync(join(upstream, 'selected', 'value.txt'), 'selected v1') +writeFileSync(join(upstream, 'omitted', 'value.txt'), 'omitted') +git(upstream, '-c', 'core.hooksPath=/dev/null', 'submodule', 'add', pathToFileURL(dependency).href, 'dependency') +commit(upstream, 'first marketplace') + +// Positive control: the configured hook really runs during an ordinary checkout. +git(root, 'clone', pathToFileURL(upstream).href, join(root, 'control')) +assert.equal(readFileSync(marker, 'utf8'), 'hook ran\n') +rmSync(marker) + +// Observe every production invocation while retaining the actual executor/Git. +const executor = { ...await import('../../utils/execFileNoThrow.js') } +const calls: string[][] = [] +mock.module('../../utils/execFileNoThrow.js', () => ({ + ...executor, + execFileNoThrowWithCwd: (...args: Parameters) => { + calls.push([...args[1]]) + return executor.execFileNoThrowWithCwd(...args) + }, +})) +const { gitClone, gitPull } = await import('../../utils/plugins/marketplaceManager.js') +const sparse = scenario === 'sparse' ? ['selected'] : undefined +const cloned = await gitClone(pathToFileURL(upstream).href, target, 'main', sparse) +assert.equal(cloned.code, 0, cloned.stderr) +assert.equal(readFileSync(join(target, 'selected', 'value.txt'), 'utf8'), 'selected v1') +assert.equal(existsSync(join(target, 'omitted', 'value.txt')), !sparse) +assert.equal(git(target, 'rev-parse', '--is-shallow-repository'), 'true') +assert.equal(existsSync(marker), false, 'plugin clone ran a configured hook') +if (!sparse) assert.equal(readFileSync(join(target, 'dependency', 'value.txt'), 'utf8'), 'dependency v1') + +if (scenario.startsWith('pull')) { + writeFileSync(join(dependency, 'value.txt'), 'dependency v2') + commit(dependency, 'update dependency') + git(join(upstream, 'dependency'), '-c', 'core.hooksPath=/dev/null', 'pull', '--ff-only') + writeFileSync(join(upstream, 'selected', 'value.txt'), 'selected v2') + commit(upstream, 'update marketplace') + const pulled = await gitPull(target, scenario === 'pull-ref' ? 'main' : undefined) + assert.equal(pulled.code, 0, pulled.stderr) + assert.equal(readFileSync(join(target, 'selected', 'value.txt'), 'utf8'), 'selected v2') + assert.equal(readFileSync(join(target, 'dependency', 'value.txt'), 'utf8'), 'dependency v2') + assert.equal(existsSync(marker), false, 'plugin pull or submodule update ran a configured hook') + assert(calls.some(args => args.includes('submodule') && args.includes('update'))) +} +assert.equal(calls.length, scenario === 'normal' ? 1 : scenario === 'pull-ref' ? 5 : 3) +for (const args of calls) { + assert(args.some((arg, i) => arg === '-c' && args[i + 1] === 'core.hooksPath=/dev/null'), JSON.stringify(args)) +} +console.log(`verified ${scenario}: content, hooks, and ${calls.length} Git invocations`) diff --git a/src/utils/plugins/marketplaceManager.ts b/src/utils/plugins/marketplaceManager.ts index 728897f38..ed7f9d6fd 100644 --- a/src/utils/plugins/marketplaceManager.ts +++ b/src/utils/plugins/marketplaceManager.ts @@ -947,7 +947,7 @@ export async function gitClone( // in a single step on git >= 2.25. const sparseResult = await execFileNoThrowWithCwd( gitExe(), - ['sparse-checkout', 'set', '--cone', '--', ...sparsePaths], + ['-c', 'core.hooksPath=/dev/null', 'sparse-checkout', 'set', '--cone', '--', ...sparsePaths], { cwd: targetPath, timeout: timeoutMs, @@ -966,7 +966,7 @@ export async function gitClone( gitExe(), // ref was already passed to clone via --branch, so HEAD points to it; // if no ref, HEAD points to the remote's default branch. - ['checkout', 'HEAD'], + ['-c', 'core.hooksPath=/dev/null', 'checkout', 'HEAD'], { cwd: targetPath, timeout: timeoutMs,