Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion apps/blog/tsconfig.tsbuildinfo

Large diffs are not rendered by default.

19 changes: 13 additions & 6 deletions apps/desktop/src/main/lib/window-manager.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,9 +7,10 @@ class WindowManager {
private windowWorkspaces: Map<BrowserWindow, string | null> = new Map();
private restoredWindowIds: Set<number> = new Set();

async createWindow(
restoreState?: { workspaceId: string | null; bounds?: Electron.Rectangle },
): Promise<BrowserWindow> {
async createWindow(restoreState?: {
workspaceId: string | null;
bounds?: Electron.Rectangle;
}): Promise<BrowserWindow> {
const window = await MainWindow();

// Restore window bounds if provided
Expand Down Expand Up @@ -55,7 +56,7 @@ class WindowManager {
// Save final state before closing (window is still valid here)
// Get workspace ID from our map before window might be destroyed
const workspaceId = this.windowWorkspaces.get(window) ?? null;

try {
if (!window.isDestroyed()) {
const bounds = window.getBounds();
Expand All @@ -77,7 +78,10 @@ class WindowManager {
} catch (error) {
// Silently fail if window is destroyed - we'll clean up in closed handler
if (!(error instanceof Error && error.message.includes("destroyed"))) {
console.error("[WindowManager] Failed to save window state on close:", error);
console.error(
"[WindowManager] Failed to save window state on close:",
error,
);
}
}
});
Expand Down Expand Up @@ -109,7 +113,10 @@ class WindowManager {
return this.windowWorkspaces.get(window) ?? null;
}

setWorkspaceForWindow(window: BrowserWindow, workspaceId: string | null): void {
setWorkspaceForWindow(
window: BrowserWindow,
workspaceId: string | null,
): void {
this.windowWorkspaces.set(window, workspaceId);
// Persist the workspace association
windowStateManager.saveWindowState(window, workspaceId);
Expand Down
6 changes: 4 additions & 2 deletions apps/desktop/src/main/lib/window-state-manager.ts
Original file line number Diff line number Diff line change
Expand Up @@ -132,7 +132,10 @@ class WindowStateManager {

this.write(state);
} catch (error) {
console.error("[WindowStateManager] Failed to save window state by ID:", error);
console.error(
"[WindowStateManager] Failed to save window state by ID:",
error,
);
}
}

Expand All @@ -157,4 +160,3 @@ class WindowStateManager {
}

export default WindowStateManager.getInstance();

103 changes: 101 additions & 2 deletions apps/desktop/src/main/lib/workspace-ipcs.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,9 +10,9 @@ import type {
} from "shared/types";

import configManager from "./config-manager";
import windowManager from "./window-manager";
import workspaceManager from "./workspace-manager";
import worktreeManager from "./worktree-manager";
import windowManager from "./window-manager";

export function registerWorkspaceIPCs() {
// Open repository dialog
Expand Down Expand Up @@ -81,6 +81,103 @@ export function registerWorkspaceIPCs() {
mainWindow.webContents.send("workspace-opened", createResult.workspace);
});

// Select directory for cloning
ipcMain.handle("workspace-select-directory", async (event) => {
const mainWindow = BrowserWindow.fromWebContents(event.sender);
if (!mainWindow) {
return { canceled: true };
}

const result = await dialog.showOpenDialog(mainWindow, {
properties: ["openDirectory", "createDirectory"],
title: "Select Destination Folder",
});

if (result.canceled || result.filePaths.length === 0) {
return { canceled: true };
}

return {
canceled: false,
filePath: result.filePaths[0],
};
});

// Clone repository from URL
ipcMain.handle(
"workspace-clone-from-url",
async (_event, input: { url: string; destinationPath: string }) => {
try {
// Clone the repository
const cloneResult = await worktreeManager.cloneRepository(
input.url,
input.destinationPath,
);

if (!cloneResult.success || !cloneResult.path) {
return {
success: false,
error: cloneResult.error || "Failed to clone repository",
};
}

const repoPath = cloneResult.path;

// Get current branch
if (!worktreeManager.isGitRepo(repoPath)) {
return {
success: false,
error: "Cloned directory is not a valid git repository.",
};
}

const currentBranch = worktreeManager.getCurrentBranch(repoPath);
if (!currentBranch) {
return {
success: false,
error: "Could not determine current branch.",
};
}

// Check if workspace already exists for this repo
const existingWorkspaces = await workspaceManager.list();
const existingWorkspace = existingWorkspaces.find(
(ws) => ws.repoPath === repoPath,
);

if (existingWorkspace) {
// Workspace already exists, return it
return {
success: true,
data: existingWorkspace,
};
}

// Create workspace with repo name and current branch
const repoName = repoPath.split("/").pop() || "Repository";

const createResult = await workspaceManager.create({
name: repoName,
repoPath,
branch: currentBranch,
});

// Convert to IpcResponse format
return {
success: createResult.success,
data: createResult.workspace,
error: createResult.error,
};
} catch (error) {
console.error("Failed to clone repository:", error);
return {
success: false,
error: error instanceof Error ? error.message : String(error),
};
}
},
);

// List all workspaces
ipcMain.handle("workspace-list", async () => {
return await workspaceManager.list();
Expand Down Expand Up @@ -627,7 +724,9 @@ export function registerWorkspaceIPCs() {

// Detect main branch instead of using workspace.branch
// This ensures we compare against main/master, not a feature branch
const mainBranch = await worktreeManager.detectMainBranch(workspace.repoPath);
const mainBranch = await worktreeManager.detectMainBranch(
workspace.repoPath,
);

return await worktreeManager.getGitDiffFile(
worktree.path,
Expand Down
60 changes: 59 additions & 1 deletion apps/desktop/src/main/lib/worktree-manager.ts
Original file line number Diff line number Diff line change
Expand Up @@ -854,7 +854,10 @@ class WorktreeManager {
const oldPath = parts[2]; // For renamed files

// Check if file should be excluded
if (this.shouldExcludeFile(filePath) || (oldPath && this.shouldExcludeFile(oldPath))) {
if (
this.shouldExcludeFile(filePath) ||
(oldPath && this.shouldExcludeFile(oldPath))
) {
continue;
}

Expand Down Expand Up @@ -1328,6 +1331,61 @@ class WorktreeManager {
};
}
}

/**
* Clone a git repository from a URL
*/
async cloneRepository(
url: string,
destinationPath: string,
): Promise<{ success: boolean; path?: string; error?: string }> {
try {
// Validate URL format (basic check)
if (
!url.includes("github.com") &&
!url.includes("gitlab.com") &&
!url.includes("bitbucket.org")
) {
// Accept common git hosting services or any URL with .git
if (!url.endsWith(".git") && !url.startsWith("git@")) {
return {
success: false,
error:
"Invalid repository URL. Please provide a valid git repository URL.",
};
}
}

// Clone the repository
await execAsync(`git clone "${url}" "${destinationPath}"`, {
encoding: "utf-8",
});

return {
success: true,
path: destinationPath,
};
} catch (error) {
console.error("Failed to clone repository:", error);

// Extract a cleaner error message from git output
let errorMessage = error instanceof Error ? error.message : String(error);

// Try to extract the fatal/error line from stderr for a cleaner message
if (typeof error === "object" && error !== null && "stderr" in error) {
const stderr = String((error as any).stderr);
const fatalMatch = stderr.match(/fatal: (.+)/);
if (fatalMatch) {
errorMessage = fatalMatch[1];
}
}

return {
success: false,
error: errorMessage,
};
}
}
Comment on lines +1335 to +1388
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

Potential shell injection vulnerability in git clone command.

The URL and destination path are passed directly to a shell command using string interpolation with double quotes. If either parameter contains special characters or malicious input (e.g., "; rm -rf /"), this could lead to command injection.

Consider using Node's execFile or spawn with argument arrays instead of shell command strings, or at minimum, validate/sanitize inputs more rigorously.

Apply this approach for safer execution:

-			await execAsync(`git clone "${url}" "${destinationPath}"`, {
-				encoding: "utf-8",
-			});
+			const { spawn } = require("node:child_process");
+			await new Promise((resolve, reject) => {
+				const proc = spawn("git", ["clone", url, destinationPath]);
+				proc.on("close", (code) => {
+					if (code === 0) resolve(undefined);
+					else reject(new Error(`git clone failed with code ${code}`));
+				});
+			});

Alternatively, verify that the URL and destinationPath are properly validated/sanitized before use.


I'll verify the shell injection concerns by examining the actual code implementation and how execAsync handles arguments.


---

🏁 Script executed:

```shell
#!/bin/bash
# Find and examine the cloneRepository method in worktree-manager.ts
fd worktree-manager.ts --type f -x cat {} | head -1500 | tail -200

Length of output: 5074


🏁 Script executed:

#!/bin/bash
# Search for execAsync definition and usage patterns
rg -n "execAsync" -B 3 -A 8 --type ts

Length of output: 19534


🏁 Script executed:

#!/bin/bash
# Check how execAsync is imported and what library provides it
rg -n "import.*execAsync|from.*execAsync" --type ts -C 3

Length of output: 0


🏁 Script executed:

#!/bin/bash
# Look for other instances where shell commands are executed with user input
rg -n "spawn|exec|execFile" -B 2 -A 5 --type ts | head -100

Length of output: 6081


🏁 Script executed:

#!/bin/bash
# Check if there are any shell injection prevention utilities in the codebase
rg -n "escape|sanitize|quote" --type ts | head -50

Length of output: 1470


🏁 Script executed:

#!/bin/bash
# Examine the full cloneRepository method implementation
fd worktree-manager.ts --type f -x grep -n "cloneRepository" -A 60 {}

Length of output: 1676



---

🌐 Web query:

```web
Node.js child_process exec shell injection vulnerability double quotes

💡 Result:

Short answer: wrapping untrusted input in double quotes when using child_process.exec is not a safe protection against shell injection. exec spawns a shell and shell metacharacters (e.g. ; & | && || $() ... redirections, etc.) can still be interpreted or can break quoting; there have also been real-world failures where libraries meant to quote/escape input allowed metacharacters through. [1][2][3]

What to do instead (concise):

  • Avoid exec. Use execFile or spawn and pass arguments as an array (no shell interpretation). Example: child_process.spawn('ping', ['-c','1', destination]) or execFile('prog', [arg1, arg2]). This is the recommended safe pattern. [1][2]
  • If you must run a shell, either whitelist/validate allowed values strictly, or use a proven shell-escaping library and keep it up-to-date (beware past vulnerabilities such as shell-quote CVE-2021-42740). Even then prefer avoiding a shell. [2][3]
  • On Windows be careful: .bat/.cmd semantics require a shell; still validate/whitelist inputs there. [1]

Sources:

  • Node.js child_process docs (exec vs execFile/spawn, warning about unsanitized input). [1]
  • Practical guidance / OWASP-style writeups on Node command-injection prevention (advocates spawn/execFile and warns that quoting isn't sufficient). [2]
  • CVE-2021-42740 (shell-quote command-injection example showing how quoting/escaping tooling can fail). [3]

🌐 Web query:

Node.js exec vs spawn security difference

💡 Result:

Short answer: exec is riskier. exec runs a shell (via /bin/sh -c) and interprets metacharacters, so passing untrusted strings to it allows command injection. spawn and execFile invoke a program directly with an argument array (no shell) by default, which prevents shell metacharacter injection. However spawn/execFile can still be unsafe if you enable shell:true, pass unsanitized args, leak env vars, or run with excessive privileges — so always validate/whitelist input, prefer argument arrays, drop privileges, and avoid shells when possible. [1][2][3][4][5]

Key points and recommendations

  • Why exec is dangerous: it spawns a shell and concatenates the command string, so things like ;, &&, |, $(), backticks let an attacker run extra commands. Use only with fully trusted/static commands. [1][2][5]
  • Why spawn/execFile are safer: they call execve on the target program and accept an args array, so user input is passed as a single argument (no shell parsing). Prefer these when dealing with untrusted input. [1][3][4]
  • Caveats:
    • spawn(..., { shell: true }) or execFile with shell enabled reintroduces shell parsing and the risks of exec. [1][3]
    • exec buffers entire output (maxBuffer), which can cause other issues; spawn streams stdout/stderr. [4]
    • Environment variables and working directory can leak secrets—set a safe env and cwd. [3]
    • Always validate/whitelist inputs, escape or avoid calling the shell, and run with least privilege. [2][3][5]

Sources: [1][2][3][4][5]

References
[1] SecureFlag / OS Command Injection in NodeJS.
[2] Built In — Node.js app security (child_process guidance).
[3] NodeJS security blog — avoid shell, use execFile/spawn and sanitize.
[4] Stack Overflow — technical difference: exec uses shell vs spawn execve.
[5] Datadog static-analysis guidance on child_process.exec risks.


🌐 Web query:

Can command substitution happen inside double quotes shell

💡 Result:

Yes. Command substitution ( $(...) or ... ) is recognized and performed when it appears inside double quotes. The substitution's output then replaces the $(...) text; because it was inside double quotes, the shell does NOT perform word-splitting or pathname (globbing) on that output. Also, a command substitution begins its own quoting context, so quotes inside the substitution don't pair with quotes outside. Examples and standards: Bash manual and the POSIX shell spec. [1][2][3]

Example:

  • echo "$(printf 'a b')"
    prints: a b
  • but the result won't be split into separate words by the shell because it was double-quoted. [1][2]

Sources:
[1] Bash Reference Manual — Command Substitution.
[2] POSIX Shell Command Language — Command Substitution.
[3] Discussion / examples (Unix & Linux / StackOverflow).


Shell injection vulnerability confirmed in cloneRepository method.

The use of exec() with double-quoted parameters allows command substitution and shell metacharacters to be interpreted, bypassing the URL validation. A malicious URL like "https://github.com/user/repo.git$(malicious-command)" would execute arbitrary code.

The suggested fix using spawn() with an argument array is the correct approach. spawn and execFile invoke a program directly with an argument array (no shell) by default, which prevents shell metacharacter injection.

Apply the safer pattern shown in the original review comment or use similar approach for destinationPath validation.

🤖 Prompt for AI Agents
apps/desktop/src/main/lib/worktree-manager.ts around lines 1335-1388: the
current implementation uses execAsync with a single shell command string which
allows shell metacharacter/command-substitution injection via a crafted URL or
path; replace this with a non-shell child process invocation
(child_process.spawn or execFile) that passes the git command and its arguments
as an array (e.g., ['clone', url, destinationPath]) so the shell is not used,
validate/sanitize destinationPath (no shell metacharacters, path traversal, and
ensure it is a valid filesystem path), run the process and await its exit via a
Promise that resolves on exit code 0 and rejects on non-zero code while
collecting stderr/stdout for diagnostics, and propagate a safe, extracted error
message on failure.

}

export default WorktreeManager.getInstance();
6 changes: 4 additions & 2 deletions apps/desktop/src/renderer/components/ui/dialog.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -48,17 +48,19 @@ function DialogContent({
className,
children,
showCloseButton = true,
overlayClassName,
...props
}: React.ComponentProps<typeof DialogPrimitive.Content> & {
showCloseButton?: boolean;
overlayClassName?: string;
}) {
return (
<DialogPortal data-slot="dialog-portal">
<DialogOverlay />
<DialogOverlay className={overlayClassName} />
<DialogPrimitive.Content
data-slot="dialog-content"
className={cn(
"bg-background data-[state=open]:animate-in data-[state=closed]:animate-out data-[state=closed]:fade-out-0 data-[state=open]:fade-in-0 data-[state=closed]:zoom-out-95 data-[state=open]:zoom-in-95 fixed top-[50%] left-[50%] z-50 grid w-full max-w-[calc(100%-2rem)] translate-x-[-50%] translate-y-[-50%] gap-4 rounded-lg border p-6 shadow-lg duration-200 sm:max-w-lg",
"bg-background data-[state=open]:animate-in data-[state=closed]:animate-out data-[state=closed]:fade-out-0 data-[state=open]:fade-in-0 data-[state=closed]:zoom-out-95 data-[state=open]:zoom-in-95 fixed top-[50%] left-[50%] z-50 grid w-full max-w-[calc(100%-2rem)] translate-x-[-50%] translate-y-[-50%] gap-4 rounded-lg border p-6 shadow-lg duration-200",
className,
)}
{...props}
Expand Down
5 changes: 5 additions & 0 deletions apps/desktop/src/renderer/globals.css
Original file line number Diff line number Diff line change
Expand Up @@ -135,6 +135,11 @@
-webkit-font-smoothing: antialiased;
}

/* Micro 5 font for logo */
.font-micro5 {
font-family: "Micro 5", monospace;
}

/* Ensure xterm terminal fills container height */
.xterm {
height: 100% !important;
Expand Down
5 changes: 5 additions & 0 deletions apps/desktop/src/renderer/index.html
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,11 @@
<meta http-equiv="Content-Security-Policy" content="default-src 'self'; script-src 'self'; style-src 'self' 'unsafe-inline'" />
-->
<meta http-equiv="Content-Security-Policy" content="default-src 'self' *; script-src 'self' *; style-src 'self' 'unsafe-inline' *;" />

<!-- Google Fonts: Micro 5 -->
<link rel="preconnect" href="https://fonts.googleapis.com">
<link rel="preconnect" href="https://fonts.gstatic.com" crossorigin>
<link href="https://fonts.googleapis.com/css2?family=Micro+5&display=swap" rel="stylesheet">
</head>

<body>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,5 @@ interface AppFrameProps {
}

export function AppFrame({ children }: AppFrameProps) {
return (
<div className="absolute inset-0 bg-stone-950 flex">
{children}
</div>
);
return <div className="absolute inset-0 bg-stone-950 flex">{children}</div>;
}
Original file line number Diff line number Diff line change
Expand Up @@ -155,14 +155,15 @@ export const DiffContent = memo(function DiffContent({
</div>
)}
<span
className={`px-1.5 py-0.5 text-[10px] font-medium rounded ${file.status === "added"
? "bg-emerald-500/10 text-emerald-400"
: file.status === "deleted"
? "bg-rose-500/10 text-rose-400"
: file.status === "modified"
? "bg-amber-500/10 text-amber-400"
: "bg-white/5 text-zinc-400"
}`}
className={`px-1.5 py-0.5 text-[10px] font-medium rounded ${
file.status === "added"
? "bg-emerald-500/10 text-emerald-400"
: file.status === "deleted"
? "bg-rose-500/10 text-rose-400"
: file.status === "modified"
? "bg-amber-500/10 text-amber-400"
: "bg-white/5 text-zinc-400"
}`}
>
{file.status}
</span>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -49,21 +49,31 @@ export function DiffContentArea({

// Load selected file immediately
useEffect(() => {
if (selectedFile && loadFileContent && !loadedFiles.has(selectedFile) && !loadingFiles.has(selectedFile)) {
if (
selectedFile &&
loadFileContent &&
!loadedFiles.has(selectedFile) &&
!loadingFiles.has(selectedFile)
) {
loadFileContent(selectedFile);
}
}, [selectedFile, loadFileContent, loadedFiles, loadingFiles]);

// Use intersection observer to load files when they come into view
useEffect(() => {
if (viewMode !== "files" || !scrollContainerRef.current || !loadFileContent) return;
if (viewMode !== "files" || !scrollContainerRef.current || !loadFileContent)
return;

const observer = new IntersectionObserver(
(entries) => {
for (const entry of entries) {
if (entry.isIntersecting) {
const fileId = entry.target.id.replace("file-diff-", "");
if (fileId && !loadedFilesRef.current.has(fileId) && !loadingFilesRef.current.has(fileId)) {
if (
fileId &&
!loadedFilesRef.current.has(fileId) &&
!loadingFilesRef.current.has(fileId)
) {
loadFileContentRef.current?.(fileId);
}
}
Expand Down Expand Up @@ -361,4 +371,3 @@ export function DiffContentArea({
</div>
);
}

Loading
Loading