From 993b5f230ff4b5120d35463b30e4b2aa7c6ded40 Mon Sep 17 00:00:00 2001 From: haelyra <49814733+haelyra@users.noreply.github.com> Date: Fri, 9 Oct 2026 20:32:11 -0400 Subject: [PATCH 1/2] fix: guard literal Git hook redirects and env split arguments Carry forward bounded literal include/includeIf configuration capabilities and env -S/--split-string words from PRs #2858 and #2897. Charge array copy and expansion work before allocation to preserve the shared quota. Verified 731 hook cases and 60 real Git/config and registered-route controls. Dynamic remainder and original full source PRs remain held. Co-authored-by: Nguyen Thanh Dat Co-authored-by: Davemaina1 --- scripts/hooks/block-no-verify.js | 35 +++++++-- tests/hooks/block-no-verify.test.js | 118 ++++++++++++++++++++++++++++ 2 files changed, 148 insertions(+), 5 deletions(-) diff --git a/scripts/hooks/block-no-verify.js b/scripts/hooks/block-no-verify.js index 80ea60396..4448c96ef 100644 --- a/scripts/hooks/block-no-verify.js +++ b/scripts/hooks/block-no-verify.js @@ -25,7 +25,15 @@ let raw = ''; // so we normalize the candidate token to lowercase before matching. // See https://git-scm.com/docs/git-config — "The variable names are // case-insensitive." -const GIT_CONFIG_KEY_PREFIX = 'core.hookspath='; +function isHookRedirectConfigKey(key) { + const normalized = key.toLowerCase(); + return normalized === 'core.hookspath' || normalized === 'include.path' + || /^includeif\..*\.path$/.test(normalized); +} + +function isHookRedirectSetting(setting) { + return isHookRedirectConfigKey(setting.split('=', 1)[0]); +} const COMMIT_OPTIONS_WITH_VALUE = new Set([ '-m', @@ -189,7 +197,7 @@ function gitEnvironmentOverride(environment, budget) { const key = environment.get(`GIT_CONFIG_KEY_${i}`); if (key === undefined || !environment.has(`GIT_CONFIG_VALUE_${i}`)) { complete = false; break; } budget.spend(key.length + 1); - override ||= key.toLowerCase() === 'core.hookspath'; + override ||= isHookRedirectConfigKey(key); } if (complete && override) return true; } @@ -201,7 +209,7 @@ function gitEnvironmentOverride(environment, budget) { for (const command of scanShell(parameters, budget).commands) { for (const word of command.words) { budget.spend(word.value.length + 1); - if (word.value.toLowerCase().startsWith(GIT_CONFIG_KEY_PREFIX)) return true; + if (isHookRedirectSetting(word.value)) return true; } } } @@ -219,8 +227,9 @@ function checkGitWords(words, budget, start = 0, environmentOverride = false) { if (value === '-c' || value === '--config-env') { const setting = words[index + 1]?.value || ''; budget.spend(setting.length + 1); - override ||= setting.toLowerCase().startsWith(GIT_CONFIG_KEY_PREFIX); - } else if (value.toLowerCase().startsWith(`-c${GIT_CONFIG_KEY_PREFIX}`) || value.toLowerCase().startsWith(`--config-env=${GIT_CONFIG_KEY_PREFIX}`)) override = true; + override ||= isHookRedirectSetting(setting); + } else if (value.startsWith('-c')) override ||= isHookRedirectSetting(value.slice(2)); + else if (value.startsWith('--config-env=')) override ||= isHookRedirectSetting(value.slice('--config-env='.length)); if (GIT_GLOBAL_VALUES.has(value)) index++; } const command = words[index]?.value; @@ -358,6 +367,22 @@ function executableWords(words, budget, inherited = new Map(), callerValues = in const flag = words[i].value; budget.spend(flag.length + 1); if (flag === '--') { i++; break; } + if (env && (flag === '-S' || flag === '--split-string' + || flag.startsWith('--split-string=') || flag.startsWith('-S'))) { + const separated = flag === '-S' || flag === '--split-string'; + const payload = separated ? words[i + 1]?.value || '' + : flag.slice(flag.startsWith('--split-string=') ? '--split-string='.length : 2); + budget.spend(payload.length + 1); + const scan = scanShell(payload, budget); + // Only literal word splitting is understood here. Keep complex env + // syntax opaque rather than pretending to execute it as a shell. + if (scan.commands.length !== 1 || scan.commands[0].nested.length + || scan.commands[0].redirects.length || scan.commands[0].words.some(word => word.dynamic)) return suffix(i - 1); + // Pay for both slices and the expanded array before allocating them. + budget.spend(2 * words.length + scan.commands[0].words.length); + words = [...words.slice(0, i), ...scan.commands[0].words, ...words.slice(i + (separated ? 2 : 1))]; + continue; + } if (env && (flag === '-i' || flag === '--ignore-environment')) resetEnvironment(); if (env && (flag === '-u' || flag === '--unset')) resetEnvironment(words[i + 1]?.value || ''); else if (env && flag.startsWith('--unset=')) resetEnvironment(flag.slice('--unset='.length)); diff --git a/tests/hooks/block-no-verify.test.js b/tests/hooks/block-no-verify.test.js index 0dc0375ae..c3d3913d7 100644 --- a/tests/hooks/block-no-verify.test.js +++ b/tests/hooks/block-no-verify.test.js @@ -1994,6 +1994,124 @@ else { } } +// Contributor carry-forward regressions for #2858 and #2897. +for (const [name, command, expected] of [ + [ + "blocks include.path config override", + "git -c include.path=/tmp/evil.conf commit -m x", + 2 + ], + [ + "blocks quoted include.path config", + "git \"-c\" \"include.path=/tmp/evil.conf\" push origin main", + 2 + ], + [ + "blocks inline include.path config", + "git -cinclude.path=/tmp/evil.conf commit -m x", + 2 + ], + [ + "blocks include.path config-env", + "git --config-env=include.path=EVIL commit -m x", + 2 + ], + [ + "blocks separated include.path config-env", + "git --config-env include.path=EVIL commit -m x", + 2 + ], + [ + "blocks case-insensitive include.path", + "git -c INCLUDE.PATH=/tmp/evil.conf commit -m x", + 2 + ], + [ + "blocks conditional include capability", + "git -c includeIf.gitdir:**/.path=/tmp/evil.conf commit -m x", + 2 + ], + [ + "blocks counted include.path environment", + "GIT_CONFIG_COUNT=1 GIT_CONFIG_KEY_0=include.path GIT_CONFIG_VALUE_0=/tmp/evil.conf git commit -m x", + 2 + ], + [ + "blocks encoded include.path parameters", + "GIT_CONFIG_PARAMETERS=\"'include.path'='/tmp/evil.conf'\" git commit -m x", + 2 + ], + [ + "allows unrelated config value", + "git -c user.name=ECC commit -m x", + 0 + ], + [ + "allows include setting outside protected command", + "git -c include.path=/tmp/evil.conf status", + 0 + ], + [ + "blocks env long inline split-string", + "env --split-string='git commit --no-verify -m x'", + 2 + ], + [ + "blocks env split-string after ignore-environment", + "env -i --split-string='git commit --no-verify -m x'", + 2 + ], + [ + "blocks env separated long split-string", + "env --split-string 'git commit --no-verify -m x'", + 2 + ], + [ + "blocks env attached short split-string", + "env -S'git commit --no-verify -m x'", + 2 + ], + [ + "blocks appended argv after env split-string", + "env --split-string='git commit' --no-verify -m x", + 2 + ], + [ + "allows quoted bypass text passed through env printf", + "env --split-string=\"printf '%s' 'git commit --no-verify -m x'\"", + 0 + ] +]) { + if (test(name, () => { + const result = runHook({ tool_input: { command } }); + assert.strictEqual(result.code, expected, result.stderr); + })) passed++; else failed++; +} + +for (const n of [100, 1000, 4000]) { + for (const tail of ['git status', 'git commit --no-verify']) { + if (test(`literal env split copies stay within the input quota at ${n}: ${tail}`, () => { + const command = 'env ' + '-S -i '.repeat(n) + tail; + const quota = 24 * (command.length + 1) + 4096; + const result = countedClassification(command); + assert.strictEqual(result.result.exitCode, 2, JSON.stringify(result)); + assert.match(result.result.stderr, /work budget/); + assert.ok(result.copiedElements <= quota, JSON.stringify(result)); + assert.ok(result.copiedElements <= result.spent, JSON.stringify(result)); + // The rejected charge may exceed the quota, but allocation must not run. + assert.ok(result.spent >= quota && result.spent <= quota + 2 * (2 * n + 4) + 1, JSON.stringify(result)); + })) passed++; else failed++; + } +} +for (const n of [0, 1, 2, 4]) { + if (test(`shallow literal env splits allow harmless argv at ${n}`, () => { + const command = 'env ' + '-S -i '.repeat(n) + 'git status'; + const result = countedClassification(command); + assert.strictEqual(result.result.exitCode, 0, JSON.stringify(result)); + assert.ok(result.copiedElements <= result.spent, JSON.stringify(result)); + })) passed++; else failed++; +} + console.log('─'.repeat(50)); console.log(`Passed: ${passed} Failed: ${failed}`); From 34a97fa898a1b8c23766a80c12657afd6b85222a Mon Sep 17 00:00:00 2001 From: haelyra <49814733+haelyra@users.noreply.github.com> Date: Fri, 9 Oct 2026 21:00:40 -0400 Subject: [PATCH 2/2] fix: reject unsupported env split-string escapes Validate env -S and --split-string escape syntax before shell scanning or opaque fallback. Preserve literal single-quoted data and charge each validation character against the shared input-proportional work budget. Verified 787 focused cases and 290 real Git/config, harmless native env and registered-routing controls. Keeps the prior paid-copy protection. Co-authored-by: Nguyen Thanh Dat Co-authored-by: Davemaina1 --- scripts/hooks/block-no-verify.js | 21 ++++++++++++++++++++ tests/hooks/block-no-verify.test.js | 30 +++++++++++++++++++++++++++++ 2 files changed, 51 insertions(+) diff --git a/scripts/hooks/block-no-verify.js b/scripts/hooks/block-no-verify.js index 4448c96ef..962dd9e74 100644 --- a/scripts/hooks/block-no-verify.js +++ b/scripts/hooks/block-no-verify.js @@ -266,6 +266,23 @@ function assignmentValues(prior, operand, append, dynamic, budget) { return [...values]; } +class UnsupportedEnvSplitEscape extends Error {} + +function validateEnvSplitEscapes(payload, budget) { + let quote = null; + for (let index = 0; index < payload.length; index++) { + budget.spend(); + const character = payload[index]; + // env -S has its own escapes. Only literal single-quoted backslashes + // (excluding env's \\ and \' escapes) agree with the shell scanner. + if (character === '\\' && (quote !== "'" || payload[index + 1] === '\\' || payload[index + 1] === "'")) { + throw new UnsupportedEnvSplitEscape(); + } + if (character === "'" && quote !== '"') quote = quote === "'" ? null : "'"; + else if (character === '"' && quote !== "'") quote = quote === '"' ? null : '"'; + } +} + // Only explicit option grammars remove wrapper operands. Unknown launchers are // opaque/conservative, never guessed from a name found among data arguments. function executableWords(words, budget, inherited = new Map(), callerValues = inherited) { @@ -373,6 +390,7 @@ function executableWords(words, budget, inherited = new Map(), callerValues = in const payload = separated ? words[i + 1]?.value || '' : flag.slice(flag.startsWith('--split-string=') ? '--split-string='.length : 2); budget.spend(payload.length + 1); + validateEnvSplitEscapes(payload, budget); const scan = scanShell(payload, budget); // Only literal word splitting is understood here. Keep complex env // syntax opaque rather than pretending to execute it as a shell. @@ -726,6 +744,9 @@ function checkCommand(input) { } } } catch (error) { + if (error instanceof UnsupportedEnvSplitEscape) { + return { blocked: true, reason: 'BLOCKED: Unsupported env split-string escape; hook-bypass safety could not be established.' }; + } if (!(error instanceof RangeError)) throw error; return { blocked: true, reason: 'BLOCKED: Shell analysis work budget exceeded; hook-bypass safety could not be established.' }; } diff --git a/tests/hooks/block-no-verify.test.js b/tests/hooks/block-no-verify.test.js index c3d3913d7..5fa21cef0 100644 --- a/tests/hooks/block-no-verify.test.js +++ b/tests/hooks/block-no-verify.test.js @@ -2112,6 +2112,36 @@ for (const n of [0, 1, 2, 4]) { })) passed++; else failed++; } +const quoteSplitPayload = payload => `'${payload.replace(/'/g, `'\\''`)}'`; +const escapedSplitCases = [ + ['unquoted escaped separators', 'git\\_commit\\_--no-verify', 2], + ['single-quoted literal separators', "'git\\_commit\\_--no-verify'", 0], + ['double-quoted escaped separators', '"git\\_commit\\_--no-verify"', 2], + ['mixed quoted escaped separators', 'git\\_commit\\_"--no-verify"', 2], + ['unsupported stop escape', 'printf ok\\c ignored', 2], + ['unsupported double-quoted stop escape', 'printf "ok\\c" ignored', 2], + ['single-quoted stop data', "printf '%s' 'ok\\c'", 0], + ['single-quoted separator data', "printf '%s' 'git\\_commit\\_--no-verify'", 0], + ['single-quoted newline format', "printf '%s\\n' 'git commit --no-verify'", 0], + ['unsupported newline escape', 'printf ok\\n', 2], + ['unsupported tab escape', 'printf ok\\t', 2], + ['unsupported escaped backslash', 'printf ok\\\\_', 2], + ['unsupported single-quoted backslash escape', "printf '%s' 'ok\\\\_'", 2], + ['unsupported single-quoted apostrophe escape', "printf '%s' 'ok\\'data'", 2], +]; +for (const [name, payload, expected] of escapedSplitCases) { + for (const option of ['-S ', '-S', '--split-string=', '--split-string ']) { + if (test(`env escape boundary ${name}: ${option}`, () => { + const command = 'env ' + option + quoteSplitPayload(payload); + for (const input of [command, JSON.stringify({ tool_input: { command } })]) { + const result = runHook(input); + assert.strictEqual(result.code, expected, result.stderr); + if (expected === 2) assert.match(result.stderr, /Unsupported env split-string escape/); + } + })) passed++; else failed++; + } +} + console.log('─'.repeat(50)); console.log(`Passed: ${passed} Failed: ${failed}`);