diff --git a/package-lock.json b/package-lock.json index cd2fae1..91f0c5b 100644 --- a/package-lock.json +++ b/package-lock.json @@ -1,12 +1,12 @@ { "name": "@notionhq/notion-mcp-server", - "version": "2.4.0", + "version": "2.4.1", "lockfileVersion": 3, "requires": true, "packages": { "": { "name": "@notionhq/notion-mcp-server", - "version": "2.4.0", + "version": "2.4.1", "license": "MIT", "dependencies": { "@modelcontextprotocol/sdk": "^1.25.1", diff --git a/package.json b/package.json index 899fe7e..97d1d0a 100644 --- a/package.json +++ b/package.json @@ -6,7 +6,7 @@ "mcp", "server" ], - "version": "2.4.0", + "version": "2.4.1", "license": "MIT", "type": "module", "scripts": { diff --git a/src/openapi-mcp-server/mcp/__tests__/proxy.test.ts b/src/openapi-mcp-server/mcp/__tests__/proxy.test.ts index 65cd0a4..8e48d49 100644 --- a/src/openapi-mcp-server/mcp/__tests__/proxy.test.ts +++ b/src/openapi-mcp-server/mcp/__tests__/proxy.test.ts @@ -834,5 +834,186 @@ describe('MCPProxy', () => { }, ) }) + + it('should handle API-create-a-comment parent provided as a JSON string', async () => { + const mockResponse = { + data: { id: 'new-comment-id' }, + status: 200, + headers: new Headers({ 'content-type': 'application/json' }), + } + ;(HttpClient.prototype.executeOperation as ReturnType).mockResolvedValue(mockResponse) + + ;(proxy as any).openApiLookup = { + 'API-create-a-comment': { + operationId: 'create-a-comment', + responses: { '200': { description: 'Success' } }, + method: 'post', + path: '/comments', + }, + } + + const server = (proxy as any).server + const handlers = server.setRequestHandler.mock.calls.flatMap((x: unknown[]) => x).filter((x: unknown) => typeof x === 'function') + const callToolHandler = handlers[1] + + // Some clients double-encode `parent` as a JSON string. Forwarding that to + // the Notion API makes it throw on `"block_id" in ` and return a + // 500, so deserialize it back to an object first. + const parentAsString = JSON.stringify({ page_id: '3870bb29-1a64-816b-8641-c87ca28062d0' }) + + await expect( + callToolHandler({ + params: { + name: 'API-create-a-comment', + arguments: { + parent: parentAsString, + rich_text: [{ text: { content: 'Hello' } }], + }, + }, + }), + ).resolves.toBeDefined() + + expect(HttpClient.prototype.executeOperation).toHaveBeenCalledWith( + expect.anything(), + expect.objectContaining({ + parent: { page_id: '3870bb29-1a64-816b-8641-c87ca28062d0' }, + }), + ) + }) + + it('should deserialize a stringified object nested inside an array element object', async () => { + const mockResponse = { + data: { id: 'new-page-id' }, + status: 200, + headers: new Headers({ 'content-type': 'application/json' }), + } + ;(HttpClient.prototype.executeOperation as ReturnType).mockResolvedValue(mockResponse) + + ;(proxy as any).openApiLookup = { + 'API-appendBlockChildren': { + operationId: 'appendBlockChildren', + responses: { '200': { description: 'Success' } }, + method: 'patch', + path: '/blocks/{block_id}/children', + }, + } + + const server = (proxy as any).server + const handlers = server.setRequestHandler.mock.calls.flatMap((x: unknown[]) => x).filter((x: unknown) => typeof x === 'function') + const callToolHandler = handlers[1] + + // The array element is a real object, but one of its properties is itself + // a stringified object. The previous shallow array handling left this as a + // string; the uniform recursive walk now normalizes it. + const children = [ + { + object: 'block', + type: 'paragraph', + paragraph: JSON.stringify({ rich_text: [{ type: 'text', text: { content: 'Hello' } }] }), + }, + ] + + await callToolHandler({ + params: { + name: 'API-appendBlockChildren', + arguments: { children }, + }, + }) + + expect(HttpClient.prototype.executeOperation).toHaveBeenCalledWith( + expect.anything(), + { + children: [ + { + object: 'block', + type: 'paragraph', + paragraph: { rich_text: [{ type: 'text', text: { content: 'Hello' } }] }, + }, + ], + }, + ) + }) + + it('should deserialize a double-stringified parent', async () => { + const mockResponse = { + data: { id: 'new-comment-id' }, + status: 200, + headers: new Headers({ 'content-type': 'application/json' }), + } + ;(HttpClient.prototype.executeOperation as ReturnType).mockResolvedValue(mockResponse) + + ;(proxy as any).openApiLookup = { + 'API-create-a-comment': { + operationId: 'create-a-comment', + responses: { '200': { description: 'Success' } }, + method: 'post', + path: '/comments', + }, + } + + const server = (proxy as any).server + const handlers = server.setRequestHandler.mock.calls.flatMap((x: unknown[]) => x).filter((x: unknown) => typeof x === 'function') + const callToolHandler = handlers[1] + + // A client that serialized `parent` twice: JSON.stringify(JSON.stringify(parent)). + const doubleEncodedParent = JSON.stringify(JSON.stringify({ page_id: '3870bb29-1a64-816b-8641-c87ca28062d0' })) + + await callToolHandler({ + params: { + name: 'API-create-a-comment', + arguments: { + parent: doubleEncodedParent, + rich_text: [{ text: { content: 'Hello' } }], + }, + }, + }) + + expect(HttpClient.prototype.executeOperation).toHaveBeenCalledWith( + expect.anything(), + expect.objectContaining({ + parent: { page_id: '3870bb29-1a64-816b-8641-c87ca28062d0' }, + }), + ) + }) + + it('should not coerce scalar or quoted-scalar string params', async () => { + const mockResponse = { + data: { success: true }, + status: 200, + headers: new Headers({ 'content-type': 'application/json' }), + } + ;(HttpClient.prototype.executeOperation as ReturnType).mockResolvedValue(mockResponse) + + ;(proxy as any).openApiLookup = { + 'API-search': { + operationId: 'search', + responses: { '200': { description: 'Success' } }, + method: 'post', + path: '/search', + }, + } + + const server = (proxy as any).server + const handlers = server.setRequestHandler.mock.calls.flatMap((x: unknown[]) => x).filter((x: unknown) => typeof x === 'function') + const callToolHandler = handlers[1] + + await callToolHandler({ + params: { + name: 'API-search', + arguments: { + // Looks like JSON scalars, but the schema wants strings: keep as-is + // rather than coercing to number/boolean or unwrapping the quotes. + count: '123', + flag: 'true', + quoted: '"hello"', + }, + }, + }) + + expect(HttpClient.prototype.executeOperation).toHaveBeenCalledWith( + expect.anything(), + { count: '123', flag: 'true', quoted: '"hello"' }, + ) + }) }) }) diff --git a/src/openapi-mcp-server/mcp/proxy.ts b/src/openapi-mcp-server/mcp/proxy.ts index 7469f88..0ff8004 100644 --- a/src/openapi-mcp-server/mcp/proxy.ts +++ b/src/openapi-mcp-server/mcp/proxy.ts @@ -25,62 +25,102 @@ type NewToolDefinition = { /** * Recursively deserialize stringified JSON values in parameters. - * This handles the case where MCP clients (like Cursor, Claude Code) double-serialize - * nested object parameters, sending them as JSON strings instead of objects. + * This handles the case where MCP clients (like Cursor, Claude Code, and some + * SDKs) double-serialize nested object/array parameters, sending them as JSON + * strings instead of structured values. + * + * The whole argument tree is walked uniformly: every object property and every + * array element is visited, JSON-looking strings are decoded, and the decoded + * result is walked again. This normalizes deeply nested cases — including a + * stringified object that sits inside an array element object (e.g. + * `{ children: [{ paragraph: '{"rich_text":[...]}' }] }`) and values that were + * JSON-encoded more than once (e.g. `JSON.stringify(JSON.stringify(parent))`) — + * before the request is forwarded to the Notion API. * * @see https://github.com/makenotion/notion-mcp-server/issues/176 */ function deserializeParams(params: Record): Record { const result: Record = {} - for (const [key, value] of Object.entries(params)) { - if (typeof value === 'string') { - // Check if the string looks like a JSON object or array - const trimmed = value.trim() - if ((trimmed.startsWith('{') && trimmed.endsWith('}')) || - (trimmed.startsWith('[') && trimmed.endsWith(']'))) { - try { - const parsed = JSON.parse(value) - // Only use parsed value if it's an object or array - if (typeof parsed === 'object' && parsed !== null) { - // Recursively deserialize nested objects - result[key] = Array.isArray(parsed) - ? parsed - : deserializeParams(parsed as Record) - continue - } - } catch { - // If parsing fails, keep the original string value - } - } - } else if (Array.isArray(value)) { - // Deserialize any JSON-string items within the array - result[key] = value.map((item) => { - if (typeof item !== 'string') return item - const trimmed = item.trim() - if ( - (trimmed.startsWith('{') && trimmed.endsWith('}')) || - (trimmed.startsWith('[') && trimmed.endsWith(']')) - ) { - try { - const parsed = JSON.parse(item) - if (typeof parsed === 'object' && parsed !== null) { - return Array.isArray(parsed) - ? parsed - : deserializeParams(parsed as Record) - } - } catch { - // If parsing fails, keep the original string item - } - } - return item - }) - continue + result[key] = deserializeValue(value) + } + return result +} + +/** + * Normalize a single value: decode a JSON-encoded string into the structured + * value it represents (recursing into the result), walk into every array + * element, and walk into every nested object property. Non-JSON strings and + * scalars are returned unchanged, so values the schema legitimately wants as + * strings (and numbers/booleans encoded as strings) are left intact. + */ +function deserializeValue(value: unknown): unknown { + if (typeof value === 'string') { + return unwrapJsonString(value) + } + + if (Array.isArray(value)) { + return value.map(deserializeValue) + } + + if (typeof value === 'object' && value !== null) { + const result: Record = {} + for (const [key, nested] of Object.entries(value)) { + result[key] = deserializeValue(nested) } - result[key] = value + return result } - return result + return value +} + +// Bound how many JSON-decode passes we attempt on a single string. One pass +// handles the common single-encoding; extra passes absorb double/triple +// serialization without unbounded work on adversarial input. +const MAX_UNWRAP_DEPTH = 3 + +/** + * Resolve a (possibly multiply-)JSON-encoded string to the object or array it + * represents. Only strings that ultimately decode to an object or array are + * transformed (and then recursively normalized); a string that decodes to a + * scalar (number/boolean/null) or to another plain string is returned + * unchanged, so genuine string values are never corrupted. + */ +function unwrapJsonString(value: string): unknown { + let current = value + for (let depth = 0; depth < MAX_UNWRAP_DEPTH; depth++) { + const trimmed = current.trim() + // Only attempt a parse when the string could encode an object/array + // (`{...}`/`[...]`) or wrap one in a JSON string literal (`"..."`). This + // skips the common case of ordinary text without touching JSON.parse. + const couldBeEncoded = + (trimmed.startsWith('{') && trimmed.endsWith('}')) || + (trimmed.startsWith('[') && trimmed.endsWith(']')) || + (trimmed.startsWith('"') && trimmed.endsWith('"')) + if (!couldBeEncoded) { + break + } + + let parsed: unknown + try { + parsed = JSON.parse(trimmed) + } catch { + break + } + + if (typeof parsed === 'object' && parsed !== null) { + return deserializeValue(parsed) + } + if (typeof parsed === 'string') { + // Peeled one layer of JSON-string encoding; loop to see whether it wraps + // a structured value (double-encoding). + current = parsed + continue + } + // Decoded to a scalar — not a structured value; leave the original intact. + break + } + return value } // import this class, extend and return server