From f3920f77753e998da8f8e9afbf83d0f9e5756bc1 Mon Sep 17 00:00:00 2001 From: Sudhir Verma <9924513+sudhirverma@users.noreply.github.com> Date: Fri, 17 Jul 2026 14:19:13 +0530 Subject: [PATCH 1/5] fix(daemon): validate HTTP API bodies and config write paths (H4/H5) Add Zod schemas for every mutating web route and allowlist workspace locations so POST /api/config cannot traverse or write outside known paths. --- docs/CONFIGURATION.md | 5 + docs/CORE.md | 15 + docs/TESTING.md | 4 +- docs/decisions.md | 21 + .../daemon/src/core/config-schema.test.ts | 89 +++++ packages/daemon/src/core/config-schema.ts | 167 ++++++++ packages/daemon/src/core/index.ts | 10 + packages/daemon/src/daemon/web/api-schemas.ts | 211 ++++++++++ .../daemon/src/daemon/web/openai-compat.ts | 21 +- packages/daemon/src/daemon/web/server.ts | 361 ++++++++++++------ .../src/daemon/web/validate-body.test.ts | 108 ++++++ .../daemon/src/daemon/web/validate-body.ts | 135 +++++++ .../daemon/tests/integration/web-sse.test.ts | 79 +++- 13 files changed, 1095 insertions(+), 131 deletions(-) create mode 100644 packages/daemon/src/core/config-schema.test.ts create mode 100644 packages/daemon/src/core/config-schema.ts create mode 100644 packages/daemon/src/daemon/web/api-schemas.ts create mode 100644 packages/daemon/src/daemon/web/validate-body.test.ts create mode 100644 packages/daemon/src/daemon/web/validate-body.ts diff --git a/docs/CONFIGURATION.md b/docs/CONFIGURATION.md index afd1b83..be4a2a9 100644 --- a/docs/CONFIGURATION.md +++ b/docs/CONFIGURATION.md @@ -309,6 +309,11 @@ Click the settings icon to choose where config is saved: - **User**: `~/.config/abbenay/config.yaml` (default) - **Workspace**: `/.config/abbenay/config.yaml` (requires VS Code connection) +HTTP `POST /api/config` validates the body with Zod (`ConfigFile` shape) and +only allows workspace `location` values that match a currently connected / +allowlisted workspace path. Path traversal (`..`) and unknown locations are +rejected with no file write. + ## VS Code Extension The VS Code extension: diff --git a/docs/CORE.md b/docs/CORE.md index eee07e5..7852c2e 100644 --- a/docs/CORE.md +++ b/docs/CORE.md @@ -124,6 +124,21 @@ config.providers!['my-openai'].models!['gpt-4o'] = { temperature: 0.7 }; saveConfig(config); ``` +### Config validation (Zod) + +`ConfigFileSchema` and `PolicyConfigSchema` (plus `parseConfigFile`) validate +config-shaped objects before persistence. The daemon HTTP API uses the same +schemas for `POST /api/config` and related routes. + +```typescript +import { parseConfigFile } from '@abbenay/core'; + +const result = parseConfigFile(raw); +if (!result.success) { + console.error(result.error.issues); +} +``` + ## API Reference ### CoreState diff --git a/docs/TESTING.md b/docs/TESTING.md index 1b3e8a9..f8b6e51 100644 --- a/docs/TESTING.md +++ b/docs/TESTING.md @@ -68,12 +68,14 @@ Pure unit tests with no I/O, no network, no processes. |------|----------------| | `src/core/mock.test.ts` | Mock engine: echo, fixed, error, empty, slow modes | | `src/core/config.test.ts` | Config loading, merging, validation | +| `src/core/config-schema.test.ts` | Zod ConfigFile / PolicyConfig schemas (types, field injection) | | `src/core/tool-registry.test.ts` | Glob matching, tool registration, resolution, policy filtering | | `src/core/tool-approval.test.ts` | Shared validator: disabled / auto_approve / require_approval / default ask | | `src/daemon/mcp-server.test.ts` | MCP authorizeAndExecute honors tool_policy (no bypass) | | `src/state.test.ts` | DaemonState: provider listing, model listing, chat flow | | `src/daemon/chat-prompt.test.ts` | `parseApprovalInput` case-sensitive routing | | `src/daemon/web/openai-compat.test.ts` | OpenAI format mapping: models, finish reasons, stream chunks, complete responses | +| `src/daemon/web/validate-body.test.ts` | HTTP body parse helpers + workspace path allowlist / traversal | | `src/core/session-store.test.ts` | SessionStore: CRUD, appendMessage, updateTitle, updateSummary, index consistency | | `src/core/session-summarizer.test.ts` | generateSessionSummary, maybeSummarize interval logic, error handling | @@ -86,7 +88,7 @@ Tests that start real servers, make real HTTP/gRPC calls. | `tests/integration/grpc-streaming.test.ts` | gRPC unary RPCs + streaming + cancellation + concurrency | | `tests/integration/grpc-tls.test.ts` | gRPC TLS bind policy + SetSecret/GetSecret over TLS | | `tests/integration/grpc-bind-e2e.test.ts` | Subprocess E2E: localhost OK, 0.0.0.0 refuse, TLS/insecure OK | -| `tests/integration/web-sse.test.ts` | Web API endpoints + SSE chat streaming + errors + disconnect | +| `tests/integration/web-sse.test.ts` | Web API endpoints + SSE chat + config Zod validation / workspace allowlist | | `tests/integration/openai-compat.test.ts` | OpenAI-compatible API: /v1/models, streaming, non-streaming, errors, tool calls | | `tests/integration/sessions.test.ts` | Session REST API: CRUD endpoints, session chat SSE streaming + persistence | | `tests/integration/mcp-http-policy.test.ts` | `/mcp` auth + connection consent + tool_policy E2E | diff --git a/docs/decisions.md b/docs/decisions.md index 6f335f4..f53316f 100644 --- a/docs/decisions.md +++ b/docs/decisions.md @@ -556,3 +556,24 @@ query-secret patterns (`?key=`, `query.apiKey` / `query['apiKey']`, **Rationale:** Query-string secrets leak into access logs, reverse proxies, browser history, and `Referer` headers. Header/body transport matches other engines (Bearer / `x-api-key`) and closes findings H2/H3. + +--- + +## DR-036: Zod validation for all HTTP API request bodies + +**Date:** 2026-07-17 +**Decision:** Validate every mutating HTTP web API request body with a Zod +schema before business logic runs. Invalid bodies return HTTP 400. Config +writes (`POST /api/config`, provider configure/delete with workspace target) +additionally require `location` / `workspacePath` to be `'user'` or an +allowlisted connected workspace path; path segments containing `..` are +rejected (400) and non-allowlisted paths return 403 with no file write. +Shared `ConfigFile` / `PolicyConfig` schemas live in `@abbenay/core` +(`config-schema.ts`) and are reused by the web layer (`api-schemas.ts`). +Schemas use `.strict()` so unexpected fields are rejected (field-injection +defense). OpenAI `/v1/chat/completions` allows optional `tools` so DR-032 +passthrough continues to work under validation. +**Rationale:** Unvalidated `req.body` destructuring allowed arbitrary config +writes, path traversal into workspace config paths, type confusion, and +field injection (findings H4/H5). Auth (DR-030) authenticates the caller but +does not constrain payload shape or write targets. diff --git a/packages/daemon/src/core/config-schema.test.ts b/packages/daemon/src/core/config-schema.test.ts new file mode 100644 index 0000000..70c8019 --- /dev/null +++ b/packages/daemon/src/core/config-schema.test.ts @@ -0,0 +1,89 @@ +/** + * Unit tests for ConfigFile / PolicyConfig Zod schemas. + */ + +import { describe, it, expect } from 'vitest'; +import { + ConfigFileSchema, + PolicyConfigSchema, + parseConfigFile, + VirtualNameSchema, +} from './config-schema.js'; + +describe('VirtualNameSchema', () => { + it('accepts valid virtual names', () => { + expect(VirtualNameSchema.safeParse('openai').success).toBe(true); + expect(VirtualNameSchema.safeParse('my-provider_v2').success).toBe(true); + }); + + it('rejects uppercase, spaces, and slashes', () => { + expect(VirtualNameSchema.safeParse('OpenAI').success).toBe(false); + expect(VirtualNameSchema.safeParse('my provider').success).toBe(false); + expect(VirtualNameSchema.safeParse('a/b').success).toBe(false); + }); +}); + +describe('ConfigFileSchema', () => { + it('accepts a minimal valid config', () => { + const result = parseConfigFile({ providers: {} }); + expect(result.success).toBe(true); + }); + + it('accepts a full provider + models shape', () => { + const result = parseConfigFile({ + providers: { + openai: { + engine: 'openai', + models: { + 'gpt-4o': { temperature: 0.2, max_tokens: 1024 }, + }, + }, + }, + tool_policy: { auto_approve: ['mcp:safe/*'] }, + }); + expect(result.success).toBe(true); + }); + + it('rejects unknown top-level keys (field injection)', () => { + const withEvil = ConfigFileSchema.safeParse({ providers: {}, evil: true }); + expect(withEvil.success).toBe(false); + }); + + it('rejects wrong types for required provider fields', () => { + const result = parseConfigFile({ + providers: { + openai: { engine: 42 }, + }, + }); + expect(result.success).toBe(false); + }); + + it('rejects out-of-range temperature', () => { + const result = parseConfigFile({ + providers: { + openai: { + engine: 'openai', + models: { 'gpt-4o': { temperature: 9 } }, + }, + }, + }); + expect(result.success).toBe(false); + }); +}); + +describe('PolicyConfigSchema', () => { + it('accepts nested policy fields', () => { + const result = PolicyConfigSchema.safeParse({ + sampling: { temperature: 0.3 }, + output: { format: 'json_only', max_tokens: 512 }, + }); + expect(result.success).toBe(true); + }); + + it('rejects invalid format enum', () => { + const result = PolicyConfigSchema.safeParse({ + output: { format: 'xml' }, + }); + expect(result.success).toBe(false); + }); +}); diff --git a/packages/daemon/src/core/config-schema.ts b/packages/daemon/src/core/config-schema.ts new file mode 100644 index 0000000..bacc9c9 --- /dev/null +++ b/packages/daemon/src/core/config-schema.ts @@ -0,0 +1,167 @@ +/** + * Zod schemas for Abbenay configuration shapes. + * + * Shared by the HTTP API (request validation) and any core callers that need + * typed config parsing. Keep in sync with the interfaces in config.ts / + * policies.ts / tool-registry.ts. + */ + +import { z } from 'zod'; +import { isValidVirtualName } from './config.js'; + +/** Virtual provider/model/policy name (lowercase alphanumeric + ._-). */ +export const VirtualNameSchema = z + .string() + .min(1) + .refine(isValidVirtualName, { + message: 'must be lowercase alphanumeric with dots, hyphens, or underscores', + }); + +export const ModelConfigSchema = z + .object({ + model_id: z.string().min(1).optional(), + policy: z.string().min(1).optional(), + system_prompt: z.string().optional(), + system_prompt_mode: z.enum(['prepend', 'replace']).optional(), + temperature: z.number().min(0).max(2).optional(), + top_p: z.number().min(0).max(1).optional(), + top_k: z.number().int().nonnegative().optional(), + max_tokens: z.number().int().positive().optional(), + timeout: z.number().positive().optional(), + }) + .strict(); + +export const ProviderConfigSchema = z + .object({ + engine: z.string().min(1), + api_key_keychain_name: z.string().min(1).optional(), + api_key_env_var_name: z.string().min(1).optional(), + base_url: z.string().min(1).optional(), + models: z.record(z.string(), ModelConfigSchema).optional(), + }) + .strict(); + +export const McpServerConfigSchema = z + .object({ + command: z.string().optional(), + args: z.array(z.string()).optional(), + url: z.string().optional(), + transport: z.enum(['stdio', 'http', 'sse']), + enabled: z.boolean(), + headers: z.record(z.string(), z.string()).optional(), + env: z.record(z.string(), z.string()).optional(), + max_response_size: z.number().int().positive().optional(), + }) + .strict(); + +export const ToolPolicyConfigSchema = z + .object({ + max_tool_iterations: z.number().int().positive().optional(), + auto_approve: z.array(z.string()).optional(), + require_approval: z.array(z.string()).optional(), + disabled_tools: z.array(z.string()).optional(), + aliases: z.record(z.string(), z.string()).optional(), + }) + .strict(); + +export const ConsumerCapabilitiesSchema = z + .object({ + inline_policy: z.boolean().optional(), + mcp_register: z.boolean().optional(), + }) + .strict(); + +export const ConsumerConfigSchema = z + .object({ + token_env: z.string().min(1).optional(), + token_keychain: z.string().min(1).optional(), + capabilities: ConsumerCapabilitiesSchema, + }) + .strict(); + +export const ServerConfigSchema = z + .object({ + api_token: z.string().min(1).optional(), + api_token_env: z.string().min(1).optional(), + host: z.string().min(1).optional(), + cors_origins: z.array(z.string().min(1)).optional(), + }) + .strict(); + +/** + * Full config.yaml shape. Unknown top-level keys are rejected (`.strict()`) + * to block field injection into saved config files. + */ +export const ConfigFileSchema = z + .object({ + providers: z.record(z.string(), ProviderConfigSchema).optional(), + mcp_servers: z.record(z.string(), McpServerConfigSchema).optional(), + tool_policy: ToolPolicyConfigSchema.optional(), + consumers: z.record(z.string(), ConsumerConfigSchema).optional(), + server: ServerConfigSchema.optional(), + }) + .strict(); + +export type ConfigFileParsed = z.infer; + +// ── Policy config (policies.yaml entries) ─────────────────────────────── + +export const PolicyConfigSchema = z + .object({ + sampling: z + .object({ + temperature: z.number().min(0).max(2).optional(), + top_p: z.number().min(0).max(1).optional(), + top_k: z.number().int().nonnegative().optional(), + }) + .strict() + .optional(), + output: z + .object({ + max_tokens: z.number().int().positive().optional(), + reserved_output_tokens: z.number().int().nonnegative().optional(), + format: z.enum(['text', 'json_only', 'markdown']).optional(), + system_prompt_snippet: z.string().optional(), + system_prompt_mode: z.enum(['prepend', 'append', 'replace']).optional(), + }) + .strict() + .optional(), + context: z + .object({ + context_threshold: z.number().optional(), + compression_strategy: z.enum(['none', 'truncate', 'rolling_summary']).optional(), + }) + .strict() + .optional(), + tool: z + .object({ + max_tool_iterations: z.number().int().positive().optional(), + tool_mode: z.enum(['auto', 'ask', 'none']).optional(), + }) + .strict() + .optional(), + reliability: z + .object({ + retry_on_invalid_json: z.boolean().optional(), + timeout: z.number().positive().optional(), + }) + .strict() + .optional(), + }) + .strict(); + +export type PolicyConfigParsed = z.infer; + +/** + * Parse and validate a ConfigFile-shaped value. + * Returns a structured result (never throws). + */ +export function parseConfigFile(raw: unknown): + | { success: true; data: ConfigFileParsed } + | { success: false; error: z.ZodError } { + const result = ConfigFileSchema.safeParse(raw); + if (!result.success) { + return { success: false, error: result.error }; + } + return { success: true, data: result.data }; +} diff --git a/packages/daemon/src/core/index.ts b/packages/daemon/src/core/index.ts index 7b4574f..5c22c35 100644 --- a/packages/daemon/src/core/index.ts +++ b/packages/daemon/src/core/index.ts @@ -152,6 +152,16 @@ export { isValidVirtualName, } from './config.js'; +/** Zod schemas for ConfigFile / PolicyConfig validation */ +export { + ConfigFileSchema, + PolicyConfigSchema, + ModelConfigSchema, + ProviderConfigSchema, + VirtualNameSchema, + parseConfigFile, +} from './config-schema.js'; + /** Policy management */ export { BUILTIN_POLICIES, diff --git a/packages/daemon/src/daemon/web/api-schemas.ts b/packages/daemon/src/daemon/web/api-schemas.ts new file mode 100644 index 0000000..d1ea29c --- /dev/null +++ b/packages/daemon/src/daemon/web/api-schemas.ts @@ -0,0 +1,211 @@ +/** + * Zod request-body schemas for every mutating HTTP web API route. + * + * Config shapes are shared with `@abbenay/core` via config-schema.ts. + * Route schemas use `.strict()` so unexpected fields are rejected before + * business logic runs. + */ + +import { z } from 'zod'; +import { + ConfigFileSchema, + PolicyConfigSchema, + VirtualNameSchema, +} from '../../core/config-schema.js'; + +/** Empty / ignored body (routes that do not consume JSON fields). */ +export const EmptyBodySchema = z.preprocess( + (v) => (v === undefined || v === null ? {} : v), + z.object({}).strict(), +); + +// ── Auth ──────────────────────────────────────────────────────────────── + +export const LoginBodySchema = z + .object({ + token: z.string().optional(), + api_token: z.string().optional(), + }) + .strict(); + +// ── Config ────────────────────────────────────────────────────────────── + +export const PostConfigBodySchema = z + .object({ + /** Defaults to user-level config when omitted. */ + location: z.string().min(1).optional(), + config: ConfigFileSchema, + }) + .strict(); + +// ── Secrets ───────────────────────────────────────────────────────────── + +export const PostSecretByKeyBodySchema = z + .object({ + value: z.string().min(1), + }) + .strict(); + +export const PostSecretBodySchema = z + .object({ + key: z.string().min(1), + value: z.string().min(1), + }) + .strict(); + +// ── Chat ──────────────────────────────────────────────────────────────── + +const ChatMessageSchema = z + .object({ + role: z.string().min(1), + content: z.union([z.string(), z.null()]).optional(), + name: z.string().optional(), + tool_call_id: z.string().optional(), + tool_calls: z.array(z.unknown()).optional(), + }) + .strict(); + +const ChatToolDefSchema = z + .object({ + name: z.string().optional(), + description: z.string().optional(), + input_schema: z.union([z.string(), z.record(z.unknown())]).optional(), + }) + .strict(); + +export const PostChatBodySchema = z + .object({ + model: z.string().min(1), + messages: z.array(ChatMessageSchema).min(1), + temperature: z.number().optional(), + top_p: z.number().optional(), + top_k: z.number().optional(), + max_tokens: z.number().optional(), + timeout: z.number().optional(), + tools: z.array(ChatToolDefSchema).optional(), + tool_mode: z.enum(['auto', 'ask', 'none', 'passthrough']).optional(), + }) + .strict(); + +export const PostChatApproveBodySchema = z + .object({ + requestId: z.string().min(1), + decision: z.enum(['allow', 'deny', 'abort']), + }) + .strict(); + +// ── Provider ──────────────────────────────────────────────────────────── + +export const PostProviderConfigureBodySchema = z + .object({ + engine: z.string().min(1).optional(), + apiKey: z.string().min(1).optional(), + envVarName: z.string().min(1).optional(), + baseUrl: z.string().min(1).optional(), + target: z.enum(['user', 'workspace']).optional(), + workspacePath: z.string().min(1).optional(), + }) + .strict() + .superRefine((data, ctx) => { + if (data.target === 'workspace' && !data.workspacePath) { + ctx.addIssue({ + code: z.ZodIssueCode.custom, + message: 'workspacePath is required when target is "workspace"', + path: ['workspacePath'], + }); + } + }); + +// ── Policies ──────────────────────────────────────────────────────────── + +export const PostPolicyBodySchema = z + .object({ + name: VirtualNameSchema, + config: PolicyConfigSchema, + }) + .strict(); + +// ── Sessions ──────────────────────────────────────────────────────────── + +export const PostSessionBodySchema = z + .object({ + model: z.string().min(1), + title: z.string().optional(), + policy: z.string().optional(), + metadata: z.record(z.string(), z.string()).optional(), + }) + .strict(); + +export const PostSessionChatBodySchema = z + .object({ + message: z + .object({ + role: z.string().min(1).optional(), + content: z.string().min(1), + }) + .strict(), + }) + .strict(); + +// ── Discover models (DR-035: keys via header/body, never query) ────────── + +export const DiscoverModelsBodySchema = z + .object({ + apiKey: z.string().min(1).optional(), + baseUrl: z.string().min(1).optional(), + providerId: z.string().min(1).optional(), + }) + .strict(); + +// ── MCP connection consent + tool approval (DR-033 / DR-034) ──────────── + +export const PostMcpConnectionDecisionBodySchema = z + .object({ + decision: z.enum(['allow', 'deny']), + remember: z.boolean().optional(), + }) + .strict(); + +export const PostMcpApprovalBodySchema = z + .object({ + decision: z.enum(['allow', 'deny', 'abort']), + }) + .strict(); + +// ── OpenAI-compatible ─────────────────────────────────────────────────── + +const OpenAIChatMessageSchema = z + .object({ + role: z.string().min(1), + content: z.union([z.string(), z.null(), z.array(z.unknown())]).optional(), + name: z.string().optional(), + tool_call_id: z.string().optional(), + tool_calls: z.unknown().optional(), + }) + .passthrough(); + +export const PostOpenAIChatCompletionsBodySchema = z + .object({ + model: z.string().min(1), + messages: z.array(OpenAIChatMessageSchema).min(1), + stream: z.boolean().optional(), + temperature: z.number().optional(), + top_p: z.number().optional(), + max_tokens: z.number().optional(), + max_completion_tokens: z.number().optional(), + // DR-032: optional client tools for opt-in passthrough (validated/mapped in openai-compat). + tools: z.array(z.unknown()).optional(), + }) + .strict(); + +export type PostConfigBody = z.infer; +export type PostChatBody = z.infer; +export type PostChatApproveBody = z.infer; +export type PostProviderConfigureBody = z.infer; +export type PostPolicyBody = z.infer; +export type PostSessionBody = z.infer; +export type PostSessionChatBody = z.infer; +export type DiscoverModelsBody = z.infer; +export type PostMcpConnectionDecisionBody = z.infer; +export type PostMcpApprovalBody = z.infer; +export type PostOpenAIChatCompletionsBody = z.infer; diff --git a/packages/daemon/src/daemon/web/openai-compat.ts b/packages/daemon/src/daemon/web/openai-compat.ts index 89daab2..da7f4d7 100644 --- a/packages/daemon/src/daemon/web/openai-compat.ts +++ b/packages/daemon/src/daemon/web/openai-compat.ts @@ -20,6 +20,8 @@ import { type ConfigFile, type OpenAICompatToolsMode, } from '../../core/config.js'; +import { PostOpenAIChatCompletionsBodySchema } from './api-schemas.js'; +import { parseRequestBody } from './validate-body.js'; // ── Format helpers (exported for unit testing) ────────────────────────── @@ -323,6 +325,12 @@ export function registerOpenAIRoutes(app: Express, state: DaemonState): void { * POST /v1/chat/completions — chat with streaming or non-streaming response. */ app.post('/v1/chat/completions', (req: Request, res: Response) => { + const parsed = parseRequestBody(PostOpenAIChatCompletionsBodySchema, req.body); + if (!parsed.success) { + openAIError(res, 400, parsed.error, 'invalid_request_error'); + return; + } + const { model, messages, @@ -332,18 +340,9 @@ export function registerOpenAIRoutes(app: Express, state: DaemonState): void { max_tokens, max_completion_tokens, tools, - } = req.body; - - if (!model) { - openAIError(res, 400, 'model is required', 'invalid_request_error'); - return; - } - if (!messages || !Array.isArray(messages) || messages.length === 0) { - openAIError(res, 400, 'messages is required and must be a non-empty array', 'invalid_request_error'); - return; - } + } = parsed.data; - const chatMessages = messages.map((m: unknown) => normalizeOpenAIChatMessage(m)); + const chatMessages = messages.map((m) => normalizeOpenAIChatMessage(m)); const requestParams: Record = {}; if (temperature != null) requestParams.temperature = temperature; diff --git a/packages/daemon/src/daemon/web/server.ts b/packages/daemon/src/daemon/web/server.ts index 46a7a12..ef159e1 100644 --- a/packages/daemon/src/daemon/web/server.ts +++ b/packages/daemon/src/daemon/web/server.ts @@ -16,7 +16,7 @@ import * as path from 'node:path'; import * as fs from 'node:fs'; import { fileURLToPath } from 'node:url'; import { getDefaultSocketPath } from '../transport.js'; -import { loadConfig, saveConfig, loadWorkspaceConfig, saveWorkspaceConfig, getUserConfigPath, getWorkspaceConfigPath, isValidVirtualName, type ConfigFile, type ProviderConfig } from '../../core/config.js'; +import { loadConfig, saveConfig, loadWorkspaceConfig, saveWorkspaceConfig, getUserConfigPath, getWorkspaceConfigPath, type ConfigFile, type ProviderConfig } from '../../core/config.js'; import { listAllPolicies, loadCustomPolicies, saveCustomPolicies, BUILTIN_POLICY_NAMES, type PolicyConfig } from '../../core/policies.js'; import { maybeSummarize, generateSessionSummary } from '../../core/session-summarizer.js'; import type { DaemonState } from '../state.js'; @@ -41,6 +41,29 @@ import { type RequestWithOwner, } from './http-security.js'; import { LOCAL_SESSION_OWNER } from '../../core/session-store.js'; +import { + DiscoverModelsBodySchema, + EmptyBodySchema, + LoginBodySchema, + PostChatApproveBodySchema, + PostChatBodySchema, + PostConfigBodySchema, + PostMcpApprovalBodySchema, + PostMcpConnectionDecisionBodySchema, + PostPolicyBodySchema, + PostProviderConfigureBodySchema, + PostSecretBodySchema, + PostSecretByKeyBodySchema, + PostSessionBodySchema, + PostSessionChatBodySchema, +} from './api-schemas.js'; +import { + parseRequestBody, + resolveConfigLocation, + checkWorkspaceLocation, + collectAllowlistedWorkspaces, + sendBadRequest, +} from './validate-body.js'; export type { WebSecurityOptions, ResolvedHttpSecurity } from './http-security.js'; @@ -185,11 +208,14 @@ export function createWebApp(state: DaemonState, options?: WebSecurityOptions): res.type('html').send(html); }; - const extractLoginToken = (req: Request): string => { - const body = req.body as { token?: unknown; api_token?: unknown } | undefined; - if (typeof body?.token === 'string') return body.token; - if (typeof body?.api_token === 'string') return body.api_token; - return ''; + const extractLoginToken = (req: Request): { ok: true; token: string } | { ok: false; error: string } => { + const parsed = parseRequestBody(LoginBodySchema, req.body); + if (!parsed.success) { + return { ok: false, error: parsed.error }; + } + if (typeof parsed.data.token === 'string') return { ok: true, token: parsed.data.token }; + if (typeof parsed.data.api_token === 'string') return { ok: true, token: parsed.data.api_token }; + return { ok: true, token: '' }; }; app.get('/login', (req, res) => { @@ -208,9 +234,19 @@ export function createWebApp(state: DaemonState, options?: WebSecurityOptions): res.redirect(302, '/'); return; } - const token = extractLoginToken(req); - if (!timingSafeEqualString(token, security.apiToken)) { - const wantsJson = req.is('application/json') || (req.headers.accept || '').includes('application/json'); + const wantsJson = req.is('application/json') || (req.headers.accept || '').includes('application/json'); + const extracted = extractLoginToken(req); + if (!extracted.ok) { + if (wantsJson) { + sendBadRequest(res, extracted.error); + return; + } + res.status(400).type('html').send( + LOGIN_PAGE.replace('{{ERROR}}', `

