fix(permissions): normalize MinGW paths in validatePath before resolu…#1287
Open
Ericcc-Ma wants to merge 1 commit into
Open
fix(permissions): normalize MinGW paths in validatePath before resolu…#1287Ericcc-Ma wants to merge 1 commit into
Ericcc-Ma wants to merge 1 commit into
Conversation
…tion
`validatePath` previously used platform-specific `path.isAbsolute` and
`path.resolve`. On Windows, this created a sandbox-escape class:
- A model emitting `/c/Users/foo/sensitive.txt` (MinGW form, the way
Git Bash writes paths) reaches `validatePath`.
- `path.isAbsolute('/c/Users/foo/sensitive.txt')` returns true on
Windows (treated as a drive-relative absolute path).
- `path.resolve('D:\\project', '/c/Users/foo/sensitive.txt')` returns
`D:\\c\\Users\\foo\\sensitive.txt` (resolved against the current drive).
- The validator compares `D:\\c\\Users\\foo\\sensitive.txt` against the
allowed-directories list. If `D:\\` is broadly allowed (e.g. the
user's working directory is on D:), validation passes.
- Git Bash actually writes to `C:\\Users\\foo\\sensitive.txt` — a
completely different filesystem location — bypassing the sandbox.
The fix: on Windows, normalize MinGW-style absolute paths
(`/c/Users/foo`, `/cygdrive/c/Users/foo`) to Windows paths
(`C:\\Users\\foo`) via `posixPathToWindowsPath` BEFORE the
isAbsolute/resolve step. The validator's path space now matches the
shell's, so the comparison is against the actual target the shell will
write to.
`posixPathToWindowsPath` is a no-op for already-Windows paths and
relative paths (it just flips slashes), so applying it
unconditionally on Windows is safe.
Test coverage added in
`src/utils/permissions/__tests__/pathValidation.test.ts`:
- `/c/...` → `C:\\...` conversion (all 7 cases incl. drive-letter case,
bare mount, nested paths)
- `/cygdrive/c/...` → `C:\\...` conversion
- Already-Windows paths pass through unchanged
- Relative paths pass through (just slash direction flipped)
- SECURITY regression test: MinGW path that escapes allowed dirs is
now correctly denied at the right location
- SECURITY regression test: cygdrive variant of the above
Verified on Windows 11 + Bun 1.3.14:
- `bun run precheck`: 5906/5906 pass (was 5896/5896, +10 new tests)
- `bun run build`: succeeds
Co-Authored-By: Claude <noreply@anthropic.com>
Contributor
📝 WalkthroughWalkthroughWindows path validation now normalizes MinGW-style absolute paths on Windows before resolution. The tests cover canonical drive-letter conversion, relative path handling, bare drive mounts, and a regression where escaped MinGW inputs must resolve to the intended Windows location. ChangesWindows MinGW Path Validation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/utils/permissions/__tests__/pathValidation.test.ts (1)
5-24: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMove shared mock registration into
beforeAll.The mocks use the shared mock helpers, but they are registered at module scope. Please keep the dynamic imports after mock registration inside
beforeAllso this follows the repo’s test isolation pattern, then rerunbun run precheck. As per coding guidelines,**/__tests__/**/*.test.{ts,tsx}should “Useuse-shared-mockpattern: import mock from tests/mocks/ ... and pass tomock.module()inbeforeAllblock”.♻️ Suggested structure
-import { describe, expect, mock, test } from 'bun:test' +import { beforeAll, describe, expect, mock, test } from 'bun:test' import { logMock } from '../../../../tests/mocks/log' import { debugMock } from '../../../../tests/mocks/debug' -// Cut the bootstrap/state dependency chain (mock.module requirement). -mock.module('src/utils/log.ts', logMock) -mock.module('src/utils/debug.ts', debugMock) -mock.module('bun:bundle', () => ({ - feature: (_name: string) => false, -})) +type ValidatePath = (typeof import('../pathValidation.js'))['validatePath'] +type GetEmptyToolPermissionContext = + (typeof import('../../../Tool.js'))['getEmptyToolPermissionContext'] + +let validatePath: ValidatePath +let getEmptyToolPermissionContext: GetEmptyToolPermissionContext + +beforeAll(async () => { + // Cut the bootstrap/state dependency chain (mock.module requirement). + mock.module('src/utils/log.ts', logMock) + mock.module('src/utils/debug.ts', debugMock) + mock.module('bun:bundle', () => ({ + feature: (_name: string) => false, + })) -// MACRO is a build-time define injected by `bun --define` (see -// scripts/dev.ts → -d flags). Without it, `declare const MACRO` references -// in source code resolve to `undefined` at runtime and crash any function -// that touches `MACRO.VERSION` (e.g. `getBundledSkillsRoot` via -// `checkReadableInternalPath`). -// Setting it on globalThis lets the bare `MACRO` identifier resolve at -// runtime in tests. -;(globalThis as unknown as { MACRO: { VERSION: string } }).MACRO = { - VERSION: 'test', -} + ;(globalThis as unknown as { MACRO: { VERSION: string } }).MACRO = { + VERSION: 'test', + } -const { validatePath } = await import('../pathValidation.js') -const { getEmptyToolPermissionContext } = await import('../../../Tool.js') + ;({ validatePath } = await import('../pathValidation.js')) + ;({ getEmptyToolPermissionContext } = await import('../../../Tool.js')) +})🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/utils/permissions/__tests__/pathValidation.test.ts` around lines 5 - 24, Move the shared mock setup in pathValidation.test.ts into a beforeAll block, following the repo’s use-shared-mock test pattern. Register the mocked modules with mock.module inside beforeAll, then keep the dynamic imports for validatePath and getEmptyToolPermissionContext after those registrations so the test isolation order is preserved.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/utils/permissions/pathValidation.ts`:
- Around line 492-496: The glob validation path in validateGlobPattern still
checks absolute paths before MinGW normalization, so Git Bash-style bases like
/c/... can be resolved against the wrong Windows location. Update
validateGlobPattern to apply the same posixPathToWindowsPath normalization used
later in pathForResolve before any isAbsolute/resolve logic, and make sure the
glob base is validated using the normalized Windows path when getPlatform()
returns windows.
---
Nitpick comments:
In `@src/utils/permissions/__tests__/pathValidation.test.ts`:
- Around line 5-24: Move the shared mock setup in pathValidation.test.ts into a
beforeAll block, following the repo’s use-shared-mock test pattern. Register the
mocked modules with mock.module inside beforeAll, then keep the dynamic imports
for validatePath and getEmptyToolPermissionContext after those registrations so
the test isolation order is preserved.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 91e40998-ea9c-4c81-9e95-0cd7e8424db3
📒 Files selected for processing (2)
src/utils/permissions/__tests__/pathValidation.test.tssrc/utils/permissions/pathValidation.ts
Comment on lines
+492
to
+496
| const pathForResolve = | ||
| getPlatform() === 'windows' ? posixPathToWindowsPath(cleanPath) : cleanPath | ||
| const absolutePath = isAbsolute(pathForResolve) | ||
| ? pathForResolve | ||
| : resolve(cwd, pathForResolve) |
Contributor
There was a problem hiding this comment.
🔒 Security & Privacy | 🔴 Critical | ⚡ Quick win
Apply the MinGW normalization before glob validation too.
Read glob inputs return through validateGlobPattern before Line 492, and that helper still uses direct isAbsolute/resolve on the glob base. A path like /c/Users/foo/*.txt can therefore be validated against the wrong Windows location while Git Bash expands it on C:\....
🛡️ Suggested fix
+function pathForPlatformResolve(cleanPath: string): string {
+ return getPlatform() === 'windows'
+ ? posixPathToWindowsPath(cleanPath)
+ : cleanPath
+}
+
export function validateGlobPattern(
cleanPath: string,
cwd: string,
toolPermissionContext: ToolPermissionContext,
operationType: FileOperationType,
): ResolvedPathCheckResult {
if (containsPathTraversal(cleanPath)) {
// For patterns with path traversal, resolve the full path
- const absolutePath = isAbsolute(cleanPath)
- ? cleanPath
- : resolve(cwd, cleanPath)
+ const pathForResolve = pathForPlatformResolve(cleanPath)
+ const absolutePath = isAbsolute(pathForResolve)
+ ? pathForResolve
+ : resolve(cwd, pathForResolve)
const { resolvedPath, isCanonical } = safeResolvePath(
getFsImplementation(),
absolutePath,
)
@@
const basePath = getGlobBaseDirectory(cleanPath)
- const absoluteBasePath = isAbsolute(basePath)
- ? basePath
- : resolve(cwd, basePath)
+ const basePathForResolve = pathForPlatformResolve(basePath)
+ const absoluteBasePath = isAbsolute(basePathForResolve)
+ ? basePathForResolve
+ : resolve(cwd, basePathForResolve)
const { resolvedPath, isCanonical } = safeResolvePath(
getFsImplementation(),
absoluteBasePath,
)
@@
- const pathForResolve =
- getPlatform() === 'windows' ? posixPathToWindowsPath(cleanPath) : cleanPath
+ const pathForResolve = pathForPlatformResolve(cleanPath)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const pathForResolve = | |
| getPlatform() === 'windows' ? posixPathToWindowsPath(cleanPath) : cleanPath | |
| const absolutePath = isAbsolute(pathForResolve) | |
| ? pathForResolve | |
| : resolve(cwd, pathForResolve) | |
| function pathForPlatformResolve(cleanPath: string): string { | |
| return getPlatform() === 'windows' | |
| ? posixPathToWindowsPath(cleanPath) | |
| : cleanPath | |
| } | |
| export function validateGlobPattern( | |
| cleanPath: string, | |
| cwd: string, | |
| toolPermissionContext: ToolPermissionContext, | |
| operationType: FileOperationType, | |
| ): ResolvedPathCheckResult { | |
| if (containsPathTraversal(cleanPath)) { | |
| // For patterns with path traversal, resolve the full path | |
| const pathForResolve = pathForPlatformResolve(cleanPath) | |
| const absolutePath = isAbsolute(pathForResolve) | |
| ? pathForResolve | |
| : resolve(cwd, pathForResolve) | |
| const { resolvedPath, isCanonical } = safeResolvePath( | |
| getFsImplementation(), | |
| absolutePath, | |
| ) | |
| ... | |
| } | |
| const basePath = getGlobBaseDirectory(cleanPath) | |
| const basePathForResolve = pathForPlatformResolve(basePath) | |
| const absoluteBasePath = isAbsolute(basePathForResolve) | |
| ? basePathForResolve | |
| : resolve(cwd, basePathForResolve) | |
| const { resolvedPath, isCanonical } = safeResolvePath( | |
| getFsImplementation(), | |
| absoluteBasePath, | |
| ) | |
| ... | |
| const pathForResolve = pathForPlatformResolve(cleanPath) | |
| const absolutePath = isAbsolute(pathForResolve) | |
| ? pathForResolve | |
| : resolve(cwd, pathForResolve) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/utils/permissions/pathValidation.ts` around lines 492 - 496, The glob
validation path in validateGlobPattern still checks absolute paths before MinGW
normalization, so Git Bash-style bases like /c/... can be resolved against the
wrong Windows location. Update validateGlobPattern to apply the same
posixPathToWindowsPath normalization used later in pathForResolve before any
isAbsolute/resolve logic, and make sure the glob base is validated using the
normalized Windows path when getPlatform() returns windows.
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.
…tion
validatePathpreviously used platform-specificpath.isAbsoluteandpath.resolve. On Windows, this created a sandbox-escape class:/c/Users/foo/sensitive.txt(MinGW form, the way Git Bash writes paths) reachesvalidatePath.path.isAbsolute('/c/Users/foo/sensitive.txt')returns true on Windows (treated as a drive-relative absolute path).path.resolve('D:\\project', '/c/Users/foo/sensitive.txt')returnsD:\\c\\Users\\foo\\sensitive.txt(resolved against the current drive).D:\\c\\Users\\foo\\sensitive.txtagainst the allowed-directories list. IfD:\\is broadly allowed (e.g. the user's working directory is on D:), validation passes.C:\\Users\\foo\\sensitive.txt— a completely different filesystem location — bypassing the sandbox.The fix: on Windows, normalize MinGW-style absolute paths (
/c/Users/foo,/cygdrive/c/Users/foo) to Windows paths (C:\\Users\\foo) viaposixPathToWindowsPathBEFORE the isAbsolute/resolve step. The validator's path space now matches the shell's, so the comparison is against the actual target the shell will write to.posixPathToWindowsPathis a no-op for already-Windows paths and relative paths (it just flips slashes), so applying it unconditionally on Windows is safe.Test coverage added in
src/utils/permissions/__tests__/pathValidation.test.ts:/c/...→C:\\...conversion (all 7 cases incl. drive-letter case, bare mount, nested paths)/cygdrive/c/...→C:\\...conversionVerified on Windows 11 + Bun 1.3.14:
bun run precheck: 5906/5906 pass (was 5896/5896, +10 new tests)bun run build: succeedsSummary by CodeRabbit