security(paste-image): harden against 7 findings from PR #84 review (#90)

Hardens `/api/sessions/:id/paste-image` against the seven findings flagged in the dismissed security review on #84. Each commit addresses one finding.

- LOW: Collision-free filenames (`paste-${ts}-${rand4}${ext}`)
- MED: Symlink check on image dir (`lstat` + non-recursive mkdir + `O_EXCL|O_NOFOLLOW`)
- MED: Magic-byte validation (PNG/JPEG/GIF/WebP/BMP)
- HIGH: CSRF protection (Origin/Referer match req.host; non-browser clients send `X-Codeman-CSRF`)
- MED: Swap hand-rolled multipart parser to @fastify/multipart with `limits: { fileSize: 10MB, files: 1, fields: 4 }`
- MED: Rate limit (30/min per IP+session) + hourly GC of `paste-*` files older than 7d
- LOW: Use `terminal.paste(text)` instead of `sendInput(text)` so bracketed-paste markers survive

Co-authored-by: Aamer Akhter <aakhter@gmail.com>

package.json

@@ -52,6 +52,7 @@
   "dependencies": {
     "@fastify/compress": "^8.3.1",
     "@fastify/cookie": "^11.0.2",
+    "@fastify/multipart": "^10.0.0",
     "@fastify/static": "^8.0.0",
     "@fastify/websocket": "^11.2.0",
     "@xterm/addon-fit": "^0.11.0",

src/web/paste-image-gc.ts

@@ -0,0 +1,69 @@
+/**
+ * @fileoverview Periodic GC for paste-image files.
+ *
+ * Without cleanup, /api/sessions/:id/paste-image accumulates files indefinitely
+ * under {workingDir}/.claude-images/. The route only triggers cleanup on
+ * killMux=true session deletion, so long-lived sessions can fill disk under
+ * heavy pasting. This sweeper bounds disk use by deleting `paste-*` files
+ * older than MAX_AGE_MS from each live session's image dir on an interval.
+ *
+ * Conservative defaults — only files matching the `paste-` prefix are
+ * considered, and we lstat (not stat) so a planted symlink cannot escape the
+ * image dir.
+ */
+import fs from 'node:fs/promises';
+import { join } from 'node:path';
+import type { SessionPort } from './ports/index.js';
+
+const MAX_AGE_MS = 7 * 24 * 60 * 60 * 1000; // 7 days
+const SWEEP_INTERVAL_MS = 60 * 60 * 1000; // 1 hour
+const INITIAL_DELAY_MS = 30 * 1000; // 30s after startup
+
+export async function sweepPasteImagesOnce(
+  ctx: Pick<SessionPort, 'sessions'>,
+  now: number = Date.now()
+): Promise<{ scanned: number; deleted: number }> {
+  const cutoff = now - MAX_AGE_MS;
+  let scanned = 0;
+  let deleted = 0;
+  for (const session of ctx.sessions.values()) {
+    const dir = join(session.workingDir, '.claude-images');
+    let entries: string[];
+    try {
+      entries = await fs.readdir(dir);
+    } catch {
+      continue; // dir absent — nothing to do
+    }
+    for (const name of entries) {
+      if (!name.startsWith('paste-')) continue;
+      const p = join(dir, name);
+      scanned += 1;
+      try {
+        const st = await fs.lstat(p);
+        if (!st.isFile()) continue;
+        if (st.mtimeMs < cutoff) {
+          await fs.unlink(p);
+          deleted += 1;
+        }
+      } catch {
+        // best-effort: skip permission/race errors silently
+      }
+    }
+  }
+  return { scanned, deleted };
+}
+
+export function startPasteImageGc(ctx: Pick<SessionPort, 'sessions'>): () => void {
+  const initial = setTimeout(() => {
+    void sweepPasteImagesOnce(ctx);
+  }, INITIAL_DELAY_MS);
+  const interval = setInterval(() => {
+    void sweepPasteImagesOnce(ctx);
+  }, SWEEP_INTERVAL_MS);
+  if (typeof initial.unref === 'function') initial.unref();
+  if (typeof interval.unref === 'function') interval.unref();
+  return (): void => {
+    clearTimeout(initial);
+    clearInterval(interval);
+  };
+}

src/web/public/image-input.js

@@ -87,10 +87,15 @@ Object.assign(CodemanApp.prototype, {
         e.preventDefault();
         self._uploadAndInsertImages(imageFiles);
       } else {
-        // No image -- extract text and send to terminal
+        // No image -- route text through xterm's paste() so bracketed-paste
+        // markers (CSI 200~ ... CSI 201~) survive when the inner application
+        // has enabled bracketed-paste mode (Claude Code does). Sending text
+        // via raw sendInput() strips those markers and makes pasted input
+        // indistinguishable from typed input, weakening the CLI's
+        // prompt-injection defenses.
         var text = e.clipboardData ? e.clipboardData.getData('text/plain') : '';
         e.preventDefault();
-        if (text) self.sendInput(text);
+        if (text && self.terminal) self.terminal.paste(text);
       }
     });
 

