From 5b9948653dd7c05a65d88e59157cdc2bef20828d Mon Sep 17 00:00:00 2001 From: Ali Mohammad Date: Tue, 29 Sep 2026 02:18:01 +0300 Subject: [PATCH] LOU-K2: opt built-in I/O tools into the sandbox seam The SDK's fail-closed sandboxing seam (ToolDescriptor.requiresSandbox / sandboxExecute, SandboxAdapter, executeToolWithSandboxGuard) was fully implemented and wired into AgentExecutor/resume.ts but unused by every shipped built-in tool, so the feature was dead in practice. Flags the tools that do meaningful external I/O - http (arbitrary model-chosen outbound requests), github, jira (external API calls scoped by config, still real network reach), and slack (webhook POST) - with requiresSandbox: true and a real sandboxExecute() that routes the actual outbound fetch through the configured SandboxAdapter via a new shared helper (sandboxFetch.ts): it runs the request as a Node subprocess through sandbox.run(), so a real isolation backend (e.g. SubprocessSandbox) gets a genuine chance to mediate the call, while NoopSandbox reproduces the exact same network request unsandboxed callers always got. currentDate/dayName are left unflagged - they do no I/O, so a sandbox boundary isn't a meaningful security property for them. execute() is left untouched on every tool (not stubbed out): this codebase has real call sites (examples/ops-pipeline's guardedPr.ts, and existing tests) that invoke descriptor.tool.execute() directly, bypassing executeToolWithSandboxGuard entirely - stubbing execute() to fail closed would have broken them. sandboxExecute() is added as a genuine parallel path used by guard-routed callers (AgentExecutor, resume.ts); execute() remains real and directly callable exactly as before. - github.ts/jira.ts: added a register() override that wraps every fetch-performing tool's execute() with withSandboxedFetch(), which swaps globalThis.fetch for the sandboxed implementation only for the duration of that call - avoiding a hand-written sandboxed reimplementation of all ~50 register*() methods. GitHub's already-out-of-scope tools (OUT_OF_SCOPE_TOOLS) are left unflagged since they throw before ever reaching the network. - http.ts: refactored makeHttpRequest() into a transport-parameterized performHttpRequest() so the SSRF-checking/redirect-following logic is shared between the direct (undici) and sandboxed transports. - slack.ts: added postSlackAlertViaSandbox(), mirroring postSlackAlert() but posting through sandboxHttpFetch(). - Extended http.test.ts/github.test.ts/slack.test.ts and added jira.test.ts (no prior test file existed) to prove: (a) behavior is unchanged under NoopSandbox, (b) a custom SandboxAdapter is genuinely invoked for the outbound call, (c) the tool fails closed per the existing guard contract when sandboxExecute is missing. - Also exported slack.ts from tools/built-in/index.ts (previously missing from the barrel). Co-Authored-By: Claude Sonnet 5 --- src/tools/built-in/github.test.ts | 90 +++++++++++++++ src/tools/built-in/github.ts | 28 +++++ src/tools/built-in/http.test.ts | 72 +++++++++++- src/tools/built-in/http.ts | 162 +++++++++++++++++++++----- src/tools/built-in/index.ts | 1 + src/tools/built-in/jira.test.ts | 160 ++++++++++++++++++++++++++ src/tools/built-in/jira.ts | 27 +++++ src/tools/built-in/sandboxFetch.ts | 177 +++++++++++++++++++++++++++++ src/tools/built-in/slack.test.ts | 79 +++++++++++++ src/tools/built-in/slack.ts | 49 ++++++++ 10 files changed, 816 insertions(+), 29 deletions(-) create mode 100644 src/tools/built-in/jira.test.ts create mode 100644 src/tools/built-in/sandboxFetch.ts diff --git a/src/tools/built-in/github.test.ts b/src/tools/built-in/github.test.ts index 5f737e8c4..df1178a1c 100644 --- a/src/tools/built-in/github.test.ts +++ b/src/tools/built-in/github.test.ts @@ -1,5 +1,8 @@ import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; import { GitHubTools, createGitHubTools } from './github'; +import { NoopSandbox } from '../../security/sandboxCore'; +import type { SandboxAdapter } from '../../security/sandboxCore'; +import { executeToolWithSandboxGuard } from '../../execution/sandboxGuard'; describe('GitHubTools scope enforcement (LOU-E14)', () => { let githubTools: GitHubTools; @@ -104,3 +107,90 @@ describe('GitHubTools scope enforcement (LOU-E14)', () => { } }); }); + +describe('GitHubTools sandbox seam (LOU-K2)', () => { + let githubTools: GitHubTools; + + beforeEach(() => { + githubTools = createGitHubTools({ + token: 'test-token', + owner: 'test-owner', + repo: 'test-repo', + }); + }); + + it('flags requiresSandbox on an in-scope (real-fetching) tool, and leaves execute() untouched', async () => { + const descriptor = githubTools.get('github_get_pull_request'); + expect(descriptor?.requiresSandbox).toBe(true); + expect(typeof descriptor?.sandboxExecute).toBe('function'); + + // execute() itself is still real and directly callable (unsandboxed + // callers like examples/ops-pipeline's guardedPr.ts rely on this). + const fetchSpy = vi.fn().mockResolvedValue({ + ok: true, + json: async () => ({ + number: 5, + title: 't', + body: 'b', + state: 'open', + html_url: 'https://github.com/test-owner/test-repo/pull/5', + head: { ref: 'feature' }, + base: { ref: 'main' }, + }), + }); + vi.stubGlobal('fetch', fetchSpy); + try { + const result = await descriptor!.tool.execute!({ prNumber: 5 }, {} as any); + expect(fetchSpy).toHaveBeenCalledTimes(1); + expect(JSON.parse(result as string).number).toBe(5); + } finally { + vi.unstubAllGlobals(); + } + }); + + it('does not flag requiresSandbox on an out-of-scope tool (it never reaches the network)', () => { + const descriptor = githubTools.get('github_create_branch'); + expect(descriptor?.requiresSandbox).toBeUndefined(); + }); + + it('(b) a custom SandboxAdapter actually gets invoked for the outbound GitHub API call', async () => { + const runSpy = vi.fn(async (cmd: string, args: string[]) => { + expect(cmd).toBe('node'); + expect(args[0]).toBe('-e'); + return { + stdout: JSON.stringify({ + status: 200, + statusText: 'OK', + headers: { 'content-type': 'application/json' }, + body: JSON.stringify({ + number: 7, + title: 'sandboxed', + body: 'b', + state: 'open', + html_url: 'https://github.com/test-owner/test-repo/pull/7', + head: { ref: 'feature' }, + base: { ref: 'main' }, + }), + }), + stderr: '', + exitCode: 0, + }; + }); + const customSandbox: SandboxAdapter = { name: 'custom-test-sandbox', run: runSpy, writeFile: vi.fn() }; + + const descriptor = githubTools.get('github_get_pull_request')!; + const result = await executeToolWithSandboxGuard('github_get_pull_request', descriptor, { prNumber: 7 }, customSandbox); + + expect(runSpy).toHaveBeenCalledTimes(1); + expect(JSON.parse(result as string).number).toBe(7); + }); + + it('(c) fails closed when requiresSandbox is true but sandboxExecute is missing', async () => { + const descriptor = githubTools.get('github_get_pull_request')!; + const broken = { ...descriptor, sandboxExecute: undefined }; + + await expect( + executeToolWithSandboxGuard('github_get_pull_request', broken, { prNumber: 1 }, NoopSandbox) + ).rejects.toThrow(/requiresSandbox but does not implement sandboxExecute/); + }); +}); diff --git a/src/tools/built-in/github.ts b/src/tools/built-in/github.ts index 81546efb3..b1303941b 100644 --- a/src/tools/built-in/github.ts +++ b/src/tools/built-in/github.ts @@ -61,6 +61,8 @@ import { tool } from 'ai'; import { z } from 'zod'; import { ToolRegistry } from '../ToolRegistry'; import { ToolDescriptor } from '../../types'; +import { SandboxAdapter } from '../../security/sandboxCore'; +import { withSandboxedFetch } from './sandboxFetch'; // ============================================================================ // Type Definitions @@ -369,7 +371,33 @@ export class GitHubTools extends ToolRegistry { `been disabled.` ); }) as typeof descriptor.tool.execute; + // Out-of-scope tools above throw before ever making an HTTP request, + // so there's no real outbound call for a sandbox boundary to be + // meaningful on - they're deliberately left unflagged below. + super.register(name, descriptor); + return; } + + // LOU-K2: every remaining GitHub tool's execute() makes a real outbound + // HTTP call to the GitHub API via the ambient global `fetch`. Flag it + // requiresSandbox and route sandboxExecute() through + // withSandboxedFetch(), which swaps `fetch` for a SandboxAdapter-routed + // implementation just for the duration of the original execute() call - + // so a caller going through executeToolWithSandboxGuard() (AgentExecutor, + // resume.ts) gets the fetch call genuinely routed through the + // configured SandboxAdapter, without every one of this file's ~30 + // register*() methods needing its own hand-written sandboxed + // reimplementation. execute() itself is left completely unchanged - + // still a real, directly-callable implementation - for callers that + // invoke descriptor.tool.execute() directly rather than through the + // guard (e.g. examples/ops-pipeline's guardedPr.ts). + if (descriptor.tool?.execute) { + const originalExecute = descriptor.tool.execute; + descriptor.requiresSandbox = true; + descriptor.sandboxExecute = (args: unknown, sandbox: SandboxAdapter) => + withSandboxedFetch(sandbox, async () => originalExecute(args as any, {} as any)); + } + super.register(name, descriptor); } diff --git a/src/tools/built-in/http.test.ts b/src/tools/built-in/http.test.ts index b2c426448..a05ca9c17 100644 --- a/src/tools/built-in/http.test.ts +++ b/src/tools/built-in/http.test.ts @@ -4,7 +4,10 @@ import https from 'https'; import dns from 'dns'; import selfsigned from 'selfsigned'; import type { AddressInfo } from 'net'; -import { makeHttpRequest } from './http'; +import { makeHttpRequest, createHttpTool } from './http'; +import { NoopSandbox } from '../../security/sandboxCore'; +import type { SandboxAdapter } from '../../security/sandboxCore'; +import { executeToolWithSandboxGuard } from '../../execution/sandboxGuard'; function listen(server: http.Server): Promise { return new Promise((resolve) => { @@ -311,4 +314,71 @@ describe('makeHttpRequest', () => { expect(secureResult).not.toBe('unexpectedly-resolved'); }); }); + + describe('sandbox seam (LOU-K2)', () => { + it('flags requiresSandbox and implements sandboxExecute', () => { + const descriptor = createHttpTool(); + expect(descriptor.requiresSandbox).toBe(true); + expect(typeof descriptor.sandboxExecute).toBe('function'); + }); + + it('(a) NoopSandbox: sandboxExecute() gets the exact same real response as the unsandboxed execute() path', async () => { + const localServer = http.createServer((_req, res) => { + res.writeHead(200, { 'Content-Type': 'text/plain' }); + res.end('sandboxed-ok'); + }); + const baseUrl = await listen(localServer); + try { + const descriptor = createHttpTool(); + + const direct = await descriptor.tool.execute!({ url: baseUrl, method: 'GET' }, {} as any); + const viaGuard = await executeToolWithSandboxGuard('http', descriptor, { url: baseUrl, method: 'GET' }, NoopSandbox); + + expect(direct).toBe('sandboxed-ok'); + expect(viaGuard).toBe('sandboxed-ok'); + } finally { + await new Promise((resolve) => localServer.close(() => resolve())); + } + }, 15000); + + it('(b) a custom SandboxAdapter actually gets invoked for the outbound request', async () => { + const runSpy = vi.fn(async (cmd: string, args: string[]) => { + expect(cmd).toBe('node'); + expect(args[0]).toBe('-e'); + return { + stdout: JSON.stringify({ status: 200, statusText: 'OK', headers: {}, body: 'from-custom-sandbox' }), + stderr: '', + exitCode: 0, + }; + }); + const customSandbox: SandboxAdapter = { + name: 'custom-test-sandbox', + run: runSpy, + writeFile: vi.fn(), + }; + + const descriptor = createHttpTool(); + // TEST-NET-3 (RFC 5737, 203.0.113.0/24): a real, non-blocked-range IP + // literal that skips isBlockedHost()'s DNS-resolution path entirely + // (isIP() short-circuits it) - so this test never touches real DNS. + const result = await executeToolWithSandboxGuard( + 'http', + descriptor, + { url: 'https://203.0.113.10/', method: 'GET' }, + customSandbox + ); + + expect(runSpy).toHaveBeenCalledTimes(1); + expect(result).toBe('from-custom-sandbox'); + }); + + it('(c) fails closed when requiresSandbox is true but sandboxExecute is missing', async () => { + const descriptor = createHttpTool(); + const broken = { ...descriptor, sandboxExecute: undefined }; + + await expect( + executeToolWithSandboxGuard('http', broken, { url: 'https://203.0.113.10/', method: 'GET' }, NoopSandbox) + ).rejects.toThrow(/requiresSandbox but does not implement sandboxExecute/); + }); + }); }); diff --git a/src/tools/built-in/http.ts b/src/tools/built-in/http.ts index 2a2f3179d..6b3887423 100644 --- a/src/tools/built-in/http.ts +++ b/src/tools/built-in/http.ts @@ -4,6 +4,8 @@ import { isIP } from 'net'; import { promises as dnsPromises } from 'dns'; import { Agent, fetch as undiciFetch } from 'undici'; import { ToolDescriptor } from '../../types'; +import { SandboxAdapter } from '../../security/sandboxCore'; +import { sandboxHttpFetch } from './sandboxFetch'; /** * HTTP Tool Configuration Options @@ -140,21 +142,77 @@ async function isBlockedHost(hostname: string): Promise { } /** - * Makes HTTP requests to external APIs + * A minimal fetch-response-shaped transport function that actually performs + * the outbound request. `makeHttpRequest()` is transport-agnostic: the + * default transport calls undici's `fetch` directly (unchanged, original + * behavior); `makeHttpRequestViaSandbox()` below supplies a transport that + * routes the same request through a SandboxAdapter instead (LOU-K2). */ -export async function makeHttpRequest({ - url, - method, - headers, - body, - options = {}, -}: { - url: string; - method: 'GET' | 'POST' | 'PUT' | 'DELETE' | 'PATCH'; - headers?: Record; - body?: string; - options?: HttpToolOptions; -}): Promise { +type HttpTransport = ( + url: string, + init: { + method: string; + headers: Record; + body?: string; + signal: AbortSignal; + redirect: 'manual'; + } +) => Promise; + +/** + * Default transport: the exact undici `fetch` + per-request TLS dispatcher + * behavior this file always had. Per-request TLS verification is scoped to + * a dedicated undici Agent (dispatcher) rather than the process-wide + * NODE_TLS_REJECT_UNAUTHORIZED env var, since the env var is global mutable + * state and toggling it around an await point would race under concurrent + * requests. + */ +function createDirectTransport(validateSSL: boolean): { transport: HttpTransport; close: () => Promise } { + const dispatcher = new Agent({ connect: { rejectUnauthorized: validateSSL } }); + return { + transport: (url, init) => undiciFetch(url, { ...init, dispatcher }) as unknown as Promise, + close: () => dispatcher.close(), + }; +} + +/** + * Transport that routes the request through a SandboxAdapter (LOU-K2) via + * sandboxHttpFetch() rather than calling undici's fetch directly - the + * actual outbound network call happens inside `sandbox.run()` (a Node + * subprocess under NoopSandbox; a real isolated command under e.g. + * SubprocessSandbox) instead of in this process. + */ +function createSandboxTransport(sandbox: SandboxAdapter, validateSSL: boolean, timeoutMs: number): HttpTransport { + return (url, init) => + sandboxHttpFetch( + sandbox, + { url, method: init.method, headers: init.headers, body: init.body, insecureTLS: !validateSSL }, + { timeoutMs } + ); +} + +/** + * Core request/redirect/SSRF logic, parameterized by `transport` so it can + * be shared between the direct (unsandboxed) and sandboxed code paths + * without duplicating the SSRF-checking/redirect-following logic. + */ +async function performHttpRequest( + { + url, + method, + headers, + body, + options = {}, + }: { + url: string; + method: 'GET' | 'POST' | 'PUT' | 'DELETE' | 'PATCH'; + headers?: Record; + body?: string; + options?: HttpToolOptions; + }, + transport: HttpTransport, + getCleanup: () => (() => void | Promise) | void +): Promise { const parsedUrl = new URL(url); if (await isBlockedHost(parsedUrl.hostname)) { throw new Error(`Request to blocked host ${parsedUrl.hostname} rejected by SSRF denylist`); @@ -162,16 +220,7 @@ export async function makeHttpRequest({ const controller = new AbortController(); const timeoutId = setTimeout(() => controller.abort(), options.timeout ?? 30000); - - // Per-request TLS verification is scoped to a dedicated undici Agent - // (dispatcher) rather than the process-wide NODE_TLS_REJECT_UNAUTHORIZED - // env var. The env var is global mutable state: toggling it around an - // await point is a race under concurrent requests, since one in-flight - // request's TLS setting can leak into another. A per-request dispatcher - // has no such cross-request interference. - const dispatcher = new Agent({ - connect: { rejectUnauthorized: options.validateSSL !== false }, - }); + const cleanup = getCleanup(); try { const fetchOptions = { @@ -183,12 +232,11 @@ export async function makeHttpRequest({ body: body && method !== 'GET' ? body : undefined, signal: controller.signal, redirect: 'manual' as const, - dispatcher, }; let currentUrl = url; let redirectCount = 0; - let response = await undiciFetch(currentUrl, fetchOptions); + let response = await transport(currentUrl, fetchOptions); while (response.status >= 300 && response.status < 400 && response.headers.get('location')) { redirectCount++; @@ -200,7 +248,7 @@ export async function makeHttpRequest({ if (await isBlockedHost(redirectHostname)) { throw new Error(`Request to blocked host ${redirectHostname} rejected by SSRF denylist`); } - response = await undiciFetch(currentUrl, fetchOptions); + response = await transport(currentUrl, fetchOptions); } clearTimeout(timeoutId); @@ -227,10 +275,51 @@ export async function makeHttpRequest({ throw new Error('HTTP request failed with unknown error'); } finally { clearTimeout(timeoutId); - await dispatcher.close(); + await cleanup?.(); } } +/** + * Makes HTTP requests to external APIs. Unchanged, original behavior: goes + * straight to undici's fetch (with the per-request TLS dispatcher), never + * routed through any SandboxAdapter. This remains the implementation behind + * the HTTP tool's plain `execute()` - see makeHttpRequestViaSandbox() below + * for the sandboxed path used by `sandboxExecute()`. + */ +export async function makeHttpRequest(args: { + url: string; + method: 'GET' | 'POST' | 'PUT' | 'DELETE' | 'PATCH'; + headers?: Record; + body?: string; + options?: HttpToolOptions; +}): Promise { + const { transport, close } = createDirectTransport(args.options?.validateSSL !== false); + return performHttpRequest(args, transport, () => close); +} + + + +/** + * Same request/redirect/SSRF logic as makeHttpRequest(), but the actual + * outbound fetch is performed through `sandbox` (LOU-K2) via + * sandboxHttpFetch() instead of calling undici's fetch directly in this + * process. + */ +export async function makeHttpRequestViaSandbox( + args: { + url: string; + method: 'GET' | 'POST' | 'PUT' | 'DELETE' | 'PATCH'; + headers?: Record; + body?: string; + options?: HttpToolOptions; + }, + sandbox: SandboxAdapter +): Promise { + const timeoutMs = args.options?.timeout ?? 30000; + const transport = createSandboxTransport(sandbox, args.options?.validateSSL !== false, timeoutMs + 5000); + return performHttpRequest(args, transport, () => undefined); +} + /** * Create HTTP Tool * Factory function to create an HTTP tool with custom options @@ -250,6 +339,23 @@ export function createHttpTool(options: HttpToolOptions = {}): ToolDescriptor { return makeHttpRequest({ url, method, headers, body, options }); }, }), + // LOU-K2: httpTool makes arbitrary, model-chosen outbound HTTP requests + // - the highest-risk built-in tool for a sandbox boundary to be + // meaningful on. A caller going through executeToolWithSandboxGuard() + // (AgentExecutor, resume.ts) gets the actual fetch routed through the + // configured SandboxAdapter via sandboxExecute(); execute() above is + // left unchanged (still real, directly callable) for callers that + // invoke descriptor.tool.execute() directly. + requiresSandbox: true, + sandboxExecute: async (args, sandbox) => { + const { url, method, headers, body } = args as { + url: string; + method: 'GET' | 'POST' | 'PUT' | 'DELETE' | 'PATCH'; + headers?: Record; + body?: string; + }; + return makeHttpRequestViaSandbox({ url, method, headers, body, options }, sandbox); + }, }; } diff --git a/src/tools/built-in/index.ts b/src/tools/built-in/index.ts index 65805858f..a11c41aa2 100644 --- a/src/tools/built-in/index.ts +++ b/src/tools/built-in/index.ts @@ -9,3 +9,4 @@ export * from './http'; export * from './email'; export * from './jira'; export * from './github'; +export * from './slack'; diff --git a/src/tools/built-in/jira.test.ts b/src/tools/built-in/jira.test.ts new file mode 100644 index 000000000..97895da67 --- /dev/null +++ b/src/tools/built-in/jira.test.ts @@ -0,0 +1,160 @@ +import { describe, it, expect, vi, afterEach } from 'vitest'; +import http from 'http'; +import type { AddressInfo } from 'net'; +import { createJiraTools } from './jira'; +import { NoopSandbox } from '../../security/sandboxCore'; +import type { SandboxAdapter } from '../../security/sandboxCore'; +import { executeToolWithSandboxGuard } from '../../execution/sandboxGuard'; + +describe('JiraTools sandbox seam (LOU-K2)', () => { + afterEach(() => { + vi.unstubAllGlobals(); + }); + + it('flags requiresSandbox on every registered tool and implements sandboxExecute', () => { + const jiraTools = createJiraTools({ + baseUrl: 'https://jira.test', + email: 'bot@example.com', + apiToken: 'token', + }); + + for (const name of jiraTools.list()) { + const descriptor = jiraTools.get(name)!; + expect(descriptor.requiresSandbox, `${name} should require sandbox`).toBe(true); + expect(typeof descriptor.sandboxExecute, `${name} should implement sandboxExecute`).toBe('function'); + } + }); + + it('execute() is left unchanged and still makes a real request directly (unsandboxed callers)', async () => { + const jiraTools = createJiraTools({ + baseUrl: 'https://jira.test', + email: 'bot@example.com', + apiToken: 'token', + }); + const fetchSpy = vi.fn().mockResolvedValue({ + ok: true, + json: async () => ({ + key: 'PROJ-1', + fields: { + summary: 's', + description: null, + issuetype: { name: 'Task' }, + status: { name: 'Open' }, + project: { key: 'PROJ' }, + created: '2024-01-01', + updated: '2024-01-01', + }, + }), + }); + vi.stubGlobal('fetch', fetchSpy); + + const descriptor = jiraTools.get('jira_get_ticket')!; + const result = await descriptor.tool.execute!({ ticketKey: 'PROJ-1' }, {} as any); + + expect(fetchSpy).toHaveBeenCalledTimes(1); + expect(JSON.parse(result as string).key).toBe('PROJ-1'); + }); + + it('(a) NoopSandbox: sandboxExecute() hits a real local Jira-shaped server, same result as execute()', async () => { + const server = http.createServer((_req, res) => { + res.writeHead(200, { 'Content-Type': 'application/json' }); + res.end( + JSON.stringify({ + key: 'PROJ-2', + fields: { + summary: 'sandboxed summary', + description: null, + issuetype: { name: 'Bug' }, + status: { name: 'Open' }, + project: { key: 'PROJ' }, + created: '2024-01-01', + updated: '2024-01-01', + }, + }) + ); + }); + const port = await new Promise((resolve) => { + server.listen(0, '127.0.0.1', () => resolve((server.address() as AddressInfo).port)); + }); + + try { + const jiraTools = createJiraTools({ + baseUrl: `http://127.0.0.1:${port}`, + email: 'bot@example.com', + apiToken: 'token', + }); + const descriptor = jiraTools.get('jira_get_ticket')!; + + const result = await executeToolWithSandboxGuard( + 'jira_get_ticket', + descriptor, + { ticketKey: 'PROJ-2' }, + NoopSandbox + ); + + expect(JSON.parse(result as string).summary).toBe('sandboxed summary'); + } finally { + await new Promise((resolve) => server.close(() => resolve())); + } + }, 15000); + + it('(b) a custom SandboxAdapter actually gets invoked for the outbound Jira API call', async () => { + const runSpy = vi.fn(async (cmd: string, args: string[]) => { + expect(cmd).toBe('node'); + expect(args[0]).toBe('-e'); + return { + stdout: JSON.stringify({ + status: 200, + statusText: 'OK', + headers: { 'content-type': 'application/json' }, + body: JSON.stringify({ + key: 'PROJ-3', + fields: { + summary: 'from custom sandbox', + description: null, + issuetype: { name: 'Task' }, + status: { name: 'Open' }, + project: { key: 'PROJ' }, + created: '2024-01-01', + updated: '2024-01-01', + }, + }), + }), + stderr: '', + exitCode: 0, + }; + }); + const customSandbox: SandboxAdapter = { name: 'custom-test-sandbox', run: runSpy, writeFile: vi.fn() }; + + const jiraTools = createJiraTools({ + baseUrl: 'https://jira.test', + email: 'bot@example.com', + apiToken: 'token', + }); + const descriptor = jiraTools.get('jira_get_ticket')!; + + const result = await executeToolWithSandboxGuard( + 'jira_get_ticket', + descriptor, + { ticketKey: 'PROJ-3' }, + customSandbox + ); + + expect(runSpy).toHaveBeenCalledTimes(1); + expect(JSON.parse(result as string).summary).toBe('from custom sandbox'); + }); + + it('(c) fails closed when requiresSandbox is true but sandboxExecute is missing', async () => { + const jiraTools = createJiraTools({ + baseUrl: 'https://jira.test', + email: 'bot@example.com', + apiToken: 'token', + }); + const descriptor = jiraTools.get('jira_get_ticket')!; + const broken = { ...descriptor, sandboxExecute: undefined }; + + await expect( + executeToolWithSandboxGuard('jira_get_ticket', broken, { ticketKey: 'PROJ-1' }, NoopSandbox) + ).rejects.toThrow(/requiresSandbox but does not implement sandboxExecute/); + }); +}); diff --git a/src/tools/built-in/jira.ts b/src/tools/built-in/jira.ts index 81505a18f..4deafd17a 100644 --- a/src/tools/built-in/jira.ts +++ b/src/tools/built-in/jira.ts @@ -21,6 +21,9 @@ import { tool } from 'ai'; import { z } from 'zod'; import { ToolRegistry } from '../ToolRegistry'; +import { ToolDescriptor } from '../../types'; +import { SandboxAdapter } from '../../security/sandboxCore'; +import { withSandboxedFetch } from './sandboxFetch'; // ============================================================================ // Type Definitions @@ -83,6 +86,30 @@ export class JiraTools extends ToolRegistry { this.registerAllTools(); } + /** + * LOU-K2: every Jira tool's execute() makes a real outbound HTTP call to + * the configured Jira instance via the ambient global `fetch`. Flag it + * requiresSandbox and route sandboxExecute() through withSandboxedFetch(), + * which swaps `fetch` for a SandboxAdapter-routed implementation just for + * the duration of the original execute() call - so a caller going through + * executeToolWithSandboxGuard() (AgentExecutor, resume.ts) gets the fetch + * call genuinely routed through the configured SandboxAdapter, without + * every one of this file's register*() methods needing its own + * hand-written sandboxed reimplementation. execute() itself is left + * completely unchanged - still a real, directly-callable implementation - + * for callers that invoke descriptor.tool.execute() directly rather than + * through the guard. + */ + public register(name: string, descriptor: ToolDescriptor): void { + if (descriptor.tool?.execute) { + const originalExecute = descriptor.tool.execute; + descriptor.requiresSandbox = true; + descriptor.sandboxExecute = (args: unknown, sandbox: SandboxAdapter) => + withSandboxedFetch(sandbox, async () => originalExecute(args as any, {} as any)); + } + super.register(name, descriptor); + } + private registerAllTools() { this.registerGetTicket(); this.registerSearchTickets(); diff --git a/src/tools/built-in/sandboxFetch.ts b/src/tools/built-in/sandboxFetch.ts new file mode 100644 index 000000000..6f92fcfcf --- /dev/null +++ b/src/tools/built-in/sandboxFetch.ts @@ -0,0 +1,177 @@ +/** + * Shared "route an outbound HTTP call through a SandboxAdapter" helper + * (LOU-K2). + * + * `SandboxAdapter` (src/security/sandboxCore.ts) only exposes `run()` (run a + * command, capture stdout/stderr/exitCode) and `writeFile()` - there is no + * generic way to hand it an arbitrary in-process JS closure like `fetch()`. + * To genuinely route a tool's outbound HTTP call through the adapter (so a + * real isolation backend like SubprocessSandbox gets a chance to mediate + * it), the request has to be performed as a subprocess: this module encodes + * the request as JSON, runs a small Node one-liner via `sandbox.run('node', + * ['-e', SCRIPT], ...)` that performs the actual `fetch()` and prints the + * response back as JSON on stdout, and decodes that into a real `Response`. + * + * Under NoopSandbox (the default, zero-isolation adapter - see + * sandboxCore.ts) this still performs the exact same network request as + * calling `fetch()` in-process would, just via a child `node` process - so + * behavior is unchanged, only the seam it's routed through changes. + */ + +import { SandboxAdapter } from '../../security/sandboxCore'; + +/** + * The request shape encoded (as base64 JSON, via the SANDBOX_FETCH_REQUEST + * env var - not argv, so there's no shell-quoting/argv-length concern for + * arbitrary headers/bodies) and handed to the sandboxed Node script. + */ +export interface SandboxFetchRequest { + url: string; + method: string; + headers?: Record; + body?: string; + /** + * When true, the sandboxed process disables TLS certificate verification + * for this request (mirrors HttpToolOptions.validateSSL === false in + * http.ts). Left undefined/false, TLS is verified normally. + */ + insecureTLS?: boolean; +} + +/** + * Node one-liner run inside the sandbox. Reads the request from + * SANDBOX_FETCH_REQUEST, performs the real fetch(), and writes exactly one + * JSON line to stdout - the only thing this module's caller parses. + * `redirect: 'manual'` so any SSRF/redirect-chasing policy a caller + * implements (see http.ts's per-hop isBlockedHost check) stays in control + * of following redirects rather than this script silently doing it. + */ +const SANDBOX_FETCH_SCRIPT = ` +const req = JSON.parse(Buffer.from(process.env.SANDBOX_FETCH_REQUEST, 'base64').toString('utf-8')); +if (req.insecureTLS) { + process.env.NODE_TLS_REJECT_UNAUTHORIZED = '0'; +} +(async () => { + try { + const res = await fetch(req.url, { + method: req.method, + headers: req.headers, + body: req.body, + redirect: 'manual', + }); + const text = await res.text(); + const headers = {}; + res.headers.forEach((value, key) => { headers[key] = value; }); + process.stdout.write(JSON.stringify({ + status: res.status, + statusText: res.statusText, + headers, + body: text, + })); + } catch (error) { + process.stderr.write(String((error && error.stack) || error)); + process.exitCode = 1; + } +})(); +`; + +export interface SandboxFetchOptions { + /** Milliseconds before the sandboxed request is killed. Defaults to 30s. */ + timeoutMs?: number; +} + +/** + * Performs `request` as a real outbound HTTP call routed through + * `sandbox.run()`, and resolves with a real `Response` built from the + * sandboxed process's JSON stdout - so callers can keep using the ordinary + * `Response` API (`.ok`, `.status`, `.headers`, `.text()`, `.json()`). + */ +export async function sandboxHttpFetch( + sandbox: SandboxAdapter, + request: SandboxFetchRequest, + options: SandboxFetchOptions = {} +): Promise { + 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, + }); + + if (result.exitCode !== 0) { + throw new Error( + `Sandboxed HTTP request to ${request.url} failed: ${result.stderr || `exit code ${result.exitCode}`}` + ); + } + + let parsed: { status: number; statusText: string; headers: Record; body: string }; + try { + parsed = JSON.parse(result.stdout); + } catch { + throw new Error(`Sandboxed HTTP request to ${request.url} returned unparsable output: ${result.stdout}`); + } + + return new Response(parsed.body, { + status: parsed.status, + statusText: parsed.statusText, + headers: parsed.headers, + }); +} + +/** + * Normalizes a fetch-style `HeadersInit` (plain object, `Headers`, or + * `[key, value][]`) into a plain `Record` for encoding. + */ +function normalizeHeaders(headers?: HeadersInit): Record | undefined { + if (!headers) return undefined; + if (headers instanceof Headers) { + const result: Record = {}; + headers.forEach((value, key) => { + result[key] = value; + }); + return result; + } + if (Array.isArray(headers)) { + return Object.fromEntries(headers); + } + return { ...headers }; +} + +/** + * Swaps `globalThis.fetch` for a sandbox-routed implementation for the + * duration of `fn()`, restoring the original afterward (even if `fn` + * throws). This lets tool code that calls the ambient global `fetch` + * (github.ts, jira.ts) route its real outbound HTTP call through a + * SandboxAdapter without every call site inside those tools needing to + * thread a `sandbox` argument through - see GitHubTools.register() / + * JiraTools.register(), which wrap each tool's `execute()` with this for + * `sandboxExecute()`, while leaving `execute()` itself untouched (still a + * real, directly-callable implementation for callers that invoke + * `descriptor.tool.execute()` directly rather than through + * executeToolWithSandboxGuard - e.g. examples/ops-pipeline). + * + * NOT safe across concurrent calls that each mutate globalThis.fetch at the + * same time (a second, interleaved withSandboxedFetch() call would restore + * the wrong original fetch). This codebase executes one tool call at a time + * within a given AgentExecutor step today; if that changes, per-call fetch + * injection (like http.ts's dedicated sandboxExecute) is needed instead of + * a global monkey-patch. + */ +export async function withSandboxedFetch(sandbox: SandboxAdapter, fn: () => Promise): Promise { + const original = globalThis.fetch; + const sandboxedFetch = ((input: RequestInfo | URL, init?: RequestInit) => { + const url = typeof input === 'string' || input instanceof URL ? String(input) : input.url; + const method = init?.method ?? (typeof input !== 'string' && !(input instanceof URL) ? input.method : undefined) ?? 'GET'; + const headers = normalizeHeaders(init?.headers); + const body = init?.body != null ? String(init.body) : undefined; + return sandboxHttpFetch(sandbox, { url, method, headers, body }); + }) as typeof fetch; + + globalThis.fetch = sandboxedFetch; + try { + return await fn(); + } finally { + globalThis.fetch = original; + } +} diff --git a/src/tools/built-in/slack.test.ts b/src/tools/built-in/slack.test.ts index 05800fbc7..faead2da8 100644 --- a/src/tools/built-in/slack.test.ts +++ b/src/tools/built-in/slack.test.ts @@ -1,5 +1,10 @@ import { describe, it, expect, vi } from 'vitest'; +import http from 'http'; +import type { AddressInfo } from 'net'; import { buildSlackAlertPayload, createSlackTool, postSlackAlert } from './slack'; +import { NoopSandbox } from '../../security/sandboxCore'; +import type { SandboxAdapter } from '../../security/sandboxCore'; +import { executeToolWithSandboxGuard } from '../../execution/sandboxGuard'; function mockFetchOk() { return vi.fn().mockResolvedValue({ @@ -75,3 +80,77 @@ describe('postSlackAlert / createSlackTool (mocked Slack API)', () => { expect(fetchImpl).toHaveBeenCalledTimes(1); }); }); + +describe('slack tool sandbox seam (LOU-K2)', () => { + it('flags requiresSandbox and implements sandboxExecute', () => { + const descriptor = createSlackTool({ webhookUrl: 'https://hooks.slack.test/services/mock' }); + expect(descriptor.requiresSandbox).toBe(true); + expect(typeof descriptor.sandboxExecute).toBe('function'); + }); + + it('(a) NoopSandbox: sandboxExecute() posts the same payload to a real local webhook, unchanged from execute()', async () => { + let received: any; + const server = http.createServer((req, res) => { + let body = ''; + req.on('data', (chunk) => (body += chunk)); + req.on('end', () => { + received = JSON.parse(body); + res.writeHead(200, { 'Content-Type': 'text/plain' }); + res.end(''); + }); + }); + const port = await new Promise((resolve) => { + server.listen(0, '127.0.0.1', () => resolve((server.address() as AddressInfo).port)); + }); + const webhookUrl = `http://127.0.0.1:${port}/`; + + try { + const descriptor = createSlackTool({ webhookUrl }); + const result = await executeToolWithSandboxGuard( + 'slack', + descriptor, + { channel: '#incidents', message: 'Deploy failed', approvalId: 'approval-123' }, + NoopSandbox + ); + + expect(result).toEqual({ ok: true }); + expect(received.channel).toBe('#incidents'); + expect(received.blocks[1].elements[0].value).toBe('approval-123'); + } finally { + await new Promise((resolve) => server.close(() => resolve())); + } + }, 15000); + + it('(b) a custom SandboxAdapter actually gets invoked for the webhook POST', async () => { + const runSpy = vi.fn(async (cmd: string) => { + expect(cmd).toBe('node'); + return { stdout: JSON.stringify({ status: 200, statusText: 'OK', headers: {}, body: '' }), stderr: '', exitCode: 0 }; + }); + const customSandbox: SandboxAdapter = { name: 'custom-test-sandbox', run: runSpy, writeFile: vi.fn() }; + + const descriptor = createSlackTool({ webhookUrl: 'https://hooks.slack.test/services/mock' }); + const result = await executeToolWithSandboxGuard( + 'slack', + descriptor, + { channel: '#incidents', message: 'hi', approvalId: 'approval-1' }, + customSandbox + ); + + expect(runSpy).toHaveBeenCalledTimes(1); + expect(result).toEqual({ ok: true }); + }); + + it('(c) fails closed when requiresSandbox is true but sandboxExecute is missing', async () => { + const descriptor = createSlackTool({ webhookUrl: 'https://hooks.slack.test/services/mock' }); + const broken = { ...descriptor, sandboxExecute: undefined }; + + await expect( + executeToolWithSandboxGuard( + 'slack', + broken, + { channel: '#incidents', message: 'hi', approvalId: 'approval-1' }, + NoopSandbox + ) + ).rejects.toThrow(/requiresSandbox but does not implement sandboxExecute/); + }); +}); diff --git a/src/tools/built-in/slack.ts b/src/tools/built-in/slack.ts index 0c99fcf6f..477d11a3f 100644 --- a/src/tools/built-in/slack.ts +++ b/src/tools/built-in/slack.ts @@ -24,6 +24,8 @@ import { tool } from 'ai'; import { z } from 'zod'; import { ToolDescriptor } from '../../types'; +import { SandboxAdapter } from '../../security/sandboxCore'; +import { sandboxHttpFetch } from './sandboxFetch'; export const SLACK_WEBHOOK_URL_ENV_KEY = 'SLACK_WEBHOOK_URL'; @@ -127,6 +129,42 @@ export async function postSlackAlert( return { ok: true }; } +/** + * Same as postSlackAlert(), but the outbound webhook POST is routed through + * `sandbox` (LOU-K2) via sandboxHttpFetch() rather than calling `fetch` + * directly in this process. + */ +export async function postSlackAlertViaSandbox( + channel: string, + message: string, + approvalId: string, + sandbox: SandboxAdapter, + options: SlackToolOptions = {} +): Promise<{ ok: boolean }> { + const webhookUrl = options.webhookUrl ?? process.env[SLACK_WEBHOOK_URL_ENV_KEY]; + if (!webhookUrl) { + throw new Error( + `Slack tool: no webhook URL configured. Set ${SLACK_WEBHOOK_URL_ENV_KEY} or pass options.webhookUrl.` + ); + } + + const payload = buildSlackAlertPayload(channel, message, approvalId); + + const response = await sandboxHttpFetch(sandbox, { + url: webhookUrl, + method: 'POST', + headers: { 'Content-Type': 'application/json' }, + body: JSON.stringify(payload), + }); + + if (!response.ok) { + const errorText = await response.text(); + throw new Error(`Slack tool: webhook post failed: ${response.statusText} - ${errorText}`); + } + + return { ok: true }; +} + /** * Creates the Slack alert ToolDescriptor. Requires {channel, message, * approvalId} - `approvalId` is the id of a REAL LOU-C PendingApproval @@ -148,6 +186,17 @@ export function createSlackTool(options: SlackToolOptions = {}): ToolDescriptor return postSlackAlert(channel, message, approvalId, options); }, }), + // LOU-K2: this tool POSTs to a webhook URL read from an env var / + // options (an external endpoint, not something the model chooses + // directly, but still a real outbound network call) - route it through + // executeToolWithSandboxGuard()'s sandboxExecute() path. execute() + // above is left unchanged for direct callers (e.g. + // examples/ops-pipeline, which calls descriptor.tool.execute() itself). + requiresSandbox: true, + sandboxExecute: async (args, sandbox) => { + const { channel, message, approvalId } = args as { channel: string; message: string; approvalId: string }; + return postSlackAlertViaSandbox(channel, message, approvalId, sandbox, options); + }, }; }