diff --git a/src/config/map-limits.ts b/src/config/map-limits.ts index d67082b4..b8fc5529 100644 --- a/src/config/map-limits.ts +++ b/src/config/map-limits.ts @@ -43,6 +43,13 @@ export const MAX_SSE_CLIENTS = 100; */ export const MAX_TODOS_PER_SESSION = 500; +/** + * Maximum cron-job run-history records retained across all jobs. Oldest runs + * (by startedAt) are pruned when exceeded — bounds state.json growth from + * frequently-firing or perpetually-skipped jobs. + */ +export const MAX_CRON_RUN_HISTORY = 500; + // ============================================================================ // Pending Tool Calls Limits // ============================================================================ diff --git a/src/cron/cron-service.ts b/src/cron/cron-service.ts index e2b19464..e274ee2b 100644 --- a/src/cron/cron-service.ts +++ b/src/cron/cron-service.ts @@ -14,7 +14,7 @@ import { Session } from '../session.js'; import { SseEvent } from '../web/sse-events.js'; import { CronJobSchema } from '../web/schemas.js'; import { getErrorMessage, createErrorResponse, ApiErrorCode } from '../types/api.js'; -import { MAX_CONCURRENT_SESSIONS } from '../config/map-limits.js'; +import { MAX_CONCURRENT_SESSIONS, MAX_CRON_RUN_HISTORY } from '../config/map-limits.js'; import { CRON_READY_MAX_ATTEMPTS, CRON_READY_SETTLE_MS } from '../config/server-timing.js'; import { isBlockedAttachmentPath, loadAttachmentGuardConfig } from '../config/attachment-guard.js'; import { validateSessionFilePath } from '../web/route-helpers.js'; @@ -28,6 +28,16 @@ export type CronDeps = SessionPort & EventPort & ConfigPort & InfraPort; const delay = (ms: number): Promise => new Promise((r) => setTimeout(r, ms)); +/** Hard ceiling on a prompt-file read (defends against unbounded-read DoS). */ +const MAX_PROMPT_FILE_BYTES = 1024 * 1024; + +/** Order-insensitive equality for the weekly-days arrays. */ +function sameDays(a: number[] | undefined, b: number[] | undefined): boolean { + const x = [...(a ?? [])].sort((p, q) => p - q); + const y = [...(b ?? [])].sort((p, q) => p - q); + return x.length === y.length && x.every((v, i) => v === y[i]); +} + export class CronService { constructor(private readonly deps: CronDeps) {} @@ -99,17 +109,21 @@ export class CronService { if (!existing) return null; const now = Date.now(); - // A completed one-time job is only re-armed when the SCHEDULE itself is - // edited — otherwise a cosmetic edit (rename, notes) would silently - // resurrect a job that already fired and run it again. - const scheduleTouched = - patch.scheduleType !== undefined || - patch.runAt !== undefined || - patch.intervalMinutes !== undefined || - patch.dailyTime !== undefined || - patch.weeklyTime !== undefined || - patch.weeklyDays !== undefined; - const reArm = existing.scheduleType !== 'once' || !existing.completedOnce || scheduleTouched; + // A completed one-time job is only re-armed when the SCHEDULE actually + // CHANGES — otherwise a cosmetic edit would silently resurrect a job that + // already fired. We compare VALUES, not field-presence: the edit form + // round-trips the full job (incl. unchanged scheduleType/runAt) on every + // save, so a presence check would always re-arm. Only a real schedule + // change re-arms. + const changed = (next: T | undefined, prev: T): boolean => next !== undefined && next !== prev; + const scheduleChanged = + changed(patch.scheduleType, existing.scheduleType) || + changed(patch.runAt, existing.runAt) || + changed(patch.intervalMinutes, existing.intervalMinutes) || + changed(patch.dailyTime, existing.dailyTime) || + changed(patch.weeklyTime, existing.weeklyTime) || + (patch.weeklyDays !== undefined && !sameDays(patch.weeklyDays, existing.weeklyDays)); + const reArm = existing.scheduleType !== 'once' || !existing.completedOnce || scheduleChanged; const updated: CronJob = { ...existing, @@ -245,6 +259,7 @@ export class CronService { createdSessionUrl: null, }; this.store.setCronJobRun(run.id, run); + this.pruneRunHistory(); this.deps.broadcast(SseEvent.CronRunCreated, run); // Resolve the prompt. @@ -327,13 +342,19 @@ export class CronService { /** * Guards a prompt-file path before it is read. The path is user-supplied via - * the API, so without this an attacker (or a hostile job config) could read - * arbitrary host files (e.g. /etc/passwd, SSH keys) into a Claude session. + * the API and its contents are injected into an agent session (an exfil sink + * over SSE/terminal), so an unconfined read would let a hostile job config + * pull arbitrary host files — including the SERVER PROCESS'S OWN secrets via + * `/proc/self/environ` — into the session. * - * Mirrors the attachment-serving guard (`resolveServableAttachmentPath` in - * file-routes): realpath-resolve, then reject via the shared blocklist - * (`/etc`, `/root`, secret locations) plus optional workspace confinement. - * Returns the symlink-resolved path to read. + * A denylist is the wrong posture for an exfil sink (it kept missing `/proc`, + * `/dev`, other users' `~/.ssh`, modern cloud creds…). So the PRIMARY gate is + * an allowlist: the prompt file must resolve INSIDE the job's working + * directory. A symlink escaping the workspace fails this because we check the + * realpath-resolved target. We additionally require a regular file (rejects + * directories, FIFOs, and `/dev/*` character devices that would hang or OOM + * the unbounded read) within a sane size cap, and keep the shared blocklist as + * cheap defense-in-depth. Returns the symlink-resolved path to read. */ private async resolveSafePromptPath(rawPath: string, workingDir: string): Promise { let resolved: string; @@ -343,13 +364,27 @@ export class CronService { throw new Error('prompt file path could not be resolved'); } + // Defense-in-depth blocklist (secret locations, /etc, /root). const guard = await loadAttachmentGuardConfig(); - const blocked = - isBlockedAttachmentPath(resolved, guard.blockedTrees) || - isBlockedAttachmentPath(rawPath, guard.blockedTrees) || - (guard.confineToWorkspace && !validateSessionFilePath(workingDir, resolved)); + if (isBlockedAttachmentPath(resolved, guard.blockedTrees)) { + throw new Error('prompt file path is blocked'); + } + + // Primary gate: the prompt file must live inside the job's workspace. + if (!validateSessionFilePath(workingDir, resolved)) { + throw new Error('prompt file path must be inside the job working directory'); + } + + // Reject non-regular files and oversized files (DoS via unbounded read). + let info; + try { + info = statSync(resolved); + } catch { + throw new Error('prompt file path could not be resolved'); + } + if (!info.isFile()) throw new Error('prompt file path is not a regular file'); + if (info.size > MAX_PROMPT_FILE_BYTES) throw new Error('prompt file is too large'); - if (blocked) throw new Error('prompt file path is blocked'); return resolved; } @@ -401,6 +436,11 @@ export class CronService { } private recordSkippedRun(job: CronJob): void { + // Coalesce consecutive skips: if the job is already in a skip streak, don't + // record again — a perpetually-skipped interval job would otherwise write a + // run every tick forever and bloat state.json. + if (this.listRuns(job.id)[0]?.status === 'skipped') return; + const now = Date.now(); const run: CronJobRun = { id: uuidv4(), @@ -415,16 +455,29 @@ export class CronService { createdSessionUrl: null, }; this.store.setCronJobRun(run.id, run); + this.pruneRunHistory(); this.deps.broadcast(SseEvent.CronRunCreated, run); - this.updateJobLastStatus(job.id, 'skipped'); + // A skip is NOT a run: surface it as the lastStatus, but do NOT advance + // lastRunAt (no session was created). + this.updateJobLastStatus(job.id, 'skipped', { touchLastRun: false }); } - private updateJobLastStatus(jobId: string, status: CronJobRunStatus): void { + /** Prune the oldest run records (by startedAt) once the global cap is exceeded. */ + private pruneRunHistory(): void { + const runs = Object.values(this.store.getCronJobRuns()); + if (runs.length <= MAX_CRON_RUN_HISTORY) return; + runs.sort((a, b) => a.startedAt - b.startedAt); + for (const run of runs.slice(0, runs.length - MAX_CRON_RUN_HISTORY)) { + this.store.removeCronJobRun(run.id); + } + } + + private updateJobLastStatus(jobId: string, status: CronJobRunStatus, opts: { touchLastRun?: boolean } = {}): void { const fresh = this.store.getCronJob(jobId); if (!fresh) return; const now = Date.now(); fresh.lastStatus = status; - fresh.lastRunAt = now; + if (opts.touchLastRun !== false) fresh.lastRunAt = now; fresh.updatedAt = now; this.store.setCronJob(fresh.id, fresh); this.broadcastListChanged(); diff --git a/test/cron-service.test.ts b/test/cron-service.test.ts index 97f1be6e..af83bed5 100644 --- a/test/cron-service.test.ts +++ b/test/cron-service.test.ts @@ -12,7 +12,7 @@ */ import { describe, it, expect, beforeEach, vi } from 'vitest'; -import { mkdtempSync, writeFileSync } from 'node:fs'; +import { mkdtempSync, mkdirSync, writeFileSync, symlinkSync } from 'node:fs'; import { tmpdir } from 'node:os'; import { join } from 'node:path'; import { CronService, type CronDeps } from '../src/cron/cron-service.js'; @@ -142,6 +142,25 @@ describe('CronService', () => { expect(updated!.nextRunAt).toBeNull(); }); + it('does NOT resurrect a fired once-job when the edit form round-trips the unchanged schedule', async () => { + // Reproduces the real UI flow: the edit modal re-sends the FULL job + // (scheduleType + runAt unchanged) on every save. A field-presence check + // would wrongly re-arm; we compare VALUES, so an unchanged schedule does not. + const runAt = Date.now() - 1000; + const job = svc.service.createJob(mkInput({ scheduleType: 'once', runAt, intervalMinutes: undefined })); + await svc.service.tickDueJobs(Date.now()); + await flush(); + expect(svc.service.getJob(job.id)!.completedOnce).toBe(true); + + // Full-body edit changing only the name; schedule values identical. + const updated = svc.service.updateJob(job.id, { name: 'renamed', scheduleType: 'once', runAt, enabled: false }); + expect(updated!.completedOnce).toBe(true); + + // Even re-enabling afterward must not bring the dead job back to life. + const reenabled = svc.service.setEnabled(job.id, true); + expect(reenabled!.nextRunAt).toBeNull(); + }); + it('re-arms a completed once-job when the schedule itself is edited', async () => { const job = svc.service.createJob( mkInput({ scheduleType: 'once', runAt: Date.now() - 1000, intervalMinutes: undefined }) @@ -282,61 +301,108 @@ describe('CronService', () => { expect(runs[0].status).toBe('skipped'); const after = local.service.getJob(job.id)!; expect(after.lastStatus).toBe('skipped'); + // A skip is not a run: lastRunAt must NOT advance. + expect(after.lastRunAt).toBeNull(); expect(after.nextRunAt!).toBeGreaterThan(fireAt); expect(after.lastDueKey).not.toBeNull(); }); + + it('coalesces consecutive skips — a perpetually-skipped job does not flood run history', async () => { + const sessions = new Map([['s1', { mode: 'claude' }]]); + const local = makeService(sessions); + const job = local.service.createJob( + mkInput({ agentType: 'claude', concurrencyPolicy: 'skip_if_same_agent_running', intervalMinutes: 10 }) + ); + + // Drive 50 due ticks; the same-mode session keeps it skipped every time. + for (let i = 0; i < 50; i++) { + const due = local.service.getJob(job.id)!.nextRunAt! + 1; + await local.service.tickDueJobs(due); + await flush(); + } + + // Exactly ONE skipped run is recorded for the whole skip streak. + expect(local.service.listRuns(job.id).length).toBe(1); + expect(local.service.listRuns(job.id)[0].status).toBe('skipped'); + }); }); describe('resolvePrompt path guard', () => { - it('blocks a prompt_file_path pointing at a sensitive system file', async () => { - const job = svc.service.createJob( - mkInput({ promptMode: 'prompt_file_path', promptFilePath: '/etc/passwd', promptText: undefined }) + // A real workspace dir for the in-workspace / confinement cases. + let ws: string; + beforeEach(() => { + ws = mkdtempSync(join(tmpdir(), 'codeman-cron-ws-')); + }); + + const fileJob = (promptFilePath: string, workingDir: string) => + svc.service.createJob( + mkInput({ promptMode: 'prompt_file_path', promptFilePath, promptText: undefined, workingDir }) ); - const run = await svc.service.runNow(job.id); - expect(run).not.toBeNull(); + + it('blocks a sensitive system file via the blocklist (/etc/passwd)', async () => { + const run = await svc.service.runNow(fileJob('/etc/passwd', ws).id); expect(run!.status).toBe('failed'); - // Must fail at prompt resolution (blocked), NOT later at the missing workingDir — - // i.e. the file content must never be read. + // Fails at prompt resolution (blocked) — content is never read. expect(run!.errorMessage).toMatch(/Prompt error/i); expect(run!.errorMessage).toMatch(/block/i); - // No session was created for a blocked job. expect(svc.sessions.size).toBe(0); }); - it('blocks a prompt_file_path under a default-blocked tree (/root)', async () => { - const job = svc.service.createJob( - mkInput({ promptMode: 'prompt_file_path', promptFilePath: '/root/.bashrc', promptText: undefined }) - ); - const run = await svc.service.runNow(job.id); + it('blocks /proc/self/environ (server-process env exfil) via workspace confinement', async () => { + const run = await svc.service.runNow(fileJob('/proc/self/environ', ws).id); expect(run!.status).toBe('failed'); expect(run!.errorMessage).toMatch(/Prompt error/i); + expect(run!.errorMessage).toMatch(/inside the job working directory/i); + }); + + it('blocks a regular file that lives OUTSIDE the job workspace', async () => { + const outside = mkdtempSync(join(tmpdir(), 'codeman-cron-outside-')); + const file = join(outside, 'prompt.md'); + writeFileSync(file, 'do the thing'); + const run = await svc.service.runNow(fileJob(file, ws).id); + expect(run!.status).toBe('failed'); + expect(run!.errorMessage).toMatch(/inside the job working directory/i); + }); + + it('blocks a non-regular file (directory) inside the workspace', async () => { + const sub = join(ws, 'adir'); + mkdirSync(sub); + const run = await svc.service.runNow(fileJob(sub, ws).id); + expect(run!.status).toBe('failed'); + expect(run!.errorMessage).toMatch(/not a regular file/i); + }); + + it('blocks an oversized prompt file (unbounded-read DoS)', async () => { + const file = join(ws, 'huge.md'); + writeFileSync(file, Buffer.alloc(1024 * 1024 + 1, 0x61)); + const run = await svc.service.runNow(fileJob(file, ws).id); + expect(run!.status).toBe('failed'); + expect(run!.errorMessage).toMatch(/too large/i); }); it('fails cleanly (no throw) when the prompt file does not exist', async () => { - const job = svc.service.createJob( - mkInput({ - promptMode: 'prompt_file_path', - promptFilePath: '/tmp/codeman-cron-no-such-prompt-file.md', - promptText: undefined, - }) - ); - const run = await svc.service.runNow(job.id); + const run = await svc.service.runNow(fileJob(join(ws, 'nope.md'), ws).id); expect(run!.status).toBe('failed'); expect(run!.errorMessage).toMatch(/Prompt error/i); }); - it('allows an ordinary prompt file outside the blocklist (passes resolution)', async () => { - const dir = mkdtempSync(join(tmpdir(), 'codeman-cron-prompt-')); - const file = join(dir, 'prompt.md'); + it('allows a regular prompt file INSIDE the job workspace (passes resolution)', async () => { + const file = join(ws, 'prompt.md'); writeFileSync(file, 'do the thing'); - const job = svc.service.createJob( - mkInput({ promptMode: 'prompt_file_path', promptFilePath: file, promptText: undefined }) - ); - const run = await svc.service.runNow(job.id); - expect(run!.status).toBe('failed'); // still fails — workingDir (MISSING_DIR) does not exist - // ...but it got PAST prompt resolution: the failure is the workingDir, not a Prompt error. + const run = await svc.service.runNow(fileJob(file, ws).id); + // Got past prompt resolution + workingDir checks; fails only at the + // (mock-incomplete) session-launch step — NOT a prompt error. + expect(run!.status).toBe('failed'); expect(run!.errorMessage).not.toMatch(/Prompt error/i); - expect(run!.errorMessage).toMatch(/workingDir/i); + expect(run!.errorMessage).toMatch(/Session launch failed/i); + }); + + it('blocks a symlink inside the workspace that escapes to /etc/passwd', async () => { + const link = join(ws, 'sneaky.md'); + symlinkSync('/etc/passwd', link); + const run = await svc.service.runNow(fileJob(link, ws).id); + expect(run!.status).toBe('failed'); + expect(run!.errorMessage).toMatch(/Prompt error/i); }); });