From eca846248d6e6837d0881f7dcc251fc390aa37bd Mon Sep 17 00:00:00 2001 From: affaan-m <124439313+affaan-m@users.noreply.github.com> Date: Mon, 28 Sep 2026 07:05:14 -0400 Subject: [PATCH] fix(hooks): track append assignments in Git override checks Preserve the original contributor histories and apply the exact independently reviewed repair. Source-Parent: dee884656eb5ed3bee0129d63fd762e146672a41 Review-Manifest-SHA256: 0c7eeef7c2722a605d049daf2dc3c2473c10dd616005afcba21049fc036d8ce9 --- scripts/hooks/block-no-verify.js | 75 +++++++++++----- tests/hooks/block-no-verify.test.js | 133 +++++++++++++++++++++++++++- 2 files changed, 183 insertions(+), 25 deletions(-) diff --git a/scripts/hooks/block-no-verify.js b/scripts/hooks/block-no-verify.js index abf289779..80ea60396 100644 --- a/scripts/hooks/block-no-verify.js +++ b/scripts/hooks/block-no-verify.js @@ -244,6 +244,19 @@ function checkGitWords(words, budget, start = 0, environmentOverride = false) { return null; } +// Keep literal outcomes and the empty result of an unresolved expansion. The +// latter is a base for later visible += operands, not arbitrary evaluation. +function assignmentValues(prior, operand, append, dynamic, budget) { + const base = prior === undefined ? '' : prior; + budget.spend((append ? base.length : 0) + operand.length + 1); + const values = new Set([append ? base + operand : operand]); + if (dynamic) { + if (prior !== undefined) values.add(prior); + values.add(append ? base : ''); + } + return [...values]; +} + // 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) { @@ -262,25 +275,30 @@ function executableWords(words, budget, inherited = new Map(), callerValues = in function assignment(token) { const { value, dynamic } = token; const equals = value.indexOf('='); - const key = value.slice(0, equals); + const append = !environmentAssignments && value[equals - 1] === '+'; + const key = value.slice(0, append ? equals - 1 : equals); if (/^GIT_CONFIG_(?:COUNT|PARAMETERS|(?:KEY|VALUE)_[0-9]+)$/.test(key)) { - const assigned = value.slice(equals + 1); + const operand = value.slice(equals + 1); const count = environments.length; budget.spend(count + 1); for (let n = 0; n < count; n++) { const environment = environments[n]; - // Expansion precedes env's reset. Caller values remain separate from - // the child environment; keep both that possible value and the new - // literal spelling without interpreting expansion syntax. - if (dynamic && callerValues.has(key)) { + // Shell prefix appends can see local values, even when not exported. + // Repeated operands use the prior outcome in this same prefix. + const prior = prefixAssignments.has(key) && environment.has(key) + ? environment.get(key) : callerValues.get(key); + const values = assignmentValues(prior, operand, append, dynamic, budget); + for (const alternative of values.slice(1)) { budget.spend(environment.size + 1); - const prior = new Map(environment); - prior.set(key, callerValues.get(key)); - environments.push(prior); + const variant = new Map(environment); + variant.set(key, alternative); + environments.push(variant); } - environment.set(key, assigned); + environment.set(key, values[0]); } - prefixAssignments.set(key, assigned); + // Retain ordered operations so same-shell states apply each append once. + if (!prefixAssignments.has(key)) prefixAssignments.set(key, []); + prefixAssignments.get(key).push({ value: operand, append, dynamic }); if (dynamic) dynamicAssignments.add(key); } } @@ -297,7 +315,7 @@ function executableWords(words, budget, inherited = new Map(), callerValues = in while (i < words.length) { const token = words[i]; budget.spend(token.value.length + token.raw.length + 1); - if (assignments && /^[A-Za-z_][A-Za-z0-9_]*=/.test(environmentAssignments ? token.value : token.raw)) { assignment(token); i++; continue; } + if (assignments && /^[A-Za-z_][A-Za-z0-9_]*\+?=/.test(environmentAssignments ? token.value : token.raw)) { assignment(token); i++; continue; } if (!token.quoted && CONTROL_WORDS.has(token.value)) { i++; continue; } const name = basename(token.value); if (name === 'command') { @@ -479,21 +497,30 @@ function updateShellState(state, normalized, budget) { const states = [state]; const result = (handled, changed, uncertain = false) => ({ handled, changed, uncertain, states }); if (!local) return result(false, false); - function assign(name, value, dynamic = false) { + function assign(name, value, dynamic = false, append = false) { const count = states.length; budget.spend(count + 1); for (let n = 0; n < count; n++) { const current = states[n]; if (current.readonly.has(name)) continue; - // Preserve the known possible value AND the new literal spelling. - // Both alternatives subsequently receive the declaration attributes. - if (dynamic && current.variables.has(name)) states.push(copyShellState(current, budget)); - current.variables.set(name, value); + const values = assignmentValues(current.variables.get(name), value, append, dynamic, budget); + for (const alternative of values.slice(1)) { + const variant = copyShellState(current, budget); + variant.variables.set(name, alternative); + states.push(variant); + } + current.variables.set(name, values[0]); + } + } + function assignPrefixes() { + budget.spend(prefixAssignments.size + 1); + for (const [key, operations] of prefixAssignments) { + budget.spend(operations.length + 1); + for (const operation of operations) assign(key, operation.value, operation.dynamic, operation.append); } } if (assignmentOnly) { - budget.spend(prefixAssignments.size + 1); - for (const [name, value] of prefixAssignments) assign(name, value, dynamicAssignments.has(name)); + assignPrefixes(); return result(true, prefixAssignments.size > 0, dynamicAssignments.size > 0); } // Exact builtin names only: /some/path/export is an external executable. @@ -521,17 +548,17 @@ function updateShellState(state, normalized, budget) { else uncertain = true; } if (passive && !uncertain) return result(true, false); - let changed = false; - budget.spend(prefixAssignments.size + 1); - for (const [key, value] of prefixAssignments) { assign(key, value, dynamicAssignments.has(key)); changed = true; } + let changed = prefixAssignments.size > 0; + assignPrefixes(); for (; i < words.length; i++) { const value = words[i].value; budget.spend(2 * value.length + 1); const equals = value.indexOf('='); - const key = equals < 0 ? value : value.slice(0, equals); + const append = equals > 0 && value[equals - 1] === '+'; + const key = equals < 0 ? value : value.slice(0, append ? equals - 1 : equals); if (!GIT_ENV_NAME.test(key)) continue; changed = true; - if (name !== 'unset' && equals >= 0) assign(key, value.slice(equals + 1), words[i].dynamic); + if (name !== 'unset' && equals >= 0) assign(key, value.slice(equals + 1), words[i].dynamic, append); budget.spend(states.length + 1); for (const current of states) { if (name === 'unset') { diff --git a/tests/hooks/block-no-verify.test.js b/tests/hooks/block-no-verify.test.js index 7081b6e59..0dc0375ae 100644 --- a/tests/hooks/block-no-verify.test.js +++ b/tests/hooks/block-no-verify.test.js @@ -1812,7 +1812,138 @@ const stickyEnvironmentCases = Object.freeze([ ["readonly print flag assignment prevents later reset", 2, "export GIT_CONFIG_PARAMETERS=\"'core.hooksPath=/dev/null'\"; readonly -p GIT_CONFIG_PARAMETERS=\"'core.hooksPath=/dev/null'\"; unset GIT_CONFIG_PARAMETERS; git commit"], ["readonly print flag safe Git control", 0, "readonly -p GIT_CONFIG_PARAMETERS=\"'core.hooksPath=/dev/null'\"; git status"] ]); -for (const [family, expected, command] of stickyEnvironmentCases) { +// Append assignments are shell syntax before a command or in declarations. +// env's NAME+=VALUE is a different, literal variable name, not shell append. +const appendEnvironmentCases = [ + ['prefix from unset', 'GIT_CONFIG_PARAMETERS+="\'core.hooksPath=/dev/null\'"'], + ['export from unset', 'export GIT_CONFIG_PARAMETERS+="\'core.hooksPath=/dev/null\'";'], + ['declare from unset', 'declare -x GIT_CONFIG_PARAMETERS+="\'core.hooksPath=/dev/null\'";'], + ['typeset from unset', 'typeset -x GIT_CONFIG_PARAMETERS+="\'core.hooksPath=/dev/null\'";'], + ['standalone then export', 'GIT_CONFIG_PARAMETERS+="\'core.hooksPath=/dev/null\'"; export GIT_CONFIG_PARAMETERS;'], + ['append to exported safe parameters', 'export GIT_CONFIG_PARAMETERS="\'color.ui=false\'"; GIT_CONFIG_PARAMETERS+=" \'core.hooksPath=/dev/null\'";'], + ['append split declaration', 'export GIT_CONFIG_PARAMETERS="\'core.hooks"; export GIT_CONFIG_PARAMETERS+="Path=/dev/null\'";'], + ['append split standalone', 'GIT_CONFIG_PARAMETERS="\'core.hooks"; GIT_CONFIG_PARAMETERS+="Path=/dev/null\'"; export GIT_CONFIG_PARAMETERS;'], + ['append split prefix from local', 'GIT_CONFIG_PARAMETERS="\'core.hooks"; GIT_CONFIG_PARAMETERS+="Path=/dev/null\'"'], + ['multiple prefix operands', 'GIT_CONFIG_PARAMETERS="\'core.hooks" GIT_CONFIG_PARAMETERS+="Path=/dev/null\'"'], + ['count triplet prefix', 'GIT_CONFIG_COUNT+=1 GIT_CONFIG_KEY_0+=core.hooksPath GIT_CONFIG_VALUE_0+=/dev/null'], + ['count triplet declaration', 'export GIT_CONFIG_COUNT+=1 GIT_CONFIG_KEY_0+=core.hooksPath GIT_CONFIG_VALUE_0+=/dev/null;'], + ['split key declaration', 'export GIT_CONFIG_COUNT=1 GIT_CONFIG_KEY_0=core. GIT_CONFIG_VALUE_0=/dev/null; declare -x GIT_CONFIG_KEY_0+=hooksPath;'], + ['split key prefix', 'GIT_CONFIG_KEY_0=core.; GIT_CONFIG_COUNT=1 GIT_CONFIG_KEY_0+=hooksPath GIT_CONFIG_VALUE_0=/dev/null'], + ['conditional append possible', 'false && export GIT_CONFIG_PARAMETERS+="\'core.hooksPath=/dev/null\'";'], + ['append cannot erase prior unknown-value alternative', 'export GIT_CONFIG_PARAMETERS="\'core.hooksPath=/dev/null\'"; GIT_CONFIG_PARAMETERS+="$UNKNOWN";'], + ['dynamic prefix cannot erase prior alternative', 'export GIT_CONFIG_PARAMETERS="\'core.hooksPath=/dev/null\'"; GIT_CONFIG_PARAMETERS+="$UNKNOWN"'], + ['dynamic declaration retains new literal operand', 'export GIT_CONFIG_PARAMETERS=""; export GIT_CONFIG_PARAMETERS+="\'core.hooksPath=/dev/null\'$UNKNOWN";'], + ['dynamic prefix retains new literal operand', 'GIT_CONFIG_PARAMETERS+="\'core.hooksPath=/dev/null\'$UNKNOWN"'], +]; +const appendCases = []; +for (const [family, setup] of appendEnvironmentCases) { + appendCases.push([`append ${family}`, 2, `${setup} git commit`]); + appendCases.push([`append ${family} safe Git control`, 0, `${setup} git status`]); +} +appendCases.push( + ['unexported append stays local', 0, 'GIT_CONFIG_PARAMETERS+="\'core.hooksPath=/dev/null\'"; git commit'], + ['export attribute removal remains effective', 0, 'export GIT_CONFIG_PARAMETERS+="\'core.hooksPath=/dev/null\'"; export -n GIT_CONFIG_PARAMETERS; git commit'], + ['explicit unset removes appended state', 0, 'export GIT_CONFIG_PARAMETERS+="\'core.hooksPath=/dev/null\'"; unset GIT_CONFIG_PARAMETERS; git commit'], + ['child environment reset removes appended state', 0, 'export GIT_CONFIG_PARAMETERS+="\'core.hooksPath=/dev/null\'"; env -i git commit'], + ['env append-like name is literal data', 0, 'env GIT_CONFIG_PARAMETERS+="\'core.hooksPath=/dev/null\'" git commit'], + ['env append-like name after reset is literal data', 0, 'export GIT_CONFIG_PARAMETERS="\'core.hooksPath=/dev/null\'"; env -i GIT_CONFIG_PARAMETERS+= git commit'], + ['env literal name does not reset real exported key', 2, 'export GIT_CONFIG_PARAMETERS="\'core.hooksPath=/dev/null\'"; env GIT_CONFIG_PARAMETERS+= git commit'], + ['quoted shell assignment name stays data', 0, '"GIT_CONFIG_PARAMETERS+=\'core.hooksPath=/dev/null\'" git commit'], + ['readonly safe value cannot acquire appended override', 0, 'declare -rx GIT_CONFIG_PARAMETERS=""; GIT_CONFIG_PARAMETERS+="\'core.hooksPath=/dev/null\'"; git commit'], + ['readonly unsafe value cannot lose override through append', 2, 'export GIT_CONFIG_PARAMETERS="\'core.hooksPath=/dev/null\'"; readonly GIT_CONFIG_PARAMETERS; GIT_CONFIG_PARAMETERS+="x"; git commit'], + ['ordinary append parameters do not disable hooks', 0, 'export GIT_CONFIG_PARAMETERS="\'color.ui="; GIT_CONFIG_PARAMETERS+="false\'"; git commit'], + ['empty appended count is not an override', 0, 'export GIT_CONFIG_COUNT=0; GIT_CONFIG_COUNT+=""; git commit'], +); + +appendCases.push(...[ + [ + "unknown prior export commit", + 2, + "export GIT_CONFIG_PARAMETERS=\"$UNKNOWN\"; export GIT_CONFIG_PARAMETERS+=\"'core.hooksPath=/dev/null'\"; git commit" + ], + [ + "unknown prior export status", + 0, + "export GIT_CONFIG_PARAMETERS=\"$UNKNOWN\"; export GIT_CONFIG_PARAMETERS+=\"'core.hooksPath=/dev/null'\"; git status" + ], + [ + "unknown prior standalone commit", + 2, + "GIT_CONFIG_PARAMETERS=\"$UNKNOWN\"; GIT_CONFIG_PARAMETERS+=\"'core.hooksPath=/dev/null'\"; export GIT_CONFIG_PARAMETERS; git commit" + ], + [ + "unknown prior standalone status", + 0, + "GIT_CONFIG_PARAMETERS=\"$UNKNOWN\"; GIT_CONFIG_PARAMETERS+=\"'core.hooksPath=/dev/null'\"; export GIT_CONFIG_PARAMETERS; git status" + ], + [ + "unknown prior prefix commit", + 2, + "GIT_CONFIG_PARAMETERS=\"$UNKNOWN\"; GIT_CONFIG_PARAMETERS+=\"'core.hooksPath=/dev/null'\" git commit" + ], + [ + "unknown prior prefix status", + 0, + "GIT_CONFIG_PARAMETERS=\"$UNKNOWN\"; GIT_CONFIG_PARAMETERS+=\"'core.hooksPath=/dev/null'\" git status" + ], + [ + "unknown repeated prefix commit", + 2, + "GIT_CONFIG_PARAMETERS=\"$UNKNOWN\" GIT_CONFIG_PARAMETERS+=\"'core.hooksPath=/dev/null'\" git commit" + ], + [ + "unknown repeated prefix status", + 0, + "GIT_CONFIG_PARAMETERS=\"$UNKNOWN\" GIT_CONFIG_PARAMETERS+=\"'core.hooksPath=/dev/null'\" git status" + ], + [ + "three ordered prefix operands commit", + 2, + "GIT_CONFIG_PARAMETERS=\"'core.\" GIT_CONFIG_PARAMETERS+=\"hooks\" GIT_CONFIG_PARAMETERS+=\"Path=/dev/null'\" git commit" + ], + [ + "three ordered prefix operands status", + 0, + "GIT_CONFIG_PARAMETERS=\"'core.\" GIT_CONFIG_PARAMETERS+=\"hooks\" GIT_CONFIG_PARAMETERS+=\"Path=/dev/null'\" git status" + ], + [ + "unknown count followed by literal append commit", + 2, + "export GIT_CONFIG_COUNT=\"$UNKNOWN\" GIT_CONFIG_KEY_0=core.hooksPath GIT_CONFIG_VALUE_0=/dev/null; GIT_CONFIG_COUNT+=1; git commit" + ], + [ + "unknown count followed by literal append status", + 0, + "export GIT_CONFIG_COUNT=\"$UNKNOWN\" GIT_CONFIG_KEY_0=core.hooksPath GIT_CONFIG_VALUE_0=/dev/null; GIT_CONFIG_COUNT+=1; git status" + ], + [ + "unknown prior child context commit", + 2, + "export GIT_CONFIG_PARAMETERS=\"$UNKNOWN\"; sh -c \"GIT_CONFIG_PARAMETERS+=\\\"'core.hooksPath=/dev/null'\\\"; git commit\"" + ], + [ + "unknown prior child context status", + 0, + "export GIT_CONFIG_PARAMETERS=\"$UNKNOWN\"; sh -c \"GIT_CONFIG_PARAMETERS+=\\\"'core.hooksPath=/dev/null'\\\"; git status\"" + ], + [ + "literal prior is not unresolved '$UNKNOWN'", + 0, + "GIT_CONFIG_PARAMETERS='$UNKNOWN'; export GIT_CONFIG_PARAMETERS+=\"'core.hooksPath=/dev/null'\"; git commit" + ], + [ + "literal prior is not unresolved \\$UNKNOWN", + 0, + "GIT_CONFIG_PARAMETERS=\\$UNKNOWN; export GIT_CONFIG_PARAMETERS+=\"'core.hooksPath=/dev/null'\"; git commit" + ], + [ + "literal prior is not unresolved \"\\$UNKNOWN\"", + 0, + "GIT_CONFIG_PARAMETERS=\"\\$UNKNOWN\"; export GIT_CONFIG_PARAMETERS+=\"'core.hooksPath=/dev/null'\"; git commit" + ] +]); + +for (const [family, expected, command] of [...stickyEnvironmentCases, ...appendCases]) { if (test(`${family} ${expected}: ${JSON.stringify(command)}`, () => { const result = runHook(command); assert.strictEqual(result.code, expected, result.stderr);