From fd632f17ee6f64beb4fb7e52c11d6f545bb627c4 Mon Sep 17 00:00:00 2001 From: Georgis Andonis Date: Fri, 11 Sep 2026 13:02:50 +0300 Subject: [PATCH 1/4] fix(mcp): harden send_feedback against integer-extra drift, error-log echo, and dispatcher shadowing - matchesExtraSchema: a declared "integer" extra accepted any number, fractional ones included (3.5); now requires a whole value per JSON Schema semantics. - handleFeedback: a thrown onFeedback was logged with its full message, which can echo agent-controlled report text (PII, credentials, log-forging newlines) into host logs; now logs only the error name, matching the report log beside it. - prepareToolCall: every call named like the feedback tool was flagged as feedback, so a dispatcher following the documented flow suppressed a real colliding tool; a host-supplied originalTool is stateless proof a real tool owns the name and now wins. Backports the hardening found while porting the feature to posthog-python (PostHog/posthog-python#939). Generated-By: PostHog Desktop Task-Id: 0b5063cb-6fcf-4364-be5e-de945b1448f0 --- .changeset/mcp-feedback-hardening.md | 5 ++ packages/mcp/src/__tests__/feedback.test.ts | 59 ++++++++++++++++++++- packages/mcp/src/extensions/feedback.ts | 15 +++++- packages/mcp/src/extensions/posthog-mcp.ts | 8 ++- packages/mcp/src/types.ts | 11 ++-- 5 files changed, 91 insertions(+), 7 deletions(-) create mode 100644 .changeset/mcp-feedback-hardening.md diff --git a/.changeset/mcp-feedback-hardening.md b/.changeset/mcp-feedback-hardening.md new file mode 100644 index 0000000000..c100323e84 --- /dev/null +++ b/.changeset/mcp-feedback-hardening.md @@ -0,0 +1,5 @@ +--- +'@posthog/mcp': patch +--- + +Harden the send_feedback tool: a declared `integer` extra now rejects fractional numbers, a thrown `onFeedback` handler logs only the exception type (its message can echo agent-controlled report text), and `prepareToolCall` treats a supplied `originalTool` as proof a real application tool owns the feedback name so the real tool is dispatched instead of being flagged as feedback. diff --git a/packages/mcp/src/__tests__/feedback.test.ts b/packages/mcp/src/__tests__/feedback.test.ts index 52d70c542d..afec68fe52 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,12 @@ 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') + throw new Error('backend down for jane@example.com') }, }, }) @@ -517,6 +549,13 @@ describe('collectFeedback (send_feedback virtual tool)', () => { expect(result.content[0].text).toContain('recorded') + // Only the exception's type reaches the log — an error message can echo + // agent-controlled report text (PII, log-forging newlines). + const warning = logged.find((line) => line.includes('onFeedback handler threw')) + expect(warning).toContain('Error') + expect(warning).not.toContain('backend down') + expect(warning).not.toContain('jane@example.com') + await new Promise((r) => setTimeout(r, 50)) expect(capture.findCapturesByEvent(PostHogMCPAnalyticsEvent.Feedback)).toHaveLength(1) @@ -701,6 +740,24 @@ describe('PostHogMCP (custom dispatcher path)', () => { await posthog.shutdown() }) + it('a supplied originalTool wins a feedback-name collision', async () => { + const posthog = newClient({ collectFeedback: true }) + + // The host holds a real tool by the feedback name and passes it through — + // stateless proof of ownership, so the call dispatches as a real tool. + const realTool = { inputSchema: { type: 'object', properties: { note: { type: 'string' } } } } + const collided = posthog.prepareToolCall(SEND_FEEDBACK, { note: 'hi' }, { originalTool: realTool }) + 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..e4a98d8cd6 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) @@ -342,7 +348,12 @@ export async function handleFeedback( return { content: [{ type: 'text' as const, text: reply }] } } } catch (error) { - logger(`Warning: onFeedback handler threw; returning the default acknowledgement - ${error}`) + // Only the exception's type, matching the report log above: a handler can + // echo the unsanitized report (PII, credentials, log-forging newlines, + // unbounded length) into its error message, and that agent-controlled + // text does not belong in host logs any more than `report.summary` does. + const errorName = error instanceof Error ? error.name : typeof error + logger(`Warning: onFeedback handler threw (${errorName}); 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..aa2eca9a66 100644 --- a/packages/mcp/src/extensions/posthog-mcp.ts +++ b/packages/mcp/src/extensions/posthog-mcp.ts @@ -304,7 +304,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 /** From 8f1702d29c43fa50081b271ae82323139a38506d Mon Sep 17 00:00:00 2001 From: Georgis Andonis <6430745+gesh@users.noreply.github.com> Date: Fri, 11 Sep 2026 17:39:23 +0300 Subject: [PATCH 2/4] fix(mcp): avoid logging thrown feedback values Generated-By: PostHog Desktop Task-Id: 9a0c57df-9c72-446a-acb4-cb319a85d5d7 --- packages/mcp/src/__tests__/feedback.test.ts | 11 +++++++---- packages/mcp/src/extensions/feedback.ts | 11 ++++------- 2 files changed, 11 insertions(+), 11 deletions(-) diff --git a/packages/mcp/src/__tests__/feedback.test.ts b/packages/mcp/src/__tests__/feedback.test.ts index afec68fe52..c01122579f 100644 --- a/packages/mcp/src/__tests__/feedback.test.ts +++ b/packages/mcp/src/__tests__/feedback.test.ts @@ -540,7 +540,9 @@ describe('collectFeedback (send_feedback virtual tool)', () => { logger: (message: string) => logged.push(message), collectFeedback: { onFeedback: async () => { - throw new Error('backend down for jane@example.com') + const error = new Error('backend down for jane@example.com') + error.name = 'jane@example.com\nforged log line' + throw error }, }, }) @@ -549,12 +551,13 @@ describe('collectFeedback (send_feedback virtual tool)', () => { expect(result.content[0].text).toContain('recorded') - // Only the exception's type reaches the log — an error message can echo - // agent-controlled report text (PII, log-forging newlines). + // 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).toContain('Error') + 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) diff --git a/packages/mcp/src/extensions/feedback.ts b/packages/mcp/src/extensions/feedback.ts index e4a98d8cd6..db11c08119 100644 --- a/packages/mcp/src/extensions/feedback.ts +++ b/packages/mcp/src/extensions/feedback.ts @@ -347,13 +347,10 @@ export async function handleFeedback( if (typeof reply === 'string' && reply.trim()) { return { content: [{ type: 'text' as const, text: reply }] } } - } catch (error) { - // Only the exception's type, matching the report log above: a handler can - // echo the unsanitized report (PII, credentials, log-forging newlines, - // unbounded length) into its error message, and that agent-controlled - // text does not belong in host logs any more than `report.summary` does. - const errorName = error instanceof Error ? error.name : typeof error - logger(`Warning: onFeedback handler threw (${errorName}); returning the default acknowledgement`) + } 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() From 8fd81980794027cc2650889a36269d1ca203d1ef Mon Sep 17 00:00:00 2001 From: Georgis Andonis <6430745+gesh@users.noreply.github.com> Date: Fri, 11 Sep 2026 17:40:45 +0300 Subject: [PATCH 3/4] refactor: apply simplify pass Generated-By: PostHog Desktop Task-Id: 9a0c57df-9c72-446a-acb4-cb319a85d5d7 --- .changeset/mcp-feedback-hardening.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.changeset/mcp-feedback-hardening.md b/.changeset/mcp-feedback-hardening.md index c100323e84..3d4a3304e6 100644 --- a/.changeset/mcp-feedback-hardening.md +++ b/.changeset/mcp-feedback-hardening.md @@ -2,4 +2,4 @@ '@posthog/mcp': patch --- -Harden the send_feedback tool: a declared `integer` extra now rejects fractional numbers, a thrown `onFeedback` handler logs only the exception type (its message can echo agent-controlled report text), and `prepareToolCall` treats a supplied `originalTool` as proof a real application tool owns the feedback name so the real tool is dispatched instead of being flagged as feedback. +Harden feedback validation, error handling, and tool-name collision routing. From 382e0845220e906f57935fca5c69bb4b49405232 Mon Sep 17 00:00:00 2001 From: Georgis Andonis <6430745+gesh@users.noreply.github.com> Date: Fri, 11 Sep 2026 17:57:01 +0300 Subject: [PATCH 4/4] fix(mcp): document feedback tool ownership Generated-By: PostHog Desktop Task-Id: 9a0c57df-9c72-446a-acb4-cb319a85d5d7 --- packages/mcp/README.md | 7 ++++++- packages/mcp/src/__tests__/feedback.test.ts | 17 +++++++++++++---- packages/mcp/src/extensions/posthog-mcp.ts | 6 +++++- 3 files changed, 24 insertions(+), 6 deletions(-) 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 c01122579f..a514ab9ad5 100644 --- a/packages/mcp/src/__tests__/feedback.test.ts +++ b/packages/mcp/src/__tests__/feedback.test.ts @@ -746,10 +746,19 @@ describe('PostHogMCP (custom dispatcher path)', () => { it('a supplied originalTool wins a feedback-name collision', async () => { const posthog = newClient({ collectFeedback: true }) - // The host holds a real tool by the feedback name and passes it through — - // stateless proof of ownership, so the call dispatches as a real tool. - const realTool = { inputSchema: { type: 'object', properties: { note: { type: 'string' } } } } - const collided = posthog.prepareToolCall(SEND_FEEDBACK, { note: 'hi' }, { originalTool: realTool }) + 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() diff --git a/packages/mcp/src/extensions/posthog-mcp.ts b/packages/mcp/src/extensions/posthog-mcp.ts index aa2eca9a66..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()