${extracted.error}

`), + ); + return; + } + if (!timingSafeEqualString(extracted.token, security.apiToken)) { if (wantsJson) { res.status(401).json({ error: 'Invalid token' }); return; @@ -221,7 +257,6 @@ export function createWebApp(state: DaemonState, options?: WebSecurityOptions): return; } setAuthCookies(res, security.apiToken, cookieOpts(req)); - const wantsJson = req.is('application/json') || (req.headers.accept || '').includes('application/json'); if (wantsJson) { res.status(204).end(); return; @@ -356,12 +391,13 @@ export function createWebApp(state: DaemonState, options?: WebSecurityOptions): * Body: { decision: 'allow' | 'deny', remember?: boolean } */ app.post('/api/mcp/connections/:requestId', (req, res) => { - const { requestId } = req.params; - const { decision, remember } = req.body as { decision?: string; remember?: boolean }; - if (!decision || !['allow', 'deny'].includes(decision)) { - res.status(400).json({ error: 'decision must be allow or deny' }); + const parsed = parseRequestBody(PostMcpConnectionDecisionBodySchema, req.body); + if (!parsed.success) { + sendBadRequest(res, parsed.error); return; } + const { requestId } = req.params; + const { decision, remember } = parsed.data; const pending = pendingMcpConnections.get(requestId); if (!pending) { res.status(404).json({ error: `No pending MCP connection with requestId "${requestId}"` }); @@ -375,7 +411,7 @@ export function createWebApp(state: DaemonState, options?: WebSecurityOptions): if (decision === 'allow' && remember && typeof state.mcpServer?.rememberClient === 'function') { state.mcpServer.rememberClient(pending.clientName); } - pending.resolve(decision as 'allow' | 'deny'); + pending.resolve(decision); pendingMcpConnections.delete(requestId); res.json({ success: true }); }); @@ -445,19 +481,20 @@ export function createWebApp(state: DaemonState, options?: WebSecurityOptions): * Body: { decision: 'allow' | 'deny' | 'abort' } */ app.post('/api/mcp/approvals/:requestId', (req, res) => { - const { requestId } = req.params; - const { decision } = req.body as { decision?: string }; - if (!decision || !['allow', 'deny', 'abort'].includes(decision)) { - res.status(400).json({ error: 'decision must be allow, deny, or abort' }); + const parsed = parseRequestBody(PostMcpApprovalBodySchema, req.body); + if (!parsed.success) { + sendBadRequest(res, parsed.error); return; } + const { requestId } = req.params; + const { decision } = parsed.data; const pending = pendingMcpApprovals.get(requestId); if (!pending) { res.status(404).json({ error: `No pending MCP approval with requestId "${requestId}"` }); return; } console.log(`[Web] MCP tool approval: ${pending.namespacedName || pending.toolName} → ${decision} (requestId=${requestId})`); - pending.resolve(decision as 'allow' | 'deny' | 'abort'); + pending.resolve(decision); pendingMcpApprovals.delete(requestId); res.json({ success: true }); }); @@ -544,27 +581,28 @@ export function createWebApp(state: DaemonState, options?: WebSecurityOptions): return; } - const body = (req.body && typeof req.body === 'object' ? req.body : {}) as { - apiKey?: unknown; - baseUrl?: unknown; - providerId?: unknown; - }; + const parsed = parseRequestBody(DiscoverModelsBodySchema, req.body); + if (!parsed.success) { + sendBadRequest(res, parsed.error); + return; + } + const body = parsed.data; const headerKey = req.headers['x-api-key']; const headerKeyStr = Array.isArray(headerKey) ? headerKey[0] : headerKey; let apiKey: string | undefined; if (typeof headerKeyStr === 'string' && headerKeyStr.length > 0) { apiKey = headerKeyStr; - } else if (typeof body.apiKey === 'string' && body.apiKey.length > 0) { + } else if (body.apiKey) { apiKey = body.apiKey; } let baseUrl = - (typeof body.baseUrl === 'string' && body.baseUrl) || + body.baseUrl || (typeof req.query.baseUrl === 'string' ? req.query.baseUrl : undefined) || undefined; const providerId = - (typeof body.providerId === 'string' && body.providerId) || + body.providerId || (typeof req.query.providerId === 'string' ? req.query.providerId : undefined) || undefined; @@ -631,22 +669,30 @@ export function createWebApp(state: DaemonState, options?: WebSecurityOptions): /** * GET /api/config?location=user| - Get configuration * Returns { config: ConfigFile, path: string } + * Workspace locations must be allowlisted (same rules as POST). */ app.get('/api/config', (req, res) => { try { - const location = (req.query.location as string) || 'user'; + const location = typeof req.query.location === 'string' && req.query.location + ? req.query.location + : 'user'; + const loc = resolveConfigLocation(location, state); + if (!loc.ok) { + res.status(loc.status).json({ error: loc.error }); + return; + } + let config: ConfigFile; let configPath: string; - - if (location === 'user') { + + if (loc.kind === 'user') { config = loadConfig() || { providers: {} }; configPath = getUserConfigPath(); } else { - // location is a workspace path - config = loadWorkspaceConfig(location) || { providers: {} }; - configPath = getWorkspaceConfigPath(location); + config = loadWorkspaceConfig(loc.resolved) || { providers: {} }; + configPath = getWorkspaceConfigPath(loc.resolved); } - + res.json({ config, path: configPath }); } catch (err: unknown) { const msg = err instanceof Error ? err.message : String(err); @@ -654,25 +700,37 @@ export function createWebApp(state: DaemonState, options?: WebSecurityOptions): res.status(500).json({ error: msg }); } }); - + /** * POST /api/config - Save configuration * Accepts { location: 'user' | workspacePath, config: ConfigFile } + * Body is Zod-validated; workspace locations must be allowlisted. */ app.post('/api/config', (req, res) => { try { - const { location, config } = req.body; - const loc = location || 'user'; + const parsed = parseRequestBody(PostConfigBodySchema, req.body); + if (!parsed.success) { + sendBadRequest(res, parsed.error); + return; + } + + const { config } = parsed.data; + const location = parsed.data.location ?? 'user'; + const loc = resolveConfigLocation(location, state); + if (!loc.ok) { + res.status(loc.status).json({ error: loc.error }); + return; + } + let savedPath: string; - - if (loc === 'user') { + if (loc.kind === 'user') { saveConfig(config); savedPath = getUserConfigPath(); } else { - saveWorkspaceConfig(loc, config); - savedPath = getWorkspaceConfigPath(loc); + saveWorkspaceConfig(loc.resolved, config); + savedPath = getWorkspaceConfigPath(loc.resolved); } - + res.json({ success: true, path: savedPath }); state.notifyModelsChanged('config_changed'); state.refreshMcpConnections().catch((err: unknown) => { @@ -782,12 +840,12 @@ export function createWebApp(state: DaemonState, options?: WebSecurityOptions): app.post('/api/secrets/:key', async (req, res) => { try { const key = req.params.key; - const { value } = req.body; - if (!value) { - res.status(400).json({ error: 'value required' }); + const parsed = parseRequestBody(PostSecretByKeyBodySchema, req.body); + if (!parsed.success) { + sendBadRequest(res, parsed.error); return; } - await state.secretStore.set(key, value); + await state.secretStore.set(key, parsed.data.value); res.json({ success: true }); state.notifyModelsChanged('secret_updated'); } catch (err: unknown) { @@ -796,19 +854,19 @@ export function createWebApp(state: DaemonState, options?: WebSecurityOptions): res.status(500).json({ error: msg }); } }); - + /** * POST /api/secrets - Set a secret (API key) — legacy route * Body: { key: string, value: string } */ app.post('/api/secrets', async (req, res) => { try { - const { key, value } = req.body; - if (!key || !value) { - res.status(400).json({ error: 'key and value required' }); + const parsed = parseRequestBody(PostSecretBodySchema, req.body); + if (!parsed.success) { + sendBadRequest(res, parsed.error); return; } - await state.secretStore.set(key, value); + await state.secretStore.set(parsed.data.key, parsed.data.value); res.json({ success: true }); state.notifyModelsChanged('secret_updated'); } catch (err: unknown) { @@ -817,12 +875,17 @@ export function createWebApp(state: DaemonState, options?: WebSecurityOptions): res.status(500).json({ error: msg }); } }); - + /** * DELETE /api/secrets/:key - Delete a secret */ app.delete('/api/secrets/:key', async (req, res) => { try { + const parsed = parseRequestBody(EmptyBodySchema, req.body); + if (!parsed.success) { + sendBadRequest(res, parsed.error); + return; + } await state.secretStore.delete(req.params.key); res.json({ success: true }); state.notifyModelsChanged('secret_deleted'); @@ -876,15 +939,12 @@ export function createWebApp(state: DaemonState, options?: WebSecurityOptions): * Body: { requestId: string, decision: 'allow' | 'deny' | 'abort' } */ app.post('/api/chat/:chatId/approve', (req, res) => { - const { requestId, decision } = req.body; - if (!requestId || !decision) { - res.status(400).json({ error: 'requestId and decision required' }); - return; - } - if (!['allow', 'deny', 'abort'].includes(decision)) { - res.status(400).json({ error: 'decision must be allow, deny, or abort' }); + const parsed = parseRequestBody(PostChatApproveBodySchema, req.body); + if (!parsed.success) { + sendBadRequest(res, parsed.error); return; } + const { requestId, decision } = parsed.data; const pending = pendingApprovals.get(requestId); if (!pending) { @@ -910,13 +970,24 @@ export function createWebApp(state: DaemonState, options?: WebSecurityOptions): * emitted and the stream pauses until POST /api/chat/:chatId/approve resolves it. */ app.post('/api/chat', (req, res) => { - const { model, messages, temperature, top_p, top_k, max_tokens, timeout: reqTimeout, tools, tool_mode } = req.body; - - if (!model || !messages) { - res.status(400).json({ error: 'model and messages required' }); + const parsed = parseRequestBody(PostChatBodySchema, req.body); + if (!parsed.success) { + sendBadRequest(res, parsed.error); return; } + const { + model, + messages, + temperature, + top_p, + top_k, + max_tokens, + timeout: reqTimeout, + tools, + tool_mode, + } = parsed.data; + const chatId = crypto.randomUUID(); // Set up SSE @@ -943,7 +1014,7 @@ export function createWebApp(state: DaemonState, options?: WebSecurityOptions): }; // Convert web messages to the format state.chat() expects - const chatMessages = messages.map((m: { role?: string; content?: string; name?: string; tool_call_id?: string; tool_calls?: unknown[] }) => ({ + const chatMessages = messages.map((m) => ({ role: m.role || 'user', content: m.content ?? '', name: m.name || undefined, @@ -961,11 +1032,15 @@ export function createWebApp(state: DaemonState, options?: WebSecurityOptions): const hasParams = Object.keys(requestParams).length > 0; // Build tool options - const toolDefs = Array.isArray(tools) ? tools.map((t: { name?: string; description?: string; input_schema?: string | Record }) => ({ - name: t.name || '', - description: t.description || '', - inputSchema: typeof t.input_schema === 'string' ? t.input_schema : JSON.stringify(t.input_schema || {}), - })).filter((t) => t.name) : undefined; + const toolDefs = tools + ? tools.map((t) => ({ + name: t.name || '', + description: t.description || '', + inputSchema: typeof t.input_schema === 'string' + ? t.input_schema + : JSON.stringify(t.input_schema || {}), + })).filter((t) => t.name) + : undefined; const toolOptions: ChatToolOptions = { toolMode: tool_mode || 'auto', @@ -1044,20 +1119,35 @@ export function createWebApp(state: DaemonState, options?: WebSecurityOptions): app.post('/api/provider/:id/configure', async (req, res) => { try { const providerId = req.params.id; - const { engine, apiKey, envVarName, baseUrl, target, workspacePath } = req.body; - - const config: ConfigFile = (target === 'workspace' && workspacePath - ? loadWorkspaceConfig(workspacePath) + const parsed = parseRequestBody(PostProviderConfigureBodySchema, req.body); + if (!parsed.success) { + sendBadRequest(res, parsed.error); + return; + } + const { engine, apiKey, envVarName, baseUrl, target, workspacePath } = parsed.data; + + let resolvedWorkspace: string | undefined; + if (target === 'workspace' && workspacePath) { + const check = checkWorkspaceLocation(workspacePath, collectAllowlistedWorkspaces(state)); + if (!check.ok) { + res.status(check.status).json({ error: check.error }); + return; + } + resolvedWorkspace = check.resolved; + } + + const config: ConfigFile = (resolvedWorkspace + ? loadWorkspaceConfig(resolvedWorkspace) : loadConfig()) || { providers: {} }; - + if (!config.providers) config.providers = {}; - + // Initialize or update provider config const existing: Partial & { display_name?: string } = config.providers[providerId] ? { ...config.providers[providerId] } : {}; if (engine) existing.engine = engine; delete existing.display_name; config.providers[providerId] = existing as ProviderConfig; - + if (apiKey) { const keychainName = `${providerId.toUpperCase()}_API_KEY`; await state.secretStore.set(keychainName, apiKey); @@ -1067,17 +1157,17 @@ export function createWebApp(state: DaemonState, options?: WebSecurityOptions): config.providers[providerId].api_key_env_var_name = envVarName; delete config.providers[providerId].api_key_keychain_name; } - + if (baseUrl) { config.providers[providerId].base_url = baseUrl; } - - if (target === 'workspace' && workspacePath) { - saveWorkspaceConfig(workspacePath, config); + + if (resolvedWorkspace) { + saveWorkspaceConfig(resolvedWorkspace, config); } else { saveConfig(config); } - + res.json({ success: true }); state.notifyModelsChanged('provider_configured'); } catch (err: unknown) { @@ -1086,35 +1176,55 @@ export function createWebApp(state: DaemonState, options?: WebSecurityOptions): res.status(500).json({ error: msg }); } }); - + /** * DELETE /api/provider/:id - Remove a provider configuration */ app.delete('/api/provider/:id', async (req, res) => { try { + const parsed = parseRequestBody(EmptyBodySchema, req.body); + if (!parsed.success) { + sendBadRequest(res, parsed.error); + return; + } + const providerId = req.params.id; - const target = req.query.target as string || 'user'; - const workspacePath = req.query.workspacePath as string; - - const config: ConfigFile = (target === 'workspace' && workspacePath - ? loadWorkspaceConfig(workspacePath) + const target = typeof req.query.target === 'string' ? req.query.target : 'user'; + const workspacePath = typeof req.query.workspacePath === 'string' ? req.query.workspacePath : undefined; + + let resolvedWorkspace: string | undefined; + if (target === 'workspace') { + if (!workspacePath) { + sendBadRequest(res, 'workspacePath query param is required when target is "workspace"'); + return; + } + const check = checkWorkspaceLocation(workspacePath, collectAllowlistedWorkspaces(state)); + if (!check.ok) { + res.status(check.status).json({ error: check.error }); + return; + } + resolvedWorkspace = check.resolved; + } + + const config: ConfigFile = (resolvedWorkspace + ? loadWorkspaceConfig(resolvedWorkspace) : loadConfig()) || { providers: {} }; - + if (config.providers && config.providers[providerId]) { const keychainName = config.providers[providerId].api_key_keychain_name; if (keychainName) { try { await state.secretStore.delete(keychainName); } catch { /* ignore */ } } - + delete config.providers[providerId]; - - if (target === 'workspace' && workspacePath) { - saveWorkspaceConfig(workspacePath, config); + + if (resolvedWorkspace) { + saveWorkspaceConfig(resolvedWorkspace, config); } else { saveConfig(config); } } - + res.json({ success: true }); state.notifyModelsChanged('provider_removed'); } catch (err: unknown) { @@ -1172,6 +1282,11 @@ export function createWebApp(state: DaemonState, options?: WebSecurityOptions): // POST /api/mcp-servers/:id/reconnect — reconnect a failed MCP server app.post('/api/mcp-servers/:id/reconnect', async (req, res) => { try { + const parsed = parseRequestBody(EmptyBodySchema, req.body); + if (!parsed.success) { + sendBadRequest(res, parsed.error); + return; + } await state.mcpClientPool.reconnect(req.params.id); res.json({ success: true }); } catch (err: unknown) { @@ -1216,26 +1331,19 @@ export function createWebApp(state: DaemonState, options?: WebSecurityOptions): // POST /api/policies — upsert a custom policy app.post('/api/policies', (req, res) => { try { - const { name, config } = req.body as { name: string; config: PolicyConfig }; - if (!name || typeof name !== 'string') { - res.status(400).json({ error: 'Policy name is required' }); - return; - } - if (!isValidVirtualName(name)) { - res.status(400).json({ error: 'Policy name must be lowercase alphanumeric with dots, hyphens, or underscores' }); + const parsed = parseRequestBody(PostPolicyBodySchema, req.body); + if (!parsed.success) { + sendBadRequest(res, parsed.error); return; } + const { name, config } = parsed.data; if (BUILTIN_POLICY_NAMES.includes(name)) { res.status(400).json({ error: `Cannot overwrite built-in policy "${name}". Duplicate it with a different name.` }); return; } - if (!config || typeof config !== 'object') { - res.status(400).json({ error: 'Policy config is required' }); - return; - } const custom = loadCustomPolicies(); - custom[name] = config; + custom[name] = config as PolicyConfig; saveCustomPolicies(custom); res.json({ success: true, name }); } catch (err: unknown) { @@ -1247,6 +1355,11 @@ export function createWebApp(state: DaemonState, options?: WebSecurityOptions): // DELETE /api/policies/:name — delete a custom policy app.delete('/api/policies/:name', (req, res) => { try { + const parsed = parseRequestBody(EmptyBodySchema, req.body); + if (!parsed.success) { + sendBadRequest(res, parsed.error); + return; + } const { name } = req.params; if (BUILTIN_POLICY_NAMES.includes(name)) { res.status(400).json({ error: `Cannot delete built-in policy "${name}"` }); @@ -1274,8 +1387,13 @@ export function createWebApp(state: DaemonState, options?: WebSecurityOptions): }); // POST /api/mcp-server/start — start the MCP server on this Express app - app.post('/api/mcp-server/start', async (_req, res) => { + app.post('/api/mcp-server/start', async (req, res) => { try { + const parsed = parseRequestBody(EmptyBodySchema, req.body); + if (!parsed.success) { + sendBadRequest(res, parsed.error); + return; + } if (state.mcpServer.isRunning) { res.json({ success: true, message: 'MCP server already running' }); return; @@ -1289,8 +1407,13 @@ export function createWebApp(state: DaemonState, options?: WebSecurityOptions): }); // POST /api/mcp-server/stop — stop the MCP server - app.post('/api/mcp-server/stop', async (_req, res) => { + app.post('/api/mcp-server/stop', async (req, res) => { try { + const parsed = parseRequestBody(EmptyBodySchema, req.body); + if (!parsed.success) { + sendBadRequest(res, parsed.error); + return; + } await state.mcpServer.stop(); res.json({ success: true }); } catch (err: unknown) { @@ -1306,11 +1429,12 @@ export function createWebApp(state: DaemonState, options?: WebSecurityOptions): app.post('/api/sessions', async (req, res) => { try { - const { model, title, policy, metadata } = req.body; - if (!model) { - res.status(400).json({ error: 'model is required' }); + const parsed = parseRequestBody(PostSessionBodySchema, req.body); + if (!parsed.success) { + sendBadRequest(res, parsed.error); return; } + const { model, title, policy, metadata } = parsed.data; const session = await state.sessionStore.create( model, title, @@ -1384,6 +1508,11 @@ export function createWebApp(state: DaemonState, options?: WebSecurityOptions): app.delete('/api/sessions/:id', async (req, res) => { try { + const parsed = parseRequestBody(EmptyBodySchema, req.body); + if (!parsed.success) { + sendBadRequest(res, parsed.error); + return; + } await state.sessionStore.deleteOwned(req.params.id, sessionOwner(req)); res.json({ success: true }); } catch (err: unknown) { @@ -1426,17 +1555,17 @@ export function createWebApp(state: DaemonState, options?: WebSecurityOptions): app.post('/api/sessions/:id/chat', async (req, res) => { const sessionId = req.params.id; - const { message } = req.body; const owner = sessionOwner(req); - if (!message || !message.content) { - res.status(400).json({ error: 'message with content is required' }); + const parsed = parseRequestBody(PostSessionChatBodySchema, req.body); + if (!parsed.success) { + sendBadRequest(res, parsed.error); return; } const chatMessage = { - role: (message.role as string) || 'user', - content: message.content as string, + role: parsed.data.message.role || 'user', + content: parsed.data.message.content, }; let session; diff --git a/packages/daemon/src/daemon/web/validate-body.test.ts b/packages/daemon/src/daemon/web/validate-body.test.ts new file mode 100644 index 0000000..779f1f5 --- /dev/null +++ b/packages/daemon/src/daemon/web/validate-body.test.ts @@ -0,0 +1,108 @@ +/** + * Unit tests for HTTP body validation and workspace location allowlisting. + */ + +import { describe, it, expect } from 'vitest'; +import * as path from 'node:path'; +import { z } from 'zod'; +import { + containsPathTraversal, + checkWorkspaceLocation, + formatZodError, + parseRequestBody, +} from './validate-body.js'; +import { PostChatBodySchema, PostConfigBodySchema } from './api-schemas.js'; + +describe('containsPathTraversal', () => { + it('detects .. segments on Unix and Windows separators', () => { + expect(containsPathTraversal('../../etc/passwd')).toBe(true); + expect(containsPathTraversal('foo/../bar')).toBe(true); + expect(containsPathTraversal('foo\\..\\bar')).toBe(true); + }); + + it('allows paths that merely contain dots in a segment name', () => { + expect(containsPathTraversal('/home/user/my..project')).toBe(false); + expect(containsPathTraversal('/home/user/project')).toBe(false); + }); +}); + +describe('checkWorkspaceLocation', () => { + const allowed = [path.resolve('/tmp/allowed-ws')]; + + it('accepts an allowlisted absolute path', () => { + const result = checkWorkspaceLocation('/tmp/allowed-ws', allowed); + expect(result.ok).toBe(true); + if (result.ok) { + expect(result.resolved).toBe(path.resolve('/tmp/allowed-ws')); + } + }); + + it('rejects path traversal with 400', () => { + const result = checkWorkspaceLocation('../../etc/passwd', allowed); + expect(result.ok).toBe(false); + if (!result.ok) { + expect(result.status).toBe(400); + expect(result.error).toMatch(/traversal/i); + } + }); + + it('rejects paths outside the allowlist with 403', () => { + const result = checkWorkspaceLocation('/tmp/other-ws', allowed); + expect(result.ok).toBe(false); + if (!result.ok) { + expect(result.status).toBe(403); + expect(result.error).toMatch(/allowlisted/i); + } + }); +}); + +describe('parseRequestBody', () => { + it('parses a valid chat body', () => { + const result = parseRequestBody(PostChatBodySchema, { + model: 'openai/gpt-4o', + messages: [{ role: 'user', content: 'hi' }], + }); + expect(result.success).toBe(true); + }); + + it('rejects chat body with wrong types before business logic', () => { + const result = parseRequestBody(PostChatBodySchema, { + model: 1, + messages: [{ role: 'user', content: 'hi' }], + }); + expect(result.success).toBe(false); + if (!result.success) { + expect(result.error).toContain('model'); + } + }); + + it('rejects config body with invalid schema', () => { + const result = parseRequestBody(PostConfigBodySchema, { + location: 'user', + config: { providers: { x: { engine: false } } }, + }); + expect(result.success).toBe(false); + }); + + it('rejects unexpected top-level fields on config body', () => { + const result = parseRequestBody(PostConfigBodySchema, { + location: 'user', + config: { providers: {} }, + extra: true, + }); + expect(result.success).toBe(false); + }); +}); + +describe('formatZodError', () => { + it('includes path and message', () => { + const schema = z.object({ model: z.string() }); + const parsed = schema.safeParse({ model: 1 }); + expect(parsed.success).toBe(false); + if (!parsed.success) { + const msg = formatZodError(parsed.error); + expect(msg).toContain('model'); + expect(msg).toMatch(/Invalid request body/); + } + }); +}); diff --git a/packages/daemon/src/daemon/web/validate-body.ts b/packages/daemon/src/daemon/web/validate-body.ts new file mode 100644 index 0000000..55dc516 --- /dev/null +++ b/packages/daemon/src/daemon/web/validate-body.ts @@ -0,0 +1,135 @@ +/** + * HTTP request body validation helpers. + * + * Every mutating web API route parses its body with a Zod schema via + * `parseRequestBody`. Invalid bodies return HTTP 400 before business logic. + */ + +import * as path from 'node:path'; +import type { Response } from 'express'; +import { z } from 'zod'; +import type { DaemonState } from '../state.js'; + +/** + * Format a ZodError into a concise API error string. + */ +export function formatZodError(error: z.ZodError): string { + const issue = error.issues[0]; + if (!issue) return 'Invalid request body'; + const loc = issue.path.length > 0 ? issue.path.join('.') : '(root)'; + return `Invalid request body: ${loc}: ${issue.message}`; +} + +/** + * Parse `body` with `schema`. Never throws. + */ +export function parseRequestBody( + schema: z.ZodType, + body: unknown, +): { success: true; data: T } | { success: false; error: string } { + const result = schema.safeParse(body ?? {}); + if (!result.success) { + return { success: false, error: formatZodError(result.error) }; + } + return { success: true, data: result.data }; +} + +/** + * Send a 400 JSON error for a validation failure. Returns true so callers + * can `if (sendBadRequest(...)) return;`. + */ +export function sendBadRequest(res: Response, error: string): true { + res.status(400).json({ error }); + return true; +} + +/** + * True when `location` contains a `..` path segment (Unix or Windows separators). + */ +export function containsPathTraversal(location: string): boolean { + const normalized = location.replace(/\\/g, '/'); + return normalized.split('/').includes('..'); +} + +/** + * Collect absolute workspace paths the daemon currently trusts + * (connected VS Code workspaces + registered client workspace paths). + */ +export function collectAllowlistedWorkspaces(state: DaemonState): string[] { + const set = new Set(); + const add = (p?: string): void => { + if (typeof p === 'string' && p.trim().length > 0) { + set.add(path.resolve(p)); + } + }; + + for (const w of state.getVSCodeWorkspaces()) { + add(w); + } + for (const c of state.getClients()) { + add(c.workspacePath); + for (const wp of c.workspacePaths ?? []) { + add(wp); + } + } + return [...set]; +} + +export type WorkspaceLocationCheck = + | { ok: true; resolved: string } + | { ok: false; status: 400 | 403; error: string }; + +/** + * Validate a workspace config location against path-traversal rules and the + * allowlisted set of connected workspace paths. + * + * - Traversal (`..`) → 400 + * - Outside allowlist → 403 + */ +export function checkWorkspaceLocation( + location: string, + allowlisted: string[], +): WorkspaceLocationCheck { + if (typeof location !== 'string' || location.trim().length === 0) { + return { ok: false, status: 400, error: 'location must be a non-empty string' }; + } + if (containsPathTraversal(location)) { + return { + ok: false, + status: 400, + error: 'Invalid location: path traversal is not allowed', + }; + } + + const resolved = path.resolve(location); + const allowed = new Set(allowlisted.map((p) => path.resolve(p))); + if (!allowed.has(resolved)) { + return { + ok: false, + status: 403, + error: 'location is not an allowlisted workspace path', + }; + } + return { ok: true, resolved }; +} + +export type ConfigLocationResult = + | { ok: true; kind: 'user' } + | { ok: true; kind: 'workspace'; resolved: string } + | { ok: false; status: 400 | 403; error: string }; + +/** + * Resolve a config `location` value: `'user'` or an allowlisted workspace path. + */ +export function resolveConfigLocation( + location: string, + state: DaemonState, +): ConfigLocationResult { + if (location === 'user') { + return { ok: true, kind: 'user' }; + } + const allowlisted = collectAllowlistedWorkspaces(state); + const check = checkWorkspaceLocation(location, allowlisted); + if (!check.ok) return check; + return { ok: true, kind: 'workspace', resolved: check.resolved }; +} diff --git a/packages/daemon/tests/integration/web-sse.test.ts b/packages/daemon/tests/integration/web-sse.test.ts index 3049aa5..2fc3939 100644 --- a/packages/daemon/tests/integration/web-sse.test.ts +++ b/packages/daemon/tests/integration/web-sse.test.ts @@ -23,6 +23,7 @@ import * as path from 'node:path'; // Mock config paths so POST /api/config writes to a temp dir, not the real user config const tmpConfigDir = fs.mkdtempSync(path.join(os.tmpdir(), 'abbenay-web-test-')); +const tmpWorkspaceDir = fs.mkdtempSync(path.join(os.tmpdir(), 'abbenay-web-ws-')); vi.mock('../../src/core/paths.js', async (importOriginal) => { const actual = await importOriginal(); return { @@ -89,7 +90,7 @@ function createMockState(): DaemonState { }, getVSCodeWorkspaces(): string[] { - return ['/home/test/project']; + return [tmpWorkspaceDir]; }, notifyModelsChanged(_reason: string): void { @@ -158,6 +159,7 @@ afterAll(async () => { await new Promise((resolve) => httpServer.close(() => resolve())); } fs.rmSync(tmpConfigDir, { recursive: true, force: true }); + fs.rmSync(tmpWorkspaceDir, { recursive: true, force: true }); }); // ─── HTTP Helpers ─────────────────────────────────────────────────────── @@ -319,7 +321,7 @@ describe('Web API - Unary Endpoints (direct DaemonState)', () => { it('should list workspaces from DaemonState (wrapped)', async () => { const { statusCode, body } = await httpRequest('GET', `${baseUrl}/api/workspaces`); expect(statusCode).toBe(200); - expect(body.workspaces).toContain('/home/test/project'); + expect(body.workspaces).toContain(tmpWorkspaceDir); }); it('should get daemon status from DaemonState (camelCase)', async () => { @@ -342,7 +344,7 @@ describe('Web API - Unary Endpoints (direct DaemonState)', () => { expect(statusCode).toBe(400); expect(body.error).toContain('model'); }); - + it('should return 400 for POST /api/chat without messages', async () => { const { statusCode, body } = await httpRequest('POST', `${baseUrl}/api/chat`, { model: 'openai/gpt-4o', @@ -350,6 +352,15 @@ describe('Web API - Unary Endpoints (direct DaemonState)', () => { expect(statusCode).toBe(400); expect(body.error).toContain('messages'); }); + + it('should return 400 for POST /api/chat with malformed body (wrong types)', async () => { + const { statusCode, body } = await httpRequest('POST', `${baseUrl}/api/chat`, { + model: 123, + messages: 'not-an-array', + }); + expect(statusCode).toBe(400); + expect(body.error).toMatch(/Invalid request body/); + }); }); describe('Web API - Config Endpoints', () => { @@ -378,6 +389,68 @@ describe('Web API - Config Endpoints', () => { expect(statusCode).toBe(200); expect(body.success).toBe(true); expect(body).toHaveProperty('path'); + expect(fs.existsSync(body.path)).toBe(true); + }); + + it('POST /api/config with allowlisted workspace location saves config', async () => { + const { statusCode, body } = await httpRequest('POST', `${baseUrl}/api/config`, { + location: tmpWorkspaceDir, + config: { + providers: { + openai: { engine: 'openai', models: { 'gpt-4o': {} } }, + }, + }, + }); + expect(statusCode).toBe(200); + expect(body.success).toBe(true); + const savedPath = path.join(tmpWorkspaceDir, '.config', 'abbenay', 'config.yaml'); + expect(body.path).toBe(savedPath); + expect(fs.existsSync(savedPath)).toBe(true); + expect(fs.readFileSync(savedPath, 'utf-8')).toContain('openai'); + }); + + it('POST /api/config rejects path traversal location and writes nothing', async () => { + const before = fs.existsSync(path.join(tmpConfigDir, 'config.yaml')) + ? fs.readFileSync(path.join(tmpConfigDir, 'config.yaml'), 'utf-8') + : null; + const { statusCode, body } = await httpRequest('POST', `${baseUrl}/api/config`, { + location: '../../etc/passwd', + config: { providers: { evil: { engine: 'openai' } } }, + }); + expect([400, 403]).toContain(statusCode); + expect(body.error).toMatch(/traversal|allowlisted/i); + expect(fs.existsSync('/etc/passwd.config')).toBe(false); + // Must not create a workspace config under a traversal target + expect(fs.existsSync(path.join(process.cwd(), '../../etc/passwd', '.config', 'abbenay', 'config.yaml'))).toBe(false); + const after = fs.existsSync(path.join(tmpConfigDir, 'config.yaml')) + ? fs.readFileSync(path.join(tmpConfigDir, 'config.yaml'), 'utf-8') + : null; + expect(after).toBe(before); + }); + + it('POST /api/config rejects location outside allowlist', async () => { + const outside = fs.mkdtempSync(path.join(os.tmpdir(), 'abbenay-outside-')); + try { + const target = path.join(outside, '.config', 'abbenay', 'config.yaml'); + const { statusCode, body } = await httpRequest('POST', `${baseUrl}/api/config`, { + location: outside, + config: { providers: { x: { engine: 'openai' } } }, + }); + expect(statusCode).toBe(403); + expect(body.error).toMatch(/allowlisted/i); + expect(fs.existsSync(target)).toBe(false); + } finally { + fs.rmSync(outside, { recursive: true, force: true }); + } + }); + + it('POST /api/config rejects invalid config schema', async () => { + const { statusCode, body } = await httpRequest('POST', `${baseUrl}/api/config`, { + location: 'user', + config: { providers: { openai: { engine: 123 } }, unexpected_top_level: true }, + }); + expect(statusCode).toBe(400); + expect(body.error).toMatch(/Invalid request body/); }); }); From 5cf77463c7bd54755896d15c8d6949d4761fba0f Mon Sep 17 00:00:00 2001 From: Sudhir Verma <9924513+sudhirverma@users.noreply.github.com> Date: Fri, 17 Jul 2026 14:24:13 +0530 Subject: [PATCH 2/5] fix(daemon): keep config Zod schemas loadable under shallow mocks Move virtual-name validation into config-schema and use importOriginal in config.js mocks so server import chains no longer break CI. --- packages/daemon/src/core/config-schema.ts | 18 +++++++++++++++++- packages/daemon/src/core/config.ts | 18 ++---------------- packages/daemon/src/core/index.ts | 2 ++ packages/daemon/src/state.test.ts | 18 +++++++++++------- .../integration/grpc-real-service.test.ts | 18 +++++++++++------- 5 files changed, 43 insertions(+), 31 deletions(-) diff --git a/packages/daemon/src/core/config-schema.ts b/packages/daemon/src/core/config-schema.ts index bacc9c9..af4add5 100644 --- a/packages/daemon/src/core/config-schema.ts +++ b/packages/daemon/src/core/config-schema.ts @@ -4,10 +4,26 @@ * Shared by the HTTP API (request validation) and any core callers that need * typed config parsing. Keep in sync with the interfaces in config.ts / * policies.ts / tool-registry.ts. + * + * Name validation lives here (not in config.ts) so schema loading does not + * depend on the config I/O module — tests often mock config.js shallowly. */ import { z } from 'zod'; -import { isValidVirtualName } from './config.js'; + +/** + * Regex for virtual provider and model names. + * Lowercase alphanumeric, dots, hyphens, underscores. No slashes, no spaces. + */ +export const VIRTUAL_NAME_REGEX = /^[a-z0-9][a-z0-9._-]*$/; + +/** + * Validate a virtual name (provider or model). + * Engine model IDs (containing slashes) are NOT validated by this. + */ +export function isValidVirtualName(name: string): boolean { + return VIRTUAL_NAME_REGEX.test(name); +} /** Virtual provider/model/policy name (lowercase alphanumeric + ._-). */ export const VirtualNameSchema = z diff --git a/packages/daemon/src/core/config.ts b/packages/daemon/src/core/config.ts index 506d73c..0c2832c 100644 --- a/packages/daemon/src/core/config.ts +++ b/packages/daemon/src/core/config.ts @@ -17,23 +17,9 @@ import { getWorkspaceConfigPath as _getWorkspaceConfigPath, } from './paths.js'; -// ── Name validation ──────────────────────────────────────────────────── +// ── Name validation (canonical implementation in config-schema.ts) ───── -/** - * Regex for virtual provider and model names. - * Lowercase alphanumeric, dots, hyphens, underscores. No slashes, no spaces. - * Engine model IDs from discovery (which may contain slashes) are exempt. - */ -const VIRTUAL_NAME_REGEX = /^[a-z0-9][a-z0-9._-]*$/; - -/** - * Validate a virtual name (provider or model). - * Returns true if valid, false otherwise. - * Engine model IDs (containing slashes) are NOT validated by this — they pass through. - */ -export function isValidVirtualName(name: string): boolean { - return VIRTUAL_NAME_REGEX.test(name); -} +export { isValidVirtualName, VIRTUAL_NAME_REGEX } from './config-schema.js'; // ── Config interfaces ────────────────────────────────────────────────── diff --git a/packages/daemon/src/core/index.ts b/packages/daemon/src/core/index.ts index 5c22c35..e36f5d1 100644 --- a/packages/daemon/src/core/index.ts +++ b/packages/daemon/src/core/index.ts @@ -159,6 +159,8 @@ export { ModelConfigSchema, ProviderConfigSchema, VirtualNameSchema, + VIRTUAL_NAME_REGEX, + isValidVirtualName, parseConfigFile, } from './config-schema.js'; diff --git a/packages/daemon/src/state.test.ts b/packages/daemon/src/state.test.ts index 0e24e10..d3b6ca6 100644 --- a/packages/daemon/src/state.test.ts +++ b/packages/daemon/src/state.test.ts @@ -21,13 +21,17 @@ const mockResolveEngineModelId = vi.fn().mockImplementation( (name: string, cfg: ModelConfig) => cfg.model_id || name ); -vi.mock('./core/config.js', () => ({ - loadConfig: (...a: unknown[]) => mockLoadConfig(...a), - loadWorkspaceConfig: (...a: unknown[]) => mockLoadWorkspaceConfig(...a), - mergeConfigs: (...a: unknown[]) => mockMergeConfigs(...a), - mergeMultipleWorkspaceConfigs: (...a: unknown[]) => mockMergeMultipleWorkspaceConfigs(...a), - resolveEngineModelId: (...a: unknown[]) => mockResolveEngineModelId(...a), -})); +vi.mock('./core/config.js', async (importOriginal) => { + const actual = await importOriginal(); + return { + ...actual, + loadConfig: (...a: unknown[]) => mockLoadConfig(...a), + loadWorkspaceConfig: (...a: unknown[]) => mockLoadWorkspaceConfig(...a), + mergeConfigs: (...a: unknown[]) => mockMergeConfigs(...a), + mergeMultipleWorkspaceConfigs: (...a: unknown[]) => mockMergeMultipleWorkspaceConfigs(...a), + resolveEngineModelId: (...a: unknown[]) => mockResolveEngineModelId(...a), + }; +}); // ── Mock: core/engines ─────────────────────────────────────────────────────── diff --git a/packages/daemon/tests/integration/grpc-real-service.test.ts b/packages/daemon/tests/integration/grpc-real-service.test.ts index db48648..bfa3e9e 100644 --- a/packages/daemon/tests/integration/grpc-real-service.test.ts +++ b/packages/daemon/tests/integration/grpc-real-service.test.ts @@ -34,13 +34,17 @@ const mockResolveEngineModelId = vi.fn().mockImplementation( (name: string, cfg: { model_id?: string }) => cfg.model_id || name, ); -vi.mock('../../src/core/config.js', () => ({ - loadConfig: (...a: unknown[]) => mockLoadConfig(...a), - loadWorkspaceConfig: (...a: unknown[]) => mockLoadWorkspaceConfig(...a), - mergeConfigs: (...a: unknown[]) => mockMergeConfigs(...a), - mergeMultipleWorkspaceConfigs: (...a: unknown[]) => mockMergeMultipleWorkspaceConfigs(...a), - resolveEngineModelId: (...a: unknown[]) => mockResolveEngineModelId(...a), -})); +vi.mock('../../src/core/config.js', async (importOriginal) => { + const actual = await importOriginal(); + return { + ...actual, + loadConfig: (...a: unknown[]) => mockLoadConfig(...a), + loadWorkspaceConfig: (...a: unknown[]) => mockLoadWorkspaceConfig(...a), + mergeConfigs: (...a: unknown[]) => mockMergeConfigs(...a), + mergeMultipleWorkspaceConfigs: (...a: unknown[]) => mockMergeMultipleWorkspaceConfigs(...a), + resolveEngineModelId: (...a: unknown[]) => mockResolveEngineModelId(...a), + }; +}); const mockGetEngines = vi.fn().mockReturnValue([ { id: 'mock', requiresKey: false, supportsTools: false, createModel: () => { throw new Error('mock'); } }, From f08496c0d9782214ba4bcdb744f50c2b67490255 Mon Sep 17 00:00:00 2001 From: Sudhir Verma <9924513+sudhirverma@users.noreply.github.com> Date: Fri, 17 Jul 2026 14:42:23 +0530 Subject: [PATCH 3/5] fix(daemon): remove duplicate isValidVirtualName export tsc failed in CI because the barrel re-exported isValidVirtualName from both config and config-schema after the schema move. --- packages/daemon/src/core/index.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/packages/daemon/src/core/index.ts b/packages/daemon/src/core/index.ts index e36f5d1..9fc8326 100644 --- a/packages/daemon/src/core/index.ts +++ b/packages/daemon/src/core/index.ts @@ -149,7 +149,6 @@ export { getConfiguredProviders, resolveEngineModelId, getEnabledModelNames, - isValidVirtualName, } from './config.js'; /** Zod schemas for ConfigFile / PolicyConfig validation */ From 5da56220f86592691746601334c2a887e81d93b1 Mon Sep 17 00:00:00 2001 From: Sudhir Verma <9924513+sudhirverma@users.noreply.github.com> Date: Fri, 17 Jul 2026 23:34:04 +0530 Subject: [PATCH 4/5] fix(daemon): allow openai_compat fields in ConfigFileSchema Keep DR-032 passthrough settings persistable via POST /api/config after Zod .strict() validation (H4/H5). --- docs/decisions.md | 4 ++- .../daemon/src/core/config-schema.test.ts | 34 +++++++++++++++++++ packages/daemon/src/core/config-schema.ts | 11 ++++++ packages/daemon/src/core/index.ts | 2 ++ 4 files changed, 50 insertions(+), 1 deletion(-) diff --git a/docs/decisions.md b/docs/decisions.md index f53316f..dd0cddc 100644 --- a/docs/decisions.md +++ b/docs/decisions.md @@ -572,7 +572,9 @@ Shared `ConfigFile` / `PolicyConfig` schemas live in `@abbenay/core` (`config-schema.ts`) and are reused by the web layer (`api-schemas.ts`). Schemas use `.strict()` so unexpected fields are rejected (field-injection defense). OpenAI `/v1/chat/completions` allows optional `tools` so DR-032 -passthrough continues to work under validation. +passthrough continues to work under validation. `ConfigFileSchema` / +`ModelConfigSchema` include `openai_compat` and `openai_compat_tools` so +dashboard/`POST /api/config` can persist DR-032 settings without 400s. **Rationale:** Unvalidated `req.body` destructuring allowed arbitrary config writes, path traversal into workspace config paths, type confusion, and field injection (findings H4/H5). Auth (DR-030) authenticates the caller but diff --git a/packages/daemon/src/core/config-schema.test.ts b/packages/daemon/src/core/config-schema.test.ts index 70c8019..8de8783 100644 --- a/packages/daemon/src/core/config-schema.test.ts +++ b/packages/daemon/src/core/config-schema.test.ts @@ -69,6 +69,40 @@ describe('ConfigFileSchema', () => { }); expect(result.success).toBe(false); }); + + it('accepts openai_compat tools passthrough (DR-032)', () => { + const result = parseConfigFile({ + providers: {}, + openai_compat: { tools: 'passthrough' }, + }); + expect(result.success).toBe(true); + }); + + it('accepts per-model openai_compat_tools override', () => { + const result = parseConfigFile({ + providers: { + openai: { + engine: 'openai', + models: { 'gpt-4o': { openai_compat_tools: 'passthrough' } }, + }, + }, + }); + expect(result.success).toBe(true); + }); + + it('rejects invalid openai_compat.tools values', () => { + const result = parseConfigFile({ + openai_compat: { tools: 'auto' }, + }); + expect(result.success).toBe(false); + }); + + it('rejects unknown keys under openai_compat', () => { + const result = ConfigFileSchema.safeParse({ + openai_compat: { tools: 'passthrough', evil: true }, + }); + expect(result.success).toBe(false); + }); }); describe('PolicyConfigSchema', () => { diff --git a/packages/daemon/src/core/config-schema.ts b/packages/daemon/src/core/config-schema.ts index af4add5..b99f9ca 100644 --- a/packages/daemon/src/core/config-schema.ts +++ b/packages/daemon/src/core/config-schema.ts @@ -33,6 +33,15 @@ export const VirtualNameSchema = z message: 'must be lowercase alphanumeric with dots, hyphens, or underscores', }); +/** Opt-in mode for OpenAI-compatible `/v1` tools (DR-032). */ +export const OpenAICompatToolsModeSchema = z.enum(['off', 'passthrough']); + +export const OpenAICompatConfigSchema = z + .object({ + tools: OpenAICompatToolsModeSchema.optional(), + }) + .strict(); + export const ModelConfigSchema = z .object({ model_id: z.string().min(1).optional(), @@ -44,6 +53,7 @@ export const ModelConfigSchema = z top_k: z.number().int().nonnegative().optional(), max_tokens: z.number().int().positive().optional(), timeout: z.number().positive().optional(), + openai_compat_tools: OpenAICompatToolsModeSchema.optional(), }) .strict(); @@ -115,6 +125,7 @@ export const ConfigFileSchema = z tool_policy: ToolPolicyConfigSchema.optional(), consumers: z.record(z.string(), ConsumerConfigSchema).optional(), server: ServerConfigSchema.optional(), + openai_compat: OpenAICompatConfigSchema.optional(), }) .strict(); diff --git a/packages/daemon/src/core/index.ts b/packages/daemon/src/core/index.ts index 9fc8326..651236a 100644 --- a/packages/daemon/src/core/index.ts +++ b/packages/daemon/src/core/index.ts @@ -157,6 +157,8 @@ export { PolicyConfigSchema, ModelConfigSchema, ProviderConfigSchema, + OpenAICompatConfigSchema, + OpenAICompatToolsModeSchema, VirtualNameSchema, VIRTUAL_NAME_REGEX, isValidVirtualName, From fe156a268aef0b667f81a17906ffc11659b88b93 Mon Sep 17 00:00:00 2001 From: "Bradley A. Thornton" Date: Fri, 17 Jul 2026 11:44:31 -0700 Subject: [PATCH 5/5] fix(daemon): strip OpenAI extras; harden workspace path checks Avoid .strict() on /v1/chat/completions so DR-020 clients with stream_options/user/stop keep working. Reject NUL in locations, compare realpath when present, and validate empty bodies on MCP DELETE routes. --- docs/decisions.md | 10 ++++-- packages/daemon/src/daemon/web/api-schemas.ts | 9 +++-- packages/daemon/src/daemon/web/server.ts | 10 ++++++ .../src/daemon/web/validate-body.test.ts | 33 ++++++++++++++++++- .../daemon/src/daemon/web/validate-body.ts | 26 ++++++++++++--- .../daemon/tests/integration/web-sse.test.ts | 5 ++- 6 files changed, 79 insertions(+), 14 deletions(-) diff --git a/docs/decisions.md b/docs/decisions.md index dd0cddc..2350867 100644 --- a/docs/decisions.md +++ b/docs/decisions.md @@ -570,11 +570,15 @@ allowlisted connected workspace path; path segments containing `..` are rejected (400) and non-allowlisted paths return 403 with no file write. Shared `ConfigFile` / `PolicyConfig` schemas live in `@abbenay/core` (`config-schema.ts`) and are reused by the web layer (`api-schemas.ts`). -Schemas use `.strict()` so unexpected fields are rejected (field-injection -defense). OpenAI `/v1/chat/completions` allows optional `tools` so DR-032 -passthrough continues to work under validation. `ConfigFileSchema` / +Most schemas use `.strict()` so unexpected fields are rejected +(field-injection defense). OpenAI `/v1/chat/completions` instead **strips** +unknown client fields (e.g. `stream_options`, `user`) so DR-020 SDKs keep +working, while still requiring `model` / non-empty `messages` and allowing +optional `tools` for DR-032 passthrough. `ConfigFileSchema` / `ModelConfigSchema` include `openai_compat` and `openai_compat_tools` so dashboard/`POST /api/config` can persist DR-032 settings without 400s. +Workspace location checks also reject NUL bytes and compare `realpath` when +the path exists. **Rationale:** Unvalidated `req.body` destructuring allowed arbitrary config writes, path traversal into workspace config paths, type confusion, and field injection (findings H4/H5). Auth (DR-030) authenticates the caller but diff --git a/packages/daemon/src/daemon/web/api-schemas.ts b/packages/daemon/src/daemon/web/api-schemas.ts index d1ea29c..cb014e8 100644 --- a/packages/daemon/src/daemon/web/api-schemas.ts +++ b/packages/daemon/src/daemon/web/api-schemas.ts @@ -2,8 +2,9 @@ * Zod request-body schemas for every mutating HTTP web API route. * * Config shapes are shared with `@abbenay/core` via config-schema.ts. - * Route schemas use `.strict()` so unexpected fields are rejected before - * business logic runs. + * Most route schemas use `.strict()` so unexpected fields are rejected before + * business logic runs. OpenAI `/v1` uses `.strip()` so unknown client fields + * do not 400 (DR-020 compatibility). */ import { z } from 'zod'; @@ -184,6 +185,8 @@ const OpenAIChatMessageSchema = z }) .passthrough(); +// Strip unknown OpenAI client fields (stream_options, user, stop, …) rather than +// .strict()-rejecting them — DR-020 clients send extras by default. export const PostOpenAIChatCompletionsBodySchema = z .object({ model: z.string().min(1), @@ -196,7 +199,7 @@ export const PostOpenAIChatCompletionsBodySchema = z // DR-032: optional client tools for opt-in passthrough (validated/mapped in openai-compat). tools: z.array(z.unknown()).optional(), }) - .strict(); + .strip(); export type PostConfigBody = z.infer; export type PostChatBody = z.infer; diff --git a/packages/daemon/src/daemon/web/server.ts b/packages/daemon/src/daemon/web/server.ts index ef159e1..39febf7 100644 --- a/packages/daemon/src/daemon/web/server.ts +++ b/packages/daemon/src/daemon/web/server.ts @@ -421,6 +421,11 @@ export function createWebApp(state: DaemonState, options?: WebSecurityOptions): */ app.delete('/api/mcp/connections/sessions/:sessionId', async (req, res) => { try { + const parsed = parseRequestBody(EmptyBodySchema, req.body); + if (!parsed.success) { + sendBadRequest(res, parsed.error); + return; + } if (typeof state.mcpServer?.revokeSession !== 'function') { res.status(404).json({ error: 'MCP server does not support session revoke' }); return; @@ -441,6 +446,11 @@ export function createWebApp(state: DaemonState, options?: WebSecurityOptions): * DELETE /api/mcp/connections/remembered/:clientName — forget a remembered client */ app.delete('/api/mcp/connections/remembered/:clientName', (req, res) => { + const parsed = parseRequestBody(EmptyBodySchema, req.body); + if (!parsed.success) { + sendBadRequest(res, parsed.error); + return; + } const clientName = decodeURIComponent(req.params.clientName || '').trim(); if (!clientName) { res.status(400).json({ error: 'clientName is required' }); diff --git a/packages/daemon/src/daemon/web/validate-body.test.ts b/packages/daemon/src/daemon/web/validate-body.test.ts index 779f1f5..8d4686a 100644 --- a/packages/daemon/src/daemon/web/validate-body.test.ts +++ b/packages/daemon/src/daemon/web/validate-body.test.ts @@ -11,7 +11,11 @@ import { formatZodError, parseRequestBody, } from './validate-body.js'; -import { PostChatBodySchema, PostConfigBodySchema } from './api-schemas.js'; +import { + PostChatBodySchema, + PostConfigBodySchema, + PostOpenAIChatCompletionsBodySchema, +} from './api-schemas.js'; describe('containsPathTraversal', () => { it('detects .. segments on Unix and Windows separators', () => { @@ -54,6 +58,15 @@ describe('checkWorkspaceLocation', () => { expect(result.error).toMatch(/allowlisted/i); } }); + + it('rejects null bytes with 400', () => { + const result = checkWorkspaceLocation('/tmp/allowed-ws\0/../etc/passwd', allowed); + expect(result.ok).toBe(false); + if (!result.ok) { + expect(result.status).toBe(400); + expect(result.error).toMatch(/null/i); + } + }); }); describe('parseRequestBody', () => { @@ -92,6 +105,24 @@ describe('parseRequestBody', () => { }); expect(result.success).toBe(false); }); + + it('strips unknown OpenAI client fields instead of rejecting them', () => { + const result = parseRequestBody(PostOpenAIChatCompletionsBodySchema, { + model: 'openai/gpt-4o', + messages: [{ role: 'user', content: 'hi' }], + stream: true, + stream_options: { include_usage: true }, + user: 'client-1', + stop: ['\n'], + }); + expect(result.success).toBe(true); + if (result.success) { + expect(result.data.model).toBe('openai/gpt-4o'); + expect(result.data.stream).toBe(true); + expect('stream_options' in result.data).toBe(false); + expect('user' in result.data).toBe(false); + } + }); }); describe('formatZodError', () => { diff --git a/packages/daemon/src/daemon/web/validate-body.ts b/packages/daemon/src/daemon/web/validate-body.ts index 55dc516..dde396e 100644 --- a/packages/daemon/src/daemon/web/validate-body.ts +++ b/packages/daemon/src/daemon/web/validate-body.ts @@ -5,11 +5,22 @@ * `parseRequestBody`. Invalid bodies return HTTP 400 before business logic. */ +import * as fs from 'node:fs'; import * as path from 'node:path'; import type { Response } from 'express'; import { z } from 'zod'; import type { DaemonState } from '../state.js'; +/** Resolve to a real path when the target exists; otherwise path.resolve. */ +function resolvePathKey(p: string): string { + const resolved = path.resolve(p); + try { + return fs.realpathSync(resolved); + } catch { + return resolved; + } +} + /** * Format a ZodError into a concise API error string. */ @@ -58,8 +69,8 @@ export function containsPathTraversal(location: string): boolean { export function collectAllowlistedWorkspaces(state: DaemonState): string[] { const set = new Set(); const add = (p?: string): void => { - if (typeof p === 'string' && p.trim().length > 0) { - set.add(path.resolve(p)); + if (typeof p === 'string' && p.trim().length > 0 && !p.includes('\0')) { + set.add(resolvePathKey(p)); } }; @@ -93,6 +104,13 @@ export function checkWorkspaceLocation( if (typeof location !== 'string' || location.trim().length === 0) { return { ok: false, status: 400, error: 'location must be a non-empty string' }; } + if (location.includes('\0')) { + return { + ok: false, + status: 400, + error: 'Invalid location: null bytes are not allowed', + }; + } if (containsPathTraversal(location)) { return { ok: false, @@ -101,8 +119,8 @@ export function checkWorkspaceLocation( }; } - const resolved = path.resolve(location); - const allowed = new Set(allowlisted.map((p) => path.resolve(p))); + const resolved = resolvePathKey(location); + const allowed = new Set(allowlisted.map((p) => resolvePathKey(p))); if (!allowed.has(resolved)) { return { ok: false, diff --git a/packages/daemon/tests/integration/web-sse.test.ts b/packages/daemon/tests/integration/web-sse.test.ts index 2fc3939..3e243af 100644 --- a/packages/daemon/tests/integration/web-sse.test.ts +++ b/packages/daemon/tests/integration/web-sse.test.ts @@ -417,9 +417,8 @@ describe('Web API - Config Endpoints', () => { location: '../../etc/passwd', config: { providers: { evil: { engine: 'openai' } } }, }); - expect([400, 403]).toContain(statusCode); - expect(body.error).toMatch(/traversal|allowlisted/i); - expect(fs.existsSync('/etc/passwd.config')).toBe(false); + expect(statusCode).toBe(400); + expect(body.error).toMatch(/traversal/i); // Must not create a workspace config under a traversal target expect(fs.existsSync(path.join(process.cwd(), '../../etc/passwd', '.config', 'abbenay', 'config.yaml'))).toBe(false); const after = fs.existsSync(path.join(tmpConfigDir, 'config.yaml'))