fix(hooks): normalize-ifs.mjs generalizes :+/+ whitespace bypass beyond IFS - #2585
Merged
Conversation
…nd 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
Contributor
Greptile SummaryThe PR generalizes whitespace alternate-value normalization beyond
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains. Important Files Changed
Reviews (2): Last reviewed commit: "fix: normalize-ifs.mjs :+/+ generalizati..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Contributor
Codegraph Impact Analysis1 functions changed → 0 callers affected across 0 files
|
…ters 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
Contributor
Author
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
normalize-ifs.mjscorrectly recognizes\${IFS:+ }/\${IFS+ }as producing a literal space, but this operator's behavior has nothing to do with the variable being namedIFSspecifically — bash's\${VAR:+word}substituteswordwheneverVARis set and non-null, regardless ofVAR's own value.\${HOME:+ },\${PWD:+ },\${PATH:+ }, or any other normally-set variable works identically.\${IFS},\$IFS, and the substring-expansion form) stayIFS-specific, since they extract from IFS's own known default value (space/tab/newline) — that reasoning doesn't generalize to other variables, whose actual values aren't known statically. Only the:+/+alternate-value replacement — whose substituted text is entirely independent of the named variable's value — now matches any bash identifier shape ([A-Za-z_][A-Za-z0-9_]*) in that position.docs/examples/claude-code-hooks/normalize-ifs.mjsbyte-identical to.claude/hooks/normalize-ifs.mjs, as enforced bytests/unit/hook-guard-git-clean.test.ts.Closes #2558
Test plan
tests/unit/hook-guard-git-ifs-bypass.test.ts: blocksgit\${HOME:+ }resetandgit\${PWD+ }reset(generalized bypass via non-IFS variables), does not flag\${SOME_VAR:+x}(non-whitespace substituted text, any name).expected false to be true); restored the fix and all pass again.hook-guard-git-ifs-bypass,hook-guard-git-clean,hook-guard-git-branch-validation,hook-guard-git-commit-cwd-fallback) — 82/82 pass, confirming the byte-identical-copies check still holds.npm run lintclean.npx vitest run: 5528/5528 tests pass (344 test files, +3 new).