fix: Prevent command injection and stale state in test runner

This commit is contained in:
Shirone
2026-01-21 16:12:36 +01:00
parent b73885e04a
commit 4ab927a5fb
5 changed files with 70 additions and 30 deletions

View File

@@ -209,6 +209,16 @@ class TestRunnerService {
return `test-${Date.now()}-${Math.random().toString(36).slice(2, 11)}`; return `test-${Date.now()}-${Math.random().toString(36).slice(2, 11)}`;
} }
/**
* Sanitize a test file path to prevent command injection
* Allows only safe characters for file paths
*/
private sanitizeTestFile(testFile: string): string {
// Remove any shell metacharacters and normalize path
// Allow only alphanumeric, dots, slashes, hyphens, underscores, colons (for Windows paths)
return testFile.replace(/[^a-zA-Z0-9.\\/_\-:]/g, '');
}
/** /**
* Start tests in a worktree using the provided command * Start tests in a worktree using the provided command
* *
@@ -252,9 +262,11 @@ class TestRunnerService {
// Build the final command (append test file if specified) // Build the final command (append test file if specified)
let finalCommand = command; let finalCommand = command;
if (testFile) { if (testFile) {
// Sanitize test file path to prevent command injection
const sanitizedFile = this.sanitizeTestFile(testFile);
// Append the test file to the command // Append the test file to the command
// Most test runners support: command -- file or command file // Most test runners support: command -- file or command file
finalCommand = `${command} -- ${testFile}`; finalCommand = `${command} -- ${sanitizedFile}`;
} }
// Parse command into cmd and args (shell execution) // Parse command into cmd and args (shell execution)
@@ -650,15 +662,19 @@ export function getTestRunnerService(): TestRunnerService {
} }
// Cleanup on process exit // Cleanup on process exit
process.on('SIGTERM', async () => { process.on('SIGTERM', () => {
if (testRunnerServiceInstance) { if (testRunnerServiceInstance) {
await testRunnerServiceInstance.cleanup(); testRunnerServiceInstance.cleanup().catch((err) => {
logger.error('Cleanup failed on SIGTERM:', err);
});
} }
}); });
process.on('SIGINT', async () => { process.on('SIGINT', () => {
if (testRunnerServiceInstance) { if (testRunnerServiceInstance) {
await testRunnerServiceInstance.cleanup(); testRunnerServiceInstance.cleanup().catch((err) => {
logger.error('Cleanup failed on SIGINT:', err);
});
} }
}); });

View File

@@ -61,6 +61,12 @@ function getStatusIndicator(status: TestRunStatus | null): {
className: 'bg-blue-500/10 text-blue-500', className: 'bg-blue-500/10 text-blue-500',
icon: <span className="w-1.5 h-1.5 rounded-full bg-blue-500 animate-pulse" />, icon: <span className="w-1.5 h-1.5 rounded-full bg-blue-500 animate-pulse" />,
}; };
case 'pending':
return {
text: 'Pending',
className: 'bg-amber-500/10 text-amber-500',
icon: <Clock className="w-3 h-3" />,
};
case 'passed': case 'passed':
return { return {
text: 'Passed', text: 'Passed',

View File

@@ -280,33 +280,36 @@ export function WorktreePanel({
); );
// Handler to start tests for a worktree // Handler to start tests for a worktree
const handleStartTests = useCallback(async (worktree: WorktreeInfo) => { const handleStartTests = useCallback(
setIsStartingTests(true); async (worktree: WorktreeInfo) => {
try { setIsStartingTests(true);
const api = getElectronAPI(); try {
if (!api?.worktree?.startTests) { const api = getElectronAPI();
toast.error('Test runner API not available'); if (!api?.worktree?.startTests) {
return; toast.error('Test runner API not available');
} return;
}
const result = await api.worktree.startTests(worktree.path, { projectPath }); const result = await api.worktree.startTests(worktree.path, { projectPath });
if (result.success) { if (result.success) {
toast.success('Tests started', { toast.success('Tests started', {
description: `Running tests in ${worktree.branch}`, description: `Running tests in ${worktree.branch}`,
}); });
} else { } else {
toast.error('Failed to start tests', {
description: result.error || 'Unknown error',
});
}
} catch (error) {
toast.error('Failed to start tests', { toast.error('Failed to start tests', {
description: result.error || 'Unknown error', description: error instanceof Error ? error.message : 'Unknown error',
}); });
} finally {
setIsStartingTests(false);
} }
} catch (error) { },
toast.error('Failed to start tests', { [projectPath]
description: error instanceof Error ? error.message : 'Unknown error', );
});
} finally {
setIsStartingTests(false);
}
}, []);
// Handler to stop tests for a worktree // Handler to stop tests for a worktree
const handleStopTests = useCallback( const handleStopTests = useCallback(

View File

@@ -64,12 +64,14 @@ export function TestingSection({ project }: TestingSectionProps) {
setIsSaving(true); setIsSaving(true);
try { try {
const httpClient = getHttpApiClient(); const httpClient = getHttpApiClient();
const normalizedCommand = testCommand.trim();
const response = await httpClient.settings.updateProject(project.path, { const response = await httpClient.settings.updateProject(project.path, {
testCommand: testCommand.trim() || undefined, testCommand: normalizedCommand || undefined,
}); });
if (response.success) { if (response.success) {
setOriginalTestCommand(testCommand); setTestCommand(normalizedCommand);
setOriginalTestCommand(normalizedCommand);
toast.success('Test command saved'); toast.success('Test command saved');
} else { } else {
toast.error('Failed to save test command', { toast.error('Failed to save test command', {

View File

@@ -126,6 +126,9 @@ export function useTestLogs({
// Track the current session ID for filtering events // Track the current session ID for filtering events
const currentSessionId = useRef<string | null>(targetSessionId ?? null); const currentSessionId = useRef<string | null>(targetSessionId ?? null);
// Guard against stale fetch results when switching worktrees/sessions
const fetchSeq = useRef(0);
/** /**
* Derived state: whether tests are currently running * Derived state: whether tests are currently running
*/ */
@@ -137,11 +140,16 @@ export function useTestLogs({
const fetchLogs = useCallback(async () => { const fetchLogs = useCallback(async () => {
if (!worktreePath && !targetSessionId) return; if (!worktreePath && !targetSessionId) return;
// Increment sequence to guard against stale responses
const seq = ++fetchSeq.current;
setState((prev) => ({ ...prev, isLoading: true, error: null })); setState((prev) => ({ ...prev, isLoading: true, error: null }));
try { try {
const api = getElectronAPI(); const api = getElectronAPI();
if (!api?.worktree?.getTestLogs) { if (!api?.worktree?.getTestLogs) {
// Check if this request is still current
if (seq !== fetchSeq.current) return;
setState((prev) => ({ setState((prev) => ({
...prev, ...prev,
isLoading: false, isLoading: false,
@@ -152,6 +160,9 @@ export function useTestLogs({
const result = await api.worktree.getTestLogs(worktreePath ?? undefined, targetSessionId); const result = await api.worktree.getTestLogs(worktreePath ?? undefined, targetSessionId);
// Check if this request is still current (prevent stale updates)
if (seq !== fetchSeq.current) return;
if (result.success && result.result) { if (result.success && result.result) {
const { sessionId, command, status, testFile, logs, startedAt, finishedAt, exitCode } = const { sessionId, command, status, testFile, logs, startedAt, finishedAt, exitCode } =
result.result; result.result;
@@ -183,6 +194,8 @@ export function useTestLogs({
})); }));
} }
} catch (error) { } catch (error) {
// Check if this request is still current
if (seq !== fetchSeq.current) return;
logger.error('Failed to fetch test logs:', error); logger.error('Failed to fetch test logs:', error);
setState((prev) => ({ setState((prev) => ({
...prev, ...prev,