diff --git a/.changeset/mcp-feedback-hardening.md b/.changeset/mcp-feedback-hardening.md new file mode 100644 index 0000000000..3d4a3304e6 --- /dev/null +++ b/.changeset/mcp-feedback-hardening.md @@ -0,0 +1,5 @@ +--- +'@posthog/mcp': patch +--- + +Harden feedback validation, error handling, and tool-name collision routing. diff --git a/packages/mcp/README.md b/packages/mcp/README.md index 977f56ae3c..67ba81b839 100644 --- a/packages/mcp/README.md +++ b/packages/mcp/README.md @@ -263,7 +263,8 @@ const posthog = new PostHogMCP(process.env.POSTHOG_PROJECT_TOKEN, { return { tools: posthog.prepareToolList(myTools, { collectFeedback: true }) } // tools/call dispatcher -const prepared = posthog.prepareToolCall(name, rawArgs) +const originalTool = myTools.find((tool) => tool.name === name) +const prepared = posthog.prepareToolCall(name, rawArgs, { originalTool }) if (prepared.isFeedback) { posthog.captureFeedback({ report: prepared.feedbackReport!, ...identity }) await myFeedbackBackend.record(prepared.feedbackReport!) @@ -271,6 +272,10 @@ if (prepared.isFeedback) { } ``` +`originalTool` must come from the application's tool list before `prepareToolList()` adds PostHog's +virtual tools. This lets a real application tool with the configured feedback name win, including +when `tools/list` and `tools/call` reach different server replicas. + `send_feedback` covers what `reportMissing` covers — a capability gap is `feedback_type: "missing_capability"` — so new integrations should enable only `collectFeedback`. `reportMissing` and its `$mcp_missing_capability` event stay unchanged for existing users; enabling diff --git a/packages/mcp/src/__tests__/feedback.test.ts b/packages/mcp/src/__tests__/feedback.test.ts index 52d70c542d..a514ab9ad5 100644 --- a/packages/mcp/src/__tests__/feedback.test.ts +++ b/packages/mcp/src/__tests__/feedback.test.ts @@ -420,6 +420,36 @@ describe('collectFeedback (send_feedback virtual tool)', () => { await capture.stop() }) + it('rejects a fractional number for a declared integer extra, accepting whole values', async () => { + const capture = new EventCapture() + await capture.start() + const onFeedback = vi.fn() + instrument(server, fakePostHog(), { + collectFeedback: { extraProperties: { score: { type: 'integer' } }, onFeedback }, + }) + + await callTool(client, SEND_FEEDBACK, { + feedback_type: 'praise', + summary: 'Great tools.', + score: 3.5, // fractional — a JSON Schema `integer` does not accept it + }) + await callTool(client, SEND_FEEDBACK, { + feedback_type: 'praise', + summary: 'Great tools again.', + score: 3, // whole — conforms + }) + + await new Promise((r) => setTimeout(r, 50)) + const events = capture.findCapturesByEvent(PostHogMCPAnalyticsEvent.Feedback) + expect(events[0].properties.$mcp_feedback_score).toBeUndefined() + expect(events[1].properties.$mcp_feedback_score).toBe(3) + expect(onFeedback.mock.calls[0][0].extras).toEqual({}) + expect(onFeedback.mock.calls[0][0].raw.score).toBe(3.5) + expect(onFeedback.mock.calls[1][0].extras).toEqual({ score: 3 }) + + await capture.stop() + }) + it('redacts credential-named keys inside nested extras', async () => { const capture = new EventCapture() await capture.start() @@ -505,10 +535,14 @@ describe('collectFeedback (send_feedback virtual tool)', () => { it('falls back to the default reply when onFeedback throws, and still captures the event', async () => { const capture = new EventCapture() await capture.start() + const logged: string[] = [] instrument(server, fakePostHog(), { + logger: (message: string) => logged.push(message), collectFeedback: { onFeedback: async () => { - throw new Error('backend down') + const error = new Error('backend down for jane@example.com') + error.name = 'jane@example.com\nforged log line' + throw error }, }, }) @@ -517,6 +551,14 @@ describe('collectFeedback (send_feedback virtual tool)', () => { expect(result.content[0].text).toContain('recorded') + // No part of the thrown value reaches the log. Both its message and its + // mutable name can contain agent-controlled PII or log-forging newlines. + const warning = logged.find((line) => line.includes('onFeedback handler threw')) + expect(warning).toBe('Warning: onFeedback handler threw; returning the default acknowledgement') + expect(warning).not.toContain('backend down') + expect(warning).not.toContain('jane@example.com') + expect(warning).not.toContain('forged log line') + await new Promise((r) => setTimeout(r, 50)) expect(capture.findCapturesByEvent(PostHogMCPAnalyticsEvent.Feedback)).toHaveLength(1) @@ -701,6 +743,33 @@ describe('PostHogMCP (custom dispatcher path)', () => { await posthog.shutdown() }) + it('a supplied originalTool wins a feedback-name collision', async () => { + const posthog = newClient({ collectFeedback: true }) + + const realTools = [ + { + name: SEND_FEEDBACK, + inputSchema: { type: 'object', properties: { note: { type: 'string' } } }, + }, + ] + const listed = posthog.prepareToolList(realTools, { collectFeedback: true }) + expect(listed).toHaveLength(1) + + // The descriptor comes from the host's original list, not the prepared list. + // This is stateless proof that the real application tool owns the name. + const originalTool = realTools.find((tool) => tool.name === SEND_FEEDBACK) + const collided = posthog.prepareToolCall(SEND_FEEDBACK, { note: 'hi' }, { originalTool }) + expect(collided.isFeedback).toBe(false) + expect(collided.feedbackReport).toBeUndefined() + + // Without originalTool the name match stands. + const virtual = posthog.prepareToolCall(SEND_FEEDBACK, { feedback_type: 'other', summary: 'A note.' }) + expect(virtual.isFeedback).toBe(true) + expect(virtual.feedbackReport).toBeDefined() + + await posthog.shutdown() + }) + it('captureFeedback emits $mcp_feedback with the report properties and intent', async () => { const posthog = newClient({ collectFeedback: { toolName: 'agent-feedback', extraProperties: { product_area: { type: 'string' } } }, diff --git a/packages/mcp/src/extensions/feedback.ts b/packages/mcp/src/extensions/feedback.ts index 954a2dd58d..db11c08119 100644 --- a/packages/mcp/src/extensions/feedback.ts +++ b/packages/mcp/src/extensions/feedback.ts @@ -184,7 +184,13 @@ function parseSentiment(value: unknown): FeedbackSentiment | undefined { */ function matchesExtraSchema(value: unknown, schema: FeedbackExtraPropertySchema): boolean { const type = Array.isArray(value) ? 'array' : value === null ? 'null' : typeof value - if (schema.type !== type && !(schema.type === 'integer' && typeof value === 'number')) { + if (schema.type === 'integer') { + // A JSON Schema `integer` accepts a whole-valued number (`3`, `3.0`) but + // not a fractional one (`3.5`) — `typeof` alone can't tell them apart. + if (!Number.isInteger(value)) { + return false + } + } else if (schema.type !== type) { return false } return !Array.isArray(schema.enum) || schema.enum.includes(value as string) @@ -341,8 +347,10 @@ export async function handleFeedback( if (typeof reply === 'string' && reply.trim()) { return { content: [{ type: 'text' as const, text: reply }] } } - } catch (error) { - logger(`Warning: onFeedback handler threw; returning the default acknowledgement - ${error}`) + } catch { + // The whole thrown value is host-controlled and mutable. Do not log any + // part of it because it can contain PII or log-forging newlines. + logger('Warning: onFeedback handler threw; returning the default acknowledgement') } } return sendFeedbackResult() diff --git a/packages/mcp/src/extensions/posthog-mcp.ts b/packages/mcp/src/extensions/posthog-mcp.ts index 2e237f2772..d80f939dbd 100644 --- a/packages/mcp/src/extensions/posthog-mcp.ts +++ b/packages/mcp/src/extensions/posthog-mcp.ts @@ -278,8 +278,12 @@ export class PostHogMCP extends PostHog { * * @example * ```ts + * const originalTool = myTools.find((tool) => tool.name === name) * const { intent, intentSource, llmModel, llmModelSource, args, isMissingCapability } = - * posthog.prepareToolCall(name, rawArgs, { requestMeta: request.params?._meta }) + * posthog.prepareToolCall(name, rawArgs, { + * originalTool, + * requestMeta: request.params?._meta, + * }) * if (isMissingCapability) { * posthog.captureMissingCapability({ context: intent, llmModel, llmModelSource, ...identity }) * return getMoreToolsResult() @@ -304,7 +308,13 @@ export class PostHogMCP extends PostHog { ? resolveModel({ params: { arguments: args, _meta: options.requestMeta } }, ownsModel) : undefined const strippedArgs = stripContext(args) - const isFeedback = this.#feedbackOptions !== undefined && name === this.#feedbackToolName + // A supplied `originalTool` is a real application tool by this name (it + // comes from the host's own list, which never holds the virtual tool), so + // the real tool wins — the stateless twin of instrument()'s listing-derived + // collision handling. Without it the name match stands, and the documented + // remedy for a collision is configuring a non-colliding `toolName`. + const isFeedback = + this.#feedbackOptions !== undefined && name === this.#feedbackToolName && options.originalTool == null return { intent, intentSource: intent ? 'context_parameter' : undefined, diff --git a/packages/mcp/src/types.ts b/packages/mcp/src/types.ts index 6acd5f7be7..01361f17c5 100644 --- a/packages/mcp/src/types.ts +++ b/packages/mcp/src/types.ts @@ -689,8 +689,11 @@ export interface PrepareToolListOptions { /** Options for {@link PostHogMCP.prepareToolCall}. */ export interface PrepareToolCallOptions { /** - * The tool descriptor before PostHog preparation. Pass this on stateless or + * The tool descriptor before PostHog preparation, from the host's own tool + * list (the SDK's virtual tools never exist there). Pass this on stateless or * multi-replica servers so SDK argument ownership is resolved per request. + * Passing it also disambiguates a feedback-tool name collision: a real tool + * by that name is dispatched normally instead of being flagged as feedback. */ originalTool?: { inputSchema?: unknown } /** The incoming `tools/call` request's `_meta`, used for recognized client model metadata. */ @@ -717,8 +720,10 @@ export interface PreparedToolCall { isMissingCapability: boolean /** * True when `name` is the `send_feedback` virtual tool AND the constructor's - * `collectFeedback` option is set. Always false without that opt-in, so a real - * tool that happens to use the name is never shadowed. + * `collectFeedback` option is set AND no `originalTool` was supplied. Always + * false without that opt-in — and a supplied `originalTool` proves a real + * application tool owns the name — so a real tool that happens to use the + * name is never shadowed. */ isFeedback: boolean /**