Fix #1727: cap MCP framing input, batches and kept command_batch results

The server read stdio lines and remote replies without any byte bound, answered batches of any size, and command_batch kept every step result: one faulty peer or batch could allocate and work without bound. Request lines are now read in bounded chunks (16 MiB; longer lines get a -32600 reply, are skipped, and framing continues), protocol batches hold at most 64 messages, control-channel replies past 256 MiB drop the connection with the usual uncertainty error, command_batch takes at most 256 steps and keeps at most 8 MiB of step results (past that the batch stops with the reason, still closing its atomic undo group). Eight new tests cover the line/batch/step/result caps and the atomic-group close; docs/mcp.md and the command_batch tool text document them. cargo test -p vectorcraft-mcp passes.
This commit is contained in:
Fahri Meral authored and bflatastic committed 2026-10-14 04:31:43 +07:00
1 parent 85c81ff1e9
commit b75ced8a70
6 files changed
+306 -16

No files matched your search

+15 -4
View File
@@ -1,6 +1,6 @@
//! Where MCP tool calls end up: a control-channel method call.
use std::io::{BufRead, BufReader, Write};
use std::io::{BufReader, Write};
use std::net::{TcpStream, ToSocketAddrs};
use std::time::Duration;
@@ -24,6 +24,12 @@ pub struct Remote {
next_id: u64,
}
/// Longest accepted control-channel reply line (bytes, without the newline). The app answers
/// small JSON objects except for images, which reach tens of megabytes; anything longer is refused
/// and the connection dropped, so a faulty endpoint can't make the server buffer without bound
/// (#1727).
const MAX_REPLY_BYTES: usize = 256 * 1024 * 1024;
impl Remote {
/// Connect to `addr` (e.g. `127.0.0.1:7979`), failing fast when nothing is listening.
pub fn connect(addr: &str) -> std::io::Result<Self> {
@@ -65,11 +71,16 @@ impl Remote {
writer.write_all(line.as_bytes())?;
writer.write_all(b"\n")?;
writer.flush()?;
let mut reply = String::new();
if reader.read_line(&mut reply)? == 0 {
let mut buf = Vec::new();
if !crate::server::read_line_capped(reader, &mut buf, MAX_REPLY_BYTES)? {
return Err(std::io::Error::new(std::io::ErrorKind::UnexpectedEof, "control channel closed"));
}
Ok(reply)
if buf.len() > MAX_REPLY_BYTES {
// A faulty endpoint's endless line is refused rather than buffered: the connection is
// dropped and the call reports uncertainty, as for any reply that never arrived whole.
return Err(std::io::Error::new(std::io::ErrorKind::InvalidData, format!("control reply longer than {MAX_REPLY_BYTES} bytes")));
}
String::from_utf8(buf).map_err(|e| std::io::Error::new(std::io::ErrorKind::InvalidData, e))
}
}
+2
View File
@@ -47,6 +47,8 @@ mod tests_freeform;
#[cfg(test)]
mod tests_gradient;
#[cfg(test)]
mod tests_limits;
#[cfg(test)]
mod tests_links;
#[cfg(test)]
mod tests_liquify;
+79 -7
View File
@@ -21,6 +21,14 @@ const INVALID_PARAMS: i64 = -32602;
const INTERNAL_ERROR: i64 = -32603;
const RESOURCE_NOT_FOUND: i64 = -32002;
/// Longest accepted request line (bytes, without the newline). Requests are small JSON objects;
/// anything longer is answered with an error and skipped, so one huge line can't make the server
/// allocate without bound (#1727).
pub(crate) const MAX_LINE_BYTES: usize = 16 * 1024 * 1024;
/// Most messages one batch may hold. Batches were removed from the protocol; this only bounds what
/// older clients can make the server parse and answer at once (#1727).
pub(crate) const MAX_BATCH_MESSAGES: usize = 64;
const INSTRUCTIONS: &str = "VectorCraft is a professional vector illustration app. Coordinates are \
points in document space (y down, origin at the first artboard's top-left; a new document is 612×792). \
Draw with draw_shape / draw_path, change colours with set_paint, look with render_preview and doc_inspect. \
@@ -48,6 +56,49 @@ fn error(id: Value, code: i64, message: impl Into<String>) -> Value {
json!({"jsonrpc": "2.0", "id": id, "error": {"code": code, "message": message.into()}})
}
/// Read one `\n`-terminated line into `buf` (newline stripped). `Ok(false)` at end of stream.
/// `buf` holding more than `max` bytes means the line is overlong: no newline arrived within the
/// bound, so the bytes so far are only its start. Only `max + 1` bytes are ever buffered for one
/// line, in the reader's own bounded chunks, so a peer can't make us allocate without bound
/// (#1727).
pub(crate) fn read_line_capped(reader: &mut impl BufRead, buf: &mut Vec<u8>, max: usize) -> std::io::Result<bool> {
buf.clear();
loop {
let take = {
let chunk = reader.fill_buf()?;
if chunk.is_empty() {
return Ok(!buf.is_empty());
}
let upto = chunk.iter().position(|&b| b == b'\n').map_or(chunk.len(), |i| i + 1);
upto.min(max.saturating_add(1).saturating_sub(buf.len()))
};
if take == 0 {
// Already past the bound: the caller sees `buf.len() > max`.
return Ok(true);
}
// The chunk is still buffered (nothing was consumed), so the same bytes are there; the
// `min` keeps even a misbehaving reader from panicking us.
let chunk = reader.fill_buf()?;
let take = take.min(chunk.len());
buf.extend_from_slice(&chunk[..take]);
reader.consume(take);
if buf.last() == Some(&b'\n') {
buf.pop();
if buf.last() == Some(&b'\r') {
buf.pop();
}
return Ok(true);
}
}
}
/// One reply line on the protocol stream.
fn write_reply(output: &mut impl Write, line: &str) -> std::io::Result<()> {
output.write_all(line.as_bytes())?;
output.write_all(b"\n")?;
output.flush()
}
impl Server {
pub fn new(backend: Box<dyn Backend>) -> Self {
Self { backend, initialized: false, modern: false, logging: false }
@@ -80,17 +131,36 @@ impl Server {
/// Serve newline-delimited JSON-RPC until `input` closes. Logs go to stderr only (stdout is
/// the protocol stream).
pub fn serve(&mut self, input: impl BufRead, mut output: impl Write) -> std::io::Result<()> {
for line in input.lines() {
let line = line?;
let reply = self.handle_line(&line);
let mut reader = input;
let mut buf = Vec::new();
loop {
if !read_line_capped(&mut reader, &mut buf, MAX_LINE_BYTES)? {
return Ok(());
}
if buf.len() > MAX_LINE_BYTES {
// Longer than anything a client can mean: answer the framing violation, skip the
// rest of the line in bounded chunks, and keep serving what follows.
write_reply(
&mut output,
&error(Value::Null, INVALID_REQUEST, format!("request line longer than {MAX_LINE_BYTES} bytes")).to_string(),
)?;
loop {
if !read_line_capped(&mut reader, &mut buf, MAX_LINE_BYTES)? {
return Ok(());
}
if buf.len() <= MAX_LINE_BYTES {
break;
}
}
continue;
}
let line = std::str::from_utf8(&buf).map_err(|e| std::io::Error::new(std::io::ErrorKind::InvalidData, e))?;
let reply = self.handle_line(line);
let notes = self.take_notifications();
for line in reply.iter().chain(notes.iter()) {
output.write_all(line.as_bytes())?;
output.write_all(b"\n")?;
write_reply(&mut output, line)?;
}
output.flush()?;
}
Ok(())
}
/// Handle one line; returns the reply line (None for notifications and blank lines).
@@ -104,6 +174,8 @@ impl Server {
// Batches were removed in 2025-06-18; still answer older clients sensibly.
if batch.is_empty() {
Some(error(Value::Null, INVALID_REQUEST, "empty batch"))
} else if batch.len() > MAX_BATCH_MESSAGES {
Some(error(Value::Null, INVALID_REQUEST, format!("batch of {} messages exceeds {MAX_BATCH_MESSAGES}", batch.len())))
} else {
let replies: Vec<Value> = batch.into_iter().filter_map(|m| self.handle(m)).collect();
(!replies.is_empty()).then_some(Value::Array(replies))
+174
View File
@@ -0,0 +1,174 @@
//! Caps on what one message, batch or tool call can make the server do (#1727): overlong
//! framing is answered and skipped rather than buffered, batches and `command_batch` have message,
//! step and kept-result limits.
use std::io::BufRead;
use serde_json::{Value, json};
use crate::server::{MAX_BATCH_MESSAGES, MAX_LINE_BYTES, read_line_capped};
use crate::tools::MAX_COMMAND_BATCH_STEPS;
use crate::{Headless, Server};
fn rpc(s: &mut Server, method: &str, params: Value) -> Value {
s.handle(json!({"jsonrpc":"2.0","id":1,"method":method,"params":params})).unwrap()
}
fn call(s: &mut Server, name: &str, arguments: Value) -> Value {
let r = rpc(s, "tools/call", json!({"name":name,"arguments":arguments}));
assert!(r.get("error").is_none(), "{r}");
r["result"].clone()
}
fn payload(r: &Value) -> Value {
serde_json::from_str(r["content"][0]["text"].as_str().unwrap()).unwrap()
}
fn error_text(r: &Value) -> String {
r["content"][0]["text"].as_str().unwrap_or_default().to_string()
}
/// One request line, exactly as it arrives on stdin.
fn line(method: &str, params: Value) -> String {
json!({"jsonrpc": "2.0", "id": 1, "method": method, "params": params}).to_string() + "\n"
}
/// Serve every line of `input`; the replies in order.
fn serve(s: &mut Server, input: &[u8]) -> Vec<Value> {
let mut out = Vec::new();
s.serve(input, &mut out).unwrap();
String::from_utf8(out).unwrap().lines().map(|l| serde_json::from_str::<Value>(l).unwrap()).collect()
}
// ---------- line bounds ----------
#[test]
fn read_line_capped_bounds_a_line_without_a_newline() {
let mut buf = Vec::new();
// Three times the cap, no newline: only `max + 1` bytes are buffered, and the caller sees an
// overlong line.
let input = vec![b'x'; 3 * MAX_LINE_BYTES];
assert!(read_line_capped(&mut &input[..], &mut buf, MAX_LINE_BYTES).unwrap());
assert_eq!(buf.len(), MAX_LINE_BYTES + 1);
// Exactly the cap, no newline: the whole last line still parses.
let input = vec![b'x'; MAX_LINE_BYTES];
assert!(read_line_capped(&mut &input[..], &mut buf, MAX_LINE_BYTES).unwrap());
assert_eq!(buf.len(), MAX_LINE_BYTES);
// Newlines and CRLF still split, and the stream ends cleanly.
let input = b"{\"a\":1}\n{\"b\":2}\r\n";
let mut lines = Vec::new();
let mut reader = &input[..];
while read_line_capped(&mut reader, &mut buf, MAX_LINE_BYTES).unwrap() {
lines.push(String::from_utf8(buf.clone()).unwrap());
}
assert_eq!(lines, ["{\"a\":1}", "{\"b\":2}"]);
assert!(reader.fill_buf().unwrap().is_empty());
}
#[test]
fn overlong_request_line_is_answered_and_framing_continues() {
let mut s = Server::new(Box::new(Headless::with_document()));
let mut input = vec![b'x'; MAX_LINE_BYTES + 100];
input.extend_from_slice(b"\n");
input.extend_from_slice(line("ping", json!({})).as_bytes());
input.extend_from_slice(line("ping", json!({})).as_bytes());
let replies = serve(&mut s, &input);
assert_eq!(replies.len(), 3, "{replies:?}");
assert_eq!(replies[0]["error"]["code"], -32600);
assert!(replies[0]["error"]["message"].as_str().unwrap_or_default().contains("longer than"), "{replies:?}");
assert_eq!(replies[1]["result"], json!({}), "framing continues: {replies:?}");
assert_eq!(replies[2]["result"], json!({}));
}
#[test]
fn line_at_the_cap_is_still_a_message() {
let mut s = Server::new(Box::new(Headless::with_document()));
// Exactly the cap, newline-terminated: it parses (and fails as JSON, not as framing).
let mut input = vec![b'x'; MAX_LINE_BYTES];
input.extend_from_slice(b"\n");
let replies = serve(&mut s, &input);
assert_eq!(replies.len(), 1, "{replies:?}");
assert_eq!(replies[0]["error"]["code"], -32700, "{replies:?}");
assert!(replies[0]["error"]["message"].as_str().unwrap_or_default().contains("parse error"), "{replies:?}");
}
// ---------- batch bounds ----------
fn batch(n: usize) -> String {
let messages: Vec<Value> = (0..n).map(|i| json!({"jsonrpc": "2.0", "id": i, "method": "ping", "params": {}})).collect();
Value::Array(messages).to_string()
}
#[test]
fn batch_over_the_message_cap_is_refused() {
let mut s = Server::new(Box::new(Headless::with_document()));
let reply: Value = serde_json::from_str(&s.handle_line(&batch(MAX_BATCH_MESSAGES + 1)).unwrap()).unwrap();
assert_eq!(reply["error"]["code"], -32600, "{reply}");
assert!(reply["error"]["message"].as_str().unwrap_or_default().contains("exceeds"), "{reply}");
}
#[test]
fn batch_at_the_message_cap_is_answered() {
let mut s = Server::new(Box::new(Headless::with_document()));
let reply: Value = serde_json::from_str(&s.handle_line(&batch(MAX_BATCH_MESSAGES)).unwrap()).unwrap();
assert_eq!(reply.as_array().map(Vec::len), Some(MAX_BATCH_MESSAGES), "{reply}");
}
// ---------- command_batch bounds ----------
#[test]
fn command_batch_over_the_step_cap_is_refused_before_anything_runs() {
let mut s = Server::new(Box::new(Headless::with_document()));
let steps: Vec<Value> =
(0..MAX_COMMAND_BATCH_STEPS + 1).map(|i| json!({"id": "shape.rectangle", "params": {"x": i, "y": 1, "width": 5, "height": 5}})).collect();
let before = payload(&call(&mut s, "doc_inspect", json!({"depth": 0})))["objects"].clone();
let r = call(&mut s, "command_batch", json!({"steps": steps}));
assert_eq!(r["isError"], true, "{r}");
assert!(error_text(&r).contains(&format!("at most {MAX_COMMAND_BATCH_STEPS}")), "{r}");
assert_eq!(payload(&call(&mut s, "doc_inspect", json!({"depth": 0})))["objects"], before, "nothing ran: {r}");
}
/// A document whose inspect is large, then a batch of inspects: the kept results hit the byte cap
/// and the batch stops with the reason, instead of piling up an unbounded reply.
fn big_document(s: &mut Server) {
for _ in 0..6 {
let steps: Vec<Value> = (0..250).map(|i| json!({"id": "shape.rectangle", "params": {"x": i, "y": 1, "width": 5, "height": 5}})).collect();
let r = call(s, "command_batch", json!({"steps": steps}));
assert_eq!(r["isError"], false, "{r}");
}
}
#[test]
fn command_batch_stops_when_kept_results_grow_too_big() {
let mut s = Server::new(Box::new(Headless::with_document()));
big_document(&mut s);
let steps: Vec<Value> = (0..20).map(|_| json!({"id": "document.inspect", "params": {}})).collect();
let r = call(&mut s, "command_batch", json!({"steps": steps}));
assert_eq!(r["isError"], true, "{r}");
let p = payload(&r);
let results = p["results"].as_array().unwrap();
assert!(results.len() < steps.len(), "the batch stopped early: {}", results.len());
assert!(!results.is_empty(), "earlier results are kept: {p}");
assert_eq!(results[0]["ok"], json!(true), "{p}");
assert_eq!(p["failed"], 1, "{p}");
let completed = p["completed"].as_u64().unwrap();
assert_eq!(completed + 1, results.len() as u64, "{p}");
assert!(results.last().unwrap()["error"].as_str().unwrap_or_default().contains("exceed"), "{p}");
}
#[test]
fn atomic_batch_still_closes_its_undo_group_when_results_grow_too_big() {
let mut s = Server::new(Box::new(Headless::with_document()));
big_document(&mut s);
let objects = |s: &mut Server| payload(&call(s, "doc_inspect", json!({"depth":0})))["objects"].as_u64().unwrap();
let before = objects(&mut s);
// Edits first (grouped), then the inspects that breach the cap: the stopped batch must still
// close its group into one undo step, not leave it open.
let mut steps = vec![json!({"id": "shape.rectangle", "params": {"x": 1, "y": 1, "width": 5, "height": 5}})];
steps.extend((0..20).map(|_| json!({"id": "document.inspect", "params": {}})));
let r = call(&mut s, "command_batch", json!({"steps": steps, "atomic": true, "label": "Capped batch"}));
assert_eq!(r["isError"], true, "{r}");
assert_eq!(objects(&mut s), before + 1, "the edit ran: {r}");
assert_eq!(payload(&call(&mut s, "run_command", json!({"command": "edit.undo"})))["undone"], "Capped batch");
assert_eq!(objects(&mut s), before, "one undo removed the grouped edit");
}
+28 -4
View File
@@ -96,9 +96,9 @@ pub fn tool_definitions() -> Vec<Value> {
tool(
"command_batch",
"Run several commands",
"Run steps in order; each edit is its own undo step, or with atomic:true the whole batch is one (named `label`, else after its first edit). A param string that is exactly \"$N.path\" (N: 0-based step index) or \"$last.path\" takes that value from an earlier step's result, e.g. {\"id\":\"$1.id\"}; \"$$\" starts a literal \"$\". Returns completed, failed and results. Stops on the first error by default.",
"Run steps in order; each edit is its own undo step, or with atomic:true the whole batch is one (named `label`, else after its first edit). A param string that is exactly \"$N.path\" (N: 0-based step index) or \"$last.path\" takes that value from an earlier step's result, e.g. {\"id\":\"$1.id\"}; \"$$\" starts a literal \"$\". Returns completed, failed and results. Stops on the first error by default. At most 256 steps; kept step results total at most 8 MiB, past which the batch stops with the reason.",
obj(
json!({"steps": {"type":"array", "items":obj(json!({"id":string("Command id"),"params":{"type":"object"}}), &["id"])}, "stop_on_error":{"type":"boolean", "default":true}, "atomic":{"type":"boolean", "default":false, "description":"Make the batch's edits one undo step (needs an open document)"}, "label":string("The undo step's name with atomic")}),
json!({"steps": {"type":"array", "maxItems": 256, "items":obj(json!({"id":string("Command id"),"params":{"type":"object"}}), &["id"])}, "stop_on_error":{"type":"boolean", "default":true}, "atomic":{"type":"boolean", "default":false, "description":"Make the batch's edits one undo step (needs an open document)"}, "label":string("The undo step's name with atomic")}),
&["steps"],
),
false,
@@ -916,8 +916,17 @@ pub fn call_tool(b: &mut dyn Backend, name: &str, args: &Value) -> ToolResult {
}
}
/// Most steps one `command_batch` may hold, and most bytes its kept step results may total.
/// Batches run sequentially and every result is kept for later steps' `"$N.path"` references, so
/// without caps one batch could do unbounded work and pile up an unbounded reply (#1727).
pub(crate) const MAX_COMMAND_BATCH_STEPS: usize = 256;
pub(crate) const MAX_COMMAND_BATCH_BYTES: usize = 8 * 1024 * 1024;
fn command_batch(b: &mut dyn Backend, a: &Args) -> Result<ToolResult, String> {
let steps = a.get("steps").and_then(Value::as_array).ok_or("`steps` must be an array")?;
if steps.len() > MAX_COMMAND_BATCH_STEPS {
return Err(format!("`steps` holds {} steps; at most {MAX_COMMAND_BATCH_STEPS}", steps.len()));
}
let stop = match a.get("stop_on_error") {
None => true,
Some(v) => v.as_bool().ok_or("`stop_on_error` must be a boolean")?,
@@ -935,6 +944,9 @@ fn command_batch(b: &mut dyn Backend, a: &Args) -> Result<ToolResult, String> {
let (mut completed, mut failed, mut results) = (0, 0, Vec::new());
// Each step's result (null for a failed one), for later steps' `"$N.path"` references.
let mut values: Vec<Value> = vec![];
// The kept results all go back in the reply, so their total size is capped: one batch can't
// pile up an unbounded answer (#1727).
let mut kept_bytes = 0usize;
for step in steps {
let result = vectorcraft_engine::guard::catch_panic(|| {
let args = step.as_object().ok_or("each step must be an object")?;
@@ -952,9 +964,21 @@ fn command_batch(b: &mut dyn Backend, a: &Args) -> Result<ToolResult, String> {
.unwrap_or_else(|e| Err(format!("internal error: {e}")));
match result {
Ok(v) => {
let entry = json!({"ok": true, "result": v});
let size = entry.to_string().len();
if kept_bytes.saturating_add(size) > MAX_COMMAND_BATCH_BYTES {
// The results already kept stay; this step's is replaced by the reason the
// batch stops. The loop breaks either way — carrying on would defeat the cap —
// and the atomic group below still closes, as for any stopped batch.
failed += 1;
values.push(Value::Null);
results.push(json!({"ok": false, "error": format!("batch results exceed {MAX_COMMAND_BATCH_BYTES} bytes; stopping the batch")}));
break;
}
kept_bytes += size;
completed += 1;
values.push(v.clone());
results.push(json!({"ok":true,"result":v}));
values.push(entry["result"].clone());
results.push(entry);
}
Err(e) => {
failed += 1;
+8 -1
View File
@@ -111,6 +111,13 @@ accepted too. Clients requesting `2026-07-28` through `initialize`, or through a
List TTL is 600000 ms; live reads use zero. Older clients keep the original response shapes.
This does not add streaming or input requests — see [Not implemented](#not-implemented).
One message is one line of at most 16 MiB: a longer line is answered with `-32600` and skipped,
and the server keeps serving what follows. A batch holds at most 64 messages. `command_batch`
holds at most 256 steps, and the step results it keeps (for later steps' `"$N.path"` references
and the reply) total at most 8 MiB — past that the batch stops with the reason, closing its atomic
undo group as usual. Replies from a connected app are refused past 256 MiB and the connection
dropped; the call reports uncertainty like any reply that never arrived whole.
`initialize` advertises:
| Capability | What it covers |
@@ -242,7 +249,7 @@ The older names below remain listed because existing workflows use them.
|---|---|
| `command_list` | Optional `filter`, `enabled_only`; returns the command array |
| `command_run` | `id`, optional `params`; runs the command through the existing backend. An unknown key in `params` is an error that names it and the nearest key the command takes (`invalid parameters for `file.new`: unknown parameter `widht`; did you mean `width`?`), so a misspelt key no longer runs the command with its default (see [Parameter keys](#parameter-keys)) |
| `command_batch` | `steps: [{id, params?}]`, optional `stop_on_error` (default true), `atomic` and `label`; returns `completed`, `failed`, `results: [{ok, result\|error}]`. Each edit has its own undo step, or with `atomic: true` the batch's edits are one undo step named `label` (else after its first edit; `edit.undoGroup.begin`/`end` underneath); failures set `isError`. A param string `"$N.path"` or `"$last.path"` takes a value from an earlier step's result (see `run` below) |
| `command_batch` | `steps: [{id, params?}]` (at most 256), optional `stop_on_error` (default true), `atomic` and `label`; returns `completed`, `failed`, `results: [{ok, result\|error}]`. Each edit has its own undo step, or with `atomic: true` the batch's edits are one undo step named `label` (else after its first edit; `edit.undoGroup.begin`/`end` underneath); failures set `isError`. A param string `"$N.path"` or `"$last.path"` takes a value from an earlier step's result (see `run` below). Kept step results total at most 8 MiB; past that the batch stops with the reason |
| `doc_inspect` | Optional `depth`, `childLimit`; same summary as `inspect_document` |
| `render_preview` | Optional `max_side` (1–4096, default 1024) and `artboard` (0-based, default 0); inline PNG of that artboard without editing it. Artboards too large to render within the allocation bound return a tool error |
| `ui_inspect`, `ui_screenshot` | Connected desktop state/window capture; tool errors in headless mode |