fix(permissions): normalize MinGW paths in validatePath before resolu…#1287
Open
Ericcc-Ma wants to merge 2 commits into
Open
fix(permissions): normalize MinGW paths in validatePath before resolu…#1287Ericcc-Ma wants to merge 2 commits into
Ericcc-Ma wants to merge 2 commits 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)
✨ 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.
… warning CodeRabbit flagged the PR with "Docstring coverage is 50.00% which is insufficient (threshold 80.00%)". Added JSDoc to: 1. `formatDirectoryList` in pathValidation.ts — the only production function in the changed files that lacked JSDoc. Brief description of the truncation behavior + reference to MAX_DIRS_TO_LIST. 2. Both `describeIfWindows` blocks in pathValidation.test.ts — explain the security rationale of the MinGW-normalization fix and the sandbox-escape regression scenarios. Individual `test()` calls already self-document via their string descriptions. Co-Authored-By: Claude <noreply@anthropic.com>
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.
Summary — Sandbox Escape / Windows Path Validation
validatePathpreviously used platform-specificpath.isAbsoluteandpath.resolve. On Windows, this created a sandbox-escapeclass: a model emitting
/c/Users/foo/sensitive.txt(MinGW form, the way Git Bash writes paths) reaches validation, gets resolvedto
D:\\c\\Users\\foo\\sensitive.txt(resolved against the current drive), passes validation against an allowlist that includesD:\\, while Git Bash actually writes toC:\\Users\\foo\\sensitive.txt— a completely different filesystem location.Root Cause
The validator's path space (Windows host) didn't match the shell's path space (Git Bash MinGW). Specifically:
On Windows:
isAbsolute('/c/Users/foo/sensitive.txt')→true(drive-relative absolute path)resolve('D:\\project', '/c/Users/foo/sensitive.txt')→D:\\c\\Users\\foo\\sensitive.txtThe validator then compares
D:\\c\\Users\\foo\\sensitive.txtagainst the allowed-directories list. IfD:\\is broadly allowed,validation passes — but Git Bash writes to
C:\\Users\\foo\\sensitive.txt. The validator's path space and the shell's path spacedon't match.
Attack Scenario
D:\\projects\\appecho malicious > /c/Users/victim/AppData/creds.txtD:\\c\\Users\\victim\\AppData\\creds.txt→ resolved againstD:\\(allowed) → passes validationC:\\Users\\victim\\AppData\\creds.txt— outside any allowed dir, sandbox escapedFix
Normalize MinGW-style absolute paths (
/c/Users/foo,/cygdrive/c/Users/foo) to Windows paths (C:\\Users\\foo) via the existingposixPathToWindowsPathhelper (already insrc/utils/windowsPaths.ts, used by ACP and other paths) BEFORE the isAbsolute/resolvestep:
posixPathToWindowsPathis a no-op for already-Windows paths and relative paths (it just flips slashes), so applying itunconditionally on Windows is safe.
Test Coverage
10 new tests in
src/utils/permissions/__tests__/pathValidation.test.ts:MinGW path normalization (8 tests):
/c/Users/foo/file.txt→C:\\Users\\foo\\file.txt/cygdrive/c/Users/foo/file.txt→C:\\Users\\foo\\file.txt/d/...→D:\\.../c→C:\\/c/→C:\\Sandbox escape regression (2 tests):
C:\\Users\\foo, notD:\\c\\Users\\foo)Verification
bun run precheckon Windows 11: 5906/5906 pass (was 5896/5896, +10 new tests)bun run buildon Windows 11: succeeds (bothdist/cli-bun.jsanddist/cli-node.js)resolve('D:\\project', '/c/Users/foo/...')→D:\\c\\Users\\foo\\...on Windows before fix; now properlynormalized to
C:\\Users\\foo\\...Environment Tested