Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions src/security/sandboxCore.ts
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,8 @@ export interface SandboxRunOptions {
env?: Record<string, string>;
/** 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;
}

/**
Expand Down Expand Up @@ -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) {
Expand Down
2 changes: 1 addition & 1 deletion src/tools/built-in/http.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 }
);
}

Expand Down
118 changes: 118 additions & 0 deletions src/tools/built-in/sandboxFetch.test.ts
Original file line number Diff line number Diff line change
@@ -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<SandboxResult>(() => 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);
});
});
43 changes: 39 additions & 4 deletions src/tools/built-in/sandboxFetch.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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<T>(pending: Promise<T>, signal: AbortSignal | undefined, url: string): Promise<T> {
if (!signal) {
return pending;
}
return new Promise<T>((resolve, reject) => {
const onAbort = () => reject(sandboxAbortError(url));
signal.addEventListener('abort', onAbort, { once: true });
pending.then(resolve, reject).finally(() => signal.removeEventListener('abort', onAbort));
});
}

/**
Expand All @@ -92,13 +118,22 @@ export async function sandboxHttpFetch(
request: SandboxFetchRequest,
options: SandboxFetchOptions = {}
): Promise<Response> {
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(
Expand Down
32 changes: 20 additions & 12 deletions src/tools/built-in/slack.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand All @@ -119,6 +120,7 @@ export async function postSlackAlert(
method: 'POST',
headers: { 'Content-Type': 'application/json' },
body: JSON.stringify(payload),
signal,
});

if (!response.ok) {
Expand All @@ -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) {
Expand All @@ -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();
Expand All @@ -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 /
Expand All @@ -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);
},
};
}
Expand Down
Loading