diff --git a/src/security/sandboxCore.ts b/src/security/sandboxCore.ts index c28d790ce..b17eeb26b 100644 --- a/src/security/sandboxCore.ts +++ b/src/security/sandboxCore.ts @@ -35,6 +35,8 @@ export interface SandboxRunOptions { env?: Record; /** Timeout in milliseconds after which the command is killed. */ timeoutMs?: number; + /** Aborts the command (LOU-U17); `run()` then rejects with an `AbortError`. Adapters may ignore it. */ + signal?: AbortSignal; } /** @@ -79,6 +81,7 @@ export const NoopSandbox: SandboxAdapter = { cwd: opts.cwd, env: opts.env ? { ...process.env, ...opts.env } : process.env, timeout: opts.timeoutMs, + signal: opts.signal, }); return { stdout: stdout.toString(), stderr: stderr.toString(), exitCode: 0 }; } catch (error: any) { diff --git a/src/tools/built-in/http.ts b/src/tools/built-in/http.ts index df3d4b8a4..8804cadb1 100644 --- a/src/tools/built-in/http.ts +++ b/src/tools/built-in/http.ts @@ -198,7 +198,7 @@ function createSandboxTransport(sandbox: SandboxAdapter, validateSSL: boolean, t sandboxHttpFetch( sandbox, { url, method: init.method, headers: init.headers, body: init.body, insecureTLS: !validateSSL }, - { timeoutMs } + { timeoutMs, signal: init.signal } ); } diff --git a/src/tools/built-in/sandboxFetch.test.ts b/src/tools/built-in/sandboxFetch.test.ts new file mode 100644 index 000000000..74c2cb88a --- /dev/null +++ b/src/tools/built-in/sandboxFetch.test.ts @@ -0,0 +1,118 @@ +import { describe, it, expect, vi } from 'vitest'; +import type { SandboxAdapter, SandboxRunOptions, SandboxResult } from '../../security/sandboxCore'; +import { NoopSandbox } from '../../security/sandboxCore'; +import { sandboxHttpFetch } from './sandboxFetch'; +import { createHttpTool } from './http'; +import { createSlackTool } from './slack'; + +const REQUEST = { url: 'https://example.test/hook', method: 'GET' }; +const OK_RESULT: SandboxResult = { + stdout: JSON.stringify({ status: 200, statusText: 'OK', headers: {}, body: 'hi' }), + stderr: '', + exitCode: 0, +}; + +type RunArgs = [string, string[], SandboxRunOptions]; + +/** The options object `run` was called with the first time. */ +function firstRunOptions(run: { mock: { calls: unknown[][] } }): SandboxRunOptions { + return (run.mock.calls[0] as unknown as RunArgs)[2]; +} + +/** A sandbox whose run() never settles on its own (no network involved). */ +function hangingSandbox() { + const run = vi.fn(() => new Promise(() => undefined)); + const sandbox: SandboxAdapter = { name: 'hang', run, writeFile: async () => undefined }; + return { sandbox, run }; +} + +describe('sandboxHttpFetch cancellation (LOU-U17)', () => { + it('(a) an already-aborted signal rejects with an AbortError without running anything', async () => { + const { sandbox, run } = hangingSandbox(); + const controller = new AbortController(); + controller.abort(); + + await expect(sandboxHttpFetch(sandbox, REQUEST, { signal: controller.signal })).rejects.toMatchObject({ + name: 'AbortError', + }); + expect(run).not.toHaveBeenCalled(); + }); + + it('(b) aborting mid-flight rejects promptly with an AbortError', async () => { + const { sandbox, run } = hangingSandbox(); + const controller = new AbortController(); + const pending = sandboxHttpFetch(sandbox, REQUEST, { signal: controller.signal, timeoutMs: 60_000 }); + expect(run).toHaveBeenCalledTimes(1); + + const start = Date.now(); + setTimeout(() => controller.abort(), 20); + await expect(pending).rejects.toMatchObject({ name: 'AbortError' }); + expect(Date.now() - start).toBeLessThan(2000); + }); + + it('(c) the signal reaches sandbox.run() options', async () => { + const run = vi.fn(async () => OK_RESULT); + const controller = new AbortController(); + + const response = await sandboxHttpFetch({ name: 'spy', run, writeFile: async () => undefined }, REQUEST, { + signal: controller.signal, + }); + + expect(await response.text()).toBe('hi'); + expect(firstRunOptions(run).signal).toBe(controller.signal); + }); + + it('without a signal behaves as before', async () => { + const run = vi.fn(async () => OK_RESULT); + const response = await sandboxHttpFetch({ name: 'spy', run, writeFile: async () => undefined }, REQUEST); + expect(response.status).toBe(200); + }); + + it('NoopSandbox kills the child process when the signal aborts', async () => { + const controller = new AbortController(); + const pending = NoopSandbox.run('node', ['-e', 'setTimeout(() => {}, 30000)'], { signal: controller.signal }); + setTimeout(() => controller.abort(), 50); + await expect(pending).rejects.toMatchObject({ name: 'AbortError' }); + }); + + it('http tool sandboxExecute forwards its abortSignal to the sandbox', async () => { + const { sandbox, run } = hangingSandbox(); + const controller = new AbortController(); + const pending = createHttpTool().sandboxExecute!({ url: 'http://93.184.216.34/', method: 'GET' }, sandbox, { + abortSignal: controller.signal, + }); + await vi.waitFor(() => expect(run).toHaveBeenCalled()); + expect(firstRunOptions(run).signal).toBeDefined(); + + controller.abort(); + await expect(pending).rejects.toMatchObject({ name: 'AbortError' }); + }); + + it('slack tool sandboxExecute forwards its abortSignal to the sandbox', async () => { + const { sandbox, run } = hangingSandbox(); + const controller = new AbortController(); + const descriptor = createSlackTool({ webhookUrl: 'https://hooks.slack.test/services/mock' }); + const pending = descriptor.sandboxExecute!({ channel: '#c', message: 'm', approvalId: 'a' }, sandbox, { + abortSignal: controller.signal, + }); + await vi.waitFor(() => expect(run).toHaveBeenCalled()); + expect(firstRunOptions(run).signal).toBe(controller.signal); + + controller.abort(); + await expect(pending).rejects.toMatchObject({ name: 'AbortError' }); + }); + + it('slack tool execute passes its abortSignal to the fetch implementation', async () => { + const fetchImpl = vi.fn(async () => ({ ok: true, status: 200, text: async () => '' }) as Response); + const controller = new AbortController(); + await createSlackTool({ + webhookUrl: 'https://hooks.slack.test/services/mock', + fetchImpl: fetchImpl as unknown as typeof fetch, + }).tool.execute!( + { channel: '#c', message: 'm', approvalId: 'a' }, + { toolCallId: 't', messages: [], abortSignal: controller.signal } + ); + const init = (fetchImpl.mock.calls[0] as unknown as [string, RequestInit])[1]; + expect(init.signal).toBe(controller.signal); + }); +}); diff --git a/src/tools/built-in/sandboxFetch.ts b/src/tools/built-in/sandboxFetch.ts index c484e444e..c2d604ebb 100644 --- a/src/tools/built-in/sandboxFetch.ts +++ b/src/tools/built-in/sandboxFetch.ts @@ -79,6 +79,32 @@ if (req.insecureTLS) { export interface SandboxFetchOptions { /** Milliseconds before the sandboxed request is killed. Defaults to 30s. */ timeoutMs?: number; + /** + * Cancels the request (LOU-U17). An already-aborted signal rejects without + * running anything in the sandbox; an abort mid-flight rejects promptly + * with an `AbortError` and is also handed to `sandbox.run()` so an adapter + * that supports it can kill the sandboxed process. + */ + signal?: AbortSignal; +} + +/** The `AbortError`-shaped rejection reported when the caller cancels. */ +function sandboxAbortError(url: string): Error { + const error = new Error(`Sandboxed HTTP request to ${url} was aborted`); + error.name = 'AbortError'; + return error; +} + +/** Races `pending` against `signal`, rejecting with an AbortError as soon as it aborts. */ +function rejectOnAbort(pending: Promise, signal: AbortSignal | undefined, url: string): Promise { + if (!signal) { + return pending; + } + return new Promise((resolve, reject) => { + const onAbort = () => reject(sandboxAbortError(url)); + signal.addEventListener('abort', onAbort, { once: true }); + pending.then(resolve, reject).finally(() => signal.removeEventListener('abort', onAbort)); + }); } /** @@ -92,13 +118,22 @@ export async function sandboxHttpFetch( request: SandboxFetchRequest, options: SandboxFetchOptions = {} ): Promise { + const { signal } = options; + if (signal?.aborted) { + throw sandboxAbortError(request.url); + } const encoded = Buffer.from(JSON.stringify(request), 'utf-8').toString('base64'); const timeoutMs = options.timeoutMs ?? 30000; - const result = await sandbox.run('node', ['-e', SANDBOX_FETCH_SCRIPT], { - env: { SANDBOX_FETCH_REQUEST: encoded }, - timeoutMs, - }); + const result = await rejectOnAbort( + sandbox.run('node', ['-e', SANDBOX_FETCH_SCRIPT], { + env: { SANDBOX_FETCH_REQUEST: encoded }, + timeoutMs, + signal, + }), + signal, + request.url + ); if (result.exitCode !== 0) { throw new Error( diff --git a/src/tools/built-in/slack.ts b/src/tools/built-in/slack.ts index 477d11a3f..158a37678 100644 --- a/src/tools/built-in/slack.ts +++ b/src/tools/built-in/slack.ts @@ -103,7 +103,8 @@ export async function postSlackAlert( channel: string, message: string, approvalId: string, - options: SlackToolOptions = {} + options: SlackToolOptions = {}, + signal?: AbortSignal ): Promise<{ ok: boolean }> { const webhookUrl = options.webhookUrl ?? process.env[SLACK_WEBHOOK_URL_ENV_KEY]; if (!webhookUrl) { @@ -119,6 +120,7 @@ export async function postSlackAlert( method: 'POST', headers: { 'Content-Type': 'application/json' }, body: JSON.stringify(payload), + signal, }); if (!response.ok) { @@ -139,7 +141,8 @@ export async function postSlackAlertViaSandbox( message: string, approvalId: string, sandbox: SandboxAdapter, - options: SlackToolOptions = {} + options: SlackToolOptions = {}, + signal?: AbortSignal ): Promise<{ ok: boolean }> { const webhookUrl = options.webhookUrl ?? process.env[SLACK_WEBHOOK_URL_ENV_KEY]; if (!webhookUrl) { @@ -150,12 +153,16 @@ export async function postSlackAlertViaSandbox( const payload = buildSlackAlertPayload(channel, message, approvalId); - const response = await sandboxHttpFetch(sandbox, { - url: webhookUrl, - method: 'POST', - headers: { 'Content-Type': 'application/json' }, - body: JSON.stringify(payload), - }); + const response = await sandboxHttpFetch( + sandbox, + { + url: webhookUrl, + method: 'POST', + headers: { 'Content-Type': 'application/json' }, + body: JSON.stringify(payload), + }, + { signal } + ); if (!response.ok) { const errorText = await response.text(); @@ -182,8 +189,9 @@ export function createSlackTool(options: SlackToolOptions = {}): ToolDescriptor message: z.string().describe('Alert message text'), approvalId: z.string().describe('The pending approval id the "Fix it" button will resolve'), }), - execute: async ({ channel, message, approvalId }) => { - return postSlackAlert(channel, message, approvalId, options); + execute: async ({ channel, message, approvalId }, executeOptions) => { + // `?.`: direct callers have historically passed no options object. + return postSlackAlert(channel, message, approvalId, options, executeOptions?.abortSignal); }, }), // LOU-K2: this tool POSTs to a webhook URL read from an env var / @@ -193,9 +201,9 @@ export function createSlackTool(options: SlackToolOptions = {}): ToolDescriptor // above is left unchanged for direct callers (e.g. // examples/ops-pipeline, which calls descriptor.tool.execute() itself). requiresSandbox: true, - sandboxExecute: async (args, sandbox) => { + sandboxExecute: async (args, sandbox, callOptions) => { const { channel, message, approvalId } = args as { channel: string; message: string; approvalId: string }; - return postSlackAlertViaSandbox(channel, message, approvalId, sandbox, options); + return postSlackAlertViaSandbox(channel, message, approvalId, sandbox, options, callOptions?.abortSignal); }, }; }