feat: Add path validation and security improvements to worktree routes

This commit is contained in:
gsxdsm
2026-02-17 10:17:23 -08:00
parent a09a2c76ae
commit f7b3f75163
3 changed files with 106 additions and 64 deletions

View File

@@ -101,7 +101,12 @@ export function createWorktreeRoutes(
requireValidWorktree,
createPullHandler()
);
router.post('/checkout-branch', requireValidWorktree, createCheckoutBranchHandler());
router.post(
'/checkout-branch',
validatePathParams('worktreePath'),
requireValidWorktree,
createCheckoutBranchHandler()
);
router.post(
'/list-branches',
validatePathParams('worktreePath'),

View File

@@ -2,15 +2,15 @@
* POST /checkout-branch endpoint - Create and checkout a new branch
*
* Note: Git repository validation (isGitRepo, hasCommits) is handled by
* the requireValidWorktree middleware in index.ts
* the requireValidWorktree middleware in index.ts.
* Path validation (ALLOWED_ROOT_DIRECTORY) is handled by validatePathParams
* middleware in index.ts.
*/
import type { Request, Response } from 'express';
import { exec } from 'child_process';
import { promisify } from 'util';
import { getErrorMessage, logError } from '../common.js';
const execAsync = promisify(exec);
import path from 'path';
import { stat } from 'fs/promises';
import { getErrorMessage, logError, isValidBranchName, execGitCommand } from '../common.js';
export function createCheckoutBranchHandler() {
return async (req: Request, res: Response): Promise<void> => {
@@ -36,27 +36,47 @@ export function createCheckoutBranchHandler() {
return;
}
// Validate branch name (basic validation)
const invalidChars = /[\s~^:?*[\\]/;
if (invalidChars.test(branchName)) {
// Validate branch name using shared allowlist: /^[a-zA-Z0-9._\-/]+$/
if (!isValidBranchName(branchName)) {
res.status(400).json({
success: false,
error: 'Branch name contains invalid characters',
error:
'Invalid branch name. Must contain only letters, numbers, dots, dashes, underscores, or slashes.',
});
return;
}
// Get current branch for reference
const { stdout: currentBranchOutput } = await execAsync('git rev-parse --abbrev-ref HEAD', {
cwd: worktreePath,
});
// Resolve and validate worktreePath to prevent traversal attacks.
// The validatePathParams middleware checks against ALLOWED_ROOT_DIRECTORY,
// but we also resolve the path and verify it exists as a directory.
const resolvedPath = path.resolve(worktreePath);
try {
const stats = await stat(resolvedPath);
if (!stats.isDirectory()) {
res.status(400).json({
success: false,
error: 'worktreePath is not a directory',
});
return;
}
} catch {
res.status(400).json({
success: false,
error: 'worktreePath does not exist or is not accessible',
});
return;
}
// Get current branch for reference (using argument array to avoid shell injection)
const currentBranchOutput = await execGitCommand(
['rev-parse', '--abbrev-ref', 'HEAD'],
resolvedPath
);
const currentBranch = currentBranchOutput.trim();
// Check if branch already exists
try {
await execAsync(`git rev-parse --verify ${branchName}`, {
cwd: worktreePath,
});
await execGitCommand(['rev-parse', '--verify', branchName], resolvedPath);
// Branch exists
res.status(400).json({
success: false,
@@ -67,10 +87,8 @@ export function createCheckoutBranchHandler() {
// Branch doesn't exist, good to create
}
// Create and checkout the new branch
await execAsync(`git checkout -b ${branchName}`, {
cwd: worktreePath,
});
// Create and checkout the new branch (using argument array to avoid shell injection)
await execGitCommand(['checkout', '-b', branchName], resolvedPath);
res.json({
success: true,

View File

@@ -1,4 +1,3 @@
// @ts-nocheck - header component props with optional handlers and status variants
import { memo, useState } from 'react';
import type { DraggableAttributes, DraggableSyntheticListeners } from '@dnd-kit/core';
import { Feature } from '@/store/app-store';
@@ -39,37 +38,53 @@ function DuplicateMenuItems({
onDuplicateAsChild?: () => void;
}) {
if (!onDuplicate) return null;
// When there's no sub-child action, render a simple menu item (no DropdownMenuSub wrapper)
if (!onDuplicateAsChild) {
return (
<DropdownMenuItem
onClick={(e) => {
e.stopPropagation();
onDuplicate();
}}
className="text-xs"
>
<Copy className="w-3 h-3 mr-2" />
Duplicate
</DropdownMenuItem>
);
}
// When sub-child action is available, render a proper DropdownMenuSub with
// DropdownMenuSubTrigger and DropdownMenuSubContent per Radix conventions
return (
<DropdownMenuSub>
<div className="flex items-center">
<DropdownMenuSubTrigger className="text-xs">
<Copy className="w-3 h-3 mr-2" />
Duplicate
</DropdownMenuSubTrigger>
<DropdownMenuSubContent>
<DropdownMenuItem
onClick={(e) => {
e.stopPropagation();
onDuplicate();
}}
className="text-xs flex-1 pr-0 rounded-r-none"
className="text-xs"
>
<Copy className="w-3 h-3 mr-2" />
Duplicate
</DropdownMenuItem>
{onDuplicateAsChild && (
<DropdownMenuSubTrigger className="text-xs px-1 rounded-l-none border-l border-border/30 h-8" />
)}
</div>
{onDuplicateAsChild && (
<DropdownMenuSubContent>
<DropdownMenuItem
onClick={(e) => {
e.stopPropagation();
onDuplicateAsChild();
}}
className="text-xs"
>
<GitFork className="w-3 h-3 mr-2" />
Duplicate as Child
</DropdownMenuItem>
</DropdownMenuSubContent>
)}
<DropdownMenuItem
onClick={(e) => {
e.stopPropagation();
onDuplicateAsChild();
}}
className="text-xs"
>
<GitFork className="w-3 h-3 mr-2" />
Duplicate as Child
</DropdownMenuItem>
</DropdownMenuSubContent>
</DropdownMenuSub>
);
}
@@ -122,7 +137,7 @@ export const CardHeaderSection = memo(function CardHeaderSection({
<div className="absolute top-2 right-2 flex items-center gap-1">
<div className="flex items-center justify-center gap-2 bg-[var(--status-in-progress)]/15 border border-[var(--status-in-progress)]/50 rounded-md px-2 py-0.5">
<Spinner size="xs" />
{feature.startedAt && (
{typeof feature.startedAt === 'string' && (
<CountUpTimer
startedAt={feature.startedAt}
className="text-[var(--status-in-progress)] text-[10px]"
@@ -191,6 +206,7 @@ export const CardHeaderSection = memo(function CardHeaderSection({
!isSelectionMode &&
(feature.status === 'backlog' ||
feature.status === 'interrupted' ||
// @ts-expect-error 'ready' is a valid runtime status used for backlog display but not in FeatureStatusWithPipeline union
feature.status === 'ready') && (
<div className="absolute top-2 right-2 flex items-center gap-1">
<Button
@@ -217,26 +233,29 @@ export const CardHeaderSection = memo(function CardHeaderSection({
>
<Trash2 className="w-4 h-4" />
</Button>
<DropdownMenu>
<DropdownMenuTrigger asChild>
<Button
variant="ghost"
size="sm"
className="h-6 w-6 p-0 hover:bg-muted/80 rounded-md"
onClick={(e) => e.stopPropagation()}
onPointerDown={(e) => e.stopPropagation()}
data-testid={`menu-backlog-${feature.id}`}
>
<MoreVertical className="w-3.5 h-3.5 text-muted-foreground" />
</Button>
</DropdownMenuTrigger>
<DropdownMenuContent align="end" className="w-40">
<DuplicateMenuItems
onDuplicate={onDuplicate}
onDuplicateAsChild={onDuplicateAsChild}
/>
</DropdownMenuContent>
</DropdownMenu>
{/* Only render overflow menu when there are actionable items */}
{onDuplicate && (
<DropdownMenu>
<DropdownMenuTrigger asChild>
<Button
variant="ghost"
size="sm"
className="h-6 w-6 p-0 hover:bg-muted/80 rounded-md"
onClick={(e) => e.stopPropagation()}
onPointerDown={(e) => e.stopPropagation()}
data-testid={`menu-backlog-${feature.id}`}
>
<MoreVertical className="w-3.5 h-3.5 text-muted-foreground" />
</Button>
</DropdownMenuTrigger>
<DropdownMenuContent align="end" className="w-40">
<DuplicateMenuItems
onDuplicate={onDuplicate}
onDuplicateAsChild={onDuplicateAsChild}
/>
</DropdownMenuContent>
</DropdownMenu>
)}
</div>
)}