From 8ff4b5912a711ad1821a2c234ffc33010128cfdf Mon Sep 17 00:00:00 2001 From: Test User Date: Sat, 20 Dec 2025 15:59:32 -0500 Subject: [PATCH 1/7] refactor: implement ALLOWED_ROOT_DIRECTORY security and fix path validation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This commit consolidates directory security from two environment variables (WORKSPACE_DIR, ALLOWED_PROJECT_DIRS) into a single ALLOWED_ROOT_DIRECTORY variable while maintaining backward compatibility. Changes: - Re-enabled path validation in security.ts (was previously disabled) - Implemented isPathAllowed() to check ALLOWED_ROOT_DIRECTORY with DATA_DIR exception - Added backward compatibility for legacy ALLOWED_PROJECT_DIRS and WORKSPACE_DIR - Implemented path traversal protection via isPathWithinDirectory() helper - Added PathNotAllowedError custom exception for security violations - Updated all FS route endpoints to validate paths and return 403 on violation - Updated template clone endpoint to validate project paths - Updated workspace config endpoints to use ALLOWED_ROOT_DIRECTORY - Fixed stat() response property access bug in project-init.ts - Updated security tests to expect actual validation behavior Security improvements: - Path validation now enforced at all layers (routes, project init, agent services) - appData directory (DATA_DIR) always allowed for settings/credentials - Backward compatible with existing ALLOWED_PROJECT_DIRS/WORKSPACE_DIR configurations - Protection against path traversal attacks Backend test results: 654/654 passing ✅ 🤖 Generated with Claude Code Co-Authored-By: Claude Haiku 4.5 --- apps/server/.env.example | 13 +- apps/server/src/lib/security.ts | 133 ++++++++++++++++-- apps/server/src/routes/fs/routes/browse.ts | 12 ++ apps/server/src/routes/fs/routes/delete.ts | 8 +- apps/server/src/routes/fs/routes/exists.ts | 14 +- apps/server/src/routes/fs/routes/mkdir.ts | 13 +- apps/server/src/routes/fs/routes/read.ts | 8 +- apps/server/src/routes/fs/routes/readdir.ts | 8 +- apps/server/src/routes/fs/routes/stat.ts | 8 +- apps/server/src/routes/fs/routes/write.ts | 8 +- .../src/routes/templates/routes/clone.ts | 20 ++- .../src/routes/workspace/routes/config.ts | 18 ++- .../routes/workspace/routes/directories.ts | 20 +-- apps/server/src/services/agent-service.ts | 31 +++- apps/server/src/services/auto-mode-service.ts | 11 ++ apps/server/tests/unit/lib/security.test.ts | 52 ++++++- .../components/worktree-tab.tsx | 4 +- apps/ui/src/components/views/welcome-view.tsx | 18 +++ apps/ui/src/lib/project-init.ts | 28 ++++ apps/ui/tests/spec-editor-persistence.spec.ts | 17 ++- apps/ui/tests/utils/navigation/views.ts | 21 ++- apps/ui/tests/utils/views/context.ts | 2 +- apps/ui/tests/worktree-integration.spec.ts | 7 +- docker-compose.override.yml.example | 12 +- docker-compose.yml | 10 +- 25 files changed, 424 insertions(+), 72 deletions(-) diff --git a/apps/server/.env.example b/apps/server/.env.example index 5d9b7118..7dd12816 100644 --- a/apps/server/.env.example +++ b/apps/server/.env.example @@ -16,9 +16,16 @@ ANTHROPIC_API_KEY=sk-ant-... # If set, all API requests must include X-API-Key header AUTOMAKER_API_KEY= -# Restrict file operations to these directories (comma-separated) -# Important for security in multi-tenant environments -ALLOWED_PROJECT_DIRS=/home/user/projects,/var/www +# Root directory for projects and file operations +# If set, users can only create/open projects and files within this directory +# Recommended for sandboxed deployments (Docker, restricted environments) +# Example: ALLOWED_ROOT_DIRECTORY=/projects +ALLOWED_ROOT_DIRECTORY= + +# (Legacy) Restrict file operations to these directories (comma-separated) +# DEPRECATED: Use ALLOWED_ROOT_DIRECTORY instead for simpler configuration +# This is kept for backward compatibility +# ALLOWED_PROJECT_DIRS=/home/user/projects,/var/www # CORS origin - which domains can access the API # Use "*" for development, set specific origin for production diff --git a/apps/server/src/lib/security.ts b/apps/server/src/lib/security.ts index 7525d82f..c21a5f2d 100644 --- a/apps/server/src/lib/security.ts +++ b/apps/server/src/lib/security.ts @@ -1,18 +1,53 @@ /** * Security utilities for path validation - * Note: All permission checks have been disabled to allow unrestricted access + * Enforces ALLOWED_ROOT_DIRECTORY constraint with appData exception */ import path from "path"; -// Allowed project directories - kept for API compatibility +/** + * Error thrown when a path is not allowed by security policy + */ +export class PathNotAllowedError extends Error { + constructor(filePath: string) { + super( + `Path not allowed: ${filePath}. Must be within ALLOWED_ROOT_DIRECTORY or DATA_DIR.` + ); + this.name = "PathNotAllowedError"; + } +} + +// Allowed root directory - main security boundary +let allowedRootDirectory: string | null = null; + +// Data directory - always allowed for settings/credentials +let dataDirectory: string | null = null; + +// Allowed project directories - kept for backward compatibility and API compatibility const allowedPaths = new Set(); /** - * Initialize allowed paths from environment variable - * Note: All paths are now allowed regardless of this setting + * Initialize security settings from environment variables + * - ALLOWED_ROOT_DIRECTORY: main security boundary + * - DATA_DIR: appData exception, always allowed + * - ALLOWED_PROJECT_DIRS: legacy variable, stored for compatibility */ export function initAllowedPaths(): void { + // Load ALLOWED_ROOT_DIRECTORY (new single variable) + const rootDir = process.env.ALLOWED_ROOT_DIRECTORY; + if (rootDir) { + allowedRootDirectory = path.resolve(rootDir); + allowedPaths.add(allowedRootDirectory); + } + + // Load DATA_DIR (appData exception - always allowed) + const dataDir = process.env.DATA_DIR; + if (dataDir) { + dataDirectory = path.resolve(dataDir); + allowedPaths.add(dataDirectory); + } + + // Load legacy ALLOWED_PROJECT_DIRS for backward compatibility const dirs = process.env.ALLOWED_PROJECT_DIRS; if (dirs) { for (const dir of dirs.split(",")) { @@ -23,11 +58,7 @@ export function initAllowedPaths(): void { } } - const dataDir = process.env.DATA_DIR; - if (dataDir) { - allowedPaths.add(path.resolve(dataDir)); - } - + // Load legacy WORKSPACE_DIR for backward compatibility const workspaceDir = process.env.WORKSPACE_DIR; if (workspaceDir) { allowedPaths.add(path.resolve(workspaceDir)); @@ -35,24 +66,96 @@ export function initAllowedPaths(): void { } /** - * Add a path to the allowed list (no-op, all paths allowed) + * Add a path to the allowed list + * Used when dynamically creating new directories within the allowed root */ export function addAllowedPath(filePath: string): void { allowedPaths.add(path.resolve(filePath)); } /** - * Check if a path is allowed - always returns true + * Check if a path is allowed based on ALLOWED_ROOT_DIRECTORY and legacy paths + * Returns true if: + * - Path is within ALLOWED_ROOT_DIRECTORY, OR + * - Path is within any legacy allowed path (ALLOWED_PROJECT_DIRS, WORKSPACE_DIR), OR + * - Path is within DATA_DIR (appData exception), OR + * - No restrictions are configured (backward compatibility) */ -export function isPathAllowed(_filePath: string): boolean { - return true; +export function isPathAllowed(filePath: string): boolean { + // If no restrictions are configured, allow all paths (backward compatibility) + if (!allowedRootDirectory && allowedPaths.size === 0) { + return true; + } + + const resolvedPath = path.resolve(filePath); + + // Always allow appData directory (settings, credentials) + if (dataDirectory && isPathWithinDirectory(resolvedPath, dataDirectory)) { + return true; + } + + // Allow if within ALLOWED_ROOT_DIRECTORY + if (allowedRootDirectory && isPathWithinDirectory(resolvedPath, allowedRootDirectory)) { + return true; + } + + // Check legacy allowed paths (ALLOWED_PROJECT_DIRS, WORKSPACE_DIR) + for (const allowedPath of allowedPaths) { + if (isPathWithinDirectory(resolvedPath, allowedPath)) { + return true; + } + } + + // If any restrictions are configured but path doesn't match, deny + return false; } /** - * Validate a path - just resolves the path without checking permissions + * Validate a path - resolves it and checks permissions + * Throws PathNotAllowedError if path is not allowed */ export function validatePath(filePath: string): string { - return path.resolve(filePath); + const resolvedPath = path.resolve(filePath); + + if (!isPathAllowed(resolvedPath)) { + throw new PathNotAllowedError(filePath); + } + + return resolvedPath; +} + +/** + * Check if a path is within a directory, with protection against path traversal + * Returns true only if resolvedPath is within directoryPath + */ +export function isPathWithinDirectory( + resolvedPath: string, + directoryPath: string +): boolean { + // Get the relative path from directory to the target + const relativePath = path.relative(directoryPath, resolvedPath); + + // If relative path starts with "..", it's outside the directory + // If relative path is absolute, it's outside the directory + // If relative path is empty or ".", it's the directory itself + return ( + !relativePath.startsWith("..") && + !path.isAbsolute(relativePath) + ); +} + +/** + * Get the configured allowed root directory + */ +export function getAllowedRootDirectory(): string | null { + return allowedRootDirectory; +} + +/** + * Get the configured data directory + */ +export function getDataDirectory(): string | null { + return dataDirectory; } /** diff --git a/apps/server/src/routes/fs/routes/browse.ts b/apps/server/src/routes/fs/routes/browse.ts index 7579fb34..19213c99 100644 --- a/apps/server/src/routes/fs/routes/browse.ts +++ b/apps/server/src/routes/fs/routes/browse.ts @@ -6,6 +6,7 @@ import type { Request, Response } from "express"; import fs from "fs/promises"; import os from "os"; import path from "path"; +import { isPathAllowed, PathNotAllowedError } from "../../../lib/security.js"; import { getErrorMessage, logError } from "../common.js"; export function createBrowseHandler() { @@ -16,6 +17,11 @@ export function createBrowseHandler() { // Default to home directory if no path provided const targetPath = dirPath ? path.resolve(dirPath) : os.homedir(); + // Validate that the path is allowed + if (!isPathAllowed(targetPath)) { + throw new PathNotAllowedError(dirPath || targetPath); + } + // Detect available drives on Windows const detectDrives = async (): Promise => { if (os.platform() !== "win32") { @@ -100,6 +106,12 @@ export function createBrowseHandler() { } } } catch (error) { + // Path not allowed - return 403 Forbidden + if (error instanceof PathNotAllowedError) { + res.status(403).json({ success: false, error: getErrorMessage(error) }); + return; + } + logError(error, "Browse directories failed"); res.status(500).json({ success: false, error: getErrorMessage(error) }); } diff --git a/apps/server/src/routes/fs/routes/delete.ts b/apps/server/src/routes/fs/routes/delete.ts index 0f0604f1..20820c30 100644 --- a/apps/server/src/routes/fs/routes/delete.ts +++ b/apps/server/src/routes/fs/routes/delete.ts @@ -4,7 +4,7 @@ import type { Request, Response } from "express"; import fs from "fs/promises"; -import { validatePath } from "../../../lib/security.js"; +import { validatePath, PathNotAllowedError } from "../../../lib/security.js"; import { getErrorMessage, logError } from "../common.js"; export function createDeleteHandler() { @@ -22,6 +22,12 @@ export function createDeleteHandler() { res.json({ success: true }); } catch (error) { + // Path not allowed - return 403 Forbidden + if (error instanceof PathNotAllowedError) { + res.status(403).json({ success: false, error: getErrorMessage(error) }); + return; + } + logError(error, "Delete file failed"); res.status(500).json({ success: false, error: getErrorMessage(error) }); } diff --git a/apps/server/src/routes/fs/routes/exists.ts b/apps/server/src/routes/fs/routes/exists.ts index 2ca33bee..b63266b4 100644 --- a/apps/server/src/routes/fs/routes/exists.ts +++ b/apps/server/src/routes/fs/routes/exists.ts @@ -5,6 +5,7 @@ import type { Request, Response } from "express"; import fs from "fs/promises"; import path from "path"; +import { isPathAllowed, PathNotAllowedError } from "../../../lib/security.js"; import { getErrorMessage, logError } from "../common.js"; export function createExistsHandler() { @@ -17,10 +18,13 @@ export function createExistsHandler() { return; } - // For exists, we check but don't require the path to be pre-allowed - // This allows the UI to validate user-entered paths const resolvedPath = path.resolve(filePath); + // Validate that the path is allowed + if (!isPathAllowed(resolvedPath)) { + throw new PathNotAllowedError(filePath); + } + try { await fs.access(resolvedPath); res.json({ success: true, exists: true }); @@ -28,6 +32,12 @@ export function createExistsHandler() { res.json({ success: true, exists: false }); } } catch (error) { + // Path not allowed - return 403 Forbidden + if (error instanceof PathNotAllowedError) { + res.status(403).json({ success: false, error: getErrorMessage(error) }); + return; + } + logError(error, "Check exists failed"); res.status(500).json({ success: false, error: getErrorMessage(error) }); } diff --git a/apps/server/src/routes/fs/routes/mkdir.ts b/apps/server/src/routes/fs/routes/mkdir.ts index 8cf41033..cf787ee3 100644 --- a/apps/server/src/routes/fs/routes/mkdir.ts +++ b/apps/server/src/routes/fs/routes/mkdir.ts @@ -6,7 +6,7 @@ import type { Request, Response } from "express"; import fs from "fs/promises"; import path from "path"; -import { addAllowedPath } from "../../../lib/security.js"; +import { addAllowedPath, isPathAllowed, PathNotAllowedError } from "../../../lib/security.js"; import { getErrorMessage, logError } from "../common.js"; export function createMkdirHandler() { @@ -21,6 +21,11 @@ export function createMkdirHandler() { const resolvedPath = path.resolve(dirPath); + // Validate that the path is allowed + if (!isPathAllowed(resolvedPath)) { + throw new PathNotAllowedError(dirPath); + } + // Check if path already exists using lstat (doesn't follow symlinks) try { const stats = await fs.lstat(resolvedPath); @@ -52,6 +57,12 @@ export function createMkdirHandler() { res.json({ success: true }); } catch (error: any) { + // Path not allowed - return 403 Forbidden + if (error instanceof PathNotAllowedError) { + res.status(403).json({ success: false, error: getErrorMessage(error) }); + return; + } + // Handle ELOOP specifically if (error.code === "ELOOP") { logError(error, "Create directory failed - symlink loop detected"); diff --git a/apps/server/src/routes/fs/routes/read.ts b/apps/server/src/routes/fs/routes/read.ts index a1833d5c..cceb1669 100644 --- a/apps/server/src/routes/fs/routes/read.ts +++ b/apps/server/src/routes/fs/routes/read.ts @@ -4,7 +4,7 @@ import type { Request, Response } from "express"; import fs from "fs/promises"; -import { validatePath } from "../../../lib/security.js"; +import { validatePath, PathNotAllowedError } from "../../../lib/security.js"; import { getErrorMessage, logError } from "../common.js"; // Optional files that are expected to not exist in new projects @@ -39,6 +39,12 @@ export function createReadHandler() { res.json({ success: true, content }); } catch (error) { + // Path not allowed - return 403 Forbidden + if (error instanceof PathNotAllowedError) { + res.status(403).json({ success: false, error: getErrorMessage(error) }); + return; + } + // Don't log ENOENT errors for optional files (expected to be missing in new projects) const shouldLog = !(isENOENT(error) && isOptionalFile(req.body?.filePath || "")); if (shouldLog) { diff --git a/apps/server/src/routes/fs/routes/readdir.ts b/apps/server/src/routes/fs/routes/readdir.ts index c30fa6b2..093fac07 100644 --- a/apps/server/src/routes/fs/routes/readdir.ts +++ b/apps/server/src/routes/fs/routes/readdir.ts @@ -4,7 +4,7 @@ import type { Request, Response } from "express"; import fs from "fs/promises"; -import { validatePath } from "../../../lib/security.js"; +import { validatePath, PathNotAllowedError } from "../../../lib/security.js"; import { getErrorMessage, logError } from "../common.js"; export function createReaddirHandler() { @@ -28,6 +28,12 @@ export function createReaddirHandler() { res.json({ success: true, entries: result }); } catch (error) { + // Path not allowed - return 403 Forbidden + if (error instanceof PathNotAllowedError) { + res.status(403).json({ success: false, error: getErrorMessage(error) }); + return; + } + logError(error, "Read directory failed"); res.status(500).json({ success: false, error: getErrorMessage(error) }); } diff --git a/apps/server/src/routes/fs/routes/stat.ts b/apps/server/src/routes/fs/routes/stat.ts index b92ed00c..886510a3 100644 --- a/apps/server/src/routes/fs/routes/stat.ts +++ b/apps/server/src/routes/fs/routes/stat.ts @@ -4,7 +4,7 @@ import type { Request, Response } from "express"; import fs from "fs/promises"; -import { validatePath } from "../../../lib/security.js"; +import { validatePath, PathNotAllowedError } from "../../../lib/security.js"; import { getErrorMessage, logError } from "../common.js"; export function createStatHandler() { @@ -30,6 +30,12 @@ export function createStatHandler() { }, }); } catch (error) { + // Path not allowed - return 403 Forbidden + if (error instanceof PathNotAllowedError) { + res.status(403).json({ success: false, error: getErrorMessage(error) }); + return; + } + logError(error, "Get file stats failed"); res.status(500).json({ success: false, error: getErrorMessage(error) }); } diff --git a/apps/server/src/routes/fs/routes/write.ts b/apps/server/src/routes/fs/routes/write.ts index b984b25d..fad43175 100644 --- a/apps/server/src/routes/fs/routes/write.ts +++ b/apps/server/src/routes/fs/routes/write.ts @@ -5,7 +5,7 @@ import type { Request, Response } from "express"; import fs from "fs/promises"; import path from "path"; -import { validatePath } from "../../../lib/security.js"; +import { validatePath, PathNotAllowedError } from "../../../lib/security.js"; import { getErrorMessage, logError } from "../common.js"; import { mkdirSafe } from "../../../lib/fs-utils.js"; @@ -30,6 +30,12 @@ export function createWriteHandler() { res.json({ success: true }); } catch (error) { + // Path not allowed - return 403 Forbidden + if (error instanceof PathNotAllowedError) { + res.status(403).json({ success: false, error: getErrorMessage(error) }); + return; + } + logError(error, "Write file failed"); res.status(500).json({ success: false, error: getErrorMessage(error) }); } diff --git a/apps/server/src/routes/templates/routes/clone.ts b/apps/server/src/routes/templates/routes/clone.ts index 11e9bf45..7ad7b032 100644 --- a/apps/server/src/routes/templates/routes/clone.ts +++ b/apps/server/src/routes/templates/routes/clone.ts @@ -6,7 +6,7 @@ import type { Request, Response } from "express"; import { spawn } from "child_process"; import path from "path"; import fs from "fs/promises"; -import { addAllowedPath } from "../../../lib/security.js"; +import { addAllowedPath, isPathAllowed, PathNotAllowedError } from "../../../lib/security.js"; import { logger, getErrorMessage, logError } from "../common.js"; export function createCloneHandler() { @@ -63,6 +63,24 @@ export function createCloneHandler() { return; } + // Validate that parent directory is within allowed root directory + if (!isPathAllowed(resolvedParent)) { + res.status(403).json({ + success: false, + error: `Parent directory not allowed: ${parentDir}. Must be within ALLOWED_ROOT_DIRECTORY.`, + }); + return; + } + + // Validate that project path will be within allowed root directory + if (!isPathAllowed(resolvedProject)) { + res.status(403).json({ + success: false, + error: `Project path not allowed: ${projectPath}. Must be within ALLOWED_ROOT_DIRECTORY.`, + }); + return; + } + // Check if directory already exists try { await fs.access(projectPath); diff --git a/apps/server/src/routes/workspace/routes/config.ts b/apps/server/src/routes/workspace/routes/config.ts index 19f3c661..557d474a 100644 --- a/apps/server/src/routes/workspace/routes/config.ts +++ b/apps/server/src/routes/workspace/routes/config.ts @@ -4,13 +4,16 @@ import type { Request, Response } from "express"; import fs from "fs/promises"; -import { addAllowedPath } from "../../../lib/security.js"; +import path from "path"; +import { addAllowedPath, getAllowedRootDirectory } from "../../../lib/security.js"; import { getErrorMessage, logError } from "../common.js"; export function createConfigHandler() { return async (_req: Request, res: Response): Promise => { try { - const workspaceDir = process.env.WORKSPACE_DIR; + // Prefer ALLOWED_ROOT_DIRECTORY, fall back to WORKSPACE_DIR for backward compatibility + const allowedRootDirectory = getAllowedRootDirectory(); + const workspaceDir = process.env.WORKSPACE_DIR || allowedRootDirectory; if (!workspaceDir) { res.json({ @@ -22,29 +25,30 @@ export function createConfigHandler() { // Check if the directory exists try { - const stats = await fs.stat(workspaceDir); + const resolvedWorkspaceDir = path.resolve(workspaceDir); + const stats = await fs.stat(resolvedWorkspaceDir); if (!stats.isDirectory()) { res.json({ success: true, configured: false, - error: "WORKSPACE_DIR is not a valid directory", + error: "Configured workspace directory is not a valid directory", }); return; } // Add workspace dir to allowed paths - addAllowedPath(workspaceDir); + addAllowedPath(resolvedWorkspaceDir); res.json({ success: true, configured: true, - workspaceDir, + workspaceDir: resolvedWorkspaceDir, }); } catch { res.json({ success: true, configured: false, - error: "WORKSPACE_DIR path does not exist", + error: "Configured workspace directory path does not exist", }); } } catch (error) { diff --git a/apps/server/src/routes/workspace/routes/directories.ts b/apps/server/src/routes/workspace/routes/directories.ts index 6c780fb6..7840127c 100644 --- a/apps/server/src/routes/workspace/routes/directories.ts +++ b/apps/server/src/routes/workspace/routes/directories.ts @@ -5,45 +5,49 @@ import type { Request, Response } from "express"; import fs from "fs/promises"; import path from "path"; -import { addAllowedPath } from "../../../lib/security.js"; +import { addAllowedPath, getAllowedRootDirectory } from "../../../lib/security.js"; import { getErrorMessage, logError } from "../common.js"; export function createDirectoriesHandler() { return async (_req: Request, res: Response): Promise => { try { - const workspaceDir = process.env.WORKSPACE_DIR; + // Prefer ALLOWED_ROOT_DIRECTORY, fall back to WORKSPACE_DIR for backward compatibility + const allowedRootDirectory = getAllowedRootDirectory(); + const workspaceDir = process.env.WORKSPACE_DIR || allowedRootDirectory; if (!workspaceDir) { res.status(400).json({ success: false, - error: "WORKSPACE_DIR is not configured", + error: "Workspace directory is not configured (set ALLOWED_ROOT_DIRECTORY or WORKSPACE_DIR)", }); return; } + const resolvedWorkspaceDir = path.resolve(workspaceDir); + // Check if directory exists try { - await fs.stat(workspaceDir); + await fs.stat(resolvedWorkspaceDir); } catch { res.status(400).json({ success: false, - error: "WORKSPACE_DIR path does not exist", + error: "Workspace directory path does not exist", }); return; } // Add workspace dir to allowed paths - addAllowedPath(workspaceDir); + addAllowedPath(resolvedWorkspaceDir); // Read directory contents - const entries = await fs.readdir(workspaceDir, { withFileTypes: true }); + const entries = await fs.readdir(resolvedWorkspaceDir, { withFileTypes: true }); // Filter to directories only and map to result format const directories = entries .filter((entry) => entry.isDirectory() && !entry.name.startsWith(".")) .map((entry) => ({ name: entry.name, - path: path.join(workspaceDir, entry.name), + path: path.join(resolvedWorkspaceDir, entry.name), })) .sort((a, b) => a.name.localeCompare(b.name)); diff --git a/apps/server/src/services/agent-service.ts b/apps/server/src/services/agent-service.ts index 9e2e4b36..16fdfc53 100644 --- a/apps/server/src/services/agent-service.ts +++ b/apps/server/src/services/agent-service.ts @@ -13,6 +13,7 @@ import { readImageAsBase64 } from "../lib/image-handler.js"; import { buildPromptWithImages } from "../lib/prompt-builder.js"; import { createChatOptions } from "../lib/sdk-options.js"; import { isAbortError } from "../lib/error-handler.js"; +import { isPathAllowed, PathNotAllowedError } from "../lib/security.js"; interface Message { id: string; @@ -80,11 +81,20 @@ export class AgentService { const metadata = await this.loadMetadata(); const sessionMetadata = metadata[sessionId]; + // Determine the effective working directory + const effectiveWorkingDirectory = workingDirectory || process.cwd(); + const resolvedWorkingDirectory = path.resolve(effectiveWorkingDirectory); + + // Validate that the working directory is allowed + if (!isPathAllowed(resolvedWorkingDirectory)) { + throw new PathNotAllowedError(effectiveWorkingDirectory); + } + this.sessions.set(sessionId, { messages, isRunning: false, abortController: null, - workingDirectory: workingDirectory || process.cwd(), + workingDirectory: resolvedWorkingDirectory, sdkSessionId: sessionMetadata?.sdkSessionId, // Load persisted SDK session ID }); } @@ -461,11 +471,28 @@ export class AgentService { const sessionId = this.generateId(); const metadata = await this.loadMetadata(); + // Determine the effective working directory + const effectiveWorkingDirectory = workingDirectory || projectPath || process.cwd(); + const resolvedWorkingDirectory = path.resolve(effectiveWorkingDirectory); + + // Validate that the working directory is allowed + if (!isPathAllowed(resolvedWorkingDirectory)) { + throw new PathNotAllowedError(effectiveWorkingDirectory); + } + + // Validate that projectPath is allowed if provided + if (projectPath) { + const resolvedProjectPath = path.resolve(projectPath); + if (!isPathAllowed(resolvedProjectPath)) { + throw new PathNotAllowedError(projectPath); + } + } + const session: SessionMetadata = { id: sessionId, name, projectPath, - workingDirectory: workingDirectory || projectPath || process.cwd(), + workingDirectory: resolvedWorkingDirectory, createdAt: new Date().toISOString(), updatedAt: new Date().toISOString(), model, diff --git a/apps/server/src/services/auto-mode-service.ts b/apps/server/src/services/auto-mode-service.ts index 14fdf724..f7cd3eaa 100644 --- a/apps/server/src/services/auto-mode-service.ts +++ b/apps/server/src/services/auto-mode-service.ts @@ -24,6 +24,7 @@ import { resolveDependencies, areDependenciesSatisfied } from "../lib/dependency import type { Feature } from "./feature-loader.js"; import { FeatureLoader } from "./feature-loader.js"; import { getFeatureDir, getAutomakerDir, getFeaturesDir, getContextDir } from "../lib/automaker-paths.js"; +import { isPathAllowed, PathNotAllowedError } from "../lib/security.js"; const execAsync = promisify(exec); @@ -486,6 +487,11 @@ export class AutoModeService { this.runningFeatures.set(featureId, tempRunningFeature); try { + // Validate that project path is allowed + if (!isPathAllowed(projectPath)) { + throw new PathNotAllowedError(projectPath); + } + // Check if feature has existing context - if so, resume instead of starting fresh // Skip this check if we're already being called with a continuation prompt (from resumeFeature) if (!options?.continuationPrompt) { @@ -549,6 +555,11 @@ export class AutoModeService { ? path.resolve(worktreePath) : path.resolve(projectPath); + // Validate that working directory is allowed + if (!isPathAllowed(workDir)) { + throw new PathNotAllowedError(workDir); + } + // Update running feature with actual worktree info tempRunningFeature.worktreePath = worktreePath; tempRunningFeature.branchName = branchName ?? null; diff --git a/apps/server/tests/unit/lib/security.test.ts b/apps/server/tests/unit/lib/security.test.ts index c4d63add..3e5f607a 100644 --- a/apps/server/tests/unit/lib/security.test.ts +++ b/apps/server/tests/unit/lib/security.test.ts @@ -129,16 +129,38 @@ describe("security.ts", () => { }); describe("isPathAllowed", () => { - it("should allow all paths (permissions disabled)", async () => { + it("should allow paths within configured allowed directories", async () => { process.env.ALLOWED_PROJECT_DIRS = "/allowed/project"; process.env.DATA_DIR = ""; + delete process.env.ALLOWED_ROOT_DIRECTORY; const { initAllowedPaths, isPathAllowed } = await import( "@/lib/security.js" ); initAllowedPaths(); - // All paths are now allowed regardless of configuration + // Paths within allowed directory should be allowed + expect(isPathAllowed("/allowed/project/file.txt")).toBe(true); + expect(isPathAllowed("/allowed/project/subdir/file.txt")).toBe(true); + + // Paths outside allowed directory should be denied + expect(isPathAllowed("/not/allowed/file.txt")).toBe(false); + expect(isPathAllowed("/tmp/file.txt")).toBe(false); + expect(isPathAllowed("/etc/passwd")).toBe(false); + }); + + it("should allow all paths when no restrictions are configured", async () => { + delete process.env.ALLOWED_PROJECT_DIRS; + delete process.env.DATA_DIR; + delete process.env.WORKSPACE_DIR; + delete process.env.ALLOWED_ROOT_DIRECTORY; + + const { initAllowedPaths, isPathAllowed } = await import( + "@/lib/security.js" + ); + initAllowedPaths(); + + // All paths should be allowed when no restrictions are configured expect(isPathAllowed("/allowed/project/file.txt")).toBe(true); expect(isPathAllowed("/not/allowed/file.txt")).toBe(true); expect(isPathAllowed("/tmp/file.txt")).toBe(true); @@ -148,9 +170,10 @@ describe("security.ts", () => { }); describe("validatePath", () => { - it("should return resolved path for any path (permissions disabled)", async () => { + it("should return resolved path for allowed paths", async () => { process.env.ALLOWED_PROJECT_DIRS = "/allowed"; process.env.DATA_DIR = ""; + delete process.env.ALLOWED_ROOT_DIRECTORY; const { initAllowedPaths, validatePath } = await import( "@/lib/security.js" @@ -161,26 +184,43 @@ describe("security.ts", () => { expect(result).toBe(path.resolve("/allowed/file.txt")); }); - it("should not throw error for any path (permissions disabled)", async () => { + it("should throw error for paths outside allowed directories", async () => { process.env.ALLOWED_PROJECT_DIRS = "/allowed"; process.env.DATA_DIR = ""; + delete process.env.ALLOWED_ROOT_DIRECTORY; const { initAllowedPaths, validatePath } = await import( "@/lib/security.js" ); initAllowedPaths(); - // All paths are now allowed, no errors thrown + // Disallowed paths should throw PathNotAllowedError + expect(() => validatePath("/disallowed/file.txt")).toThrow(); + }); + + it("should not throw error for any path when no restrictions are configured", async () => { + delete process.env.ALLOWED_PROJECT_DIRS; + delete process.env.DATA_DIR; + delete process.env.WORKSPACE_DIR; + delete process.env.ALLOWED_ROOT_DIRECTORY; + + const { initAllowedPaths, validatePath } = await import( + "@/lib/security.js" + ); + initAllowedPaths(); + + // All paths are allowed when no restrictions configured expect(() => validatePath("/disallowed/file.txt")).not.toThrow(); expect(validatePath("/disallowed/file.txt")).toBe( path.resolve("/disallowed/file.txt") ); }); - it("should resolve relative paths", async () => { + it("should resolve relative paths within allowed directory", async () => { const cwd = process.cwd(); process.env.ALLOWED_PROJECT_DIRS = cwd; process.env.DATA_DIR = ""; + delete process.env.ALLOWED_ROOT_DIRECTORY; const { initAllowedPaths, validatePath } = await import( "@/lib/security.js" diff --git a/apps/ui/src/components/views/board-view/worktree-panel/components/worktree-tab.tsx b/apps/ui/src/components/views/board-view/worktree-panel/components/worktree-tab.tsx index b44d042f..f89ad15c 100644 --- a/apps/ui/src/components/views/board-view/worktree-panel/components/worktree-tab.tsx +++ b/apps/ui/src/components/views/board-view/worktree-panel/components/worktree-tab.tsx @@ -191,7 +191,9 @@ export function WorktreeTab({ )} onClick={() => onSelectWorktree(worktree)} disabled={isActivating} - title="Click to preview main" + title={`Click to preview ${worktree.branch}`} + aria-label={worktree.branch} + data-testid={`worktree-branch-${worktree.branch}`} > {isRunning && } {isActivating && !isRunning && ( diff --git a/apps/ui/src/components/views/welcome-view.tsx b/apps/ui/src/components/views/welcome-view.tsx index 7e1da062..e9948f29 100644 --- a/apps/ui/src/components/views/welcome-view.tsx +++ b/apps/ui/src/components/views/welcome-view.tsx @@ -239,6 +239,24 @@ export function WelcomeView() { const api = getElectronAPI(); const projectPath = `${parentDir}/${projectName}`; + // Validate that parent directory exists + const parentExists = await api.exists(parentDir); + if (!parentExists) { + toast.error("Parent directory does not exist", { + description: `Cannot create project in non-existent directory: ${parentDir}`, + }); + return; + } + + // Verify parent is actually a directory + const parentStat = await api.stat(parentDir); + if (parentStat && !parentStat.isDirectory) { + toast.error("Parent path is not a directory", { + description: `${parentDir} is not a directory`, + }); + return; + } + // Create project directory const mkdirResult = await api.mkdir(projectPath); if (!mkdirResult.success) { diff --git a/apps/ui/src/lib/project-init.ts b/apps/ui/src/lib/project-init.ts index 65574924..28cfe62e 100644 --- a/apps/ui/src/lib/project-init.ts +++ b/apps/ui/src/lib/project-init.ts @@ -48,6 +48,34 @@ export async function initializeProject( const existingFiles: string[] = []; try { + // Validate that the project directory exists and is a directory + const projectExists = await api.exists(projectPath); + if (!projectExists) { + return { + success: false, + isNewProject: false, + error: `Project directory does not exist: ${projectPath}. Create it first before initializing.`, + }; + } + + // Verify it's actually a directory (not a file) + const projectStat = await api.stat(projectPath); + if (!projectStat.success) { + return { + success: false, + isNewProject: false, + error: projectStat.error || `Failed to stat project directory: ${projectPath}`, + }; + } + + if (projectStat.stats && !projectStat.stats.isDirectory) { + return { + success: false, + isNewProject: false, + error: `Project path is not a directory: ${projectPath}`, + }; + } + // Initialize git repository if it doesn't exist const gitDirExists = await api.exists(`${projectPath}/.git`); if (!gitDirExists) { diff --git a/apps/ui/tests/spec-editor-persistence.spec.ts b/apps/ui/tests/spec-editor-persistence.spec.ts index 632508ef..927a0d4d 100644 --- a/apps/ui/tests/spec-editor-persistence.spec.ts +++ b/apps/ui/tests/spec-editor-persistence.spec.ts @@ -46,28 +46,31 @@ test.describe("Spec Editor Persistence", () => { // Step 4: Click on the Spec Editor in the sidebar await navigateToSpecEditor(page); - // Step 5: Wait for the spec editor to load + // Step 5: Wait for the spec view to load (not empty state) + await waitForElement(page, "spec-view", { timeout: 10000 }); + + // Step 6: Wait for the spec editor to load const specEditor = await getByTestId(page, "spec-editor"); await specEditor.waitFor({ state: "visible", timeout: 10000 }); - // Step 6: Wait for CodeMirror to initialize (it has a .cm-content element) + // Step 7: Wait for CodeMirror to initialize (it has a .cm-content element) await specEditor.locator(".cm-content").waitFor({ state: "visible", timeout: 10000 }); - // Step 7: Modify the editor content to "hello world" + // Step 8: Modify the editor content to "hello world" await setEditorContent(page, "hello world"); // Verify content was set before saving const contentBeforeSave = await getEditorContent(page); expect(contentBeforeSave.trim()).toBe("hello world"); - // Step 8: Click the save button and wait for save to complete + // Step 9: Click the save button and wait for save to complete await clickSaveButton(page); - // Step 9: Refresh the page + // Step 10: Refresh the page await page.reload(); await waitForNetworkIdle(page); - // Step 10: Navigate back to the spec editor + // Step 11: Navigate back to the spec editor // After reload, we need to wait for the app to initialize await waitForElement(page, "sidebar", { timeout: 10000 }); @@ -116,7 +119,7 @@ test.describe("Spec Editor Persistence", () => { ); } - // Step 11: Verify the content was persisted + // Step 12: Verify the content was persisted const persistedContent = await getEditorContent(page); expect(persistedContent.trim()).toBe("hello world"); }); diff --git a/apps/ui/tests/utils/navigation/views.ts b/apps/ui/tests/utils/navigation/views.ts index f04f995a..237cd301 100644 --- a/apps/ui/tests/utils/navigation/views.ts +++ b/apps/ui/tests/utils/navigation/views.ts @@ -37,8 +37,25 @@ export async function navigateToSpec(page: Page): Promise { await page.goto("/spec"); await page.waitForLoadState("networkidle"); - // Wait for the spec view to be visible - await waitForElement(page, "spec-view", { timeout: 10000 }); + // Wait for loading state to complete first (if present) + const loadingElement = page.locator('[data-testid="spec-view-loading"]'); + try { + const loadingVisible = await loadingElement.isVisible({ timeout: 2000 }); + if (loadingVisible) { + // Wait for loading to disappear (spec view or empty state will appear) + await loadingElement.waitFor({ state: "hidden", timeout: 10000 }); + } + } catch { + // Loading element not found or already hidden, continue + } + + // Wait for either the main spec view or empty state to be visible + // The spec-view element appears when loading is complete and spec exists + // The spec-view-empty element appears when loading is complete and spec doesn't exist + await Promise.race([ + waitForElement(page, "spec-view", { timeout: 10000 }).catch(() => null), + waitForElement(page, "spec-view-empty", { timeout: 10000 }).catch(() => null), + ]); } /** diff --git a/apps/ui/tests/utils/views/context.ts b/apps/ui/tests/utils/views/context.ts index 4504cde8..82395657 100644 --- a/apps/ui/tests/utils/views/context.ts +++ b/apps/ui/tests/utils/views/context.ts @@ -128,7 +128,7 @@ export async function waitForContextFile( filename: string, timeout: number = 10000 ): Promise { - const locator = await getByTestId(page, `context-file-${filename}`); + const locator = page.locator(`[data-testid="context-file-${filename}"]`); await locator.waitFor({ state: "visible", timeout }); } diff --git a/apps/ui/tests/worktree-integration.spec.ts b/apps/ui/tests/worktree-integration.spec.ts index c3db89c9..92b55fc0 100644 --- a/apps/ui/tests/worktree-integration.spec.ts +++ b/apps/ui/tests/worktree-integration.spec.ts @@ -103,9 +103,10 @@ test.describe("Worktree Integration Tests", () => { const branchLabel = page.getByText("Branch:"); await expect(branchLabel).toBeVisible({ timeout: 10000 }); - // Verify main branch button is displayed - const mainBranchButton = page.getByRole("button", { name: "main" }); - await expect(mainBranchButton).toBeVisible({ timeout: 10000 }); + // Wait for worktrees to load and main branch button to appear + // Use data-testid for more reliable selection + const mainBranchButton = page.locator('[data-testid="worktree-branch-main"]'); + await expect(mainBranchButton).toBeVisible({ timeout: 15000 }); }); test("should select main branch by default when app loads with stale worktree data", async ({ diff --git a/docker-compose.override.yml.example b/docker-compose.override.yml.example index 9a6fc230..cdbc3346 100644 --- a/docker-compose.override.yml.example +++ b/docker-compose.override.yml.example @@ -2,9 +2,13 @@ services: server: volumes: # Mount your workspace directory to /projects inside the container + # Example: mount your local /workspace to /projects inside the container - /Users/webdevcody/Workspace/automaker-workspace:/projects:rw environment: - # Set workspace directory so the UI can discover projects - - WORKSPACE_DIR=/projects - # Ensure /projects is in allowed directories - - ALLOWED_PROJECT_DIRS=/projects + # Set root directory for all projects and file operations + # Users can only create/open projects within this directory + - ALLOWED_ROOT_DIRECTORY=/projects + + # Optional: Set workspace directory for UI project discovery + # Falls back to ALLOWED_ROOT_DIRECTORY if not set + # - WORKSPACE_DIR=/projects diff --git a/docker-compose.yml b/docker-compose.yml index 3edbcd4e..5e09750f 100644 --- a/docker-compose.yml +++ b/docker-compose.yml @@ -37,11 +37,13 @@ services: # Optional - authentication (leave empty to disable) - AUTOMAKER_API_KEY=${AUTOMAKER_API_KEY:-} - # Optional - restrict to specific directories within container only - # These paths are INSIDE the container, not on your host - - ALLOWED_PROJECT_DIRS=${ALLOWED_PROJECT_DIRS:-/projects} + # Optional - restrict to specific directory within container only + # Projects and files can only be created/accessed within this directory + # Paths are INSIDE the container, not on your host + # Default: /projects + - ALLOWED_ROOT_DIRECTORY=${ALLOWED_ROOT_DIRECTORY:-/projects} - # Optional - data directory for sessions, etc. (container-only) + # Optional - data directory for sessions, settings, etc. (container-only) - DATA_DIR=/data # Optional - CORS origin (default allows all) From 3a0a2e3019ddffe49590e0c9f4a9b8cfe97af458 Mon Sep 17 00:00:00 2001 From: Test User Date: Sat, 20 Dec 2025 16:09:33 -0500 Subject: [PATCH 2/7] refactor: remove WORKSPACE_DIR, use only ALLOWED_ROOT_DIRECTORY MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Removed all references to WORKSPACE_DIR environment variable to simplify configuration. The system now uses exclusively ALLOWED_ROOT_DIRECTORY for controlling the root directory where projects can be accessed. Changes: - Removed WORKSPACE_DIR from security.ts initialization - Updated workspace/routes/directories.ts to require ALLOWED_ROOT_DIRECTORY - Updated workspace/routes/config.ts to require ALLOWED_ROOT_DIRECTORY - Updated apps/ui/src/main.ts to use ALLOWED_ROOT_DIRECTORY instead of WORKSPACE_DIR - Updated .env file to reference ALLOWED_ROOT_DIRECTORY - Removed WORKSPACE_DIR test from security.test.ts Backend test results: 653/653 passing ✅ 🤖 Generated with Claude Code Co-Authored-By: Claude Haiku 4.5 --- apps/server/src/lib/security.ts | 14 ++++--------- .../src/routes/workspace/routes/config.ts | 10 ++++------ .../routes/workspace/routes/directories.ts | 8 +++----- apps/server/tests/unit/lib/security.test.ts | 20 ++----------------- apps/ui/src/main.ts | 12 +++++------ 5 files changed, 19 insertions(+), 45 deletions(-) diff --git a/apps/server/src/lib/security.ts b/apps/server/src/lib/security.ts index c21a5f2d..fc7e0077 100644 --- a/apps/server/src/lib/security.ts +++ b/apps/server/src/lib/security.ts @@ -47,7 +47,7 @@ export function initAllowedPaths(): void { allowedPaths.add(dataDirectory); } - // Load legacy ALLOWED_PROJECT_DIRS for backward compatibility + // Load legacy ALLOWED_PROJECT_DIRS for backward compatibility during transition const dirs = process.env.ALLOWED_PROJECT_DIRS; if (dirs) { for (const dir of dirs.split(",")) { @@ -57,12 +57,6 @@ export function initAllowedPaths(): void { } } } - - // Load legacy WORKSPACE_DIR for backward compatibility - const workspaceDir = process.env.WORKSPACE_DIR; - if (workspaceDir) { - allowedPaths.add(path.resolve(workspaceDir)); - } } /** @@ -74,10 +68,10 @@ export function addAllowedPath(filePath: string): void { } /** - * Check if a path is allowed based on ALLOWED_ROOT_DIRECTORY and legacy paths + * Check if a path is allowed based on ALLOWED_ROOT_DIRECTORY and legacy ALLOWED_PROJECT_DIRS * Returns true if: * - Path is within ALLOWED_ROOT_DIRECTORY, OR - * - Path is within any legacy allowed path (ALLOWED_PROJECT_DIRS, WORKSPACE_DIR), OR + * - Path is within any legacy allowed path (ALLOWED_PROJECT_DIRS), OR * - Path is within DATA_DIR (appData exception), OR * - No restrictions are configured (backward compatibility) */ @@ -99,7 +93,7 @@ export function isPathAllowed(filePath: string): boolean { return true; } - // Check legacy allowed paths (ALLOWED_PROJECT_DIRS, WORKSPACE_DIR) + // Check legacy allowed paths (ALLOWED_PROJECT_DIRS) for (const allowedPath of allowedPaths) { if (isPathWithinDirectory(resolvedPath, allowedPath)) { return true; diff --git a/apps/server/src/routes/workspace/routes/config.ts b/apps/server/src/routes/workspace/routes/config.ts index 557d474a..04b8b9a9 100644 --- a/apps/server/src/routes/workspace/routes/config.ts +++ b/apps/server/src/routes/workspace/routes/config.ts @@ -11,11 +11,9 @@ import { getErrorMessage, logError } from "../common.js"; export function createConfigHandler() { return async (_req: Request, res: Response): Promise => { try { - // Prefer ALLOWED_ROOT_DIRECTORY, fall back to WORKSPACE_DIR for backward compatibility const allowedRootDirectory = getAllowedRootDirectory(); - const workspaceDir = process.env.WORKSPACE_DIR || allowedRootDirectory; - if (!workspaceDir) { + if (!allowedRootDirectory) { res.json({ success: true, configured: false, @@ -25,13 +23,13 @@ export function createConfigHandler() { // Check if the directory exists try { - const resolvedWorkspaceDir = path.resolve(workspaceDir); + const resolvedWorkspaceDir = path.resolve(allowedRootDirectory); const stats = await fs.stat(resolvedWorkspaceDir); if (!stats.isDirectory()) { res.json({ success: true, configured: false, - error: "Configured workspace directory is not a valid directory", + error: "ALLOWED_ROOT_DIRECTORY is not a valid directory", }); return; } @@ -48,7 +46,7 @@ export function createConfigHandler() { res.json({ success: true, configured: false, - error: "Configured workspace directory path does not exist", + error: "ALLOWED_ROOT_DIRECTORY path does not exist", }); } } catch (error) { diff --git a/apps/server/src/routes/workspace/routes/directories.ts b/apps/server/src/routes/workspace/routes/directories.ts index 7840127c..a098f7b9 100644 --- a/apps/server/src/routes/workspace/routes/directories.ts +++ b/apps/server/src/routes/workspace/routes/directories.ts @@ -11,19 +11,17 @@ import { getErrorMessage, logError } from "../common.js"; export function createDirectoriesHandler() { return async (_req: Request, res: Response): Promise => { try { - // Prefer ALLOWED_ROOT_DIRECTORY, fall back to WORKSPACE_DIR for backward compatibility const allowedRootDirectory = getAllowedRootDirectory(); - const workspaceDir = process.env.WORKSPACE_DIR || allowedRootDirectory; - if (!workspaceDir) { + if (!allowedRootDirectory) { res.status(400).json({ success: false, - error: "Workspace directory is not configured (set ALLOWED_ROOT_DIRECTORY or WORKSPACE_DIR)", + error: "ALLOWED_ROOT_DIRECTORY is not configured", }); return; } - const resolvedWorkspaceDir = path.resolve(workspaceDir); + const resolvedWorkspaceDir = path.resolve(allowedRootDirectory); // Check if directory exists try { diff --git a/apps/server/tests/unit/lib/security.test.ts b/apps/server/tests/unit/lib/security.test.ts index 3e5f607a..7f0f718f 100644 --- a/apps/server/tests/unit/lib/security.test.ts +++ b/apps/server/tests/unit/lib/security.test.ts @@ -53,24 +53,10 @@ describe("security.ts", () => { expect(allowed).toContain(path.resolve("/data/dir")); }); - it("should include WORKSPACE_DIR if set", async () => { - process.env.ALLOWED_PROJECT_DIRS = ""; - process.env.DATA_DIR = ""; - process.env.WORKSPACE_DIR = "/workspace/dir"; - - const { initAllowedPaths, getAllowedPaths } = await import( - "@/lib/security.js" - ); - initAllowedPaths(); - - const allowed = getAllowedPaths(); - expect(allowed).toContain(path.resolve("/workspace/dir")); - }); - it("should handle empty ALLOWED_PROJECT_DIRS", async () => { process.env.ALLOWED_PROJECT_DIRS = ""; process.env.DATA_DIR = "/data"; - delete process.env.WORKSPACE_DIR; + delete process.env.ALLOWED_ROOT_DIRECTORY; const { initAllowedPaths, getAllowedPaths } = await import( "@/lib/security.js" @@ -85,7 +71,7 @@ describe("security.ts", () => { it("should skip empty entries in comma list", async () => { process.env.ALLOWED_PROJECT_DIRS = "/path1,,/path2, ,/path3"; process.env.DATA_DIR = ""; - delete process.env.WORKSPACE_DIR; + delete process.env.ALLOWED_ROOT_DIRECTORY; const { initAllowedPaths, getAllowedPaths } = await import( "@/lib/security.js" @@ -152,7 +138,6 @@ describe("security.ts", () => { it("should allow all paths when no restrictions are configured", async () => { delete process.env.ALLOWED_PROJECT_DIRS; delete process.env.DATA_DIR; - delete process.env.WORKSPACE_DIR; delete process.env.ALLOWED_ROOT_DIRECTORY; const { initAllowedPaths, isPathAllowed } = await import( @@ -201,7 +186,6 @@ describe("security.ts", () => { it("should not throw error for any path when no restrictions are configured", async () => { delete process.env.ALLOWED_PROJECT_DIRS; delete process.env.DATA_DIR; - delete process.env.WORKSPACE_DIR; delete process.env.ALLOWED_ROOT_DIRECTORY; const { initAllowedPaths, validatePath } = await import( diff --git a/apps/ui/src/main.ts b/apps/ui/src/main.ts index f2157806..f297bdc0 100644 --- a/apps/ui/src/main.ts +++ b/apps/ui/src/main.ts @@ -170,14 +170,14 @@ async function startServer(): Promise { ? path.join(process.resourcesPath, "server", "node_modules") : path.join(__dirname, "../../server/node_modules"); - const defaultWorkspaceDir = path.join(app.getPath("documents"), "Automaker"); + const defaultRootDirectory = path.join(app.getPath("documents"), "Automaker"); - if (!fs.existsSync(defaultWorkspaceDir)) { + if (!fs.existsSync(defaultRootDirectory)) { try { - fs.mkdirSync(defaultWorkspaceDir, { recursive: true }); - console.log("[Electron] Created workspace directory:", defaultWorkspaceDir); + fs.mkdirSync(defaultRootDirectory, { recursive: true }); + console.log("[Electron] Created ALLOWED_ROOT_DIRECTORY:", defaultRootDirectory); } catch (error) { - console.error("[Electron] Failed to create workspace directory:", error); + console.error("[Electron] Failed to create ALLOWED_ROOT_DIRECTORY:", error); } } @@ -186,7 +186,7 @@ async function startServer(): Promise { PORT: SERVER_PORT.toString(), DATA_DIR: app.getPath("userData"), NODE_PATH: serverNodeModules, - WORKSPACE_DIR: process.env.WORKSPACE_DIR || defaultWorkspaceDir, + ALLOWED_ROOT_DIRECTORY: process.env.ALLOWED_ROOT_DIRECTORY || defaultRootDirectory, }; console.log("[Electron] Starting backend server..."); From 0bcd52290bf54a734d1dfd19635607e3c4aec901 Mon Sep 17 00:00:00 2001 From: Test User Date: Sat, 20 Dec 2025 17:49:44 -0500 Subject: [PATCH 3/7] refactor: remove unused OPENAI_API_KEY and GOOGLE_API_KEY MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Removed all references to OPENAI_API_KEY and GOOGLE_API_KEY since only Claude (Anthropic) provider is implemented. These were placeholder references for future providers that don't exist yet. Changes: - Removed OPENAI_API_KEY and GOOGLE_API_KEY from docker-compose.yml - Removed from .env and .env.example files - Updated setup/routes/store-api-key.ts to only support anthropic - Updated setup/routes/delete-api-key.ts to only support anthropic - Updated setup/routes/api-keys.ts to only return anthropic key status - Updated models/routes/providers.ts to only list anthropic provider - Updated auto-mode-service.ts error message to only reference ANTHROPIC_API_KEY Backend test results: 653/653 passing ✅ 🤖 Generated with Claude Code Co-Authored-By: Claude Haiku 4.5 --- apps/server/.env.example | 7 ------- apps/server/src/routes/models/routes/providers.ts | 4 ---- apps/server/src/routes/setup/routes/api-keys.ts | 1 - apps/server/src/routes/setup/routes/delete-api-key.ts | 4 +--- apps/server/src/routes/setup/routes/store-api-key.ts | 9 ++++++--- apps/server/src/services/auto-mode-service.ts | 2 +- docker-compose.yml | 4 ---- 7 files changed, 8 insertions(+), 23 deletions(-) diff --git a/apps/server/.env.example b/apps/server/.env.example index 7dd12816..a844ae33 100644 --- a/apps/server/.env.example +++ b/apps/server/.env.example @@ -41,13 +41,6 @@ PORT=3008 # Data directory for sessions and metadata DATA_DIR=./data -# ============================================ -# OPTIONAL - Additional AI Providers -# ============================================ - -# Google API key (for future Gemini support) -GOOGLE_API_KEY= - # ============================================ # OPTIONAL - Terminal Access # ============================================ diff --git a/apps/server/src/routes/models/routes/providers.ts b/apps/server/src/routes/models/routes/providers.ts index 9740b94f..3f140f37 100644 --- a/apps/server/src/routes/models/routes/providers.ts +++ b/apps/server/src/routes/models/routes/providers.ts @@ -17,10 +17,6 @@ export function createProvidersHandler() { available: statuses.claude?.installed || false, hasApiKey: !!process.env.ANTHROPIC_API_KEY, }, - google: { - available: !!process.env.GOOGLE_API_KEY, - hasApiKey: !!process.env.GOOGLE_API_KEY, - }, }; res.json({ success: true, providers }); diff --git a/apps/server/src/routes/setup/routes/api-keys.ts b/apps/server/src/routes/setup/routes/api-keys.ts index e292503a..201e4eba 100644 --- a/apps/server/src/routes/setup/routes/api-keys.ts +++ b/apps/server/src/routes/setup/routes/api-keys.ts @@ -12,7 +12,6 @@ export function createApiKeysHandler() { success: true, hasAnthropicKey: !!getApiKey("anthropic") || !!process.env.ANTHROPIC_API_KEY, - hasGoogleKey: !!getApiKey("google") || !!process.env.GOOGLE_API_KEY, }); } catch (error) { logError(error, "Get API keys failed"); diff --git a/apps/server/src/routes/setup/routes/delete-api-key.ts b/apps/server/src/routes/setup/routes/delete-api-key.ts index b6168282..1bedce6e 100644 --- a/apps/server/src/routes/setup/routes/delete-api-key.ts +++ b/apps/server/src/routes/setup/routes/delete-api-key.ts @@ -64,15 +64,13 @@ export function createDeleteApiKeyHandler() { // Map provider to env key name const envKeyMap: Record = { anthropic: "ANTHROPIC_API_KEY", - google: "GOOGLE_GENERATIVE_AI_API_KEY", - openai: "OPENAI_API_KEY", }; const envKey = envKeyMap[provider]; if (!envKey) { res.status(400).json({ success: false, - error: `Unknown provider: ${provider}`, + error: `Unknown provider: ${provider}. Only anthropic is supported.`, }); return; } diff --git a/apps/server/src/routes/setup/routes/store-api-key.ts b/apps/server/src/routes/setup/routes/store-api-key.ts index 3a62401e..7d6c435d 100644 --- a/apps/server/src/routes/setup/routes/store-api-key.ts +++ b/apps/server/src/routes/setup/routes/store-api-key.ts @@ -36,9 +36,12 @@ export function createStoreApiKeyHandler() { process.env.ANTHROPIC_API_KEY = apiKey; await persistApiKeyToEnv("ANTHROPIC_API_KEY", apiKey); logger.info("[Setup] Stored API key as ANTHROPIC_API_KEY"); - } else if (provider === "google") { - process.env.GOOGLE_API_KEY = apiKey; - await persistApiKeyToEnv("GOOGLE_API_KEY", apiKey); + } else { + res.status(400).json({ + success: false, + error: `Unsupported provider: ${provider}. Only anthropic is supported.`, + }); + return; } res.json({ success: true }); diff --git a/apps/server/src/services/auto-mode-service.ts b/apps/server/src/services/auto-mode-service.ts index f7cd3eaa..ae5f24cb 100644 --- a/apps/server/src/services/auto-mode-service.ts +++ b/apps/server/src/services/auto-mode-service.ts @@ -1945,7 +1945,7 @@ This mock response was generated because AUTOMAKER_MOCK_AGENT=true was set. ) { throw new Error( "Authentication failed: Invalid or expired API key. " + - "Please check your ANTHROPIC_API_KEY or GOOGLE_API_KEY, or run 'claude login' to re-authenticate." + "Please check your ANTHROPIC_API_KEY, or run 'claude login' to re-authenticate." ); } diff --git a/docker-compose.yml b/docker-compose.yml index 5e09750f..4ef9cc98 100644 --- a/docker-compose.yml +++ b/docker-compose.yml @@ -48,10 +48,6 @@ services: # Optional - CORS origin (default allows all) - CORS_ORIGIN=${CORS_ORIGIN:-*} - - # Optional - additional API keys - - OPENAI_API_KEY=${OPENAI_API_KEY:-} - - GOOGLE_API_KEY=${GOOGLE_API_KEY:-} volumes: # ONLY named volumes - these are isolated from your host filesystem # This volume persists data between restarts but is container-managed From ade80484bb4fb92a690115d5193db805300737f8 Mon Sep 17 00:00:00 2001 From: Test User Date: Sat, 20 Dec 2025 18:13:34 -0500 Subject: [PATCH 4/7] fix: enforce ALLOWED_ROOT_DIRECTORY path validation across all routes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This fixes a critical security issue where path parameters from client requests were not validated against ALLOWED_ROOT_DIRECTORY, allowing attackers to access files and directories outside the configured root directory. Changes: - Add validatePath() checks to 29 route handlers that accept path parameters - Validate paths in agent routes (workingDirectory, imagePaths) - Validate paths in feature routes (projectPath) - Validate paths in worktree routes (projectPath, worktreePath) - Validate paths in git routes (projectPath, filePath) - Validate paths in auto-mode routes (projectPath, worktreePath) - Validate paths in settings/suggestions routes (projectPath) - Return 403 Forbidden for paths outside ALLOWED_ROOT_DIRECTORY - Maintain backward compatibility (unrestricted when env var not set) Security Impact: - Prevents directory traversal attacks - Prevents unauthorized file access - Prevents arbitrary code execution via unvalidated paths All validation follows the existing pattern in fs routes and session creation, using the validatePath() function from lib/security.ts which checks against both ALLOWED_ROOT_DIRECTORY and DATA_DIR (appData). Tests: All 653 unit tests passing 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Haiku 4.5 --- apps/server/src/routes/agent/routes/send.ts | 22 +++++++++++++++++++ apps/server/src/routes/agent/routes/start.ts | 17 ++++++++++++++ .../auto-mode/routes/analyze-project.ts | 15 +++++++++++++ .../routes/auto-mode/routes/commit-feature.ts | 18 +++++++++++++++ .../routes/auto-mode/routes/run-feature.ts | 15 +++++++++++++ .../src/routes/features/routes/create.ts | 17 +++++++++++--- .../src/routes/features/routes/delete.ts | 15 +++++++++++++ apps/server/src/routes/features/routes/get.ts | 15 +++++++++++++ .../server/src/routes/features/routes/list.ts | 17 +++++++++++--- .../src/routes/features/routes/update.ts | 15 +++++++++++++ apps/server/src/routes/git/routes/diffs.ts | 15 +++++++++++++ .../server/src/routes/git/routes/file-diff.ts | 16 ++++++++++++++ .../src/routes/settings/routes/get-project.ts | 15 +++++++++++++ .../routes/settings/routes/update-project.ts | 15 +++++++++++++ .../src/routes/suggestions/routes/generate.ts | 15 +++++++++++++ .../src/routes/worktree/routes/commit.ts | 15 +++++++++++++ .../src/routes/worktree/routes/create.ts | 15 +++++++++++++ .../src/routes/worktree/routes/delete.ts | 16 ++++++++++++++ .../src/routes/worktree/routes/diffs.ts | 15 +++++++++++++ .../src/routes/worktree/routes/file-diff.ts | 16 ++++++++++++++ .../server/src/routes/worktree/routes/info.ts | 15 +++++++++++++ .../src/routes/worktree/routes/init-git.ts | 15 +++++++++++++ .../routes/worktree/routes/list-branches.ts | 15 +++++++++++++ .../src/routes/worktree/routes/merge.ts | 15 +++++++++++++ .../routes/worktree/routes/open-in-editor.ts | 15 +++++++++++++ .../server/src/routes/worktree/routes/pull.ts | 15 +++++++++++++ .../server/src/routes/worktree/routes/push.ts | 15 +++++++++++++ .../src/routes/worktree/routes/start-dev.ts | 16 ++++++++++++++ .../src/routes/worktree/routes/status.ts | 15 +++++++++++++ docker-compose.override.yml.example | 4 ---- 30 files changed, 449 insertions(+), 10 deletions(-) diff --git a/apps/server/src/routes/agent/routes/send.ts b/apps/server/src/routes/agent/routes/send.ts index fa012e89..59575584 100644 --- a/apps/server/src/routes/agent/routes/send.ts +++ b/apps/server/src/routes/agent/routes/send.ts @@ -6,6 +6,7 @@ import type { Request, Response } from "express"; import { AgentService } from "../../../services/agent-service.js"; import { createLogger } from "../../../lib/logger.js"; import { getErrorMessage, logError } from "../common.js"; +import { validatePath, PathNotAllowedError } from "../../../lib/security.js"; const logger = createLogger("Agent"); @@ -29,6 +30,27 @@ export function createSendHandler(agentService: AgentService) { return; } + // Validate paths are within ALLOWED_ROOT_DIRECTORY + try { + if (workingDirectory) { + validatePath(workingDirectory); + } + if (imagePaths && imagePaths.length > 0) { + for (const imagePath of imagePaths) { + validatePath(imagePath); + } + } + } catch (error) { + if (error instanceof PathNotAllowedError) { + res.status(403).json({ + success: false, + error: error.message, + }); + return; + } + throw error; + } + // Start the message processing (don't await - it streams via WebSocket) agentService .sendMessage({ diff --git a/apps/server/src/routes/agent/routes/start.ts b/apps/server/src/routes/agent/routes/start.ts index 3686bad5..c4900cb1 100644 --- a/apps/server/src/routes/agent/routes/start.ts +++ b/apps/server/src/routes/agent/routes/start.ts @@ -6,6 +6,7 @@ import type { Request, Response } from "express"; import { AgentService } from "../../../services/agent-service.js"; import { createLogger } from "../../../lib/logger.js"; import { getErrorMessage, logError } from "../common.js"; +import { validatePath, PathNotAllowedError } from "../../../lib/security.js"; const logger = createLogger("Agent"); @@ -24,6 +25,22 @@ export function createStartHandler(agentService: AgentService) { return; } + // Validate paths are within ALLOWED_ROOT_DIRECTORY + if (workingDirectory) { + try { + validatePath(workingDirectory); + } catch (error) { + if (error instanceof PathNotAllowedError) { + res.status(403).json({ + success: false, + error: error.message, + }); + return; + } + throw error; + } + } + const result = await agentService.startConversation({ sessionId, workingDirectory, diff --git a/apps/server/src/routes/auto-mode/routes/analyze-project.ts b/apps/server/src/routes/auto-mode/routes/analyze-project.ts index 28a2d489..9f625abc 100644 --- a/apps/server/src/routes/auto-mode/routes/analyze-project.ts +++ b/apps/server/src/routes/auto-mode/routes/analyze-project.ts @@ -6,6 +6,7 @@ import type { Request, Response } from "express"; import type { AutoModeService } from "../../../services/auto-mode-service.js"; import { createLogger } from "../../../lib/logger.js"; import { getErrorMessage, logError } from "../common.js"; +import { validatePath, PathNotAllowedError } from "../../../lib/security.js"; const logger = createLogger("AutoMode"); @@ -21,6 +22,20 @@ export function createAnalyzeProjectHandler(autoModeService: AutoModeService) { return; } + // Validate paths are within ALLOWED_ROOT_DIRECTORY + try { + validatePath(projectPath); + } catch (error) { + if (error instanceof PathNotAllowedError) { + res.status(403).json({ + success: false, + error: error.message, + }); + return; + } + throw error; + } + // Start analysis in background autoModeService.analyzeProject(projectPath).catch((error) => { logger.error(`[AutoMode] Project analysis error:`, error); diff --git a/apps/server/src/routes/auto-mode/routes/commit-feature.ts b/apps/server/src/routes/auto-mode/routes/commit-feature.ts index aaf2e6f5..3e4bcbea 100644 --- a/apps/server/src/routes/auto-mode/routes/commit-feature.ts +++ b/apps/server/src/routes/auto-mode/routes/commit-feature.ts @@ -5,6 +5,7 @@ import type { Request, Response } from "express"; import type { AutoModeService } from "../../../services/auto-mode-service.js"; import { getErrorMessage, logError } from "../common.js"; +import { validatePath, PathNotAllowedError } from "../../../lib/security.js"; export function createCommitFeatureHandler(autoModeService: AutoModeService) { return async (req: Request, res: Response): Promise => { @@ -25,6 +26,23 @@ export function createCommitFeatureHandler(autoModeService: AutoModeService) { return; } + // Validate paths are within ALLOWED_ROOT_DIRECTORY + try { + validatePath(projectPath); + if (worktreePath) { + validatePath(worktreePath); + } + } catch (error) { + if (error instanceof PathNotAllowedError) { + res.status(403).json({ + success: false, + error: error.message, + }); + return; + } + throw error; + } + const commitHash = await autoModeService.commitFeature( projectPath, featureId, diff --git a/apps/server/src/routes/auto-mode/routes/run-feature.ts b/apps/server/src/routes/auto-mode/routes/run-feature.ts index bae005f3..25405c29 100644 --- a/apps/server/src/routes/auto-mode/routes/run-feature.ts +++ b/apps/server/src/routes/auto-mode/routes/run-feature.ts @@ -6,6 +6,7 @@ import type { Request, Response } from "express"; import type { AutoModeService } from "../../../services/auto-mode-service.js"; import { createLogger } from "../../../lib/logger.js"; import { getErrorMessage, logError } from "../common.js"; +import { validatePath, PathNotAllowedError } from "../../../lib/security.js"; const logger = createLogger("AutoMode"); @@ -26,6 +27,20 @@ export function createRunFeatureHandler(autoModeService: AutoModeService) { return; } + // Validate path is within ALLOWED_ROOT_DIRECTORY + try { + validatePath(projectPath); + } catch (error) { + if (error instanceof PathNotAllowedError) { + res.status(403).json({ + success: false, + error: error.message, + }); + return; + } + throw error; + } + // Start execution in background // executeFeature derives workDir from feature.branchName autoModeService diff --git a/apps/server/src/routes/features/routes/create.ts b/apps/server/src/routes/features/routes/create.ts index fda12589..19601c71 100644 --- a/apps/server/src/routes/features/routes/create.ts +++ b/apps/server/src/routes/features/routes/create.ts @@ -7,7 +7,7 @@ import { FeatureLoader, type Feature, } from "../../../services/feature-loader.js"; -import { addAllowedPath } from "../../../lib/security.js"; +import { validatePath, PathNotAllowedError } from "../../../lib/security.js"; import { getErrorMessage, logError } from "../common.js"; export function createCreateHandler(featureLoader: FeatureLoader) { @@ -28,8 +28,19 @@ export function createCreateHandler(featureLoader: FeatureLoader) { return; } - // Add project path to allowed paths - addAllowedPath(projectPath); + // Validate path is within ALLOWED_ROOT_DIRECTORY + try { + validatePath(projectPath); + } catch (error) { + if (error instanceof PathNotAllowedError) { + res.status(403).json({ + success: false, + error: error.message, + }); + return; + } + throw error; + } const created = await featureLoader.create(projectPath, feature); res.json({ success: true, feature: created }); diff --git a/apps/server/src/routes/features/routes/delete.ts b/apps/server/src/routes/features/routes/delete.ts index bf5408d5..9ee29b01 100644 --- a/apps/server/src/routes/features/routes/delete.ts +++ b/apps/server/src/routes/features/routes/delete.ts @@ -5,6 +5,7 @@ import type { Request, Response } from "express"; import { FeatureLoader } from "../../../services/feature-loader.js"; import { getErrorMessage, logError } from "../common.js"; +import { validatePath, PathNotAllowedError } from "../../../lib/security.js"; export function createDeleteHandler(featureLoader: FeatureLoader) { return async (req: Request, res: Response): Promise => { @@ -24,6 +25,20 @@ export function createDeleteHandler(featureLoader: FeatureLoader) { return; } + // Validate path is within ALLOWED_ROOT_DIRECTORY + try { + validatePath(projectPath); + } catch (error) { + if (error instanceof PathNotAllowedError) { + res.status(403).json({ + success: false, + error: error.message, + }); + return; + } + throw error; + } + const success = await featureLoader.delete(projectPath, featureId); res.json({ success }); } catch (error) { diff --git a/apps/server/src/routes/features/routes/get.ts b/apps/server/src/routes/features/routes/get.ts index 17900bb0..c7a6c095 100644 --- a/apps/server/src/routes/features/routes/get.ts +++ b/apps/server/src/routes/features/routes/get.ts @@ -5,6 +5,7 @@ import type { Request, Response } from "express"; import { FeatureLoader } from "../../../services/feature-loader.js"; import { getErrorMessage, logError } from "../common.js"; +import { validatePath, PathNotAllowedError } from "../../../lib/security.js"; export function createGetHandler(featureLoader: FeatureLoader) { return async (req: Request, res: Response): Promise => { @@ -24,6 +25,20 @@ export function createGetHandler(featureLoader: FeatureLoader) { return; } + // Validate path is within ALLOWED_ROOT_DIRECTORY + try { + validatePath(projectPath); + } catch (error) { + if (error instanceof PathNotAllowedError) { + res.status(403).json({ + success: false, + error: error.message, + }); + return; + } + throw error; + } + const feature = await featureLoader.get(projectPath, featureId); if (!feature) { res.status(404).json({ success: false, error: "Feature not found" }); diff --git a/apps/server/src/routes/features/routes/list.ts b/apps/server/src/routes/features/routes/list.ts index 33dc68b6..892a8c63 100644 --- a/apps/server/src/routes/features/routes/list.ts +++ b/apps/server/src/routes/features/routes/list.ts @@ -4,7 +4,7 @@ import type { Request, Response } from "express"; import { FeatureLoader } from "../../../services/feature-loader.js"; -import { addAllowedPath } from "../../../lib/security.js"; +import { validatePath, PathNotAllowedError } from "../../../lib/security.js"; import { getErrorMessage, logError } from "../common.js"; export function createListHandler(featureLoader: FeatureLoader) { @@ -19,8 +19,19 @@ export function createListHandler(featureLoader: FeatureLoader) { return; } - // Add project path to allowed paths - addAllowedPath(projectPath); + // Validate path is within ALLOWED_ROOT_DIRECTORY + try { + validatePath(projectPath); + } catch (error) { + if (error instanceof PathNotAllowedError) { + res.status(403).json({ + success: false, + error: error.message, + }); + return; + } + throw error; + } const features = await featureLoader.getAll(projectPath); res.json({ success: true, features }); diff --git a/apps/server/src/routes/features/routes/update.ts b/apps/server/src/routes/features/routes/update.ts index 68be887b..b33eb549 100644 --- a/apps/server/src/routes/features/routes/update.ts +++ b/apps/server/src/routes/features/routes/update.ts @@ -8,6 +8,7 @@ import { type Feature, } from "../../../services/feature-loader.js"; import { getErrorMessage, logError } from "../common.js"; +import { validatePath, PathNotAllowedError } from "../../../lib/security.js"; export function createUpdateHandler(featureLoader: FeatureLoader) { return async (req: Request, res: Response): Promise => { @@ -26,6 +27,20 @@ export function createUpdateHandler(featureLoader: FeatureLoader) { return; } + // Validate path is within ALLOWED_ROOT_DIRECTORY + try { + validatePath(projectPath); + } catch (error) { + if (error instanceof PathNotAllowedError) { + res.status(403).json({ + success: false, + error: error.message, + }); + return; + } + throw error; + } + const updated = await featureLoader.update( projectPath, featureId, diff --git a/apps/server/src/routes/git/routes/diffs.ts b/apps/server/src/routes/git/routes/diffs.ts index eb532a03..a258f004 100644 --- a/apps/server/src/routes/git/routes/diffs.ts +++ b/apps/server/src/routes/git/routes/diffs.ts @@ -5,6 +5,7 @@ import type { Request, Response } from "express"; import { getErrorMessage, logError } from "../common.js"; import { getGitRepositoryDiffs } from "../../common.js"; +import { validatePath, PathNotAllowedError } from "../../../lib/security.js"; export function createDiffsHandler() { return async (req: Request, res: Response): Promise => { @@ -16,6 +17,20 @@ export function createDiffsHandler() { return; } + // Validate paths are within ALLOWED_ROOT_DIRECTORY + try { + validatePath(projectPath); + } catch (error) { + if (error instanceof PathNotAllowedError) { + res.status(403).json({ + success: false, + error: error.message, + }); + return; + } + throw error; + } + try { const result = await getGitRepositoryDiffs(projectPath); res.json({ diff --git a/apps/server/src/routes/git/routes/file-diff.ts b/apps/server/src/routes/git/routes/file-diff.ts index fdf66998..4229a123 100644 --- a/apps/server/src/routes/git/routes/file-diff.ts +++ b/apps/server/src/routes/git/routes/file-diff.ts @@ -7,6 +7,7 @@ import { exec } from "child_process"; import { promisify } from "util"; import { getErrorMessage, logError } from "../common.js"; import { generateSyntheticDiffForNewFile } from "../../common.js"; +import { validatePath, PathNotAllowedError } from "../../../lib/security.js"; const execAsync = promisify(exec); @@ -25,6 +26,21 @@ export function createFileDiffHandler() { return; } + // Validate paths are within ALLOWED_ROOT_DIRECTORY + try { + validatePath(projectPath); + validatePath(filePath); + } catch (error) { + if (error instanceof PathNotAllowedError) { + res.status(403).json({ + success: false, + error: error.message, + }); + return; + } + throw error; + } + try { // First check if the file is untracked const { stdout: status } = await execAsync( diff --git a/apps/server/src/routes/settings/routes/get-project.ts b/apps/server/src/routes/settings/routes/get-project.ts index 58f6ce7e..9a2c9ba9 100644 --- a/apps/server/src/routes/settings/routes/get-project.ts +++ b/apps/server/src/routes/settings/routes/get-project.ts @@ -11,6 +11,7 @@ import type { Request, Response } from "express"; import type { SettingsService } from "../../../services/settings-service.js"; import { getErrorMessage, logError } from "../common.js"; +import { validatePath, PathNotAllowedError } from "../../../lib/security.js"; /** * Create handler factory for POST /api/settings/project @@ -31,6 +32,20 @@ export function createGetProjectHandler(settingsService: SettingsService) { return; } + // Validate paths are within ALLOWED_ROOT_DIRECTORY + try { + validatePath(projectPath); + } catch (error) { + if (error instanceof PathNotAllowedError) { + res.status(403).json({ + success: false, + error: error.message, + }); + return; + } + throw error; + } + const settings = await settingsService.getProjectSettings(projectPath); res.json({ diff --git a/apps/server/src/routes/settings/routes/update-project.ts b/apps/server/src/routes/settings/routes/update-project.ts index 5dc38df0..cccad9f4 100644 --- a/apps/server/src/routes/settings/routes/update-project.ts +++ b/apps/server/src/routes/settings/routes/update-project.ts @@ -12,6 +12,7 @@ import type { Request, Response } from "express"; import type { SettingsService } from "../../../services/settings-service.js"; import type { ProjectSettings } from "../../../types/settings.js"; import { getErrorMessage, logError } from "../common.js"; +import { validatePath, PathNotAllowedError } from "../../../lib/security.js"; /** * Create handler factory for PUT /api/settings/project @@ -43,6 +44,20 @@ export function createUpdateProjectHandler(settingsService: SettingsService) { return; } + // Validate paths are within ALLOWED_ROOT_DIRECTORY + try { + validatePath(projectPath); + } catch (error) { + if (error instanceof PathNotAllowedError) { + res.status(403).json({ + success: false, + error: error.message, + }); + return; + } + throw error; + } + const settings = await settingsService.updateProjectSettings( projectPath, updates diff --git a/apps/server/src/routes/suggestions/routes/generate.ts b/apps/server/src/routes/suggestions/routes/generate.ts index beafd10f..d9f4582b 100644 --- a/apps/server/src/routes/suggestions/routes/generate.ts +++ b/apps/server/src/routes/suggestions/routes/generate.ts @@ -12,6 +12,7 @@ import { logError, } from "../common.js"; import { generateSuggestions } from "../generate-suggestions.js"; +import { validatePath, PathNotAllowedError } from "../../../lib/security.js"; const logger = createLogger("Suggestions"); @@ -28,6 +29,20 @@ export function createGenerateHandler(events: EventEmitter) { return; } + // Validate paths are within ALLOWED_ROOT_DIRECTORY + try { + validatePath(projectPath); + } catch (error) { + if (error instanceof PathNotAllowedError) { + res.status(403).json({ + success: false, + error: error.message, + }); + return; + } + throw error; + } + const { isRunning } = getSuggestionsStatus(); if (isRunning) { res.json({ diff --git a/apps/server/src/routes/worktree/routes/commit.ts b/apps/server/src/routes/worktree/routes/commit.ts index 273c7964..69575a90 100644 --- a/apps/server/src/routes/worktree/routes/commit.ts +++ b/apps/server/src/routes/worktree/routes/commit.ts @@ -6,6 +6,7 @@ import type { Request, Response } from "express"; import { exec } from "child_process"; import { promisify } from "util"; import { getErrorMessage, logError } from "../common.js"; +import { validatePath, PathNotAllowedError } from "../../../lib/security.js"; const execAsync = promisify(exec); @@ -25,6 +26,20 @@ export function createCommitHandler() { return; } + // Validate paths are within ALLOWED_ROOT_DIRECTORY + try { + validatePath(worktreePath); + } catch (error) { + if (error instanceof PathNotAllowedError) { + res.status(403).json({ + success: false, + error: error.message, + }); + return; + } + throw error; + } + // Check for uncommitted changes const { stdout: status } = await execAsync("git status --porcelain", { cwd: worktreePath, diff --git a/apps/server/src/routes/worktree/routes/create.ts b/apps/server/src/routes/worktree/routes/create.ts index 690afe48..c8953963 100644 --- a/apps/server/src/routes/worktree/routes/create.ts +++ b/apps/server/src/routes/worktree/routes/create.ts @@ -20,6 +20,7 @@ import { ensureInitialCommit, } from "../common.js"; import { trackBranch } from "./branch-tracking.js"; +import { validatePath, PathNotAllowedError } from "../../../lib/security.js"; const execAsync = promisify(exec); @@ -91,6 +92,20 @@ export function createCreateHandler() { return; } + // Validate paths are within ALLOWED_ROOT_DIRECTORY + try { + validatePath(projectPath); + } catch (error) { + if (error instanceof PathNotAllowedError) { + res.status(403).json({ + success: false, + error: error.message, + }); + return; + } + throw error; + } + if (!(await isGitRepo(projectPath))) { res.status(400).json({ success: false, diff --git a/apps/server/src/routes/worktree/routes/delete.ts b/apps/server/src/routes/worktree/routes/delete.ts index a0cb8eea..08eb30d3 100644 --- a/apps/server/src/routes/worktree/routes/delete.ts +++ b/apps/server/src/routes/worktree/routes/delete.ts @@ -6,6 +6,7 @@ import type { Request, Response } from "express"; import { exec } from "child_process"; import { promisify } from "util"; import { isGitRepo, getErrorMessage, logError } from "../common.js"; +import { validatePath, PathNotAllowedError } from "../../../lib/security.js"; const execAsync = promisify(exec); @@ -26,6 +27,21 @@ export function createDeleteHandler() { return; } + // Validate paths are within ALLOWED_ROOT_DIRECTORY + try { + validatePath(projectPath); + validatePath(worktreePath); + } catch (error) { + if (error instanceof PathNotAllowedError) { + res.status(403).json({ + success: false, + error: error.message, + }); + return; + } + throw error; + } + if (!(await isGitRepo(projectPath))) { res.status(400).json({ success: false, diff --git a/apps/server/src/routes/worktree/routes/diffs.ts b/apps/server/src/routes/worktree/routes/diffs.ts index d3b6ed09..8a94f21c 100644 --- a/apps/server/src/routes/worktree/routes/diffs.ts +++ b/apps/server/src/routes/worktree/routes/diffs.ts @@ -7,6 +7,7 @@ import path from "path"; import fs from "fs/promises"; import { getErrorMessage, logError } from "../common.js"; import { getGitRepositoryDiffs } from "../../common.js"; +import { validatePath, PathNotAllowedError } from "../../../lib/security.js"; export function createDiffsHandler() { return async (req: Request, res: Response): Promise => { @@ -26,6 +27,20 @@ export function createDiffsHandler() { return; } + // Validate paths are within ALLOWED_ROOT_DIRECTORY + try { + validatePath(projectPath); + } catch (error) { + if (error instanceof PathNotAllowedError) { + res.status(403).json({ + success: false, + error: error.message, + }); + return; + } + throw error; + } + // Git worktrees are stored in project directory const worktreePath = path.join(projectPath, ".worktrees", featureId); diff --git a/apps/server/src/routes/worktree/routes/file-diff.ts b/apps/server/src/routes/worktree/routes/file-diff.ts index 70306b6a..3d26bf42 100644 --- a/apps/server/src/routes/worktree/routes/file-diff.ts +++ b/apps/server/src/routes/worktree/routes/file-diff.ts @@ -9,6 +9,7 @@ import path from "path"; import fs from "fs/promises"; import { getErrorMessage, logError } from "../common.js"; import { generateSyntheticDiffForNewFile } from "../../common.js"; +import { validatePath, PathNotAllowedError } from "../../../lib/security.js"; const execAsync = promisify(exec); @@ -29,6 +30,21 @@ export function createFileDiffHandler() { return; } + // Validate paths are within ALLOWED_ROOT_DIRECTORY + try { + validatePath(projectPath); + validatePath(filePath); + } catch (error) { + if (error instanceof PathNotAllowedError) { + res.status(403).json({ + success: false, + error: error.message, + }); + return; + } + throw error; + } + // Git worktrees are stored in project directory const worktreePath = path.join(projectPath, ".worktrees", featureId); diff --git a/apps/server/src/routes/worktree/routes/info.ts b/apps/server/src/routes/worktree/routes/info.ts index 1a5bb463..50dbb371 100644 --- a/apps/server/src/routes/worktree/routes/info.ts +++ b/apps/server/src/routes/worktree/routes/info.ts @@ -8,6 +8,7 @@ import { promisify } from "util"; import path from "path"; import fs from "fs/promises"; import { getErrorMessage, logError, normalizePath } from "../common.js"; +import { validatePath, PathNotAllowedError } from "../../../lib/security.js"; const execAsync = promisify(exec); @@ -29,6 +30,20 @@ export function createInfoHandler() { return; } + // Validate paths are within ALLOWED_ROOT_DIRECTORY + try { + validatePath(projectPath); + } catch (error) { + if (error instanceof PathNotAllowedError) { + res.status(403).json({ + success: false, + error: error.message, + }); + return; + } + throw error; + } + // Check if worktree exists (git worktrees are stored in project directory) const worktreePath = path.join(projectPath, ".worktrees", featureId); try { diff --git a/apps/server/src/routes/worktree/routes/init-git.ts b/apps/server/src/routes/worktree/routes/init-git.ts index 0aecc8af..49e7e64e 100644 --- a/apps/server/src/routes/worktree/routes/init-git.ts +++ b/apps/server/src/routes/worktree/routes/init-git.ts @@ -8,6 +8,7 @@ import { promisify } from "util"; import { existsSync } from "fs"; import { join } from "path"; import { getErrorMessage, logError } from "../common.js"; +import { validatePath, PathNotAllowedError } from "../../../lib/security.js"; const execAsync = promisify(exec); @@ -26,6 +27,20 @@ export function createInitGitHandler() { return; } + // Validate paths are within ALLOWED_ROOT_DIRECTORY + try { + validatePath(projectPath); + } catch (error) { + if (error instanceof PathNotAllowedError) { + res.status(403).json({ + success: false, + error: error.message, + }); + return; + } + throw error; + } + // Check if .git already exists const gitDirPath = join(projectPath, ".git"); if (existsSync(gitDirPath)) { diff --git a/apps/server/src/routes/worktree/routes/list-branches.ts b/apps/server/src/routes/worktree/routes/list-branches.ts index 0b07eb17..1b0cd9e6 100644 --- a/apps/server/src/routes/worktree/routes/list-branches.ts +++ b/apps/server/src/routes/worktree/routes/list-branches.ts @@ -6,6 +6,7 @@ import type { Request, Response } from "express"; import { exec } from "child_process"; import { promisify } from "util"; import { getErrorMessage, logWorktreeError } from "../common.js"; +import { validatePath, PathNotAllowedError } from "../../../lib/security.js"; const execAsync = promisify(exec); @@ -30,6 +31,20 @@ export function createListBranchesHandler() { return; } + // Validate paths are within ALLOWED_ROOT_DIRECTORY + try { + validatePath(worktreePath); + } catch (error) { + if (error instanceof PathNotAllowedError) { + res.status(403).json({ + success: false, + error: error.message, + }); + return; + } + throw error; + } + // Get current branch const { stdout: currentBranchOutput } = await execAsync( "git rev-parse --abbrev-ref HEAD", diff --git a/apps/server/src/routes/worktree/routes/merge.ts b/apps/server/src/routes/worktree/routes/merge.ts index f9499d85..df109bc7 100644 --- a/apps/server/src/routes/worktree/routes/merge.ts +++ b/apps/server/src/routes/worktree/routes/merge.ts @@ -7,6 +7,7 @@ import { exec } from "child_process"; import { promisify } from "util"; import path from "path"; import { getErrorMessage, logError } from "../common.js"; +import { validatePath, PathNotAllowedError } from "../../../lib/security.js"; const execAsync = promisify(exec); @@ -29,6 +30,20 @@ export function createMergeHandler() { return; } + // Validate paths are within ALLOWED_ROOT_DIRECTORY + try { + validatePath(projectPath); + } catch (error) { + if (error instanceof PathNotAllowedError) { + res.status(403).json({ + success: false, + error: error.message, + }); + return; + } + throw error; + } + const branchName = `feature/${featureId}`; // Git worktrees are stored in project directory const worktreePath = path.join(projectPath, ".worktrees", featureId); diff --git a/apps/server/src/routes/worktree/routes/open-in-editor.ts b/apps/server/src/routes/worktree/routes/open-in-editor.ts index 04f9815f..7eecaf7d 100644 --- a/apps/server/src/routes/worktree/routes/open-in-editor.ts +++ b/apps/server/src/routes/worktree/routes/open-in-editor.ts @@ -7,6 +7,7 @@ import type { Request, Response } from "express"; import { exec } from "child_process"; import { promisify } from "util"; import { getErrorMessage, logError } from "../common.js"; +import { validatePath, PathNotAllowedError } from "../../../lib/security.js"; const execAsync = promisify(exec); @@ -108,6 +109,20 @@ export function createOpenInEditorHandler() { return; } + // Validate paths are within ALLOWED_ROOT_DIRECTORY + try { + validatePath(worktreePath); + } catch (error) { + if (error instanceof PathNotAllowedError) { + res.status(403).json({ + success: false, + error: error.message, + }); + return; + } + throw error; + } + const editor = await detectDefaultEditor(); try { diff --git a/apps/server/src/routes/worktree/routes/pull.ts b/apps/server/src/routes/worktree/routes/pull.ts index 119192d0..5150ae7e 100644 --- a/apps/server/src/routes/worktree/routes/pull.ts +++ b/apps/server/src/routes/worktree/routes/pull.ts @@ -6,6 +6,7 @@ import type { Request, Response } from "express"; import { exec } from "child_process"; import { promisify } from "util"; import { getErrorMessage, logError } from "../common.js"; +import { validatePath, PathNotAllowedError } from "../../../lib/security.js"; const execAsync = promisify(exec); @@ -24,6 +25,20 @@ export function createPullHandler() { return; } + // Validate paths are within ALLOWED_ROOT_DIRECTORY + try { + validatePath(worktreePath); + } catch (error) { + if (error instanceof PathNotAllowedError) { + res.status(403).json({ + success: false, + error: error.message, + }); + return; + } + throw error; + } + // Get current branch name const { stdout: branchOutput } = await execAsync( "git rev-parse --abbrev-ref HEAD", diff --git a/apps/server/src/routes/worktree/routes/push.ts b/apps/server/src/routes/worktree/routes/push.ts index d9447a2b..bf95374c 100644 --- a/apps/server/src/routes/worktree/routes/push.ts +++ b/apps/server/src/routes/worktree/routes/push.ts @@ -6,6 +6,7 @@ import type { Request, Response } from "express"; import { exec } from "child_process"; import { promisify } from "util"; import { getErrorMessage, logError } from "../common.js"; +import { validatePath, PathNotAllowedError } from "../../../lib/security.js"; const execAsync = promisify(exec); @@ -25,6 +26,20 @@ export function createPushHandler() { return; } + // Validate paths are within ALLOWED_ROOT_DIRECTORY + try { + validatePath(worktreePath); + } catch (error) { + if (error instanceof PathNotAllowedError) { + res.status(403).json({ + success: false, + error: error.message, + }); + return; + } + throw error; + } + // Get branch name const { stdout: branchOutput } = await execAsync( "git rev-parse --abbrev-ref HEAD", diff --git a/apps/server/src/routes/worktree/routes/start-dev.ts b/apps/server/src/routes/worktree/routes/start-dev.ts index fcd0cec7..5a17b3fd 100644 --- a/apps/server/src/routes/worktree/routes/start-dev.ts +++ b/apps/server/src/routes/worktree/routes/start-dev.ts @@ -9,6 +9,7 @@ import type { Request, Response } from "express"; import { getDevServerService } from "../../../services/dev-server-service.js"; import { getErrorMessage, logError } from "../common.js"; +import { validatePath, PathNotAllowedError } from "../../../lib/security.js"; export function createStartDevHandler() { return async (req: Request, res: Response): Promise => { @@ -34,6 +35,21 @@ export function createStartDevHandler() { return; } + // Validate paths are within ALLOWED_ROOT_DIRECTORY + try { + validatePath(projectPath); + validatePath(worktreePath); + } catch (error) { + if (error instanceof PathNotAllowedError) { + res.status(403).json({ + success: false, + error: error.message, + }); + return; + } + throw error; + } + const devServerService = getDevServerService(); const result = await devServerService.startDevServer(projectPath, worktreePath); diff --git a/apps/server/src/routes/worktree/routes/status.ts b/apps/server/src/routes/worktree/routes/status.ts index 3f56ef17..d317ad0f 100644 --- a/apps/server/src/routes/worktree/routes/status.ts +++ b/apps/server/src/routes/worktree/routes/status.ts @@ -8,6 +8,7 @@ import { promisify } from "util"; import path from "path"; import fs from "fs/promises"; import { getErrorMessage, logError } from "../common.js"; +import { validatePath, PathNotAllowedError } from "../../../lib/security.js"; const execAsync = promisify(exec); @@ -29,6 +30,20 @@ export function createStatusHandler() { return; } + // Validate paths are within ALLOWED_ROOT_DIRECTORY + try { + validatePath(projectPath); + } catch (error) { + if (error instanceof PathNotAllowedError) { + res.status(403).json({ + success: false, + error: error.message, + }); + return; + } + throw error; + } + // Git worktrees are stored in project directory const worktreePath = path.join(projectPath, ".worktrees", featureId); diff --git a/docker-compose.override.yml.example b/docker-compose.override.yml.example index cdbc3346..99d7a5dd 100644 --- a/docker-compose.override.yml.example +++ b/docker-compose.override.yml.example @@ -8,7 +8,3 @@ services: # Set root directory for all projects and file operations # Users can only create/open projects within this directory - ALLOWED_ROOT_DIRECTORY=/projects - - # Optional: Set workspace directory for UI project discovery - # Falls back to ALLOWED_ROOT_DIRECTORY if not set - # - WORKSPACE_DIR=/projects From f3c9e828e2002bf068dc35088bb5bb7481e692af Mon Sep 17 00:00:00 2001 From: Test User Date: Sat, 20 Dec 2025 18:45:39 -0500 Subject: [PATCH 5/7] refactor: integrate secure file system operations across services MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This commit replaces direct file system operations with a secure file system adapter to enhance security by enforcing path validation. The changes include: - Replaced `fs` imports with `secureFs` in various services and utilities. - Updated file operations in `agent-service`, `auto-mode-service`, `feature-loader`, and `settings-service` to use the secure file system methods. - Ensured that all file I/O operations are validated against the ALLOWED_ROOT_DIRECTORY. This refactor aims to prevent unauthorized file access and improve overall security posture. Tests: All unit tests passing. 🤖 Generated with Claude Code --- apps/server/src/lib/automaker-paths.ts | 6 +- apps/server/src/lib/fs-utils.ts | 8 +- apps/server/src/lib/image-handler.ts | 4 +- apps/server/src/lib/secure-fs.ts | 156 ++++++++++++++++++ apps/server/src/middleware/validate-paths.ts | 69 ++++++++ apps/server/src/routes/agent/index.ts | 5 +- apps/server/src/routes/agent/routes/send.ts | 23 --- apps/server/src/routes/agent/routes/start.ts | 18 -- apps/server/src/routes/auto-mode/index.ts | 7 +- .../auto-mode/routes/analyze-project.ts | 15 -- .../routes/auto-mode/routes/commit-feature.ts | 18 -- .../routes/auto-mode/routes/run-feature.ts | 15 -- apps/server/src/routes/features/index.ts | 11 +- .../src/routes/features/routes/create.ts | 15 -- .../src/routes/features/routes/delete.ts | 15 -- apps/server/src/routes/features/routes/get.ts | 15 -- .../server/src/routes/features/routes/list.ts | 15 -- .../src/routes/features/routes/update.ts | 15 -- apps/server/src/routes/git/index.ts | 5 +- apps/server/src/routes/git/routes/diffs.ts | 15 -- .../server/src/routes/git/routes/file-diff.ts | 16 -- apps/server/src/routes/settings/index.ts | 5 +- .../src/routes/settings/routes/get-project.ts | 15 -- .../routes/settings/routes/update-project.ts | 15 -- apps/server/src/routes/suggestions/index.ts | 3 +- .../src/routes/suggestions/routes/generate.ts | 15 -- apps/server/src/routes/worktree/index.ts | 29 ++-- .../src/routes/worktree/routes/commit.ts | 15 -- .../src/routes/worktree/routes/create.ts | 15 -- .../src/routes/worktree/routes/delete.ts | 16 -- .../src/routes/worktree/routes/diffs.ts | 15 -- .../src/routes/worktree/routes/file-diff.ts | 16 -- .../server/src/routes/worktree/routes/info.ts | 15 -- .../src/routes/worktree/routes/init-git.ts | 15 -- .../routes/worktree/routes/list-branches.ts | 15 -- .../src/routes/worktree/routes/merge.ts | 15 -- .../routes/worktree/routes/open-in-editor.ts | 15 -- .../server/src/routes/worktree/routes/pull.ts | 15 -- .../server/src/routes/worktree/routes/push.ts | 15 -- .../src/routes/worktree/routes/start-dev.ts | 16 -- .../src/routes/worktree/routes/status.ts | 15 -- apps/server/src/services/agent-service.ts | 14 +- apps/server/src/services/auto-mode-service.ts | 56 +++---- apps/server/src/services/feature-loader.ts | 36 ++-- apps/server/src/services/settings-service.ts | 13 +- 45 files changed, 329 insertions(+), 551 deletions(-) create mode 100644 apps/server/src/lib/secure-fs.ts create mode 100644 apps/server/src/middleware/validate-paths.ts diff --git a/apps/server/src/lib/automaker-paths.ts b/apps/server/src/lib/automaker-paths.ts index 988d7bbc..556650f8 100644 --- a/apps/server/src/lib/automaker-paths.ts +++ b/apps/server/src/lib/automaker-paths.ts @@ -9,7 +9,7 @@ * Directory creation is handled separately by ensure* functions. */ -import fs from "fs/promises"; +import * as secureFs from "./secure-fs.js"; import path from "path"; /** @@ -149,7 +149,7 @@ export function getBranchTrackingPath(projectPath: string): string { */ export async function ensureAutomakerDir(projectPath: string): Promise { const automakerDir = getAutomakerDir(projectPath); - await fs.mkdir(automakerDir, { recursive: true }); + await secureFs.mkdir(automakerDir, { recursive: true }); return automakerDir; } @@ -211,6 +211,6 @@ export function getProjectSettingsPath(projectPath: string): string { * @returns Promise resolving to the created data directory path */ export async function ensureDataDir(dataDir: string): Promise { - await fs.mkdir(dataDir, { recursive: true }); + await secureFs.mkdir(dataDir, { recursive: true }); return dataDir; } diff --git a/apps/server/src/lib/fs-utils.ts b/apps/server/src/lib/fs-utils.ts index 5b67124a..ac60dda6 100644 --- a/apps/server/src/lib/fs-utils.ts +++ b/apps/server/src/lib/fs-utils.ts @@ -2,7 +2,7 @@ * File system utilities that handle symlinks safely */ -import fs from "fs/promises"; +import * as secureFs from "./secure-fs.js"; import path from "path"; /** @@ -14,7 +14,7 @@ export async function mkdirSafe(dirPath: string): Promise { // Check if path already exists using lstat (doesn't follow symlinks) try { - const stats = await fs.lstat(resolvedPath); + const stats = await secureFs.lstat(resolvedPath); // Path exists - if it's a directory or symlink, consider it success if (stats.isDirectory() || stats.isSymbolicLink()) { return; @@ -36,7 +36,7 @@ export async function mkdirSafe(dirPath: string): Promise { // Path doesn't exist, create it try { - await fs.mkdir(resolvedPath, { recursive: true }); + await secureFs.mkdir(resolvedPath, { recursive: true }); } catch (error: any) { // Handle race conditions and symlink issues if (error.code === "EEXIST" || error.code === "ELOOP") { @@ -52,7 +52,7 @@ export async function mkdirSafe(dirPath: string): Promise { */ export async function existsSafe(filePath: string): Promise { try { - await fs.lstat(filePath); + await secureFs.lstat(filePath); return true; } catch (error: any) { if (error.code === "ENOENT") { diff --git a/apps/server/src/lib/image-handler.ts b/apps/server/src/lib/image-handler.ts index 167f948f..e0c92688 100644 --- a/apps/server/src/lib/image-handler.ts +++ b/apps/server/src/lib/image-handler.ts @@ -8,7 +8,7 @@ * - Path resolution (relative/absolute) */ -import fs from "fs/promises"; +import * as secureFs from "./secure-fs.js"; import path from "path"; /** @@ -63,7 +63,7 @@ export function getMimeTypeForImage(imagePath: string): string { * @throws Error if file cannot be read */ export async function readImageAsBase64(imagePath: string): Promise { - const imageBuffer = await fs.readFile(imagePath); + const imageBuffer = await secureFs.readFile(imagePath) as Buffer; const base64Data = imageBuffer.toString("base64"); const mimeType = getMimeTypeForImage(imagePath); diff --git a/apps/server/src/lib/secure-fs.ts b/apps/server/src/lib/secure-fs.ts new file mode 100644 index 00000000..ba253cd6 --- /dev/null +++ b/apps/server/src/lib/secure-fs.ts @@ -0,0 +1,156 @@ +/** + * Secure File System Adapter + * + * All file I/O operations must go through this adapter to enforce + * ALLOWED_ROOT_DIRECTORY restrictions at the actual access point, + * not just at the API layer. This provides defense-in-depth security. + */ + +import fs from "fs/promises"; +import path from "path"; +import { validatePath } from "./security.js"; + +/** + * Wrapper around fs.access that validates path first + */ +export async function access(filePath: string, mode?: number): Promise { + const validatedPath = validatePath(filePath); + return fs.access(validatedPath, mode); +} + +/** + * Wrapper around fs.readFile that validates path first + */ +export async function readFile( + filePath: string, + encoding?: BufferEncoding +): Promise { + const validatedPath = validatePath(filePath); + if (encoding) { + return fs.readFile(validatedPath, encoding); + } + return fs.readFile(validatedPath); +} + +/** + * Wrapper around fs.writeFile that validates path first + */ +export async function writeFile( + filePath: string, + data: string | Buffer, + encoding?: BufferEncoding +): Promise { + const validatedPath = validatePath(filePath); + return fs.writeFile(validatedPath, data, encoding as any); +} + +/** + * Wrapper around fs.mkdir that validates path first + */ +export async function mkdir( + dirPath: string, + options?: { recursive?: boolean; mode?: number } +): Promise { + const validatedPath = validatePath(dirPath); + return fs.mkdir(validatedPath, options); +} + +/** + * Wrapper around fs.readdir that validates path first + */ +export async function readdir( + dirPath: string, + options?: { withFileTypes?: boolean; encoding?: BufferEncoding } +): Promise { + const validatedPath = validatePath(dirPath); + return fs.readdir(validatedPath, options as any); +} + +/** + * Wrapper around fs.stat that validates path first + */ +export async function stat(filePath: string): Promise { + const validatedPath = validatePath(filePath); + return fs.stat(validatedPath); +} + +/** + * Wrapper around fs.rm that validates path first + */ +export async function rm( + filePath: string, + options?: { recursive?: boolean; force?: boolean } +): Promise { + const validatedPath = validatePath(filePath); + return fs.rm(validatedPath, options); +} + +/** + * Wrapper around fs.unlink that validates path first + */ +export async function unlink(filePath: string): Promise { + const validatedPath = validatePath(filePath); + return fs.unlink(validatedPath); +} + +/** + * Wrapper around fs.copyFile that validates both paths first + */ +export async function copyFile( + src: string, + dest: string, + mode?: number +): Promise { + const validatedSrc = validatePath(src); + const validatedDest = validatePath(dest); + return fs.copyFile(validatedSrc, validatedDest, mode); +} + +/** + * Wrapper around fs.appendFile that validates path first + */ +export async function appendFile( + filePath: string, + data: string | Buffer, + encoding?: BufferEncoding +): Promise { + const validatedPath = validatePath(filePath); + return fs.appendFile(validatedPath, data, encoding as any); +} + +/** + * Wrapper around fs.rename that validates both paths first + */ +export async function rename( + oldPath: string, + newPath: string +): Promise { + const validatedOldPath = validatePath(oldPath); + const validatedNewPath = validatePath(newPath); + return fs.rename(validatedOldPath, validatedNewPath); +} + +/** + * Wrapper around fs.lstat that validates path first + * Returns file stats without following symbolic links + */ +export async function lstat(filePath: string): Promise { + const validatedPath = validatePath(filePath); + return fs.lstat(validatedPath); +} + +/** + * Wrapper around path.join that returns resolved path + * Does NOT validate - use this for path construction, then pass to other operations + */ +export function joinPath(...pathSegments: string[]): string { + return path.join(...pathSegments); +} + +/** + * Wrapper around path.resolve that returns resolved path + * Does NOT validate - use this for path construction, then pass to other operations + */ +export function resolvePath(...pathSegments: string[]): string { + return path.resolve(...pathSegments); +} diff --git a/apps/server/src/middleware/validate-paths.ts b/apps/server/src/middleware/validate-paths.ts new file mode 100644 index 00000000..e4052ab8 --- /dev/null +++ b/apps/server/src/middleware/validate-paths.ts @@ -0,0 +1,69 @@ +/** + * Middleware for validating path parameters against ALLOWED_ROOT_DIRECTORY + * Provides a clean, reusable way to validate paths without repeating the same + * try-catch block in every route handler + */ + +import type { Request, Response, NextFunction } from "express"; +import { validatePath, PathNotAllowedError } from "../lib/security.js"; + +/** + * Creates a middleware that validates specified path parameters in req.body + * @param paramNames - Names of parameters to validate (e.g., 'projectPath', 'worktreePath') + * @example + * router.post('/create', validatePathParams('projectPath'), handler); + * router.post('/delete', validatePathParams('projectPath', 'worktreePath'), handler); + * router.post('/send', validatePathParams('workingDirectory?', 'imagePaths[]'), handler); + * + * Special syntax: + * - 'paramName?' - Optional parameter (only validated if present) + * - 'paramName[]' - Array parameter (validates each element) + */ +export function validatePathParams(...paramNames: string[]) { + return (req: Request, res: Response, next: NextFunction): void => { + try { + for (const paramName of paramNames) { + // Handle optional parameters (paramName?) + if (paramName.endsWith("?")) { + const actualName = paramName.slice(0, -1); + const value = req.body[actualName]; + if (value) { + validatePath(value); + } + continue; + } + + // Handle array parameters (paramName[]) + if (paramName.endsWith("[]")) { + const actualName = paramName.slice(0, -2); + const values = req.body[actualName]; + if (Array.isArray(values) && values.length > 0) { + for (const value of values) { + validatePath(value); + } + } + continue; + } + + // Handle regular parameters + const value = req.body[paramName]; + if (value) { + validatePath(value); + } + } + + next(); + } catch (error) { + if (error instanceof PathNotAllowedError) { + res.status(403).json({ + success: false, + error: error.message, + }); + return; + } + + // Re-throw unexpected errors + throw error; + } + }; +} diff --git a/apps/server/src/routes/agent/index.ts b/apps/server/src/routes/agent/index.ts index ed12e296..61f34656 100644 --- a/apps/server/src/routes/agent/index.ts +++ b/apps/server/src/routes/agent/index.ts @@ -5,6 +5,7 @@ import { Router } from "express"; import { AgentService } from "../../services/agent-service.js"; import type { EventEmitter } from "../../lib/events.js"; +import { validatePathParams } from "../../middleware/validate-paths.js"; import { createStartHandler } from "./routes/start.js"; import { createSendHandler } from "./routes/send.js"; import { createHistoryHandler } from "./routes/history.js"; @@ -18,8 +19,8 @@ export function createAgentRoutes( ): Router { const router = Router(); - router.post("/start", createStartHandler(agentService)); - router.post("/send", createSendHandler(agentService)); + router.post("/start", validatePathParams("workingDirectory?"), createStartHandler(agentService)); + router.post("/send", validatePathParams("workingDirectory?", "imagePaths[]"), createSendHandler(agentService)); router.post("/history", createHistoryHandler(agentService)); router.post("/stop", createStopHandler(agentService)); router.post("/clear", createClearHandler(agentService)); diff --git a/apps/server/src/routes/agent/routes/send.ts b/apps/server/src/routes/agent/routes/send.ts index 59575584..f9e71cfb 100644 --- a/apps/server/src/routes/agent/routes/send.ts +++ b/apps/server/src/routes/agent/routes/send.ts @@ -6,8 +6,6 @@ import type { Request, Response } from "express"; import { AgentService } from "../../../services/agent-service.js"; import { createLogger } from "../../../lib/logger.js"; import { getErrorMessage, logError } from "../common.js"; -import { validatePath, PathNotAllowedError } from "../../../lib/security.js"; - const logger = createLogger("Agent"); export function createSendHandler(agentService: AgentService) { @@ -30,27 +28,6 @@ export function createSendHandler(agentService: AgentService) { return; } - // Validate paths are within ALLOWED_ROOT_DIRECTORY - try { - if (workingDirectory) { - validatePath(workingDirectory); - } - if (imagePaths && imagePaths.length > 0) { - for (const imagePath of imagePaths) { - validatePath(imagePath); - } - } - } catch (error) { - if (error instanceof PathNotAllowedError) { - res.status(403).json({ - success: false, - error: error.message, - }); - return; - } - throw error; - } - // Start the message processing (don't await - it streams via WebSocket) agentService .sendMessage({ diff --git a/apps/server/src/routes/agent/routes/start.ts b/apps/server/src/routes/agent/routes/start.ts index c4900cb1..8cd111f6 100644 --- a/apps/server/src/routes/agent/routes/start.ts +++ b/apps/server/src/routes/agent/routes/start.ts @@ -6,8 +6,6 @@ import type { Request, Response } from "express"; import { AgentService } from "../../../services/agent-service.js"; import { createLogger } from "../../../lib/logger.js"; import { getErrorMessage, logError } from "../common.js"; -import { validatePath, PathNotAllowedError } from "../../../lib/security.js"; - const logger = createLogger("Agent"); export function createStartHandler(agentService: AgentService) { @@ -25,22 +23,6 @@ export function createStartHandler(agentService: AgentService) { return; } - // Validate paths are within ALLOWED_ROOT_DIRECTORY - if (workingDirectory) { - try { - validatePath(workingDirectory); - } catch (error) { - if (error instanceof PathNotAllowedError) { - res.status(403).json({ - success: false, - error: error.message, - }); - return; - } - throw error; - } - } - const result = await agentService.startConversation({ sessionId, workingDirectory, diff --git a/apps/server/src/routes/auto-mode/index.ts b/apps/server/src/routes/auto-mode/index.ts index 8ad4510c..33878b0e 100644 --- a/apps/server/src/routes/auto-mode/index.ts +++ b/apps/server/src/routes/auto-mode/index.ts @@ -6,6 +6,7 @@ import { Router } from "express"; import type { AutoModeService } from "../../services/auto-mode-service.js"; +import { validatePathParams } from "../../middleware/validate-paths.js"; import { createStopFeatureHandler } from "./routes/stop-feature.js"; import { createStatusHandler } from "./routes/status.js"; import { createRunFeatureHandler } from "./routes/run-feature.js"; @@ -22,16 +23,16 @@ export function createAutoModeRoutes(autoModeService: AutoModeService): Router { router.post("/stop-feature", createStopFeatureHandler(autoModeService)); router.post("/status", createStatusHandler(autoModeService)); - router.post("/run-feature", createRunFeatureHandler(autoModeService)); + router.post("/run-feature", validatePathParams("projectPath"), createRunFeatureHandler(autoModeService)); router.post("/verify-feature", createVerifyFeatureHandler(autoModeService)); router.post("/resume-feature", createResumeFeatureHandler(autoModeService)); router.post("/context-exists", createContextExistsHandler(autoModeService)); - router.post("/analyze-project", createAnalyzeProjectHandler(autoModeService)); + router.post("/analyze-project", validatePathParams("projectPath"), createAnalyzeProjectHandler(autoModeService)); router.post( "/follow-up-feature", createFollowUpFeatureHandler(autoModeService) ); - router.post("/commit-feature", createCommitFeatureHandler(autoModeService)); + router.post("/commit-feature", validatePathParams("projectPath", "worktreePath?"), createCommitFeatureHandler(autoModeService)); router.post("/approve-plan", createApprovePlanHandler(autoModeService)); return router; diff --git a/apps/server/src/routes/auto-mode/routes/analyze-project.ts b/apps/server/src/routes/auto-mode/routes/analyze-project.ts index 9f625abc..28a2d489 100644 --- a/apps/server/src/routes/auto-mode/routes/analyze-project.ts +++ b/apps/server/src/routes/auto-mode/routes/analyze-project.ts @@ -6,7 +6,6 @@ import type { Request, Response } from "express"; import type { AutoModeService } from "../../../services/auto-mode-service.js"; import { createLogger } from "../../../lib/logger.js"; import { getErrorMessage, logError } from "../common.js"; -import { validatePath, PathNotAllowedError } from "../../../lib/security.js"; const logger = createLogger("AutoMode"); @@ -22,20 +21,6 @@ export function createAnalyzeProjectHandler(autoModeService: AutoModeService) { return; } - // Validate paths are within ALLOWED_ROOT_DIRECTORY - try { - validatePath(projectPath); - } catch (error) { - if (error instanceof PathNotAllowedError) { - res.status(403).json({ - success: false, - error: error.message, - }); - return; - } - throw error; - } - // Start analysis in background autoModeService.analyzeProject(projectPath).catch((error) => { logger.error(`[AutoMode] Project analysis error:`, error); diff --git a/apps/server/src/routes/auto-mode/routes/commit-feature.ts b/apps/server/src/routes/auto-mode/routes/commit-feature.ts index 3e4bcbea..aaf2e6f5 100644 --- a/apps/server/src/routes/auto-mode/routes/commit-feature.ts +++ b/apps/server/src/routes/auto-mode/routes/commit-feature.ts @@ -5,7 +5,6 @@ import type { Request, Response } from "express"; import type { AutoModeService } from "../../../services/auto-mode-service.js"; import { getErrorMessage, logError } from "../common.js"; -import { validatePath, PathNotAllowedError } from "../../../lib/security.js"; export function createCommitFeatureHandler(autoModeService: AutoModeService) { return async (req: Request, res: Response): Promise => { @@ -26,23 +25,6 @@ export function createCommitFeatureHandler(autoModeService: AutoModeService) { return; } - // Validate paths are within ALLOWED_ROOT_DIRECTORY - try { - validatePath(projectPath); - if (worktreePath) { - validatePath(worktreePath); - } - } catch (error) { - if (error instanceof PathNotAllowedError) { - res.status(403).json({ - success: false, - error: error.message, - }); - return; - } - throw error; - } - const commitHash = await autoModeService.commitFeature( projectPath, featureId, diff --git a/apps/server/src/routes/auto-mode/routes/run-feature.ts b/apps/server/src/routes/auto-mode/routes/run-feature.ts index 25405c29..bae005f3 100644 --- a/apps/server/src/routes/auto-mode/routes/run-feature.ts +++ b/apps/server/src/routes/auto-mode/routes/run-feature.ts @@ -6,7 +6,6 @@ import type { Request, Response } from "express"; import type { AutoModeService } from "../../../services/auto-mode-service.js"; import { createLogger } from "../../../lib/logger.js"; import { getErrorMessage, logError } from "../common.js"; -import { validatePath, PathNotAllowedError } from "../../../lib/security.js"; const logger = createLogger("AutoMode"); @@ -27,20 +26,6 @@ export function createRunFeatureHandler(autoModeService: AutoModeService) { return; } - // Validate path is within ALLOWED_ROOT_DIRECTORY - try { - validatePath(projectPath); - } catch (error) { - if (error instanceof PathNotAllowedError) { - res.status(403).json({ - success: false, - error: error.message, - }); - return; - } - throw error; - } - // Start execution in background // executeFeature derives workDir from feature.branchName autoModeService diff --git a/apps/server/src/routes/features/index.ts b/apps/server/src/routes/features/index.ts index d4406766..dcd98f56 100644 --- a/apps/server/src/routes/features/index.ts +++ b/apps/server/src/routes/features/index.ts @@ -4,6 +4,7 @@ import { Router } from "express"; import { FeatureLoader } from "../../services/feature-loader.js"; +import { validatePathParams } from "../../middleware/validate-paths.js"; import { createListHandler } from "./routes/list.js"; import { createGetHandler } from "./routes/get.js"; import { createCreateHandler } from "./routes/create.js"; @@ -15,11 +16,11 @@ import { createGenerateTitleHandler } from "./routes/generate-title.js"; export function createFeaturesRoutes(featureLoader: FeatureLoader): Router { const router = Router(); - router.post("/list", createListHandler(featureLoader)); - router.post("/get", createGetHandler(featureLoader)); - router.post("/create", createCreateHandler(featureLoader)); - router.post("/update", createUpdateHandler(featureLoader)); - router.post("/delete", createDeleteHandler(featureLoader)); + router.post("/list", validatePathParams("projectPath"), createListHandler(featureLoader)); + router.post("/get", validatePathParams("projectPath"), createGetHandler(featureLoader)); + router.post("/create", validatePathParams("projectPath"), createCreateHandler(featureLoader)); + router.post("/update", validatePathParams("projectPath"), createUpdateHandler(featureLoader)); + router.post("/delete", validatePathParams("projectPath"), createDeleteHandler(featureLoader)); router.post("/agent-output", createAgentOutputHandler(featureLoader)); router.post("/generate-title", createGenerateTitleHandler()); diff --git a/apps/server/src/routes/features/routes/create.ts b/apps/server/src/routes/features/routes/create.ts index 19601c71..8d228608 100644 --- a/apps/server/src/routes/features/routes/create.ts +++ b/apps/server/src/routes/features/routes/create.ts @@ -7,7 +7,6 @@ import { FeatureLoader, type Feature, } from "../../../services/feature-loader.js"; -import { validatePath, PathNotAllowedError } from "../../../lib/security.js"; import { getErrorMessage, logError } from "../common.js"; export function createCreateHandler(featureLoader: FeatureLoader) { @@ -28,20 +27,6 @@ export function createCreateHandler(featureLoader: FeatureLoader) { return; } - // Validate path is within ALLOWED_ROOT_DIRECTORY - try { - validatePath(projectPath); - } catch (error) { - if (error instanceof PathNotAllowedError) { - res.status(403).json({ - success: false, - error: error.message, - }); - return; - } - throw error; - } - const created = await featureLoader.create(projectPath, feature); res.json({ success: true, feature: created }); } catch (error) { diff --git a/apps/server/src/routes/features/routes/delete.ts b/apps/server/src/routes/features/routes/delete.ts index 9ee29b01..bf5408d5 100644 --- a/apps/server/src/routes/features/routes/delete.ts +++ b/apps/server/src/routes/features/routes/delete.ts @@ -5,7 +5,6 @@ import type { Request, Response } from "express"; import { FeatureLoader } from "../../../services/feature-loader.js"; import { getErrorMessage, logError } from "../common.js"; -import { validatePath, PathNotAllowedError } from "../../../lib/security.js"; export function createDeleteHandler(featureLoader: FeatureLoader) { return async (req: Request, res: Response): Promise => { @@ -25,20 +24,6 @@ export function createDeleteHandler(featureLoader: FeatureLoader) { return; } - // Validate path is within ALLOWED_ROOT_DIRECTORY - try { - validatePath(projectPath); - } catch (error) { - if (error instanceof PathNotAllowedError) { - res.status(403).json({ - success: false, - error: error.message, - }); - return; - } - throw error; - } - const success = await featureLoader.delete(projectPath, featureId); res.json({ success }); } catch (error) { diff --git a/apps/server/src/routes/features/routes/get.ts b/apps/server/src/routes/features/routes/get.ts index c7a6c095..17900bb0 100644 --- a/apps/server/src/routes/features/routes/get.ts +++ b/apps/server/src/routes/features/routes/get.ts @@ -5,7 +5,6 @@ import type { Request, Response } from "express"; import { FeatureLoader } from "../../../services/feature-loader.js"; import { getErrorMessage, logError } from "../common.js"; -import { validatePath, PathNotAllowedError } from "../../../lib/security.js"; export function createGetHandler(featureLoader: FeatureLoader) { return async (req: Request, res: Response): Promise => { @@ -25,20 +24,6 @@ export function createGetHandler(featureLoader: FeatureLoader) { return; } - // Validate path is within ALLOWED_ROOT_DIRECTORY - try { - validatePath(projectPath); - } catch (error) { - if (error instanceof PathNotAllowedError) { - res.status(403).json({ - success: false, - error: error.message, - }); - return; - } - throw error; - } - const feature = await featureLoader.get(projectPath, featureId); if (!feature) { res.status(404).json({ success: false, error: "Feature not found" }); diff --git a/apps/server/src/routes/features/routes/list.ts b/apps/server/src/routes/features/routes/list.ts index 892a8c63..cc20b1a1 100644 --- a/apps/server/src/routes/features/routes/list.ts +++ b/apps/server/src/routes/features/routes/list.ts @@ -4,7 +4,6 @@ import type { Request, Response } from "express"; import { FeatureLoader } from "../../../services/feature-loader.js"; -import { validatePath, PathNotAllowedError } from "../../../lib/security.js"; import { getErrorMessage, logError } from "../common.js"; export function createListHandler(featureLoader: FeatureLoader) { @@ -19,20 +18,6 @@ export function createListHandler(featureLoader: FeatureLoader) { return; } - // Validate path is within ALLOWED_ROOT_DIRECTORY - try { - validatePath(projectPath); - } catch (error) { - if (error instanceof PathNotAllowedError) { - res.status(403).json({ - success: false, - error: error.message, - }); - return; - } - throw error; - } - const features = await featureLoader.getAll(projectPath); res.json({ success: true, features }); } catch (error) { diff --git a/apps/server/src/routes/features/routes/update.ts b/apps/server/src/routes/features/routes/update.ts index b33eb549..68be887b 100644 --- a/apps/server/src/routes/features/routes/update.ts +++ b/apps/server/src/routes/features/routes/update.ts @@ -8,7 +8,6 @@ import { type Feature, } from "../../../services/feature-loader.js"; import { getErrorMessage, logError } from "../common.js"; -import { validatePath, PathNotAllowedError } from "../../../lib/security.js"; export function createUpdateHandler(featureLoader: FeatureLoader) { return async (req: Request, res: Response): Promise => { @@ -27,20 +26,6 @@ export function createUpdateHandler(featureLoader: FeatureLoader) { return; } - // Validate path is within ALLOWED_ROOT_DIRECTORY - try { - validatePath(projectPath); - } catch (error) { - if (error instanceof PathNotAllowedError) { - res.status(403).json({ - success: false, - error: error.message, - }); - return; - } - throw error; - } - const updated = await featureLoader.update( projectPath, featureId, diff --git a/apps/server/src/routes/git/index.ts b/apps/server/src/routes/git/index.ts index eb8ce590..25dc333d 100644 --- a/apps/server/src/routes/git/index.ts +++ b/apps/server/src/routes/git/index.ts @@ -3,14 +3,15 @@ */ import { Router } from "express"; +import { validatePathParams } from "../../middleware/validate-paths.js"; import { createDiffsHandler } from "./routes/diffs.js"; import { createFileDiffHandler } from "./routes/file-diff.js"; export function createGitRoutes(): Router { const router = Router(); - router.post("/diffs", createDiffsHandler()); - router.post("/file-diff", createFileDiffHandler()); + router.post("/diffs", validatePathParams("projectPath"), createDiffsHandler()); + router.post("/file-diff", validatePathParams("projectPath", "filePath"), createFileDiffHandler()); return router; } diff --git a/apps/server/src/routes/git/routes/diffs.ts b/apps/server/src/routes/git/routes/diffs.ts index a258f004..eb532a03 100644 --- a/apps/server/src/routes/git/routes/diffs.ts +++ b/apps/server/src/routes/git/routes/diffs.ts @@ -5,7 +5,6 @@ import type { Request, Response } from "express"; import { getErrorMessage, logError } from "../common.js"; import { getGitRepositoryDiffs } from "../../common.js"; -import { validatePath, PathNotAllowedError } from "../../../lib/security.js"; export function createDiffsHandler() { return async (req: Request, res: Response): Promise => { @@ -17,20 +16,6 @@ export function createDiffsHandler() { return; } - // Validate paths are within ALLOWED_ROOT_DIRECTORY - try { - validatePath(projectPath); - } catch (error) { - if (error instanceof PathNotAllowedError) { - res.status(403).json({ - success: false, - error: error.message, - }); - return; - } - throw error; - } - try { const result = await getGitRepositoryDiffs(projectPath); res.json({ diff --git a/apps/server/src/routes/git/routes/file-diff.ts b/apps/server/src/routes/git/routes/file-diff.ts index 4229a123..fdf66998 100644 --- a/apps/server/src/routes/git/routes/file-diff.ts +++ b/apps/server/src/routes/git/routes/file-diff.ts @@ -7,7 +7,6 @@ import { exec } from "child_process"; import { promisify } from "util"; import { getErrorMessage, logError } from "../common.js"; import { generateSyntheticDiffForNewFile } from "../../common.js"; -import { validatePath, PathNotAllowedError } from "../../../lib/security.js"; const execAsync = promisify(exec); @@ -26,21 +25,6 @@ export function createFileDiffHandler() { return; } - // Validate paths are within ALLOWED_ROOT_DIRECTORY - try { - validatePath(projectPath); - validatePath(filePath); - } catch (error) { - if (error instanceof PathNotAllowedError) { - res.status(403).json({ - success: false, - error: error.message, - }); - return; - } - throw error; - } - try { // First check if the file is untracked const { stdout: status } = await execAsync( diff --git a/apps/server/src/routes/settings/index.ts b/apps/server/src/routes/settings/index.ts index 73944f84..53c4556e 100644 --- a/apps/server/src/routes/settings/index.ts +++ b/apps/server/src/routes/settings/index.ts @@ -14,6 +14,7 @@ import { Router } from "express"; import type { SettingsService } from "../../services/settings-service.js"; +import { validatePathParams } from "../../middleware/validate-paths.js"; import { createGetGlobalHandler } from "./routes/get-global.js"; import { createUpdateGlobalHandler } from "./routes/update-global.js"; import { createGetCredentialsHandler } from "./routes/get-credentials.js"; @@ -57,8 +58,8 @@ export function createSettingsRoutes(settingsService: SettingsService): Router { router.put("/credentials", createUpdateCredentialsHandler(settingsService)); // Project settings - router.post("/project", createGetProjectHandler(settingsService)); - router.put("/project", createUpdateProjectHandler(settingsService)); + router.post("/project", validatePathParams("projectPath"), createGetProjectHandler(settingsService)); + router.put("/project", validatePathParams("projectPath"), createUpdateProjectHandler(settingsService)); // Migration from localStorage router.post("/migrate", createMigrateHandler(settingsService)); diff --git a/apps/server/src/routes/settings/routes/get-project.ts b/apps/server/src/routes/settings/routes/get-project.ts index 9a2c9ba9..58f6ce7e 100644 --- a/apps/server/src/routes/settings/routes/get-project.ts +++ b/apps/server/src/routes/settings/routes/get-project.ts @@ -11,7 +11,6 @@ import type { Request, Response } from "express"; import type { SettingsService } from "../../../services/settings-service.js"; import { getErrorMessage, logError } from "../common.js"; -import { validatePath, PathNotAllowedError } from "../../../lib/security.js"; /** * Create handler factory for POST /api/settings/project @@ -32,20 +31,6 @@ export function createGetProjectHandler(settingsService: SettingsService) { return; } - // Validate paths are within ALLOWED_ROOT_DIRECTORY - try { - validatePath(projectPath); - } catch (error) { - if (error instanceof PathNotAllowedError) { - res.status(403).json({ - success: false, - error: error.message, - }); - return; - } - throw error; - } - const settings = await settingsService.getProjectSettings(projectPath); res.json({ diff --git a/apps/server/src/routes/settings/routes/update-project.ts b/apps/server/src/routes/settings/routes/update-project.ts index cccad9f4..5dc38df0 100644 --- a/apps/server/src/routes/settings/routes/update-project.ts +++ b/apps/server/src/routes/settings/routes/update-project.ts @@ -12,7 +12,6 @@ import type { Request, Response } from "express"; import type { SettingsService } from "../../../services/settings-service.js"; import type { ProjectSettings } from "../../../types/settings.js"; import { getErrorMessage, logError } from "../common.js"; -import { validatePath, PathNotAllowedError } from "../../../lib/security.js"; /** * Create handler factory for PUT /api/settings/project @@ -44,20 +43,6 @@ export function createUpdateProjectHandler(settingsService: SettingsService) { return; } - // Validate paths are within ALLOWED_ROOT_DIRECTORY - try { - validatePath(projectPath); - } catch (error) { - if (error instanceof PathNotAllowedError) { - res.status(403).json({ - success: false, - error: error.message, - }); - return; - } - throw error; - } - const settings = await settingsService.updateProjectSettings( projectPath, updates diff --git a/apps/server/src/routes/suggestions/index.ts b/apps/server/src/routes/suggestions/index.ts index 176ac5c2..a4b2ec20 100644 --- a/apps/server/src/routes/suggestions/index.ts +++ b/apps/server/src/routes/suggestions/index.ts @@ -4,6 +4,7 @@ import { Router } from "express"; import type { EventEmitter } from "../../lib/events.js"; +import { validatePathParams } from "../../middleware/validate-paths.js"; import { createGenerateHandler } from "./routes/generate.js"; import { createStopHandler } from "./routes/stop.js"; import { createStatusHandler } from "./routes/status.js"; @@ -11,7 +12,7 @@ import { createStatusHandler } from "./routes/status.js"; export function createSuggestionsRoutes(events: EventEmitter): Router { const router = Router(); - router.post("/generate", createGenerateHandler(events)); + router.post("/generate", validatePathParams("projectPath"), createGenerateHandler(events)); router.post("/stop", createStopHandler()); router.get("/status", createStatusHandler()); diff --git a/apps/server/src/routes/suggestions/routes/generate.ts b/apps/server/src/routes/suggestions/routes/generate.ts index d9f4582b..beafd10f 100644 --- a/apps/server/src/routes/suggestions/routes/generate.ts +++ b/apps/server/src/routes/suggestions/routes/generate.ts @@ -12,7 +12,6 @@ import { logError, } from "../common.js"; import { generateSuggestions } from "../generate-suggestions.js"; -import { validatePath, PathNotAllowedError } from "../../../lib/security.js"; const logger = createLogger("Suggestions"); @@ -29,20 +28,6 @@ export function createGenerateHandler(events: EventEmitter) { return; } - // Validate paths are within ALLOWED_ROOT_DIRECTORY - try { - validatePath(projectPath); - } catch (error) { - if (error instanceof PathNotAllowedError) { - res.status(403).json({ - success: false, - error: error.message, - }); - return; - } - throw error; - } - const { isRunning } = getSuggestionsStatus(); if (isRunning) { res.json({ diff --git a/apps/server/src/routes/worktree/index.ts b/apps/server/src/routes/worktree/index.ts index 304d0678..6d81c854 100644 --- a/apps/server/src/routes/worktree/index.ts +++ b/apps/server/src/routes/worktree/index.ts @@ -3,6 +3,7 @@ */ import { Router } from "express"; +import { validatePathParams } from "../../middleware/validate-paths.js"; import { createInfoHandler } from "./routes/info.js"; import { createStatusHandler } from "./routes/status.js"; import { createListHandler } from "./routes/list.js"; @@ -32,27 +33,27 @@ import { createListDevServersHandler } from "./routes/list-dev-servers.js"; export function createWorktreeRoutes(): Router { const router = Router(); - router.post("/info", createInfoHandler()); - router.post("/status", createStatusHandler()); + router.post("/info", validatePathParams("projectPath"), createInfoHandler()); + router.post("/status", validatePathParams("projectPath"), createStatusHandler()); router.post("/list", createListHandler()); - router.post("/diffs", createDiffsHandler()); - router.post("/file-diff", createFileDiffHandler()); - router.post("/merge", createMergeHandler()); - router.post("/create", createCreateHandler()); - router.post("/delete", createDeleteHandler()); + router.post("/diffs", validatePathParams("projectPath"), createDiffsHandler()); + router.post("/file-diff", validatePathParams("projectPath", "filePath"), createFileDiffHandler()); + router.post("/merge", validatePathParams("projectPath"), createMergeHandler()); + router.post("/create", validatePathParams("projectPath"), createCreateHandler()); + router.post("/delete", validatePathParams("projectPath", "worktreePath"), createDeleteHandler()); router.post("/create-pr", createCreatePRHandler()); router.post("/pr-info", createPRInfoHandler()); - router.post("/commit", createCommitHandler()); - router.post("/push", createPushHandler()); - router.post("/pull", createPullHandler()); + router.post("/commit", validatePathParams("worktreePath"), createCommitHandler()); + router.post("/push", validatePathParams("worktreePath"), createPushHandler()); + router.post("/pull", validatePathParams("worktreePath"), createPullHandler()); router.post("/checkout-branch", createCheckoutBranchHandler()); - router.post("/list-branches", createListBranchesHandler()); + router.post("/list-branches", validatePathParams("worktreePath"), createListBranchesHandler()); router.post("/switch-branch", createSwitchBranchHandler()); - router.post("/open-in-editor", createOpenInEditorHandler()); + router.post("/open-in-editor", validatePathParams("worktreePath"), createOpenInEditorHandler()); router.get("/default-editor", createGetDefaultEditorHandler()); - router.post("/init-git", createInitGitHandler()); + router.post("/init-git", validatePathParams("projectPath"), createInitGitHandler()); router.post("/migrate", createMigrateHandler()); - router.post("/start-dev", createStartDevHandler()); + router.post("/start-dev", validatePathParams("projectPath", "worktreePath"), createStartDevHandler()); router.post("/stop-dev", createStopDevHandler()); router.post("/list-dev-servers", createListDevServersHandler()); diff --git a/apps/server/src/routes/worktree/routes/commit.ts b/apps/server/src/routes/worktree/routes/commit.ts index 69575a90..273c7964 100644 --- a/apps/server/src/routes/worktree/routes/commit.ts +++ b/apps/server/src/routes/worktree/routes/commit.ts @@ -6,7 +6,6 @@ import type { Request, Response } from "express"; import { exec } from "child_process"; import { promisify } from "util"; import { getErrorMessage, logError } from "../common.js"; -import { validatePath, PathNotAllowedError } from "../../../lib/security.js"; const execAsync = promisify(exec); @@ -26,20 +25,6 @@ export function createCommitHandler() { return; } - // Validate paths are within ALLOWED_ROOT_DIRECTORY - try { - validatePath(worktreePath); - } catch (error) { - if (error instanceof PathNotAllowedError) { - res.status(403).json({ - success: false, - error: error.message, - }); - return; - } - throw error; - } - // Check for uncommitted changes const { stdout: status } = await execAsync("git status --porcelain", { cwd: worktreePath, diff --git a/apps/server/src/routes/worktree/routes/create.ts b/apps/server/src/routes/worktree/routes/create.ts index c8953963..690afe48 100644 --- a/apps/server/src/routes/worktree/routes/create.ts +++ b/apps/server/src/routes/worktree/routes/create.ts @@ -20,7 +20,6 @@ import { ensureInitialCommit, } from "../common.js"; import { trackBranch } from "./branch-tracking.js"; -import { validatePath, PathNotAllowedError } from "../../../lib/security.js"; const execAsync = promisify(exec); @@ -92,20 +91,6 @@ export function createCreateHandler() { return; } - // Validate paths are within ALLOWED_ROOT_DIRECTORY - try { - validatePath(projectPath); - } catch (error) { - if (error instanceof PathNotAllowedError) { - res.status(403).json({ - success: false, - error: error.message, - }); - return; - } - throw error; - } - if (!(await isGitRepo(projectPath))) { res.status(400).json({ success: false, diff --git a/apps/server/src/routes/worktree/routes/delete.ts b/apps/server/src/routes/worktree/routes/delete.ts index 08eb30d3..a0cb8eea 100644 --- a/apps/server/src/routes/worktree/routes/delete.ts +++ b/apps/server/src/routes/worktree/routes/delete.ts @@ -6,7 +6,6 @@ import type { Request, Response } from "express"; import { exec } from "child_process"; import { promisify } from "util"; import { isGitRepo, getErrorMessage, logError } from "../common.js"; -import { validatePath, PathNotAllowedError } from "../../../lib/security.js"; const execAsync = promisify(exec); @@ -27,21 +26,6 @@ export function createDeleteHandler() { return; } - // Validate paths are within ALLOWED_ROOT_DIRECTORY - try { - validatePath(projectPath); - validatePath(worktreePath); - } catch (error) { - if (error instanceof PathNotAllowedError) { - res.status(403).json({ - success: false, - error: error.message, - }); - return; - } - throw error; - } - if (!(await isGitRepo(projectPath))) { res.status(400).json({ success: false, diff --git a/apps/server/src/routes/worktree/routes/diffs.ts b/apps/server/src/routes/worktree/routes/diffs.ts index 8a94f21c..d3b6ed09 100644 --- a/apps/server/src/routes/worktree/routes/diffs.ts +++ b/apps/server/src/routes/worktree/routes/diffs.ts @@ -7,7 +7,6 @@ import path from "path"; import fs from "fs/promises"; import { getErrorMessage, logError } from "../common.js"; import { getGitRepositoryDiffs } from "../../common.js"; -import { validatePath, PathNotAllowedError } from "../../../lib/security.js"; export function createDiffsHandler() { return async (req: Request, res: Response): Promise => { @@ -27,20 +26,6 @@ export function createDiffsHandler() { return; } - // Validate paths are within ALLOWED_ROOT_DIRECTORY - try { - validatePath(projectPath); - } catch (error) { - if (error instanceof PathNotAllowedError) { - res.status(403).json({ - success: false, - error: error.message, - }); - return; - } - throw error; - } - // Git worktrees are stored in project directory const worktreePath = path.join(projectPath, ".worktrees", featureId); diff --git a/apps/server/src/routes/worktree/routes/file-diff.ts b/apps/server/src/routes/worktree/routes/file-diff.ts index 3d26bf42..70306b6a 100644 --- a/apps/server/src/routes/worktree/routes/file-diff.ts +++ b/apps/server/src/routes/worktree/routes/file-diff.ts @@ -9,7 +9,6 @@ import path from "path"; import fs from "fs/promises"; import { getErrorMessage, logError } from "../common.js"; import { generateSyntheticDiffForNewFile } from "../../common.js"; -import { validatePath, PathNotAllowedError } from "../../../lib/security.js"; const execAsync = promisify(exec); @@ -30,21 +29,6 @@ export function createFileDiffHandler() { return; } - // Validate paths are within ALLOWED_ROOT_DIRECTORY - try { - validatePath(projectPath); - validatePath(filePath); - } catch (error) { - if (error instanceof PathNotAllowedError) { - res.status(403).json({ - success: false, - error: error.message, - }); - return; - } - throw error; - } - // Git worktrees are stored in project directory const worktreePath = path.join(projectPath, ".worktrees", featureId); diff --git a/apps/server/src/routes/worktree/routes/info.ts b/apps/server/src/routes/worktree/routes/info.ts index 50dbb371..1a5bb463 100644 --- a/apps/server/src/routes/worktree/routes/info.ts +++ b/apps/server/src/routes/worktree/routes/info.ts @@ -8,7 +8,6 @@ import { promisify } from "util"; import path from "path"; import fs from "fs/promises"; import { getErrorMessage, logError, normalizePath } from "../common.js"; -import { validatePath, PathNotAllowedError } from "../../../lib/security.js"; const execAsync = promisify(exec); @@ -30,20 +29,6 @@ export function createInfoHandler() { return; } - // Validate paths are within ALLOWED_ROOT_DIRECTORY - try { - validatePath(projectPath); - } catch (error) { - if (error instanceof PathNotAllowedError) { - res.status(403).json({ - success: false, - error: error.message, - }); - return; - } - throw error; - } - // Check if worktree exists (git worktrees are stored in project directory) const worktreePath = path.join(projectPath, ".worktrees", featureId); try { diff --git a/apps/server/src/routes/worktree/routes/init-git.ts b/apps/server/src/routes/worktree/routes/init-git.ts index 49e7e64e..0aecc8af 100644 --- a/apps/server/src/routes/worktree/routes/init-git.ts +++ b/apps/server/src/routes/worktree/routes/init-git.ts @@ -8,7 +8,6 @@ import { promisify } from "util"; import { existsSync } from "fs"; import { join } from "path"; import { getErrorMessage, logError } from "../common.js"; -import { validatePath, PathNotAllowedError } from "../../../lib/security.js"; const execAsync = promisify(exec); @@ -27,20 +26,6 @@ export function createInitGitHandler() { return; } - // Validate paths are within ALLOWED_ROOT_DIRECTORY - try { - validatePath(projectPath); - } catch (error) { - if (error instanceof PathNotAllowedError) { - res.status(403).json({ - success: false, - error: error.message, - }); - return; - } - throw error; - } - // Check if .git already exists const gitDirPath = join(projectPath, ".git"); if (existsSync(gitDirPath)) { diff --git a/apps/server/src/routes/worktree/routes/list-branches.ts b/apps/server/src/routes/worktree/routes/list-branches.ts index 1b0cd9e6..0b07eb17 100644 --- a/apps/server/src/routes/worktree/routes/list-branches.ts +++ b/apps/server/src/routes/worktree/routes/list-branches.ts @@ -6,7 +6,6 @@ import type { Request, Response } from "express"; import { exec } from "child_process"; import { promisify } from "util"; import { getErrorMessage, logWorktreeError } from "../common.js"; -import { validatePath, PathNotAllowedError } from "../../../lib/security.js"; const execAsync = promisify(exec); @@ -31,20 +30,6 @@ export function createListBranchesHandler() { return; } - // Validate paths are within ALLOWED_ROOT_DIRECTORY - try { - validatePath(worktreePath); - } catch (error) { - if (error instanceof PathNotAllowedError) { - res.status(403).json({ - success: false, - error: error.message, - }); - return; - } - throw error; - } - // Get current branch const { stdout: currentBranchOutput } = await execAsync( "git rev-parse --abbrev-ref HEAD", diff --git a/apps/server/src/routes/worktree/routes/merge.ts b/apps/server/src/routes/worktree/routes/merge.ts index df109bc7..f9499d85 100644 --- a/apps/server/src/routes/worktree/routes/merge.ts +++ b/apps/server/src/routes/worktree/routes/merge.ts @@ -7,7 +7,6 @@ import { exec } from "child_process"; import { promisify } from "util"; import path from "path"; import { getErrorMessage, logError } from "../common.js"; -import { validatePath, PathNotAllowedError } from "../../../lib/security.js"; const execAsync = promisify(exec); @@ -30,20 +29,6 @@ export function createMergeHandler() { return; } - // Validate paths are within ALLOWED_ROOT_DIRECTORY - try { - validatePath(projectPath); - } catch (error) { - if (error instanceof PathNotAllowedError) { - res.status(403).json({ - success: false, - error: error.message, - }); - return; - } - throw error; - } - const branchName = `feature/${featureId}`; // Git worktrees are stored in project directory const worktreePath = path.join(projectPath, ".worktrees", featureId); diff --git a/apps/server/src/routes/worktree/routes/open-in-editor.ts b/apps/server/src/routes/worktree/routes/open-in-editor.ts index 7eecaf7d..04f9815f 100644 --- a/apps/server/src/routes/worktree/routes/open-in-editor.ts +++ b/apps/server/src/routes/worktree/routes/open-in-editor.ts @@ -7,7 +7,6 @@ import type { Request, Response } from "express"; import { exec } from "child_process"; import { promisify } from "util"; import { getErrorMessage, logError } from "../common.js"; -import { validatePath, PathNotAllowedError } from "../../../lib/security.js"; const execAsync = promisify(exec); @@ -109,20 +108,6 @@ export function createOpenInEditorHandler() { return; } - // Validate paths are within ALLOWED_ROOT_DIRECTORY - try { - validatePath(worktreePath); - } catch (error) { - if (error instanceof PathNotAllowedError) { - res.status(403).json({ - success: false, - error: error.message, - }); - return; - } - throw error; - } - const editor = await detectDefaultEditor(); try { diff --git a/apps/server/src/routes/worktree/routes/pull.ts b/apps/server/src/routes/worktree/routes/pull.ts index 5150ae7e..119192d0 100644 --- a/apps/server/src/routes/worktree/routes/pull.ts +++ b/apps/server/src/routes/worktree/routes/pull.ts @@ -6,7 +6,6 @@ import type { Request, Response } from "express"; import { exec } from "child_process"; import { promisify } from "util"; import { getErrorMessage, logError } from "../common.js"; -import { validatePath, PathNotAllowedError } from "../../../lib/security.js"; const execAsync = promisify(exec); @@ -25,20 +24,6 @@ export function createPullHandler() { return; } - // Validate paths are within ALLOWED_ROOT_DIRECTORY - try { - validatePath(worktreePath); - } catch (error) { - if (error instanceof PathNotAllowedError) { - res.status(403).json({ - success: false, - error: error.message, - }); - return; - } - throw error; - } - // Get current branch name const { stdout: branchOutput } = await execAsync( "git rev-parse --abbrev-ref HEAD", diff --git a/apps/server/src/routes/worktree/routes/push.ts b/apps/server/src/routes/worktree/routes/push.ts index bf95374c..d9447a2b 100644 --- a/apps/server/src/routes/worktree/routes/push.ts +++ b/apps/server/src/routes/worktree/routes/push.ts @@ -6,7 +6,6 @@ import type { Request, Response } from "express"; import { exec } from "child_process"; import { promisify } from "util"; import { getErrorMessage, logError } from "../common.js"; -import { validatePath, PathNotAllowedError } from "../../../lib/security.js"; const execAsync = promisify(exec); @@ -26,20 +25,6 @@ export function createPushHandler() { return; } - // Validate paths are within ALLOWED_ROOT_DIRECTORY - try { - validatePath(worktreePath); - } catch (error) { - if (error instanceof PathNotAllowedError) { - res.status(403).json({ - success: false, - error: error.message, - }); - return; - } - throw error; - } - // Get branch name const { stdout: branchOutput } = await execAsync( "git rev-parse --abbrev-ref HEAD", diff --git a/apps/server/src/routes/worktree/routes/start-dev.ts b/apps/server/src/routes/worktree/routes/start-dev.ts index 5a17b3fd..fcd0cec7 100644 --- a/apps/server/src/routes/worktree/routes/start-dev.ts +++ b/apps/server/src/routes/worktree/routes/start-dev.ts @@ -9,7 +9,6 @@ import type { Request, Response } from "express"; import { getDevServerService } from "../../../services/dev-server-service.js"; import { getErrorMessage, logError } from "../common.js"; -import { validatePath, PathNotAllowedError } from "../../../lib/security.js"; export function createStartDevHandler() { return async (req: Request, res: Response): Promise => { @@ -35,21 +34,6 @@ export function createStartDevHandler() { return; } - // Validate paths are within ALLOWED_ROOT_DIRECTORY - try { - validatePath(projectPath); - validatePath(worktreePath); - } catch (error) { - if (error instanceof PathNotAllowedError) { - res.status(403).json({ - success: false, - error: error.message, - }); - return; - } - throw error; - } - const devServerService = getDevServerService(); const result = await devServerService.startDevServer(projectPath, worktreePath); diff --git a/apps/server/src/routes/worktree/routes/status.ts b/apps/server/src/routes/worktree/routes/status.ts index d317ad0f..3f56ef17 100644 --- a/apps/server/src/routes/worktree/routes/status.ts +++ b/apps/server/src/routes/worktree/routes/status.ts @@ -8,7 +8,6 @@ import { promisify } from "util"; import path from "path"; import fs from "fs/promises"; import { getErrorMessage, logError } from "../common.js"; -import { validatePath, PathNotAllowedError } from "../../../lib/security.js"; const execAsync = promisify(exec); @@ -30,20 +29,6 @@ export function createStatusHandler() { return; } - // Validate paths are within ALLOWED_ROOT_DIRECTORY - try { - validatePath(projectPath); - } catch (error) { - if (error instanceof PathNotAllowedError) { - res.status(403).json({ - success: false, - error: error.message, - }); - return; - } - throw error; - } - // Git worktrees are stored in project directory const worktreePath = path.join(projectPath, ".worktrees", featureId); diff --git a/apps/server/src/services/agent-service.ts b/apps/server/src/services/agent-service.ts index 16fdfc53..460d784b 100644 --- a/apps/server/src/services/agent-service.ts +++ b/apps/server/src/services/agent-service.ts @@ -5,7 +5,7 @@ import { AbortError } from "@anthropic-ai/claude-agent-sdk"; import path from "path"; -import fs from "fs/promises"; +import * as secureFs from "../lib/secure-fs.js"; import type { EventEmitter } from "../lib/events.js"; import { ProviderFactory } from "../providers/provider-factory.js"; import type { ExecuteOptions } from "../providers/types.js"; @@ -63,7 +63,7 @@ export class AgentService { } async initialize(): Promise { - await fs.mkdir(this.stateDir, { recursive: true }); + await secureFs.mkdir(this.stateDir, { recursive: true }); } /** @@ -401,7 +401,7 @@ export class AgentService { const sessionFile = path.join(this.stateDir, `${sessionId}.json`); try { - const data = await fs.readFile(sessionFile, "utf-8"); + const data = await secureFs.readFile(sessionFile, "utf-8") as string; return JSON.parse(data); } catch { return []; @@ -412,7 +412,7 @@ export class AgentService { const sessionFile = path.join(this.stateDir, `${sessionId}.json`); try { - await fs.writeFile( + await secureFs.writeFile( sessionFile, JSON.stringify(messages, null, 2), "utf-8" @@ -425,7 +425,7 @@ export class AgentService { async loadMetadata(): Promise> { try { - const data = await fs.readFile(this.metadataFile, "utf-8"); + const data = await secureFs.readFile(this.metadataFile, "utf-8") as string; return JSON.parse(data); } catch { return {}; @@ -433,7 +433,7 @@ export class AgentService { } async saveMetadata(metadata: Record): Promise { - await fs.writeFile( + await secureFs.writeFile( this.metadataFile, JSON.stringify(metadata, null, 2), "utf-8" @@ -551,7 +551,7 @@ export class AgentService { // Delete session file try { const sessionFile = path.join(this.stateDir, `${sessionId}.json`); - await fs.unlink(sessionFile); + await secureFs.unlink(sessionFile); } catch { // File may not exist } diff --git a/apps/server/src/services/auto-mode-service.ts b/apps/server/src/services/auto-mode-service.ts index ae5f24cb..2fa43b8f 100644 --- a/apps/server/src/services/auto-mode-service.ts +++ b/apps/server/src/services/auto-mode-service.ts @@ -14,7 +14,7 @@ import type { ExecuteOptions } from "../providers/types.js"; import { exec } from "child_process"; import { promisify } from "util"; import path from "path"; -import fs from "fs/promises"; +import * as secureFs from "../lib/secure-fs.js"; import type { EventEmitter } from "../lib/events.js"; import { buildPromptWithImages } from "../lib/prompt-builder.js"; import { resolveModelString, DEFAULT_MODELS } from "../lib/model-resolver.js"; @@ -698,7 +698,7 @@ export class AutoModeService { let hasContext = false; try { - await fs.access(contextPath); + await secureFs.access(contextPath); hasContext = true; } catch { // No context @@ -706,7 +706,7 @@ export class AutoModeService { if (hasContext) { // Load previous context and continue - const context = await fs.readFile(contextPath, "utf-8"); + const context = await secureFs.readFile(contextPath, "utf-8") as string; return this.executeFeatureWithContext( projectPath, featureId, @@ -766,7 +766,7 @@ export class AutoModeService { const contextPath = path.join(featureDir, "agent-output.md"); let previousContext = ""; try { - previousContext = await fs.readFile(contextPath, "utf-8"); + previousContext = await secureFs.readFile(contextPath, "utf-8") as string; } catch { // No previous context } @@ -832,7 +832,7 @@ Address the follow-up instructions above. Review the previous work and make the const featureDirForImages = getFeatureDir(projectPath, featureId); const featureImagesDir = path.join(featureDirForImages, "images"); - await fs.mkdir(featureImagesDir, { recursive: true }); + await secureFs.mkdir(featureImagesDir, { recursive: true }); for (const imagePath of imagePaths) { try { @@ -841,7 +841,7 @@ Address the follow-up instructions above. Review the previous work and make the const destPath = path.join(featureImagesDir, filename); // Copy the image - await fs.copyFile(imagePath, destPath); + await secureFs.copyFile(imagePath, destPath); // Store the absolute path (external storage uses absolute paths) copiedImagePaths.push(destPath); @@ -883,7 +883,7 @@ Address the follow-up instructions above. Review the previous work and make the const featurePath = path.join(featureDirForSave, "feature.json"); try { - await fs.writeFile(featurePath, JSON.stringify(feature, null, 2)); + await secureFs.writeFile(featurePath, JSON.stringify(feature, null, 2)); } catch (error) { console.error(`[AutoMode] Failed to save feature.json:`, error); } @@ -949,7 +949,7 @@ Address the follow-up instructions above. Review the previous work and make the let workDir = projectPath; try { - await fs.access(worktreePath); + await secureFs.access(worktreePath); workDir = worktreePath; } catch { // No worktree @@ -1018,7 +1018,7 @@ Address the follow-up instructions above. Review the previous work and make the // Use the provided worktree path if given if (providedWorktreePath) { try { - await fs.access(providedWorktreePath); + await secureFs.access(providedWorktreePath); workDir = providedWorktreePath; console.log(`[AutoMode] Committing in provided worktree: ${workDir}`); } catch { @@ -1034,7 +1034,7 @@ Address the follow-up instructions above. Review the previous work and make the featureId ); try { - await fs.access(legacyWorktreePath); + await secureFs.access(legacyWorktreePath); workDir = legacyWorktreePath; console.log(`[AutoMode] Committing in legacy worktree: ${workDir}`); } catch { @@ -1097,7 +1097,7 @@ Address the follow-up instructions above. Review the previous work and make the const contextPath = path.join(featureDir, "agent-output.md"); try { - await fs.access(contextPath); + await secureFs.access(contextPath); return true; } catch { return false; @@ -1115,9 +1115,9 @@ Address the follow-up instructions above. Review the previous work and make the try { // Check if directory exists first - await fs.access(contextDir); + await secureFs.access(contextDir); - const files = await fs.readdir(contextDir); + const files = await secureFs.readdir(contextDir) as string[]; // Filter for text-based context files (case-insensitive for Windows) const textFiles = files.filter((f) => { const lower = f.toLowerCase(); @@ -1130,7 +1130,7 @@ Address the follow-up instructions above. Review the previous work and make the for (const file of textFiles) { // Use path.join for cross-platform path construction const filePath = path.join(contextDir, file); - const content = await fs.readFile(filePath, "utf-8"); + const content = await secureFs.readFile(filePath, "utf-8") as string; contents.push(`## ${file}\n\n${content}`); } @@ -1229,8 +1229,8 @@ Format your response as a structured markdown document.`; // Save analysis to .automaker directory const automakerDir = getAutomakerDir(projectPath); const analysisPath = path.join(automakerDir, "project-analysis.md"); - await fs.mkdir(automakerDir, { recursive: true }); - await fs.writeFile(analysisPath, analysisResult); + await secureFs.mkdir(automakerDir, { recursive: true }); + await secureFs.writeFile(analysisPath, analysisResult); this.emitAutoModeEvent("auto_mode_feature_complete", { featureId: analysisFeatureId, @@ -1498,7 +1498,7 @@ Format your response as a structured markdown document.`; const featurePath = path.join(featureDir, "feature.json"); try { - const data = await fs.readFile(featurePath, "utf-8"); + const data = await secureFs.readFile(featurePath, "utf-8") as string; return JSON.parse(data); } catch { return null; @@ -1515,7 +1515,7 @@ Format your response as a structured markdown document.`; const featurePath = path.join(featureDir, "feature.json"); try { - const data = await fs.readFile(featurePath, "utf-8"); + const data = await secureFs.readFile(featurePath, "utf-8") as string; const feature = JSON.parse(data); feature.status = status; feature.updatedAt = new Date().toISOString(); @@ -1527,7 +1527,7 @@ Format your response as a structured markdown document.`; // Clear the timestamp when moving to other statuses feature.justFinishedAt = undefined; } - await fs.writeFile(featurePath, JSON.stringify(feature, null, 2)); + await secureFs.writeFile(featurePath, JSON.stringify(feature, null, 2)); } catch { // Feature file may not exist } @@ -1550,7 +1550,7 @@ Format your response as a structured markdown document.`; ); try { - const data = await fs.readFile(featurePath, "utf-8"); + const data = await secureFs.readFile(featurePath, "utf-8") as string; const feature = JSON.parse(data); // Initialize planSpec if it doesn't exist @@ -1571,7 +1571,7 @@ Format your response as a structured markdown document.`; } feature.updatedAt = new Date().toISOString(); - await fs.writeFile(featurePath, JSON.stringify(feature, null, 2)); + await secureFs.writeFile(featurePath, JSON.stringify(feature, null, 2)); } catch (error) { console.error(`[AutoMode] Failed to update planSpec for ${featureId}:`, error); } @@ -1582,7 +1582,7 @@ Format your response as a structured markdown document.`; const featuresDir = getFeaturesDir(projectPath); try { - const entries = await fs.readdir(featuresDir, { withFileTypes: true }); + const entries = await secureFs.readdir(featuresDir, { withFileTypes: true }) as any[]; const allFeatures: Feature[] = []; const pendingFeatures: Feature[] = []; @@ -1595,7 +1595,7 @@ Format your response as a structured markdown document.`; "feature.json" ); try { - const data = await fs.readFile(featurePath, "utf-8"); + const data = await secureFs.readFile(featurePath, "utf-8") as string; const feature = JSON.parse(data); allFeatures.push(feature); @@ -1799,7 +1799,7 @@ This helps parse your summary correctly in the output logs.`; // Create a mock file with "yellow" content as requested in the test const mockFilePath = path.join(workDir, "yellow.txt"); - await fs.writeFile(mockFilePath, "yellow"); + await secureFs.writeFile(mockFilePath, "yellow"); this.emitAutoModeEvent("auto_mode_progress", { featureId, @@ -1824,8 +1824,8 @@ This is a mock agent response for CI/CD testing. This mock response was generated because AUTOMAKER_MOCK_AGENT=true was set. `; - await fs.mkdir(path.dirname(outputPath), { recursive: true }); - await fs.writeFile(outputPath, mockOutput); + await secureFs.mkdir(path.dirname(outputPath), { recursive: true }); + await secureFs.writeFile(outputPath, mockOutput); console.log( `[AutoMode] MOCK MODE: Completed mock execution for feature ${featureId}` @@ -1901,8 +1901,8 @@ This mock response was generated because AUTOMAKER_MOCK_AGENT=true was set. // Helper to write current responseText to file const writeToFile = async (): Promise => { try { - await fs.mkdir(path.dirname(outputPath), { recursive: true }); - await fs.writeFile(outputPath, responseText); + await secureFs.mkdir(path.dirname(outputPath), { recursive: true }); + await secureFs.writeFile(outputPath, responseText); } catch (error) { // Log but don't crash - file write errors shouldn't stop execution console.error( diff --git a/apps/server/src/services/feature-loader.ts b/apps/server/src/services/feature-loader.ts index 9b812642..888a81f0 100644 --- a/apps/server/src/services/feature-loader.ts +++ b/apps/server/src/services/feature-loader.ts @@ -4,7 +4,7 @@ */ import path from "path"; -import fs from "fs/promises"; +import * as secureFs from "../lib/secure-fs.js"; import { getFeaturesDir, getFeatureDir, @@ -88,7 +88,7 @@ export class FeatureLoader { if (!newPathSet.has(oldPath)) { try { // Paths are now absolute - await fs.unlink(oldPath); + await secureFs.unlink(oldPath); console.log(`[FeatureLoader] Deleted orphaned image: ${oldPath}`); } catch (error) { // Ignore errors when deleting (file may already be gone) @@ -116,7 +116,7 @@ export class FeatureLoader { } const featureImagesDir = this.getFeatureImagesDir(projectPath, featureId); - await fs.mkdir(featureImagesDir, { recursive: true }); + await secureFs.mkdir(featureImagesDir, { recursive: true }); const updatedPaths: Array = []; @@ -139,7 +139,7 @@ export class FeatureLoader { // Check if file exists try { - await fs.access(fullOriginalPath); + await secureFs.access(fullOriginalPath); } catch { console.warn( `[FeatureLoader] Image not found, skipping: ${fullOriginalPath}` @@ -152,14 +152,14 @@ export class FeatureLoader { const newPath = path.join(featureImagesDir, filename); // Copy the file - await fs.copyFile(fullOriginalPath, newPath); + await secureFs.copyFile(fullOriginalPath, newPath); console.log( `[FeatureLoader] Copied image: ${originalPath} -> ${newPath}` ); // Try to delete the original temp file try { - await fs.unlink(fullOriginalPath); + await secureFs.unlink(fullOriginalPath); } catch { // Ignore errors when deleting temp file } @@ -217,13 +217,13 @@ export class FeatureLoader { // Check if features directory exists try { - await fs.access(featuresDir); + await secureFs.access(featuresDir); } catch { return []; } // Read all feature directories - const entries = await fs.readdir(featuresDir, { withFileTypes: true }); + const entries = await secureFs.readdir(featuresDir, { withFileTypes: true }) as any[]; const featureDirs = entries.filter((entry) => entry.isDirectory()); // Load each feature @@ -233,7 +233,7 @@ export class FeatureLoader { const featureJsonPath = this.getFeatureJsonPath(projectPath, featureId); try { - const content = await fs.readFile(featureJsonPath, "utf-8"); + const content = await secureFs.readFile(featureJsonPath, "utf-8") as string; const feature = JSON.parse(content); if (!feature.id) { @@ -280,7 +280,7 @@ export class FeatureLoader { async get(projectPath: string, featureId: string): Promise { try { const featureJsonPath = this.getFeatureJsonPath(projectPath, featureId); - const content = await fs.readFile(featureJsonPath, "utf-8"); + const content = await secureFs.readFile(featureJsonPath, "utf-8") as string; return JSON.parse(content); } catch (error) { if ((error as NodeJS.ErrnoException).code === "ENOENT") { @@ -309,7 +309,7 @@ export class FeatureLoader { await ensureAutomakerDir(projectPath); // Create feature directory - await fs.mkdir(featureDir, { recursive: true }); + await secureFs.mkdir(featureDir, { recursive: true }); // Migrate images from temp directory to feature directory const migratedImagePaths = await this.migrateImages( @@ -328,7 +328,7 @@ export class FeatureLoader { }; // Write feature.json - await fs.writeFile( + await secureFs.writeFile( featureJsonPath, JSON.stringify(feature, null, 2), "utf-8" @@ -380,7 +380,7 @@ export class FeatureLoader { // Write back to file const featureJsonPath = this.getFeatureJsonPath(projectPath, featureId); - await fs.writeFile( + await secureFs.writeFile( featureJsonPath, JSON.stringify(updatedFeature, null, 2), "utf-8" @@ -396,7 +396,7 @@ export class FeatureLoader { async delete(projectPath: string, featureId: string): Promise { try { const featureDir = this.getFeatureDir(projectPath, featureId); - await fs.rm(featureDir, { recursive: true, force: true }); + await secureFs.rm(featureDir, { recursive: true, force: true }); console.log(`[FeatureLoader] Deleted feature ${featureId}`); return true; } catch (error) { @@ -417,7 +417,7 @@ export class FeatureLoader { ): Promise { try { const agentOutputPath = this.getAgentOutputPath(projectPath, featureId); - const content = await fs.readFile(agentOutputPath, "utf-8"); + const content = await secureFs.readFile(agentOutputPath, "utf-8") as string; return content; } catch (error) { if ((error as NodeJS.ErrnoException).code === "ENOENT") { @@ -440,10 +440,10 @@ export class FeatureLoader { content: string ): Promise { const featureDir = this.getFeatureDir(projectPath, featureId); - await fs.mkdir(featureDir, { recursive: true }); + await secureFs.mkdir(featureDir, { recursive: true }); const agentOutputPath = this.getAgentOutputPath(projectPath, featureId); - await fs.writeFile(agentOutputPath, content, "utf-8"); + await secureFs.writeFile(agentOutputPath, content, "utf-8"); } /** @@ -455,7 +455,7 @@ export class FeatureLoader { ): Promise { try { const agentOutputPath = this.getAgentOutputPath(projectPath, featureId); - await fs.unlink(agentOutputPath); + await secureFs.unlink(agentOutputPath); } catch (error) { if ((error as NodeJS.ErrnoException).code !== "ENOENT") { throw error; diff --git a/apps/server/src/services/settings-service.ts b/apps/server/src/services/settings-service.ts index d733bbd1..c51b88ce 100644 --- a/apps/server/src/services/settings-service.ts +++ b/apps/server/src/services/settings-service.ts @@ -7,8 +7,7 @@ * - Per-project settings ({projectPath}/.automaker/settings.json) */ -import fs from "fs/promises"; -import path from "path"; +import * as secureFs from "../lib/secure-fs.js"; import { createLogger } from "../lib/logger.js"; import { getGlobalSettingsPath, @@ -47,12 +46,12 @@ async function atomicWriteJson(filePath: string, data: unknown): Promise { const content = JSON.stringify(data, null, 2); try { - await fs.writeFile(tempPath, content, "utf-8"); - await fs.rename(tempPath, filePath); + await secureFs.writeFile(tempPath, content, "utf-8"); + await secureFs.rename(tempPath, filePath); } catch (error) { // Clean up temp file if it exists try { - await fs.unlink(tempPath); + await secureFs.unlink(tempPath); } catch { // Ignore cleanup errors } @@ -65,7 +64,7 @@ async function atomicWriteJson(filePath: string, data: unknown): Promise { */ async function readJsonFile(filePath: string, defaultValue: T): Promise { try { - const content = await fs.readFile(filePath, "utf-8"); + const content = await secureFs.readFile(filePath, "utf-8") as string; return JSON.parse(content) as T; } catch (error) { if ((error as NodeJS.ErrnoException).code === "ENOENT") { @@ -81,7 +80,7 @@ async function readJsonFile(filePath: string, defaultValue: T): Promise { */ async function fileExists(filePath: string): Promise { try { - await fs.access(filePath); + await secureFs.access(filePath); return true; } catch { return false; From 86d92e610b4b8d1f8404123676f2bff515899a1a Mon Sep 17 00:00:00 2001 From: Test User Date: Sat, 20 Dec 2025 20:49:28 -0500 Subject: [PATCH 6/7] refactor: streamline ALLOWED_ROOT_DIRECTORY handling and remove legacy support This commit refactors the handling of ALLOWED_ROOT_DIRECTORY by removing legacy support for ALLOWED_PROJECT_DIRS and simplifying the security logic. Key changes include: - Removed deprecated ALLOWED_PROJECT_DIRS references from .env.example and security.ts. - Updated initAllowedPaths() to focus solely on ALLOWED_ROOT_DIRECTORY and DATA_DIR. - Enhanced logging for ALLOWED_ROOT_DIRECTORY configuration status. - Adjusted route handlers to utilize the new workspace directory logic. - Introduced a centralized storage module for localStorage operations to improve consistency and error handling. These changes aim to enhance security and maintainability by consolidating directory management into a single variable. Tests: All unit tests passing. --- apps/server/.env.example | 5 - apps/server/Dockerfile | 8 + apps/server/src/lib/security.ts | 55 +++---- .../src/routes/workspace/routes/config.ts | 10 +- apps/server/tests/setup.ts | 1 - apps/server/tests/unit/lib/security.test.ts | 150 ++++++++---------- apps/ui/playwright.config.ts | 6 +- .../dialogs/file-browser-dialog.tsx | 129 ++++++++++----- apps/ui/src/components/new-project-modal.tsx | 34 ++-- .../worktree-panel/worktree-panel.tsx | 6 +- .../src/components/views/interview-view.tsx | 44 ++++- apps/ui/src/hooks/use-settings-migration.ts | 9 +- apps/ui/src/lib/electron.ts | 24 +-- apps/ui/src/lib/http-api-client.ts | 1 + apps/ui/src/lib/storage.ts | 100 ++++++++++++ apps/ui/src/lib/workspace-config.ts | 107 +++++++++++++ apps/ui/src/main.ts | 40 ++--- 17 files changed, 485 insertions(+), 244 deletions(-) create mode 100644 apps/ui/src/lib/storage.ts create mode 100644 apps/ui/src/lib/workspace-config.ts diff --git a/apps/server/.env.example b/apps/server/.env.example index a844ae33..9fbb4cbd 100644 --- a/apps/server/.env.example +++ b/apps/server/.env.example @@ -22,11 +22,6 @@ AUTOMAKER_API_KEY= # Example: ALLOWED_ROOT_DIRECTORY=/projects ALLOWED_ROOT_DIRECTORY= -# (Legacy) Restrict file operations to these directories (comma-separated) -# DEPRECATED: Use ALLOWED_ROOT_DIRECTORY instead for simpler configuration -# This is kept for backward compatibility -# ALLOWED_PROJECT_DIRS=/home/user/projects,/var/www - # CORS origin - which domains can access the API # Use "*" for development, set specific origin for production CORS_ORIGIN=* diff --git a/apps/server/Dockerfile b/apps/server/Dockerfile index 8c019a2f..8bfa8980 100644 --- a/apps/server/Dockerfile +++ b/apps/server/Dockerfile @@ -26,6 +26,14 @@ RUN npm run build --workspace=apps/server # Production stage FROM node:20-alpine +# Install git, curl, and GitHub CLI +RUN apk add --no-cache git curl && \ + GH_VERSION=$(curl -s https://api.github.com/repos/cli/cli/releases/latest | grep '"tag_name"' | cut -d '"' -f 4 | sed 's/v//') && \ + curl -L "https://github.com/cli/cli/releases/download/v${GH_VERSION}/gh_${GH_VERSION}_linux_amd64.tar.gz" -o gh.tar.gz && \ + tar -xzf gh.tar.gz && \ + mv gh_*_linux_amd64/bin/gh /usr/local/bin/gh && \ + rm -rf gh.tar.gz gh_*_linux_amd64 + WORKDIR /app # Create non-root user diff --git a/apps/server/src/lib/security.ts b/apps/server/src/lib/security.ts index fc7e0077..58dbf628 100644 --- a/apps/server/src/lib/security.ts +++ b/apps/server/src/lib/security.ts @@ -23,21 +23,27 @@ let allowedRootDirectory: string | null = null; // Data directory - always allowed for settings/credentials let dataDirectory: string | null = null; -// Allowed project directories - kept for backward compatibility and API compatibility +// Allowed paths set - stores ALLOWED_ROOT_DIRECTORY and DATA_DIR const allowedPaths = new Set(); /** * Initialize security settings from environment variables * - ALLOWED_ROOT_DIRECTORY: main security boundary * - DATA_DIR: appData exception, always allowed - * - ALLOWED_PROJECT_DIRS: legacy variable, stored for compatibility */ export function initAllowedPaths(): void { - // Load ALLOWED_ROOT_DIRECTORY (new single variable) + // Load ALLOWED_ROOT_DIRECTORY const rootDir = process.env.ALLOWED_ROOT_DIRECTORY; if (rootDir) { allowedRootDirectory = path.resolve(rootDir); allowedPaths.add(allowedRootDirectory); + console.log( + `[Security] ✓ ALLOWED_ROOT_DIRECTORY configured: ${allowedRootDirectory}` + ); + } else { + console.log( + "[Security] ⚠️ ALLOWED_ROOT_DIRECTORY not set - allowing access to all paths" + ); } // Load DATA_DIR (appData exception - always allowed) @@ -45,17 +51,7 @@ export function initAllowedPaths(): void { if (dataDir) { dataDirectory = path.resolve(dataDir); allowedPaths.add(dataDirectory); - } - - // Load legacy ALLOWED_PROJECT_DIRS for backward compatibility during transition - const dirs = process.env.ALLOWED_PROJECT_DIRS; - if (dirs) { - for (const dir of dirs.split(",")) { - const trimmed = dir.trim(); - if (trimmed) { - allowedPaths.add(path.resolve(trimmed)); - } - } + console.log(`[Security] ✓ DATA_DIR configured: ${dataDirectory}`); } } @@ -68,19 +64,13 @@ export function addAllowedPath(filePath: string): void { } /** - * Check if a path is allowed based on ALLOWED_ROOT_DIRECTORY and legacy ALLOWED_PROJECT_DIRS + * Check if a path is allowed based on ALLOWED_ROOT_DIRECTORY * Returns true if: * - Path is within ALLOWED_ROOT_DIRECTORY, OR - * - Path is within any legacy allowed path (ALLOWED_PROJECT_DIRS), OR * - Path is within DATA_DIR (appData exception), OR * - No restrictions are configured (backward compatibility) */ export function isPathAllowed(filePath: string): boolean { - // If no restrictions are configured, allow all paths (backward compatibility) - if (!allowedRootDirectory && allowedPaths.size === 0) { - return true; - } - const resolvedPath = path.resolve(filePath); // Always allow appData directory (settings, credentials) @@ -88,19 +78,21 @@ export function isPathAllowed(filePath: string): boolean { return true; } - // Allow if within ALLOWED_ROOT_DIRECTORY - if (allowedRootDirectory && isPathWithinDirectory(resolvedPath, allowedRootDirectory)) { + // If no ALLOWED_ROOT_DIRECTORY restriction is configured, allow all paths + // Note: DATA_DIR is checked above as an exception, but doesn't restrict other paths + if (!allowedRootDirectory) { return true; } - // Check legacy allowed paths (ALLOWED_PROJECT_DIRS) - for (const allowedPath of allowedPaths) { - if (isPathWithinDirectory(resolvedPath, allowedPath)) { - return true; - } + // Allow if within ALLOWED_ROOT_DIRECTORY + if ( + allowedRootDirectory && + isPathWithinDirectory(resolvedPath, allowedRootDirectory) + ) { + return true; } - // If any restrictions are configured but path doesn't match, deny + // If restrictions are configured but path doesn't match, deny return false; } @@ -132,10 +124,7 @@ export function isPathWithinDirectory( // If relative path starts with "..", it's outside the directory // If relative path is absolute, it's outside the directory // If relative path is empty or ".", it's the directory itself - return ( - !relativePath.startsWith("..") && - !path.isAbsolute(relativePath) - ); + return !relativePath.startsWith("..") && !path.isAbsolute(relativePath); } /** diff --git a/apps/server/src/routes/workspace/routes/config.ts b/apps/server/src/routes/workspace/routes/config.ts index 04b8b9a9..46a0ac67 100644 --- a/apps/server/src/routes/workspace/routes/config.ts +++ b/apps/server/src/routes/workspace/routes/config.ts @@ -5,18 +5,25 @@ import type { Request, Response } from "express"; import fs from "fs/promises"; import path from "path"; -import { addAllowedPath, getAllowedRootDirectory } from "../../../lib/security.js"; +import { + addAllowedPath, + getAllowedRootDirectory, + getDataDirectory, +} from "../../../lib/security.js"; import { getErrorMessage, logError } from "../common.js"; export function createConfigHandler() { return async (_req: Request, res: Response): Promise => { try { const allowedRootDirectory = getAllowedRootDirectory(); + const dataDirectory = getDataDirectory(); if (!allowedRootDirectory) { + // When ALLOWED_ROOT_DIRECTORY is not set, return DATA_DIR as default directory res.json({ success: true, configured: false, + defaultDir: dataDirectory || null, }); return; } @@ -41,6 +48,7 @@ export function createConfigHandler() { success: true, configured: true, workspaceDir: resolvedWorkspaceDir, + defaultDir: resolvedWorkspaceDir, }); } catch { res.json({ diff --git a/apps/server/tests/setup.ts b/apps/server/tests/setup.ts index 3ac88134..2b00c614 100644 --- a/apps/server/tests/setup.ts +++ b/apps/server/tests/setup.ts @@ -8,7 +8,6 @@ import { vi, beforeEach } from "vitest"; // Set test environment variables process.env.NODE_ENV = "test"; process.env.DATA_DIR = "/tmp/test-data"; -process.env.ALLOWED_PROJECT_DIRS = "/tmp/test-projects"; // Reset all mocks before each test beforeEach(() => { diff --git a/apps/server/tests/unit/lib/security.test.ts b/apps/server/tests/unit/lib/security.test.ts index 7f0f718f..2928fefd 100644 --- a/apps/server/tests/unit/lib/security.test.ts +++ b/apps/server/tests/unit/lib/security.test.ts @@ -11,81 +11,49 @@ describe("security.ts", () => { }); describe("initAllowedPaths", () => { - it("should parse comma-separated directories from environment", async () => { - process.env.ALLOWED_PROJECT_DIRS = "/path1,/path2,/path3"; + it("should load ALLOWED_ROOT_DIRECTORY if set", async () => { + process.env.ALLOWED_ROOT_DIRECTORY = "/projects"; process.env.DATA_DIR = ""; - const { initAllowedPaths, getAllowedPaths } = await import( - "@/lib/security.js" - ); + const { initAllowedPaths, getAllowedPaths, getAllowedRootDirectory } = + await import("@/lib/security.js"); initAllowedPaths(); const allowed = getAllowedPaths(); - expect(allowed).toContain(path.resolve("/path1")); - expect(allowed).toContain(path.resolve("/path2")); - expect(allowed).toContain(path.resolve("/path3")); - }); - - it("should trim whitespace from paths", async () => { - process.env.ALLOWED_PROJECT_DIRS = " /path1 , /path2 , /path3 "; - process.env.DATA_DIR = ""; - - const { initAllowedPaths, getAllowedPaths } = await import( - "@/lib/security.js" - ); - initAllowedPaths(); - - const allowed = getAllowedPaths(); - expect(allowed).toContain(path.resolve("/path1")); - expect(allowed).toContain(path.resolve("/path2")); + expect(allowed).toContain(path.resolve("/projects")); + expect(getAllowedRootDirectory()).toBe(path.resolve("/projects")); }); it("should always include DATA_DIR if set", async () => { - process.env.ALLOWED_PROJECT_DIRS = ""; + delete process.env.ALLOWED_ROOT_DIRECTORY; process.env.DATA_DIR = "/data/dir"; - const { initAllowedPaths, getAllowedPaths } = await import( - "@/lib/security.js" - ); + const { initAllowedPaths, getAllowedPaths } = + await import("@/lib/security.js"); initAllowedPaths(); const allowed = getAllowedPaths(); expect(allowed).toContain(path.resolve("/data/dir")); }); - it("should handle empty ALLOWED_PROJECT_DIRS", async () => { - process.env.ALLOWED_PROJECT_DIRS = ""; + it("should handle both ALLOWED_ROOT_DIRECTORY and DATA_DIR", async () => { + process.env.ALLOWED_ROOT_DIRECTORY = "/projects"; process.env.DATA_DIR = "/data"; - delete process.env.ALLOWED_ROOT_DIRECTORY; - const { initAllowedPaths, getAllowedPaths } = await import( - "@/lib/security.js" - ); + const { initAllowedPaths, getAllowedPaths } = + await import("@/lib/security.js"); initAllowedPaths(); const allowed = getAllowedPaths(); - expect(allowed).toHaveLength(1); - expect(allowed[0]).toBe(path.resolve("/data")); - }); - - it("should skip empty entries in comma list", async () => { - process.env.ALLOWED_PROJECT_DIRS = "/path1,,/path2, ,/path3"; - process.env.DATA_DIR = ""; - delete process.env.ALLOWED_ROOT_DIRECTORY; - - const { initAllowedPaths, getAllowedPaths } = await import( - "@/lib/security.js" - ); - initAllowedPaths(); - - const allowed = getAllowedPaths(); - expect(allowed).toHaveLength(3); + expect(allowed).toContain(path.resolve("/projects")); + expect(allowed).toContain(path.resolve("/data")); + expect(allowed).toHaveLength(2); }); }); describe("addAllowedPath", () => { it("should add path to allowed list", async () => { - process.env.ALLOWED_PROJECT_DIRS = ""; + delete process.env.ALLOWED_ROOT_DIRECTORY; process.env.DATA_DIR = ""; const { initAllowedPaths, addAllowedPath, getAllowedPaths } = @@ -99,7 +67,7 @@ describe("security.ts", () => { }); it("should resolve relative paths before adding", async () => { - process.env.ALLOWED_PROJECT_DIRS = ""; + delete process.env.ALLOWED_ROOT_DIRECTORY; process.env.DATA_DIR = ""; const { initAllowedPaths, addAllowedPath, getAllowedPaths } = @@ -115,14 +83,12 @@ describe("security.ts", () => { }); describe("isPathAllowed", () => { - it("should allow paths within configured allowed directories", async () => { - process.env.ALLOWED_PROJECT_DIRS = "/allowed/project"; + it("should allow paths within ALLOWED_ROOT_DIRECTORY", async () => { + process.env.ALLOWED_ROOT_DIRECTORY = "/allowed/project"; process.env.DATA_DIR = ""; - delete process.env.ALLOWED_ROOT_DIRECTORY; - const { initAllowedPaths, isPathAllowed } = await import( - "@/lib/security.js" - ); + const { initAllowedPaths, isPathAllowed } = + await import("@/lib/security.js"); initAllowedPaths(); // Paths within allowed directory should be allowed @@ -136,13 +102,11 @@ describe("security.ts", () => { }); it("should allow all paths when no restrictions are configured", async () => { - delete process.env.ALLOWED_PROJECT_DIRS; delete process.env.DATA_DIR; delete process.env.ALLOWED_ROOT_DIRECTORY; - const { initAllowedPaths, isPathAllowed } = await import( - "@/lib/security.js" - ); + const { initAllowedPaths, isPathAllowed } = + await import("@/lib/security.js"); initAllowedPaths(); // All paths should be allowed when no restrictions are configured @@ -152,17 +116,33 @@ describe("security.ts", () => { expect(isPathAllowed("/etc/passwd")).toBe(true); expect(isPathAllowed("/any/path")).toBe(true); }); + + it("should allow all paths when DATA_DIR is set but ALLOWED_ROOT_DIRECTORY is not", async () => { + process.env.DATA_DIR = "/data"; + delete process.env.ALLOWED_ROOT_DIRECTORY; + + const { initAllowedPaths, isPathAllowed } = + await import("@/lib/security.js"); + initAllowedPaths(); + + // DATA_DIR should be allowed + expect(isPathAllowed("/data/settings.json")).toBe(true); + // But all other paths should also be allowed when ALLOWED_ROOT_DIRECTORY is not set + expect(isPathAllowed("/allowed/project/file.txt")).toBe(true); + expect(isPathAllowed("/not/allowed/file.txt")).toBe(true); + expect(isPathAllowed("/tmp/file.txt")).toBe(true); + expect(isPathAllowed("/etc/passwd")).toBe(true); + expect(isPathAllowed("/any/path")).toBe(true); + }); }); describe("validatePath", () => { it("should return resolved path for allowed paths", async () => { - process.env.ALLOWED_PROJECT_DIRS = "/allowed"; + process.env.ALLOWED_ROOT_DIRECTORY = "/allowed"; process.env.DATA_DIR = ""; - delete process.env.ALLOWED_ROOT_DIRECTORY; - const { initAllowedPaths, validatePath } = await import( - "@/lib/security.js" - ); + const { initAllowedPaths, validatePath } = + await import("@/lib/security.js"); initAllowedPaths(); const result = validatePath("/allowed/file.txt"); @@ -170,13 +150,11 @@ describe("security.ts", () => { }); it("should throw error for paths outside allowed directories", async () => { - process.env.ALLOWED_PROJECT_DIRS = "/allowed"; + process.env.ALLOWED_ROOT_DIRECTORY = "/allowed"; process.env.DATA_DIR = ""; - delete process.env.ALLOWED_ROOT_DIRECTORY; - const { initAllowedPaths, validatePath } = await import( - "@/lib/security.js" - ); + const { initAllowedPaths, validatePath } = + await import("@/lib/security.js"); initAllowedPaths(); // Disallowed paths should throw PathNotAllowedError @@ -184,13 +162,11 @@ describe("security.ts", () => { }); it("should not throw error for any path when no restrictions are configured", async () => { - delete process.env.ALLOWED_PROJECT_DIRS; delete process.env.DATA_DIR; delete process.env.ALLOWED_ROOT_DIRECTORY; - const { initAllowedPaths, validatePath } = await import( - "@/lib/security.js" - ); + const { initAllowedPaths, validatePath } = + await import("@/lib/security.js"); initAllowedPaths(); // All paths are allowed when no restrictions configured @@ -202,13 +178,11 @@ describe("security.ts", () => { it("should resolve relative paths within allowed directory", async () => { const cwd = process.cwd(); - process.env.ALLOWED_PROJECT_DIRS = cwd; + process.env.ALLOWED_ROOT_DIRECTORY = cwd; process.env.DATA_DIR = ""; - delete process.env.ALLOWED_ROOT_DIRECTORY; - const { initAllowedPaths, validatePath } = await import( - "@/lib/security.js" - ); + const { initAllowedPaths, validatePath } = + await import("@/lib/security.js"); initAllowedPaths(); const result = validatePath("./file.txt"); @@ -218,26 +192,26 @@ describe("security.ts", () => { describe("getAllowedPaths", () => { it("should return array of allowed paths", async () => { - process.env.ALLOWED_PROJECT_DIRS = "/path1,/path2"; + process.env.ALLOWED_ROOT_DIRECTORY = "/projects"; process.env.DATA_DIR = "/data"; - const { initAllowedPaths, getAllowedPaths } = await import( - "@/lib/security.js" - ); + const { initAllowedPaths, getAllowedPaths } = + await import("@/lib/security.js"); initAllowedPaths(); const result = getAllowedPaths(); expect(Array.isArray(result)).toBe(true); - expect(result.length).toBeGreaterThan(0); + expect(result.length).toBe(2); + expect(result).toContain(path.resolve("/projects")); + expect(result).toContain(path.resolve("/data")); }); it("should return resolved paths", async () => { - process.env.ALLOWED_PROJECT_DIRS = "/test"; + process.env.ALLOWED_ROOT_DIRECTORY = "/test"; process.env.DATA_DIR = ""; - const { initAllowedPaths, getAllowedPaths } = await import( - "@/lib/security.js" - ); + const { initAllowedPaths, getAllowedPaths } = + await import("@/lib/security.js"); initAllowedPaths(); const result = getAllowedPaths(); diff --git a/apps/ui/playwright.config.ts b/apps/ui/playwright.config.ts index 65ab32cb..c2a4ac46 100644 --- a/apps/ui/playwright.config.ts +++ b/apps/ui/playwright.config.ts @@ -40,8 +40,7 @@ export default defineConfig({ PORT: String(serverPort), // Enable mock agent in CI to avoid real API calls AUTOMAKER_MOCK_AGENT: mockAgent ? "true" : "false", - // Allow access to test directories and common project paths - ALLOWED_PROJECT_DIRS: "/Users,/home,/tmp,/var/folders", + // No ALLOWED_ROOT_DIRECTORY restriction - allow all paths for testing }, }, // Frontend Vite dev server @@ -54,7 +53,8 @@ export default defineConfig({ ...process.env, VITE_SKIP_SETUP: "true", // Skip electron plugin in CI - no display available for Electron - VITE_SKIP_ELECTRON: process.env.CI === "true" ? "true" : undefined, + VITE_SKIP_ELECTRON: + process.env.CI === "true" ? "true" : undefined, }, }, ], diff --git a/apps/ui/src/components/dialogs/file-browser-dialog.tsx b/apps/ui/src/components/dialogs/file-browser-dialog.tsx index 1687218a..b6a05ab0 100644 --- a/apps/ui/src/components/dialogs/file-browser-dialog.tsx +++ b/apps/ui/src/components/dialogs/file-browser-dialog.tsx @@ -1,4 +1,3 @@ - import { useState, useEffect, useRef, useCallback } from "react"; import { FolderOpen, @@ -21,6 +20,11 @@ import { } from "@/components/ui/dialog"; import { Button } from "@/components/ui/button"; import { Input } from "@/components/ui/input"; +import { getJSON, setJSON } from "@/lib/storage"; +import { + getDefaultWorkspaceDirectory, + saveLastProjectDirectory, +} from "@/lib/workspace-config"; interface DirectoryEntry { name: string; @@ -50,38 +54,22 @@ const RECENT_FOLDERS_KEY = "file-browser-recent-folders"; const MAX_RECENT_FOLDERS = 5; function getRecentFolders(): string[] { - if (typeof window === "undefined") return []; - try { - const stored = localStorage.getItem(RECENT_FOLDERS_KEY); - return stored ? JSON.parse(stored) : []; - } catch { - return []; - } + return getJSON(RECENT_FOLDERS_KEY) ?? []; } function addRecentFolder(path: string): void { - if (typeof window === "undefined") return; - try { - const recent = getRecentFolders(); - // Remove if already exists, then add to front - const filtered = recent.filter((p) => p !== path); - const updated = [path, ...filtered].slice(0, MAX_RECENT_FOLDERS); - localStorage.setItem(RECENT_FOLDERS_KEY, JSON.stringify(updated)); - } catch { - // Ignore localStorage errors - } + const recent = getRecentFolders(); + // Remove if already exists, then add to front + const filtered = recent.filter((p) => p !== path); + const updated = [path, ...filtered].slice(0, MAX_RECENT_FOLDERS); + setJSON(RECENT_FOLDERS_KEY, updated); } function removeRecentFolder(path: string): string[] { - if (typeof window === "undefined") return []; - try { - const recent = getRecentFolders(); - const updated = recent.filter((p) => p !== path); - localStorage.setItem(RECENT_FOLDERS_KEY, JSON.stringify(updated)); - return updated; - } catch { - return []; - } + const recent = getRecentFolders(); + const updated = recent.filter((p) => p !== path); + setJSON(RECENT_FOLDERS_KEY, updated); + return updated; } export function FileBrowserDialog({ @@ -110,17 +98,16 @@ export function FileBrowserDialog({ } }, [open]); - const handleRemoveRecent = useCallback((e: React.MouseEvent, path: string) => { - e.stopPropagation(); - const updated = removeRecentFolder(path); - setRecentFolders(updated); - }, []); + const handleRemoveRecent = useCallback( + (e: React.MouseEvent, path: string) => { + e.stopPropagation(); + const updated = removeRecentFolder(path); + setRecentFolders(updated); + }, + [] + ); - const handleSelectRecent = useCallback((path: string) => { - browseDirectory(path); - }, []); - - const browseDirectory = async (dirPath?: string) => { + const browseDirectory = useCallback(async (dirPath?: string) => { setLoading(true); setError(""); setWarning(""); @@ -155,7 +142,14 @@ export function FileBrowserDialog({ } finally { setLoading(false); } - }; + }, []); + + const handleSelectRecent = useCallback( + (path: string) => { + browseDirectory(path); + }, + [browseDirectory] + ); // Reset current path when dialog closes useEffect(() => { @@ -169,12 +163,46 @@ export function FileBrowserDialog({ } }, [open]); - // Load initial path or home directory when dialog opens + // Load initial path or workspace directory when dialog opens useEffect(() => { if (open && !currentPath) { - browseDirectory(initialPath); + // Priority order: + // 1. Last selected directory from this file browser (from localStorage) + // 2. initialPath prop (from parent component) + // 3. Default workspace directory + // 4. Home directory + const loadInitialPath = async () => { + try { + // First, check for last selected directory from getDefaultWorkspaceDirectory + // which already implements the priority: last used > Documents/Automaker > DATA_DIR + const defaultDir = await getDefaultWorkspaceDirectory(); + + // If we have a default directory, use it (unless initialPath is explicitly provided and different) + const pathToUse = initialPath || defaultDir; + + if (pathToUse) { + // Pre-fill the path input immediately + setPathInput(pathToUse); + // Then browse to that directory + browseDirectory(pathToUse); + } else { + // No default directory, browse home directory + browseDirectory(); + } + } catch (err) { + // If config fetch fails, try initialPath or fall back to home directory + if (initialPath) { + setPathInput(initialPath); + browseDirectory(initialPath); + } else { + browseDirectory(); + } + } + }; + + loadInitialPath(); } - }, [open, initialPath]); + }, [open, initialPath, currentPath, browseDirectory]); const handleSelectDirectory = (dir: DirectoryEntry) => { browseDirectory(dir.path); @@ -211,6 +239,8 @@ export function FileBrowserDialog({ const handleSelect = useCallback(() => { if (currentPath) { addRecentFolder(currentPath); + // Save to last project directory so it's used as default next time + saveLastProjectDirectory(currentPath); onSelect(currentPath); onOpenChange(false); } @@ -296,7 +326,9 @@ export function FileBrowserDialog({ title={folder} > - {getFolderName(folder)} + + {getFolderName(folder)} + - diff --git a/apps/ui/src/components/new-project-modal.tsx b/apps/ui/src/components/new-project-modal.tsx index 0af03a5d..93eef763 100644 --- a/apps/ui/src/components/new-project-modal.tsx +++ b/apps/ui/src/components/new-project-modal.tsx @@ -1,4 +1,3 @@ - import { useState, useEffect } from "react"; import { Dialog, @@ -26,11 +25,12 @@ import { } from "lucide-react"; import { starterTemplates, type StarterTemplate } from "@/lib/templates"; import { getElectronAPI } from "@/lib/electron"; -import { getHttpApiClient } from "@/lib/http-api-client"; import { cn } from "@/lib/utils"; import { useFileBrowser } from "@/contexts/file-browser-context"; - -const LAST_PROJECT_DIR_KEY = "automaker:lastProjectDir"; +import { + getDefaultWorkspaceDirectory, + saveLastProjectDirectory, +} from "@/lib/workspace-config"; interface ValidationErrors { projectName?: boolean; @@ -81,25 +81,15 @@ export function NewProjectModal({ // Fetch workspace directory when modal opens useEffect(() => { if (open) { - // First, check localStorage for last used directory - const lastUsedDir = localStorage.getItem(LAST_PROJECT_DIR_KEY); - if (lastUsedDir) { - setWorkspaceDir(lastUsedDir); - return; - } - - // Fall back to server config if no saved directory setIsLoadingWorkspace(true); - const httpClient = getHttpApiClient(); - httpClient.workspace - .getConfig() - .then((result) => { - if (result.success && result.workspaceDir) { - setWorkspaceDir(result.workspaceDir); + getDefaultWorkspaceDirectory() + .then((defaultDir) => { + if (defaultDir) { + setWorkspaceDir(defaultDir); } }) .catch((error) => { - console.error("Failed to get workspace config:", error); + console.error("Failed to get default workspace directory:", error); }) .finally(() => { setIsLoadingWorkspace(false); @@ -211,7 +201,7 @@ export function NewProjectModal({ if (selectedPath) { setWorkspaceDir(selectedPath); // Save to localStorage for next time - localStorage.setItem(LAST_PROJECT_DIR_KEY, selectedPath); + saveLastProjectDirectory(selectedPath); // Clear any workspace error when a valid directory is selected if (errors.workspaceDir) { setErrors((prev) => ({ ...prev, workspaceDir: false })); @@ -296,9 +286,7 @@ export function NewProjectModal({ {projectPath || workspaceDir} - ) : ( - No workspace configured - )} + ) : null}