src/web/routes/session-routes.ts

@@ -10,6 +10,7 @@ import { homedir } from 'node:os';
 import { existsSync, statSync, mkdirSync, writeFileSync } from 'node:fs';
 import { execFile } from 'node:child_process';
 import fs from 'node:fs/promises';
+import { randomBytes } from 'node:crypto';
 import {
   ApiErrorCode,
   createErrorResponse,
@@ -122,6 +123,76 @@ export function stripInkRedrawBloat(buffer: string): string {
   return parts.join('');
 }
 
+/**
+ * Validate image bytes against a declared extension. Sniffs the first ~12 bytes
+ * for a known magic-number signature. Defends against polyglots (e.g. HTML or
+ * SVG disguised under a `Content-Type: image/png` header) and against simple
+ * extension-only spoofing — both the multipart filename and the Content-Type
+ * are attacker-controlled, the raw bytes are not.
+ *
+ * Signatures: https://en.wikipedia.org/wiki/List_of_file_signatures
+ */
+export function imageMagicMatchesExt(data: Buffer, ext: string): boolean {
+  if (data.length < 12) return false;
+  const u32be = (off: number): number => data.readUInt32BE(off);
+  switch (ext) {
+    case '.png':
+      return u32be(0) === 0x89504e47 && u32be(4) === 0x0d0a1a0a;
+    case '.jpg':
+    case '.jpeg':
+      return data[0] === 0xff && data[1] === 0xd8 && data[2] === 0xff;
+    case '.gif':
+      return (
+        data[0] === 0x47 &&
+        data[1] === 0x49 &&
+        data[2] === 0x46 &&
+        data[3] === 0x38 &&
+        (data[4] === 0x37 || data[4] === 0x39) &&
+        data[5] === 0x61
+      );
+    case '.webp':
+      // RIFF....WEBP
+      return u32be(0) === 0x52494646 && u32be(8) === 0x57454250;
+    case '.bmp':
+      return data[0] === 0x42 && data[1] === 0x4d;
+    default:
+      return false;
+  }
+}
+
+// Per-(IP, sessionId) token bucket for paste-image. 30 requests/minute.
+// Bucket map entries are pruned when they drift > 1h stale to bound memory
+// against a flood of unique IP keys.
+const PASTE_RATE_TOKENS = 30;
+const PASTE_RATE_REFILL_PER_MS = PASTE_RATE_TOKENS / 60_000;
+const PASTE_BUCKET_TTL_MS = 60 * 60 * 1000;
+const PASTE_BUCKET_GC_THRESHOLD = 1000;
+const pasteRateBuckets = new Map<string, { tokens: number; lastRefill: number }>();
+
+export function consumePasteToken(key: string, now: number = Date.now()): boolean {
+  if (pasteRateBuckets.size > PASTE_BUCKET_GC_THRESHOLD) {
+    for (const [k, b] of pasteRateBuckets) {
+      if (now - b.lastRefill > PASTE_BUCKET_TTL_MS) pasteRateBuckets.delete(k);
+    }
+  }
+  let b = pasteRateBuckets.get(key);
+  if (!b) {
+    b = { tokens: PASTE_RATE_TOKENS, lastRefill: now };
+    pasteRateBuckets.set(key, b);
+  }
+  const delta = (now - b.lastRefill) * PASTE_RATE_REFILL_PER_MS;
+  b.tokens = Math.min(PASTE_RATE_TOKENS, b.tokens + delta);
+  b.lastRefill = now;
+  if (b.tokens < 1) return false;
+  b.tokens -= 1;
+  return true;
+}
+
+// Test hook: reset between runs.
+export function _resetPasteRateBuckets(): void {
+  pasteRateBuckets.clear();
+}
+
 export function registerSessionRoutes(
   app: FastifyInstance,
   ctx: SessionPort & EventPort & ConfigPort & InfraPort & AuthPort
@@ -1442,76 +1513,98 @@ export function registerSessionRoutes(
   // Paste Image (clipboard / drag-drop upload)
   // ═══════════════════════════════════════════════════════════════
 
-  const MAX_PASTE_IMAGE_SIZE = 10 * 1024 * 1024; // 10 MB
   const ALLOWED_IMAGE_EXTS = new Set(['.png', '.jpg', '.jpeg', '.gif', '.webp', '.bmp']);
+  // The 10MB size cap is enforced by @fastify/multipart (registered in server.ts).
 
   app.post('/api/sessions/:id/paste-image', async (req, reply) => {
+    // CSRF defense: state-changing routes must come from same origin.
+    // Cookies are SameSite=lax, multipart/form-data is a "simple" CORS request
+    // (no preflight), so a cross-origin <form enctype="multipart/form-data">
+    // submit attaches the session cookie unimpeded. Reject unless Origin/Referer
+    // matches req.host. Non-browser clients (no Origin AND no Referer) must
+    // supply X-Codeman-CSRF — a header browsers cannot add cross-origin without
+    // a preflight, which our CORS config does not allow from other origins.
+    const reqHost = req.headers.host;
+    const origin = req.headers.origin;
+    const referer = req.headers.referer;
+    let csrfOk = false;
+    if (origin) {
+      try {
+        csrfOk = new URL(origin).host === reqHost;
+      } catch {
+        /* invalid Origin → not ok */
+      }
+    } else if (referer) {
+      try {
+        csrfOk = new URL(referer).host === reqHost;
+      } catch {
+        /* invalid Referer → not ok */
+      }
+    } else {
+      csrfOk = !!req.headers['x-codeman-csrf'];
+    }
+    if (!csrfOk) {
+      reply.code(403);
+      return createErrorResponse(ApiErrorCode.INVALID_INPUT, 'CSRF check failed');
+    }
+
     const { id } = req.params as { id: string };
+
+    // Rate limit per (IP, sessionId): 30/min. Defends against disk-fill DoS
+    // — even an authenticated attacker can otherwise loop 10MB POSTs.
+    if (!consumePasteToken(`${req.ip}:${id}`)) {
+      reply.code(429);
+      reply.header('Retry-After', '60');
+      return createErrorResponse(ApiErrorCode.INVALID_INPUT, 'Rate limit exceeded (30 uploads/min per session)');
+    }
+
     const session = findSessionOrFail(ctx, id);
 
-    const contentType = req.headers['content-type'] ?? '';
-    if (!contentType.includes('multipart/form-data')) {
+    if (!req.isMultipart()) {
       reply.code(400);
       return createErrorResponse(ApiErrorCode.INVALID_INPUT, 'Expected multipart/form-data');
     }
 
-    // Parse multipart boundary
-    const boundaryMatch = contentType.match(/boundary=(.+?)(?:;|$)/);
-    if (!boundaryMatch) {
-      reply.code(400);
-      return createErrorResponse(ApiErrorCode.INVALID_INPUT, 'Missing boundary');
+    // Read the single file part. @fastify/multipart enforces the 10MB size cap
+    // and the 1-file/4-field count limits (server.ts), replacing a hand-rolled
+    // boundary scanner with several bugs: literal boundary matches anywhere in
+    // body, LF-only clients silently corrupted the last byte (hard-coded \r\n
+    // offsets), no part-count cap.
+    let part: import('@fastify/multipart').MultipartFile | undefined;
+    try {
+      part = await req.file();
+    } catch (err: unknown) {
+      reply.code(413);
+      return createErrorResponse(ApiErrorCode.INVALID_INPUT, getErrorMessage(err) || 'Invalid multipart payload');
     }
-
-    // Collect raw body with size limit
-    const chunks: Buffer[] = [];
-    let totalSize = 0;
-    for await (const chunk of req.raw) {
-      totalSize += chunk.length;
-      if (totalSize > MAX_PASTE_IMAGE_SIZE) {
-        reply.code(413);
-        return createErrorResponse(ApiErrorCode.INVALID_INPUT, 'File too large (max 10MB)');
-      }
-      chunks.push(chunk as Buffer);
-    }
-    const body = Buffer.concat(chunks);
-
-    // Extract image from multipart body
-    const boundary = '--' + boundaryMatch[1];
-    const boundaryBuf = Buffer.from(boundary);
-    const parts: { headers: string; data: Buffer }[] = [];
-    let pos = 0;
-
-    while (pos < body.length) {
-      const start = body.indexOf(boundaryBuf, pos);
-      if (start === -1) break;
-      const afterBoundary = start + boundaryBuf.length;
-      if (body[afterBoundary] === 0x2d && body[afterBoundary + 1] === 0x2d) break;
-      const headerStart = afterBoundary + 2;
-      const headerEnd = body.indexOf(Buffer.from('\r\n\r\n'), headerStart);
-      if (headerEnd === -1) break;
-      const headers = body.subarray(headerStart, headerEnd).toString();
-      const dataStart = headerEnd + 4;
-      const nextBoundary = body.indexOf(boundaryBuf, dataStart);
-      const dataEnd = nextBoundary === -1 ? body.length : nextBoundary - 2;
-      parts.push({ headers, data: body.subarray(dataStart, dataEnd) });
-      pos = nextBoundary === -1 ? body.length : nextBoundary;
-    }
-
-    const imagePart = parts.find((p) => p.headers.includes('name="image"'));
-    if (!imagePart || imagePart.data.length === 0) {
+    if (!part) {
       reply.code(400);
       return createErrorResponse(ApiErrorCode.INVALID_INPUT, 'No image uploaded');
     }
+    if (part.fieldname !== 'image') {
+      reply.code(400);
+      return createErrorResponse(ApiErrorCode.INVALID_INPUT, `Unexpected field "${part.fieldname}", expected "image"`);
+    }
+    let imageBytes: Buffer;
+    try {
+      imageBytes = await part.toBuffer();
+    } catch (err: unknown) {
+      reply.code(413);
+      return createErrorResponse(ApiErrorCode.INVALID_INPUT, getErrorMessage(err) || 'File too large (max 10MB)');
+    }
+    if (imageBytes.length === 0) {
+      reply.code(400);
+      return createErrorResponse(ApiErrorCode.INVALID_INPUT, 'Empty file');
+    }
 
-    // Determine extension from filename or Content-Type
+    // Determine extension from filename or Content-Type.
     let ext = '.png';
-    const filenameMatch = imagePart.headers.match(/filename="(.+?)"/);
-    if (filenameMatch) {
-      const origExt = extname(filenameMatch[1]).toLowerCase();
+    if (part.filename) {
+      const origExt = extname(part.filename).toLowerCase();
       if (ALLOWED_IMAGE_EXTS.has(origExt)) ext = origExt;
     }
-    const ctMatch = imagePart.headers.match(/Content-Type:\s*image\/(png|jpeg|jpg|webp|gif|bmp)/i);
-    if (ctMatch) {
+    const mimeMatch = (part.mimetype || '').toLowerCase().match(/^image\/(png|jpeg|jpg|webp|gif|bmp)$/);
+    if (mimeMatch) {
       const map: Record<string, string> = {
         png: '.png',
         jpeg: '.jpg',
@@ -1520,7 +1613,7 @@ export function registerSessionRoutes(
         gif: '.gif',
         bmp: '.bmp',
       };
-      ext = map[ctMatch[1].toLowerCase()] ?? ext;
+      ext = map[mimeMatch[1]] ?? ext;
     }
 
     if (!ALLOWED_IMAGE_EXTS.has(ext)) {
@@ -1531,14 +1624,47 @@ export function registerSessionRoutes(
       );
     }
 
-    // Save to {workingDir}/.claude-images/
-    const imageDir = join(session.workingDir, '.claude-images');
-    if (!existsSync(imageDir)) {
-      mkdirSync(imageDir, { recursive: true });
+    // Sniff actual bytes — filename and Content-Type are both attacker-supplied.
+    // Polyglot HTML/PNG would otherwise pass and serve back with image/png MIME.
+    if (!imageMagicMatchesExt(imageBytes, ext)) {
+      reply.code(415);
+      return createErrorResponse(ApiErrorCode.INVALID_INPUT, `Image bytes do not match declared type ${ext}`);
     }
-    const filename = `paste-${Date.now()}${ext}`;
+
+    // Save to {workingDir}/.claude-images/
+    // Refuse symlinks at imageDir — an agent or postinstall script could plant
+    // `.claude-images -> ~/.ssh/` and redirect future writes outside workingDir.
+    // We lstat (not stat) so we see the symlink itself. Use mkdir without
+    // `recursive` so the leaf creation does not follow a symlink either, and
+    // O_EXCL|O_NOFOLLOW on the file open so the write itself is symlink-safe.
+    const imageDir = join(session.workingDir, '.claude-images');
+    try {
+      const dirStat = await fs.lstat(imageDir);
+      if (dirStat.isSymbolicLink() || !dirStat.isDirectory()) {
+        reply.code(403);
+        return createErrorResponse(ApiErrorCode.INVALID_INPUT, '.claude-images is not a regular directory');
+      }
+    } catch (err: unknown) {
+      if ((err as NodeJS.ErrnoException).code !== 'ENOENT') throw err;
+      // Non-recursive mkdir: errors on EEXIST and does not follow symlinks for
+      // the leaf. session.workingDir is guaranteed to exist (live session).
+      await fs.mkdir(imageDir);
+    }
+    // Date.now() collides on same-ms uploads from two tabs (last-write wins
+    // silently). Append 8 hex chars so concurrent pastes get distinct names.
+    const filename = `paste-${Date.now()}-${randomBytes(4).toString('hex')}${ext}`;
     const filepath = join(imageDir, filename);
-    await fs.writeFile(filepath, imagePart.data);
+    // O_EXCL: refuse to overwrite (collision is impossible with random suffix,
+    // but defends against TOCTOU). O_NOFOLLOW: refuse if filepath is a symlink.
+    const fh = await fs.open(
+      filepath,
+      fs.constants.O_WRONLY | fs.constants.O_CREAT | fs.constants.O_EXCL | fs.constants.O_NOFOLLOW
+    );
+    try {
+      await fh.writeFile(imageBytes);
+    } finally {
+      await fh.close();
+    }
 
     return { success: true, path: filepath, filename };
   });

src/web/server.ts

@@ -32,6 +32,8 @@ import fastifyCompress from '@fastify/compress';
 import fastifyCookie from '@fastify/cookie';
 import fastifyStatic from '@fastify/static';
 import fastifyWebsocket from '@fastify/websocket';
+import fastifyMultipart from '@fastify/multipart';
+import { startPasteImageGc } from './paste-image-gc.js';
 import { join, dirname } from 'node:path';
 import { fileURLToPath } from 'node:url';
 import { existsSync, mkdirSync, readFileSync, chmodSync, rmSync } from 'node:fs';
@@ -229,6 +231,7 @@ export class WebServer extends EventEmitter {
   private pushStore: PushSubscriptionStore = new PushSubscriptionStore();
   private teamWatcher: TeamWatcher = new TeamWatcher();
   private _orchestratorLoop: import('../orchestrator-loop.js').OrchestratorLoop | null = null;
+  private _pasteImageGcStop: (() => void) | null = null;
   private teamWatcherHandlers: {
     teamCreated: (config: unknown) => void;
     teamUpdated: (config: unknown) => void;
@@ -539,6 +542,18 @@ export class WebServer extends EventEmitter {
     // WebSocket support (terminal I/O — low-latency bidirectional channel)
     await this.app.register(fastifyWebsocket);
 
+    // Multipart parsing (used by paste-image). Replaces a hand-rolled
+    // boundary scanner that had several edge-case bugs: literal boundary
+    // anywhere in body was a match, LF-only clients silently corrupted the
+    // last byte (hard-coded \r\n offsets), and there was no part-count cap.
+    await this.app.register(fastifyMultipart, {
+      limits: {
+        fileSize: 10 * 1024 * 1024, // 10MB per file
+        files: 1, // paste-image only ever sends one file
+        fields: 4, // small headroom for accompanying form fields
+      },
+    });
+
     // Security headers + CORS
     registerSecurityHeaders(this.app, this.https);
     this.app.get('/', async (_req, reply) => {
@@ -1541,6 +1556,12 @@ export class WebServer extends EventEmitter {
     // Clean up stale sessions from state file that don't have active mux sessions
     this.cleanupStaleSessions();
 
+    // Bound disk use under heavy paste-image traffic: delete `paste-*` files
+    // older than 7 days from each live session's .claude-images/ hourly.
+    if (!this.testMode) {
+      this._pasteImageGcStop = startPasteImageGc({ sessions: this.sessions });
+    }
+
     await this.app.listen({ port: this.port, host: '0.0.0.0' });
     const protocol = this.https ? 'https' : 'http';
     console.log(`Codeman web interface running at ${protocol}://localhost:${this.port}`);
@@ -1894,6 +1915,11 @@ export class WebServer extends EventEmitter {
     // Set stopping flag to prevent new timer creation during shutdown
     this.sse.setStopping();
 
+    if (this._pasteImageGcStop) {
+      this._pasteImageGcStop();
+      this._pasteImageGcStop = null;
+    }
+
     // Dispose all managed timers (intervals + resettable timeouts)
     this.cleanup.dispose();