From d094d9ec5037796d3b30a12f782fc123d0e0540a Mon Sep 17 00:00:00 2001 From: arkon Date: Sun, 1 Mar 2026 17:26:31 +0100 Subject: [PATCH] =?UTF-8?q?fix:=20code=20cleanup=20=E2=80=94=20path=20trav?= =?UTF-8?q?ersal,=20test=20leaks,=20dead=20code,=20consistency?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Add path traversal protection to GET /api/cases/:name and fix-plan - Use safePathSchema for LinkCaseSchema.path - Fix QR auth test timer leak (afterAll → afterEach) and env var try/finally - Remove dead terminal size check after Zod validation in resize route - Remove no-op sampleCount guard in adaptive timing - Replace hardcoded values with constants in notification-manager and subagent-windows - Add Zod validation to POST /api/auth/revoke - Use _apiPut instead of raw fetch in subagent-windows - Add SwipeHandler.cleanup() for consistency with other mobile handlers - Move NiceConfig/ProcessStats from types/plan.ts to types/common.ts Co-Authored-By: Claude Opus 4.6 --- src/respawn-adaptive-timing.ts | 5 ----- src/types/common.ts | 30 ++++++++++++++++++++++++++ src/types/plan.ts | 30 -------------------------- src/web/public/mobile-handlers.js | 22 +++++++++++++++++-- src/web/public/notification-manager.js | 12 +++++------ src/web/public/subagent-windows.js | 10 +++------ src/web/routes/case-routes.ts | 16 ++++++++++++++ src/web/routes/session-routes.ts | 10 --------- src/web/routes/system-routes.ts | 7 +++--- src/web/schemas.ts | 7 +++++- test/qr-auth.test.ts | 16 ++++++++------ 11 files changed, 94 insertions(+), 71 deletions(-) diff --git a/src/respawn-adaptive-timing.ts b/src/respawn-adaptive-timing.ts index 1c1eef1e..e03ab205 100644 --- a/src/respawn-adaptive-timing.ts +++ b/src/respawn-adaptive-timing.ts @@ -76,11 +76,6 @@ export class RespawnAdaptiveTiming { * @returns Completion confirm timeout in milliseconds */ getAdaptiveCompletionConfirmMs(): number { - // Need at least 5 samples before adjusting - if (this.timingHistory.sampleCount < 5) { - return this.timingHistory.adaptiveCompletionConfirmMs; - } - return this.timingHistory.adaptiveCompletionConfirmMs; } diff --git a/src/types/common.ts b/src/types/common.ts index fec95849..e7b94d06 100644 --- a/src/types/common.ts +++ b/src/types/common.ts @@ -29,6 +29,36 @@ export interface BufferConfig { /** * Resource types that can be registered for cleanup. */ +/** + * Configuration for process priority using `nice`. + * Lower priority reduces CPU contention with other processes. + */ +export interface NiceConfig { + /** Whether nice priority is enabled */ + enabled: boolean; + /** Nice value (-20 to 19, default: 10 = lower priority) */ + niceValue: number; +} + +export const DEFAULT_NICE_CONFIG: NiceConfig = { + enabled: false, + niceValue: 10, +}; + +/** + * Process resource statistics + */ +export interface ProcessStats { + /** Memory usage in megabytes */ + memoryMB: number; + /** CPU usage percentage */ + cpuPercent: number; + /** Number of child processes */ + childCount: number; + /** Timestamp of stats collection */ + updatedAt: number; +} + export type CleanupResourceType = 'timer' | 'interval' | 'watcher' | 'listener' | 'stream'; /** diff --git a/src/types/plan.ts b/src/types/plan.ts index 68dda632..38fd2e0b 100644 --- a/src/types/plan.ts +++ b/src/types/plan.ts @@ -11,36 +11,6 @@ export type TddPhase = 'setup' | 'test' | 'impl' | 'verify' | 'review'; /** Development phase in TDD cycle (alias for TddPhase) */ export type PlanPhase = TddPhase; -/** - * Configuration for process priority using `nice`. - * Lower priority reduces CPU contention with other processes. - */ -export interface NiceConfig { - /** Whether nice priority is enabled */ - enabled: boolean; - /** Nice value (-20 to 19, default: 10 = lower priority) */ - niceValue: number; -} - -export const DEFAULT_NICE_CONFIG: NiceConfig = { - enabled: false, - niceValue: 10, -}; - -/** - * Process resource statistics - */ -export interface ProcessStats { - /** Memory usage in megabytes */ - memoryMB: number; - /** CPU usage percentage */ - cpuPercent: number; - /** Number of child processes */ - childCount: number; - /** Timestamp of stats collection */ - updatedAt: number; -} - /** * A single plan item for plan orchestration. * Moved here from plan-orchestrator.ts to break circular dependency. diff --git a/src/web/public/mobile-handlers.js b/src/web/public/mobile-handlers.js index f0e7f195..64b98383 100644 --- a/src/web/public/mobile-handlers.js +++ b/src/web/public/mobile-handlers.js @@ -403,6 +403,10 @@ const SwipeHandler = { maxSwipeTime: 300, // Maximum ms for a swipe gesture maxVerticalDrift: 100, // Max vertical movement allowed + _touchStartHandler: null, + _touchEndHandler: null, + _element: null, + /** Initialize swipe handling */ init() { // Only on touch devices @@ -411,8 +415,22 @@ const SwipeHandler = { const terminal = document.querySelector('.main'); if (!terminal) return; - terminal.addEventListener('touchstart', (e) => this.onTouchStart(e), { passive: true }); - terminal.addEventListener('touchend', (e) => this.onTouchEnd(e), { passive: true }); + this._element = terminal; + this._touchStartHandler = (e) => this.onTouchStart(e); + this._touchEndHandler = (e) => this.onTouchEnd(e); + terminal.addEventListener('touchstart', this._touchStartHandler, { passive: true }); + terminal.addEventListener('touchend', this._touchEndHandler, { passive: true }); + }, + + /** Remove swipe listeners */ + cleanup() { + if (this._element && this._touchStartHandler) { + this._element.removeEventListener('touchstart', this._touchStartHandler); + this._element.removeEventListener('touchend', this._touchEndHandler); + } + this._touchStartHandler = null; + this._touchEndHandler = null; + this._element = null; }, onTouchStart(e) { diff --git a/src/web/public/notification-manager.js b/src/web/public/notification-manager.js index aa29b1e2..3377e688 100644 --- a/src/web/public/notification-manager.js +++ b/src/web/public/notification-manager.js @@ -188,9 +188,9 @@ class NotificationManager { count: 1, }; - // Add to log (cap at 100) + // Add to log (cap at NOTIFICATION_LIST_CAP) this.notifications.unshift(notification); - if (this.notifications.length > 100) this.notifications.pop(); + if (this.notifications.length > NOTIFICATION_LIST_CAP) this.notifications.pop(); // Track for grouping const timeout = setTimeout(() => this.groupingMap.delete(groupKey), GROUPING_TIMEOUT_MS); @@ -298,9 +298,9 @@ class NotificationManager { } if (Notification.permission !== 'granted') return; - // Rate limit: max 1 per 3 seconds + // Rate limit const now = Date.now(); - if (now - this.lastBrowserNotifTime < 3000) return; + if (now - this.lastBrowserNotifTime < BROWSER_NOTIF_RATE_LIMIT_MS) return; this.lastBrowserNotifTime = now; const notif = new Notification(`Codeman: ${title}`, { @@ -318,8 +318,8 @@ class NotificationManager { notif.close(); }; - // Auto-close after 8s - setTimeout(() => notif.close(), 8000); + // Auto-close + setTimeout(() => notif.close(), AUTO_CLOSE_NOTIFICATION_MS); } async requestPermission() { diff --git a/src/web/public/subagent-windows.js b/src/web/public/subagent-windows.js index 797ad482..1bf4d6df 100644 --- a/src/web/public/subagent-windows.js +++ b/src/web/public/subagent-windows.js @@ -111,11 +111,7 @@ Object.assign(CodemanApp.prototype, { // Save to server for cross-browser persistence try { - await fetch('/api/subagent-window-states', { - method: 'PUT', - headers: { 'Content-Type': 'application/json' }, - body: JSON.stringify(windowStates) - }); + await this._apiPut('/api/subagent-window-states', windowStates); } catch (err) { console.error('Failed to save subagent window states to server:', err); } @@ -178,7 +174,7 @@ Object.assign(CodemanApp.prototype, { if (windowData && windowData.element) { // Parse position values and clamp to viewport let left = parseInt(position.left, 10) || 50; - let top = parseInt(position.top, 10) || 120; + let top = parseInt(position.top, 10) || WINDOW_INITIAL_TOP_PX; const viewportWidth = window.innerWidth; const viewportHeight = window.innerHeight; const windowWidth = 420; @@ -545,7 +541,7 @@ Object.assign(CodemanApp.prototype, { } else { // Normal positioning startX = 50; - startY = 120; + startY = WINDOW_INITIAL_TOP_PX; maxCols = Math.floor((viewportWidth - startX - 50) / (windowWidth + gap)) || 1; } diff --git a/src/web/routes/case-routes.ts b/src/web/routes/case-routes.ts index b461c576..21ca881a 100644 --- a/src/web/routes/case-routes.ts +++ b/src/web/routes/case-routes.ts @@ -162,6 +162,14 @@ export function registerCaseRoutes(app: FastifyInstance, ctx: EventPort & Config app.get('/api/cases/:name', async (req) => { const { name } = req.params as { name: string }; + // Security: Path traversal protection + const resolvedPath = resolve(join(CASES_DIR, name)); + const resolvedBase = resolve(CASES_DIR); + const relPath = relative(resolvedBase, resolvedPath); + if (relPath.startsWith('..') || isAbsolute(relPath)) { + return createErrorResponse(ApiErrorCode.INVALID_INPUT, 'Invalid case name'); + } + // First check linked cases const linkedCasesFile = join(homedir(), '.codeman', 'linked-cases.json'); try { @@ -197,6 +205,14 @@ export function registerCaseRoutes(app: FastifyInstance, ctx: EventPort & Config app.get('/api/cases/:name/fix-plan', async (req) => { const { name } = req.params as { name: string }; + // Security: Path traversal protection + const resolvedPath = resolve(join(CASES_DIR, name)); + const resolvedBase = resolve(CASES_DIR); + const relPath = relative(resolvedBase, resolvedPath); + if (relPath.startsWith('..') || isAbsolute(relPath)) { + return createErrorResponse(ApiErrorCode.INVALID_INPUT, 'Invalid case name'); + } + // Get case path (check linked cases first, then CASES_DIR) let casePath: string | null = null; diff --git a/src/web/routes/session-routes.ts b/src/web/routes/session-routes.ts index ef31251e..d006ff0a 100644 --- a/src/web/routes/session-routes.ts +++ b/src/web/routes/session-routes.ts @@ -43,8 +43,6 @@ import { RunSummaryTracker } from '../../run-summary.js'; import { MAX_INPUT_LENGTH, - MAX_TERMINAL_COLS, - MAX_TERMINAL_ROWS, MAX_SESSION_NAME_LENGTH, } from '../../config/terminal-limits.js'; @@ -495,14 +493,6 @@ export function registerSessionRoutes( return createErrorResponse(ApiErrorCode.NOT_FOUND, 'Session not found'); } - // Note: Zod already validates that cols and rows are positive integers within bounds - if (cols > MAX_TERMINAL_COLS || rows > MAX_TERMINAL_ROWS) { - return createErrorResponse( - ApiErrorCode.INVALID_INPUT, - `Terminal dimensions exceed maximum (${MAX_TERMINAL_COLS}x${MAX_TERMINAL_ROWS})` - ); - } - session.resize(cols, rows); return { success: true }; }); diff --git a/src/web/routes/system-routes.ts b/src/web/routes/system-routes.ts index ee890d02..56ab3d20 100644 --- a/src/web/routes/system-routes.ts +++ b/src/web/routes/system-routes.ts @@ -19,6 +19,7 @@ import { CpuLimitSchema, SubagentWindowStatesSchema, SubagentParentMapSchema, + RevokeSessionSchema, } from '../schemas.js'; import { subagentWatcher } from '../../subagent-watcher.js'; import { imageWatcher } from '../../image-watcher.js'; @@ -194,9 +195,9 @@ export function registerSystemRoutes( // ========== Auth Session Revocation ========== app.post('/api/auth/revoke', async (req) => { - const body = req.body as { sessionToken?: string } | undefined; - if (body?.sessionToken) { - ctx.authSessions?.delete(body.sessionToken); + const result = RevokeSessionSchema.safeParse(req.body); + if (result.success && result.data.sessionToken) { + ctx.authSessions?.delete(result.data.sessionToken); } else { // Revoke all sessions (nuclear option) ctx.authSessions?.clear(); diff --git a/src/web/schemas.ts b/src/web/schemas.ts index 76997b2f..0cd8e588 100644 --- a/src/web/schemas.ts +++ b/src/web/schemas.ts @@ -396,7 +396,12 @@ export const ScheduledRunSchema = z.object({ /** POST /api/cases/link */ export const LinkCaseSchema = z.object({ name: z.string().regex(/^[a-zA-Z0-9_-]+$/, 'Invalid case name format'), - path: z.string().min(1).max(1000), + path: safePathSchema, +}); + +/** POST /api/auth/revoke */ +export const RevokeSessionSchema = z.object({ + sessionToken: z.string().min(1).max(200).optional(), }); /** POST /api/generate-plan */ diff --git a/test/qr-auth.test.ts b/test/qr-auth.test.ts index 9f72f637..2cc0864d 100644 --- a/test/qr-auth.test.ts +++ b/test/qr-auth.test.ts @@ -38,8 +38,8 @@ describe('QR Token Manager (unit)', () => { tm.startTokenRotation(); }); - afterAll(() => { - // Clean up any lingering timers + afterEach(() => { + // Clean up rotation timer for this test's TunnelManager tm?.stopTokenRotation(); }); @@ -234,11 +234,13 @@ describe('QR Auth Integration', () => { const savedPass = process.env.CODEMAN_PASSWORD; delete process.env.CODEMAN_PASSWORD; - const res = await fetch(`${baseUrl}/q/ANYCODE`, { redirect: 'manual' }); - expect(res.status).toBe(302); - expect(res.headers.get('location')).toBe('/'); - - process.env.CODEMAN_PASSWORD = savedPass; + try { + const res = await fetch(`${baseUrl}/q/ANYCODE`, { redirect: 'manual' }); + expect(res.status).toBe(302); + expect(res.headers.get('location')).toBe('/'); + } finally { + process.env.CODEMAN_PASSWORD = savedPass; + } }); it('GET /api/tunnel/qr should return authEnabled flag', async () => {