Improve code docker2 #201
Improve code docker2 #201
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>
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>
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
…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.
|
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. WalkthroughIntroduces ALLOWED_ROOT_DIRECTORY-based path enforcement, a secure-fs adapter that validates paths, and middleware to validate request path parameters; replaces fs/promises usage with secure-fs across server services and routes; removes Google/OpenAI API key support; adds UI storage and workspace-config helpers and updates related components and tests. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as HTTP Client
participant Middleware as validatePathParams
participant Route as Route Handler
participant SecureFS as secure-fs Adapter
participant Security as security module
participant NodeFS as Node fs
Client->>Middleware: POST /api/... with path param(s)
Middleware->>Security: validatePath(param)
Security->>Security: resolve & check ALLOWED_ROOT_DIRECTORY / DATA_DIR
alt allowed
Security-->>Middleware: resolved path
Middleware->>Route: next()
Route->>SecureFS: perform I/O (readFile/mkdir/etc.) with resolved path
SecureFS->>Security: validatePath(path) /* double-check */
Security-->>SecureFS: ok
SecureFS->>NodeFS: fs operation
NodeFS-->>SecureFS: result
SecureFS-->>Route: data / success
Route-->>Client: 200 OK
else not allowed
Security-->>Middleware: throw PathNotAllowedError
Middleware-->>Client: 403 Forbidden + message
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Areas requiring extra attention:
Possibly related PRs
Suggested labels
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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 significantly enhances the application's security posture by implementing a robust, centralized file system access control mechanism. It introduces a new 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
|
Code Review - PR #201: Improve Docker & SecurityOverall AssessmentThis is a significant security-focused refactor that introduces comprehensive path validation and file system access controls. The implementation demonstrates a defense-in-depth approach with validation at multiple layers. However, there are several important issues that need to be addressed before merging. Status: 🔴 Critical Issues1. Type Safety Violations in
|
![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 and well-executed security improvements by sandboxing file system access. The introduction of a secure file system adapter (secure-fs.ts), path validation middleware, and a unified ALLOWED_ROOT_DIRECTORY configuration provides strong defense-in-depth. The refactoring extends to the UI, with better abstractions for storage and workspace management. The Docker configuration is also updated to reflect these changes. Overall, this is an excellent enhancement to the application's security posture and robustness. I have a few suggestions to further improve the code, mainly around typing and removing some leftover code from a previous implementation.
apps/server/src/lib/secure-fs.ts
Outdated
| encoding?: BufferEncoding | ||
| ): Promise<void> { | ||
| const validatedPath = validatePath(filePath); | ||
| return fs.writeFile(validatedPath, data, encoding as any); |
There was a problem hiding this comment.
The use of as any here bypasses TypeScript's type safety. The options parameter of fs.writeFile is optional, so you can handle the encoding parameter in a type-safe way by conditionally providing it.
| return fs.writeFile(validatedPath, data, encoding as any); | |
| return fs.writeFile(validatedPath, data, encoding); |
apps/server/src/lib/secure-fs.ts
Outdated
| options?: { withFileTypes?: boolean; encoding?: BufferEncoding } | ||
| ): Promise<string[] | any[]> { | ||
| const validatedPath = validatePath(dirPath); | ||
| return fs.readdir(validatedPath, options as any); |
There was a problem hiding this comment.
Using as any for the options in fs.readdir is not type-safe and hides potential issues with the options object structure. The overloads for fs.readdir are specific about the combination of withFileTypes and encoding.
To maintain type safety, you can use function overloads in your wrapper to correctly mirror the behavior of fs.readdir.
| return fs.readdir(validatedPath, options as any); | |
| return fs.readdir(validatedPath, options); |
apps/server/src/lib/secure-fs.ts
Outdated
| encoding?: BufferEncoding | ||
| ): Promise<void> { | ||
| const validatedPath = validatePath(filePath); | ||
| return fs.appendFile(validatedPath, data, encoding as any); |
There was a problem hiding this comment.
Similar to writeFile, the as any cast for encoding in fs.appendFile is not ideal. The options argument is optional, so you can pass encoding directly to maintain type safety.
| return fs.appendFile(validatedPath, data, encoding as any); | |
| return fs.appendFile(validatedPath, data, encoding); |
apps/server/src/lib/security.ts
Outdated
| export function addAllowedPath(filePath: string): void { | ||
| allowedPaths.add(path.resolve(filePath)); | ||
| } |
There was a problem hiding this comment.
The addAllowedPath function and the allowedPaths set appear to be remnants of a previous implementation. The main security check in isPathAllowed relies solely on allowedRootDirectory and dataDirectory, and does not check the allowedPaths set.
This means that any calls to addAllowedPath throughout the codebase have no effect on the security validation, which could lead to incorrect assumptions about how the security model works. This dead code should be removed to avoid confusion.
apps/server/Dockerfile
Outdated
| GH_VERSION=$(curl -s https://api.github.com/repos/cli/cli/releases/latest | grep '"tag_name"' | cut -d '"' -f 4 | sed 's/v//') && \ | ||
| curl -L "https://github.com/cli/cli/releases/download/v${GH_VERSION}/gh_${GH_VERSION}_linux_amd64.tar.gz" -o gh.tar.gz && \ | ||
| tar -xzf gh.tar.gz && \ | ||
| mv gh_*_linux_amd64/bin/gh /usr/local/bin/gh && \ | ||
| rm -rf gh.tar.gz gh_*_linux_amd64 |
There was a problem hiding this comment.
For more deterministic and reliable builds, it's a good practice to pin the version of tools you install. Fetching the latest version of the GitHub CLI can introduce unexpected changes or breakages in your CI/CD pipeline.
Additionally, using a wildcard in the mv command (mv gh_*_linux_amd64/bin/gh) can be fragile if the extracted directory name format changes in a future release. Using the version variable would make this more robust.
I suggest pinning the version and adjusting the script accordingly.
GH_VERSION="2.49.2" && \
curl -L "https://github.com/cli/cli/releases/download/v${GH_VERSION}/gh_${GH_VERSION}_linux_amd64.tar.gz" -o gh.tar.gz && \
tar -xzf gh.tar.gz && \
mv "gh_${GH_VERSION}_linux_amd64/bin/gh" /usr/local/bin/gh && \
rm -rf gh.tar.gz
| await secureFs.access(contextDir); | ||
|
|
||
| const files = await fs.readdir(contextDir); | ||
| const files = await secureFs.readdir(contextDir) as string[]; |
There was a problem hiding this comment.
The cast to string[] here is incorrect when withFileTypes is true, as fs.readdir would return Dirent[]. This is a symptom of the as any cast in the secure-fs.ts wrapper. Once the readdir wrapper in secure-fs.ts is correctly typed with overloads, this cast can be corrected or removed.
| const files = await secureFs.readdir(contextDir) as string[]; | |
| const files = (await secureFs.readdir(contextDir, { withFileTypes: true })) as fs.Dirent[]; |
|
|
||
| try { | ||
| const entries = await fs.readdir(featuresDir, { withFileTypes: true }); | ||
| const entries = await secureFs.readdir(featuresDir, { withFileTypes: true }) as any[]; |
There was a problem hiding this comment.
The cast to any[] here is hiding a type issue. When withFileTypes: true is used, fs.readdir returns an array of Dirent objects. This is related to the as any cast in the secure-fs wrapper. For type safety, this should be cast to fs.Dirent[] after the underlying issue in secure-fs is addressed.
| const entries = await secureFs.readdir(featuresDir, { withFileTypes: true }) as any[]; | |
| const entries = (await secureFs.readdir(featuresDir, { withFileTypes: true })) as fs.Dirent[]; |
|
|
||
| // Read all feature directories | ||
| const entries = await fs.readdir(featuresDir, { withFileTypes: true }); | ||
| const entries = await secureFs.readdir(featuresDir, { withFileTypes: true }) as any[]; |
There was a problem hiding this comment.
The cast to any[] here is hiding a type issue. When withFileTypes: true is used, fs.readdir returns an array of Dirent objects. This is related to the as any cast in the secure-fs wrapper. For type safety, this should be cast to fs.Dirent[] after the underlying issue in secure-fs is addressed.
| const entries = await secureFs.readdir(featuresDir, { withFileTypes: true }) as any[]; | |
| const entries = (await secureFs.readdir(featuresDir, { withFileTypes: true })) as fs.Dirent[]; |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
apps/server/src/services/auto-mode-service.ts (3)
943-1038: Add path validation toverifyFeatureandcommitFeaturebefore running shell commandsBoth methods execute shell commands using
cwdderived from untrusted path parameters without validating the resolved path against the allowed directory whitelist. This creates a security vulnerability ifprojectPath,featureId, orprovidedWorktreePathcould direct execution outside ALLOWED_ROOT_DIRECTORY/DATA_DIR.Add validation at method entry and before each
execAsynccall:
- Validate
projectPathat the top of both methods- Validate resolved
workDirbefore anyexecAsynccall (after determining which path to use)The necessary imports (
isPathAllowed,PathNotAllowedError) are already present in the file and follow the pattern used inexecuteFeature(lines 491, 559).
1192-1234: Add path validation toanalyzeProjectfor consistency and security
analyzeProjectpassesprojectPathascwdto an agent with file operation tools (Read, Glob, Grep) and saves analysis results viasecureFs. While the write operations are protected, the agent tools operate relative tocwd, which should be validated just like inexecuteFeature. Add this check at the start of the method:async analyzeProject(projectPath: string): Promise<void> { + if (!isPathAllowed(projectPath)) { + throw new PathNotAllowedError(projectPath); + } const abortController = new AbortController();
727-891: followUpFeature lacks projectPath validation present in executeFeature
followUpFeaturedoes not validateprojectPathwithisPathAllowedat the method entry, unlikeexecuteFeature. This creates an inconsistent validation pattern where:
executeFeaturevalidatesprojectPathearly (line checks withisPathAllowed)followUpFeaturederivesworkDirfromprojectPathwithout validation- Both pass
workDirtorunAgentwhich executes relative to that directory- Image copying via
secureFs.copyFileis protected (validates destination), but the working directory itself bypasses this checkFor defense-in-depth consistency, add
isPathAllowed(projectPath)validation at the start offollowUpFeature:async followUpFeature( projectPath: string, @@ ): Promise<void> { + // Validate project path early for defense-in-depth + if (!isPathAllowed(projectPath)) { + throw new PathNotAllowedError(projectPath); + } + const abortController = new AbortController();
🧹 Nitpick comments (18)
apps/ui/tests/utils/navigation/views.ts (1)
40-50: Overly broad exception handling may mask real issues.The catch block on line 48 silently continues on any error, including network timeouts, selector mismatches, or other unexpected failures. Consider catching only the specific error type expected (e.g., timeout errors), so that other exceptions are properly surfaced:
🔎 Suggested refinement
- try { - const loadingVisible = await loadingElement.isVisible({ timeout: 2000 }); - if (loadingVisible) { - // Wait for loading to disappear (spec view or empty state will appear) - await loadingElement.waitFor({ state: "hidden", timeout: 10000 }); - } - } catch { - // Loading element not found or already hidden, continue - } + try { + const loadingVisible = await loadingElement.isVisible({ timeout: 2000 }); + if (loadingVisible) { + // Wait for loading to disappear (spec view or empty state will appear) + await loadingElement.waitFor({ state: "hidden", timeout: 10000 }); + } + } catch (error) { + // Only suppress timeout errors; propagate other failures + if (!error.toString().includes("Timeout")) { + throw error; + } + // Loading element not found or already hidden, continue + }apps/ui/tests/utils/views/context.ts (1)
131-131: Consider standardizing element selection across the file.The file now mixes two approaches for selecting elements by test ID:
- Direct
page.locator('[data-testid="..."]')(lines 11, 21, 29, 131)getByTestId(page, "...")helper (lines 36, 47, 144, 151, 176)This is particularly evident with context-file selectors:
clickContextFile(line 21) uses directpage.locatorwaitForContextFile(line 131) now uses directpage.locatorselectContextFile(line 144) usesgetByTestIdStandardizing on one approach (preferably the
getByTestIdhelper for consistency with the imported utility) would improve maintainability and make the codebase more uniform.🔎 Suggested refactor to use getByTestId consistently
- const locator = page.locator(`[data-testid="context-file-${filename}"]`); + const locator = await getByTestId(page, `context-file-${filename}`);Note: You may want to apply similar changes to lines 11, 21, and 29 for consistency throughout the file.
apps/ui/src/components/views/welcome-view.tsx (1)
242-259: Good defensive validation before directory creation.The pre-checks for parent directory existence and type validation provide excellent user feedback and prevent confusing errors. This aligns well with the PR's path security enhancements.
Optional: Consider handling the edge case where
statreturns null.The condition on line 253 (
parentStat && !parentStat.isDirectory) will skip the directory type check ifstatreturns null/undefined. While the subsequentmkdircall will still fail appropriately, you could provide a more specific error message by explicitly handling this case.🔎 Proposed enhancement
// Verify parent is actually a directory const parentStat = await api.stat(parentDir); -if (parentStat && !parentStat.isDirectory) { +if (!parentStat) { + toast.error("Cannot access parent directory", { + description: `Unable to read directory information: ${parentDir}`, + }); + return; +} +if (!parentStat.isDirectory) { toast.error("Parent path is not a directory", { description: `${parentDir} is not a directory`, }); return; }Note: The theoretical TOCTOU race condition between the existence check and
mkdiris acceptable here, as any filesystem changes will be caught by themkdiroperation itself.apps/ui/src/components/views/board-view/worktree-panel/components/worktree-tab.tsx (1)
194-196: Consider sanitizing branch names in data-testid attributes.The dynamic title and aria-label improvements look good. However, the
data-testidconstruction at line 196 uses the raw branch name, which may contain special characters like slashes (e.g.,feature/my-branchbecomesworktree-branch-feature/my-branch). While Playwright's attribute selectors handle this, it's not ideal and could cause issues with CSS selectors or other test frameworks.🔎 Proposed refactor to sanitize data-testid
title={`Click to preview ${worktree.branch}`} aria-label={worktree.branch} - data-testid={`worktree-branch-${worktree.branch}`} + data-testid={`worktree-branch-${worktree.branch.replace(/[^a-zA-Z0-9_-]/g, '-')}`}And update the test accordingly:
// In the test file const mainBranchButton = page.locator('[data-testid="worktree-branch-main"]');Note: If you make this change, ensure all related tests use the same sanitization logic for consistency.
apps/server/src/routes/templates/routes/clone.ts (1)
9-9: Verify if PathNotAllowedError is needed.
PathNotAllowedErroris imported but not thrown in the validation blocks (lines 67-82). The code creates plain error responses instead. If this import is unused, consider removing it.#!/bin/bash # Description: Check if PathNotAllowedError is used elsewhere in clone.ts # Search for PathNotAllowedError usage rg -n 'PathNotAllowedError' apps/server/src/routes/templates/routes/clone.tsapps/server/src/routes/workspace/routes/directories.ts (1)
24-35: Consider including the path in the error message.The error message "Workspace directory path does not exist" could be more helpful by including which path was checked.
🔎 Suggested improvement
res.status(400).json({ success: false, - error: "Workspace directory path does not exist", + error: `Workspace directory does not exist: ${resolvedWorkspaceDir}`, });apps/ui/src/lib/project-init.ts (1)
51-77: Consider extracting validation logic into a reusable function.The project path validation logic (existence check, stat verification, directory check) is well-implemented with clear error messages. However, this validation pattern might be useful elsewhere in the codebase.
🔎 Optional refactor to improve reusability
Extract the validation into a separate function:
+/** + * Validates that a path exists and is a directory + * @param path - The path to validate + * @returns Validation result with error if invalid + */ +async function validateProjectDirectory(path: string): Promise<{ + valid: boolean; + error?: string; +}> { + const api = getElectronAPI(); + + const projectExists = await api.exists(path); + if (!projectExists) { + return { + valid: false, + error: `Project directory does not exist: ${path}. Create it first before initializing.`, + }; + } + + const projectStat = await api.stat(path); + if (!projectStat.success) { + return { + valid: false, + error: projectStat.error || `Failed to stat project directory: ${path}`, + }; + } + + if (projectStat.stats && !projectStat.stats.isDirectory) { + return { + valid: false, + error: `Project path is not a directory: ${path}`, + }; + } + + return { valid: true }; +} + export async function initializeProject( projectPath: string ): Promise<ProjectInitResult> { const api = getElectronAPI(); const createdFiles: string[] = []; const existingFiles: string[] = []; try { - // Validate that the project directory exists and is a directory - const projectExists = await api.exists(projectPath); - if (!projectExists) { - return { - success: false, - isNewProject: false, - error: `Project directory does not exist: ${projectPath}. Create it first before initializing.`, - }; - } - - // Verify it's actually a directory (not a file) - const projectStat = await api.stat(projectPath); - if (!projectStat.success) { - return { - success: false, - isNewProject: false, - error: projectStat.error || `Failed to stat project directory: ${projectPath}`, - }; - } - - if (projectStat.stats && !projectStat.stats.isDirectory) { + const validation = await validateProjectDirectory(projectPath); + if (!validation.valid) { return { success: false, isNewProject: false, - error: `Project path is not a directory: ${projectPath}`, + error: validation.error, }; }apps/server/src/middleware/validate-paths.ts (1)
36-46: Consider adding type guard for array element validation.If
req.body[actualName]contains an array with non-string elements,validatePath(value)may throw an unexpected error that gets re-thrown rather than handled gracefully.🔎 Suggested improvement
// Handle array parameters (paramName[]) if (paramName.endsWith("[]")) { const actualName = paramName.slice(0, -2); const values = req.body[actualName]; if (Array.isArray(values) && values.length > 0) { for (const value of values) { + if (typeof value === "string") { validatePath(value); + } } } continue; }apps/server/src/services/agent-service.ts (1)
65-99: Working directory normalization and validation look correct, with one minor follow‑upNormalizing
effectiveWorkingDirectoryviapath.resolveand validating it withisPathAllowedbefore storing in the in‑memory session and metadata is sound and keeps sessions tied to an absolute, policy‑compliant path.One thing to consider for defense‑in‑depth: in
sendMessage, a caller‑providedworkingDirectorycan temporarily overridesession.workingDirectoryfor the Claude agentcwdwithout an explicitisPathAllowedcheck here. You’re already validating at the route layer, but adding a guard like this would make the service robust against any future route changes:Suggested guard in
sendMessagefor overrideworkingDirectoryasync sendMessage({ sessionId, message, workingDirectory, @@ }) { @@ - // Build SDK options using centralized configuration + // Resolve and (optionally) validate working directory override + const executionCwd = + workingDirectory || session.workingDirectory; + + // Optional defense-in-depth: validate here as well as in route middleware + // if (executionCwd && !isPathAllowed(executionCwd)) { + // throw new PathNotAllowedError(executionCwd); + // } + + // Build SDK options using centralized configuration const sdkOptions = createChatOptions({ - cwd: workingDirectory || session.workingDirectory, + cwd: executionCwd, @@ const options: ExecuteOptions = { @@ - cwd: workingDirectory || session.workingDirectory, + cwd: executionCwd,apps/ui/src/components/dialogs/file-browser-dialog.tsx (3)
101-152: useCallback‑wrapped recent handlers and browseDirectory are stable and safeWrapping
handleRemoveRecentandbrowseDirectoryinuseCallbackwith stable dependencies helps avoid needless re-renders and keeps the network call site centralized. The error/warning reset andloadingflag handling look correct, andhandleSelectRecentreusesbrowseDirectorycleanly.
166-205: Initial path resolution matches the new workspace config, but the inline priority comment is slightly misleadingThe effect correctly prefers:
getDefaultWorkspaceDirectory()(which already bakes in ALLOWED_ROOT_DIRECTORY, last project dir, Documents, DATA_DIR),- overridden by an explicit
initialPathwhen provided,- and finally falls back to “home” when all else fails.
The comment above still mentions a separate “last selected directory from this file browser” step, but that’s now effectively handled inside
getDefaultWorkspaceDirectory. Consider updating the comment to mirror the actual order and avoid confusion for future maintainers.
448-466: Keyboard shortcut hint logic is fine but could be slightly more robustUsing
navigator.platform?.includes("Mac")behind atypeof navigator !== "undefined"guard is safe and keeps SSR working. If you want to future‑proof this, you could eventually prefernavigator.userAgentDataor a more generic check, but it’s not critical for this UI hint.apps/server/src/services/auto-mode-service.ts (1)
686-722: Consider adding explicit path validation in resumeFeature for defense‑in‑depth
resumeFeaturerelies on secureFs to enforce path policy when readingagent-output.md, and then callsexecuteFeature, which validatesprojectPath/workDir. This is functionally safe, but adding an earlyisPathAllowed(projectPath)check here would surface configuration mistakes sooner and make the service’s contract clearer.apps/ui/src/lib/workspace-config.ts (1)
42-99: Default workspace directory resolution matches the documented behaviorThe
getDefaultWorkspaceDirectoryimplementation:
- Prefers
workspaceDirwhenALLOWED_ROOT_DIRECTORYis configured.- Otherwise falls back to last project dir, then Documents/Automaker, then
defaultDirfrom the API.- On failure or exceptions, gracefully retries last project dir and Documents before returning
null.This centralizes all path selection logic in one place and works cleanly with the storage abstraction and Electron HTTP bridge.
apps/server/src/lib/security.ts (1)
117-148: isPathWithinDirectory and accessor helpers behave as expected; consider clarifying addAllowedPath’s roleThe
isPathWithinDirectoryimplementation usingpath.relativeand checks against".."/absolute paths is a standard and reliable way to detect containment, and thegetAllowedRootDirectory/getDataDirectory/getAllowedPathsaccessors make introspection easy.One subtle point:
addAllowedPathonly affects whatgetAllowedPaths()returns; it does not change howisPathAllowed/validatePathdecide which paths are permitted (that’s still strictly ALLOWED_ROOT_DIRECTORY + DATA_DIR). If you intend to support additional dynamic roots in the future, a short comment to that effect nearaddAllowedPathcould prevent confusion.apps/server/src/lib/secure-fs.ts (3)
38-45: Improve type safety by removingas anycasts.The
encoding as anycasts on lines 44 and 118 bypass TypeScript's type checking. Consider using proper function overloads or typing the encoding parameter more precisely to match the fs.promises signature without type assertions.🔎 Proposed refactor using proper types
For
writeFile:export async function writeFile( filePath: string, data: string | Buffer, encoding?: BufferEncoding ): Promise<void> { const validatedPath = validatePath(filePath); - return fs.writeFile(validatedPath, data, encoding as any); + return fs.writeFile(validatedPath, data, encoding ? { encoding } : undefined); }For
appendFile:export async function appendFile( filePath: string, data: string | Buffer, encoding?: BufferEncoding ): Promise<void> { const validatedPath = validatePath(filePath); - return fs.appendFile(validatedPath, data, encoding as any); + return fs.appendFile(validatedPath, data, encoding ? { encoding } : undefined); }Also applies to: 112-119
61-67: Use proper return types instead ofany.Lines 66, 72, and 137 use
anytypes where more specific types are available:
statandlstatshould returnPromise<fs.Stats>readdiroptions should be typed properly to avoid theas anycastUsing
anyeliminates type safety benefits for consumers of this module.🔎 Proposed refactor with proper types
Import the Stats type at the top:
-import fs from "fs/promises"; +import fs, { type Stats } from "fs/promises";For
stat:-export async function stat(filePath: string): Promise<any> { +export async function stat(filePath: string): Promise<Stats> { const validatedPath = validatePath(filePath); return fs.stat(validatedPath); }For
lstat:-export async function lstat(filePath: string): Promise<any> { +export async function lstat(filePath: string): Promise<Stats> { const validatedPath = validatePath(filePath); return fs.lstat(validatedPath); }For
readdir, consider using proper overloads:+export async function readdir( + dirPath: string +): Promise<string[]>; +export async function readdir( + dirPath: string, + options: { withFileTypes: true; encoding?: BufferEncoding } +): Promise<fs.Dirent[]>; +export async function readdir( + dirPath: string, + options: { withFileTypes?: false; encoding?: BufferEncoding } +): Promise<string[]>; export async function readdir( dirPath: string, options?: { withFileTypes?: boolean; encoding?: BufferEncoding } -): Promise<string[] | any[]> { +): Promise<string[] | fs.Dirent[]> { const validatedPath = validatePath(dirPath); - return fs.readdir(validatedPath, options as any); + return fs.readdir(validatedPath, options as any) as Promise<string[] | fs.Dirent[]>; }Also applies to: 69-75, 133-140
142-156: Consider enhancing documentation for non-validating helpers.The comments clearly state these functions don't validate, which is good. However, consider making it even more explicit that the returned paths must be validated before use to prevent accidental security bypasses.
🔎 Suggested documentation enhancement
/** * Wrapper around path.join that returns resolved path - * Does NOT validate - use this for path construction, then pass to other operations + * ⚠️ DOES NOT VALIDATE - use only for path construction. + * The returned path MUST be passed to a validated operation (e.g., readFile, writeFile) + * or explicitly validated via validatePath() before use. */ export function joinPath(...pathSegments: string[]): string { return path.join(...pathSegments); } /** * Wrapper around path.resolve that returns resolved path - * Does NOT validate - use this for path construction, then pass to other operations + * ⚠️ DOES NOT VALIDATE - use only for path construction. + * The returned path MUST be passed to a validated operation (e.g., readFile, writeFile) + * or explicitly validated via validatePath() before use. */ export function resolvePath(...pathSegments: string[]): string { return path.resolve(...pathSegments); }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (60)
apps/server/.env.example(1 hunks)apps/server/Dockerfile(1 hunks)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/lib/security.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/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/git/index.ts(1 hunks)apps/server/src/routes/models/routes/providers.ts(0 hunks)apps/server/src/routes/settings/index.ts(2 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/suggestions/index.ts(1 hunks)apps/server/src/routes/templates/routes/clone.ts(2 hunks)apps/server/src/routes/workspace/routes/config.ts(1 hunks)apps/server/src/routes/workspace/routes/directories.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)apps/server/tests/setup.ts(0 hunks)apps/server/tests/unit/lib/security.test.ts(5 hunks)apps/ui/playwright.config.ts(2 hunks)apps/ui/src/components/dialogs/file-browser-dialog.tsx(8 hunks)apps/ui/src/components/new-project-modal.tsx(4 hunks)apps/ui/src/components/views/board-view/worktree-panel/components/worktree-tab.tsx(1 hunks)apps/ui/src/components/views/board-view/worktree-panel/worktree-panel.tsx(2 hunks)apps/ui/src/components/views/interview-view.tsx(6 hunks)apps/ui/src/components/views/welcome-view.tsx(1 hunks)apps/ui/src/hooks/use-settings-migration.ts(5 hunks)apps/ui/src/lib/electron.ts(3 hunks)apps/ui/src/lib/http-api-client.ts(1 hunks)apps/ui/src/lib/project-init.ts(1 hunks)apps/ui/src/lib/storage.ts(1 hunks)apps/ui/src/lib/workspace-config.ts(1 hunks)apps/ui/src/main.ts(5 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 (7)
- apps/server/src/routes/agent/routes/start.ts
- apps/server/src/routes/models/routes/providers.ts
- apps/server/src/routes/setup/routes/api-keys.ts
- apps/server/src/routes/features/routes/create.ts
- apps/server/tests/setup.ts
- apps/server/src/routes/features/routes/list.ts
- apps/server/src/routes/agent/routes/send.ts
🧰 Additional context used
🧬 Code graph analysis (26)
apps/ui/tests/spec-editor-persistence.spec.ts (1)
apps/ui/tests/utils/views/spec-editor.ts (3)
setEditorContent(79-105)getEditorContent(65-74)clickSaveButton(110-122)
apps/server/src/routes/templates/routes/clone.ts (1)
apps/server/src/lib/security.ts (1)
isPathAllowed(73-97)
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/ui/src/hooks/use-settings-migration.ts (1)
apps/ui/src/lib/storage.ts (2)
getItem(21-28)removeItem(51-59)
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/ui/src/components/new-project-modal.tsx (1)
apps/ui/src/lib/workspace-config.ts (2)
getDefaultWorkspaceDirectory(42-99)saveLastProjectDirectory(105-107)
apps/server/src/routes/fs/routes/mkdir.ts (2)
apps/server/src/lib/security.ts (2)
isPathAllowed(73-97)PathNotAllowedError(11-18)apps/server/src/routes/common.ts (1)
getErrorMessage(370-372)
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/tests/unit/lib/security.test.ts (1)
apps/server/src/lib/security.ts (5)
initAllowedPaths(34-56)getAllowedPaths(147-149)getAllowedRootDirectory(133-135)isPathAllowed(73-97)validatePath(103-111)
apps/server/src/routes/workspace/routes/config.ts (1)
apps/server/src/lib/security.ts (3)
getAllowedRootDirectory(133-135)getDataDirectory(140-142)addAllowedPath(62-64)
apps/ui/src/components/views/interview-view.tsx (1)
apps/ui/src/lib/workspace-config.ts (2)
getDefaultWorkspaceDirectory(42-99)saveLastProjectDirectory(105-107)
apps/server/src/routes/suggestions/index.ts (2)
apps/server/src/middleware/validate-paths.ts (1)
validatePathParams(22-69)apps/server/src/routes/suggestions/routes/generate.ts (1)
createGenerateHandler(18-63)
apps/ui/src/components/views/board-view/worktree-panel/worktree-panel.tsx (1)
apps/ui/src/lib/storage.ts (2)
getItem(21-28)setItem(36-44)
apps/server/src/routes/worktree/index.ts (2)
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/lib/secure-fs.ts (1)
apps/server/src/lib/security.ts (1)
validatePath(103-111)
apps/server/src/routes/fs/routes/browse.ts (2)
apps/server/src/lib/security.ts (2)
isPathAllowed(73-97)PathNotAllowedError(11-18)apps/server/src/routes/common.ts (1)
getErrorMessage(370-372)
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/settings/index.ts (3)
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/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/services/auto-mode-service.ts (1)
apps/server/src/lib/security.ts (2)
isPathAllowed(73-97)PathNotAllowedError(11-18)
apps/ui/src/lib/electron.ts (1)
apps/ui/src/lib/storage.ts (3)
getJSON(66-74)setJSON(82-88)removeItem(51-59)
apps/server/src/routes/fs/routes/delete.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/directories.ts (1)
apps/server/src/lib/security.ts (2)
getAllowedRootDirectory(133-135)addAllowedPath(62-64)
apps/server/src/routes/fs/routes/read.ts (2)
apps/server/src/lib/security.ts (1)
PathNotAllowedError(11-18)apps/server/src/routes/common.ts (1)
getErrorMessage(370-372)
apps/ui/src/main.ts (1)
init.mjs (1)
__dirname(22-22)
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)
⏰ 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). (4)
- GitHub Check: e2e
- GitHub Check: claude-review
- GitHub Check: e2e
- GitHub Check: claude-review
🔇 Additional comments (67)
apps/ui/tests/worktree-integration.spec.ts (1)
106-109: Reconsider the selector choice—the timeout increase stems from a step backward in test strategy.The selector change to
data-testid="worktree-branch-main"requires a 50% timeout increase (15s vs 10s), but the very next test in the same file achieves the same result usinggetByRole("button", { name: "main" })with only a 10s timeout. Playwright best practices recommend prioritizing user-facing attributes and explicit contracts such as getByRole(), notdata-testid. Usingdata-testidhere appears to be a step backward; rather than accepting the longer timeout, either revert togetByRole()or provide justification for whydata-testidis necessary in this specific case.apps/ui/playwright.config.ts (2)
43-43: LGTM - Appropriate testing configuration.The comment clearly documents that path restrictions are disabled for testing, which is appropriate for the test environment where you need unrestricted file access.
56-57: LGTM - Formatting improvement.The ternary operator formatting is clear and maintains the same logic.
apps/server/src/routes/templates/routes/clone.ts (2)
66-73: LGTM - Proper path validation.The parent directory validation is correctly placed after path traversal checks and uses the resolved path. The error message clearly explains the restriction.
75-82: LGTM - Comprehensive path security.The project path validation prevents directory creation outside the allowed root directory. Good placement before filesystem operations.
docker-compose.override.yml.example (1)
5-10: LGTM - Clearer configuration model.The shift to a single
ALLOWED_ROOT_DIRECTORYsimplifies the configuration and the comments clearly explain the usage.apps/server/.env.example (1)
19-23: LGTM - Comprehensive documentation.The documentation clearly explains the security implications, recommended usage, and provides a helpful example. The empty default allows unrestricted access for development while making it easy to enable restrictions for production.
apps/server/src/routes/setup/routes/delete-api-key.ts (1)
73-73: LGTM - Consistent error messaging.The error message correctly reflects the removal of Google provider support and is consistent with the changes in
store-api-key.ts.apps/server/src/routes/workspace/routes/directories.ts (2)
8-22: LGTM - Proper configuration validation.The validation correctly checks if the root directory is configured and provides a clear error message. The integration with the centralized security model is appropriate.
38-53: LGTM - Comprehensive path registration.The implementation correctly:
- Registers the workspace directory to the allowed paths
- Filters and sorts directories appropriately
- Registers each discovered directory to the allowed paths for subsequent access
This ensures that discovered directories can be accessed in future requests while maintaining the security boundaries.
docker-compose.yml (1)
40-44: LGTM! Clear security boundary configuration.The migration from
ALLOWED_PROJECT_DIRStoALLOWED_ROOT_DIRECTORYsimplifies the path security model with a single root directory. The comments clearly explain that paths are container-internal, and the default/projectsvalue is appropriate.apps/ui/tests/spec-editor-persistence.spec.ts (2)
49-50: Good addition for test stability.Adding an explicit wait for the spec-view to load before accessing the editor improves test reliability by ensuring the view is fully rendered.
86-120: Robust content verification with fallback.The polling approach with 30-second timeout and fallback to
waitForFunctioneffectively handles CodeMirror's asynchronous content loading after page reload. The try-catch within the polling loop prevents false failures.apps/ui/src/components/views/board-view/worktree-panel/worktree-panel.tsx (1)
11-11: LGTM! Clean migration to storage abstraction.The shift from direct
localStorageaccess to thestoragemodule'sgetItem/setItemfunctions centralizes storage logic and removes the need for runtimewindowavailability checks.Also applies to: 95-100
apps/server/src/routes/fs/routes/delete.ts (1)
7-7: LGTM! Consistent security error handling.The addition of explicit
PathNotAllowedErrorhandling with a 403 response aligns with the PR's path security model and matches the pattern used across other filesystem routes.Also applies to: 25-29
apps/server/src/routes/suggestions/index.ts (1)
7-7: LGTM! Path validation middleware applied correctly.The
validatePathParams("projectPath")middleware integration ensures the/generateroute validates paths before executing the handler, consistent with the PR's security model.Also applies to: 15-15
apps/server/src/routes/fs/routes/write.ts (1)
8-8: LGTM! Consistent security error handling.The
PathNotAllowedErrorhandling with 403 response follows the same pattern as other filesystem routes and properly integrates with the path security system.Also applies to: 33-37
apps/server/src/routes/fs/routes/stat.ts (1)
7-7: LGTM! Consistent security error handling.The
PathNotAllowedErrorhandling follows the established pattern across filesystem routes, ensuring consistent 403 responses for disallowed paths.Also applies to: 33-37
apps/server/src/routes/features/index.ts (1)
7-7: LGTM! Path validation middleware successfully integrated.The middleware correctly validates
projectPathbefore handler execution for all routes that require it. The/agent-outputand/generate-titleroutes appropriately skip validation since they don't useprojectPath.Note: The handlers still perform their own
projectPathvalidation checks (e.g.,if (!projectPath)). While this creates some redundancy, it maintains defense-in-depth and doesn't harm correctness.Also applies to: 19-23
apps/server/src/routes/fs/routes/readdir.ts (1)
7-7: LGTM! Consistent error handling for path security.The PathNotAllowedError handling is implemented correctly and consistently with other filesystem routes. The 403 response is returned before the generic error handling, ensuring proper HTTP status codes for authorization failures.
Also applies to: 31-35
apps/server/src/routes/fs/routes/read.ts (1)
7-7: LGTM! Path security integrated with existing error handling.The PathNotAllowedError handling is correctly positioned before the ENOENT checks, ensuring security violations are caught and returned as 403 responses. The existing optional-file logging behavior is preserved.
Also applies to: 42-46
apps/server/src/routes/git/index.ts (1)
6-6: LGTM! Path validation applied to git routes.Both git routes are correctly wrapped with path validation middleware. The
/file-diffroute appropriately validates bothprojectPathandfilePathparameters, while/diffsvalidates onlyprojectPath.Also applies to: 13-14
apps/server/src/lib/automaker-paths.ts (1)
12-12: LGTM! Filesystem operations migrated to secure wrapper.The migration from
fs/promisestosecureFsensures that all directory creation operations go through path validation. The function signatures and behavior remain unchanged, maintaining backward compatibility while adding security.Also applies to: 152-152, 214-214
apps/server/src/lib/image-handler.ts (1)
11-11: LGTM! Image reading migrated to secure filesystem.The migration to
secureFs.readFileadds path validation for image operations. The explicitBuffercast is appropriate sincereadFilewithout an encoding parameter returns a Buffer.Also applies to: 66-66
apps/server/src/routes/fs/routes/mkdir.ts (2)
9-9: LGTM! Path validation enforced before directory creation.The security checks are well-structured:
- Early validation with
isPathAllowedprevents unauthorized directory creation- 403 response for forbidden paths with proper error handling
- Dynamic tracking with
addAllowedPathfor created/verified directoriesThe interaction between
isPathAllowed(which checks againstALLOWED_ROOT_DIRECTORY) andaddAllowedPath(which tracks specific paths) appears intentional for granular path management.Also applies to: 24-27, 60-64
34-34: Remove or clarifyaddAllowedPathcall for pre-existing directories.The function is documented as "Used when dynamically creating new directories within the allowed root" (line 60, security.ts), yet it's called both when a directory already exists (mkdir.ts:34) and when one is newly created (mkdir.ts:56). The
allowedPathsSet is not used for actual path validation —isPathAllowed()checks only ALLOWED_ROOT_DIRECTORY and DATA_DIR — making this tracking inconsistent with its stated purpose.Either remove the call for pre-existing directories or clarify the intent if tracking is needed for other purposes. If the Set becomes unused elsewhere, consider removing
addAllowedPathentirely to avoid future maintenance confusion.apps/server/src/routes/settings/index.ts (1)
17-17: LGTM! Path validation applied to project settings routes.The middleware is correctly applied only to the project-specific settings endpoints (
/project), while global settings and credentials routes appropriately remain unwrapped since they don't useprojectPath.Also applies to: 61-62
apps/server/src/routes/fs/routes/exists.ts (2)
21-26: LGTM! Security validation correctly placed before filesystem access.The path validation is appropriately positioned after resolution and before any filesystem operations, following the same pattern as other fs route handlers.
34-42: Error handling follows consistent pattern.The 403 response for
PathNotAllowedErroris correctly separated from the 500 catch-all, maintaining consistency with other fs routes.apps/ui/src/lib/http-api-client.ts (1)
765-771: LGTM! Type extension aligns with server-side workspace config changes.The optional
defaultDirfield correctly reflects the server's ability to provide a default workspace directory while maintaining backward compatibility.apps/server/src/routes/fs/routes/browse.ts (2)
108-116: LGTM! Error handling follows consistent pattern.The 403 response for
PathNotAllowedErroris correctly separated from other error handling paths.
17-23: Path validation correctly placed and error handling is appropriate.Validation occurs after path resolution, before file operations. When
dirPathis not provided, the handler defaults toos.homedir(), which is then subject toALLOWED_ROOT_DIRECTORYrestrictions if configured. This is intentional behavior—if restrictions are configured, the home directory must fall within the allowed root, or accessing it will correctly return a 403 Forbidden response. The implementation properly enforces these security boundaries.apps/server/src/routes/agent/index.ts (2)
22-23: LGTM! Path validation middleware correctly applied.The
validatePathParamsmiddleware appropriately validates optionalworkingDirectoryand arrayimagePaths[]parameters before the handlers execute. The parameter syntax correctly matches the handler signatures.
27-27: No path validation required for/modelroute.This route accepts only
sessionIdandmodelparameters from the request body for session configuration, not filesystem paths. No path-based operations are performed.apps/ui/src/hooks/use-settings-migration.ts (2)
23-23: LGTM! Storage abstraction improves resilience.The
getItemandremoveItemwrappers from@/lib/storageprovide graceful handling when localStorage is unavailable or throws errors, improving reliability across different browser environments.
126-158: Consistent storage abstraction applied throughout migration logic.All direct
localStoragecalls have been replaced with the abstractedgetItemandremoveItemfunctions, maintaining consistent error handling.apps/server/src/middleware/validate-paths.ts (2)
22-53: Well-designed reusable middleware with clear parameter syntax.The middleware correctly handles optional (
?) and array ([]) parameter conventions. The documentation examples are helpful.
56-67: LGTM! Error handling correctly distinguishes PathNotAllowedError.Re-throwing unexpected errors allows Express's error handling middleware to catch them appropriately.
apps/server/src/routes/worktree/index.ts (1)
36-56: LGTM! Path validation middleware applied consistently.The validatePathParams middleware is correctly applied to all routes that accept path parameters (projectPath, worktreePath, filePath). Routes without path parameters (like /list, /create-pr, /pr-info, /checkout-branch, /switch-branch, /migrate, /stop-dev, /list-dev-servers) appropriately skip validation.
apps/ui/src/main.ts (1)
183-187: Good security practice: conditional path restriction.The ALLOWED_ROOT_DIRECTORY is now only set when explicitly provided in the environment, allowing unrestricted access by default in development while enabling path restrictions in production/Docker environments. The comment clearly documents this behavior.
apps/server/src/services/settings-service.ts (2)
44-60: Good addition: error propagation after cleanup.The migration to secureFs is correct. Notably, line 58 now properly throws the error after cleanup, ensuring that atomicWriteJson doesn't silently swallow errors. This improves error handling reliability.
65-76: Type cast required for secureFs compatibility.The cast to
stringon line 67 is necessary because secureFs.readFile returns a union type. This is expected behavior given the secure wrapper's type signatures.apps/ui/src/components/new-project-modal.tsx (2)
85-96: LGTM! Centralized workspace configuration.The migration from localStorage to workspace-config utilities provides better separation of concerns. Error handling is appropriate with console logging for debugging.
272-303: Verify UX: empty state when no workspace is configured.Line 289 now shows nothing when workspaceDir is empty, removing the previous "No workspace configured" error message. This might be less alarming for users, but could make it harder to understand why the Browse button needs to be clicked.
Consider if this empty state provides sufficient user guidance, or if a less alarming informational message would improve UX (e.g., "Click Browse to select a directory").
apps/server/src/routes/workspace/routes/config.ts (2)
18-29: LGTM! Proper fallback to DATA_DIR.When ALLOWED_ROOT_DIRECTORY is not set, the endpoint correctly returns
configured: falsewith DATA_DIR as the default directory. This provides a sensible fallback for unconfigured environments.
32-52: Good security practice: validate before registering path.The code correctly validates that ALLOWED_ROOT_DIRECTORY exists and is a directory before calling
addAllowedPath. This prevents registration of invalid paths in the security allow-list.apps/server/src/services/feature-loader.ts (1)
7-7: LGTM! Comprehensive migration to secure filesystem.All filesystem operations throughout the FeatureLoader have been migrated to use the secure-fs wrapper, ensuring consistent path validation across feature management operations. Type casts are appropriately added where the secure wrapper returns union types.
apps/ui/src/components/views/interview-view.tsx (2)
94-121: LGTM! Proper async state management with cleanup.The useEffect correctly loads the default workspace directory with proper guards:
isMountedflag prevents state updates after unmount- Line 104 guard prevents overwriting an already-set projectPath
- Error handling logs issues without crashing
329-335: Good UX: persist directory selection.Setting
initialPathto the current projectPath provides continuity, and callingsaveLastProjectDirectoryensures the user's choice is remembered for future sessions.apps/server/src/lib/fs-utils.ts (2)
5-47: LGTM! Enhanced symlink safety in mkdirSafe.The migration to secureFs is correct, and the ELOOP handling (lines 29-32) provides better safety when dealing with symlink loops. The console.warn at line 30 aids debugging without throwing errors that could break workflows.
49-67: Verify ELOOP treatment in existsSafe.Line 63 treats ELOOP as "exists = true", which is documented in the comment but might be counterintuitive. A symlink loop means the path exists but is not usable. Consider if callers of existsSafe expect this behavior, or if ELOOP should throw or return false.
This behavior is intentional per the comments, but verify that all callers of
existsSafecan handle the case where the path "exists" but is actually a broken symlink loop and may not be accessible.apps/server/src/services/agent-service.ts (2)
400-441: secureFs usage for session and metadata persistence is consistentReplacing raw
fs.promiseswithsecureFs.readFile/writeFileand casting the"utf-8"reads tostringkeeps the I/O consistent with the new path‑validation layer and avoids accidental writes outside allowed roots. The catch‑and‑fallback behavior ([]/{}) on read failures mirrors the previous semantics and should be fine as long asdataDiris mapped underDATA_DIRorALLOWED_ROOT_DIRECTORY.
545-555: Deleting session files via secureFs aligns with the new security modelUsing
secureFs.unlinkfor session file deletion is the right move; it enforces the same path policy as reads/writes and still safely ignores missing files via the surrounding try/catch.apps/ui/src/components/dialogs/file-browser-dialog.tsx (3)
23-73: Storage abstraction for recent folders is a good improvementSwitching
RECENT_FOLDERS_KEYaccess togetJSON/setJSONand centralizing add/remove logic keeps recent-folder persistence consistent with the new storage layer and safe under SSR (thanks to guards instorage.ts). The de‑duplication and capped history (MAX_RECENT_FOLDERS) logic is also clear.
239-247: Persisting last project directory via saveLastProjectDirectory is aligned with workspace utilitiesCalling
saveLastProjectDirectory(currentPath)when the user confirms selection ensures the next open will respect this directory viagetDefaultWorkspaceDirectory. This ties the dialog behavior neatly into the new workspace-config module.
320-339: Truncating recent folder labels while keeping the full path in the title is a good UX compromiseUsing
getFolderNameplustitle={folder}and atruncatespan balances readability and discoverability, especially with the new wrapping layout for recent folders.apps/ui/src/lib/storage.ts (1)
1-100: Centralized storage abstraction is robust and SSR‑safeThe
isStorageAvailableguard plus try/catch around all localStorage calls makes this module safe in SSR and privacy‑restricted environments.getJSON/setJSONadd a clean, typed way to work with JSON in storage, and the exportedstorageobject gives a convenient aggregate API.apps/server/src/services/auto-mode-service.ts (3)
489-563: executeFeature’s project/workDir path validation is solidValidating both
projectPathand the resolvedworkDirwithisPathAllowedbefore any agent execution is exactly what you want for the new ALLOWED_ROOT_DIRECTORY model; it prevents accidental or malicious runs outside the configured boundary while still allowing.automakerworktrees.
1112-1163: loadContextFiles correctly relies on secureFs for enforcement and filters text filesUsing
secureFs.access/readdir/readFileto pull in.md/.txtfiles from the resolved context directory keeps these reads within the same security envelope as the rest of the system. The generated system prompt wrapper is also careful to emphasize project‑specific rules and package‑manager usage.
1757-1907: secureFs writes in mock and real agent output paths look correctIn both MOCK mode and the real agent stream:
- Output paths are derived under
workDirorgetFeatureDir(projectPath, featureId).- Directories are created with
secureFs.mkdir(..., { recursive: true }).- Writes use
secureFs.writeFileso they’re subject to the same path policy.The debounced writes in
runAgentare especially nice for large streams and log errors without crashing the run.apps/ui/src/lib/workspace-config.ts (2)
18-29: Electron-backed Documents path helper is straightforward and safely logged
getDefaultDocumentsPathcorrectly delegates togetElectronAPI().getPath("documents")and joinsAutomaker, with errors logged only whenwindow.consoleexists. Returningnullon failure makes its use ingetDefaultWorkspaceDirectorysimple.
101-107: saveLastProjectDirectory correctly routes persistence through the storage layerUsing
setItem(LAST_PROJECT_DIR_KEY, path)keeps the “last project dir” concept in sync withgetDefaultWorkspaceDirectoryand avoids direct localStorage usage.apps/server/tests/unit/lib/security.test.ts (1)
14-51: Security tests comprehensively cover the new ALLOWED_ROOT_DIRECTORY + DATA_DIR behaviorThe updated tests for
initAllowedPaths,isPathAllowed,validatePath, andgetAllowedPathsalign well with the new semantics:
- ALLOWED_ROOT_DIRECTORY and DATA_DIR loading and coexistence.
- “No restrictions configured” cases where all paths are allowed.
- Correct handling of relative paths within the allowed root.
This suite should give good confidence that future changes to
security.tswon’t accidentally weaken the path model.Also applies to: 85-137, 139-191, 193-220
apps/ui/src/lib/electron.ts (1)
3-4: Migrating project persistence to storage.getJSON/setJSON is clean and SSR‑friendlyUsing
getJSON/setJSONfor projects and trashed projects, plusremoveItemwhen clearing the current project, removes direct localStorage coupling from this module while keeping behavior identical. The?? []fallbacks ensure callers always see arrays even when storage is unavailable.Also applies to: 2670-2712
apps/server/src/lib/security.ts (2)
8-55: Initialization and error type cleanly define the security boundary
PathNotAllowedErrorgives a clear, uniform error for policy violations, andinitAllowedPathscorrectly normalizes ALLOWED_ROOT_DIRECTORY/DATA_DIR and logs their configuration. Treating “no ALLOWED_ROOT_DIRECTORY” as unrestricted (while still honoring DATA_DIR as an exception) preserves backward compatibility.
73-111: isPathAllowed/validatePath implementation is correct and resistant to traversal
isPathAllowed:
- Always permits paths under
DATA_DIR.- Falls back to “allow all” when no root is configured.
- Uses
isPathWithinDirectoryto ensure paths are truly insideALLOWED_ROOT_DIRECTORY.
validatePathlayeringpath.resolveand then throwingPathNotAllowedErroron failure gives a simple, safe contract for callers.apps/server/src/lib/secure-fs.ts (1)
1-11: Excellent defense-in-depth security design.The module provides a clean abstraction layer that enforces path validation at the I/O boundary, ensuring all filesystem operations go through security checks regardless of where they're called from.
apps/server/Dockerfile
Outdated
| # Install git, curl, and GitHub CLI | ||
| RUN apk add --no-cache git curl && \ | ||
| GH_VERSION=$(curl -s https://api.github.com/repos/cli/cli/releases/latest | grep '"tag_name"' | cut -d '"' -f 4 | sed 's/v//') && \ | ||
| curl -L "https://github.com/cli/cli/releases/download/v${GH_VERSION}/gh_${GH_VERSION}_linux_amd64.tar.gz" -o gh.tar.gz && \ | ||
| tar -xzf gh.tar.gz && \ | ||
| mv gh_*_linux_amd64/bin/gh /usr/local/bin/gh && \ | ||
| rm -rf gh.tar.gz gh_*_linux_amd64 | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Search for usage of gh CLI in the server codebase
rg -n 'gh\b|github-cli' apps/server/src/ --type ts --type js 2>/dev/null || echo "No direct gh command usage found"
# Also check for gh in shell scripts or other files
rg -n '\bgh\b' apps/server/ --type sh 2>/dev/null || echo "No sh usage found"
# Check if there are other references to GitHub API or operations
rg -n 'GitHub|github' apps/server/src/ -i --type ts --type js -c | head -20Repository: AutoMaker-Org/automaker
Length of output: 6477
🌐 Web query:
alpine github cli package availability
💡 Result:
Yes — Alpine provides GitHub CLI as the package named "github-cli" in the community repository (multiple branches/architectures). Install with:
- Enable community repo (if not already) and install:
apk add github-cli - To get the very latest from edge:
echo "@community http://dl-cdn.alpinelinux.org/alpine/edge/community" >> /etc/apk/repositories
apk add github-cli@community
Package pages / builds: [1][2]. Installation docs: [3].
Sources:
[1] pkgs.alpinelinux.org — github-cli (v3.23 community).
[2] pkgs.alpinelinux.org — github-cli (edge community).
[3] GitHub CLI install (Alpine) — apk add github-cli.
🌐 Web query:
gh cli musl alpine compatibility amd64
💡 Result:
Short answer: Official gh release tarballs on GitHub are built for glibc (linux amd64) — there isn’t an official "linux-musl" amd64 binary from cli/cli. On Alpine you should install the alpine-packaged gh (apk add github-cli from community/stable) or build from source / use the Alpine package (aports). Using the official linux_amd64 tarball on musl-based Alpine may fail unless you add a glibc-compat layer (gcompat/glibc) or run the distro package. [1][2]
Sources:
- Official gh install notes (mentions Alpine package / edge community repo). [1]
- gh GitHub releases (linux amd64 assets — no linux-musl asset present). [2]
Critical: Non-deterministic builds, architecture incompatibility, and missing security verification.
The gh CLI is actively used in production code for PR operations (across pr-info.ts, create-pr.ts, and gh-status.ts), making this a critical issue.
The installation block has several critical problems:
- Non-deterministic builds: Fetching the
latestrelease means builds are not reproducible and break when new versions are released. - Architecture mismatch: Alpine uses musl libc, but the official
linux_amd64build is glibc-based and incompatible. Without a glibc-compat layer, the binary will fail on Alpine. - No signature/checksum verification: Downloading and executing binaries without verification is a security risk.
- Brittle parsing: The
grep | cut | sedchain is fragile and breaks with API response changes. - Build failure risk: Network issues or GitHub API downtime will cause builds to fail.
Use Alpine's native package instead:
-# Install git, curl, and GitHub CLI
-RUN apk add --no-cache git curl && \
- GH_VERSION=$(curl -s https://api.github.com/repos/cli/cli/releases/latest | grep '"tag_name"' | cut -d '"' -f 4 | sed 's/v//') && \
- curl -L "https://github.com/cli/cli/releases/download/v${GH_VERSION}/gh_${GH_VERSION}_linux_amd64.tar.gz" -o gh.tar.gz && \
- tar -xzf gh.tar.gz && \
- mv gh_*_linux_amd64/bin/gh /usr/local/bin/gh && \
- rm -rf gh.tar.gz gh_*_linux_amd64
+# Install git and GitHub CLI from Alpine packages
+RUN apk add --no-cache git github-cliAlpine's github-cli package is available in the community repository and is the recommended approach for musl-based systems.
📝 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.
| # Install git, curl, and GitHub CLI | |
| RUN apk add --no-cache git curl && \ | |
| GH_VERSION=$(curl -s https://api.github.com/repos/cli/cli/releases/latest | grep '"tag_name"' | cut -d '"' -f 4 | sed 's/v//') && \ | |
| curl -L "https://github.com/cli/cli/releases/download/v${GH_VERSION}/gh_${GH_VERSION}_linux_amd64.tar.gz" -o gh.tar.gz && \ | |
| tar -xzf gh.tar.gz && \ | |
| mv gh_*_linux_amd64/bin/gh /usr/local/bin/gh && \ | |
| rm -rf gh.tar.gz gh_*_linux_amd64 | |
| # Install git and GitHub CLI from Alpine packages | |
| RUN apk add --no-cache git github-cli |
🤖 Prompt for AI Agents
In apps/server/Dockerfile around lines 29-36, the current RUN block dynamically
downloads the gh binary from the GitHub releases API using fragile parsing, no
integrity checks, and a glibc-based binary that is incompatible with Alpine;
replace this whole dynamic-download approach with Alpine's native package:
enable the community repository if not already enabled and install github-cli
via apk (optionally pin the apk package version to ensure reproducible builds),
remove the curl/grep/cut/sed/tar/mv steps and any reliance on GitHub API at
build time, and ensure the Dockerfile uses --no-cache and a fixed package list
to keep builds deterministic and compatible with musl.
| } else { | ||
| res.status(400).json({ | ||
| success: false, | ||
| error: `Unsupported provider: ${provider}. Only anthropic is supported.`, | ||
| }); | ||
| return; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Search for remaining Google provider references
# Search for Google/google provider references in UI and docs
rg -n -i 'google.*provider|provider.*google' apps/ui/src/ apps/server/src/ --type=ts --type=tsx -C2
# Search for GOOGLE_API_KEY references
rg -n 'GOOGLE_API_KEY' apps/ --type=ts --type=tsx --type=env -C2Repository: AutoMaker-Org/automaker
Length of output: 122
🏁 Script executed:
#!/bin/bash
# Get more context around the changes in the store-api-key file
cat -n apps/server/src/routes/setup/routes/store-api-key.ts | head -50
# Check for any provider enum or type definitions
rg -n 'provider.*=|type.*Provider|enum.*Provider' apps/server/src/ --type=ts -A2 -B2 | head -50Repository: AutoMaker-Org/automaker
Length of output: 5306
🏁 Script executed:
#!/bin/bash
# Search for "google" string anywhere in the codebase (case-insensitive)
rg -i 'google' apps/ --max-count=20
# Search for provider configuration or UI files
find apps/ui -name "*provider*" -o -name "*api*" -o -name "*setup*" 2>/dev/null | head -20
# Check for any .env or config files
find apps -name ".env*" -o -name "*config*" -o -name "*.env" 2>/dev/null | head -10
# Search for files mentioning "Unsupported provider" or API key setup UI
rg -i 'unsupported|api.*key.*setup|provider.*select' apps/ui/ --max-count=10Repository: AutoMaker-Org/automaker
Length of output: 10620
🏁 Script executed:
#!/bin/bash
# Check the api-providers.ts file to see the full Google configuration
sed -n '1,150p' apps/ui/src/config/api-providers.ts
# Check what the settings-view is actually rendering
rg -A 10 "api-providers" apps/ui/src/components/views/settings-view/ --type tsRepository: AutoMaker-Org/automaker
Length of output: 7051
Backend correctly rejects Google provider; UI already disabled Google support.
The backend implementation correctly restricts the API to only Anthropic. However, the removal is incomplete: the UI already has Google disabled (commented out in api-providers.ts), but orphaned code remains in data models (settings.ts, app-store.ts), state management (use-api-key-management.ts), endpoint documentation comments, and test fixtures. These should be cleaned up to avoid confusion about provider support.
Remaining cleanup:
- Remove commented Google provider config in
api-providers.ts - Update endpoint documentation in
update-credentials.tsandget-credentials.ts - Remove
googlefield from settings type definition - Remove Google key references from test fixtures
- Clean up unused state variables in
use-api-key-management.ts
🤖 Prompt for AI Agents
In apps/server/src/routes/setup/routes/store-api-key.ts lines 39-45 and related
files, the backend already restricts providers to Anthropic but leftover
Google-related code and comments remain elsewhere; remove the commented Google
provider entry in api-providers.ts, update endpoint documentation comments in
update-credentials.ts and get-credentials.ts to reflect only Anthropic, remove
the google field from the settings type in settings.ts and any Google-related
fields in app-store.ts, delete Google key references from test fixtures, and
prune unused state variables and handlers in use-api-key-management.ts that
reference Google so the codebase consistently reflects only Anthropic support.
| // 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.
Risk of silent test failure with Promise.race.
If both waitForElement calls fail, Promise.race will resolve with [null, null] rather than rejecting. The test will continue without verifying that either spec-view or spec-view-empty actually appeared, potentially masking real issues where the page fails to load properly.
Consider asserting that at least one element was found:
🔎 Proposed fix
- // 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 results = await Promise.race([
+ waitForElement(page, "spec-view", { timeout: 10000 }).catch(() => null),
+ waitForElement(page, "spec-view-empty", { timeout: 10000 }).catch(() => null),
+ ]);
+ if (!results) {
+ throw new Error(
+ "Spec view failed to load: neither spec-view nor spec-view-empty element appeared"
+ );
+ }🤖 Prompt for AI Agents
In apps/ui/tests/utils/navigation/views.ts around lines 52 to 58, the
Promise.race approach can silently allow both waits to fail and let the test
continue; change the logic to explicitly verify that at least one element was
found (e.g., use Promise.allSettled or Promise.any and then assert the result)
and throw or fail the test if both waitForElement calls returned null/failed so
the test does not proceed when neither spec-view nor spec-view-empty appeared.
webdevcody
added
Testers-Requested
This commit introduces several improvements to the security and file handling mechanisms across the application. Key changes include: - Updated the Dockerfile to pin the GitHub CLI version for reproducible builds. - Refactored the secure file system operations to ensure consistent path validation and type handling. - Removed legacy path management functions and streamlined the allowed paths logic in the security module. - Enhanced route handlers to validate path parameters against the ALLOWED_ROOT_DIRECTORY, improving security against unauthorized access. - Updated the settings service to focus solely on the Anthropic API key, removing references to Google and OpenAI keys. These changes aim to enhance security, maintainability, and clarity in the codebase. Tests: All unit tests passing.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
apps/server/src/services/settings-service.ts (2)
65-76: Add TypeScript overloads tosecureFs.readFilefor proper type narrowing.The
readFilefunction insecure-fs.tslacks proper overloads and always returnsPromise<string | Buffer>. When an encoding is provided, TypeScript cannot narrow the return type tostring, necessitating theas stringcast. Instead of adding runtime type guards, add overloads to match Node.js fs 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> { // existing implementation }This eliminates the need for casts throughout the codebase and provides compile-time type safety.
495-506: Confirm intentional removal of Google/OpenAI API key support and address type mismatch.The credentials migration intentionally removes support for Google and OpenAI API keys—only Anthropic keys are migrated. This is evident from the
Credentialsinterface inapps/server/src/types/settings.ts, which only definesapiKeys.anthropic, and the commented-out Google provider config inapps/ui/src/config/api-providers.ts(lines 93–99).However, there's a type inconsistency:
apps/ui/src/components/views/settings-view/api-keys/hooks/use-api-key-management.tsstill referencesapiKeys.google(lines 25, 48), which will now be undefined post-migration, potentially causing runtime errors or type violations.Ensure this removal aligns with PR objectives and consider:
- Removing the unused
googleKeyreferences from the UI hook or updating the Credentials type to explicitly exclude these fields- Adding a deprecation notice or migration guide for existing users
- Clarifying in release notes that Google/OpenAI keys will not be preserved
♻️ Duplicate comments (2)
apps/server/src/routes/auto-mode/index.ts (1)
21-37: Consistent path validation applied across all routes.The previous review concern about inconsistent path validation has been addressed. All routes that accept filesystem path parameters now have appropriate
validatePathParamsmiddleware, with correct handling of optional (?) and array ([]) parameters.apps/server/src/services/auto-mode-service.ts (1)
1585-1598: Type safety withwithFileTypesoption.When using
withFileTypes: true,secureFs.readdirshould returnDirent[]. As noted in a previous review, ensure thesecure-fswrapper is properly typed with overloads, or add an explicit type annotation here for clarity:const entries = await secureFs.readdir(featuresDir, { withFileTypes: true }) as import('fs').Dirent[];
🧹 Nitpick comments (2)
apps/server/src/routes/workspace/routes/directories.ts (1)
28-45: Consider usingsecureFsfor consistency.While reading from
allowedRootDirectoryis inherently safe (it's the root of allowed paths), usingsecureFswould maintain consistency with the pattern used in other service files. This is optional since the current implementation is correct.apps/server/src/routes/templates/routes/clone.ts (1)
9-9:PathNotAllowedErroris imported but unused.The code uses inline 403 responses (lines 68-72, 77-81) instead of throwing
PathNotAllowedError. Either remove the unused import or refactor to throw the error for consistency withmkdir.ts.🔎 Option 1: Remove unused import
-import { isPathAllowed, PathNotAllowedError } from "../../../lib/security.js"; +import { isPathAllowed } from "../../../lib/security.js";
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (20)
apps/server/Dockerfile(1 hunks)apps/server/src/lib/secure-fs.ts(1 hunks)apps/server/src/lib/security.ts(1 hunks)apps/server/src/routes/auto-mode/index.ts(2 hunks)apps/server/src/routes/fs/routes/browse.ts(2 hunks)apps/server/src/routes/fs/routes/mkdir.ts(3 hunks)apps/server/src/routes/fs/routes/resolve-directory.ts(0 hunks)apps/server/src/routes/fs/routes/save-board-background.ts(0 hunks)apps/server/src/routes/fs/routes/save-image.ts(0 hunks)apps/server/src/routes/fs/routes/validate-path.ts(1 hunks)apps/server/src/routes/settings/routes/get-credentials.ts(1 hunks)apps/server/src/routes/settings/routes/update-credentials.ts(1 hunks)apps/server/src/routes/templates/routes/clone.ts(2 hunks)apps/server/src/routes/workspace/routes/config.ts(1 hunks)apps/server/src/routes/workspace/routes/directories.ts(1 hunks)apps/server/src/services/auto-mode-service.ts(27 hunks)apps/server/src/services/settings-service.ts(4 hunks)apps/server/src/types/settings.ts(0 hunks)apps/server/tests/unit/lib/security.test.ts(3 hunks)apps/server/tests/unit/services/settings-service.test.ts(2 hunks)
💤 Files with no reviewable changes (4)
- apps/server/src/routes/fs/routes/save-board-background.ts
- apps/server/src/types/settings.ts
- apps/server/src/routes/fs/routes/resolve-directory.ts
- apps/server/src/routes/fs/routes/save-image.ts
✅ Files skipped from review due to trivial changes (1)
- apps/server/src/routes/settings/routes/get-credentials.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/server/src/routes/fs/routes/browse.ts
🧰 Additional context used
🧬 Code graph analysis (7)
apps/server/src/routes/workspace/routes/directories.ts (1)
apps/server/src/lib/security.ts (1)
getAllowedRootDirectory(120-122)
apps/server/src/routes/auto-mode/index.ts (4)
apps/server/src/middleware/validate-paths.ts (1)
validatePathParams(22-69)apps/server/src/routes/auto-mode/routes/verify-feature.ts (1)
createVerifyFeatureHandler(9-37)apps/server/src/routes/auto-mode/routes/context-exists.ts (1)
createContextExistsHandler(9-37)apps/server/src/routes/auto-mode/routes/commit-feature.ts (1)
createCommitFeatureHandler(9-39)
apps/server/src/services/auto-mode-service.ts (1)
apps/server/src/lib/security.ts (2)
isPathAllowed(60-84)PathNotAllowedError(11-18)
apps/server/src/routes/fs/routes/mkdir.ts (2)
apps/server/src/lib/security.ts (2)
isPathAllowed(60-84)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 (5)
initAllowedPaths(31-51)getAllowedPaths(134-143)getAllowedRootDirectory(120-122)isPathAllowed(60-84)validatePath(90-98)
apps/server/src/routes/templates/routes/clone.ts (1)
apps/server/src/lib/security.ts (1)
isPathAllowed(60-84)
apps/server/src/lib/secure-fs.ts (1)
apps/server/src/lib/security.ts (1)
validatePath(90-98)
⏰ 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 (30)
apps/server/src/services/auto-mode-service.ts (4)
490-493: Path validation correctly guards feature execution.Good defensive check at the entry point of feature execution. Validating
projectPathearly prevents downstream operations on disallowed paths.
558-561: Validates derived working directory separately.Good practice to validate
workDirafter deriving it fromworktreePathorprojectPath, as worktrees could potentially be located outside allowed directories.
1946-1949: Error message updated to reflect supported authentication.The error message correctly references only
ANTHROPIC_API_KEYandclaude login, aligning with the removal of Google/OpenAI API key support from the codebase.
17-17: Comprehensive migration to secure filesystem wrapper.The service consistently uses
secureFsfor all file I/O operations (access, readFile, writeFile, mkdir, copyFile, readdir), ensuring path validation is enforced at the filesystem layer.apps/server/src/routes/settings/routes/update-credentials.ts (1)
1-9: Documentation accurately reflects Anthropic-only credential support.The updated documentation correctly describes the narrowed scope of this endpoint, aligning with the removal of Google and OpenAI API key support from the codebase.
apps/server/src/routes/fs/routes/validate-path.ts (2)
8-8: Removed dynamic path allowlisting in favor of static configuration.The endpoint now only checks
isPathAllowedwithout adding paths to an allowed list. This aligns with the centralizedALLOWED_ROOT_DIRECTORYapproach where allowed paths are defined by configuration rather than dynamically added at runtime.
24-38: Appropriate use of rawfs.statfor path validation.Using
fs.statdirectly (rather thansecureFs) is correct here since this endpoint's purpose is to validate whether a path should be allowed. The actual security boundary is provided byisPathAllowed()at line 37, which returns the authorization status without performing side effects.apps/server/src/routes/workspace/routes/directories.ts (1)
14-22: EnforcesALLOWED_ROOT_DIRECTORYconfiguration requirement.Good validation that returns a clear error message when the root directory is not configured. This ensures the workspace directories endpoint only operates within explicitly defined boundaries.
apps/server/src/routes/workspace/routes/config.ts (1)
14-59: LGTM!The handler correctly uses the centralized security utilities to retrieve and validate the workspace configuration. The direct
fs.statusage is appropriate here since it's validating the server's own configured directory, not user-supplied paths.apps/server/src/routes/fs/routes/mkdir.ts (2)
22-27: LGTM!Path validation is correctly placed after resolution but before any filesystem operations, ensuring unauthorized paths are rejected early.
55-60: LGTM!The
PathNotAllowedErroris properly caught and returns a 403 status, consistent with the security model.apps/server/src/routes/templates/routes/clone.ts (1)
66-82: LGTM on the validation logic.Both parent directory and project path are correctly validated against the allowed paths before proceeding with the clone operation. The dual validation ensures neither the parent nor the target path can escape the security boundary.
apps/server/tests/unit/services/settings-service.test.ts (2)
250-269: LGTM!Test correctly updated to reflect the anthropic-only API key structure while maintaining verification of the deep merge behavior.
310-316: LGTM!Test updated to use the simplified credentials structure.
apps/server/src/lib/security.ts (4)
11-18: LGTM!Well-structured custom error class with a descriptive message that helps users understand why their request was rejected and what the constraints are.
31-51: LGTM!Environment-based initialization with clear logging. The console output provides good visibility into the security configuration state at startup.
60-84: LGTM!The access control logic is well-structured with clear priority:
- Always allow DATA_DIR (necessary for settings/credentials)
- Allow all when no restriction is configured (backward compatibility)
- Enforce ALLOWED_ROOT_DIRECTORY boundary when set
104-115: Path containment check is correct.The
path.relativeapproach properly handles path traversal:
- Returns
..prefix when target escapes the directory- Returns absolute path if on different drives (Windows)
- Returns empty string or relative path when contained
apps/server/tests/unit/lib/security.test.ts (4)
14-51: LGTM! Comprehensive test coverage for the new ALLOWED_ROOT_DIRECTORY configuration.The test suite properly covers:
- Loading ALLOWED_ROOT_DIRECTORY and verifying via getAllowedRootDirectory()
- DATA_DIR exception handling
- Combined ALLOWED_ROOT_DIRECTORY + DATA_DIR scenarios
- Proper path resolution
The tests align well with the security module's new behavior.
54-106: LGTM! Excellent test coverage for isPathAllowed behavior.The tests correctly verify:
- Paths within ALLOWED_ROOT_DIRECTORY are allowed
- Paths outside ALLOWED_ROOT_DIRECTORY are denied
- All paths are allowed when no restrictions are configured
- DATA_DIR doesn't restrict other paths when ALLOWED_ROOT_DIRECTORY is not set
This properly exercises the new security boundary logic.
108-160: LGTM! validatePath tests cover critical security scenarios.The tests properly verify:
- Allowed paths return resolved paths
- Disallowed paths throw errors
- No restrictions allow all paths
- Relative path resolution works correctly within allowed directory
162-189: LGTM! getAllowedPaths tests verify correct array composition.Tests confirm that getAllowedPaths returns an array containing both ALLOWED_ROOT_DIRECTORY and DATA_DIR when configured, with proper path resolution.
apps/server/src/services/settings-service.ts (3)
10-10: LGTM! Secure filesystem integration.The migration from direct
fs/promisestosecureFsenforces path validation at the I/O layer, providing defense-in-depth security.
44-60: LGTM! Proper atomic write implementation with secure-fs.The atomic write pattern (write-to-temp-then-rename) is correctly implemented using secureFs operations, and the error is properly rethrown after cleanup attempt.
270-286: Intentional refactor to focus solely on Anthropic credentials.This change is part of a deliberate refactor (commit 9cf12b9) to remove support for Google and OpenAI keys and focus solely on Anthropic. All callers in the codebase—
get-credentials.tsandupdate-credentials.ts—have already been updated to work with the new signature, and their documentation reflects that onlyanthropicis returned. No code is accessing the removed properties.apps/server/src/lib/secure-fs.ts (5)
1-12: LGTM! Excellent security pattern with clear documentation.The secure filesystem adapter enforces ALLOWED_ROOT_DIRECTORY restrictions at the I/O layer, providing defense-in-depth security. The documentation clearly explains the purpose and approach.
17-46: LGTM! Type-safe wrapper implementations.The
access,readFile, andwriteFilewrappers properly handle optional encoding parameters in a type-safe manner. The past review comments aboutas anycasts have been addressed - the current implementation correctly passes encoding conditionally without type casts.
62-79: LGTM! Proper function overloads for readdir.The function overloads correctly handle the
withFileTypesoption and properly distinguish between returningstring[]vsDirent[]. The past review comment about type safety has been addressed.
111-119: LGTM! Dual path validation for copy and rename operations.Both
copyFileandrenamecorrectly validate both source and destination paths before performing the operation, ensuring security boundaries are enforced for both paths.Also applies to: 136-143
154-168: LGTM! Clear documentation for non-validating path helpers.The
joinPathandresolvePathhelpers are clearly documented as NOT performing validation. This is appropriate since these are for path construction prior to validation by other operations.
| # Install git, curl, and GitHub CLI (pinned version for reproducible builds) | ||
| RUN apk add --no-cache git curl && \ | ||
| GH_VERSION="2.63.2" && \ | ||
| curl -L "https://github.com/cli/cli/releases/download/v${GH_VERSION}/gh_${GH_VERSION}_linux_amd64.tar.gz" -o gh.tar.gz && \ | ||
| tar -xzf gh.tar.gz && \ | ||
| mv "gh_${GH_VERSION}_linux_amd64/bin/gh" /usr/local/bin/gh && \ | ||
| rm -rf gh.tar.gz "gh_${GH_VERSION}_linux_amd64" |
There was a problem hiding this comment.
CRITICAL: glibc binary incompatible with Alpine's musl libc.
While pinning the version to 2.63.2 addresses build reproducibility, the critical architecture incompatibility flagged in previous reviews remains unresolved. The official linux_amd64 gh CLI binary is compiled against glibc, but Alpine Linux uses musl libc. This binary will fail at runtime on Alpine without a glibc compatibility layer (which is not installed).
Since the gh CLI is actively used in production code (as confirmed in previous reviews), this will cause runtime failures when the application attempts to execute gh commands.
The recommended solution is to use Alpine's native github-cli package from the community repository:
-# Install git, curl, and GitHub CLI (pinned version for reproducible builds)
-RUN apk add --no-cache git curl && \
- GH_VERSION="2.63.2" && \
- curl -L "https://github.com/cli/cli/releases/download/v${GH_VERSION}/gh_${GH_VERSION}_linux_amd64.tar.gz" -o gh.tar.gz && \
- tar -xzf gh.tar.gz && \
- mv "gh_${GH_VERSION}_linux_amd64/bin/gh" /usr/local/bin/gh && \
- rm -rf gh.tar.gz "gh_${GH_VERSION}_linux_amd64"
+# Install git and GitHub CLI from Alpine packages
+RUN apk add --no-cache git github-cliThis approach:
- Uses musl-compatible binaries built specifically for Alpine
- Eliminates download/extraction complexity
- Removes network dependency during build
- Provides package-managed updates and security patches
If version pinning is required for reproducibility, you can pin the Alpine package version: apk add --no-cache git github-cli=2.63.2-r0 (verify exact package version in Alpine repos).
Additional concern: The current approach also lacks checksum or signature verification, creating a supply-chain security risk.
To verify the gh CLI functionality and Alpine package availability, run:
#!/bin/bash
# Verify gh CLI is used in the codebase
echo "=== Checking gh CLI usage in server code ==="
rg -n '\bgh\b' apps/server/src/ --type ts -C2 | head -50
# Check Alpine package availability and versions
echo -e "\n=== Checking Alpine github-cli package availability ==="
echo "Testing with Alpine container..."
docker run --rm alpine:3.19 sh -c 'apk update && apk search github-cli'🤖 Prompt for AI Agents
In apps/server/Dockerfile around lines 29-35, the Dockerfile downloads the
upstream linux_amd64 gh tarball (glibc-linked) which is incompatible with
Alpine's musl and lacks checksum verification; replace that block to install the
Alpine-native github-cli package via apk (e.g., apk add --no-cache git
github-cli) instead of downloading/extracting the binary, optionally pin the
package version to a specific Alpine package release if reproducible builds are
required (verify the exact version in the Alpine repo before pinning), remove
the curl/tar/mv/rm steps and any temporary files, and ensure supply-chain safety
by preferring the package manager or adding checksum/signature verification if
you must download an external binary.
| export async function stat(filePath: string): Promise<any> { | ||
| const validatedPath = validatePath(filePath); | ||
| return fs.stat(validatedPath); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Use proper return types instead of any.
Both stat and lstat return any, which bypasses TypeScript's type safety. These should return the proper type from fs/promises:
🔎 Proposed fix for type safety
+import type { Stats } from "fs";
/**
* Wrapper around fs.stat that validates path first
*/
-export async function stat(filePath: string): Promise<any> {
+export async function stat(filePath: string): Promise<Stats> {
const validatedPath = validatePath(filePath);
return fs.stat(validatedPath);
}
/**
* Wrapper around fs.lstat that validates path first
* Returns file stats without following symbolic links
*/
-export async function lstat(filePath: string): Promise<any> {
+export async function lstat(filePath: string): Promise<Stats> {
const validatedPath = validatePath(filePath);
return fs.lstat(validatedPath);
}Also applies to: 149-152
🤖 Prompt for AI Agents
In apps/server/src/lib/secure-fs.ts around lines 84-87 (and also for lstat at
149-152), the functions return Promise<any>; change them to return
Promise<Stats> by importing the proper type (import type { Stats } from 'fs')
and updating the function signatures to stat(filePath: string): Promise<Stats>
and lstat(filePath: string): Promise<Stats>; keep the existing validatePath and
fs.promises calls but replace any with the Stats type to restore TypeScript type
safety.
PR Review: Docker Security ImprovementsI have completed a comprehensive review of this PR. This is a well-architected security enhancement that introduces defense-in-depth path validation. Overall, the changes are solid, but I have identified several important issues that should be addressed. Strengths1. Excellent Defense-in-Depth Architecture
2. Clean Middleware Design
3. Good Test Coverage
CRITICAL: Incomplete Migration to secure-fsProblem: Many files still use direct fs imports instead of secure-fs, creating security bypass opportunities. Found 30+ files that import fs directly, including:
Why this matters:
Recommendation: Migrate all files to use secure-fs for true defense-in-depth. HIGH: Redundant Validation in clone.tsLocation: apps/server/src/routes/templates/routes/clone.ts:52-64 The custom path traversal check is redundant since isPathAllowed already handles this. Two sources of truth is error-prone. Recommendation: Remove custom check, rely on isPathAllowed. MEDIUM IssuesInconsistent Error HandlingSome routes use middleware validation, others use inline. This creates confusion. Recommendation: Standardize on middleware approach and document exceptions. Missing Test Cases
Type Safety in secure-fs.tsUsing Promise at lines 84 and 149 defeats TypeScript benefits. Recommendation: Use Promise from fs module. Security AnalysisStrengths:
Concerns:
Final RecommendationsMust Fix Before Merge:
Should Fix Before Merge:
Consider for Future:
VerdictRating: 7/10 - Good foundation, needs completion The core design is excellent, but the incomplete migration from fs to secure-fs undermines the security improvements. Recommendation: Request changes to address CRITICAL and HIGH issues before merging. Great work on the architecture! Once the migration is complete, this will significantly improve security. |
Summary by CodeRabbit
Configuration Changes
Security Improvements
Infrastructure
UI / UX
✏️ Tip: You can customize this high-level summary in your review settings.