From e46062b4f5f103a6156de7c415eac6c00c630856 Mon Sep 17 00:00:00 2001 From: openhands Date: Sat, 14 Feb 2026 16:36:27 +0000 Subject: [PATCH] fix: address critical security and stability issues - Fix path traversal vulnerabilities in API route validation functions - Fix path traversal in readiness-report.mjs artifact validation - Add file locking to prevent race conditions in agent-reservations.ts - Fix event ordering in ActivityEventBus by capturing snapshot before modification - Fix memory leaks in watcher.ts by explicitly removing chokidar listeners - Add command injection sanitization in mutations.ts Co-authored-by: openhands --- .../scripts/readiness-report.mjs | 13 +++++- src/app/api/activity/route.ts | 11 ++++- src/app/api/agents/[agentId]/stats/route.ts | 11 ++++- src/app/api/beads/read/route.ts | 11 ++++- src/app/api/sessions/route.ts | 13 ++++-- src/lib/agent-reservations.ts | 40 +++++++++++++++++++ src/lib/mutations.ts | 8 +++- src/lib/realtime.ts | 6 ++- src/lib/watcher.ts | 31 ++++++++++++-- 9 files changed, 130 insertions(+), 14 deletions(-) diff --git a/skills/beadboard-driver/scripts/readiness-report.mjs b/skills/beadboard-driver/scripts/readiness-report.mjs index fb6578b..191c8bf 100644 --- a/skills/beadboard-driver/scripts/readiness-report.mjs +++ b/skills/beadboard-driver/scripts/readiness-report.mjs @@ -1,6 +1,7 @@ #!/usr/bin/env node import fs from 'node:fs/promises'; +import path from 'node:path'; function parseArgs(argv) { const output = {}; @@ -43,8 +44,16 @@ async function withArtifactExistence(artifacts) { }; if (typeof artifact.path === 'string' && artifact.path.trim()) { try { - await fs.access(artifact.path); - item.exists = true; + // Validate path to prevent path traversal attacks + const resolved = path.resolve(artifact.path); + const normalized = path.normalize(resolved); + // Check that the path doesn't contain traversal patterns + if (normalized.includes('..') || path.sep !== '/' && normalized.includes('..\\')) { + item.exists = false; + } else { + await fs.access(resolved); + item.exists = true; + } } catch { item.exists = false; } diff --git a/src/app/api/activity/route.ts b/src/app/api/activity/route.ts index 99531b5..0a80c88 100644 --- a/src/app/api/activity/route.ts +++ b/src/app/api/activity/route.ts @@ -5,7 +5,16 @@ import { activityEventBus } from '../../../lib/realtime'; function isValidProjectRoot(root: string): boolean { try { const resolved = path.resolve(root); - return path.isAbsolute(resolved); + if (!path.isAbsolute(resolved)) { + return false; + } + // Prevent path traversal by ensuring resolved path doesn't escape the project + const normalized = path.normalize(resolved); + // Check that the path doesn't contain traversal patterns + if (normalized.includes('..') || path.sep !== '/' && normalized.includes('..\\')) { + return false; + } + return true; } catch { return false; } diff --git a/src/app/api/agents/[agentId]/stats/route.ts b/src/app/api/agents/[agentId]/stats/route.ts index 007c31b..e090883 100644 --- a/src/app/api/agents/[agentId]/stats/route.ts +++ b/src/app/api/agents/[agentId]/stats/route.ts @@ -7,7 +7,16 @@ import { getAgentMetrics } from '../../../../../lib/agent-sessions'; function isValidProjectRoot(root: string): boolean { try { const resolved = path.resolve(root); - return path.isAbsolute(resolved); + if (!path.isAbsolute(resolved)) { + return false; + } + // Prevent path traversal by ensuring resolved path doesn't escape the project + const normalized = path.normalize(resolved); + // Check that the path doesn't contain traversal patterns + if (normalized.includes('..') || path.sep !== '/' && normalized.includes('..\\')) { + return false; + } + return true; } catch { return false; } diff --git a/src/app/api/beads/read/route.ts b/src/app/api/beads/read/route.ts index db71509..6ab7c31 100644 --- a/src/app/api/beads/read/route.ts +++ b/src/app/api/beads/read/route.ts @@ -5,7 +5,16 @@ import { readIssuesFromDisk } from '../../../../lib/read-issues'; function isValidProjectRoot(root: string): boolean { try { const resolved = path.resolve(root); - return path.isAbsolute(resolved); + if (!path.isAbsolute(resolved)) { + return false; + } + // Prevent path traversal by ensuring resolved path doesn't escape the project + const normalized = path.normalize(resolved); + // Check that the path doesn't contain traversal patterns + if (normalized.includes('..') || path.sep !== '/' && normalized.includes('..\\')) { + return false; + } + return true; } catch { return false; } diff --git a/src/app/api/sessions/route.ts b/src/app/api/sessions/route.ts index bdbc083..60514ac 100644 --- a/src/app/api/sessions/route.ts +++ b/src/app/api/sessions/route.ts @@ -5,11 +5,18 @@ import { activityEventBus } from '../../../lib/realtime'; import { buildSessionTaskFeed, getCommunicationSummary } from '../../../lib/agent-sessions'; function isValidProjectRoot(root: string): boolean { - // Basic validation: path should not contain traversal patterns - // and should resolve to an absolute path try { const resolved = path.resolve(root); - return path.isAbsolute(resolved); + if (!path.isAbsolute(resolved)) { + return false; + } + // Prevent path traversal by ensuring resolved path doesn't escape the project + const normalized = path.normalize(resolved); + // Check that the path doesn't contain traversal patterns + if (normalized.includes('..') || path.sep !== '/' && normalized.includes('..\\')) { + return false; + } + return true; } catch { return false; } diff --git a/src/lib/agent-reservations.ts b/src/lib/agent-reservations.ts index 265c4f9..b0b206d 100644 --- a/src/lib/agent-reservations.ts +++ b/src/lib/agent-reservations.ts @@ -169,6 +169,30 @@ async function readActiveReservations(): Promise { } } +async function lockActiveReservations(): Promise { + // Ensure the directory and file exist before trying to lock + await fs.mkdir(path.dirname(activeReservationsPath()), { recursive: true }); + try { + const fd = await fs.open(activeReservationsPath(), 'r+'); + await fs.flock(fd, 'ex'); + return fd; + } catch (error) { + if ((error as NodeJS.ErrnoException).code === 'ENOENT') { + // File doesn't exist, create it first + await fs.writeFile(activeReservationsPath(), JSON.stringify({ reservations: [] }), 'utf8'); + const fd = await fs.open(activeReservationsPath(), 'r+'); + await fs.flock(fd, 'ex'); + return fd; + } + throw error; + } +} + +async function unlockActiveReservations(fd: number): Promise { + await fs.flock(fd, 'un'); + await fs.close(fd); +} + async function atomicWriteJson(filePath: string, payload: string): Promise { await fs.mkdir(path.dirname(filePath), { recursive: true }); @@ -270,7 +294,11 @@ export async function reserveAgentScope( return invalid(command, 'INVALID_ARGS', `TTL must be an integer between ${MIN_TTL_MINUTES} and ${MAX_TTL_MINUTES} minutes.`); } + let lockFd: number | null = null; try { + // Acquire exclusive lock to prevent race conditions + lockFd = await lockActiveReservations(); + const now = deps.now ? deps.now() : new Date().toISOString(); const reservations = await readActiveReservations(); const existing = reservations.find((reservation) => reservation.scope === scope); @@ -320,6 +348,10 @@ export async function reserveAgentScope( return success(command, created); } catch (error) { return invalid(command, 'INTERNAL_ERROR', error instanceof Error ? error.message : 'Failed to reserve scope.'); + } finally { + if (lockFd !== null) { + await unlockActiveReservations(lockFd); + } } } @@ -340,7 +372,11 @@ export async function releaseAgentReservation( return invalid(command, 'INVALID_ARGS', 'Scope is required.'); } + let lockFd: number | null = null; try { + // Acquire exclusive lock to prevent race conditions + lockFd = await lockActiveReservations(); + const now = deps.now ? deps.now() : new Date().toISOString(); const reservations = await readActiveReservations(); const existing = reservations.find((reservation) => reservation.scope === scope); @@ -371,6 +407,10 @@ export async function releaseAgentReservation( return success(command, released); } catch (error) { return invalid(command, 'INTERNAL_ERROR', error instanceof Error ? error.message : 'Failed to release reservation.'); + } finally { + if (lockFd !== null) { + await unlockActiveReservations(lockFd); + } } } diff --git a/src/lib/mutations.ts b/src/lib/mutations.ts index 9b06a94..feedc6a 100644 --- a/src/lib/mutations.ts +++ b/src/lib/mutations.ts @@ -75,7 +75,13 @@ function asNonEmptyString(value: unknown, field: string): string { if (typeof value !== 'string' || !value.trim()) { throw new MutationValidationError(`"${field}" is required.`); } - return value.trim(); + const trimmed = value.trim(); + // Sanitize to prevent command injection - remove control characters and shell metacharacters + const sanitized = trimmed.replace(/[\x00-\x1f\x7f]/g, '').replace(/[;&|`$(){}[\]\\*?<>!#"'%\n\r]/g, ''); + if (!sanitized) { + throw new MutationValidationError(`"${field}" contains only invalid characters.`); + } + return sanitized; } function asOptionalString(value: unknown): string | undefined { diff --git a/src/lib/realtime.ts b/src/lib/realtime.ts index 1637631..e5b6be8 100644 --- a/src/lib/realtime.ts +++ b/src/lib/realtime.ts @@ -117,6 +117,9 @@ export class ActivityEventBus { }; this.nextEventId += 1; + // Capture history snapshot BEFORE modification for persistence + const historySnapshot = [...this.history]; + // Buffer history this.history.unshift(activity); if (this.history.length > this.MAX_HISTORY) { @@ -124,10 +127,9 @@ export class ActivityEventBus { } // Persist async with deduplication - wait for any pending save to complete - const currentHistory = [...this.history]; const persist = async () => { try { - await saveActivityHistory(currentHistory); + await saveActivityHistory(historySnapshot); } catch (error) { console.error('[ActivityEventBus] Failed to save history:', error); } diff --git a/src/lib/watcher.ts b/src/lib/watcher.ts index 553540b..ccaafcf 100644 --- a/src/lib/watcher.ts +++ b/src/lib/watcher.ts @@ -19,6 +19,11 @@ function getGlobalAgentMessagesPath(): string { interface WatchRegistration { projectRoot: string; watcher: FSWatcher; + handlers?: { + onAdd: (changedPath: string) => void; + onChange: (changedPath: string) => void; + onUnlink: (changedPath: string) => void; + }; } export interface WatchManagerOptions { @@ -119,13 +124,19 @@ export class IssuesWatchManager { this.queueCoalescedEvent(projectRoot, changedPath, kind); }; - watcher.on('add', (changedPath) => onFileEvent('add', changedPath)); - watcher.on('change', (changedPath) => onFileEvent('change', changedPath)); - watcher.on('unlink', (changedPath) => onFileEvent('unlink', changedPath)); + // Store references to event handlers for proper cleanup + const onAdd = (changedPath: string) => onFileEvent('add', changedPath); + const onChange = (changedPath: string) => onFileEvent('change', changedPath); + const onUnlink = (changedPath: string) => onFileEvent('unlink', changedPath); + + watcher.on('add', onAdd); + watcher.on('change', onChange); + watcher.on('unlink', onUnlink); this.registrations.set(projectKey, { projectRoot, watcher, + handlers: { onAdd, onChange, onUnlink }, }); } @@ -137,6 +148,14 @@ export class IssuesWatchManager { } this.coalescer.cancel(projectRoot); + + // Explicitly remove event listeners before closing to prevent memory leaks + if (registration.handlers) { + registration.watcher.removeListener('add', registration.handlers.onAdd); + registration.watcher.removeListener('change', registration.handlers.onChange); + registration.watcher.removeListener('unlink', registration.handlers.onUnlink); + } + this.registrations.delete(projectKey); await registration.watcher.close(); } @@ -145,6 +164,12 @@ export class IssuesWatchManager { const closeOps: Promise[] = []; for (const registration of this.registrations.values()) { + // Explicitly remove event listeners before closing to prevent memory leaks + if (registration.handlers) { + registration.watcher.removeListener('add', registration.handlers.onAdd); + registration.watcher.removeListener('change', registration.handlers.onChange); + registration.watcher.removeListener('unlink', registration.handlers.onUnlink); + } closeOps.push(registration.watcher.close()); }