Conversation
…dation 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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
|
Caution Review failedThe pull request is closed. Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughThis PR centralizes path security by introducing ALLOWED_ROOT_DIRECTORY, adds a secure-fs adapter that validates paths before all filesystem operations, enforces path validation via middleware and route-level checks, removes Google/OpenAI provider handling, and updates services, routes, tests, and UI to use the new security/storage utilities. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Router
participant ValidateMiddleware as ValidateParams
participant Handler
participant Security as security.ts
participant SecureFS as secure-fs
Client->>Router: HTTP request (with path param)
Router->>ValidateParams: invoke validatePathParams
ValidateParams->>Security: validatePath(param)
alt allowed
ValidateParams-->>Handler: next()
Handler->>Security: validatePath(targetPaths...)
Security->>SecureFS: resolve & allow
Handler->>SecureFS: readFile/writeFile/...
SecureFS->>Security: validatePath(...) (internal)
SecureFS-->>Handler: result
Handler-->>Client: 200/Success
else denied
Security-->>ValidateParams: throws PathNotAllowedError
ValidateParams-->>Client: 403 Forbidden
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested labels
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (17)
Comment |
Summary of ChangesHello @webdevcody, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a major overhaul to the application's file system security, moving from a flexible multi-directory allowance to a more secure, single-root directory constraint. This change is crucial for sandboxed deployments and improves overall system integrity by strictly controlling where file operations can occur. Additionally, it refines API key management by removing Google API key integration and enhances the user experience with better front-end validation for project creation. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
PR Review: Improve Docker Security & Path ValidationOverviewThis PR introduces a significant security improvement by implementing proper path validation and sandboxing for Docker deployments. The migration from ✅ Strengths1. Strong Security Foundation
2. Comprehensive Coverage
3. Good Test CoverageThe test file (apps/server/tests/unit/lib/security.test.ts) covers:
🔴 Critical Issues1. Missing Test Coverage for New Security FeaturesThe test file was updated but doesn't fully test the new // apps/server/tests/unit/lib/security.test.ts
// Tests reference ALLOWED_ROOT_DIRECTORY but primarily test legacy behaviorMissing test cases:
Recommendation: Add dedicated test suite for ALLOWED_ROOT_DIRECTORY: describe("ALLOWED_ROOT_DIRECTORY security", () => {
it("should block path traversal attempts", async () => {
process.env.ALLOWED_ROOT_DIRECTORY = "/projects";
const { initAllowedPaths, isPathAllowed } = await import("@/lib/security.js");
initAllowedPaths();
expect(isPathAllowed("/projects/../etc/passwd")).toBe(false);
expect(isPathAllowed("/etc/passwd")).toBe(false);
expect(isPathAllowed("/projects/safe/file.txt")).toBe(true);
});
it("should allow DATA_DIR even when ALLOWED_ROOT_DIRECTORY is set", async () => {
process.env.ALLOWED_ROOT_DIRECTORY = "/projects";
process.env.DATA_DIR = "/data";
const { initAllowedPaths, isPathAllowed } = await import("@/lib/security.js");
initAllowedPaths();
expect(isPathAllowed("/data/settings.json")).toBe(true);
expect(isPathAllowed("/projects/file.txt")).toBe(true);
});
});2. Race Condition in Path ValidationIn apps/server/src/routes/templates/routes/clone.ts:66-82, there's a TOCTOU (Time-of-check Time-of-use) vulnerability: // Validate that parent directory is within allowed root directory
if (!isPathAllowed(resolvedParent)) {
res.status(403).json({ ... });
return;
}
// Validate that project path will be within allowed root directory
if (!isPathAllowed(resolvedProject)) { // ← Check happens here
res.status(403).json({ ... });
return;
}
// Check if directory already exists
try {
await fs.access(projectPath); // ← But directory could be created between check and useImpact: Low severity but worth noting. An attacker with concurrent access could potentially exploit timing. Recommendation: This is acceptable for the current use case, but consider adding a comment explaining the TOCTOU risk is mitigated by single-user context. 3. Inconsistent Error Handling in browse.tsIn apps/server/src/routes/fs/routes/browse.ts:17-23: const targetPath = dirPath ? path.resolve(dirPath) : os.homedir();
// Validate that the path is allowed
if (!isPathAllowed(targetPath)) {
throw new PathNotAllowedError(dirPath || targetPath);
}Issue: When Recommendation: Either:
4. Security Documentation MissingThe .env.example file has good inline comments, but there's no comprehensive security documentation: Missing:
Recommendation: Add
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces significant security enhancements by sandboxing file system operations within a configurable ALLOWED_ROOT_DIRECTORY. This is a great improvement, especially for Dockerized and multi-tenant environments. The changes are consistently applied across various services and API routes, including updating Docker configurations and Electron's main process setup. The legacy ALLOWED_PROJECT_DIRS is deprecated gracefully with backward compatibility in mind. Additionally, support for Google and OpenAI providers has been removed, streamlining the focus to Anthropic.
My review includes suggestions to simplify the path validation logic, centralize error handling for better maintainability, and ensure consistent error handling patterns across different routes. Overall, this is a well-executed and important update.
| 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; | ||
| } | ||
|
|
||
| const workspaceDir = process.env.WORKSPACE_DIR; | ||
| if (workspaceDir) { | ||
| allowedPaths.add(path.resolve(workspaceDir)); | ||
| // Allow if within ALLOWED_ROOT_DIRECTORY | ||
| if (allowedRootDirectory && isPathWithinDirectory(resolvedPath, allowedRootDirectory)) { | ||
| return true; | ||
| } | ||
|
|
||
| // Check legacy allowed paths (ALLOWED_PROJECT_DIRS) | ||
| for (const allowedPath of allowedPaths) { | ||
| if (isPathWithinDirectory(resolvedPath, allowedPath)) { | ||
| return true; | ||
| } | ||
| } | ||
|
|
||
| // If any restrictions are configured but path doesn't match, deny | ||
| return false; | ||
| } |
There was a problem hiding this comment.
The isPathAllowed function contains some redundant logic. The dataDirectory and allowedRootDirectory are checked explicitly and then checked again inside the for loop because they are added to allowedPaths in initAllowedPaths. This can be simplified to a single loop over allowedPaths, which would make the code cleaner and slightly more efficient. Also, the initial check if (!allowedRootDirectory && allowedPaths.size === 0) can be simplified to if (allowedPaths.size === 0) since allowedPaths will only be empty if no directory restrictions are configured.
export function isPathAllowed(filePath: string): boolean {
// If no restrictions are configured, allow all paths (backward compatibility)
if (allowedPaths.size === 0) {
return true;
}
const resolvedPath = path.resolve(filePath);
// Check against all configured allowed paths
for (const allowedPath of allowedPaths) {
if (isPathWithinDirectory(resolvedPath, allowedPath)) {
return true;
}
}
// If any restrictions are configured but path doesn't match, deny
return false;
}| // Path not allowed - return 403 Forbidden | ||
| if (error instanceof PathNotAllowedError) { | ||
| res.status(403).json({ success: false, error: getErrorMessage(error) }); | ||
| return; | ||
| } |
There was a problem hiding this comment.
The error handling for PathNotAllowedError is duplicated across all the file system route handlers (browse.ts, delete.ts, exists.ts, mkdir.ts, read.ts, readdir.ts, stat.ts, and write.ts). To improve maintainability and reduce code duplication, consider creating a centralized Express error-handling middleware. This middleware would check for PathNotAllowedError and send the appropriate 403 response, simplifying the catch blocks in each route handler.
Example of a middleware:
// src/middleware/error-handler.ts
import { PathNotAllowedError } from "../lib/security.js";
import { getErrorMessage } from "../lib/common.js";
export function handlePathNotAllowedError(err, req, res, next) {
if (err instanceof PathNotAllowedError) {
res.status(403).json({ success: false, error: getErrorMessage(err) });
} else {
next(err);
}
}You would then register this middleware in your main Express app file after all your routes. The route handlers would need to be updated to call next(error) in their catch blocks.
| // 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; | ||
| } |
There was a problem hiding this comment.
The path validation logic here sends a 403 response directly, which is inconsistent with the pattern used in the fs routes (e.g., browse.ts, delete.ts). Those routes throw a PathNotAllowedError and handle it in a catch block. Adopting the throw pattern here would improve consistency across the codebase. This would also allow for more centralized error handling if you decide to use an error-handling middleware.
You would then need to add a catch block for PathNotAllowedError in the main try...catch block of this handler.
| // 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; | |
| } | |
| // Validate that parent directory is within allowed root directory | |
| if (!isPathAllowed(resolvedParent)) { | |
| throw new PathNotAllowedError(parentDir); | |
| } | |
| // Validate that project path will be within allowed root directory | |
| if (!isPathAllowed(resolvedProject)) { | |
| throw new PathNotAllowedError(projectPath); | |
| } |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
apps/ui/tests/utils/views/context.ts (1)
131-131: LGTM! Consider applying this pattern consistently across the file.The change to use
page.locatordirectly is correct and aligns with the direct selector pattern used inclickContextFile(line 21). However, other functions in this file still use thegetByTestIdhelper (lines 36, 47, 144, 151, 176), creating a mixed pattern.For improved consistency and maintainability, consider standardizing on one approach throughout the file—either the direct
page.locator('[data-testid="..."]')pattern (which aligns with the broader repository changes mentioned in the summary) or thegetByTestIdhelper.apps/ui/tests/spec-editor-persistence.spec.ts (1)
86-120: Consider simplifying the dual-wait strategy.The test uses both polling (lines 86-106) and a fallback
waitForFunction(lines 109-120) to verify content persistence. While this defensive approach improves reliability, it adds complexity.If you find the tests stable in CI, you could simplify by choosing one approach. However, if you're experiencing intermittent failures, this dual strategy is reasonable for robustness.
apps/ui/src/components/views/board-view/worktree-panel/components/worktree-tab.tsx (1)
194-196: Improve aria-label descriptiveness and consider data-testid sanitization.The
aria-labelon Line 195 contains only the branch name, which lacks context for screen reader users. Thetitleattribute on Line 194 is more descriptive. Additionally, thedata-testidon Line 196 uses the branch name directly, which could contain special characters (slashes, etc.) that might complicate test selectors.🔎 Suggested improvements
- title={`Click to preview ${worktree.branch}`} - aria-label={worktree.branch} - data-testid={`worktree-branch-${worktree.branch}`} + title={`Click to preview ${worktree.branch}`} + aria-label={`Select ${worktree.branch} branch`} + data-testid={`worktree-branch-${worktree.branch.replace(/[^a-zA-Z0-9-]/g, '-')}`}This makes the aria-label more descriptive and sanitizes the branch name in the data-testid to avoid potential issues with special characters.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (30)
apps/server/.env.example(1 hunks)apps/server/src/lib/security.ts(2 hunks)apps/server/src/routes/fs/routes/browse.ts(3 hunks)apps/server/src/routes/fs/routes/delete.ts(2 hunks)apps/server/src/routes/fs/routes/exists.ts(2 hunks)apps/server/src/routes/fs/routes/mkdir.ts(3 hunks)apps/server/src/routes/fs/routes/read.ts(2 hunks)apps/server/src/routes/fs/routes/readdir.ts(2 hunks)apps/server/src/routes/fs/routes/stat.ts(2 hunks)apps/server/src/routes/fs/routes/write.ts(2 hunks)apps/server/src/routes/models/routes/providers.ts(0 hunks)apps/server/src/routes/setup/routes/api-keys.ts(0 hunks)apps/server/src/routes/setup/routes/delete-api-key.ts(1 hunks)apps/server/src/routes/setup/routes/store-api-key.ts(1 hunks)apps/server/src/routes/templates/routes/clone.ts(2 hunks)apps/server/src/routes/workspace/routes/config.ts(2 hunks)apps/server/src/routes/workspace/routes/directories.ts(1 hunks)apps/server/src/services/agent-service.ts(3 hunks)apps/server/src/services/auto-mode-service.ts(4 hunks)apps/server/tests/unit/lib/security.test.ts(5 hunks)apps/ui/src/components/views/board-view/worktree-panel/components/worktree-tab.tsx(1 hunks)apps/ui/src/components/views/welcome-view.tsx(1 hunks)apps/ui/src/lib/project-init.ts(1 hunks)apps/ui/src/main.ts(2 hunks)apps/ui/tests/spec-editor-persistence.spec.ts(2 hunks)apps/ui/tests/utils/navigation/views.ts(1 hunks)apps/ui/tests/utils/views/context.ts(1 hunks)apps/ui/tests/worktree-integration.spec.ts(1 hunks)docker-compose.override.yml.example(1 hunks)docker-compose.yml(1 hunks)
💤 Files with no reviewable changes (2)
- apps/server/src/routes/models/routes/providers.ts
- apps/server/src/routes/setup/routes/api-keys.ts
🧰 Additional context used
🧬 Code graph analysis (11)
apps/server/src/routes/fs/routes/readdir.ts (2)
apps/server/src/lib/security.ts (1)
PathNotAllowedError(11-18)apps/server/src/routes/common.ts (1)
getErrorMessage(370-372)
apps/server/src/routes/fs/routes/exists.ts (2)
apps/server/src/lib/security.ts (2)
isPathAllowed(78-105)PathNotAllowedError(11-18)apps/server/src/routes/common.ts (1)
getErrorMessage(370-372)
apps/server/src/services/auto-mode-service.ts (1)
apps/server/src/lib/security.ts (2)
isPathAllowed(78-105)PathNotAllowedError(11-18)
apps/server/src/routes/workspace/routes/directories.ts (1)
apps/server/src/lib/security.ts (2)
getAllowedRootDirectory(144-146)addAllowedPath(66-68)
apps/server/src/routes/templates/routes/clone.ts (1)
apps/server/src/lib/security.ts (1)
isPathAllowed(78-105)
apps/server/src/routes/fs/routes/stat.ts (2)
apps/server/src/lib/security.ts (1)
PathNotAllowedError(11-18)apps/server/src/routes/common.ts (1)
getErrorMessage(370-372)
apps/server/src/routes/workspace/routes/config.ts (1)
apps/server/src/lib/security.ts (2)
getAllowedRootDirectory(144-146)addAllowedPath(66-68)
apps/server/src/routes/fs/routes/write.ts (2)
apps/server/src/lib/security.ts (1)
PathNotAllowedError(11-18)apps/server/src/routes/common.ts (1)
getErrorMessage(370-372)
apps/server/src/routes/fs/routes/browse.ts (2)
apps/server/src/lib/security.ts (2)
isPathAllowed(78-105)PathNotAllowedError(11-18)apps/server/src/routes/common.ts (1)
getErrorMessage(370-372)
apps/server/tests/unit/lib/security.test.ts (1)
apps/server/src/lib/security.ts (3)
initAllowedPaths(35-60)isPathAllowed(78-105)validatePath(111-119)
apps/server/src/routes/fs/routes/mkdir.ts (2)
apps/server/src/lib/security.ts (2)
isPathAllowed(78-105)PathNotAllowedError(11-18)apps/server/src/routes/common.ts (1)
getErrorMessage(370-372)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: e2e
- GitHub Check: claude-review
🔇 Additional comments (36)
apps/ui/src/lib/project-init.ts (1)
51-77: Excellent defensive validation!The three-stage validation (existence check, stat retrieval, directory verification) is well-structured with clear error messages at each stage. The early-return pattern prevents confusing downstream errors and provides actionable feedback to users. The null-safety check on line 71 (
projectStat.stats && !projectStat.stats.isDirectory) properly guards against undefined stats.apps/ui/tests/spec-editor-persistence.spec.ts (2)
49-50: LGTM! Defensive wait improves test reliability.Adding an explicit wait for the spec-view container before accessing the editor is a solid defensive measure that helps prevent race conditions when the view might not be fully initialized. The comment clarifying "(not empty state)" is helpful context.
52-122: Consistent step renumbering maintains test documentation.The step comments have been correctly renumbered (6→7, 7→8, etc.) to accommodate the new wait step. This maintains clear test flow documentation.
apps/ui/tests/utils/navigation/views.ts (1)
40-50: LGTM! Good defensive handling of loading states.The try-catch pattern appropriately handles cases where the loading element may not appear, and the timeout values are reasonable for test execution.
apps/ui/tests/worktree-integration.spec.ts (1)
106-109: 15-second timeout on worktree visibility check is an outlier—verify it addresses a genuine performance issue.Line 109 sets a 15000ms timeout for the main branch button visibility, which is the only instance of this value in the test suite (most assertions use 5-10 seconds). Research shows async wait issues account for 45% of test flakiness, and proper fixes involve awaiting responses rather than increasing timeouts. Confirm this timeout increase is necessary due to slow git/worktree operations rather than a workaround masking an underlying issue.
apps/ui/src/components/views/welcome-view.tsx (1)
242-258: LGTM! Robust pre-validation prevents invalid project creation.The parent directory validation correctly checks existence and ensures the path is actually a directory before attempting to create the project. The early-return pattern with clear error messages improves user experience and prevents downstream errors.
apps/server/src/routes/setup/routes/delete-api-key.ts (1)
73-73: LGTM! Error message accurately reflects anthropic-only support.The updated error message clearly communicates that only anthropic is supported, aligning with the provider restriction changes across the PR.
docker-compose.yml (1)
40-44: LGTM! Environment variable rename aligns with centralized security model.The shift from
ALLOWED_PROJECT_DIRStoALLOWED_ROOT_DIRECTORYwith clear container-only commentary and a sensible default (/projects) supports the PR's centralized path validation objectives.docker-compose.override.yml.example (1)
5-14: LGTM! Clear migration guidance for environment variables.The updated comments effectively explain the transition to
ALLOWED_ROOT_DIRECTORYand document the optionalWORKSPACE_DIRwith its fallback behavior. This helps users understand the configuration changes.apps/ui/src/main.ts (1)
173-189: LGTM! Consistent variable renaming aligns with security model.The rename from
defaultWorkspaceDirtodefaultRootDirectoryis applied consistently throughout the file, with appropriate updates to log messages and environment variable wiring. This maintains functionality while aligning with the centralizedALLOWED_ROOT_DIRECTORYconcept.apps/server/src/routes/setup/routes/store-api-key.ts (1)
39-45: LGTM! Explicit rejection of unsupported providers improves clarity.The addition of an explicit
elseblock that returns a 400 error for unsupported providers makes the API behavior clear and predictable. This aligns with the PR's objective to restrict provider support to anthropic only.apps/server/src/routes/fs/routes/readdir.ts (1)
7-7: LGTM! Proper 403 handling for disallowed paths.The addition of
PathNotAllowedErrorhandling with a 403 Forbidden response correctly enforces the centralized path security validation. The pattern is consistent with other filesystem routes and uses the proper HTTP status code for authorization failures.Also applies to: 31-35
apps/server/src/routes/fs/routes/delete.ts (1)
7-7: LGTM! Consistent 403 handling aligns with security model.The
PathNotAllowedErrorhandling mirrors the implementation in other filesystem routes (e.g., readdir.ts), providing consistent 403 Forbidden responses for disallowed paths. This uniform approach across the codebase strengthens the security boundary enforcement.Also applies to: 25-29
apps/server/src/routes/fs/routes/write.ts (1)
8-8: LGTM! Consistent security pattern implementation.The PathNotAllowedError handling follows a clean, consistent pattern:
- Early path validation via
validatePath()at line 25- Proper 403 response with structured error message
- Early return prevents fallthrough to generic 500 error
This aligns well with the centralized security model introduced across all fs routes.
Also applies to: 33-37
apps/server/src/routes/fs/routes/stat.ts (1)
7-7: LGTM! Consistent with other route handlers.Error handling pattern matches write.ts and other fs routes. Clean separation between path security violations (403) and other errors (500).
Also applies to: 33-37
apps/server/src/routes/fs/routes/read.ts (1)
7-7: LGTM! Proper error handling order.The 403 check correctly precedes the ENOENT handling for optional files. This prevents information leakage about file existence outside allowed directories.
Also applies to: 42-46
apps/server/src/routes/fs/routes/browse.ts (1)
9-9: LGTM! Proper layered error handling.The implementation correctly separates security validation from filesystem errors:
- Line 21-22: Path validation happens in outer try block before any fs operations
- Lines 84-106: Inner catch handles filesystem permission errors gracefully (returns success with empty dirs + warning)
- Lines 109-113: Outer catch handles PathNotAllowedError with 403
This prevents disallowed path navigation while preserving user-friendly permission denied messaging.
Also applies to: 20-23, 109-113
apps/server/src/routes/fs/routes/exists.ts (1)
8-8: LGTM! Consistent path validation pattern.Follows the same secure pattern as browse.ts with early path validation before filesystem access.
Also applies to: 23-26, 35-39
apps/server/.env.example (1)
19-28: LGTM! Clear migration path documented.The configuration change is well-documented with:
- Clear explanation of ALLOWED_ROOT_DIRECTORY purpose
- Deprecation notice for legacy ALLOWED_PROJECT_DIRS
- Example usage
- Backward compatibility note
This makes the migration straightforward for existing users.
apps/server/src/routes/workspace/routes/directories.ts (1)
24-53: LGTM! Proper integration with centralized security model.The logic correctly:
- Resolves the workspace directory from the allowed root (line 24)
- Validates directory existence before proceeding (lines 27-35)
- Adds all discovered directories to allowed paths (line 53)
- Updates error messages to reference the new terminology (line 32)
apps/server/tests/unit/lib/security.test.ts (2)
118-154: LGTM! Comprehensive test coverage for both modes.The tests properly verify:
- Path restrictions when
ALLOWED_PROJECT_DIRSis configured (lines 118-136)- Unrestricted mode when no configuration is present (lines 138-154)
This ensures backward compatibility is maintained.
158-217: LGTM! Thorough validatePath test coverage.Tests verify:
- Path resolution for allowed paths (lines 158-170)
- Error throwing for disallowed paths (lines 172-184)
- Unrestricted mode behavior (lines 186-201)
- Relative path resolution (lines 203-216)
Excellent coverage of both happy and error paths.
apps/server/src/services/agent-service.ts (2)
84-97: LGTM! Path validation correctly implemented in startConversation.The validation properly resolves the path to absolute before checking
isPathAllowed, and throwsPathNotAllowedErrorwith the original user input for clearer error messages. The validation occurs before the session is created, preventing unauthorized directory access.
474-495: LGTM! Comprehensive path validation in createSession.Good defense-in-depth approach:
- Validates the effective working directory (with proper fallback chain)
- Additionally validates
projectPathwhen explicitly providedThis ensures both the working directory and any associated project path are within allowed boundaries before session creation.
apps/server/src/routes/templates/routes/clone.ts (1)
66-82: LGTM! Proper path validation for template cloning.The implementation correctly validates both:
- The parent directory where the project will be cloned
- The resulting project path
The validation placement after path-traversal checks but before filesystem operations is the correct order. The 403 responses with descriptive messages align with the security pattern used across other routes.
apps/server/src/routes/fs/routes/mkdir.ts (2)
24-27: LGTM! Path validation before directory creation.The validation occurs immediately after path resolution and before any filesystem operations, correctly preventing unauthorized directory creation.
60-64: LGTM! Proper 403 error handling for disallowed paths.The
PathNotAllowedErroris caught and returns a 403 Forbidden response, which is the appropriate HTTP status for access denial. The error handling priority is correct - path validation errors are checked before other error types.apps/server/src/routes/workspace/routes/config.ts (2)
14-22: LGTM! Clean migration to getAllowedRootDirectory.The endpoint now properly uses the centralized security function instead of direct environment variable access, and returns
configured: falsewhen no root directory is set.
26-43: LGTM! Proper path resolution and validation.The implementation:
- Resolves to absolute path for consistency
- Validates the path is a directory
- Adds to allowed paths for subsequent operations
- Returns the resolved path to clients
apps/server/src/services/auto-mode-service.ts (3)
490-493: LGTM! Early path validation for project access.Validating
projectPathat the start ofexecuteFeatureprevents any operations on unauthorized directories. This is the correct place to fail fast before any feature loading or worktree operations.
558-561: LGTM! Second validation for computed working directory.This validation is necessary because
workDirmay differ from the originalprojectPathwhen worktrees are used. The defense-in-depth approach ensures the final working directory is also within allowed boundaries, guarding against any path manipulation during worktree resolution.
1948-1948: Cleanup: Removed Google API key reference.The error message now only references
ANTHROPIC_API_KEY, which aligns with the PR summary mentioning removal of Google API key support.apps/server/src/lib/security.ts (4)
8-18: LGTM! Well-designed custom error class.The
PathNotAllowedErrorprovides clear messaging about what paths are allowed, making it easier for users to understand security policy violations.
35-60: LGTM! Comprehensive initialization with backward compatibility.The initialization properly:
- Loads the new
ALLOWED_ROOT_DIRECTORYas the primary security boundary- Handles
DATA_DIRas an always-allowed exception- Maintains legacy
ALLOWED_PROJECT_DIRSsupport for migrationAll paths are resolved to absolute, ensuring consistent comparison regardless of how the environment variables are set.
78-105: LGTM! Robust path validation logic.The
isPathAllowedfunction correctly implements a layered security model:
- Backward compatibility when no restrictions are configured (line 80-82)
- Always allows
DATA_DIRfor app settings (lines 87-89)- Validates against
ALLOWED_ROOT_DIRECTORY(lines 92-94)- Falls back to legacy
ALLOWED_PROJECT_DIRS(lines 97-101)The path is resolved to absolute before any comparison, preventing path traversal attacks.
125-139: LGTM! Secure path containment check.The
isPathWithinDirectoryfunction correctly usespath.relativeto detect if a path escapes its intended directory. The two checks (lines 136-137) properly catch:
- Paths that traverse upward (
..prefix)- Paths on different drives/roots (absolute relative path)
This handles common path traversal attack vectors.
| const allowedRootDirectory = getAllowedRootDirectory(); | ||
|
|
||
| if (!workspaceDir) { | ||
| if (!allowedRootDirectory) { | ||
| res.status(400).json({ | ||
| success: false, | ||
| error: "WORKSPACE_DIR is not configured", | ||
| error: "ALLOWED_ROOT_DIRECTORY is not configured", | ||
| }); | ||
| return; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "directories.ts" -type f 2>/dev/null | head -20Repository: AutoMaker-Org/automaker
Length of output: 123
🏁 Script executed:
cat -n ./apps/server/src/routes/workspace/routes/directories.tsRepository: AutoMaker-Org/automaker
Length of output: 2461
🏁 Script executed:
rg "ALLOWED_ROOT_DIRECTORY" --type typescript --type javascript --type json -B 3 -A 3Repository: AutoMaker-Org/automaker
Length of output: 101
🏁 Script executed:
find . -name "*BREAK*" -o -name "*MIGRAT*" -o -name "*CHANG*" 2>/dev/null | grep -iE "\.(md|txt|doc)" | head -20Repository: AutoMaker-Org/automaker
Length of output: 49
🏁 Script executed:
rg "ALLOWED_ROOT_DIRECTORY" --type ts --type js --type json -B 2 -A 2Repository: AutoMaker-Org/automaker
Length of output: 8513
🏁 Script executed:
cat -n ./apps/server/src/lib/security.ts 2>/dev/null || cat -n ./apps/server/src/lib/security.js 2>/dev/nullRepository: AutoMaker-Org/automaker
Length of output: 5821
🏁 Script executed:
find . -maxdepth 2 -name "README*" -o -name "CHANGELOG*" -o -name ".env*" 2>/dev/null | head -20Repository: AutoMaker-Org/automaker
Length of output: 78
🏁 Script executed:
grep -r "ALLOWED_ROOT_DIRECTORY" . --include="*.md" --include="*.txt" 2>/dev/null | head -20Repository: AutoMaker-Org/automaker
Length of output: 49
🏁 Script executed:
head -100 ./README.mdRepository: AutoMaker-Org/automaker
Length of output: 4672
🏁 Script executed:
grep -A 10 -B 2 "ALLOWED\|environment\|config" ./README.md | head -50Repository: AutoMaker-Org/automaker
Length of output: 49
🏁 Script executed:
find . -name "*.env*" -o -name "*docker*" -o -name "*setup*" 2>/dev/null | grep -v node_modules | head -20Repository: AutoMaker-Org/automaker
Length of output: 682
🏁 Script executed:
cat ./apps/server/.env.exampleRepository: AutoMaker-Org/automaker
Length of output: 1830
🏁 Script executed:
rg "initAllowedPaths" --type ts -B 3 -A 3Repository: AutoMaker-Org/automaker
Length of output: 13023
🏁 Script executed:
grep -A 30 "ALLOWED_ROOT_DIRECTORY" ./apps/ui/src/main.ts | head -40Repository: AutoMaker-Org/automaker
Length of output: 1301
🏁 Script executed:
cat ./DISCLAIMER.md 2>/dev/null | head -80Repository: AutoMaker-Org/automaker
Length of output: 3710
Confirm breaking change: /directories endpoint requires ALLOWED_ROOT_DIRECTORY.
This endpoint now returns a 400 error if ALLOWED_ROOT_DIRECTORY is not configured. This is an intentional breaking change reflecting a security-first design, but the migration path is unclear:
- Electron deployments: Handled automatically with a default directory
- Direct API deployments: Will fail immediately with a 400 error
- Legacy users: Those relying on
ALLOWED_PROJECT_DIRSwill still fail on this endpoint, as it exclusively requires the new variable
Recommendations:
- Add startup validation that logs a warning if
ALLOWED_ROOT_DIRECTORYis not configured for direct API deployments - Document this breaking change explicitly in migration guides or CHANGELOG
- Consider whether legacy
ALLOWED_PROJECT_DIRSshould be accepted as a fallback for backward compatibility
🤖 Prompt for AI Agents
In apps/server/src/routes/workspace/routes/directories.ts around lines 14 to 22,
the endpoint now returns 400 when ALLOWED_ROOT_DIRECTORY is unset which breaks
direct API and legacy users; add a startup-time validation that detects when
ALLOWED_ROOT_DIRECTORY is missing in non-electron deployments and logs a clear
warning (including guidance to set ALLOWED_ROOT_DIRECTORY or migrate), update
startup/docs/CHANGELOG to document the breaking change and migration steps, and
optionally implement a fallback in this route to accept ALLOWED_PROJECT_DIRS
when ALLOWED_ROOT_DIRECTORY is absent (or else ensure the startup warning
enforces the required env and prevents running without it).
| // 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), | ||
| ]); |
There was a problem hiding this comment.
Verify that at least one element appeared after the race.
The Promise.race pattern with individual .catch(() => null) handlers will always resolve successfully, even if both waitForElement calls timeout. This means navigateToSpec could complete without either element appearing, leading to flaky tests that pass when the spec view fails to load.
🔎 Proposed fix to add verification
// 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),
- ]);
+ const result = await Promise.race([
+ waitForElement(page, "spec-view", { timeout: 10000 }).catch(() => null),
+ waitForElement(page, "spec-view-empty", { timeout: 10000 }).catch(() => null),
+ ]);
+
+ if (!result) {
+ throw new Error("Neither spec-view nor spec-view-empty appeared after loading");
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // 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), | |
| ]); | |
| // 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 | |
| const result = await Promise.race([ | |
| waitForElement(page, "spec-view", { timeout: 10000 }).catch(() => null), | |
| waitForElement(page, "spec-view-empty", { timeout: 10000 }).catch(() => null), | |
| ]); | |
| if (!result) { | |
| throw new Error("Neither spec-view nor spec-view-empty appeared after loading"); | |
| } |
🤖 Prompt for AI Agents
In apps/ui/tests/utils/navigation/views.ts around lines 52 to 58, the
Promise.race with per-call .catch(() => null) hides timeouts so navigateToSpec
can return even when neither element appears; replace the current pattern with
awaiting Promise.allSettled for both waitForElement calls (each with the same
timeout), then inspect the results and throw an error (or fail the test) if
neither promise fulfilled, ensuring at least one of "spec-view" or
"spec-view-empty" actually appeared.
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 <noreply@anthropic.com>
Pull Request Review: Security Path Validation ImplementationThis PR implements comprehensive path validation across the codebase to enforce ✅ Strengths1. Comprehensive Coverage
2. Strong Security Foundation
3. Backward Compatibility
4. Good Test Coverage
5. Clean Configuration
|
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/server/src/routes/worktree/routes/commit.ts (1)
63-63: Critical: Command injection vulnerability in git commit message handlingThe code uses
util.promisify(child_process.exec)with shell enabled to execute git commands. The user-suppliedmessageparameter from line 16 is interpolated directly into a shell command template at line 63. While double quotes are escaped, this leaves shell metacharacters like$, backticks (`),|,&,;, and\unescaped. An attacker can inject commands using patterns likemessage = 'innocent$(malicious_command)message'.Replace the template string approach with array-based command execution to prevent shell interpretation:
- await execAsync(`git commit -m "${message.replace(/"/g, '\\"')}"`, { - cwd: worktreePath, - }); + await execAsync('git', ['commit', '-m', message], { + cwd: worktreePath, + });Alternatively, use a library like
simple-gitthat handles command construction safely.
🧹 Nitpick comments (13)
apps/server/src/routes/features/routes/get.ts (1)
28-40: Security improvement looks good; consider extracting to middleware.The path validation logic correctly enforces
ALLOWED_ROOT_DIRECTORYbefore retrieving features. However, this identical 13-line block is repeated across all 5 feature route handlers (get,create,list,update,delete).Consider extracting this into Express middleware to reduce duplication:
Example middleware approach
// In a shared middleware file, e.g., ../middleware/validate-project-path.ts import { validatePath, PathNotAllowedError } from "../../../lib/security.js"; import type { Request, Response, NextFunction } from "express"; export function validateProjectPath(req: Request, res: Response, next: NextFunction): void { const { projectPath } = req.body as { projectPath?: string }; if (!projectPath) { return next(); // Let route handler deal with missing path } try { validatePath(projectPath); next(); } catch (error) { if (error instanceof PathNotAllowedError) { res.status(403).json({ success: false, error: error.message }); return; } next(error); } }Then apply to all feature routes:
router.post("/get", validateProjectPath, createGetHandler(featureLoader));apps/server/src/routes/auto-mode/routes/analyze-project.ts (1)
9-9: LGTM! Security validation correctly implemented.The path validation logic is well-structured:
- Validates before the potentially risky
analyzeProjectoperation- Returns 403 for disallowed paths, which is the appropriate status code
- Properly rethrows unexpected errors to the outer handler
The nested try-catch pattern is appropriate here since
PathNotAllowedErrorrequires distinct handling from other validation errors.
Optional nitpick: Line 25's comment says "Validate paths" (plural), but only one path is being validated. Consider "Validate path is within ALLOWED_ROOT_DIRECTORY" for precision.
Also applies to: 25-37
docker-compose.override.yml.example (1)
4-6: Consider using a more generic path placeholder in the example.The hardcoded developer-specific path
/Users/webdevcody/Workspace/automaker-workspacemight be confusing for users who copy this example. Consider using a placeholder like/path/to/your/local/projectsto make it clearer that this value needs customization.🔎 Suggested improvement
# 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 + # Example: replace the path below with your local workspace directory + - /path/to/your/local/projects:/projects:rwapps/server/src/routes/auto-mode/routes/run-feature.ts (1)
30-42: Capture and use the resolved path returned byvalidatePath.The
validatePathfunction returns a canonicalized, resolved path, but the return value is discarded. The originalprojectPathis then passed toexecuteFeatureon line 47. IfexecuteFeatureresolves the path differently internally, there could be a subtle inconsistency or potential bypass of the validation.🔎 Proposed fix to use the resolved path
// Validate path is within ALLOWED_ROOT_DIRECTORY + let validatedPath: string; try { - validatePath(projectPath); + validatedPath = 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 - .executeFeature(projectPath, featureId, useWorktrees ?? false, false) + .executeFeature(validatedPath, featureId, useWorktrees ?? false, false) .catch((error) => {apps/server/src/routes/settings/routes/update-project.ts (1)
47-59: Security validation implemented correctly.The path validation enforces
ALLOWED_ROOT_DIRECTORYconstraints before updating project settings, with appropriate 403 response for unauthorized paths. The error handling pattern is clean and consistent.💡 Optional: Consider using the resolved path
validatePathreturns the resolved (normalized) path, but it's currently not captured. Consider using the resolved path for consistency:- // Validate paths are within ALLOWED_ROOT_DIRECTORY - try { - validatePath(projectPath); - } catch (error) { + // Validate and resolve paths within ALLOWED_ROOT_DIRECTORY + let resolvedProjectPath: string; + try { + resolvedProjectPath = 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, + resolvedProjectPath, updates );This ensures the service receives the same normalized path that was validated, preventing potential inconsistencies from different path resolution contexts.
apps/server/src/routes/git/routes/diffs.ts (1)
20-32: LGTM! Path validation enhances security.The validation logic correctly prevents unauthorized path access by:
- Validating before any filesystem operations
- Returning 403 for forbidden paths with a clear error message
- Rethrowing other validation errors to the outer handler
The nested try-catch structure is intentional and appropriate, as each level handles different error scenarios (validation errors vs. git operation errors vs. unexpected errors).
If this pattern is repeated across many route handlers, consider extracting it into Express middleware for consistency and maintainability:
// Example middleware approach (for future consideration) export const validateProjectPath = (req: Request, res: Response, next: NextFunction) => { try { const { projectPath } = req.body; validatePath(projectPath); next(); } catch (error) { if (error instanceof PathNotAllowedError) { res.status(403).json({ success: false, error: error.message }); return; } next(error); } };apps/server/src/routes/settings/routes/get-project.ts (1)
36-47: Consider flattening the nested try-catch for clarity.The nested try-catch is functional but could be simplified. Since this appears to be a consistent pattern across routes, this is purely optional.
Alternative pattern (optional)
Move the PathNotAllowedError handling to the outer catch block:
- // Validate path is within ALLOWED_ROOT_DIRECTORY or DATA_DIR - try { - validatePath(projectPath); - } catch (error) { - if (error instanceof PathNotAllowedError) { - res.status(403).json({ - success: false, - error: error.message, - }); - return; - } - throw error; - } + // Validate path is within ALLOWED_ROOT_DIRECTORY or DATA_DIR + validatePath(projectPath); const settings = await settingsService.getProjectSettings(projectPath); res.json({ success: true, settings, }); } catch (error) { + if (error instanceof PathNotAllowedError) { + res.status(403).json({ + success: false, + error: error.message, + }); + return; + } logError(error, "Get project settings failed"); res.status(500).json({ success: false, error: getErrorMessage(error) }); }This achieves the same result with a flatter structure.
apps/server/src/routes/worktree/routes/open-in-editor.ts (2)
112-124: Path validation successfully added.The validation correctly ensures
worktreePathis withinALLOWED_ROOT_DIRECTORYbefore proceeding with editor operations, returning 403 for disallowed paths.This validation pattern is duplicated across multiple route files (merge.ts, pull.ts, delete.ts, diffs.ts, create.ts). Consider extracting it into a reusable Express middleware or helper function to reduce duplication and improve maintainability:
Example middleware approach
// In lib/security.ts or middleware/path-validation.ts export function validateRequestPaths(...pathKeys: string[]) { return (req: Request, res: Response, next: NextFunction) => { try { for (const key of pathKeys) { const pathValue = req.body[key]; if (pathValue) { validatePath(pathValue); } } next(); } catch (error) { if (error instanceof PathNotAllowedError) { res.status(403).json({ success: false, error: error.message, }); return; } next(error); } }; }Usage:
router.post('/open-in-editor', validateRequestPaths('worktreePath'), createOpenInEditorHandler());
129-129: Consider shell injection hardening.While path validation limits the attack surface, shell commands using template string interpolation with file paths can still be vulnerable to injection if paths contain shell metacharacters (especially double quotes). Paths like
/allowed/dir"evil; rm -rf /tmp; echo "could break out of the quoted string.Consider using Node.js
execFile(which doesn't invoke a shell) instead ofexecto eliminate shell injection risks:Safer approach using execFile
-import { exec } from "child_process"; +import { execFile } from "child_process"; import { promisify } from "util"; -const execAsync = promisify(exec); +const execFileAsync = promisify(execFile); // In detectDefaultEditor: try { - await execAsync("which cursor || where cursor"); + await execFileAsync("which", ["cursor"]); cachedEditor = { name: "Cursor", command: "cursor" }; return cachedEditor; } catch { // Cursor not found, try alternatives } // In createOpenInEditorHandler: try { - await execAsync(`${editor.command} "${worktreePath}"`); + await execFileAsync(editor.command, [worktreePath]); // ... } catch (editorError) { // Fallback - await execAsync(openCommand); + if (platform === "darwin") { + await execFileAsync("open", [worktreePath]); + } else if (platform === "win32") { + await execFileAsync("explorer", [worktreePath]); + } else { + await execFileAsync("xdg-open", [worktreePath]); + } }Alternatively, use a shell-escaping library like
shell-quoteif shell features (like||in line 33) are required.Also applies to: 144-154
apps/server/src/routes/worktree/routes/merge.ts (1)
58-64: Consider shell injection hardening for git commands.Similar to open-in-editor.ts, shell commands with interpolated variables (especially
branchNameandworktreePath) could be vulnerable if those values contain shell metacharacters. WhileworktreePathis now path-validated andbranchNameis user-controlled, neither is shell-escaped.Consider using
execFilewith git arguments passed as an array, or sanitize/validate thebranchNameformat (e.g., ensure it matches^[a-zA-Z0-9/_-]+$) before use in shell commands.Example with execFile
import { execFile } from "child_process"; const execFileAsync = promisify(execFile); // Instead of: await execAsync(`git worktree remove "${worktreePath}" --force`, { cwd: projectPath }); await execAsync(`git branch -D ${branchName}`, { cwd: projectPath }); // Use: await execFileAsync("git", ["worktree", "remove", worktreePath, "--force"], { cwd: projectPath }); await execFileAsync("git", ["branch", "-D", branchName], { cwd: projectPath });Also applies to: 78-81
apps/server/src/routes/worktree/routes/pull.ts (1)
43-69: Consider shell injection hardening.Git commands use interpolated
worktreePath(validated) andbranchName(user-controlled) without shell escaping. Consider usingexecFileto pass arguments safely, or validatebranchNameformat.Example with execFile
import { execFile } from "child_process"; const execFileAsync = promisify(execFile); // Replace exec calls: const { stdout: branchOutput } = await execFileAsync("git", ["rev-parse", "--abbrev-ref", "HEAD"], { cwd: worktreePath }); await execFileAsync("git", ["fetch", "origin"], { cwd: worktreePath }); const { stdout: status } = await execFileAsync("git", ["status", "--porcelain"], { cwd: worktreePath }); const { stdout: pullOutput } = await execFileAsync("git", ["pull", "origin", branchName], { cwd: worktreePath });apps/server/src/routes/worktree/routes/delete.ts (1)
56-77: Consider shell injection hardening.Git commands use interpolated
worktreePathandbranchNamewithout shell escaping. While path validation limits risk, usingexecFilewould eliminate shell injection entirely.Example with execFile
import { execFile } from "child_process"; const execFileAsync = promisify(execFile); // Replace: const { stdout } = await execFileAsync("git", ["rev-parse", "--abbrev-ref", "HEAD"], { cwd: worktreePath }); await execFileAsync("git", ["worktree", "remove", worktreePath, "--force"], { cwd: projectPath }); await execFileAsync("git", ["worktree", "prune"], { cwd: projectPath }); await execFileAsync("git", ["branch", "-D", branchName], { cwd: projectPath });apps/server/src/routes/worktree/routes/create.ts (1)
152-171: Consider shell injection hardening.Git commands use interpolated
branchNameandworktreePathwithout shell escaping. Consider usingexecFileor validatingbranchNameformat to prevent potential injection.Example with execFile and format validation
// Validate branchName format before use const BRANCH_NAME_REGEX = /^[a-zA-Z0-9/_-]+$/; if (!BRANCH_NAME_REGEX.test(branchName)) { res.status(400).json({ success: false, error: "Invalid branch name format", }); return; } // Use execFile for safety: import { execFile } from "child_process"; const execFileAsync = promisify(execFile); await execFileAsync("git", ["rev-parse", "--verify", branchName], { cwd: projectPath }); if (branchExists) { await execFileAsync("git", ["worktree", "add", worktreePath, branchName], { cwd: projectPath }); } else { await execFileAsync("git", ["worktree", "add", "-b", branchName, worktreePath, base], { cwd: projectPath }); }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (30)
apps/server/src/routes/agent/routes/send.ts(2 hunks)apps/server/src/routes/agent/routes/start.ts(2 hunks)apps/server/src/routes/auto-mode/routes/analyze-project.ts(2 hunks)apps/server/src/routes/auto-mode/routes/commit-feature.ts(2 hunks)apps/server/src/routes/auto-mode/routes/run-feature.ts(2 hunks)apps/server/src/routes/features/routes/create.ts(2 hunks)apps/server/src/routes/features/routes/delete.ts(2 hunks)apps/server/src/routes/features/routes/get.ts(2 hunks)apps/server/src/routes/features/routes/list.ts(2 hunks)apps/server/src/routes/features/routes/update.ts(2 hunks)apps/server/src/routes/git/routes/diffs.ts(2 hunks)apps/server/src/routes/git/routes/file-diff.ts(2 hunks)apps/server/src/routes/settings/routes/get-project.ts(2 hunks)apps/server/src/routes/settings/routes/update-project.ts(2 hunks)apps/server/src/routes/suggestions/routes/generate.ts(2 hunks)apps/server/src/routes/worktree/routes/commit.ts(2 hunks)apps/server/src/routes/worktree/routes/create.ts(2 hunks)apps/server/src/routes/worktree/routes/delete.ts(2 hunks)apps/server/src/routes/worktree/routes/diffs.ts(2 hunks)apps/server/src/routes/worktree/routes/file-diff.ts(2 hunks)apps/server/src/routes/worktree/routes/info.ts(2 hunks)apps/server/src/routes/worktree/routes/init-git.ts(2 hunks)apps/server/src/routes/worktree/routes/list-branches.ts(2 hunks)apps/server/src/routes/worktree/routes/merge.ts(2 hunks)apps/server/src/routes/worktree/routes/open-in-editor.ts(2 hunks)apps/server/src/routes/worktree/routes/pull.ts(2 hunks)apps/server/src/routes/worktree/routes/push.ts(2 hunks)apps/server/src/routes/worktree/routes/start-dev.ts(2 hunks)apps/server/src/routes/worktree/routes/status.ts(2 hunks)docker-compose.override.yml.example(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (21)
apps/server/src/routes/worktree/routes/merge.ts (1)
apps/server/src/lib/security.ts (2)
validatePath(111-119)PathNotAllowedError(11-18)
apps/server/src/routes/auto-mode/routes/analyze-project.ts (1)
apps/server/src/lib/security.ts (2)
validatePath(111-119)PathNotAllowedError(11-18)
apps/server/src/routes/worktree/routes/open-in-editor.ts (1)
apps/server/src/lib/security.ts (2)
validatePath(111-119)PathNotAllowedError(11-18)
apps/server/src/routes/features/routes/update.ts (1)
apps/server/src/lib/security.ts (2)
validatePath(111-119)PathNotAllowedError(11-18)
apps/server/src/routes/worktree/routes/file-diff.ts (1)
apps/server/src/lib/security.ts (2)
validatePath(111-119)PathNotAllowedError(11-18)
apps/server/src/routes/worktree/routes/pull.ts (1)
apps/server/src/lib/security.ts (2)
validatePath(111-119)PathNotAllowedError(11-18)
apps/server/src/routes/worktree/routes/status.ts (1)
apps/server/src/lib/security.ts (2)
validatePath(111-119)PathNotAllowedError(11-18)
apps/server/src/routes/settings/routes/get-project.ts (1)
apps/server/src/lib/security.ts (2)
validatePath(111-119)PathNotAllowedError(11-18)
apps/server/src/routes/worktree/routes/list-branches.ts (1)
apps/server/src/lib/security.ts (2)
validatePath(111-119)PathNotAllowedError(11-18)
apps/server/src/routes/worktree/routes/create.ts (1)
apps/server/src/lib/security.ts (2)
validatePath(111-119)PathNotAllowedError(11-18)
apps/server/src/routes/settings/routes/update-project.ts (1)
apps/server/src/lib/security.ts (2)
validatePath(111-119)PathNotAllowedError(11-18)
apps/server/src/routes/worktree/routes/init-git.ts (1)
apps/server/src/lib/security.ts (2)
validatePath(111-119)PathNotAllowedError(11-18)
apps/server/src/routes/agent/routes/send.ts (1)
apps/server/src/lib/security.ts (2)
validatePath(111-119)PathNotAllowedError(11-18)
apps/server/src/routes/features/routes/create.ts (1)
apps/server/src/lib/security.ts (2)
validatePath(111-119)PathNotAllowedError(11-18)
apps/server/src/routes/features/routes/list.ts (1)
apps/server/src/lib/security.ts (2)
validatePath(111-119)PathNotAllowedError(11-18)
apps/server/src/routes/worktree/routes/push.ts (1)
apps/server/src/lib/security.ts (2)
validatePath(111-119)PathNotAllowedError(11-18)
apps/server/src/routes/git/routes/file-diff.ts (1)
apps/server/src/lib/security.ts (2)
validatePath(111-119)PathNotAllowedError(11-18)
apps/server/src/routes/auto-mode/routes/run-feature.ts (1)
apps/server/src/lib/security.ts (2)
validatePath(111-119)PathNotAllowedError(11-18)
apps/server/src/routes/worktree/routes/diffs.ts (1)
apps/server/src/lib/security.ts (2)
validatePath(111-119)PathNotAllowedError(11-18)
apps/server/src/routes/worktree/routes/start-dev.ts (1)
apps/server/src/lib/security.ts (2)
validatePath(111-119)PathNotAllowedError(11-18)
apps/server/src/routes/worktree/routes/delete.ts (1)
apps/server/src/lib/security.ts (2)
validatePath(111-119)PathNotAllowedError(11-18)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: claude-review
- GitHub Check: e2e
🔇 Additional comments (33)
apps/server/src/routes/auto-mode/routes/commit-feature.ts (2)
8-8: LGTM! Security module import added.The import correctly brings in the path validation utilities needed for the new security checks.
29-44: LGTM! Path validation adds important security layer.The validation correctly checks both
projectPathand optionalworktreePathbefore proceeding with the commit operation. The 403 response for unauthorized paths is appropriate.The nested try-catch pattern is functional, though slightly unconventional. Since the AI summary indicates this pattern is used consistently across multiple routes, maintaining consistency is more valuable than refactoring this instance alone.
apps/server/src/routes/features/routes/create.ts (1)
31-43: Path validation correctly guards feature creation.The validation ensures
projectPathis withinALLOWED_ROOT_DIRECTORYbefore creating features. The middleware refactor suggested inget.tswould also eliminate this duplication.apps/server/src/routes/features/routes/list.ts (1)
22-34: LGTM!Path validation correctly enforces security boundary before listing features. Consistent with the pattern across other feature routes.
apps/server/src/routes/features/routes/update.ts (1)
30-42: LGTM!Path validation correctly enforces security boundary before updating features.
apps/server/src/routes/features/routes/delete.ts (1)
28-40: LGTM!Path validation correctly enforces security boundary before deleting features. This completes the consistent security enforcement across all feature CRUD operations.
docker-compose.override.yml.example (1)
7-10: LGTM! Environment configuration aligns correctly.The
ALLOWED_ROOT_DIRECTORYenvironment variable is properly configured to match the container mount point/projects. The comments clearly explain the security constraints, and this aligns with the centralized path allowlist enforcement introduced in the broader PR changes.apps/server/src/routes/auto-mode/routes/run-feature.ts (1)
9-9: Excellent security addition!The path validation logic correctly enforces the allowlist before executing features, and the error handling appropriately returns a 403 response for disallowed paths while re-throwing unexpected errors.
Also applies to: 30-42
apps/server/src/routes/settings/routes/update-project.ts (1)
15-15: LGTM!The import statement correctly brings in the security validation utilities needed for path validation.
apps/server/src/routes/suggestions/routes/generate.ts (2)
15-15: LGTM: Security utilities imported correctly.The import of
validatePathandPathNotAllowedErrorproperly supports the new path validation logic below.
32-44: LGTM: Path validation properly enforces security boundary.The early validation of
projectPathcorrectly prevents unauthorized access before any operations begin. The error handling appropriately returns 403 for path violations while re-throwing unexpected errors.apps/server/src/routes/git/routes/diffs.ts (1)
8-8: LGTM! Security module import is appropriate.The import of
validatePathandPathNotAllowedErrorenables path validation to prevent potential path traversal attacks.apps/server/src/routes/worktree/routes/push.ts (1)
9-9: LGTM!The import statement correctly brings in the necessary security utilities for path validation.
apps/server/src/routes/settings/routes/get-project.ts (1)
14-14: LGTM: Security module integration.The import correctly brings in the path validation utilities needed for enforcing security boundaries.
apps/server/src/routes/worktree/routes/start-dev.ts (2)
12-12: LGTM! Security imports added.The import of
validatePathandPathNotAllowedErrorenables the path security validation added below.
38-51: Path validation is consistently implemented across all worktree routes.The security enhancement in start-dev.ts follows the same validation pattern found across all worktree route handlers (create, commit, delete, diffs, file-diff, info, init-git, list-branches, merge, open-in-editor, pull, push, and status). All routes uniformly import validatePath and PathNotAllowedError from the security module, wrap path validation in try-catch blocks, and return 403 for forbidden paths while propagating unexpected errors.
apps/server/src/routes/worktree/routes/status.ts (1)
11-11: Import added correctly.The security module imports are correctly added to support path validation.
apps/server/src/routes/worktree/routes/file-diff.ts (1)
12-12: LGTM: Security module import added.The import of security validation utilities is correct and necessary for the path validation logic.
apps/server/src/routes/worktree/routes/info.ts (2)
11-11: LGTM! Security utilities imported correctly.The import of
validatePathandPathNotAllowedErroraligns with the centralized path validation approach introduced across the codebase.
33-45: Path validation implemented correctly.The validation logic properly enforces
ALLOWED_ROOT_DIRECTORYconstraints and returns appropriate 403 responses for unauthorized paths.apps/server/src/routes/agent/routes/send.ts (2)
9-9: LGTM! Security imports added correctly.The import of
validatePathandPathNotAllowedErrorfrom the security module is appropriate for the path validation functionality being added.
33-68: Well-structured validation flow with appropriate error handling.The overall approach is solid:
- Path validation occurs before starting the asynchronous
sendMessageoperation, failing fast for invalid paths- The nested try-catch pattern appropriately distinguishes
PathNotAllowedError(403) from other errors (500)- The immediate response (line 68) maintains the documented behavior where actual responses come via WebSocket
- Clear comment explains the validation purpose
However, please address the critical issue regarding the use of resolved paths (see separate comment).
apps/server/src/routes/worktree/routes/list-branches.ts (1)
9-9: LGTM - Security module imports added correctly.The imports align with the project-wide path validation initiative and match the security module exports shown in the relevant code snippets.
apps/server/src/routes/worktree/routes/commit.ts (1)
9-9: LGTM! Path validation properly integrated.The addition of path validation using the new security module is well-implemented:
- Correct placement after input validation but before git operations
- Proper error handling that catches
PathNotAllowedErrorspecifically and returns 403- Response structure consistent with existing error patterns
- Other errors are correctly re-thrown to the outer handler
This change effectively prevents path traversal attacks on the commit endpoint.
Also applies to: 29-41
apps/server/src/routes/agent/routes/start.ts (2)
9-9: Good addition for centralized security enforcement.The security imports are correctly added and align with the PR's objective to enforce path allowlist validation across routes.
28-42: Path validation is properly enforced for both explicit and default working directories.The code validates the
workingDirectoryparameter at the route level when explicitly provided. The default path (process.cwd()) is automatically validated at the service layer inagentService.startConversation()viaisPathAllowed()before being used, ensuring both user-supplied and default paths respect security boundaries.Optional: Simplify nested error handling.
The nested try-catch structure (catching
PathNotAllowedErrorinside a broader try-catch) is functionally correct but adds complexity. Consider extracting path validation before the main try-catch or using a helper function to streamline the error flow.Likely an incorrect or invalid review comment.
apps/server/src/routes/worktree/routes/merge.ts (1)
33-45: Path validation correctly applied.The validation ensures
projectPathis withinALLOWED_ROOT_DIRECTORYbefore git operations. SinceworktreePathis derived from the validatedprojectPath(line 49), both paths are protected.apps/server/src/routes/worktree/routes/pull.ts (1)
28-40: Path validation correctly enforces security boundary.The validation ensures
worktreePathis withinALLOWED_ROOT_DIRECTORYbefore executing git operations.apps/server/src/routes/worktree/routes/delete.ts (1)
30-43: Both paths correctly validated.The validation ensures both
projectPathandworktreePathare withinALLOWED_ROOT_DIRECTORYbefore deletion operations.apps/server/src/routes/worktree/routes/diffs.ts (1)
30-42: Path validation correctly applied.The validation ensures
projectPathis withinALLOWED_ROOT_DIRECTORY. SinceworktreePathis derived fromprojectPath(line 45) and paths are used ascwdparameters rather than in shell command strings, the approach is secure.apps/server/src/routes/worktree/routes/create.ts (1)
95-107: Path validation correctly enforces security boundary.The validation ensures
projectPathis withinALLOWED_ROOT_DIRECTORYbefore worktree creation. SinceworktreePathis derived from the validatedprojectPath(line 144), both paths are protected.apps/server/src/routes/worktree/routes/init-git.ts (1)
11-11: LGTM: Security primitives properly imported.The import of
validatePathandPathNotAllowedErrorcorrectly brings in the centralized security validation utilities.apps/server/src/routes/git/routes/file-diff.ts (1)
10-10: LGTM: Security imports added.The import of
validatePathandPathNotAllowedErroris correct and aligns with the security hardening objectives.
| // 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; | ||
| } |
There was a problem hiding this comment.
Critical: Use the resolved paths returned by validatePath.
The validatePath function returns the resolved path after validation, but this return value is currently discarded. The original paths from req.body are passed to sendMessage, which could lead to path resolution inconsistencies if the paths are resolved differently within the service.
Security implication: If relative paths are provided and sendMessage resolves them relative to a different base directory, the validation could potentially be bypassed.
Recommendation: Capture and use the resolved paths returned by validatePath to ensure the exact paths that were validated are the ones being used.
🔎 Proposed fix to use resolved paths
+ // Validate and resolve paths to ensure they're within ALLOWED_ROOT_DIRECTORY
+ let resolvedWorkingDirectory: string | undefined = workingDirectory;
+ let resolvedImagePaths: string[] | undefined = imagePaths;
+
- // Validate paths are within ALLOWED_ROOT_DIRECTORY
try {
if (workingDirectory) {
- validatePath(workingDirectory);
+ resolvedWorkingDirectory = validatePath(workingDirectory);
}
if (imagePaths && imagePaths.length > 0) {
- for (const imagePath of imagePaths) {
- validatePath(imagePath);
- }
+ resolvedImagePaths = imagePaths.map((imagePath) => 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({
sessionId,
message,
- workingDirectory,
- imagePaths,
+ workingDirectory: resolvedWorkingDirectory,
+ imagePaths: resolvedImagePaths,
model,
})Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In apps/server/src/routes/agent/routes/send.ts around lines 33 to 52, the code
calls validatePath(...) but discards its resolved return value; capture the
returned resolved workingDirectory and resolved image paths (map imagePaths
through validatePath) and replace the original req.body paths with these
resolved values before calling sendMessage; keep the existing
PathNotAllowedError handling and rethrow behavior, but ensure sendMessage
receives the validated/resolved workingDirectory and imagePaths arrays so the
actual validated paths cannot be bypassed.
| // 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; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check the actual file-diff.ts implementation
echo "=== Checking file-diff.ts git route ==="
cat apps/server/src/routes/git/routes/file-diff.ts 2>/dev/null | head -100
echo -e "\n=== Checking if path module is imported ==="
head -20 apps/server/src/routes/git/routes/file-diff.ts 2>/dev/null | grep -i "import.*path"
echo -e "\n=== Checking validatePath implementation ==="
fd validatePath --type f | head -5
echo -e "\n=== Checking worktree file-diff ==="
cat apps/server/src/routes/worktree/routes/file-diff.ts 2>/dev/null | head -100Repository: AutoMaker-Org/automaker
Length of output: 5349
🏁 Script executed:
#!/bin/bash
# Find and examine validatePath implementation
echo "=== Finding security module ==="
find . -name "security.js" -o -name "security.ts" | head -5
echo -e "\n=== Examining validatePath function ==="
cat apps/server/src/lib/security.ts 2>/dev/null | head -150Repository: AutoMaker-Org/automaker
Length of output: 4593
🏁 Script executed:
#!/bin/bash
# Verify the vulnerability scenario and check other similar patterns
echo "=== Check if projectPath is absolute ==="
rg -n "projectPath" apps/server/src/routes/git/routes/file-diff.ts -A 2 -B 2 | head -30
echo -e "\n=== Check generateSyntheticDiffForNewFile implementation ==="
rg -n "generateSyntheticDiffForNewFile" apps/server/src/routes/common.ts -A 20 | head -50
echo -e "\n=== Look for similar validation patterns in other routes ==="
fd "file-diff\|git-diff" --type f | xargs grep -l "validatePath" 2>/dev/null
echo -e "\n=== Check if there are any other places validating relative file paths ==="
rg -n "validatePath.*filePath" apps/server/src --type=tsRepository: AutoMaker-Org/automaker
Length of output: 3469
🏁 Script executed:
#!/bin/bash
# Verify the actual vulnerability with a concrete scenario
echo "=== Verify path.resolve behavior when filePath contains traversal ==="
node <<'EOF'
const path = require('path');
// Scenario from the review
const projectPath = "/allowed/project";
const filePath = "../../etc/passwd";
console.log("=== Current (vulnerable) validation ===");
const resolvedFilePath = path.resolve(filePath);
console.log("validatePath(filePath) resolves to:", resolvedFilePath);
console.log("\n=== Actual file accessed ===");
const actualPath = path.join(projectPath, filePath);
console.log("path.join(projectPath, filePath):", actualPath);
console.log("\n=== Are these the same? ===");
console.log("Same:", resolvedFilePath === actualPath);
console.log("\n=== What should be validated ===");
const correctValidation = path.resolve(path.join(projectPath, filePath));
console.log("path.resolve(path.join(...)):", correctValidation);
console.log("\n=== Worktree scenario ===");
const worktreePath = path.join(projectPath, ".worktrees", "feature-1");
const actualWorktreePath = path.join(worktreePath, filePath);
console.log("Actual access in worktree:", actualWorktreePath);
EOF
echo -e "\n=== Check if path is imported in git file-diff.ts ==="
grep "^import.*path" apps/server/src/routes/git/routes/file-diff.ts || echo "NOT IMPORTED"
echo -e "\n=== Check all files using validatePath with file paths ==="
rg -l "validatePath" apps/server/src/routes --type=tsRepository: AutoMaker-Org/automaker
Length of output: 2300
🏁 Script executed:
#!/bin/bash
# Check if other routes have the same vulnerability pattern
echo "=== Check fs/read.ts (uses filePath directly) ==="
cat apps/server/src/routes/fs/routes/read.ts | head -50
echo -e "\n=== Check fs/write.ts (uses filePath directly) ==="
cat apps/server/src/routes/fs/routes/write.ts | head -50
echo -e "\n=== Check if any route correctly validates combined path ==="
rg -n "path\.join.*validatePath|validatePath.*path\.join" apps/server/src/routes --type=tsRepository: AutoMaker-Org/automaker
Length of output: 3183
🏁 Script executed:
#!/bin/bash
# Final verification: check both file-diff routes side by side for comparison
echo "=== Git file-diff validation block ==="
sed -n '29,42p' apps/server/src/routes/git/routes/file-diff.ts
echo -e "\n=== Worktree file-diff validation block ==="
sed -n '33,46p' apps/server/src/routes/worktree/routes/file-diff.ts
echo -e "\n=== Check how filePath is used in git commands in both files ==="
echo "Git file-diff usage:"
rg -n "filePath" apps/server/src/routes/git/routes/file-diff.ts | grep -E "(cwd|git|diff)"
echo -e "\nWorktree file-diff usage:"
rg -n "filePath" apps/server/src/routes/worktree/routes/file-diff.ts | grep -E "(cwd|git|diff)"
echo -e "\n=== Confirm path module not in git file-diff imports ==="
head -15 apps/server/src/routes/git/routes/file-diff.ts | grep "import"Repository: AutoMaker-Org/automaker
Length of output: 2012
Critical security vulnerability: Path validation bypass in file-diff routes.
Both apps/server/src/routes/git/routes/file-diff.ts (line 32) and apps/server/src/routes/worktree/routes/file-diff.ts (line 36) validate filePath independently, but the actual file accessed is path.join(projectPath, filePath) or path.join(worktreePath, filePath). This creates a path traversal vulnerability.
Example: filePath = "../../etc/passwd" passes validation against the server's current directory but accesses /allowed/project/../../etc/passwd = /allowed/etc/passwd relative to the project.
Both files require the fix:
+import path from "path";
+
// Validate paths are within ALLOWED_ROOT_DIRECTORY
try {
validatePath(projectPath);
- validatePath(filePath);
+ validatePath(path.join(projectPath, filePath));
} catch (error) {(For worktree, use path.join(projectPath, ".worktrees", featureId, filePath) to account for the full access path.)
🤖 Prompt for AI Agents
In apps/server/src/routes/git/routes/file-diff.ts around lines 29 to 42, the
code validates projectPath and filePath separately which allows path-traversal
when the actual access uses path.join(projectPath, filePath); instead validate
the fully resolved/joined path (resolve path.join(projectPath, filePath)) with
validatePath and ensure it is within ALLOWED_ROOT_DIRECTORY before accessing the
file; for the worktree variant
(apps/server/src/routes/worktree/routes/file-diff.ts around line 36) do the same
but validate the resolved path using path.join(projectPath, ".worktrees",
featureId, filePath) so the final canonical path is checked rather than the
individual components.
| return; | ||
| } | ||
|
|
||
| // Validate paths are within ALLOWED_ROOT_DIRECTORY |
There was a problem hiding this comment.
Update comment to reflect both allowed directories.
The comment mentions only ALLOWED_ROOT_DIRECTORY, but based on the PathNotAllowedError message in security.ts, the validation also allows DATA_DIR.
🔎 Suggested fix
- // Validate paths are within ALLOWED_ROOT_DIRECTORY
+ // Validate path is within ALLOWED_ROOT_DIRECTORY or DATA_DIR🤖 Prompt for AI Agents
In apps/server/src/routes/settings/routes/get-project.ts around line 35, update
the inline comment that currently references only ALLOWED_ROOT_DIRECTORY so it
accurately reflects both allowed directories (ALLOWED_ROOT_DIRECTORY and
DATA_DIR) used by the path validation; change the comment text to mention both
constants (and optionally a brief note that paths are validated against either
ALLOWED_ROOT_DIRECTORY or DATA_DIR) so it matches the PathNotAllowedError
behavior in security.ts.
| // 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; | ||
| } |
There was a problem hiding this comment.
Critical: Incorrect path validation allows potential path traversal.
The validation logic has several security and correctness issues:
-
Incorrect
filePathvalidation:filePathis used as a relative path in git commands (lines 56, 69) withcwd: worktreePath, butvalidatePath(filePath)resolves it relative to the server's working directory, not the worktree. This causes incorrect validation. -
Path traversal vulnerability: If
filePathcontains..segments (e.g.,"../../../../../../etc/passwd"), the resolved path could escape the worktree bounds even thoughprojectPathis validated. -
Missing
worktreePathvalidation: The constructedworktreePath(line 49) is never validated to ensure it's within allowed directories. -
Unused return values:
validatePathreturns the resolved path, but the return values are discarded.
Correct approach: Validate the complete resolved path by joining worktreePath and filePath before use.
🔎 Proposed fix for proper path validation
- // Validate paths are within ALLOWED_ROOT_DIRECTORY
+ // Validate projectPath is 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);
+
+ // Validate worktree path and full file path
+ try {
+ validatePath(worktreePath);
+ const fullFilePath = path.join(worktreePath, filePath);
+ validatePath(fullFilePath);
+ } catch (error) {
+ if (error instanceof PathNotAllowedError) {
+ res.status(403).json({
+ success: false,
+ error: error.message,
+ });
+ return;
+ }
+ throw error;
+ }Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In apps/server/src/routes/worktree/routes/file-diff.ts around lines 33 to 46,
the current validation calls validatePath(projectPath) and
validatePath(filePath) separately and discards return values, which allows path
traversal because filePath is validated relative to the server CWD instead of
the worktree and worktreePath itself is never validated; fix by calling
validatePath on the resolved project/worktree path
(validatePath(worktreePathResolved) using the returned resolved path), then join
and normalize worktreePathResolved with filePath (e.g.,
path.join(worktreePathResolved, filePath) and path.resolve) and call
validatePath on that combined resolved path, use the returned resolved paths for
cwd and git ops, and keep the PathNotAllowedError handling as-is to return 403
on invalid paths.
| // Check if worktree exists (git worktrees are stored in project directory) | ||
| const worktreePath = path.join(projectPath, ".worktrees", featureId); |
There was a problem hiding this comment.
Potential path traversal vulnerability with featureId.
The featureId parameter from user input is used to construct worktreePath without validation or sanitization. If featureId contains path traversal sequences like ../../.., the resulting path could escape the validated projectPath directory and bypass the ALLOWED_ROOT_DIRECTORY security boundary.
Example attack:
projectPath = "/allowed/project"(passes validation)featureId = "../../../etc"worktreePath = "/allowed/project/.worktrees/../../../etc"(escapes allowed directory)
Validate the constructed worktreePath or sanitize featureId to prevent path traversal.
🔎 Proposed fix: Validate the constructed worktreePath
// Check if worktree exists (git worktrees are stored in project directory)
const worktreePath = path.join(projectPath, ".worktrees", featureId);
+
+// Validate the constructed worktree path
+try {
+ validatePath(worktreePath);
+} catch (error) {
+ if (error instanceof PathNotAllowedError) {
+ res.status(403).json({
+ success: false,
+ error: error.message,
+ });
+ return;
+ }
+ throw error;
+}
+
try {
await fs.access(worktreePath);Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In apps/server/src/routes/worktree/routes/info.ts around lines 47-48, featureId
is concatenated into a filesystem path allowing path traversal; ensure the
resolved worktreePath stays inside the intended project directory before use:
construct the candidate path with path.join(projectPath, ".worktrees",
featureId), then compute its absolute normalized path using path.resolve (or
path.normalize+resolve) and verify it starts with the resolved projectPath (or
ALLOWED_ROOT_DIRECTORY) prefix; if the check fails, reject the request
(400/403). Alternatively, sanitize featureId up-front by disallowing path
separators or ".." segments and only allow a safe pattern (e.g., alphanumeric,
hyphen, underscore) before joining.
| // 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; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n apps/server/src/routes/worktree/routes/init-git.ts | sed -n '1,100p'Repository: AutoMaker-Org/automaker
Length of output: 2585
🏁 Script executed:
#!/bin/bash
# Find and read the init-git.ts file
find . -name init-git.ts -type f | head -5Repository: AutoMaker-Org/automaker
Length of output: 119
🏁 Script executed:
#!/bin/bash
# Search for the init-git.ts file
git ls-files | grep -E "(init-git|worktree)" | head -20Repository: AutoMaker-Org/automaker
Length of output: 1070
🏁 Script executed:
#!/bin/bash
# Check if the file exists and read it
if [ -f "apps/server/src/routes/worktree/routes/init-git.ts" ]; then
echo "File found. Reading lines 1-100:"
head -100 "apps/server/src/routes/worktree/routes/init-git.ts" | cat -n
fiRepository: AutoMaker-Org/automaker
Length of output: 2618
🏁 Script executed:
#!/bin/bash
# List the worktree routes directory
ls -la apps/server/src/routes/worktree/routes/ 2>/dev/null || echo "Directory not found or listing failed"Repository: AutoMaker-Org/automaker
Length of output: 1616
🏁 Script executed:
#!/bin/bash
# Search for validatePath function definition
rg -n "validatePath" --type ts --type tsx 2>/dev/null | head -20Repository: AutoMaker-Org/automaker
Length of output: 49
🏁 Script executed:
#!/bin/bash
# Search for the init-git file
fd "init-git" --type f 2>/dev/null | head -10Repository: AutoMaker-Org/automaker
Length of output: 117
🏁 Script executed:
#!/bin/bash
# Look for typescript files in worktree
fd . "apps/server/src/routes" --name "*.ts" 2>/dev/null | head -30Repository: AutoMaker-Org/automaker
Length of output: 49
🏁 Script executed:
#!/bin/bash
# 1. Find the init-git.ts file
echo "=== Searching for init-git.ts ==="
find . -path "*/worktree/*" -name "*.ts" -type f 2>/dev/null
echo -e "\n=== Searching for validatePath definition ==="
rg -n "^\s*(export\s+)?(function|const)\s+validatePath" --type ts 2>/dev/null | head -20Repository: AutoMaker-Org/automaker
Length of output: 1630
🏁 Script executed:
#!/bin/bash
# Get file info and contents of the init-git.ts file
if [ -f "apps/server/src/routes/worktree/routes/init-git.ts" ]; then
echo "=== File found. Reading content (lines 20-70) ==="
sed -n '20,70p' "apps/server/src/routes/worktree/routes/init-git.ts" | cat -n
else
echo "File not found at expected path, searching..."
find . -name "init-git.ts" 2>/dev/null
fiRepository: AutoMaker-Org/automaker
Length of output: 1706
🏁 Script executed:
#!/bin/bash
# Search for validatePath usage and its definition
rg -B 2 -A 5 "validatePath\(" --type ts | head -50Repository: AutoMaker-Org/automaker
Length of output: 3394
🏁 Script executed:
#!/bin/bash
# Find where validatePath is defined
rg -n "function validatePath|const validatePath|export.*validatePath" --type tsRepository: AutoMaker-Org/automaker
Length of output: 159
🏁 Script executed:
#!/bin/bash
git ls-files | grep -E "\.ts$" | grep -i "git\|validate" | head -20Repository: AutoMaker-Org/automaker
Length of output: 421
🏁 Script executed:
#!/bin/bash
# Comprehensive verification of the TOCTOU vulnerability claim
echo "=== Step 1: Find the init-git.ts file ==="
FILE_PATH=$(find . -name "init-git.ts" -type f 2>/dev/null | head -1)
if [ -z "$FILE_PATH" ]; then
echo "ERROR: init-git.ts not found"
exit 1
fi
echo "Found: $FILE_PATH"
echo -e "\n=== Step 2: Read lines 30-42 from the file ==="
sed -n '30,42p' "$FILE_PATH" | cat -n
echo -e "\n=== Step 3: Find validatePath definition ==="
rg -n "validatePath" --type ts | grep -E "(function|const|export)" | head -5
echo -e "\n=== Step 4: Check for similar patterns in other routes ==="
rg -n "validatePath" --type ts --glob 'apps/server/src/routes/**/*.ts' | wc -l
echo "Total validatePath calls found:"
rg -n "validatePath" --type ts --glob 'apps/server/src/routes/**/*.ts'Repository: AutoMaker-Org/automaker
Length of output: 9336
🏁 Script executed:
#!/bin/bash
set -e
# 1. Locate file
echo "[1] Locating init-git.ts"
FILE=$(find . -name "init-git.ts" 2>/dev/null | head -1)
if [ -z "$FILE" ]; then
echo "File not found!"
exit 1
fi
echo "Found at: $FILE"
# 2. Read the problematic section
echo -e "\n[2] Code snippet (lines 25-50)"
wc -l "$FILE"
sed -n '25,50p' "$FILE" | nl
# 3. Find validatePath definition
echo -e "\n[3] Looking for validatePath definition"
rg "function validatePath|export.*validatePath|const validatePath" --type ts -A 3
# 4. Check related code usage
echo -e "\n[4] Usage of validatePath in routes"
rg -n "validatePath" --type ts apps/server/src/routes 2>/dev/null || echo "No matches in routes"
# 5. Examine cwd usage with execAsync
echo -e "\n[5] Check execAsync calls with cwd"
rg -n "execAsync.*cwd" --type ts apps/server/src/routes 2>/dev/nullRepository: AutoMaker-Org/automaker
Length of output: 303
🏁 Script executed:
#!/bin/bash
# Main verification script
echo "Verifying TOCTOU vulnerability in init-git.ts"
# Find the target file
TARGET=$(find . -path "*worktree/routes/init-git.ts" 2>/dev/null)
if [ -n "$TARGET" ]; then
echo "=== File content (lines 1-80) ==="
cat "$TARGET" | head -80 | nl
echo -e "\n=== Checking validatePath definition ==="
grep -rn "function validatePath\|const validatePath" --include="*.ts" . 2>/dev/null | head -10
echo -e "\n=== All validatePath calls in routes ==="
grep -rn "validatePath(" --include="*.ts" apps/server/src/routes 2>/dev/null
else
echo "Target file not found. Searching for similar files..."
find . -name "*init-git*" 2>/dev/null
fiRepository: AutoMaker-Org/automaker
Length of output: 3974
🏁 Script executed:
#!/bin/bash
# Systematic verification of init-git.ts TOCTOU vulnerability
# 1. Find and read the file
file_path=$(git ls-files | grep "init-git.ts" | head -1)
if [ -z "$file_path" ]; then
echo "File not found via git ls-files, trying find..."
file_path=$(find . -name "init-git.ts" 2>/dev/null | head -1)
fi
if [ -n "$file_path" ]; then
echo "=== File: $file_path ==="
echo "Lines 25-65:"
sed -n '25,65p' "$file_path" | cat -n
echo -e "\n=== Looking for validatePath function ==="
rg -n "export.*validatePath|function validatePath|const validatePath.*=" --type ts -A 5
echo -e "\n=== Examining the vulnerability claim ==="
echo "Looking for: validatePath() called without capturing return"
rg -B 3 -A 3 "validatePath\(" "$file_path"
echo -e "\n=== Checking usage of projectPath after validation ==="
sed -n '30,70p' "$file_path" | grep -n "projectPath"
else
echo "File not found"
exit 1
fiRepository: AutoMaker-Org/automaker
Length of output: 2379
🏁 Script executed:
#!/bin/bash
# Verification script for TOCTOU vulnerability in init-git.ts
rg -n "init-git" --type ts | head -5Repository: AutoMaker-Org/automaker
Length of output: 491
🏁 Script executed:
git ls-files "*.ts" | grep -i initRepository: AutoMaker-Org/automaker
Length of output: 149
🏁 Script executed:
fd -e ts -p "*init*"Repository: AutoMaker-Org/automaker
Length of output: 352
🏁 Script executed:
ls -la apps/server/src/routes/worktree/routes/ 2>/dev/null || find . -type d -name "worktree" 2>/dev/nullRepository: AutoMaker-Org/automaker
Length of output: 1616
🏁 Script executed:
#!/bin/bash
# Verify the TOCTOU claim in init-git.ts
# 1. Try to locate the file
target_file="apps/server/src/routes/worktree/routes/init-git.ts"
if [ -f "$target_file" ]; then
echo "=== File found at $target_file ==="
echo "=== Lines 25-70 (with line numbers) ==="
sed -n '25,70p' "$target_file" | cat -n
echo -e "\n=== Searching for validatePath definition ==="
find . -name "*.ts" -type f -exec grep -l "function validatePath\|export.*validatePath" {} \; 2>/dev/null | head -5
echo -e "\n=== Checking if validatePath returns a value ==="
grep -rn "return.*validatePath\|validatePath.*=" . --include="*.ts" 2>/dev/null | head -10
else
echo "File not found at: $target_file"
echo "Searching for it..."
find . -name "init-git.ts" 2>/dev/null
fiRepository: AutoMaker-Org/automaker
Length of output: 2309
🏁 Script executed:
#!/bin/bash
# Comprehensive verification of init-git.ts TOCTOU vulnerability
echo "=== Locating and reading init-git.ts ==="
file_to_check="apps/server/src/routes/worktree/routes/init-git.ts"
if [ -f "$file_to_check" ]; then
echo "File found. Reading relevant sections..."
echo -e "\n--- Lines 1-50 with line numbers ---"
head -50 "$file_to_check" | cat -n
echo -e "\n--- Lines 30-70 (focus area) ---"
sed -n '30,70p' "$file_to_check" | cat -n
echo -e "\n=== Searching for validatePath function definition ==="
rg "function validatePath|export.*validatePath|const validatePath" --type ts -A 8
echo -e "\n=== Checking all validatePath usages in the file ==="
rg -n "validatePath" "$file_to_check"
else
echo "File not found at $file_to_check. Searching..."
find . -type f -name "init-git.ts" 2>/dev/null
fiRepository: AutoMaker-Org/automaker
Length of output: 4022
🏁 Script executed:
cat -n apps/server/src/routes/worktree/routes/init-git.ts 2>/dev/null | head -100Repository: AutoMaker-Org/automaker
Length of output: 2585
🏁 Script executed:
rg -n "validatePath" apps/server/src/routes --type ts 2>/dev/nullRepository: AutoMaker-Org/automaker
Length of output: 8059
🏁 Script executed:
rg -B 2 -A 10 "function validatePath|export.*validatePath" --type ts 2>/dev/nullRepository: AutoMaker-Org/automaker
Length of output: 762
🏁 Script executed:
#!/bin/bash
# Verify init-git.ts code and validatePath function
echo "=== Reading init-git.ts (first 100 lines) ==="
head -100 apps/server/src/routes/worktree/routes/init-git.ts 2>/dev/null | cat -n
echo -e "\n=== Finding validatePath function ==="
rg -n "validatePath" --type ts -B 2 -A 5 2>/dev/null | head -50
echo -e "\n=== Verify the vulnerability claim - check other routes ==="
rg -A 5 "validatePath\(" apps/server/src/routes --type ts 2>/dev/null | head -80Repository: AutoMaker-Org/automaker
Length of output: 12568
🏁 Script executed:
#!/bin/bash
# Step 1: Check if file exists and read it
if [ -f "apps/server/src/routes/worktree/routes/init-git.ts" ]; then
echo "=== File Content (Lines 1-100) ==="
cat -n "apps/server/src/routes/worktree/routes/init-git.ts" | head -100
# Step 2: Find validatePath definition
echo -e "\n=== Searching for validatePath definition ==="
rg "function validatePath|const validatePath|export" --type ts | grep -i validate | head -20
# Step 3: Check usage patterns
echo -e "\n=== Checking how validatePath is used across routes ==="
rg -n "validatePath\(" --type ts apps/server/src/routes/ | head -30
else
echo "File not found"
find . -name "init-git.ts" 2>/dev/null
fiRepository: AutoMaker-Org/automaker
Length of output: 5854
🏁 Script executed:
git ls-files apps/server/src/routes/worktree/routes/init-git.ts 2>/dev/null && wc -l apps/server/src/routes/worktree/routes/init-git.ts && sed -n '1,100p' apps/server/src/routes/worktree/routes/init-git.ts | cat -nRepository: AutoMaker-Org/automaker
Length of output: 2690
🏁 Script executed:
#!/bin/bash
# Verification of TOCTOU vulnerability claim in init-git.ts
# Check if file exists
if [ -f "apps/server/src/routes/worktree/routes/init-git.ts" ]; then
echo "=== init-git.ts exists. Lines 1-100: ==="
cat -n "apps/server/src/routes/worktree/routes/init-git.ts" | head -100
echo -e "\n=== Search for validatePath definition across the codebase: ==="
rg "function validatePath|export.*validatePath|const validatePath\s*=" --type ts -A 10
echo -e "\n=== Check usage of validatePath in routes: ==="
rg -n "validatePath" --type ts apps/server/src/routes/
echo -e "\n=== Check other init files for similar patterns: ==="
rg -n "validatePath" --type ts --glob "apps/server/src/routes/**/*.ts" | head -30
else
echo "File not found. Searching for worktree routes..."
find . -path "*worktree*" -name "*.ts" 2>/dev/null | head -20
fiRepository: AutoMaker-Org/automaker
Length of output: 14653
🏁 Script executed:
#!/bin/bash
# First, verify the file exists and its structure
echo "=== Step 1: Verify file and read content ==="
if [ ! -f "apps/server/src/routes/worktree/routes/init-git.ts" ]; then
echo "File not found, attempting to locate it..."
find . -name "init-git.ts" -type f
exit 1
fi
echo "File found. Total lines:"
wc -l "apps/server/src/routes/worktree/routes/init-git.ts"
echo -e "\n=== Lines 20-75 (focusing on lines 30-42 mentioned in review) ==="
sed -n '20,75p' "apps/server/src/routes/worktree/routes/init-git.ts" | cat -n
echo -e "\n=== Step 2: Find validatePath function definition ==="
rg "function validatePath|export.*validatePath|const validatePath\s*=" --type ts -A 12
echo -e "\n=== Step 3: All validatePath calls in server routes ==="
rg -n "validatePath\(" --type ts "apps/server/src/routes/" 2>/dev/null | head -20
echo -e "\n=== Step 4: Pattern in init-git.ts - validatePath usage ==="
grep -n "validatePath\|projectPath\|existsSync\|execAsync" "apps/server/src/routes/worktree/routes/init-git.ts"Repository: AutoMaker-Org/automaker
Length of output: 5250
🏁 Script executed:
#!/bin/bash
# Comprehensive TOCTOU vulnerability verification
FILE="apps/server/src/routes/worktree/routes/init-git.ts"
if [ ! -f "$FILE" ]; then
echo "ERROR: $FILE not found"
find . -name "init-git.ts" 2>/dev/null
exit 1
fi
echo "=== Reading init-git.ts (entire file with line numbers) ==="
cat -n "$FILE"
echo -e "\n=== Searching for validatePath function ==="
rg -n "function validatePath|const validatePath\s*=|export.*validatePath" --type ts -A 10
echo -e "\n=== Checking route files for validatePath pattern ==="
rg -n "validatePath\|existsSync.*projectPath\|execAsync.*projectPath" --type ts apps/server/src/routes/Repository: AutoMaker-Org/automaker
Length of output: 3364
🏁 Script executed:
#!/bin/bash
# 1. Check file and read it
head -n 100 apps/server/src/routes/worktree/routes/init-git.ts | cat -n
# 2. Find validatePath definition
rg "function validatePath|const validatePath\s*=|export.*validatePath" --type ts -A 8 --max-count=5
# 3. Check usage pattern in routes
rg -n "validatePath\(" --type ts apps/server/src/routes/ | head -10Repository: AutoMaker-Org/automaker
Length of output: 3985
🏁 Script executed:
#!/bin/bash
# Check file exists
test -f "apps/server/src/routes/worktree/routes/init-git.ts" || {
echo "File not found, searching..."
find . -name "init-git.ts" -type f
exit 1
}
# Read the file
echo "=== init-git.ts (lines 1-100) ==="
sed -n '1,100p' "apps/server/src/routes/worktree/routes/init-git.ts" | cat -n
# Find validatePath
echo -e "\n=== validatePath function ==="
rg "function validatePath|const validatePath|export.*validatePath" --type ts -A 10
# Check other route patterns
echo -e "\n=== validatePath usage in all routes ==="
rg -n "validatePath\(" --type ts --glob "apps/server/src/routes/**/*.ts"Repository: AutoMaker-Org/automaker
Length of output: 6857
🏁 Script executed:
head -100 apps/server/src/routes/worktree/routes/init-git.ts && echo "---" && rg "function validatePath" --type ts -A 8Repository: AutoMaker-Org/automaker
Length of output: 2570
Capture and use the resolved path returned by validatePath.
The validatePath() function validates and resolves paths to absolute form, then returns the resolved path. At line 32, this return value is discarded, allowing the code to continue using the original projectPath at lines 45 and 60. Use the resolved path consistently:
// Validate and resolve paths within ALLOWED_ROOT_DIRECTORY
+ let validatedProjectPath: string;
try {
- validatePath(projectPath);
+ validatedProjectPath = 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");
+ const gitDirPath = join(validatedProjectPath, ".git");
if (existsSync(gitDirPath)) {
res.json({
success: true,
result: {
initialized: false,
message: "Git repository already exists",
},
});
return;
}
// Initialize git and create an initial empty commit
await execAsync(
`git init && git commit --allow-empty -m "Initial commit"`,
- { cwd: projectPath }
+ { cwd: validatedProjectPath }
);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // 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; | |
| } | |
| // Validate and resolve paths within ALLOWED_ROOT_DIRECTORY | |
| let validatedProjectPath: string; | |
| try { | |
| validatedProjectPath = 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(validatedProjectPath, ".git"); | |
| if (existsSync(gitDirPath)) { | |
| res.json({ | |
| success: true, | |
| result: { | |
| initialized: false, | |
| message: "Git repository already exists", | |
| }, | |
| }); | |
| return; | |
| } | |
| // Initialize git and create an initial empty commit | |
| await execAsync( | |
| `git init && git commit --allow-empty -m "Initial commit"`, | |
| { cwd: validatedProjectPath } | |
| ); |
🤖 Prompt for AI Agents
In apps/server/src/routes/worktree/routes/init-git.ts around lines 30 to 42,
validatePath() returns the resolved absolute path but its return value is
currently discarded; capture that return (e.g., const resolvedProjectPath =
validatePath(projectPath)) and on success replace subsequent uses of the
original projectPath (the calls at about lines 45 and 60) with the
resolvedProjectPath so the code consistently uses the validated absolute path;
preserve the PathNotAllowedError handling and rethrow other errors unchanged.
| // 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; | ||
| } |
There was a problem hiding this comment.
Use the resolved path returned by validatePath to ensure consistent security validation.
The validatePath function returns the resolved absolute path, but the code discards it and continues using the original worktreePath in subsequent git commands (lines 51, 60, 84, 90). This creates a potential security gap:
validatePathresolves and validates the absolute path (handling symlinks, relative paths, etc.)- Git commands use the original path which may resolve differently
- This could lead to TOCTOU issues, symlink handling inconsistencies, or validation bypasses
Since this PR focuses on security improvements, it's essential to use the validated resolved path throughout.
🔎 Proposed fix to capture and use the resolved path
- // Validate paths are within ALLOWED_ROOT_DIRECTORY
+ // Validate path is within ALLOWED_ROOT_DIRECTORY
try {
- validatePath(worktreePath);
+ const resolvedWorktreePath = 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",
- { cwd: worktreePath }
+ { cwd: resolvedWorktreePath }
);Then update lines 60, 84, and 90 similarly to use resolvedWorktreePath.
Minor: The comment says "paths" (plural) but only one path is validated.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // 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; | |
| } | |
| // Validate path is within ALLOWED_ROOT_DIRECTORY | |
| try { | |
| const resolvedWorktreePath = 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", | |
| { cwd: resolvedWorktreePath } | |
| ); |
🤖 Prompt for AI Agents
In apps/server/src/routes/worktree/routes/list-branches.ts around lines 34 to
46, the call to validatePath currently discards the resolved absolute path;
capture the returned resolved path (e.g., const resolvedWorktreePath =
validatePath(worktreePath)) and use that resolvedWorktreePath in all subsequent
git commands and checks (lines referenced around 51, 60, 84, 90) instead of the
original worktreePath to prevent TOCTOU/symlink validation gaps; also update the
comment text to say "path" (singular) since only one path is validated.
| // 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; | ||
| } |
There was a problem hiding this comment.
Critical: Use the resolved path returned by validatePath.
The validatePath function returns a resolved path that should be used in all subsequent operations. Currently, the return value is discarded and the original worktreePath is used in git commands (lines 46, 54, 59). This creates a security vulnerability where the normalized path is validated but the original path is executed, potentially allowing path traversal attacks.
🔎 Proposed fix to use the resolved path
- // Validate paths are within ALLOWED_ROOT_DIRECTORY
+ // Validate path is within ALLOWED_ROOT_DIRECTORY and use resolved path
+ let resolvedWorktreePath: string;
try {
- validatePath(worktreePath);
+ resolvedWorktreePath = validatePath(worktreePath);
} catch (error) {
if (error instanceof PathNotAllowedError) {
res.status(403).json({Then update all usages of worktreePath to use resolvedWorktreePath:
// Get branch name
const { stdout: branchOutput } = await execAsync(
"git rev-parse --abbrev-ref HEAD",
- { cwd: worktreePath }
+ { cwd: resolvedWorktreePath }
);
const branchName = branchOutput.trim();
// Push the branch
const forceFlag = force ? "--force" : "";
try {
await execAsync(`git push -u origin ${branchName} ${forceFlag}`, {
- cwd: worktreePath,
+ cwd: resolvedWorktreePath,
});
} catch {
// Try setting upstream
await execAsync(`git push --set-upstream origin ${branchName} ${forceFlag}`, {
- cwd: worktreePath,
+ cwd: resolvedWorktreePath,
});
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In apps/server/src/routes/worktree/routes/push.ts around lines 29 to 41,
validatePath(...) currently has its return value discarded; capture the resolved
path (e.g., const resolvedWorktreePath = validatePath(worktreePath)) and use
that validated/resolvedWorktreePath in all subsequent operations instead of the
original worktreePath (update the git command calls and any other usages around
lines ~46, ~54, and ~59 to reference resolvedWorktreePath); preserve the same
error handling for PathNotAllowedError and rethrow other errors.
| // 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; | ||
| } |
There was a problem hiding this comment.
Critical: Validate the constructed worktreePath, not just projectPath.
The current validation only checks projectPath, but the actual path used in git operations is worktreePath = path.join(projectPath, ".worktrees", featureId) (line 48). Since featureId is user-controlled, a malicious value like "../../../etc" could escape the allowed directory via path traversal, bypassing the security control.
🔎 Proposed fix: Validate worktreePath after construction
- // Validate paths are within ALLOWED_ROOT_DIRECTORY
+ // Git worktrees are stored in project directory
+ const worktreePath = path.join(projectPath, ".worktrees", featureId);
+
+ // 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;
}
-
- // Git worktrees are stored in project directory
- const worktreePath = path.join(projectPath, ".worktrees", featureId);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // 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); | |
| // 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; | |
| } |
🤖 Prompt for AI Agents
In apps/server/src/routes/worktree/routes/status.ts around lines 33 to 45, the
code only validates projectPath but later constructs worktreePath =
path.join(projectPath, ".worktrees", featureId), so a malicious featureId can
cause path traversal; to fix, construct worktreePath first (using path.resolve
to normalize), then validate that resolved worktreePath is inside
ALLOWED_ROOT_DIRECTORY (call validatePath(worktreePath) and handle
PathNotAllowedError the same way), and optionally enforce featureId is a single
path segment (e.g., reject values containing path separators) before using it.
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
Code Review - PR #197: Improve Docker Security & Path ValidationOverviewThis PR implements a comprehensive security overhaul focused on path validation and access control. The changes introduce ✅ StrengthsSecurity Implementation
Code Quality
Testing
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
apps/server/src/middleware/validate-paths.ts (1)
48-53: Consider validating presence vs. truthiness for required parameters.Line 50 checks
if (value)which treats empty strings,0, andfalseas absent. If a path parameter is accidentally an empty string, it will bypass validation. Consider checking!== undefinedinstead:// Handle regular parameters const value = req.body[paramName]; - if (value) { + if (value !== undefined) { validatePath(value); }This ensures that invalid values like empty strings are caught by
validatePathrather than silently skipped.apps/server/src/services/auto-mode-service.ts (1)
1746-2388: Consider extracting plan approval and task execution logic into separate methods.The
runAgentmethod spans over 600 lines and handles multiple responsibilities: planning, approval, revision, multi-agent task execution, and streaming. While the logic appears correct, the method's complexity makes it harder to maintain and test.Consider extracting:
- Plan approval workflow into
handlePlanApproval(...)- Plan revision loop into
regeneratePlanWithFeedback(...)- Multi-agent task execution into
executeTasksSequentially(...)This would improve readability and make the code easier to test and modify.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (20)
apps/server/src/lib/automaker-paths.ts(3 hunks)apps/server/src/lib/fs-utils.ts(4 hunks)apps/server/src/lib/image-handler.ts(2 hunks)apps/server/src/lib/secure-fs.ts(1 hunks)apps/server/src/middleware/validate-paths.ts(1 hunks)apps/server/src/routes/agent/index.ts(2 hunks)apps/server/src/routes/agent/routes/send.ts(0 hunks)apps/server/src/routes/agent/routes/start.ts(0 hunks)apps/server/src/routes/auto-mode/index.ts(2 hunks)apps/server/src/routes/features/index.ts(2 hunks)apps/server/src/routes/features/routes/create.ts(0 hunks)apps/server/src/routes/features/routes/list.ts(0 hunks)apps/server/src/routes/git/index.ts(1 hunks)apps/server/src/routes/settings/index.ts(2 hunks)apps/server/src/routes/suggestions/index.ts(1 hunks)apps/server/src/routes/worktree/index.ts(2 hunks)apps/server/src/services/agent-service.ts(8 hunks)apps/server/src/services/auto-mode-service.ts(27 hunks)apps/server/src/services/feature-loader.ts(15 hunks)apps/server/src/services/settings-service.ts(4 hunks)
💤 Files with no reviewable changes (4)
- apps/server/src/routes/features/routes/create.ts
- apps/server/src/routes/agent/routes/send.ts
- apps/server/src/routes/agent/routes/start.ts
- apps/server/src/routes/features/routes/list.ts
✅ Files skipped from review due to trivial changes (1)
- apps/server/src/lib/automaker-paths.ts
🧰 Additional context used
🧬 Code graph analysis (9)
apps/server/src/middleware/validate-paths.ts (1)
apps/server/src/lib/security.ts (2)
validatePath(111-119)PathNotAllowedError(11-18)
apps/server/src/routes/settings/index.ts (4)
apps/ui/src/utils/router.ts (1)
router(11-15)apps/server/src/middleware/validate-paths.ts (1)
validatePathParams(22-69)apps/server/src/routes/settings/routes/get-project.ts (1)
createGetProjectHandler(21-45)apps/server/src/routes/settings/routes/update-project.ts (1)
createUpdateProjectHandler(22-60)
apps/server/src/routes/auto-mode/index.ts (8)
apps/server/src/middleware/validate-paths.ts (1)
validatePathParams(22-69)apps/server/src/routes/auto-mode/routes/run-feature.ts (1)
createRunFeatureHandler(12-47)apps/server/src/routes/auto-mode/routes/verify-feature.ts (1)
createVerifyFeatureHandler(9-37)apps/server/src/routes/auto-mode/routes/resume-feature.ts (1)
createResumeFeatureHandler(12-43)apps/server/src/routes/auto-mode/routes/context-exists.ts (1)
createContextExistsHandler(9-37)apps/server/src/routes/auto-mode/routes/analyze-project.ts (1)
createAnalyzeProjectHandler(12-35)apps/server/src/routes/auto-mode/routes/follow-up-feature.ts (1)
createFollowUpFeatureHandler(12-59)apps/server/src/routes/auto-mode/routes/commit-feature.ts (1)
createCommitFeatureHandler(9-39)
apps/server/src/routes/worktree/index.ts (1)
apps/server/src/middleware/validate-paths.ts (1)
validatePathParams(22-69)
apps/server/src/routes/features/index.ts (6)
apps/server/src/middleware/validate-paths.ts (1)
validatePathParams(22-69)apps/server/src/routes/features/routes/list.ts (1)
createListHandler(9-28)apps/server/src/routes/features/routes/get.ts (1)
createGetHandler(9-39)apps/server/src/routes/features/routes/create.ts (1)
createCreateHandler(12-37)apps/server/src/routes/features/routes/update.ts (1)
createUpdateHandler(12-40)apps/server/src/routes/features/routes/delete.ts (1)
createDeleteHandler(9-34)
apps/server/src/services/auto-mode-service.ts (1)
apps/server/src/lib/security.ts (2)
isPathAllowed(78-105)PathNotAllowedError(11-18)
apps/server/src/lib/secure-fs.ts (1)
apps/server/src/lib/security.ts (1)
validatePath(111-119)
apps/server/src/routes/agent/index.ts (3)
apps/server/src/middleware/validate-paths.ts (1)
validatePathParams(22-69)apps/server/src/routes/agent/routes/start.ts (1)
createStartHandler(11-37)apps/server/src/routes/agent/routes/send.ts (1)
createSendHandler(11-51)
apps/server/src/routes/suggestions/index.ts (3)
apps/server/src/lib/events.ts (1)
EventEmitter(31-34)apps/server/src/middleware/validate-paths.ts (1)
validatePathParams(22-69)apps/server/src/routes/suggestions/routes/generate.ts (1)
createGenerateHandler(18-63)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: e2e
- GitHub Check: claude-review
🔇 Additional comments (15)
apps/server/src/routes/suggestions/index.ts (1)
15-15: LGTM!Path validation middleware correctly applied to the
/generateroute, which requiresprojectPathas confirmed by the handler implementation.apps/server/src/routes/git/index.ts (1)
13-14: LGTM!Path validation middleware correctly applied to both git routes, ensuring
projectPathandfilePathare validated before processing.apps/server/src/routes/settings/index.ts (1)
61-62: LGTM!Path validation middleware correctly applied to both project settings endpoints, which require
projectPathas confirmed by the handler implementations.apps/server/src/routes/features/index.ts (2)
19-23: LGTM!Path validation middleware correctly applied to all feature CRUD endpoints (
/list,/get,/create,/update,/delete), which requireprojectPathas confirmed by the handler implementations.
24-25: These routes don't accept path parameters and don't requirevalidatePathParamsmiddleware.POST routes with literal paths like
/agent-outputand/generate-titledon't define any path parameters (which require colon-prefixed segments like/:id). Path validation middleware is only applicable when routes contain parameterized segments in their path definition.apps/server/src/routes/agent/index.ts (2)
22-23: LGTM!Path validation middleware correctly applied with proper handling of optional parameters (
workingDirectory?) and array parameters (imagePaths[]), as confirmed by the handler implementations.
27-27: No path validation middleware needed—this route has no path parameters.The
/modelendpoint only acceptssessionIdandmodelfrom the request body, not from the URL path. The handler already validates these required body fields.validatePathParamsmiddleware is unnecessary for routes without path parameters (e.g., routes with patterns like/model/:id).Likely an incorrect or invalid review comment.
apps/server/src/routes/auto-mode/index.ts (3)
26-26: LGTM!Path validation middleware correctly applied to
/run-feature, which requiresprojectPathas confirmed by the handler implementation.
30-30: LGTM!Path validation middleware correctly applied to
/analyze-project, which requiresprojectPathas confirmed by the handler implementation.
35-35: LGTM!Path validation middleware correctly applied to
/commit-featurewith proper handling of requiredprojectPathand optionalworktreePath?.apps/server/src/services/settings-service.ts (4)
10-10: LGTM!Successfully migrated from direct filesystem operations to the secure filesystem wrapper, aligning with the broader security improvements in this PR.
44-60: LGTM!Atomic write operation correctly uses secure filesystem operations with proper cleanup on errors. The temp file cleanup is appropriately wrapped in try-catch to avoid masking the original error.
65-76: LGTM with improved error handling!The function now logs and handles all read errors gracefully, not just
ENOENT. This is a reliability improvement that ensures corrupted or inaccessible files fall back to defaults instead of crashing.
81-88: LGTM!File existence check correctly uses secure filesystem operations with appropriate error handling.
apps/server/src/lib/secure-fs.ts (1)
1-156: LGTM! Solid security-focused design.The secure filesystem wrapper provides a clean, consistent security layer with several strengths:
- Defense-in-depth: Validates paths at the actual I/O point, not just at API boundaries
- Comprehensive coverage: Wraps all necessary fs operations (access, read, write, mkdir, rm, copy, rename, etc.)
- Correct dual-path validation: Operations like
copyFileandrenameproperly validate both source and destination- Clear design separation: Path construction helpers (
joinPath,resolvePath) explicitly don't validate, as documentedThe type casts to
anyfor options are pragmatic given the complexity of Node.js fs APIs and don't compromise security.
| */ | ||
| export async function readImageAsBase64(imagePath: string): Promise<ImageData> { | ||
| const imageBuffer = await fs.readFile(imagePath); | ||
| const imageBuffer = await secureFs.readFile(imagePath) as Buffer; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find the image-handler.ts file and check its imports
find . -name "image-handler.ts" -type f 2>/dev/null | head -5Repository: AutoMaker-Org/automaker
Length of output: 105
🏁 Script executed:
#!/bin/bash
# Read the image-handler.ts file and look for secureFs import and usage
cat -n ./apps/server/src/lib/image-handler.ts | head -80Repository: AutoMaker-Org/automaker
Length of output: 2633
🏁 Script executed:
#!/bin/bash
# Find and read the secure-fs.js file
find ./apps/server/src -name "secure-fs.js" -o -name "secure-fs.ts" | head -3Repository: AutoMaker-Org/automaker
Length of output: 101
🏁 Script executed:
#!/bin/bash
# Read the secure-fs.ts file
cat -n ./apps/server/src/lib/secure-fs.tsRepository: AutoMaker-Org/automaker
Length of output: 5361
Fix the secureFs.readFile type signature with function overloads.
The as Buffer cast at line 66 is unsafe because secureFs.readFile returns Promise<string | Buffer> without distinguishing between calls with and without encoding. Update secure-fs.ts to use function overloads matching Node's fs.readFile behavior:
export async function readFile(filePath: string): Promise<Buffer>;
export async function readFile(filePath: string, encoding: BufferEncoding): Promise<string>;
export async function readFile(
filePath: string,
encoding?: BufferEncoding
): Promise<string | Buffer> {
const validatedPath = validatePath(filePath);
if (encoding) {
return fs.readFile(validatedPath, encoding);
}
return fs.readFile(validatedPath);
}This eliminates the need for type assertion and lets TypeScript infer the correct return type based on whether encoding is provided.
🤖 Prompt for AI Agents
In apps/server/src/lib/image-handler.ts around line 66 the code unsafely casts
secureFs.readFile to Buffer; update the secure-fs.ts implementation to provide
TypeScript function overloads matching Node's fs.readFile (one overload
returning Promise<Buffer> for calls without encoding and one returning
Promise<string> when a BufferEncoding is passed), implement the body to validate
the path and call fs.readFile with or without encoding accordingly, then remove
the `as Buffer` cast in image-handler so the call without encoding correctly
infers a Buffer return type.
| 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) | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Consider adding path validation to additional routes.
Based on the handler implementations shown in the relevant code snippets, several routes accept projectPath but don't have validatePathParams middleware applied:
/verify-feature(Line 27) - acceptsprojectPathandfeatureId/resume-feature(Line 28) - acceptsprojectPathandfeatureId/context-exists(Line 29) - acceptsprojectPathandfeatureId/follow-up-feature(Lines 31-34) - acceptsprojectPath,featureId, andimagePaths
For consistency with the security pattern applied across other routes in this PR, these endpoints should also validate their path parameters.
🔎 Proposed fix to add path validation
- router.post("/verify-feature", createVerifyFeatureHandler(autoModeService));
- router.post("/resume-feature", createResumeFeatureHandler(autoModeService));
- router.post("/context-exists", createContextExistsHandler(autoModeService));
+ router.post("/verify-feature", validatePathParams("projectPath"), createVerifyFeatureHandler(autoModeService));
+ router.post("/resume-feature", validatePathParams("projectPath"), createResumeFeatureHandler(autoModeService));
+ router.post("/context-exists", validatePathParams("projectPath"), createContextExistsHandler(autoModeService));
router.post(
"/follow-up-feature",
+ validatePathParams("projectPath", "imagePaths[]"),
createFollowUpFeatureHandler(autoModeService)
);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| 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("/verify-feature", validatePathParams("projectPath"), createVerifyFeatureHandler(autoModeService)); | |
| router.post("/resume-feature", validatePathParams("projectPath"), createResumeFeatureHandler(autoModeService)); | |
| router.post("/context-exists", validatePathParams("projectPath"), createContextExistsHandler(autoModeService)); | |
| router.post("/analyze-project", validatePathParams("projectPath"), createAnalyzeProjectHandler(autoModeService)); | |
| router.post( | |
| "/follow-up-feature", | |
| validatePathParams("projectPath", "imagePaths[]"), | |
| createFollowUpFeatureHandler(autoModeService) | |
| ); |
🤖 Prompt for AI Agents
In apps/server/src/routes/auto-mode/index.ts around lines 27 to 34, several
routes that accept projectPath and featureId (and follow-up-feature also accepts
imagePaths) lack the validatePathParams middleware; update the router.post calls
for /verify-feature, /resume-feature, /context-exists to prepend
validatePathParams("projectPath", "featureId") before the handler, and for
/follow-up-feature prepend validatePathParams("projectPath", "featureId") (and
include "imagePaths" only if imagePaths is a path parameter) so validation runs
prior to createXHandler(autoModeService).
…y 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.
Code Review: PR #197 - Improve Docker SecurityOverall AssessmentThis PR introduces significant security improvements by implementing a comprehensive path validation system with defense-in-depth security. The changes are well-structured and thoughtfully designed. However, there are several critical security issues and inconsistencies that need to be addressed before merging. ✅ Strengths
🔴 Critical Issues1. Incomplete Migration to secure-fs WrapperIssue: Many critical file operations still use Evidence:
Impact: The defense-in-depth security model is compromised. If an attacker bypasses API-level validation, they can access any file. Recommendation: // Instead of:
import fs from "fs/promises";
const content = await fs.readFile(resolvedPath, "utf-8");
// Use:
import * as secureFs from "../../../lib/secure-fs.js";
const content = await secureFs.readFile(resolvedPath, "utf-8");All file I/O should go through 2. Windows Drive Enumeration Bypasses SecurityIssue: In Code: const detectDrives = async (): Promise<string[]> => {
if (os.platform() !== "win32") {
return [];
}
const drives: string[] = [];
const letters = "ABCDEFGHIJKLMNOPQRSTUVWXYZ";
for (const letter of letters) {
const drivePath = `${letter}:\\`;
try {
await fs.access(drivePath); // No security check!
drives.push(drivePath);
} catch {
// Drive doesn't exist, skip it
}
}
return drives;
};Impact: On Windows, users can see all available drives even when Recommendation: Filter drives based on security policy: const drives: string[] = [];
for (const letter of letters) {
const drivePath = `${letter}:\\`;
try {
await fs.access(drivePath);
// Only include if within allowed root or no restrictions
if (isPathAllowed(drivePath)) {
drives.push(drivePath);
}
} catch {
// Drive doesn't exist, skip it
}
}3. Browse Endpoint Returns Parent Path Without ValidationIssue: In const parentPath = path.dirname(targetPath);
const hasParent = parentPath !== targetPath;
// ... returns parentPath without checking isPathAllowed(parentPath)Impact: The UI might receive and display parent paths outside Recommendation: Validate parent path before returning: const parentPath = path.dirname(targetPath);
const hasParent = parentPath !== targetPath && isPathAllowed(parentPath);4. addAllowedPath Usage May Be ProblematicIssue: In // Add to allowed paths
addAllowedPath(projectPath);Concern: This appears to be legacy code from the previous security model. If Recommendation: Review if this is still needed. The validation should rely on
|
1 participant
Summary by CodeRabbit
New Features
Bug Fixes
Chores
✏️ Tip: You can customize this high-level summary in your review settings.