Conversation
|
Caution Review failedThe pull request is closed. WalkthroughReplace onReload with onTabCreated across PlanView components to enable optimistic tab creation and immediate tab selection; TaskCard now builds and shell-escapes a dynamic Claude prompt and uses result.tab to derive newTabId; terminal tabs are spawned immediately via dynamic tmux-manager import; Terminal no longer receives an initial command prop; status label "planning" → "In Progress". Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant TaskCard
participant Escaper as Sanitizer
participant IPC
participant Backend as WorkspaceAPI
participant UI as Parent (NewLayoutMain)
User->>TaskCard: Start task (title + description)
TaskCard->>Escaper: build & escape prompt
Escaper-->>TaskCard: escapedPrompt
TaskCard->>IPC: create tab ("claude [escapedPrompt]")
IPC->>Backend: create tab -> returns result (success, tab)
alt result.success && result.tab
TaskCard->>Backend: mark task as planning
TaskCard->>UI: onTabCreated(worktreeId, tab) %% optimistic update
UI-->>TaskCard: selectTab(newTabId) %% immediate selection
end
sequenceDiagram
participant UI as Main Process (tab-operations)
participant Tmux as tmux-manager (dynamic)
participant TerminalProc as Terminal Process
UI->>Tmux: dynamic import tmux-manager
UI->>Tmux: create(tabId, cwd, command, cols=80, rows=30)
Tmux->>TerminalProc: spawn terminal attached to tabId
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (5)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apps/desktop/src/renderer/screens/main/components/PlanView/TaskCard.tsx (1)
111-113: Consider more robust synchronization instead of setTimeout.The 100ms delay is a workaround for ensuring workspace data is reloaded before selecting the tab. This could fail if the reload takes longer or succeed with unnecessary delay.
Consider options:
- Have
onReload()return a promise you can await before callingonTabSelect- Make
onTabSelecthandle cases where the tab isn't immediately available- If the current approach is reliable in practice, add a comment explaining the timing dependency
// Reload workspace to get updated tab data await onReload(); // Select the new tab after reload const newTabId = result.tab?.id; if (newTabId) { - // Small delay to ensure workspace is reloaded + // Wait for workspace reload to propagate to UI state setTimeout(() => { onTabSelect(targetWorktreeId, newTabId); }, 100); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/desktop/src/renderer/screens/main/components/PlanView/TaskCard.tsx(1 hunks)apps/desktop/src/renderer/screens/main/components/PlanView/TaskDetailModal.tsx(1 hunks)
⏰ 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). (1)
- GitHub Check: Unit Test
🔇 Additional comments (1)
apps/desktop/src/renderer/screens/main/components/PlanView/TaskDetailModal.tsx (1)
39-39: LGTM! Better semantic accuracy.The label change from "Pending" to "In Progress" more accurately reflects that planning is an active state. This aligns well with the workflow where starting a task (in TaskCard.tsx) sets the status to "planning".
| const taskPrompt = `${task.title}\n\n${task.description || ""}`.trim(); | ||
| // Escape quotes and newlines for shell command | ||
| const escapedPrompt = taskPrompt | ||
| .replace(/\\/g, '\\\\') // Escape backslashes first | ||
| .replace(/"/g, '\\"') // Escape double quotes | ||
| .replace(/\n/g, '\\n'); // Escape newlines | ||
| const result = await window.ipcRenderer.invoke("tab-create", { | ||
| workspaceId: currentWorkspace.id, | ||
| worktreeId: targetWorktreeId, | ||
| name: `Task: ${task.slug}`, | ||
| type: "terminal", | ||
| command: `claude "hi"`, | ||
| command: `claude "${escapedPrompt}"`, |
There was a problem hiding this comment.
Critical: Incomplete shell escaping enables command injection.
The current escaping logic only handles backslashes, double quotes, and newlines. It's missing critical shell metacharacters that are interpreted within double quotes:
$→ variable expansion:${task.title}containing$HOMEwould expand`→ command substitution: backticks allow arbitrary command execution!→ history expansion (in some shells with histexpand enabled)
If a task title or description contains these characters, they will be executed in the shell context, allowing arbitrary code execution.
Recommended fix: Use a safer approach by passing the prompt as a separate argument or using proper shell escaping:
- try {
- // Create a new terminal with claude command
- const taskPrompt = `${task.title}\n\n${task.description || ""}`.trim();
- // Escape quotes and newlines for shell command
- const escapedPrompt = taskPrompt
- .replace(/\\/g, '\\\\') // Escape backslashes first
- .replace(/"/g, '\\"') // Escape double quotes
- .replace(/\n/g, '\\n'); // Escape newlines
+ try {
+ // Create a new terminal with claude command
+ const taskPrompt = `${task.title}\n\n${task.description || ""}`.trim();
+ // Escape for shell: backslashes, quotes, dollar signs, backticks, and newlines
+ const escapedPrompt = taskPrompt
+ .replace(/\\/g, '\\\\') // Escape backslashes first
+ .replace(/"/g, '\\"') // Escape double quotes
+ .replace(/\$/g, '\\$') // Escape dollar signs
+ .replace(/`/g, '\\`') // Escape backticks
+ .replace(/!/g, '\\!') // Escape exclamation marks
+ .replace(/\n/g, '\\n'); // Escape newlines
const result = await window.ipcRenderer.invoke("tab-create", {Better alternative: If the IPC handler supports it, consider passing the prompt as a separate parameter rather than embedding it in the command string, avoiding shell escaping entirely.
📝 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.
| const taskPrompt = `${task.title}\n\n${task.description || ""}`.trim(); | |
| // Escape quotes and newlines for shell command | |
| const escapedPrompt = taskPrompt | |
| .replace(/\\/g, '\\\\') // Escape backslashes first | |
| .replace(/"/g, '\\"') // Escape double quotes | |
| .replace(/\n/g, '\\n'); // Escape newlines | |
| const result = await window.ipcRenderer.invoke("tab-create", { | |
| workspaceId: currentWorkspace.id, | |
| worktreeId: targetWorktreeId, | |
| name: `Task: ${task.slug}`, | |
| type: "terminal", | |
| command: `claude "hi"`, | |
| command: `claude "${escapedPrompt}"`, | |
| const taskPrompt = `${task.title}\n\n${task.description || ""}`.trim(); | |
| // Escape for shell: backslashes, quotes, dollar signs, backticks, and newlines | |
| const escapedPrompt = taskPrompt | |
| .replace(/\\/g, '\\\\') // Escape backslashes first | |
| .replace(/"/g, '\\"') // Escape double quotes | |
| .replace(/\$/g, '\\$') // Escape dollar signs | |
| .replace(/`/g, '\\`') // Escape backticks | |
| .replace(/!/g, '\\!') // Escape exclamation marks | |
| .replace(/\n/g, '\\n'); // Escape newlines | |
| const result = await window.ipcRenderer.invoke("tab-create", { | |
| workspaceId: currentWorkspace.id, | |
| worktreeId: targetWorktreeId, | |
| name: `Task: ${task.slug}`, | |
| type: "terminal", | |
| command: `claude "${escapedPrompt}"`, |
🤖 Prompt for AI Agents
In apps/desktop/src/renderer/screens/main/components/PlanView/TaskCard.tsx
around lines 82 to 93, the current escaping only handles backslashes, double
quotes and newlines leaving shell metacharacters like $, `, and ! unescaped
which allows command injection; fix by not embedding the prompt in a single
shell command string—change the ipcRenderer.invoke payload to send the prompt as
a separate field (e.g. prompt: taskPrompt) or send a command + args array so the
main process can spawn/execFile safely with the prompt as an argument, or if you
cannot change the IPC contract, implement robust shell-escaping on the renderer
side (escape $, `, ! and properly handle single quotes using the standard
close-quote-escape-quote pattern) and update the main process to use
spawn/execFile with args rather than passing a full shell command to a shell.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
apps/desktop/src/renderer/screens/main/components/PlanView/TaskCard.tsx (1)
82-93: Critical: Command injection vulnerability remains unfixed.The incomplete shell escaping identified in the previous review has not been addressed. The current implementation still only escapes backslashes, double quotes, and newlines, leaving critical shell metacharacters unescaped:
$→ variable expansion and command substitution`→ command substitution!→ history expansionA task title containing these characters will be executed in the shell context, allowing arbitrary code execution.
Required fix: Apply the escaping suggested in the previous review or, preferably, refactor to pass the prompt as a separate IPC parameter rather than embedding it in a shell command string. The latter approach eliminates the need for shell escaping entirely and is more secure.
const escapedPrompt = taskPrompt .replace(/\\/g, '\\\\') // Escape backslashes first .replace(/"/g, '\\"') // Escape double quotes + .replace(/\$/g, '\\$') // Escape dollar signs + .replace(/`/g, '\\`') // Escape backticks + .replace(/!/g, '\\!') // Escape exclamation marks .replace(/\n/g, '\\n'); // Escape newlines
🧹 Nitpick comments (1)
apps/desktop/src/renderer/screens/main/components/NewLayout/NewLayoutMain.tsx (1)
377-389: LGTM - Dual reload ensures workspace consistency.The
reloadWorkspaceDatafunction correctly refreshes both the workspace list and the current workspace, ensuring that changes (like new tabs created in TaskCard.tsx) are immediately reflected in the UI state.Consider adding a brief comment explaining why both loads are necessary, for future maintainability:
// Reload both workspaces and current workspace const reloadWorkspaceData = async () => { + // Refresh the workspace list first await loadAllWorkspaces(); + // Then refresh the current workspace to ensure tab/worktree changes are reflected if (currentWorkspace) { const refreshedWorkspace = await window.ipcRenderer.invoke( "workspace-get", currentWorkspace.id, ); if (refreshedWorkspace) { setCurrentWorkspace(refreshedWorkspace); } } };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
apps/desktop/src/main/lib/workspace/tab-operations.ts(1 hunks)apps/desktop/src/renderer/screens/main/components/MainContent/TabContent.tsx(0 hunks)apps/desktop/src/renderer/screens/main/components/MainContent/Terminal.tsx(0 hunks)apps/desktop/src/renderer/screens/main/components/NewLayout/NewLayoutMain.tsx(2 hunks)apps/desktop/src/renderer/screens/main/components/PlanView/TaskCard.tsx(2 hunks)
💤 Files with no reviewable changes (2)
- apps/desktop/src/renderer/screens/main/components/MainContent/Terminal.tsx
- apps/desktop/src/renderer/screens/main/components/MainContent/TabContent.tsx
🔇 Additional comments (2)
apps/desktop/src/renderer/screens/main/components/NewLayout/NewLayoutMain.tsx (1)
966-966: Correct integration with PlanView.The change from
loadAllWorkspacestoreloadWorkspaceDataensures that PlanView operations (especially tab creation in TaskCard) properly update both the workspace list and the current workspace state.apps/desktop/src/renderer/screens/main/components/PlanView/TaskCard.tsx (1)
96-110: LGTM - Immediate tab selection is safe and properly handled.The logic correctly validates, captures the tab ID, updates status, and reloads workspace before selecting the tab. Terminal component has robust initialization handling: multiple guards (
terminalIdRef,isDisposed,isInitialSetup), retry logic for container dimensions, and write queuing during resize. The tmux process creation intab-operations.tsis awaited before tab selection occurs, eliminating race conditions. Terminal gracefully handles async initialization even if displayed early.
| // Create terminal process immediately for terminal tabs with commands | ||
| if (tab.type === "terminal" && tab.command) { | ||
| const tmuxManager = await import("../tmux-manager").then( | ||
| (m) => m.default, | ||
| ); | ||
| const workingDir = worktree.path || workspace.repoPath; | ||
|
|
||
| // Create terminal with command - it will be executed automatically | ||
| await tmuxManager.create({ | ||
| id: tab.id, | ||
| cwd: workingDir, | ||
| command: tab.command, | ||
| // Use default dimensions, Terminal component will resize when it loads | ||
| cols: 80, | ||
| rows: 30, | ||
| }); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Verify command sanitization in tmux-manager to prevent injection.
The tab.command string is passed directly to tmuxManager.create() without validation or sanitization in this layer. Based on the related changes in TaskCard.tsx (which constructs commands using incomplete shell escaping), there is a potential command injection vulnerability if:
- The command originates from user-controlled data (task titles/descriptions)
- tmux-manager executes the command in a shell context without proper escaping
Run the following script to examine how tmux-manager handles the command parameter:
If tmux-manager passes the command to a shell without additional escaping, you must either:
- Sanitize/validate
tab.commandhere before passing it - Pass the command as a structured array of arguments rather than a shell string
- Ensure the command is properly escaped at the source (TaskCard.tsx - see related comment)
🏁 Script executed:
#!/bin/bash
# Description: Verify tmux-manager's command handling for injection risks
# Check tmux-manager implementation
ast-grep --pattern $'create($$$) {
$$$
}'
# Also search for how command is used
rg -nP --type=ts -C5 'command.*exec|spawn.*command' apps/desktop/src/main/lib/tmux-managerLength of output: 78
🏁 Script executed:
#!/bin/bash
# Find tmux-manager files
fd -e ts -e js "tmux-manager" apps/desktop/src/main/lib/
# Also check parent directory structure
find apps/desktop/src/main/lib -maxdepth 2 -type f -name "*tmux*" 2>/dev/null
# Search for create function calls and command usage
rg -nP --type=ts "create\s*\(" apps/desktop/src/main/lib/ -A3 | head -50Length of output: 2356
🏁 Script executed:
#!/bin/bash
# View tmux-manager.ts create() method implementation
sed -n '197,250p' apps/desktop/src/main/lib/tmux-manager.ts
# Also search for command execution patterns (spawn, exec, shell)
rg -nP --type=ts "spawn|exec|shell" apps/desktop/src/main/lib/tmux-manager.ts -B2 -A2Length of output: 3782
🏁 Script executed:
#!/bin/bash
# View executeCommand method implementation
sed -n '476,490p' apps/desktop/src/main/lib/tmux-manager.ts
# Also look for sendKeys or write operations
rg -nP --type=ts "sendKeys|write.*command|pty\.write" apps/desktop/src/main/lib/tmux-manager.ts -B2 -A2Length of output: 670
🏁 Script executed:
#!/bin/bash
# View the write() method implementation
rg -nP --type=ts "^\s*write\s*\(" apps/desktop/src/main/lib/tmux-manager.ts -A15Length of output: 510
🏁 Script executed:
#!/bin/bash
# View tab-operations.ts to see where tab.command comes from
sed -n '1,50p' apps/desktop/src/main/lib/workspace/tab-operations.ts
# Search for tab.command assignment
rg -nP --type=ts "tab\.command\s*=" apps/desktop/src/main/lib/workspace/tab-operations.ts -B3 -A1
# Check input source
sed -n '40,50p' apps/desktop/src/main/lib/workspace/tab-operations.tsLength of output: 1963
🏁 Script executed:
#!/bin/bash
# Find TaskCard files
fd -e tsx -e ts "TaskCard" apps/desktop/src
# Search for command construction in TaskCard
rg -nP "command.*=" apps/desktop/src --type=tsx -B2 -A2 | grep -i "task\|card" -A5 -B5 | head -80Length of output: 99
🏁 Script executed:
#!/bin/bash
# View TaskCard.tsx - search for command construction
rg -n "command" apps/desktop/src/renderer/screens/main/components/PlanView/TaskCard.tsx -B3 -A3
# Also check for shell escaping patterns
rg -n "escape|quote|shell" apps/desktop/src/renderer/screens/main/components/PlanView/TaskCard.tsx -B2 -A2Length of output: 1143
Verify command sanitization in tmux-manager to prevent injection — VULNERABILITY CONFIRMED.
The tab.command string is passed directly to tmuxManager.create() without validation or sanitization. The underlying vulnerability stems from incomplete shell escaping in TaskCard.tsx (lines 84-87), which only escapes backslashes, double quotes, and newlines but omits critical shell metacharacters like $, backticks (`), !, and command substitution syntax.
Execution path:
- TaskCard.tsx constructs command with incomplete escaping
- Command flows through input.command → tab.command → tmuxManager.create() → PTY.write()
- tmux-manager writes directly to bash PTY (line 462) without additional escaping
Risks:
- User input containing
$(command)or`command`bypasses current escaping and executes arbitrary code - No secondary validation layer exists between command construction and shell execution
Required fix: Either properly escape all shell metacharacters using a robust method (e.g., shell escaping library or printf %q equivalent) or refactor to pass commands as argument arrays rather than shell strings.
🤖 Prompt for AI Agents
In apps/desktop/src/main/lib/workspace/tab-operations.ts around lines 103 to
119, tab.command is forwarded directly into tmuxManager.create() which passes it
to a shell/PTY without sufficient escaping, allowing shell metacharacter
injection; fix by validating and sanitizing the command before passing it:
either change the call site and tmux-manager API to accept command as an
argument array (preferred) and pass args directly to the PTY/shell without
joining into a shell string, or, if a string must be used, run the command
through a proven shell-escaping library (or implement printf %q-style quoting)
that escapes all metacharacters ($, `, !, ;, |, &, >, <, (), {}, *, ?, [, ],
etc.), and add a defensive runtime check that rejects or escapes any dangerous
constructs before invoking tmuxManager.create().
1 participant
Description
Related Issues
Type of Change
Testing
Screenshots (if applicable)
Additional Notes
Summary by CodeRabbit
Bug Fixes
New Features
Changes
Chores