fix: address PR #173 security and code quality feedback

Security fixes:
- Enhanced branch name sanitization for cross-platform filesystem safety
  (handles Windows-invalid chars, reserved names, path length limits)
- Added branch name validation in pr-info.ts to prevent command injection
- Sanitized prUrl in kanban-card to only allow http/https URLs

Code quality improvements:
- Fixed placeholder issue where {owner}/{repo} was passed literally to gh api
- Replaced async forEach with Promise.all for proper async handling
- Display PR number extracted from URL in kanban cards

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
This commit is contained in:
Cody Seibert
2025-12-19 20:39:38 -05:00
parent 6c25680115
commit ec7c2892c2
7 changed files with 153 additions and 132 deletions

View File

@@ -20,12 +20,38 @@ export interface WorktreeMetadata {
pr?: WorktreePRInfo;
}
/**
* Sanitize branch name for cross-platform filesystem safety
*/
function sanitizeBranchName(branch: string): string {
// Replace characters that are invalid or problematic on various filesystems:
// - Forward and backslashes (path separators)
// - Windows invalid chars: : * ? " < > |
// - Other potentially problematic chars
let safeBranch = branch
.replace(/[/\\:*?"<>|]/g, "-") // Replace invalid chars with dash
.replace(/\s+/g, "_") // Replace spaces with underscores
.replace(/\.+$/g, "") // Remove trailing dots (Windows issue)
.replace(/-+/g, "-") // Collapse multiple dashes
.replace(/^-|-$/g, ""); // Remove leading/trailing dashes
// Truncate to safe length (leave room for path components)
safeBranch = safeBranch.substring(0, 200);
// Handle Windows reserved names (CON, PRN, AUX, NUL, COM1-9, LPT1-9)
const windowsReserved = /^(CON|PRN|AUX|NUL|COM[1-9]|LPT[1-9])$/i;
if (windowsReserved.test(safeBranch) || safeBranch.length === 0) {
safeBranch = `_${safeBranch || "branch"}`;
}
return safeBranch;
}
/**
* Get the path to the worktree metadata directory
*/
function getWorktreeMetadataDir(projectPath: string, branch: string): string {
// Sanitize branch name for filesystem (replace / with -)
const safeBranch = branch.replace(/\//g, "-");
const safeBranch = sanitizeBranchName(branch);
return path.join(projectPath, ".automaker", "worktrees", safeBranch);
}

View File

@@ -8,6 +8,24 @@ import { promisify } from "util";
import { getErrorMessage, logError } from "../common.js";
import { updateWorktreePRInfo } from "../../../lib/worktree-metadata.js";
// Shell escaping utility to prevent command injection
function shellEscape(arg: string): string {
if (process.platform === "win32") {
// Windows CMD shell escaping
return `"${arg.replace(/"/g, '""')}"`;
} else {
// Unix shell escaping
return `'${arg.replace(/'/g, "'\\''")}'`;
}
}
// Validate branch name to prevent command injection
function isValidBranchName(name: string): boolean {
// Git branch names cannot contain: space, ~, ^, :, ?, *, [, \, or control chars
// Also reject shell metacharacters for safety
return /^[a-zA-Z0-9._\-/]+$/.test(name) && name.length < 250;
}
const execAsync = promisify(exec);
// Extended PATH to include common tool installation locations
@@ -78,6 +96,15 @@ export function createCreatePRHandler() {
);
const branchName = branchOutput.trim();
// Validate branch name for security
if (!isValidBranchName(branchName)) {
res.status(400).json({
success: false,
error: "Invalid branch name contains unsafe characters",
});
return;
}
// Check for uncommitted changes
const { stdout: status } = await execAsync("git status --porcelain", {
cwd: worktreePath,

View File

@@ -42,6 +42,15 @@ const execEnv = {
PATH: extendedPath,
};
/**
* Validate branch name to prevent command injection.
* Git branch names cannot contain: space, ~, ^, :, ?, *, [, \, or control chars.
* We also reject shell metacharacters for safety.
*/
function isValidBranchName(name: string): boolean {
return /^[a-zA-Z0-9._\-/]+$/.test(name) && name.length < 250;
}
export interface PRComment {
id: number;
author: string;
@@ -79,6 +88,15 @@ export function createPRInfoHandler() {
return;
}
// Validate branch name to prevent command injection
if (!isValidBranchName(branchName)) {
res.status(400).json({
success: false,
error: "Invalid branch name contains unsafe characters",
});
return;
}
// Check if gh CLI is available
let ghCliAvailable = false;
try {
@@ -226,35 +244,38 @@ export function createPRInfoHandler() {
// Get review comments (inline code comments)
let reviewComments: PRComment[] = [];
try {
const reviewsEndpoint = targetRepo
? `repos/${targetRepo}/pulls/${prNumber}/comments`
: `repos/{owner}/{repo}/pulls/${prNumber}/comments`;
const reviewsCmd = `gh api ${reviewsEndpoint}`;
const { stdout: reviewsOutput } = await execAsync(
reviewsCmd,
{ cwd: worktreePath, env: execEnv }
);
const reviewsData = JSON.parse(reviewsOutput);
reviewComments = reviewsData.map((c: {
id: number;
user: { login: string };
body: string;
path: string;
line?: number;
original_line?: number;
created_at: string;
}) => ({
id: c.id,
author: c.user?.login || "unknown",
body: c.body,
path: c.path,
line: c.line || c.original_line,
createdAt: c.created_at,
isReviewComment: true,
}));
} catch (error) {
console.warn("[PRInfo] Failed to fetch review comments:", error);
// Only fetch review comments if we have repository info
if (targetRepo) {
try {
const reviewsEndpoint = `repos/${targetRepo}/pulls/${prNumber}/comments`;
const reviewsCmd = `gh api ${reviewsEndpoint}`;
const { stdout: reviewsOutput } = await execAsync(
reviewsCmd,
{ cwd: worktreePath, env: execEnv }
);
const reviewsData = JSON.parse(reviewsOutput);
reviewComments = reviewsData.map((c: {
id: number;
user: { login: string };
body: string;
path: string;
line?: number;
original_line?: number;
created_at: string;
}) => ({
id: c.id,
author: c.user?.login || "unknown",
body: c.body,
path: c.path,
line: c.line || c.original_line,
createdAt: c.created_at,
isReviewComment: true,
}));
} catch (error) {
console.warn("[PRInfo] Failed to fetch review comments:", error);
}
} else {
console.warn("[PRInfo] Cannot fetch review comments: repository info not available");
}
const prInfo: PRInfo = {