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 <openhands@all-hands.dev>
This commit is contained in:
parent
d1140c9809
commit
a3f2ceef52
8 changed files with 108 additions and 9 deletions
|
|
@ -9,7 +9,7 @@
|
||||||
"start": "next start",
|
"start": "next start",
|
||||||
"lint": "eslint .",
|
"lint": "eslint .",
|
||||||
"typecheck": "tsc --noEmit",
|
"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": {
|
"dependencies": {
|
||||||
"@xyflow/react": "^12.10.0",
|
"@xyflow/react": "^12.10.0",
|
||||||
|
|
|
||||||
|
|
@ -1,9 +1,26 @@
|
||||||
import { activityEventBus } from '../../../lib/realtime';
|
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<Response> {
|
export async function GET(request: Request): Promise<Response> {
|
||||||
const url = new URL(request.url);
|
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);
|
const history = activityEventBus.getHistory(projectRoot);
|
||||||
|
|
||||||
return Response.json(history);
|
return Response.json(history);
|
||||||
|
|
|
||||||
|
|
@ -1,15 +1,30 @@
|
||||||
import { NextResponse } from 'next/server';
|
import { NextResponse } from 'next/server';
|
||||||
|
import path from 'node:path';
|
||||||
import { readIssuesFromDisk } from '../../../../../lib/read-issues';
|
import { readIssuesFromDisk } from '../../../../../lib/read-issues';
|
||||||
import { activityEventBus } from '../../../../../lib/realtime';
|
import { activityEventBus } from '../../../../../lib/realtime';
|
||||||
import { getAgentMetrics } from '../../../../../lib/agent-sessions';
|
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(
|
export async function GET(
|
||||||
request: Request,
|
request: Request,
|
||||||
{ params }: { params: Promise<{ agentId: string }> }
|
{ params }: { params: Promise<{ agentId: string }> }
|
||||||
): Promise<Response> {
|
): Promise<Response> {
|
||||||
const { agentId } = await params;
|
const { agentId } = await params;
|
||||||
const url = new URL(request.url);
|
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 {
|
try {
|
||||||
const issues = await readIssuesFromDisk({ projectRoot, preferBd: true });
|
const issues = await readIssuesFromDisk({ projectRoot, preferBd: true });
|
||||||
|
|
|
||||||
|
|
@ -1,10 +1,27 @@
|
||||||
import { NextResponse } from 'next/server';
|
import { NextResponse } from 'next/server';
|
||||||
|
import path from 'node:path';
|
||||||
import { readIssuesFromDisk } from '../../../../lib/read-issues';
|
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<Response> {
|
export async function GET(request: Request): Promise<Response> {
|
||||||
const url = new URL(request.url);
|
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 {
|
try {
|
||||||
const issues = await readIssuesFromDisk({ projectRoot, preferBd: true });
|
const issues = await readIssuesFromDisk({ projectRoot, preferBd: true });
|
||||||
|
|
|
||||||
|
|
@ -17,6 +17,8 @@ async function readLastTouchedVersion(filePath: string): Promise<number | null>
|
||||||
if ((error as NodeJS.ErrnoException).code === 'ENOENT') {
|
if ((error as NodeJS.ErrnoException).code === 'ENOENT') {
|
||||||
return null;
|
return null;
|
||||||
}
|
}
|
||||||
|
// Log non-ENOENT errors but don't swallow them silently
|
||||||
|
console.error('[Events] Failed to read last-touched version:', error);
|
||||||
return null;
|
return null;
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
|
||||||
|
|
@ -3,11 +3,30 @@ import { readIssuesFromDisk } from '../../../lib/read-issues';
|
||||||
import { activityEventBus } from '../../../lib/realtime';
|
import { activityEventBus } from '../../../lib/realtime';
|
||||||
import { buildSessionTaskFeed, getCommunicationSummary } from '../../../lib/agent-sessions';
|
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 const dynamic = 'force-dynamic';
|
||||||
|
|
||||||
export async function GET(request: Request): Promise<Response> {
|
export async function GET(request: Request): Promise<Response> {
|
||||||
const url = new URL(request.url);
|
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 {
|
try {
|
||||||
const issues = await readIssuesFromDisk({ projectRoot, preferBd: true });
|
const issues = await readIssuesFromDisk({ projectRoot, preferBd: true });
|
||||||
|
|
|
||||||
|
|
@ -98,6 +98,12 @@ function trimOrEmpty(value: unknown): string {
|
||||||
return typeof value === 'string' ? value.trim() : '';
|
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<T>(command: MailCommandName, data: T): MailCommandResponse<T> {
|
function success<T>(command: MailCommandName, data: T): MailCommandResponse<T> {
|
||||||
return {
|
return {
|
||||||
ok: true,
|
ok: true,
|
||||||
|
|
@ -330,6 +336,10 @@ export async function readAgentMessage(
|
||||||
return invalid(command, 'MESSAGE_NOT_FOUND', 'Message id is required.');
|
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 {
|
try {
|
||||||
const existing = await readMessageIndex(messageId);
|
const existing = await readMessageIndex(messageId);
|
||||||
if (!existing) {
|
if (!existing) {
|
||||||
|
|
@ -374,6 +384,10 @@ export async function ackAgentMessage(
|
||||||
return invalid(command, 'MESSAGE_NOT_FOUND', 'Message id is required.');
|
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 {
|
try {
|
||||||
const existing = await readMessageIndex(messageId);
|
const existing = await readMessageIndex(messageId);
|
||||||
if (!existing) {
|
if (!existing) {
|
||||||
|
|
|
||||||
|
|
@ -38,7 +38,7 @@ export class IssuesEventBus {
|
||||||
private nextSubscriberId = 1;
|
private nextSubscriberId = 1;
|
||||||
|
|
||||||
emit(projectRoot: string, changedPath?: string, kind: IssuesChangeKind = 'changed'): IssuesChangedEvent {
|
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 canonicalProjectRoot = canonicalizeWindowsPath(projectRoot);
|
||||||
const projectKey = windowsPathKey(canonicalProjectRoot);
|
const projectKey = windowsPathKey(canonicalProjectRoot);
|
||||||
const event: IssuesChangedEvent = {
|
const event: IssuesChangedEvent = {
|
||||||
|
|
@ -94,6 +94,7 @@ export class ActivityEventBus {
|
||||||
private readonly history: ActivityEvent[] = [];
|
private readonly history: ActivityEvent[] = [];
|
||||||
private readonly MAX_HISTORY = 100;
|
private readonly MAX_HISTORY = 100;
|
||||||
private initialized = false;
|
private initialized = false;
|
||||||
|
private savePromise: Promise<void> | null = null;
|
||||||
|
|
||||||
private nextSubscriberId = 1;
|
private nextSubscriberId = 1;
|
||||||
|
|
||||||
|
|
@ -121,8 +122,22 @@ export class ActivityEventBus {
|
||||||
this.history.pop();
|
this.history.pop();
|
||||||
}
|
}
|
||||||
|
|
||||||
// Persist async
|
// Persist async with deduplication - wait for any pending save to complete
|
||||||
void saveActivityHistory(this.history);
|
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()) {
|
for (const subscriber of this.subscribers.values()) {
|
||||||
if (!subscriber.projectKey || subscriber.projectKey === projectKey) {
|
if (!subscriber.projectKey || subscriber.projectKey === projectKey) {
|
||||||
|
|
|
||||||
Loading…
Add table
Add a link
Reference in a new issue