From 8b1cbb741be3105a0f9c499f7f832fa8dca90236 Mon Sep 17 00:00:00 2001 From: Gh0stly <26194374+Gh0stlyKn1ght@users.noreply.github.com> Date: Sun, 4 Oct 2026 13:58:59 -0400 Subject: [PATCH] security: authenticate control channels and bound transport resources (#61) * security: authenticate and bound control transports * fix: keep connection limiter compatible with current stable --- Cargo.lock | 2 + Cargo.toml | 1 + README.md | 2 +- SECURITY.md | 2 +- apps/photocraft-cli/src/lib.rs | 37 ++- apps/photocraft/Cargo.toml | 1 + apps/photocraft/src/control_server.rs | 59 ++++- apps/photocraft/src/main.rs | 34 ++- book/src/architecture/automation.md | 2 +- book/src/automation/control-protocol.md | 9 +- book/src/automation/mcp.md | 8 +- book/src/automation/overview.md | 2 +- book/src/automation/security-model.md | 10 +- book/src/security/automation-security.md | 11 +- book/src/security/overview.md | 8 +- crates/automation/Cargo.toml | 1 + crates/automation/src/bridge.rs | 26 ++- crates/automation/src/lib.rs | 1 + crates/automation/src/rpc.rs | 106 ++++++++- crates/automation/src/security.rs | 272 +++++++++++++++++++++++ crates/automation/src/server.rs | 13 +- crates/automation/tests/mcp.rs | 43 +++- docs/control-protocol.md | 42 +++- docs/development.md | 14 +- 24 files changed, 629 insertions(+), 77 deletions(-) create mode 100644 crates/automation/src/security.rs diff --git a/Cargo.lock b/Cargo.lock index 69a67253..99311e5f 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3242,6 +3242,7 @@ dependencies = [ "egui", "log", "open", + "photocraft-automation", "photocraft-codecs", "photocraft-color", "photocraft-compose", @@ -3274,6 +3275,7 @@ name = "photocraft-automation" version = "0.1.1" dependencies = [ "base64", + "getrandom 0.3.4", "photocraft-codecs", "photocraft-compose", "photocraft-doc", diff --git a/Cargo.toml b/Cargo.toml index 5f49402a..02d9e5c3 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -48,6 +48,7 @@ rfd = "0.17" log = "0.4" rayon = "1" criterion = { version = "0.5", default-features = false } +getrandom = "0.3" [workspace.lints.rust] unsafe_code = "forbid" diff --git a/README.md b/README.md index 6a7b2a82..70ea0729 100644 --- a/README.md +++ b/README.md @@ -235,7 +235,7 @@ photocraft-cli batch --actions grade.json --in ./raw --out ./graded photocraft-cli mcp ``` -The desktop app also listens on a local control channel (`photocraft --control`) for inspecting UI state, driving tools with pointer events, and taking offscreen screenshots. Every image in this README was rendered that way. See [`docs/control-protocol.md`](docs/control-protocol.md). +The desktop app also offers an authenticated, loopback-only control channel (`photocraft --control`) for inspecting UI state, driving tools with pointer events, and taking offscreen screenshots. Every image in this README was rendered that way. See [`docs/control-protocol.md`](docs/control-protocol.md). ## Under the hood diff --git a/SECURITY.md b/SECURITY.md index b7186f52..fd141506 100644 --- a/SECURITY.md +++ b/SECURITY.md @@ -49,4 +49,4 @@ General bugs without a security impact should use the project's normal issue and ## Current limitations -The current loopback control channel has no authentication or capability model. Loopback binding is not an authorization boundary. Automation file operations are not restricted to configured workspace roots, and explicit request, batch, and connection limits are not yet implemented. See the [security documentation](book/src/security/overview.md) for the source-backed status and hardening roadmap. +TCP control connections require a 256-bit bearer token before method dispatch and enforce request-line, active-connection, and batch-step limits. These protections do not provide a capability model: an authenticated client still receives the complete exposed control surface. Automation file operations are not restricted to configured workspace roots, and JSON-depth, render, document-memory, command-duration, client-identity, encryption, and audit controls are not yet implemented. See the [security documentation](book/src/security/overview.md) for the source-backed status and hardening roadmap. diff --git a/apps/photocraft-cli/src/lib.rs b/apps/photocraft-cli/src/lib.rs index 43a1295e..f2bba008 100644 --- a/apps/photocraft-cli/src/lib.rs +++ b/apps/photocraft-cli/src/lib.rs @@ -6,7 +6,7 @@ use std::io::Write; use std::path::{Path, PathBuf}; -use photocraft_automation::{Headless, PhotocraftMcp, files}; +use photocraft_automation::{Headless, PhotocraftMcp, files, security}; use photocraft_io::ExportOptions; use serde_json::{Value, json}; @@ -27,9 +27,9 @@ USAGE: Run a droplet (File › Automate › Create Droplet) on images and folders. photocraft-cli commands [--json] [--filter ] List the engine command registry. - photocraft-cli mcp [--bridge <127.0.0.1:port>] + photocraft-cli mcp [--bridge <127.0.0.1:port>] [--control-token <64-hex> | --control-token-file ] Run the MCP server on stdio (headless engine, or bridge to a running `photocraft --control `). - photocraft-cli serve [--port ] + photocraft-cli serve [--port ] [--control-token <64-hex> | --control-token-file ] Keep one headless session open and answer JSON lines ({\"id\",\"method\",\"params\"}) on stdio, or on 127.0.0.1:. Methods: engine.execute, engine.commands, doc.open/new/save/inspect/render/ select/close, session.list, batch, methods (docs/control-protocol.md#headless-server). @@ -40,7 +40,21 @@ struct Args { flags: Vec<(String, Option)>, } -const VALUE_FLAGS: &[&str] = &["--format", "--quality", "--new", "--cmd", "--params", "--out", "--actions", "--in", "--filter", "--bridge", "--port"]; +const VALUE_FLAGS: &[&str] = &[ + "--format", + "--quality", + "--new", + "--cmd", + "--params", + "--out", + "--actions", + "--in", + "--filter", + "--bridge", + "--port", + "--control-token", + "--control-token-file", +]; fn parse(args: &[String]) -> Result { let mut a = Args { positional: Vec::new(), flags: Vec::new() }; @@ -328,7 +342,14 @@ fn serve(a: &Args, err: &mut dyn Write) -> R { Some(port) => { let port: u16 = port.parse().map_err(|_| format!("bad --port `{port}`"))?; let addr = format!("127.0.0.1:{port}"); - photocraft_automation::rpc::serve_tcp(&addr, h, |local| { + let (supplied, token_file) = security::token_inputs(a.get("--control-token").map(str::to_owned), a.get("--control-token-file").map(PathBuf::from)); + let token = security::server_token(supplied.as_deref(), token_file.as_deref()).map_err(|e| e.to_string())?; + if let Some(path) = &token_file { + let _ = writeln!(err, "photocraft-cli control token file: {}", path.display()); + } else if supplied.is_none() { + let _ = writeln!(err, "photocraft-cli control token: {token}"); + } + photocraft_automation::rpc::serve_tcp(&addr, h, token, |local| { let _ = writeln!(err, "photocraft-cli serving on {local}"); }) .map_err(|e| e.to_string()) @@ -342,7 +363,11 @@ fn serve(a: &Args, err: &mut dyn Write) -> R { fn mcp(a: &Args) -> R { let server = match a.get("--bridge") { - Some(addr) => PhotocraftMcp::bridge(addr).map_err(|e| e.to_string())?, + Some(addr) => { + let (supplied, token_file) = security::token_inputs(a.get("--control-token").map(str::to_owned), a.get("--control-token-file").map(PathBuf::from)); + let token = security::client_token(supplied.as_deref(), token_file.as_deref()).map_err(|e| e.to_string())?; + PhotocraftMcp::bridge(addr, &token).map_err(|e| e.to_string())? + } None => PhotocraftMcp::headless(), }; let rt = tokio::runtime::Builder::new_multi_thread().enable_all().build().map_err(|e| e.to_string())?; diff --git a/apps/photocraft/Cargo.toml b/apps/photocraft/Cargo.toml index 91b13cd4..9b0dcc9e 100644 --- a/apps/photocraft/Cargo.toml +++ b/apps/photocraft/Cargo.toml @@ -20,6 +20,7 @@ photocraft-engine = { workspace = true } photocraft-codecs = { workspace = true } photocraft-io = { workspace = true } photocraft-format = { workspace = true } +photocraft-automation = { workspace = true } photocraft-ui-egui = { workspace = true } eframe = { workspace = true, features = ["default_fonts", "wgpu", "accesskit", "wayland", "x11"] } egui = { workspace = true } diff --git a/apps/photocraft/src/control_server.rs b/apps/photocraft/src/control_server.rs index 946e2718..d5b747f2 100644 --- a/apps/photocraft/src/control_server.rs +++ b/apps/photocraft/src/control_server.rs @@ -1,15 +1,19 @@ //! Localhost JSON-lines control server (one request per line, one reply per line). //! Loopback only. This is the transport the MCP server will wrap. -use std::io::{BufRead, BufReader, Write}; +use std::io::{BufReader, Write}; use std::net::{TcpListener, TcpStream}; +use std::sync::Arc; use std::sync::mpsc::{Receiver, Sender, channel}; use std::time::Duration; +use photocraft_automation::security::{ + ConnectionLimiter, LineRead, MAX_CONNECTIONS, MAX_REQUEST_BYTES, authentication_reply, configure_stream, read_bounded_line, +}; use photocraft_ui_egui::ControlRequest; use serde_json::{Value, json}; -pub fn start(port: u16, ctx: egui::Context) -> Receiver { +pub fn start(port: u16, token: String, ctx: egui::Context) -> Receiver { let (tx, rx) = channel::(); let listener = match TcpListener::bind(("127.0.0.1", port)) { Ok(l) => l, @@ -20,21 +24,57 @@ pub fn start(port: u16, ctx: egui::Context) -> Receiver { }; eprintln!("photocraft: control server listening on 127.0.0.1:{port}"); std::thread::spawn(move || { - for stream in listener.incoming().flatten() { + let limiter = ConnectionLimiter::new(MAX_CONNECTIONS); + let token = Arc::new(token); + for mut stream in listener.incoming().flatten() { + let Some(permit) = limiter.try_acquire() else { + let _ = configure_stream(&stream); + let _ = writeln!(stream, "{}", json!({"id": null, "ok": false, "error": "connection limit reached"})); + continue; + }; let tx = tx.clone(); let ctx = ctx.clone(); - std::thread::spawn(move || serve(stream, tx, ctx)); + let token = Arc::clone(&token); + std::thread::spawn(move || { + let _permit = permit; + serve(stream, &token, tx, ctx); + }); } }); rx } -fn serve(stream: TcpStream, tx: Sender, ctx: egui::Context) { +fn serve(stream: TcpStream, token: &str, tx: Sender, ctx: egui::Context) { + if configure_stream(&stream).is_err() { + return; + } let Ok(read) = stream.try_clone() else { return }; + let mut reader = BufReader::new(read); let mut out = stream; - for line in BufReader::new(read).lines() { - let Ok(line) = line else { break }; - if line.trim().is_empty() { + let mut line = String::new(); + let mut authenticated = false; + loop { + match read_bounded_line(&mut reader, &mut line) { + Ok(LineRead::Eof) | Err(_) => break, + Ok(LineRead::TooLong) => { + let reply = json!({ + "id": null, + "ok": false, + "error": format!("request exceeds {MAX_REQUEST_BYTES} bytes"), + }); + let _ = writeln!(out, "{reply}"); + let _ = out.flush(); + break; + } + Ok(LineRead::Line) if line.trim().is_empty() => continue, + Ok(LineRead::Line) => {} + } + if !authenticated { + let (reply, ok) = authentication_reply(&line, token); + authenticated = ok; + if writeln!(out, "{reply}").is_err() || out.flush().is_err() || !authenticated { + break; + } continue; } let reply = match serde_json::from_str::(&line) { @@ -58,5 +98,8 @@ fn serve(stream: TcpStream, tx: Sender, ctx: egui::Context) { if writeln!(out, "{reply}").is_err() { break; } + if out.flush().is_err() { + break; + } } } diff --git a/apps/photocraft/src/main.rs b/apps/photocraft/src/main.rs index 2c0f40bd..d42debda 100644 --- a/apps/photocraft/src/main.rs +++ b/apps/photocraft/src/main.rs @@ -1,9 +1,10 @@ //! Photocraft desktop app. //! -//! Usage: `photocraft [--control ] [files…]` +//! Usage: `photocraft [--control ] [--control-token <64-hex> | +//! --control-token-file ] [files…]` //! //! `--control ` (or `PHOTOCRAFT_CONTROL_PORT`) starts a localhost JSON-lines control server. -//! Each line `{"id":1,"method":"ui.inspect","params":{}}` gets a reply line +//! The first line must authenticate; subsequent request lines get reply lines. //! `{"id":1,"ok":true,"result":…}`. See `photocraft_ui_egui::control` for the methods. // Release builds on Windows are GUI-subsystem apps, so launching from the Start Menu or Explorer @@ -34,11 +35,15 @@ fn app_icon() -> egui::IconData { fn main() -> eframe::Result { crash_guard::install_hook(); let mut control_port: Option = std::env::var("PHOTOCRAFT_CONTROL_PORT").ok().and_then(|p| p.parse().ok()); + let mut control_token = None; + let mut control_token_file = None; let mut files = Vec::new(); let mut args = std::env::args().skip(1); while let Some(a) = args.next() { match a.as_str() { "--control" => control_port = args.next().and_then(|p| p.parse().ok()), + "--control-token" => control_token = args.next(), + "--control-token-file" => control_token_file = args.next().map(std::path::PathBuf::from), "--version" => { println!("photocraft {}", photocraft_engine::build_info::long_version()); return Ok(()); @@ -49,6 +54,27 @@ fn main() -> eframe::Result { } } + let control = if let Some(port) = control_port { + let (supplied, token_file) = photocraft_automation::security::token_inputs(control_token, control_token_file); + let token = match photocraft_automation::security::server_token(supplied.as_deref(), token_file.as_deref()) { + Ok(token) => token, + Err(e) => { + eprintln!("photocraft: cannot configure control authentication: {e}"); + return Ok(()); + } + }; + if let Some(path) = token_file { + eprintln!("photocraft: control token file: {}", path.display()); + } else if supplied.is_none() { + eprintln!("photocraft: control token: {token}"); + } else { + eprintln!("photocraft: using supplied control token"); + } + Some((port, token)) + } else { + None + }; + let options = eframe::NativeOptions { viewport: egui::ViewportBuilder::default() .with_icon(app_icon()) @@ -75,8 +101,8 @@ fn main() -> eframe::Result { { app.set_wgpu(rs); } - if let Some(port) = control_port { - let rx = control_server::start(port, cc.egui_ctx.clone()); + if let Some((port, token)) = control { + let rx = control_server::start(port, token, cc.egui_ctx.clone()); app = app.with_control(rx); } for f in files { diff --git a/book/src/architecture/automation.md b/book/src/architecture/automation.md index 1930d90a..2122a79a 100644 --- a/book/src/architecture/automation.md +++ b/book/src/architecture/automation.md @@ -16,4 +16,4 @@ Headless file operations are implemented in `crates/automation/src/files.rs`; RP The control and headless TCP listeners are restricted to loopback addresses. Stdio MCP avoids a listening network socket, but the MCP process still acts with the filesystem permissions of the user who launched it. -Loopback is not authentication. Current transports do not implement authentication, capability scopes, allowed filesystem roots, request-byte limits, batch-step limits, or connection limits. See the [Automation security model](../automation/security-model.md) and [Automation security](../security/automation-security.md) before exposing or extending these surfaces. +Loopback is not authentication. TCP transports now require a bearer token and enforce request-byte, batch-step, and active-connection limits, but they do not implement capability scopes, allowed filesystem roots, client identity, encryption, or operation budgets. See the [Automation security model](../automation/security-model.md) and [Automation security](../security/automation-security.md) before exposing or extending these surfaces. diff --git a/book/src/automation/control-protocol.md b/book/src/automation/control-protocol.md index a00ea526..c74c124d 100644 --- a/book/src/automation/control-protocol.md +++ b/book/src/automation/control-protocol.md @@ -1,8 +1,9 @@ # Control protocol -The desktop control protocol is newline-delimited JSON over a TCP listener bound to `127.0.0.1`. Each request carries `id`, `method`, and optional `params`; each reply includes the matching `id` and either a result or error. +The desktop control protocol is newline-delimited JSON over a TCP listener bound to `127.0.0.1`. The first frame on every connection must be `auth` with the configured 256-bit bearer token. After authentication, each request carries `id`, `method`, and optional `params`; each reply includes the matching `id` and either a result or error. ```json +{"id":"auth","method":"auth","params":{"token":"<64-hex>"}} {"id":1,"method":"ui.inspect","params":{}} ``` @@ -16,8 +17,8 @@ Implementation is split across: ## Current security posture -**Implemented:** the desktop server binds to IPv4 loopback, the bridge accepts only loopback-style addresses, and request handlers use a 60-second response timeout. +**Implemented:** the desktop server binds to IPv4 loopback; every TCP connection authenticates before dispatch; the bridge accepts only loopback-style addresses and authenticates on connection; encoded request lines are limited to 1 MiB; active connections are limited to 16; socket I/O and handler waits use timeouts; MCP and headless batches are limited to 256 steps. -**Known limitations:** there is no authentication handshake, encryption, client identity, per-method capability check, request-line byte limit, connection limit, or bounded worker pool. The listener creates one thread per accepted connection. Any local process able to connect can attempt exposed operations with PhotoCraft's user permissions. +**Known limitations:** the token is a bearer credential and grants the complete exposed control surface. There is no encryption, client identity, per-method capability check, allowed filesystem root, JSON-depth limit, or operation/resource budget. The listener creates one thread per authenticated active connection, within the connection cap. -Enable `--control` only for the duration of a trusted local automation session. See [Automation security](../security/automation-security.md) for the proposed gateway model. +Use a private token file, enable `--control` only for a trusted local automation session, and never proxy the unencrypted protocol beyond loopback. See [Automation security](../security/automation-security.md) for the remaining gateway work. diff --git a/book/src/automation/mcp.md b/book/src/automation/mcp.md index ed30c8d4..580254c6 100644 --- a/book/src/automation/mcp.md +++ b/book/src/automation/mcp.md @@ -11,18 +11,18 @@ Start the headless server with: photocraft-cli mcp ``` -Bridge to a running desktop application with: +Bridge to a running desktop application with the same private token file used by the app: ```sh -photocraft-cli mcp --bridge 127.0.0.1:7878 +photocraft-cli mcp --bridge 127.0.0.1:7878 --control-token-file /private/path/control.token ``` Tools cover session/document operations, command discovery and execution, batching, and—when bridged—UI inspection and control. Tool schemas improve correctness but are not authorization boundaries. ## Security notes -Stdio MCP does not open a network listener. Its client can still invoke file operations and engine commands using the MCP process's operating-system permissions. Bridge mode additionally inherits the unauthenticated control-channel limitation. +Stdio MCP does not open a network listener. Its client can still invoke file operations and engine commands using the MCP process's operating-system permissions. Bridge mode authenticates its loopback control connection with a bearer token before invoking any control method. -The current server does not issue scoped sessions or distinguish trusted tools from high-impact tools. A connected client is not restricted to document-only access. Do not expose the stdio transport through an untrusted broker, shell, or remote service without adding an external policy boundary. +The current server does not issue scoped sessions or distinguish trusted tools from high-impact tools. Authentication proves possession of the token; it does not restrict an authenticated client to document-only access. Do not expose either transport through an untrusted broker, shell, proxy, or remote service without adding an external policy boundary. Capability-scoped MCP access is [proposed](../security/automation-security.md), not implemented. diff --git a/book/src/automation/overview.md b/book/src/automation/overview.md index c684364c..bbc77a1e 100644 --- a/book/src/automation/overview.md +++ b/book/src/automation/overview.md @@ -6,7 +6,7 @@ PhotoCraft automation has three entry styles: 2. `photocraft-cli serve` keeps a headless session alive over JSON lines on stdio or loopback TCP. 3. `photocraft-cli mcp` exposes MCP tools over stdio, either against an in-process headless session or through a bridge to the desktop control server. -The desktop application can start a loopback JSON control server with `photocraft --control `. It exposes engine and UI operations, including pointer/type input and screenshots. +The desktop application can start an authenticated loopback JSON control server with `photocraft --control `. It exposes engine and UI operations, including pointer/type input and screenshots. Each TCP connection must present the configured bearer token before any operation is dispatched. All editing routes eventually reach the command registry. Transport behavior is not equivalent, however: only the live bridge can use UI-specific methods, and file operations execute in the process that owns the selected backend. diff --git a/book/src/automation/security-model.md b/book/src/automation/security-model.md index 29aeb5e4..6e8b94dc 100644 --- a/book/src/automation/security-model.md +++ b/book/src/automation/security-model.md @@ -7,13 +7,13 @@ Automation requests are untrusted input with potential effects beyond the active | Loopback-only TCP binding | **Implemented** for desktop control and headless TCP serve | | Stdio transport | **Implemented** for MCP and headless JSON lines | | Command-level error handling | **Implemented**, with command-specific tests and `panic_hunt` coverage | -| Control-channel authentication | **Known limitation:** absent | +| Control-channel authentication | **Implemented:** 256-bit bearer token required before TCP method dispatch | | MCP capability scopes | **Known limitation:** absent | | Allowed read/write roots | **Known limitation:** absent | | Symlink-safe capability filesystem | **Known limitation:** absent | -| Request-byte and JSON-depth limits | **Known limitation:** absent at the transport boundary | -| Batch-step limit | **Known limitation:** absent | -| Connection/worker limit | **Known limitation:** absent for TCP servers | +| Request-byte and JSON-depth limits | **Partial:** 1 MiB request-line limit; no explicit JSON-depth policy | +| Batch-step limit | **Implemented:** 256 steps for headless and MCP batches | +| Connection/worker limit | **Implemented:** 16 active TCP connections; one worker thread per accepted active connection | | Security audit events | **Proposed** | ## Proposed gateway @@ -34,4 +34,4 @@ MCP or control client command engine / UI shell ``` -A token alone would identify possession of a secret but would not limit what that client can do. The proposed design combines a random per-launch credential with explicit capabilities, filesystem handles, resource budgets, and command policy. Until those controls land, process isolation and least-privileged execution are the practical containment mechanisms. +The current token proves possession of a secret but does not limit what that client can do. The remaining design combines the authenticated transport with explicit capabilities, filesystem handles, resource budgets, and command policy. Until those controls land, private token handling, process isolation, and least-privileged execution are practical containment mechanisms. diff --git a/book/src/security/automation-security.md b/book/src/security/automation-security.md index df0e438d..b3a8d294 100644 --- a/book/src/security/automation-security.md +++ b/book/src/security/automation-security.md @@ -6,25 +6,24 @@ Automation is a privilege boundary because requests can cause filesystem access, - Desktop control binds to `127.0.0.1` and uses one JSON request/reply per line. - Headless TCP refuses a successfully bound non-loopback address. +- Every desktop-control and headless-TCP connection must authenticate with a 256-bit bearer token before method dispatch. +- Encoded request lines are limited to 1 MiB, active TCP connections to 16, and headless/MCP batches to 256 steps. - MCP normally uses stdio and can optionally bridge to loopback control TCP. - The bridge and UI request paths use timeouts. - Engine commands are expected to reject invalid parameters without panicking. ## Known limitations -- no authentication or session establishment on control/headless TCP; - no per-client or per-tool capabilities; - no filesystem read/write root restriction; -- no bounded request line or JSON-depth policy at transport entry; -- no maximum batch length; -- no connection count or bounded worker pool; +- no explicit JSON-depth, response-size, render, document-memory, or command-duration budget; - one thread per accepted TCP connection; - no structured security audit event stream; - full-size renders and expensive engine commands are not charged to a session budget. ## Proposed session model -A future security gateway should create a short-lived authenticated session from a cryptographically random per-launch credential. Authentication should occur before method discovery. +The transport now establishes an authenticated connection from a cryptographically random credential before method discovery. A future security gateway should turn that authenticated connection into a short-lived, capability-scoped session. Capabilities should be explicit and composable, for example: @@ -56,4 +55,4 @@ Limits should fail with stable errors, be applied before expensive work, and be ## Deployment guidance today -Prefer stdio, start control TCP only when needed, bind only to loopback, use a least-privileged OS account, and isolate untrusted agents from sensitive files. Do not tunnel or proxy the current loopback protocol to another host. +Prefer stdio, start control TCP only when needed, use a private token file, keep the listener on loopback, use a least-privileged OS account, and isolate untrusted agents from sensitive files. Do not commit or log bearer tokens. Do not tunnel or proxy the unencrypted loopback protocol to another host. diff --git a/book/src/security/overview.md b/book/src/security/overview.md index a5a27a05..e9545076 100644 --- a/book/src/security/overview.md +++ b/book/src/security/overview.md @@ -10,8 +10,8 @@ PhotoCraft processes complex attacker-controlled files and exposes automation th | Raster codecs | Default dimension/pixel/allocation limits; decoder-specific budgets; malformed/property tests; fuzz targets | Continuous fuzzing and more format-specific semantic cases | | `.pcraft` | Manifest/blob/total-byte limits; bounded decompression; CRC/hash and version checks; fuzz target | Tighter complexity/entry limits and directory-bundle path-race analysis | | Engine commands | Error-return contract, focused bad-parameter tests, adversarial `panic_hunt` test | Continue eliminating input-derived panic and exhaustion paths | -| Control TCP | Loopback binding and timeouts | No authentication, capabilities, request/batch/connection limits, or audit trail | -| MCP | Stdio transport; typed tool schemas; headless/bridge separation | No authorization scopes or filesystem policy | +| Control TCP | Loopback binding; 256-bit bearer-token authentication; 1 MiB request lines; 16 active connections; I/O timeouts | No capabilities, filesystem policy, encryption, client identity, JSON-depth/operation budgets, or audit trail | +| MCP | Stdio transport; typed tool schemas; headless/bridge separation; authenticated bridge; 256-step batch limit | No authorization scopes or filesystem policy | | Dependencies | Cargo lockfile and normal CI build/test/clippy | No `cargo-audit` or `cargo-deny` job/configuration confirmed in current tree | | Releases | Checksums and platform signing/notarization paths when secrets are configured | Signing can be absent; no SBOM generation confirmed in current tree | @@ -19,9 +19,9 @@ The table is a snapshot of the current repository, not a guarantee about deploye ## Security priorities -1. Add authenticated, capability-scoped automation sessions. +1. Add capability-scoped automation sessions on top of the authenticated transport. 2. Restrict automation file access through explicit workspace handles. -3. Bound request size, JSON complexity, batch length, connections, and render work. +3. Add JSON-complexity, response-size, render, document-memory, and command-duration budgets. 4. Expand PSD/PSB, codec, and `.pcraft` fuzzing with regression corpora. 5. Add dependency advisories/license policy checks and release SBOMs. diff --git a/crates/automation/Cargo.toml b/crates/automation/Cargo.toml index cdb23bd4..d6b9c0ed 100644 --- a/crates/automation/Cargo.toml +++ b/crates/automation/Cargo.toml @@ -24,6 +24,7 @@ rmcp = { version = "3.5", default-features = false, features = ["server", "macro tokio = { version = "1", features = ["rt-multi-thread", "macros", "io-std", "sync", "net", "io-util", "time"] } base64 = "0.22" schemars = "1" +getrandom = { workspace = true } [dev-dependencies] rmcp = { version = "3.5", default-features = false, features = ["client", "server", "macros", "transport-io"] } diff --git a/crates/automation/src/bridge.rs b/crates/automation/src/bridge.rs index 43f6ee92..77efa392 100644 --- a/crates/automation/src/bridge.rs +++ b/crates/automation/src/bridge.rs @@ -11,11 +11,13 @@ use tokio::net::TcpStream; use tokio::sync::Mutex; use crate::AutomationError; +use crate::security::{AUTH_METHOD, MAX_REQUEST_BYTES, validate_token}; type Conn = (BufReader, tokio::net::tcp::OwnedWriteHalf); pub struct BridgeClient { addr: String, + token: String, conn: Mutex>, next_id: AtomicU64, timeout: Duration, @@ -24,13 +26,15 @@ pub struct BridgeClient { impl BridgeClient { /// `addr` such as `127.0.0.1:7878`. Only loopback addresses are accepted, /// matching the server, which binds to loopback only. - pub fn new(addr: impl Into) -> Result { + pub fn new(addr: impl Into, token: impl Into) -> Result { let addr = addr.into(); + let token = token.into(); let host = addr.rsplit_once(':').map(|(h, _)| h).unwrap_or(&addr); if !matches!(host, "127.0.0.1" | "localhost" | "[::1]" | "::1") { return Err(AutomationError::BadRequest(format!("bridge address must be loopback, got `{addr}`"))); } - Ok(BridgeClient { addr, conn: Mutex::new(None), next_id: AtomicU64::new(1), timeout: Duration::from_secs(60) }) + validate_token(&token)?; + Ok(BridgeClient { addr, token: token.to_ascii_lowercase(), conn: Mutex::new(None), next_id: AtomicU64::new(1), timeout: Duration::from_secs(60) }) } pub fn with_timeout(mut self, t: Duration) -> Self { @@ -51,9 +55,20 @@ impl BridgeClient { let s = tokio::time::timeout(Duration::from_secs(5), TcpStream::connect(&self.addr)) .await .map_err(|_| AutomationError::Bridge(format!("timed out connecting to {}", self.addr)))? - .map_err(|e| AutomationError::Bridge(format!("cannot connect to {} ({e}); start the app with `photocraft --control `", self.addr)))?; + .map_err(|e| { + AutomationError::Bridge(format!( + "cannot connect to {} ({e}); start the app with `photocraft --control ` and matching control credentials", + self.addr + )) + })?; let (r, w) = s.into_split(); - *guard = Some((BufReader::new(r), w)); + let mut conn = (BufReader::new(r), w); + let auth_id = self.next_id.fetch_add(1, Ordering::Relaxed); + let auth = tokio::time::timeout(Duration::from_secs(5), exchange(&mut conn, auth_id, AUTH_METHOD, &json!({"token": self.token}))) + .await + .map_err(|_| AutomationError::Bridge("control authentication timed out".into()))??; + auth?; + *guard = Some(conn); } let id = self.next_id.fetch_add(1, Ordering::Relaxed); let Some(conn) = guard.as_mut() else { @@ -83,6 +98,9 @@ impl BridgeClient { async fn exchange(conn: &mut Conn, id: u64, method: &str, params: &Value) -> Result, AutomationError> { let mut line = serde_json::to_string(&json!({"id": id, "method": method, "params": params})).map_err(|e| AutomationError::Other(e.to_string()))?; line.push('\n'); + if line.len() > MAX_REQUEST_BYTES { + return Ok(Err(AutomationError::BadRequest(format!("request is {} bytes; maximum is {MAX_REQUEST_BYTES}", line.len())))); + } let io = |e: std::io::Error| AutomationError::Bridge(e.to_string()); conn.1.write_all(line.as_bytes()).await.map_err(io)?; conn.1.flush().await.map_err(io)?; diff --git a/crates/automation/src/lib.rs b/crates/automation/src/lib.rs index d3c115ef..7405c41b 100644 --- a/crates/automation/src/lib.rs +++ b/crates/automation/src/lib.rs @@ -20,6 +20,7 @@ pub mod bridge; pub mod files; pub mod headless; pub mod rpc; +pub mod security; pub mod server; pub use bridge::BridgeClient; diff --git a/crates/automation/src/rpc.rs b/crates/automation/src/rpc.rs index cb99ba91..e98dbd88 100644 --- a/crates/automation/src/rpc.rs +++ b/crates/automation/src/rpc.rs @@ -21,6 +21,9 @@ use std::sync::{Arc, Mutex, PoisonError}; use base64::Engine as _; use serde_json::{Value, json}; +use crate::security::{ + ConnectionLimiter, LineRead, MAX_BATCH_STEPS, MAX_CONNECTIONS, MAX_REQUEST_BYTES, authentication_reply, configure_stream, read_bounded_line, +}; use crate::{AutomationError, Headless}; /// Method names served by [`Headless::handle`]. @@ -122,6 +125,9 @@ impl Headless { /// `stopOnError` is false; the reply lists every step's result. pub fn batch(&mut self, p: &Value) -> Result { let steps = p.get("steps").and_then(Value::as_array).ok_or_else(|| bad("batch needs `steps`"))?; + if steps.len() > MAX_BATCH_STEPS { + return Err(bad(format!("batch contains {} steps; maximum is {MAX_BATCH_STEPS}", steps.len()))); + } let stop = p.get("stopOnError").and_then(Value::as_bool).unwrap_or(true); let mut results = Vec::with_capacity(steps.len()); let mut failed = 0usize; @@ -186,21 +192,67 @@ pub fn serve_lines(h: &Mutex, r: impl BufRead, mut w: impl Write) -> s Ok(()) } -/// Serve on a loopback TCP address (one thread per connection, one shared session). -/// Refuses non-loopback addresses. -pub fn serve_tcp(addr: &str, h: Arc>, ready: impl FnOnce(std::net::SocketAddr)) -> Result<(), AutomationError> { +fn serve_tcp_connection(h: &Mutex, stream: std::net::TcpStream, token: &str) -> std::io::Result<()> { + configure_stream(&stream)?; + let read = stream.try_clone()?; + let mut reader = std::io::BufReader::new(read); + let mut out = stream; + let mut line = String::new(); + let mut authenticated = false; + loop { + match read_bounded_line(&mut reader, &mut line)? { + LineRead::Eof => return Ok(()), + LineRead::TooLong => { + let reply = json!({ + "id": null, + "ok": false, + "error": format!("request exceeds {MAX_REQUEST_BYTES} bytes"), + }); + writeln!(out, "{reply}")?; + out.flush()?; + return Ok(()); + } + LineRead::Line if line.trim().is_empty() => continue, + LineRead::Line => {} + } + let reply = if authenticated { + respond(h, &line) + } else { + let (reply, ok) = authentication_reply(&line, token); + authenticated = ok; + reply + }; + writeln!(out, "{reply}")?; + out.flush()?; + if !authenticated { + return Ok(()); + } + } +} + +/// Serve authenticated JSON lines on a loopback TCP address with one shared session. +/// Refuses non-loopback addresses and caps active connections and request bytes. +pub fn serve_tcp(addr: &str, h: Arc>, token: String, ready: impl FnOnce(std::net::SocketAddr)) -> Result<(), AutomationError> { let listener = TcpListener::bind(addr).map_err(|e| AutomationError::Io(format!("bind {addr}: {e}")))?; let local = listener.local_addr().map_err(|e| AutomationError::Io(e.to_string()))?; if !local.ip().is_loopback() { return Err(bad(format!("{addr} is not a loopback address"))); } ready(local); + let limiter = ConnectionLimiter::new(MAX_CONNECTIONS); + let token = Arc::new(token); for stream in listener.incoming() { - let Ok(stream) = stream else { continue }; + let Ok(mut stream) = stream else { continue }; + let Some(permit) = limiter.try_acquire() else { + let _ = configure_stream(&stream); + let _ = writeln!(stream, "{}", json!({"id": null, "ok": false, "error": "connection limit reached"})); + continue; + }; let h = h.clone(); + let token = Arc::clone(&token); std::thread::spawn(move || { - let Ok(read) = stream.try_clone() else { return }; - let _ = serve_lines(&h, std::io::BufReader::new(read), stream); + let _permit = permit; + let _ = serve_tcp_connection(&h, stream, &token); }); } Ok(()) @@ -279,6 +331,15 @@ mod tests { assert!(insp.to_string().contains("\"B\"")); } + #[test] + fn batch_rejects_too_many_steps() { + let h = session(); + let mut g = h.lock().unwrap(); + let steps = vec![json!({"command": "command.list"}); MAX_BATCH_STEPS + 1]; + let error = g.handle("batch", json!({"steps": steps})).unwrap_err(); + assert!(error.to_string().contains("maximum is 256")); + } + #[test] fn render_to_file_and_base64() { let h = session(); @@ -298,18 +359,45 @@ mod tests { #[test] fn tcp_serves_loopback() { use std::io::{BufReader, Write as _}; + const TOKEN: &str = "0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef"; let h = Arc::new(Mutex::new(Headless::new())); let (tx, rx) = std::sync::mpsc::channel(); std::thread::spawn(move || { - let _ = serve_tcp("127.0.0.1:0", h, move |a| tx.send(a).unwrap()); + let _ = serve_tcp("127.0.0.1:0", h, TOKEN.into(), move |a| tx.send(a).unwrap()); }); let addr = rx.recv().unwrap(); let mut s = std::net::TcpStream::connect(addr).unwrap(); - writeln!(s, r#"{{"id":"a","method":"methods"}}"#).unwrap(); + writeln!(s, r#"{{"id":"auth","method":"auth","params":{{"token":"{TOKEN}"}}}}"#).unwrap(); + let mut reader = BufReader::new(s.try_clone().unwrap()); let mut line = String::new(); - BufReader::new(s.try_clone().unwrap()).read_line(&mut line).unwrap(); + reader.read_line(&mut line).unwrap(); + let auth: Value = serde_json::from_str(&line).unwrap(); + assert_eq!(auth["result"]["authenticated"], true); + writeln!(s, r#"{{"id":"a","method":"methods"}}"#).unwrap(); + line.clear(); + reader.read_line(&mut line).unwrap(); let v: Value = serde_json::from_str(&line).unwrap(); assert_eq!(v["id"], "a"); assert!(v["result"].as_array().unwrap().iter().any(|m| m == "batch")); } + + #[test] + fn tcp_rejects_requests_before_authentication() { + use std::io::{BufReader, Write as _}; + const TOKEN: &str = "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"; + let h = Arc::new(Mutex::new(Headless::new())); + let (tx, rx) = std::sync::mpsc::channel(); + std::thread::spawn(move || { + let _ = serve_tcp("127.0.0.1:0", h, TOKEN.into(), move |a| tx.send(a).unwrap()); + }); + let addr = rx.recv().unwrap(); + let mut s = std::net::TcpStream::connect(addr).unwrap(); + writeln!(s, r#"{{"id":1,"method":"methods"}}"#).unwrap(); + let mut line = String::new(); + BufReader::new(s).read_line(&mut line).unwrap(); + let v: Value = serde_json::from_str(&line).unwrap(); + assert_eq!(v["ok"], false); + assert_eq!(v["error"], "authentication required"); + assert!(v.get("result").is_none()); + } } diff --git a/crates/automation/src/security.rs b/crates/automation/src/security.rs new file mode 100644 index 00000000..463f012a --- /dev/null +++ b/crates/automation/src/security.rs @@ -0,0 +1,272 @@ +//! Shared security primitives for the loopback JSON control transports. + +use std::fs::OpenOptions; +use std::io::{BufRead, Write}; +use std::path::{Path, PathBuf}; +use std::sync::Arc; +use std::sync::atomic::{AtomicUsize, Ordering}; +use std::time::Duration; + +use serde_json::{Value, json}; + +use crate::AutomationError; + +/// The first request on every TCP connection must use this method. +pub const AUTH_METHOD: &str = "auth"; +/// Maximum encoded JSON request line, including its newline. +pub const MAX_REQUEST_BYTES: usize = 1 << 20; +/// Maximum simultaneously serviced TCP connections per listener. +pub const MAX_CONNECTIONS: usize = 16; +/// Maximum commands or method calls in one batch. +pub const MAX_BATCH_STEPS: usize = 256; +/// Idle/read and write timeout for loopback TCP connections. +pub const IO_TIMEOUT: Duration = Duration::from_secs(30); + +const TOKEN_BYTES: usize = 32; +const TOKEN_HEX_LEN: usize = TOKEN_BYTES * 2; + +/// Result of reading one bounded JSON-lines frame. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum LineRead { + Eof, + Line, + TooLong, +} + +/// Read one line without ever buffering more than [`MAX_REQUEST_BYTES`] plus one byte. +pub fn read_bounded_line(reader: &mut impl BufRead, line: &mut String) -> std::io::Result { + line.clear(); + let mut limited = std::io::Read::take(reader, (MAX_REQUEST_BYTES + 1) as u64); + let n = limited.read_line(line)?; + if n == 0 { + Ok(LineRead::Eof) + } else if n > MAX_REQUEST_BYTES { + Ok(LineRead::TooLong) + } else { + Ok(LineRead::Line) + } +} + +/// Generate a 256-bit bearer token with the operating system CSPRNG. +pub fn generate_token() -> Result { + let mut bytes = [0u8; TOKEN_BYTES]; + getrandom::fill(&mut bytes).map_err(|e| AutomationError::Other(format!("cannot generate control token: {e}")))?; + let mut token = String::with_capacity(TOKEN_HEX_LEN); + const HEX: &[u8; 16] = b"0123456789abcdef"; + for byte in bytes { + token.push(HEX[(byte >> 4) as usize] as char); + token.push(HEX[(byte & 0x0f) as usize] as char); + } + Ok(token) +} + +/// Accept only the fixed-width hexadecimal representation emitted by [`generate_token`]. +pub fn validate_token(token: &str) -> Result<(), AutomationError> { + if token.len() != TOKEN_HEX_LEN || !token.bytes().all(|b| b.is_ascii_hexdigit()) { + return Err(AutomationError::BadRequest("control token must contain exactly 64 hexadecimal characters".into())); + } + Ok(()) +} + +/// Compare fixed-width tokens without an early exit on a mismatching byte. +pub fn token_matches(expected: &str, supplied: &str) -> bool { + if expected.len() != TOKEN_HEX_LEN || supplied.len() != TOKEN_HEX_LEN { + return false; + } + expected.bytes().zip(supplied.bytes()).fold(0u8, |different, (a, b)| different | (a ^ b)) == 0 +} + +/// Validate the first TCP frame without exposing any control method before authentication. +pub fn authentication_reply(line: &str, expected_token: &str) -> (Value, bool) { + let req: Value = match serde_json::from_str(line) { + Ok(v) => v, + Err(_) => { + return (json!({"id": null, "ok": false, "error": "authentication required"}), false); + } + }; + let id = req.get("id").cloned().unwrap_or(Value::Null); + let supplied = req.get("params").and_then(|p| p.get("token")).and_then(Value::as_str).unwrap_or(""); + let ok = req.get("method").and_then(Value::as_str) == Some(AUTH_METHOD) && token_matches(expected_token, supplied); + if ok { + (json!({"id": id, "ok": true, "result": {"authenticated": true}}), true) + } else { + (json!({"id": id, "ok": false, "error": "authentication required"}), false) + } +} + +fn read_token_file(path: &Path) -> Result { + let token = std::fs::read_to_string(path).map_err(|e| AutomationError::Io(format!("{}: {e}", path.display())))?; + let token = token.trim().to_owned(); + validate_token(&token)?; + Ok(token) +} + +fn create_token_file(path: &Path, token: &str) -> Result<(), AutomationError> { + if let Some(parent) = path.parent().filter(|p| !p.as_os_str().is_empty()) { + std::fs::create_dir_all(parent).map_err(|e| AutomationError::Io(format!("{}: {e}", parent.display())))?; + } + let mut options = OpenOptions::new(); + options.write(true).create_new(true); + #[cfg(unix)] + { + use std::os::unix::fs::OpenOptionsExt; + options.mode(0o600); + } + let mut file = options.open(path).map_err(|e| AutomationError::Io(format!("{}: {e}", path.display())))?; + writeln!(file, "{token}").map_err(|e| AutomationError::Io(format!("{}: {e}", path.display()))) +} + +/// Resolve a server token. With no supplied token or file, a fresh token is returned. +/// A missing token file is created atomically; an existing one is read and validated. +pub fn server_token(supplied: Option<&str>, token_file: Option<&Path>) -> Result { + if supplied.is_some() && token_file.is_some() { + return Err(AutomationError::BadRequest("use either a control token or a control token file, not both".into())); + } + if let Some(token) = supplied { + validate_token(token)?; + return Ok(token.to_ascii_lowercase()); + } + let token = generate_token()?; + let Some(path) = token_file else { + return Ok(token); + }; + match read_token_file(path) { + Ok(existing) => Ok(existing.to_ascii_lowercase()), + Err(AutomationError::Io(_)) if !path.exists() => { + match create_token_file(path, &token) { + Ok(()) => Ok(token), + Err(AutomationError::Io(_)) if path.exists() => { + // Another process won the create-new race. + read_token_file(path).map(|v| v.to_ascii_lowercase()) + } + Err(e) => Err(e), + } + } + Err(e) => Err(e), + } +} + +/// Resolve the token used by a client. Clients never silently generate credentials. +pub fn client_token(supplied: Option<&str>, token_file: Option<&Path>) -> Result { + if supplied.is_some() && token_file.is_some() { + return Err(AutomationError::BadRequest("use either a control token or a control token file, not both".into())); + } + if let Some(token) = supplied { + validate_token(token)?; + return Ok(token.to_ascii_lowercase()); + } + token_file.map_or_else( + || { + Err(AutomationError::BadRequest( + "bridge mode needs --control-token, --control-token-file, PHOTOCRAFT_CONTROL_TOKEN, or PHOTOCRAFT_CONTROL_TOKEN_FILE".into(), + )) + }, + |path| read_token_file(path).map(|v| v.to_ascii_lowercase()), + ) +} + +/// Counts active connections and returns a permit only while below the configured maximum. +pub struct ConnectionLimiter { + active: AtomicUsize, + max: usize, +} + +impl ConnectionLimiter { + pub fn new(max: usize) -> Arc { + Arc::new(Self { active: AtomicUsize::new(0), max }) + } + + pub fn try_acquire(self: &Arc) -> Option { + let mut current = self.active.load(Ordering::Acquire); + loop { + if current >= self.max { + return None; + } + match self.active.compare_exchange_weak(current, current + 1, Ordering::AcqRel, Ordering::Acquire) { + Ok(_) => return Some(ConnectionPermit { limiter: Arc::clone(self) }), + Err(actual) => current = actual, + } + } + } +} + +pub struct ConnectionPermit { + limiter: Arc, +} + +impl Drop for ConnectionPermit { + fn drop(&mut self) { + self.limiter.active.fetch_sub(1, Ordering::AcqRel); + } +} + +/// Apply idle and write timeouts before handing a socket to a connection worker. +pub fn configure_stream(stream: &std::net::TcpStream) -> std::io::Result<()> { + stream.set_read_timeout(Some(IO_TIMEOUT))?; + stream.set_write_timeout(Some(IO_TIMEOUT)) +} + +/// Environment-aware token inputs shared by the desktop app and CLI. +pub fn token_inputs(supplied: Option, token_file: Option) -> (Option, Option) { + let supplied = supplied.or_else(|| std::env::var("PHOTOCRAFT_CONTROL_TOKEN").ok()); + let token_file = token_file.or_else(|| std::env::var_os("PHOTOCRAFT_CONTROL_TOKEN_FILE").map(PathBuf::from)); + (supplied, token_file) +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn generated_tokens_are_valid_and_distinct() { + let a = generate_token().unwrap(); + let b = generate_token().unwrap(); + validate_token(&a).unwrap(); + assert_ne!(a, b); + assert!(token_matches(&a, &a)); + assert!(!token_matches(&a, &b)); + assert!(!token_matches(&a, "short")); + } + + #[test] + fn bounded_reader_rejects_an_oversized_line() { + let input = format!("{}\n", "x".repeat(MAX_REQUEST_BYTES + 1)); + let mut reader = std::io::Cursor::new(input); + let mut line = String::new(); + assert_eq!(read_bounded_line(&mut reader, &mut line).unwrap(), LineRead::TooLong); + assert_eq!(line.len(), MAX_REQUEST_BYTES + 1); + } + + #[test] + fn authentication_does_not_dispatch_without_the_token() { + let token = generate_token().unwrap(); + let (reply, authenticated) = authentication_reply(r#"{"id":1,"method":"methods","params":{}}"#, &token); + assert!(!authenticated); + assert_eq!(reply["error"], "authentication required"); + assert!(reply.get("result").is_none()); + + let line = json!({"id": 2, "method": AUTH_METHOD, "params": {"token": token}}).to_string(); + let (reply, authenticated) = authentication_reply(&line, &token); + assert!(authenticated); + assert_eq!(reply["result"]["authenticated"], true); + } + + #[test] + fn token_file_round_trips_between_server_and_client() { + let path = std::env::temp_dir().join(format!("photocraft-control-token-{}-{}.txt", std::process::id(), generate_token().unwrap())); + let server = server_token(None, Some(&path)).unwrap(); + let client = client_token(None, Some(&path)).unwrap(); + assert_eq!(server, client); + assert!(token_matches(&server, &client)); + std::fs::remove_file(path).unwrap(); + } + + #[test] + fn connection_limiter_releases_capacity() { + let limiter = ConnectionLimiter::new(1); + let permit = limiter.try_acquire().unwrap(); + assert!(limiter.try_acquire().is_none()); + drop(permit); + assert!(limiter.try_acquire().is_some()); + } +} diff --git a/crates/automation/src/server.rs b/crates/automation/src/server.rs index 5b0a633c..ea9efca1 100644 --- a/crates/automation/src/server.rs +++ b/crates/automation/src/server.rs @@ -15,6 +15,7 @@ use serde_json::{Value, json}; use crate::bridge::BridgeClient; use crate::headless::Headless; +use crate::security::MAX_BATCH_STEPS; use crate::{AutomationError, files}; /// Where tools are executed. @@ -185,8 +186,9 @@ fn to_result(r: Result) -> Result Result { Ok(fail(format!( - "`{name}` drives the live GUI and needs bridge mode: run `photocraft-cli mcp --bridge 127.0.0.1:` \ - with the app started as `photocraft --control `" + "`{name}` drives the live GUI and needs bridge mode: start the app with \ + `photocraft --control --control-token-file `, then run \ + `photocraft-cli mcp --bridge 127.0.0.1: --control-token-file `" ))) } @@ -195,8 +197,8 @@ impl PhotocraftMcp { Self::with_backend(Backend::Headless(Arc::new(Mutex::new(Headless::new())))) } - pub fn bridge(addr: &str) -> Result { - Ok(Self::with_backend(Backend::Bridge(Arc::new(BridgeClient::new(addr)?)))) + pub fn bridge(addr: &str, token: &str) -> Result { + Ok(Self::with_backend(Backend::Bridge(Arc::new(BridgeClient::new(addr, token)?)))) } pub fn with_backend(backend: Backend) -> Self { @@ -427,6 +429,9 @@ impl PhotocraftMcp { #[tool(description = "Run several engine commands in one call (fewer round trips). Returns {completed, failed, \ results:[{ok, result|error}]}; stops at the first error unless stop_on_error is false.")] async fn command_batch(&self, Parameters(p): Parameters) -> Result { + if p.steps.len() > MAX_BATCH_STEPS { + return Ok(fail(format!("batch contains {} steps; maximum is {MAX_BATCH_STEPS}", p.steps.len()))); + } let stop = p.stop_on_error.unwrap_or(true); let steps: Vec = p.steps.into_iter().map(|s| json!({"command": s.id, "params": s.params.unwrap_or_else(|| json!({}))})).collect(); let args = json!({"steps": steps, "stopOnError": stop}); diff --git a/crates/automation/tests/mcp.rs b/crates/automation/tests/mcp.rs index 24ed25be..d2802189 100644 --- a/crates/automation/tests/mcp.rs +++ b/crates/automation/tests/mcp.rs @@ -10,6 +10,8 @@ use rmcp::{ClientHandler, RoleClient, ServiceExt}; use serde_json::{Value, json}; use tokio::io::{AsyncBufReadExt, AsyncWriteExt, BufReader}; +const CONTROL_TOKEN: &str = "0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef"; + #[derive(Clone, Default)] struct Client; impl ClientHandler for Client { @@ -196,6 +198,7 @@ async fn fake_app() -> (String, tokio::task::JoinHandle>) { let addr = listener.local_addr().unwrap().to_string(); let h = tokio::spawn(async move { let mut seen = Vec::new(); + let mut authenticated = false; let (sock, _) = listener.accept().await.unwrap(); let (r, mut w) = sock.into_split(); let mut lines = BufReader::new(r).lines(); @@ -204,6 +207,13 @@ async fn fake_app() -> (String, tokio::task::JoinHandle>) { let id = req["id"].clone(); let method = req["method"].as_str().unwrap_or("").to_owned(); let reply = match method.as_str() { + "auth" if req["params"]["token"] == CONTROL_TOKEN => { + authenticated = true; + json!({"id": id, "ok": true, "result": {"authenticated": true}}) + } + _ if !authenticated => { + json!({"id": id, "ok": false, "error": "authentication required"}) + } "ui.inspect" => { json!({"id": id, "ok": true, "result": {"tool": "brush", "panels": ["layers"]}}) } @@ -234,7 +244,7 @@ async fn fake_app() -> (String, tokio::task::JoinHandle>) { #[tokio::test(flavor = "multi_thread")] async fn bridge_forwards_to_control_protocol() { let (addr, app) = fake_app().await; - let client = connect(PhotocraftMcp::bridge(&addr).unwrap()).await; + let client = connect(PhotocraftMcp::bridge(&addr, CONTROL_TOKEN).unwrap()).await; let ui = json_of(&call(&client, "ui_inspect", json!({})).await); assert_eq!(ui["tool"], "brush"); @@ -258,14 +268,27 @@ async fn bridge_forwards_to_control_protocol() { #[test] fn bridge_rejects_non_loopback() { - assert!(PhotocraftMcp::bridge("10.0.0.5:7878").is_err()); - assert!(PhotocraftMcp::bridge("127.0.0.1:7878").is_ok()); - assert!(PhotocraftMcp::bridge("localhost:1").is_ok()); + assert!(PhotocraftMcp::bridge("10.0.0.5:7878", CONTROL_TOKEN).is_err()); + assert!(PhotocraftMcp::bridge("127.0.0.1:7878", CONTROL_TOKEN).is_ok()); + assert!(PhotocraftMcp::bridge("localhost:1", CONTROL_TOKEN).is_ok()); + assert!(PhotocraftMcp::bridge("localhost:1", "short").is_err()); +} + +#[tokio::test(flavor = "multi_thread")] +async fn bridge_rejects_wrong_token_before_control_methods() { + let (addr, app) = fake_app().await; + let wrong = "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"; + let client = connect(PhotocraftMcp::bridge(&addr, wrong).unwrap()).await; + let r = call(&client, "ui_inspect", json!({})).await; + assert_eq!(r.is_error, Some(true)); + assert!(text(&r).contains("authentication required"), "{}", text(&r)); + client.cancel().await.unwrap(); + app.abort(); } #[tokio::test(flavor = "multi_thread")] async fn bridge_reports_unreachable_app() { - let client = connect(PhotocraftMcp::bridge("127.0.0.1:1").unwrap()).await; + let client = connect(PhotocraftMcp::bridge("127.0.0.1:1", CONTROL_TOKEN).unwrap()).await; let r = call(&client, "ui_inspect", json!({})).await; assert_eq!(r.is_error, Some(true)); assert!(text(&r).contains("--control"), "{}", text(&r)); @@ -295,3 +318,13 @@ async fn command_batch_runs_steps_in_order() { assert!(doc.contains("\"Two\"") && !doc.contains("\"Three\"")); client.cancel().await.unwrap(); } + +#[tokio::test(flavor = "multi_thread")] +async fn command_batch_rejects_too_many_steps() { + let client = connect(PhotocraftMcp::headless()).await; + let steps: Vec = (0..=photocraft_automation::security::MAX_BATCH_STEPS).map(|_| json!({"id": "command.list"})).collect(); + let r = call(&client, "command_batch", json!({"steps": steps})).await; + assert_eq!(r.is_error, Some(true)); + assert!(text(&r).contains("maximum is 256"), "{}", text(&r)); + client.cancel().await.unwrap(); +} diff --git a/docs/control-protocol.md b/docs/control-protocol.md index 472e3ed7..2e66d7cc 100644 --- a/docs/control-protocol.md +++ b/docs/control-protocol.md @@ -1,6 +1,20 @@ # Control protocol -The desktop app (`photocraft --control `) listens on `127.0.0.1:` (loopback only). Each request is one JSON line: +The desktop app listens on `127.0.0.1:` (loopback only). Start it with a token file so the credential is not exposed in the process command line: + +```sh +photocraft --control 7878 --control-token-file /private/path/photocraft-control.token +``` + +If the file does not exist, PhotoCraft creates it with a fresh 256-bit token. On Unix the new file is mode `0600`; on Windows, protect it with an appropriate user-only ACL. An existing file is reused. If neither a token nor token file is configured, PhotoCraft generates a token for that launch and writes it to standard error. `PHOTOCRAFT_CONTROL_TOKEN` and `PHOTOCRAFT_CONTROL_TOKEN_FILE` are the environment-variable equivalents. + +The first request on every TCP connection must authenticate. No other method is dispatched before this succeeds: + +```json +{"id": "auth", "method": "auth", "params": {"token": "<64 hexadecimal characters>"}} +``` + +After the successful authentication reply, each request is one JSON line: ```json {"id": 1, "method": "ui.inspect", "params": {}} @@ -100,9 +114,14 @@ magenta alignment lines. `ui.pointer` drives the same code, so agents get identi `photocraft-automation` provides an MCP server built on the official Rust SDK (`rmcp`). It runs in one of two modes: - **Headless** (`photocraft-cli mcp`): an in-process `photocraft_engine::Session`. There is no window. -- **Bridge** (`photocraft-cli mcp --bridge 127.0.0.1:7878`): every tool is forwarded to a running `photocraft --control 7878` over this protocol, so agents see and drive the live app. +- **Bridge** (`photocraft-cli mcp --bridge 127.0.0.1:7878 --control-token-file `): every tool is forwarded to a running `photocraft --control 7878 --control-token-file ` over this protocol, so agents see and drive the live app. -The bridge keeps one TCP connection open. It reconnects once if a request fails, and it skips reply lines whose `id` doesn't match the request (for example, stale replies to requests that timed out). It only accepts loopback addresses, because the app only listens on loopback. +The bridge keeps one authenticated TCP connection open. It reconnects and authenticates once if a request fails, and it skips reply lines whose `id` doesn't match the request (for example, stale replies to requests that timed out). It only accepts loopback addresses, because the app only listens on loopback. Supply its bearer token with `--control-token-file`, `--control-token`, `PHOTOCRAFT_CONTROL_TOKEN_FILE`, or `PHOTOCRAFT_CONTROL_TOKEN`: + +```sh +photocraft-cli mcp --bridge 127.0.0.1:7878 \ + --control-token-file /private/path/photocraft-control.token +``` How each MCP tool maps onto control methods in bridge mode: @@ -124,13 +143,15 @@ How each MCP tool maps onto control methods in bridge mode: `doc_select` and `doc_close` work only in headless mode. The `ui_*` tools and `control_call` work only in bridge mode; in headless mode they return a tool error that explains how to start bridge mode. -**Security note:** the control port has no authentication. Any local process can drive the app. Only enable `--control` when you need it. A token handshake is planned (architecture §12). +**Security note:** TCP control uses a bearer token, not client identity or per-method authorization. A client that possesses the token receives the full exposed control surface, including file operations, UI input, command execution, and application control. Keep token files private, do not commit or log tokens, and do not pass a token directly on a shared system where process command lines are visible. The protocol is unencrypted and must remain on loopback; do not tunnel or proxy it to an untrusted host. Capability scopes and filesystem roots are not yet implemented. ## Headless server `photocraft-cli serve` keeps one headless engine session (no window, no GPU) and answers the same JSON-lines envelope on stdio, or on `127.0.0.1:` with `--port ` (loopback only; each -connection shares the session). It is the fastest way for a script or agent to make many edits: +authenticated connection shares the session). TCP uses the same first-frame `auth` exchange and +token options as desktop control. Stdio does not require this TCP handshake because access is +inherited from the process pipe. It is the fastest way for a script or agent to make many edits: no MCP framing, no app start-up per command. Implementation: `crates/automation/src/rpc.rs`. | Method | Params | @@ -155,3 +176,14 @@ printf '%s\n' \ The MCP server has the same batching as the `command_batch` tool (`{steps:[{id, params}], stop_on_error}`), in headless and bridge mode. + +## Transport limits + +The desktop and headless TCP listeners currently enforce: + +- a 1 MiB maximum encoded request line; +- at most 16 simultaneously serviced connections per listener; +- a 30-second socket read/write timeout; +- at most 256 steps in a headless `batch` or MCP `command_batch` request. + +An oversized line, excess connection, or unauthenticated request is rejected before command dispatch. These limits do not impose JSON-depth, response-size, render, document-memory, command-duration, filesystem-root, or capability budgets. diff --git a/docs/development.md b/docs/development.md index 72769dd7..54179fba 100644 --- a/docs/development.md +++ b/docs/development.md @@ -9,7 +9,7 @@ ```sh cargo run --release -p photocraft -- path/to/image.psd # desktop app -cargo run --release -p photocraft -- --control 7878 img.jpg # with the JSON control server +cargo run --release -p photocraft -- --control 7878 --control-token-file .private/control.token img.jpg cargo test --workspace # everything cargo xtask ci # fmt + clippy + tests + layers + wasm cargo xtask stats # tests and lines per crate @@ -23,6 +23,8 @@ Image code is slow at `opt-level 0`, so the workspace profile builds dependencie | Variable | Effect | |---|---| | `PHOTOCRAFT_CONTROL_PORT` | Same as `--control ` | +| `PHOTOCRAFT_CONTROL_TOKEN` | 64-hex bearer token for control TCP (avoid on shared systems where environment inspection is possible) | +| `PHOTOCRAFT_CONTROL_TOKEN_FILE` | Read, or create for a server, the control bearer-token file | | `PHOTOCRAFT_CPU_CANVAS=1` | Force the CPU canvas path instead of the wgpu shader canvas | | `PHOTOCRAFT_GPU_TILE=2048` | Force GPU canvas tiling (tests tile seams) | | `PHOTOCRAFT_FX_NOCACHE=1` | Bypass the CPU layer-effect map cache (`compose::effect_maps`) | @@ -40,10 +42,12 @@ Keys are the field names of `theme::Tokens` (`crates/ui-egui/src/theme.rs`). Edi ## Driving the app programmatically -Start the app with `--control 7878`. It then accepts JSON lines on `127.0.0.1:7878`: +Start the app with a private token file. It then accepts authenticated JSON lines on `127.0.0.1:7878`: ```sh -printf '%s\n' '{"id":1,"method":"engine.execute","params":{"command":"layer.newAdjustmentLayer.hueSaturation","params":{"hue":30}}}' \ +TOKEN=$(tr -d '\r\n' < .private/control.token) +printf '%s\n' "{\"id\":\"auth\",\"method\":\"auth\",\"params\":{\"token\":\"$TOKEN\"}}" \ + '{"id":1,"method":"engine.execute","params":{"command":"layer.newAdjustmentLayer.hueSaturation","params":{"hue":30}}}' \ '{"id":2,"method":"ui.screenshot","params":{"path":"/tmp/shot.png"}}' | nc 127.0.0.1 7878 ``` @@ -86,7 +90,7 @@ Keep one `PcraftWriter` per open document: re-saving then only compresses and wr `photocraft-cli mcp` serves MCP on stdio using `crates/automation`, which is built on `rmcp`: - **Headless:** `photocraft-cli mcp`. It drives an in-process engine session. -- **Live app:** start `photocraft --control 7878`, then run `photocraft-cli mcp --bridge 127.0.0.1:7878`. See `docs/control-protocol.md#mcp-bridge`. +- **Live app:** start `photocraft --control 7878 --control-token-file `, then run `photocraft-cli mcp --bridge 127.0.0.1:7878 --control-token-file `. See `docs/control-protocol.md#mcp-bridge`. Tools: @@ -106,7 +110,7 @@ Claude Code (`.mcp.json` in the repo root, or `claude mcp add`): }, "photocraft-live": { "command": "/path/to/photocraft/target/release/photocraft-cli", - "args": ["mcp", "--bridge", "127.0.0.1:7878"] + "args": ["mcp", "--bridge", "127.0.0.1:7878", "--control-token-file", "/private/path/photocraft-control.token"] } } }