diff --git a/CHANGELOG.md b/CHANGELOG.md index af4e585..8e85fb0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -55,6 +55,14 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - **`mcpls_core::mcp::{HoverParams, DefinitionParams, CallHierarchyPrepareParams}`** — Breaking change: removed along with the other three position-only wrapper structs described above (`SignatureHelpParams`, `GoToImplementationParams`, `GoToTypeDefinitionParams` were never re-exported from `mcp::mod`). Use `mcpls_core::mcp::PositionParams` directly. No deprecation shim, per pre-1.0 policy. (#302) +### Security + +- **Explicit size caps added on config-file reads, cached LSP notification data, MCP tool string params, and LSP error messages forwarded to callers** — several inputs were previously bounded only by an outer transport/protocol limit (or not bounded at all), each a defense-in-depth gap found during a security audit: + - `ServerConfig::load_from` now reads the config file through a bounded `Read::take(MAX_CONFIG_FILE_BYTES + 1)` and rejects it with `Error::FileSizeLimitExceeded` (8 MiB cap) if that limit is exceeded, instead of calling `std::fs::read_to_string` with no upper bound. A bounded read, not a `std::fs::metadata` size pre-check, is required: `metadata().len()` reports `0` for character devices, FIFOs, and many procfs entries regardless of how much data they can actually produce (e.g. `/dev/zero`), so a pre-check alone can be bypassed by a path pointing at one. (#309) + - `rename_symbol`'s `new_name` MCP parameter is now capped at 1000 bytes and `get_completions`'s `trigger` at 8 bytes (`Error::InvalidToolParams` on overflow), matching the existing cap already in place on `workspace_symbol_search`'s `query`. (#309) + - `NotificationCache::store_log`/`store_message` now truncate each cached message to 256 KiB via a new shared `truncate_string` helper (`crate::util`), and `store_diagnostics` truncates each diagnostic's `message` field the same way, then additionally bounds the *whole* diagnostics list to 1 MiB of serialized JSON via `cap_diagnostics_entry_size` — a per-message cap alone does not bound a `Vec`'s length or its several other free-form/arbitrary-JSON fields (`source`, `code`, `code_description`, `related_information`, `data`, `tags`), so a hostile server could still publish e.g. 100k small diagnostics, or one diagnostic with a multi-MiB `data` blob or `source` string, without ever exceeding the per-message cap. `cap_diagnostics_entry_size` guarantees this bound with a final, unconditional check rather than assuming its field-specific mitigations (severity-preferential truncation to the largest fitting prefix via binary search for many diagnostics — not a lossy, severity-blind flat halve; dropping opaque fields and truncating `source`/`code` for one still-oversized diagnostic) cover every case, logs a `tracing::warn!` whenever it drops a diagnostic or a `data`/`code_description`/`related_information` field (the latter can silently break a later `textDocument/codeAction` request's quick fix, per the LSP spec's `data` round-trip contract) so the degradation is visible rather than silent, and skips the full JSON-serialization pass it would otherwise need on every `publishDiagnostics` (a hot path) via a conservative cheap size estimate whenever no diagnostic carries `data`/`code_description`/`related_information`/`tags` — that estimate multiplies each string field's raw byte length by a worst-case JSON-escaping factor of 6 (a control character like NUL costs 6 bytes as `\u00XX` once encoded) rather than summing raw lengths directly, since the latter could undercount an escape-heavy message enough to let an oversized entry skip the real check entirely. The existing `MAX_LOG_ENTRIES`/`MAX_SERVER_MESSAGES`/`MAX_DIAGNOSTIC_ENTRIES` caps bound entry *count* only, not the byte size of any single entry. (#311) + - `LspClient`'s handling of a JSON-RPC error response from a spawned LSP server now truncates the message forwarded to the MCP caller in `Error::LspServerError` to 4 KiB, instead of sending the full unbounded `error.message` — previously only the separate, much shorter (200-byte) log-line truncation existed, and the caller-facing message was unbounded. (#313) + ### Fixed - **`run_stdio` registered its `SIGTERM`/`SIGINT` handler too late to catch a signal sent early in startup** — `serve_with` previously ran config validation, workspace-root heuristics, and `spawn_lsp_servers_background` (which spawns LSP child processes concurrently) before `run_stdio` ever registered a signal handler, and `run_stdio` itself only did so *after* `mcp_server.serve(..)`'s MCP `initialize` handshake resolved — `rmcp`'s `serve(..)` awaits the client's first message internally, so a signal arriving at any point up to and including that wait fell through to the OS's default disposition (immediate termination, skipping `Translator::shutdown_servers` and risking an orphaned LSP child process mid-spawn; the exact failure mode #270 was filed to prevent). New `ShutdownSignal` type is now constructed once, as the first statement in `serve_with`, before any of that startup work, and moved by value into whichever transport runs; `run_stdio` races it against the handshake itself, then reuses the same instance in its existing post-handshake `select!`. Also fixed a related gap introduced while designing the reused handle: `SIGINT` was previously re-registered via `tokio::signal::ctrl_c()` on every wait (a fresh one-shot listener each time), which silently lost a signal delivered while a different `select!` branch was being polled — `ShutdownSignal` now holds a persistent listener per signal kind (`SIGTERM` and `SIGINT` on Unix via `tokio::signal::unix::signal`, `Ctrl-C` on Windows via the persistent `tokio::signal::windows::ctrl_c()` stream) for its entire lifetime instead. (#318) diff --git a/crates/mcpls-core/src/bridge/notifications.rs b/crates/mcpls-core/src/bridge/notifications.rs index 630e67a..1f740cc 100644 --- a/crates/mcpls-core/src/bridge/notifications.rs +++ b/crates/mcpls-core/src/bridge/notifications.rs @@ -7,12 +7,52 @@ use std::collections::{BTreeMap, HashMap, VecDeque}; use chrono::{DateTime, Utc}; use lsp_types::{Diagnostic as LspDiagnostic, Uri}; use serde::{Deserialize, Serialize}; +use tracing::warn; use crate::config::ServerId; +use crate::util::{truncate_str, truncate_string}; /// Maximum number of log entries to store. const MAX_LOG_ENTRIES: usize = 100; +/// Maximum size, in bytes, of a single cached log message, server message, +/// or a single diagnostic's free-form `message` text. +/// +/// `MAX_LOG_ENTRIES`/`MAX_SERVER_MESSAGES`/`MAX_DIAGNOSTIC_ENTRIES` bound +/// the *number* of cached entries, but not the size of any one entry -- a +/// spawned LSP server could publish a single pathologically large message +/// and still fit under those caps while consuming unbounded memory (#311). +/// This is independent of the transport-level `MAX_CONTENT_LENGTH` cap in +/// `lsp::transport`, which bounds a whole JSON-RPC frame, not one field +/// within it. 256 KiB comfortably fits any realistic diagnostic or log +/// message while still capping the worst case. +/// +/// This alone does not bound a whole diagnostics *entry* (a +/// `Vec`), only one diagnostic's `message` field -- see +/// `MAX_DIAGNOSTICS_ENTRY_BYTES` for the entry-level cap. +const MAX_ENTRY_TEXT_BYTES: usize = 256 * 1024; + +/// Maximum serialized size, in bytes, of a single document's *whole* +/// diagnostics list (`Vec`), enforced by +/// [`cap_diagnostics_entry_size`]. +/// +/// `MAX_ENTRY_TEXT_BYTES` alone does not bound this: it only truncates one +/// diagnostic's `message` field, but the list's *length* is uncapped, and +/// `LspDiagnostic` carries several more free-form or arbitrary-JSON fields +/// besides `message` (`source`, `code`, `code_description`, +/// `related_information`, `data`). A hostile server can stay under +/// `MAX_ENTRY_TEXT_BYTES` on every individual message while still +/// publishing e.g. 100k diagnostics for one URI, or a single diagnostic +/// with a multi-MiB `data` blob -- both still fit under the transport-level +/// `lsp::transport::MAX_CONTENT_LENGTH` (10 MiB) per notification, and +/// `MAX_DIAGNOSTIC_ENTRIES` bounds only the *number* of distinct cached +/// URIs, not their individual size, so up to 1000 such entries could +/// otherwise accumulate to gigabytes. 1 MiB is far larger than any +/// realistic diagnostics list for one file, and combined with +/// `MAX_DIAGNOSTIC_ENTRIES` bounds the cache's total diagnostics footprint +/// to roughly 1 GiB in the worst case. +const MAX_DIAGNOSTICS_ENTRY_BYTES: usize = 1024 * 1024; + /// Global budget for distinct-URI diagnostic entries, shared work-conservingly /// across every registered diagnostics-route server rather than claimed by /// one server alone. @@ -56,6 +96,250 @@ fn uri_cache_key(uri: &str) -> std::borrow::Cow<'_, str> { /// Maximum number of server messages to store. const MAX_SERVER_MESSAGES: usize = 50; +/// Conservative fixed-field/JSON-structure overhead assumed per diagnostic +/// (`range`, `severity`, and object/field-name punctuation) by +/// [`cap_diagnostics_entry_size`]'s cheap size estimate. Deliberately +/// generous relative to the true overhead (`range` alone serializes to +/// roughly 70 bytes) so the estimate can only ever *overcount*, never +/// undercount, actual serialized size. +const DIAGNOSTIC_ESTIMATE_OVERHEAD_BYTES: usize = 256; + +/// Worst-case JSON string-escaping expansion factor, applied to each raw +/// string field's byte length in [`cap_diagnostics_entry_size`]'s cheap +/// size estimate. +/// +/// A raw byte's serialized JSON form is at most 6 bytes: `"` and `\` and +/// the five control characters with a short escape (`\b \f \n \r \t`) cost +/// 2 bytes, but every other control character (`U+0000`..=`U+001F`, e.g. +/// NUL) has no short escape and is emitted as `\u00XX` -- 6 bytes for 1 raw +/// byte. The original estimate summed raw string lengths directly and +/// could *undercount* an escape-heavy string (e.g. all-NUL) by up to this +/// factor, letting an oversized entry skip the real `fits` check +/// entirely -- multiplying by it keeps the estimate a true upper bound on +/// serialized size rather than merely a typical-case guess. +const JSON_ESCAPE_WORST_CASE_FACTOR: usize = 6; + +/// Last-resort message length used by [`cap_diagnostics_entry_size`]'s +/// terminal-enforcement fallback -- small enough that a single diagnostic +/// (fixed-size `range`/`severity` plus this one short string, every other +/// field cleared) can never approach [`MAX_DIAGNOSTICS_ENTRY_BYTES`] +/// regardless of JSON encoding overhead. +const DIAGNOSTIC_TERMINAL_FALLBACK_MESSAGE_BYTES: usize = 1024; + +/// Ordinal rank used to sort diagnostics by severity before +/// [`cap_diagnostics_entry_size`] truncates an oversized list -- lower rank +/// sorts first, so it is kept preferentially (#311 S6). +/// +/// `DiagnosticSeverity`'s inner value is private, so its natural numeric +/// ordering (`ERROR` < `WARNING` < `INFORMATION` < `HINT`) can't be read +/// directly; `Option`'s *derived* `Ord` would also rank +/// `None` before every `Some` value, the opposite of what's wanted here +/// (no reported severity is treated as least important, same as `HINT`). +/// This maps explicitly instead of relying on either. +const fn diagnostic_severity_rank(diagnostic: &LspDiagnostic) -> u8 { + match diagnostic.severity { + Some(lsp_types::DiagnosticSeverity::ERROR) => 0, + Some(lsp_types::DiagnosticSeverity::WARNING) => 1, + Some(lsp_types::DiagnosticSeverity::INFORMATION) => 2, + // An unrecognized (future) severity value is treated the same as + // no severity at all: least important, not most. + Some(_) | None => 3, + } +} + +/// Largest `k` such that `fits(&diagnostics[..k])`, found via binary search +/// rather than a linear scan or a flat halve (#311 S6). +/// +/// Correct because a JSON array's serialized length is monotonically +/// non-decreasing in its element count -- appending a diagnostic can only +/// add bytes, never remove them -- so `fits(&diagnostics[..k])` is `true` +/// for a contiguous run of small `k` and `false` for every larger `k`, +/// exactly the shape a boundary binary search requires. `fits(&[])` is +/// always `true`, so the search is well-defined even if no diagnostic at +/// all fits individually. +fn largest_fitting_prefix( + diagnostics: &[LspDiagnostic], + fits: impl Fn(&[LspDiagnostic]) -> bool, +) -> usize { + let (mut lo, mut hi) = (0usize, diagnostics.len()); + while lo < hi { + let mid = lo + (hi - lo).div_ceil(2); + if fits(&diagnostics[..mid]) { + lo = mid; + } else { + hi = mid - 1; + } + } + lo +} + +/// Bounds `diagnostics`' serialized size to at most +/// `MAX_DIAGNOSTICS_ENTRY_BYTES` (#311 C1 fix). +/// +/// Measures the list's *actual* serialized size via `serde_json::to_vec` +/// rather than bounding each field individually -- that covers every +/// field on `LspDiagnostic` (`source`, `code`, `code_description`, +/// `related_information`, `data`, `tags`) at once, not just `message`. +/// +/// # Guarantee +/// +/// The postcondition -- the returned list's serialized size is at most +/// `MAX_DIAGNOSTICS_ENTRY_BYTES` -- is enforced directly by a final, +/// unconditional check at the end of this function, not merely assumed to +/// follow from the field-specific mitigations below it. Those mitigations +/// are best-effort (preserve as much real content as fits) and only cover +/// the fields known today; the terminal step is what actually guarantees +/// the bound holds even if a mitigation is incomplete or `LspDiagnostic` +/// gains a new unbounded field in a future `lsp-types` upgrade. +/// +/// # Cost (#311 S5) +/// +/// `publishDiagnostics` is a hot path (rust-analyzer republishes +/// whole-workspace diagnostics on every save), so this avoids a full +/// `serde_json` serialization pass whenever every diagnostic's size is +/// cheaply accountable from `message`/`source`/`code` alone (i.e. none +/// carry `data`, `code_description`, `related_information`, or `tags`, +/// each of which needs real serialization to size safely) and a +/// conservative *upper bound* on their sum already fits. The estimate is +/// not their raw byte length: JSON string escaping can expand a byte up to +/// [`JSON_ESCAPE_WORST_CASE_FACTOR`]-fold (a NUL-heavy string previously +/// let this fast path undercount actual serialized size by that much and +/// skip the real `fits` check below entirely), so raw lengths are +/// multiplied by that factor before comparing against the cap. +/// +/// # Visibility (#311 S7) +/// +/// Every mitigation that drops or truncates real content -- discarding +/// diagnostics entirely, or clearing a survivor's `data` (which the LSP +/// spec says is preserved through to a later `textDocument/codeAction` +/// request, so losing it can silently break that diagnostic's quick fix) +/// -- logs a `tracing::warn!` so the degradation is visible rather than a +/// silent, hard-to-diagnose gap in what a caller sees. +fn cap_diagnostics_entry_size(uri: &Uri, diagnostics: &mut Vec) { + let fits = |ds: &[LspDiagnostic]| { + // A serialization error is conservatively treated as "does not + // fit" (triggers the mitigations below) rather than as success. + // `LspDiagnostic`'s fields can't actually produce one in practice + // (no floats, no non-string map keys anywhere in `Diagnostic` or + // `serde_json::Value`'s own object representation), but failing + // safe costs nothing here. + serde_json::to_vec(ds).is_ok_and(|bytes| bytes.len() <= MAX_DIAGNOSTICS_ENTRY_BYTES) + }; + + let cheaply_estimable = diagnostics.iter().all(|d| { + d.data.is_none() + && d.code_description.is_none() + && d.related_information.is_none() + && d.tags.is_none() + }); + if cheaply_estimable { + let estimated: usize = diagnostics + .iter() + .map(|d| { + let raw_string_bytes = d.message.len() + + d.source.as_deref().map_or(0, str::len) + + match &d.code { + Some(lsp_types::NumberOrString::String(s)) => s.len(), + _ => 0, + }; + raw_string_bytes * JSON_ESCAPE_WORST_CASE_FACTOR + + DIAGNOSTIC_ESTIMATE_OVERHEAD_BYTES + }) + .sum(); + if estimated <= MAX_DIAGNOSTICS_ENTRY_BYTES { + return; + } + } + + if fits(diagnostics) { + return; + } + + let original_count = diagnostics.len(); + + // Prefer dropping lower-severity diagnostics first (a stable sort, so + // same-severity diagnostics keep their original -- typically + // file-position -- relative order), then keep the largest prefix that + // actually fits rather than a flat halve, which both overshoots (a + // list one byte over the cap would otherwise lose half its + // diagnostics) and was severity-blind (would keep hundreds of leading + // HINT-level noise over a later ERROR). At least one diagnostic is + // always kept here so the mitigations below have a survivor to act on. + diagnostics.sort_by_key(diagnostic_severity_rank); + let keep = largest_fitting_prefix(diagnostics, fits).max(1); + diagnostics.truncate(keep); + if diagnostics.len() < original_count { + warn!( + "diagnostics for {} exceeded the {MAX_DIAGNOSTICS_ENTRY_BYTES}-byte cache cap; kept \ + the {} highest-severity of {original_count} diagnostics", + uri.as_str(), + diagnostics.len(), + ); + } + + // Drop opaque/structured fields first -- cheap, and often enough on + // its own (e.g. the single-huge-`data`-blob shape). + if diagnostics.len() == 1 && !fits(diagnostics) { + let diagnostic = &mut diagnostics[0]; + let had_data = diagnostic.data.is_some(); + diagnostic.data = None; + diagnostic.code_description = None; + diagnostic.related_information = None; + diagnostic.tags = None; + warn!( + "diagnostic for {} exceeded the cache cap; dropped its data/code_description/\ + related_information/tags fields{}", + uri.as_str(), + if had_data { + " (a later code-action request for this diagnostic may not resolve its quick fix)" + } else { + "" + }, + ); + } + + // Still oversized: `source`/`code` (plain strings, unlike the opaque + // fields above) are truncated rather than dropped, to preserve some + // content. + if diagnostics.len() == 1 && !fits(diagnostics) { + let diagnostic = &mut diagnostics[0]; + if let Some(source) = &diagnostic.source { + diagnostic.source = Some(truncate_str(source, MAX_ENTRY_TEXT_BYTES)); + } + if let Some(lsp_types::NumberOrString::String(code)) = &diagnostic.code { + diagnostic.code = Some(lsp_types::NumberOrString::String(truncate_str( + code, + MAX_ENTRY_TEXT_BYTES, + ))); + } + } + + // Terminal enforcement: guarantee the postcondition directly rather + // than trusting the mitigations above to have covered every case -- + // see this function's doc. + if !fits(diagnostics) { + diagnostics.truncate(1); + if let Some(diagnostic) = diagnostics.first_mut() { + diagnostic.message = truncate_str( + &diagnostic.message, + DIAGNOSTIC_TERMINAL_FALLBACK_MESSAGE_BYTES, + ); + diagnostic.source = None; + diagnostic.code = None; + diagnostic.code_description = None; + diagnostic.related_information = None; + diagnostic.tags = None; + diagnostic.data = None; + } + warn!( + "diagnostic for {} still exceeded the cache cap after every other mitigation; \ + truncated its message to {DIAGNOSTIC_TERMINAL_FALLBACK_MESSAGE_BYTES} bytes and \ + cleared all other fields", + uri.as_str(), + ); + } +} + /// Information about diagnostics for a document. #[derive(Debug, Clone, Serialize, Deserialize)] pub struct DiagnosticInfo { @@ -280,6 +564,13 @@ impl NotificationCache { /// Store diagnostics for a document published by `server_id`. /// + /// Each diagnostic's `message` is truncated to `MAX_ENTRY_TEXT_BYTES`, + /// and the whole list is bounded to `MAX_DIAGNOSTICS_ENTRY_BYTES` + /// serialized bytes, before storing (#311). When that bound requires + /// dropping diagnostics, the *survivors* come back sorted by severity + /// (`diagnostic_severity_rank`: `ERROR` first), not in the original + /// publish/file-position order -- see [`Self::get_diagnostics`]. + /// /// If diagnostics already exist for the URI, they are replaced and the /// entry is repositioned to the back of its owner's eviction order, so /// a URI republished on every edit is tracked as most-recently-written @@ -317,8 +608,22 @@ impl NotificationCache { server_id: &ServerId, uri: &Uri, version: Option, - diagnostics: Vec, + mut diagnostics: Vec, ) { + // Bound each diagnostic's free-form message text (#311); see + // `MAX_ENTRY_TEXT_BYTES`. `mem::take` + `truncate_string` avoids an + // extra clone on the common (already-under-limit) path, since + // `message` is already an owned `String` here. + for diagnostic in &mut diagnostics { + diagnostic.message = truncate_string( + std::mem::take(&mut diagnostic.message), + MAX_ENTRY_TEXT_BYTES, + ); + } + // Bound the whole list's serialized size (#311 C1); see + // `MAX_DIAGNOSTICS_ENTRY_BYTES`. + cap_diagnostics_entry_size(uri, &mut diagnostics); + let key = uri_cache_key(uri.as_str()).into_owned(); let info = DiagnosticInfo { uri: uri.clone(), @@ -371,10 +676,11 @@ impl NotificationCache { /// Store a log entry. /// /// Maintains a maximum of `MAX_LOG_ENTRIES` entries, removing oldest when full. + /// `message` is truncated to `MAX_ENTRY_TEXT_BYTES` before storing. pub fn store_log(&mut self, level: LogLevel, message: String) { let entry = LogEntry { level, - message, + message: truncate_string(message, MAX_ENTRY_TEXT_BYTES), timestamp: Utc::now(), }; @@ -387,10 +693,11 @@ impl NotificationCache { /// Store a server message. /// /// Maintains a maximum of `MAX_SERVER_MESSAGES` entries, removing oldest when full. + /// `message` is truncated to `MAX_ENTRY_TEXT_BYTES` before storing. pub fn store_message(&mut self, message_type: MessageType, message: String) { let msg = ServerMessage { message_type, - message, + message: truncate_string(message, MAX_ENTRY_TEXT_BYTES), timestamp: Utc::now(), }; @@ -401,6 +708,12 @@ impl NotificationCache { } /// Get diagnostics for a document URI. + /// + /// If the stored list was ever truncated by `store_diagnostics`'s + /// `MAX_DIAGNOSTICS_ENTRY_BYTES` cap (#311), the diagnostics here are in + /// severity order (`ERROR` first), not the original publish/file-position + /// order -- callers that assume file-position order should not rely on + /// it after a cap-triggered truncation. #[inline] #[must_use] pub fn get_diagnostics(&self, uri: &str) -> Option<&DiagnosticInfo> { @@ -579,6 +892,434 @@ mod tests { assert_eq!(stored.diagnostics[0].message, "test error"); } + /// #311: a single diagnostic's `message` must be bounded independently + /// of `MAX_DIAGNOSTIC_ENTRIES`, which only caps the number of entries. + #[test] + fn test_store_diagnostics_truncates_oversized_message() { + let mut cache = NotificationCache::new(); + let uri: Uri = "file:///test.rs".parse().unwrap(); + let oversized = "a".repeat(MAX_ENTRY_TEXT_BYTES + 100); + + let diagnostic = LspDiagnostic { + range: Range { + start: Position { + line: 0, + character: 0, + }, + end: Position { + line: 0, + character: 5, + }, + }, + severity: Some(lsp_types::DiagnosticSeverity::ERROR), + message: oversized.clone(), + code: None, + source: None, + code_description: None, + related_information: None, + tags: None, + data: None, + }; + + cache.store_diagnostics(&test_server(), &uri, Some(1), vec![diagnostic]); + + let stored = &cache.get_diagnostics(uri.as_str()).unwrap().diagnostics[0].message; + assert!(stored.len() < oversized.len()); + assert!(stored.ends_with("... (truncated)")); + } + + /// Minimal diagnostic with an arbitrary `message`, for tests that only + /// care about size/count bounds rather than range/severity details. + fn minimal_diagnostic(message: String) -> LspDiagnostic { + LspDiagnostic { + range: Range { + start: Position { + line: 0, + character: 0, + }, + end: Position { + line: 0, + character: 5, + }, + }, + severity: Some(lsp_types::DiagnosticSeverity::ERROR), + message, + code: None, + source: None, + code_description: None, + related_information: None, + tags: None, + data: None, + } + } + + /// #311 C1: `MAX_ENTRY_TEXT_BYTES` alone bounds one `message` field, not + /// the whole entry -- many diagnostics, each individually small, must + /// still be capped in aggregate. + #[test] + fn test_store_diagnostics_caps_aggregate_size_for_many_small_diagnostics() { + let mut cache = NotificationCache::new(); + let uri: Uri = "file:///test.rs".parse().unwrap(); + + // Each diagnostic is far under MAX_ENTRY_TEXT_BYTES individually, + // but 5000 of them comfortably exceeds MAX_DIAGNOSTICS_ENTRY_BYTES + // in aggregate. + let diagnostics: Vec = (0..5000) + .map(|i| { + minimal_diagnostic(format!( + "diagnostic number {i}, padded: {}", + "x".repeat(200) + )) + }) + .collect(); + let original_count = diagnostics.len(); + + cache.store_diagnostics(&test_server(), &uri, Some(1), diagnostics); + + let stored = &cache.get_diagnostics(uri.as_str()).unwrap().diagnostics; + assert!( + stored.len() < original_count, + "aggregate cap must trim the list, kept {} of {original_count}", + stored.len() + ); + assert!(!stored.is_empty(), "must keep at least one diagnostic"); + let serialized_len = serde_json::to_vec(stored).unwrap().len(); + assert!( + serialized_len <= MAX_DIAGNOSTICS_ENTRY_BYTES, + "stored entry must fit the aggregate cap, got {serialized_len} bytes" + ); + } + + /// #311 S6: a naive flat halve would keep only the first N/2 + /// diagnostics even when far more than that would actually fit -- + /// truncation must find the largest prefix that fits instead. + #[test] + fn test_store_diagnostics_truncation_keeps_largest_fitting_prefix() { + let mut cache = NotificationCache::new(); + let uri: Uri = "file:///test.rs".parse().unwrap(); + + // Each diagnostic serializes to roughly 300 bytes; ~3800 of them + // fit under the 1 MiB cap, well over half of the 5000 published -- + // a flat halve would incorrectly stop at 2500. + let diagnostics: Vec = (0..5000) + .map(|i| minimal_diagnostic(format!("diagnostic {i}: {}", "x".repeat(250)))) + .collect(); + + cache.store_diagnostics(&test_server(), &uri, Some(1), diagnostics); + + let stored = &cache.get_diagnostics(uri.as_str()).unwrap().diagnostics; + assert!( + stored.len() > 2600, + "largest-fitting-prefix search must keep far more than half, kept {}", + stored.len() + ); + let serialized_len = serde_json::to_vec(stored).unwrap().len(); + assert!(serialized_len <= MAX_DIAGNOSTICS_ENTRY_BYTES); + // The search must find the *largest* fitting prefix, not just *a* + // fitting one: one more diagnostic than what was kept must no + // longer fit (otherwise it should have been kept too). + let mut with_one_more = stored.clone(); + with_one_more.push(minimal_diagnostic(format!( + "diagnostic overflow: {}", + "x".repeat(250) + ))); + assert!( + serde_json::to_vec(&with_one_more).unwrap().len() > MAX_DIAGNOSTICS_ENTRY_BYTES, + "kept count must be the largest that fits, not merely a fitting count" + ); + } + + /// #311 S6: truncation must prefer keeping higher-severity diagnostics, + /// not just whichever the server happened to publish first -- a late + /// `ERROR` must survive over leading `HINT`-level noise. + #[test] + fn test_store_diagnostics_truncation_prefers_higher_severity() { + let mut cache = NotificationCache::new(); + let uri: Uri = "file:///test.rs".parse().unwrap(); + + let mut diagnostics: Vec = (0..5000) + .map(|i| { + let mut d = minimal_diagnostic(format!("hint {i}: {}", "x".repeat(200))); + d.severity = Some(lsp_types::DiagnosticSeverity::HINT); + d + }) + .collect(); + let mut trailing_error = minimal_diagnostic("the one real error".to_string()); + trailing_error.severity = Some(lsp_types::DiagnosticSeverity::ERROR); + diagnostics.push(trailing_error); + + cache.store_diagnostics(&test_server(), &uri, Some(1), diagnostics); + + let stored = &cache.get_diagnostics(uri.as_str()).unwrap().diagnostics; + assert!( + stored.iter().any(|d| d.message == "the one real error"), + "the trailing ERROR diagnostic must survive truncation over leading HINT noise" + ); + } + + /// Captures `tracing` events emitted while a closure runs, mirroring + /// `transport::tests::http_tests::CapturedMessages` -- there is no + /// shared `tracing_test`-style helper in this codebase to reuse. + #[derive(Clone, Default)] + struct CapturedMessages(std::sync::Arc>>); + + impl tracing_subscriber::Layer for CapturedMessages { + fn on_event( + &self, + event: &tracing::Event<'_>, + _ctx: tracing_subscriber::layer::Context<'_, S>, + ) { + struct MessageVisitor(String); + impl tracing::field::Visit for MessageVisitor { + fn record_debug( + &mut self, + field: &tracing::field::Field, + value: &dyn std::fmt::Debug, + ) { + if field.name() == "message" { + self.0 = format!("{value:?}"); + } + } + } + let mut visitor = MessageVisitor(String::new()); + event.record(&mut visitor); + self.0.lock().unwrap().push(visitor.0); + } + } + + /// #311 S7 / M7: truncating the diagnostics list must not be silent -- + /// a caller with no visibility into this cache would otherwise have no + /// way to know a `get_cached_diagnostics` result is incomplete. + #[test] + fn test_store_diagnostics_warns_when_truncating_list() { + use tracing_subscriber::layer::SubscriberExt as _; + + let mut cache = NotificationCache::new(); + let uri: Uri = "file:///test.rs".parse().unwrap(); + let diagnostics: Vec = (0..5000) + .map(|i| minimal_diagnostic(format!("diagnostic {i}: {}", "x".repeat(250)))) + .collect(); + + let captured = CapturedMessages::default(); + let subscriber = tracing_subscriber::registry().with(captured.clone()); + let guard = tracing::subscriber::set_default(subscriber); + cache.store_diagnostics(&test_server(), &uri, Some(1), diagnostics); + drop(guard); + + let messages = captured.0.lock().unwrap().clone(); + assert!( + messages + .iter() + .any(|m| m.contains("highest-severity") && m.contains("file:///test.rs")), + "expected a truncation warning naming the URI, got: {messages:?}" + ); + } + + /// #311 S7: dropping a diagnostic's `data` breaks the LSP contract that + /// it round-trips to a later `textDocument/codeAction` request -- this + /// must be logged, not silent. + #[test] + fn test_store_diagnostics_warns_when_dropping_data_blob() { + use tracing_subscriber::layer::SubscriberExt as _; + + let mut cache = NotificationCache::new(); + let uri: Uri = "file:///test.rs".parse().unwrap(); + let mut diagnostic = minimal_diagnostic("small message".to_string()); + diagnostic.data = Some(serde_json::json!({ + "blob": "x".repeat(MAX_DIAGNOSTICS_ENTRY_BYTES + 1000), + })); + + let captured = CapturedMessages::default(); + let subscriber = tracing_subscriber::registry().with(captured.clone()); + let guard = tracing::subscriber::set_default(subscriber); + cache.store_diagnostics(&test_server(), &uri, Some(1), vec![diagnostic]); + drop(guard); + + let messages = captured.0.lock().unwrap().clone(); + assert!( + messages.iter().any(|m| m.contains("code-action")), + "expected a warning noting the code-action quick-fix impact, got: {messages:?}" + ); + } + + /// #311 C1: a single diagnostic dominated by an oversized `data` blob + /// must be capped even though `message` alone is small -- the aggregate + /// list-halving path can't shrink a one-element list, so the opaque + /// fields on that single diagnostic must be dropped instead. + #[test] + fn test_store_diagnostics_drops_oversized_data_blob_on_single_diagnostic() { + let mut cache = NotificationCache::new(); + let uri: Uri = "file:///test.rs".parse().unwrap(); + + let mut diagnostic = minimal_diagnostic("small message".to_string()); + diagnostic.data = Some(serde_json::json!({ + "blob": "x".repeat(MAX_DIAGNOSTICS_ENTRY_BYTES + 1000), + })); + + cache.store_diagnostics(&test_server(), &uri, Some(1), vec![diagnostic]); + + let stored = &cache.get_diagnostics(uri.as_str()).unwrap().diagnostics; + assert_eq!(stored.len(), 1); + assert_eq!(stored[0].message, "small message"); + assert!( + stored[0].data.is_none(), + "oversized data blob must be dropped" + ); + let serialized_len = serde_json::to_vec(stored).unwrap().len(); + assert!( + serialized_len <= MAX_DIAGNOSTICS_ENTRY_BYTES, + "stored entry must fit the aggregate cap after dropping data, got {serialized_len} bytes" + ); + } + + /// #311 C1 follow-up: an oversized `source` (not `data`) on a single + /// diagnostic must also be brought back under the cap -- the + /// opaque-field-drop mitigation alone does not touch `source`, which is + /// a plain string and must be truncated instead. + #[test] + fn test_store_diagnostics_truncates_oversized_source_on_single_diagnostic() { + let mut cache = NotificationCache::new(); + let uri: Uri = "file:///test.rs".parse().unwrap(); + + let mut diagnostic = minimal_diagnostic("small message".to_string()); + diagnostic.source = Some("x".repeat(MAX_DIAGNOSTICS_ENTRY_BYTES + 1000)); + + cache.store_diagnostics(&test_server(), &uri, Some(1), vec![diagnostic]); + + let stored = &cache.get_diagnostics(uri.as_str()).unwrap().diagnostics; + assert_eq!(stored.len(), 1); + assert_eq!(stored[0].message, "small message"); + let serialized_len = serde_json::to_vec(stored).unwrap().len(); + assert!( + serialized_len <= MAX_DIAGNOSTICS_ENTRY_BYTES, + "stored entry must fit the aggregate cap after truncating source, got {serialized_len} bytes" + ); + } + + /// #311 C1 follow-up: `cap_diagnostics_entry_size`'s postcondition -- + /// the result always fits `MAX_DIAGNOSTICS_ENTRY_BYTES` -- must hold + /// even when every uncapped field is maxed out simultaneously, not just + /// one at a time. This is the terminal-enforcement guarantee itself, + /// exercised end to end through `store_diagnostics` rather than by + /// calling the private function directly. + #[test] + fn test_store_diagnostics_caps_single_diagnostic_with_every_field_maxed_out() { + let mut cache = NotificationCache::new(); + let uri: Uri = "file:///test.rs".parse().unwrap(); + + // Each field individually exceeds MAX_ENTRY_TEXT_BYTES (so + // source/code truncation is exercised) and the combination exceeds + // MAX_DIAGNOSTICS_ENTRY_BYTES, without needing to allocate multiple + // megabytes per field just to prove the same point. + let mut diagnostic = minimal_diagnostic("x".repeat(MAX_ENTRY_TEXT_BYTES + 1000)); + diagnostic.source = Some("x".repeat(MAX_ENTRY_TEXT_BYTES + 1000)); + diagnostic.code = Some(lsp_types::NumberOrString::String( + "x".repeat(MAX_ENTRY_TEXT_BYTES + 1000), + )); + diagnostic.data = Some(serde_json::json!({ "blob": "x".repeat(MAX_ENTRY_TEXT_BYTES) })); + diagnostic.tags = Some(vec![lsp_types::DiagnosticTag::UNNECESSARY; 50]); + diagnostic.related_information = Some(vec![ + lsp_types::DiagnosticRelatedInformation { + location: lsp_types::Location { + uri: uri.clone(), + range: Range::default(), + }, + message: "x".repeat(1000), + }; + 5 + ]); + + cache.store_diagnostics(&test_server(), &uri, Some(1), vec![diagnostic]); + + let stored = &cache.get_diagnostics(uri.as_str()).unwrap().diagnostics; + assert_eq!(stored.len(), 1); + let serialized_len = serde_json::to_vec(stored).unwrap().len(); + assert!( + serialized_len <= MAX_DIAGNOSTICS_ENTRY_BYTES, + "postcondition must hold even with every field maxed out, got {serialized_len} bytes" + ); + } + + /// #311 C1 follow-up: exercises `cap_diagnostics_entry_size`'s terminal + /// fallback directly. `message` is the one field the field-specific + /// mitigations never touch (they only cover + /// `source`/`code`/`data`/`code_description`/`related_information`/ + /// `tags`), so an oversized, *untruncated* message -- as it would be if + /// this private function were ever called without `store_diagnostics`'s + /// own prior message truncation -- must still be brought under budget + /// by the terminal step, not left to slip through. + #[test] + fn test_cap_diagnostics_entry_size_terminal_fallback_bounds_untruncated_message() { + let uri: Uri = "file:///test.rs".parse().unwrap(); + let mut diagnostics = vec![minimal_diagnostic( + "x".repeat(MAX_DIAGNOSTICS_ENTRY_BYTES + 1000), + )]; + + cap_diagnostics_entry_size(&uri, &mut diagnostics); + + assert_eq!(diagnostics.len(), 1); + assert!( + diagnostics[0].message.len() <= DIAGNOSTIC_TERMINAL_FALLBACK_MESSAGE_BYTES + 20, + "terminal fallback must truncate the message itself, got {} bytes", + diagnostics[0].message.len() + ); + let serialized_len = serde_json::to_vec(&diagnostics).unwrap().len(); + assert!( + serialized_len <= MAX_DIAGNOSTICS_ENTRY_BYTES, + "postcondition must hold via the terminal fallback, got {serialized_len} bytes" + ); + } + + /// #311 S5: when no diagnostic carries `data`/`code_description`/ + /// `related_information`/`tags` and the cheap size estimate is already + /// under budget, nothing should be modified -- the fast path must not + /// alter content it didn't need to touch. + #[test] + fn test_store_diagnostics_cheap_path_leaves_small_diagnostics_untouched() { + let mut cache = NotificationCache::new(); + let uri: Uri = "file:///test.rs".parse().unwrap(); + + let mut diagnostic = minimal_diagnostic("a small, ordinary diagnostic message".to_string()); + diagnostic.source = Some("rustc".to_string()); + + cache.store_diagnostics(&test_server(), &uri, Some(1), vec![diagnostic]); + + let stored = &cache.get_diagnostics(uri.as_str()).unwrap().diagnostics; + assert_eq!(stored.len(), 1); + assert_eq!(stored[0].message, "a small, ordinary diagnostic message"); + assert_eq!(stored[0].source.as_deref(), Some("rustc")); + } + + /// #311 S5 follow-up: the critic's exact counterexample. A NUL-heavy + /// message's *raw* byte length looks small enough for the cheap + /// estimate to skip the real check, but its *serialized* (JSON-escaped) + /// size is up to `JSON_ESCAPE_WORST_CASE_FACTOR`x larger -- each NUL + /// byte costs 6 bytes as `\u0000` once JSON-encoded. Three diagnostics + /// at exactly `MAX_ENTRY_TEXT_BYTES` of NULs each previously passed the + /// old raw-length estimate (787,200 bytes, under the 1 MiB cap) while + /// actually serializing to roughly 4.5 MiB -- letting an entry ~4.5x + /// over budget skip `fits`/truncation/terminal-fallback entirely. + #[test] + fn test_store_diagnostics_cheap_path_escape_safe_for_control_character_heavy_message() { + let mut cache = NotificationCache::new(); + let uri: Uri = "file:///test.rs".parse().unwrap(); + + let nul_heavy_message = "\0".repeat(MAX_ENTRY_TEXT_BYTES); + let diagnostics: Vec = (0..3) + .map(|_| minimal_diagnostic(nul_heavy_message.clone())) + .collect(); + + cache.store_diagnostics(&test_server(), &uri, Some(1), diagnostics); + + let stored = &cache.get_diagnostics(uri.as_str()).unwrap().diagnostics; + let serialized_len = serde_json::to_vec(stored).unwrap().len(); + assert!( + serialized_len <= MAX_DIAGNOSTICS_ENTRY_BYTES, + "escape-heavy content must not let the cheap-estimate fast path skip the real cap, \ + got {serialized_len} bytes" + ); + } + #[test] fn test_store_diagnostics_replaces_existing() { let mut cache = NotificationCache::new(); @@ -656,6 +1397,31 @@ mod tests { ); } + /// #311: `MAX_LOG_ENTRIES` bounds the number of log entries, but not the + /// size of any one entry -- an oversized message must be truncated + /// rather than stored verbatim. + #[test] + fn test_store_log_truncates_oversized_message() { + let mut cache = NotificationCache::new(); + let oversized = "a".repeat(MAX_ENTRY_TEXT_BYTES + 100); + + cache.store_log(LogLevel::Info, oversized.clone()); + + let stored = &cache.logs()[0].message; + assert!(stored.len() < oversized.len()); + assert!(stored.ends_with("... (truncated)")); + } + + #[test] + fn test_store_log_does_not_truncate_message_at_or_below_limit() { + let mut cache = NotificationCache::new(); + let message = "a".repeat(MAX_ENTRY_TEXT_BYTES); + + cache.store_log(LogLevel::Info, message.clone()); + + assert_eq!(cache.logs()[0].message, message); + } + #[test] fn test_clear_logs() { let mut cache = NotificationCache::new(); @@ -711,6 +1477,19 @@ mod tests { assert_eq!(cache.messages_count(), 0); } + /// #311: same per-entry byte cap as `store_log`, applied to server messages. + #[test] + fn test_store_message_truncates_oversized_message() { + let mut cache = NotificationCache::new(); + let oversized = "a".repeat(MAX_ENTRY_TEXT_BYTES + 100); + + cache.store_message(MessageType::Info, oversized.clone()); + + let stored = &cache.messages()[0].message; + assert!(stored.len() < oversized.len()); + assert!(stored.ends_with("... (truncated)")); + } + #[test] fn test_log_levels() { let mut cache = NotificationCache::new(); diff --git a/crates/mcpls-core/src/bridge/translator/assist.rs b/crates/mcpls-core/src/bridge/translator/assist.rs index 62d250f..c7e27e1 100644 --- a/crates/mcpls-core/src/bridge/translator/assist.rs +++ b/crates/mcpls-core/src/bridge/translator/assist.rs @@ -12,7 +12,7 @@ use super::dto::{ SignatureInfo, SignatureParameter, }; use crate::config::ToolKind; -use crate::error::Result; +use crate::error::{Error, Result}; /// Extract hover contents as markdown string. /// Convert LSP `Documentation` to a plain string. @@ -23,13 +23,38 @@ fn extract_documentation(doc: lsp_types::Documentation) -> String { } } +/// Maximum length, in bytes, of a `get_completions` `trigger` parameter. +/// +/// The LSP spec defines `triggerCharacter` as a single character, but +/// `CompletionsParams.trigger` is still an unbounded free-form `String` +/// forwarded to the LSP server as `trigger_character` with no cap of its +/// own (#309 M3) -- the same forwarding-without-a-cap shape `new_name` and +/// `query` had. 8 bytes comfortably covers any single Unicode codepoint (at +/// most 4 bytes in UTF-8) with margin, while still rejecting anything that +/// isn't plausibly "one character". +pub(super) const MAX_TRIGGER_CHARACTER_BYTES: usize = 8; + +/// Validate parameters for `handle_completions`. +fn validate_completions_params(trigger: Option<&str>) -> Result<()> { + if let Some(trigger) = trigger + && trigger.len() > MAX_TRIGGER_CHARACTER_BYTES + { + return Err(Error::InvalidToolParams(format!( + "trigger too long: {} bytes (max {MAX_TRIGGER_CHARACTER_BYTES})", + trigger.len() + ))); + } + Ok(()) +} + impl Translator { /// Handle completions request. /// /// # Errors /// - /// Returns an error if the LSP request fails, the file cannot be opened, - /// or the routed server does not advertise `completionProvider` support. + /// Returns an error if `trigger` exceeds the maximum allowed length, + /// the LSP request fails, the file cannot be opened, or the routed + /// server does not advertise `completionProvider` support. pub async fn handle_completions( &self, file_path: String, @@ -37,6 +62,8 @@ impl Translator { character: u32, trigger: Option, ) -> Result { + validate_completions_params(trigger.as_deref())?; + let (server_id, client, uri) = self .prepare_gated_document( &file_path, @@ -261,3 +288,28 @@ impl Translator { Ok(InlayHintsResult { hints }) } } + +#[cfg(test)] +#[allow(clippy::unwrap_used, clippy::expect_used)] +mod tests { + use super::*; + + /// #309 M3: `trigger` has no cap of its own even though the LSP spec + /// defines it as a single character. + #[test] + fn test_validate_completions_params_rejects_oversized_trigger() { + let trigger = "a".repeat(MAX_TRIGGER_CHARACTER_BYTES + 1); + let result = validate_completions_params(Some(&trigger)); + assert!(matches!(result, Err(Error::InvalidToolParams(_)))); + } + + #[test] + fn test_validate_completions_params_accepts_typical_trigger_char() { + assert!(validate_completions_params(Some(".")).is_ok()); + } + + #[test] + fn test_validate_completions_params_accepts_none() { + assert!(validate_completions_params(None).is_ok()); + } +} diff --git a/crates/mcpls-core/src/bridge/translator/edits.rs b/crates/mcpls-core/src/bridge/translator/edits.rs index 8c34ad9..3729e0d 100644 --- a/crates/mcpls-core/src/bridge/translator/edits.rs +++ b/crates/mcpls-core/src/bridge/translator/edits.rs @@ -77,6 +77,26 @@ fn validate_code_action_params( Ok(()) } +/// Maximum length, in bytes, of a `rename_symbol` `new_name` parameter. +/// +/// `new_name` is forwarded to the routed LSP server as-is with no inherent +/// bound of its own -- unlike `workspace_symbol_search`'s `query` (see +/// `validate_workspace_symbol_params`), it previously relied entirely on +/// outer transport limits (#309). No real identifier approaches this length +/// in any language mcpls targets. +pub(super) const MAX_NEW_NAME_LENGTH: usize = 1_000; + +/// Validate parameters for `handle_rename`. +fn validate_rename_params(new_name: &str) -> Result<()> { + if new_name.len() > MAX_NEW_NAME_LENGTH { + return Err(Error::InvalidToolParams(format!( + "new_name too long: {} bytes (max {MAX_NEW_NAME_LENGTH})", + new_name.len() + ))); + } + Ok(()) +} + /// Convert LSP code action to MCP code action. `uri` is the queried /// document's own URI, used for the action's `diagnostics` (always scoped to /// the requested document); `edit.changes` carries its own per-file URIs. @@ -147,8 +167,9 @@ impl Translator { /// /// # Errors /// - /// Returns an error if the LSP request fails, the file cannot be opened, - /// or the routed server does not advertise `renameProvider` support. + /// Returns an error if `new_name` exceeds the maximum allowed length, + /// the LSP request fails, the file cannot be opened, or the routed + /// server does not advertise `renameProvider` support. pub async fn handle_rename( &self, file_path: String, @@ -156,6 +177,8 @@ impl Translator { character: u32, new_name: String, ) -> Result { + validate_rename_params(&new_name)?; + let (server_id, client, uri) = self .prepare_gated_document(&file_path, ToolKind::Rename, "renameProvider", |caps| { matches!( @@ -412,6 +435,34 @@ mod tests { use crate::bridge::translator::dto::DiagnosticSeverity; use crate::bridge::translator::testing::*; + /// #309: `new_name` has no inherent bound of its own and is forwarded to + /// the LSP server as-is, so it must be rejected before that happens. + #[test] + fn test_validate_rename_params_rejects_oversized_new_name() { + let new_name = "a".repeat(MAX_NEW_NAME_LENGTH + 1); + let result = validate_rename_params(&new_name); + assert!(matches!(result, Err(Error::InvalidToolParams(_)))); + } + + #[test] + fn test_validate_rename_params_accepts_name_at_exact_limit() { + let new_name = "a".repeat(MAX_NEW_NAME_LENGTH); + assert!(validate_rename_params(&new_name).is_ok()); + } + + #[test] + fn test_validate_rename_params_accepts_typical_identifier() { + assert!(validate_rename_params("my_variable").is_ok()); + } + + /// #309: length checks have no lower bound -- an empty `new_name` is + /// syntactically valid input for this validator (semantic rejection of + /// an empty rename target, if desired, is a separate concern). + #[test] + fn test_validate_rename_params_accepts_empty_string() { + assert!(validate_rename_params("").is_ok()); + } + #[tokio::test] async fn test_handle_code_actions_invalid_kind() { let translator = Translator::new(); diff --git a/crates/mcpls-core/src/bridge/translator/routing.rs b/crates/mcpls-core/src/bridge/translator/routing.rs index 245a656..c144f3f 100644 --- a/crates/mcpls-core/src/bridge/translator/routing.rs +++ b/crates/mcpls-core/src/bridge/translator/routing.rs @@ -327,6 +327,8 @@ mod tests { use super::*; use crate::bridge::NotificationCache; + use crate::bridge::translator::assist::MAX_TRIGGER_CHARACTER_BYTES; + use crate::bridge::translator::edits::MAX_NEW_NAME_LENGTH; use crate::bridge::translator::testing::*; use crate::config::{LspServerConfig, ToolRouter}; use crate::error::Error; @@ -942,6 +944,20 @@ mod tests { )); } + /// #309: an oversized `new_name` must be rejected before any server + /// routing is attempted, so no LSP server needs to be registered here. + #[tokio::test] + async fn test_handle_rename_rejects_oversized_new_name() { + let translator = Translator::new(); + let new_name = "a".repeat(MAX_NEW_NAME_LENGTH + 1); + + let result = translator + .handle_rename("/main.rs".to_string(), 1, 1, new_name) + .await; + + assert!(matches!(result, Err(Error::InvalidToolParams(_)))); + } + #[tokio::test] async fn test_handle_rename_blocked_when_capability_not_supported() { let dir = TempDir::new().unwrap(); @@ -1262,6 +1278,20 @@ mod tests { )); } + /// #309 M3: an oversized `trigger` must be rejected before any server + /// routing is attempted. + #[tokio::test] + async fn test_handle_completions_rejects_oversized_trigger() { + let translator = Translator::new(); + let trigger = "a".repeat(MAX_TRIGGER_CHARACTER_BYTES + 1); + + let result = translator + .handle_completions("/main.rs".to_string(), 1, 1, Some(trigger)) + .await; + + assert!(matches!(result, Err(Error::InvalidToolParams(_)))); + } + #[tokio::test] async fn test_handle_completions_blocked_when_capability_not_supported() { let dir = TempDir::new().unwrap(); diff --git a/crates/mcpls-core/src/bridge/translator/symbols.rs b/crates/mcpls-core/src/bridge/translator/symbols.rs index 41d62ce..04b4b06 100644 --- a/crates/mcpls-core/src/bridge/translator/symbols.rs +++ b/crates/mcpls-core/src/bridge/translator/symbols.rs @@ -46,7 +46,7 @@ fn validate_workspace_symbol_params(query: &str, kind_filter: Option<&str>) -> R if query.len() > MAX_QUERY_LENGTH { return Err(Error::InvalidToolParams(format!( - "Query too long: {} chars (max {MAX_QUERY_LENGTH})", + "Query too long: {} bytes (max {MAX_QUERY_LENGTH})", query.len() ))); } diff --git a/crates/mcpls-core/src/config/mod.rs b/crates/mcpls-core/src/config/mod.rs index 9d607c6..b20f7bc 100644 --- a/crates/mcpls-core/src/config/mod.rs +++ b/crates/mcpls-core/src/config/mod.rs @@ -8,6 +8,7 @@ mod routing; mod server; use std::collections::{HashMap, HashSet}; +use std::io::Read; use std::path::{Path, PathBuf}; pub use language::{base_language_id, react_variant_language_id}; @@ -410,6 +411,30 @@ pub enum ProjectConfigTrust { Trusted, } +/// Maximum size, in bytes, of a config file `load_from` will read. +/// +/// A config file is trusted TOML on a normal setup, but nothing stops a +/// path from pointing at an arbitrarily large or adversarial file (e.g. a +/// misconfigured `$MCPLS_CONFIG`) -- `load_from` used to call +/// `std::fs::read_to_string` with no upper bound, so it could be made to +/// buffer an unbounded amount of memory before `toml::from_str` ever runs +/// (#309). 8 MiB is far larger than any legitimate `mcpls.toml`, which +/// realistically stays in the low kilobytes even with dozens of configured +/// servers. +/// +/// Enforced via a bounded read (`Read::take`), not a `std::fs::metadata` +/// pre-check: `metadata().len()` reports `0` for character devices, FIFOs, +/// and many procfs entries regardless of how much data they can actually +/// produce (e.g. `/dev/zero`), so a path pointing at one of those would +/// sail past a size-only pre-check and still block `read_to_string` on an +/// effectively infinite read -- the exact "slow/infinite device" case #309 +/// named. A pure metadata check is also TOCTOU-able for a regular file that +/// grows between the check and the read. Reading `MAX_CONFIG_FILE_BYTES + +/// 1` bytes, one past the cap, is what distinguishes "exactly at the +/// boundary" (allowed) from "over" (rejected) without needing a second +/// syscall. +const MAX_CONFIG_FILE_BYTES: u64 = 8 * 1024 * 1024; + impl ServerConfig { /// Build the effective extension map used for language detection. /// @@ -546,9 +571,10 @@ impl ServerConfig { /// /// # Errors /// - /// Returns an error if the file doesn't exist or parsing fails. + /// Returns an error if the file doesn't exist, exceeds the maximum + /// allowed config file size, or parsing fails. pub fn load_from(path: &Path) -> Result { - let content = std::fs::read_to_string(path).map_err(|e| { + let file = std::fs::File::open(path).map_err(|e| { if e.kind() == std::io::ErrorKind::NotFound { Error::ConfigNotFound(path.to_path_buf()) } else { @@ -556,6 +582,22 @@ impl ServerConfig { } })?; + // Bounded read, not a `metadata().len()` pre-check -- see + // `MAX_CONFIG_FILE_BYTES`'s doc for why the pre-check alone is + // bypassable. + let mut buf = Vec::new(); + file.take(MAX_CONFIG_FILE_BYTES + 1) + .read_to_end(&mut buf) + .map_err(Error::Io)?; + if buf.len() as u64 > MAX_CONFIG_FILE_BYTES { + return Err(Error::FileSizeLimitExceeded { + size: buf.len() as u64, + max: MAX_CONFIG_FILE_BYTES, + }); + } + let content = String::from_utf8(buf) + .map_err(|e| Error::InvalidConfig(format!("config file is not valid UTF-8: {e}")))?; + let config: Self = toml::from_str(&content)?; config.validate()?; Ok(config) @@ -970,6 +1012,67 @@ mod tests { assert!(result.is_err()); } + /// #309: a config file larger than `MAX_CONFIG_FILE_BYTES` must be + /// rejected before `read_to_string` buffers it, not merely fail to + /// parse as TOML afterward. + #[test] + fn test_load_from_rejects_oversized_file() { + let tmp_dir = TempDir::new().unwrap(); + let config_path = tmp_dir.path().join("oversized.toml"); + + // One byte over the cap; content doesn't need to be valid TOML since + // the size check runs before parsing. + let oversized = "#".repeat(usize::try_from(MAX_CONFIG_FILE_BYTES).unwrap() + 1); + fs::write(&config_path, &oversized).unwrap(); + + let result = ServerConfig::load_from(&config_path); + assert!(matches!( + result, + Err(Error::FileSizeLimitExceeded { max, .. }) if max == MAX_CONFIG_FILE_BYTES + )); + } + + #[test] + fn test_load_from_accepts_file_at_exact_size_cap() { + let tmp_dir = TempDir::new().unwrap(); + let config_path = tmp_dir.path().join("exact.toml"); + + // Pad a valid, minimal TOML document with a trailing comment up to + // exactly the cap -- the boundary itself must not be rejected. + let mut toml_content = "[workspace]\n# ".to_string(); + toml_content.push_str( + &"a".repeat(usize::try_from(MAX_CONFIG_FILE_BYTES).unwrap() - toml_content.len()), + ); + assert_eq!(toml_content.len() as u64, MAX_CONFIG_FILE_BYTES); + fs::write(&config_path, &toml_content).unwrap(); + + let result = ServerConfig::load_from(&config_path); + assert!(result.is_ok(), "expected Ok, got {result:?}"); + } + + /// #309 S1: `std::fs::metadata` reports `len() == 0` for character + /// devices regardless of how much data they can actually produce -- + /// `/dev/zero` is the canonical example. A size check based on metadata + /// alone would pass and let `load_from` block on an effectively + /// infinite read; the bounded `Read::take` must still reject it via + /// `MAX_CONFIG_FILE_BYTES`, not hang or OOM. + #[cfg(unix)] + #[test] + fn test_load_from_rejects_infinite_special_file() { + let path = Path::new("/dev/zero"); + assert_eq!( + fs::metadata(path).unwrap().len(), + 0, + "test assumption: /dev/zero must report zero length" + ); + + let result = ServerConfig::load_from(path); + assert!(matches!( + result, + Err(Error::FileSizeLimitExceeded { max, .. }) if max == MAX_CONFIG_FILE_BYTES + )); + } + #[test] fn test_validate_empty_language_id() { let tmp_dir = TempDir::new().unwrap(); diff --git a/crates/mcpls-core/src/lib.rs b/crates/mcpls-core/src/lib.rs index 073015c..e84d19d 100644 --- a/crates/mcpls-core/src/lib.rs +++ b/crates/mcpls-core/src/lib.rs @@ -41,6 +41,7 @@ pub mod error; pub mod lsp; pub mod mcp; pub mod transport; +mod util; use std::collections::{HashMap, HashSet}; use std::path::{Component, PathBuf}; diff --git a/crates/mcpls-core/src/lsp/client.rs b/crates/mcpls-core/src/lsp/client.rs index a8c5b16..9f5a782 100644 --- a/crates/mcpls-core/src/lsp/client.rs +++ b/crates/mcpls-core/src/lsp/client.rs @@ -32,8 +32,24 @@ const SERVER_CANCELLED_MAX_RETRIES: u32 = 3; const SERVER_CANCELLED_INITIAL_DELAY_MS: u64 = 500; /// Byte-length threshold for truncating an LSP error message before logging it. +/// +/// Kept short since this feeds a single `tracing::error!` log line, not the +/// MCP caller -- see `MAX_ERROR_MESSAGE_CALLER_BYTES` for that budget. const MAX_ERROR_MESSAGE_LOG_BYTES: usize = 200; +/// Byte-length threshold for the LSP error message forwarded to the MCP +/// caller in [`Error::LspServerError`] (#313). +/// +/// Deliberately much larger than `MAX_ERROR_MESSAGE_LOG_BYTES`: a +/// legitimate LSP error (e.g. a verbose rust-analyzer type-mismatch +/// diagnostic reported through an error response) can run into the low +/// kilobytes, and that detail is useful to the calling model -- a log line +/// should stay terse, but a truncated-to-200-bytes error handed to the +/// model would cut off real content on every longer-but-honest error. Still +/// far below #311's 256 KiB cache-entry cap: this string is echoed directly +/// into the MCP tool result / model context, not merely cached. +const MAX_ERROR_MESSAGE_CALLER_BYTES: usize = 4 * 1024; + /// Upper bound on the effective timeout for completion requests, regardless /// of `request_timeout_seconds`. /// @@ -491,23 +507,15 @@ impl LspClient { result } - /// Truncate an LSP server's error message for logging, bounding the payload to at most - /// [`MAX_ERROR_MESSAGE_LOG_BYTES`] bytes (the full formatted string is slightly longer). + /// Truncate an LSP server's error message for the `tracing::error!` log + /// line, bounding it to at most [`MAX_ERROR_MESSAGE_LOG_BYTES`] bytes + /// (the full formatted string is slightly longer). /// - /// `message` is attacker-influenceable (echoed back by the spawned LSP server), so the - /// cut point is the last UTF-8 char boundary at or before that limit rather than a raw - /// byte index, which would panic if it fell inside a multi-byte codepoint. + /// Log-line use only -- the message forwarded to the MCP caller in + /// [`Error::LspServerError`] is truncated separately, to the larger + /// [`MAX_ERROR_MESSAGE_CALLER_BYTES`] (#313). fn truncate_error_message_for_log(message: &str) -> String { - if message.len() <= MAX_ERROR_MESSAGE_LOG_BYTES { - return message.to_string(); - } - let cut = message - .char_indices() - .map(|(i, _)| i) - .take_while(|&i| i <= MAX_ERROR_MESSAGE_LOG_BYTES) - .last() - .unwrap_or(0); - format!("{}... (truncated)", &message[..cut]) + crate::util::truncate_str(message, MAX_ERROR_MESSAGE_LOG_BYTES) } async fn message_loop_inner( @@ -560,11 +568,20 @@ impl LspClient { if let Some(sender) = sender { if let Some(error) = response.error { - let message = Self::truncate_error_message_for_log(&error.message); - error!("LSP error response: {} (code {})", message, error.code); + let log_message = Self::truncate_error_message_for_log(&error.message); + error!("LSP error response: {} (code {})", log_message, error.code); + // Truncated separately from the log line, to the larger + // MAX_ERROR_MESSAGE_CALLER_BYTES -- the raw message is + // unbounded and attacker-influenceable (#313), but a + // log-line-sized cut would also clip legitimate long + // errors before the model ever sees them (S2). + let caller_message = crate::util::truncate_str( + &error.message, + MAX_ERROR_MESSAGE_CALLER_BYTES, + ); let _ = sender.send(Err(Error::LspServerError { code: error.code, - message: error.message, + message: caller_message, data: error.data, })); } else if let Some(result) = response.result { @@ -929,56 +946,6 @@ mod tests { assert!(sender.is_none(), "Should not find sender for unknown ID"); } - #[tokio::test] - async fn test_long_error_message_truncation() { - use crate::lsp::types::{JsonRpcError, JsonRpcResponse, RequestId}; - - let pending_requests: Arc> = Arc::new(Mutex::new(HashMap::new())); - let (response_tx, response_rx) = oneshot::channel::>(); - - pending_requests - .lock() - .await - .insert(RequestId::Number(1), response_tx); - - let long_message = "x".repeat(250); - let error_response = JsonRpcResponse { - jsonrpc: "2.0".to_string(), - id: RequestId::Number(1), - result: None, - error: Some(JsonRpcError { - code: -32700, - message: long_message.clone(), - data: None, - }), - }; - - let sender = pending_requests.lock().await.remove(&error_response.id); - if let Some(sender) = sender - && let Some(error) = error_response.error - { - let _ = sender.send(Err(Error::LspServerError { - code: error.code, - message: error.message, - data: error.data, - })); - } - - let result = response_rx.await.unwrap(); - assert!(result.is_err()); - - if let Err(Error::LspServerError { code, message, .. }) = result { - assert_eq!(code, -32700); - assert_eq!( - message.len(), - 250, - "Full message should be preserved in Error" - ); - } else { - panic!("Expected LspServerError"); - } - } - #[test] fn test_truncate_error_message_for_log_handles_multibyte_boundary() { // 199 ASCII bytes followed by a 3-byte UTF-8 char ('€') straddles the byte-200 cut. @@ -1230,6 +1197,25 @@ mod tests { stdin.flush().await.unwrap(); } + /// Writes a framed JSON-RPC error response with an arbitrary code/message. + async fn write_error_response( + stdin: &mut ChildStdin, + id: &Value, + code: i32, + message: &str, + ) { + let response = serde_json::json!({ + "jsonrpc": "2.0", + "id": id, + "error": { "code": code, "message": message }, + }); + let content = serde_json::to_string(&response).unwrap(); + let header = format!("Content-Length: {}\r\n\r\n", content.len()); + stdin.write_all(header.as_bytes()).await.unwrap(); + stdin.write_all(content.as_bytes()).await.unwrap(); + stdin.flush().await.unwrap(); + } + /// Writes a framed JSON-RPC success response. async fn write_success_response(stdin: &mut ChildStdin, id: &Value, result: Value) { let response = serde_json::json!({ @@ -1379,5 +1365,88 @@ mod tests { let result = request_task.await.unwrap(); assert_eq!(result.unwrap(), expected_result); } + + /// #313: an oversized, server-controlled error message must be + /// truncated before it reaches the MCP caller in + /// `Error::LspServerError`, not just before it is logged. Routes + /// through the real `message_loop_inner` (via `fake_lsp_client`) + /// rather than constructing the error by hand, so it actually + /// exercises the fix. + #[tokio::test] + async fn test_oversized_error_message_truncated_for_caller() { + let (client, mut server) = fake_lsp_client(); + + let request_task = tokio::spawn(async move { + client + .request::<_, Value>( + "textDocument/hover", + serde_json::json!({}), + Duration::from_secs(30), + ) + .await + }); + + let mut reader = BufReader::new(&mut server.write_stdout); + let request = read_framed_message(&mut reader).await; + let id = request["id"].clone(); + let oversized_message = "x".repeat(MAX_ERROR_MESSAGE_CALLER_BYTES + 500); + write_error_response(&mut server.read_half_stdin, &id, -32603, &oversized_message) + .await; + + let result = request_task.await.unwrap(); + + match result { + Err(Error::LspServerError { code, message, .. }) => { + assert_eq!(code, -32603); + assert!( + message.len() < oversized_message.len(), + "caller-facing message must be truncated, got {} bytes", + message.len() + ); + assert!(message.ends_with("... (truncated)")); + } + other => panic!("expected truncated LspServerError, got {other:?}"), + } + } + + /// #313 S2: a legitimate error message longer than the log-line cap + /// (`MAX_ERROR_MESSAGE_LOG_BYTES`, 200 bytes) but shorter than the + /// caller-facing cap must reach the MCP caller intact -- the + /// caller-facing budget must not silently collapse to the log + /// budget. + #[tokio::test] + async fn test_error_message_between_log_and_caller_caps_reaches_caller_intact() { + let (client, mut server) = fake_lsp_client(); + + let request_task = tokio::spawn(async move { + client + .request::<_, Value>( + "textDocument/hover", + serde_json::json!({}), + Duration::from_secs(30), + ) + .await + }); + + let mut reader = BufReader::new(&mut server.write_stdout); + let request = read_framed_message(&mut reader).await; + let id = request["id"].clone(); + let message = "x".repeat(MAX_ERROR_MESSAGE_LOG_BYTES + 50); + write_error_response(&mut server.read_half_stdin, &id, -32603, &message).await; + + let result = request_task.await.unwrap(); + + match result { + Err(Error::LspServerError { + message: returned, .. + }) => { + assert_eq!( + returned, message, + "message under the caller cap must not be truncated" + ); + } + other => panic!("expected untruncated LspServerError, got {other:?}"), + } + } } } diff --git a/crates/mcpls-core/src/util.rs b/crates/mcpls-core/src/util.rs new file mode 100644 index 0000000..738e120 --- /dev/null +++ b/crates/mcpls-core/src/util.rs @@ -0,0 +1,117 @@ +//! Small helpers shared across `mcpls-core` modules. + +/// Marker appended to a truncated string; the returned string can be up to +/// `max_bytes + TRUNCATION_MARKER.len()` bytes, not exactly `max_bytes`. +const TRUNCATION_MARKER: &str = "... (truncated)"; + +/// Truncate `s` to at most `max_bytes` bytes, cutting on the last UTF-8 char +/// boundary at or before the limit and appending a truncation marker. The +/// returned string can be up to `max_bytes + TRUNCATION_MARKER.len()` bytes +/// when truncation occurs -- the marker is appended after the cut, not +/// counted against the limit. +/// +/// `s` is typically attacker-influenceable (forwarded from a spawned LSP +/// server), so the cut point is found via `char_indices` rather than a raw +/// byte index, which would panic if it fell inside a multi-byte codepoint. +/// +/// Always allocates a fresh `String`, even when `s` is already within the +/// limit. Prefer [`truncate_string`] when the caller already owns `s` and +/// truncation is expected to be rare, to skip that allocation on the common +/// path. +pub fn truncate_str(s: &str, max_bytes: usize) -> String { + if s.len() <= max_bytes { + return s.to_string(); + } + let cut = s + .char_indices() + .map(|(i, _)| i) + .take_while(|&i| i <= max_bytes) + .last() + .unwrap_or(0); + format!("{}{TRUNCATION_MARKER}", &s[..cut]) +} + +/// Truncate an owned `String` to at most `max_bytes` bytes in place (same +/// cut/marker semantics as [`truncate_str`]), returning `s` unchanged and +/// without allocating when it is already within the limit. +/// +/// This is the common case on the hot paths that call it -- +/// `NotificationCache::store_log`/`store_message` on every +/// `window/logMessage`/`showMessage`, and each diagnostic's `message` field +/// on every `publishDiagnostics` -- where [`truncate_str`]'s unconditional +/// `s.to_string()` would otherwise clone the message on every call just to +/// hand back an equivalent copy. +pub fn truncate_string(mut s: String, max_bytes: usize) -> String { + if s.len() <= max_bytes { + return s; + } + let cut = s + .char_indices() + .map(|(i, _)| i) + .take_while(|&i| i <= max_bytes) + .last() + .unwrap_or(0); + s.truncate(cut); + s.push_str(TRUNCATION_MARKER); + s +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn no_truncation_at_or_below_limit() { + let exact = "a".repeat(10); + assert_eq!(truncate_str(&exact, 10), exact); + assert_eq!(truncate_str("", 10), ""); + } + + #[test] + fn truncates_just_above_limit() { + let message = "a".repeat(11); + assert_eq!( + truncate_str(&message, 10), + format!("{}... (truncated)", "a".repeat(10)) + ); + } + + #[test] + fn handles_multibyte_char_boundary() { + // Each 'é' is 2 bytes; a raw byte-index cut at 5 would fall inside one. + let message = "é".repeat(10); + let truncated = truncate_str(&message, 5); + assert!(truncated.starts_with(&"é".repeat(2))); + assert!(truncated.ends_with("... (truncated)")); + } + + #[test] + fn truncate_string_no_truncation_at_or_below_limit() { + let exact = "a".repeat(10); + assert_eq!(truncate_string(exact.clone(), 10), exact); + assert_eq!(truncate_string(String::new(), 10), ""); + } + + #[test] + fn truncate_string_truncates_just_above_limit() { + let message = "a".repeat(11); + assert_eq!( + truncate_string(message, 10), + format!("{}... (truncated)", "a".repeat(10)) + ); + } + + #[test] + fn truncate_string_handles_multibyte_char_boundary() { + let message = "é".repeat(10); + let truncated = truncate_string(message, 5); + assert!(truncated.starts_with(&"é".repeat(2))); + assert!(truncated.ends_with("... (truncated)")); + } + + #[test] + fn truncate_str_and_truncate_string_agree() { + let message = "x".repeat(500); + assert_eq!(truncate_str(&message, 100), truncate_string(message, 100)); + } +}