Repository navigation
Denylist scan: read binary and oddly encoded content as git sees it - #1873
Conversation
The scan could be bypassed, or crashed, by content that is not plain UTF-8 text: - The diff was split with str.splitlines(), which also breaks at \r, \f, \v, \x1c-\x1e, U+0085, U+2028 and U+2029. Text after one of those lost its "+" and went unchecked, a "diff --git" after one reset the parser, and "@@" after one was refused as a hunk header (the crash a video triggered). The diff, commit messages, extra text files and the audit now split on "\n" only, as git does. - Binary files were left out of the diff with exclude pathspecs and found again with ls-tree by path. A directory replacing a binary file of the same name hid every file under it, a name starting with ":" was read as pathspec magic and skipped, and each path went on the command line twice, so a change with many media files could not be scanned. Paths, status and new blob ids now come from one "git diff --raw --numstat -z", blobs are read by id through a single "git cat-file --batch", only deleted files are skipped, and a blob that cannot be read fails the check. - Binary-ness came from the working tree's attributes, so a change could add a .gitattributes rule moving its own text files out of the line diff. Both diffs now run with --attr-source set to the base commit (git 2.40+), and the number of "Binary files" lines must match the numstat count. - A file git calls binary was only searched for ASCII runs of six or more printable characters, which dropped tab- or NBSP-separated and accented terms, UTF-16 text and short metadata values. Such a file is now checked line by line when it is text with NUL bytes, UTF-16 or UTF-32, and otherwise its UTF-8 (with multibyte characters), UTF-16 and UTF-32 runs of five or more characters are checked. In those runs only terms of five or more characters count: in 3 GB of random bytes the committed list's four-character terms matched by chance about once per 100 MB, and longer terms never did. - The audit skipped every file holding a NUL byte; it now reads each file the same way. - Commit messages are read from the commit objects, so a \x1e in a message no longer splits the log record and hides the rest; a line that is not UTF-8 is also read without NULs and as Windows-1252. Findings are still reported by location without the matched words, with the same exit codes, and house-term warnings are unchanged.
--numstat counts a binary file whose mode alone changed, but the patch prints no "Binary files ... differ" line for it, so the consistency check between the two failed such a change with exit 2. Only binary files whose blob changed are now counted against the patch, and a mode change reads nothing, since the content is what the base already had. The test for a name starting with ":" now uses that name alone: a second name with exclude magic made the old ls-tree call fail outright, which hid whether the bypass itself was caught.
A review of the binary-scan rework found these gaps: - An empty file that the base .gitattributes marks binary (this repository's own "*.pdf binary"), added or deleted, failed the scan with exit 2: --numstat counts it as binary, but the patch has no "Binary files" line for it. The patch is now read with --full-index, and the new object of each "Binary files" line must be a blob that is read whole, or a deletion. That is the direction that matters; an empty file or a new mode needs no such line. - UTF-16 and UTF-32 text was read by dropping its NULs whenever that gave UTF-8, so it was split at every 0x0A byte, including the one inside U+200A (0A 20), and a term with a hair space between its words passed. Such text is now decoded first and split on "\n" characters. A UTF-16 file with a byte-order mark and no NUL byte (Latin Extended letters and a thin space) is diffed by git as text, so every changed text blob is now read as well and decoded if it is UTF-16 or UTF-32, in the scan and in the audit. - A text file with a NUL and one invalid byte fell to the readable-run scan, where a NUL or control byte between words, and every term under five characters, went unmatched. Text is now judged by its share of odd bytes (control bytes and bytes that are not UTF-8: at most 2%, or 8), not by strict UTF-8, and a line that is not UTF-8 gets its Windows-1252 reading also when it holds NULs. In binary data a single NUL or control byte between two words is read as a space, and a term under five characters counts as a word of prose: whole, next to whitespace and beside another word. 1 GB of random bytes and the 29.5 MB of binary blobs in the history gave no chance match with the committed list; the expected rate for its four-character terms is about one in 750 GB. - Commit messages were read from the commit objects only, so a message stored in Shift_JIS or ISO-2022-JP under an encoding header passed. They are now also read through "git log -z --encoding=UTF-8", whose records must match rev-list, and decoded with the declared codec in Python, for an encoding the git build cannot convert. - Tests now pin the fail-closed paths: a missing object or one of another kind, a binary file the patch leaves out that is not read whole, and a commit that git log does not show.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 976160fe5f
ℹ️ 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".
| are not a secret: with the committed salt anyone can test a guessed name. Failures give the location, not the matched words. The check fails | ||
| closed when git or the hash file misbehaves. | ||
|
|
||
| Every file a change adds or changes is also read whole. UTF-16 or UTF-32 text |
There was a problem hiding this comment.
Remove tool attribution from the commit identity
Commit def543046dfbbbc088fe6ebf111bb9668a2cec73 records both its author and committer as Codex <codex@openai.com>. The repository explicitly prohibits tool attribution in author and committer fields, so this commit violates the repository's publication rules regardless of the scanner's correctness. Use an approved non-tool commit identity before merging.
AGENTS.md reference: AGENTS.md:L15-L21
Useful? React with 👍 / 👎.
…r arrays posing as UTF-32 A term under five characters inside a binary file's readable text counted only next to whitespace, so a value in XMP, a PDF dictionary or JSON metadata was missed even with other words around it. It now also counts when punctuation alone bounds it and the run holds two other words of three or more letters. 1 GB of random bytes gave no chance match of the committed list, and the rule admits one more four-letter word per GB than before (about one chance match in 700 GB). Arrays of 16- or 32-bit numbers below 65,536 have the NUL bytes of UTF-16 or UTF-32 text, and were decoded and checked as text, where accented letters among them could spell a term by chance. A guess without a byte-order mark now counts only when at least half the decoded characters are ASCII text.
…; keep CONTRIBUTING's line ends The previous change accepted a UTF-16 or UTF-32 guess without a byte-order mark only when half its characters were ASCII, which dropped Russian, Greek or Japanese text: a short term in it was no longer found. A guess now counts when it reads as text in some script: half its characters ASCII text, or one in a hundred whitespace, or all but one in twenty of the others from at most four scripts. Arrays of 16- or 32-bit numbers still fail all three: in random arrays from ranges between 0 and 65,535, under 0.2% of the characters were whitespace and the four commonest scripts held at most 90% of the others. The number-array test now plants the term's letters side by side, so the scanner before the change finds them, and new tests cover Russian, Japanese and Chinese text without a byte-order mark. CONTRIBUTING.md is back to its CRLF line ends, so its diff is only the edited paragraph.
…ators and folding letters The script count of the previous change split Japanese and Chinese punctuation and the long-vowel mark into scripts of their own, so katakana-heavy text and text with brackets on long lines were still dropped. Its whitespace and script rules also let in arrays of numbers in one narrow range, and arrays of token ids whose line ends decode to a lone CR, where accented letters folded to ASCII gave chance matches. The measurements the previous commit gave for arrays held only for wide ranges. A guess without a byte-order mark now counts when half its characters are ASCII text, or else when one in a hundred is a separator (a space, a line end, or the punctuation of CJK, Hindi and Arabic text; a lone CR is not one) and under one in ten of its other characters fold to an ASCII letter or digit. Prose in eight scripts had 5% to 23% separators and no folding letters; of uniform arrays from 16 ranges only values inside the CJK punctuation block passed, and those fold to no letter; arrays of BPE token ids have 18% to 24% folding letters.
…hes of ASCII text Each earlier rule for deciding whether a UTF-16 or UTF-32 guess without a byte-order mark was text either dropped real text (Japanese with fullwidth digits, Chinese with half-width punctuation, text after a long English preamble) or let number arrays in. The chance matches in those arrays come from letters scattered among unrelated characters being joined into a term. A guess is now always decoded. When it is at least half ASCII text it is checked as text, as before. Otherwise each line is checked in stretches of ASCII text, cut at every other character but a space, punctuation or a combining mark: a term counts only when written in ASCII letters, as a name in Russian, Japanese or Chinese text is. With 2,000 invented four-letter terms, arrays of 65,536 numbers (uniform, in one range of accented or fullwidth letters, or token ids) gave up to 53 chance matches read as text and none read by stretches. In such text, a name written in accented or fullwidth letters is not found. The array tests now draw their values from one generator each, and show that the plain reading matches invented terms where the scan does not.
…cters and dash symbols A name in wide text without a byte-order mark was missed when a soft hyphen, a zero-width space, or a dash that a Japanese encoding decodes to a symbol (U+2212, U+FF5E, U+FF0B, or the long-vowel mark typed as a dash) sat between its words. Those characters no longer end a stretch of ASCII text. With the same 2,000 invented terms, the number arrays still give no chance match.
…t each separator between a term's words The halfwidth katakana long-vowel mark (U+FF70) now joins a stretch of ASCII text, like its fullwidth twin. A test puts each invisible character and dash symbol between the two words of a term, so removing any one of them fails it. The comment now says where each symbol comes from: the fullwidth tilde is what cp932 decodes the wave dash to, and U+FF0B is a plus sign.
Why
The vendor denylist scan in #1870 learned to skip binary files in the line diff and check their readable text instead. An independent review then found ways a denylisted term could still get through:
splitlines(), which also breaks at\r,\v,\f,\x1c–\x1e, U+0085, U+2028 and U+2029. This was the real cause of the "unparseable diff hunk header" crash on the video.:;.gitattributesrule or a NUL byte.A second review of the first fix found seven more gaps: empty binary-marked files, UTF-16 split inside characters, text with one stray byte, short terms in metadata, commit-message encodings, untested fail-closed paths, and UTF-16 text with no NUL bytes.
A third review found two more:
A fourth review found that the fix for number arrays also dropped text in other scripts without a byte-order mark (Russian, Greek, Japanese), and that CONTRIBUTING.md had lost its CRLF line ends. A fifth review found two problems with that round's script rule:
A sixth review found that the next rule (separators and folding letters) dropped three kinds of text:
So the accept-or-reject test is gone: such a guess is now always decoded and read safely, as below.
What changes
scripts/check_denylist.py:\nonly, as git does. This applies to the diff, commit messages,--text-fileinputs and the audit. Inside a hunk,diff --gitand@@can no longer pose as headers. The patch is parsed as bytes.git diff --raw --numstat -z --no-renames --no-abbrevgives each path's status, blob ids and binary flag. It runs with--attr-source=<base>,--no-ext-diff,--no-textconvand--no-relative.git cat-file --batch. No path is ever parsed as a pathspec, and no command line grows with the number of files.git log -z --encoding=UTF-8, as stored, and decoded with the codec named in the commit's encoding header. The records must matchrev-list.CONTRIBUTING.mdsays how binary files are read, what isn't matched inside them, and that the scan needs git 2.40 or later.Checks
tests/unit/test_check_denylist.py: 181 passed.CONTRIBUTING.mdkeeps its CRLF line ends: against main its diff is the 14 added lines.@@crash.check_denylist.py scan --base origin/main --head HEAD: exit 0.audit: exit 0, with house-term warnings only, the same as before.ruff check: clean.Known limits, documented in the code:
"Xyzw"or(Xyzw).