fix(mcp): a bare 'null' JSON-RPC line must not crash the server - #2511
Conversation
… the server
Gemini PR review (HIGH): JSON.parse('null') is valid JSON returning null, so
dispatch(null) throws on null.id (mcp.ts:105) and the catch handler's own
req.id throws again → uncaught → the stdio server crashes on a single 'null'
line. Guard: after parse, drop any non-object (null/primitive) like an
unparseable line — it carries no id to attribute. Regression test drives raw
null/primitive lines and asserts the server survives + answers initialize.
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds validation to the MCP server to drop incoming JSON-RPC requests that are null or bare primitives, preventing server crashes. It also introduces a test helper and test cases to verify this behavior. The review feedback suggests extending this validation to explicitly reject arrays (which have a typeof value of 'object') since batch requests are not supported, and updating the test cases to cover this scenario.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| // primitives are valid JSON but carry no id to attribute — drop them like an | ||
| // unparseable line. Without this, `dispatch(null)` (and the catch below) throw | ||
| // on `null.id`, and the uncaught error crashes the server on a `null` line. | ||
| if (req === null || typeof req !== 'object') return; |
There was a problem hiding this comment.
While typeof req === 'object' correctly filters out primitives and null, it does not filter out arrays (e.g., [] or a JSON-RPC batch request). In JavaScript, typeof [] is 'object'. If an array is passed, it will proceed to dispatch(req), where req.id and req.method will be undefined, causing it to be treated as a notification and silently ignored. Since this server does not support batch requests, we should explicitly reject arrays using Array.isArray(req) to ensure only actual non-null objects are processed.
| if (req === null || typeof req !== 'object') return; | |
| if (req === null || typeof req !== 'object' || Array.isArray(req)) return; |
| test('a bare `null` / primitive line is dropped and does not crash the server', async () => { | ||
| // `JSON.parse('null')` is valid JSON but not a JSON-RPC object; without the | ||
| // non-object guard, dispatch(null) throws on null.id and the server crashes. | ||
| const res = await driveMcpRaw(repo, ['null', '5', 'true', '"str"', JSON.stringify(INIT)]); |
There was a problem hiding this comment.
Add an empty array [] to the list of raw malformed lines to verify that the server also safely drops arrays without crashing or processing them.
| const res = await driveMcpRaw(repo, ['null', '5', 'true', '"str"', JSON.stringify(INIT)]); | |
| const res = await driveMcpRaw(repo, ['null', '5', 'true', '"str"', '[]', JSON.stringify(INIT)]); |
1 participant
Finding (Gemini review on #2510, HIGH)
src/term-commands/mcp.ts— the newline JSON-RPC loop parses each line withJSON.parse, catching only parse errors. ButJSON.parse('null')is valid JSON returningnull:reqbecomesnull,dispatch(null)throws accessingnull.id(mcp.ts:105), and thecatch (e)handler also readsreq.idonnull→ a second, uncaught TypeError → the stdio MCP server crashes on a singlenullline. (Bare primitives like5/truereachdispatchharmlessly — onlynullcrashes — but the guard covers all non-objects.)Fix
After a successful parse, drop any value that isn't a non-null object — it can't be a JSON-RPC request and carries no id to attribute, exactly like an unparseable line:
Test
New regression drives raw
null/5/true/"str"lines followed by a validinitializeand asserts the server survives + answers initialize (previously it crashed on thenullline). Verified against the builtdist/genie.jstoo.Gates:
bun run checkgreen; typecheck clean; 15 mcp tests pass.