diff --git a/docs/security-architecture.md b/docs/security-architecture.md index c52453e8..2dd15db6 100644 --- a/docs/security-architecture.md +++ b/docs/security-architecture.md @@ -320,7 +320,37 @@ injected from API JSON (`innerHTML`), not via `file-raw`, so they are unaffected `/api/download` additionally refuses a blocklist of sensitive paths (`/etc/shadow`, `~/.ssh/`, `.env`, `*credentials*`, `.aws/credentials`, …). This is **defense‑in‑depth, not the primary boundary** — the realpath containment is -the control. +the control. The blocklist patterns are shared (`src/web/sensitive-path.ts`) with +the attachment guard below. + +### External attachments (registry) & the magic‑link trust boundary + +Live external attachments (`src/attachment-registry.ts`) mint an `att_` id +for a host file so browser requests carry the id, never an absolute path. Serving +is by id (`GET /api/sessions/:id/attachments/:attachmentId/raw`, 50 MB cap, +`nosniff`) and re‑resolves the symlink + re‑checks the **attachment guard** +(`src/config/attachment-guard.ts`: the shared sensitive‑path blocklist **plus** +the `/root` and `/etc` trees, extendable via `attachmentBlockedPaths` / +`CODEMAN_ATTACHMENT_BLOCKED_PATHS`) on every request. Unlike the workspace file +routes, attachments are intentionally **cross‑workspace** — so the effective gate +is the blocklist + a 6‑extension allowlist (`png/pdf/docx/pptx/md/txt`), not +realpath containment. + +Two registration paths, with **different trust**: + +- **Explicit `POST /api/sessions/:id/attachments`** (and `codeman attach`, which + POSTs directly inside a managed session) — a deliberate, Origin‑guarded HTTP + request. Allowed cross‑workspace (subject to the guard). This is the supported + path for codeman‑publish and the `~/.codeman` review‑card loop. +- **Terminal `codeman://attach?path=…` magic links** — scanned passively from + session output. Terminal output is **attacker‑influenceable** (a prompt‑injected + session can print an arbitrary path), and registration here is server‑side with + no Origin gate and broadcasts the `rawUrl` over SSE to all clients. This path is + therefore **force‑confined to the session workspace** (`forceWorkspaceConfinement` + in `registerExternalAttachment`, wired in `WebServer.registerAttachment`), + regardless of the global confine setting — a passive magic link cannot expose a + file outside the session's own workspace. Cross‑workspace attach must go through + the explicit POST path above. ### SSE log‑tail route — intentional extra read roots diff --git a/src/attachment-registry.ts b/src/attachment-registry.ts index fe7b64cf..3bfd4b40 100644 --- a/src/attachment-registry.ts +++ b/src/attachment-registry.ts @@ -48,6 +48,10 @@ export class AttachmentRegistrationError extends Error { } } +/** Per-session attachment cap. Bounds memory against a client (or a + * prompt-injected magic-link flood) registering unbounded distinct paths. */ +const MAX_ATTACHMENTS_PER_SESSION = 200; + class AttachmentRegistry { private recordsBySession = new Map>(); @@ -58,6 +62,12 @@ class AttachmentRegistry { this.recordsBySession.set(record.sessionId, records); } records.set(record.attachmentId, record); + // Evict oldest (insertion-order) entries beyond the cap. + while (records.size > MAX_ATTACHMENTS_PER_SESSION) { + const oldest = records.keys().next().value; + if (oldest === undefined) break; + records.delete(oldest); + } } get(sessionId: string, attachmentId: string): AttachmentRecord | undefined { @@ -134,12 +144,22 @@ export function attachmentRecordToEvent(record: AttachmentRecord): AttachmentReg /** Options for {@link registerExternalAttachment}. */ export interface RegisterExternalAttachmentOptions { /** - * The registering session's working directory. Required only to enforce - * workspace confinement when that mode is enabled - * (`attachmentConfineToWorkspace` / `CODEMAN_ATTACHMENT_CONFINE`); ignored in - * the default blocklist mode. + * The registering session's working directory. Required to enforce workspace + * confinement — either when the global mode is enabled + * (`attachmentConfineToWorkspace` / `CODEMAN_ATTACHMENT_CONFINE`) or when + * {@link forceWorkspaceConfinement} is set for this call. */ sessionWorkingDir?: string; + /** + * Force workspace confinement for THIS registration regardless of the global + * setting. Used by the terminal-output `codeman://attach` magic-link scanner: + * terminal output is attacker-influenceable (a prompt-injected session can + * print an arbitrary path), so passive magic links may only reference files + * inside the session workspace. Deliberate cross-workspace attachment still + * works through the explicit, Origin-guarded `POST /attachments` route and the + * `codeman attach` CLI (which POSTs directly when a session id is known). + */ + forceWorkspaceConfinement?: boolean; } export async function registerExternalAttachment( @@ -162,10 +182,11 @@ export async function registerExternalAttachment( // path before doing anything else. const guard = await loadAttachmentGuardConfig(); - if (guard.confineToWorkspace) { - // Strict mode (opt-in, default OFF): the file MUST resolve inside the - // session's workspace. Strictly more restrictive than the blocklist — - // breaks cross-workspace attachment, which is why it is off by default. + if (guard.confineToWorkspace || options.forceWorkspaceConfinement) { + // Workspace-confined: the file MUST resolve inside the session's workspace. + // Applies when the global strict mode is on (opt-in, default OFF) OR when + // the caller forces it for this registration (the magic-link scanner — see + // forceWorkspaceConfinement). Strictly more restrictive than the blocklist. const workingDir = options.sessionWorkingDir; if (!workingDir || !validateSessionFilePath(workingDir, resolvedPath)) { throw new AttachmentRegistrationError('Access to this file is blocked', 403); diff --git a/src/cli.ts b/src/cli.ts index ba5be45f..b209567a 100644 --- a/src/cli.ts +++ b/src/cli.ts @@ -13,8 +13,8 @@ import { createRequire } from 'module'; import http from 'node:http'; import https from 'node:https'; import { readFileSync } from 'node:fs'; -import { homedir } from 'node:os'; -import { isAbsolute, join } from 'node:path'; +import { isAbsolute } from 'node:path'; +import { dataPath } from './config/instance.js'; import { getSessionManager } from './session-manager.js'; import { getTaskQueue } from './task-queue.js'; import { getRalphLoop } from './ralph-loop.js'; @@ -34,7 +34,7 @@ function makeAttachmentMagicLink(filePath: string): string { } function readCodemanEnv(): Record { - const envPath = join(homedir(), '.codeman', '.env'); + const envPath = dataPath('.env'); try { const text = readFileSync(envPath, 'utf-8'); const result: Record = {}; diff --git a/src/image-watcher.ts b/src/image-watcher.ts index e0f8c7ae..0d1bb667 100644 --- a/src/image-watcher.ts +++ b/src/image-watcher.ts @@ -20,8 +20,12 @@ import { KeyedDebouncer } from './utils/index.js'; // ========== Constants ========== /** Supported image file extensions (lowercase) */ -const IMAGE_POPUP_EXTENSIONS = new Set(['.jpg', '.jpeg', '.gif', '.webp', '.bmp', '.svg']); -const ATTACHMENT_EXTENSIONS = new Set(['.png', '.pdf', '.docx', '.pptx']); +// PNG stays on the image-popup path: it's the dominant screenshot format and the +// frontend only wires the `image:detected` popup today. The attachment-card UI +// that would consume `attachment:detected` for images is out of scope for this +// PR, so routing PNG to it would silently break the dropped-screenshot popup. +const IMAGE_POPUP_EXTENSIONS = new Set(['.png', '.jpg', '.jpeg', '.gif', '.webp', '.bmp', '.svg']); +const ATTACHMENT_EXTENSIONS = new Set(['.pdf', '.docx', '.pptx']); const DETECTED_FILE_EXTENSIONS = new Set([...IMAGE_POPUP_EXTENSIONS, ...ATTACHMENT_EXTENSIONS]); /** Time to wait for file writes to stabilize (ms) */ diff --git a/src/web/routes/file-routes.ts b/src/web/routes/file-routes.ts index 8f40febd..149e28f7 100644 --- a/src/web/routes/file-routes.ts +++ b/src/web/routes/file-routes.ts @@ -71,6 +71,18 @@ async function serveRawFile( download?: boolean ): Promise { const stat = await fs.stat(resolvedPath); + const MAX_RAW_ATTACHMENT_SIZE = 50 * 1024 * 1024; // 50MB, matching file-raw / download + if (stat.size > MAX_RAW_ATTACHMENT_SIZE) { + reply + .code(413) + .send( + createErrorResponse( + ApiErrorCode.INVALID_INPUT, + `File too large (${Math.round(stat.size / 1024 / 1024)}MB > ${MAX_RAW_ATTACHMENT_SIZE / 1024 / 1024}MB limit)` + ) + ); + return; + } const content = createReadStream(resolvedPath); const safeName = sanitizeDownloadName(fileName); if (download || extension === 'svg') { @@ -116,14 +128,16 @@ function getAttachmentOr404( * rejects any record outside the session workspace. Returns true (and sends a * 403) when blocked. */ -async function rejectIfSensitiveRecord( +async function resolveServableAttachmentPath( reply: FastifyReply, record: AttachmentRecord, sessionWorkingDir?: string -): Promise { +): Promise { let pathToCheck = record.filePath; + let resolved = false; try { pathToCheck = realpathSync(record.filePath); + resolved = true; } catch { // Fall back to the stored (already realpath-resolved at registration) path. } @@ -137,9 +151,12 @@ async function rejectIfSensitiveRecord( if (blocked) { reply.code(403).send(createErrorResponse(ApiErrorCode.INVALID_INPUT, 'Access to this file is blocked')); - return true; + return null; } - return false; + // Serve the freshly-resolved path, not the stored one: if a path component + // became a symlink after registration, the guard checked the resolved target + // but streaming record.filePath would follow the symlink to a swapped file. + return resolved ? pathToCheck : record.filePath; } export function registerFileRoutes(app: FastifyInstance, ctx: SessionPort & EventPort): void { @@ -486,10 +503,11 @@ export function registerFileRoutes(app: FastifyInstance, ctx: SessionPort & Even const session = findSessionOrFail(ctx, id); const record = getAttachmentOr404(reply, id, attachmentId); if (!record) return; - if (await rejectIfSensitiveRecord(reply, record, session.workingDir)) return; + const servePath = await resolveServableAttachmentPath(reply, record, session.workingDir); + if (!servePath) return; try { - await serveRawFile(reply, record.filePath, record.fileName, record.extension, download === 'true'); + await serveRawFile(reply, servePath, record.fileName, record.extension, download === 'true'); } catch (err) { reply .code(500) diff --git a/src/web/server.ts b/src/web/server.ts index ae812568..b5678534 100644 --- a/src/web/server.ts +++ b/src/web/server.ts @@ -1268,13 +1268,22 @@ export class WebServer extends EventEmitter { /** * Register a terminal-requested external file as a live attachment and * broadcast it. Triggered by the session's `attachmentRequested` event - * (codeman://attach magic links). Registration enforces the COD-53 - * attachment-guard policy. + * (codeman://attach magic links). Because terminal output is + * attacker-influenceable (a prompt-injected session can print an arbitrary + * `codeman://attach?path=` link), the scanned path is FORCE-confined to the + * session workspace — passive magic links can't expose arbitrary host files. + * Deliberate cross-workspace attachment goes through the explicit, + * Origin-guarded `POST /attachments` route (and `codeman attach`, which POSTs + * directly inside a managed session). Registration also enforces the COD-53 + * blocklist as defense-in-depth. */ private async registerAttachment(sessionId: string, filePath: string): Promise { const session = this.sessions.get(sessionId); if (!session) return; - const event = await registerExternalAttachment(sessionId, filePath, { sessionWorkingDir: session.workingDir }); + const event = await registerExternalAttachment(sessionId, filePath, { + sessionWorkingDir: session.workingDir, + forceWorkspaceConfinement: true, + }); this.broadcast(SseEvent.AttachmentDetected, event); } diff --git a/test/image-watcher.test.ts b/test/image-watcher.test.ts index c7e97955..716f509e 100644 --- a/test/image-watcher.test.ts +++ b/test/image-watcher.test.ts @@ -145,9 +145,9 @@ describe('ImageWatcher', () => { // ========== Image Detection ========== describe('image detection', () => { - it('should emit attachment:detected for .png files', () => { + it('should emit image:detected (popup) for .png files', () => { const handler = vi.fn(); - watcher.on('attachment:detected', handler); + watcher.on('image:detected', handler); watcher.watchSession('session-1', '/home/user/project'); const chokidarWatcher = mockWatchers.get('/home/user/project')!; @@ -161,14 +161,11 @@ describe('ImageWatcher', () => { expect(event.fileName).toBe('screenshot.png'); expect(event.filePath).toBe('/home/user/project/screenshot.png'); expect(event.relativePath).toBe('screenshot.png'); - expect(event.extension).toBe('png'); - expect(event.attachmentType).toBe('image'); - expect(event.size).toBe(2048); }); - it('should not emit legacy image:detected for .png attachment cards', () => { + it('should not emit attachment:detected for .png (stays on the popup path)', () => { const handler = vi.fn(); - watcher.on('image:detected', handler); + watcher.on('attachment:detected', handler); watcher.watchSession('session-1', '/home/user/project'); mockWatchers.get('/home/user/project')!.emit('add', '/home/user/project/screenshot.png'); diff --git a/test/routes/file-routes-attachment-path-guard.test.ts b/test/routes/file-routes-attachment-path-guard.test.ts index 258335b9..8a854386 100644 --- a/test/routes/file-routes-attachment-path-guard.test.ts +++ b/test/routes/file-routes-attachment-path-guard.test.ts @@ -51,7 +51,11 @@ vi.mock('../../src/file-stream-manager.js', () => ({ import fs from 'node:fs/promises'; import { createReadStream, realpathSync } from 'node:fs'; -import { attachmentRegistry, type AttachmentRecord } from '../../src/attachment-registry.js'; +import { + attachmentRegistry, + registerExternalAttachment, + type AttachmentRecord, +} from '../../src/attachment-registry.js'; const mockedStat = vi.mocked(fs.stat); const mockedRealpathSync = vi.mocked(realpathSync); @@ -321,4 +325,34 @@ describe('file-routes attachment path guard (COD-53)', () => { expect(body.success).toBe(true); expect(body.data.fileName).toBe('jira-autoloop-questions.md'); }); + + // ===== Magic-link scan path: FORCED workspace confinement ===== + // The terminal-output `codeman://attach` scanner registers with + // forceWorkspaceConfinement: true so a prompt-injected session printing an + // arbitrary path can't expose a host file, even though global confine is OFF. + describe('forced workspace confinement (magic-link scan path)', () => { + it('rejects an out-of-workspace path even when global confinement is OFF', async () => { + mockedRealpathSync.mockImplementation((p: string) => p as never); + mockedStat.mockResolvedValue({ size: 10, isFile: () => true, mtimeMs: 1 } as never); + await expect( + registerExternalAttachment('test-session-mlc', '/home/someone/secret/report.pdf', { + sessionWorkingDir: '/tmp/test-workdir', + forceWorkspaceConfinement: true, + }) + ).rejects.toMatchObject({ statusCode: 403 }); + attachmentRegistry.clearSession('test-session-mlc'); + }); + + it('allows an in-workspace path on the forced path', async () => { + const inside = '/tmp/test-workdir/sub/report.pdf'; + mockedRealpathSync.mockReturnValue(inside as never); + mockedStat.mockResolvedValue({ size: 10, isFile: () => true, mtimeMs: 1 } as never); + const event = await registerExternalAttachment('test-session-mlc', inside, { + sessionWorkingDir: '/tmp/test-workdir', + forceWorkspaceConfinement: true, + }); + expect(event.fileName).toBe('report.pdf'); + attachmentRegistry.clearSession('test-session-mlc'); + }); + }); });