fix: pass gitleaks --log-opts without a shell so Windows keeps the caret - #1676
Open
ppcvote wants to merge 1 commit into
Open
fix: pass gitleaks --log-opts without a shell so Windows keeps the caret#1676ppcvote wants to merge 1 commit into
ppcvote wants to merge 1 commit into
Conversation
runCommand spawned with shell:true, and the range argument was written with POSIX single quotes. With shell:true Node joins command and args into one command line for the platform shell, and cmd.exe neither treats ' as a quote nor leaves ^ alone. Measured through the same code path on Windows: intended, one entry: ["--log-opts='--first-parent abc123^..def456'"] shell:true actual: ["--log-opts='--first-parent", "abc123..def456'"] no shell: ["--log-opts='--first-parent abc123^..def456'"] The argument is split at the space, and the caret is deleted. The caret is the part that matters. <sha>^..<sha> starts at the parent of commitFrom; <sha>..<sha> starts at commitFrom. Losing it narrows the range by one commit, and that commit is the first of the push. gitleaks then exits 0 having skipped a commit that was in scope, so on Windows the scan silently covered less than it reported. Dropping shell:true makes the argv identical on win32 and linux, at which point the embedded quotes are unnecessary and are removed with it. runCommand is only called with git and gitleaks, both native executables; spawn without a shell resolves both through PATH on Windows, verified locally. The test reads spawn.mock.calls[1] and asserts the exact argv entry, that no argument contains a quote, and that shell is not true. It fails on the unmodified tree and passes with the change. resolves finos#1675
✅ Deploy Preview for endearing-brigadeiros-63f9d0 canceled.
|
|
|
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.
Description
runCommandspawned withshell: true, and the range argument was written with POSIX single quotes. Withshell: trueNode joins the command and its arguments into a single command line and hands it to the platform shell.cmd.exedoes not treat'as a quote character, and does treat^as an escape character.Measured on Windows through the same code path, with
commitFrom = abc123,commitTo = def456:["--log-opts='--first-parent abc123^..def456'"]shell: true["--log-opts='--first-parent", "abc123..def456'"]["--log-opts='--first-parent abc123^..def456'"]Two things go wrong at once: the argument splits at the space, and the
^is deleted.The caret is the part that matters.
<sha>^..<sha>starts at the parent ofcommitFrom;<sha>..<sha>starts atcommitFromitself. Losing it narrows the range by one commit, and the commit that falls out is the first commit of the push being scanned. gitleaks then runs, exits 0, and reports no leaks, having skipped a commit that was in scope. On Windows the scan quietly covered less than it claimed.Related Issue
Resolves #1675
Changes
Two lines in
src/proxy/processors/push-action/gitleaks.ts:spawn(command, args, { cwd, shell: true })becomesspawn(command, args, { cwd })'...'around the--log-optsvalue is removed, since with the argument passed as one array element there is nothing for a shell to re-parserunCommandis only ever called withgitandgitleaks, both native executables rather than.cmdshims, so nothing here depended on shell resolution. I verified on Windows thatspawn('git', ['--version'], {})with no shell resolves through PATH and exits 0, and thegit rev-listcall at line 152 passes only simple tokens, so it is unaffected either way.Test
One test added to
test/processors/gitLeaks.test.ts, which already mocksnode:child_process, so no gitleaks binary is needed. It readsspawn.mock.calls[1](call 0 beinggit rev-list) and asserts three things: the exact argv entry--log-opts=--first-parent abc123^..def456, that no argument contains a', and thatshellis nottrue.Confirmed it discriminates by stashing the source change and re-running: 1 failed / 10 passed on the unmodified tree, 11 passed with the fix.
Full suite is unchanged: 48 failures before this branch and the same 48 after, with one extra passing test. Those failures are pre-existing on Windows and unrelated.
prettier --checkandeslintare clean on both touched files; the twotscerrors onmainare insrc/ui/views/PushDetails/components/AttestationInfo.tsxand predate this change.Note
An asymmetry worth naming: the repository does run a Windows CI job, but nothing currently asserts on what
spawnreceives, so a platform-dependent quoting bug can pass every job. The test added here closes that specific gap; whether it is worth generalising is your call.