From dca638d08d9a2ad0e642677e92d9fc0e08cb4a86 Mon Sep 17 00:00:00 2001 From: AHMET BAYHAN BAYRAMOGLU <49499275+ABB65@users.noreply.github.com> Date: Mon, 13 Jul 2026 19:55:02 +0300 Subject: [PATCH] fix(mcp): stop tripping simple-git block-unsafe guard on every write simple-git >= 3.34 bundles @simple-git/argv-parser, whose block-unsafe guard rejects a git invocation when a guard-listed variable (EDITOR, GIT_ASKPASS, PAGER, GIT_SSH_COMMAND, ...) is passed EXPLICITLY through .env(). The transaction layer spread ...process.env into .env() to set the commit author, so once a transitive bump pulled simple-git to 3.36.0 every worktree write (content_save, model_save, merge) started failing with `Use of "EDITOR" is not permitted...` in any host that exports those variables (VS Code, Claude Code, CI). The guard only scans the object handed to .env(), never the inherited environment. - Commit identity now flows through `-c user.name`/`-c user.email` config (authorConfig) instead of .env(). These keys are on no unsafe list and git honours them for author + committer alike, so the guard is never touched. CONTENTRAIN_AUTHOR_NAME/EMAIL overrides preserved. - networkGit (push/fetch) legitimately needs the inherited askpass/SSH/ proxy environment, so it keeps .env() but opts out of the affected guard categories via `unsafe` (NETWORK_UNSAFE). Closes the same latent failure in contentrain_submit that the report missed. - Both concerns centralized in git/identity.ts so no future call site can reintroduce a process.env spread. simple-git pinned to ^3.36.0. Tests: tests/git/identity.test.ts (5) incl. a control asserting the old .env(process.env) path still trips the guard; existing tests/git suites (49) green; tsc + oxlint clean. --- .changeset/mcp-block-unsafe-guard.md | 30 ++++++++ packages/mcp/package.json | 2 +- packages/mcp/src/git/branch-lifecycle.ts | 9 ++- packages/mcp/src/git/identity.ts | 52 +++++++++++++ packages/mcp/src/git/transaction.ts | 32 ++------ packages/mcp/tests/git/identity.test.ts | 96 ++++++++++++++++++++++++ pnpm-lock.yaml | 29 ++++++- 7 files changed, 222 insertions(+), 28 deletions(-) create mode 100644 .changeset/mcp-block-unsafe-guard.md create mode 100644 packages/mcp/src/git/identity.ts create mode 100644 packages/mcp/tests/git/identity.test.ts diff --git a/.changeset/mcp-block-unsafe-guard.md b/.changeset/mcp-block-unsafe-guard.md new file mode 100644 index 0000000..d9b61e1 --- /dev/null +++ b/.changeset/mcp-block-unsafe-guard.md @@ -0,0 +1,30 @@ +--- +"@contentrain/mcp": patch +--- + +fix(mcp): stop tripping simple-git's block-unsafe guard on every write + +`git commit` (and every other worktree operation) began failing with +`Use of "EDITOR"/"GIT_ASKPASS" is not permitted…` in any host that exports +those variables (VS Code, Claude Code, CI). Nothing in Contentrain changed — a +transitive bump of `simple-git` to `>= 3.34` pulled in `@simple-git/argv-parser`, +whose block-unsafe guard rejects a `git` invocation when a guard-listed variable +is passed **explicitly** through `.env()`. The transaction layer had been +spreading `...process.env` into `.env()` to set the commit author, so the guard +saw those inherited variables and refused to run. + +- Commit identity now flows through `-c user.name` / `-c user.email` config + (`authorConfig`) instead of `.env()`. These keys are not on any unsafe list and + git honours them for both author and committer, so the guard is never touched — + regardless of what the host exports. `CONTENTRAIN_AUTHOR_NAME/EMAIL` overrides + are preserved. +- The one instance that legitimately needs the inherited environment — network + push/fetch, which relies on the host's askpass/SSH/proxy setup — keeps `.env()` + but opts out of the affected guard categories via `unsafe` (`NETWORK_UNSAFE`), + leaving arg-injection protections intact. This closes the same latent failure + in `contentrain_submit`. +- Both concerns are centralized in `git/identity.ts` so no future call site can + reintroduce a `process.env` spread. `simple-git` is pinned to `^3.36.0`. + +Covered by `tests/git/identity.test.ts`, including a control that asserts the old +`.env(process.env)` path still trips the guard. diff --git a/packages/mcp/package.json b/packages/mcp/package.json index fbe7546..b0e464d 100644 --- a/packages/mcp/package.json +++ b/packages/mcp/package.json @@ -163,7 +163,7 @@ "dependencies": { "@contentrain/types": "workspace:*", "@modelcontextprotocol/sdk": "^1.12.0", - "simple-git": "^3.27.0", + "simple-git": "^3.36.0", "typescript": "^5.7.0", "zod": "^3.24.0" }, diff --git a/packages/mcp/src/git/branch-lifecycle.ts b/packages/mcp/src/git/branch-lifecycle.ts index 56845a4..c369c42 100644 --- a/packages/mcp/src/git/branch-lifecycle.ts +++ b/packages/mcp/src/git/branch-lifecycle.ts @@ -1,6 +1,7 @@ import { simpleGit, type SimpleGit } from 'simple-git' import { CONTENTRAIN_BRANCH, type ContentrainConfig } from '@contentrain/types' import { readConfig } from '../core/config.js' +import { NETWORK_UNSAFE } from './identity.js' export interface CleanupResult { deleted: number @@ -356,9 +357,15 @@ function contentrainRemoteName(): string { * simple-git instance hardened for network operations: `timeout.block` kills * the child after N ms without output (hung SSH passphrase prompts), and * `GIT_TERMINAL_PROMPT=0` refuses interactive HTTPS credential prompts. + * + * This is the one place that legitimately inherits the real environment — push + * needs the host's askpass/SSH/proxy setup to authenticate. Because `.env()` is + * therefore unavoidable, `unsafe` opts out of the block-unsafe guard categories + * that inherited variables (EDITOR, GIT_ASKPASS, …) would otherwise trip; see + * NETWORK_UNSAFE. Arg-injection protections stay intact. */ function networkGit(projectRoot: string, timeoutMs: number): SimpleGit { - return simpleGit({ baseDir: projectRoot, timeout: { block: timeoutMs } }) + return simpleGit({ baseDir: projectRoot, timeout: { block: timeoutMs }, unsafe: NETWORK_UNSAFE }) .env({ ...process.env, GIT_TERMINAL_PROMPT: '0' }) } diff --git a/packages/mcp/src/git/identity.ts b/packages/mcp/src/git/identity.ts new file mode 100644 index 0000000..0246b25 --- /dev/null +++ b/packages/mcp/src/git/identity.ts @@ -0,0 +1,52 @@ +/** + * Git identity + guard-safe `simple-git` construction for MCP write operations. + * + * simple-git >= 3.34 bundles `@simple-git/argv-parser`, whose block-unsafe + * guard rejects a `git` invocation when any of ~18 "unsafe" variables (EDITOR, + * GIT_ASKPASS, PAGER, GIT_SSH_COMMAND, GIT_PROXY_COMMAND, …) is passed + * EXPLICITLY through `.env()`. Crucially, the guard only scans the object + * handed to `.env()` — it never inspects the inherited process environment. + * + * The rule this module enforces: NEVER spread `process.env` into `.env()`. + * - Commit identity is supplied as `-c user.*` config (guard-safe: `user.name` + * / `user.email` are not on any unsafe list, and git honours them for both + * the author and the committer). See {@link authorConfig}. + * - The rare instance that genuinely needs the inherited environment — network + * push/fetch, which relies on the host's askpass/SSH/proxy setup to + * authenticate — opts out of the affected guard categories via `unsafe` + * instead of hiding the environment. See {@link NETWORK_UNSAFE}. + */ + +const DEFAULT_AUTHOR_NAME = 'Contentrain' +const DEFAULT_AUTHOR_EMAIL = 'ai@contentrain.io' + +/** + * Commit identity as `-c` config entries for `simpleGit(dir, { config })`. + * Passed as arguments (not env) so the block-unsafe guard is never triggered, + * regardless of what the host process exports. Sets author + committer alike. + */ +export function authorConfig(): string[] { + const name = process.env['CONTENTRAIN_AUTHOR_NAME'] ?? DEFAULT_AUTHOR_NAME + const email = process.env['CONTENTRAIN_AUTHOR_EMAIL'] ?? DEFAULT_AUTHOR_EMAIL + return [`user.name=${name}`, `user.email=${email}`] +} + +/** + * Guard opt-outs for network `git` instances that MUST inherit the real + * environment (credential askpass helpers, SSH agent, proxy) to authenticate a + * push/fetch. Covers every guard category reachable from an inherited env var, + * so the command never trips regardless of what the host (VS Code, CI) exports + * — while still leaving arg-injection protections (custom binaries, `ext::` + * protocol, `--upload-pack`) intact. + */ +export const NETWORK_UNSAFE = { + allowUnsafeAskPass: true, + allowUnsafeConfigEnvCount: true, + allowUnsafeConfigPaths: true, + allowUnsafeDiffExternal: true, + allowUnsafeEditor: true, + allowUnsafeGitProxy: true, + allowUnsafePager: true, + allowUnsafeSshCommand: true, + allowUnsafeTemplateDir: true, +} diff --git a/packages/mcp/src/git/transaction.ts b/packages/mcp/src/git/transaction.ts index 1bd2882..7feb226 100644 --- a/packages/mcp/src/git/transaction.ts +++ b/packages/mcp/src/git/transaction.ts @@ -6,6 +6,7 @@ import { randomUUID } from 'node:crypto' import { readConfig } from '../core/config.js' import { writeContext } from '../core/context.js' import { deleteRemoteBranch, type RemoteDeleteResult } from './branch-lifecycle.js' +import { authorConfig } from './identity.js' import { branchTimestamp } from '../util/id.js' import { migrateLegacyBranches } from '../providers/local/migration.js' import type { SyncResult, WorkflowMode } from '@contentrain/types' @@ -60,25 +61,6 @@ export async function ensureContentBranch(projectRoot: string): Promise { } } -/** - * Commit identity for worktree operations, supplied via environment instead - * of two `git config` spawns per transaction. Git honors GIT_AUTHOR_* / - * GIT_COMMITTER_* for both the sync merges and the feature-branch commit, so - * a worktree git built with this env needs no `git config user.*` calls. - * `process.env` is spread so PATH/HOME and any GIT_* already set survive. - */ -function authorEnv(): Record { - const name = process.env['CONTENTRAIN_AUTHOR_NAME'] ?? 'Contentrain' - const email = process.env['CONTENTRAIN_AUTHOR_EMAIL'] ?? 'ai@contentrain.io' - return { - ...process.env, - GIT_AUTHOR_NAME: name, - GIT_AUTHOR_EMAIL: email, - GIT_COMMITTER_NAME: name, - GIT_COMMITTER_EMAIL: email, - } -} - async function selectiveSync( projectRoot: string, _worktreePath: string, @@ -241,9 +223,10 @@ export async function createTransaction( // Create worktree on contentrain branch await git.raw(['worktree', 'add', worktreePath, CONTENTRAIN_BRANCH]) - // Commit identity comes from the environment (see authorEnv) — no - // `git config user.*` spawns. - const wtGit = simpleGit(worktreePath).env(authorEnv()) + // Commit identity comes from `-c user.*` config (see authorConfig) — passed + // as args, never via `.env()`, so simple-git's block-unsafe guard is never + // triggered by an inherited EDITOR/GIT_ASKPASS/etc. + const wtGit = simpleGit(worktreePath, { config: authorConfig() }) // Sync contentrain with base branch (bring main changes into contentrain) try { @@ -454,8 +437,9 @@ export async function mergeBranch( const worktreePath = join(tmpdir(), `cr-merge-${randomUUID()}`) await git.raw(['worktree', 'add', worktreePath, CONTENTRAIN_BRANCH]) - // Commit identity from the environment (see authorEnv) — no config spawns. - const wtGit = simpleGit(worktreePath).env(authorEnv()) + // Commit identity from `-c user.*` config (see authorConfig) — guard-safe, + // no `.env()` spread. + const wtGit = simpleGit(worktreePath, { config: authorConfig() }) try { // Merge the feature branch into contentrain diff --git a/packages/mcp/tests/git/identity.test.ts b/packages/mcp/tests/git/identity.test.ts new file mode 100644 index 0000000..9685311 --- /dev/null +++ b/packages/mcp/tests/git/identity.test.ts @@ -0,0 +1,96 @@ +import { afterEach, beforeEach, describe, expect, it } from 'vitest' +import { mkdtemp, rm, writeFile } from 'node:fs/promises' +import { tmpdir } from 'node:os' +import { join } from 'node:path' +import { simpleGit } from 'simple-git' +import { authorConfig, NETWORK_UNSAFE } from '../../src/git/identity.js' + +/** + * Regression coverage for the simple-git block-unsafe guard. + * + * simple-git >= 3.34 (`@simple-git/argv-parser`) rejects a `git` invocation + * when a guard-listed variable (EDITOR, GIT_ASKPASS, …) is passed EXPLICITLY + * through `.env()`. The previous code spread `process.env` into `.env()` to set + * the commit author, which tripped the guard in any host (VS Code, Claude Code, + * CI) that exports those variables. These tests pin the fix: identity via + * `-c user.*` config, and network instances opt out of the guard categories. + */ + +const TOUCHED = [ + 'EDITOR', + 'GIT_ASKPASS', + 'GIT_AUTHOR_NAME', + 'GIT_AUTHOR_EMAIL', + 'GIT_COMMITTER_NAME', + 'GIT_COMMITTER_EMAIL', + 'CONTENTRAIN_AUTHOR_NAME', + 'CONTENTRAIN_AUTHOR_EMAIL', +] as const + +let repo: string +const saved: Record = {} + +beforeEach(async () => { + for (const key of TOUCHED) saved[key] = process.env[key] + + // Simulate a host that exports guard-tripping variables (the real-world bug). + process.env['EDITOR'] = 'vi' + process.env['GIT_ASKPASS'] = '/usr/bin/true' + // Identity must come from our `-c` config, not ambient GIT_AUTHOR_*/defaults. + for (const key of TOUCHED.slice(2)) delete process.env[key] + + repo = await mkdtemp(join(tmpdir(), 'cr-identity-')) + const git = simpleGit(repo) + await git.init() + await writeFile(join(repo, 'a.txt'), 'hello\n') +}) + +afterEach(async () => { + for (const key of TOUCHED) { + if (saved[key] === undefined) delete process.env[key] + else process.env[key] = saved[key] + } + await rm(repo, { recursive: true, force: true }) +}) + +describe('git identity — block-unsafe guard', () => { + it('commits via `-c user.*` config while EDITOR/GIT_ASKPASS are set (guard not tripped)', async () => { + const git = simpleGit(repo, { config: authorConfig() }) + await git.add('.') + await git.commit('add a') + + const author = (await git.raw(['log', '-1', '--format=%an <%ae>'])).trim() + const committer = (await git.raw(['log', '-1', '--format=%cn <%ce>'])).trim() + expect(author).toBe('Contentrain ') + // `user.*` config sets committer identity too, matching the old env behavior. + expect(committer).toBe('Contentrain ') + }) + + it('honors CONTENTRAIN_AUTHOR_* overrides through the config path', async () => { + process.env['CONTENTRAIN_AUTHOR_NAME'] = 'Ada' + process.env['CONTENTRAIN_AUTHOR_EMAIL'] = 'ada@example.com' + const git = simpleGit(repo, { config: authorConfig() }) + await git.add('.') + await git.commit('add a') + + const author = (await git.raw(['log', '-1', '--format=%an <%ae>'])).trim() + expect(author).toBe('Ada ') + }) + + it('control: spreading process.env into `.env()` trips the guard (the original bug)', async () => { + const git = simpleGit(repo).env({ ...process.env }) + await expect(git.raw(['status'])).rejects.toThrow(/not permitted/i) + }) + + it('networkGit-style instance runs despite inherited env via NETWORK_UNSAFE', async () => { + const git = simpleGit({ baseDir: repo, unsafe: NETWORK_UNSAFE }) + .env({ ...process.env, GIT_TERMINAL_PROMPT: '0' }) + const status = await git.raw(['status', '--porcelain']) + expect(status).toContain('a.txt') + }) + + it('control: the same inherited-env instance without NETWORK_UNSAFE trips the guard', async () => { + const git = simpleGit({ baseDir: repo }).env({ ...process.env, GIT_TERMINAL_PROMPT: '0' }) + await expect(git.raw(['status'])).rejects.toThrow(/not permitted/i) + }) +}) diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index c0d5075..9f626d7 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -167,8 +167,8 @@ importers: specifier: ^1.12.0 version: 1.27.1(zod@3.25.76) simple-git: - specifier: ^3.27.0 - version: 3.33.0 + specifier: ^3.36.0 + version: 3.36.0 typescript: specifier: ^5.7.0 version: 5.9.3 @@ -1453,6 +1453,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==} + '@swc/helpers@0.5.19': resolution: {integrity: sha512-QamiFeIK3txNjgUTNppE6MiG3p7TdninpZu0E0PbqVh1a9FNLT2FRhisaa4NcaX52XVhA5l7Pk58Ft7Sqi/2sA==} @@ -2915,6 +2921,9 @@ packages: simple-git@3.33.0: resolution: {integrity: sha512-D4V/tGC2sjsoNhoMybKyGoE+v8A60hRawKQ1iFRA1zwuDgGZCBJ4ByOzZ5J8joBbi4Oam0qiPH+GhzmSBwbJng==} + simple-git@3.36.0: + resolution: {integrity: sha512-cGQjLjK8bxJw4QuYT7gxHw3/IouVESbhahSsHrX97MzCL1gu2u7oy38W6L2ZIGECEfIBG4BabsWDPjBxJENv9Q==} + sisteransi@1.0.5: resolution: {integrity: sha512-bLGGlR1QxBcynn2d5YmDX4MGjlZvy2MRBDRNHLJ8VI6l6+9FUiyTFNJ0IveOSP0bcXgVDPRcfGqA0pjaqUpfVg==} @@ -4399,6 +4408,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 + '@swc/helpers@0.5.19': dependencies: tslib: 2.8.1 @@ -5950,6 +5965,16 @@ snapshots: transitivePeerDependencies: - supports-color + simple-git@3.36.0: + dependencies: + '@kwsites/file-exists': 1.1.1 + '@kwsites/promise-deferred': 1.1.1 + '@simple-git/args-pathspec': 1.0.3 + '@simple-git/argv-parser': 1.1.1 + debug: 4.4.3 + transitivePeerDependencies: + - supports-color + sisteransi@1.0.5: {} slash@3.0.0: {}