From de13fd24781943c8414d3330b9c0dba78b87307f Mon Sep 17 00:00:00 2001 From: unknown Date: Sat, 18 Apr 2026 18:01:23 +0200 Subject: [PATCH] Fix UNC path validation: cover all corruption sites on Windows PR #3921 fixes isPathWithinAllowedDirectories for UNC paths, but three other sites in the filesystem server also corrupt UNC paths via path.resolve or preserve them incorrectly through path.normalize: 1. path-validation.ts::normalizePossiblyUNCPath path.normalize preserves trailing separators on UNC paths on Windows (unlike path.resolve for drive paths), so 'normalizedDir + path.sep' produces a double separator that startsWith() never matches. Strip the trailing separator to harmonize. 2. lib.ts::validatePath Calls path.resolve(expandedPath) before the allowed-directory check, which on Windows corrupts UNC paths into drive-relative paths (\\\\server\\share -> C:\\server\\share). Skip path.resolve for UNC paths and let normalizePath handle them. 3. index.ts bootstrap Same path.resolve corruption when normalizing allowed directories at server startup, before they are stored via setAllowedDirectories. Same guard applied. All non-UNC paths retain existing behavior (path.resolve + normalize). Tests added: - 4 tests in path-validation.test.ts for the trailing-separator case - 6 tests in lib.test.ts for validatePath end-to-end on UNC, gated behind process.platform === 'win32' and using the existing fs mock pattern - Bug #3 (bootstrap) has no direct unit test since UNC access in CI is not available, but the fix applies the same principle as validatePath and is logically covered. Fixes #3756 Supersedes #3921 --- src/filesystem/__tests__/lib.test.ts | 58 +++++++++++++++++ .../__tests__/path-validation.test.ts | 62 +++++++++++++++++++ src/filesystem/index.ts | 5 +- src/filesystem/lib.ts | 8 ++- src/filesystem/path-validation.ts | 40 +++++++++++- 5 files changed, 168 insertions(+), 5 deletions(-) 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; }