From 085d40b1a1193218fa71fbede10e0f43f96240f4 Mon Sep 17 00:00:00 2001 From: carlos-alm Date: Tue, 18 Aug 2026 23:18:45 -0600 Subject: [PATCH 1/2] fix(hooks): normalize-ifs.mjs generalizes :+/+ whitespace bypass beyond IFS \${IFS:+ }/\${IFS+ } was matched by literal name, but bash's alternate-value expansion substitutes its own "word" whenever the named variable is set and non-null -- true for almost any commonly-set variable (HOME, PWD, PATH, ...), not just IFS. The substituted text has nothing to do with the named variable's own value, so a per-variable-name check could never fully close this class: an obfuscator can pick any variable known to be set in the target shell. This one replacement (only) now matches any bash identifier shape in that position, instead of the literal name IFS -- the other three replacements stay IFS-specific since they extract from IFS's own known default value, which doesn't generalize the same way. Closes #2558 docs check acknowledged Impact: 1 functions changed, 0 affected --- .claude/hooks/normalize-ifs.mjs | 68 +++++++++++-------- .../claude-code-hooks/normalize-ifs.mjs | 68 +++++++++++-------- tests/unit/hook-guard-git-ifs-bypass.test.ts | 24 +++++++ 3 files changed, 106 insertions(+), 54 deletions(-) diff --git a/.claude/hooks/normalize-ifs.mjs b/.claude/hooks/normalize-ifs.mjs index 0f8c24f39..db96f7f3c 100644 --- a/.claude/hooks/normalize-ifs.mjs +++ b/.claude/hooks/normalize-ifs.mjs @@ -1,6 +1,8 @@ #!/usr/bin/env node // normalize-ifs.mjs — reads a shell command line from stdin and writes it -// back with every $IFS/${IFS} reference replaced by a single literal space +// back with every $IFS/${IFS} reference (plus, for the `:+`/`+` +// alternate-value form only, the equivalent reference to any other +// normally-set variable — #2558) replaced by a single literal space // (#2451). Bash's own field-splitting on an unquoted $IFS/${IFS} expansion // produces exactly this effect at execution time — `git${IFS}checkout` // looks like one token as command TEXT, but Git actually receives @@ -82,37 +84,49 @@ // a run of ALL zeros after the minus sign — matched separately here via // `-0+` (a literal minus followed by one or more zeros and nothing else). // -// `${IFS:+ }`/`${IFS+ }` (alternate-value expansion, restricted to +// `${VAR:+ }`/`${VAR+ }` (alternate-value expansion, restricted to // ALL-whitespace content — Greptile review): unlike substring, this -// operator does NOT extract from IFS's own value at all — it substitutes -// an entirely separate, attacker-chosen string `word` whenever IFS IS set -// and non-null (`:+`) or merely set (`+`), which normally means `word` is -// what actually comes out, not IFS's value. Because of that, this is -// matched ONLY when `word` consists of one or more spaces/tabs and -// NOTHING else — `${IFS:+ }` really does expand to a literal space -// (`word` itself, not derived from IFS), so it's exactly as safe to -// normalize as the whole-variable form; `${IFS:+x}` is not touched, since +// operator does NOT extract from the variable's own value at all — it +// substitutes an entirely separate, attacker-chosen string `word` whenever +// VAR IS set and non-null (`:+`) or merely set (`+`), which normally means +// `word` is what actually comes out, not VAR's value. Because of that, +// this is matched ONLY when `word` consists of one or more spaces/tabs and +// NOTHING else — `${VAR:+ }` really does expand to a literal space +// (`word` itself, not derived from VAR), so it's exactly as safe to +// normalize as the whole-variable form; `${VAR:+x}` is not touched, since // `x` is not whitespace and substituting a space for it would fabricate a // token boundary bash never produces (an earlier version of this // normalizer treated the operator as unconditionally unsafe and missed // this whitespace-content special case — Greptile review). // -// Deliberately does NOT generalize to "any `${IFS...}`": an -// earlier version tried that and was wrong (Greptile review) — -// `${IFS/pattern/replacement}` (substitutes `replacement` wherever -// `pattern` matches within IFS's value) and the bash 4.4+ `${IFS@Q}`-style -// transformation operators (e.g. `@Q` shell-quotes the value, producing -// `$' \t\n'`-shaped text) can ALSO produce arbitrary non-whitespace text, -// the same class of problem `:+`/`+` have — and unlike `:+`/`+`, there is -// no simple "restrict to all-whitespace content" fix for them, since -// `pattern`/`replacement ` are two separate, differently-shaped fields. -// Left unhandled rather than risk another incorrect generalization; a -// determined obfuscator using one of these is a known, accepted gap in -// this heuristic guard (see #2558 for tracking the closely related, -// broader problem that `:+`/`+` with all-whitespace content isn't even -// unique to the `IFS` name — `${HOME:+ }`/`${PWD:+ }`/any other normally-set -// variable works identically, which a per-variable-name normalizer like -// this one can never fully close). +// Unlike the other three replacements below, this one is NOT restricted to +// the literal name `IFS`: the operator's behavior has nothing to do with +// what VAR's own value actually is, only with whether VAR is normally set +// and non-null — true for almost any commonly-set variable (`HOME`, `PWD`, +// `PATH`, ...), not just `IFS`. A per-variable-name check could never fully +// close this class, since the variable name in the bypass isn't fixed +// (#2558) — so this one matches any bash identifier shape +// (`[A-Za-z_][A-Za-z0-9_]*`) in that position, erring toward normalizing +// (the safe direction for a guard) even for a variable that happens not to +// be set in a given shell, rather than trying to track which variables are +// actually set. +// +// Deliberately does NOT generalize the other three replacements below to +// "any `${VAR...}`" — only `:+`/`+` generalizes across variable +// names, because only its substituted text is entirely independent of the +// variable's own value. `${IFS/pattern/replacement}` (substitutes +// `replacement` wherever `pattern` matches within IFS's value) and the +// bash 4.4+ `${IFS@Q}`-style transformation operators (e.g. `@Q` +// shell-quotes the value, producing `$' \t\n'`-shaped text) can ALSO +// produce arbitrary non-whitespace text, the same class of problem `:+`/`+` +// have — but unlike `:+`/`+`, there is no simple "restrict to all-whitespace +// content" fix for them (`pattern`/`replacement` are two separate, +// differently-shaped fields), AND they only produce a whitespace-only +// result by relying on IFS's own specific default value in the first +// place, so generalizing them to other variable names wouldn't even be +// meaningful the way it is for `:+`/`+`. Left unhandled rather than risk +// another incorrect generalization; a determined obfuscator using one of +// these remains a known, accepted gap in this heuristic guard. // // The bare `$IFS` form must not swallow the start of a longer variable // name — `$IFSOMETHING` references a completely different (and almost @@ -136,7 +150,7 @@ process.stdin.on('end', () => { const normalized = input .replace(/\$\{IFS\}/g, ' ') .replace(/\$\{IFS: *(?:0*[0-2]|-0*[1-3]|-0+)(?::0*[1-9]\d*)?\}/g, ' ') - .replace(/\$\{IFS:?\+[ \t]+\}/g, ' ') + .replace(/\$\{[A-Za-z_][A-Za-z0-9_]*:?\+[ \t]+\}/g, ' ') .replace(/\$IFS(?![A-Za-z0-9_])/g, ' '); process.stdout.write(normalized); }); diff --git a/docs/examples/claude-code-hooks/normalize-ifs.mjs b/docs/examples/claude-code-hooks/normalize-ifs.mjs index 0f8c24f39..db96f7f3c 100644 --- a/docs/examples/claude-code-hooks/normalize-ifs.mjs +++ b/docs/examples/claude-code-hooks/normalize-ifs.mjs @@ -1,6 +1,8 @@ #!/usr/bin/env node // normalize-ifs.mjs — reads a shell command line from stdin and writes it -// back with every $IFS/${IFS} reference replaced by a single literal space +// back with every $IFS/${IFS} reference (plus, for the `:+`/`+` +// alternate-value form only, the equivalent reference to any other +// normally-set variable — #2558) replaced by a single literal space // (#2451). Bash's own field-splitting on an unquoted $IFS/${IFS} expansion // produces exactly this effect at execution time — `git${IFS}checkout` // looks like one token as command TEXT, but Git actually receives @@ -82,37 +84,49 @@ // a run of ALL zeros after the minus sign — matched separately here via // `-0+` (a literal minus followed by one or more zeros and nothing else). // -// `${IFS:+ }`/`${IFS+ }` (alternate-value expansion, restricted to +// `${VAR:+ }`/`${VAR+ }` (alternate-value expansion, restricted to // ALL-whitespace content — Greptile review): unlike substring, this -// operator does NOT extract from IFS's own value at all — it substitutes -// an entirely separate, attacker-chosen string `word` whenever IFS IS set -// and non-null (`:+`) or merely set (`+`), which normally means `word` is -// what actually comes out, not IFS's value. Because of that, this is -// matched ONLY when `word` consists of one or more spaces/tabs and -// NOTHING else — `${IFS:+ }` really does expand to a literal space -// (`word` itself, not derived from IFS), so it's exactly as safe to -// normalize as the whole-variable form; `${IFS:+x}` is not touched, since +// operator does NOT extract from the variable's own value at all — it +// substitutes an entirely separate, attacker-chosen string `word` whenever +// VAR IS set and non-null (`:+`) or merely set (`+`), which normally means +// `word` is what actually comes out, not VAR's value. Because of that, +// this is matched ONLY when `word` consists of one or more spaces/tabs and +// NOTHING else — `${VAR:+ }` really does expand to a literal space +// (`word` itself, not derived from VAR), so it's exactly as safe to +// normalize as the whole-variable form; `${VAR:+x}` is not touched, since // `x` is not whitespace and substituting a space for it would fabricate a // token boundary bash never produces (an earlier version of this // normalizer treated the operator as unconditionally unsafe and missed // this whitespace-content special case — Greptile review). // -// Deliberately does NOT generalize to "any `${IFS...}`": an -// earlier version tried that and was wrong (Greptile review) — -// `${IFS/pattern/replacement}` (substitutes `replacement` wherever -// `pattern` matches within IFS's value) and the bash 4.4+ `${IFS@Q}`-style -// transformation operators (e.g. `@Q` shell-quotes the value, producing -// `$' \t\n'`-shaped text) can ALSO produce arbitrary non-whitespace text, -// the same class of problem `:+`/`+` have — and unlike `:+`/`+`, there is -// no simple "restrict to all-whitespace content" fix for them, since -// `pattern`/`replacement ` are two separate, differently-shaped fields. -// Left unhandled rather than risk another incorrect generalization; a -// determined obfuscator using one of these is a known, accepted gap in -// this heuristic guard (see #2558 for tracking the closely related, -// broader problem that `:+`/`+` with all-whitespace content isn't even -// unique to the `IFS` name — `${HOME:+ }`/`${PWD:+ }`/any other normally-set -// variable works identically, which a per-variable-name normalizer like -// this one can never fully close). +// Unlike the other three replacements below, this one is NOT restricted to +// the literal name `IFS`: the operator's behavior has nothing to do with +// what VAR's own value actually is, only with whether VAR is normally set +// and non-null — true for almost any commonly-set variable (`HOME`, `PWD`, +// `PATH`, ...), not just `IFS`. A per-variable-name check could never fully +// close this class, since the variable name in the bypass isn't fixed +// (#2558) — so this one matches any bash identifier shape +// (`[A-Za-z_][A-Za-z0-9_]*`) in that position, erring toward normalizing +// (the safe direction for a guard) even for a variable that happens not to +// be set in a given shell, rather than trying to track which variables are +// actually set. +// +// Deliberately does NOT generalize the other three replacements below to +// "any `${VAR...}`" — only `:+`/`+` generalizes across variable +// names, because only its substituted text is entirely independent of the +// variable's own value. `${IFS/pattern/replacement}` (substitutes +// `replacement` wherever `pattern` matches within IFS's value) and the +// bash 4.4+ `${IFS@Q}`-style transformation operators (e.g. `@Q` +// shell-quotes the value, producing `$' \t\n'`-shaped text) can ALSO +// produce arbitrary non-whitespace text, the same class of problem `:+`/`+` +// have — but unlike `:+`/`+`, there is no simple "restrict to all-whitespace +// content" fix for them (`pattern`/`replacement` are two separate, +// differently-shaped fields), AND they only produce a whitespace-only +// result by relying on IFS's own specific default value in the first +// place, so generalizing them to other variable names wouldn't even be +// meaningful the way it is for `:+`/`+`. Left unhandled rather than risk +// another incorrect generalization; a determined obfuscator using one of +// these remains a known, accepted gap in this heuristic guard. // // The bare `$IFS` form must not swallow the start of a longer variable // name — `$IFSOMETHING` references a completely different (and almost @@ -136,7 +150,7 @@ process.stdin.on('end', () => { const normalized = input .replace(/\$\{IFS\}/g, ' ') .replace(/\$\{IFS: *(?:0*[0-2]|-0*[1-3]|-0+)(?::0*[1-9]\d*)?\}/g, ' ') - .replace(/\$\{IFS:?\+[ \t]+\}/g, ' ') + .replace(/\$\{[A-Za-z_][A-Za-z0-9_]*:?\+[ \t]+\}/g, ' ') .replace(/\$IFS(?![A-Za-z0-9_])/g, ' '); process.stdout.write(normalized); }); diff --git a/tests/unit/hook-guard-git-ifs-bypass.test.ts b/tests/unit/hook-guard-git-ifs-bypass.test.ts index c99d0ed1f..8deb55363 100644 --- a/tests/unit/hook-guard-git-ifs-bypass.test.ts +++ b/tests/unit/hook-guard-git-ifs-bypass.test.ts @@ -207,6 +207,30 @@ describe('guard-git.sh IFS whitespace-expansion bypass (#2451)', () => { expect(isDenied('git${IFS+ }reset')).toBe(true); }); + it('still blocks git reset via a whitespace-only alternate-value expansion on a non-IFS, normally-set variable (#2558)', () => { + // ${HOME:+ } works identically to ${IFS:+ } — the operator substitutes + // the literal "word" whenever the named variable is set and non-null, + // regardless of what that variable's own value actually is. HOME is + // normally set in any real shell, so this produces the same token + // boundary as the IFS-specific form; a per-variable-name check could + // never fully close this class, since the variable name isn't fixed. + // biome-ignore lint/suspicious/noTemplateCurlyInString: literal bash syntax under test, not a missed template literal + expect(isDenied('git${HOME:+ }reset')).toBe(true); + }); + + it('still blocks git reset via the bare (colon-less) whitespace-only alternate-value form on a non-IFS variable', () => { + // biome-ignore lint/suspicious/noTemplateCurlyInString: literal bash syntax under test, not a missed template literal + expect(isDenied('git${PWD+ }reset')).toBe(true); + }); + + it('does not invent a token boundary from a non-whitespace alternate-value expansion on a non-IFS variable', () => { + // ${SOME_VAR:+x} substitutes the literal "x", not whitespace — the + // all-whitespace-content restriction applies regardless of which + // variable is named. + // biome-ignore lint/suspicious/noTemplateCurlyInString: literal bash syntax under test, not a missed template literal + expect(isDenied('echo git${SOME_VAR:+x}reset')).toBe(false); + }); + it('does not invent a token boundary from an empty IFS alternate-value expansion', () => { // ${IFS:+} (nothing between + and }) substitutes an EMPTY string when // IFS is set and non-null — an unquoted empty expansion contributes From c1f17cb5814558de13178759cd29c0fe2b6579f6 Mon Sep 17 00:00:00 2001 From: carlos-alm Date: Tue, 18 Aug 2026 23:41:20 -0600 Subject: [PATCH 2/2] fix: normalize-ifs.mjs :+/+ generalization covers bash special parameters too An ordinary bash identifier isn't the only thing that can sit in the :+/+ alternate-value position -- special parameters ($?, $$, $#, $-, $!) and positional parameters (${10:+ } etc.) use the same syntax and the same always/normally-set semantics, but none of them match an identifier shape ([A-Za-z_][A-Za-z0-9_]*). Verified directly against real bash: ${?:+ }/${$:+ }/${#:+ }/${-:+ } all substitute the whitespace word exactly like an ordinary variable would. The regex now matches an identifier, a bare digit sequence, or one of ?$!#@*- in that position. docs check acknowledged Impact: 1 functions changed, 0 affected --- .claude/hooks/normalize-ifs.mjs | 14 +++++++++- .../claude-code-hooks/normalize-ifs.mjs | 14 +++++++++- tests/unit/hook-guard-git-ifs-bypass.test.ts | 26 +++++++++++++++++++ 3 files changed, 52 insertions(+), 2 deletions(-) diff --git a/.claude/hooks/normalize-ifs.mjs b/.claude/hooks/normalize-ifs.mjs index db96f7f3c..7dcfa721c 100644 --- a/.claude/hooks/normalize-ifs.mjs +++ b/.claude/hooks/normalize-ifs.mjs @@ -111,6 +111,18 @@ // be set in a given shell, rather than trying to track which variables are // actually set. // +// Also matches bash's SPECIAL parameters in that position — `?` (exit +// status), `$` (PID), `#` (positional-parameter count), `-` (current shell +// option flags), `!` (last background PID), and a bare digit sequence +// (positional parameters, `${10:+ }` etc.) — verified directly against real +// bash (Greptile review): `${?:+ }`/`${$:+ }`/`${#:+ }`/`${-:+ }` all +// substitute the whitespace word exactly like an ordinary variable would, +// and `?`/`$`/`#`/`-` are always set in any shell (unlike `!`/digit +// parameters, which depend on whether a job has been backgrounded or +// positional arguments are present — matched anyway, erring toward +// normalizing). None of these characters overlap with the identifier +// alternative above, so a single alternation covers both without ambiguity. +// // Deliberately does NOT generalize the other three replacements below to // "any `${VAR...}`" — only `:+`/`+` generalizes across variable // names, because only its substituted text is entirely independent of the @@ -150,7 +162,7 @@ process.stdin.on('end', () => { const normalized = input .replace(/\$\{IFS\}/g, ' ') .replace(/\$\{IFS: *(?:0*[0-2]|-0*[1-3]|-0+)(?::0*[1-9]\d*)?\}/g, ' ') - .replace(/\$\{[A-Za-z_][A-Za-z0-9_]*:?\+[ \t]+\}/g, ' ') + .replace(/\$\{(?:[A-Za-z_][A-Za-z0-9_]*|[0-9]+|[?$!#@*-]):?\+[ \t]+\}/g, ' ') .replace(/\$IFS(?![A-Za-z0-9_])/g, ' '); process.stdout.write(normalized); }); diff --git a/docs/examples/claude-code-hooks/normalize-ifs.mjs b/docs/examples/claude-code-hooks/normalize-ifs.mjs index db96f7f3c..7dcfa721c 100644 --- a/docs/examples/claude-code-hooks/normalize-ifs.mjs +++ b/docs/examples/claude-code-hooks/normalize-ifs.mjs @@ -111,6 +111,18 @@ // be set in a given shell, rather than trying to track which variables are // actually set. // +// Also matches bash's SPECIAL parameters in that position — `?` (exit +// status), `$` (PID), `#` (positional-parameter count), `-` (current shell +// option flags), `!` (last background PID), and a bare digit sequence +// (positional parameters, `${10:+ }` etc.) — verified directly against real +// bash (Greptile review): `${?:+ }`/`${$:+ }`/`${#:+ }`/`${-:+ }` all +// substitute the whitespace word exactly like an ordinary variable would, +// and `?`/`$`/`#`/`-` are always set in any shell (unlike `!`/digit +// parameters, which depend on whether a job has been backgrounded or +// positional arguments are present — matched anyway, erring toward +// normalizing). None of these characters overlap with the identifier +// alternative above, so a single alternation covers both without ambiguity. +// // Deliberately does NOT generalize the other three replacements below to // "any `${VAR...}`" — only `:+`/`+` generalizes across variable // names, because only its substituted text is entirely independent of the @@ -150,7 +162,7 @@ process.stdin.on('end', () => { const normalized = input .replace(/\$\{IFS\}/g, ' ') .replace(/\$\{IFS: *(?:0*[0-2]|-0*[1-3]|-0+)(?::0*[1-9]\d*)?\}/g, ' ') - .replace(/\$\{[A-Za-z_][A-Za-z0-9_]*:?\+[ \t]+\}/g, ' ') + .replace(/\$\{(?:[A-Za-z_][A-Za-z0-9_]*|[0-9]+|[?$!#@*-]):?\+[ \t]+\}/g, ' ') .replace(/\$IFS(?![A-Za-z0-9_])/g, ' '); process.stdout.write(normalized); }); diff --git a/tests/unit/hook-guard-git-ifs-bypass.test.ts b/tests/unit/hook-guard-git-ifs-bypass.test.ts index 8deb55363..af68306b5 100644 --- a/tests/unit/hook-guard-git-ifs-bypass.test.ts +++ b/tests/unit/hook-guard-git-ifs-bypass.test.ts @@ -223,6 +223,32 @@ describe('guard-git.sh IFS whitespace-expansion bypass (#2451)', () => { expect(isDenied('git${PWD+ }reset')).toBe(true); }); + it('still blocks git reset via a whitespace-only alternate-value expansion on the $? special parameter (Greptile review)', () => { + // ${?:+ } is always-set (bash's exit-status special parameter is never + // unset), so this substitutes the literal space "word" exactly like an + // ordinary variable would -- verified directly against real bash. + // Neither special parameters nor bare digit sequences (positional + // parameters) match a bash identifier shape, so they need their own + // branch in the alternation, not just the ordinary-variable one above. + // biome-ignore lint/suspicious/noTemplateCurlyInString: literal bash syntax under test, not a missed template literal + expect(isDenied('git${?:+ }reset')).toBe(true); + }); + + it('still blocks git reset via a whitespace-only alternate-value expansion on the $$ (PID) special parameter', () => { + // biome-ignore lint/suspicious/noTemplateCurlyInString: literal bash syntax under test, not a missed template literal + expect(isDenied('git${$:+ }reset')).toBe(true); + }); + + it('still blocks git reset via a whitespace-only alternate-value expansion on the $# (arg count) special parameter', () => { + // biome-ignore lint/suspicious/noTemplateCurlyInString: literal bash syntax under test, not a missed template literal + expect(isDenied('git${#:+ }reset')).toBe(true); + }); + + it('still blocks git reset via a whitespace-only alternate-value expansion on the $- (shell options) special parameter', () => { + // biome-ignore lint/suspicious/noTemplateCurlyInString: literal bash syntax under test, not a missed template literal + expect(isDenied('git${-:+ }reset')).toBe(true); + }); + it('does not invent a token boundary from a non-whitespace alternate-value expansion on a non-IFS variable', () => { // ${SOME_VAR:+x} substitutes the literal "x", not whitespace — the // all-whitespace-content restriction applies regardless of which