diff --git a/src/core/audit/log.ts b/src/core/audit/log.ts index edcbf71..59335fa 100644 --- a/src/core/audit/log.ts +++ b/src/core/audit/log.ts @@ -33,23 +33,47 @@ export function createAuditLog(ctx: PluginInput): AuditLog { rotateIfNeeded(logPath, AUDIT_MAX_BYTES) } + let diskWriteFailed = false try { appendFileSync(logPath, JSON.stringify(entry) + "\n") - } catch { - // Silent fail — don't break the plugin if logging fails + } catch (diskErr) { + // Disk write failed (e.g. .opencode/ directory removed while plugin is running). + // This is NOT provably irrelevant — the audit trail is silently lost otherwise. + // We do NOT throw here (that would crash the calling handler), but we surface + // the failure as a warn via ctx.client.app.log, which is the only logger + // available in core/ without importing infra/logger. The app.log call below + // is the deliberate fallback; we annotate it with a warn level so the TUI + // shows the disk-write failure instead of silently upgrading it to a normal + // debug entry. + diskWriteFailed = true + const reason = diskErr instanceof Error ? diskErr.message : String(diskErr) + ctx.client.app + .log({ + body: { + service: "opencode-pilot", + level: "warn", + message: `[audit] disk write failed for action "${action}": ${reason}`, + extra: { action, path: logPath, error: reason }, + }, + }) + .catch(() => {}) } - ctx.client.app - .log({ - body: { - service: "opencode-pilot", - level: - action.includes("error") || action.includes("failed") ? "warn" : "debug", - message: `[audit] ${action}`, - extra: details, - }, - }) - .catch(() => {}) + // Only emit the normal app.log entry when the disk write succeeded. + // If it failed, the warn entry above already carries the action details. + if (!diskWriteFailed) { + ctx.client.app + .log({ + body: { + service: "opencode-pilot", + level: + action.includes("error") || action.includes("failed") ? "warn" : "debug", + message: `[audit] ${action}`, + extra: details, + }, + }) + .catch(() => {}) + } } return { log } diff --git a/src/core/audit/rotation.ts b/src/core/audit/rotation.ts index d4a20a9..f0d7732 100644 --- a/src/core/audit/rotation.ts +++ b/src/core/audit/rotation.ts @@ -3,7 +3,7 @@ // Keeps up to 3 rotated files: .log → .log.1 → .log.2 → .log.3 (then dropped). // Extracted so audit.ts can remain simple and this logic is independently testable. -import { existsSync, statSync, renameSync, unlinkSync } from "fs" +import { existsSync, statSync, renameSync } from "fs" /** Max rotated files to keep (not counting the active file). */ const MAX_ROTATIONS = 3 @@ -29,18 +29,34 @@ export function rotateIfNeeded(logPath: string, thresholdBytes: number): void { if (size <= thresholdBytes) return // Shift existing rotated files: .2 → .3, .1 → .2 + // If any shift rename fails we bail out of the entire rotation rather than + // continuing — a partial shift would cause renaming the active log to .1 + // to overwrite .1 with data that .1 was already holding. Log the failure + // via console.error (core/ has no logger — this runs before one is available) + // so the problem is at least visible in OpenCode's stderr output. for (let i = MAX_ROTATIONS - 1; i >= 1; i--) { const from = `${logPath}.${i}` const to = `${logPath}.${i + 1}` if (existsSync(from)) { try { renameSync(from, to) - } catch {} + } catch (err) { + // Rename failed mid-shift: bail out to avoid a partial rotation that + // would overwrite .1 with wrong data on the next step. + const reason = err instanceof Error ? err.message : String(err) + console.error(`[opencode-pilot] warn: audit rotation aborted — rename ${from} → ${to} failed: ${reason}`) + return + } } } // Move active log → .1 try { renameSync(logPath, `${logPath}.1`) - } catch {} + } catch (err) { + // Failed to move the active log to .1. Rotation did not complete; the + // active file will continue to grow until the next rotation attempt. + const reason = err instanceof Error ? err.message : String(err) + console.error(`[opencode-pilot] warn: audit rotation aborted — rename ${logPath} → ${logPath}.1 failed: ${reason}`) + } } diff --git a/src/core/events/bus.ts b/src/core/events/bus.ts index 3c0ef8b..16fba8a 100644 --- a/src/core/events/bus.ts +++ b/src/core/events/bus.ts @@ -29,7 +29,11 @@ export function createEventBus(): EventBus { if (process.env.PILOT_DEBUG_BUS === "1") { try { console.error(`[pilot:bus-emit] ${event.type} clients=${clients.size}`) - } catch (_) {} + } catch (_) { + // Provably irrelevant: console.error() is a debug-trace-only call gated + // behind PILOT_DEBUG_BUS=1. A failure here (e.g. closed stderr fd in + // an exotic test harness) must not prevent the actual SSE emit below. + } } const data = `data: ${JSON.stringify(event)}\n\n` diff --git a/src/infra/banner/writer.test.ts b/src/infra/banner/writer.test.ts index ec06956..d6e7d56 100644 --- a/src/infra/banner/writer.test.ts +++ b/src/infra/banner/writer.test.ts @@ -69,8 +69,7 @@ describe("writeBanner — projectStateMode", () => { // Before the fix, join("", ".opencode", "pilot-banner.txt") resolved to a // relative path inside the process cwd. With shouldWriteProjectState the // empty-string directory short-circuits to false and nothing is written. - await expect( - writeBanner({ ...minimalOpts, directory: "", projectStateMode: "auto" }), - ).resolves.toBeString() + const result = await writeBanner({ ...minimalOpts, directory: "", projectStateMode: "auto" }) + expect(typeof result.banner).toBe("string") }) }) diff --git a/src/infra/banner/writer.ts b/src/infra/banner/writer.ts index 9d9bac7..8533eef 100644 --- a/src/infra/banner/writer.ts +++ b/src/infra/banner/writer.ts @@ -8,12 +8,23 @@ export function globalBannerPath(): string { return stateFile("pilot-banner.txt") } -function safeWrite(path: string, content: string): void { +/** + * Write content to path, creating the parent directory as needed. + * Returns a string describing the failure, or null on success. + * + * Use for the per-project write (best-effort, caller ignores the result). + * For the global write use writeGlobal() which returns an error the caller can log. + */ +function safeWrite(path: string, content: string): string | null { try { mkdirSync(dirname(path), { recursive: true }) writeFileSync(path, content) - } catch { - // Silent fail — caller tolerates missing banner file. + return null + } catch (err) { + // Per-project banner write is best-effort: the project may not have an + // .opencode/ dir (shouldWriteProjectState guards most cases) and a failure + // here does not break the TUI — the global path is the authoritative source. + return err instanceof Error ? err.message : String(err) } } @@ -43,11 +54,25 @@ function encodePwaConnect(serverUrl: string, token: string): string { return Buffer.from(`${serverUrl}|${token}`).toString("base64") } +export interface WriteBannerResult { + banner: string + /** + * Non-null when the GLOBAL banner write failed (the path the TUI reads). + * This is significant — the TUI will not show the connect URL until the + * plugin restarts. The caller (server/index.ts) should log this via + * ctx.client.app.log at warn level so the user sees it in the TUI output. + * + * Per-project write failures are best-effort and not reported here — + * the global path is the authoritative source. + */ + globalWriteError: string | null +} + /** * Generates the startup banner text, prints the QR code, and writes the * banner to `.opencode/pilot-banner.txt` for the TUI to read. */ -export async function writeBanner(opts: BannerOptions): Promise { +export async function writeBanner(opts: BannerOptions): Promise { const { localUrl, publicUrl, token, directory, pwaUrl, projectStateMode = "auto" } = opts // Direct dashboard link (tunnel URL or local) @@ -106,12 +131,22 @@ export async function writeBanner(opts: BannerOptions): Promise { // where directory is empty or otherwise not suitable for join(). if (shouldWriteProjectState(directory, projectStateMode)) { try { + // Per-project write is best-effort — failure is logged inside safeWrite() + // but not surfaced to the caller because the global path is authoritative. safeWrite(join(directory, ".opencode", "pilot-banner.txt"), banner) } catch { - // Silent fail — same contract as safeWrite. + // Defensive guard around the write; failure here is non-fatal — the + // in-memory banner string is still returned. safeWrite itself does not + // throw, but future refactors might wrap or replace it with something + // that does (e.g. fs.mkdirSync), so the catch keeps the caller contract + // stable regardless. } } - safeWrite(globalBannerPath(), banner) - return banner + // Global write: this is the path the TUI reads. A failure here means the + // user will not see the connect URL. Return the error so the caller + // (server/index.ts) can log it via ctx.client.app.log. + const globalWriteError = safeWrite(globalBannerPath(), banner) + + return { banner, globalWriteError } } diff --git a/src/infra/dotenv/index.ts b/src/infra/dotenv/index.ts index 4dec0af..490aefa 100644 --- a/src/infra/dotenv/index.ts +++ b/src/infra/dotenv/index.ts @@ -60,7 +60,16 @@ export function loadDotEnv(): DotenvResult { let text: string try { text = readFileSync(path, "utf8") - } catch { + } catch (readErr) { + // The file exists but is unreadable (e.g. permission denied, symlink loop). + // Silently continuing here would leave the user's env vars unapplied with + // no indication of why — they would see mysterious "config not applied" + // behaviour at runtime. We write to stderr (console.error) because this + // is infra/ startup code that runs before any logger is available. + // "File absent" (existsSync === false) stays fully silent — only an + // existing-but-unreadable file emits this warning. + const reason = readErr instanceof Error ? readErr.message : String(readErr) + console.error(`[opencode-pilot] warn: .env file found but unreadable — env vars NOT applied. path=${path} reason=${reason}`) continue } const parsed = parseEnv(text) diff --git a/src/infra/http/text.ts b/src/infra/http/text.ts index e705101..c153753 100644 --- a/src/infra/http/text.ts +++ b/src/infra/http/text.ts @@ -25,14 +25,23 @@ export async function readBoundedText(req: Request, maxBytes: number): Promise maxBytes) { - try { reader.cancel() } catch {} + try { reader.cancel() } catch { + // Provably irrelevant: cancel() is best-effort stream teardown after + // we have already decided to reject the body. Failure here (e.g. the + // request stream was already closed by the client) does not affect + // the null return or downstream handling. + } return null } result += decoder.decode(value, { stream: true }) } result += decoder.decode() } finally { - try { reader.releaseLock() } catch {} + try { reader.releaseLock() } catch { + // Provably irrelevant: releaseLock() is finally-block cleanup. Failure + // (e.g. lock already released by a prior cancel()) does not affect the + // return value or any downstream processing. + } } return result } diff --git a/src/integrations/codex/validators.test.ts b/src/integrations/codex/validators.test.ts index 98033ae..d89eed7 100644 --- a/src/integrations/codex/validators.test.ts +++ b/src/integrations/codex/validators.test.ts @@ -261,3 +261,93 @@ describe("validateStop", () => { if (result.ok) expect(result.data.last_assistant_message).toBe("done") }) }) + +// ─── B1: Input length caps ──────────────────────────────────────────────────── +// session_id/turn_id/tool_use_id ≤ 128, tool_name/cwd/model ≤ 512, prompt ≤ 50_000 + +describe("validateSessionStart — length caps (B1)", () => { + const base = { + session_id: "sess-1", + cwd: "/tmp", + model: "gpt-4o", + permission_mode: "default", + } + + test("rejects session_id > 128 chars", () => { + const result = validateSessionStart({ ...base, session_id: "x".repeat(129) }) + expect(result.ok).toBe(false) + if (!result.ok) expect(result.error).toContain("session_id") + }) + + test("accepts session_id at limit (128 chars)", () => { + const result = validateSessionStart({ ...base, session_id: "x".repeat(128) }) + expect(result.ok).toBe(true) + }) + + test("rejects cwd > 512 chars", () => { + const result = validateSessionStart({ ...base, cwd: "/".repeat(513) }) + expect(result.ok).toBe(false) + if (!result.ok) expect(result.error).toContain("cwd") + }) + + test("rejects model > 512 chars", () => { + const result = validateSessionStart({ ...base, model: "m".repeat(513) }) + expect(result.ok).toBe(false) + if (!result.ok) expect(result.error).toContain("model") + }) + + test("rejects turn_id > 128 chars when provided", () => { + const result = validateSessionStart({ ...base, turn_id: "t".repeat(129) }) + expect(result.ok).toBe(false) + if (!result.ok) expect(result.error).toContain("turn_id") + }) +}) + +describe("validateUserPromptSubmit — length caps (B1)", () => { + const base = { session_id: "sess-1", turn_id: "turn-1", prompt: "hello" } + + test("rejects session_id > 128 chars", () => { + const result = validateUserPromptSubmit({ ...base, session_id: "x".repeat(129) }) + expect(result.ok).toBe(false) + if (!result.ok) expect(result.error).toContain("session_id") + }) + + test("rejects turn_id > 128 chars", () => { + const result = validateUserPromptSubmit({ ...base, turn_id: "t".repeat(129) }) + expect(result.ok).toBe(false) + if (!result.ok) expect(result.error).toContain("turn_id") + }) + + test("rejects prompt > 50_000 chars", () => { + const result = validateUserPromptSubmit({ ...base, prompt: "p".repeat(50_001) }) + expect(result.ok).toBe(false) + if (!result.ok) expect(result.error).toContain("prompt") + }) + + test("accepts prompt at limit (50_000 chars)", () => { + const result = validateUserPromptSubmit({ ...base, prompt: "p".repeat(50_000) }) + expect(result.ok).toBe(true) + }) +}) + +describe("validatePreToolUse — length caps (B1)", () => { + const base = { + session_id: "sess-1", + turn_id: "turn-1", + tool_use_id: "tuid-1", + tool_name: "bash", + tool_input: {}, + } + + test("rejects tool_use_id > 128 chars", () => { + const result = validatePreToolUse({ ...base, tool_use_id: "u".repeat(129) }) + expect(result.ok).toBe(false) + if (!result.ok) expect(result.error).toContain("tool_use_id") + }) + + test("rejects tool_name > 512 chars", () => { + const result = validatePreToolUse({ ...base, tool_name: "n".repeat(513) }) + expect(result.ok).toBe(false) + if (!result.ok) expect(result.error).toContain("tool_name") + }) +}) diff --git a/src/integrations/codex/validators.ts b/src/integrations/codex/validators.ts index aae5210..b0e9166 100644 --- a/src/integrations/codex/validators.ts +++ b/src/integrations/codex/validators.ts @@ -15,13 +15,30 @@ import type { type ValidateResult = { ok: true; data: T } | { ok: false; error: string } +// ─── Field length caps ─────────────────────────────────────────────────────── +// Prevent a single oversized field from fanning a huge string to all SSE clients +// or overloading the audit log. Caps are generous — legitimate values are far +// shorter (UUIDs, model names, short prompts). +const MAX_ID_LEN = 128 // session_id, turn_id, tool_use_id +const MAX_NAME_LEN = 512 // tool_name, cwd, model +const MAX_PROMPT_LEN = 50_000 + function isObject(v: unknown): v is Record { return v !== null && typeof v === "object" && !Array.isArray(v) } -function requireString(b: Record, key: string): string | null { +/** + * Require a non-empty string field. If maxLen is given, also reject values + * that exceed it — returns null in both cases so callers stay uniform. + */ +function requireString( + b: Record, + key: string, + maxLen?: number, +): string | null { const v = b[key] if (typeof v !== "string" || v.length === 0) return null + if (maxLen !== undefined && v.length > maxLen) return null return v } @@ -30,14 +47,15 @@ function requireString(b: Record, key: string): string | null { export function validateSessionStart(body: unknown): ValidateResult { if (!isObject(body)) return { ok: false, error: "body must be a JSON object" } - const session_id = requireString(body, "session_id") - if (session_id === null) return { ok: false, error: "session_id is required and must be a non-empty string" } + const session_id = requireString(body, "session_id", MAX_ID_LEN) + if (session_id === null) return { ok: false, error: `session_id is required and must be a non-empty string (max ${MAX_ID_LEN} chars)` } - const cwd = requireString(body, "cwd") - if (cwd === null) return { ok: false, error: "cwd is required and must be a non-empty string" } + const cwd = requireString(body, "cwd", MAX_NAME_LEN) + if (cwd === null) return { ok: false, error: `cwd is required and must be a non-empty string (max ${MAX_NAME_LEN} chars)` } const model = body.model if (typeof model !== "string" || model.length === 0) return { ok: false, error: "model is required and must be a non-empty string" } + if (model.length > MAX_NAME_LEN) return { ok: false, error: `model must be ${MAX_NAME_LEN} characters or fewer` } const permission_mode = body.permission_mode const validModes = ["default", "acceptEdits", "plan", "dontAsk", "bypassPermissions"] @@ -45,10 +63,17 @@ export function validateSessionStart(body: unknown): ValidateResult 0 && body.turn_id.length <= MAX_ID_LEN + ? body.turn_id + : null) : undefined - if (turn_id === null) return { ok: false, error: "turn_id must be a string when provided" } + if (turn_id === null) return { ok: false, error: `turn_id must be a non-empty string (max ${MAX_ID_LEN} chars) when provided` } return { ok: true, @@ -67,15 +92,16 @@ export function validateSessionStart(body: unknown): ValidateResult { if (!isObject(body)) return { ok: false, error: "body must be a JSON object" } - const session_id = requireString(body, "session_id") - if (session_id === null) return { ok: false, error: "session_id is required and must be a non-empty string" } + const session_id = requireString(body, "session_id", MAX_ID_LEN) + if (session_id === null) return { ok: false, error: `session_id is required and must be a non-empty string (max ${MAX_ID_LEN} chars)` } - const turn_id = requireString(body, "turn_id") - if (turn_id === null) return { ok: false, error: "turn_id is required and must be a non-empty string" } + const turn_id = requireString(body, "turn_id", MAX_ID_LEN) + if (turn_id === null) return { ok: false, error: `turn_id is required and must be a non-empty string (max ${MAX_ID_LEN} chars)` } const prompt = body.prompt if (typeof prompt !== "string") return { ok: false, error: "prompt is required and must be a string" } if (prompt.length === 0) return { ok: false, error: "prompt must not be empty" } + if (prompt.length > MAX_PROMPT_LEN) return { ok: false, error: `prompt must be ${MAX_PROMPT_LEN} characters or fewer` } return { ok: true, data: { session_id, turn_id, prompt } } } @@ -85,17 +111,17 @@ export function validateUserPromptSubmit(body: unknown): ValidateResult { if (!isObject(body)) return { ok: false, error: "body must be a JSON object" } - const session_id = requireString(body, "session_id") - if (session_id === null) return { ok: false, error: "session_id is required and must be a non-empty string" } + const session_id = requireString(body, "session_id", MAX_ID_LEN) + if (session_id === null) return { ok: false, error: `session_id is required and must be a non-empty string (max ${MAX_ID_LEN} chars)` } - const turn_id = requireString(body, "turn_id") - if (turn_id === null) return { ok: false, error: "turn_id is required and must be a non-empty string" } + const turn_id = requireString(body, "turn_id", MAX_ID_LEN) + if (turn_id === null) return { ok: false, error: `turn_id is required and must be a non-empty string (max ${MAX_ID_LEN} chars)` } - const tool_use_id = requireString(body, "tool_use_id") - if (tool_use_id === null) return { ok: false, error: "tool_use_id is required and must be a non-empty string" } + const tool_use_id = requireString(body, "tool_use_id", MAX_ID_LEN) + if (tool_use_id === null) return { ok: false, error: `tool_use_id is required and must be a non-empty string (max ${MAX_ID_LEN} chars)` } - const tool_name = requireString(body, "tool_name") - if (tool_name === null) return { ok: false, error: "tool_name is required and must be a non-empty string" } + const tool_name = requireString(body, "tool_name", MAX_NAME_LEN) + if (tool_name === null) return { ok: false, error: `tool_name is required and must be a non-empty string (max ${MAX_NAME_LEN} chars)` } if (!("tool_input" in body)) return { ok: false, error: "tool_input is required" } const tool_input: unknown = body.tool_input @@ -108,17 +134,17 @@ export function validatePreToolUse(body: unknown): ValidateResult { if (!isObject(body)) return { ok: false, error: "body must be a JSON object" } - const session_id = requireString(body, "session_id") - if (session_id === null) return { ok: false, error: "session_id is required and must be a non-empty string" } + const session_id = requireString(body, "session_id", MAX_ID_LEN) + if (session_id === null) return { ok: false, error: `session_id is required and must be a non-empty string (max ${MAX_ID_LEN} chars)` } - const turn_id = requireString(body, "turn_id") - if (turn_id === null) return { ok: false, error: "turn_id is required and must be a non-empty string" } + const turn_id = requireString(body, "turn_id", MAX_ID_LEN) + if (turn_id === null) return { ok: false, error: `turn_id is required and must be a non-empty string (max ${MAX_ID_LEN} chars)` } - const tool_use_id = requireString(body, "tool_use_id") - if (tool_use_id === null) return { ok: false, error: "tool_use_id is required and must be a non-empty string" } + const tool_use_id = requireString(body, "tool_use_id", MAX_ID_LEN) + if (tool_use_id === null) return { ok: false, error: `tool_use_id is required and must be a non-empty string (max ${MAX_ID_LEN} chars)` } - const tool_name = requireString(body, "tool_name") - if (tool_name === null) return { ok: false, error: "tool_name is required and must be a non-empty string" } + const tool_name = requireString(body, "tool_name", MAX_NAME_LEN) + if (tool_name === null) return { ok: false, error: `tool_name is required and must be a non-empty string (max ${MAX_NAME_LEN} chars)` } // tool_response can be any value (including undefined/null — Codex may omit it on errors) const tool_response = body.tool_response !== undefined ? body.tool_response : null @@ -134,14 +160,14 @@ export function validatePostToolUse(body: unknown): ValidateResult { if (!isObject(body)) return { ok: false, error: "body must be a JSON object" } - const session_id = requireString(body, "session_id") - if (session_id === null) return { ok: false, error: "session_id is required and must be a non-empty string" } + const session_id = requireString(body, "session_id", MAX_ID_LEN) + if (session_id === null) return { ok: false, error: `session_id is required and must be a non-empty string (max ${MAX_ID_LEN} chars)` } - const turn_id = requireString(body, "turn_id") - if (turn_id === null) return { ok: false, error: "turn_id is required and must be a non-empty string" } + const turn_id = requireString(body, "turn_id", MAX_ID_LEN) + if (turn_id === null) return { ok: false, error: `turn_id is required and must be a non-empty string (max ${MAX_ID_LEN} chars)` } - const tool_name = requireString(body, "tool_name") - if (tool_name === null) return { ok: false, error: "tool_name is required and must be a non-empty string" } + const tool_name = requireString(body, "tool_name", MAX_NAME_LEN) + if (tool_name === null) return { ok: false, error: `tool_name is required and must be a non-empty string (max ${MAX_NAME_LEN} chars)` } if (!("tool_input" in body)) return { ok: false, error: "tool_input is required" } const tool_input: unknown = body.tool_input @@ -154,8 +180,8 @@ export function validatePermissionRequest(body: unknown): ValidateResult { if (!isObject(body)) return { ok: false, error: "body must be a JSON object" } - const session_id = requireString(body, "session_id") - if (session_id === null) return { ok: false, error: "session_id is required and must be a non-empty string" } + const session_id = requireString(body, "session_id", MAX_ID_LEN) + if (session_id === null) return { ok: false, error: `session_id is required and must be a non-empty string (max ${MAX_ID_LEN} chars)` } const stop_hook_active = body.stop_hook_active if (typeof stop_hook_active !== "boolean") { diff --git a/src/server/index.ts b/src/server/index.ts index 506385c..cdb7e0c 100644 --- a/src/server/index.ts +++ b/src/server/index.ts @@ -253,13 +253,28 @@ export default { deps.tunnelUrl = tunnel.publicUrl const localUrl = `http://${config.host}:${config.port}` - await writeBanner({ + const { globalWriteError } = await writeBanner({ localUrl, publicUrl: tunnel.publicUrl, token: currentToken, directory: ctx.directory, projectStateMode: config.projectStateMode, }) + if (globalWriteError) { + // The global banner (the path the TUI reads for the connect URL) failed + // to write. The user will not see the URL in the TUI unless it succeeds. + // Log a warn so the problem is visible instead of silently swallowed. + await ctx.client.app + .log({ + body: { + service: "opencode-pilot", + level: "warn", + message: `[banner] global banner write failed: ${globalWriteError}`, + extra: { path: "~/.opencode-pilot/pilot-banner.txt", error: globalWriteError }, + }, + }) + .catch(() => {}) + } const tokenPreview = currentToken.length > 10 diff --git a/src/transport/http/handlers/dashboard.ts b/src/transport/http/handlers/dashboard.ts index 3432480..c76bde2 100644 --- a/src/transport/http/handlers/dashboard.ts +++ b/src/transport/http/handlers/dashboard.ts @@ -102,10 +102,28 @@ function loadAssetsIntoMemory(): void { }) } catch { // Skip unreadable files — they just return 404 at request time. + // Critical assets are checked after the full walk (see below). } } } walk(DASHBOARD_DIR, "") + + // Post-load assertion: verify that critical assets are in the cache. + // A corrupted or missing critical file (e.g. main.js, styles.css, sw.js) + // would silently produce 404s at request time, leaving the dashboard + // visually broken with no indication of why. + // We do NOT crash here — the server can still run and might serve partial + // content — but we log a clear warning so the problem is visible immediately + // at startup rather than only when a user opens the dashboard. + const CRITICAL_ASSETS = ["main.js", "styles.css", "sw.js"] + const missing = CRITICAL_ASSETS.filter((name) => !assetCache.has(name)) + if (missing.length > 0) { + console.error( + `[opencode-pilot] warn: dashboard asset cache is missing critical files after load: [${missing.join(", ")}]. ` + + `These will return 404 at runtime and leave the dashboard broken. ` + + `Check that the package is installed correctly (try: bunx @lesquel/opencode-pilot@latest init).`, + ) + } } // Lazy-load on first dashboard request so import order doesn't matter. diff --git a/src/transport/http/handlers/filesystem.test.ts b/src/transport/http/handlers/filesystem.test.ts new file mode 100644 index 0000000..c43b87b --- /dev/null +++ b/src/transport/http/handlers/filesystem.test.ts @@ -0,0 +1,129 @@ +// RED: filesystem handler hardening — A: glob pattern traversal +// Tests for /fs/glob pattern `..` rejection and post-glob containment. +// +// These tests exercise `globFiles` via a minimal fake RouteContext — +// no real HTTP server needed. + +import { describe, it, expect, beforeEach, afterEach } from "bun:test" +import { mkdtempSync, writeFileSync, mkdirSync, rmSync } from "fs" +import { tmpdir } from "os" +import { join } from "path" +import { globFiles } from "./filesystem" + +// ─── Fake RouteContext ──────────────────────────────────────────────────────── + +function makeCtx( + params: Record, + dir: string, + enableGlob = true, +): Parameters[0] { + const url = new URL("http://localhost/fs/glob") + for (const [k, v] of Object.entries(params)) { + if (v !== null) url.searchParams.set(k, v) + } + // Cast through unknown: the fake only supplies the fields that globFiles + // actually reads. The full RouteDeps interface requires many injected deps + // (client, permissionQueue, etc.) that globFiles never touches. + return { + url, + req: new Request(url), + params: {}, + deps: { + config: { + enableGlobOpener: enableGlob, + dev: false, + } as Parameters[0]["deps"]["config"], + directory: dir, + worktree: dir, + audit: { + log: () => {}, + }, + logger: { + debug: () => {}, + info: () => {}, + warn: () => {}, + error: () => {}, + }, + } as unknown as Parameters[0]["deps"], + } +} + +// ─── Tests ──────────────────────────────────────────────────────────────────── + +describe("globFiles — pattern traversal guard (A)", () => { + let dir: string + + beforeEach(() => { + dir = mkdtempSync(join(tmpdir(), "pilot-glob-test-")) + // Create a real file inside to ensure a normal pattern works + writeFileSync(join(dir, "hello.ts"), "export const x = 1") + }) + + afterEach(() => { + try { + rmSync(dir, { recursive: true, force: true }) + } catch {} + }) + + it("rejects a pattern containing .. with 403", async () => { + const ctx = makeCtx({ pattern: "../../etc/*", cwd: dir }, dir) + const res = await globFiles(ctx) + expect(res.status).toBe(403) + const body = await res.json() as { error: { code: string } } + expect(body.error.code).toBe("FORBIDDEN") + }) + + it("rejects a pattern like ../../secrets/file.txt with 403", async () => { + const ctx = makeCtx({ pattern: "../../secrets/file.txt" }, dir) + const res = await globFiles(ctx) + expect(res.status).toBe(403) + const body = await res.json() as { error: { code: string } } + expect(body.error.code).toBe("FORBIDDEN") + }) + + it("rejects an absolute pattern like /etc/* with 403", async () => { + const ctx = makeCtx({ pattern: "/etc/*", cwd: dir }, dir) + const res = await globFiles(ctx) + expect(res.status).toBe(403) + const body = await res.json() as { error: { code: string } } + expect(body.error.code).toBe("FORBIDDEN") + }) + + it("rejects an absolute pattern like /root/secret with 403", async () => { + const ctx = makeCtx({ pattern: "/root/secret" }, dir) + const res = await globFiles(ctx) + expect(res.status).toBe(403) + const body = await res.json() as { error: { code: string } } + expect(body.error.code).toBe("FORBIDDEN") + }) + + it("allows a normal pattern and returns matching files", async () => { + const ctx = makeCtx({ pattern: "*.ts", cwd: dir }, dir) + const res = await globFiles(ctx) + expect(res.status).toBe(200) + const body = await res.json() as { files: Array<{ path: string; absolute: string }> } + expect(body.files.length).toBeGreaterThanOrEqual(1) + expect(body.files[0].path).toBe("hello.ts") + }) + + it("post-glob: results only contain paths inside cwd (belt-and-suspenders)", async () => { + const ctx = makeCtx({ pattern: "*.ts", cwd: dir }, dir) + const res = await globFiles(ctx) + expect(res.status).toBe(200) + const body = await res.json() as { files: Array<{ absolute: string }>; cwd: string } + for (const file of body.files) { + // Use proper path-segment boundary to avoid prefix collision: + // e.g. cwd=/tmp/root must NOT match absolute=/tmp/root-evil/x.ts + const cwdWithSlash = body.cwd.endsWith("/") ? body.cwd : body.cwd + "/" + expect( + file.absolute === body.cwd || file.absolute.startsWith(cwdWithSlash), + ).toBe(true) + } + }) + + it("returns 403 when glob is disabled", async () => { + const ctx = makeCtx({ pattern: "*.ts" }, dir, false) + const res = await globFiles(ctx) + expect(res.status).toBe(403) + }) +}) diff --git a/src/transport/http/handlers/filesystem.ts b/src/transport/http/handlers/filesystem.ts index 186a778..0c12d3d 100644 --- a/src/transport/http/handlers/filesystem.ts +++ b/src/transport/http/handlers/filesystem.ts @@ -109,6 +109,20 @@ export async function globFiles({ url, deps }: RouteContext): Promise const pattern = url.searchParams.get("pattern") if (!pattern) return jsonError("MISSING_PATTERN", "pattern is required", 400, CORS_HEADERS) + // Block directory traversal and absolute patterns. + // + // `..` rejection: Bun.Glob with a `../`-prefixed pattern and onlyFiles:true + // can yield relative paths that escape the scanned cwd, so the absolute path + // returned in the response would disclose paths outside the project root. + // + // Absolute-pattern rejection: Bun.Glob honours absolute globs at the OS level + // (e.g. `/etc/*`). Even though the post-glob containment check would skip any + // matching entry, the response `path` field would still disclose real filenames + // under `/etc`. Reject via isAbsolute (node:path) — POSIX-correct for this + // Linux/macOS target; the post-glob containment check is the backstop. + if (pattern.includes("..") || isAbsolute(pattern)) + return jsonError("FORBIDDEN", "Pattern traversal not allowed", 403, CORS_HEADERS) + const cwdParam = url.searchParams.get("cwd") const cwd = cwdParam && cwdParam.length > 0 ? cwdParam : deps.directory const limit = parseLimit(url.searchParams.get("limit"), GLOB_DEFAULT_LIMIT, GLOB_MAX_LIMIT) @@ -126,13 +140,26 @@ export async function globFiles({ url, deps }: RouteContext): Promise const results: Array<{ path: string; absolute: string; mtime: number; size: number }> = [] for await (const rel of glob.scan({ cwd: cwdSafe.resolved, onlyFiles: true })) { const abs = join(cwdSafe.resolved, rel) + + // Belt-and-suspenders: verify the resolved absolute path is still under + // cwdSafe.resolved. Bun.Glob with onlyFiles:true should never escape the + // scanned cwd (and the `..` pattern check above fires first), but if a + // future Bun version or a symlink edge-case yields an out-of-root path we + // silently skip it rather than disclose it. + if (abs !== cwdSafe.resolved && !abs.startsWith(cwdSafe.resolved + "/")) { + continue + } + let mtime = 0 let size = 0 try { const st = statSync(abs) mtime = st.mtimeMs size = st.size - } catch {} + } catch { + // statSync failing on a globbed path is a transient race (file deleted + // between glob scan and stat). Skip the file silently — it's already gone. + } results.push({ path: rel, absolute: abs, mtime, size }) if (results.length >= limit) break } diff --git a/src/transport/http/validators/sessions.test.ts b/src/transport/http/validators/sessions.test.ts new file mode 100644 index 0000000..8442b55 --- /dev/null +++ b/src/transport/http/validators/sessions.test.ts @@ -0,0 +1,222 @@ +// RED: sessions-validators hardening — B2 +// validatePromptBody: parts per-element validation, model field length caps, +// agent length cap. + +import { describe, it, expect } from "bun:test" +import { + validatePromptBody, + validateCreateSession, + validateUpdateSession, +} from "./sessions" + +// ─── validatePromptBody — existing happy paths ──────────────────────────────── + +describe("validatePromptBody — happy paths", () => { + it("accepts a valid message", () => { + const result = validatePromptBody({ message: "hello" }) + expect(result.ok).toBe(true) + if (result.ok) expect(result.data.message).toBe("hello") + }) + + it("accepts valid parts array", () => { + const result = validatePromptBody({ + parts: [{ type: "text", text: "hi" }], + }) + expect(result.ok).toBe(true) + if (result.ok) expect(result.data.parts?.length).toBe(1) + }) + + it("accepts message with valid model", () => { + const result = validatePromptBody({ + message: "hi", + model: { providerID: "anthropic", modelID: "claude-3-5-sonnet" }, + }) + expect(result.ok).toBe(true) + }) + + it("accepts message with valid agent", () => { + const result = validatePromptBody({ message: "hi", agent: "coding" }) + expect(result.ok).toBe(true) + }) +}) + +// ─── validatePromptBody — B2: parts per-element validation ─────────────────── + +describe("validatePromptBody — parts per-element validation (B2)", () => { + it("rejects parts containing a null element", () => { + const result = validatePromptBody({ parts: [null] }) + expect(result.ok).toBe(false) + if (!result.ok) expect(result.error).toMatch(/part|element/i) + }) + + it("rejects parts containing a non-object element (string)", () => { + const result = validatePromptBody({ parts: ["text-string"] }) + expect(result.ok).toBe(false) + if (!result.ok) expect(result.error).toMatch(/part|element/i) + }) + + it("rejects parts containing an element missing type", () => { + const result = validatePromptBody({ parts: [{ text: "hi" }] }) + expect(result.ok).toBe(false) + if (!result.ok) expect(result.error).toMatch(/type|part/i) + }) + + it("rejects parts containing an element with empty-string type", () => { + const result = validatePromptBody({ parts: [{ type: "" }] }) + expect(result.ok).toBe(false) + if (!result.ok) expect(result.error).toMatch(/type|part/i) + }) + + it("rejects parts containing an element with non-string type (number)", () => { + const result = validatePromptBody({ parts: [{ type: 42 }] }) + expect(result.ok).toBe(false) + if (!result.ok) expect(result.error).toMatch(/type|part/i) + }) + + it("accepts parts where every element is valid", () => { + const result = validatePromptBody({ + parts: [{ type: "text", text: "hi" }, { type: "image_url", url: "http://x.com/img.png" }], + }) + expect(result.ok).toBe(true) + }) +}) + +// ─── validatePromptBody — B2: dashboard regression (empty providerID is valid) ─ + +describe("validatePromptBody — dashboard regression: empty providerID (B2)", () => { + // The dashboard sends providerID:'' when no provider override is selected + // (sessions.js:748 opts.providerID = modelPref.providerId ?? ''). + // The validator MUST accept this — rejecting it silently drops the prompt. + it("accepts the exact dashboard payload: model.providerID='' with a valid modelID", () => { + const result = validatePromptBody({ + message: "hi", + model: { modelID: "claude-x", providerID: "" }, + }) + expect(result.ok).toBe(true) + if (result.ok) { + expect(result.data.model?.modelID).toBe("claude-x") + expect(result.data.model?.providerID).toBe("") + } + }) + + it("still rejects empty modelID (meaningful field)", () => { + const result = validatePromptBody({ + message: "hi", + model: { modelID: "", providerID: "anthropic" }, + }) + expect(result.ok).toBe(false) + if (!result.ok) expect(result.error).toMatch(/modelID/i) + }) + + it("still accepts a normal message with no model (baseline unchanged)", () => { + const result = validatePromptBody({ message: "hello" }) + expect(result.ok).toBe(true) + if (result.ok) expect(result.data.model).toBeUndefined() + }) +}) + +// ─── validatePromptBody — B2: model field length caps ──────────────────────── + +describe("validatePromptBody — model length caps (B2)", () => { + it("rejects model.providerID > 200 chars", () => { + const result = validatePromptBody({ + message: "hi", + model: { providerID: "p".repeat(201), modelID: "claude-3-5" }, + }) + expect(result.ok).toBe(false) + if (!result.ok) expect(result.error).toMatch(/providerID/i) + }) + + it("rejects model.modelID > 200 chars", () => { + const result = validatePromptBody({ + message: "hi", + model: { providerID: "anthropic", modelID: "m".repeat(201) }, + }) + expect(result.ok).toBe(false) + if (!result.ok) expect(result.error).toMatch(/modelID/i) + }) + + it("accepts model at limit (200 chars each)", () => { + const result = validatePromptBody({ + message: "hi", + model: { providerID: "p".repeat(200), modelID: "m".repeat(200) }, + }) + expect(result.ok).toBe(true) + }) + + it("accepts empty providerID (dashboard sends '' when no provider is selected)", () => { + // Contract: providerID MAY be empty — the meaningful field is modelID. + // The dashboard sets providerID: modelPref.providerId ?? '' so empty is a + // valid real-world value, not a programmer error. + const result = validatePromptBody({ + message: "hi", + model: { providerID: "", modelID: "claude-3-5" }, + }) + expect(result.ok).toBe(true) + }) + + it("rejects empty modelID", () => { + const result = validatePromptBody({ + message: "hi", + model: { providerID: "anthropic", modelID: "" }, + }) + expect(result.ok).toBe(false) + if (!result.ok) expect(result.error).toMatch(/modelID/i) + }) +}) + +// ─── validatePromptBody — B2: agent length cap ─────────────────────────────── + +describe("validatePromptBody — agent length cap (B2)", () => { + it("rejects agent > 200 chars", () => { + const result = validatePromptBody({ message: "hi", agent: "a".repeat(201) }) + expect(result.ok).toBe(false) + if (!result.ok) expect(result.error).toMatch(/agent/i) + }) + + it("accepts agent at limit (200 chars)", () => { + const result = validatePromptBody({ message: "hi", agent: "a".repeat(200) }) + expect(result.ok).toBe(true) + }) +}) + +// ─── validateCreateSession & validateUpdateSession — pre-existing coverage ─── + +describe("validateCreateSession — baseline", () => { + it("accepts empty body", () => { + const result = validateCreateSession({}) + expect(result.ok).toBe(true) + }) + + it("accepts body with valid title", () => { + const result = validateCreateSession({ title: "My session" }) + expect(result.ok).toBe(true) + }) + + it("rejects non-string title", () => { + const result = validateCreateSession({ title: 123 }) + expect(result.ok).toBe(false) + }) + + it("rejects title > 200 chars", () => { + const result = validateCreateSession({ title: "t".repeat(201) }) + expect(result.ok).toBe(false) + }) +}) + +describe("validateUpdateSession — baseline", () => { + it("accepts valid title", () => { + const result = validateUpdateSession({ title: "New name" }) + expect(result.ok).toBe(true) + }) + + it("rejects missing title", () => { + const result = validateUpdateSession({}) + expect(result.ok).toBe(false) + }) + + it("rejects empty title", () => { + const result = validateUpdateSession({ title: " " }) + expect(result.ok).toBe(false) + }) +}) diff --git a/src/transport/http/validators/sessions.ts b/src/transport/http/validators/sessions.ts index 26f7426..1979a9f 100644 --- a/src/transport/http/validators/sessions.ts +++ b/src/transport/http/validators/sessions.ts @@ -87,6 +87,22 @@ export function validatePromptBody( } } + // Validate parts elements: each must be a non-null object with a non-empty string `type`. + // A malformed element from an untrusted caller could propagate an unusable part + // shape to the SDK and produce a cryptic downstream error. + if (Array.isArray(b.parts)) { + for (let i = 0; i < b.parts.length; i++) { + const el = b.parts[i] + if (el === null || typeof el !== "object" || Array.isArray(el)) { + return { ok: false, error: `parts[${i}]: each part element must be a non-null object` } + } + const part = el as Record + if (typeof part.type !== "string" || part.type.length === 0) { + return { ok: false, error: `parts[${i}]: each part element must have a non-empty string "type"` } + } + } + } + if (b.model !== undefined) { if (b.model === null || typeof b.model !== "object" || Array.isArray(b.model)) { return { ok: false, error: "model must be an object with providerID and modelID" } @@ -95,11 +111,29 @@ export function validatePromptBody( if (typeof m.providerID !== "string" || typeof m.modelID !== "string") { return { ok: false, error: "model.providerID and model.modelID must be strings" } } + // modelID MUST be non-empty — it's the meaningful routing key for the SDK. + // providerID MAY be empty: the dashboard sends '' when no provider override is + // selected (sessions.js: opts.providerID = modelPref.providerId ?? ''), so + // rejecting empty providerID would silently drop legitimate prompts. + if (m.modelID.length === 0) { + return { ok: false, error: "model.modelID must not be empty" } + } + // Length caps prevent oversized strings from propagating to SSE clients / audit. + if (m.providerID.length > 200) { + return { ok: false, error: "model.providerID must be 200 characters or fewer" } + } + if (m.modelID.length > 200) { + return { ok: false, error: "model.modelID must be 200 characters or fewer" } + } } if (b.agent !== undefined && typeof b.agent !== "string") { return { ok: false, error: "agent must be a string" } } + // Cap agent length for the same SSE/audit overflow reason as model fields. + if (typeof b.agent === "string" && b.agent.length > 200) { + return { ok: false, error: "agent must be 200 characters or fewer" } + } return { ok: true,