diff --git a/apps/deckcraft/src/control_server.rs b/apps/deckcraft/src/control_server.rs index 0abac12a..b4b80c1d 100644 --- a/apps/deckcraft/src/control_server.rs +++ b/apps/deckcraft/src/control_server.rs @@ -1,8 +1,11 @@ //! Loopback JSON-lines control server: one request per line, one reply per line. //! This is the transport the MCP server (`deckcraft-cli mcp --connect`) wraps. //! -//! Every resource a client can hold is bounded ([`Limits`]): simultaneous connections, request -//! line length, queued requests, and how long a socket may sit idle or blocked on a write. +//! The server only reads requests: a line that is not a JSON object with a string `method` (an +//! HTTP request line, a stray `nc`, binary data) gets one error reply and the connection is +//! closed, so nothing sent after it runs. Every resource a client can hold is bounded +//! ([`Limits`]): simultaneous connections, request line length, queued requests, and how long a +//! socket may sit idle or blocked on a write. use std::io::{BufRead, BufReader, ErrorKind, Read, Write}; use std::net::{TcpListener, TcpStream}; @@ -120,6 +123,53 @@ fn read_line(r: &mut impl BufRead, max: usize) -> Line { } } +/// One request line, parsed. +enum Parsed { + Request { + id: Value, + method: String, + params: Value, + }, + Blank, + /// Not a request: reply with this error and close the connection. + Reject(String), +} + +fn parse(bytes: &[u8]) -> Parsed { + let Ok(text) = std::str::from_utf8(bytes) else { + return Parsed::Reject("not a control request (invalid UTF-8); closing the connection".into()); + }; + let text = text.trim(); + if text.is_empty() { + return Parsed::Blank; + } + if is_http_request_line(text) { + return Parsed::Reject("HTTP is not served here: this port takes JSON lines; closing the connection".into()); + } + let msg = match serde_json::from_str::(text) { + Ok(v) => v, + Err(e) => return Parsed::Reject(format!("bad JSON ({e}); expected one JSON request object per line; closing the connection")), + }; + let Some(obj) = msg.as_object() else { + return Parsed::Reject("not a control request (expected a JSON object); closing the connection".into()); + }; + let Some(method) = obj.get("method").and_then(Value::as_str) else { + return Parsed::Reject("not a control request (missing string `method`); closing the connection".into()); + }; + Parsed::Request { + id: obj.get("id").cloned().unwrap_or(Value::Null), + method: method.to_string(), + params: obj.get("params").cloned().unwrap_or(json!({})), + } +} + +/// An HTTP/1 request line (`POST / HTTP/1.1`) or the HTTP/2 preface (`PRI * HTTP/2.0`). +fn is_http_request_line(text: &str) -> bool { + const METHODS: [&str; 10] = ["GET", "HEAD", "POST", "PUT", "DELETE", "CONNECT", "OPTIONS", "TRACE", "PATCH", "PRI"]; + text.split_once(' ').is_some_and(|(method, _)| METHODS.contains(&method)) + || text.rsplit_once(' ').is_some_and(|(_, version)| version.starts_with("HTTP/")) +} + fn serve(stream: TcpStream, tx: &SyncSender, ctx: &egui::Context, limits: Limits) { if stream.set_read_timeout(Some(limits.idle_timeout)).is_err() || stream.set_write_timeout(Some(limits.write_timeout)).is_err() { return; @@ -136,20 +186,14 @@ fn serve(stream: TcpStream, tx: &SyncSender, ctx: &egui::Context } Line::Closed => break, }; - let Ok(line) = String::from_utf8(line) else { - if writeln!(out, "{}", json!({"ok": false, "error": "bad JSON: not UTF-8"})).is_err() { + let reply = match parse(&line) { + Parsed::Blank => continue, + Parsed::Reject(e) => { + // The connection closes either way; a peer that can't read the reason loses nothing. + let _ = writeln!(out, "{}", json!({"ok": false, "error": e})); break; } - continue; - }; - if line.trim().is_empty() { - continue; - } - let reply = match serde_json::from_str::(&line) { - Ok(msg) => { - let id = msg.get("id").cloned().unwrap_or(Value::Null); - let method = msg.get("method").and_then(Value::as_str).unwrap_or("").to_string(); - let params = msg.get("params").cloned().unwrap_or(json!({})); + Parsed::Request { id, method, params } => { let (req, rrx) = ControlRequest::new(method, params); let state = Arc::clone(&req.state); let mut r = match tx.try_send(req) { @@ -177,7 +221,6 @@ fn serve(stream: TcpStream, tx: &SyncSender, ctx: &egui::Context } r } - Err(e) => json!({"ok": false, "error": format!("bad JSON: {e}")}), }; if let Err(e) = writeln!(out, "{reply}") { if matches!(e.kind(), ErrorKind::WouldBlock | ErrorKind::TimedOut) { @@ -245,6 +288,45 @@ mod tests { assert_eq!(v["result"], "ui.inspect"); } + /// Every reply line until the server closes the connection. + fn replies_until_closed(r: &mut BufReader) -> Vec { + std::iter::from_fn(|| reply(r)).collect() + } + + /// A browser can POST to the port: its request line closes the connection before the JSON + /// line in its body is read, so nothing is dispatched. + #[test] + fn an_http_request_closes_the_connection_before_its_body_is_dispatched() { + let (addr, rx) = server(Limits { max_line: 512, ..small() }); + let (mut r, mut w) = client(addr); + w.write_all( + b"POST / HTTP/1.1\r\nHost: 127.0.0.1\r\nContent-Type: text/plain\r\n\r\n\ + {\"id\":1,\"method\":\"engine.execute\",\"params\":{\"command\":\"slide.new\"}}\n", + ) + .unwrap(); + let replies = replies_until_closed(&mut r); + assert_eq!(replies.len(), 1, "{replies:?}"); + assert!(replies[0]["error"].as_str().unwrap().contains("closing the connection"), "{replies:?}"); + assert!(rx.try_recv().is_err(), "the body was dispatched"); + } + + #[test] + fn a_line_that_is_not_a_request_closes_the_connection() { + let (addr, rx) = server(small()); + ui(rx); + for junk in [&b"hello\n"[..], b"[1,2]\n", b"{\"id\":1}\n", b"{\"method\":5}\n", b"\xff\xfe\n", b"GET /x HTTP/1.1\n"] { + let (mut r, mut w) = client(addr); + w.write_all(junk).unwrap(); + w.write_all(b"{\"id\":2,\"method\":\"m\"}\n").unwrap(); + let replies = replies_until_closed(&mut r); + assert_eq!(replies.len(), 1, "{replies:?}"); + assert_eq!(replies[0]["ok"], false); + } + assert!(is_http_request_line("OPTIONS * HTTP/1.1")); + assert!(is_http_request_line("PRI * HTTP/2.0")); + assert!(!is_http_request_line("{\"method\":\"GET /\"}")); + } + #[test] fn oversized_line_closes_connection() { let (addr, rx) = server(small()); diff --git a/docs/control-protocol.md b/docs/control-protocol.md index e715d1c7..e0d6d227 100644 --- a/docs/control-protocol.md +++ b/docs/control-protocol.md @@ -3,6 +3,10 @@ Start the app with `--control `. The server listens on `127.0.0.1` only and speaks JSON lines: one request object per line, one reply per line. +A line that is not a JSON object with a string `method` (an HTTP request, text that isn't JSON, +bytes that aren't UTF-8) gets one error reply and the server closes the connection; nothing sent +after it runs. Blank lines are skipped. + The connected MCP client requires the reply's numeric `id` to match its request, `ok` to be a boolean, and exactly one of `result` (success) or a string `error` (failure). It caps each reply at 16 MiB including its terminating newline, accepts CRLF, and rejects EOF inside a frame. Invalid