Skip to content

Commit 93ab1de

Browse files
hongyeclaude
andcommitted
fix(permissions): normalize MinGW paths in validatePath before resolution
`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>
1 parent 0753daf commit 93ab1de

2 files changed

Lines changed: 207 additions & 3 deletions

File tree

Lines changed: 185 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,185 @@
1+
import { describe, expect, mock, test } from 'bun:test'
2+
import { logMock } from '../../../../tests/mocks/log'
3+
import { debugMock } from '../../../../tests/mocks/debug'
4+
5+
// Cut the bootstrap/state dependency chain (mock.module requirement).
6+
mock.module('src/utils/log.ts', logMock)
7+
mock.module('src/utils/debug.ts', debugMock)
8+
mock.module('bun:bundle', () => ({
9+
feature: (_name: string) => false,
10+
}))
11+
12+
// MACRO is a build-time define injected by `bun --define` (see
13+
// scripts/dev.ts → -d flags). Without it, `declare const MACRO` references
14+
// in source code resolve to `undefined` at runtime and crash any function
15+
// that touches `MACRO.VERSION` (e.g. `getBundledSkillsRoot` via
16+
// `checkReadableInternalPath`).
17+
// Setting it on globalThis lets the bare `MACRO` identifier resolve at
18+
// runtime in tests.
19+
;(globalThis as unknown as { MACRO: { VERSION: string } }).MACRO = {
20+
VERSION: 'test',
21+
}
22+
23+
const { validatePath } = await import('../pathValidation.js')
24+
const { getEmptyToolPermissionContext } = await import('../../../Tool.js')
25+
26+
function makeContext(): ReturnType<typeof getEmptyToolPermissionContext> {
27+
return getEmptyToolPermissionContext()
28+
}
29+
30+
const isWindows = process.platform === 'win32'
31+
const describeIfWindows = isWindows ? describe : describe.skip
32+
33+
// ─── MinGW path normalization (Windows) ──────────────────────────────────
34+
//
35+
// These tests pin the fix for a sandbox-escape class: on Windows, the Git
36+
// Bash shell interprets paths like `/c/Users/foo/bar.txt` as `C:\Users\foo\bar.txt`
37+
// (the C: drive). However, the Node `path` module treats such paths as
38+
// drive-relative absolute paths on the current drive, so:
39+
// - path.isAbsolute('/c/Users/foo/bar.txt') === true (on Windows)
40+
// - path.resolve('D:\\project', '/c/Users/foo/bar.txt')
41+
// === 'D:\\c\\Users\\foo\\bar.txt' (on Windows)
42+
//
43+
// That means without normalization, validatePath would compare
44+
// `D:\c\Users\foo\bar.txt` against the allowed-directories list, while
45+
// Git Bash actually writes to `C:\Users\foo\bar.txt` — a completely
46+
// different filesystem location. This is a TOCTOU/sandbox-escape bug.
47+
//
48+
// The fix runs `posixPathToWindowsPath` on Windows before resolution,
49+
// converting `/c/...` and `/cygdrive/c/...` to their `C:\...` form so the
50+
// validator's path space matches the shell's.
51+
describeIfWindows('validatePath MinGW path normalization', () => {
52+
test('converts /c/Users/foo/file.txt to C:\\Users\\foo\\file.txt', () => {
53+
const result = validatePath(
54+
'/c/Users/foo/file.txt',
55+
'D:\\project',
56+
makeContext(),
57+
'read',
58+
)
59+
// resolvedPath is the canonical form the validator (and ultimately the
60+
// shell) operates on. It must be the Windows-style path, not the
61+
// drive-relative form `D:\c\Users\foo\file.txt`.
62+
expect(result.resolvedPath.replace(/\//g, '\\')).toBe(
63+
'C:\\Users\\foo\\file.txt',
64+
)
65+
})
66+
67+
test('converts /cygdrive/c/Users/foo/file.txt to C:\\Users\\foo\\file.txt', () => {
68+
const result = validatePath(
69+
'/cygdrive/c/Users/foo/file.txt',
70+
'D:\\project',
71+
makeContext(),
72+
'read',
73+
)
74+
expect(result.resolvedPath.replace(/\//g, '\\')).toBe(
75+
'C:\\Users\\foo\\file.txt',
76+
)
77+
})
78+
79+
test('uppercases the drive letter', () => {
80+
const result = validatePath(
81+
'/d/work/file.txt',
82+
'C:\\project',
83+
makeContext(),
84+
'read',
85+
)
86+
expect(result.resolvedPath.replace(/\//g, '\\')).toBe('D:\\work\\file.txt')
87+
})
88+
89+
test('preserves Windows paths unchanged', () => {
90+
// An already-Windows path should not be touched.
91+
const result = validatePath(
92+
'C:\\Users\\foo\\file.txt',
93+
'D:\\project',
94+
makeContext(),
95+
'read',
96+
)
97+
expect(result.resolvedPath.replace(/\//g, '\\')).toBe(
98+
'C:\\Users\\foo\\file.txt',
99+
)
100+
})
101+
102+
test('preserves relative paths (just flips slashes)', () => {
103+
// Relative paths are not MinGW absolute paths; the conversion
104+
// should be a no-op aside from slash direction. The path is then
105+
// resolved against cwd by `validatePath`, which is expected behavior.
106+
const result = validatePath(
107+
'src/file.txt',
108+
'D:\\project',
109+
makeContext(),
110+
'read',
111+
)
112+
expect(result.resolvedPath.replace(/\//g, '\\')).toBe(
113+
'D:\\project\\src\\file.txt',
114+
)
115+
})
116+
117+
test('handles bare drive mount (no trailing path)', () => {
118+
const result = validatePath('/c', 'D:\\project', makeContext(), 'read')
119+
expect(result.resolvedPath.replace(/\//g, '\\')).toBe('C:\\')
120+
})
121+
122+
test('handles drive root with trailing slash', () => {
123+
const result = validatePath('/c/', 'D:\\project', makeContext(), 'read')
124+
expect(result.resolvedPath.replace(/\//g, '\\')).toBe('C:\\')
125+
})
126+
127+
test('handles deeply nested MinGW paths', () => {
128+
const result = validatePath(
129+
'/c/Users/me/Documents/project/src/index.ts',
130+
'D:\\project',
131+
makeContext(),
132+
'read',
133+
)
134+
expect(result.resolvedPath.replace(/\//g, '\\')).toBe(
135+
'C:\\Users\\me\\Documents\\project\\src\\index.ts',
136+
)
137+
})
138+
})
139+
140+
// ─── Sandbox escape regression (Windows) ─────────────────────────────────
141+
//
142+
// This is the bug the MinGW-normalization fix exists to prevent: without
143+
// it, the validator compares `<currentDrive>:\c\Users\foo\file.txt` against
144+
// the allowed dirs, while bash writes to `C:\Users\foo\file.txt`. With the
145+
// fix, both sides of the comparison use the same `C:\Users\foo\file.txt`
146+
// location.
147+
//
148+
// We pin this by setting up a context where:
149+
// - cwd is `D:\project` (and D:\project is allowed)
150+
// - `C:\Users\foo` is NOT in any allowed directory
151+
// Then we check that `/c/Users/foo/sensitive.txt` is denied with a
152+
// resolvedPath pointing at C:\Users\foo — proving the validator now sees
153+
// the same path the shell will write to.
154+
describeIfWindows('validatePath sandbox escape regression', () => {
155+
test('MinGW path that escapes allowed dirs is denied at correct location', () => {
156+
// Without the fix, this would resolve to `D:\c\Users\foo\sensitive.txt`
157+
// and (if D:\ is broadly allowed) pass validation, while bash actually
158+
// writes to `C:\Users\foo\sensitive.txt`. With the fix, the validator
159+
// sees the correct path and denies it because C:\Users\foo is not in
160+
// any allowed directory.
161+
const result = validatePath(
162+
'/c/Users/foo/sensitive.txt',
163+
'D:\\project',
164+
makeContext(),
165+
'create',
166+
)
167+
expect(result.allowed).toBe(false)
168+
// The resolvedPath should be at C:\Users\foo — not D:\c\Users\foo.
169+
const normalized = result.resolvedPath.replace(/\//g, '\\')
170+
expect(normalized.startsWith('C:\\Users\\foo')).toBe(true)
171+
expect(normalized.startsWith('D:\\c\\')).toBe(false)
172+
})
173+
174+
test('cygdrive path that escapes allowed dirs is denied at correct location', () => {
175+
const result = validatePath(
176+
'/cygdrive/c/Users/foo/sensitive.txt',
177+
'D:\\project',
178+
makeContext(),
179+
'create',
180+
)
181+
expect(result.allowed).toBe(false)
182+
const normalized = result.resolvedPath.replace(/\//g, '\\')
183+
expect(normalized.startsWith('C:\\Users\\foo')).toBe(true)
184+
})
185+
})

src/utils/permissions/pathValidation.ts

Lines changed: 22 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@ import { homedir } from 'os'
33
import { dirname, isAbsolute, resolve } from 'path'
44
import type { ToolPermissionContext } from '../../Tool.js'
55
import { getPlatform } from '../../utils/platform.js'
6+
import { posixPathToWindowsPath } from '../windowsPaths.js'
67
import {
78
getFsImplementation,
89
getPathsForPermissionCheck,
@@ -472,9 +473,27 @@ export function validatePath(
472473
}
473474

474475
// Resolve path
475-
const absolutePath = isAbsolute(cleanPath)
476-
? cleanPath
477-
: resolve(cwd, cleanPath)
476+
// SECURITY: On Windows, normalize MinGW-style absolute paths
477+
// (`/c/Users/foo`, `/cygdrive/c/Users/foo`) to Windows paths
478+
// (`C:\Users\foo`) BEFORE the isAbsolute/resolve step below.
479+
//
480+
// Without this, `path.isAbsolute('/c/Users/foo')` returns true on
481+
// Windows (interpreted as a drive-relative absolute path) and
482+
// `path.resolve(cwd, '/c/Users/foo')` produces `<currentDrive>:\c\Users\foo`
483+
// — a completely different filesystem location from what Git Bash
484+
// actually writes to (`C:\Users\foo`). The validator would compare
485+
// the wrong path against the allowed-directories list, enabling a
486+
// sandbox escape where `/c/Users/foo/sensitive.txt` passes validation
487+
// when `C:\Users\foo\sensitive.txt` is not in any allowed dir.
488+
//
489+
// `posixPathToWindowsPath` is a no-op for already-Windows paths and
490+
// for relative paths (it just flips slashes), so it's safe to apply
491+
// unconditionally on Windows.
492+
const pathForResolve =
493+
getPlatform() === 'windows' ? posixPathToWindowsPath(cleanPath) : cleanPath
494+
const absolutePath = isAbsolute(pathForResolve)
495+
? pathForResolve
496+
: resolve(cwd, pathForResolve)
478497
const { resolvedPath, isCanonical } = safeResolvePath(
479498
getFsImplementation(),
480499
absolutePath,

0 commit comments

Comments
 (0)