diff --git a/src/filesystem/__tests__/lib.test.ts b/src/filesystem/__tests__/lib.test.ts index f7e585af22..5e9fc7950b 100644 --- a/src/filesystem/__tests__/lib.test.ts +++ b/src/filesystem/__tests__/lib.test.ts @@ -228,6 +228,64 @@ describe('Lib Functions', () => { process.cwd = originalCwd; } }); + + // Tests for UNC path resolution (bug #3 in the companion PR): + // validatePath calls path.resolve() on the absolute path before the + // allowed-directory check. On Windows, path.resolve corrupts UNC paths + // (\\server\share becomes C:\server\share via path.normalize stripping + // a leading backslash). Without the UNC guard, every UNC access throws + // "Access denied" even when the UNC directory is explicitly allowed. + describe('UNC path handling on Windows', () => { + beforeEach(() => { + if (process.platform !== 'win32') return; + setAllowedDirectories(['\\\\server\\share\\project']); + // Simulate fs.realpath returning the UNC path as-is + mockFs.realpath.mockImplementation(async (p: any) => p.toString()); + }); + + it('accepts UNC file path within allowed UNC directory', async () => { + if (process.platform !== 'win32') return; + const testPath = '\\\\server\\share\\project\\file.txt'; + const result = await validatePath(testPath); + expect(result).toBe(testPath); + }); + + it('accepts UNC directory path matching allowed UNC exactly', async () => { + if (process.platform !== 'win32') return; + const testPath = '\\\\server\\share\\project'; + const result = await validatePath(testPath); + expect(result).toBe(testPath); + }); + + it('accepts nested UNC subdirectory within allowed UNC', async () => { + if (process.platform !== 'win32') return; + const testPath = '\\\\server\\share\\project\\src\\sub\\deep.ts'; + const result = await validatePath(testPath); + expect(result).toBe(testPath); + }); + + it('rejects UNC path outside allowed UNC directory', async () => { + if (process.platform !== 'win32') return; + const testPath = '\\\\server\\share\\other\\file.txt'; + await expect(validatePath(testPath)) + .rejects.toThrow('Access denied - path outside allowed directories'); + }); + + it('rejects UNC path on a different server', async () => { + if (process.platform !== 'win32') return; + const testPath = '\\\\other-server\\share\\project\\file.txt'; + await expect(validatePath(testPath)) + .rejects.toThrow('Access denied - path outside allowed directories'); + }); + + it('accepts UNC file path when allowed UNC has trailing separator', async () => { + if (process.platform !== 'win32') return; + setAllowedDirectories(['\\\\server\\share\\project\\']); + const testPath = '\\\\server\\share\\project\\file.txt'; + const result = await validatePath(testPath); + expect(result).toBe(testPath); + }); + }); }); }); diff --git a/src/filesystem/__tests__/path-validation.test.ts b/src/filesystem/__tests__/path-validation.test.ts index 81ad247ee2..7b6543ac95 100644 --- a/src/filesystem/__tests__/path-validation.test.ts +++ b/src/filesystem/__tests__/path-validation.test.ts @@ -439,6 +439,68 @@ describe('Path Validation', () => { expect(isPathWithinAllowedDirectories('\\\\other\\share\\project', allowed)).toBe(false); } }); + + it('allows UNC path within allowed UNC directory', () => { + if (path.sep === '\\') { + const allowed = ['\\\\server\\share\\project']; + expect(isPathWithinAllowedDirectories('\\\\server\\share\\project\\subdir\\file.txt', allowed)).toBe(true); + } + }); + + it('blocks UNC path outside allowed UNC directory', () => { + if (path.sep === '\\') { + const allowed = ['\\\\server\\share\\project']; + expect(isPathWithinAllowedDirectories('\\\\server\\share\\other', allowed)).toBe(false); + expect(isPathWithinAllowedDirectories('\\\\other\\share\\project', allowed)).toBe(false); + } + }); + + it('allows exact UNC path match', () => { + if (path.sep === '\\') { + const allowed = ['\\\\server\\share\\project']; + expect(isPathWithinAllowedDirectories('\\\\server\\share\\project', allowed)).toBe(true); + } + }); + + // Tests for UNC trailing-slash regression (bug #2 in the companion PR): + // path.normalize preserves trailing separators on UNC paths on Windows, + // while path.resolve strips them for drive paths. Without harmonization, + // the `normalizedDir + path.sep` comparison produces a double separator + // that startsWith() never matches, making every access fail even when + // the allowed directory is configured with a trailing backslash. + it('allows file inside allowed UNC directory with trailing separator', () => { + if (path.sep === '\\') { + const allowed = ['\\\\server\\share\\project\\']; + expect(isPathWithinAllowedDirectories('\\\\server\\share\\project\\file.txt', allowed)).toBe(true); + } + }); + + it('allows subdirectories inside allowed UNC directory with trailing separator', () => { + if (path.sep === '\\') { + const allowed = ['\\\\server\\share\\project\\']; + expect(isPathWithinAllowedDirectories('\\\\server\\share\\project\\src\\index.ts', allowed)).toBe(true); + expect(isPathWithinAllowedDirectories('\\\\server\\share\\project\\deeply\\nested\\file', allowed)).toBe(true); + } + }); + + it('allows exact match against allowed UNC directory with trailing separator', () => { + if (path.sep === '\\') { + const allowed = ['\\\\server\\share\\project\\']; + // Path without trailing separator + expect(isPathWithinAllowedDirectories('\\\\server\\share\\project', allowed)).toBe(true); + // Path with trailing separator + expect(isPathWithinAllowedDirectories('\\\\server\\share\\project\\', allowed)).toBe(true); + } + }); + + it('blocks sibling UNC paths with common prefix when allowed has trailing separator', () => { + if (path.sep === '\\') { + const allowed = ['\\\\server\\share\\project\\']; + expect(isPathWithinAllowedDirectories('\\\\server\\share\\project2', allowed)).toBe(false); + expect(isPathWithinAllowedDirectories('\\\\server\\share\\project_backup', allowed)).toBe(false); + expect(isPathWithinAllowedDirectories('\\\\server\\share\\projectile', allowed)).toBe(false); + } + }); }); describe('Symlink Tests', () => { diff --git a/src/filesystem/index.ts b/src/filesystem/index.ts index 7b67e63e58..c2ee6aa2f8 100644 --- a/src/filesystem/index.ts +++ b/src/filesystem/index.ts @@ -45,7 +45,10 @@ if (args.length === 0) { let allowedDirectories = (await Promise.all( args.map(async (dir) => { const expanded = expandHome(dir); - const absolute = path.resolve(expanded); + // UNC-aware: path.resolve corrupts UNC paths (\\server\share becomes + // C:\server\share on Windows). Skip it for UNC paths and let normalizePath + // handle them. Same fix applied in lib.ts validatePath. + const absolute = expanded.startsWith('\\\\') ? expanded : path.resolve(expanded); const normalizedOriginal = normalizePath(absolute); try { // Security: Resolve symlinks in allowed directories during startup diff --git a/src/filesystem/lib.ts b/src/filesystem/lib.ts index 17e4654cd5..badc8aca4d 100644 --- a/src/filesystem/lib.ts +++ b/src/filesystem/lib.ts @@ -98,8 +98,14 @@ function resolveRelativePathAgainstAllowedDirectories(relativePath: string): str // Security & Validation Functions export async function validatePath(requestedPath: string): Promise { const expandedPath = expandHome(requestedPath); + // UNC-aware resolution: path.resolve() on Windows corrupts UNC paths + // (\\server\share becomes C:\server\share via path.normalize stripping + // a leading backslash, then path.resolve treating it as drive-relative). + // For UNC paths we skip path.resolve entirely and let normalizePath handle + // them. PR #3921 only patched isPathWithinAllowedDirectories; validatePath + // needs the same treatment upstream of it. const absolute = path.isAbsolute(expandedPath) - ? path.resolve(expandedPath) + ? (expandedPath.startsWith('\\\\') ? expandedPath : path.resolve(expandedPath)) : resolveRelativePathAgainstAllowedDirectories(expandedPath); const normalizedRequested = normalizePath(absolute); diff --git a/src/filesystem/path-validation.ts b/src/filesystem/path-validation.ts index 972e9c49d0..7ecf15b5dc 100644 --- a/src/filesystem/path-validation.ts +++ b/src/filesystem/path-validation.ts @@ -1,8 +1,42 @@ import path from 'path'; +/** + * Checks if a path is a UNC path (e.g. \\server\share). + */ +function isUNCPath(p: string): boolean { + return p.startsWith('\\\\'); +} + +/** + * Normalizes a path that may be a UNC path on Windows. + * + * On Windows, path.normalize can strip one leading backslash from UNC paths + * (e.g. \\server\share becomes \server\share), and then path.resolve + * interprets it as a drive-relative path (e.g. C:\server\share). This + * function preserves the UNC prefix through normalization. + */ +function normalizePossiblyUNCPath(p: string): string { + if (isUNCPath(p)) { + let normalized = path.normalize(p); + // path.normalize may strip a leading backslash from UNC paths + if (!normalized.startsWith('\\\\')) { + normalized = '\\' + normalized; + } + // path.normalize preserves trailing separators for UNC paths on Windows, + // while path.resolve (used for non-UNC paths below) strips them. Harmonize + // here: without stripping, `normalizedDir + path.sep` in the caller produces + // a double separator that startsWith() never matches. + if (normalized.length > 2 && normalized.endsWith(path.sep)) { + normalized = normalized.slice(0, -1); + } + return normalized; + } + return path.resolve(path.normalize(p)); +} + /** * Checks if an absolute path is within any of the allowed directories. - * + * * @param absolutePath - The absolute path to check (will be normalized) * @param allowedDirectories - Array of absolute allowed directory paths (will be normalized) * @returns true if the path is within an allowed directory, false otherwise @@ -27,7 +61,7 @@ export function isPathWithinAllowedDirectories(absolutePath: string, allowedDire // Normalize the input path let normalizedPath: string; try { - normalizedPath = path.resolve(path.normalize(absolutePath)); + normalizedPath = normalizePossiblyUNCPath(absolutePath); } catch { return false; } @@ -51,7 +85,7 @@ export function isPathWithinAllowedDirectories(absolutePath: string, allowedDire // Normalize the allowed directory let normalizedDir: string; try { - normalizedDir = path.resolve(path.normalize(dir)); + normalizedDir = normalizePossiblyUNCPath(dir); } catch { return false; }