fix(desktop): always allow unsafe custom binary for simple-git - #71944
Open
martin-lin-asg wants to merge 1 commit into
Open
fix(desktop): always allow unsafe custom binary for simple-git#71944martin-lin-asg wants to merge 1 commit into
martin-lin-asg wants to merge 1 commit into
Conversation
simple-git's custom-binary validation rejects paths containing spaces (the default Windows install is C:\Program Files\Git\cmd\git.exe) and also rejects non-standard paths from MSYS/WSL shims. Previously, allowUnsafeCustomBinary was enabled conditionally only when the git binary path contained whitespace. This missed cases where the path contained other characters that fail the regex check. Since gitBin is resolved by the Electron main process from known install locations and PATH — never from user input — the binary path is trusted and the escape hatch can be safely enabled unconditionally.
alt-glitch
added
type/bug
Collaborator
teknium1
reviewed
Jul 30, 2026
teknium1
left a comment
teknium1
left a comment
Contributor
There was a problem hiding this comment.
Thanks for extending the Windows Git-path handling. The broader trusted-resolver approach may be appropriate, but the stated reproduction does not demonstrate a remaining failure on current main.
Problems
apps/desktop/electron/git-review-ops.ts:58already enablesallowUnsafeCustomBinaryfor any whitespace-containinggitBin. The cited/c/Program Files/Git/cmd/git.exeexample therefore takes the existing safe branch.apps/desktop/dist/electron-main.mjs:8463shows simple-git rejects characters beyond whitespace, butapps/desktop/electron/git-review-ops.test.ts:37covers only a spaced path. The PR does not identify or test a non-whitespace resolver output that reaches the rejection.
Suggested changes
- Add a regression case for a concrete non-whitespace character that
resolveGitBinary()can return (apps/desktop/electron/main.ts:2216-2231) and that simple-git rejects, so the broader opt-in is tied to a verified failure.
Automated hermes-sweeper review.
| trimmed: false, | ||
| ...(gitBin && /\s/.test(gitBin) ? { unsafe: { allowUnsafeCustomBinary: true } } : {}) | ||
| unsafe: { allowUnsafeCustomBinary: true } | ||
| }) |
Contributor
There was a problem hiding this comment.
The MSYS example in the PR body contains a space, so current main's gitBin && /\s/.test(gitBin) condition already enables this option for it. Please add a regression case for a concrete non-whitespace rejected character produced by resolveGitBinary() to demonstrate the remaining gap.
teknium1
added
the
sweeper:blast-moderate
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
3 participants
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.
Problem
On Windows,
simple-git's custom-binary validation rejects git paths containing spaces (the default install isC:\Program Files\Git\cmd\git.exe) and also rejects non-standard paths from MSYS/WSL shims.The previous fix enabled
allowUnsafeCustomBinaryonly when the git binary path contained whitespace — but the regex check also rejects other characters, so the error still fires for MSYS-style paths like/c/Program Files/Git/cmd/git.exe.Fix
Unconditionally enable
unsafe.allowUnsafeCustomBinary: trueingitFor(). This is safe becausegitBinis resolved by the Electron main process from known install locations and PATH — never from renderer/user input.Files Changed
apps/desktop/electron/git-review-ops.ts—gitFor()now always passes the unsafe escape hatch tosimpleGit()Error eliminated