diff --git a/CHANGELOG.md b/CHANGELOG.md index 58a60c2..5ad8e1c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,19 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### ⚠️ Breaking + +- **`deepcode mcp serve` now applies your permission settings.** It executed + Read/Write/Edit/Bash for any connected MCP peer with no mode, no permission + rules, no file contract and no `PreToolUse` hooks — the same shape as the + `runAgent` bypass fixed in #181, in a different entry point. Every call now + goes through the central gate, a call that would need approval is **refused** + (nobody is attached to that pipe to approve it), and a permissive + `permissions.defaultMode` is clamped to `default` exactly as a scheduled job's + is. A peer can now do what `permissions.allow` says it can and nothing else, + so anyone relying on the old behaviour must add rules — or start the server + with an explicit `--mode`. `--sandbox` also applies now; it did not before. + ### 🔒 Security - **A sub-agent did not inherit the file contract.** The `Task` delegation diff --git a/apps/cli/src/cli.ts b/apps/cli/src/cli.ts index 2ea2a0b..ac67d16 100644 --- a/apps/cli/src/cli.ts +++ b/apps/cli/src/cli.ts @@ -100,6 +100,10 @@ async function main(): Promise { cwd: process.cwd(), output: process.stdout, errOutput: process.stderr, + // `mcp serve` has no attached user, so a permissive ambient mode is + // clamped unless --mode says otherwise. Same rule as a scheduled job. + mode: args.mode, + sandbox: args.sandbox, }); } if (args.positional[0] === 'app-server') { diff --git a/apps/cli/src/mcp-cmd.test.ts b/apps/cli/src/mcp-cmd.test.ts index ffd0708..7bc5ce0 100644 --- a/apps/cli/src/mcp-cmd.test.ts +++ b/apps/cli/src/mcp-cmd.test.ts @@ -1,5 +1,9 @@ +import { mkdir, mkdtemp, rm, writeFile } from 'node:fs/promises'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; import { Writable } from 'node:stream'; -import { describe, expect, it } from 'vitest'; +import { afterEach, beforeEach, describe, expect, it } from 'vitest'; +import type { Mode, ServeMcpStdioOpts } from '@deepcode/core'; import { runMcpCommand } from './mcp-cmd.js'; function sink(): { stream: Writable; text: () => string } { @@ -51,3 +55,96 @@ describe('runMcpCommand', () => { expect(err.text()).toMatch(/\[mcp\] ready: Read, Write/); }); }); + +// The served tools are Read/Write/Edit/Bash in a real project, and nobody is on +// the other end of the pipe to approve anything. +describe('runMcpCommand serve — permission posture', () => { + let home: string; + let cwd: string; + + beforeEach(async () => { + home = await mkdtemp(join(tmpdir(), 'dc-mcp-home-')); + cwd = await mkdtemp(join(tmpdir(), 'dc-mcp-cwd-')); + }); + afterEach(async () => { + await rm(home, { recursive: true, force: true }); + await rm(cwd, { recursive: true, force: true }); + }); + + async function serve(opts: { mode?: Mode } = {}): Promise<{ + err: string; + captured: ServeMcpStdioOpts | undefined; + }> { + const out = sink(); + const err = sink(); + let captured: ServeMcpStdioOpts | undefined; + await runMcpCommand(['serve'], { + cwd, + home, + output: out.stream, + errOutput: err.stream, + mode: opts.mode, + serve: async (o) => { + captured = o; + }, + }); + return { err: err.text(), captured }; + } + + async function writeUserSettings(settings: Record): Promise { + await mkdir(join(home, '.deepcode'), { recursive: true }); + await writeFile(join(home, '.deepcode', 'settings.json'), JSON.stringify(settings)); + } + + it('passes a gate at all — the server cannot be built without one', async () => { + const { captured } = await serve(); + expect(captured?.gate).toBeTypeOf('function'); + }); + + it('refuses a call that would need approval', async () => { + const { captured } = await serve(); + const verdict = await captured!.gate({ + tool: 'Write', + input: { file_path: join(cwd, 'x.txt'), content: 'x' }, + }); + expect(verdict.allowed).toBe(false); + expect(verdict.reason).toMatch(/no attached user/); + }); + + it('clamps a permissive ambient mode and says so', async () => { + // `bypassPermissions` in settings.json is a choice about sitting at a REPL. + // Inheriting it here would hand "never ask me" to whatever connected. + await writeUserSettings({ permissions: { defaultMode: 'bypassPermissions' } }); + const { err, captured } = await serve(); + expect(err).toMatch(/was not applied to this unattended run/); + expect(err).toMatch(/mode=default/); + + const verdict = await captured!.gate({ + tool: 'Bash', + input: { command: 'rm -rf /' }, + }); + expect(verdict.allowed).toBe(false); + }); + + it('--mode is the explicit opt-in back out of the clamp', async () => { + await writeUserSettings({ permissions: { defaultMode: 'bypassPermissions' } }); + const { err, captured } = await serve({ mode: 'bypassPermissions' }); + expect(err).not.toMatch(/was not applied/); + expect(err).toMatch(/mode=bypassPermissions/); + expect((await captured!.gate({ tool: 'Bash', input: { command: 'echo hi' } })).allowed).toBe( + true, + ); + }); + + it('honours permissions.allow without any mode change', async () => { + await writeUserSettings({ permissions: { allow: ['Read'] } }); + const { captured } = await serve(); + expect( + (await captured!.gate({ tool: 'Read', input: { file_path: join(cwd, 'a') } })).allowed, + ).toBe(true); + expect( + (await captured!.gate({ tool: 'Write', input: { file_path: join(cwd, 'a'), content: '' } })) + .allowed, + ).toBe(false); + }); +}); diff --git a/apps/cli/src/mcp-cmd.ts b/apps/cli/src/mcp-cmd.ts index 2ebf5c0..31e2a11 100644 --- a/apps/cli/src/mcp-cmd.ts +++ b/apps/cli/src/mcp-cmd.ts @@ -6,12 +6,23 @@ // line goes to stderr, and nothing else may touch stdout. import { + HookDispatcher, VERSION, + buildMcpGate, + describeClamp, + gateUntrustedSettings, + loadFileContract, + loadSettings, mcpServableTools, + resolveTriggerMode, serveMcpOverStdio, + withSandboxMode, + type Mode, + type SandboxMode, type ServeMcpStdioOpts, } from '@deepcode/core'; import type { Writable } from 'node:stream'; +import { TrustStore } from './trust.js'; export interface McpCmdDeps { cwd: string; @@ -23,6 +34,12 @@ export interface McpCmdDeps { signal?: AbortSignal; /** Serve implementation — injectable so tests don't grab the real stdio. */ serve?: (opts: ServeMcpStdioOpts) => Promise; + /** Override `~/.deepcode` (tests). */ + home?: string; + /** `--mode`: the explicit opt-in out of the unattended clamp. */ + mode?: Mode; + /** `--sandbox`: tightens the sandbox for served commands. */ + sandbox?: SandboxMode; } export async function runMcpCommand(sub: string[], deps: McpCmdDeps): Promise { @@ -32,13 +49,62 @@ export async function runMcpCommand(sub: string[], deps: McpCmdDeps): Promise 0) { + err.write( + `[mcp] untrusted directory — ignoring project ${trustGate.gated.join(', ')}. ` + + `Run \`deepcode trust\` to enable.\n`, + ); + } + + // Nobody is attached to this pipe, so a permissive `defaultMode` picked for + // REPL convenience must not become the posture of whatever connects. Same + // clamp, and the same explicit opt-in, that scheduled jobs use. + const ambient = (settings.permissions?.defaultMode ?? 'default') as Mode; + const resolved = resolveTriggerMode(deps.mode ? { mode: deps.mode } : undefined, ambient); + const clampNote = describeClamp(resolved); + if (clampNote) err.write(`[mcp] ${clampNote}\n`); + + const contract = await loadFileContract({ cwd, home: deps.home }); + if (contract.status === 'invalid') { + err.write(`[mcp] file contract could not be parsed: ${contract.error}\n`); + } + + const hooks = new HookDispatcher({ + hooks: settings.hooks, + disableAllHooks: settings.disableAllHooks, + allowedHttpHookUrls: settings.allowedHttpHookUrls, + }); + err.write( - `DeepCode MCP server v${VERSION} — exposing ${tools.length} tools over stdio in ${deps.cwd}\n`, + `DeepCode MCP server v${VERSION} — exposing ${tools.length} tools over stdio in ${cwd}\n`, ); + err.write(`[mcp] mode=${resolved.mode}; calls needing approval are refused, not granted\n`); + await (deps.serve ?? serveMcpOverStdio)({ - cwd: deps.cwd, + cwd, version: VERSION, signal: deps.signal, + gate: buildMcpGate({ + cwd, + mode: resolved.mode, + permissions: settings.permissions, + contract: contract.contract, + hooks, + autoMode: settings.autoMode, + }), + contract: contract.contract, + sandboxConfig: withSandboxMode(settings.sandbox, deps.sandbox), onReady: (names) => err.write(`[mcp] ready: ${names.join(', ')}\n`), }); return 0; diff --git a/docs/security-model.md b/docs/security-model.md index 6858466..523ef24 100644 --- a/docs/security-model.md +++ b/docs/security-model.md @@ -24,6 +24,32 @@ decreasing order of operator severity: | 8 | Model reads an in-project secret (`.env`, `*.pem`) through a read tool | Partly mitigated | File contract `read: deny` — covers Read/Grep/Glob, **not Bash** (see below) | | 9 | Unattended job runs with a permissive mode inherited from interactive settings | Mitigated | Trigger profile clamp + `onApprovalRequired` | | 10 | User cannot audit or undo what the agent wrote | Mitigated | Change ledger + `deepcode ledger rollback` through the apply ceremony | +| 11 | An MCP peer calls DeepCode's own tools without any policy | Mitigated | `mcp serve` routes every call through `dispatchToolCall`; `ask` is refused (see below) | + +### `deepcode mcp serve` runs with nobody attached + +`mcp serve` exposes Read / Write / Edit / Bash / Grep / Glob to whatever MCP +client connects — typically another agent. It executed them directly, with no +mode, no permission rules, no file contract and no `PreToolUse` hooks: the same +shape as the `runAgent` bypass fixed in #181, in a different entry point. The +original plan listed the missing permission model as a known risk and deferred +the design document; the feature shipped without either. + +Every call now goes through `dispatchToolCall`, and: + +- **`ask` is refused, not granted.** There is no user on that pipe. Granting + would make "whoever connected" the authority on what may run. +- **A permissive `permissions.defaultMode` is clamped** to `default`, exactly as + a scheduled job's is. `bypassPermissions` is a decision about sitting at a + REPL; inheriting it here hands "never ask me" to a peer. +- **`--mode` is the explicit opt-in** back out of that clamp, and `--sandbox` + tightens the sandbox for served commands. +- Directory trust still gates project settings, so an untrusted checkout cannot + widen the posture the server runs under. + +The practical consequence: a peer can do what `permissions.allow` says it can, +and nothing else. That is a real reduction in capability for anyone who was +relying on the old behaviour, and it is the point. ### Residual risk: the file contract is policy, not a boundary diff --git a/packages/core/src/index.ts b/packages/core/src/index.ts index 3ac88ff..278467f 100644 --- a/packages/core/src/index.ts +++ b/packages/core/src/index.ts @@ -276,6 +276,7 @@ export { connectAllMcpServers, closeAllMcpServers, buildMcpServer, + buildMcpGate, serveMcpOverStdio, mcpServableTools, MCP_SERVE_EXCLUDE, diff --git a/packages/core/src/mcp/index.ts b/packages/core/src/mcp/index.ts index 70b28f1..0af28ff 100644 --- a/packages/core/src/mcp/index.ts +++ b/packages/core/src/mcp/index.ts @@ -33,10 +33,14 @@ export { export { buildMcpServer, + buildMcpGate, serveMcpOverStdio, mcpServableTools, MCP_SERVE_EXCLUDE, type BuildMcpServerOpts, + type McpGateOptions, + type McpGateVerdict, + type McpToolGate, type ServeMcpStdioOpts, } from './serve.js'; diff --git a/packages/core/src/mcp/serve.test.ts b/packages/core/src/mcp/serve.test.ts index 0ee3029..629ba47 100644 --- a/packages/core/src/mcp/serve.test.ts +++ b/packages/core/src/mcp/serve.test.ts @@ -5,7 +5,11 @@ import { mkdtemp, rm } from 'node:fs/promises'; import { tmpdir } from 'node:os'; import { join } from 'node:path'; import { afterEach, beforeEach, describe, expect, it } from 'vitest'; -import { buildMcpServer, MCP_SERVE_EXCLUDE, mcpServableTools } from './serve.js'; +import { parseFileContract } from '../config/file-contract.js'; +import { buildMcpGate, buildMcpServer, MCP_SERVE_EXCLUDE, mcpServableTools } from './serve.js'; + +/** The gate the existing round-trip tests run under: everything permitted. */ +const allowAll = async () => ({ allowed: true, reason: 'test' }); describe('mcpServableTools', () => { it('excludes interactive / host-coupled tools', () => { @@ -30,7 +34,12 @@ describe('buildMcpServer over an in-memory transport', () => { beforeEach(async () => { dir = await mkdtemp(join(tmpdir(), 'dc-mcp-serve-')); const [clientTransport, serverTransport] = InMemoryTransport.createLinkedPair(); - const server = buildMcpServer({ cwd: dir, name: 'deepcode-test', version: '9.9.9' }); + const server = buildMcpServer({ + cwd: dir, + name: 'deepcode-test', + version: '9.9.9', + gate: allowAll, + }); client = new Client({ name: 'test-client', version: '0.0.0' }, { capabilities: {} }); await Promise.all([server.connect(serverTransport), client.connect(clientTransport)]); }); @@ -87,3 +96,97 @@ describe('buildMcpServer over an in-memory transport', () => { expect(res.isError).toBe(true); }); }); + +// `mcp serve` hands Read/Write/Edit/Bash to whatever connected. It used to do +// that with no mode, no permission rules, no file contract and no PreToolUse +// hooks — the same shape as the #181 runAgent bypass, in a different entry +// point. `gate` is required so a host cannot omit it by accident. +describe('the gate', () => { + let dir: string; + + beforeEach(async () => { + dir = await mkdtemp(join(tmpdir(), 'dc-mcp-gate-')); + }); + afterEach(async () => { + await rm(dir, { recursive: true, force: true }); + }); + + async function connect(server: ReturnType): Promise { + const [clientTransport, serverTransport] = InMemoryTransport.createLinkedPair(); + const c = new Client({ name: 'test-client', version: '0.0.0' }, { capabilities: {} }); + await Promise.all([server.connect(serverTransport), c.connect(clientTransport)]); + return c; + } + + it('refuses a denied call without executing it', async () => { + const c = await connect( + buildMcpServer({ + cwd: dir, + gate: async () => ({ allowed: false, reason: 'denied by permission rules' }), + }), + ); + const file = join(dir, 'should-not-exist.txt'); + const res = (await c.callTool({ + name: 'Write', + arguments: { file_path: file, content: 'x' }, + })) as { content: Array<{ text: string }>; isError?: boolean }; + + expect(res.isError).toBe(true); + expect(res.content[0]!.text).toMatch(/denied by permission rules/); + await expect(fs.access(file)).rejects.toThrow(); + await c.close(); + }); + + it('sees the arguments the peer actually sent', async () => { + const seen: Array<{ tool: string; input: Record }> = []; + const c = await connect( + buildMcpServer({ + cwd: dir, + gate: async (req) => { + seen.push(req); + return { allowed: true, reason: 'ok' }; + }, + }), + ); + await c.callTool({ name: 'Read', arguments: { file_path: join(dir, 'x') } }); + expect(seen).toEqual([{ tool: 'Read', input: { file_path: join(dir, 'x') } }]); + await c.close(); + }); +}); + +describe('buildMcpGate', () => { + const cwd = '/work/repo'; + + it('refuses an `ask`, because nobody is attached to be asked', async () => { + // Fail-closed, matching an unattended cron run. Granting instead would make + // "who connected" the authority on what may run. + const gate = buildMcpGate({ cwd, mode: 'default' }); + const verdict = await gate({ tool: 'Write', input: { file_path: 'a.ts', content: 'x' } }); + expect(verdict.allowed).toBe(false); + expect(verdict.reason).toMatch(/no attached user/); + expect(verdict.reason).toMatch(/permissions.allow/); + }); + + it('allows what settings explicitly allow', async () => { + const gate = buildMcpGate({ + cwd, + mode: 'default', + permissions: { allow: ['Read'] }, + }); + expect((await gate({ tool: 'Read', input: { file_path: 'a.ts' } })).allowed).toBe(true); + }); + + it('honours a file contract deny', async () => { + const gate = buildMcpGate({ + cwd, + mode: 'bypassPermissions', + contract: parseFileContract( + ['version: 1', 'rules:', ' - glob: "**/.env*"', ' read: deny'].join('\n'), + ), + }); + // bypassPermissions cannot waive a contract deny, here as anywhere else. + const verdict = await gate({ tool: 'Read', input: { file_path: '/work/repo/.env' } }); + expect(verdict.allowed).toBe(false); + expect(verdict.reason).toMatch(/file contract/); + }); +}); diff --git a/packages/core/src/mcp/serve.ts b/packages/core/src/mcp/serve.ts index 5261e78..1d2c977 100644 --- a/packages/core/src/mcp/serve.ts +++ b/packages/core/src/mcp/serve.ts @@ -11,8 +11,67 @@ import { Server } from '@modelcontextprotocol/sdk/server/index.js'; import { StdioServerTransport } from '@modelcontextprotocol/sdk/server/stdio.js'; import { CallToolRequestSchema, ListToolsRequestSchema } from '@modelcontextprotocol/sdk/types.js'; +import { dispatchToolCall } from '../harness/tool-dispatcher.js'; import { BUILTIN_TOOLS } from '../tools/registry.js'; -import type { ToolContext, ToolHandler } from '../types.js'; +import type { FileContract } from '../config/file-contract.js'; +import type { AutoModeConfig, PermissionRules } from '../config/types.js'; +import type { HookDispatcher } from '../hooks/index.js'; +import type { Mode, ToolContext, ToolHandler } from '../types.js'; + +/** The verdict for one MCP tool call. `allowed: false` is returned to the peer. */ +export interface McpGateVerdict { + allowed: boolean; + reason: string; +} + +export type McpToolGate = (req: { + tool: string; + input: Record; +}) => Promise; + +export interface McpGateOptions { + cwd: string; + mode: Mode; + permissions?: PermissionRules; + contract?: FileContract; + hooks?: HookDispatcher; + autoMode?: AutoModeConfig; +} + +/** + * The gate every served tool call goes through. + * + * `mcp serve` hands Read/Write/Edit/Bash to whatever MCP client connected. There + * is no human on this side of the pipe, so `ask` cannot be asked and resolves to + * a refusal — the same fail-closed rule an unattended cron run follows. A peer + * that wants more has to be granted it in `settings.json`, where it is written + * down and reviewable, rather than by being the one who connected. + */ +export function buildMcpGate(options: McpGateOptions): McpToolGate { + return async ({ tool, input }) => { + const verdict = await dispatchToolCall({ + tool, + input, + mode: options.mode, + rules: options.permissions, + contract: options.contract, + hooks: options.hooks, + cwd: options.cwd, + autoMode: options.autoMode, + }); + if (verdict.decision === 'allow') return { allowed: true, reason: verdict.reason }; + if (verdict.decision === 'ask') { + return { + allowed: false, + reason: + `${verdict.reason}. \`deepcode mcp serve\` has no attached user, so a call needing ` + + `approval is refused rather than granted. Add a matching rule to ` + + `permissions.allow in settings.json, or start the server with an explicit --mode.`, + }; + } + return { allowed: false, reason: verdict.reason }; + }; +} /** Tools that can't run statelessly over MCP (need host-interactive context). */ export const MCP_SERVE_EXCLUDE = new Set([ @@ -42,6 +101,14 @@ export function mcpServableTools(tools: ToolHandler[] = BUILTIN_TOOLS): ToolHand export interface BuildMcpServerOpts { /** Project directory tools resolve relative paths against. */ cwd: string; + /** + * Policy gate for every call. **Required** — this server used to execute + * Read/Write/Edit/Bash for any connected peer with no mode, no permission + * rules, no file contract and no PreToolUse hooks. Making it required is the + * point: safety must not depend on a host remembering an optional argument. + * Build one with `buildMcpGate`. + */ + gate: McpToolGate; /** Override the served tool set (default: stateless BUILTIN_TOOLS). */ tools?: ToolHandler[]; name?: string; @@ -50,6 +117,8 @@ export interface BuildMcpServerOpts { signal?: AbortSignal; /** Optional sandbox config forwarded to the Bash tool. */ sandboxConfig?: ToolContext['sandboxConfig']; + /** Path-axis rules, so Grep/Glob filter their results here too. */ + contract?: FileContract; } /** @@ -81,16 +150,23 @@ export function buildMcpServer(opts: BuildMcpServerOpts): Server { isError: true, }; } + const input = (req.params.arguments ?? {}) as Record; + const verdict = await opts.gate({ tool: tool.name, input }); + if (!verdict.allowed) { + return { + content: [{ type: 'text' as const, text: `Refused: ${verdict.reason}` }], + isError: true, + }; + } + const ctx: ToolContext = { cwd: opts.cwd, signal: opts.signal, sandboxConfig: opts.sandboxConfig, + contract: opts.contract, }; try { - const result = await tool.execute( - (req.params.arguments ?? {}) as Record, - ctx, - ); + const result = await tool.execute(input, ctx); return { content: [{ type: 'text' as const, text: result.content }], isError: result.isError ?? false,