fix(cron): harden cron fixes against adversarial-review findings

Two blind adversarial reviewers found real holes in the prior cron commits:

SECURITY (was CRITICAL): the prompt-file guard was blocklist-only by default,
so promptFilePath:/proc/self/environ leaked the SERVER PROCESS's entire
environment (every secret) into the agent session, and /dev/zero or a FIFO
caused an unbounded readFile → OOM/hang DoS. A denylist is the wrong posture
for an exfil-into-LLM sink. resolveSafePromptPath now:
  - confines the realpath-resolved file to the job's working dir (ALLOWLIST) —
    closes /proc, /dev, other homes, modern cloud-cred paths, and symlink escapes
  - requires a regular file (rejects dirs/FIFOs/char devices)
  - caps the read at MAX_PROMPT_FILE_BYTES (1 MiB)
  - keeps the /etc,/root,secrets blocklist as defense-in-depth

LOGIC:
  - once-rearm (was MED, defeated in prod): the edit UI round-trips the full
    job, so the field-PRESENCE re-arm check always fired → a renamed fired
    once-job could be resurrected via edit→re-enable. Now compares schedule
    VALUES; an unchanged schedule never re-arms.
  - skipped-run history (was HIGH): recording a skip every tick was unbounded
    state.json growth. Now coalesces consecutive skips (one record per streak)
    and prunes global run history to MAX_CRON_RUN_HISTORY (500), covering the
    launch path too.
  - skip bookkeeping (was MED): a skip no longer advances lastRunAt (nothing
    ran); lastStatus still reflects 'skipped'.

Regression tests added/updated (43 pass): /proc/self/environ + outside-workspace
+ symlink-escape + non-regular + oversized all blocked, in-workspace file
passes; UI-path once resurrection blocked; consecutive skips coalesce to one
record; skip leaves lastRunAt null.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PmvZR12aX2v8K7YhqxPUAU

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
 // ============================================================================

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<void> => 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 = <T>(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<string> {
     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();

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<string, { mode: string }>([['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);
     });
   });