From a3f2ceef52d8d5233778d295019c87c4c8136fbb Mon Sep 17 00:00:00 2001 From: openhands Date: Sat, 14 Feb 2026 08:43:04 +0000 Subject: [PATCH] fix: address Qodo code review findings - Add missing snapshot-differ.test.ts to npm test script - Fix path traversal vulnerability in agent-mail.ts with message ID validation - Fix readLastTouchedVersion to log errors instead of silently swallowing them - Sanitize log statements to not leak full paths - Add projectRoot validation to all API routes - Fix activity persistence write race conditions with promise chaining Co-authored-by: openhands --- package.json | 2 +- src/app/api/activity/route.ts | 19 ++++++++++++++++++- src/app/api/agents/[agentId]/stats/route.ts | 17 ++++++++++++++++- src/app/api/beads/read/route.ts | 21 +++++++++++++++++++-- src/app/api/events/route.ts | 2 ++ src/app/api/sessions/route.ts | 21 ++++++++++++++++++++- src/lib/agent-mail.ts | 14 ++++++++++++++ src/lib/realtime.ts | 21 ++++++++++++++++++--- 8 files changed, 108 insertions(+), 9 deletions(-) diff --git a/package.json b/package.json index e818f15..37ac8ab 100644 --- a/package.json +++ b/package.json @@ -9,7 +9,7 @@ "start": "next start", "lint": "eslint .", "typecheck": "tsc --noEmit", - "test": "node --test tests/bootstrap.test.mjs && node --import tsx --test tests/lib/parser.test.ts && node --import tsx --test tests/lib/pathing.test.ts && node --import tsx --test tests/lib/project-context.test.ts && node --import tsx --test tests/lib/project-scope.test.ts && node --import tsx --test tests/lib/aggregate-read.test.ts && node --import tsx --test tests/lib/kanban.test.ts && node --import tsx --test tests/lib/graph.test.ts && node --import tsx --test tests/lib/graph-view.test.ts && node --import tsx --test tests/lib/read-text-retry.test.ts && node --import tsx --test tests/lib/read-issues.test.ts && node --import tsx --test tests/lib/bd-path.test.ts && node --import tsx --test tests/lib/bridge.test.ts && node --import tsx --test tests/lib/mutations.test.ts && node --import tsx --test tests/lib/issue-editor.test.ts && node --import tsx --test tests/lib/writeback.test.ts && node --import tsx --test tests/lib/realtime.test.ts && node --import tsx --test tests/lib/coalescer.test.ts && node --import tsx --test tests/lib/watcher.test.ts && node --import tsx --test tests/lib/registry.test.ts && node --import tsx --test tests/lib/agent-registry.test.ts && node --import tsx --test tests/lib/agent-mail.test.ts && node --import tsx --test tests/lib/agent-reservations.test.ts && node --import tsx --test tests/lib/scanner.test.ts && node --import tsx --test tests/skills/beadboard-driver/resolve-bb.test.ts && node --import tsx --test tests/skills/beadboard-driver/generate-agent-name.test.ts && node --import tsx --test tests/skills/beadboard-driver/session-preflight.test.ts && node --import tsx --test tests/skills/beadboard-driver/readiness-report.test.ts && node --import tsx --test tests/skills/beadboard-driver/skill-local-runner.test.ts && node --import tsx --test tests/api/projects-route.test.ts && node --import tsx --test tests/api/mutations-routes.test.ts && node --import tsx --test tests/api/events-route.test.ts && node --test tests/guards/no-direct-jsonl-write.test.mjs && node --test tests/guards/no-inline-style-in-kanban.test.mjs && node --test tests/guards/kanban-responsive-contract.test.mjs && node --test tests/guards/graph-responsive-contract.test.mjs" + "test": "node --test tests/bootstrap.test.mjs && node --import tsx --test tests/lib/parser.test.ts && node --import tsx --test tests/lib/pathing.test.ts && node --import tsx --test tests/lib/project-context.test.ts && node --import tsx --test tests/lib/project-scope.test.ts && node --import tsx --test tests/lib/aggregate-read.test.ts && node --import tsx --test tests/lib/kanban.test.ts && node --import tsx --test tests/lib/graph.test.ts && node --import tsx --test tests/lib/graph-view.test.ts && node --import tsx --test tests/lib/read-text-retry.test.ts && node --import tsx --test tests/lib/read-issues.test.ts && node --import tsx --test tests/lib/bd-path.test.ts && node --import tsx --test tests/lib/bridge.test.ts && node --import tsx --test tests/lib/mutations.test.ts && node --import tsx --test tests/lib/issue-editor.test.ts && node --import tsx --test tests/lib/writeback.test.ts && node --import tsx --test tests/lib/realtime.test.ts && node --import tsx --test tests/lib/coalescer.test.ts && node --import tsx --test tests/lib/snapshot-differ.test.ts && node --import tsx --test tests/lib/watcher.test.ts && node --import tsx --test tests/lib/registry.test.ts && node --import tsx --test tests/lib/agent-registry.test.ts && node --import tsx --test tests/lib/agent-mail.test.ts && node --import tsx --test tests/lib/agent-reservations.test.ts && node --import tsx --test tests/lib/scanner.test.ts && node --import tsx --test tests/skills/beadboard-driver/resolve-bb.test.ts && node --import tsx --test tests/skills/beadboard-driver/generate-agent-name.test.ts && node --import tsx --test tests/skills/beadboard-driver/session-preflight.test.ts && node --import tsx --test tests/skills/beadboard-driver/readiness-report.test.ts && node --import tsx --test tests/skills/beadboard-driver/skill-local-runner.test.ts && node --import tsx --test tests/api/projects-route.test.ts && node --import tsx --test tests/api/mutations-routes.test.ts && node --import tsx --test tests/api/events-route.test.ts && node --test tests/guards/no-direct-jsonl-write.test.mjs && node --test tests/guards/no-inline-style-in-kanban.test.mjs && node --test tests/guards/kanban-responsive-contract.test.mjs && node --test tests/guards/graph-responsive-contract.test.mjs" }, "dependencies": { "@xyflow/react": "^12.10.0", diff --git a/src/app/api/activity/route.ts b/src/app/api/activity/route.ts index 5e61375..5b618f8 100644 --- a/src/app/api/activity/route.ts +++ b/src/app/api/activity/route.ts @@ -1,9 +1,26 @@ import { activityEventBus } from '../../../lib/realtime'; +function isValidProjectRoot(root: string): boolean { + try { + const resolved = require('path').resolve(root); + return require('path').isAbsolute(resolved); + } catch { + return false; + } +} + export async function GET(request: Request): Promise { const url = new URL(request.url); - const projectRoot = url.searchParams.get('projectRoot') || undefined; + const projectRootParam = url.searchParams.get('projectRoot'); + + if (projectRootParam && !isValidProjectRoot(projectRootParam)) { + return NextResponse.json( + { error: 'Invalid projectRoot path' }, + { status: 400 } + ); + } + const projectRoot = projectRootParam || undefined; const history = activityEventBus.getHistory(projectRoot); return Response.json(history); diff --git a/src/app/api/agents/[agentId]/stats/route.ts b/src/app/api/agents/[agentId]/stats/route.ts index 0a8d4b4..007c31b 100644 --- a/src/app/api/agents/[agentId]/stats/route.ts +++ b/src/app/api/agents/[agentId]/stats/route.ts @@ -1,15 +1,30 @@ import { NextResponse } from 'next/server'; +import path from 'node:path'; import { readIssuesFromDisk } from '../../../../../lib/read-issues'; import { activityEventBus } from '../../../../../lib/realtime'; import { getAgentMetrics } from '../../../../../lib/agent-sessions'; +function isValidProjectRoot(root: string): boolean { + try { + const resolved = path.resolve(root); + return path.isAbsolute(resolved); + } catch { + return false; + } +} + export async function GET( request: Request, { params }: { params: Promise<{ agentId: string }> } ): Promise { const { agentId } = await params; const url = new URL(request.url); - const projectRoot = url.searchParams.get('projectRoot') ?? process.cwd(); + const projectRootParam = url.searchParams.get('projectRoot'); + const projectRoot = projectRootParam ?? process.cwd(); + + if (projectRootParam && !isValidProjectRoot(projectRootParam)) { + return NextResponse.json({ ok: false, error: 'Invalid projectRoot path' }, { status: 400 }); + } try { const issues = await readIssuesFromDisk({ projectRoot, preferBd: true }); diff --git a/src/app/api/beads/read/route.ts b/src/app/api/beads/read/route.ts index 71ca8a5..98ba521 100644 --- a/src/app/api/beads/read/route.ts +++ b/src/app/api/beads/read/route.ts @@ -1,10 +1,27 @@ import { NextResponse } from 'next/server'; - +import path from 'node:path'; import { readIssuesFromDisk } from '../../../../lib/read-issues'; +function isValidProjectRoot(root: string): boolean { + try { + const resolved = path.resolve(root); + return path.isAbsolute(resolved); + } catch { + return false; + } +} + export async function GET(request: Request): Promise { const url = new URL(request.url); - const projectRoot = url.searchParams.get('projectRoot') ?? process.cwd(); + const projectRootParam = url.searchParams.get('projectRoot'); + const projectRoot = projectRootParam ?? process.cwd(); + + if (projectRootParam && !isValidProjectRoot(projectRootParam)) { + return NextResponse.json( + { ok: false, error: { classification: 'validation', message: 'Invalid projectRoot path' } }, + { status: 400 } + ); + } try { const issues = await readIssuesFromDisk({ projectRoot, preferBd: true }); diff --git a/src/app/api/events/route.ts b/src/app/api/events/route.ts index f6809d9..f2525fc 100644 --- a/src/app/api/events/route.ts +++ b/src/app/api/events/route.ts @@ -17,6 +17,8 @@ async function readLastTouchedVersion(filePath: string): Promise if ((error as NodeJS.ErrnoException).code === 'ENOENT') { return null; } + // Log non-ENOENT errors but don't swallow them silently + console.error('[Events] Failed to read last-touched version:', error); return null; } } diff --git a/src/app/api/sessions/route.ts b/src/app/api/sessions/route.ts index 0478b46..49c192e 100644 --- a/src/app/api/sessions/route.ts +++ b/src/app/api/sessions/route.ts @@ -3,11 +3,30 @@ import { readIssuesFromDisk } from '../../../lib/read-issues'; 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 = require('path').resolve(root); + return require('path').isAbsolute(resolved); + } catch { + return false; + } +} + export const dynamic = 'force-dynamic'; export async function GET(request: Request): Promise { const url = new URL(request.url); - const projectRoot = url.searchParams.get('projectRoot') ?? process.cwd(); + const projectRootParam = url.searchParams.get('projectRoot'); + const projectRoot = projectRootParam ?? process.cwd(); + + if (projectRootParam && !isValidProjectRoot(projectRoot)) { + return NextResponse.json( + { ok: false, error: { classification: 'validation', message: 'Invalid projectRoot path' } }, + { status: 400 } + ); + } try { const issues = await readIssuesFromDisk({ projectRoot, preferBd: true }); diff --git a/src/lib/agent-mail.ts b/src/lib/agent-mail.ts index 46b4661..7b2363e 100644 --- a/src/lib/agent-mail.ts +++ b/src/lib/agent-mail.ts @@ -98,6 +98,12 @@ function trimOrEmpty(value: unknown): string { return typeof value === 'string' ? value.trim() : ''; } +function isValidMessageId(value: string): boolean { + // Message IDs must be alphanumeric with underscores, hyphens, and colons + // This prevents path traversal attacks + return /^[a-zA-Z0-9_\-:]+$/.test(value); +} + function success(command: MailCommandName, data: T): MailCommandResponse { return { ok: true, @@ -330,6 +336,10 @@ export async function readAgentMessage( return invalid(command, 'MESSAGE_NOT_FOUND', 'Message id is required.'); } + if (!isValidMessageId(messageId)) { + return invalid(command, 'INVALID_MESSAGE_ID', 'Message id contains invalid characters.'); + } + try { const existing = await readMessageIndex(messageId); if (!existing) { @@ -374,6 +384,10 @@ export async function ackAgentMessage( return invalid(command, 'MESSAGE_NOT_FOUND', 'Message id is required.'); } + if (!isValidMessageId(messageId)) { + return invalid(command, 'INVALID_MESSAGE_ID', 'Message id contains invalid characters.'); + } + try { const existing = await readMessageIndex(messageId); if (!existing) { diff --git a/src/lib/realtime.ts b/src/lib/realtime.ts index e500403..92332fe 100644 --- a/src/lib/realtime.ts +++ b/src/lib/realtime.ts @@ -38,7 +38,7 @@ export class IssuesEventBus { private nextSubscriberId = 1; emit(projectRoot: string, changedPath?: string, kind: IssuesChangeKind = 'changed'): IssuesChangedEvent { - console.log(`[IssuesBus] Emitting event: ${kind} for ${projectRoot} (${changedPath})`); + console.log(`[IssuesBus] Emitting event: ${kind} for project (${changedPath ? path.basename(changedPath) : 'unknown'})`); const canonicalProjectRoot = canonicalizeWindowsPath(projectRoot); const projectKey = windowsPathKey(canonicalProjectRoot); const event: IssuesChangedEvent = { @@ -94,6 +94,7 @@ export class ActivityEventBus { private readonly history: ActivityEvent[] = []; private readonly MAX_HISTORY = 100; private initialized = false; + private savePromise: Promise | null = null; private nextSubscriberId = 1; @@ -121,8 +122,22 @@ export class ActivityEventBus { this.history.pop(); } - // Persist async - void saveActivityHistory(this.history); + // Persist async with deduplication - wait for any pending save to complete + const currentHistory = [...this.history]; + const persist = async () => { + try { + await saveActivityHistory(currentHistory); + } catch (error) { + console.error('[ActivityEventBus] Failed to save history:', error); + } + }; + + if (this.savePromise === null) { + this.savePromise = persist(); + } else { + // Chain to existing promise to prevent concurrent writes + this.savePromise = this.savePromise.then(persist); + } for (const subscriber of this.subscribers.values()) { if (!subscriber.projectKey || subscriber.projectKey === projectKey) {