fix: coord review remediations — path traversal, JSON parse, race condition
Addresses code review findings from P2-005:
- Validate projectPath against allowed workspace roots (path traversal)
- Guard JSON.parse with try/catch in loadMission, readActiveSession, readSessionLock
- Add delay after stale lock removal to reduce race window
- Add @Inject(CoordService) per project guideline (no emitDecoratorMetadata)
- Eliminate double loadMission in getTaskStatus via shared buildStatusSummary
- Fix fragile prompt-inclusion check to test original command for {prompt}
- Add mkdir to writeAtomic for consistency with other atomic helpers
Closes #80
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
This commit is contained in:
@@ -1,15 +1,39 @@
|
|||||||
import { Controller, Get, NotFoundException, Param, Query, UseGuards } from '@nestjs/common';
|
import {
|
||||||
|
BadRequestException,
|
||||||
|
Controller,
|
||||||
|
Get,
|
||||||
|
Inject,
|
||||||
|
NotFoundException,
|
||||||
|
Param,
|
||||||
|
Query,
|
||||||
|
UseGuards,
|
||||||
|
} from '@nestjs/common';
|
||||||
|
import path from 'node:path';
|
||||||
import { AuthGuard } from '../auth/auth.guard.js';
|
import { AuthGuard } from '../auth/auth.guard.js';
|
||||||
import { CoordService } from './coord.service.js';
|
import { CoordService } from './coord.service.js';
|
||||||
|
|
||||||
|
/** Only paths under these roots are allowed for coord queries. */
|
||||||
|
const ALLOWED_ROOTS = [process.cwd()];
|
||||||
|
|
||||||
|
function resolveAndValidatePath(raw: string | undefined): string {
|
||||||
|
const resolved = path.resolve(raw ?? process.cwd());
|
||||||
|
const isAllowed = ALLOWED_ROOTS.some(
|
||||||
|
(root) => resolved === root || resolved.startsWith(`${root}/`),
|
||||||
|
);
|
||||||
|
if (!isAllowed) {
|
||||||
|
throw new BadRequestException('projectPath is outside the allowed workspace');
|
||||||
|
}
|
||||||
|
return resolved;
|
||||||
|
}
|
||||||
|
|
||||||
@Controller('api/coord')
|
@Controller('api/coord')
|
||||||
@UseGuards(AuthGuard)
|
@UseGuards(AuthGuard)
|
||||||
export class CoordController {
|
export class CoordController {
|
||||||
constructor(private readonly coordService: CoordService) {}
|
constructor(@Inject(CoordService) private readonly coordService: CoordService) {}
|
||||||
|
|
||||||
@Get('status')
|
@Get('status')
|
||||||
async missionStatus(@Query('projectPath') projectPath?: string) {
|
async missionStatus(@Query('projectPath') projectPath?: string) {
|
||||||
const resolvedPath = projectPath ?? process.cwd();
|
const resolvedPath = resolveAndValidatePath(projectPath);
|
||||||
const status = await this.coordService.getMissionStatus(resolvedPath);
|
const status = await this.coordService.getMissionStatus(resolvedPath);
|
||||||
if (!status) throw new NotFoundException('No active coord mission found');
|
if (!status) throw new NotFoundException('No active coord mission found');
|
||||||
return status;
|
return status;
|
||||||
@@ -17,13 +41,13 @@ export class CoordController {
|
|||||||
|
|
||||||
@Get('tasks')
|
@Get('tasks')
|
||||||
async listTasks(@Query('projectPath') projectPath?: string) {
|
async listTasks(@Query('projectPath') projectPath?: string) {
|
||||||
const resolvedPath = projectPath ?? process.cwd();
|
const resolvedPath = resolveAndValidatePath(projectPath);
|
||||||
return this.coordService.listTasks(resolvedPath);
|
return this.coordService.listTasks(resolvedPath);
|
||||||
}
|
}
|
||||||
|
|
||||||
@Get('tasks/:taskId')
|
@Get('tasks/:taskId')
|
||||||
async taskStatus(@Param('taskId') taskId: string, @Query('projectPath') projectPath?: string) {
|
async taskStatus(@Param('taskId') taskId: string, @Query('projectPath') projectPath?: string) {
|
||||||
const resolvedPath = projectPath ?? process.cwd();
|
const resolvedPath = resolveAndValidatePath(projectPath);
|
||||||
const detail = await this.coordService.getTaskStatus(resolvedPath, taskId);
|
const detail = await this.coordService.getTaskStatus(resolvedPath, taskId);
|
||||||
if (!detail) throw new NotFoundException(`Task ${taskId} not found in coord mission`);
|
if (!detail) throw new NotFoundException(`Task ${taskId} not found in coord mission`);
|
||||||
return detail;
|
return detail;
|
||||||
|
|||||||
@@ -379,7 +379,14 @@ export async function loadMission(projectPath: string): Promise<Mission> {
|
|||||||
throw error;
|
throw error;
|
||||||
}
|
}
|
||||||
|
|
||||||
const mission = normalizeMission(JSON.parse(raw), resolvedProjectPath);
|
let parsed: unknown;
|
||||||
|
try {
|
||||||
|
parsed = JSON.parse(raw);
|
||||||
|
} catch {
|
||||||
|
throw new Error(`Invalid JSON in mission file: ${filePath}`);
|
||||||
|
}
|
||||||
|
|
||||||
|
const mission = normalizeMission(parsed, resolvedProjectPath);
|
||||||
if (mission.status === 'inactive') {
|
if (mission.status === 'inactive') {
|
||||||
throw new Error('Mission exists but is inactive. Re-initialize with mosaic coord init.');
|
throw new Error('Mission exists but is inactive. Re-initialize with mosaic coord init.');
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -195,11 +195,12 @@ function resolveLaunchCommand(
|
|||||||
return [runtime, '-p', prompt];
|
return [runtime, '-p', prompt];
|
||||||
}
|
}
|
||||||
|
|
||||||
|
const hasPromptPlaceholder = configuredCommand.some((value) => value === '{prompt}');
|
||||||
const withInterpolation = configuredCommand.map((value) =>
|
const withInterpolation = configuredCommand.map((value) =>
|
||||||
value === '{prompt}' ? prompt : value,
|
value === '{prompt}' ? prompt : value,
|
||||||
);
|
);
|
||||||
|
|
||||||
if (withInterpolation.includes(prompt)) {
|
if (hasPromptPlaceholder) {
|
||||||
return withInterpolation;
|
return withInterpolation;
|
||||||
}
|
}
|
||||||
|
|
||||||
@@ -224,28 +225,9 @@ async function writeAtomicJson(filePath: string, payload: unknown): Promise<void
|
|||||||
async function readSessionLock(mission: Mission): Promise<SessionLockState | undefined> {
|
async function readSessionLock(mission: Mission): Promise<SessionLockState | undefined> {
|
||||||
const filePath = sessionLockPath(mission);
|
const filePath = sessionLockPath(mission);
|
||||||
|
|
||||||
|
let raw: string;
|
||||||
try {
|
try {
|
||||||
const raw = await fs.readFile(filePath, 'utf8');
|
raw = await fs.readFile(filePath, 'utf8');
|
||||||
const data = JSON.parse(raw) as Partial<SessionLockState>;
|
|
||||||
|
|
||||||
if (
|
|
||||||
typeof data.session_id !== 'string' ||
|
|
||||||
(data.runtime !== 'claude' && data.runtime !== 'codex') ||
|
|
||||||
typeof data.pid !== 'number' ||
|
|
||||||
typeof data.started_at !== 'string' ||
|
|
||||||
typeof data.project_path !== 'string'
|
|
||||||
) {
|
|
||||||
return undefined;
|
|
||||||
}
|
|
||||||
|
|
||||||
return {
|
|
||||||
session_id: data.session_id,
|
|
||||||
runtime: data.runtime,
|
|
||||||
pid: data.pid,
|
|
||||||
started_at: data.started_at,
|
|
||||||
project_path: data.project_path,
|
|
||||||
milestone_id: data.milestone_id,
|
|
||||||
};
|
|
||||||
} catch (error) {
|
} catch (error) {
|
||||||
if (
|
if (
|
||||||
typeof error === 'object' &&
|
typeof error === 'object' &&
|
||||||
@@ -257,6 +239,32 @@ async function readSessionLock(mission: Mission): Promise<SessionLockState | und
|
|||||||
}
|
}
|
||||||
throw error;
|
throw error;
|
||||||
}
|
}
|
||||||
|
|
||||||
|
let data: Partial<SessionLockState>;
|
||||||
|
try {
|
||||||
|
data = JSON.parse(raw) as Partial<SessionLockState>;
|
||||||
|
} catch {
|
||||||
|
return undefined;
|
||||||
|
}
|
||||||
|
|
||||||
|
if (
|
||||||
|
typeof data.session_id !== 'string' ||
|
||||||
|
(data.runtime !== 'claude' && data.runtime !== 'codex') ||
|
||||||
|
typeof data.pid !== 'number' ||
|
||||||
|
typeof data.started_at !== 'string' ||
|
||||||
|
typeof data.project_path !== 'string'
|
||||||
|
) {
|
||||||
|
return undefined;
|
||||||
|
}
|
||||||
|
|
||||||
|
return {
|
||||||
|
session_id: data.session_id,
|
||||||
|
runtime: data.runtime,
|
||||||
|
pid: data.pid,
|
||||||
|
started_at: data.started_at,
|
||||||
|
project_path: data.project_path,
|
||||||
|
milestone_id: data.milestone_id,
|
||||||
|
};
|
||||||
}
|
}
|
||||||
|
|
||||||
async function writeSessionLock(mission: Mission, lock: SessionLockState): Promise<void> {
|
async function writeSessionLock(mission: Mission, lock: SessionLockState): Promise<void> {
|
||||||
|
|||||||
@@ -78,7 +78,12 @@ async function readActiveSession(mission: Mission): Promise<MissionSession | und
|
|||||||
throw error;
|
throw error;
|
||||||
}
|
}
|
||||||
|
|
||||||
const lock = JSON.parse(lockRaw) as SessionLockState;
|
let lock: SessionLockState;
|
||||||
|
try {
|
||||||
|
lock = JSON.parse(lockRaw) as SessionLockState;
|
||||||
|
} catch {
|
||||||
|
return undefined;
|
||||||
|
}
|
||||||
if (
|
if (
|
||||||
typeof lock.session_id !== 'string' ||
|
typeof lock.session_id !== 'string' ||
|
||||||
(lock.runtime !== 'claude' && lock.runtime !== 'codex') ||
|
(lock.runtime !== 'claude' && lock.runtime !== 'codex') ||
|
||||||
@@ -106,10 +111,10 @@ async function readActiveSession(mission: Mission): Promise<MissionSession | und
|
|||||||
};
|
};
|
||||||
}
|
}
|
||||||
|
|
||||||
export async function getMissionStatus(mission: Mission): Promise<MissionStatusSummary> {
|
async function buildStatusSummary(
|
||||||
const freshMission = await loadMission(mission.projectPath);
|
freshMission: Mission,
|
||||||
const tasks = await readTasks(freshMission);
|
tasks: MissionTask[],
|
||||||
|
): Promise<MissionStatusSummary> {
|
||||||
const done = tasks.filter((task) => task.status === 'done').length;
|
const done = tasks.filter((task) => task.status === 'done').length;
|
||||||
const inProgress = tasks.filter((task) => task.status === 'in-progress').length;
|
const inProgress = tasks.filter((task) => task.status === 'in-progress').length;
|
||||||
const pending = tasks.filter((task) => task.status === 'not-started').length;
|
const pending = tasks.filter((task) => task.status === 'not-started').length;
|
||||||
@@ -151,6 +156,12 @@ export async function getMissionStatus(mission: Mission): Promise<MissionStatusS
|
|||||||
};
|
};
|
||||||
}
|
}
|
||||||
|
|
||||||
|
export async function getMissionStatus(mission: Mission): Promise<MissionStatusSummary> {
|
||||||
|
const freshMission = await loadMission(mission.projectPath);
|
||||||
|
const tasks = await readTasks(freshMission);
|
||||||
|
return buildStatusSummary(freshMission, tasks);
|
||||||
|
}
|
||||||
|
|
||||||
export async function getTaskStatus(mission: Mission, taskId: string): Promise<TaskDetail> {
|
export async function getTaskStatus(mission: Mission, taskId: string): Promise<TaskDetail> {
|
||||||
const freshMission = await loadMission(mission.projectPath);
|
const freshMission = await loadMission(mission.projectPath);
|
||||||
const tasks = await readTasks(freshMission);
|
const tasks = await readTasks(freshMission);
|
||||||
@@ -164,7 +175,7 @@ export async function getTaskStatus(mission: Mission, taskId: string): Promise<T
|
|||||||
throw new Error(`Duplicate task IDs found: ${taskId}`);
|
throw new Error(`Duplicate task IDs found: ${taskId}`);
|
||||||
}
|
}
|
||||||
|
|
||||||
const summary = await getMissionStatus(freshMission);
|
const summary = await buildStatusSummary(freshMission, tasks);
|
||||||
|
|
||||||
return {
|
return {
|
||||||
missionId: freshMission.id,
|
missionId: freshMission.id,
|
||||||
|
|||||||
@@ -219,6 +219,7 @@ async function acquireLock(lockPath: string): Promise<void> {
|
|||||||
const stats = await fs.stat(lockPath);
|
const stats = await fs.stat(lockPath);
|
||||||
if (Date.now() - stats.mtimeMs > TASKS_LOCK_STALE_MS) {
|
if (Date.now() - stats.mtimeMs > TASKS_LOCK_STALE_MS) {
|
||||||
await fs.rm(lockPath, { force: true });
|
await fs.rm(lockPath, { force: true });
|
||||||
|
await delay(TASKS_LOCK_RETRY_MS);
|
||||||
continue;
|
continue;
|
||||||
}
|
}
|
||||||
} catch (statError) {
|
} catch (statError) {
|
||||||
@@ -240,6 +241,7 @@ async function releaseLock(lockPath: string): Promise<void> {
|
|||||||
|
|
||||||
async function writeAtomic(filePath: string, content: string): Promise<void> {
|
async function writeAtomic(filePath: string, content: string): Promise<void> {
|
||||||
const directory = path.dirname(filePath);
|
const directory = path.dirname(filePath);
|
||||||
|
await fs.mkdir(directory, { recursive: true });
|
||||||
const tempPath = path.join(
|
const tempPath = path.join(
|
||||||
directory,
|
directory,
|
||||||
`.TASKS.md.tmp-${process.pid}-${Date.now()}-${Math.random().toString(16).slice(2)}`,
|
`.TASKS.md.tmp-${process.pid}-${Date.now()}-${Math.random().toString(16).slice(2)}`,
|
||||||
|
|||||||
Reference in New Issue
Block a user