Repository navigation
fix(mcp): validate tool arguments with zod and constrain http methods - #555
Conversation
|
@esanbasirat Great news! 🎉 Based on an automated assessment of this PR, the linked Wave issue(s) no longer count against your application limits. You can now already apply to more issues while waiting for a review of this PR. Keep up the great work! 🚀 |
…vertised schema from it `bazaar_search` and `paid_call` coerced their arguments with `String()`, so `url: 42` became the string "42" and `url` missing entirely became the string "undefined" — both of which were then fetched. Arguments are now parsed with a Zod schema before the handler runs, and a failure answers ERR_BAD_REQUEST naming the offending argument. The part I like most is that `inputSchema` is now generated from the same Zod object via `zodToToolInputSchema()` rather than hand-written beside it. The advertised schema for `paid_call` already claimed `additionalProperties: false` while the handler accepted anything, which is exactly the drift #411 is about; one source of truth is what stops it coming back. `method` is now an enum of the seven HTTP methods instead of any string. Security posture is unchanged where it matters: the SSRF guard and the `checkPaidCallTarget` allow-list still run, and this only refuses malformed calls earlier than they would have. One test updated while merging. `tests/mcp-paid-call-allowlist.test.ts` asserted that a call smuggling `paidCallAllowlist: ['evil.test']` fails with ERR_HOST_NOT_ALLOWED; with `.strict()` the unrecognised keys are now refused as ERR_BAD_REQUEST before the host check runs. The security property is intact and arguably stronger — unknown arguments are rejected rather than ignored, and no payment leaves the process either way — so the assertion now accepts either code, matching the precedent already in that file. The host check keeps its nine other assertions. Verified on the merge result: `tsc --noEmit` silent, 834 tests pass. The two remaining failures (`tests/openapi.test.ts` HTTP-method coverage and `graphqlSchemaDrift`) fail identically on clean `main` and are not from this PR. Closes #411 Claude-Session: https://claude.ai/code/session_01USgemLt4Rnz4SGB1Srf3GB
|
Merged. Good, focused PR — one issue, three files, up to date with The part worth calling out: generating Also right that I checked that this doesn't weaken anything: One test I updated while merging
That's the test encoding a code, not a security regression. The property it exists to protect — an argument cannot widen the allow-list, and no payment leaves the process — holds either way, and is arguably stronger now: unknown arguments are rejected rather than silently ignored. I changed it to accept either code, which is the precedent already used a few lines above it, and the host check keeps its nine other assertions so no coverage is lost. Worth knowing for next time: it's the kind of break that only shows up in the full suite, not in the file you're working on. Running the whole thing before pushing would have caught it. One nit, not worth a change
Verified on the merge result rather than your branch: Closes #411. |
The MCP server reported a hard-coded `version: '1.0.0'` while the package was at 0.2.0, so every client was told something untrue about what it was talking to. The version now comes from `package.json`, and the name becomes `lens-mcp-server` — a client-affecting rename the PR flagged as such. Two fixes while merging: - **It did not compile.** The version read used `fileURLToPath(import.meta.url)`, and this project is `module: commonjs`, under which `import.meta` is `error TS1343`. Switched to `__dirname`, which has precedent in three existing files under `src/__tests__` — and which the PR's own new test already used, so the source was the odd one out. `../../package.json` reaches the repo root from both `src/mcp/` and `dist/mcp/`, since `rootDir` is `src` and `outDir` is `dist`. - **The read is now non-fatal.** It runs at module load, so an unreadable or unparseable `package.json` would have stopped the server booting over a version string. Falls back to `0.0.0-unknown`, which is self-describing in a client's log. Test conflict against #555 resolved by keeping both: the `ListToolsRequestSchema` and `PAID_CALL_HTTP_METHODS` imports that PR added, plus this one's new server-identity block. Verified on the merge result: `tsc --noEmit` silent, 889 tests pass across 82 suites, nothing red. Claude-Session: https://claude.ai/code/session_01USgemLt4Rnz4SGB1Srf3GB
…l error handling `bazaar_search` awaited `ssrfSafeFetch` with no timeout, so an upstream that accepted the connection and then went quiet hung the tool call indefinitely — and `await res.json()` on a non-JSON body threw a raw parse error at the MCP client. Now: a 10s `AbortSignal.timeout` matching the house deadline in `src/alerts/delivery.ts`, a content-type check before parsing, a guarded `res.json()`, and timeouts distinguished from other failures so the message says "did not respond within 10000ms" rather than something about JSON. Every error now names the upstream host. That is the caller's own `url` argument coming back to them, so it reveals nothing they did not supply, and it is the difference between a debuggable failure and "ERR_SERVICE_UNREACHABLE". Two merge resolutions, both against work that landed earlier today: - `tests/mcp.test.ts` conflicted with #555's validation suite. Additive on both sides, so both blocks are kept. - #555's two `paid_call` assertions pinned the fetch options exactly (`{ method: 'POST' }`) and now also see the new `signal`. Loosened to `expect.objectContaining`, which is what those tests actually mean — they assert the method is passed through, not that nothing else is. Verified on the merge result: `tsc --noEmit` silent, 901 tests pass across 83 suites, nothing red. Claude-Session: https://claude.ai/code/session_01USgemLt4Rnz4SGB1Srf3GB
3 participants
Summary
This PR addresses #411 by validating tool arguments against Zod schemas before handler execution and deriving advertised MCP `inputSchema` definitions directly from those Zod definitions, preventing schema drift.
Changes
Closes #411