diff --git a/tests/security/git_workflow_read_only_probe.mjs b/tests/security/git_workflow_read_only_probe.mjs new file mode 100644 index 000000000..fdae48d4b --- /dev/null +++ b/tests/security/git_workflow_read_only_probe.mjs @@ -0,0 +1,34 @@ +import assert from 'node:assert/strict'; +import {spawn, execFileSync} from 'node:child_process'; +import {readFileSync,writeFileSync,mkdirSync,mkdtempSync,existsSync,chmodSync} from 'node:fs'; +import {tmpdir} from 'node:os'; +import path from 'node:path'; +import vm from 'node:vm'; +const source=readFileSync(process.argv[2],'utf8'); +const defs=source.slice(source.indexOf('function runGitCommand'),source.indexOf('class GitWorkflowMCPServer')); +let failures=0; +for (const kind of ['fsmonitor','fsmonitor-long','external','external-staged','textconv','textconv-staged','write-hook']) { + const root=mkdtempSync(path.join(tmpdir(),'git-readonly-')); + const env={PATH:process.env.PATH,HOME:root,GIT_CONFIG_NOSYSTEM:'1'}; + const git=(...args)=>execFileSync('git',args,{cwd:root,env,stdio:'pipe'}); + git('init','-q');git('config','user.name','Fixture');git('config','user.email','fixture@example.invalid'); + writeFileSync(path.join(root,'file.txt'),'before\n');git('add','.');git('commit','-qm','baseline'); + const marker=path.join(root,'marker'); + const helper=path.join(root,'helper.sh'); + writeFileSync(helper,'#!/bin/sh\nprintf fixture > "'+marker+'"\n');chmodSync(helper,0o755); + writeFileSync(path.join(root,'file.txt'),'after\n'); + if(kind.endsWith('-staged'))git('add','file.txt'); + if(kind.startsWith('fsmonitor'))git('config','core.fsmonitor',helper); + if(kind.startsWith('external'))git('config','diff.external',helper); + if(kind.startsWith('textconv')){git('config','diff.fixture.textconv',helper);writeFileSync(path.join(root,'.gitattributes'),'*.txt diff=fixture\n');} + if(kind==='write-hook'){const hook=path.join(root,'.git/hooks/pre-commit');writeFileSync(hook,readFileSync(helper));chmodSync(hook,0o755);} + const context=vm.createContext({spawn,path,process:{env},WORKSPACE_ROOT:root}); + vm.runInContext(defs,context); + try { + const result=await vm.runInContext(kind.startsWith('fsmonitor')?'gitStatus({porcelain:'+(!kind.endsWith('-long'))+'})':kind==='write-hook'?'gitCommit({message:"fixture",allowEmpty:true})':'gitDiff({staged:'+kind.endsWith('-staged')+',pathspec:"file.txt"})',context); + if(kind==='write-hook')assert.ok(existsSync(marker),'explicit commit hook behavior preserved'); + else {assert.ok(!existsSync(marker),kind+' must not execute repository helper');if(!kind.startsWith('fsmonitor'))assert.ok(result.stdout.includes('+after'),'ordinary diff preserved');} + console.log('PASS '+kind); + } catch(error){failures++;console.log('FAIL '+kind+': '+error.message);} +} +process.exitCode=failures?1:0; diff --git a/tests/unit/test_git_read_only_security.py b/tests/unit/test_git_read_only_security.py new file mode 100644 index 000000000..5947a7aea --- /dev/null +++ b/tests/unit/test_git_read_only_security.py @@ -0,0 +1,24 @@ +"""Real Git marker regressions for the exact read-only launcher function bodies. + +No MCP server or provider is launched. The Node VM excludes SDK imports and +transport wiring; this test does not establish MCP end-to-end acceptance. +""" +import subprocess +import unittest +from pathlib import Path + + +class GitReadOnlySecurityTests(unittest.TestCase): + def test_repository_helpers_do_not_run_from_read_tools(self): + root = Path(__file__).resolve().parents[2] + result = subprocess.run( + ["node", str(root / "tests/security/git_workflow_read_only_probe.mjs"), + str(root / "tools/mcp/git_workflow_server.mjs")], + capture_output=True, text=True, timeout=30, check=False, + ) + self.assertEqual(result.returncode, 0, result.stdout + result.stderr) + self.assertEqual(result.stdout.count("PASS "), 7) + + +if __name__ == "__main__": + unittest.main() diff --git a/tools/mcp/git_workflow_server.mjs b/tools/mcp/git_workflow_server.mjs index 9ee2fcd47..33bb6ada4 100644 --- a/tools/mcp/git_workflow_server.mjs +++ b/tools/mcp/git_workflow_server.mjs @@ -28,9 +28,14 @@ if (!fs.existsSync(WORKSPACE_ROOT)) { process.exit(1); } -function runGitCommand(args, { allowEmpty = false } = {}) { +function runGitCommand(args, { allowEmpty = false, readOnly = false } = {}) { return new Promise((resolve, reject) => { - const child = spawn('git', args, { + // Read tools must not execute helpers supplied by repository configuration. + // Explicit write tools retain their existing Git/hook behavior. + const commandArgs = readOnly + ? ['--no-pager', '-c', 'core.fsmonitor=false', ...args] + : args; + const child = spawn('git', commandArgs, { cwd: WORKSPACE_ROOT, env: process.env, }); @@ -62,7 +67,8 @@ function runGitCommand(args, { allowEmpty = false } = {}) { async function gitStatus({ porcelain = true } = {}) { const args = porcelain ? ['status', '--short', '--branch'] : ['status']; - return runGitCommand(args); + args.push('--ignore-submodules=dirty'); + return runGitCommand(args, { readOnly: true }); } async function gitAdd({ files }) { @@ -109,14 +115,14 @@ async function gitPull({ remote, branch, rebase = false } = {}) { } async function gitDiff({ pathspec, staged = false } = {}) { - const args = ['diff']; + const args = ['diff', '--no-ext-diff', '--no-textconv', '--ignore-submodules=dirty']; if (staged) { args.push('--staged'); } if (pathspec && pathspec.length > 0) { args.push('--', pathspec); } - return runGitCommand(args); + return runGitCommand(args, { readOnly: true }); } class GitWorkflowMCPServer {