refactor: replace fs with secureFs for improved file handling #206
Conversation
This commit updates various modules to utilize the secure file system operations from the secureFs module instead of the native fs module. Key changes include: - Replaced fs imports with secureFs in multiple route handlers and services to enhance security and consistency in file operations. - Added centralized validation for working directories in the sdk-options module to ensure all AI model invocations are secure. These changes aim to improve the security and maintainability of file handling across the application.
|
Caution Review failedThe pull request is closed. Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughCentralizes working-directory validation, replaces fs/promises with a secure-fs abstraction across server routes and services, expands the UI theme system with many new themes and theme UI, and normalizes string literal and type literal quoting (double → single) across the codebase. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Areas requiring extra attention:
Possibly related PRs
Suggested labels
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
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 refactors the application's file handling mechanisms to prioritize security and maintainability. By replacing direct Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
PR Review: Refactor to use secureFs for improved file handlingOverviewThis PR makes significant security and maintainability improvements by replacing native ✅ StrengthsSecurity Improvements (Server-Side)
Code Quality
|
![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 is a substantial and valuable pull request that significantly enhances the application's security posture by centralizing file system operations through a new secureFs module. The introduction of validateWorkingDirectory in the SDK options is a particularly strong improvement, ensuring all AI agent actions are properly sandboxed. The extensive UI refactoring to introduce many new themes and improve the theme selection experience is also a fantastic addition. I have a couple of suggestions to further improve security and type safety, but overall, this is excellent work.
| await mkdirSafe(path.dirname(path.resolve(filePath))); | ||
| await secureFs.writeFile(filePath, content, 'utf-8'); |
There was a problem hiding this comment.
This call to mkdirSafe could introduce a security vulnerability. The secureFs.writeFile on the next line correctly validates the full filePath, but mkdirSafe operates on the parent directory without the same validation. If filePath were to contain path traversal sequences (e.g., ../), path.resolve would resolve it, and mkdirSafe might then create a directory outside of the ALLOWED_ROOT_DIRECTORY before writeFile has a chance to throw a security error.
For a defense-in-depth approach, this directory creation step should also be secure. I recommend replacing mkdirSafe with a direct call to secureFs.mkdir, which includes the necessary path validation.
| await mkdirSafe(path.dirname(path.resolve(filePath))); | |
| await secureFs.writeFile(filePath, content, 'utf-8'); | |
| await secureFs.mkdir(path.dirname(filePath), { recursive: true }); | |
| await secureFs.writeFile(filePath, content, 'utf-8'); |
| try { | ||
| const metadataPath = getWorktreeMetadataPath(projectPath, branch); | ||
| const content = await fs.readFile(metadataPath, "utf-8"); | ||
| const content = (await secureFs.readFile(metadataPath, 'utf-8')) as string; |
There was a problem hiding this comment.
The type assertion as string is necessary here because the secureFs.readFile wrapper returns Promise<string | Buffer>. To improve type safety and remove the need for casting throughout the codebase, consider adding function overloads to the secureFs.readFile implementation. This would allow TypeScript to correctly infer a Promise<string> return type when an encoding is provided, mirroring the behavior of the native fs.readFile and making the code more robust.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
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/routes/fs/routes/mkdir.ts (1)
26-31: Symlink handling may be overly permissive.Line 28 treats both directories and symbolic links as success. However, a symlink could point to a file rather than a directory. If the caller expects a directory to exist, this could cause issues downstream.
Consider verifying the symlink target:
Proposed fix
const stats = await secureFs.lstat(resolvedPath); // Path exists - if it's a directory or symlink, consider it success - if (stats.isDirectory() || stats.isSymbolicLink()) { + if (stats.isDirectory()) { res.json({ success: true }); return; } + // For symlinks, verify the target is a directory + if (stats.isSymbolicLink()) { + const targetStats = await secureFs.stat(resolvedPath); + if (targetStats.isDirectory()) { + res.json({ success: true }); + return; + } + } // It's a file - can't create directoryapps/server/src/routes/worktree/routes/branch-tracking.ts (1)
12-16: Consider backward compatibility for the requiredcreatedAtfield.The
createdAtfield is defined as required in theTrackedBranchinterface, but existing tracked branch data files (.automaker/active-branches.json) created before this change won't have this field. This could cause issues when reading existing data.🔎 Suggested fix: Make createdAt optional or add migration logic
Option 1: Make the field optional to handle existing data gracefully:
export interface TrackedBranch { name: string; - createdAt: string; + createdAt?: string; lastActivatedAt?: string; }Option 2: Add migration logic in
getTrackedBranchesto populate missingcreatedAtvalues:export async function getTrackedBranches(projectPath: string): Promise<TrackedBranch[]> { try { const filePath = getBranchTrackingPath(projectPath); const content = (await secureFs.readFile(filePath, 'utf-8')) as string; const data: BranchTrackingData = JSON.parse(content); - return data.branches || []; + const branches = data.branches || []; + // Migrate old data without createdAt + return branches.map(b => ({ + ...b, + createdAt: b.createdAt || new Date().toISOString() + })); } catch (error: any) {apps/server/src/routes/worktree/routes/create.ts (1)
149-156: Potential command injection viabranchNamein git commands.The
branchNamefrom user input is used directly in shell commands without sanitization or quoting. While thesanitizedNameis created for directory paths (line 129), the originalbranchNameis used in git commands:
- Line 151:
git worktree add "${worktreePath}" ${branchName}- Line 155:
git worktree add -b ${branchName} "${worktreePath}" ${base}A malicious branch name like
main; rm -rf /could execute arbitrary commands.🔎 Proposed fix
Either validate/sanitize the branch name or properly quote it:
if (branchExists) { // Use existing branch - createCmd = `git worktree add "${worktreePath}" ${branchName}`; + createCmd = `git worktree add "${worktreePath}" "${branchName}"`; } else { // Create new branch from base or HEAD const base = baseBranch || 'HEAD'; - createCmd = `git worktree add -b ${branchName} "${worktreePath}" ${base}`; + createCmd = `git worktree add -b "${branchName}" "${worktreePath}" "${base}"`; }Better yet, validate
branchNameagainst a safe pattern (e.g.,/^[a-zA-Z0-9_\-\/]+$/) before use.
🧹 Nitpick comments (25)
apps/ui/src/styles/themes/sepia.css (2)
85-87: Inconsistent color format: convert hex colors to OKLCH for consistency.Lines 86-87 use hex color values while the entire theme uses OKLCH custom properties. For consistency and maintainability, convert these to OKLCH format to match the rest of the theme palette.
.sepia .animated-outline-gradient { - background: conic-gradient(from 90deg at 50% 50%, #92400e 0%, #a16207 50%, #92400e 100%); + background: conic-gradient(from 90deg at 50% 50%, oklch(0.45 0.14 50) 0%, oklch(0.5 0.12 50) 50%, oklch(0.45 0.14 50) 100%); }
89-92: Avoid!important; prefer CSS specificity or a dedicated override class.Using
!importantin lines 90-91 can create maintenance challenges and conflicts with component-level styles. Consider using a more specific selector or a dedicated override class instead.-.sepia .animated-outline-inner { - background: oklch(0.97 0.02 70) !important; - color: #92400e !important; +.sepia .animated-outline-inner { + background: oklch(0.97 0.02 70); + color: oklch(0.45 0.14 50); }Note: Also convert the hex color to OKLCH format (as mentioned in the previous comment) for full consistency.
apps/server/src/routes/fs/routes/delete-board-background.ts (1)
27-37: Consider more specific error handling in the catch block.The empty catch block at line 35 swallows all errors, including potentially important ones like
PathNotAllowedErroror file permission issues. While the comment indicates that a missing directory is acceptable, this broad error handling could mask security violations or other problems.Consider handling specific error cases:
- } catch { - // Directory may not exist, that's fine + } catch (error) { + // Directory may not exist, that's fine + // But propagate security errors + if (error instanceof PathNotAllowedError) { + throw error; + } + // Otherwise, silently continue (directory doesn't exist) }This would ensure security boundaries are respected while maintaining the lenient behavior for missing directories.
apps/server/src/routes/fs/routes/validate-path.ts (1)
24-39: Consider handling PathNotAllowedError in the inner catch block.The empty catch at line 37 treats all errors as "Path does not exist", including
PathNotAllowedError. This could be misleading – a forbidden path would be reported as non-existent rather than as a security violation.🔎 Recommended improvement for clearer error messaging
} catch { - res.status(400).json({ success: false, error: 'Path does not exist' }); + } catch (error) { + // Re-throw security errors to be handled by outer catch + if (error instanceof PathNotAllowedError) { + throw error; + } + res.status(400).json({ success: false, error: 'Path does not exist' }); }This would allow the outer catch to return a 403 for security violations (see next comment).
apps/ui/src/styles/themes/solarizedlight.css (1)
85-92: Consider converting hex colors to oklch for consistency.Lines 86 and 91 use hex color values (
#268bd2,#2aa198) while the rest of the theme uses oklch. For consistency with the Paper theme and modern CSS practices, consider converting these to oklch:.solarizedlight .animated-outline-gradient { - background: conic-gradient(from 90deg at 50% 50%, #268bd2 0%, #2aa198 50%, #268bd2 100%); + background: conic-gradient( + from 90deg at 50% 50%, + oklch(0.55 0.15 230) 0%, + oklch(0.6 0.15 180) 50%, + oklch(0.55 0.15 230) 100% + ); } .solarizedlight .animated-outline-inner { background: oklch(0.98 0.01 90) !important; - color: #268bd2 !important; + color: oklch(0.55 0.15 230) !important; }This maintains the Solarized color identity while aligning with the oklch color space used throughout the theme.
apps/server/src/routes/fs/routes/write.ts (1)
26-27: Potential path inconsistency between directory creation and file write.
mkdirSafereceivespath.resolve(filePath)(absolute), butsecureFs.writeFilereceives the rawfilePath(possibly relative). IfsecureFs.writeFileinternally resolves paths differently, this could lead to unexpected behavior or a subtle TOCTOU gap.Consider using the resolved path consistently:
Proposed fix
+ const resolvedPath = path.resolve(filePath); // Ensure parent directory exists (symlink-safe) - await mkdirSafe(path.dirname(path.resolve(filePath))); - await secureFs.writeFile(filePath, content, 'utf-8'); + await mkdirSafe(path.dirname(resolvedPath)); + await secureFs.writeFile(resolvedPath, content, 'utf-8');apps/server/src/routes/fs/routes/read.ts (1)
18-20: Consider extractingisENOENTto a shared utility.This function is duplicated from
apps/server/src/routes/worktree/common.ts(lines 117-119). Consider importing it from a shared location to reduce duplication.apps/server/src/routes/fs/routes/save-image.ts (1)
49-52: Consider addingPathNotAllowedErrorhandling for consistency.Other handlers in this PR (e.g.,
image.ts,browse.ts) explicitly catchPathNotAllowedErrorand return a 403 status. This handler catches all errors generically. For consistency, consider adding specific handling:🔎 Proposed fix
+import { PathNotAllowedError } from '@automaker/platform'; + // ... inside the handler } catch (error) { + if (error instanceof PathNotAllowedError) { + res.status(403).json({ success: false, error: 'Path not allowed' }); + return; + } logError(error, 'Save image failed'); res.status(500).json({ success: false, error: getErrorMessage(error) }); }apps/server/src/routes/fs/routes/resolve-directory.ts (2)
42-49: Consider extracting the home directory resolution for clarity.The pattern
process.env.HOME || process.env.USERPROFILE || ''is repeated four times. Extracting it would improve readability:🔎 Proposed refactor
+ const homeDir = process.env.HOME || process.env.USERPROFILE || ''; const searchPaths: string[] = [ process.cwd(), - process.env.HOME || process.env.USERPROFILE || '', - path.join(process.env.HOME || process.env.USERPROFILE || '', 'Documents'), - path.join(process.env.HOME || process.env.USERPROFILE || '', 'Desktop'), - path.join(process.env.HOME || process.env.USERPROFILE || '', 'Projects'), + homeDir, + path.join(homeDir, 'Documents'), + path.join(homeDir, 'Desktop'), + path.join(homeDir, 'Projects'), ].filter(Boolean);
112-115: Consider addingPathNotAllowedErrorhandling for consistency.Similar to other handlers in this PR, explicit handling of
PathNotAllowedErrorwould provide consistent 403 responses when accessing restricted paths:🔎 Proposed fix
+import { PathNotAllowedError } from '@automaker/platform'; + // ... inside the handler } catch (error) { + if (error instanceof PathNotAllowedError) { + res.status(403).json({ success: false, error: 'Path not allowed' }); + return; + } logError(error, 'Resolve directory failed'); res.status(500).json({ success: false, error: getErrorMessage(error) }); }apps/server/src/routes/fs/routes/browse.ts (1)
89-92: Permission warning is macOS-specific.The warning message references "System Preferences > Privacy & Security" which is specific to macOS. Consider making this platform-aware since the code also handles Windows drives:
🔎 Proposed fix
+ const warningMessage = os.platform() === 'darwin' + ? 'Permission denied - grant Full Disk Access to Terminal in System Preferences > Privacy & Security' + : 'Permission denied - check folder permissions'; res.json({ success: true, currentPath: targetPath, parentPath: hasParent ? parentPath : null, directories: [], drives, - warning: - 'Permission denied - grant Full Disk Access to Terminal in System Preferences > Privacy & Security', + warning: warningMessage, });apps/ui/src/styles/themes/lavender.css (1)
85-92: Consider using CSS variables for hard-coded gradient colors.The animated outline sections use hard-coded hex color values (#a855f7, #8b5cf6) which could reference existing CSS variables (e.g., var(--primary), var(--brand-500)) for better maintainability and consistency with the rest of the theme.
💡 Example refactor using CSS variables
.lavender .animated-outline-gradient { - background: conic-gradient(from 90deg at 50% 50%, #a855f7 0%, #8b5cf6 50%, #a855f7 100%); + background: conic-gradient(from 90deg at 50% 50%, var(--primary) 0%, var(--brand-600) 50%, var(--primary) 100%); } .lavender .animated-outline-inner { background: oklch(0.98 0.015 285) !important; - color: #a855f7 !important; + color: var(--primary) !important; }apps/ui/src/styles/themes/blossom.css (1)
85-92: Consider using CSS variables for hard-coded gradient colors.The animated outline sections use hard-coded hex color values (#ec4899, #f472b6) which could reference existing CSS variables for better maintainability and consistency with the theme system.
💡 Example refactor using CSS variables
.blossom .animated-outline-gradient { - background: conic-gradient(from 90deg at 50% 50%, #ec4899 0%, #f472b6 50%, #ec4899 100%); + background: conic-gradient(from 90deg at 50% 50%, var(--primary) 0%, var(--brand-400) 50%, var(--primary) 100%); } .blossom .animated-outline-inner { background: oklch(0.99 0.015 330) !important; - color: #ec4899 !important; + color: var(--primary) !important; }apps/ui/src/styles/themes/forest.css (1)
91-98: Consider using CSS variables for hard-coded gradient colors.The animated outline sections use hard-coded hex color values (#22c55e, #16a34a, #86efac) which could reference existing CSS variables for better maintainability.
💡 Example refactor using CSS variables
.forest .animated-outline-gradient { - background: conic-gradient(from 90deg at 50% 50%, #22c55e 0%, #16a34a 50%, #22c55e 100%); + background: conic-gradient(from 90deg at 50% 50%, var(--primary) 0%, var(--brand-600) 50%, var(--primary) 100%); } .forest .animated-outline-inner { background: oklch(0.16 0.025 150) !important; - color: #86efac !important; + color: var(--brand-400) !important; }apps/ui/src/styles/themes/ocean.css (1)
91-98: Consider using CSS variables for hard-coded gradient colors.The animated outline sections use hard-coded hex color values (#3b82f6, #0ea5e9, #93c5fd) which could reference existing CSS variables for better maintainability and consistency.
💡 Example refactor using CSS variables
.ocean .animated-outline-gradient { - background: conic-gradient(from 90deg at 50% 50%, #3b82f6 0%, #0ea5e9 50%, #3b82f6 100%); + background: conic-gradient(from 90deg at 50% 50%, var(--primary) 0%, var(--chart-2) 50%, var(--primary) 100%); } .ocean .animated-outline-inner { background: oklch(0.16 0.035 230) !important; - color: #93c5fd !important; + color: var(--brand-400) !important; }apps/server/src/routes/worktree/routes/status.ts (1)
12-12: Consider importing execAsync from common.ts to avoid duplication.The file defines its own
execAsync = promisify(exec)on line 12, but according to the relevant code snippets,common.tsalready exportsexecAsyncat line 12. Consider importing it instead to maintain a single source of truth.🔎 Proposed refactor
-import { getErrorMessage, logError } from '../common.js'; +import { execAsync, getErrorMessage, logError } from '../common.js'; -const execAsync = promisify(exec);Remove the unused imports:
-import { exec } from 'child_process'; -import { promisify } from 'util';apps/ui/src/styles/themes/gruvboxlight.css (1)
85-92: Consider using CSS variables for hard-coded gradient colors.The animated outline sections use hard-coded hex color values (#d79921, #b57614) which could reference existing CSS variables for better maintainability and consistency with the theme system.
💡 Example refactor using CSS variables
.gruvboxlight .animated-outline-gradient { - background: conic-gradient(from 90deg at 50% 50%, #d79921 0%, #b57614 50%, #d79921 100%); + background: conic-gradient(from 90deg at 50% 50%, var(--primary) 0%, var(--brand-600) 50%, var(--primary) 100%); } .gruvboxlight .animated-outline-inner { background: oklch(0.98 0.015 85) !important; - color: #b57614 !important; + color: var(--brand-600) !important; }apps/ui/src/components/views/setup-view/steps/theme-step.tsx (1)
41-98: Consider extracting a sharedThemeTabGroupcomponent.The dark/light tab toggle and theme grid rendering logic is nearly identical to
appearance-section.tsx. Extracting a reusable component would reduce duplication and ensure consistent behavior across both views.apps/ui/src/styles/themes/nordlight.css (1)
85-92: Consider using CSS variables instead of hardcoded hex colors.Lines 86 and 91 use hardcoded hex color values (
#5e81ac,#81a1c1) instead of CSS custom properties. For consistency and maintainability, consider using the CSS variables defined earlier (e.g.,var(--primary)orvar(--sidebar-primary)).🔎 Example refactor using CSS variables
.nordlight .animated-outline-gradient { - background: conic-gradient(from 90deg at 50% 50%, #5e81ac 0%, #81a1c1 50%, #5e81ac 100%); + background: conic-gradient(from 90deg at 50% 50%, var(--primary) 0%, var(--brand-400) 50%, var(--primary) 100%); } .nordlight .animated-outline-inner { background: oklch(0.99 0.008 220) !important; - color: #5e81ac !important; + color: var(--primary) !important; }apps/ui/src/styles/themes/snow.css (1)
85-92: Consider using CSS variables instead of hardcoded hex colors.Similar to other theme files, lines 86 and 91 use hardcoded hex color values (
#6366f1,#8b5cf6) instead of CSS custom properties. For consistency and maintainability across all theme files, consider using the CSS variables defined earlier.🔎 Example refactor using CSS variables
.snow .animated-outline-gradient { - background: conic-gradient(from 90deg at 50% 50%, #6366f1 0%, #8b5cf6 50%, #6366f1 100%); + background: conic-gradient(from 90deg at 50% 50%, var(--primary) 0%, var(--chart-4) 50%, var(--primary) 100%); } .snow .animated-outline-inner { background: oklch(1 0 0) !important; - color: #6366f1 !important; + color: var(--primary) !important; }apps/ui/src/routes/__root.tsx (1)
129-132: Optional: Simplify the type assertion.The type assertion
('system' as ThemeOption['value'])could be simplified to just'system'since TypeScript can infer the comparison:const themeClasses = themeOptions .map((option) => option.value) - .filter((theme) => theme !== ('system' as ThemeOption['value'])); + .filter((theme) => theme !== 'system');apps/server/src/routes/worktree/routes/create.ts (1)
24-24: Consider importingexecAsyncfrom common module.
execAsyncis already exported from../common.js(per the relevant code snippets). Importing it instead of redeclaring locally would reduce duplication:import { isGitRepo, getErrorMessage, logError, normalizePath, ensureInitialCommit, + execAsync, } from '../common.js'; import { trackBranch } from './branch-tracking.js'; -const execAsync = promisify(exec);apps/server/src/routes/templates/routes/clone.ts (1)
96-109: Optional: Simplify parent directory existence check.The nested try/catch for setting
parentExistsis verbose. Consider using a helper:- let parentExists = false; - try { - await secureFs.access(parentDir); - parentExists = true; - } catch { - parentExists = false; - } + const parentExists = await secureFs.access(parentDir).then(() => true, () => false); if (!parentExists) {apps/ui/src/components/layout/sidebar.tsx (1)
196-197: Remove redundantkeyprop from internal wrapper div.The
key={option.value}on line 197 inside theThemeMenuItemcomponent is redundant since this component is rendered via.map()and thekeyis already correctly placed on the<ThemeMenuItem>element at the call sites (lines 1672, 1689). Thekeyprop on internal elements doesn't serve React's reconciliation purpose.🔎 Proposed fix
return ( <div - key={option.value} onPointerEnter={() => onPreviewEnter(option.value)} onPointerLeave={onPreviewLeave} >apps/server/src/routes/app-spec/generate-spec.ts (1)
41-61: Consider extracting the default tech stack configuration.The default technology stack (lines 53-60) is hardcoded as a multi-line string. Consider extracting this to a configuration constant for easier maintenance and reusability.
📋 Optional refactor: Extract to constant
At the top of the file, add:
const DEFAULT_TECH_STACK = `Default Technology Stack: - Framework: TanStack Start (React-based full-stack framework) - Database: PostgreSQL with Drizzle ORM - UI Components: shadcn/ui - Styling: Tailwind CSS - Frontend: React Use these technologies as the foundation for the specification.`;Then use:
} else { // Use default tech stack - techStackDefaults = `Default Technology Stack: - ... - Use these technologies as the foundation for the specification.`; + techStackDefaults = DEFAULT_TECH_STACK; }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (62)
apps/server/src/lib/sdk-options.ts(11 hunks)apps/server/src/lib/worktree-metadata.ts(7 hunks)apps/server/src/routes/app-spec/generate-features-from-spec.ts(4 hunks)apps/server/src/routes/app-spec/generate-spec.ts(5 hunks)apps/server/src/routes/app-spec/parse-and-create-features.ts(2 hunks)apps/server/src/routes/fs/routes/browse.ts(6 hunks)apps/server/src/routes/fs/routes/delete-board-background.ts(4 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/image.ts(2 hunks)apps/server/src/routes/fs/routes/mkdir.ts(3 hunks)apps/server/src/routes/fs/routes/read.ts(3 hunks)apps/server/src/routes/fs/routes/readdir.ts(2 hunks)apps/server/src/routes/fs/routes/resolve-directory.ts(5 hunks)apps/server/src/routes/fs/routes/save-board-background.ts(2 hunks)apps/server/src/routes/fs/routes/save-image.ts(2 hunks)apps/server/src/routes/fs/routes/stat.ts(2 hunks)apps/server/src/routes/fs/routes/validate-path.ts(2 hunks)apps/server/src/routes/fs/routes/write.ts(3 hunks)apps/server/src/routes/templates/routes/clone.ts(8 hunks)apps/server/src/routes/workspace/routes/config.ts(3 hunks)apps/server/src/routes/workspace/routes/directories.ts(4 hunks)apps/server/src/routes/worktree/common.ts(5 hunks)apps/server/src/routes/worktree/routes/branch-tracking.ts(4 hunks)apps/server/src/routes/worktree/routes/create.ts(7 hunks)apps/server/src/routes/worktree/routes/diffs.ts(4 hunks)apps/server/src/routes/worktree/routes/file-diff.ts(2 hunks)apps/server/src/routes/worktree/routes/info.ts(3 hunks)apps/server/src/routes/worktree/routes/init-git.ts(2 hunks)apps/server/src/routes/worktree/routes/list.ts(7 hunks)apps/server/src/routes/worktree/routes/status.ts(2 hunks)apps/server/src/services/agent-service.ts(19 hunks)apps/server/src/services/auto-mode-service.ts(86 hunks)apps/server/src/services/dev-server-service.ts(13 hunks)apps/ui/src/components/layout/sidebar.tsx(82 hunks)apps/ui/src/components/views/settings-view/appearance/appearance-section.tsx(3 hunks)apps/ui/src/components/views/settings-view/shared/types.ts(2 hunks)apps/ui/src/components/views/setup-view/steps/theme-step.tsx(4 hunks)apps/ui/src/config/terminal-themes.ts(3 hunks)apps/ui/src/config/theme-options.ts(1 hunks)apps/ui/src/routes/__root.tsx(8 hunks)apps/ui/src/store/app-store.ts(73 hunks)apps/ui/src/styles/global.css(8 hunks)apps/ui/src/styles/theme-imports.ts(1 hunks)apps/ui/src/styles/themes/blossom.css(1 hunks)apps/ui/src/styles/themes/forest.css(1 hunks)apps/ui/src/styles/themes/github.css(1 hunks)apps/ui/src/styles/themes/gruvboxlight.css(1 hunks)apps/ui/src/styles/themes/lavender.css(1 hunks)apps/ui/src/styles/themes/mint.css(1 hunks)apps/ui/src/styles/themes/nordlight.css(1 hunks)apps/ui/src/styles/themes/ocean.css(1 hunks)apps/ui/src/styles/themes/paper.css(1 hunks)apps/ui/src/styles/themes/peach.css(1 hunks)apps/ui/src/styles/themes/rose.css(1 hunks)apps/ui/src/styles/themes/sand.css(1 hunks)apps/ui/src/styles/themes/sepia.css(1 hunks)apps/ui/src/styles/themes/sky.css(1 hunks)apps/ui/src/styles/themes/snow.css(1 hunks)apps/ui/src/styles/themes/solarizedlight.css(1 hunks)libs/git-utils/src/diff.ts(8 hunks)libs/types/src/settings.ts(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (30)
apps/server/src/routes/fs/routes/validate-path.ts (2)
apps/server/src/lib/secure-fs.ts (1)
secureFs(8-23)apps/server/src/routes/app-spec/common.ts (1)
logError(57-66)
apps/server/src/routes/worktree/routes/diffs.ts (2)
apps/server/src/lib/secure-fs.ts (1)
secureFs(8-23)apps/server/src/routes/worktree/common.ts (1)
logError(143-143)
apps/server/src/routes/fs/routes/resolve-directory.ts (2)
apps/server/src/lib/secure-fs.ts (1)
secureFs(8-23)apps/server/src/routes/app-spec/common.ts (1)
logError(57-66)
apps/server/src/routes/fs/routes/mkdir.ts (2)
apps/server/src/lib/secure-fs.ts (1)
secureFs(8-23)apps/server/src/routes/app-spec/common.ts (1)
logError(57-66)
apps/server/src/routes/worktree/routes/branch-tracking.ts (1)
apps/server/src/lib/secure-fs.ts (1)
secureFs(8-23)
apps/server/src/routes/fs/routes/delete.ts (4)
apps/server/src/routes/worktree/routes/delete.ts (1)
createDeleteHandler(13-80)apps/server/src/lib/secure-fs.ts (1)
secureFs(8-23)apps/server/src/routes/app-spec/common.ts (1)
logError(57-66)apps/server/src/routes/fs/common.ts (1)
logError(15-15)
apps/ui/src/components/views/setup-view/steps/theme-step.tsx (1)
apps/ui/src/config/theme-options.ts (2)
darkThemes(340-340)lightThemes(343-343)
apps/server/src/routes/worktree/routes/create.ts (2)
apps/server/src/routes/worktree/common.ts (3)
execAsync(13-13)isGitRepo(105-112)logError(143-143)apps/server/src/lib/secure-fs.ts (1)
secureFs(8-23)
apps/ui/src/config/theme-options.ts (1)
apps/ui/src/components/views/settings-view/shared/types.ts (1)
Theme(3-3)
apps/server/src/routes/worktree/routes/list.ts (5)
apps/server/src/routes/worktree/common.ts (3)
execAsync(13-13)normalizePath(98-100)logError(143-143)libs/types/src/settings.ts (1)
WorktreeInfo(328-339)apps/ui/src/types/electron.d.ts (1)
WorktreeInfo(578-583)apps/server/src/lib/secure-fs.ts (1)
secureFs(8-23)apps/server/src/routes/app-spec/common.ts (1)
logError(57-66)
apps/ui/src/components/layout/sidebar.tsx (3)
apps/ui/src/config/theme-options.ts (2)
darkThemes(340-340)lightThemes(343-343)apps/ui/src/store/app-store.ts (1)
ThemeMode(16-16)libs/types/src/settings.ts (1)
ThemeMode(24-59)
apps/server/src/routes/app-spec/generate-spec.ts (6)
apps/server/src/routes/templates/common.ts (1)
logger(11-11)apps/server/src/lib/sdk-options.ts (1)
createSpecGenerationOptions(173-191)apps/server/src/lib/app-spec-format.ts (3)
specOutputSchema(10-10)SpecOutput(9-9)specToXml(27-100)apps/server/src/routes/app-spec/common.ts (1)
logAuthStatus(37-52)apps/server/src/lib/secure-fs.ts (1)
secureFs(8-23)apps/server/src/routes/app-spec/generate-features-from-spec.ts (1)
generateFeaturesFromSpec(18-162)
apps/server/src/routes/fs/routes/browse.ts (2)
apps/server/src/lib/secure-fs.ts (1)
secureFs(8-23)apps/server/src/routes/worktree/common.ts (1)
logError(143-143)
apps/server/src/routes/fs/routes/image.ts (2)
apps/server/src/lib/secure-fs.ts (1)
secureFs(8-23)apps/server/src/routes/worktree/common.ts (1)
logError(143-143)
apps/server/src/routes/app-spec/generate-features-from-spec.ts (3)
apps/server/src/lib/secure-fs.ts (1)
secureFs(8-23)apps/server/src/routes/app-spec/common.ts (1)
logAuthStatus(37-52)apps/server/src/routes/app-spec/parse-and-create-features.ts (1)
parseAndCreateFeatures(13-94)
apps/server/src/routes/fs/routes/stat.ts (3)
apps/server/src/lib/secure-fs.ts (1)
secureFs(8-23)apps/server/src/routes/app-spec/common.ts (1)
logError(57-66)apps/server/src/routes/fs/common.ts (1)
logError(15-15)
apps/server/src/routes/fs/routes/read.ts (3)
apps/server/src/routes/worktree/common.ts (2)
isENOENT(118-120)logError(143-143)apps/server/src/lib/secure-fs.ts (1)
secureFs(8-23)apps/server/src/routes/app-spec/common.ts (1)
logError(57-66)
apps/server/src/routes/worktree/routes/status.ts (2)
apps/server/src/lib/secure-fs.ts (1)
secureFs(8-23)apps/server/src/routes/worktree/common.ts (2)
execAsync(13-13)logError(143-143)
apps/server/src/services/dev-server-service.ts (1)
apps/server/src/lib/secure-fs.ts (1)
secureFs(8-23)
apps/server/src/routes/fs/routes/save-board-background.ts (2)
apps/server/src/lib/secure-fs.ts (1)
secureFs(8-23)apps/server/src/routes/app-spec/common.ts (1)
logError(57-66)
apps/server/src/routes/fs/routes/write.ts (2)
apps/server/src/lib/secure-fs.ts (1)
secureFs(8-23)apps/server/src/routes/app-spec/common.ts (1)
logError(57-66)
apps/server/src/routes/fs/routes/delete-board-background.ts (2)
apps/server/src/lib/secure-fs.ts (1)
secureFs(8-23)apps/server/src/routes/app-spec/common.ts (1)
logError(57-66)
apps/server/src/services/agent-service.ts (3)
apps/ui/src/types/electron.d.ts (1)
Message(13-20)apps/server/src/lib/sdk-options.ts (1)
validateWorkingDirectory(36-48)apps/server/src/lib/secure-fs.ts (1)
secureFs(8-23)
libs/git-utils/src/diff.ts (2)
apps/server/src/lib/secure-fs.ts (1)
secureFs(8-23)libs/git-utils/src/types.ts (1)
FileStatus(34-38)
apps/server/src/routes/worktree/routes/info.ts (2)
apps/server/src/lib/secure-fs.ts (1)
secureFs(8-23)apps/server/src/routes/worktree/common.ts (2)
execAsync(13-13)logError(143-143)
apps/ui/src/config/terminal-themes.ts (2)
apps/ui/src/store/app-store.ts (1)
ThemeMode(16-16)libs/types/src/settings.ts (1)
ThemeMode(24-59)
apps/ui/src/components/views/settings-view/appearance/appearance-section.tsx (1)
apps/ui/src/config/theme-options.ts (2)
darkThemes(340-340)lightThemes(343-343)
apps/server/src/services/auto-mode-service.ts (8)
apps/server/src/routes/worktree/common.ts (1)
execAsync(13-13)apps/ui/src/store/app-store.ts (4)
PlanningMode(216-216)ParsedTask(241-247)PlanSpec(250-261)Feature(231-238)libs/types/src/settings.ts (1)
PlanningMode(65-65)init.mjs (1)
lines(83-83).github/scripts/upload-to-r2.js (1)
path(7-7)apps/server/src/lib/sdk-options.ts (1)
validateWorkingDirectory(36-48)apps/server/src/services/feature-loader.ts (2)
getFeatureDir(163-165)Feature(20-20)apps/server/src/providers/provider-factory.ts (1)
ProviderFactory(13-115)
apps/ui/src/store/app-store.ts (6)
libs/types/src/settings.ts (3)
KanbanCardDetailLevel(62-62)ThemeMode(24-59)AIProfile(128-145)libs/types/src/index.ts (4)
KanbanCardDetailLevel(76-76)ThemeMode(75-75)Feature(20-20)AIProfile(81-81)apps/ui/src/components/views/settings-view/shared/types.ts (1)
Project(24-29)apps/ui/src/lib/electron.ts (1)
Project(2657-2663)libs/types/src/feature.ts (1)
Feature(15-49)apps/ui/src/hooks/use-settings-migration.ts (2)
syncSettingsToServer(202-247)syncProjectSettingsToServer(296-325)
apps/server/src/routes/fs/routes/save-image.ts (2)
apps/server/src/lib/secure-fs.ts (1)
secureFs(8-23)apps/server/src/routes/app-spec/common.ts (1)
logError(57-66)
🪛 GitHub Actions: Test Suite
apps/server/src/services/dev-server-service.ts
[error] 1-1: startDevServer: AssertionError in tests — expected spawn to be called with npm run dev (or other package manager) for detection logic. The tests assert specific package manager commands (npm, yarn, pnpm, bun) but the spawn mock did not receive these calls.
[error] 1-1: Overall test suite for dev-server-service.ts failed. The following integration/unit tests did not pass, indicating issues with process spawning logic and state management for dev servers.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: e2e
- GitHub Check: claude-review
| } catch (error) { | ||
| logError(error, "Delete board background failed"); | ||
| logError(error, 'Delete board background failed'); | ||
| res.status(500).json({ success: false, error: getErrorMessage(error) }); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add PathNotAllowedError handling for consistency with other routes.
This route lacks specific handling for PathNotAllowedError, unlike other routes in this PR (exists.ts, delete.ts, stat.ts) which return 403 for path security violations. Currently, a PathNotAllowedError would result in a 500 response instead of the more appropriate 403 Forbidden.
🔎 Recommended fix for consistent security error handling
} catch (error) {
+ // Path not allowed - return 403 Forbidden
+ if (error instanceof PathNotAllowedError) {
+ res.status(403).json({ success: false, error: getErrorMessage(error) });
+ return;
+ }
+
logError(error, 'Delete board background failed');
res.status(500).json({ success: false, error: getErrorMessage(error) });
}📝 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.
| } catch (error) { | |
| logError(error, "Delete board background failed"); | |
| logError(error, 'Delete board background failed'); | |
| res.status(500).json({ success: false, error: getErrorMessage(error) }); | |
| } | |
| } catch (error) { | |
| // Path not allowed - return 403 Forbidden | |
| if (error instanceof PathNotAllowedError) { | |
| res.status(403).json({ success: false, error: getErrorMessage(error) }); | |
| return; | |
| } | |
| logError(error, 'Delete board background failed'); | |
| res.status(500).json({ success: false, error: getErrorMessage(error) }); | |
| } |
🤖 Prompt for AI Agents
In apps/server/src/routes/fs/routes/delete-board-background.ts around lines 40
to 43, the catch block does not treat PathNotAllowedError specially so path
security violations return 500 instead of 403; update the catch to detect if
error is an instance of PathNotAllowedError (import it if not already) and call
res.status(403).json({ success: false, error: getErrorMessage(error) }) for that
case, otherwise keep the existing logError and res.status(500) behavior for
other errors.
| } catch (error) { | ||
| logError(error, "Save board background failed"); | ||
| logError(error, 'Save board background failed'); | ||
| res.status(500).json({ success: false, error: getErrorMessage(error) }); | ||
| } | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add PathNotAllowedError handling for consistency.
Like delete-board-background.ts, this route lacks specific handling for PathNotAllowedError. For consistency with other routes in this PR (exists.ts, delete.ts, stat.ts), consider adding 403 response for path security violations.
🔎 Recommended fix for consistent security error handling
} catch (error) {
+ // Path not allowed - return 403 Forbidden
+ if (error instanceof PathNotAllowedError) {
+ res.status(403).json({ success: false, error: getErrorMessage(error) });
+ return;
+ }
+
logError(error, 'Save board background failed');
res.status(500).json({ success: false, error: getErrorMessage(error) });
}Don't forget to import PathNotAllowedError at the top of the file:
+import { PathNotAllowedError } from '@automaker/platform';📝 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.
| } catch (error) { | |
| logError(error, "Save board background failed"); | |
| logError(error, 'Save board background failed'); | |
| res.status(500).json({ success: false, error: getErrorMessage(error) }); | |
| } | |
| }; | |
| } catch (error) { | |
| // Path not allowed - return 403 Forbidden | |
| if (error instanceof PathNotAllowedError) { | |
| res.status(403).json({ success: false, error: getErrorMessage(error) }); | |
| return; | |
| } | |
| logError(error, 'Save board background failed'); | |
| res.status(500).json({ success: false, error: getErrorMessage(error) }); | |
| } | |
| }; |
🤖 Prompt for AI Agents
In apps/server/src/routes/fs/routes/save-board-background.ts around lines 47 to
51, the catch block only logs and returns a 500; add specific handling for
PathNotAllowedError to return a 403 like other routes. Update the imports to
include PathNotAllowedError, then in the catch: if error is an instance of
PathNotAllowedError respond with res.status(403).json({ success: false, error:
getErrorMessage(error) }); otherwise keep the existing logError and 500
response; ensure the logic order checks the PathNotAllowedError before the
generic error handling.
| } catch (error) { | ||
| logError(error, "Validate path failed"); | ||
| logError(error, 'Validate path failed'); | ||
| res.status(500).json({ success: false, error: getErrorMessage(error) }); | ||
| } | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add PathNotAllowedError handling for consistency.
Like several other board-related routes, this lacks specific handling for PathNotAllowedError in the outer catch. For consistency with other routes in this PR (exists.ts, delete.ts, stat.ts), add a 403 response for path security violations.
🔎 Recommended fix for consistent security error handling
} catch (error) {
+ // Path not allowed - return 403 Forbidden
+ if (error instanceof PathNotAllowedError) {
+ res.status(403).json({ success: false, error: getErrorMessage(error) });
+ return;
+ }
+
logError(error, 'Validate path failed');
res.status(500).json({ success: false, error: getErrorMessage(error) });
}Don't forget to import PathNotAllowedError at the top:
+import { PathNotAllowedError } from '@automaker/platform';📝 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.
| } catch (error) { | |
| logError(error, "Validate path failed"); | |
| logError(error, 'Validate path failed'); | |
| res.status(500).json({ success: false, error: getErrorMessage(error) }); | |
| } | |
| }; | |
| } catch (error) { | |
| // Path not allowed - return 403 Forbidden | |
| if (error instanceof PathNotAllowedError) { | |
| res.status(403).json({ success: false, error: getErrorMessage(error) }); | |
| return; | |
| } | |
| logError(error, 'Validate path failed'); | |
| res.status(500).json({ success: false, error: getErrorMessage(error) }); | |
| } | |
| }; |
🤖 Prompt for AI Agents
In apps/server/src/routes/fs/routes/validate-path.ts around lines 40 to 44, the
outer catch block does not handle PathNotAllowedError specifically; update the
catch to check if error is an instance of PathNotAllowedError and if so return
res.status(403).json({ success: false, error: getErrorMessage(error) }),
otherwise keep the existing logging and 500 response; also add an import for
PathNotAllowedError at the top of the file to ensure the instanceof check works.
| try { | ||
| await secureFs.access(gitDirPath); | ||
| // .git exists | ||
| res.json({ | ||
| success: true, | ||
| result: { | ||
| initialized: false, | ||
| message: "Git repository already exists", | ||
| message: 'Git repository already exists', | ||
| }, | ||
| }); | ||
| return; | ||
| } catch { | ||
| // .git doesn't exist, continue with initialization | ||
| } |
There was a problem hiding this comment.
Empty catch block may mask security errors from secureFs.
The catch block assumes any error from secureFs.access means ".git doesn't exist", but secureFs may throw security-related errors (e.g., path traversal attempts). Catching all errors and proceeding with initialization could bypass security checks.
Consider checking the error type or code to distinguish "file not found" from security violations:
🔎 Proposed fix
try {
await secureFs.access(gitDirPath);
// .git exists
res.json({
success: true,
result: {
initialized: false,
message: 'Git repository already exists',
},
});
return;
- } catch {
- // .git doesn't exist, continue with initialization
+ } catch (err) {
+ // Only continue if .git doesn't exist (ENOENT)
+ if ((err as NodeJS.ErrnoException).code !== 'ENOENT') {
+ throw err; // Re-throw security or other errors
+ }
+ // .git doesn't exist, continue with initialization
}| // Initialize git and create an initial empty commit | ||
| await execAsync( | ||
| `git init && git commit --allow-empty -m "Initial commit"`, | ||
| { cwd: projectPath } | ||
| ); | ||
| await execAsync(`git init && git commit --allow-empty -m "Initial commit"`, { | ||
| cwd: projectPath, | ||
| }); |
There was a problem hiding this comment.
User-controlled projectPath used in shell execution without validation.
The projectPath is taken directly from req.body and used as cwd for execAsync, bypassing the secureFs security layer. An attacker could specify any directory path (e.g., "/etc", "/var/www") to initialize git repositories in unintended locations.
The PR mentions adding validateWorkingDirectory in sdk-options.ts—consider applying similar validation here before executing shell commands:
🔎 Proposed fix (example pattern)
+import { validateWorkingDirectory } from '../../../lib/validate-path.js'; // adjust import as needed
export function createInitGitHandler() {
return async (req: Request, res: Response): Promise<void> => {
try {
const { projectPath } = req.body as {
projectPath: string;
};
if (!projectPath) {
res.status(400).json({
success: false,
error: 'projectPath required',
});
return;
}
+ // Validate projectPath is within allowed directories
+ validateWorkingDirectory(projectPath);
+
// Check if .git already exists
const gitDirPath = join(projectPath, '.git');Run the following script to check if other worktree routes validate projectPath before shell execution:
#!/bin/bash
# Check how other routes handle projectPath validation before execAsync/exec calls
rg -n -B5 -A5 'execAsync|exec\(' apps/server/src/routes/worktree/🤖 Prompt for AI Agents
In apps/server/src/routes/worktree/routes/init-git.ts around lines 46-49, the
code uses the user-provided projectPath directly as cwd for execAsync which
bypasses secureFs and allows initializing git in arbitrary filesystem locations;
before calling execAsync, run the same validateWorkingDirectory (or equivalent
logic from sdk-options.ts) to ensure projectPath is within the allowed workspace
root, normalize and resolve the path, reject or return 400 on validation
failure, and then use the validated/resolved path as cwd (or perform the git
init via the secureFs layer) so no unvalidated user path is passed to shell
execution.
|
|
||
| {/* Nav Items - Scrollable */} | ||
| <nav className={cn("flex-1 overflow-y-auto px-3 pb-2", sidebarOpen ? "mt-5" : "mt1")}> | ||
| <nav className={cn('flex-1 overflow-y-auto px-3 pb-2', sidebarOpen ? 'mt-5' : 'mt1')}> |
There was a problem hiding this comment.
Fix Tailwind class typo: mt1 should be mt-1.
The class mt1 is not a valid Tailwind utility. It should be mt-1 to add margin-top.
🔎 Proposed fix
- <nav className={cn('flex-1 overflow-y-auto px-3 pb-2', sidebarOpen ? 'mt-5' : 'mt1')}>
+ <nav className={cn('flex-1 overflow-y-auto px-3 pb-2', sidebarOpen ? 'mt-5' : 'mt-1')}>📝 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.
| <nav className={cn('flex-1 overflow-y-auto px-3 pb-2', sidebarOpen ? 'mt-5' : 'mt1')}> | |
| <nav className={cn('flex-1 overflow-y-auto px-3 pb-2', sidebarOpen ? 'mt-5' : 'mt-1')}> |
🤖 Prompt for AI Agents
In apps/ui/src/components/layout/sidebar.tsx around line 1751, the Tailwind
class string uses an invalid utility 'mt1'; replace it with the correct 'mt-1'
so the conditional class becomes sidebarOpen ? 'mt-5' : 'mt-1' to apply the
intended top margin when sidebarOpen is false.
| <button | ||
| onClick={() => setActiveTab('dark')} | ||
| className={cn( | ||
| 'flex items-center gap-1.5 px-3 py-1.5 rounded-md text-sm font-medium transition-all duration-200', | ||
| activeTab === 'dark' | ||
| ? 'bg-brand-500 text-white shadow-sm' | ||
| : 'text-muted-foreground hover:text-foreground' | ||
| )} | ||
| > | ||
| <Moon className="w-3.5 h-3.5" /> | ||
| Dark | ||
| </button> | ||
| <button | ||
| onClick={() => setActiveTab('light')} | ||
| className={cn( | ||
| 'flex items-center gap-1.5 px-3 py-1.5 rounded-md text-sm font-medium transition-all duration-200', | ||
| activeTab === 'light' | ||
| ? 'bg-brand-500 text-white shadow-sm' | ||
| : 'text-muted-foreground hover:text-foreground' | ||
| )} | ||
| > | ||
| <Sun className="w-3.5 h-3.5" /> | ||
| Light | ||
| </button> | ||
| </div> |
There was a problem hiding this comment.
Add type="button" to tab buttons.
Same concern as in theme-step.tsx: these buttons default to type="submit" and could cause issues if the component is placed inside a form.
🔎 Proposed fix
<button
+ type="button"
onClick={() => setActiveTab('dark')}
className={cn(
'flex items-center gap-1.5 px-3 py-1.5 rounded-md text-sm font-medium transition-all duration-200',
activeTab === 'dark'
? 'bg-brand-500 text-white shadow-sm'
: 'text-muted-foreground hover:text-foreground'
)}
>
<Moon className="w-3.5 h-3.5" />
Dark
</button>
<button
+ type="button"
onClick={() => setActiveTab('light')}
className={cn(📝 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.
| <button | |
| onClick={() => setActiveTab('dark')} | |
| className={cn( | |
| 'flex items-center gap-1.5 px-3 py-1.5 rounded-md text-sm font-medium transition-all duration-200', | |
| activeTab === 'dark' | |
| ? 'bg-brand-500 text-white shadow-sm' | |
| : 'text-muted-foreground hover:text-foreground' | |
| )} | |
| > | |
| <Moon className="w-3.5 h-3.5" /> | |
| Dark | |
| </button> | |
| <button | |
| onClick={() => setActiveTab('light')} | |
| className={cn( | |
| 'flex items-center gap-1.5 px-3 py-1.5 rounded-md text-sm font-medium transition-all duration-200', | |
| activeTab === 'light' | |
| ? 'bg-brand-500 text-white shadow-sm' | |
| : 'text-muted-foreground hover:text-foreground' | |
| )} | |
| > | |
| <Sun className="w-3.5 h-3.5" /> | |
| Light | |
| </button> | |
| </div> | |
| <button | |
| type="button" | |
| onClick={() => setActiveTab('dark')} | |
| className={cn( | |
| 'flex items-center gap-1.5 px-3 py-1.5 rounded-md text-sm font-medium transition-all duration-200', | |
| activeTab === 'dark' | |
| ? 'bg-brand-500 text-white shadow-sm' | |
| : 'text-muted-foreground hover:text-foreground' | |
| )} | |
| > | |
| <Moon className="w-3.5 h-3.5" /> | |
| Dark | |
| </button> | |
| <button | |
| type="button" | |
| onClick={() => setActiveTab('light')} | |
| className={cn( | |
| 'flex items-center gap-1.5 px-3 py-1.5 rounded-md text-sm font-medium transition-all duration-200', | |
| activeTab === 'light' | |
| ? 'bg-brand-500 text-white shadow-sm' | |
| : 'text-muted-foreground hover:text-foreground' | |
| )} | |
| > | |
| <Sun className="w-3.5 h-3.5" /> | |
| Light | |
| </button> | |
| </div> |
🤖 Prompt for AI Agents
In apps/ui/src/components/views/settings-view/appearance/appearance-section.tsx
around lines 54 to 78, the Dark/Light tab buttons are missing an explicit type
attribute and will default to type="submit" when inside a form; update both
<button> elements to include type="button" to prevent accidental form submits,
leaving all other props and className logic unchanged.
| name: string; | ||
| path: string; | ||
| theme?: Theme; | ||
| theme?: string; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, find the types.ts file and examine its current state
cat -n "apps/ui/src/components/views/settings-view/shared/types.ts" | head -50Repository: AutoMaker-Org/automaker
Length of output: 1041
🏁 Script executed:
#!/bin/bash
# Search for how Project.theme is used in the codebase
rg -n "\.theme\s*=" apps/ui/src --type=ts -A 2 -B 2 | head -100Repository: AutoMaker-Org/automaker
Length of output: 911
🏁 Script executed:
#!/bin/bash
# Search for where Project objects are created or modified with theme values
rg -n "theme\s*:\s*" apps/ui/src --type=ts -B 2 -A 2 | grep -A 5 -B 5 "Project\|theme:" | head -150Repository: AutoMaker-Org/automaker
Length of output: 8177
🏁 Script executed:
#!/bin/bash
# Check what ThemeMode and Theme types are
rg -n "type ThemeMode|type Theme|export.*Theme" apps/ui/src --type=ts | head -50Repository: AutoMaker-Org/automaker
Length of output: 1472
🏁 Script executed:
#!/bin/bash
# Get the full Theme type definition
sed -n '30,40p' apps/ui/src/config/theme-options.tsRepository: AutoMaker-Org/automaker
Length of output: 247
🏁 Script executed:
#!/bin/bash
# Get the full Theme type definition
sed -n '32,67p' apps/ui/src/config/theme-options.tsRepository: AutoMaker-Org/automaker
Length of output: 560
🏁 Script executed:
#!/bin/bash
# Check what ThemeMode is
rg -n "type ThemeMode\s*=" apps/ui/src --type=ts -A 5Repository: AutoMaker-Org/automaker
Length of output: 49
🏁 Script executed:
#!/bin/bash
# Search for ThemeMode definition
rg -n "ThemeMode" apps/ui/src/store/app-store.ts | head -20Repository: AutoMaker-Org/automaker
Length of output: 793
🏁 Script executed:
#!/bin/bash
# Check the imports at the top of app-store.ts
head -20 apps/ui/src/store/app-store.tsRepository: AutoMaker-Org/automaker
Length of output: 503
🏁 Script executed:
#!/bin/bash
# Check what ThemeMode actually is - search in the electron types
find . -name "*.ts" -type f -exec grep -l "type ThemeMode\s*=" {} \;Repository: AutoMaker-Org/automaker
Length of output: 95
🏁 Script executed:
#!/bin/bash
# Check the ThemeMode definition
rg -n "type ThemeMode" libs/types/src/settings.ts -A 10Repository: AutoMaker-Org/automaker
Length of output: 267
🏁 Script executed:
#!/bin/bash
# Now let me check one more thing - see if ThemeMode includes all Theme values
sed -n '24,60p' libs/types/src/settings.tsRepository: AutoMaker-Org/automaker
Length of output: 577
🏁 Script executed:
#!/bin/bash
# Let me check the casting in settings-view.tsx more closely
sed -n '55,65p' "apps/ui/src/components/views/settings-view.tsx"Repository: AutoMaker-Org/automaker
Length of output: 305
🏁 Script executed:
#!/bin/bash
# Check if there are any direct string assignments to project.theme (not from ThemeMode)
rg -n "theme\s*:\s*['\"]" apps/ui/src --type=ts | grep -v "theme:" | head -20Repository: AutoMaker-Org/automaker
Length of output: 49
🏁 Script executed:
#!/bin/bash
# Let me check the exact type that should be used by looking at how project.theme is assigned
rg -n "project\.theme\s*=" apps/ui/src/store/app-store.ts -B 2 -A 2Repository: AutoMaker-Org/automaker
Length of output: 49
🏁 Script executed:
#!/bin/bash
# Check what values are actually assigned to project.theme in the store
rg -n "p\.id === projectId.*theme" apps/ui/src/store/app-store.ts -B 3 -A 3Repository: AutoMaker-Org/automaker
Length of output: 372
Use ThemeMode type for proper type safety.
Changing theme from string to a specific type is necessary for type safety. However, the correct type should be ThemeMode (not just Theme), since the store assigns ThemeMode | null to this field. ThemeMode includes all valid theme values plus the 'system' mode:
export interface Project {
id: string;
name: string;
path: string;
- theme?: string;
+ theme?: ThemeMode;
}Import ThemeMode from '@automaker/types' if not already available, or re-export it alongside Theme.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In apps/ui/src/components/views/settings-view/shared/types.ts around line 28,
the theme property is currently typed as string but should be ThemeMode to match
the store which assigns ThemeMode | null; update the type to theme?: ThemeMode
and add an import for ThemeMode from '@automaker/types' (or re-export it
alongside Theme) so the file uses the correct union of valid theme values
including 'system'.
| <button | ||
| onClick={() => setActiveTab('dark')} | ||
| className={cn( | ||
| 'flex items-center gap-2 px-6 py-2.5 rounded-lg font-medium transition-all duration-200', | ||
| activeTab === 'dark' | ||
| ? 'bg-brand-500 text-white shadow-lg shadow-brand-500/25' | ||
| : 'bg-accent/50 text-muted-foreground hover:bg-accent hover:text-foreground' | ||
| )} | ||
| > | ||
| <Moon className="w-4 h-4" /> | ||
| Dark Themes | ||
| </button> | ||
| <button | ||
| onClick={() => setActiveTab('light')} | ||
| className={cn( | ||
| 'flex items-center gap-2 px-6 py-2.5 rounded-lg font-medium transition-all duration-200', | ||
| activeTab === 'light' | ||
| ? 'bg-brand-500 text-white shadow-lg shadow-brand-500/25' | ||
| : 'bg-accent/50 text-muted-foreground hover:bg-accent hover:text-foreground' | ||
| )} | ||
| > | ||
| <Sun className="w-4 h-4" /> | ||
| Light Themes | ||
| </button> | ||
| </div> |
There was a problem hiding this comment.
Add type="button" to prevent unintended form submissions.
Native <button> elements default to type="submit". If this component is ever rendered inside a form, these tabs could inadvertently submit it.
🔎 Proposed fix
<button
+ type="button"
onClick={() => setActiveTab('dark')}
className={cn(
'flex items-center gap-2 px-6 py-2.5 rounded-lg font-medium transition-all duration-200',
activeTab === 'dark'
? 'bg-brand-500 text-white shadow-lg shadow-brand-500/25'
: 'bg-accent/50 text-muted-foreground hover:bg-accent hover:text-foreground'
)}
>
<Moon className="w-4 h-4" />
Dark Themes
</button>
<button
+ type="button"
onClick={() => setActiveTab('light')}
className={cn(📝 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.
| <button | |
| onClick={() => setActiveTab('dark')} | |
| className={cn( | |
| 'flex items-center gap-2 px-6 py-2.5 rounded-lg font-medium transition-all duration-200', | |
| activeTab === 'dark' | |
| ? 'bg-brand-500 text-white shadow-lg shadow-brand-500/25' | |
| : 'bg-accent/50 text-muted-foreground hover:bg-accent hover:text-foreground' | |
| )} | |
| > | |
| <Moon className="w-4 h-4" /> | |
| Dark Themes | |
| </button> | |
| <button | |
| onClick={() => setActiveTab('light')} | |
| className={cn( | |
| 'flex items-center gap-2 px-6 py-2.5 rounded-lg font-medium transition-all duration-200', | |
| activeTab === 'light' | |
| ? 'bg-brand-500 text-white shadow-lg shadow-brand-500/25' | |
| : 'bg-accent/50 text-muted-foreground hover:bg-accent hover:text-foreground' | |
| )} | |
| > | |
| <Sun className="w-4 h-4" /> | |
| Light Themes | |
| </button> | |
| </div> | |
| <button | |
| type="button" | |
| onClick={() => setActiveTab('dark')} | |
| className={cn( | |
| 'flex items-center gap-2 px-6 py-2.5 rounded-lg font-medium transition-all duration-200', | |
| activeTab === 'dark' | |
| ? 'bg-brand-500 text-white shadow-lg shadow-brand-500/25' | |
| : 'bg-accent/50 text-muted-foreground hover:bg-accent hover:text-foreground' | |
| )} | |
| > | |
| <Moon className="w-4 h-4" /> | |
| Dark Themes | |
| </button> | |
| <button | |
| type="button" | |
| onClick={() => setActiveTab('light')} | |
| className={cn( | |
| 'flex items-center gap-2 px-6 py-2.5 rounded-lg font-medium transition-all duration-200', | |
| activeTab === 'light' | |
| ? 'bg-brand-500 text-white shadow-lg shadow-brand-500/25' | |
| : 'bg-accent/50 text-muted-foreground hover:bg-accent hover:text-foreground' | |
| )} | |
| > | |
| <Sun className="w-4 h-4" /> | |
| Light Themes | |
| </button> | |
| </div> |
🤖 Prompt for AI Agents
In apps/ui/src/components/views/setup-view/steps/theme-step.tsx around lines 43
to 67, the two <button> elements lack an explicit type, so they default to
type="submit" and can unintentionally submit enclosing forms; update both
buttons to include type="button" to prevent accidental form submission while
preserving their onClick handlers and styles.
| :is( | ||
| .dark, | ||
| .retro, | ||
| .dracula, | ||
| .nord, | ||
| .monokai, | ||
| .tokyonight, | ||
| .solarized, | ||
| .gruvbox, | ||
| .catppuccin, | ||
| .onedark, | ||
| .synthwave, | ||
| .red, | ||
| .sunset, | ||
| .gray | ||
| ) | ||
| ::-webkit-scrollbar { | ||
| width: 8px; | ||
| height: 8px; | ||
| } | ||
|
|
||
| :is(.dark, .retro, .dracula, .nord, .monokai, .tokyonight, .solarized, .gruvbox, .catppuccin, .onedark, .synthwave, .red, .sunset, .gray) ::-webkit-scrollbar-track { | ||
| :is( | ||
| .dark, | ||
| .retro, | ||
| .dracula, | ||
| .nord, | ||
| .monokai, | ||
| .tokyonight, | ||
| .solarized, | ||
| .gruvbox, | ||
| .catppuccin, | ||
| .onedark, | ||
| .synthwave, | ||
| .red, | ||
| .sunset, | ||
| .gray | ||
| ) | ||
| ::-webkit-scrollbar-track { |
There was a problem hiding this comment.
Missing forest and ocean themes in scrollbar selectors.
The new dark themes forest and ocean (declared on lines 19-20) are not included in the scrollbar styling selectors. This may cause inconsistent scrollbar appearance for these themes.
🔎 Proposed fix
:is(
.dark,
.retro,
.dracula,
.nord,
.monokai,
.tokyonight,
.solarized,
.gruvbox,
.catppuccin,
.onedark,
.synthwave,
.red,
.sunset,
- .gray
+ .gray,
+ .forest,
+ .ocean
)
::-webkit-scrollbar {Apply the same fix to the ::-webkit-scrollbar-track selector on lines 414-430.
📝 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.
| :is( | |
| .dark, | |
| .retro, | |
| .dracula, | |
| .nord, | |
| .monokai, | |
| .tokyonight, | |
| .solarized, | |
| .gruvbox, | |
| .catppuccin, | |
| .onedark, | |
| .synthwave, | |
| .red, | |
| .sunset, | |
| .gray | |
| ) | |
| ::-webkit-scrollbar { | |
| width: 8px; | |
| height: 8px; | |
| } | |
| :is(.dark, .retro, .dracula, .nord, .monokai, .tokyonight, .solarized, .gruvbox, .catppuccin, .onedark, .synthwave, .red, .sunset, .gray) ::-webkit-scrollbar-track { | |
| :is( | |
| .dark, | |
| .retro, | |
| .dracula, | |
| .nord, | |
| .monokai, | |
| .tokyonight, | |
| .solarized, | |
| .gruvbox, | |
| .catppuccin, | |
| .onedark, | |
| .synthwave, | |
| .red, | |
| .sunset, | |
| .gray | |
| ) | |
| ::-webkit-scrollbar-track { | |
| :is( | |
| .dark, | |
| .retro, | |
| .dracula, | |
| .nord, | |
| .monokai, | |
| .tokyonight, | |
| .solarized, | |
| .gruvbox, | |
| .catppuccin, | |
| .onedark, | |
| .synthwave, | |
| .red, | |
| .sunset, | |
| .gray, | |
| .forest, | |
| .ocean | |
| ) | |
| ::-webkit-scrollbar { | |
| width: 8px; | |
| height: 8px; | |
| } | |
| :is( | |
| .dark, | |
| .retro, | |
| .dracula, | |
| .nord, | |
| .monokai, | |
| .tokyonight, | |
| .solarized, | |
| .gruvbox, | |
| .catppuccin, | |
| .onedark, | |
| .synthwave, | |
| .red, | |
| .sunset, | |
| .gray, | |
| .forest, | |
| .ocean | |
| ) | |
| ::-webkit-scrollbar-track { |
🤖 Prompt for AI Agents
In apps/ui/src/styles/global.css around lines 393 to 430 the ::-webkit-scrollbar
and ::-webkit-scrollbar-track selectors list theme classes but omit the new dark
themes forest and ocean; update both selector lists to include .forest and
.ocean so those themes receive the same scrollbar and scrollbar-track styling as
the other dark themes.
This commit modifies the unit tests in dev-server-service.test.ts to replace the usage of the native fs.existsSync method with secureFs.access for improved security and consistency in file handling. Key changes include: - Updated all instances of existsSync to utilize secureFs.access, ensuring that file existence checks are performed using the secure file system operations. - Adjusted mock implementations to reflect the new secureFs methods, enhancing the reliability of the tests. These changes aim to align the testing strategy with the recent refactor for enhanced security in file operations.
Code Review - PR #206: Security-focused fs to secureFs Refactor📊 Executive SummaryThis PR implements a significant security improvement by migrating from native Recommendation: 🔴 Request Changes - Incomplete migration and missing security tests are blocking issues. ✅ Strengths1. Excellent Security Architecture
2. Consistent Refactoring Pattern
3. Theme System Enhancements
🚨 Critical Issues (Must Fix Before Merge)1. Incomplete Migration - Security Bypass RiskHIGH SEVERITY: Critical files still using native Files requiring immediate migration:
Impact: Terminal service and setup routes (API key management!) can access files outside Recommendation: # Find all remaining fs imports
grep -r "from ['\"]fs['\"]" apps/server/src/
grep -r "from ['\"]fs/promises['\"]" apps/server/src/Complete migration for all findings. 2. Zero Test Coverage for Critical Security FeatureCRITICAL: Missing tests: describe('validateWorkingDirectory', () => {
it('should throw PathNotAllowedError when path outside ALLOWED_ROOT_DIRECTORY');
it('should allow path within ALLOWED_ROOT_DIRECTORY');
it('should resolve relative paths before validation');
it('should allow DATA_DIR paths as exception');
it('should throw with descriptive error message');
});Also missing:
Recommendation: Add comprehensive test suite before merge. This is a security-critical feature. 3. PathNotAllowedError Constructor MismatchBug in // libs/platform/src/security.ts - Constructor signature
export class PathNotAllowedError extends Error {
constructor(filePath: string) { // Takes single param
super(`Path not allowed: ${filePath}. Must be within...`);
// apps/server/src/lib/sdk-options.ts - Usage
throw new PathNotAllowedError( // ❌ Tries to pass custom message
`Working directory "${cwd}" (resolved: ${resolvedCwd}) is not allowed...`
);Fix options:
4. Security Errors Swallowed in Error HandlersIn } catch (error) {
logger.error(`Failed to generate synthetic diff for ${fullPath}:`, error);
return `[Unable to read file content]`; // ❌ Hides PathNotAllowedError
}Issue: Users see "Unable to read file content" instead of explicit security violation message. Fix: } catch (error) {
if (error instanceof PathNotAllowedError) {
throw error; // Don't swallow security errors
}
logger.error(`Failed to generate synthetic diff for ${fullPath}:`, error);
return `[Unable to read file content]`;
}
|
This commit updates various modules to utilize the secure file system operations from the secureFs module instead of the native fs module. Key changes include:
These changes aim to improve the security and maintainability of file handling across the application.
Summary by CodeRabbit
New Features
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.