Repository navigation
fix: bound inbound WebSocket message size #1129
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,69 @@ | ||
| package io.modelcontextprotocol.kotlin.sdk.integration | ||
|
|
||
| import io.kotest.matchers.shouldBe | ||
| import io.ktor.client.HttpClient | ||
| import io.ktor.client.plugins.websocket.WebSockets | ||
| import io.ktor.client.plugins.websocket.webSocketSession | ||
| import io.ktor.client.request.header | ||
| import io.ktor.http.HttpHeaders | ||
| import io.ktor.server.engine.embeddedServer | ||
| import io.ktor.websocket.Frame | ||
| import io.modelcontextprotocol.kotlin.sdk.client.Client | ||
| import io.modelcontextprotocol.kotlin.sdk.client.mcpWebSocketTransport | ||
| import io.modelcontextprotocol.kotlin.sdk.server.Server | ||
| import io.modelcontextprotocol.kotlin.sdk.server.ServerOptions | ||
| import io.modelcontextprotocol.kotlin.sdk.server.mcpWebSocket | ||
| import io.modelcontextprotocol.kotlin.sdk.shared.MCP_SUBPROTOCOL | ||
| import io.modelcontextprotocol.kotlin.sdk.types.Implementation | ||
| import io.modelcontextprotocol.kotlin.sdk.types.ServerCapabilities | ||
| import io.modelcontextprotocol.kotlin.test.utils.actualPort | ||
| import io.modelcontextprotocol.kotlin.test.utils.runIntegrationTest | ||
| import kotlinx.serialization.json.buildJsonObject | ||
| import kotlinx.serialization.json.put | ||
| import kotlinx.serialization.json.putJsonObject | ||
| import org.junit.jupiter.api.Test | ||
| import kotlin.time.Duration.Companion.seconds | ||
| import io.ktor.client.engine.cio.CIO as ClientCIO | ||
| import io.ktor.server.cio.CIO as ServerCIO | ||
|
|
||
| class WebSocketMessageSizeIntegrationTest { | ||
|
|
||
| @Test | ||
| fun `server closes a connection that sends a message over 16 MiB and keeps serving others`() = | ||
| runIntegrationTest(timeout = 30.seconds) { | ||
| val server = Server(Implementation("test-server", "1.0.0"), ServerOptions(ServerCapabilities())) | ||
| val engine = embeddedServer(ServerCIO, host = HOST, port = 0) { mcpWebSocket { server } } | ||
| .startSuspend(wait = false) | ||
| val httpClient = HttpClient(ClientCIO) { install(WebSockets) } | ||
| val url = "ws://$HOST:${engine.actualPort()}" | ||
| try { | ||
| val peer = httpClient.webSocketSession(url) { | ||
| header(HttpHeaders.SecWebSocketProtocol, MCP_SUBPROTOCOL) | ||
| } | ||
|
|
||
| // A valid request: without the limit the server would answer it instead of closing. | ||
| peer.send(Frame.Text(ping(padding = "a".repeat(16 * 1024 * 1024)))) | ||
|
|
||
| peer.incoming.receiveCatching().getOrNull() shouldBe null | ||
| val client = Client(Implementation("test-client", "1.0.0")) | ||
| client.connect(httpClient.mcpWebSocketTransport(url)) | ||
| client.ping() | ||
| client.close() | ||
| } finally { | ||
| httpClient.close() | ||
| engine.stopSuspend(gracePeriodMillis = 0, timeoutMillis = 1_000) | ||
| server.close() | ||
| } | ||
| } | ||
|
|
||
| private fun ping(padding: String): String = buildJsonObject { | ||
| put("jsonrpc", "2.0") | ||
| put("id", 1) | ||
| put("method", "ping") | ||
| putJsonObject("params") { put("padding", padding) } | ||
| }.toString() | ||
|
|
||
| private companion object { | ||
| const val HOST = "127.0.0.1" | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,75 @@ | ||
| package io.modelcontextprotocol.kotlin.sdk.shared | ||
|
|
||
| import io.kotest.matchers.collections.shouldHaveSize | ||
| import io.kotest.matchers.shouldBe | ||
| import io.kotest.matchers.types.shouldBeInstanceOf | ||
| import io.ktor.websocket.Frame | ||
| import io.ktor.websocket.FrameTooBigException | ||
| import io.ktor.websocket.WebSocketExtension | ||
| import io.ktor.websocket.WebSocketSession | ||
| import io.modelcontextprotocol.kotlin.sdk.types.JSONRPCMessage | ||
| import io.modelcontextprotocol.kotlin.sdk.types.JSONRPCRequest | ||
| import kotlinx.coroutines.CompletableDeferred | ||
| import kotlinx.coroutines.Job | ||
| import kotlinx.coroutines.channels.Channel | ||
| import kotlinx.coroutines.channels.ReceiveChannel | ||
| import kotlinx.coroutines.channels.SendChannel | ||
| import kotlinx.coroutines.test.runTest | ||
| import org.junit.jupiter.api.Test | ||
| import java.util.concurrent.CopyOnWriteArrayList | ||
| import kotlin.coroutines.CoroutineContext | ||
|
|
||
| class WebSocketMcpTransportTest { | ||
|
|
||
| @Test | ||
| fun `should keep reading after messages it cannot parse`() = runTest { | ||
| val session = FakeWebSocketSession() | ||
| val transport = TestTransport(session) | ||
| val errors = CopyOnWriteArrayList<Throwable>() | ||
| val received = CompletableDeferred<JSONRPCMessage>() | ||
| transport.onError { errors += it } | ||
| transport.onMessage { received.complete(it) } | ||
| transport.start() | ||
|
|
||
| session.incomingFrames.send(Frame.Binary(true, byteArrayOf(1, 2, 3))) | ||
| session.incomingFrames.send(Frame.Text("{")) | ||
| session.incomingFrames.send(Frame.Text("""{"jsonrpc":"2.0","id":1,"method":"ping"}""")) | ||
|
|
||
| received.await().shouldBeInstanceOf<JSONRPCRequest>().method shouldBe "ping" | ||
| errors shouldHaveSize 2 | ||
| session.incomingFrames.close() | ||
| } | ||
|
|
||
| @Test | ||
| fun `should report a session failure through onError`() = runTest { | ||
| val session = FakeWebSocketSession() | ||
| val transport = TestTransport(session) | ||
| val error = CompletableDeferred<Throwable>() | ||
| transport.onError { error.complete(it) } | ||
| transport.start() | ||
|
|
||
| session.incomingFrames.close(FrameTooBigException(42)) | ||
|
|
||
| error.await().shouldBeInstanceOf<FrameTooBigException>() | ||
| } | ||
|
|
||
| private class TestTransport(override val session: WebSocketSession) : WebSocketMcpTransport() { | ||
| override suspend fun initializeSession() = Unit | ||
| } | ||
| } | ||
|
|
||
| private class FakeWebSocketSession : WebSocketSession { | ||
| val incomingFrames = Channel<Frame>(Channel.UNLIMITED) | ||
|
|
||
| override val coroutineContext: CoroutineContext = Job() | ||
| override var masking: Boolean = false | ||
| override var maxFrameSize: Long = Long.MAX_VALUE | ||
| override val incoming: ReceiveChannel<Frame> get() = incomingFrames | ||
| override val outgoing: SendChannel<Frame> = Channel(Channel.UNLIMITED) | ||
| override val extensions: List<WebSocketExtension<*>> = emptyList() | ||
|
|
||
| override suspend fun flush() = Unit | ||
|
|
||
| @Deprecated("Use cancel() instead.", level = DeprecationLevel.ERROR) | ||
| override fun terminate() = Unit | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -18,8 +18,13 @@ import kotlinx.coroutines.awaitCancellation | |
|
|
||
| private val logger = KotlinLogging.logger {} | ||
|
|
||
| // Ktor's default maxFrameSize does not limit inbound messages at all. | ||
| private const val DEFAULT_MAX_FRAME_SIZE: Long = 16L * 1024 * 1024 | ||
|
|
||
| /** | ||
| * Configures the Ktor Application to handle Model Context Protocol (MCP) over WebSocket. | ||
| * | ||
| * Set `maxFrameSize` when installing the `WebSockets` plugin: by default Ktor does not limit inbound messages. | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Blocking: the bound is applied only where the SDK itself installs the plugin, so both Failure scenario: Suggested fix: also clamp on the session itself in
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The |
||
| */ | ||
| @KtorDsl | ||
| public fun Route.mcpWebSocket(block: () -> Server) { | ||
|
|
@@ -30,6 +35,8 @@ public fun Route.mcpWebSocket(block: () -> Server) { | |
|
|
||
| /** | ||
| * Configures the Ktor Application to handle Model Context Protocol (MCP) over WebSocket. | ||
| * | ||
| * Set `maxFrameSize` when installing the `WebSockets` plugin: by default Ktor does not limit inbound messages. | ||
| */ | ||
| @KtorDsl | ||
| public fun Route.mcpWebSocket(path: String, block: () -> Server) { | ||
|
|
@@ -40,10 +47,14 @@ public fun Route.mcpWebSocket(path: String, block: () -> Server) { | |
|
|
||
| /** | ||
| * Configures the Ktor Application to handle Model Context Protocol (MCP) over WebSocket. | ||
| * | ||
| * Installs the `WebSockets` plugin with inbound messages limited to 16 MiB. | ||
| */ | ||
| @KtorDsl | ||
| public fun Application.mcpWebSocket(block: () -> Server) { | ||
| install(WebSockets) | ||
| install(WebSockets) { | ||
| maxFrameSize = DEFAULT_MAX_FRAME_SIZE | ||
| } | ||
|
|
||
| routing { | ||
| mcpWebSocket(block) | ||
|
|
@@ -52,10 +63,14 @@ public fun Application.mcpWebSocket(block: () -> Server) { | |
|
|
||
| /** | ||
| * Configures the Ktor Application to handle Model Context Protocol (MCP) over WebSocket at the specified path. | ||
| * | ||
| * Installs the `WebSockets` plugin with inbound messages limited to 16 MiB. | ||
| */ | ||
| @KtorDsl | ||
| public fun Application.mcpWebSocket(path: String, block: () -> Server) { | ||
| install(WebSockets) | ||
| install(WebSockets) { | ||
| maxFrameSize = DEFAULT_MAX_FRAME_SIZE | ||
| } | ||
|
|
||
| routing { | ||
| mcpWebSocket(path, block) | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Problem: 16 MiB is hardcoded with no way to raise it on the
Application.mcpWebSocketoverloads. They callinstall(WebSockets)internally, and pre-installing the plugin with your ownmaxFrameSizedoesn't layer on top — I checked on this branch and it fails withDuplicatePluginException: … already installed with the same key as 'WebSockets'. So "Breaking Changes: None" doesn't hold for anyone whose peers send larger messages.Failure scenario: a server hosted via
Application.mcpWebSocketwhose clients call a tool with a base64-encoded document or image argument above 16 MiB. That works today; after upgrading, Ktor closes the connection withTOO_BIGpart-way through the request, and the only remedy is to abandon this overload forinstall(WebSockets) { maxFrameSize = … }+routing { mcpWebSocket { … } }— which is the path that currently has no limit at all. Between the two, the SDK offers "hardcoded 16 MiB" or "unlimited", with nothing in between.Suggested fix: take the limit as a parameter so the safe default still ships but stays overridable without switching entry point, e.g.
public fun Application.mcpWebSocket(maxFrameSize: Long = DEFAULT_MAX_FRAME_SIZE, block: () -> Server)(and thepathvariant), passing it through to both the plugin config and the session clamp from the other comment.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
DuplicatePluginException isn't new
I'd rather not add a parameter in a security fix, since it needs new public API plus hidden overloads for binary compatibility