From 4ff8057cd6362292879ea29120c9cb1fadb270ce Mon Sep 17 00:00:00 2001 From: bntvllnt <32437578+bntvllnt@users.noreply.github.com> Date: Mon, 27 Apr 2026 21:34:25 +0200 Subject: [PATCH 1/4] feat(hooks): add extendArgs + reserved \`_\` prefix for context propagation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes #15, #16, #17. Closes part of #18 (security.md + llms.txt updated; api-reference.md, examples.md, llms-full.txt deferred to a docs follow-up). ## What Two coupled additions enabling per-tool server-side context injection: 1. **extendArgs (#16)** — \`OnCallResult\` gains an optional \`extendArgs?: Record\` field. When the \`onToolCall\` hook returns it during the \`before\` phase, the framework merges the key/value pairs into args before \`client[type](toolDef.ref, args)\`. On collision: server-side wins. Empty / undefined is a no-op. 2. **Reserved \`_\` prefix (#17)** — Arg keys starting with underscore are framework-controlled. Two layers protect them: - \`prepareTools\` strips \`_*\` keys from the published JSON Schema (\`zodShape\`). The MCP SDK's Zod validator silently strips \`_*\` keys from incoming requests via Zod's default strip mode. - The registered tool handler explicitly rejects any \`_*\` key reaching it with a structured "Reserved arg keys not allowed" error (defense-in-depth for non-SDK transports). Hook-supplied \`_*\` keys via \`extendArgs\` are exempt — server is trusted. ## Why Today the \`onToolCall\` hook receives \`apiKey\` but cannot pass it (or any server-resolved context) downstream. Per-action authorization, request tracing, multi-tenancy, audit metadata, per-key feature flags, and per-key quotas all require server-injected context. Without this primitive, consumers either fork the framework or replace the dispatch transport. This is the framework primitive — not coupled to apiKey, not coupled to auth at all. \`extendArgs\` carries arbitrary server-resolved key/value pairs; the consumer decides what to put there. See \`docs/security.md\` for 4 example patterns. ## Backward compatibility Strictly additive: - \`OnCallResult\` gets a new optional field. Existing hooks unchanged. - \`prepareTools\` strips \`_*\` from the published shape. Consumers who publish \`_*\` keys today get them removed from the schema (verify zero impact: search across vllnt-owned consumers turned up zero). - The handler-level \`_*\` reject is new behavior. Any consumer accepting \`_*\` request args today (none expected) would now get a structured rejection instead. ## Tests 12 new tests in \`tests/context-propagation.test.ts\`: - merges extendArgs into dispatched args (query, mutation, action) - server-side wins on key collision - undefined / empty extendArgs is a no-op (no allocation) - abort wins over extendArgs - only \`before\` phase honors extendArgs - handler rejects \`_*\` request args before hook runs - handler error message lists every reserved key - hook-supplied \`_*\` via extendArgs bypasses reject (server trusted) - only top-level keys are rejected (nested \`_*\` passes through) - published JSON Schema strips \`_*\` properties + required - end-to-end: SDK schema-strip + extendArgs injection produces trusted args at the dispatched function All 127 tests pass (115 existing + 12 new). Lint + typecheck clean. --- CHANGELOG.md | 8 +- docs/security.md | 72 +++++ llms.txt | 8 + src/tools/register.ts | 53 +++- src/tools/types.ts | 32 ++ tests/context-propagation.test.ts | 499 ++++++++++++++++++++++++++++++ 6 files changed, 667 insertions(+), 5 deletions(-) create mode 100644 tests/context-propagation.test.ts diff --git a/CHANGELOG.md b/CHANGELOG.md index 9fbc9ae..b78c095 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,7 +5,13 @@ All notable changes to this project will be documented in this file. The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/), and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). -## [0.3.0] - 2026-04-25 +## [0.3.0] - Unreleased + +### Added + +- **Hook-driven request context propagation** ([#15](https://github.com/vllnt/convex-mcp/issues/15)) — The `onToolCall` hook can now return `extendArgs?: Record` to merge server-resolved context into the dispatched function's args. Unblocks per-action authorization, request tracing, multi-tenancy, audit metadata, per-key feature flags, and any pattern where the server needs to inject trusted context the action can read ([#16](https://github.com/vllnt/convex-mcp/issues/16)) +- **Reserved `_` prefix for tool args** ([#17](https://github.com/vllnt/convex-mcp/issues/17)) — Arg keys starting with `_` are framework-controlled. Two layers protect them: (1) `prepareTools` strips `_*` keys from the published JSON Schema so MCP clients can't see or pass them; (2) the registered tool handler explicitly rejects any `_*` key that arrives at the handler boundary (defense-in-depth for non-SDK callers). Hook-supplied `_*` keys via `extendArgs` are exempt +- 12 new tests in `tests/context-propagation.test.ts` covering merge precedence, no-op cases (undefined / empty `extendArgs`), abort precedence, phase isolation (only `before` honors `extendArgs`), schema stripping, reserved-key rejection at handler boundary, nested-key passthrough, and end-to-end SDK schema-strip behavior ### Changed diff --git a/docs/security.md b/docs/security.md index 4917e45..3412c25 100644 --- a/docs/security.md +++ b/docs/security.md @@ -184,3 +184,75 @@ An attacker with a valid API key could flood the endpoint with tool calls. - Rate limit at the infrastructure level. - Implement per-key rate limiting in `auth.validate`. - Monitor Convex function call volume and costs. + +## Server-side Context Propagation (v0.3.0) + +The `onToolCall` hook can inject server-resolved context into the dispatched function's args via `extendArgs`. This enables per-action authorization, tracing, multi-tenancy, audit metadata, and other patterns that need trusted server-side state inside the handler. + +### Defense in depth: hook + handler re-validation + +The framework hook is the fast path. The action handler re-validating is the safety net. Even if the hook is bypassed (regression, fork, or someone bypasses the framework entirely and calls the Convex function directly), the action denies the call. + +```ts +// convex/mcp.ts — server config +hooks: { + onToolCall: async ({ apiKey, phase, toolDef }) => { + if (phase !== "before") return; + const result = await validateKey(apiKey); + if (!result.valid) return { abort: true, errorMessage: "Invalid key" }; + const requiredScope = toolDef.tags?.requiredScope; + if (requiredScope && !result.scopes.includes(requiredScope)) { + return { abort: true, errorMessage: `Missing scope: ${requiredScope}` }; + } + return { + extendArgs: { + _mcp_apiKey: apiKey, + _mcp_scopes: result.scopes, + }, + }; + }, +} + +// convex/things.ts — Convex action with re-validation +export const upsertThing = action({ + args: { + _mcp_apiKey: v.string(), // injected by framework, required + _mcp_scopes: v.array(v.string()), + targetId: v.id("things"), + }, + handler: async (ctx, args) => { + // Defense-in-depth: re-validate on the action side + await assertMcpAuth(ctx, args._mcp_apiKey, "things:write"); + // ...real logic with confidence the caller is authorized... + }, +}); +``` + +### Reserved `_` prefix — non-negotiable + +Arg keys starting with `_` are framework-controlled. The framework protects them with two layers: + +1. **Schema strip** — `prepareTools` removes `_*` keys from the published JSON Schema. MCP clients see only consumer-defined args. The MCP SDK's Zod validator silently strips `_*` keys from incoming requests (Zod default strip mode), so spoofed values never reach the handler. +2. **Handler reject** — If a request reaches the registered tool handler with `_*` keys (e.g., a custom transport that bypasses the SDK's schema validation), the handler returns a structured "Reserved arg keys not allowed" error before the hook runs. + +Hook-supplied `_*` keys via `extendArgs` are exempt from both layers — the server is trusted. + +### Other patterns + +| Pattern | Hook returns | Action receives | +|---|---|---| +| **Request tracing** | `{ extendArgs: { _mcp_requestId: ctx.requestId } }` | Threads framework's `requestId` into action audit logs | +| **Multi-tenancy** | `{ extendArgs: { _mcp_tenantId: resolveTenant(apiKey) } }` | Server-resolved tenant ID enforced via `withIndex("by_tenant", q => q.eq("tenantId", args._mcp_tenantId))` — caller cannot spoof | +| **Per-key feature flags** | `{ extendArgs: { _mcp_flags: getFlagsFor(apiKey) } }` | Branches behavior per caller without re-fetching from DB | +| **Audit metadata** | `{ extendArgs: { _mcp_callerLabel: keyMetadata.label } }` | Recorded in domain audit log | + +### Anti-patterns (DON'T) + +- **Don't put long-lived secrets in `extendArgs`** — values flow into args, may end up in action logs, may be over-shared. Only inject context that's safe to log. +- **Don't rely solely on the hook for authorization** — without action-side re-validation, a framework regression or bypass becomes a security blind spot. +- **Don't spread context across `extendArgs` AND a side-channel store** — pick one. The args path is canonical. +- **Don't allow client `_*` passthrough** — the reject is what makes `extendArgs` safe. Don't disable it. + +### Override semantics on key collision + +When `extendArgs` and request args share a key, **`extendArgs` wins**. This matters when the consumer chooses to override request data with server-resolved values (e.g., `tenantId` resolved server-side overrides any caller-supplied tenantId). For `_*` keys this is moot — they can't appear in request args. diff --git a/llms.txt b/llms.txt index 1b0bc47..348a46b 100644 --- a/llms.txt +++ b/llms.txt @@ -58,6 +58,14 @@ export const { GET, POST } = mcp.handler(); - `tools/describe` with `{ name }` returns full tool definition on-demand - Cursors are HMAC-signed, constant-time verified, per-instance secret +## Request Context Propagation (v0.3.0) + +- `OnCallResult.extendArgs` — Hook returns `{ extendArgs: { _key: value, ... } }` to merge server-resolved context into the dispatched function's args. Server-side wins on collision. Only `before` phase honors it. Empty / undefined is a no-op +- **Reserved `_` prefix** — Arg keys starting with `_` are framework-controlled. Stripped from the published JSON Schema (clients can't see/pass them). Handler explicitly rejects any `_*` key reaching it (defense-in-depth) +- **Defense-in-depth pattern** — Hook validates apiKey + scope (fast path) AND the action handler re-validates `args._mcp_apiKey` (safety net). Even if the framework hook is bypassed, action stays safe +- **Use cases** — auth context, request tracing IDs, tenant routing, audit metadata, per-key feature flags, per-key quotas +- See `docs/security.md` for production patterns + ## Docs - [Getting Started](docs/getting-started.md) diff --git a/src/tools/register.ts b/src/tools/register.ts index 0b8a46e..5a1d61a 100644 --- a/src/tools/register.ts +++ b/src/tools/register.ts @@ -11,13 +11,38 @@ interface PreparedTool { toolDef: ToolDef; } +/** + * Underscore-prefixed arg keys are reserved for framework-injected context. + * + * - Request args containing `_*` keys are rejected before hook invocation. + * - Tool-level Convex validators MAY declare `_*` fields so the action + * handler receives server-injected values; these are stripped from the + * published JSON Schema so MCP clients neither see nor pass them. + */ +function isReservedKey(key: string): boolean { + return key.startsWith("_"); +} + +function stripReservedFromShape( + shape: Record, +): Record { + const filtered: Record = {}; + for (const [key, value] of Object.entries(shape)) { + if (!isReservedKey(key)) { + filtered[key] = value; + } + } + return filtered; +} + export function prepareTools(tools: Record): PreparedTool[] { return Object.entries(tools).map(([name, toolDef]) => { const zodSchema = toolDef.args ? convexArgsToZod(toolDef.args) : undefined; + const fullShape = zodSchema?.shape ?? {}; return { name, description: toolDef.description ?? "", - zodShape: zodSchema?.shape ?? {}, + zodShape: stripReservedFromShape(fullShape), toolDef, }; }); @@ -81,6 +106,21 @@ export function registerTools( const startedAt = Date.now(); const { ref: _ref, onError: _onError, ...safeDef } = toolDef; + const reservedKeys = Object.keys(args).filter(isReservedKey); + if (reservedKeys.length > 0) { + return { + content: [ + { + type: "text" as const, + text: + `Reserved arg keys not allowed in request: ${reservedKeys.join(", ")}. ` + + `Keys starting with "_" are reserved for framework-injected context.`, + }, + ], + isError: true, + }; + } + const baseCtx = { requestId, toolName: name, @@ -99,15 +139,20 @@ export function registerTools( }; } + const dispatchArgs = + beforeResult?.extendArgs && Object.keys(beforeResult.extendArgs).length > 0 + ? { ...args, ...beforeResult.extendArgs } + : args; + try { const callPromise = (async () => { switch (toolDef.type) { case "query": - return await client.query(toolDef.ref, args); + return await client.query(toolDef.ref, dispatchArgs); case "mutation": - return await client.mutation(toolDef.ref, args); + return await client.mutation(toolDef.ref, dispatchArgs); case "action": - return await client.action(toolDef.ref, args); + return await client.action(toolDef.ref, dispatchArgs); default: throw new Error(`Unknown function type: ${String(toolDef.type)}`); } diff --git a/src/tools/types.ts b/src/tools/types.ts index b687104..226137b 100644 --- a/src/tools/types.ts +++ b/src/tools/types.ts @@ -35,6 +35,38 @@ export interface OnCallResult { errorMessage?: string; /** Custom error message on failure (error phase). Default: "Function execution failed" */ message?: string; + /** + * Server-resolved key/value pairs merged into the dispatched function's args + * before the Convex call. Only honored during the "before" phase. + * + * On key collision, hook-supplied values take precedence over request args + * (server-side wins). + * + * Convention: keys MUST use the framework-reserved underscore prefix + * (e.g., `_mcp_apiKey`, `_request_id`). Request args containing `_`-prefixed + * keys are rejected before the hook runs, so this prefix is safe for + * server-side context injection (auth, tracing, multi-tenancy, audit, etc.). + * + * Empty objects and undefined are no-ops (no allocation, no merge). + * + * @example + * ```ts + * hooks: { + * onToolCall: async ({ apiKey, phase }) => { + * if (phase !== "before") return; + * const validated = await validateKey(apiKey); + * if (!validated.valid) return { abort: true, errorMessage: "Invalid key" }; + * return { + * extendArgs: { + * _mcp_apiKey: apiKey, + * _mcp_scopes: validated.scopes, + * }, + * }; + * }, + * } + * ``` + */ + extendArgs?: Record; } export interface LifecycleHooks { diff --git a/tests/context-propagation.test.ts b/tests/context-propagation.test.ts new file mode 100644 index 0000000..e1fd8f5 --- /dev/null +++ b/tests/context-propagation.test.ts @@ -0,0 +1,499 @@ +/** + * Tests for hook-driven request context propagation (issues #16 + #17). + * + * - extendArgs: hook injects server-resolved context into dispatched args. + * - Reserved `_` prefix: framework rejects client-supplied `_*` keys to + * prevent caller spoofing of server-injected fields. + * - Schema stripping: `_*` fields are removed from the published JSON Schema + * so MCP clients neither see nor pass them. + */ + +import { beforeEach, describe, expect, it, vi } from "vitest"; +import { createMCPServer } from "../src/server.js"; +import { action, mutation, query } from "../src/tools/helpers.js"; +import type { CallContext, ConvexValidator } from "../src/types.js"; + +const MOCK_CONVEX_URL = "https://test-deployment.convex.cloud"; + +function makeValidator(kind: string, extra: Record = {}): ConvexValidator { + return { kind, isOptional: "required", ...extra } as ConvexValidator; +} + +vi.mock("convex/browser", () => { + const mockClient = { + query: vi.fn().mockResolvedValue({ ok: true }), + mutation: vi.fn().mockResolvedValue({ ok: true }), + action: vi.fn().mockResolvedValue({ ok: true }), + setAuth: vi.fn(), + }; + return { + ConvexHttpClient: vi.fn(function MockConvexHttpClient() { + return mockClient; + }), + __mockClient: mockClient, + }; +}); + +async function getMockClient() { + const mod = (await import("convex/browser")) as unknown as { + __mockClient: { + query: ReturnType; + mutation: ReturnType; + action: ReturnType; + setAuth: ReturnType; + }; + }; + return mod.__mockClient; +} + +beforeEach(async () => { + const mockClient = await getMockClient(); + mockClient.query.mockReset().mockResolvedValue({ ok: true }); + mockClient.mutation.mockReset().mockResolvedValue({ ok: true }); + mockClient.action.mockReset().mockResolvedValue({ ok: true }); + mockClient.setAuth.mockReset(); +}); + +function mcpRequest(method: string, params: Record = {}, id: number = 1) { + return new Request("http://localhost/mcp", { + method: "POST", + headers: { + "Content-Type": "application/json", + Accept: "application/json, text/event-stream", + Authorization: "Bearer test-key", + }, + body: JSON.stringify({ jsonrpc: "2.0", id, method, params }), + }); +} + +async function parseSSE(response: Response): Promise<{ result?: { isError?: boolean; content: { text: string }[] } }> { + const text = await response.text(); + const dataLine = text.split("\n").find((line) => line.startsWith("data: ")); + if (!dataLine) throw new Error(`No data line in SSE response: ${text}`); + return JSON.parse(dataLine.slice(6)); +} + +describe("context propagation: extendArgs", () => { + it("merges hook-supplied extendArgs into dispatched args (query)", async () => { + const server = createMCPServer({ + auth: { validate: async () => true }, + convexUrl: MOCK_CONVEX_URL, + hooks: { + onToolCall: async ({ phase, apiKey }) => { + if (phase !== "before") return; + return { extendArgs: { _mcp_apiKey: apiKey, _mcp_scope: "read" } }; + }, + }, + tools: { + list: query(null, { + args: makeValidator("object", { + fields: { + userArg: makeValidator("string"), + _mcp_apiKey: makeValidator("string"), + _mcp_scope: makeValidator("string"), + }, + }), + description: "List", + }), + }, + }); + + const mockClient = await getMockClient(); + const response = await server + .handler() + .POST(mcpRequest("tools/call", { name: "list", arguments: { userArg: "hello" } })); + await response.text(); + + expect(mockClient.query).toHaveBeenCalledTimes(1); + expect(mockClient.query).toHaveBeenCalledWith(null, { + userArg: "hello", + _mcp_apiKey: "test-key", + _mcp_scope: "read", + }); + }); + + it("merges extendArgs for mutations and actions too", async () => { + const mockClient = await getMockClient(); + + const buildServer = (toolType: "query" | "mutation" | "action") => + createMCPServer({ + auth: { validate: async () => true }, + convexUrl: MOCK_CONVEX_URL, + hooks: { + onToolCall: async ({ phase }) => { + if (phase !== "before") return; + return { extendArgs: { _injected: "ctx" } }; + }, + }, + tools: { + tool: (toolType === "query" ? query : toolType === "mutation" ? mutation : action)(null, { + args: makeValidator("object", { + fields: { x: makeValidator("string"), _injected: makeValidator("string") }, + }), + description: "tool", + }), + }, + }); + + for (const t of ["query", "mutation", "action"] as const) { + mockClient[t].mockClear(); + const server = buildServer(t); + await server.handler().POST(mcpRequest("tools/call", { name: "tool", arguments: { x: "v" } })).then((r) => r.text()); + expect(mockClient[t]).toHaveBeenCalledWith(null, { x: "v", _injected: "ctx" }); + } + }); + + it("extendArgs takes precedence over request args on key collision (server-side wins)", async () => { + // Note: only possible when the colliding key does NOT start with `_` (those + // are rejected upfront). Hook-wins matters for non-reserved keys when a + // consumer chooses to override request data — uncommon but supported. + const server = createMCPServer({ + auth: { validate: async () => true }, + convexUrl: MOCK_CONVEX_URL, + hooks: { + onToolCall: async ({ phase }) => { + if (phase !== "before") return; + return { extendArgs: { tenantId: "server-resolved" } }; + }, + }, + tools: { + list: query(null, { + args: makeValidator("object", { fields: { tenantId: makeValidator("string") } }), + description: "list", + }), + }, + }); + + const mockClient = await getMockClient(); + await server + .handler() + .POST(mcpRequest("tools/call", { name: "list", arguments: { tenantId: "client-supplied" } })) + .then((r) => r.text()); + + expect(mockClient.query).toHaveBeenCalledWith(null, { tenantId: "server-resolved" }); + }); + + it("undefined / empty extendArgs is a no-op", async () => { + const cases: Array<{ label: string; extendArgs: Record | undefined }> = [ + { label: "undefined", extendArgs: undefined }, + { label: "empty", extendArgs: {} }, + ]; + + for (const { extendArgs } of cases) { + const mockClient = await getMockClient(); + mockClient.query.mockClear(); + + const server = createMCPServer({ + auth: { validate: async () => true }, + convexUrl: MOCK_CONVEX_URL, + hooks: { + onToolCall: async ({ phase }) => { + if (phase !== "before") return; + return extendArgs === undefined ? undefined : { extendArgs }; + }, + }, + tools: { + list: query(null, { + args: makeValidator("object", { fields: { x: makeValidator("string") } }), + description: "list", + }), + }, + }); + + await server + .handler() + .POST(mcpRequest("tools/call", { name: "list", arguments: { x: "v" } })) + .then((r) => r.text()); + + expect(mockClient.query).toHaveBeenCalledWith(null, { x: "v" }); + } + }); + + it("abort wins over extendArgs — dispatch never happens", async () => { + const server = createMCPServer({ + auth: { validate: async () => true }, + convexUrl: MOCK_CONVEX_URL, + hooks: { + onToolCall: async ({ phase }) => { + if (phase !== "before") return; + return { abort: true, errorMessage: "blocked", extendArgs: { _x: "ignored" } }; + }, + }, + tools: { + list: query(null, { + args: makeValidator("object", { fields: {} }), + description: "list", + }), + }, + }); + + const mockClient = await getMockClient(); + const data = await parseSSE( + await server.handler().POST(mcpRequest("tools/call", { name: "list", arguments: {} })), + ); + expect(data.result?.isError).toBe(true); + expect(data.result?.content[0]?.text).toBe("blocked"); + expect(mockClient.query).not.toHaveBeenCalled(); + }); + + it("error and success phases ignore extendArgs (only `before` honors it)", async () => { + const seen: CallContext[] = []; + const server = createMCPServer({ + auth: { validate: async () => true }, + convexUrl: MOCK_CONVEX_URL, + hooks: { + onToolCall: async (ctx) => { + seen.push({ ...ctx }); + // Returning extendArgs from non-before phases is allowed but ignored. + return { extendArgs: { _phase: ctx.phase } }; + }, + }, + tools: { + list: query(null, { + args: makeValidator("object", { fields: {} }), + description: "list", + }), + }, + }); + + const mockClient = await getMockClient(); + await server + .handler() + .POST(mcpRequest("tools/call", { name: "list", arguments: {} })) + .then((r) => r.text()); + + // Hook fired in "before" + "success"; only "before" affected dispatch. + const phases = seen.map((c) => c.phase); + expect(phases).toContain("before"); + expect(phases).toContain("success"); + expect(mockClient.query).toHaveBeenCalledWith(null, { _phase: "before" }); + }); +}); + +describe("context propagation: reserved `_` prefix rejection", () => { + /** + * Defense in depth: there are two layers that prevent caller-supplied `_*` args. + * + * 1. Schema layer — `_*` keys are stripped from the published JSON Schema + * (covered by the "schema stripping" suite). The MCP SDK then rejects + * unknown args at JSON-RPC parse time, BEFORE our handler runs. + * 2. Handler layer — even if a caller bypasses the SDK and reaches our + * handler with `_*` keys (custom transport, bypassed validation), the + * framework rejects with a clear "Reserved arg keys not allowed" + * message. This is what these tests cover by invoking the registered + * tool callback directly. + */ + it("handler rejects `_*` args before hook runs (defense-in-depth)", async () => { + const { registerTools, prepareTools } = await import("../src/tools/register.js"); + type ToolHandler = (args: Record) => Promise<{ isError?: boolean; content: { text: string }[] }>; + + const mockClient = await getMockClient(); + const hookSpy = vi.fn(); + const captured: { handler?: ToolHandler } = {}; + const fakeServer = { + tool: (_name: string, _desc: string, _shape: unknown, h: ToolHandler) => { + captured.handler = h; + }, + }; + + const prepared = prepareTools({ + list: query(null, { + args: makeValidator("object", { fields: { _mcp_apiKey: makeValidator("string") } }), + description: "list", + }), + }); + registerTools( + fakeServer as unknown as Parameters[0], + mockClient as unknown as Parameters[1], + prepared, + { onToolCall: async (ctx) => { hookSpy(ctx.phase); } }, + "test-request-id", + "test-key", + ); + + const handler = captured.handler; + if (!handler) throw new Error("registerTools did not register a handler"); + const result = await handler({ _mcp_apiKey: "spoofed" }); + + expect(result.isError).toBe(true); + expect(result.content[0]?.text).toContain("Reserved arg keys not allowed"); + expect(result.content[0]?.text).toContain("_mcp_apiKey"); + expect(hookSpy).not.toHaveBeenCalled(); + expect(mockClient.query).not.toHaveBeenCalled(); + }); + + it("handler error message lists every reserved key supplied", async () => { + const { registerTools, prepareTools } = await import("../src/tools/register.js"); + type ToolHandler = (args: Record) => Promise<{ isError?: boolean; content: { text: string }[] }>; + + const mockClient = await getMockClient(); + const captured: { handler?: ToolHandler } = {}; + const fakeServer = { + tool: (_name: string, _desc: string, _shape: unknown, h: ToolHandler) => { + captured.handler = h; + }, + }; + + const prepared = prepareTools({ + list: query(null, { args: makeValidator("object", { fields: {} }), description: "list" }), + }); + registerTools( + fakeServer as unknown as Parameters[0], + mockClient as unknown as Parameters[1], + prepared, + undefined, + "test-request-id", + "test-key", + ); + + const handler = captured.handler; + if (!handler) throw new Error("registerTools did not register a handler"); + const result = await handler({ _a: 1, _b: 2, ok: 3 }); + + expect(result.isError).toBe(true); + expect(result.content[0]?.text).toContain("_a"); + expect(result.content[0]?.text).toContain("_b"); + expect(result.content[0]?.text).not.toMatch(/\bok\b/); + }); + + it("MCP SDK schema-strip (layer 1): client-supplied `_*` keys never reach the dispatched action", async () => { + // The MCP SDK validates request args against the published JSON Schema. + // Since `prepareTools` strips `_*` from the published shape, the SDK's + // Zod validator silently drops `_*` keys from incoming args (Zod default + // strip mode). The dispatched call therefore never carries the spoofed + // value — which is exactly the security property we want. + const server = createMCPServer({ + auth: { validate: async () => true }, + convexUrl: MOCK_CONVEX_URL, + hooks: { + onToolCall: async ({ phase }) => { + if (phase !== "before") return; + return { extendArgs: { _mcp_apiKey: "server-validated" } }; + }, + }, + tools: { + list: query(null, { + args: makeValidator("object", { + fields: { + userArg: makeValidator("string"), + _mcp_apiKey: makeValidator("string"), + }, + }), + description: "list", + }), + }, + }); + + const mockClient = await getMockClient(); + await server + .handler() + .POST( + mcpRequest("tools/call", { + name: "list", + arguments: { userArg: "v", _mcp_apiKey: "spoofed" }, + }), + ) + .then((r) => r.text()); + + // The dispatched args contain ONLY the server-validated value. + // The spoofed `_mcp_apiKey: "spoofed"` was stripped by the SDK before + // our handler ran, then the hook injected the trusted value. + expect(mockClient.query).toHaveBeenCalledTimes(1); + expect(mockClient.query).toHaveBeenCalledWith(null, { + userArg: "v", + _mcp_apiKey: "server-validated", + }); + }); + + it("hook-supplied `_*` keys via extendArgs bypass the reject (server is trusted)", async () => { + const server = createMCPServer({ + auth: { validate: async () => true }, + convexUrl: MOCK_CONVEX_URL, + hooks: { + onToolCall: async ({ phase }) => { + if (phase !== "before") return; + return { extendArgs: { _mcp_apiKey: "server-injected" } }; + }, + }, + tools: { + list: query(null, { + args: makeValidator("object", { + fields: { x: makeValidator("string"), _mcp_apiKey: makeValidator("string") }, + }), + description: "list", + }), + }, + }); + + const mockClient = await getMockClient(); + await server + .handler() + .POST(mcpRequest("tools/call", { name: "list", arguments: { x: "v" } })) + .then((r) => r.text()); + + expect(mockClient.query).toHaveBeenCalledWith(null, { x: "v", _mcp_apiKey: "server-injected" }); + }); + + it("only top-level keys are rejected — nested `_*` keys pass through unchanged", async () => { + const server = createMCPServer({ + auth: { validate: async () => true }, + convexUrl: MOCK_CONVEX_URL, + tools: { + list: query(null, { + args: makeValidator("object", { + fields: { + payload: makeValidator("object", { + fields: { _internal: makeValidator("string") }, + }), + }, + }), + description: "list", + }), + }, + }); + + const mockClient = await getMockClient(); + await server + .handler() + .POST( + mcpRequest("tools/call", { + name: "list", + arguments: { payload: { _internal: "client-data" } }, + }), + ) + .then((r) => r.text()); + + // Top-level reject doesn't recurse — nested `_internal` is consumer territory. + expect(mockClient.query).toHaveBeenCalledWith(null, { payload: { _internal: "client-data" } }); + }); +}); + +describe("context propagation: schema stripping", () => { + it("strips `_*` keys from published tool inputSchema", async () => { + const server = createMCPServer({ + auth: { validate: async () => true }, + convexUrl: MOCK_CONVEX_URL, + tools: { + list: query(null, { + args: makeValidator("object", { + fields: { + userArg: makeValidator("string"), + _mcp_apiKey: makeValidator("string"), + _mcp_scope: makeValidator("string"), + }, + }), + description: "list", + }), + }, + }); + + const data = await parseSSE(await server.handler().POST(mcpRequest("tools/list", {}))); + const text = data.result?.content?.[0]?.text; + const parsed = text ? JSON.parse(text) : (data.result as unknown as { tools: { name: string; inputSchema: { properties?: Record; required?: string[] } }[] }); + const tools = (parsed as { tools?: unknown[] }).tools ?? (data.result as unknown as { tools: { name: string; inputSchema: { properties?: Record; required?: string[] } }[] }).tools; + const listTool = (tools as { name: string; inputSchema: { properties?: Record; required?: string[] } }[]).find((t) => t.name === "list"); + expect(listTool).toBeDefined(); + expect(Object.keys(listTool?.inputSchema.properties ?? {})).toEqual(["userArg"]); + expect(listTool?.inputSchema.required ?? []).toEqual(["userArg"]); + }); +}); From 2822ef5fd1479490c609e35f74b06d17df5615a9 Mon Sep 17 00:00:00 2001 From: bntvllnt <32437578+bntvllnt@users.noreply.github.com> Date: Mon, 27 Apr 2026 21:57:04 +0200 Subject: [PATCH 2/4] docs: cover hooks + extendArgs + reserved prefix across all doc surfaces Closes #18. Adds full v0.3.0 coverage to docs that were missed in the initial PR: - README.md: brief mention of per-action authorization + reserved prefix in the Security section, link to docs/security.md - llms-full.txt: full Lifecycle Hooks & Request Context Propagation section (~120 lines) with all 5 patterns + anti-patterns; Types section updated with LifecycleHooks, CallContext, OnCallResult, expanded ToolDef - docs/api-reference.md: LifecycleHooks, CallContext, OnCallResult reference sections; reserved-prefix subsection; hooks field added to ServerConfig table; new types added to Exported Types - docs/examples.md: 4 worked patterns (per-action authorization with defense-in-depth, request tracing, multi-tenancy, per-key feature flags) + anti-patterns table Verification: - 127/127 tests pass, lint + typecheck green - 0 references to any specific downstream consumer across all docs --- README.md | 5 +- docs/api-reference.md | 111 ++++++++++++++++++++++++++++++++ docs/examples.md | 146 ++++++++++++++++++++++++++++++++++++++++++ llms-full.txt | 116 +++++++++++++++++++++++++++++++++ 4 files changed, 376 insertions(+), 2 deletions(-) diff --git a/README.md b/README.md index cac57dc..a9e65e0 100644 --- a/README.md +++ b/README.md @@ -175,12 +175,13 @@ Convex validators are automatically converted to JSON Schema for MCP tool defini - **Default-deny**: Auth is required. No `validate` = startup error. - **Generic errors**: Convex error messages are never leaked to MCP responses (may contain PII). Only "Function execution failed" is returned. -- **No function-level authz** (v1): A valid API key grants access to all exposed tools. Scope tools carefully. +- **Per-action authorization (v0.3.0)**: The `onToolCall` hook can return `extendArgs` to inject server-resolved context (e.g. `_mcp_apiKey`, scopes, tenantId) into the dispatched function's args. The action handler re-validates the injected context as defense-in-depth — even if the framework hook is bypassed, the action stays safe. See [Security › Server-side Context Propagation](docs/security.md#server-side-context-propagation-v030). +- **Reserved `_` prefix (v0.3.0)**: Arg keys starting with `_` are framework-controlled. Stripped from the published JSON Schema and rejected at the handler boundary; only the framework can inject them via `extendArgs`. Prevents callers from spoofing server-injected context. ### Known Limitations - **Validator duplication**: You must provide Convex validators in the MCP config alongside function references. FunctionReference carries no runtime schema info. We're exploring codegen for v2. -- **Service account model**: By default, Convex functions execute without user identity (`ctx.auth` is null). Use `convexToken` in auth config to propagate identity. +- **Service account model**: By default, Convex functions execute without user identity (`ctx.auth` is null). Use `convexToken` in auth config to propagate identity, or `extendArgs` for arg-level context propagation (v0.3.0). - **Serverless timeouts**: Vercel Hobby has 10s timeout, Pro has 60s. Use Fluid Compute for long-running actions. ## Docs diff --git a/docs/api-reference.md b/docs/api-reference.md index 5feb1f3..b735d20 100644 --- a/docs/api-reference.md +++ b/docs/api-reference.md @@ -15,6 +15,7 @@ const mcp = createMCPServer({ name?: string; // MCP server name (default: "convex-mcp") version?: string; // MCP server version (default: "0.3.0") pagination?: PaginationConfig; // opt-in pagination + two-phase discovery + hooks?: LifecycleHooks; // before/success/error tool-call hooks }); ``` @@ -29,6 +30,7 @@ const mcp = createMCPServer({ | `name` | `string` | No | `"convex-mcp"` | Server name reported in MCP `initialize` response. | | `version` | `string` | No | `"0.3.0"` | Server version reported in MCP `initialize` response. | | `pagination` | `PaginationConfig` | No | — | Opt-in pagination and two-phase discovery. See [Pagination](#pagination). | +| `hooks` | `LifecycleHooks` | No | — | Lifecycle hooks for tool calls. See [`LifecycleHooks`](#lifecyclehooks). | ### Throws @@ -203,6 +205,112 @@ Request arrives --- +## `LifecycleHooks` + +```typescript +interface LifecycleHooks { + onToolCall?: (ctx: CallContext) => Promise | OnCallResult | void; +} +``` + +Pass via `createMCPServer({ hooks: { onToolCall } })`. The hook fires three times per tool call: `before` (pre-dispatch), `success` (after the dispatched function resolves), and `error` (after it throws). + +| Phase | Purpose | +|-------|---------| +| `before` | Validate, abort, inject context via `extendArgs`. The only phase whose return value affects dispatch. | +| `success` | Observe successful results (logging, metrics). Return value is ignored. | +| `error` | Customize the error message returned to the MCP client (`return { message: "..." }`). | + +Per-tool overrides: `ToolDef.onError` runs before `LifecycleHooks.onToolCall` for the `error` phase. + +--- + +## `CallContext` + +```typescript +interface CallContext { + requestId: string; // crypto.randomUUID() — same across before/success/error for one tool call + toolName: string; // the registered tool name (e.g. "list_tasks") + toolDef: Omit; // tool def without the function ref or per-tool error hook + args: Record; + apiKey: string | undefined; // from the Authorization header; undefined if anonymous + phase: "before" | "success" | "error"; + result?: unknown; // present on "success" + error?: unknown; // present on "error" + durationMs?: number; // present on "success" and "error" + startedAt: number; // ms timestamp of dispatch start +} +``` + +--- + +## `OnCallResult` + +```typescript +interface OnCallResult { + abort?: boolean; // before phase — skip dispatch, return error response + errorMessage?: string; // before phase — message when aborting (default: "Tool call rejected") + message?: string; // error phase — replace default error message + extendArgs?: Record; // before phase — merge into dispatched args (v0.3.0+) +} +``` + +| Field | Phase | Description | +|-------|-------|-------------| +| `abort` | `before` | When `true`, framework returns an error response without dispatching. | +| `errorMessage` | `before` | Custom abort message. Falls back to `"Tool call rejected"`. | +| `message` | `error` | Replaces the framework's default `"Function execution failed"` text in the response. | +| `extendArgs` | `before` | **(v0.3.0)** Server-resolved key/value pairs merged into the dispatched function's args. Server-side wins on collision. Only honored during the `before` phase. Empty / undefined is a no-op. Keys SHOULD use the framework-reserved `_` prefix. | + +### `extendArgs` example + +```typescript +hooks: { + onToolCall: async ({ apiKey, phase }) => { + if (phase !== "before") return; + const validated = await validateKey(apiKey); + if (!validated.valid) return { abort: true, errorMessage: "Invalid key" }; + return { + extendArgs: { + _mcp_apiKey: apiKey, + _mcp_scopes: validated.scopes, + }, + }; + }, +} +``` + +The dispatched action's validator must declare the injected fields as required args: + +```typescript +export const sensitiveAction = action({ + args: { + _mcp_apiKey: v.string(), // injected by framework + _mcp_scopes: v.array(v.string()), + targetId: v.id("things"), + }, + handler: async (ctx, args) => { + await assertMcpAuth(ctx, args._mcp_apiKey, "things:write"); + // ... real logic ... + }, +}); +``` + +--- + +## Reserved `_` prefix in tool args (v0.3.0) + +Arg keys starting with underscore are framework-controlled. Two layers protect them: + +1. **Schema strip** — `prepareTools` removes `_*` keys from the published JSON Schema. The MCP SDK's Zod validator silently strips `_*` keys from incoming requests (Zod default strip mode), so spoofed values never reach the dispatched function. +2. **Handler reject** — The registered tool handler explicitly rejects any `_*` key reaching it with a structured `"Reserved arg keys not allowed"` error before the hook runs. This defends against transports that bypass the SDK's schema validation (custom transports, future protocol changes, direct `registerTools` callers). + +Hook-supplied `_*` keys via `extendArgs` are exempt from both layers — the server is trusted by definition. + +See [Security › Server-side Context Propagation](./security.md#server-side-context-propagation-v030) for the full pattern. + +--- + ## `HandlerOptions` ```typescript @@ -307,5 +415,8 @@ import type { ConvexMCPServer, FunctionType, // "query" | "mutation" | "action" ConvexValidator, + LifecycleHooks, + CallContext, + OnCallResult, } from "@vllnt/convex-mcp"; ``` diff --git a/docs/examples.md b/docs/examples.md index 052db12..bd6c190 100644 --- a/docs/examples.md +++ b/docs/examples.md @@ -322,3 +322,149 @@ export const { GET, POST } = adminMcp.handler(); import { publicMcp } from "@/convex/mcp-public"; export const { GET, POST } = publicMcp.handler(); ``` + +## Hook-driven request context propagation (v0.3.0) + +The `onToolCall` hook can return `extendArgs` to inject server-resolved context into the dispatched function's args. Combined with the framework-reserved `_` prefix, this gives you a safe channel for per-action authorization, request tracing, multi-tenancy, audit metadata, and more. + +### Per-action authorization (defense-in-depth) + +The framework hook validates the apiKey + required scope. The action handler **re-validates** the injected key — so even if the hook is bypassed (custom transport, framework regression), the action stays safe. + +```typescript +// convex/mcp.ts +import { createMCPServer, action } from "@vllnt/convex-mcp"; +import { api } from "./_generated/api"; +import { v } from "convex/values"; + +export const mcp = createMCPServer({ + auth: { validate: async (key) => Boolean(await validateKey(key)) }, + hooks: { + onToolCall: async ({ apiKey, phase, toolDef }) => { + if (phase !== "before") return; + const validated = await validateKey(apiKey); + if (!validated.valid) return { abort: true, errorMessage: "Invalid key" }; + const required = toolDef.tags?.requiredScope; + if (required && !validated.scopes.includes(required)) { + return { abort: true, errorMessage: `Missing scope: ${required}` }; + } + return { + extendArgs: { + _mcp_apiKey: apiKey, + _mcp_scopes: validated.scopes, + }, + }; + }, + }, + tools: { + upsert_thing: action(api.things.upsert, { + args: v.object({ + _mcp_apiKey: v.string(), // injected by framework + _mcp_scopes: v.array(v.string()), + targetId: v.id("things"), + payload: v.any(), + }), + description: "Upsert a thing.", + tags: { requiredScope: "things:write" }, + }), + }, +}); + +// convex/things.ts +export const upsert = action({ + args: { + _mcp_apiKey: v.string(), + _mcp_scopes: v.array(v.string()), + targetId: v.id("things"), + payload: v.any(), + }, + handler: async (ctx, args) => { + // Defense-in-depth: re-validate inside the action. + await assertMcpAuth(ctx, args._mcp_apiKey, "things:write"); + return await ctx.runMutation(internal.things.write, { + id: args.targetId, + payload: args.payload, + }); + }, +}); +``` + +### Request tracing + +```typescript +hooks: { + onToolCall: async ({ requestId, phase }) => { + if (phase !== "before") return; + return { extendArgs: { _mcp_requestId: requestId } }; + }, +} + +// In the action — correlate domain logs with framework request lifecycle: +export const doThing = action({ + args: { _mcp_requestId: v.string(), input: v.string() }, + handler: async (ctx, args) => { + logger.info("doing_thing", { requestId: args._mcp_requestId, input: args.input }); + // ... + }, +}); +``` + +### Multi-tenancy with server-resolved tenant routing + +```typescript +hooks: { + onToolCall: async ({ apiKey, phase }) => { + if (phase !== "before") return; + const tenantId = await resolveTenantFromKey(apiKey); + if (!tenantId) return { abort: true, errorMessage: "Unknown tenant" }; + return { extendArgs: { _mcp_tenantId: tenantId } }; + }, +} + +// Action enforces server-resolved tenant ID — caller cannot spoof: +export const listProjects = query({ + args: { _mcp_tenantId: v.id("tenants") }, + handler: async (ctx, args) => { + return await ctx.db + .query("projects") + .withIndex("by_tenant", (q) => q.eq("tenantId", args._mcp_tenantId)) + .take(50); + }, +}); +``` + +### Per-key feature flags + +```typescript +hooks: { + onToolCall: async ({ apiKey, phase }) => { + if (phase !== "before") return; + const flags = await getFlagsFor(apiKey); // cached lookup + return { extendArgs: { _mcp_flags: flags } }; + }, +} + +// Action branches on flags without re-fetching: +export const search = query({ + args: { + _mcp_flags: v.object({ semanticSearch: v.boolean() }), + q: v.string(), + }, + handler: async (ctx, args) => { + return args._mcp_flags.semanticSearch + ? await semanticSearch(ctx, args.q) + : await keywordSearch(ctx, args.q); + }, +}); +``` + +### Anti-patterns + +| Anti-pattern | Why bad | Fix | +|---|---|---| +| Long-lived secrets in `extendArgs` | Args flow into action logs and may be over-shared | Inject ID + scopes only; resolve secrets inside the action when needed | +| Hook-only authorization (no action re-check) | Framework regression / fork = silent security failure | Always re-validate the injected key inside the action handler | +| Splitting context across `extendArgs` AND a side-channel store | Two sources of truth; drift inevitable | Pick one — `extendArgs` is the canonical path | +| Allowing client `_*` passthrough (e.g., disabling the reject) | Defeats the entire safety model | Don't. The reject is what makes `extendArgs` trustworthy | + +See [Security › Server-side Context Propagation](./security.md#server-side-context-propagation-v030) for the full security rationale. diff --git a/llms-full.txt b/llms-full.txt index 92a23d8..7f8edfd 100644 --- a/llms-full.txt +++ b/llms-full.txt @@ -230,6 +230,7 @@ interface ServerConfig { name?: string; version?: string; pagination?: PaginationConfig; + hooks?: LifecycleHooks; // v0.3.0+ } interface PaginationConfig { @@ -247,6 +248,9 @@ interface ToolDef { type: "query" | "mutation" | "action"; args?: ConvexValidator; // Convex v.object() validator description?: string; + tags?: Record; // arbitrary metadata visible in CallContext.toolDef.tags + timeout?: number; // ms before the dispatch is aborted + onError?: (ctx: CallContext & { phase: "error" }) => Promise | OnCallResult | void; } interface ResourceDef { @@ -255,6 +259,30 @@ interface ResourceDef { description?: string; } +interface LifecycleHooks { + onToolCall?: (ctx: CallContext) => Promise | OnCallResult | void; +} + +interface CallContext { + requestId: string; // crypto.randomUUID(), shared across before/success/error for one tool call + toolName: string; + toolDef: Omit; + args: Record; + apiKey: string | undefined; + phase: "before" | "success" | "error"; + result?: unknown; // present on "success" + error?: unknown; // present on "error" + durationMs?: number; // present on "success" and "error" + startedAt: number; +} + +interface OnCallResult { + abort?: boolean; // before phase — skip dispatch, return error response + errorMessage?: string; // before phase — message when aborting (default: "Tool call rejected") + message?: string; // error phase — replace default error text ("Function execution failed") + extendArgs?: Record; // v0.3.0+ — merge into dispatched args; server-side wins on collision +} + interface ConvexMCPServer { handler: () => { GET: (request: Request) => Promise; @@ -267,6 +295,94 @@ type FunctionType = "query" | "mutation" | "action"; --- +## Lifecycle Hooks & Request Context Propagation + +The `onToolCall` hook fires three times per tool call: `before` (pre-dispatch), `success` (after the dispatched function resolves), and `error` (after it throws). The `before` phase is the only one whose return value affects dispatch. + +### Hook return shape (`OnCallResult`) + +| Field | Phase | Effect | +|-------|-------|--------| +| `abort` | `before` | When `true`, framework returns an error response without dispatching. | +| `errorMessage` | `before` | Custom abort message. Default: `"Tool call rejected"`. | +| `message` | `error` | Replaces default `"Function execution failed"` text. | +| `extendArgs` | `before` | (v0.3.0+) Server-resolved key/value pairs merged into the dispatched function's args. Server-side wins on collision. Empty / undefined is a no-op. | + +### Per-tool `onError` override + +Tools can declare their own error hook via `ToolDef.onError`. When set, it runs INSTEAD OF `LifecycleHooks.onToolCall` for the `error` phase of that tool. Useful for per-tool error redaction. + +### Request Context Propagation (v0.3.0) + +The `extendArgs` field unblocks server-side context injection. Common patterns: + +```typescript +// Per-action authorization with defense-in-depth +hooks: { + onToolCall: async ({ apiKey, phase, toolDef }) => { + if (phase !== "before") return; + const validated = await validateKey(apiKey); + if (!validated.valid) return { abort: true, errorMessage: "Invalid key" }; + const required = toolDef.tags?.requiredScope; + if (required && !validated.scopes.includes(required)) { + return { abort: true, errorMessage: `Missing scope: ${required}` }; + } + return { + extendArgs: { + _mcp_apiKey: apiKey, + _mcp_scopes: validated.scopes, + }, + }; + }, +} +``` + +The dispatched action's validator must declare the injected fields: + +```typescript +export const upsertThing = action({ + args: { + _mcp_apiKey: v.string(), // injected by framework + _mcp_scopes: v.array(v.string()), + targetId: v.id("things"), + }, + handler: async (ctx, args) => { + // Defense-in-depth: re-validate the injected key inside the action. + // Even if the framework hook is bypassed, the action stays safe. + await assertMcpAuth(ctx, args._mcp_apiKey, "things:write"); + // ... real logic with confidence the caller is authorized ... + }, +}); +``` + +### Other patterns + +| Pattern | `extendArgs` | Action receives | +|---------|--------------|-----------------| +| Request tracing | `{ _mcp_requestId: ctx.requestId }` | Threads framework's requestId into action audit logs | +| Multi-tenancy | `{ _mcp_tenantId: resolveTenant(apiKey) }` | Server-resolved tenant ID enforced via index queries | +| Per-key feature flags | `{ _mcp_flags: getFlagsFor(apiKey) }` | Branches behavior per caller | +| Per-key quotas | `{ _mcp_quotaRemaining: n }` | Enforces or warns near limit | +| Audit metadata | `{ _mcp_callerLabel: keyMetadata.label }` | Recorded in domain audit log | + +### Reserved `_` prefix (v0.3.0) + +Arg keys starting with `_` are framework-controlled. Two protection layers: + +1. **Schema strip** — `prepareTools` removes `_*` keys from the published JSON Schema. The MCP SDK's Zod validator silently strips `_*` keys from incoming requests (Zod default strip mode). +2. **Handler reject** — The registered tool handler explicitly rejects any `_*` key reaching it with `"Reserved arg keys not allowed: ..."` before the hook runs. Defense-in-depth for non-SDK transports. + +Hook-supplied `_*` keys via `extendArgs` are exempt — the server is trusted. + +### Anti-patterns + +- DON'T put long-lived secrets in `extendArgs` — values flow into args, may end up in action logs. +- DON'T rely solely on the hook for authorization — without action-side re-validation, a framework regression becomes a security blind spot. +- DON'T spread context across `extendArgs` AND a side-channel store — pick one. The args path is canonical. +- DON'T allow client `_*` passthrough — the reject is what makes `extendArgs` safe. + +--- + ## Validator Mapping (Convex -> JSON Schema) All Convex v.* types with their JSON Schema output: From 107824ae71409c8279c3c4098a706b94f34bccd9 Mon Sep 17 00:00:00 2001 From: bntvllnt <32437578+bntvllnt@users.noreply.github.com> Date: Mon, 27 Apr 2026 22:11:19 +0200 Subject: [PATCH 3/4] fix(hooks): address review WARNs for context propagation - docs/security.md: document hook fail-open behavior; require `_*` action validators to be non-optional so Convex's own validator is the safety net if the framework hook throws or is bypassed - register.ts: log `[convex-mcp] reserved-key reject` (with requestId, tool, keys) when handler-layer rejection fires - register.ts + server.ts: emit one construction-time `console.warn` listing tools that declare `_*` args without an `onToolCall` hook configured, catching the silent "stripped from schema, never injected" footgun - tests: replace `as ConvexValidator` cast with `satisfies`; add coverage for findToolsWithReservedArgs, both warn branches, and the reject log Quality gates: lint PASS, typecheck PASS, tests 132/132, coverage 100% on statements/branches/functions/lines, build PASS. --- CHANGELOG.md | 6 +- docs/security.md | 20 ++++ src/server.ts | 17 +++- src/tools/register.ts | 25 +++++ tests/context-propagation.test.ts | 158 +++++++++++++++++++++++++++++- 5 files changed, 221 insertions(+), 5 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index b78c095..c6671a0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,13 +5,15 @@ All notable changes to this project will be documented in this file. The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/), and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). -## [0.3.0] - Unreleased +## [0.3.0] - 2026-04-27 ### Added - **Hook-driven request context propagation** ([#15](https://github.com/vllnt/convex-mcp/issues/15)) — The `onToolCall` hook can now return `extendArgs?: Record` to merge server-resolved context into the dispatched function's args. Unblocks per-action authorization, request tracing, multi-tenancy, audit metadata, per-key feature flags, and any pattern where the server needs to inject trusted context the action can read ([#16](https://github.com/vllnt/convex-mcp/issues/16)) - **Reserved `_` prefix for tool args** ([#17](https://github.com/vllnt/convex-mcp/issues/17)) — Arg keys starting with `_` are framework-controlled. Two layers protect them: (1) `prepareTools` strips `_*` keys from the published JSON Schema so MCP clients can't see or pass them; (2) the registered tool handler explicitly rejects any `_*` key that arrives at the handler boundary (defense-in-depth for non-SDK callers). Hook-supplied `_*` keys via `extendArgs` are exempt -- 12 new tests in `tests/context-propagation.test.ts` covering merge precedence, no-op cases (undefined / empty `extendArgs`), abort precedence, phase isolation (only `before` honors `extendArgs`), schema stripping, reserved-key rejection at handler boundary, nested-key passthrough, and end-to-end SDK schema-strip behavior +- **Construction-time warn for unhooked `_*` args** — `createMCPServer` emits a single `console.warn` when any tool declares reserved `_*` args but no `onToolCall` hook is configured, listing the affected tools and keys. Catches a footgun where stripped args would silently fail every dispatched call against Convex's validator +- **Reserved-key reject log line** — `[convex-mcp] reserved-key reject` warn (with `requestId`, `tool`, `keys`) when the handler-layer reject fires, for operator visibility +- 17 new tests in `tests/context-propagation.test.ts` covering merge precedence, no-op cases (undefined / empty `extendArgs`), abort precedence, phase isolation (only `before` honors `extendArgs`), schema stripping, reserved-key rejection at handler boundary, nested-key passthrough, end-to-end SDK schema-strip behavior, the new construction-time warn, and the reject log line ### Changed diff --git a/docs/security.md b/docs/security.md index 3412c25..8daa77d 100644 --- a/docs/security.md +++ b/docs/security.md @@ -228,6 +228,26 @@ export const upsertThing = action({ }); ``` +### Hooks are fail-open — action validators are what make this safe + +`onToolCall` exceptions are caught and logged by the framework, then dispatch proceeds with the original args (no `abort`, no `extendArgs`). This is intentional: a transient failure inside an observability or auth-cache hook must not crash the server. But it has a security consequence: + +- If `validateKey(apiKey)` throws (DB blip, network error, code regression), the hook returns nothing. +- Dispatch proceeds with the original request args. +- `_mcp_apiKey` is **not** injected — the action receives whatever the client sent (which, for `_*` keys, is nothing — they were stripped or rejected). + +What makes this safe is the action validator. **Always declare framework-injected `_*` fields as required (non-optional) on the Convex action**: + +```ts +args: { + _mcp_apiKey: v.string(), // required — Convex rejects the call if missing + _mcp_scopes: v.array(v.string()), // required + targetId: v.id("things"), +} +``` + +If the hook throws or is bypassed, Convex's own validator rejects the call before the handler runs. If you mark `_mcp_apiKey` as `v.optional(v.string())` you delete the safety net — the handler will execute with `args._mcp_apiKey === undefined` and any code path reading it must explicitly handle that case. **Don't.** + ### Reserved `_` prefix — non-negotiable Arg keys starting with `_` are framework-controlled. The framework protects them with two layers: diff --git a/src/server.ts b/src/server.ts index f8c53d6..85d4a1a 100644 --- a/src/server.ts +++ b/src/server.ts @@ -5,7 +5,7 @@ import { validateRequest } from "./auth.js"; import { createPaginationContext } from "./pagination/context.js"; import { getOriginalToolsList, registerPaginationHandlers, registerTwoPhaseHandlers } from "./pagination/handlers.js"; import { prepareResources, registerResources } from "./resources/register.js"; -import { prepareTools, registerTools } from "./tools/register.js"; +import { findToolsWithReservedArgs, prepareTools, registerTools } from "./tools/register.js"; import type { ConvexClient, ConvexMCPServer, ServerConfig } from "./types.js"; function createDefaultClient(convexUrl: string, convexToken?: string): ConvexClient { @@ -62,6 +62,21 @@ export function createMCPServer(config: ServerConfig): ConvexMCPServer { const preparedRes = prepareResources(config.resources ?? {}); const paginationCtx = createPaginationContext(config.pagination); + if (!hooks?.onToolCall) { + const stripped = findToolsWithReservedArgs(config.tools ?? {}); + if (stripped.size > 0) { + const summary = Array.from(stripped.entries()) + .map(([tool, keys]) => `${tool} (${keys.join(", ")})`) + .join("; "); + console.warn( + `[convex-mcp] tools declare reserved \`_*\` args but no onToolCall hook is configured. ` + + `These args are stripped from the published schema and will never be injected, ` + + `so every dispatched call will fail Convex validation. ` + + `Configure hooks.onToolCall to inject them via extendArgs. Affected tools: ${summary}`, + ); + } + } + function createServerAndTransport( requestId: string, convexToken?: string, diff --git a/src/tools/register.ts b/src/tools/register.ts index 5a1d61a..c2c3b51 100644 --- a/src/tools/register.ts +++ b/src/tools/register.ts @@ -48,6 +48,26 @@ export function prepareTools(tools: Record): PreparedTool[] { }); } +/** + * Returns tools whose top-level args contain reserved `_*` keys. + * + * Used by `createMCPServer` at construction to surface a footgun: a tool that + * declares `_*` args without an `onToolCall` hook will have those args stripped + * from the published schema and never injected, so every dispatched call will + * fail Convex's own validator with "missing required arg". + */ +export function findToolsWithReservedArgs( + tools: Record, +): Map { + const result = new Map(); + for (const [name, toolDef] of Object.entries(tools)) { + if (toolDef.args?.kind !== "object" || !toolDef.args.fields) continue; + const reserved = Object.keys(toolDef.args.fields).filter(isReservedKey); + if (reserved.length > 0) result.set(name, reserved); + } + return result; +} + async function invokeHook( hooks: LifecycleHooks | undefined, ctx: CallContext, @@ -108,6 +128,11 @@ export function registerTools( const reservedKeys = Object.keys(args).filter(isReservedKey); if (reservedKeys.length > 0) { + console.warn("[convex-mcp] reserved-key reject", { + requestId, + tool: name, + keys: reservedKeys, + }); return { content: [ { diff --git a/tests/context-propagation.test.ts b/tests/context-propagation.test.ts index e1fd8f5..069d94b 100644 --- a/tests/context-propagation.test.ts +++ b/tests/context-propagation.test.ts @@ -15,8 +15,8 @@ import type { CallContext, ConvexValidator } from "../src/types.js"; const MOCK_CONVEX_URL = "https://test-deployment.convex.cloud"; -function makeValidator(kind: string, extra: Record = {}): ConvexValidator { - return { kind, isOptional: "required", ...extra } as ConvexValidator; +function makeValidator(kind: string, extra: Partial = {}): ConvexValidator { + return { kind, isOptional: "required", ...extra } satisfies ConvexValidator; } vi.mock("convex/browser", () => { @@ -497,3 +497,157 @@ describe("context propagation: schema stripping", () => { expect(listTool?.inputSchema.required ?? []).toEqual(["userArg"]); }); }); + +describe("findToolsWithReservedArgs", () => { + it("returns tools whose top-level args contain reserved `_*` keys", async () => { + const { findToolsWithReservedArgs } = await import("../src/tools/register.js"); + const result = findToolsWithReservedArgs({ + hasReserved: query(null, { + args: makeValidator("object", { + fields: { _mcp_apiKey: makeValidator("string"), userArg: makeValidator("string") }, + }), + description: "with reserved", + }), + noReserved: query(null, { + args: makeValidator("object", { fields: { userArg: makeValidator("string") } }), + description: "no reserved", + }), + noArgs: query(null, { description: "no args" }), + nonObjectArgs: query(null, { + args: makeValidator("string"), + description: "non-object args", + }), + emptyFields: query(null, { + args: makeValidator("object"), + description: "no fields", + }), + }); + + expect(Array.from(result.entries())).toEqual([["hasReserved", ["_mcp_apiKey"]]]); + }); +}); + +describe("construction-time warn for unhooked `_*` args", () => { + it("warns when tools declare `_*` args but no onToolCall hook is configured", async () => { + const warnSpy = vi.spyOn(console, "warn").mockImplementation(() => undefined); + try { + createMCPServer({ + auth: { validate: async () => true }, + convexUrl: MOCK_CONVEX_URL, + tools: { + tool1: query(null, { + args: makeValidator("object", { fields: { _mcp_apiKey: makeValidator("string") } }), + description: "tool1", + }), + tool2: query(null, { + args: makeValidator("object", { + fields: { + _mcp_tenantId: makeValidator("string"), + _mcp_scope: makeValidator("string"), + userArg: makeValidator("string"), + }, + }), + description: "tool2", + }), + }, + }); + + expect(warnSpy).toHaveBeenCalledTimes(1); + const message = warnSpy.mock.calls[0]?.[0]; + expect(message).toContain("no onToolCall hook is configured"); + expect(message).toContain("tool1 (_mcp_apiKey)"); + expect(message).toContain("tool2 (_mcp_tenantId, _mcp_scope)"); + } finally { + warnSpy.mockRestore(); + } + }); + + it("does not warn when an onToolCall hook is configured", async () => { + const warnSpy = vi.spyOn(console, "warn").mockImplementation(() => undefined); + try { + createMCPServer({ + auth: { validate: async () => true }, + convexUrl: MOCK_CONVEX_URL, + hooks: { onToolCall: async () => undefined }, + tools: { + tool1: query(null, { + args: makeValidator("object", { fields: { _mcp_apiKey: makeValidator("string") } }), + description: "tool1", + }), + }, + }); + + expect(warnSpy).not.toHaveBeenCalled(); + } finally { + warnSpy.mockRestore(); + } + }); + + it("does not warn when no tools declare `_*` args", async () => { + const warnSpy = vi.spyOn(console, "warn").mockImplementation(() => undefined); + try { + createMCPServer({ + auth: { validate: async () => true }, + convexUrl: MOCK_CONVEX_URL, + tools: { + tool1: query(null, { + args: makeValidator("object", { fields: { userArg: makeValidator("string") } }), + description: "tool1", + }), + }, + }); + + expect(warnSpy).not.toHaveBeenCalled(); + } finally { + warnSpy.mockRestore(); + } + }); +}); + +describe("reserved-key reject logging", () => { + it("logs a structured warn line when handler-layer reject fires", async () => { + const warnSpy = vi.spyOn(console, "warn").mockImplementation(() => undefined); + try { + const { registerTools, prepareTools } = await import("../src/tools/register.js"); + type ToolHandler = (args: Record) => Promise<{ isError?: boolean; content: { text: string }[] }>; + + const mockClient = await getMockClient(); + const captured: { handler?: ToolHandler } = {}; + const fakeServer = { + tool: (_name: string, _desc: string, _shape: unknown, h: ToolHandler) => { + captured.handler = h; + }, + }; + + const prepared = prepareTools({ + list: query(null, { + args: makeValidator("object", { fields: { _mcp_apiKey: makeValidator("string") } }), + description: "list", + }), + }); + registerTools( + fakeServer as unknown as Parameters[0], + mockClient as unknown as Parameters[1], + prepared, + undefined, + "rid-42", + undefined, + ); + + const handler = captured.handler; + if (!handler) throw new Error("registerTools did not register a handler"); + await handler({ _mcp_apiKey: "spoofed", _mcp_scope: "x" }); + + expect(warnSpy).toHaveBeenCalledWith( + "[convex-mcp] reserved-key reject", + expect.objectContaining({ + requestId: "rid-42", + tool: "list", + keys: ["_mcp_apiKey", "_mcp_scope"], + }), + ); + } finally { + warnSpy.mockRestore(); + } + }); +}); From 345cfee3ea8a274c34ac4cd5e56847dcf107da9f Mon Sep 17 00:00:00 2001 From: bntvllnt <32437578+bntvllnt@users.noreply.github.com> Date: Mon, 27 Apr 2026 22:12:40 +0200 Subject: [PATCH 4/4] chore(specs): archive lifecycle-hooks spec (v0.3.0 shipped) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Move specs/active/2026-03-24-lifecycle-hooks.md → specs/shipped/ and append history.log entry. Spec covers the full v0.3.0 hook + context-propagation work: lifecycle hooks, per-tool config, X-Request-Id, extendArgs, reserved `_` prefix, and the review-driven hardening (security doc callout, construction-time warn, reject log). --- specs/active/2026-03-24-lifecycle-hooks.md | 42 ------------ specs/history.log | 1 + specs/shipped/2026-03-24-lifecycle-hooks.md | 72 +++++++++++++++++++++ 3 files changed, 73 insertions(+), 42 deletions(-) delete mode 100644 specs/active/2026-03-24-lifecycle-hooks.md create mode 100644 specs/shipped/2026-03-24-lifecycle-hooks.md diff --git a/specs/active/2026-03-24-lifecycle-hooks.md b/specs/active/2026-03-24-lifecycle-hooks.md deleted file mode 100644 index dc099a3..0000000 --- a/specs/active/2026-03-24-lifecycle-hooks.md +++ /dev/null @@ -1,42 +0,0 @@ ---- -title: "Lifecycle hooks + per-tool config + observability" -status: active -created: 2026-03-24 -estimate: 2h -tier: mini ---- - -# Lifecycle hooks + per-tool config + observability - -## Context - -Add extensibility hooks to `@vllnt/convex-mcp` as integration points for `@vllnt/convex-api-keys` and `@vllnt/convex-analytics`. Simplified design from spec review: single `onToolCall` hook with phase discriminant, not 5 separate hooks. - -## Scope - -- [ ] 1. Types: `CallContext`, `OnCallResult`, `LifecycleHooks` + extend `ToolDef`/`ServerConfig` -- [ ] 2. Update `auth.ts`: return `apiKey` in success result -- [ ] 3. Update `tool.ts`: add `tags`/`onError` to ToolOptions -- [ ] 4. Implement hooks in `server.ts`: requestId, apiKey threading, hook execution, timeout -- [ ] 5. `X-Request-Id` response header (wrap SSE Response) -- [ ] 6. Export new types + update README -- [ ] 7. Tests (~12 new) - -## ACs - -- [ ] AC-1: `onToolCall` fires with `{ requestId, toolName, args, apiKey, phase: "before" }`; can return `{ abort: true }` -- [ ] AC-2: `onToolCall` fires with `phase: "success"` including `result` + `durationMs` -- [ ] AC-3: `onToolCall` fires with `phase: "error"` including `error` + `durationMs`; custom message via `{ message }` -- [ ] AC-4: Per-tool `onError` overrides server hook for error phase -- [ ] AC-5: `toolDef.tags` accessible in context -- [ ] AC-6: `toolDef.timeout` aborts long-running calls -- [ ] AC-7: `X-Request-Id` header on all responses -- [ ] AC-E1: Hook throws → server stays healthy, default behavior -- [ ] AC-E2: No hooks → existing behavior unchanged - -## Timeline - -| Action | Timestamp | Notes | -|--------|-----------|-------| -| plan | 2026-03-24 | Created | -| spec-review | 2026-03-24 | 4 perspectives. Simplified from 12→7 scope items. | diff --git a/specs/history.log b/specs/history.log index 2e22493..f41d6ba 100644 --- a/specs/history.log +++ b/specs/history.log @@ -1 +1,2 @@ 2026-03-22 | shipped | convex-mcp | 8h→6h | 6h | Init @vllnt/convex-mcp package — expose Convex functions as MCP tools via Streamable HTTP +2026-04-27 | shipped | lifecycle-hooks | 2h→3h | 3h | Lifecycle hooks + per-tool config + observability + v0.3.0 extendArgs / reserved `_` prefix diff --git a/specs/shipped/2026-03-24-lifecycle-hooks.md b/specs/shipped/2026-03-24-lifecycle-hooks.md new file mode 100644 index 0000000..4a68847 --- /dev/null +++ b/specs/shipped/2026-03-24-lifecycle-hooks.md @@ -0,0 +1,72 @@ +--- +title: "Lifecycle hooks + per-tool config + observability" +status: shipped +created: 2026-03-24 +shipped: 2026-04-27 +estimate: 2h +actual: 3h +tier: mini +--- + +# Lifecycle hooks + per-tool config + observability + +## Context + +Add extensibility hooks to `@vllnt/convex-mcp` as integration points for `@vllnt/convex-api-keys` and `@vllnt/convex-analytics`. Simplified design from spec review: single `onToolCall` hook with phase discriminant, not 5 separate hooks. + +## Scope + +- [x] 1. Types: `CallContext`, `OnCallResult`, `LifecycleHooks` + extend `ToolDef`/`ServerConfig` +- [x] 2. Update `auth.ts`: return `apiKey` in success result +- [x] 3. Update `tool.ts`: add `tags`/`onError` to ToolOptions +- [x] 4. Implement hooks in `server.ts`: requestId, apiKey threading, hook execution, timeout +- [x] 5. `X-Request-Id` response header (wrap SSE Response) +- [x] 6. Export new types + update README +- [x] 7. Tests (~12 new) +- [x] 8. v0.3.0 follow-up: `extendArgs` for hook-driven request context propagation +- [x] 9. v0.3.0 follow-up: reserved `_` prefix (schema strip + handler reject) +- [x] 10. Review-driven hardening: hook fail-open doc, construction-time warn, reject log, test fixture cleanup + +## ACs + +- [x] AC-1: `onToolCall` fires with `{ requestId, toolName, args, apiKey, phase: "before" }`; can return `{ abort: true }` +- [x] AC-2: `onToolCall` fires with `phase: "success"` including `result` + `durationMs` +- [x] AC-3: `onToolCall` fires with `phase: "error"` including `error` + `durationMs`; custom message via `{ message }` +- [x] AC-4: Per-tool `onError` overrides server hook for error phase +- [x] AC-5: `toolDef.tags` accessible in context +- [x] AC-6: `toolDef.timeout` aborts long-running calls +- [x] AC-7: `X-Request-Id` header on all responses +- [x] AC-E1: Hook throws → server stays healthy, default behavior +- [x] AC-E2: No hooks → existing behavior unchanged +- [x] AC-8: Hook can return `extendArgs` to merge server-resolved context into dispatched args (server-side wins on collision; only `before` honors) +- [x] AC-9: Reserved `_*` keys stripped from published JSON Schema; handler-layer reject for non-SDK transports +- [x] AC-10: Construction-time warn surfaces tools that declare `_*` args without an `onToolCall` hook configured + +## Quality Gates (Final) + +- Lint: PASS (biome, 28 files) +- Typecheck: PASS (both tsconfigs) +- Tests: PASS — 132/132 (was 113 pre-spec) +- Coverage: 100% on statements (312), branches (206), functions (63), lines (289) +- Build: PASS — ESM 23.20 KB, CJS 23.54 KB, DTS 6.07 KB + +## Notes + +- v0.3.0 also bundles dependency upgrades (MCP SDK 1.29, Convex 1.36, Vitest 4, TS 6) and the validators-on-Zod-4 migration (carried over from earlier work). +- `extendArgs` semantics: server-wins-on-collision, `before`-only honor, empty/undefined no-op, abort precedence over extend. All four invariants documented in JSDoc and tested. +- Two-layer reserved-prefix protection: schema strip (clients silently dropped via SDK Zod default-strip) + handler reject (defense-in-depth for non-SDK transports). Asymmetry intentional and tested. +- Hook fail-open is by design (transient failures must not crash the server). The action validator declaring `_*` fields as required is what makes the system safe — documented as non-negotiable in `docs/security.md`. +- Construction-time warn lists affected tools + keys; fires once per server when tools declare `_*` but no hook is configured. + +## Timeline + +| Action | Timestamp | Notes | +|--------|-----------|-------| +| plan | 2026-03-24 | Created | +| spec-review | 2026-03-24 | 4 perspectives. Simplified from 12→7 scope items. | +| ship (initial) | 2026-03-24 | Hooks + per-tool config + X-Request-Id landed | +| ship (v0.3.0 extend) | 2026-04-26 | extendArgs + reserved `_` prefix (commit 4ff8057) | +| docs | 2026-04-27 | Cross-surface doc coverage (commit 2822ef5) | +| review | 2026-04-27 | Deep mode — 4 WARNs, no blockers | +| ship (review fixes) | 2026-04-27 | Security doc callout, construction warn, reject log, test cleanup (commit 107824a) | +| done | 2026-04-27 | Archived |