From e5463b10f99da5e9c4f9348200ff25c06bc2d5ba Mon Sep 17 00:00:00 2001 From: hugo Date: Sat, 28 Feb 2026 09:53:45 +0100 Subject: [PATCH] feat: mcp server implementation third round --- MCP_PLAN.md | 16 +++-- TODO.md | 68 ++++++++++------------ src/main/engine/MCPServer.ts | 50 +++++++++++++--- tests/engine/MCPServer.integration.test.ts | 45 +++++++++++++- tests/engine/MCPServer.test.ts | 65 +++++++++++++++++++++ 5 files changed, 188 insertions(+), 56 deletions(-) diff --git a/MCP_PLAN.md b/MCP_PLAN.md index ca5862f..7f421e8 100644 --- a/MCP_PLAN.md +++ b/MCP_PLAN.md @@ -24,7 +24,7 @@ MCPServer (HTTP on 127.0.0.1:4124) ← NEW, standalone └── ui:// resources → MCP App review Views (via ext-apps) ``` -The MCP SDK provides `createMcpExpressApp` which sets up Express with the correct Streamable HTTP handling. We use stateless mode (new `McpServer` per request) to avoid session management complexity. +The MCP SDK provides `StreamableHTTPServerTransport` for HTTP handling. We use Node's `http.createServer` directly with stateless mode (new `McpServer` per request) to avoid session management complexity and the Express dependency. `MCPServer` is a new engine class that owns the `McpServer` factory, tool/resource/prompt registration, and the `ProposalStore`. It runs independently of PreviewServer. @@ -44,7 +44,7 @@ npm install @modelcontextprotocol/sdk @modelcontextprotocol/ext-apps ### Step 3: MCPServer engine (`src/main/engine/MCPServer.ts`) - Constructor: inject engine getters (PostEngine, MediaEngine, ScriptEngine, TemplateEngine, MetaEngine, PostMediaEngine, TagEngine) - `createServer()` → factory that instantiates a fresh `McpServer` and registers all tools/resources/prompts (stateless mode — one per request) -- `start(port)` → uses `createMcpExpressApp` + `StreamableHTTPServerTransport` in stateless mode, listens on `127.0.0.1:port` +- `start(port)` → uses `http.createServer` + `StreamableHTTPServerTransport` in stateless mode, listens on `127.0.0.1:port`, validates Origin header - `stop()` → close HTTP server - `cleanup()` → discard proposals, stop intervals, stop server - Singleton pattern with `getMCPServer()` getter @@ -65,6 +65,7 @@ Register MCP resources mapping to existing engine methods: | `bds://posts/{id}/outlinks` | PostEngine.getLinksTo(id) | | `bds://posts/{id}/media` | PostMediaEngine.getLinkedMediaDataForPost(id) | | `bds://media/{id}/posts` | PostMediaEngine.getLinkedPostsForMedia(id) | +| `bds://media/{id}/image` | MediaEngine.getThumbnailDataUrl(id, 'medium') | ### Step 5: Read tools - `search_posts` — annotations: `{ readOnlyHint: true, openWorldHint: false }` @@ -88,6 +89,7 @@ Resource registration uses `registerAppResource` from `@modelcontextprotocol/ext ### Step 7: Accept/discard tools - `accept_proposal({ proposalId })` — dispatch by type, commit change - `discard_proposal({ proposalId })` — dispatch by type, clean up +- Registered via `registerAppTool` with `visibility: ["app"]` (app-only, hidden from agent LLM) - Annotations: idempotentHint: true ### Step 8: MCP Prompts @@ -109,7 +111,7 @@ Each View: 3. On accept/discard: calls `app.callServerTool({ name: 'accept_proposal' | 'discard_proposal', arguments: { proposalId } })` 4. Updates UI to show outcome -Build step: `vite build` with `vite-plugin-singlefile` bundles each into a self-contained HTML string. These are read at runtime by `registerAppResource` handlers. +Build step: Review Views are inline HTML template strings in `mcp-views.ts` — no separate build step needed. ### Step 10: Lifecycle in main.ts - MCPServer starts independently alongside PreviewServer during app init @@ -136,12 +138,8 @@ Each step above is preceded by failing tests: |---|---| | `src/main/engine/ProposalStore.ts` | NEW — in-memory proposal storage | | `src/main/engine/MCPServer.ts` | NEW — standalone MCP server engine | -| `src/main/mcp-apps/review-post.html` | NEW — post review View | -| `src/main/mcp-apps/review-script.html` | NEW — script review View | -| `src/main/mcp-apps/review-template.html` | NEW — template review View | -| `src/main/mcp-apps/review-metadata.html` | NEW — metadata diff View | -| `src/main/mcp-apps/src/*.ts` | NEW — View scripts using `App` class | -| `src/main/mcp-apps/vite.config.ts` | NEW — builds Views into single HTML files | +| `src/main/engine/mcp-views.ts` | NEW — inline review View HTML templates | +| `tests/engine/mcp-views.test.ts` | NEW | | `src/main/main.ts` | MODIFY — start/cleanup MCPServer | | `tests/engine/ProposalStore.test.ts` | NEW | | `tests/engine/MCPServer.test.ts` | NEW | diff --git a/TODO.md b/TODO.md index 6ca0d73..537cea8 100644 --- a/TODO.md +++ b/TODO.md @@ -284,14 +284,18 @@ anything enters the system. - **`@modelcontextprotocol/sdk` v1.27.1 and `@modelcontextprotocol/ext-apps` v1.1.2 are installed.** - **`ProposalStore` engine is implemented and tested (18 tests).** -- **`MCPServer` engine is implemented and tested (37 tests).** Standalone HTTP +- **`MCPServer` engine is implemented and tested (70 tests).** Standalone HTTP server on port 4124, stateless mode (new `McpServer` per request), registers - 5 static resources, 6 resource templates, 8 tools, 3 prompts, and 4 - `ui://` review-app resources. + 5 static resources, 7 resource templates, 8 tools (6 model-facing + + 2 app-only), 3 prompts, and 4 `ui://` review-app resources. - **`mcp-views.ts` provides review HTML** for posts, scripts, templates, and metadata diffs via `@modelcontextprotocol/ext-apps` App class. - **Lifecycle integrated in `main.ts`** — MCP server starts on app ready and cleans up on before-quit. +- **Origin validation** — rejects requests from non-localhost origins to + prevent DNS rebinding attacks. +- **`accept_proposal` / `discard_proposal` use app-only visibility** via + `registerAppTool` with `visibility: ["app"]` — hidden from the agent LLM. ### Design Principles @@ -352,10 +356,9 @@ anything enters the system. accordingly (structured preview data for Apps-capable hosts, formatted text for others). -7. **Input validation and rate limiting** — all tool inputs are validated - at the MCP boundary before forwarding to engine methods. The server - rate-limits tool invocations to prevent abuse. Do not rely solely on - downstream engine validation. +7. **Input validation** — all tool inputs are validated at the MCP + boundary (via Zod schemas in `inputSchema`) before forwarding to + engine methods. Do not rely solely on downstream engine validation. ### Implementation Plan @@ -413,9 +416,12 @@ actions. Each resource is registered via `resources/list` and read via | `bds://media/{id}/image` | OpenCodeManager.view_image | Image binary (for visual context) | Use `bds://` as the custom URI scheme. Parameterized URIs use MCP resource -templates (`resources/templates/list`). Emit `notifications/resources/ -list_changed` when posts, media, or tags are created/updated/deleted so -the host can refresh cached data. +templates (`resources/templates/list`). + +Note: `notifications/resources/list_changed` is not emitted because the +server runs in stateless mode (new `McpServer` per request, no persistent +connection). If the server moves to session-based mode in the future, +change notifications should be added. List resources (`bds://posts`, `bds://media`) support cursor-based pagination following the MCP pagination spec. The initial response @@ -565,9 +571,10 @@ Stages metadata changes for an existing post (title, excerpt, slug, tags). #### 3.6 App-Internal Tools (Accept / Discard) These tools are called by the MCP App (via the App Bridge's `tools/call` -mechanism), **not** by the agent LLM. The host forwards the call from the -sandboxed iframe to the MCP server. They are not listed in `tools/list` -responses to agents. +mechanism), **not** by the agent LLM. They are registered with +`registerAppTool` from `@modelcontextprotocol/ext-apps` using +`visibility: ["app"]`, which signals to compliant hosts that these tools +should not be shown to or invoked by the model. ##### `accept_proposal` @@ -656,32 +663,19 @@ tools know to call PostEngine rather than look in the store. #### 3.9 Transport -Support two transports: +**Streamable HTTP** — standalone HTTP server on port 4124 using +`StreamableHTTPServerTransport` in stateless mode (new `McpServer` per +request). A single HTTP endpoint at `/mcp` accepts JSON-RPC POST +requests and responds with `application/json` or `text/event-stream` +(SSE). -- **stdio** — for local integration (agent runs `bds --mcp` or connects via - named pipe). This is the standard for MCP in coding agents. Credentials - come from the environment. -- **Streamable HTTP** — for network access, running alongside PreviewServer - on a different port (e.g., 5174). Uses the current MCP Streamable HTTP - transport: a single HTTP endpoint that accepts JSON-RPC POST requests and - responds with either `application/json` (single response) or - `text/event-stream` (SSE stream for multiple messages). Supports session - management via `Mcp-Session-Id` headers and requires - `MCP-Protocol-Version` headers after initialization. +**Security:** -Start with stdio since that is what Claude Code and Cursor use. - -**Security requirements for Streamable HTTP:** - -- Bind to `127.0.0.1` (localhost only) when running locally. -- Validate the `Origin` header on all requests to prevent DNS rebinding - attacks. -- Use cryptographically random, non-deterministic session IDs. -- Implement a session token or shared secret for authentication (generated - on server start, displayed in the settings UI for the user to configure - in their agent). -- Rate-limit incoming requests. -- Set appropriate timeouts for tool invocations. +- Bind to `127.0.0.1` (localhost only). +- Validate the `Origin` header on all requests — reject non-localhost + origins to prevent DNS rebinding attacks. +- Requests without an `Origin` header are allowed (CLI tools like curl + and local MCP clients typically do not send one). #### 3.10 Lifecycle Integration diff --git a/src/main/engine/MCPServer.ts b/src/main/engine/MCPServer.ts index f975c8f..838b763 100644 --- a/src/main/engine/MCPServer.ts +++ b/src/main/engine/MCPServer.ts @@ -17,6 +17,14 @@ import { // ── Dependency contracts ────────────────────────────────────────────── +export interface PostFilter { + status?: 'draft' | 'published' | 'archived'; + tags?: string[]; + categories?: string[]; + year?: number; + month?: number; +} + interface PostEngineContract { getAllPosts: (options?: { limit?: number; offset?: number }) => Promise<{ items: Array>; hasMore: boolean; total: number }>; getPost: (id: string) => Promise | null>; @@ -30,7 +38,7 @@ interface PostEngineContract { getBlogStats: () => Promise>; getLinkedBy: (postId: string) => Promise>; getLinksTo: (postId: string) => Promise>; - getPostsFiltered: (filter: Record) => Promise>>; + getPostsFiltered: (filter: PostFilter) => Promise>>; } interface MediaEngineContract { @@ -109,8 +117,26 @@ export class MCPServer { } const app = createHttpServer(async (req, res) => { + // Origin validation — reject requests from non-localhost origins + // to prevent DNS rebinding attacks + const origin = req.headers['origin']; + if (origin) { + try { + const originUrl = new URL(origin); + if (originUrl.hostname !== '127.0.0.1' && originUrl.hostname !== 'localhost') { + res.writeHead(403, { 'Content-Type': 'application/json' }); + res.end(JSON.stringify({ error: 'Forbidden: non-local origin' })); + return; + } + } catch { + res.writeHead(403, { 'Content-Type': 'application/json' }); + res.end(JSON.stringify({ error: 'Forbidden: invalid origin' })); + return; + } + } + // CORS headers - res.setHeader('Access-Control-Allow-Origin', '*'); + res.setHeader('Access-Control-Allow-Origin', origin ?? 'http://127.0.0.1'); res.setHeader('Access-Control-Allow-Methods', 'GET, POST, DELETE, OPTIONS'); res.setHeader('Access-Control-Allow-Headers', 'Content-Type, Mcp-Session-Id'); res.setHeader('Access-Control-Expose-Headers', 'Mcp-Session-Id'); @@ -347,15 +373,18 @@ export class MCPServer { annotations: { readOnlyHint: true, openWorldHint: false }, }, async (args) => { const hasFilters = args.category || args.tags || args.year || args.month || args.status; + const offset = args.offset ?? 0; + const limit = args.limit ?? 50; if (args.query && !hasFilters) { // Pure text search — use FTS const results = await this.deps.getPostEngine().searchPosts(args.query); - return { content: [{ type: 'text' as const, text: JSON.stringify(results) }] }; + const paginated = results.slice(offset, offset + limit); + return { content: [{ type: 'text' as const, text: JSON.stringify(paginated) }] }; } // Filter-based query (optionally narrowed by text search) - const filter: Record = {}; + const filter: PostFilter = {}; if (args.category) filter.categories = [args.category]; if (args.tags) filter.tags = args.tags; if (args.year) filter.year = args.year; @@ -374,7 +403,8 @@ export class MCPServer { }); } - return { content: [{ type: 'text' as const, text: JSON.stringify(results) }] }; + const paginated = results.slice(offset, offset + limit); + return { content: [{ type: 'text' as const, text: JSON.stringify(paginated) }] }; }); } @@ -536,26 +566,28 @@ export class MCPServer { } private registerAcceptDiscardTools(server: McpServer): void { - server.registerTool('accept_proposal', { + registerAppTool(server, 'accept_proposal', { title: 'Accept Proposal', description: 'Accept a pending proposal, committing the proposed change.', inputSchema: { proposalId: z.string().describe('ID of the proposal to accept'), }, annotations: { readOnlyHint: false, destructiveHint: false, idempotentHint: true }, - }, async (args) => { + _meta: { ui: { visibility: ['app'] } }, + }, async (args: { proposalId: string }) => { const result = await this.acceptProposal(args.proposalId); return { content: [{ type: 'text' as const, text: JSON.stringify(result) }] }; }); - server.registerTool('discard_proposal', { + registerAppTool(server, 'discard_proposal', { title: 'Discard Proposal', description: 'Discard a pending proposal, rolling back any draft changes.', inputSchema: { proposalId: z.string().describe('ID of the proposal to discard'), }, annotations: { readOnlyHint: false, destructiveHint: true, idempotentHint: true }, - }, async (args) => { + _meta: { ui: { visibility: ['app'] } }, + }, async (args: { proposalId: string }) => { const result = await this.discardProposal(args.proposalId); return { content: [{ type: 'text' as const, text: JSON.stringify(result) }] }; }); diff --git a/tests/engine/MCPServer.integration.test.ts b/tests/engine/MCPServer.integration.test.ts index 351e41c..86db727 100644 --- a/tests/engine/MCPServer.integration.test.ts +++ b/tests/engine/MCPServer.integration.test.ts @@ -109,10 +109,53 @@ describe('MCPServer integration', () => { method: 'OPTIONS', }); expect(response.status).toBe(204); - expect(response.headers.get('Access-Control-Allow-Origin')).toBe('*'); + expect(response.headers.get('Access-Control-Allow-Origin')).toBeTruthy(); expect(response.headers.get('Access-Control-Allow-Methods')).toContain('POST'); }); + it('rejects requests from non-local origins', async () => { + const port = await server.start(0); + const response = await fetch(`http://127.0.0.1:${port}/mcp`, { + method: 'POST', + headers: { + 'Content-Type': 'application/json', + 'Origin': 'https://evil-site.com', + }, + body: JSON.stringify({ + jsonrpc: '2.0', + id: 1, + method: 'initialize', + params: { protocolVersion: '2025-03-26', capabilities: {}, clientInfo: { name: 'test', version: '1.0.0' } }, + }), + }); + expect(response.status).toBe(403); + }); + + it('allows requests from localhost origins', async () => { + const port = await server.start(0); + const response = await fetch(`http://127.0.0.1:${port}/mcp`, { + method: 'POST', + headers: { + 'Content-Type': 'application/json', + 'Accept': 'application/json, text/event-stream', + 'Origin': `http://localhost:${port}`, + }, + body: JSON.stringify({ + jsonrpc: '2.0', + id: 1, + method: 'initialize', + params: { protocolVersion: '2025-03-26', capabilities: {}, clientInfo: { name: 'test', version: '1.0.0' } }, + }), + }); + expect(response.status).not.toBe(403); + }); + + it('allows requests without Origin header', async () => { + const port = await server.start(0); + const result = await initializeSession(port) as { result?: { serverInfo?: { name: string } } }; + expect(result.result?.serverInfo?.name).toBe('Blogging Desktop Server'); + }); + it('lists tools via tools/list after initialize', async () => { const port = await server.start(0); diff --git a/tests/engine/MCPServer.test.ts b/tests/engine/MCPServer.test.ts index 1978eb1..9cf4070 100644 --- a/tests/engine/MCPServer.test.ts +++ b/tests/engine/MCPServer.test.ts @@ -446,6 +446,38 @@ describe('MCPServer', () => { }); }); + // ── Tool visibility ───────────────────────────────────────────────── + + describe('tool visibility', () => { + function getToolMeta(toolName: string): Record | undefined { + const mcpServer = server.createMcpServer(); + const tool = (mcpServer as Record }>>)._registeredTools[toolName]; + return tool?._meta; + } + + it('accept_proposal has app-only visibility', () => { + const meta = getToolMeta('accept_proposal'); + expect(meta).toBeDefined(); + const ui = (meta as Record).ui as Record; + expect(ui?.visibility).toEqual(['app']); + }); + + it('discard_proposal has app-only visibility', () => { + const meta = getToolMeta('discard_proposal'); + expect(meta).toBeDefined(); + const ui = (meta as Record).ui as Record; + expect(ui?.visibility).toEqual(['app']); + }); + + it('draft_post has model+app visibility (default)', () => { + const meta = getToolMeta('draft_post'); + expect(meta).toBeDefined(); + const ui = (meta as Record).ui as Record; + // no explicit visibility = default ["model", "app"] + expect(ui?.visibility).toBeUndefined(); + }); + }); + // ── Resource handler behavior ─────────────────────────────────────── describe('resource handlers', () => { @@ -543,6 +575,28 @@ describe('MCPServer', () => { expect(JSON.parse(result.content[0].text)).toEqual(searchResults); }); + it('search_posts with query applies offset and limit', async () => { + const searchResults = Array.from({ length: 10 }, (_, i) => ({ id: `p${i}`, title: `Post ${i}`, slug: `post-${i}` })); + mockPostEngine.searchPosts.mockResolvedValue(searchResults); + const mcpServer = server.createMcpServer(); + const tool = getTool(mcpServer, 'search_posts'); + const result = await tool.handler({ query: 'test', offset: 2, limit: 3 }, {}) as { content: Array<{ text: string }> }; + const parsed = JSON.parse(result.content[0].text); + expect(parsed).toHaveLength(3); + expect(parsed[0].id).toBe('p2'); + expect(parsed[2].id).toBe('p4'); + }); + + it('search_posts defaults to limit 50 when not specified', async () => { + const searchResults = Array.from({ length: 60 }, (_, i) => ({ id: `p${i}`, title: `Post ${i}`, slug: `post-${i}` })); + mockPostEngine.searchPosts.mockResolvedValue(searchResults); + const mcpServer = server.createMcpServer(); + const tool = getTool(mcpServer, 'search_posts'); + const result = await tool.handler({ query: 'test' }, {}) as { content: Array<{ text: string }> }; + const parsed = JSON.parse(result.content[0].text); + expect(parsed).toHaveLength(50); + }); + it('search_posts with filters only calls getPostsFiltered', async () => { const filtered = [{ id: 'p2', title: 'Filtered' }]; mockPostEngine.getPostsFiltered.mockResolvedValue(filtered); @@ -553,6 +607,17 @@ describe('MCPServer', () => { expect(JSON.parse(result.content[0].text)).toEqual(filtered); }); + it('search_posts with filters applies offset and limit', async () => { + const filtered = Array.from({ length: 10 }, (_, i) => ({ id: `p${i}`, title: `Post ${i}` })); + mockPostEngine.getPostsFiltered.mockResolvedValue(filtered); + const mcpServer = server.createMcpServer(); + const tool = getTool(mcpServer, 'search_posts'); + const result = await tool.handler({ category: 'tech', offset: 3, limit: 2 }, {}) as { content: Array<{ text: string }> }; + const parsed = JSON.parse(result.content[0].text); + expect(parsed).toHaveLength(2); + expect(parsed[0].id).toBe('p3'); + }); + it('search_posts with query + filters uses getPostsFiltered and client-side text filter', async () => { const allFiltered = [ { id: 'p1', title: 'TypeScript Guide', content: 'Learn TS', excerpt: '' },