Harden the git argument handling flagged by a security review - #1
Open
HugoHeneault wants to merge 2 commits into
Open
HugoHeneault wants to merge 2 commits into
HugoHeneault wants to merge 2 commits into
Conversation
- Push flags take a whole flag word: a substring match turned "none" into --no-verify, skipping the pre-push hooks with nothing said about it. - Paths are read NUL terminated, the only git output that leaves a quote, a backslash or a control character unquoted. Such a file used to reach git as its quoted form, so it showed up as deleted and could not be staged. - The mtime fallback walk stops at the repo instead of climbing above it. - The directory walk is depth capped, so a deep tree cannot exhaust the stack.
The three comments marking a deliberate ceiling carried a prefix naming a local editing mode, which means nothing to a reader of this repository. The ceilings and the way to lift them stay, the prefix goes.
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.
Four findings from a review of
src/, ordered by what they cost.Push flags matched on a substring.
answer.contains('n')meant answeringnonetopush flags?pushed with--no-verify, skipping every pre-push hook, including any secretscanning, without a word in the output.
lolenabled--force-with-leasethe same way.parse_push_optionsnow takes the whole answer and rejects anything that is notl,n,ln,nlor empty.Paths came back quoted.
core.quotepath=falseonly stops the quoting of bytes above 0x80: apath holding
",\or a control character is still C-quoted by git.we"ird.txtreached us as"we\"ird.txt", a path that exists nowhere, so the file was reported as deleted andgit add --could never stage it.
diff --numstatandls-filesnow run with-zand are split on NUL.The mtime fallback climbed out of the repo. For a deleted file whose parent directories are
gone too,
path.ancestors()walked past the repo root, dating the repo from an unrelateddirectory. The walk now stops at the repo.
The directory walk was unbounded. Depth capped at
MAX_SCAN_DEPTH.Two tests added, 16 pass. Checked end to end against a throwaway repo holding
we"ird.txtandnew"file.txt: three files, counts exact, no longer reported as deleted.