Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 32 additions & 1 deletion packages/worker/src/mcp/executor.node.test.ts
Original file line number Diff line number Diff line change
@@ -1,11 +1,16 @@
import { expect, test } from 'vitest'
import { type ContentBlock } from '@modelcontextprotocol/sdk/types.js'
import {
createCapabilitySecretAccessDeniedBatchMessage,
createCapabilitySecretAccessDeniedMessage,
createHostSecretAccessDeniedBatchMessage,
createMissingSecretMessage,
} from '#mcp/secrets/errors.ts'
import { formatExecutionOutput, getExecutionErrorDetails } from './executor.ts'
import {
extractRawContent,
formatExecutionOutput,
getExecutionErrorDetails,
} from './executor.ts'

test('getExecutionErrorDetails returns concrete guidance for capability access denial', () => {
const error = new Error(
Expand Down Expand Up @@ -59,6 +64,32 @@ test('formatExecutionOutput keeps missing secret guidance intact', () => {
)
})

test('extractRawContent returns MCP content blocks from sentinel result', () => {
const content: Array<ContentBlock> = [
{
type: 'image',
data: 'iVBORw0KGgoAAAANSUhEUgAAAAEAAAAB',
mimeType: 'image/png',
},
{
type: 'text',
text: 'Screenshot of https://example.com',
},
]

expect(
extractRawContent({
__mcpContent: content,
}),
).toEqual(content)
})

test('extractRawContent returns null for non-sentinel values', () => {
expect(extractRawContent({ result: 'not raw content' })).toBeNull()
expect(extractRawContent('plain text')).toBeNull()
expect(extractRawContent(null)).toBeNull()
})

test('getExecutionErrorDetails returns batch capability approvals', () => {
const error = new Error(
createCapabilitySecretAccessDeniedBatchMessage([
Expand Down
13 changes: 13 additions & 0 deletions packages/worker/src/mcp/executor.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import { DynamicWorkerExecutor, type ExecuteResult } from '@cloudflare/codemode'
import { type ContentBlock } from '@modelcontextprotocol/sdk/types.js'
import { exports as workerExports } from 'cloudflare:workers'
type WorkerLoopbackExports = Exclude<typeof workerExports, undefined>
import { type FetchGatewayProps } from '#mcp/fetch-gateway.ts'
Expand Down Expand Up @@ -212,6 +213,18 @@ export function formatExecutionOutput(result: ExecuteResult) {
return truncateExecutionResult(result.result)
}

export function extractRawContent(value: unknown): Array<ContentBlock> | null {
if (
typeof value === 'object' &&
value !== null &&
'__mcpContent' in value &&
Array.isArray((value as { __mcpContent: unknown }).__mcpContent)
) {
return (value as { __mcpContent: Array<ContentBlock> }).__mcpContent
}
Comment on lines +216 to +224

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

cat packages/worker/src/mcp/executor.ts | head -n 230

Repository: kentcdodds/kody

Length of output: 6376


🏁 Script executed:

rg "ContentBlock" packages/worker/src/mcp/ -A 2 -B 2

Repository: kentcdodds/kody

Length of output: 5852


🏁 Script executed:

rg "extractRawContent" packages/worker/src/mcp/ -A 3 -B 1

Repository: kentcdodds/kody

Length of output: 2780


🏁 Script executed:

cat packages/worker/src/mcp/executor.node.test.ts | grep -A 20 "extractRawContent returns MCP"

Repository: kentcdodds/kody

Length of output: 512


🏁 Script executed:

cat packages/worker/src/mcp/tools/tool-response-content.ts | head -n 50

Repository: kentcdodds/kody

Length of output: 749


🏁 Script executed:

rg "type ContentBlock|interface ContentBlock" --type ts

Repository: kentcdodds/kody

Length of output: 725


Validate structure of MCP content blocks before returning from extractRawContent.

The function only verifies that __mcpContent is an array but does not validate individual block structure. Blocks missing required fields (e.g., text in text blocks, or data/mimeType in image blocks) will pass through unchecked and cause errors downstream when used by prependToolMetadataContent or other handlers.

Add a validation helper:

Proposed fix
+function isContentBlockArray(value: unknown): value is Array<ContentBlock> {
+	return (
+		Array.isArray(value) &&
+		value.every((block) => {
+			if (typeof block !== 'object' || block === null) return false
+			const candidate = block as Record<string, unknown>
+			if (candidate.type === 'text') return typeof candidate.text === 'string'
+			if (candidate.type === 'image') {
+				return (
+					typeof candidate.data === 'string' &&
+					typeof candidate.mimeType === 'string'
+				)
+			}
+			return false
+		})
+	)
+}
+
 export function extractRawContent(value: unknown): Array<ContentBlock> | null {
 	if (
 		typeof value === 'object' &&
 		value !== null &&
-		'__mcpContent' in value &&
-		Array.isArray((value as { __mcpContent: unknown }).__mcpContent)
+		Object.hasOwn(value, '__mcpContent') &&
+		isContentBlockArray((value as { __mcpContent: unknown }).__mcpContent)
 	) {
 		return (value as { __mcpContent: Array<ContentBlock> }).__mcpContent
 	}
 	return null
 }
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@packages/worker/src/mcp/executor.ts` around lines 216 - 224,
extractRawContent currently only checks that value.__mcpContent is an array but
doesn't validate each ContentBlock, letting malformed blocks propagate to
consumers like prependToolMetadataContent; add a validation step in
extractRawContent that iterates the array and rejects the whole payload (return
null) if any block fails validation. Implement/invoke a helper (e.g.,
validateContentBlock(block): boolean) that checks block.type and required fields
per type (for example: for 'text' ensure a non-empty text string; for 'image'
ensure data and mimeType are present and of correct types; for other types
validate their mandatory keys) and only return the cast Array<ContentBlock> when
every block passes validation. Ensure the helper is used by extractRawContent
before returning the array.

return null
}

function stringifyExecutionError(error: unknown) {
return error instanceof Error ? error.message : String(error)
}
Expand Down
124 changes: 119 additions & 5 deletions packages/worker/src/mcp/tools/execute.node.test.ts
Original file line number Diff line number Diff line change
@@ -1,15 +1,43 @@
import { expect, test, vi } from 'vitest'
import { registerExecuteTool } from './execute.ts'
import { type ContentBlock } from '@modelcontextprotocol/sdk/types.js'
import { beforeEach, expect, test, vi } from 'vitest'

test('registers execute tool', async () => {
const mockModule = vi.hoisted(() => ({
runCodemodeWithRegistry: vi.fn(),
getCapabilityRegistryForContext: vi.fn(async () => ({
capabilityHandlers: {
page_to_markdown: true,
},
})),
}))

vi.mock('#mcp/run-codemode-registry.ts', () => ({
runCodemodeWithRegistry: (...args: Array<unknown>) =>
mockModule.runCodemodeWithRegistry(...args),
}))

vi.mock('#mcp/capabilities/registry.ts', () => ({
getCapabilityRegistryForContext: (...args: Array<unknown>) =>
mockModule.getCapabilityRegistryForContext(...args),
}))

const { registerExecuteTool } = await import('./execute.ts')

beforeEach(() => {
vi.clearAllMocks()
})

async function getExecuteHandler() {
const registerTool = vi.fn()

await registerExecuteTool({
server: {
registerTool,
} as never,
getEnv: vi.fn(),
getCallerContext: vi.fn(),
getEnv: vi.fn(() => ({})),
getCallerContext: vi.fn(() => ({
baseUrl: 'https://example.com',
user: null,
})),
requireDomain: vi.fn(),
getLoopbackExports: vi.fn(),
} as never)
Expand All @@ -18,4 +46,90 @@ test('registers execute tool', async () => {
const [name, , handler] = registerTool.mock.calls[0] ?? []
expect(name).toBe('execute')
expect(typeof handler).toBe('function')
return handler as (input: {
code: string
conversationId?: string
}) => Promise<{
content: Array<ContentBlock>
structuredContent: {
conversationId: string
result: unknown
logs: Array<unknown>
}
isError: boolean
}>
}

test('registers execute tool', async () => {
await getExecuteHandler()
})

test('execute tool passes through raw MCP content blocks in success responses', async () => {
const handler = await getExecuteHandler()
const rawContent: Array<ContentBlock> = [
{
type: 'image',
data: 'iVBORw0KGgoAAAANSUhEUgAAAAEAAAAB',
mimeType: 'image/png',
},
{
type: 'text',
text: 'Screenshot of https://example.com',
},
]
mockModule.runCodemodeWithRegistry.mockResolvedValueOnce({
result: {
__mcpContent: rawContent,
},
logs: [{ level: 'info', message: 'captured screenshot' }],
})

const response = await handler({
code: 'async () => ({ __mcpContent: [] })',
conversationId: 'conv-123',
})

expect(response.isError).toBe(false)
expect(response.content).toEqual([
{
type: 'text',
text: 'conversationId: conv-123',
},
...rawContent,
])
expect(response.structuredContent).toEqual({
conversationId: 'conv-123',
result: null,
logs: [{ level: 'info', message: 'captured screenshot' }],
})
})

test('execute tool keeps serializing normal success results as text', async () => {
const handler = await getExecuteHandler()
mockModule.runCodemodeWithRegistry.mockResolvedValueOnce({
result: { ok: true },
logs: [],
})

const response = await handler({
code: 'async () => ({ ok: true })',
conversationId: 'conv-456',
})

expect(response.isError).toBe(false)
expect(response.content).toEqual([
{
type: 'text',
text: 'conversationId: conv-456',
},
{
type: 'text',
text: '{\n "ok": true\n}',
},
])
expect(response.structuredContent).toEqual({
conversationId: 'conv-456',
result: { ok: true },
logs: [],
})
})
14 changes: 9 additions & 5 deletions packages/worker/src/mcp/tools/execute.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import * as Sentry from '@sentry/cloudflare'
import { type ToolAnnotations } from '@modelcontextprotocol/sdk/types.js'
import { z } from 'zod'
import {
extractRawContent,
formatExecutionOutput,
getExecutionErrorDetails,
} from '#mcp/executor.ts'
Expand Down Expand Up @@ -189,17 +190,20 @@ export async function registerExecuteTool(agent: McpRegistrationAgent) {
registeredCapabilityCount,
sandboxError: false,
})
const rawContent = extractRawContent(result.result)
return {
content: prependToolMetadataContent(resolvedConversationId, [
{
type: 'text',
text: formatExecutionOutput(result),
},
...(rawContent ?? [
{
type: 'text',
text: formatExecutionOutput(result),
},
]),
Comment on lines +193 to +201

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Add a size guard for raw MCP content responses.

Line 196 now bypasses the truncation path used by formatExecutionOutput, so very large __mcpContent payloads can exceed response limits and fail tool responses.

Suggested bounded fallback
 			const rawContent = extractRawContent(result.result)
+			const MAX_RAW_CONTENT_BYTES = 200_000
+			const rawContentJson =
+				rawContent === null ? null : JSON.stringify(rawContent)
+			const safeRawContent =
+				rawContentJson && rawContentJson.length <= MAX_RAW_CONTENT_BYTES
+					? rawContent
+					: null
 			return {
 				content: prependToolMetadataContent(resolvedConversationId, [
-					...(rawContent ?? [
+					...(safeRawContent ?? [
 						{
 							type: 'text',
 							text: formatExecutionOutput(result),
 						},
 					]),
 					...formatSurfacedMemoriesMarkdown(surfacedMemories),
 				]),
 				structuredContent: {
 					conversationId: resolvedConversationId,
-					result: rawContent ? null : result.result,
+					result: safeRawContent ? null : result.result,
 					logs: result.logs ?? [],
 					...buildMemoryStructuredContent(surfacedMemories),
 				},
 				isError: false,
 			}

Also applies to: 206-206

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@packages/worker/src/mcp/tools/execute.ts` around lines 193 - 201, The code is
returning raw MCP content from extractRawContent without enforcing a size limit,
bypassing the truncation used by formatExecutionOutput; update the block that
builds the returned content (where prependToolMetadataContent is called with
resolvedConversationId and rawContent from extractRawContent(result.result)) to
detect oversized rawContent (or individual items) and, when exceeding safe
limits, replace/truncate it by calling formatExecutionOutput(result) or
otherwise trim the payload to the bounded fallback used elsewhere so the
response cannot exceed limits; ensure the same guard is applied to the analogous
path around line 206.

...formatSurfacedMemoriesMarkdown(surfacedMemories),
]),
structuredContent: {
conversationId: resolvedConversationId,
result: result.result,
result: rawContent ? null : result.result,
logs: result.logs ?? [],
...buildMemoryStructuredContent(surfacedMemories),
},
Expand Down
Loading