From 1a41ba7daf369a45fd350dbd630142f0cac70f92 Mon Sep 17 00:00:00 2001 From: Tommaso Sciortino Date: Thu, 14 Aug 2025 15:59:37 -0700 Subject: [PATCH] Prevent writing outside of the workspace roots (#6178) Co-authored-by: Jacob Richman --- .../core/src/utils/workspaceContext.test.ts | 390 ++++++++++-------- packages/core/src/utils/workspaceContext.ts | 119 +++--- 2 files changed, 258 insertions(+), 251 deletions(-) diff --git a/packages/core/src/utils/workspaceContext.test.ts b/packages/core/src/utils/workspaceContext.test.ts index 67d06b62..5446c287 100644 --- a/packages/core/src/utils/workspaceContext.test.ts +++ b/packages/core/src/utils/workspaceContext.test.ts @@ -4,280 +4,304 @@ * SPDX-License-Identifier: Apache-2.0 */ -import { describe, it, expect, beforeEach, vi } from 'vitest'; +import { describe, it, expect, beforeEach, afterEach } from 'vitest'; import * as fs from 'fs'; +import * as os from 'os'; import * as path from 'path'; import { WorkspaceContext } from './workspaceContext.js'; -vi.mock('fs'); - -describe('WorkspaceContext', () => { - let workspaceContext: WorkspaceContext; - // Use path module to create platform-agnostic paths - const mockCwd = path.resolve(path.sep, 'home', 'user', 'project'); - const mockExistingDir = path.resolve( - path.sep, - 'home', - 'user', - 'other-project', - ); - const mockNonExistentDir = path.resolve( - path.sep, - 'home', - 'user', - 'does-not-exist', - ); - const mockSymlinkDir = path.resolve(path.sep, 'home', 'user', 'symlink'); - const mockRealPath = path.resolve(path.sep, 'home', 'user', 'real-directory'); +describe('WorkspaceContext with real filesystem', () => { + let tempDir: string; + let cwd: string; + let otherDir: string; beforeEach(() => { - vi.resetAllMocks(); + // os.tmpdir() can return a path using a symlink (this is standard on macOS) + // Use fs.realpathSync to fully resolve the absolute path. + tempDir = fs.realpathSync( + fs.mkdtempSync(path.join(os.tmpdir(), 'workspace-context-test-')), + ); - // Mock fs.existsSync - vi.mocked(fs.existsSync).mockImplementation((path) => { - const pathStr = path.toString(); - return ( - pathStr === mockCwd || - pathStr === mockExistingDir || - pathStr === mockSymlinkDir || - pathStr === mockRealPath - ); - }); + cwd = path.join(tempDir, 'project'); + otherDir = path.join(tempDir, 'other-project'); - // Mock fs.statSync - vi.mocked(fs.statSync).mockImplementation((path) => { - const pathStr = path.toString(); - if (pathStr === mockNonExistentDir) { - throw new Error('ENOENT'); - } - return { - isDirectory: () => true, - } as fs.Stats; - }); + fs.mkdirSync(cwd, { recursive: true }); + fs.mkdirSync(otherDir, { recursive: true }); + }); - // Mock fs.realpathSync - vi.mocked(fs.realpathSync).mockImplementation((path) => { - const pathStr = path.toString(); - if (pathStr === mockSymlinkDir) { - return mockRealPath; - } - return pathStr; - }); + afterEach(() => { + fs.rmSync(tempDir, { recursive: true, force: true }); }); describe('initialization', () => { it('should initialize with a single directory (cwd)', () => { - workspaceContext = new WorkspaceContext(mockCwd); + const workspaceContext = new WorkspaceContext(cwd); const directories = workspaceContext.getDirectories(); - expect(directories).toHaveLength(1); - expect(directories[0]).toBe(mockCwd); + + expect(directories).toEqual([cwd]); }); it('should validate and resolve directories to absolute paths', () => { - const absolutePath = path.join(mockCwd, 'subdir'); - vi.mocked(fs.existsSync).mockImplementation( - (p) => p === mockCwd || p === absolutePath, - ); - vi.mocked(fs.realpathSync).mockImplementation((p) => p.toString()); - - workspaceContext = new WorkspaceContext(mockCwd, [absolutePath]); + const workspaceContext = new WorkspaceContext(cwd, [otherDir]); const directories = workspaceContext.getDirectories(); - expect(directories).toContain(absolutePath); + + expect(directories).toEqual([cwd, otherDir]); }); it('should reject non-existent directories', () => { + const nonExistentDir = path.join(tempDir, 'does-not-exist'); expect(() => { - new WorkspaceContext(mockCwd, [mockNonExistentDir]); + new WorkspaceContext(cwd, [nonExistentDir]); }).toThrow('Directory does not exist'); }); it('should handle empty initialization', () => { - workspaceContext = new WorkspaceContext(mockCwd, []); + const workspaceContext = new WorkspaceContext(cwd, []); const directories = workspaceContext.getDirectories(); expect(directories).toHaveLength(1); - expect(directories[0]).toBe(mockCwd); + expect(fs.realpathSync(directories[0])).toBe(cwd); }); }); describe('adding directories', () => { - beforeEach(() => { - workspaceContext = new WorkspaceContext(mockCwd); - }); - it('should add valid directories', () => { - workspaceContext.addDirectory(mockExistingDir); + const workspaceContext = new WorkspaceContext(cwd); + workspaceContext.addDirectory(otherDir); const directories = workspaceContext.getDirectories(); - expect(directories).toHaveLength(2); - expect(directories).toContain(mockExistingDir); + + expect(directories).toEqual([cwd, otherDir]); }); it('should resolve relative paths to absolute', () => { - // Since we can't mock path.resolve, we'll test with absolute paths - workspaceContext.addDirectory(mockExistingDir); + const workspaceContext = new WorkspaceContext(cwd); + const relativePath = path.relative(cwd, otherDir); + workspaceContext.addDirectory(relativePath, cwd); const directories = workspaceContext.getDirectories(); - expect(directories).toContain(mockExistingDir); + + expect(directories).toEqual([cwd, otherDir]); }); it('should reject non-existent directories', () => { + const nonExistentDir = path.join(tempDir, 'does-not-exist'); + const workspaceContext = new WorkspaceContext(cwd); + expect(() => { - workspaceContext.addDirectory(mockNonExistentDir); + workspaceContext.addDirectory(nonExistentDir); }).toThrow('Directory does not exist'); }); it('should prevent duplicate directories', () => { - workspaceContext.addDirectory(mockExistingDir); - workspaceContext.addDirectory(mockExistingDir); + const workspaceContext = new WorkspaceContext(cwd); + workspaceContext.addDirectory(otherDir); + workspaceContext.addDirectory(otherDir); const directories = workspaceContext.getDirectories(); - expect(directories.filter((d) => d === mockExistingDir)).toHaveLength(1); + + expect(directories).toHaveLength(2); }); it('should handle symbolic links correctly', () => { - workspaceContext.addDirectory(mockSymlinkDir); + const realDir = path.join(tempDir, 'real'); + fs.mkdirSync(realDir, { recursive: true }); + const symlinkDir = path.join(tempDir, 'symlink-to-real'); + fs.symlinkSync(realDir, symlinkDir, 'dir'); + const workspaceContext = new WorkspaceContext(cwd); + workspaceContext.addDirectory(symlinkDir); + const directories = workspaceContext.getDirectories(); - expect(directories).toContain(mockRealPath); - expect(directories).not.toContain(mockSymlinkDir); + + expect(directories).toEqual([cwd, realDir]); }); }); describe('path validation', () => { - beforeEach(() => { - workspaceContext = new WorkspaceContext(mockCwd, [mockExistingDir]); + it('should accept paths within workspace directories', () => { + const workspaceContext = new WorkspaceContext(cwd, [otherDir]); + const validPath1 = path.join(cwd, 'src', 'file.ts'); + const validPath2 = path.join(otherDir, 'lib', 'module.js'); + + fs.mkdirSync(path.dirname(validPath1), { recursive: true }); + fs.writeFileSync(validPath1, 'content'); + fs.mkdirSync(path.dirname(validPath2), { recursive: true }); + fs.writeFileSync(validPath2, 'content'); + + expect(workspaceContext.isPathWithinWorkspace(validPath1)).toBe(true); + expect(workspaceContext.isPathWithinWorkspace(validPath2)).toBe(true); }); - it('should accept paths within workspace directories', () => { - const validPath1 = path.join(mockCwd, 'src', 'file.ts'); - const validPath2 = path.join(mockExistingDir, 'lib', 'module.js'); + it('should accept non-existent paths within workspace directories', () => { + const workspaceContext = new WorkspaceContext(cwd, [otherDir]); + const validPath1 = path.join(cwd, 'src', 'file.ts'); + const validPath2 = path.join(otherDir, 'lib', 'module.js'); expect(workspaceContext.isPathWithinWorkspace(validPath1)).toBe(true); expect(workspaceContext.isPathWithinWorkspace(validPath2)).toBe(true); }); it('should reject paths outside workspace', () => { - const invalidPath = path.resolve( - path.dirname(mockCwd), - 'outside-workspace', - 'file.txt', - ); + const workspaceContext = new WorkspaceContext(cwd, [otherDir]); + const invalidPath = path.join(tempDir, 'outside-workspace', 'file.txt'); + expect(workspaceContext.isPathWithinWorkspace(invalidPath)).toBe(false); }); - it('should resolve symbolic links before validation', () => { - const symlinkPath = path.join(mockCwd, 'symlink-file'); - const realPath = path.join(mockCwd, 'real-file'); + it('should reject non-existent paths outside workspace', () => { + const workspaceContext = new WorkspaceContext(cwd, [otherDir]); + const invalidPath = path.join(tempDir, 'outside-workspace', 'file.txt'); - vi.mocked(fs.existsSync).mockReturnValue(true); - vi.mocked(fs.realpathSync).mockImplementation((p) => { - if (p === symlinkPath) { - return realPath; - } - return p.toString(); - }); - - expect(workspaceContext.isPathWithinWorkspace(symlinkPath)).toBe(true); + expect(workspaceContext.isPathWithinWorkspace(invalidPath)).toBe(false); }); it('should handle nested directories correctly', () => { - const nestedPath = path.join( - mockCwd, - 'deeply', - 'nested', - 'path', - 'file.txt', - ); + const workspaceContext = new WorkspaceContext(cwd, [otherDir]); + const nestedPath = path.join(cwd, 'deeply', 'nested', 'path', 'file.txt'); expect(workspaceContext.isPathWithinWorkspace(nestedPath)).toBe(true); }); it('should handle edge cases (root, parent references)', () => { - const rootPath = '/'; - const parentPath = path.dirname(mockCwd); + const workspaceContext = new WorkspaceContext(cwd, [otherDir]); + const rootPath = path.parse(tempDir).root; + const parentPath = path.dirname(cwd); expect(workspaceContext.isPathWithinWorkspace(rootPath)).toBe(false); expect(workspaceContext.isPathWithinWorkspace(parentPath)).toBe(false); }); it('should handle non-existent paths correctly', () => { - const nonExistentPath = path.join(mockCwd, 'does-not-exist.txt'); - vi.mocked(fs.existsSync).mockImplementation((p) => p !== nonExistentPath); - - // Should still validate based on path structure + const workspaceContext = new WorkspaceContext(cwd, [otherDir]); + const nonExistentPath = path.join(cwd, 'does-not-exist.txt'); expect(workspaceContext.isPathWithinWorkspace(nonExistentPath)).toBe( true, ); }); + + describe('with symbolic link', () => { + describe('in the workspace', () => { + let realDir: string; + let symlinkDir: string; + beforeEach(() => { + realDir = path.join(cwd, 'real-dir'); + fs.mkdirSync(realDir, { recursive: true }); + + symlinkDir = path.join(cwd, 'symlink-file'); + fs.symlinkSync(realDir, symlinkDir, 'dir'); + }); + + it('should accept dir paths', () => { + const workspaceContext = new WorkspaceContext(cwd); + + expect(workspaceContext.isPathWithinWorkspace(symlinkDir)).toBe(true); + }); + + it('should accept non-existent paths', () => { + const filePath = path.join(symlinkDir, 'does-not-exist.txt'); + + const workspaceContext = new WorkspaceContext(cwd); + + expect(workspaceContext.isPathWithinWorkspace(filePath)).toBe(true); + }); + + it('should accept non-existent deep paths', () => { + const filePath = path.join(symlinkDir, 'deep', 'does-not-exist.txt'); + + const workspaceContext = new WorkspaceContext(cwd); + + expect(workspaceContext.isPathWithinWorkspace(filePath)).toBe(true); + }); + }); + + describe('outside the workspace', () => { + let realDir: string; + let symlinkDir: string; + beforeEach(() => { + realDir = path.join(tempDir, 'real-dir'); + fs.mkdirSync(realDir, { recursive: true }); + + symlinkDir = path.join(cwd, 'symlink-file'); + fs.symlinkSync(realDir, symlinkDir, 'dir'); + }); + + it('should reject dir paths', () => { + const workspaceContext = new WorkspaceContext(cwd); + + expect(workspaceContext.isPathWithinWorkspace(symlinkDir)).toBe( + false, + ); + }); + + it('should reject non-existent paths', () => { + const filePath = path.join(symlinkDir, 'does-not-exist.txt'); + + const workspaceContext = new WorkspaceContext(cwd); + + expect(workspaceContext.isPathWithinWorkspace(filePath)).toBe(false); + }); + + it('should reject non-existent deep paths', () => { + const filePath = path.join(symlinkDir, 'deep', 'does-not-exist.txt'); + + const workspaceContext = new WorkspaceContext(cwd); + + expect(workspaceContext.isPathWithinWorkspace(filePath)).toBe(false); + }); + + it('should reject partially non-existent deep paths', () => { + const deepDir = path.join(symlinkDir, 'deep'); + fs.mkdirSync(deepDir, { recursive: true }); + const filePath = path.join(deepDir, 'does-not-exist.txt'); + + const workspaceContext = new WorkspaceContext(cwd); + + expect(workspaceContext.isPathWithinWorkspace(filePath)).toBe(false); + }); + }); + + it('should reject symbolic file links outside the workspace', () => { + const realFile = path.join(tempDir, 'real-file.txt'); + fs.writeFileSync(realFile, 'content'); + + const symlinkFile = path.join(cwd, 'symlink-to-real-file'); + fs.symlinkSync(realFile, symlinkFile, 'file'); + + const workspaceContext = new WorkspaceContext(cwd); + + expect(workspaceContext.isPathWithinWorkspace(symlinkFile)).toBe(false); + }); + + it('should reject non-existent symbolic file links outside the workspace', () => { + const realFile = path.join(tempDir, 'real-file.txt'); + + const symlinkFile = path.join(cwd, 'symlink-to-real-file'); + fs.symlinkSync(realFile, symlinkFile, 'file'); + + const workspaceContext = new WorkspaceContext(cwd); + + expect(workspaceContext.isPathWithinWorkspace(symlinkFile)).toBe(false); + }); + + it('should handle circular symlinks gracefully', () => { + const workspaceContext = new WorkspaceContext(cwd); + const linkA = path.join(cwd, 'link-a'); + const linkB = path.join(cwd, 'link-b'); + // Create a circular dependency: linkA -> linkB -> linkA + fs.symlinkSync(linkB, linkA, 'dir'); + fs.symlinkSync(linkA, linkB, 'dir'); + + // fs.realpathSync should throw ELOOP, and isPathWithinWorkspace should + // handle it gracefully and return false. + expect(workspaceContext.isPathWithinWorkspace(linkA)).toBe(false); + expect(workspaceContext.isPathWithinWorkspace(linkB)).toBe(false); + }); + }); }); describe('getDirectories', () => { it('should return a copy of directories array', () => { - workspaceContext = new WorkspaceContext(mockCwd); + const workspaceContext = new WorkspaceContext(cwd); const dirs1 = workspaceContext.getDirectories(); const dirs2 = workspaceContext.getDirectories(); - expect(dirs1).not.toBe(dirs2); // Different array instances - expect(dirs1).toEqual(dirs2); // Same content - }); - }); - - describe('symbolic link security', () => { - beforeEach(() => { - workspaceContext = new WorkspaceContext(mockCwd); - }); - - it('should follow symlinks but validate resolved path', () => { - const symlinkInsideWorkspace = path.join(mockCwd, 'link-to-subdir'); - const resolvedInsideWorkspace = path.join(mockCwd, 'subdir'); - - vi.mocked(fs.existsSync).mockReturnValue(true); - vi.mocked(fs.realpathSync).mockImplementation((p) => { - if (p === symlinkInsideWorkspace) { - return resolvedInsideWorkspace; - } - return p.toString(); - }); - - expect( - workspaceContext.isPathWithinWorkspace(symlinkInsideWorkspace), - ).toBe(true); - }); - - it('should prevent sandbox escape via symlinks', () => { - const symlinkEscape = path.join(mockCwd, 'escape-link'); - const resolvedOutside = path.resolve(mockCwd, '..', 'outside-file'); - - vi.mocked(fs.existsSync).mockImplementation((p) => { - const pathStr = p.toString(); - return ( - pathStr === symlinkEscape || - pathStr === resolvedOutside || - pathStr === mockCwd - ); - }); - vi.mocked(fs.realpathSync).mockImplementation((p) => { - if (p.toString() === symlinkEscape) { - return resolvedOutside; - } - return p.toString(); - }); - vi.mocked(fs.statSync).mockImplementation( - (p) => - ({ - isDirectory: () => p.toString() !== resolvedOutside, - }) as fs.Stats, - ); - - workspaceContext = new WorkspaceContext(mockCwd); - expect(workspaceContext.isPathWithinWorkspace(symlinkEscape)).toBe(false); - }); - - it('should handle circular symlinks', () => { - const circularLink = path.join(mockCwd, 'circular'); - - vi.mocked(fs.existsSync).mockReturnValue(true); - vi.mocked(fs.realpathSync).mockImplementation(() => { - throw new Error('ELOOP: too many symbolic links encountered'); - }); - - // Should handle the error gracefully - expect(workspaceContext.isPathWithinWorkspace(circularLink)).toBe(false); + expect(dirs1).not.toBe(dirs2); + expect(dirs1).toEqual(dirs2); }); }); }); diff --git a/packages/core/src/utils/workspaceContext.ts b/packages/core/src/utils/workspaceContext.ts index efbc8a4c..be99a83c 100644 --- a/packages/core/src/utils/workspaceContext.ts +++ b/packages/core/src/utils/workspaceContext.ts @@ -4,6 +4,7 @@ * SPDX-License-Identifier: Apache-2.0 */ +import { isNodeError } from '../utils/errors.js'; import * as fs from 'fs'; import * as path from 'path'; @@ -13,26 +14,21 @@ import * as path from 'path'; * in a single session. */ export class WorkspaceContext { - private directories: Set; - + private directories = new Set(); private initialDirectories: Set; /** * Creates a new WorkspaceContext with the given initial directory and optional additional directories. - * @param initialDirectory The initial working directory (usually cwd) + * @param directory The initial working directory (usually cwd) * @param additionalDirectories Optional array of additional directories to include */ - constructor(initialDirectory: string, additionalDirectories: string[] = []) { - this.directories = new Set(); - this.initialDirectories = new Set(); - - this.addDirectoryInternal(initialDirectory); - this.addInitialDirectoryInternal(initialDirectory); - - for (const dir of additionalDirectories) { - this.addDirectoryInternal(dir); - this.addInitialDirectoryInternal(dir); + constructor(directory: string, additionalDirectories: string[] = []) { + this.addDirectory(directory); + for (const additionalDirectory of additionalDirectories) { + this.addDirectory(additionalDirectory); } + + this.initialDirectories = new Set(this.directories); } /** @@ -41,16 +37,13 @@ export class WorkspaceContext { * @param basePath Optional base path for resolving relative paths (defaults to cwd) */ addDirectory(directory: string, basePath: string = process.cwd()): void { - this.addDirectoryInternal(directory, basePath); + this.directories.add(this.resolveAndValidateDir(directory, basePath)); } - /** - * Internal method to add a directory with validation. - */ - private addDirectoryInternal( + private resolveAndValidateDir( directory: string, basePath: string = process.cwd(), - ): void { + ): string { const absolutePath = path.isAbsolute(directory) ? directory : path.resolve(basePath, directory); @@ -58,47 +51,12 @@ export class WorkspaceContext { if (!fs.existsSync(absolutePath)) { throw new Error(`Directory does not exist: ${absolutePath}`); } - const stats = fs.statSync(absolutePath); if (!stats.isDirectory()) { throw new Error(`Path is not a directory: ${absolutePath}`); } - let realPath: string; - try { - realPath = fs.realpathSync(absolutePath); - } catch (_error) { - throw new Error(`Failed to resolve path: ${absolutePath}`); - } - - this.directories.add(realPath); - } - - private addInitialDirectoryInternal( - directory: string, - basePath: string = process.cwd(), - ): void { - const absolutePath = path.isAbsolute(directory) - ? directory - : path.resolve(basePath, directory); - - if (!fs.existsSync(absolutePath)) { - throw new Error(`Directory does not exist: ${absolutePath}`); - } - - const stats = fs.statSync(absolutePath); - if (!stats.isDirectory()) { - throw new Error(`Path is not a directory: ${absolutePath}`); - } - - let realPath: string; - try { - realPath = fs.realpathSync(absolutePath); - } catch (_error) { - throw new Error(`Failed to resolve path: ${absolutePath}`); - } - - this.initialDirectories.add(realPath); + return fs.realpathSync(absolutePath); } /** @@ -116,7 +74,7 @@ export class WorkspaceContext { setDirectories(directories: readonly string[]): void { this.directories.clear(); for (const dir of directories) { - this.addDirectoryInternal(dir); + this.addDirectory(dir); } } @@ -127,29 +85,43 @@ export class WorkspaceContext { */ isPathWithinWorkspace(pathToCheck: string): boolean { try { - const absolutePath = path.resolve(pathToCheck); - - let resolvedPath = absolutePath; - if (fs.existsSync(absolutePath)) { - try { - resolvedPath = fs.realpathSync(absolutePath); - } catch (_error) { - return false; - } - } + const fullyResolvedPath = this.fullyResolvedPath(pathToCheck); for (const dir of this.directories) { - if (this.isPathWithinRoot(resolvedPath, dir)) { + if (this.isPathWithinRoot(fullyResolvedPath, dir)) { return true; } } - return false; } catch (_error) { return false; } } + /** + * Fully resolves a path, including symbolic links. + * If the path does not exist, it returns the fully resolved path as it would be + * if it did exist. + */ + private fullyResolvedPath(pathToCheck: string): string { + try { + return fs.realpathSync(pathToCheck); + } catch (e: unknown) { + if ( + isNodeError(e) && + e.code === 'ENOENT' && + e.path && + // realpathSync does not set e.path correctly for symlinks to + // non-existent files. + !this.isFileSymlink(e.path) + ) { + // If it doesn't exist, e.path contains the fully resolved path. + return e.path; + } + throw e; + } + } + /** * Checks if a path is within a given root directory. * @param pathToCheck The absolute path to check @@ -167,4 +139,15 @@ export class WorkspaceContext { !path.isAbsolute(relative) ); } + + /** + * Checks if a file path is a symbolic link that points to a file. + */ + private isFileSymlink(filePath: string): boolean { + try { + return !fs.readlinkSync(filePath).endsWith('/'); + } catch (_error) { + return false; + } + } }