From d50e9bb9c645f99597aac441f5ddcca0d9336960 Mon Sep 17 00:00:00 2001 From: Danny Avila Date: Tue, 8 Sep 2026 08:36:54 -0400 Subject: [PATCH 1/4] fix: preserve preview auth on first navigation (#153) --- docs/lambda-microvm/README.md | 5 +- .../src/hosted-app/preview-gateway.test.ts | 50 +++++++++++++++++++ service/src/hosted-app/preview-gateway.ts | 45 +++++++++++++---- 3 files changed, 89 insertions(+), 11 deletions(-) create mode 100644 service/src/hosted-app/preview-gateway.test.ts diff --git a/docs/lambda-microvm/README.md b/docs/lambda-microvm/README.md index 4ec87d94..80e5e468 100644 --- a/docs/lambda-microvm/README.md +++ b/docs/lambda-microvm/README.md @@ -232,7 +232,10 @@ Content-Type: application/json ``` `GET /v1/hosted-apps/:app_id?runtime_session_hint=...` returns status and a -fresh five-minute `preview_url`; `DELETE` on the same resource terminates the +fresh five-minute `preview_url`; the authorization response loads a minimal +same-origin handoff page before opening the app so the first request includes +the host-only `SameSite=Strict` preview cookie even when LibreChat is on another +site. `DELETE` on the same resource terminates the lease. A revision is immutable. Retrying the identical spec reasserts the resident process; changing code or launch settings requires a new revision and captures a new exact checkpoint. An ambiguous provider launch is replayed only diff --git a/service/src/hosted-app/preview-gateway.test.ts b/service/src/hosted-app/preview-gateway.test.ts new file mode 100644 index 00000000..1dcf7839 --- /dev/null +++ b/service/src/hosted-app/preview-gateway.test.ts @@ -0,0 +1,50 @@ +import { describe, expect, test } from 'bun:test'; +import type { Response } from 'express'; +import { sendHostedAppPreviewAuthorizationHandoff } from './preview-gateway'; + +describe('hosted app preview authorization handoff', () => { + test('ends the cross-site redirect chain before starting a same-origin navigation', () => { + const headers = new Map(); + let status: number | undefined; + let type: string | undefined; + let body: string | undefined; + let redirects = 0; + const response = { + setHeader(name: string, value: string) { + headers.set(name.toLowerCase(), value); + return this; + }, + status(value: number) { + status = value; + return this; + }, + type(value: string) { + type = value; + return this; + }, + send(value: string) { + body = value; + return this; + }, + redirect() { + redirects += 1; + return this; + }, + } as unknown as Response; + + sendHostedAppPreviewAuthorizationHandoff(response, 'signed.token/value', 300); + + expect(status).toBe(200); + expect(type).toBe('html'); + expect(redirects).toBe(0); + expect(headers.get('cache-control')).toBe('no-store'); + expect(headers.has('location')).toBe(false); + expect(headers.get('set-cookie')).toBe( + '__Host-codeapi-app=signed.token%2Fvalue; Path=/; HttpOnly; Secure; SameSite=Strict; Max-Age=300', + ); + expect(body).toContain(''); + expect(body).toContain(''); + expect(body).not.toContain('__codeapi/authorize'); + expect(body).not.toContain('token'); + }); +}); diff --git a/service/src/hosted-app/preview-gateway.ts b/service/src/hosted-app/preview-gateway.ts index 4ce6ed5d..9c076cf0 100644 --- a/service/src/hosted-app/preview-gateway.ts +++ b/service/src/hosted-app/preview-gateway.ts @@ -49,6 +49,40 @@ function reject(res: Response, status: number, message: string): Response { return res.status(status).type('text/plain').send(message); } +/** + * End the cross-site navigation before loading the app. A SameSite=Strict + * cookie set on a cross-site HTTP redirect can remain excluded for the whole + * redirect chain. Loading this small document first makes its navigation to + * `/` originate from the preview site while keeping the cookie Strict. + */ +export function sendHostedAppPreviewAuthorizationHandoff( + res: Response, + sessionToken: string, + maxAge: number, +): Response { + res.setHeader('Set-Cookie', [ + `${COOKIE_NAME}=${encodeURIComponent(sessionToken)}`, + 'Path=/', + 'HttpOnly', + 'Secure', + 'SameSite=Strict', + `Max-Age=${maxAge}`, + ].join('; ')); + res.setHeader('Cache-Control', 'no-store'); + return res.status(200).type('html').send([ + '', + '', + '', + '', + '', + 'Opening preview', + '', + '', + '

Continue to preview

', + '', + ].join('')); +} + export async function hostedAppPreviewGateway( req: Request, res: Response, @@ -111,16 +145,7 @@ export async function hostedAppPreviewGateway( expiresAt, }, previewKey()); const maxAge = Math.max(1, Math.floor((expiresAt - Date.now()) / 1_000)); - res.setHeader('Set-Cookie', [ - `${COOKIE_NAME}=${encodeURIComponent(sessionToken)}`, - 'Path=/', - 'HttpOnly', - 'Secure', - 'SameSite=Strict', - `Max-Age=${maxAge}`, - ].join('; ')); - res.setHeader('Cache-Control', 'no-store'); - res.redirect(303, '/'); + sendHostedAppPreviewAuthorizationHandoff(res, sessionToken, maxAge); return; } From 5ee769e4db40c413b0f89ba0fe9a347082756980 Mon Sep 17 00:00:00 2001 From: Danny Avila Date: Tue, 8 Sep 2026 08:37:32 -0400 Subject: [PATCH 2/4] =?UTF-8?q?=F0=9F=A7=BE=20fix:=20Log=20Workspace=20Too?= =?UTF-8?q?l=20HTTP=20Outcomes=20(#154)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix: log workspace tool HTTP outcomes * fix: observe workspace outcomes across middleware and settlement * test: handle overloaded logger arguments in timing assertion * fix: classify workspace outcomes across middleware boundaries * fix: scope workspace logging to the exact POST endpoint --- service/src/api-server.ts | 2 + service/src/hosted-app/preview-access.ts | 9 + service/src/hosted-app/preview-gateway.ts | 13 +- service/src/middleware/execution-profile.ts | 2 + service/src/service-api.ts | 2 + service/src/workspace-tools/outcome.ts | 79 ++++ service/src/workspace-tools/router.test.ts | 401 +++++++++++++++++++- service/src/workspace-tools/router.ts | 30 +- 8 files changed, 521 insertions(+), 17 deletions(-) create mode 100644 service/src/workspace-tools/outcome.ts diff --git a/service/src/api-server.ts b/service/src/api-server.ts index fd77b55d..fc93aebf 100644 --- a/service/src/api-server.ts +++ b/service/src/api-server.ts @@ -20,6 +20,7 @@ import serviceRouter from './service/router'; import programmaticRouter from './service/programmatic-router'; import bridgeRouter from './bridge'; import workspaceToolsRouter from './workspace-tools'; +import { workspaceToolOutcomeLogging } from './workspace-tools/outcome'; import { connection } from './queue'; import { metricsHandler } from './metrics'; import { httpMetricsMiddleware } from './middleware/httpMetrics'; @@ -33,6 +34,7 @@ import { hostedAppPreviewGateway } from './hosted-app/preview-gateway'; const { LOCAL_MODE: isLocalMode } = env; const app = express(); +app.post('/v1/workspace-tools/execute', workspaceToolOutcomeLogging); app.disable('x-powered-by'); app.set('trust proxy', 1); app.use(traceHttpRequest('codeapi.api.request')); diff --git a/service/src/hosted-app/preview-access.ts b/service/src/hosted-app/preview-access.ts index 374b34ea..35abe7af 100644 --- a/service/src/hosted-app/preview-access.ts +++ b/service/src/hosted-app/preview-access.ts @@ -142,3 +142,12 @@ export function hostedAppPreviewAuthorizeUrl( origin.searchParams.set('token', token); return origin.toString(); } + +export function hostedAppRequestHostname(host: string | undefined): string | undefined { + if (host == null || host.length === 0 || /[\s/@\\]/.test(host)) return undefined; + try { + return new URL(`http://${host}`).hostname; + } catch { + return undefined; + } +} diff --git a/service/src/hosted-app/preview-gateway.ts b/service/src/hosted-app/preview-gateway.ts index 9c076cf0..301d47ad 100644 --- a/service/src/hosted-app/preview-gateway.ts +++ b/service/src/hosted-app/preview-gateway.ts @@ -5,6 +5,7 @@ import { readRuntimeSessionRecord } from '../runtime-session/registry'; import { HostedAppControlPlaneError } from './control-plane'; import { hostedAppRuntimeIdFromHostname, + hostedAppRequestHostname, HostedAppPreviewAccessError, hostedAppPreviewOwnerBinding, signHostedAppPreviewAccess, @@ -16,16 +17,6 @@ import { applyHostedAppPreviewSecurityHeaders } from './proxy-policy'; const COOKIE_NAME = '__Host-codeapi-app'; const PREVIEW_COOKIE_TTL_MS = 60 * 60_000; -function rawHostname(req: Request): string | undefined { - const host = req.headers.host; - if (!host || /[\s/@\\]/.test(host)) return undefined; - try { - return new URL(`http://${host}`).hostname; - } catch { - return undefined; - } -} - function cookie(req: Request, name: string): string | undefined { for (const item of (req.headers.cookie ?? '').split(';')) { const separator = item.indexOf('='); @@ -89,7 +80,7 @@ export async function hostedAppPreviewGateway( next: NextFunction, ): Promise { if (!env.HOSTED_APPS_ENABLED || !env.HOSTED_APP_PREVIEW_ORIGIN) return next(); - const hostname = rawHostname(req); + const hostname = hostedAppRequestHostname(req.headers.host); const runtimeId = hostname ? hostedAppRuntimeIdFromHostname(hostname, env.HOSTED_APP_PREVIEW_ORIGIN) : undefined; diff --git a/service/src/middleware/execution-profile.ts b/service/src/middleware/execution-profile.ts index 38512dc4..37b8ba96 100644 --- a/service/src/middleware/execution-profile.ts +++ b/service/src/middleware/execution-profile.ts @@ -1,5 +1,6 @@ import type { NextFunction, Request, Response } from 'express'; import { env } from '../config'; +import { recordWorkspaceToolRejection } from '../workspace-tools/outcome'; import { checkExecutionProfileExpectation, EXECUTION_PROFILE_HEADER, @@ -48,5 +49,6 @@ export function executionProfileMiddleware( ? 'mismatch' : 'invalid', }); + recordWorkspaceToolRejection(res, expectation.body.error); res.status(expectation.status).json(expectation.body); } diff --git a/service/src/service-api.ts b/service/src/service-api.ts index 49516a29..6f9a87ed 100644 --- a/service/src/service-api.ts +++ b/service/src/service-api.ts @@ -7,6 +7,7 @@ import serviceRouter from './service/router'; import programmaticRouter from './service/programmatic-router'; import bridgeRouter from './bridge'; import workspaceToolsRouter from './workspace-tools'; +import { workspaceToolOutcomeLogging } from './workspace-tools/outcome'; import { connection } from './queue'; import { env } from './config'; import logger from './logger'; @@ -14,6 +15,7 @@ import hostedAppRouter from './hosted-app/router'; import { hostedAppPreviewGateway } from './hosted-app/preview-gateway'; const app = express(); +app.post('/v1/workspace-tools/execute', workspaceToolOutcomeLogging); app.disable('x-powered-by'); app.set('trust proxy', 1); app.use(executionProfileMiddleware); diff --git a/service/src/workspace-tools/outcome.ts b/service/src/workspace-tools/outcome.ts new file mode 100644 index 00000000..d965d515 --- /dev/null +++ b/service/src/workspace-tools/outcome.ts @@ -0,0 +1,79 @@ +import type { RequestHandler, Response } from 'express'; +import type { WorkspaceToolRequest } from '../../../packages/code/src/protocol'; +import logger from '../logger'; +import { env } from '../config'; +import { hostedAppRequestHostname, hostedAppRuntimeIdFromHostname } from '../hosted-app/preview-access'; + +interface WorkspaceToolOutcome { + operation?: WorkspaceToolRequest['operation']; + workerId?: string; + errorCode?: string; + deadlineBudgetMs?: number; + dispatchDurationMs?: number; + dispatchPending: boolean; + flush: () => void; +} + +const outcomes = new WeakMap(); +const earlyErrorCodes: Record = { + 400: 'INVALID_REQUEST', + 401: 'UNAUTHENTICATED', + 403: 'AUTHORIZATION_REJECTED', + 413: 'REQUEST_TOO_LARGE', + 415: 'UNSUPPORTED_MEDIA_TYPE', + 429: 'RATE_LIMITED', + 500: 'INTERNAL_ERROR', +}; + +export function getWorkspaceToolOutcome(res: Response): WorkspaceToolOutcome { + const existing = outcomes.get(res); + if (existing != null) return existing; + const startedAt = performance.now(); + let responseEndedAt: number | undefined; + let logged = false; + const outcome: WorkspaceToolOutcome = { + dispatchPending: false, + flush: (): void => { + if (logged || responseEndedAt == null || outcome.dispatchPending) return; + logged = true; + outcomes.delete(res); + const finished = res.writableFinished; + logger.log(finished && res.statusCode < 400 ? 'info' : 'warn', 'Workspace tool request completed', { + route: '/workspace-tools/execute', + operation: outcome.operation, + workerId: outcome.workerId, + status: finished ? res.statusCode : undefined, + outcome: finished ? 'completed' : 'disconnected', + errorCode: outcome.errorCode ?? (finished ? earlyErrorCodes[res.statusCode] : undefined), + durationMs: Math.round(responseEndedAt - startedAt), + dispatchDurationMs: outcome.dispatchDurationMs, + deadlineBudgetMs: outcome.deadlineBudgetMs, + }); + }, + }; + const onResponseEnd = (): void => { + res.removeListener('finish', onResponseEnd); + res.removeListener('close', onResponseEnd); + responseEndedAt = performance.now(); + outcome.flush(); + }; + outcomes.set(res, outcome); + res.once('finish', onResponseEnd); + res.once('close', onResponseEnd); + return outcome; +} + +export function recordWorkspaceToolRejection(res: Response, errorCode: string): void { + const outcome = outcomes.get(res); + if (outcome != null) outcome.errorCode = errorCode; +} + +/** Classify raw Host like the preview gateway without moving the earlier profile guard. */ +export const workspaceToolOutcomeLogging: RequestHandler = (req, res, next): void => { + if (req.method !== 'POST') return next(); + const hostname = hostedAppRequestHostname(req.headers.host); + if (env.HOSTED_APPS_ENABLED && env.HOSTED_APP_PREVIEW_ORIGIN && hostname != null && + hostedAppRuntimeIdFromHostname(hostname, env.HOSTED_APP_PREVIEW_ORIGIN) != null) return next(); + getWorkspaceToolOutcome(res); + next(); +}; diff --git a/service/src/workspace-tools/router.test.ts b/service/src/workspace-tools/router.test.ts index 5688d739..21aa86c7 100644 --- a/service/src/workspace-tools/router.test.ts +++ b/service/src/workspace-tools/router.test.ts @@ -1,18 +1,33 @@ import { createServer } from 'node:http'; import type { Server } from 'node:http'; -import { afterEach, expect, test } from 'bun:test'; +import { afterEach, beforeEach, expect, spyOn, test } from 'bun:test'; import express, { json } from 'express'; +import rateLimitFactory from 'express-rate-limit'; +import logger from '../logger'; +import { env } from '../config'; +import { apiKeyAuth } from '../middleware/auth'; +import { workspaceToolOutcomeLogging } from './outcome'; +import { executionProfileMiddleware } from '../middleware/execution-profile'; +import { hostedAppPreviewGateway } from '../hosted-app/preview-gateway'; import { applyPrincipal } from '../auth/principal'; import { BridgeStoreError } from '../bridge/store'; import { bridgeStoreStatus, createWorkspaceToolsRouter } from './router'; let server: Server | undefined; +let logCompleted: ReturnType>; +let logSpy: ReturnType>; + +beforeEach(() => { + logCompleted = Promise.withResolvers(); + logSpy = spyOn(logger, 'log').mockImplementation(() => { logCompleted.resolve(); return logger; }); +}); afterEach(() => { server?.close(); server = undefined; + logSpy.mockRestore(); }); test('maps invalid worker results to an upstream failure', () => { @@ -66,6 +81,16 @@ test('rejects new workspace dispatches while the service is shutting down', asyn expect(response.status).toBe(503); expect(dispatched).toBe(false); + expect(logSpy).toHaveBeenCalledTimes(1); + expect(logSpy).toHaveBeenCalledWith( + 'warn', + 'Workspace tool request completed', + expect.objectContaining({ + status: 503, + errorCode: 'SERVICE_SHUTTING_DOWN', + outcome: 'completed', + }), + ); }); test.each([ @@ -131,13 +156,33 @@ test.each([ }); expect(response.status).toBe(expectedStatus); - await expect(response.json()).resolves.toMatchObject({ code: errorCode }); + expect(logSpy).toHaveBeenCalledTimes(1); + expect(logSpy).toHaveBeenCalledWith( + 'warn', + 'Workspace tool request completed', + expect.objectContaining({ + status: expectedStatus, + errorCode, + operation: 'search_text', + workerId: 'user-worker', + dispatchDurationMs: expect.any(Number), + deadlineBudgetMs: 30_000, + }), + ); + await expect(response.json()).resolves.toMatchObject({ + code: errorCode, + }); }); test('dispatches an authenticated workspace tool request to the principal-bound worker', async () => { let dispatchArgs: Record | undefined; const app = express(); app.use(json()); + app.use((_req, res, next) => { + const send = res.json.bind(res); + res.json = (body): typeof res => { setTimeout(() => send(body), 120); return res; }; + next(); + }); app.use((req, _res, next) => { applyPrincipal(req, { userId: 'user-1', @@ -199,6 +244,21 @@ test('dispatches an authenticated workspace tool request to the principal-bound }); expect(response.status).toBe(200); + const timing = logSpy.mock.calls[0].at(-1) as { durationMs: number; dispatchDurationMs: number }; + expect(timing.durationMs - timing.dispatchDurationMs).toBeGreaterThanOrEqual(100); + expect(logSpy).toHaveBeenCalledTimes(1); + expect(logSpy).toHaveBeenCalledWith( + 'info', + 'Workspace tool request completed', + expect.objectContaining({ + status: 200, + operation: 'read_file', + workerId: 'user-worker', + outcome: 'completed', + }), + ); + expect(JSON.stringify(logSpy.mock.calls)).not.toContain('# LibreChat'); + expect(JSON.stringify(logSpy.mock.calls)).not.toContain('tenant-1'); await expect(response.json()).resolves.toMatchObject({ operation: 'read_file', content: '# LibreChat', @@ -210,3 +270,340 @@ test('dispatches an authenticated workspace tool request to the principal-bound request, }); }); + +test.each([ + ['WORKER_UNAUTHORIZED', 403], + ['ASSIGNMENT_INVALID', 400], + ['RESULT_INVALID', 502], + ['ASSIGNMENT_EXPIRED', 504], + ['WORKER_OFFLINE', 503], + ['WORKER_BUSY', 503], + ['WORKER_MISMATCH', 409], +] as const)('logs store rejection %s with actual HTTP %i', async (errorCode, expectedStatus) => { + const app = express(); + app.use(json()); + app.use((req, _res, next) => { + applyPrincipal(req, { + userId: 'user-1', + tenantId: 'tenant-1', + principalSource: 'librechat_jwt', + codeWorkerId: 'user-worker', + }); + next(); + }); + app.use( + createWorkspaceToolsRouter({ + backend: 'remote-bridge', + configuredWorkerId: 'user-worker', + dynamicWorkers: true, + timeoutMs: 300_000, + store: { + async dispatchWorkspaceTool() { + throw new BridgeStoreError(errorCode, 'private diagnostic details'); + }, + }, + }), + ); + server = createServer(app); + await new Promise((resolve) => server?.listen(0, '127.0.0.1', resolve)); + const address = server.address(); + if (address == null || typeof address === 'string') throw new Error('Expected TCP listener'); + const response = await fetch(`http://127.0.0.1:${address.port}/workspace-tools/execute`, { + method: 'POST', + headers: { 'Content-Type': 'application/json' }, + body: JSON.stringify({ + protocolVersion: 1, + operation: 'list_files', + workspaceId: 'primary', + }), + }); + expect(response.status).toBe(expectedStatus); + await response.text(); + expect(logSpy).toHaveBeenCalledTimes(1); + expect(logSpy).toHaveBeenCalledWith( + 'warn', + 'Workspace tool request completed', + expect.objectContaining({ + operation: 'list_files', + workerId: 'user-worker', + status: expectedStatus, + errorCode, + outcome: 'completed', + deadlineBudgetMs: 300_000, + dispatchDurationMs: expect.any(Number), + }), + ); + expect(JSON.stringify(logSpy.mock.calls)).not.toContain('private diagnostic details'); +}); + +test.each([ + ['unauthenticated', 401, 'UNAUTHENTICATED'], + ['invalid request', 400, 'INVALID_WORKSPACE_TOOL_REQUEST'], + ['selection denied', 403, 'WORKER_SELECTION_REJECTED'], + ['invalid worker', 400, 'WORKER_SELECTION_REJECTED'], + ['no backend', 503, 'WORKSPACE_BACKEND_UNAVAILABLE'], +] as const)('logs early %s without dispatching', async (scenario, status, errorCode) => { + const app = express(); + app.use(json()); + app.use((req, _res, next) => { + const workerId = scenario === 'invalid worker' ? 'bad/worker' : 'user-worker'; + if (scenario !== 'unauthenticated') + applyPrincipal(req, { + userId: 'user-1', + tenantId: 'tenant-1', + principalSource: 'librechat_jwt', + codeWorkerId: + scenario === 'no backend' ? undefined : workerId, + }); + next(); + }); + app.use( + createWorkspaceToolsRouter({ + backend: scenario === 'no backend' ? 'http' : 'remote-bridge', + configuredWorkerId: 'user-worker', + dynamicWorkers: true, + store: { + async dispatchWorkspaceTool() { + throw new Error('Must not dispatch'); + }, + }, + }), + ); + server = createServer(app); + await new Promise((resolve) => server?.listen(0, '127.0.0.1', resolve)); + const address = server.address(); + if (address == null || typeof address === 'string') throw new Error('Expected TCP listener'); + const response = await fetch(`http://127.0.0.1:${address.port}/workspace-tools/execute`, { + method: 'POST', + headers: { + 'Content-Type': 'application/json', + ...(scenario === 'selection denied' ? { 'X-LibreChat-Code-Worker-ID': 'forged-worker' } : {}), + }, + body: JSON.stringify( + scenario === 'invalid request' + ? { operation: 'private-untrusted-operation' } + : { + protocolVersion: 1, + operation: 'list_files', + workspaceId: 'primary', + }, + ), + }); + expect(response.status).toBe(status); + await response.text(); + expect(logSpy).toHaveBeenCalledTimes(1); + expect(logSpy).toHaveBeenCalledWith( + 'warn', + 'Workspace tool request completed', + expect.objectContaining({ + status, + errorCode, + dispatchDurationMs: undefined, + }), + ); + expect(JSON.stringify(logSpy.mock.calls)).not.toContain('forged-worker'); + expect(JSON.stringify(logSpy.mock.calls)).not.toContain('private-untrusted-operation'); +}); + +test('logs a disconnected dispatch once without inventing HTTP 200', async () => { + const app = express(); + const started = Promise.withResolvers(); + const closed = Promise.withResolvers(); + const settlementGate = Promise.withResolvers(); + let dispatchAborted = false; + let closeConnection = (): void => { throw new Error('connection not ready'); }; + app.use(json()); + app.use((req, res, next) => { + applyPrincipal(req, { userId: 'user-1', tenantId: 'tenant-1', principalSource: 'librechat_jwt', codeWorkerId: 'user-worker' }); + closeConnection = (): void => { res.destroy(); }; + res.once('close', () => closed.resolve()); + next(); + }); + app.use( + createWorkspaceToolsRouter({ + backend: 'remote-bridge', + configuredWorkerId: 'user-worker', + dynamicWorkers: true, + store: { + async dispatchWorkspaceTool({ signal }) { + started.resolve(); + return await new Promise((_resolve, reject) => { + signal.addEventListener( + 'abort', + () => { + dispatchAborted = true; + void settlementGate.promise.then(() => reject(new BridgeStoreError('ASSIGNMENT_EXPIRED', 'caller left'))); + }, + { once: true }, + ); + }); + }, + }, + }), + ); + server = createServer(app); + await new Promise((resolve) => server?.listen(0, '127.0.0.1', resolve)); + const address = server.address(); + if (address == null || typeof address === 'string') throw new Error('Expected TCP listener'); + const controller = new AbortController(); + const response = fetch(`http://127.0.0.1:${address.port}/workspace-tools/execute`, { + method: 'POST', + headers: { 'Content-Type': 'application/json' }, + signal: controller.signal, + body: JSON.stringify({ protocolVersion: 1, operation: 'list_files', workspaceId: 'primary' }), + }); + await started.promise; + closeConnection(); + await expect(response).rejects.toThrow(); + await closed.promise; + expect(dispatchAborted).toBe(true); + expect(logSpy).not.toHaveBeenCalled(); + settlementGate.resolve(); + await logCompleted.promise; + expect(logSpy).toHaveBeenCalledTimes(1); + expect(logSpy).toHaveBeenCalledWith( + 'warn', + 'Workspace tool request completed', + expect.objectContaining({ + outcome: 'disconnected', + errorCode: 'ASSIGNMENT_EXPIRED', + status: undefined, + operation: 'list_files', + workerId: 'user-worker', + }), + ); +}); + +test.each(['auth', 'limit'] as const)('logs requests rejected by upstream %s middleware', async (stage) => { + const originalLocalMode = env.LOCAL_MODE; + const originalProvider = process.env.CODEAPI_AUTH_PROVIDER; + env.LOCAL_MODE = false; + process.env.CODEAPI_AUTH_PROVIDER = 'librechat-jwt'; + try { + const app = express(); + app.post('/v1/workspace-tools/execute', workspaceToolOutcomeLogging); + app.use(json()); + if (stage === 'auth') app.use(apiKeyAuth); + else app.use(rateLimitFactory({ windowMs: 60_000, max: 1 })); + let reachedHandler = false; + app.post('/v1/workspace-tools/execute', (_req, res) => { + reachedHandler = true; + res.json({ ok: true }); + }); + server = createServer(app); + await new Promise((resolve) => server?.listen(0, '127.0.0.1', resolve)); + const address = server.address(); + if (address == null || typeof address === 'string') throw new Error('Expected TCP listener'); + const request = (): Promise => fetch(`http://127.0.0.1:${address.port}/v1/workspace-tools/execute`, { + method: 'POST', headers: { 'Content-Type': 'application/json' }, body: '{}', + }); + if (stage === 'limit') { + await (await request()).text(); + logSpy.mockClear(); + reachedHandler = false; + } + const response = await request(); + await response.text(); + expect(response.status).toBe(stage === 'auth' ? 401 : 429); + expect(reachedHandler).toBe(false); + expect(logSpy.mock.calls.filter(([, message]) => message === 'Workspace tool request completed')).toHaveLength(1); + expect(logSpy).toHaveBeenCalledWith('warn', 'Workspace tool request completed', expect.objectContaining({ + status: response.status, errorCode: stage === 'auth' ? 'UNAUTHENTICATED' : 'RATE_LIMITED', + dispatchDurationMs: undefined, outcome: 'completed', + })); + } finally { + env.LOCAL_MODE = originalLocalMode; + if (originalProvider == null) delete process.env.CODEAPI_AUTH_PROVIDER; + else process.env.CODEAPI_AUTH_PROVIDER = originalProvider; + } +}); + +test.each([ + ['preview', undefined, 401, undefined], + ['preview', 'stateful', 409, undefined], + ['api', 'stateful', 409, 'execution_profile_mismatch'], + ['api', 'invalid', 400, 'invalid_execution_profile'], +] as const)('classifies %s host traffic with expected profile %s', async (hostKind, expectedProfile, status, errorCode) => { + const saved = { enabled: env.HOSTED_APPS_ENABLED, origin: env.HOSTED_APP_PREVIEW_ORIGIN, profile: env.EXECUTION_PROFILE }; + env.HOSTED_APPS_ENABLED = true; + env.HOSTED_APP_PREVIEW_ORIGIN = 'https://apps.example.test'; + env.EXECUTION_PROFILE = 'default'; + try { + const app = express(); + app.post('/v1/workspace-tools/execute', workspaceToolOutcomeLogging); + app.use(executionProfileMiddleware); + app.use(hostedAppPreviewGateway); + app.use(json()); + app.post('/v1/workspace-tools/execute', (_req, res) => { res.sendStatus(200); }); + server = createServer(app); + await new Promise((resolve) => server?.listen(0, '127.0.0.1', resolve)); + const address = server.address(); + if (address == null || typeof address === 'string') throw new Error('Expected TCP listener'); + const response = await fetch(`http://127.0.0.1:${address.port}/v1/workspace-tools/execute`, { + method: 'POST', headers: { + 'Content-Type': 'application/json', + Host: hostKind === 'preview' ? `happ-${'a'.repeat(40)}.apps.example.test` : 'api.example.test', + ...(expectedProfile == null ? {} : { 'X-CodeAPI-Expected-Profile': expectedProfile }), + }, body: '{}', + }); + await response.text(); + expect(response.status).toBe(status); + if (hostKind === 'preview') { + expect(logSpy).not.toHaveBeenCalled(); + } else { + expect(logSpy).toHaveBeenCalledTimes(1); + expect(logSpy).toHaveBeenCalledWith('warn', 'Workspace tool request completed', expect.objectContaining({ + status, errorCode, dispatchDurationMs: undefined, + })); + } + } finally { + env.HOSTED_APPS_ENABLED = saved.enabled; + env.HOSTED_APP_PREVIEW_ORIGIN = saved.origin; + env.EXECUTION_PROFILE = saved.profile; + } +}); + +test.each([ + ['POST', '/v1/workspace-tools/execute', true], + ['POST', '/v1/workspace-tools/execute/?attempt=1', true], + ['POST', '/v1/workspace-tools/execute/unknown', false], + ['POST', '/v1/workspace-tools/execute-extra', false], + ['GET', '/v1/workspace-tools/execute', false], +] as const)('logs only workspace endpoint traffic: %s %s', async (method, path, shouldLog) => { + const app = express(); + app.post('/v1/workspace-tools/execute', workspaceToolOutcomeLogging); + app.use((_req, res) => { res.sendStatus(401); }); + server = createServer(app); + await new Promise((resolve) => server?.listen(0, '127.0.0.1', resolve)); + const address = server.address(); + if (address == null || typeof address === 'string') throw new Error('Expected TCP listener'); + const response = await fetch(`http://127.0.0.1:${address.port}${path}`, { method }); + await response.text(); + expect(response.status).toBe(401); + expect(logSpy).toHaveBeenCalledTimes(shouldLog ? 1 : 0); +}); + +test.each([ + ['unsupported encoding', 'application/json', 'unsupported', '{}', 415, 'UNSUPPORTED_MEDIA_TYPE'], + ['unsupported charset', 'application/json; charset=iso-8859-1', 'identity', '{}', 415, 'UNSUPPORTED_MEDIA_TYPE'], + ['invalid json', 'application/json', 'identity', '{', 400, 'INVALID_REQUEST'], + ['oversized json', 'application/json', 'identity', JSON.stringify({ content: 'x'.repeat(100) }), 413, 'REQUEST_TOO_LARGE'], +] as const)('classifies parser rejection: %s', async (_scenario, contentType, encoding, body, status, errorCode) => { + const app = express(); + app.post('/v1/workspace-tools/execute', workspaceToolOutcomeLogging); + app.use(json({ limit: 32 })); + app.post('/v1/workspace-tools/execute', (_req, res) => { res.sendStatus(200); }); + server = createServer(app); + await new Promise((resolve) => server?.listen(0, '127.0.0.1', resolve)); + const address = server.address(); + if (address == null || typeof address === 'string') throw new Error('Expected TCP listener'); + const response = await fetch(`http://127.0.0.1:${address.port}/v1/workspace-tools/execute`, { + method: 'POST', headers: { 'Content-Type': contentType, 'Content-Encoding': encoding }, body, + }); + await response.text(); + expect(response.status).toBe(status); + expect(logSpy).toHaveBeenCalledTimes(1); + expect(logSpy).toHaveBeenCalledWith('warn', 'Workspace tool request completed', expect.objectContaining({ + status, errorCode, dispatchDurationMs: undefined, outcome: 'completed', + })); +}); diff --git a/service/src/workspace-tools/router.ts b/service/src/workspace-tools/router.ts index 7f563891..b2b56e06 100644 --- a/service/src/workspace-tools/router.ts +++ b/service/src/workspace-tools/router.ts @@ -4,6 +4,7 @@ import type { RequestHandler, Response } from 'express'; import type { AuthenticatedRequest } from '../types'; import type { RedisBridgeStore } from '../bridge/store'; +import { getWorkspaceToolOutcome } from './outcome'; import { getPrincipalOrReject } from '../auth/principal'; import { BridgeStoreError } from '../bridge/store'; import { checkServiceShutDown } from '../lifecycle'; @@ -46,18 +47,27 @@ export function createWorkspaceToolsRouter(options: WorkspaceToolsRouterOptions) router.post( '/workspace-tools/execute', asyncRoute(async (req, res) => { + const outcome = getWorkspaceToolOutcome(res); + const deadlineBudgetMs = Math.max(1, options.timeoutMs ?? 30_000); + outcome.deadlineBudgetMs = deadlineBudgetMs; const principal = getPrincipalOrReject(req, res); - if (!principal) return; + if (!principal) { + outcome.errorCode = 'UNAUTHENTICATED'; + return; + } if ((options.isShuttingDown ?? checkServiceShutDown)()) { + outcome.errorCode = 'SERVICE_SHUTTING_DOWN'; res.status(503).json({ error: 'Service is shutting down' }); return; } if (!isWorkspaceToolRequest(req.body)) { + outcome.errorCode = 'INVALID_WORKSPACE_TOOL_REQUEST'; res.status(400).json({ error: 'Invalid workspace tool request', }); return; } + outcome.operation = req.body.operation; let selection: { workerId: string; explicit: boolean } | undefined; try { @@ -70,36 +80,44 @@ export function createWorkspaceToolsRouter(options: WorkspaceToolsRouterOptions) }); } catch (error) { if (error instanceof BridgeWorkerSelectionError) { + outcome.errorCode = 'WORKER_SELECTION_REJECTED'; res.status(error.status).json({ error: error.message }); return; } throw error; } if (selection == null) { + outcome.errorCode = 'WORKSPACE_BACKEND_UNAVAILABLE'; res.status(503).json({ error: 'Workspace tools require the remote-bridge backend', }); return; } + outcome.workerId = selection.workerId; const controller = new AbortController(); - const abort = () => controller.abort(); + const abort = (): void => controller.abort(); req.once('aborted', abort); - const abortClosedResponse = () => { + const abortClosedResponse = (): void => { if (!res.writableEnded) abort(); }; res.once('close', abortClosedResponse); try { + outcome.dispatchPending = true; + const dispatchStartedAt = performance.now(); const settlement = await options.store.dispatchWorkspaceTool({ workerId: selection.workerId, tenantId: principal.tenantId, requireTenantBinding: selection.explicit && (options.dynamicWorkers || selection.workerId !== options.configuredWorkerId), request: req.body, - deadlineAtMs: Date.now() + Math.max(1, options.timeoutMs ?? 30_000), + deadlineAtMs: Date.now() + deadlineBudgetMs, signal: controller.signal, + }).finally(() => { + outcome.dispatchDurationMs = Math.round(performance.now() - dispatchStartedAt); }); if (settlement.status === 'rejected') { + outcome.errorCode = settlement.errorCode ?? 'WORKSPACE_TOOL_REJECTED'; let status = 422; if ( settlement.errorCode === 'SEARCH_TIMEOUT' || @@ -129,14 +147,18 @@ export function createWorkspaceToolsRouter(options: WorkspaceToolsRouterOptions) res.status(200).json(settlement.result); } catch (error) { if (error instanceof BridgeStoreError) { + outcome.errorCode = error.code; res.status(bridgeStoreStatus(error)).json({ error: error.message, code: error.code, }); return; } + outcome.errorCode = 'INTERNAL_ERROR'; throw error; } finally { + outcome.dispatchPending = false; + outcome.flush(); req.removeListener('aborted', abort); res.removeListener('close', abortClosedResponse); } From b243c93f092baa0c1bf42f3f9ae6f74b3213d6b7 Mon Sep 17 00:00:00 2001 From: Danny Avila Date: Tue, 8 Sep 2026 08:39:26 -0400 Subject: [PATCH 3/4] =?UTF-8?q?=F0=9F=9B=9F=20fix:=20Recover=20Hosted=20Ap?= =?UTF-8?q?p=20Stop=20Failures=20(#155)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix: recover hosted app stop failures * fix: clear recovered stop error status * test: preserve startup failure details --- api/src/hosted-app.test.ts | 58 +++++++++++++++++++++++- api/src/hosted-app.ts | 93 +++++++++++++++++++++----------------- 2 files changed, 108 insertions(+), 43 deletions(-) diff --git a/api/src/hosted-app.test.ts b/api/src/hosted-app.test.ts index 727803d1..2d4ae9b1 100644 --- a/api/src/hosted-app.test.ts +++ b/api/src/hosted-app.test.ts @@ -337,7 +337,9 @@ describe('HostedAppSupervisor', () => { expect(error).toBeInstanceOf(HostedAppError); expect(error.code).toBe('hosted_app_start_failed'); + expect(error.message).toBe('hosted app exited'); expect(supervisor.status()?.state).toBe('failed'); + expect(supervisor.status()?.message).toBe('hosted app exited'); }); test('serializes quiesced workspace access and rejects it while an app is running', async () => { @@ -424,9 +426,63 @@ describe('HostedAppSupervisor', () => { expect(error).toBeInstanceOf(HostedAppError); expect(error.code).toBe('hosted_app_cleanup_failed'); expect(error.status).toBe(503); + expect(supervisor.status()).toMatchObject({ + state: 'failed', + message: 'hosted app cleanup failed', + }); permitCleanup = true; - await supervisor.shutdown(); + const recovered = await supervisor.stop(); + expect(recovered).toMatchObject({ state: 'stopped' }); + expect(recovered).not.toHaveProperty('message'); + }); + + test('allows checkpoint and restore to retry cleanup after stop fails', async () => { + const root = await workspace(); + let permitCleanup = false; + const fixture = dependencies(root, { + killCgroup: async () => { + if (!permitCleanup) throw new Error('cgroup remains populated'); + }, + }); + const supervisor = new HostedAppSupervisor(fixture.deps); + await supervisor.start(request()); + await expect(supervisor.stop()).rejects.toMatchObject({ + code: 'hosted_app_cleanup_failed', + status: 503, + }); + const operations: string[] = []; + + await expect(supervisor.withQuiescedWorkspace(async () => { + operations.push('unsafe checkpoint'); + })).rejects.toMatchObject({ + code: 'hosted_app_cleanup_failed', + status: 503, + }); + expect(operations).toEqual([]); + + permitCleanup = true; + await supervisor.withQuiescedWorkspace(async () => { operations.push('checkpoint'); }); + await supervisor.withQuiescedWorkspace(async () => { operations.push('restore'); }); + + expect(operations).toEqual(['checkpoint', 'restore']); + expect(fixture.cgroupKills).toHaveLength(3); + }); + + test('surfaces replacement cleanup failure as retryable without spawning', async () => { + const root = await workspace(); + const fixture = dependencies(root, { + killCgroup: async () => { throw new Error('cgroup remains populated'); }, + }); + const supervisor = new HostedAppSupervisor(fixture.deps); + await supervisor.start(request()); + + const error = await supervisor.start(request({ revision: 'rev-2' })).catch(value => value); + + expect(error).toBeInstanceOf(HostedAppError); + expect(error).toMatchObject({ code: 'hosted_app_cleanup_failed', status: 503 }); + expect(supervisor.status()?.state).toBe('failed'); + expect(fixture.spawns).toHaveLength(1); }); test('fails workspace mutation closed until a failed app cgroup is drained', async () => { diff --git a/api/src/hosted-app.ts b/api/src/hosted-app.ts index d8cbfc50..64a5aab8 100644 --- a/api/src/hosted-app.ts +++ b/api/src/hosted-app.ts @@ -458,18 +458,7 @@ export class HostedAppSupervisor { } async stop(): Promise { - return this.serialize(async () => { - try { - return await this.stopImpl(); - } catch (error) { - logger.error({ err: error }, 'Hosted-app stop cleanup failed'); - throw new HostedAppError( - 'hosted_app_cleanup_failed', - 'the hosted app could not be stopped safely', - 503, - ); - } - }); + return this.serialize(() => this.stopImpl()); } async shutdown(): Promise { @@ -729,42 +718,62 @@ export class HostedAppSupervisor { private async stopImpl(preserveActive = false): Promise { const active = this.active; if (!active) return undefined; - const child = active.process; - if (!child?.pid) { + try { + const child = active.process; + if (!child?.pid) { + await this.deps.killCgroup(); + active.cgroupDrained = true; + active.status.state = 'stopped'; + if (!preserveActive) delete active.status.message; + active.status.exited_at ??= this.deps.now().toISOString(); + if (!preserveActive) this.active = undefined; + return publicStatus(active); + } + + active.status.state = 'stopping'; + this.deps.killProcessGroup(child.pid, 'SIGTERM'); + const exited = new Promise(resolve => child.once('exit', () => resolve(true))); + let timer: ReturnType | undefined; + const timedOut = new Promise(resolve => { + timer = setTimeout(() => resolve(false), config.hosted_app_stop_timeout_ms); + timer.unref?.(); + }); + const stopped = await Promise.race([exited, timedOut]); + if (timer) clearTimeout(timer); + if (!stopped && active.process?.pid) { + await this.deps.killCgroup(); + await Promise.race([ + new Promise(resolve => child.once('exit', () => resolve())), + new Promise(resolve => setTimeout(resolve, config.hosted_app_stop_timeout_ms)), + ]); + } + /* Always sweep the cgroup: the tracked parent may have exited cleanly + * while a daemonized descendant stayed alive in a different process group. */ await this.deps.killCgroup(); active.cgroupDrained = true; active.status.state = 'stopped'; + if (!preserveActive) delete active.status.message; active.status.exited_at ??= this.deps.now().toISOString(); + const status = publicStatus(active); if (!preserveActive) this.active = undefined; - return publicStatus(active); - } - - active.status.state = 'stopping'; - this.deps.killProcessGroup(child.pid, 'SIGTERM'); - const exited = new Promise(resolve => child.once('exit', () => resolve(true))); - let timer: ReturnType | undefined; - const timedOut = new Promise(resolve => { - timer = setTimeout(() => resolve(false), config.hosted_app_stop_timeout_ms); - timer.unref?.(); - }); - const stopped = await Promise.race([exited, timedOut]); - if (timer) clearTimeout(timer); - if (!stopped && active.process?.pid) { - await this.deps.killCgroup(); - await Promise.race([ - new Promise(resolve => child.once('exit', () => resolve())), - new Promise(resolve => setTimeout(resolve, config.hosted_app_stop_timeout_ms)), - ]); + return status; + } catch (error) { + /* Keep the failed record so checkpoint/restore can retry the cgroup + * sweep. A `stopping` record would permanently reject those operations + * before they reach the recoverable cleanup path. */ + active.cgroupDrained = false; + active.status.state = 'failed'; + active.status.message = 'hosted app cleanup failed'; + logger.error( + { err: error, appId: active.request.app_id }, + 'Hosted-app stop cleanup failed', + ); + throw new HostedAppError( + 'hosted_app_cleanup_failed', + 'the hosted app could not be stopped safely', + 503, + ); } - /* Always sweep the cgroup: the tracked parent may have exited cleanly - * while a daemonized descendant stayed alive in a different process group. */ - await this.deps.killCgroup(); - active.cgroupDrained = true; - active.status.state = 'stopped'; - active.status.exited_at ??= this.deps.now().toISOString(); - const status = publicStatus(active); - if (!preserveActive) this.active = undefined; - return status; } } From 1d556c023e51a49f9a37e05da6cb6d952d5f1d5e Mon Sep 17 00:00:00 2001 From: Danny Avila Date: Tue, 8 Sep 2026 08:39:37 -0400 Subject: [PATCH 4/4] fix: encode the KVM resolver handoff for the kernel command line (#157) libkrun appends every guest environment entry to the kernel command line, which linux-loader restricts to single-line printable ASCII. Since #152 the launcher entrypoint exported the runner's whole /etc/resolv.conf, so any Docker-generated file (comments, blank lines, options) made krun_start_enter panic with InvalidAscii and sandbox-runner restart-looped. Forward only the resolver directives, whitespace-normalized and joined by '|', and expand them back into lines in the guest wrapper. Validate every forwarded variable in the launcher against the command line's charset, the kernel's quote handling and its 2048-byte size limit, failing with a named error before libkrun can panic. Run the launcher's unit tests in CI. --- .github/workflows/ci.yml | 18 ++++ README.md | 9 ++ api/src/guest-dns.sh | 10 +- launcher/entrypoint.sh | 31 +++++- launcher/src/main.rs | 199 +++++++++++++++++++++++++++++++++++++-- tests/kvm_guest_dns.sh | 110 +++++++++++++++++++--- 6 files changed, 351 insertions(+), 26 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 6f79328e..71536caa 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -48,6 +48,24 @@ jobs: docker buildx build --check -f api/Dockerfile . docker buildx build --check -f docker/Dockerfile.worker-sandbox . + launcher-unit-tests: + name: Launcher Unit Tests + runs-on: ubuntu-latest + container: fedora:43 + defaults: + run: + working-directory: launcher + steps: + - uses: actions/checkout@df4cb1c069e1874edd31b4311f1884172cec0e10 # v6 + + - name: Install Rust and libkrun + # Mirrors launcher/Dockerfile's builder stage; libkrun is only packaged + # for Fedora, and the guest-environment checks link against it. + run: dnf install -y --setopt=install_weak_deps=False rust cargo libkrun-devel gcc + + - name: Cargo tests + run: cargo test + api-unit-tests: name: API Unit Tests runs-on: ubuntu-latest diff --git a/README.md b/README.md index 1e6432f7..7bff76fb 100644 --- a/README.md +++ b/README.md @@ -153,6 +153,15 @@ read-only root disk does not need modification at boot. Rebuild the runner image to pick up this layout change. A missing resolver handoff fails startup rather than leaving the guest with an unrelated public DNS server. +libkrun delivers the guest environment on the kernel command line, which only +carries single-line printable ASCII and is capped at 2048 bytes by the guest +kernel. The launcher entrypoint therefore forwards only the `nameserver`, +`search`, `domain`, `options` and `sortlist` directives, joined by `|`, and the +guest wrapper expands them back into `/etc/resolv.conf` lines. The launcher +rejects any forwarded variable that would not survive that trip (control +characters, non-ASCII bytes, quoting the kernel would split, or an oversized +environment) with a named error instead of a libkrun panic and restart loop. + To validate a deployment, execute code that creates a file in `/mnt/data`, confirm the response includes its file reference, and download it. Recreate the egress gateway with a different container IP while leaving the runner alive, diff --git a/api/src/guest-dns.sh b/api/src/guest-dns.sh index 9795ca7f..d8371444 100644 --- a/api/src/guest-dns.sh +++ b/api/src/guest-dns.sh @@ -2,6 +2,10 @@ # The guest root may be read-only. Bake the link, populate its target only # after /run is mounted, and leave direct NsJail/Lambda resolvers untouched. +# launcher/entrypoint.sh joins resolver directives with this separator because +# the handoff rides the guest kernel command line, which cannot carry newlines. +RESOLV_FIELD_SEPARATOR='|' + prepare_guest_dns() { local root="$1" mkdir -p "$root/run" @@ -12,18 +16,20 @@ prepare_guest_dns() { configure_guest_dns() { local root="${1:-}" local target="$root/run/codeapi-resolver" + local resolv_conf="${SANDBOX_RESOLV_CONF:-}" + resolv_conf="${resolv_conf//"$RESOLV_FIELD_SEPARATOR"/$'\n'}" if [ ! -L "$root/etc/resolv.conf" ] || \ [ "$(readlink "$root/etc/resolv.conf")" != '../run/codeapi-resolver/resolv.conf' ]; then return 0 fi - if ! printf '%s\n' "${SANDBOX_RESOLV_CONF:-}" | grep -Eq '^[[:space:]]*nameserver[[:space:]]+[^[:space:]#]'; then + if ! printf '%s\n' "$resolv_conf" | grep -Eq '^[[:space:]]*nameserver[[:space:]]+[^[:space:]#]'; then echo 'ERROR: KVM guest requires resolver configuration from launcher-entrypoint.sh' >&2 return 1 fi # A fresh, root-owned directory prevents a sandbox UID from replacing DNS # configuration in the runtime mount. Never reuse a pre-existing entry. (umask 077; mkdir "$target") || return 1 - printf '%s\n' "$SANDBOX_RESOLV_CONF" > "$target/resolv.conf" || return 1 + printf '%s\n' "$resolv_conf" > "$target/resolv.conf" || return 1 chmod 600 "$target/resolv.conf" || return 1 unset SANDBOX_RESOLV_CONF } diff --git a/launcher/entrypoint.sh b/launcher/entrypoint.sh index 0369ec2a..304858e4 100644 --- a/launcher/entrypoint.sh +++ b/launcher/entrypoint.sh @@ -5,8 +5,35 @@ set -e # names intact so new connections can resolve replacements after a restart. # Forward the resolver and search domains supplied by Docker or Kubernetes, # rather than pinning endpoint IPs or baking a deployment-specific nameserver. -export SANDBOX_RESOLV_CONF="$(cat /etc/resolv.conf)" -if ! printf '%s\n' "$SANDBOX_RESOLV_CONF" | grep -Eq '^[[:space:]]*nameserver[[:space:]]+[^[:space:]#]'; then +# +# libkrun places every guest environment entry on the kernel command line, +# which accepts only single-line printable ASCII and is truncated by the guest +# kernel past 2048 bytes. Keep the resolver directives alone, one per field, +# joined by a separator that api/src/guest-dns.sh expands back into lines. +RESOLV_FIELD_SEPARATOR='|' + +encode_resolv_conf() { + local LC_ALL=C + local line words encoded='' + while IFS= read -r line || [ -n "$line" ]; do + line="${line%$'\r'}" + if [[ ! "$line" =~ ^[[:space:]]*(nameserver|search|domain|options|sortlist)[[:space:]] ]]; then + continue + fi + read -ra words <<< "$line" + line="${words[*]}" + if [[ "$line" == *[!' '-'~']* || "$line" == *[\"$RESOLV_FIELD_SEPARATOR]* ]]; then + echo "ERROR: runner /etc/resolv.conf line cannot cross the kernel command line: $line" >&2 + return 1 + fi + encoded+="${encoded:+$RESOLV_FIELD_SEPARATOR}$line" + done + printf '%s' "$encoded" +} + +SANDBOX_RESOLV_CONF="$(encode_resolv_conf < /etc/resolv.conf)" +export SANDBOX_RESOLV_CONF +if [[ "$RESOLV_FIELD_SEPARATOR$SANDBOX_RESOLV_CONF" != *"${RESOLV_FIELD_SEPARATOR}nameserver "[!\#]* ]]; then echo 'ERROR: runner /etc/resolv.conf has no nameserver' >&2 exit 1 fi diff --git a/launcher/src/main.rs b/launcher/src/main.rs index cbc4b5fc..02d26418 100644 --- a/launcher/src/main.rs +++ b/launcher/src/main.rs @@ -479,6 +479,92 @@ fn is_allowed_guest_env_key(key: &str, egress_gateway_enabled: bool) -> bool { false } +/// The guest kernel keeps at most this many command-line bytes (`COMMAND_LINE_SIZE` +/// on x86-64 and aarch64). libkrun on x86-64 assembles a longer line without +/// complaint and the kernel silently drops the tail, which carries the guest +/// environment and the `-- ` epilog. +const GUEST_CMDLINE_LIMIT: usize = 2048; +/// libkrun's own entries ahead of the environment: its default kernel +/// parameters, `init=/init.krun`, `KRUN_INIT`, the block-root and rlimit +/// entries and `tsi_hijack`. About 260 bytes for this launcher; the reserve +/// leaves headroom for libkrun to grow. +const LIBKRUN_CMDLINE_RESERVE: usize = 384; + +/// libkrun wraps each entry in double quotes and joins them with spaces. +fn quoted_cmdline_len<'a>(items: impl Iterator) -> usize { + items.map(|item| item.len() + 3).sum() +} + +/// libkrun hands the guest its environment on the kernel command line, one +/// double-quoted `KEY=VALUE` token per entry. linux-loader rejects anything +/// outside printable ASCII with an `InvalidAscii` panic before boot. The guest +/// kernel's `next_arg()` then toggles quoting on every double quote inside the +/// token, ends it at the first unquoted space, and strips a leading quote from +/// the value, so an entry only survives when its quotes balance, no space is +/// left unquoted, and the value does not open with a quote. +fn guest_env_entry_problem(key: &str, value: &str) -> Option { + let mut in_quote = true; + for (offset, byte) in value.bytes().enumerate() { + match byte { + b'"' => in_quote = !in_quote, + b' ' if !in_quote => { + return Some(format!( + "{key} has a space at offset {offset} outside balanced double quotes; the guest kernel splits the entry there" + )); + } + b' '..=b'~' => {} + _ => { + return Some(format!( + "{key} contains byte 0x{byte:02x} at offset {offset}; guest environment entries travel on the kernel command line as single-line printable ASCII" + )); + } + } + } + if !in_quote { + return Some(format!( + "{key} has unbalanced double quotes; the guest kernel merges the next entry into it" + )); + } + if value.starts_with('"') { + return Some(format!( + "{key} starts with a double quote, which the guest kernel strips from the value" + )); + } + None +} + +fn guest_cmdline_problem(env: &[(String, String)], args: &[String]) -> Option { + if let Some(problem) = env + .iter() + .find_map(|(key, value)| guest_env_entry_problem(key, value)) + { + return Some(problem); + } + + let entries: Vec = env.iter().map(|(key, value)| format!("{key}={value}")).collect(); + let env_bytes = quoted_cmdline_len(entries.iter().map(String::as_str)); + let args_bytes = " -- ".len() + quoted_cmdline_len(args.iter().map(String::as_str)); + let budget = GUEST_CMDLINE_LIMIT.saturating_sub(LIBKRUN_CMDLINE_RESERVE + args_bytes); + if env_bytes <= budget { + return None; + } + + let mut sizes: Vec<(&str, usize)> = env + .iter() + .map(|(key, value)| (key.as_str(), value.len())) + .collect(); + sizes.sort_by(|a, b| b.1.cmp(&a.1).then_with(|| a.0.cmp(b.0))); + let largest: Vec = sizes + .iter() + .take(3) + .map(|(key, len)| format!("{key} ({len} bytes)")) + .collect(); + Some(format!( + "guest environment needs {env_bytes} bytes on the kernel command line but only {budget} fit under the {GUEST_CMDLINE_LIMIT}-byte kernel limit; largest entries: {}", + largest.join(", ") + )) +} + fn main() { let vcpus: u8 = env::var("LAUNCHER_VCPUS") .ok() @@ -528,19 +614,28 @@ fn main() { let egress_gateway_enabled = env::var("EGRESS_GATEWAY_URL") .map(|value| !value.trim().is_empty()) .unwrap_or(false); - let env_strs: Vec = env::vars() + let guest_env: Vec<(String, String)> = env::vars() .filter(|(k, _)| !k.starts_with("LAUNCHER_")) .filter(|(k, _)| is_allowed_guest_env_key(k, egress_gateway_enabled)) + .collect(); + // krun_set_exec supplies argv[0]; this array contains arguments only. + let guest_args: Vec = vec![ + "/sandbox_api/guest-dns.sh".into(), + "--exec".into(), + exec_path.clone(), + ]; + if let Some(problem) = guest_cmdline_problem(&guest_env, &guest_args) { + eprintln!("[launcher] ERROR: {problem}"); + process::exit(1); + } + + let env_strs: Vec = guest_env + .iter() .map(|(k, v)| cstr(&format!("{k}={v}"))) .collect(); let env_ptrs = null_term(&env_strs); - // krun_set_exec supplies argv[0]; this array contains arguments only. - let argv_strs: Vec = vec![ - cstr("/sandbox_api/guest-dns.sh"), - cstr("--exec"), - cstr(&exec_path), - ]; + let argv_strs: Vec = guest_args.iter().map(|arg| cstr(arg)).collect(); let argv_ptrs = null_term(&argv_strs); let rlimit_strs: Vec = vec![guest_nofile_rlimit(nofile_target)]; @@ -618,7 +713,95 @@ fn main() { #[cfg(test)] mod tests { - use super::{desired_nofile_soft_limit, guest_nofile_rlimit, is_allowed_guest_env_key}; + use super::{ + desired_nofile_soft_limit, guest_cmdline_problem, guest_nofile_rlimit, + is_allowed_guest_env_key, GUEST_CMDLINE_LIMIT, LIBKRUN_CMDLINE_RESERVE, + }; + + fn guest_args() -> Vec { + vec![ + "/sandbox_api/guest-dns.sh".into(), + "--exec".into(), + "/sandbox_api/entrypoint.sh".into(), + ] + } + + fn env(entries: &[(&str, &str)]) -> Vec<(String, String)> { + entries + .iter() + .map(|(key, value)| (key.to_string(), value.to_string())) + .collect() + } + + #[test] + fn guest_cmdline_accepts_single_line_printable_env() { + let env = env(&[ + ("SANDBOX_RESOLV_CONF", "nameserver 127.0.0.11|options ndots:0"), + ("EGRESS_GATEWAY_URL", "http://egress_gateway:3190"), + ("PATH", "/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin"), + ]); + assert_eq!(guest_cmdline_problem(&env, &guest_args()), None); + } + + #[test] + fn guest_cmdline_rejects_multi_line_resolv_conf_before_libkrun() { + let env = env(&[("SANDBOX_RESOLV_CONF", "# Generated by Docker Engine.\nnameserver 127.0.0.11\noptions ndots:0")]); + let problem = guest_cmdline_problem(&env, &guest_args()).expect("newline must be rejected"); + assert!(problem.starts_with("SANDBOX_RESOLV_CONF contains byte 0x0a at offset "), "{problem}"); + } + + #[test] + fn guest_cmdline_rejects_control_and_non_ascii_bytes() { + for value in ["a\tb", "caf\u{e9}", "\u{7f}", "a\rb", "\u{1b}[0m"] { + let env = env(&[("SANDBOX_LIMIT_OVERRIDES", value)]); + let problem = guest_cmdline_problem(&env, &guest_args()).expect("non-printable bytes must be rejected"); + assert!(problem.starts_with("SANDBOX_LIMIT_OVERRIDES contains byte 0x"), "{problem}"); + } + } + + #[test] + fn guest_cmdline_keeps_quotes_the_kernel_parser_preserves() { + for value in ["{\"python\":{\"run_timeout\":30}}", "{\"python\": {\"run_timeout\": 30}}", "plain words with spaces"] { + let env = env(&[("SANDBOX_LIMIT_OVERRIDES", value)]); + assert_eq!(guest_cmdline_problem(&env, &guest_args()), None, "{value}"); + } + } + + #[test] + fn guest_cmdline_rejects_quotes_that_split_or_merge_kernel_tokens() { + let cases = [ + ("{\"a b\":1}", "SANDBOX_LIMIT_OVERRIDES has a space at offset 3 outside balanced double quotes;"), + ("say \"hi", "SANDBOX_LIMIT_OVERRIDES has unbalanced double quotes;"), + ("\"quoted\"", "SANDBOX_LIMIT_OVERRIDES starts with a double quote,"), + ]; + for (value, expected) in cases { + let env = env(&[("SANDBOX_LIMIT_OVERRIDES", value)]); + let problem = guest_cmdline_problem(&env, &guest_args()).expect(value); + assert!(problem.starts_with(expected), "{problem}"); + } + } + + #[test] + fn guest_cmdline_rejects_env_that_overflows_the_kernel_limit() { + let key = "SANDBOX_EXECUTION_MANIFEST_PUBLIC_KEY"; + let big = "A".repeat(1_700); + let env = env(&[(key, big.as_str()), ("SANDBOX_RESOLV_CONF", "nameserver 127.0.0.11")]); + let problem = guest_cmdline_problem(&env, &guest_args()).expect("oversized env must be rejected"); + assert!(problem.contains(&format!("under the {GUEST_CMDLINE_LIMIT}-byte kernel limit")), "{problem}"); + assert!(problem.contains(&format!("largest entries: {key} (1700 bytes), SANDBOX_RESOLV_CONF (21 bytes)")), "{problem}"); + } + + #[test] + fn guest_cmdline_budget_accounts_for_libkrun_reserve_and_args() { + let args = guest_args(); + let args_bytes = " -- ".len() + args.iter().map(|arg| arg.len() + 3).sum::(); + let budget = GUEST_CMDLINE_LIMIT - LIBKRUN_CMDLINE_RESERVE - args_bytes; + let key = "SANDBOX_LIMIT_OVERRIDES"; + let exact = "A".repeat(budget - key.len() - "=".len() - 3); + assert_eq!(guest_cmdline_problem(&env(&[(key, exact.as_str())]), &args), None); + let over = format!("{exact}A"); + assert!(guest_cmdline_problem(&env(&[(key, over.as_str())]), &args).is_some()); + } #[test] fn guest_env_allowlist_blocks_control_plane_and_secret_vars() { diff --git a/tests/kvm_guest_dns.sh b/tests/kvm_guest_dns.sh index e00e3e38..4c073de2 100755 --- a/tests/kvm_guest_dns.sh +++ b/tests/kvm_guest_dns.sh @@ -26,6 +26,14 @@ printf '%s\n' "$SANDBOX_RESOLV_CONF" > "$TEST_DIR/expected" configure_guest_dns "$TEST_DIR/guest" cmp "$TEST_DIR/expected" "$TEST_DIR/guest/etc/resolv.conf" +# The launcher entrypoint joins directives with a separator so the handoff +# survives the kernel command line; the guest expands it back into lines. +rm -rf "$TEST_DIR/guest/run/codeapi-resolver" +SANDBOX_RESOLV_CONF='nameserver 10.96.0.10|search tenant.svc.cluster.local svc.cluster.local cluster.local|options ndots:5' +configure_guest_dns "$TEST_DIR/guest" +cmp "$TEST_DIR/expected" "$TEST_DIR/guest/etc/resolv.conf" +[[ ! -v SANDBOX_RESOLV_CONF ]] + # Never reuse a stale directory or follow an attacker-controlled runtime link. SANDBOX_RESOLV_CONF='nameserver 127.0.0.11' if configure_guest_dns "$TEST_DIR/guest" 2>/dev/null; then @@ -47,6 +55,10 @@ SANDBOX_RESOLV_CONF='# no nameserver' if configure_guest_dns "$TEST_DIR/guest" 2>/dev/null; then echo 'accepted empty guest resolver' >&2; exit 1 fi +SANDBOX_RESOLV_CONF='search example.com|options ndots:0' +if configure_guest_dns "$TEST_DIR/guest" 2>/dev/null; then + echo 'accepted encoded guest resolver without nameserver' >&2; exit 1 +fi # Direct NsJail and Lambda retain the resolver managed by their container. mkdir -p "$TEST_DIR/direct/etc" @@ -71,16 +83,81 @@ cat > "$TEST_DIR/bin/getent" <<'STUB' printf '192.0.2.99 stale-address\n' STUB chmod +x "$TEST_DIR/bin/launcher" "$TEST_DIR/bin/getent" -sed "s|/usr/local/bin/launcher|$TEST_DIR/bin/launcher|g" "$ROOT/launcher/entrypoint.sh" > "$TEST_DIR/entrypoint.sh" -PATH="$TEST_DIR/bin:$PATH" \ -EGRESS_GATEWAY_URL='https://egress_gateway:3190/base' \ -FILE_SERVER_URL='http://[::1]:3000/base' \ -SANDBOX_FORWARD_TARGET='tool_call_server:3033' \ -LAUNCHER_FILTER_VSOCK_ENOTCONN=false \ -TEST_RESOLVER_OUTPUT="$TEST_DIR/forwarded" \ -bash "$TEST_DIR/entrypoint.sh" -printf '%s\n' "$(cat /etc/resolv.conf)" > "$TEST_DIR/expected" -cmp "$TEST_DIR/expected" "$TEST_DIR/forwarded" +# libkrun appends every guest environment entry to the kernel command line, +# which linux-loader limits to single-line printable ASCII. Docker's generated +# resolv.conf is multi-line with comments, so the entrypoint must forward only +# the resolver directives, joined by the separator guest-dns.sh expands. +run_entrypoint() { + local resolv_conf="$1" + sed -e "s|/usr/local/bin/launcher|$TEST_DIR/bin/launcher|g" \ + -e "s|/etc/resolv.conf|$resolv_conf|g" \ + "$ROOT/launcher/entrypoint.sh" > "$TEST_DIR/entrypoint.sh" + rm -f "$TEST_DIR/forwarded" + PATH="$TEST_DIR/bin:$PATH" \ + EGRESS_GATEWAY_URL='https://egress_gateway:3190/base' \ + FILE_SERVER_URL='http://[::1]:3000/base' \ + SANDBOX_FORWARD_TARGET='tool_call_server:3033' \ + LAUNCHER_FILTER_VSOCK_ENOTCONN=false \ + TEST_RESOLVER_OUTPUT="$TEST_DIR/forwarded" \ + bash "$TEST_DIR/entrypoint.sh" +} +# Independent reference for the expected handoff: directives only, whitespace +# collapsed, joined by the separator. +encoded_reference() { + LC_ALL=C awk '/^[[:space:]]*(nameserver|search|domain|options|sortlist)[[:space:]]/ { sub(/\r$/, ""); $1 = $1; print }' "$1" | paste -sd '|' - +} +cat > "$TEST_DIR/docker-resolv.conf" <<'RESOLV' +# Generated by Docker Engine. +# This file can be edited; Docker Engine will not make further changes once it +# has been modified. + +nameserver 127.0.0.11 +options ndots:0 + +# Based on host file: '/etc/resolv.conf' (internal resolver) +# ExtServers: [host(10.255.255.254)] +# Overrides: [] +# Option ndots from: internal +RESOLV +run_entrypoint "$TEST_DIR/docker-resolv.conf" +[[ "$(cat "$TEST_DIR/forwarded")" == 'nameserver 127.0.0.11|options ndots:0' ]] +[[ "$(cat "$TEST_DIR/forwarded")" == "$(encoded_reference "$TEST_DIR/docker-resolv.conf")" ]] +[[ "$(wc -l < "$TEST_DIR/forwarded")" == 1 ]] +if LC_ALL=C grep -q '[^ -~]' "$TEST_DIR/forwarded"; then + echo 'forwarded resolver contains bytes the kernel command line rejects' >&2; exit 1 +fi +# The guest restores exactly the directives Docker supplied. +rm -rf "$TEST_DIR/guest/run/codeapi-resolver" +SANDBOX_RESOLV_CONF="$(cat "$TEST_DIR/forwarded")" configure_guest_dns "$TEST_DIR/guest" +printf 'nameserver 127.0.0.11\noptions ndots:0\n' > "$TEST_DIR/expected" +cmp "$TEST_DIR/expected" "$TEST_DIR/guest/etc/resolv.conf" + +# Kubernetes resolvers: tabs, CRLF, trailing spaces, and unknown keywords are +# normalized away; search domains and options survive intact. +printf 'nameserver\t10.96.0.10 \r\nsearch tenant.svc.cluster.local svc.cluster.local cluster.local\n; resolver comment\nlookup file bind\noptions ndots:5\n' > "$TEST_DIR/k8s-resolv.conf" +run_entrypoint "$TEST_DIR/k8s-resolv.conf" +[[ "$(cat "$TEST_DIR/forwarded")" == 'nameserver 10.96.0.10|search tenant.svc.cluster.local svc.cluster.local cluster.local|options ndots:5' ]] + +# The runner's own resolver must round-trip through the same reference. +run_entrypoint /etc/resolv.conf +[[ "$(cat "$TEST_DIR/forwarded")" == "$(encoded_reference /etc/resolv.conf)" ]] + +# Content that cannot cross the kernel command line fails before the launcher +# starts, instead of a libkrun InvalidAscii panic and a restart loop. +for bad in $'nameserver 1.1.1.1\nsearch caf\xc3\xa9.example\n' $'nameserver 1.1.1.1\nsearch a|b\n' $'nameserver 1.1.1.1\noptions "ndots:1"\n'; do + printf '%s' "$bad" > "$TEST_DIR/bad-resolv.conf" + if run_entrypoint "$TEST_DIR/bad-resolv.conf" 2> "$TEST_DIR/entrypoint-error"; then + echo 'forwarded resolver content the kernel command line cannot carry' >&2; exit 1 + fi + grep -q 'cannot cross the kernel command line' "$TEST_DIR/entrypoint-error" + [[ ! -e "$TEST_DIR/forwarded" ]] +done +printf '# comments only\nsearch example.com\n' > "$TEST_DIR/bad-resolv.conf" +if run_entrypoint "$TEST_DIR/bad-resolv.conf" 2> "$TEST_DIR/entrypoint-error"; then + echo 'started launcher without a nameserver' >&2; exit 1 +fi +grep -q 'has no nameserver' "$TEST_DIR/entrypoint-error" +[[ ! -e "$TEST_DIR/forwarded" ]] # Every rootfs assembly path must prepare DNS after COPY, before disk creation. python3 - "$ROOT" <<'PY' @@ -98,13 +175,18 @@ for name, count in [('api/Dockerfile', 2), ('docker/Dockerfile.worker-sandbox', assert stage.index('--prepare-rootfs /sandbox-rootfs') < stage.index('/usr/local/bin/build-rootfs-image.sh /sandbox-rootfs /sandbox-rootfs.img'), name text = (root / 'launcher/src/main.rs').read_text() assert '"SANDBOX_RESOLV_CONF"' in text.split('const ALLOW_EXACT:')[1].split('];')[0] -argv = text.split('let argv_strs:')[1].split('let argv_ptrs:')[0] +argv = text.split('let guest_args:')[1].split('let env_strs:')[0] # libkrun init supplies argv[0]. Repeating the binary here makes Bash try to # interpret /bin/bash itself as a shell script instead of the DNS wrapper. -assert 'cstr("/bin/bash")' not in argv -assert 'cstr("/sandbox_api/guest-dns.sh")' in argv -assert 'cstr("--exec")' in argv and 'cstr(&exec_path)' in argv +assert '"/bin/bash"' not in argv +assert '"/sandbox_api/guest-dns.sh".into()' in argv +assert '"--exec".into()' in argv and 'exec_path.clone()' in argv assert 'let exec_c = cstr("/bin/bash")' in text +assert 'guest_args.iter().map(|arg| cstr(arg))' in text +# Every forwarded entry is checked against the kernel command line's charset, +# quoting and size rules before libkrun can panic on it. +assert text.index('guest_cmdline_problem(&guest_env, &guest_args)') < text.index('ffi::krun_set_exec(') +assert text.index('is_allowed_guest_env_key(k, egress_gateway_enabled)') < text.index('guest_cmdline_problem(&guest_env, &guest_args)') PY # The wrapper configures DNS before a custom guest executable, independently # of the normal API entrypoint and its later /tmp mount.