[sec-check] fix: refuse -O when it is clustered with other short options - #330
Merged
Merged
Conversation
gate_git_diff.py matched a refused short option against the start of the token, so `git diff -O order1` was refused while `git diff -aOorder1` and `git diff -aO order1` passed. git bundles single-letter options into one word and reads the value of the first value-taking letter from whatever follows it, so both spellings hand git the same order file, and git 2.55 honors it: the hook exists to keep that third path closed, and this left it open (#329). Walk the letters of a short-option token instead. Every letter up to and including the first value-taking one is an option, and the rest of the word is that option's value, so `-aOorder1` is caught and `-SOAuth` -- a pickaxe search for the text `OAuth` -- is left alone. VALUED_SHORT lists the diff and log options that take a value; a letter missing from it can only over-refuse, never let an `O` through, so the set errs in the safe direction and a test checks each entry against git's own parsing. The test module gains the clustered forms as refused commands, `-S`, `-G` and `-L` values containing an `O` as allowed ones, and a real-git check that the clustered order file is read, in the same style as the existing --no-index check. Closes #329 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Danathar <Danathar@users.noreply.github.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 20708d6561
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Contributor
Verified independently: cloned
|
…tdout The per-letter check ran `git diff --cached -<letter>Oorder1 --name-only` and asserted the output was not reordered. A boolean letter that clashes with `--name-only`, such as `-s`, makes git exit 128 with nothing on stdout, which the assertion accepted -- so `s` wrongly listed in VALUED_SHORT would have kept the test green while the hook let `-sO<file>` through (Codex review on #330). Probe `-<letter>Omissing` with no output format and no such file instead. A boolean letter leaves `-Omissing` to be read next and git fails to open the order file, whatever else the command does; a value-taking letter swallows `Omissing` and never looks for it. `-a` is probed first as a positive control so a reworded message fails the test rather than letting every entry pass for the wrong reason. Verified that listing `s` now fails the test. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Danathar <Danathar@users.noreply.github.com>
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 join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
What changed
.claude/hooks/gate_git_diff.pynow walks the letters of a short-option token instead of matching only its start, so-Ois refused wherever it sits in a cluster (-aOorder1,-aO order1,-pO order1), not just when it leads the word. A newVALUED_SHORTset names thegit diff/git logshort options that take a value; the walk stops at the first of those, so a value that merely contains anO(-SOAuth,-GOpen,-L:Open:file) is still allowed.Why
Closes #329.
git bundles single-letter options into one word and reads the value of the first value-taking letter from the rest of that word, so
git diff -aOorder1isgit diff -a -O order1. The hook refused the second spelling and passed the first, and git 2.55 honors the clustered order file, so the third path the hook exists to close was still open.The fix errs in the safe direction: a letter missing from
VALUED_SHORTis walked past as a boolean, which can only over-refuse. The only way to open a gap is to list a boolean letter there, and a test checks every entry against git's own parsing of-<letter>Oorder1to catch that.How it was checked
python3 -m unittest discover -s tests-- 1553 tests, OK (1 skip: thejusttest, not installed locally).claude/and is outside the measured source treeruff check-- cleanBy hand, before the fix: the two commands in the issue reproduced exactly (
-aOorder1exit 0,-O order1exit 2), andgit diff --cached -aOorder1 --name-onlyin a scratch repo returned the files in the order file's order.tests/test_git_diff_gate.pynow carries that real-git check alongside the existing--no-indexone. I also temporarily addeda(a boolean) toVALUED_SHORTto confirm the new per-letter test fails on a wrong entry; it does.Anything a reviewer should look at first
VALUED_SHORT = frozenset("BCGILMOSUXln"). Each entry is a diff or log short option that takes a value in git's own parser (checked by test). Leaving one out is harmless over-refusal; the set is deliberately not padded with letters I could not confirm.🤖 Generated with Claude Code