From f3c9e828e2002bf068dc35088bb5bb7481e692af Mon Sep 17 00:00:00 2001 From: Test User Date: Sat, 20 Dec 2025 18:45:39 -0500 Subject: [PATCH] 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;