From 3ebf9dc7bdc46754cc3b2b8986643d7682bacfa5 Mon Sep 17 00:00:00 2001 From: charliewwdev Date: Tue, 1 Sep 2026 16:53:16 +0800 Subject: [PATCH] fix(server): survive malformed JSON-RPC frames instead of exiting MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A single malformed frame from any client could end the MCP session, which is a denial-of-service primitive against a stdio server (issue #54). Two independent defects: - Invalid UTF-8 bytes made `utf8.decoder` raise a FormatException on the *stream*. The per-line try/catch never saw it and `listen()` had no `onError`, so the exception went unhandled and terminated the process with exit code 255 — the `eof` the conformance harness reported. - Truncated or non-JSON frames were caught, but the reply went through `_sendError(null, ...)`, which returns early on a null id. The parse error was therefore silently dropped and the client saw a hung server. Decode with `allowMalformed: true` so bad bytes degrade into an ordinary parse error, add `onError`/`cancelOnError: false` to keep the read loop alive, and introduce `_sendProtocolError` for the null-id replies that JSON-RPC 2.0 requires. Frames that are valid JSON but not request objects now answer -32600 rather than being ignored. `run()` also awaits stdin closing now. It previously returned as soon as the listener was attached, so `runServer` released the single-instance lock while the server was still serving. --- lib/src/cli/server.dart | 71 ++++++++++-- test/server_malformed_frame_test.dart | 150 ++++++++++++++++++++++++++ 2 files changed, 210 insertions(+), 11 deletions(-) create mode 100644 test/server_malformed_frame_test.dart diff --git a/lib/src/cli/server.dart b/lib/src/cli/server.dart index df97b9f9..d0dd65ab 100644 --- a/lib/src/cli/server.dart +++ b/lib/src/cli/server.dart @@ -364,20 +364,55 @@ class FlutterMcpServer { } Future run() async { + final done = Completer(); + + // `allowMalformed: true` replaces invalid byte sequences with U+FFFD + // instead of throwing. A strict decoder raises FormatException on the + // *stream*, which bypasses the per-line try/catch below and kills the + // process — a single garbled byte from any client is enough to end the + // session. Malformed bytes now degrade into an ordinary parse error. stdin - .transform(utf8.decoder) + .transform(const Utf8Decoder(allowMalformed: true)) .transform(const LineSplitter()) - .listen((line) async { - if (line.trim().isEmpty) return; - try { - final request = jsonDecode(line); - if (request is Map) { - await _handleRequest(request); + .listen( + (line) async { + // A failure while handling one frame must never end the session, so + // every path here is guarded and answers with a JSON-RPC error. + try { + if (line.trim().isEmpty) return; + final dynamic request; + try { + request = jsonDecode(line); + } catch (e) { + _sendProtocolError(-32700, 'Parse error: $e'); + return; + } + if (request is Map) { + await _handleRequest(request); + } else { + // Valid JSON, but not a JSON-RPC request object. + _sendProtocolError( + -32600, 'Invalid Request: expected a JSON object'); + } + } catch (e) { + _sendProtocolError(-32603, 'Internal error: $e'); } - } catch (e) { - _sendError(null, -32700, "Parse error: $e"); - } - }); + }, + // Without an onError handler a stream-level error is unhandled and + // terminates the isolate. Keep reading instead. + onError: (Object e) { + _sendProtocolError(-32700, 'Parse error: $e'); + }, + onDone: () { + if (!done.isCompleted) done.complete(); + }, + cancelOnError: false, + ); + + // Keep `runServer` awaiting until stdin closes; otherwise it returns + // immediately and releases the single-instance lock while the server is + // still serving requests. + await done.future; } Future _handleRequest(Map request) async { @@ -1086,6 +1121,20 @@ class FlutterMcpServer { stdout.writeln(jsonEncode({"jsonrpc": "2.0", "id": id, "result": result})); } + /// Reports an error that occurred before a request id could be read. + /// + /// [_sendError] drops null ids so that notifications stay unanswered, but + /// JSON-RPC 2.0 requires parse and invalid-request errors to be reported + /// with a null id. Without this the client sees silence and treats the + /// server as hung. + void _sendProtocolError(int code, String message) { + stdout.writeln(jsonEncode({ + "jsonrpc": "2.0", + "id": null, + "error": {"code": code, "message": message}, + })); + } + void _sendError(dynamic id, int code, String message) { if (id == null) return; stdout.writeln(jsonEncode({ diff --git a/test/server_malformed_frame_test.dart b/test/server_malformed_frame_test.dart new file mode 100644 index 00000000..c73fef2e --- /dev/null +++ b/test/server_malformed_frame_test.dart @@ -0,0 +1,150 @@ +import 'dart:convert'; +import 'dart:io'; + +import 'package:async/async.dart'; +import 'package:flutter_test/flutter_test.dart'; + +/// Regression tests for the JSON-RPC read loop. +/// +/// A malformed frame from any client used to end the session: invalid UTF-8 +/// raised a stream-level FormatException that no handler caught, and a +/// truncated JSON object produced no reply at all because the parse error was +/// dropped for having a null id. Both are denial-of-service primitives against +/// a stdio MCP server, so they are pinned here. +void main() { + group('MCP server JSON-RPC framing', () { + late Process server; + late StreamQueue responses; + + /// Longest a single well-formed request may take to come back. Generous so + /// the suite stays reliable on a cold CI machine. + const responseTimeout = Duration(seconds: 30); + + /// Under `flutter test` the running executable is `flutter_tester`, which + /// cannot run a plain Dart entrypoint, so resolve the real Dart SDK. + String dartExecutable() { + final flutterRoot = Platform.environment['FLUTTER_ROOT']; + if (flutterRoot != null && flutterRoot.isNotEmpty) { + final sdkDart = File( + '$flutterRoot/bin/cache/dart-sdk/bin/dart${Platform.isWindows ? '.exe' : ''}'); + if (sdkDart.existsSync()) return sdkDart.path; + } + return 'dart'; + } + + Future send(List bytes) async { + server.stdin.add(bytes); + await server.stdin.flush(); + } + + Future sendJson(Object frame) => send(utf8.encode('${jsonEncode(frame)}\n')); + + Future> nextResponse() async { + final line = await responses.next.timeout( + responseTimeout, + onTimeout: () => throw StateError( + 'Server sent no response — it most likely died on the previous frame'), + ); + return jsonDecode(line) as Map; + } + + setUp(() async { + // The server refuses to start while another instance holds the lock. + final home = Platform.environment['HOME'] ?? + Platform.environment['USERPROFILE'] ?? + '/tmp'; + final lock = File('$home/.flutter_skill.lock'); + if (await lock.exists()) await lock.delete(); + + server = await Process.start( + dartExecutable(), + ['run', 'bin/server.dart', 'server'], + ); + server.stderr.drain(); + responses = StreamQueue(server.stdout + .transform(utf8.decoder) + .transform(const LineSplitter()) + // The child inherits the test runner's VM service flags and prints an + // "observatory listening" banner on stdout. Keep only JSON-RPC frames. + .where((line) => line.startsWith('{'))); + + // Handshake first so a failure later is unambiguously caused by the + // hostile frame under test and not by a server that never came up. + await sendJson({ + 'jsonrpc': '2.0', + 'id': 1, + 'method': 'initialize', + 'params': {}, + }); + final hello = await nextResponse(); + expect(hello['id'], 1); + expect(hello['result'], isNotNull); + }); + + tearDown(() async { + await responses.cancel(immediate: true); + server.kill(); + await server.exitCode; + }); + + /// Drives one hostile frame and asserts the server answers it and is still + /// able to serve a normal request afterwards. + Future expectSurvives( + List hostileFrame, { + required int expectedCode, + }) async { + await send(hostileFrame); + + final error = await nextResponse(); + expect(error['id'], isNull, + reason: 'JSON-RPC 2.0 requires a null id when the id is unreadable'); + expect(error['error']['code'], expectedCode); + + await sendJson({ + 'jsonrpc': '2.0', + 'id': 99, + 'method': 'tools/list', + 'params': {}, + }); + final after = await nextResponse(); + expect(after['id'], 99, + reason: 'Server must still serve requests after a malformed frame'); + } + + test('should answer a parse error when a frame is truncated JSON', () async { + await expectSurvives(utf8.encode('{"jsonrpc":"2.0","id":2,\n'), + expectedCode: -32700); + }); + + test('should answer a parse error when a frame is not JSON at all', () async { + await expectSurvives(utf8.encode('not json at all }{\n'), + expectedCode: -32700); + }); + + test('should survive invalid UTF-8 bytes instead of exiting', () async { + // A strict utf8.decoder throws on the stream rather than in the per-line + // handler, which previously terminated the process. + await expectSurvives([0xff, 0xfe, 0xfd, 0x0a], expectedCode: -32700); + }); + + test('should reject valid JSON that is not a request object', () async { + await expectSurvives(utf8.encode('[1,2,3]\n'), expectedCode: -32600); + }); + + test('should stay silent for a notification carrying no id', () async { + await sendJson({'jsonrpc': '2.0', 'method': 'notifications/initialized'}); + + // Nothing should come back for the notification, so the next line read + // must belong to the request sent after it. + await sendJson({ + 'jsonrpc': '2.0', + 'id': 50, + 'method': 'tools/list', + 'params': {}, + }); + final next = await nextResponse(); + expect(next['id'], 50, + reason: 'A notification must not produce a response'); + }); + }); +}