From 97830d40a1b2d4b72ff180c895c2ad328c417ee3 Mon Sep 17 00:00:00 2001 From: Merge Sim Date: Mon, 7 Sep 2026 11:25:12 -0700 Subject: [PATCH] fix(native-chat): harden the Codex naming thread and narrow its request gate Open the throwaway naming thread with approvalPolicy 'never' and sandbox 'read-only'. The prompt embeds untrusted user message text and the thread's frames are diverted from the journal, so any tool the host would auto-approve ran where the user could never see it; refusing server requests only covered the tools that ask. Require an exact naming-thread id on the server-REQUEST path. During the thread/start window no naming thread has a turn running, so the broad pre-id rule could only ever match a genuine sub-agent, whose approval request was auto-refused with -32001 instead of reaching the user. Also caps the structured answer before parsing it, normalizes the generated name before it is written to the user's real thread, and releases the throwaway thread with the protocol's own thread/unsubscribe. --- ...codex-conversation-name-generation.test.ts | 66 ++++++++++++++ .../codex-conversation-name-generation.ts | 90 ++++++++++++++++--- .../codex/codex-structured-session-adapter.ts | 7 +- ...ructured-session-conversation-name.test.ts | 28 ++++++ 4 files changed, 176 insertions(+), 15 deletions(-) diff --git a/src/main/codex/codex-conversation-name-generation.test.ts b/src/main/codex/codex-conversation-name-generation.test.ts index 16f4e50562f..febf0f288c1 100644 --- a/src/main/codex/codex-conversation-name-generation.test.ts +++ b/src/main/codex/codex-conversation-name-generation.test.ts @@ -98,6 +98,72 @@ describe('readCodexGeneratedTitle', () => { }) }) +describe('naming thread hardening', () => { + it('opens the throwaway thread with no approvals and no write access', async () => { + const { connection, calls } = fakeConnection() + + await run(connection, '{"title":"Fix lease probe"}') + + // The prompt embeds untrusted user text and this thread's frames never reach + // the journal, so anything the host would auto-approve would run unseen. + expect(calls.find((call) => call.method === 'thread/start')?.params).toEqual({ + cwd: '/work/repo', + ephemeral: true, + approvalPolicy: 'never', + sandbox: 'read-only' + }) + }) + + it('releases the throwaway thread with the protocol cleanup', async () => { + const { connection, calls } = fakeConnection() + + await run(connection, '{"title":"Fix lease probe"}') + + expect(calls.find((call) => call.method === 'thread/unsubscribe')?.params).toEqual({ + threadId: NAMING + }) + }) + + it('never unsubscribes the user own thread when the reply named it', async () => { + const calls: { method: string; params: Record }[] = [] + const connection: Pick = { + request: vi.fn(async (method: string, params?: Record) => { + calls.push({ method, params: params ?? {} }) + // The reply names the SESSION's thread; unsubscribing it would cut the + // user's chat off from every frame it depends on. + return method === 'thread/start' ? { thread: { id: THREAD } } : {} + }) + } + + await expect(run(connection, '{"title":"Fix lease probe"}')).resolves.toEqual({ + name: null, + settled: false + }) + expect(calls.map((call) => call.method)).not.toContain('thread/unsubscribe') + }) + + it('refuses to parse an answer far larger than any title', async () => { + // The 36-character cap is a schema request to the model, not a bound the + // host enforces on the reply. + const huge = `{"title":"${'a'.repeat(9 * 1024)}"}` + + expect(readCodexGeneratedTitle(huge)).toBeNull() + }) + + it('flattens and bounds the name before it reaches the user Codex thread', async () => { + const { connection, calls } = fakeConnection() + const sprawling = `Fix\nthe lease probe ${'x'.repeat(400)}` + + await run(connection, JSON.stringify({ title: sprawling })) + + const set = calls.find((call) => call.method === 'thread/name/set') + const name = String(set?.params.name) + expect(name).not.toContain('\n') + expect(name.length).toBeLessThanOrEqual(200) + expect(name.startsWith('Fix the lease probe ')).toBe(true) + }) +}) + describe('createCodexNamingTurnCollector', () => { it('reports a completed turn that said nothing as a DECLINE', async () => { const collector = createCodexNamingTurnCollector(60_000) diff --git a/src/main/codex/codex-conversation-name-generation.ts b/src/main/codex/codex-conversation-name-generation.ts index 3082f72870e..d5cb3bc3285 100644 --- a/src/main/codex/codex-conversation-name-generation.ts +++ b/src/main/codex/codex-conversation-name-generation.ts @@ -22,9 +22,25 @@ // takes seconds and another client may have named the thread in the meantime; a // name a person chose must never lose to one Orca inferred. +import { normalizeAgentSessionConversationName } from '../../shared/agent-session-conversation-name' import type { CodexAppServerConnection } from './codex-app-server-connection' import { readCodexThreadId, readCodexThreadName } from './codex-structured-thread-facts' +/** + * The throwaway thread runs with no approvals and no write access. + * + * The prompt embeds the user's own message text, which is untrusted, and this + * thread's frames are deliberately kept out of the journal — so any tool the + * host would auto-approve runs where the user can never see it. Orca refuses + * server requests on this thread as well, but that only covers the ones that + * ask; these two params cover the ones that do not. + */ +const NAMING_THREAD_APPROVAL_POLICY = 'never' +const NAMING_THREAD_SANDBOX = 'read-only' + +/** Past this an answer is not a title, and parsing it is wasted work. */ +const NAMING_ANSWER_MAX_BYTES = 8 * 1024 + /** Short enough to read as a tab label at a glance; also the schema's own cap. */ export const CODEX_CONVERSATION_NAME_MAX_LENGTH = 36 @@ -98,13 +114,35 @@ export type CodexNamingState = { * has. */ export function isCodexNamingFrame(state: CodexNamingState, frameThreadId: string | null): boolean { + if (isCodexNamingThread(state, frameThreadId)) { + return true + } + return ( + frameThreadId !== null && + frameThreadId !== state.threadId && + state.naming !== null && + state.namingThreadIds.size === 0 + ) +} + +/** + * The exact-match half, for the server-REQUEST path. + * + * The broad pre-id rule must not apply there. A request only arrives from a + * thread with a turn running, and the naming thread's `turn/start` is sent after + * its id is known — so inside the `thread/start` window the broad rule can only + * ever match a genuine sub-agent, whose approval request would then be refused + * with -32001 instead of reaching the user. On the notification path the same + * rule costs at most one wasted naming attempt, which is why it stays there. + */ +export function isCodexNamingThread( + state: CodexNamingState, + frameThreadId: string | null +): boolean { if (frameThreadId === null || frameThreadId === state.threadId) { return false } - if (state.namingThreadIds.has(frameThreadId)) { - return true - } - return state.naming !== null && state.namingThreadIds.size === 0 + return state.namingThreadIds.has(frameThreadId) } /** @@ -191,6 +229,11 @@ export function readCodexGeneratedTitle(answer: string | null): string | null { if (!answer) { return null } + // The 36-character cap is a request to the model, not a bound the host + // enforces: a reply that ignored the schema arrives at whatever size it likes. + if (Buffer.byteLength(answer, 'utf8') > NAMING_ANSWER_MAX_BYTES) { + return null + } let parsed: unknown try { parsed = JSON.parse(answer) @@ -277,13 +320,19 @@ export async function generateAndSetCodexConversationName( const collector = input.openNamingTurn() let result: CodexNamingTurnResult = { outcome: 'failed' } let disposableThreadId: string | null = null + let namingThreadId: string | null = null try { const opened = await connection.request( 'thread/start', - { cwd: input.cwd, ephemeral: true }, + { + cwd: input.cwd, + ephemeral: true, + approvalPolicy: NAMING_THREAD_APPROVAL_POLICY, + sandbox: NAMING_THREAD_SANDBOX + }, { timeoutMs } ) - const namingThreadId = readCodexThreadId(opened) + const openedThreadId = readCodexThreadId(opened) // No usable throwaway thread is a host that could not be asked, not a decline. // An app-server that ignored `ephemeral` hands back a NEW PERSISTED thread, // not the user's — so the id check below is not what protects them; the @@ -292,9 +341,12 @@ export async function generateAndSetCodexConversationName( // user's chat. A reply naming some OTHER pre-existing thread of the user's // is not guarded and is not treated as a real risk: `thread/start` returns // the thread it just opened. - if (!namingThreadId || namingThreadId === input.threadId) { + if (!openedThreadId || openedThreadId === input.threadId) { return { name: null, settled: false } } + // Held for the cleanup below only once it is known NOT to be the user's own + // thread: unsubscribing that one would cut the chat off from its frames. + namingThreadId = openedThreadId input.retainNamingThread(namingThreadId) // Only when `ephemeral` was NOT honoured. A truly ephemeral thread refuses // deletion ("thread is not persisted and cannot be deleted"), so attempting @@ -321,6 +373,15 @@ export async function generateAndSetCodexConversationName( result = await collector.answer } finally { input.closeNamingTurn() + // The protocol's own cleanup for a thread a client is done with. Retaining + // the id stays the load-bearing guard — a turn that timed out is never + // cancelled and can still emit — so this is best-effort on top, never a + // reason to fail the naming attempt. + if (namingThreadId) { + await connection + .request('thread/unsubscribe', { threadId: namingThreadId }, { timeoutMs }) + .catch((error: unknown) => input.onError?.('unsubscribe-naming-thread', error)) + } // Set only when the app-server persisted the thread despite `ephemeral`. // Without this, every named chat would leave a junk thread and rollout file // in the user's Codex history that Orca never shows and never reclaims. @@ -364,10 +425,13 @@ export async function generateAndSetCodexConversationName( if (!isCodexThreadReadablyUnnamed(current)) { return { name: null, settled: true } } - await connection.request( - 'thread/name/set', - { threadId: input.threadId, name: title }, - { timeoutMs } - ) - return { name: title, settled: true } + // Normalized through the same reader Orca's own copy goes through, so the + // name in the user's Codex history cannot be a multi-line or unbounded string + // that only their client would ever render. + const name = normalizeAgentSessionConversationName(title) + if (!name) { + return { name: null, settled: true } + } + await connection.request('thread/name/set', { threadId: input.threadId, name }, { timeoutMs }) + return { name, settled: true } } diff --git a/src/main/codex/codex-structured-session-adapter.ts b/src/main/codex/codex-structured-session-adapter.ts index ecc896eb77a..847dbf578e1 100644 --- a/src/main/codex/codex-structured-session-adapter.ts +++ b/src/main/codex/codex-structured-session-adapter.ts @@ -38,7 +38,7 @@ import { deliverCodexServerRequest, deliverCodexUnhandledFrame } from './codex-structured-provider-events' -import { isCodexNamingFrame } from './codex-conversation-name-generation' +import { isCodexNamingFrame, isCodexNamingThread } from './codex-conversation-name-generation' import { captureCodexConversationName, startCodexConversationNamingForTurn @@ -173,7 +173,10 @@ export class CodexStructuredSessionAdapter implements StructuredAgentSessionAdap request: Parameters[2] ): void { const session = this.sessions.get(sessionId) - if (session && isCodexNamingFrame(session, readCodexThreadId(request.params))) { + // Exact id only: the broad pre-id rule would refuse a SUB-AGENT's approval + // request during the `thread/start` window, since the naming thread has no + // turn running yet and cannot be the one asking. + if (session && isCodexNamingThread(session, readCodexThreadId(request.params))) { // An approval request from the naming turn would become a durable prompt in // the user's chat, for a command they never asked for, left pending forever // once the turn is abandoned. Refuse it so the turn settles instead. diff --git a/src/main/codex/codex-structured-session-conversation-name.test.ts b/src/main/codex/codex-structured-session-conversation-name.test.ts index 21abdd39d90..205a9133f5f 100644 --- a/src/main/codex/codex-structured-session-conversation-name.test.ts +++ b/src/main/codex/codex-structured-session-conversation-name.test.ts @@ -129,6 +129,9 @@ function namingCodex( answer?: string existingName?: string hangNamingTurn?: boolean + /** Never answers the ephemeral `thread/start`, so naming is in flight with + * NO naming thread id known: the window the broad frame rule covers. */ + hangNamingThreadStart?: boolean /** Completes the naming turn having said nothing: a genuine model decline, * which is a different fact from prose that ignored the schema. */ declineNamingTurn?: boolean @@ -148,6 +151,9 @@ function namingCodex( request: async (method: string, params?: Record) => { calls.push({ method, params: params ?? {} }) if (method === 'thread/start' && params?.ephemeral === true) { + if (options.hangNamingThreadStart) { + return await new Promise(() => {}) + } // Leaves the naming turn in flight: the thread id is known, but nothing // ever settles the collector, which is the window sub-agents run in. if (options.hangNamingTurn) { @@ -514,6 +520,28 @@ describe('Codex sub-agent threads survive the naming window', () => { expect(JSON.stringify(events)).toContain('subagent finished its work') }) + it('prompts the user for a sub-agent approval sent during the thread/start window', async () => { + // No naming thread id exists yet, so only the broad rule could match — and + // on the request path it can only ever match a genuine sub-agent, because + // the naming thread has no turn running to ask with. + const codex = namingCodex({ hangNamingThreadStart: true }) + const { events } = await dispatchedAdapter(codex) + await settle() + + codex.connections[0]!.handlers.onServerRequest?.({ + id: 92, + method: 'item/commandExecution/requestApproval', + params: { threadId: SUBAGENT_THREAD, itemId: 'item-subagent-2', command: 'pnpm test' } + }) + await settle() + + expect(codex.replies).toEqual([]) + const prompts = events.filter((event) => (event as { type?: string }).type === 'prompt') + expect(prompts).toEqual([ + expect.objectContaining({ threadId: SUBAGENT_THREAD, codexItemId: 'item-subagent-2' }) + ]) + }) + it('prompts the user for a sub-agent approval instead of auto-refusing it', async () => { const codex = namingCodex({ hangNamingTurn: true }) const { events } = await dispatchedAdapter(codex)