diff --git a/src/main/opencode/hook-service.test.ts b/src/main/opencode/hook-service.test.ts index bc68917c6..bbd60d656 100644 --- a/src/main/opencode/hook-service.test.ts +++ b/src/main/opencode/hook-service.test.ts @@ -1,6 +1,8 @@ import { describe, expect, it } from 'vitest' import { _internals } from './hook-service' +const { isUsableId, toSafeDirName } = _internals + describe('OpenCode hook plugin source', () => { it('filters child sessions via parentID lookup before forwarding events', () => { const source = _internals.getOpenCodePluginSource() @@ -19,3 +21,48 @@ describe('OpenCode hook plugin source', () => { expect(source).toContain('const client = _ctx?.client;') }) }) + +describe('OpenCode id safety guard', () => { + it('accepts the daemon-path sessionId shape (worktreeId@@uuid with ::/...)', () => { + // Why: after the daemon-parity refactor (#1148) pty.ts mints sessionIds + // like `@@` where worktreeId contains "::" and a + // filesystem path. The previous strict regex rejected every real id and + // silently dropped OPENCODE_CONFIG_DIR. Lock in that such ids are now + // accepted so the plugin dir is actually written. + const daemonSessionId = + '50c010a2-bc8e-4eb1-8847-5812133ad6df::/Users/thebr/ghostx/workspaces/noqa/autoheal@@a1b2c3d4' + expect(isUsableId(daemonSessionId)).toBe(true) + }) + + it('accepts ids at the inclusive upper length bound', () => { + expect(isUsableId('x'.repeat(1024))).toBe(true) + }) + + it('rejects empty or oversized ids', () => { + expect(isUsableId('')).toBe(false) + expect(isUsableId('x'.repeat(1025))).toBe(false) + }) + + it('rejects non-string runtime values even though the type says string', () => { + // Why: the typeof guard is defense-in-depth for any-typed callers; + // without a test, a future refactor could delete the guard silently. + expect(isUsableId(undefined as unknown as string)).toBe(false) + expect(isUsableId(null as unknown as string)).toBe(false) + expect(isUsableId(42 as unknown as string)).toBe(false) + }) + + it('derives a filesystem-safe directory name independent of the raw id', () => { + const name = toSafeDirName('50c010::/Users/thebr/x/y@@uuid') + // Pure hex, bounded length — no slashes, colons, or caller content. + expect(name).toMatch(/^[0-9a-f]{32}$/) + }) + + it('is stable across calls for the same id', () => { + const id = 'some-session-id' + expect(toSafeDirName(id)).toBe(toSafeDirName(id)) + }) + + it('produces different names for different ids', () => { + expect(toSafeDirName('a')).not.toBe(toSafeDirName('b')) + }) +}) diff --git a/src/main/opencode/hook-service.ts b/src/main/opencode/hook-service.ts index 53da57e8f..dca80d6c2 100644 --- a/src/main/opencode/hook-service.ts +++ b/src/main/opencode/hook-service.ts @@ -1,21 +1,38 @@ import { app } from 'electron' import { join } from 'path' import { mkdirSync, writeFileSync, rmSync } from 'fs' +import { createHash } from 'crypto' const ORCA_OPENCODE_PLUGIN_FILE = 'orca-opencode-status.js' -// Why: ptyId today is allocated by Orca (safe UUID-shape), but both entry -// points construct a filesystem path with it and one of them calls -// rmSync(..., recursive) on the result. Reject obviously unsafe IDs as a -// belt-and-braces guard so a future caller (or a bug that forwards an -// external ID) cannot escape userData/opencode-hooks/. -function isSafePtyId(ptyId: string): boolean { - if (!ptyId || ptyId.length === 0 || ptyId.length > 128) { - return false - } - // Allow alphanumeric, dash, underscore, period (but not leading period or - // any slashes/backslashes). - return /^[A-Za-z0-9_-][A-Za-z0-9_.-]*$/.test(ptyId) && !ptyId.includes('..') +// Why: the id passed in by pty.ts's daemon path is a sessionId shaped like +// "@@" where worktreeId itself contains "::" and a +// filesystem path (slashes, colons). Earlier the id was a simple numeric +// counter, so rejecting anything with "/" or ":" was a safe guard against +// path traversal. After the daemon-parity refactor (#1148) the sessionId +// shape changed, and the old regex silently rejected every legitimate id, +// leaving OPENCODE_CONFIG_DIR unset and the plugin never loading. +// +// Keep an input-bounds guard (non-empty, bounded length) for defense in +// depth, and derive the on-disk directory name via hash so any caller's id — +// including ones containing path separators — produces a short, stable, +// filesystem-safe name. Hashing also eliminates path-traversal risk at the +// source: the directory name is always 32 hex chars, never a prefix/suffix +// of the caller's input. +// Why: 1024 is a generous sanity cap — daemon-shaped ids embed a worktree +// filesystem path plus "@@", and this bound prevents pathological inputs +// from burning CPU in the SHA-256 step. Since the id is hashed anyway, 1024 +// is decoupled from PATH_MAX. +function isUsableId(id: string): boolean { + return typeof id === 'string' && id.length > 0 && id.length <= 1024 +} + +function toSafeDirName(id: string): string { + // Why: SHA-256 truncated to 32 hex chars (128 bits) is ample for a + // per-session directory name — collisions require ~2^64 concurrent sessions + // to become likely, far beyond any real workload. Hex keeps the name + // portable across all filesystems (no base64 padding, no `/`). + return createHash('sha256').update(id).digest('hex').slice(0, 32) } function getOpenCodePluginSource(): string { @@ -210,13 +227,13 @@ function getOpenCodePluginSource(): string { // pipeline as Claude/Codex/Gemini. export class OpenCodeHookService { clearPty(ptyId: string): void { - if (!isSafePtyId(ptyId)) { + if (!isUsableId(ptyId)) { return } - // Why: writePluginConfig creates a directory per PTY under userData. Without - // cleanup these accumulate across sessions since ptyId is a monotonically - // increasing counter. Remove the directory when the PTY is torn down. - const configDir = join(app.getPath('userData'), 'opencode-hooks', ptyId) + // Why: writePluginConfig creates a directory per PTY under userData. + // Without cleanup these accumulate across sessions. Using getConfigDir + // keeps cleanup aligned with the path writePluginConfig created. + const configDir = this.getConfigDir(ptyId) try { rmSync(configDir, { recursive: true, force: true }) } catch { @@ -242,11 +259,15 @@ export class OpenCodeHookService { return { OPENCODE_CONFIG_DIR: configDir } } + private getConfigDir(ptyId: string): string { + return join(app.getPath('userData'), 'opencode-hooks', toSafeDirName(ptyId)) + } + private writePluginConfig(ptyId: string): string | null { - if (!isSafePtyId(ptyId)) { + if (!isUsableId(ptyId)) { return null } - const configDir = join(app.getPath('userData'), 'opencode-hooks', ptyId) + const configDir = this.getConfigDir(ptyId) const pluginsDir = join(configDir, 'plugins') try { mkdirSync(pluginsDir, { recursive: true }) @@ -264,5 +285,6 @@ export class OpenCodeHookService { export const openCodeHookService = new OpenCodeHookService() export const _internals = { getOpenCodePluginSource, - isSafePtyId + isUsableId, + toSafeDirName }