Skip to content

Commit 6e413f8

Browse files
hongyeclaude
andcommitted
fix(utils): address CodeRabbit review feedback on git-bash discovery
Three improvements suggested by CodeRabbit's PR review: 1. Use path.win32 for processing `where.exe` results (MAJOR). On POSIX hosts, `path.resolve('C:\\foo')` treats backslashes as literal characters and produces a wrong (relative) result, which would let cwd shadowing slip past the security check. Switch to `path.win32.*` so the cwd filtering evaluates `where.exe`'s Windows-style paths with Windows semantics regardless of host OS. Also switched the cwd check from `startsWith` to `path.win32.relative`, which is more robust: returns `''` for cwd itself, `..`/`../...` for outside, and a non-absolute path for inside. Avoids edge cases with trailing separators and mixed case. 2. Inject `USERPROFILE` through `GitBashDiscoveryDeps` (NITPICK). The Scoop fallback in `searchDefaultBashLocations` was reading `process.env.USERPROFILE` directly, breaking hermetic testing of that branch. Threaded through as an optional deps field, defaults to `process.env.USERPROFILE` for production. 3. Add `envOverride: ''` to the manual `GitBashDiscoveryDeps` fixture in the "skips where.exe git when bash is already found via PATH" test (INLINE). Without this, the test would pass for the wrong reason (env override) on developer machines that have `CLAUDE_CODE_GIT_BASH_PATH` set, instead of exercising the PATH short-circuit behavior the test is named after. Co-Authored-By: Claude <noreply@anthropic.com>
1 parent 7766597 commit 6e413f8

2 files changed

Lines changed: 39 additions & 8 deletions

File tree

src/utils/__tests__/windowsPaths.test.ts

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -287,6 +287,10 @@ describe('findGitBashPathOrNullWithDeps', () => {
287287
return ''
288288
},
289289
cwdFn: () => '/safe/cwd',
290+
// Bypass process.env.CLAUDE_CODE_GIT_BASH_PATH so the test exercises
291+
// the intended PATH short-circuit behavior rather than passing for
292+
// the wrong reason (env override).
293+
envOverride: '',
290294
}
291295
expect(findGitBashPathOrNullWithDeps(deps)).toBe(bashPath)
292296
expect(gitProbed).toBe(false)

src/utils/windowsPaths.ts

Lines changed: 35 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -35,6 +35,13 @@ export type GitBashDiscoveryDeps = {
3535
execCommand: (cmd: string) => string
3636
/** Returns the current working directory (used to filter PATH-based lookups). */
3737
cwdFn: () => string
38+
/**
39+
* `USERPROFILE` used to derive Scoop Git install paths. When provided,
40+
* this is used instead of `process.env.USERPROFILE` — keeps the pure
41+
* helper hermetic so the Scoop fallback can be tested without
42+
* depending on the live environment.
43+
*/
44+
userProfile?: string | undefined
3845
/**
3946
* Optional override for `process.env.CLAUDE_CODE_GIT_BASH_PATH`. When
4047
* provided, this is used instead of the live environment — useful for tests.
@@ -47,6 +54,7 @@ const DEFAULT_DEPS: GitBashDiscoveryDeps = {
4754
execCommand: cmd =>
4855
execSync_DEPRECATED(cmd, { stdio: 'pipe', encoding: 'utf8' }).trim(),
4956
cwdFn: getCwd,
57+
userProfile: process.env.USERPROFILE,
5058
envOverride: undefined,
5159
}
5260

@@ -57,6 +65,7 @@ const DEFAULT_DEPS: GitBashDiscoveryDeps = {
5765
*/
5866
function searchDefaultBashLocations(
5967
checkExists: (p: string) => boolean,
68+
userProfile?: string,
6069
): string | null {
6170
const candidates = [
6271
// Standard Git for Windows install locations (both layouts).
@@ -66,7 +75,6 @@ function searchDefaultBashLocations(
6675
'C:\\Program Files (x86)\\Git\\usr\\bin\\bash.exe',
6776
]
6877
// Scoop install: %USERPROFILE%\scoop\apps\git\current\usr\bin\bash.exe
69-
const userProfile = process.env.USERPROFILE
7078
if (userProfile) {
7179
candidates.push(
7280
`${userProfile}\\scoop\\apps\\git\\current\\usr\\bin\\bash.exe`,
@@ -114,16 +122,35 @@ function findExecutableWithDeps(
114122
const result = deps.execCommand(`where.exe ${executable}`)
115123
// SECURITY: Filter out any results from the current directory
116124
// to prevent executing malicious git.bat/cmd/exe files
117-
const paths = result.split('\r\n').filter(Boolean)
118-
const cwd = deps.cwdFn().toLowerCase()
125+
//
126+
// Use path.win32.* here so that `where.exe`'s Windows-style backslash
127+
// paths are evaluated with Windows semantics regardless of the host
128+
// OS. On POSIX, `path.resolve('C:\\foo')` treats the backslashes as
129+
// literal characters and produces a wrong (relative) result, which
130+
// would let cwd shadowing slip past this check. pathWin32 is
131+
// already imported for the bash derivation below.
132+
const paths = result
133+
.split(/\r?\n/)
134+
.map(p => p.trim())
135+
.filter(Boolean)
136+
const cwd = pathWin32.resolve(deps.cwdFn()).toLowerCase()
119137

120138
for (const candidatePath of paths) {
121139
// Normalize and compare paths to ensure we're not in current directory
122-
const normalizedPath = path.resolve(candidatePath).toLowerCase()
123-
const pathDir = path.dirname(normalizedPath).toLowerCase()
140+
const normalizedPath = pathWin32.resolve(candidatePath).toLowerCase()
141+
const pathDir = pathWin32.dirname(normalizedPath).toLowerCase()
142+
// path.win32.relative(cwd, pathDir) returns:
143+
// '' → pathDir === cwd
144+
// '..' / '../...' → pathDir is outside cwd (or above it)
145+
// 'subdir/...' → pathDir is inside cwd
146+
// We reject entries whose dir is cwd itself or anywhere inside cwd.
147+
const relativePathDir = pathWin32.relative(cwd, pathDir)
124148

125-
// Skip if the executable is in the current working directory
126-
if (pathDir === cwd || normalizedPath.startsWith(cwd + path.sep)) {
149+
if (
150+
relativePathDir === '' ||
151+
(!relativePathDir.startsWith('..') &&
152+
!pathWin32.isAbsolute(relativePathDir))
153+
) {
127154
logForDebugging(
128155
`Skipping potentially malicious executable in current directory: ${candidatePath}`,
129156
)
@@ -188,7 +215,7 @@ export function findGitBashPathOrNullWithDeps(
188215
}
189216

190217
// 4. Last resort: scan common install locations for bash.exe directly
191-
return searchDefaultBashLocations(deps.checkExists)
218+
return searchDefaultBashLocations(deps.checkExists, deps.userProfile)
192219
}
193220

194221
/**

0 commit comments

Comments
 (0)