diff --git a/src/filesystem/__tests__/path-validation.test.ts b/src/filesystem/__tests__/path-validation.test.ts index 81ad247ee2..438bfb4e71 100644 --- a/src/filesystem/__tests__/path-validation.test.ts +++ b/src/filesystem/__tests__/path-validation.test.ts @@ -439,6 +439,40 @@ describe('Path Validation', () => { expect(isPathWithinAllowedDirectories('\\\\other\\share\\project', allowed)).toBe(false); } }); + + it('allows UNC path subdirectories (GitHub issue #3527)', () => { + // This test verifies that UNC paths like \\server\share\subdir are correctly + // recognized as being within allowed directories like \\server\share. + // The bug was that path.resolve() on Windows corrupts UNC paths by prepending + // the current drive letter (e.g., \\server\share becomes C:\server\share). + + // Test exact match of UNC share root + const allowedRoot = ['\\\\192.168.4.96\\Mega']; + expect(isPathWithinAllowedDirectories('\\\\192.168.4.96\\Mega', allowedRoot)).toBe(true); + + // Test subdirectories under UNC share - this was failing before the fix + expect(isPathWithinAllowedDirectories('\\\\192.168.4.96\\Mega\\Drops', allowedRoot)).toBe(true); + expect(isPathWithinAllowedDirectories('\\\\192.168.4.96\\Mega\\00R RPGs', allowedRoot)).toBe(true); + expect(isPathWithinAllowedDirectories('\\\\192.168.4.96\\Mega\\deeply\\nested\\path', allowedRoot)).toBe(true); + expect(isPathWithinAllowedDirectories('\\\\192.168.4.96\\Mega\\file.txt', allowedRoot)).toBe(true); + + // Test that different shares on same server are blocked + expect(isPathWithinAllowedDirectories('\\\\192.168.4.96\\Plex', allowedRoot)).toBe(false); + expect(isPathWithinAllowedDirectories('\\\\192.168.4.96\\Plex\\Movies', allowedRoot)).toBe(false); + + // Test that different servers are blocked + expect(isPathWithinAllowedDirectories('\\\\otherserver\\share\\path', allowedRoot)).toBe(false); + + // Test with multiple UNC allowed directories + const allowedMultiple = ['\\\\192.168.4.96\\Mega', '\\\\192.168.4.96\\Plex']; + expect(isPathWithinAllowedDirectories('\\\\192.168.4.96\\Mega\\Drops', allowedMultiple)).toBe(true); + expect(isPathWithinAllowedDirectories('\\\\192.168.4.96\\Plex\\Movies', allowedMultiple)).toBe(true); + expect(isPathWithinAllowedDirectories('\\\\192.168.4.96\\OtherShare\\file', allowedMultiple)).toBe(false); + + // Test UNC path prefix attacks are blocked + expect(isPathWithinAllowedDirectories('\\\\192.168.4.96\\MegaBackup', allowedRoot)).toBe(false); + expect(isPathWithinAllowedDirectories('\\\\192.168.4.96\\MegaExtra\\file', allowedRoot)).toBe(false); + }); }); describe('Symlink Tests', () => { diff --git a/src/filesystem/path-validation.ts b/src/filesystem/path-validation.ts index 972e9c49d0..ce5c8778e8 100644 --- a/src/filesystem/path-validation.ts +++ b/src/filesystem/path-validation.ts @@ -1,5 +1,33 @@ import path from 'path'; +/** + * Normalizes a path without corrupting UNC paths. + * On Windows, path.resolve() converts UNC paths like \\server\share to C:\server\share, + * which breaks UNC path handling. This function preserves UNC paths. + * + * @param p - The path to normalize + * @returns Normalized path + */ +function normalizePathSafe(p: string): string { + // Check if it's a UNC path (starts with \\ or //) BEFORE normalizing + // We must check this first because path.normalize on macOS corrupts UNC paths + if (p.startsWith('\\\\') || p.startsWith('//')) { + // UNC paths: normalize slashes to backslashes and remove redundant slashes + // but preserve the leading \\ + let normalized = p.replace(/\//g, '\\'); + // Ensure exactly two leading backslashes (not more, not less) + normalized = normalized.replace(/^\\+/, '\\\\'); + // Normalize any double backslashes in the rest of the path to single + // but be careful not to break the leading \\ + const rest = normalized.substring(2).replace(/\\+/g, '\\'); + return '\\\\' + rest; + } + + // For non-UNC paths, use path.normalize then path.resolve + const normalized = path.normalize(p); + return path.resolve(normalized); +} + /** * Checks if an absolute path is within any of the allowed directories. * @@ -27,13 +55,15 @@ export function isPathWithinAllowedDirectories(absolutePath: string, allowedDire // Normalize the input path let normalizedPath: string; try { - normalizedPath = path.resolve(path.normalize(absolutePath)); + normalizedPath = normalizePathSafe(absolutePath); } catch { return false; } // Verify it's absolute after normalization - if (!path.isAbsolute(normalizedPath)) { + // UNC paths are always absolute, as are resolved paths + const isUncPath = normalizedPath.startsWith('\\\\'); + if (!isUncPath && !path.isAbsolute(normalizedPath)) { throw new Error('Path must be absolute after normalization'); } @@ -51,13 +81,14 @@ export function isPathWithinAllowedDirectories(absolutePath: string, allowedDire // Normalize the allowed directory let normalizedDir: string; try { - normalizedDir = path.resolve(path.normalize(dir)); + normalizedDir = normalizePathSafe(dir); } catch { return false; } // Verify allowed directory is absolute after normalization - if (!path.isAbsolute(normalizedDir)) { + const isUncDir = normalizedDir.startsWith('\\\\'); + if (!isUncDir && !path.isAbsolute(normalizedDir)) { throw new Error('Allowed directories must be absolute paths after normalization'); } @@ -81,6 +112,18 @@ export function isPathWithinAllowedDirectories(absolutePath: string, allowedDire return pathDrive === dirDrive && normalizedPath.startsWith(normalizedDir.replace(/\\?$/, '\\')); } + // Special handling for UNC paths + // Both paths must be UNC paths for a valid match + if (isUncPath && isUncDir) { + // For UNC paths, use backslash as separator + return normalizedPath.startsWith(normalizedDir + '\\'); + } + + // If one is UNC and the other isn't, they can't match + if (isUncPath !== isUncDir) { + return false; + } + return normalizedPath.startsWith(normalizedDir + path.sep); }); }